[CrOS Multidevice] Integrate SecureChannel API into ProximityAuthMonitor.

This injects a ClientChannel into ProximityAuthMonitor, which is used if the
chromeos::features::kMultiDeviceApi is enabled. The ClientChannel is used
to get the current RSSI of the connected remote device.

In the future, the "rolling average RSSI" that is calculated in
ProximityAuthMonitor will be moved to the SecureChannel API, and returned
by it. However, to reduce immediate migration work, that logic is kept
in ProximityAuthMonitor for now.

This CL was originally submitted as [1], but was reverted [2] due to a
subtle bug in a test, which has been fixed at [3]. This CL is unchanged
from the original.

1) https://chromium-review.googlesource.com/c/chromium/src/+/1106616
2) https://chromium-review.googlesource.com/c/chromium/src/+/1111937
3) https://chromium-review.googlesource.com/c/chromium/src/+/1112422

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

chromeos/components/proximity_auth/BUILD.gn

@@ -55,6 +55,8 @@ static_library("proximity_auth") {
     "//chromeos",
     "//chromeos/components/proximity_auth/logging",
     "//chromeos/components/proximity_auth/public/interfaces",
+    "//chromeos/services/secure_channel/public/cpp/client",
+    "//chromeos/services/secure_channel/public/mojom",
     "//components/account_id",
     "//components/cryptauth",
     "//components/cryptauth/ble",
@@ -113,6 +115,7 @@ source_set("unit_tests") {
     "//chromeos",
     "//chromeos/components/proximity_auth/logging",
     "//chromeos/components/proximity_auth/logging:unit_tests",
+    "//chromeos/services/secure_channel/public/cpp/client:test_support",
     "//components/cryptauth:test_support",
     "//components/prefs:test_support",
     "//components/sync_preferences:test_support",

chromeos/components/proximity_auth/proximity_monitor_impl.cc

@@ -16,6 +16,7 @@
 #include "chromeos/components/proximity_auth/metrics.h"
 #include "chromeos/components/proximity_auth/proximity_auth_pref_manager.h"
 #include "chromeos/components/proximity_auth/proximity_monitor_observer.h"
+#include "chromeos/services/secure_channel/public/cpp/client/client_channel.h"
 #include "device/bluetooth/bluetooth_adapter.h"
 #include "device/bluetooth/bluetooth_adapter_factory.h"
 
@@ -33,13 +34,17 @@ const double kRssiSampleWeight = 0.3;
 const int kDefaultRssiThreshold = -70;
 
 ProximityMonitorImpl::ProximityMonitorImpl(
+    cryptauth::RemoteDeviceRef remote_device,
+    chromeos::secure_channel::ClientChannel* channel,
     cryptauth::Connection* connection,
     ProximityAuthPrefManager* pref_manager)
-    : connection_(connection),
+    : remote_device_(remote_device),
+      channel_(channel),
+      connection_(connection),
+      pref_manager_(pref_manager),
       remote_device_is_in_proximity_(false),
       is_active_(false),
       rssi_threshold_(kDefaultRssiThreshold),
-      pref_manager_(pref_manager),
       polling_weak_ptr_factory_(this),
       weak_ptr_factory_(this) {
   if (device::BluetoothAdapterFactory::IsBluetoothSupported()) {
@@ -76,7 +81,7 @@ void ProximityMonitorImpl::RecordProximityMetricsOnAuthSuccess() {
                                     : metrics::kUnknownProximityValue;
 
   std::string remote_device_model = metrics::kUnknownDeviceModel;
-  cryptauth::RemoteDeviceRef remote_device = connection_->remote_device();
+  cryptauth::RemoteDeviceRef remote_device = remote_device_;
   if (!remote_device.name().empty())
     remote_device_model = remote_device.name();
 
@@ -131,37 +136,69 @@ bool ProximityMonitorImpl::ShouldPoll() const {
 void ProximityMonitorImpl::Poll() {
   DCHECK(ShouldPoll());
 
-  std::string address = connection_->GetDeviceAddress();
-  BluetoothDevice* device = bluetooth_adapter_->GetDevice(address);
+  if (base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi)) {
+    if (channel_->is_disconnected()) {
+      PA_LOG(ERROR) << "Channel is disconnected.";
+      ClearProximityState();
+      return;
+    }
 
-  if (!device) {
-    PA_LOG(ERROR) << "Unknown Bluetooth device with address " << address;
-    ClearProximityState();
-    return;
-  }
-  if (!device->IsConnected()) {
-    PA_LOG(ERROR) << "Bluetooth device with address " << address
-                  << " is not connected.";
-    ClearProximityState();
-    return;
+    channel_->GetConnectionMetadata(
+        base::BindOnce(&ProximityMonitorImpl::OnGetConnectionMetadata,
+                       weak_ptr_factory_.GetWeakPtr()));
+  } else {
+    std::string address = connection_->GetDeviceAddress();
+    BluetoothDevice* device = bluetooth_adapter_->GetDevice(address);
+
+    if (!device) {
+      PA_LOG(ERROR) << "Unknown Bluetooth device with address " << address;
+      ClearProximityState();
+      return;
+    }
+    if (!device->IsConnected()) {
+      PA_LOG(ERROR) << "Bluetooth device with address " << address
+                    << " is not connected.";
+      ClearProximityState();
+      return;
+    }
+
+    device->GetConnectionInfo(
+        base::BindRepeating(&ProximityMonitorImpl::OnConnectionInfo,
+                            weak_ptr_factory_.GetWeakPtr()));
   }
+}
+
+void ProximityMonitorImpl::OnGetConnectionMetadata(
+    chromeos::secure_channel::mojom::ConnectionMetadataPtr
+        connection_metadata) {
+  DCHECK(base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi));
 
-  device->GetConnectionInfo(base::Bind(&ProximityMonitorImpl::OnConnectionInfo,
-                                       weak_ptr_factory_.GetWeakPtr()));
+  if (connection_metadata->bluetooth_connection_metadata)
+    OnGetRssi(connection_metadata->bluetooth_connection_metadata->current_rssi);
+  else
+    OnGetRssi(base::nullopt);
 }
 
 void ProximityMonitorImpl::OnConnectionInfo(
     const BluetoothDevice::ConnectionInfo& connection_info) {
+  DCHECK(!base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi));
+
+  if (connection_info.rssi == BluetoothDevice::kUnknownPower)
+    OnGetRssi(base::nullopt);
+  else
+    OnGetRssi(connection_info.rssi);
+}
+
+void ProximityMonitorImpl::OnGetRssi(const base::Optional<int32_t>& rssi) {
   if (!is_active_) {
-    PA_LOG(INFO) << "[Proximity] Got connection info after stopping";
+    PA_LOG(INFO) << "Received RSSI after stopping.";
     return;
   }
 
-  if (connection_info.rssi != BluetoothDevice::kUnknownPower) {
-    AddSample(connection_info);
+  if (rssi) {
+    AddSample(*rssi);
   } else {
-    PA_LOG(WARNING) << "[Proximity] Unknown values received from API: "
-                    << connection_info.rssi;
+    PA_LOG(WARNING) << "Received invalid RSSI value.";
     rssi_rolling_average_.reset();
     CheckForProximityStateChange();
   }
@@ -177,14 +214,13 @@ void ProximityMonitorImpl::ClearProximityState() {
   rssi_rolling_average_.reset();
 }
 
-void ProximityMonitorImpl::AddSample(
-    const BluetoothDevice::ConnectionInfo& connection_info) {
+void ProximityMonitorImpl::AddSample(int32_t rssi) {
   double weight = kRssiSampleWeight;
   if (!rssi_rolling_average_) {
-    rssi_rolling_average_.reset(new double(connection_info.rssi));
+    rssi_rolling_average_.reset(new double(rssi));
   } else {
     *rssi_rolling_average_ =
-        weight * connection_info.rssi + (1 - weight) * (*rssi_rolling_average_);
+        weight * rssi + (1 - weight) * (*rssi_rolling_average_);
   }
 
   CheckForProximityStateChange();

chromeos/components/proximity_auth/proximity_monitor_impl.h

@@ -11,14 +11,23 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
+#include "base/optional.h"
+#include "chromeos/chromeos_features.h"
 #include "chromeos/components/proximity_auth/proximity_monitor.h"
+#include "chromeos/services/secure_channel/public/mojom/secure_channel.mojom.h"
 #include "components/cryptauth/connection.h"
 #include "components/cryptauth/remote_device_ref.h"
 #include "device/bluetooth/bluetooth_device.h"
 
+namespace chromeos {
+namespace secure_channel {
+class ClientChannel;
+}  // namespace secure_channel
+}  // namespace chromeos
+
 namespace device {
 class BluetoothAdapter;
-}
+}  // namespace device
 
 namespace proximity_auth {
 
@@ -29,7 +38,9 @@ class ProximityMonitorObserver;
 class ProximityMonitorImpl : public ProximityMonitor {
  public:
   // The |connection| is not owned, and must outlive |this| instance.
-  ProximityMonitorImpl(cryptauth::Connection* connection,
+  ProximityMonitorImpl(cryptauth::RemoteDeviceRef remote_device,
+                       chromeos::secure_channel::ClientChannel* channel,
+                       cryptauth::Connection* connection,
                        ProximityAuthPrefManager* pref_manager);
   ~ProximityMonitorImpl() override;
 
@@ -62,18 +73,19 @@ class ProximityMonitorImpl : public ProximityMonitor {
   // Polls the connection information.
   void Poll();
 
-  // Callback to received the polled-for connection info.
+  void OnGetConnectionMetadata(
+      chromeos::secure_channel::mojom::ConnectionMetadataPtr
+          connection_metadata);
   void OnConnectionInfo(
       const device::BluetoothDevice::ConnectionInfo& connection_info);
+  void OnGetRssi(const base::Optional<int32_t>& rssi);
 
   // Resets the proximity state to |false|, and clears all member variables
   // tracking the proximity state.
   void ClearProximityState();
 
-  // Updates the proximity state with a new |connection_info| sample of the
-  // current RSSI.
-  void AddSample(
-      const device::BluetoothDevice::ConnectionInfo& connection_info);
+  // Updates the proximity state with a new sample of the current RSSI.
+  void AddSample(int32_t rssi);
 
   // Checks whether the proximity state has changed based on the current
   // samples. Notifies |observers_| on a change.
@@ -83,10 +95,19 @@ class ProximityMonitorImpl : public ProximityMonitor {
   // RSSI value.
   void GetRssiThresholdFromPrefs();
 
-  // The current connection being monitored. Not owned and must outlive this
-  // instance.
+  // Used to get the name of the remote device that ProximitMonitor is
+  // communicating with, for metrics purposes.
+  cryptauth::RemoteDeviceRef remote_device_;
+
+  // Used to communicate with the remote device to gauge its proximity via RSSI
+  // measurement.
+  chromeos::secure_channel::ClientChannel* channel_;
   cryptauth::Connection* connection_;
 
+  // Used to get determine the user pref for how far away the phone is allowed
+  // to be.
+  ProximityAuthPrefManager* pref_manager_;
+
   // The observers attached to the ProximityMonitor.
   base::ObserverList<ProximityMonitorObserver> observers_;
 
@@ -110,10 +131,6 @@ class ProximityMonitorImpl : public ProximityMonitor {
   // measurement.
   std::unique_ptr<double> rssi_rolling_average_;
 
-  // Contains perferences that outlive the lifetime of this object and across
-  // process restarts. Not owned and must outlive this instance.
-  ProximityAuthPrefManager* pref_manager_;
-
   // Used to vend weak pointers for polling. Using a separate factory for these
   // weak pointers allows the weak pointers to be invalidated when polling
   // stops, which effectively cancels the scheduled tasks.

chromeos/components/proximity_auth/unlock_manager_impl.cc

@@ -324,7 +324,10 @@ void UnlockManagerImpl::OnAuthAttempted(mojom::AuthType auth_type) {
 std::unique_ptr<ProximityMonitor> UnlockManagerImpl::CreateProximityMonitor(
     cryptauth::Connection* connection,
     ProximityAuthPrefManager* pref_manager) {
-  return std::make_unique<ProximityMonitorImpl>(connection, pref_manager);
+  // TODO(crbug.com/752273): Inject a real ClientChannel.
+  return std::make_unique<ProximityMonitorImpl>(connection->remote_device(),
+                                                nullptr /* channel */,
+                                                connection, pref_manager);
 }
 
 void UnlockManagerImpl::SendSignInChallenge() {

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

@@ -14,9 +14,16 @@ FakeClientChannel::FakeClientChannel() = default;
 
 FakeClientChannel::~FakeClientChannel() = default;
 
+void FakeClientChannel::InvokePendingGetConnectionMetadataCallback(
+    mojom::ConnectionMetadataPtr connection_metadata) {
+  std::move(get_connection_metadata_callback_queue_.front())
+      .Run(std::move(connection_metadata));
+  get_connection_metadata_callback_queue_.pop();
+}
+
 void FakeClientChannel::PerformGetConnectionMetadata(
     base::OnceCallback<void(mojom::ConnectionMetadataPtr)> callback) {
-  std::move(callback).Run(std::move(connection_metadata_for_next_call_));
+  get_connection_metadata_callback_queue_.push(std::move(callback));
 }
 
 void FakeClientChannel::PerformSendMessage(const std::string& payload,

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

@@ -5,6 +5,8 @@
 #ifndef CHROMEOS_SERVICES_SECURE_CHANNEL_PUBLIC_CPP_CLIENT_FAKE_CLIENT_CHANNEL_H_
 #define CHROMEOS_SERVICES_SECURE_CHANNEL_PUBLIC_CPP_CLIENT_FAKE_CLIENT_CHANNEL_H_
 
+#include <queue>
+
 #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"
@@ -22,11 +24,8 @@ class FakeClientChannel : public ClientChannel {
   using ClientChannel::NotifyDisconnected;
   using ClientChannel::NotifyMessageReceived;
 
-  void set_connection_metadata_for_next_call(
-      mojom::ConnectionMetadataPtr connection_metadata_for_next_call) {
-    connection_metadata_for_next_call_ =
-        std::move(connection_metadata_for_next_call);
-  }
+  void InvokePendingGetConnectionMetadataCallback(
+      mojom::ConnectionMetadataPtr connection_metadata);
 
   std::vector<std::pair<std::string, base::OnceClosure>>& sent_messages() {
     return sent_messages_;
@@ -41,7 +40,10 @@ class FakeClientChannel : public ClientChannel {
   void PerformSendMessage(const std::string& payload,
                           base::OnceClosure on_sent_callback) override;
 
-  mojom::ConnectionMetadataPtr connection_metadata_for_next_call_;
+  // Queues up callbacks passed into PerformGetConnectionMetadata(), to be
+  // invoked later.
+  std::queue<base::OnceCallback<void(mojom::ConnectionMetadataPtr)>>
+      get_connection_metadata_callback_queue_;
   std::vector<std::pair<std::string, base::OnceClosure>> sent_messages_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeClientChannel);