[CrOS Multidevice] Integrate SecureChannel API with ConnectionPreserver.

Uses SecureChannelClient to keep a connection open with the appropriate
remote device. This change also requires slightly tweaking HostScannerOperation
to delay unregistering the remote device in question until ConnectionPreserver
has fully had a chance to communicate with the Mojo SecureChannel service.

Bug: 824568, 752273, 855813
Change-Id: I6c8c915ab1efbae9c77e08c9c9b2406a5b4de2ad
Reviewed-on: https://chromium-review.googlesource.com/1117558
Commit-Queue: Ryan Hansberry <hansberry@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570979}

chromeos/components/tether/connection_preserver_impl.cc

@@ -5,6 +5,7 @@
 #include "chromeos/components/tether/connection_preserver_impl.h"
 
 #include "base/timer/timer.h"
+#include "chromeos/chromeos_features.h"
 #include "chromeos/components/proximity_auth/logging/logging.h"
 #include "chromeos/components/tether/ble_connection_manager.h"
 #include "chromeos/components/tether/tether_host_response_recorder.h"
@@ -17,12 +18,22 @@ namespace chromeos {
 
 namespace tether {
 
+namespace {
+
+const char kTetherFeature[] = "magic_tether";
+
+}  // namespace
+
 ConnectionPreserverImpl::ConnectionPreserverImpl(
+    device_sync::DeviceSyncClient* device_sync_client,
+    secure_channel::SecureChannelClient* secure_channel_client,
     BleConnectionManager* ble_connection_manager,
     NetworkStateHandler* network_state_handler,
     ActiveHost* active_host,
     TetherHostResponseRecorder* tether_host_response_recorder)
-    : ble_connection_manager_(ble_connection_manager),
+    : device_sync_client_(device_sync_client),
+      secure_channel_client_(secure_channel_client),
+      ble_connection_manager_(ble_connection_manager),
       network_state_handler_(network_state_handler),
       active_host_(active_host),
       tether_host_response_recorder_(tether_host_response_recorder),
@@ -71,6 +82,40 @@ void ConnectionPreserverImpl::HandleSuccessfulTetherAvailabilityResponse(
   }
 }
 
+void ConnectionPreserverImpl::OnConnectionAttemptFailure(
+    secure_channel::mojom::ConnectionAttemptFailureReason reason) {
+  DCHECK(base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi));
+  PA_LOG(WARNING) << "Failed to connect to device "
+                  << cryptauth::RemoteDeviceRef::TruncateDeviceIdForLogs(
+                         preserved_connection_device_id_)
+                  << ", error: " << reason;
+  RemovePreservedConnectionIfPresent();
+}
+
+void ConnectionPreserverImpl::OnConnection(
+    std::unique_ptr<secure_channel::ClientChannel> channel) {
+  DCHECK(base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi));
+  PA_LOG(INFO) << "Successfully preserved connection for device: "
+               << cryptauth::RemoteDeviceRef::TruncateDeviceIdForLogs(
+                      preserved_connection_device_id_);
+
+  // Simply hold on to the ClientChannel until the connection should no longer
+  // be preserved.
+  client_channel_ = std::move(channel);
+}
+
+void ConnectionPreserverImpl::OnDisconnected() {
+  DCHECK(base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi));
+  PA_LOG(INFO) << "Remote device disconnected from this device: "
+               << cryptauth::RemoteDeviceRef::TruncateDeviceIdForLogs(
+                      preserved_connection_device_id_);
+  RemovePreservedConnectionIfPresent();
+}
+
+void ConnectionPreserverImpl::OnMessageReceived(const std::string& payload) {
+  // Do nothing.
+}
+
 void ConnectionPreserverImpl::OnActiveHostChanged(
     const ActiveHost::ActiveHostChangeInfo& change_info) {
   if (change_info.new_status == ActiveHost::ActiveHostStatus::CONNECTED)
@@ -124,8 +169,27 @@ void ConnectionPreserverImpl::SetPreservedConnection(
                << ".";
 
   preserved_connection_device_id_ = device_id;
-  ble_connection_manager_->RegisterRemoteDevice(
-      device_id, request_id_, secure_channel::ConnectionPriority::kLow);
+
+  if (base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi)) {
+    base::Optional<cryptauth::RemoteDeviceRef> remote_device =
+        GetRemoteDevice(preserved_connection_device_id_);
+    if (!remote_device) {
+      PA_LOG(ERROR) << "Given invalid remote device ID: "
+                    << cryptauth::RemoteDeviceRef::TruncateDeviceIdForLogs(
+                           preserved_connection_device_id_);
+      RemovePreservedConnectionIfPresent();
+      return;
+    }
+
+    connection_attempt_ = secure_channel_client_->ListenForConnectionFromDevice(
+        *remote_device, *device_sync_client_->GetLocalDeviceMetadata(),
+        kTetherFeature, secure_channel::ConnectionPriority::kLow);
+    connection_attempt_->SetDelegate(this);
+  } else {
+    ble_connection_manager_->RegisterRemoteDevice(
+        preserved_connection_device_id_, request_id_,
+        secure_channel::ConnectionPriority::kLow);
+  }
 
   preserved_connection_timer_->Start(
       FROM_HERE, base::TimeDelta::FromSeconds(kTimeoutSeconds),
@@ -142,12 +206,28 @@ void ConnectionPreserverImpl::RemovePreservedConnectionIfPresent() {
                       preserved_connection_device_id_)
                << ".";
 
-  ble_connection_manager_->UnregisterRemoteDevice(
-      preserved_connection_device_id_, request_id_);
+  if (base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi)) {
+    connection_attempt_.reset();
+    client_channel_.reset();
+  } else {
+    ble_connection_manager_->UnregisterRemoteDevice(
+        preserved_connection_device_id_, request_id_);
+  }
+
   preserved_connection_device_id_.clear();
   preserved_connection_timer_->Stop();
 }
 
+base::Optional<cryptauth::RemoteDeviceRef>
+ConnectionPreserverImpl::GetRemoteDevice(const std::string device_id) {
+  DCHECK(base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi));
+  for (const auto& remote_device : device_sync_client_->GetSyncedDevices()) {
+    if (remote_device.GetDeviceId() == device_id)
+      return remote_device;
+  }
+  return base::nullopt;
+}
+
 void ConnectionPreserverImpl::SetTimerForTesting(
     std::unique_ptr<base::Timer> timer_for_test) {
   preserved_connection_timer_ = std::move(timer_for_test);

chromeos/components/tether/connection_preserver_impl.h

@@ -11,6 +11,12 @@
 #include "base/unguessable_token.h"
 #include "chromeos/components/tether/active_host.h"
 #include "chromeos/components/tether/connection_preserver.h"
+#include "chromeos/services/device_sync/public/cpp/device_sync_client.h"
+#include "chromeos/services/secure_channel/public/cpp/client/client_channel.h"
+#include "chromeos/services/secure_channel/public/cpp/client/connection_attempt.h"
+#include "chromeos/services/secure_channel/public/cpp/client/secure_channel_client.h"
+#include "chromeos/services/secure_channel/public/cpp/shared/connection_priority.h"
+#include "chromeos/services/secure_channel/public/mojom/secure_channel.mojom.h"
 
 namespace chromeos {
 
@@ -19,11 +25,15 @@ class NetworkStateHandler;
 namespace tether {
 
 class BleConnectionManager;
+class SecureChannelClient;
 class TetherHostResponseRecorder;
 
 // Concrete implementation of ConnectionPreserver.
-class ConnectionPreserverImpl : public ConnectionPreserver,
-                                public ActiveHost::Observer {
+class ConnectionPreserverImpl
+    : public ConnectionPreserver,
+      public secure_channel::ConnectionAttempt::Delegate,
+      public secure_channel::ClientChannel::Observer,
+      public ActiveHost::Observer {
  public:
   // The maximum duration of time that a BLE Connection should be preserved.
   // A preserved BLE Connection will be torn down if not used within this time.
@@ -32,6 +42,8 @@ class ConnectionPreserverImpl : public ConnectionPreserver,
   static constexpr const uint32_t kTimeoutSeconds = 60;
 
   ConnectionPreserverImpl(
+      device_sync::DeviceSyncClient* device_sync_client,
+      secure_channel::SecureChannelClient* secure_channel_client,
       BleConnectionManager* ble_connection_manager,
       NetworkStateHandler* network_state_handler,
       ActiveHost* active_host,
@@ -43,6 +55,16 @@ class ConnectionPreserverImpl : public ConnectionPreserver,
       const std::string& device_id) override;
 
  protected:
+  // secure_channel::ConnectionAttempt::Delegate:
+  void OnConnectionAttemptFailure(
+      secure_channel::mojom::ConnectionAttemptFailureReason reason) override;
+  void OnConnection(
+      std::unique_ptr<secure_channel::ClientChannel> channel) override;
+
+  // secure_channel::ClientChannel::Observer:
+  void OnDisconnected() override;
+  void OnMessageReceived(const std::string& payload) override;
+
   // ActiveHost::Observer:
   void OnActiveHostChanged(
       const ActiveHost::ActiveHostChangeInfo& change_info) override;
@@ -57,9 +79,13 @@ class ConnectionPreserverImpl : public ConnectionPreserver,
       const std::string& device_id);
   void SetPreservedConnection(const std::string& device_id);
   void RemovePreservedConnectionIfPresent();
+  base::Optional<cryptauth::RemoteDeviceRef> GetRemoteDevice(
+      const std::string device_id);
 
   void SetTimerForTesting(std::unique_ptr<base::Timer> timer_for_test);
 
+  device_sync::DeviceSyncClient* device_sync_client_;
+  secure_channel::SecureChannelClient* secure_channel_client_;
   BleConnectionManager* ble_connection_manager_;
   NetworkStateHandler* network_state_handler_;
   ActiveHost* active_host_;
@@ -70,6 +96,9 @@ class ConnectionPreserverImpl : public ConnectionPreserver,
 
   std::string preserved_connection_device_id_;
 
+  std::unique_ptr<secure_channel::ConnectionAttempt> connection_attempt_;
+  std::unique_ptr<secure_channel::ClientChannel> client_channel_;
+
   base::WeakPtrFactory<ConnectionPreserverImpl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ConnectionPreserverImpl);

chromeos/components/tether/host_scanner_operation.cc

@@ -152,7 +152,9 @@ HostScannerOperation::HostScannerOperation(
           connection_manager),
       tether_host_response_recorder_(tether_host_response_recorder),
       connection_preserver_(connection_preserver),
-      clock_(base::DefaultClock::GetInstance()) {}
+      clock_(base::DefaultClock::GetInstance()),
+      task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      weak_ptr_factory_(this) {}
 
 HostScannerOperation::~HostScannerOperation() = default;
 
@@ -237,7 +239,11 @@ void HostScannerOperation::OnMessageReceived(
   RecordTetherAvailabilityResponseDuration(remote_device.GetDeviceId());
 
   // Unregister the device after a TetherAvailabilityResponse has been received.
-  UnregisterDevice(remote_device);
+  // Delay this in order to let |connection_preserver_| fully preserve the
+  // connection, if necessary, before attempting to tear down the connection.
+  task_runner_->PostTask(
+      FROM_HERE, base::BindOnce(&HostScannerOperation::UnregisterDevice,
+                                weak_ptr_factory_.GetWeakPtr(), remote_device));
 }
 
 void HostScannerOperation::OnOperationStarted() {
@@ -256,8 +262,11 @@ MessageType HostScannerOperation::GetMessageTypeForConnection() {
   return MessageType::TETHER_AVAILABILITY_REQUEST;
 }
 
-void HostScannerOperation::SetClockForTest(base::Clock* clock_for_test) {
+void HostScannerOperation::SetTestDoubles(
+    base::Clock* clock_for_test,
+    scoped_refptr<base::TaskRunner> test_task_runner) {
   clock_ = clock_for_test;
+  task_runner_ = test_task_runner;
 }
 
 void HostScannerOperation::RecordTetherAvailabilityResponseDuration(

chromeos/components/tether/host_scanner_operation.h

@@ -123,12 +123,16 @@ class HostScannerOperation : public MessageTransferOperation {
  private:
   friend class HostScannerOperationTest;
 
-  void SetClockForTest(base::Clock* clock_for_test);
+  using MessageTransferOperation::UnregisterDevice;
+
+  void SetTestDoubles(base::Clock* clock_for_test,
+                      scoped_refptr<base::TaskRunner> test_task_runner);
   void RecordTetherAvailabilityResponseDuration(const std::string device_id);
 
   TetherHostResponseRecorder* tether_host_response_recorder_;
   ConnectionPreserver* connection_preserver_;
   base::Clock* clock_;
+  scoped_refptr<base::TaskRunner> task_runner_;
   base::ObserverList<Observer> observer_list_;
 
   cryptauth::RemoteDeviceRefList gms_core_notifications_disabled_devices_;
@@ -136,6 +140,8 @@ class HostScannerOperation : public MessageTransferOperation {
   std::map<std::string, base::Time>
       device_id_to_tether_availability_request_start_time_map_;
 
+  base::WeakPtrFactory<HostScannerOperation> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(HostScannerOperation);
 };
 

chromeos/components/tether/host_scanner_operation_unittest.cc

@@ -11,7 +11,9 @@
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/test/metrics/histogram_tester.h"
+#include "base/test/scoped_task_environment.h"
 #include "base/test/simple_test_clock.h"
+#include "base/test/test_simple_task_runner.h"
 #include "chromeos/components/tether/fake_ble_connection_manager.h"
 #include "chromeos/components/tether/fake_connection_preserver.h"
 #include "chromeos/components/tether/host_scan_device_prioritizer.h"
@@ -162,7 +164,8 @@ class HostScannerOperationTest : public testing::Test {
         remote_devices, operation_->remote_devices());
 
     test_clock_.SetNow(base::Time::UnixEpoch());
-    operation_->SetClockForTest(&test_clock_);
+    test_task_runner_ = base::MakeRefCounted<base::TestSimpleTaskRunner>();
+    operation_->SetTestDoubles(&test_clock_, test_task_runner_);
 
     EXPECT_FALSE(test_observer_->has_received_update());
     operation_->Initialize();
@@ -204,6 +207,7 @@ class HostScannerOperationTest : public testing::Test {
     fake_ble_connection_manager_->ReceiveMessage(
         remote_device.GetDeviceId(), CreateTetherAvailabilityResponseString(
                                          response_code, cell_provider_name));
+    test_task_runner_->RunUntilIdle();
 
     bool tether_available =
         response_code ==
@@ -261,6 +265,8 @@ class HostScannerOperationTest : public testing::Test {
         "InstantTethering.Performance.TetherAvailabilityResponseDuration", 0);
   }
 
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
+
   const std::string tether_availability_request_string_;
   const cryptauth::RemoteDeviceRefList test_devices_;
 
@@ -275,6 +281,7 @@ class HostScannerOperationTest : public testing::Test {
   std::unique_ptr<FakeConnectionPreserver> fake_connection_preserver_;
   std::unique_ptr<TestObserver> test_observer_;
   base::SimpleTestClock test_clock_;
+  scoped_refptr<base::TestSimpleTaskRunner> test_task_runner_;
   std::unique_ptr<HostScannerOperation> operation_;
 
   base::HistogramTester histogram_tester_;

chromeos/components/tether/synchronous_shutdown_object_container_impl.cc

@@ -152,6 +152,8 @@ SynchronousShutdownObjectContainerImpl::SynchronousShutdownObjectContainerImpl(
               active_host_.get(),
               base::DefaultClock::GetInstance())),
       connection_preserver_(std::make_unique<ConnectionPreserverImpl>(
+          device_sync_client,
+          secure_channel_client,
           asychronous_container->ble_connection_manager(),
           network_state_handler_,
           active_host_.get(),

chromeos/services/secure_channel/public/cpp/client/fake_client_channel.cc

@@ -12,7 +12,10 @@ namespace secure_channel {
 
 FakeClientChannel::FakeClientChannel() = default;
 
-FakeClientChannel::~FakeClientChannel() = default;
+FakeClientChannel::~FakeClientChannel() {
+  if (destructor_callback_)
+    std::move(destructor_callback_).Run();
+}
 
 void FakeClientChannel::InvokePendingGetConnectionMetadataCallback(
     mojom::ConnectionMetadataPtr connection_metadata) {

chromeos/services/secure_channel/public/cpp/client/fake_client_channel.h

@@ -7,6 +7,7 @@
 
 #include <queue>
 
+#include "base/callback.h"
 #include "base/macros.h"
 #include "chromeos/services/secure_channel/public/cpp/client/client_channel.h"
 #include "chromeos/services/secure_channel/public/mojom/secure_channel.mojom.h"
@@ -31,6 +32,10 @@ class FakeClientChannel : public ClientChannel {
     return sent_messages_;
   }
 
+  void set_destructor_callback(base::OnceClosure callback) {
+    destructor_callback_ = std::move(callback);
+  }
+
  private:
   friend class SecureChannelClientChannelImplTest;
 
@@ -45,6 +50,7 @@ class FakeClientChannel : public ClientChannel {
   std::queue<base::OnceCallback<void(mojom::ConnectionMetadataPtr)>>
       get_connection_metadata_callback_queue_;
   std::vector<std::pair<std::string, base::OnceClosure>> sent_messages_;
+  base::OnceClosure destructor_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeClientChannel);
 };

chromeos/services/secure_channel/public/cpp/client/fake_secure_channel_client.h

@@ -58,6 +58,45 @@ class FakeSecureChannelClient : public SecureChannelClient {
         device_to_connect, local_device)] = std::move(attempt);
   }
 
+  ConnectionAttempt* peek_next_initiate_connection_attempt(
+      cryptauth::RemoteDeviceRef device_to_connect,
+      cryptauth::RemoteDeviceRef local_device) {
+    auto device_id_pair = std::make_pair(device_to_connect, local_device);
+    if (!base::ContainsKey(device_pair_to_next_initiate_connection_attempt_,
+                           device_id_pair)) {
+      return nullptr;
+    }
+
+    return device_pair_to_next_initiate_connection_attempt_[device_id_pair]
+        .get();
+  }
+
+  ConnectionAttempt* peek_next_listen_connection_attempt(
+      cryptauth::RemoteDeviceRef device_to_connect,
+      cryptauth::RemoteDeviceRef local_device) {
+    auto device_id_pair = std::make_pair(device_to_connect, local_device);
+    if (!base::ContainsKey(device_pair_to_next_listen_connection_attempt_,
+                           device_id_pair)) {
+      return nullptr;
+    }
+
+    return device_pair_to_next_listen_connection_attempt_[device_id_pair].get();
+  }
+
+  void clear_next_initiate_connection_attempt(
+      cryptauth::RemoteDeviceRef device_to_connect,
+      cryptauth::RemoteDeviceRef local_device) {
+    device_pair_to_next_initiate_connection_attempt_.erase(
+        std::make_pair(device_to_connect, local_device));
+  }
+
+  void clear_next_listen_connection_attempt(
+      cryptauth::RemoteDeviceRef device_to_connect,
+      cryptauth::RemoteDeviceRef local_device) {
+    device_pair_to_next_listen_connection_attempt_.erase(
+        std::make_pair(device_to_connect, local_device));
+  }
+
   std::vector<ConnectionRequestArguments*>
   last_initiate_connection_request_arguments_list() {
     std::vector<ConnectionRequestArguments*> arguments_list_raw_;