[CrOS Multidevice] Tether: Use SecureChannelClient in MessageTransferOperation.

MessageTransferOperation now uses the SecureChannelClient to request secure connections
to remote devices, instead of BleConnectionManager. This functionality is gated behind
the chromeos::features::kMultiDeviceApi.

This CL completely migrates Tether to the SecureChannel and DeviceSync services (besides
the feature flags gating their use, which will be removed in the next milestone).

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

chromeos/components/tether/BUILD.gn

@@ -137,6 +137,7 @@ static_library("tether") {
     "//chromeos/components/tether/proto",
     "//chromeos/services/device_sync/public/cpp",
     "//chromeos/services/secure_channel/public/cpp/client",
+    "//chromeos/services/secure_channel/public/mojom",
 
     # TODO(hansberry): Remove //chromeos/services/secure_channel dependency when
     # SecureChannelClient migration is complete.
@@ -301,6 +302,7 @@ source_set("unit_tests") {
 
     # TODO(hansberry): Remove //chromeos/services/secure_channel dependency when
     # SecureChannelClient migration is complete.
+    "//chromeos/services/device_sync/public/mojom",
     "//chromeos/services/secure_channel",
     "//chromeos/services/secure_channel:test_support",
     "//chromeos/services/secure_channel/public/cpp/client:test_support",

chromeos/components/tether/message_transfer_operation.cc

@@ -7,6 +7,7 @@
 #include <memory>
 #include <set>
 
+#include "chromeos/chromeos_features.h"
 #include "chromeos/components/proximity_auth/logging/logging.h"
 #include "chromeos/components/tether/message_wrapper.h"
 #include "chromeos/components/tether/timer_factory.h"
@@ -41,6 +42,53 @@ const uint32_t MessageTransferOperation::kMaxEmptyScansPerDevice = 3;
 const uint32_t MessageTransferOperation::kMaxGattConnectionAttemptsPerDevice =
     6;
 
+MessageTransferOperation::ConnectionAttemptDelegate::ConnectionAttemptDelegate(
+    MessageTransferOperation* operation,
+    cryptauth::RemoteDeviceRef remote_device,
+    std::unique_ptr<secure_channel::ConnectionAttempt> connection_attempt)
+    : operation_(operation),
+      remote_device_(remote_device),
+      connection_attempt_(std::move(connection_attempt)) {
+  connection_attempt_->SetDelegate(this);
+}
+
+MessageTransferOperation::ConnectionAttemptDelegate::
+    ~ConnectionAttemptDelegate() = default;
+
+void MessageTransferOperation::ConnectionAttemptDelegate::
+    OnConnectionAttemptFailure(
+        secure_channel::mojom::ConnectionAttemptFailureReason reason) {
+  operation_->OnConnectionAttemptFailure(remote_device_, reason);
+}
+
+void MessageTransferOperation::ConnectionAttemptDelegate::OnConnection(
+    std::unique_ptr<secure_channel::ClientChannel> channel) {
+  operation_->OnConnection(remote_device_, std::move(channel));
+}
+
+MessageTransferOperation::ClientChannelObserver::ClientChannelObserver(
+    MessageTransferOperation* operation,
+    cryptauth::RemoteDeviceRef remote_device,
+    std::unique_ptr<secure_channel::ClientChannel> client_channel)
+    : operation_(operation),
+      remote_device_(remote_device),
+      client_channel_(std::move(client_channel)) {
+  client_channel_->AddObserver(this);
+}
+
+MessageTransferOperation::ClientChannelObserver::~ClientChannelObserver() {
+  client_channel_->RemoveObserver(this);
+}
+
+void MessageTransferOperation::ClientChannelObserver::OnDisconnected() {
+  operation_->OnDisconnected(remote_device_);
+}
+
+void MessageTransferOperation::ClientChannelObserver::OnMessageReceived(
+    const std::string& payload) {
+  operation_->OnMessageReceived(remote_device_, payload);
+}
+
 MessageTransferOperation::MessageTransferOperation(
     const cryptauth::RemoteDeviceRefList& devices_to_connect,
     secure_channel::ConnectionPriority connection_priority,
@@ -61,7 +109,8 @@ MessageTransferOperation::~MessageTransferOperation() {
   if (!initialized_)
     return;
 
-  connection_manager_->RemoveObserver(this);
+  if (!base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi))
+    connection_manager_->RemoveObserver(this);
 
   shutting_down_ = true;
 
@@ -87,21 +136,30 @@ void MessageTransferOperation::Initialize() {
   // function at that time.
   message_type_for_connection_ = GetMessageTypeForConnection();
 
-  connection_manager_->AddObserver(this);
-  OnOperationStarted();
+  if (!base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi))
+    connection_manager_->AddObserver(this);
 
-  // TODO(crbug.com/752273): Use |secure_channel_client_|.
+  OnOperationStarted();
 
   for (const auto& remote_device : remote_devices_) {
-    connection_manager_->RegisterRemoteDevice(
-        remote_device.GetDeviceId(), request_id_, connection_priority_);
-
-    cryptauth::SecureChannel::Status status;
-    if (connection_manager_->GetStatusForDevice(remote_device.GetDeviceId(),
-                                                &status) &&
-        status == cryptauth::SecureChannel::Status::AUTHENTICATED) {
-      StartTimerForDevice(remote_device);
-      OnDeviceAuthenticated(remote_device);
+    if (base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi)) {
+      remote_device_to_connection_attempt_delegate_map_[remote_device] =
+          std::make_unique<ConnectionAttemptDelegate>(
+              this, remote_device,
+              secure_channel_client_->ListenForConnectionFromDevice(
+                  remote_device, *device_sync_client_->GetLocalDeviceMetadata(),
+                  "tether", connection_priority_));
+    } else {
+      connection_manager_->RegisterRemoteDevice(
+          remote_device.GetDeviceId(), request_id_, connection_priority_);
+
+      cryptauth::SecureChannel::Status status;
+      if (connection_manager_->GetStatusForDevice(remote_device.GetDeviceId(),
+                                                  &status) &&
+          status == cryptauth::SecureChannel::Status::AUTHENTICATED) {
+        StartTimerForDevice(remote_device);
+        OnDeviceAuthenticated(remote_device);
+      }
     }
   }
 }
@@ -111,6 +169,8 @@ void MessageTransferOperation::OnSecureChannelStatusChanged(
     const cryptauth::SecureChannel::Status& old_status,
     const cryptauth::SecureChannel::Status& new_status,
     BleConnectionManager::StateChangeDetail status_change_detail) {
+  DCHECK(!base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi));
+
   base::Optional<cryptauth::RemoteDeviceRef> remote_device =
       GetRemoteDevice(device_id);
   if (!remote_device) {
@@ -166,14 +226,25 @@ void MessageTransferOperation::UnregisterDevice(
   // cause the original reference to be deleted.
   cryptauth::RemoteDeviceRef remote_device_copy = remote_device;
 
-  remote_device_to_attempts_map_.erase(remote_device_copy);
+  if (!base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi))
+    remote_device_to_attempts_map_.erase(remote_device_copy);
+
   remote_devices_.erase(std::remove(remote_devices_.begin(),
                                     remote_devices_.end(), remote_device_copy),
                         remote_devices_.end());
   StopTimerForDeviceIfRunning(remote_device_copy);
 
-  connection_manager_->UnregisterRemoteDevice(remote_device_copy.GetDeviceId(),
-                                              request_id_);
+  if (base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi)) {
+    remote_device_to_connection_attempt_delegate_map_.erase(remote_device);
+
+    if (base::ContainsKey(remote_device_to_client_channel_observer_map_,
+                          remote_device)) {
+      remote_device_to_client_channel_observer_map_.erase(remote_device);
+    }
+  } else {
+    connection_manager_->UnregisterRemoteDevice(
+        remote_device_copy.GetDeviceId(), request_id_);
+  }
 
   if (!shutting_down_ && remote_devices_.empty())
     OnOperationFinished();
@@ -182,17 +253,67 @@ void MessageTransferOperation::UnregisterDevice(
 int MessageTransferOperation::SendMessageToDevice(
     cryptauth::RemoteDeviceRef remote_device,
     std::unique_ptr<MessageWrapper> message_wrapper) {
-  return connection_manager_->SendMessage(remote_device.GetDeviceId(),
-                                          message_wrapper->ToRawMessage());
+  if (base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi)) {
+    DCHECK(base::ContainsKey(remote_device_to_client_channel_observer_map_,
+                             remote_device));
+    int sequence_number = next_message_sequence_number_++;
+    bool success =
+        remote_device_to_client_channel_observer_map_[remote_device]
+            ->channel()
+            ->SendMessage(message_wrapper->ToRawMessage(),
+                          base::BindOnce(
+                              &MessageTransferOperation::OnMessageSent,
+                              weak_ptr_factory_.GetWeakPtr(), sequence_number));
+    return success ? sequence_number : -1;
+  } else {
+    return connection_manager_->SendMessage(remote_device.GetDeviceId(),
+                                            message_wrapper->ToRawMessage());
+  }
 }
 
 uint32_t MessageTransferOperation::GetTimeoutSeconds() {
   return MessageTransferOperation::kDefaultTimeoutSeconds;
 }
 
+void MessageTransferOperation::OnConnectionAttemptFailure(
+    cryptauth::RemoteDeviceRef remote_device,
+    secure_channel::mojom::ConnectionAttemptFailureReason reason) {
+  DCHECK(base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi));
+  PA_LOG(WARNING) << "Failed to connect to device "
+                  << remote_device.GetTruncatedDeviceIdForLogs()
+                  << ", error: " << reason;
+  UnregisterDevice(remote_device);
+}
+
+void MessageTransferOperation::OnConnection(
+    cryptauth::RemoteDeviceRef remote_device,
+    std::unique_ptr<secure_channel::ClientChannel> channel) {
+  DCHECK(base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi));
+  remote_device_to_client_channel_observer_map_[remote_device] =
+      std::make_unique<ClientChannelObserver>(this, remote_device,
+                                              std::move(channel));
+  StartTimerForDevice(remote_device);
+  OnDeviceAuthenticated(remote_device);
+}
+
+void MessageTransferOperation::OnDisconnected(
+    cryptauth::RemoteDeviceRef remote_device) {
+  PA_LOG(INFO) << "Remote device disconnected from this device: "
+               << remote_device.GetTruncatedDeviceIdForLogs();
+  UnregisterDevice(remote_device);
+}
+
+void MessageTransferOperation::OnMessageReceived(
+    cryptauth::RemoteDeviceRef remote_device,
+    const std::string& payload) {
+  OnMessageReceived(remote_device.GetDeviceId(), payload);
+}
+
 void MessageTransferOperation::HandleDeviceDisconnection(
     cryptauth::RemoteDeviceRef remote_device,
     BleConnectionManager::StateChangeDetail status_change_detail) {
+  DCHECK(!base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi));
+
   ConnectAttemptCounts& attempts_for_device =
       remote_device_to_attempts_map_[remote_device];
 

chromeos/components/tether/message_transfer_operation.h

@@ -14,8 +14,11 @@
 #include "base/unguessable_token.h"
 #include "chromeos/components/tether/ble_connection_manager.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 {
 
@@ -112,6 +115,47 @@ class MessageTransferOperation : public BleConnectionManager::Observer {
   friend class HostScannerOperationTest;
   friend class MessageTransferOperationTest;
 
+  class ConnectionAttemptDelegate
+      : public secure_channel::ConnectionAttempt::Delegate {
+   public:
+    ConnectionAttemptDelegate(
+        MessageTransferOperation* operation,
+        cryptauth::RemoteDeviceRef remote_device,
+        std::unique_ptr<secure_channel::ConnectionAttempt> connection_attempt);
+    ~ConnectionAttemptDelegate() override;
+
+    // secure_channel::ConnectionAttempt::Delegate:
+    void OnConnectionAttemptFailure(
+        secure_channel::mojom::ConnectionAttemptFailureReason reason) override;
+    void OnConnection(
+        std::unique_ptr<secure_channel::ClientChannel> channel) override;
+
+   private:
+    MessageTransferOperation* operation_;
+    cryptauth::RemoteDeviceRef remote_device_;
+    std::unique_ptr<secure_channel::ConnectionAttempt> connection_attempt_;
+  };
+
+  class ClientChannelObserver : public secure_channel::ClientChannel::Observer {
+   public:
+    ClientChannelObserver(
+        MessageTransferOperation* operation,
+        cryptauth::RemoteDeviceRef remote_device,
+        std::unique_ptr<secure_channel::ClientChannel> client_channel);
+    ~ClientChannelObserver() override;
+
+    // secure_channel::ClientChannel::Observer:
+    void OnDisconnected() override;
+    void OnMessageReceived(const std::string& payload) override;
+
+    secure_channel::ClientChannel* channel() { return client_channel_.get(); }
+
+   private:
+    MessageTransferOperation* operation_;
+    cryptauth::RemoteDeviceRef remote_device_;
+    std::unique_ptr<secure_channel::ClientChannel> client_channel_;
+  };
+
   // The default number of seconds an operation should wait before a timeout
   // occurs. Once this amount of time passes, the connection will be closed.
   // Classes deriving from MessageTransferOperation should override
@@ -123,6 +167,15 @@ class MessageTransferOperation : public BleConnectionManager::Observer {
     uint32_t gatt_connection_attempts = 0;
   };
 
+  void OnConnectionAttemptFailure(
+      cryptauth::RemoteDeviceRef remote_device,
+      secure_channel::mojom::ConnectionAttemptFailureReason reason);
+  void OnConnection(cryptauth::RemoteDeviceRef remote_device,
+                    std::unique_ptr<secure_channel::ClientChannel> channel);
+  void OnDisconnected(cryptauth::RemoteDeviceRef remote_device);
+  void OnMessageReceived(cryptauth::RemoteDeviceRef remote_device,
+                         const std::string& payload);
+
   void HandleDeviceDisconnection(
       cryptauth::RemoteDeviceRef remote_device,
       BleConnectionManager::StateChangeDetail status_change_detail);
@@ -147,9 +200,18 @@ class MessageTransferOperation : public BleConnectionManager::Observer {
   bool initialized_ = false;
   bool shutting_down_ = false;
   MessageType message_type_for_connection_;
-  std::map<cryptauth::RemoteDeviceRef, ConnectAttemptCounts>
+
+  base::flat_map<cryptauth::RemoteDeviceRef,
+                 std::unique_ptr<ConnectionAttemptDelegate>>
+      remote_device_to_connection_attempt_delegate_map_;
+  base::flat_map<cryptauth::RemoteDeviceRef,
+                 std::unique_ptr<ClientChannelObserver>>
+      remote_device_to_client_channel_observer_map_;
+  int next_message_sequence_number_ = 0;
+
+  base::flat_map<cryptauth::RemoteDeviceRef, ConnectAttemptCounts>
       remote_device_to_attempts_map_;
-  std::map<cryptauth::RemoteDeviceRef, std::unique_ptr<base::Timer>>
+  base::flat_map<cryptauth::RemoteDeviceRef, std::unique_ptr<base::Timer>>
       remote_device_to_timer_map_;
   base::WeakPtrFactory<MessageTransferOperation> weak_ptr_factory_;
 

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

@@ -20,6 +20,9 @@ class FakeConnectionAttempt : public ConnectionAttemptImpl {
   FakeConnectionAttempt();
   ~FakeConnectionAttempt() override;
 
+  using ConnectionAttempt::NotifyConnectionAttemptFailure;
+  using ConnectionAttempt::NotifyConnection;
+
   // ConnectionAttemptImpl:
   void OnConnectionAttemptFailure(
       mojom::ConnectionAttemptFailureReason reason) override;

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

@@ -26,8 +26,8 @@ FakeSecureChannelClient::ConnectionRequestArguments::
 FakeSecureChannelClient::FakeSecureChannelClient() = default;
 
 FakeSecureChannelClient::~FakeSecureChannelClient() {
-  DCHECK(!next_initiate_connection_connection_attempt_);
-  DCHECK(!next_listen_for_connection_connection_attempt_);
+  DCHECK(device_pair_to_next_initiate_connection_attempt_.empty());
+  DCHECK(device_pair_to_next_listen_connection_attempt_.empty());
 }
 
 std::unique_ptr<ConnectionAttempt>
@@ -36,10 +36,11 @@ FakeSecureChannelClient::InitiateConnectionToDevice(
     cryptauth::RemoteDeviceRef local_device,
     const std::string& feature,
     ConnectionPriority connection_priority) {
-  last_initiate_connection_request_arguments_list_.push_back(
-      std::make_unique<ConnectionRequestArguments>(
-          device_to_connect, local_device, feature, connection_priority));
-  return std::move(next_initiate_connection_connection_attempt_);
+  auto remote_local_pair = std::make_pair(device_to_connect, local_device);
+  std::unique_ptr<ConnectionAttempt> connection_attempt = std::move(
+      device_pair_to_next_initiate_connection_attempt_[remote_local_pair]);
+  device_pair_to_next_initiate_connection_attempt_.erase(remote_local_pair);
+  return connection_attempt;
 }
 
 std::unique_ptr<ConnectionAttempt>
@@ -48,10 +49,11 @@ FakeSecureChannelClient::ListenForConnectionFromDevice(
     cryptauth::RemoteDeviceRef local_device,
     const std::string& feature,
     ConnectionPriority connection_priority) {
-  last_listen_for_connection_request_arguments_list_.push_back(
-      std::make_unique<ConnectionRequestArguments>(
-          device_to_connect, local_device, feature, connection_priority));
-  return std::move(next_listen_for_connection_connection_attempt_);
+  auto remote_local_pair = std::make_pair(device_to_connect, local_device);
+  std::unique_ptr<ConnectionAttempt> connection_attempt = std::move(
+      device_pair_to_next_listen_connection_attempt_[remote_local_pair]);
+  device_pair_to_next_listen_connection_attempt_.erase(remote_local_pair);
+  return connection_attempt;
 }
 
 }  // namespace secure_channel

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

@@ -42,14 +42,20 @@ class FakeSecureChannelClient : public SecureChannelClient {
   FakeSecureChannelClient();
   ~FakeSecureChannelClient() override;
 
-  void set_initiate_connection_connection_attempt(
+  void set_next_initiate_connection_attempt(
+      cryptauth::RemoteDeviceRef device_to_connect,
+      cryptauth::RemoteDeviceRef local_device,
       std::unique_ptr<ConnectionAttempt> attempt) {
-    next_initiate_connection_connection_attempt_ = std::move(attempt);
+    device_pair_to_next_initiate_connection_attempt_[std::make_pair(
+        device_to_connect, local_device)] = std::move(attempt);
   }
 
-  void set_listen_for_connection_connection_attempt(
+  void set_next_listen_connection_attempt(
+      cryptauth::RemoteDeviceRef device_to_connect,
+      cryptauth::RemoteDeviceRef local_device,
       std::unique_ptr<ConnectionAttempt> attempt) {
-    next_listen_for_connection_connection_attempt_ = std::move(attempt);
+    device_pair_to_next_listen_connection_attempt_[std::make_pair(
+        device_to_connect, local_device)] = std::move(attempt);
   }
 
   std::vector<ConnectionRequestArguments*>
@@ -85,10 +91,15 @@ class FakeSecureChannelClient : public SecureChannelClient {
       ConnectionPriority connection_priority) override;
 
  private:
-  std::unique_ptr<ConnectionAttempt>
-      next_initiate_connection_connection_attempt_;
-  std::unique_ptr<ConnectionAttempt>
-      next_listen_for_connection_connection_attempt_;
+  // First element of pair is remote device, second is local device.
+  base::flat_map<
+      std::pair<cryptauth::RemoteDeviceRef, cryptauth::RemoteDeviceRef>,
+      std::unique_ptr<ConnectionAttempt>>
+      device_pair_to_next_initiate_connection_attempt_;
+  base::flat_map<
+      std::pair<cryptauth::RemoteDeviceRef, cryptauth::RemoteDeviceRef>,
+      std::unique_ptr<ConnectionAttempt>>
+      device_pair_to_next_listen_connection_attempt_;
 
   std::vector<std::unique_ptr<ConnectionRequestArguments>>
       last_initiate_connection_request_arguments_list_;