Clear persisted CRD connection information on clean disconnect

Bug: b:283091796 Change-Id: I3842f362ea2d590cecec9e980d75e4ded7c95ee3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4701276 Reviewed-by: Joe Downing <joedow@chromium.org> Commit-Queue: Jeroen Dhollander <jeroendh@google.com> Cr-Commit-Position: refs/heads/main@{#1187119}
chromium · Aug 23, 2023 · f9b24e9 · f9b24e9
1 parent 0e1c462
commit f9b24e9
Show file tree

Hide file tree

Showing 6 changed files with 94 additions and 20 deletions.
diff --git a/chrome/browser/ash/policy/remote_commands/crd_admin_session_controller.cc b/chrome/browser/ash/policy/remote_commands/crd_admin_session_controller.cc
@@ -250,7 +250,7 @@ class CrdAdminSessionController::CrdHostSession {
   }
 
   void TryToReconnect(base::OnceClosure done_callback) {
-    CRD_DVLOG(3) << "Trying to reconnect to previous CRD session (if any)";
+    CRD_DVLOG(3) << "Checking for reconnectable session";
     remoting_service_->GetReconnectableSessionId(
         base::BindOnce(&CrdHostSession::ReconnectToSession,
                        weak_factory_.GetWeakPtr())
@@ -267,7 +267,7 @@ class CrdAdminSessionController::CrdHostSession {
           base::BindOnce(&CrdHostSession::OnStartSupportSessionResponse,
                          weak_factory_.GetWeakPtr()));
     } else {
-      CRD_DVLOG(1) << "No reconnectable CRD session found.";
+      CRD_DVLOG(3) << "No reconnectable CRD session found.";
     }
   }
 

diff --git a/remoting/host/chromeos/remote_support_host_ash.cc b/remoting/host/chromeos/remote_support_host_ash.cc
@@ -9,6 +9,7 @@
 
 #include "base/feature_list.h"
 #include "base/functional/bind.h"
+#include "base/functional/callback_helpers.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/sequence_checker.h"
@@ -164,8 +165,10 @@ void RemoteSupportHostAsh::StartSession(
   it2me_native_message_host_ash_->Connect(
       params, enterprise_params, reconnect_params,
       base::BindOnce(std::move(callback), std::move(response)),
-      base::BindOnce(&RemoteSupportHostAsh::OnClientConnected,
+      base::BindOnce(&RemoteSupportHostAsh::OnHostStateConnected,
                      base::Unretained(this), params, enterprise_params),
+      base::BindOnce(&RemoteSupportHostAsh::OnHostStateDisconnected,
+                     base::Unretained(this)),
       base::BindOnce(&RemoteSupportHostAsh::OnSessionDisconnected,
                      base::Unretained(this)));
 }
@@ -195,12 +198,18 @@ void RemoteSupportHostAsh::ReconnectToSession(SessionId session_id,
           return;
         }
 
+        DCHECK_CALLED_ON_VALID_SEQUENCE(self->sequence_checker_);
+
         if (!session.has_value()) {
           LOG(ERROR) << "CRD: No reconnectable session found";
           std::move(callback).Run(GetUnableToReconnectError());
           return;
         }
 
+        // Remove the stored session information now that we've read it, so we
+        // do not keep it around forever.
+        self->session_storage_->DeleteSession(base::DoNothing());
+
         LOG(INFO) << "CRD: Reconnectable session found - starting connection";
         self->StartSession(
             SessionParamsFromDict(*session->EnsureDict(kSessionParamsDictKey)),
@@ -220,7 +229,7 @@ mojom::SupportHostDetailsPtr RemoteSupportHostAsh::GetHostDetails() {
                                                     kFeatureAuthorizedHelper}));
 }
 
-void RemoteSupportHostAsh::OnClientConnected(
+void RemoteSupportHostAsh::OnHostStateConnected(
     mojom::SupportSessionParams params,
     absl::optional<ChromeOsEnterpriseParams> enterprise_params,
     ConnectionDetails details) {
@@ -245,8 +254,23 @@ void RemoteSupportHostAsh::OnClientConnected(
   VLOG(3) << "CRD: Not a reconnectable session";
 }
 
+void RemoteSupportHostAsh::OnHostStateDisconnected() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  // Don't allow reconnecting to the session if the client disconnects.
+  LOG(INFO) << "CRD: Clearing reconnectable session information";
+  session_storage_->DeleteSession(base::DoNothing());
+  return;
+}
+
 void RemoteSupportHostAsh::OnSessionDisconnected() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  // Don't allow reconnecting to the session if we explicitly disconnect the
+  // session.
+  LOG(INFO) << "CRD: Clearing reconnectable session information";
+  session_storage_->DeleteSession(base::DoNothing());
+
   if (it2me_native_message_host_ash_) {
     // Do not access any instance members after |cleanup_callback_| is run as
     // this instance will be destroyed by running this.

diff --git a/remoting/host/chromeos/remote_support_host_ash.h b/remoting/host/chromeos/remote_support_host_ash.h
@@ -70,9 +70,10 @@ class RemoteSupportHostAsh {
       const absl::optional<ConnectionDetails>& reconnect_params,
       StartSessionCallback callback);
 
-  void OnClientConnected(mojom::SupportSessionParams,
-                         absl::optional<ChromeOsEnterpriseParams>,
-                         ConnectionDetails details);
+  void OnHostStateConnected(mojom::SupportSessionParams,
+                            absl::optional<ChromeOsEnterpriseParams>,
+                            ConnectionDetails details);
+  void OnHostStateDisconnected();
   void OnSessionDisconnected();
 
   SEQUENCE_CHECKER(sequence_checker_);

diff --git a/remoting/host/chromeos/remote_support_host_ash_unittest.cc b/remoting/host/chromeos/remote_support_host_ash_unittest.cc
@@ -73,15 +73,20 @@ class FakeIt2MeHost : public It2MeHost {
     enterprise_params_ = value;
   }
 
-  bool WaitForConnectCall() { return connect_waiter_.Wait(); }
+  bool WaitForConnectCall() {
+    bool success = connect_waiter_.Wait();
+    connect_waiter_.Clear();
+    return success;
+  }
 
   std::string user_name() const { return user_name_; }
   ChromeOsEnterpriseParams enterprise_params() const {
     return enterprise_params_;
   }
 
   It2MeHost::Observer& observer() {
-    CHECK(observer_) << "`Connect()` has not been invoked";
+    CHECK(observer_) << "`Connect()` has not been invoked (or `Disconnect()` "
+                        "was already called)";
     CHECK(observer_.MaybeValid());
     return *observer_;
   }
@@ -237,11 +242,20 @@ class RemoteSupportHostAshTest : public testing::TestWithParam<bool> {
     return connect_result.Take();
   }
 
-  void SignalClientIsConnected() {
+  // This signal is normally sent from the chromoting code when the remote
+  // user has connected.
+  void SignalHostStateConnected() {
     it2me_host().observer().OnStateChanged(It2MeHostState::kConnected,
                                            protocol::ErrorCode::OK);
   }
 
+  // This signal is normally sent from the chromoting code when the remote
+  // user has disconnected.
+  void SignalHostStateDisconnected() {
+    it2me_host().observer().OnStateChanged(It2MeHostState::kDisconnected,
+                                           protocol::ErrorCode::OK);
+  }
+
   InMemorySessionStorage& session_storage() { return session_storage_; }
 
   bool StoreReconnectableSessionInformation(
@@ -414,7 +428,7 @@ TEST_F(RemoteSupportHostAshTest,
   EnableFeature(kEnableCrdAdminRemoteAccessV2);
 
   StartSession(ChromeOsEnterpriseParams{.allow_reconnections = true});
-  SignalClientIsConnected();
+  SignalHostStateConnected();
 
   ASSERT_TRUE(HasSession(session_storage()));
 }
@@ -423,7 +437,7 @@ TEST_F(RemoteSupportHostAshTest, ShouldNotStoreSessionInfoIfFeatureIsDisabled) {
   DisableFeature(kEnableCrdAdminRemoteAccessV2);
 
   StartSession(ChromeOsEnterpriseParams{.allow_reconnections = true});
-  SignalClientIsConnected();
+  SignalHostStateConnected();
 
   ASSERT_FALSE(HasSession(session_storage()));
 }
@@ -433,7 +447,7 @@ TEST_F(RemoteSupportHostAshTest,
   EnableFeature(kEnableCrdAdminRemoteAccessV2);
 
   StartSession(ChromeOsEnterpriseParams{.allow_reconnections = false});
-  SignalClientIsConnected();
+  SignalHostStateConnected();
 
   ASSERT_FALSE(HasSession(session_storage()));
 }
@@ -443,7 +457,7 @@ TEST_F(RemoteSupportHostAshTest,
   EnableFeature(kEnableCrdAdminRemoteAccessV2);
 
   StartSession(absl::nullopt);
-  SignalClientIsConnected();
+  SignalHostStateConnected();
 
   ASSERT_FALSE(HasSession(session_storage()));
 }
@@ -635,6 +649,32 @@ TEST_F(RemoteSupportHostAshTest,
   EXPECT_EQ(it2me_host().authorized_helper(), "the-remote-user@domain.com");
 }
 
+TEST_F(RemoteSupportHostAshTest,
+       ShouldClearReconnectableInformationWhenClientDisconnectsCleanly) {
+  EnableFeature(kEnableCrdAdminRemoteAccessV2);
+
+  StartSession(ChromeOsEnterpriseParams{.allow_reconnections = true});
+  SignalHostStateConnected();
+  ASSERT_TRUE(HasSession(session_storage()));
+
+  SignalHostStateDisconnected();
+  EXPECT_FALSE(HasSession(session_storage()));
+}
+
+TEST_F(RemoteSupportHostAshTest,
+       ShouldClearReconnectableInformationWhenAnotherSessionIsStarted) {
+  EnableFeature(kEnableCrdAdminRemoteAccessV2);
+
+  StartSession(ChromeOsEnterpriseParams{.allow_reconnections = true});
+  SignalHostStateConnected();
+  it2me_host().WaitForConnectCall();
+  ASSERT_TRUE(HasSession(session_storage()));
+
+  StartSession(/*enterprise_params=*/absl::nullopt);
+
+  EXPECT_FALSE(HasSession(session_storage()));
+}
+
 INSTANTIATE_TEST_SUITE_P(RemoteSupportHostAshTest,
                          RemoteSupportHostAshTest,
                          testing::Bool());

diff --git a/remoting/host/it2me/it2me_native_messaging_host_ash.cc b/remoting/host/it2me/it2me_native_messaging_host_ash.cc
@@ -156,16 +156,19 @@ void It2MeNativeMessageHostAsh::Connect(
     const absl::optional<ChromeOsEnterpriseParams>& enterprise_params,
     const absl::optional<ConnectionDetails>& reconnect_params,
     base::OnceClosure connected_callback,
-    ClientConnectedCallback client_connected_callback,
+    HostStateConnectedCallback host_state_connected_callback,
+    base::OnceClosure host_state_disconnected_callback,
     base::OnceClosure disconnected_callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(native_message_host_);
   DCHECK(!connected_callback_);
   DCHECK(!disconnected_callback_);
 
   connected_callback_ = std::move(connected_callback);
-  client_connected_callback_ = std::move(client_connected_callback);
   disconnected_callback_ = std::move(disconnected_callback);
+  host_state_connected_callback_ = std::move(host_state_connected_callback);
+  host_state_disconnected_callback_ =
+      std::move(host_state_disconnected_callback);
 
   auto message =
       base::Value::Dict()
@@ -297,6 +300,7 @@ void It2MeNativeMessageHostAsh::HandleHostStateChangeMessage(
     remote_->OnHostStateDisconnected(
         disconnect_reason ? *disconnect_reason
                           : ErrorCodeToString(protocol::ErrorCode::OK));
+    std::move(host_state_disconnected_callback_).Run();
   } else if (*new_state == kHostStateRequestedAccessCode) {
     remote_->OnHostStateRequestedAccessCode();
   } else if (*new_state == kHostStateReceivedAccessCode) {
@@ -332,7 +336,7 @@ void It2MeNativeMessageHostAsh::HandleHostStateChangeMessage(
     // TODO(b/283091055): Update the client connected response to contain
     // reconnection information (if the session is reconnectable), and add
     // this information to `ConnectionDetail`.
-    std::move(client_connected_callback_)
+    std::move(host_state_connected_callback_)
         .Run(ConnectionDetails(*remote_username));
 
   } else if (*new_state == kHostStateError) {

diff --git a/remoting/host/it2me/it2me_native_messaging_host_ash.h b/remoting/host/it2me/it2me_native_messaging_host_ash.h
@@ -44,7 +44,8 @@ class It2MeNativeMessageHostAsh : public extensions::NativeMessageHost::Client {
       delete;
   ~It2MeNativeMessageHostAsh() override;
 
-  using ClientConnectedCallback = base::OnceCallback<void(ConnectionDetails)>;
+  using HostStateConnectedCallback =
+      base::OnceCallback<void(ConnectionDetails)>;
 
   // Creates a new NMH instance, creates a new SupportHostObserver remote and
   // returns the pending_remote.  Start() must be called before the first call
@@ -66,7 +67,8 @@ class It2MeNativeMessageHostAsh : public extensions::NativeMessageHost::Client {
       const absl::optional<ChromeOsEnterpriseParams>& enterprise_params,
       const absl::optional<ConnectionDetails>& reconnect_params,
       base::OnceClosure connected_callback,
-      ClientConnectedCallback client_connected_callback,
+      HostStateConnectedCallback host_state_connected_callback,
+      base::OnceClosure host_state_disconnected_callback,
       base::OnceClosure disconnected_callback);
   // Disconnects an active session if one exists.
   void Disconnect();
@@ -84,7 +86,10 @@ class It2MeNativeMessageHostAsh : public extensions::NativeMessageHost::Client {
 
   base::OnceClosure connected_callback_ GUARDED_BY_CONTEXT(sequence_checker_);
 
-  ClientConnectedCallback client_connected_callback_
+  HostStateConnectedCallback host_state_connected_callback_
+      GUARDED_BY_CONTEXT(sequence_checker_);
+
+  base::OnceClosure host_state_disconnected_callback_
       GUARDED_BY_CONTEXT(sequence_checker_);
 
   base::OnceClosure disconnected_callback_