Revert "[CrOS Multidevice] Integrate SecureChannel API into ProximityAuthMonitor."

This reverts commit e092b1bd.

Reason for revert:
SecureChannelClientImplTest.TestMultipleConnections has been failing
consistently on the waterfall since this landed. Started here:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/linux-chromeos-rel/9728


Original change's description:
> [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.
> 
> R=​jhawkins@chromium.org, khorimoto@chromium.org
> 
> Bug: 824568, 752273
> Change-Id: I8d6485a5a0018fe43595b880c25a2fa9af5a1b75
> Reviewed-on: https://chromium-review.googlesource.com/1106616
> Commit-Queue: Ryan Hansberry <hansberry@chromium.org>
> Reviewed-by: James Hawkins <jhawkins@chromium.org>
> Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#569425}

TBR=jhawkins@chromium.org,khorimoto@chromium.org,hansberry@chromium.org

Change-Id: Ie22413bae9011ed505220115fa1c711fc1c7e626
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 824568, 752273
Reviewed-on: https://chromium-review.googlesource.com/1111937Reviewed-by: Nico Weber <thakis@chromium.org>
Commit-Queue: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569590}

chromeos/components/proximity_auth/BUILD.gn

@@ -55,8 +55,6 @@ 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",
@@ -115,7 +113,6 @@ 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,7 +16,6 @@
 #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"
 
@@ -34,17 +33,13 @@ 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)
-    : remote_device_(remote_device),
-      channel_(channel),
-      connection_(connection),
-      pref_manager_(pref_manager),
+    : connection_(connection),
       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()) {
@@ -81,7 +76,7 @@ void ProximityMonitorImpl::RecordProximityMetricsOnAuthSuccess() {
                                     : metrics::kUnknownProximityValue;
 
   std::string remote_device_model = metrics::kUnknownDeviceModel;
-  cryptauth::RemoteDeviceRef remote_device = remote_device_;
+  cryptauth::RemoteDeviceRef remote_device = connection_->remote_device();
   if (!remote_device.name().empty())
     remote_device_model = remote_device.name();
 
@@ -136,69 +131,37 @@ bool ProximityMonitorImpl::ShouldPoll() const {
 void ProximityMonitorImpl::Poll() {
   DCHECK(ShouldPoll());
 
-  if (base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi)) {
-    if (channel_->is_disconnected()) {
-      PA_LOG(ERROR) << "Channel is disconnected.";
-      ClearProximityState();
-      return;
-    }
-
-    channel_->GetConnectionMetadata(
-        base::BindOnce(&ProximityMonitorImpl::OnGetConnectionMetadata,
-                       weak_ptr_factory_.GetWeakPtr()));
-  } else {
-    std::string address = connection_->GetDeviceAddress();
-    BluetoothDevice* device = bluetooth_adapter_->GetDevice(address);
+  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()));
+  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;
   }
-}
-
-void ProximityMonitorImpl::OnGetConnectionMetadata(
-    chromeos::secure_channel::mojom::ConnectionMetadataPtr
-        connection_metadata) {
-  DCHECK(base::FeatureList::IsEnabled(chromeos::features::kMultiDeviceApi));
 
-  if (connection_metadata->bluetooth_connection_metadata)
-    OnGetRssi(connection_metadata->bluetooth_connection_metadata->current_rssi);
-  else
-    OnGetRssi(base::nullopt);
+  device->GetConnectionInfo(base::Bind(&ProximityMonitorImpl::OnConnectionInfo,
+                                       weak_ptr_factory_.GetWeakPtr()));
 }
 
 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) << "Received RSSI after stopping.";
+    PA_LOG(INFO) << "[Proximity] Got connection info after stopping";
     return;
   }
 
-  if (rssi) {
-    AddSample(*rssi);
+  if (connection_info.rssi != BluetoothDevice::kUnknownPower) {
+    AddSample(connection_info);
   } else {
-    PA_LOG(WARNING) << "Received invalid RSSI value.";
+    PA_LOG(WARNING) << "[Proximity] Unknown values received from API: "
+                    << connection_info.rssi;
     rssi_rolling_average_.reset();
     CheckForProximityStateChange();
   }
@@ -214,13 +177,14 @@ void ProximityMonitorImpl::ClearProximityState() {
   rssi_rolling_average_.reset();
 }
 
-void ProximityMonitorImpl::AddSample(int32_t rssi) {
+void ProximityMonitorImpl::AddSample(
+    const BluetoothDevice::ConnectionInfo& connection_info) {
   double weight = kRssiSampleWeight;
   if (!rssi_rolling_average_) {
-    rssi_rolling_average_.reset(new double(rssi));
+    rssi_rolling_average_.reset(new double(connection_info.rssi));
   } else {
     *rssi_rolling_average_ =
-        weight * rssi + (1 - weight) * (*rssi_rolling_average_);
+        weight * connection_info.rssi + (1 - weight) * (*rssi_rolling_average_);
   }
 
   CheckForProximityStateChange();

chromeos/components/proximity_auth/proximity_monitor_impl.h

@@ -11,23 +11,14 @@
 #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 {
 
@@ -38,9 +29,7 @@ class ProximityMonitorObserver;
 class ProximityMonitorImpl : public ProximityMonitor {
  public:
   // The |connection| is not owned, and must outlive |this| instance.
-  ProximityMonitorImpl(cryptauth::RemoteDeviceRef remote_device,
-                       chromeos::secure_channel::ClientChannel* channel,
-                       cryptauth::Connection* connection,
+  ProximityMonitorImpl(cryptauth::Connection* connection,
                        ProximityAuthPrefManager* pref_manager);
   ~ProximityMonitorImpl() override;
 
@@ -73,19 +62,18 @@ class ProximityMonitorImpl : public ProximityMonitor {
   // Polls the connection information.
   void Poll();
 
-  void OnGetConnectionMetadata(
-      chromeos::secure_channel::mojom::ConnectionMetadataPtr
-          connection_metadata);
+  // Callback to received the polled-for connection info.
   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 sample of the current RSSI.
-  void AddSample(int32_t rssi);
+  // Updates the proximity state with a new |connection_info| sample of the
+  // current RSSI.
+  void AddSample(
+      const device::BluetoothDevice::ConnectionInfo& connection_info);
 
   // Checks whether the proximity state has changed based on the current
   // samples. Notifies |observers_| on a change.
@@ -95,19 +83,10 @@ class ProximityMonitorImpl : public ProximityMonitor {
   // RSSI value.
   void GetRssiThresholdFromPrefs();
 
-  // 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_;
+  // The current connection being monitored. Not owned and must outlive this
+  // instance.
   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_;
 
@@ -131,6 +110,10 @@ 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,10 +324,7 @@ void UnlockManagerImpl::OnAuthAttempted(mojom::AuthType auth_type) {
 std::unique_ptr<ProximityMonitor> UnlockManagerImpl::CreateProximityMonitor(
     cryptauth::Connection* connection,
     ProximityAuthPrefManager* pref_manager) {
-  // TODO(crbug.com/752273): Inject a real ClientChannel.
-  return std::make_unique<ProximityMonitorImpl>(connection->remote_device(),
-                                                nullptr /* channel */,
-                                                connection, pref_manager);
+  return std::make_unique<ProximityMonitorImpl>(connection, pref_manager);
 }
 
 void UnlockManagerImpl::SendSignInChallenge() {

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

@@ -14,16 +14,9 @@ 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) {
-  get_connection_metadata_callback_queue_.push(std::move(callback));
+  std::move(callback).Run(std::move(connection_metadata_for_next_call_));
 }
 
 void FakeClientChannel::PerformSendMessage(const std::string& payload,

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

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