[CrOS MultiDevice] Update BleScanner.

This CL adds ConnectionRole as part of the scan filter passed to
BleScanner; then, when a scan result is returned, the delegate callback
is only invoked if a scan filter with the correct role is present. This
is necessary because BleConnectionManager should not attempt to handle
scan results for the incorrect role.

This CL also tweaks the Delegate's callback function to return a
ConnectionRole instead of a "is background" bool.

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

chromeos/services/secure_channel/ble_scanner.cc

@@ -16,7 +16,7 @@ BleScanner::BleScanner(Delegate* delegate) : delegate_(delegate) {}
 
 BleScanner::~BleScanner() = default;
 
-void BleScanner::AddScanFilter(const DeviceIdPair& scan_filter) {
+void BleScanner::AddScanFilter(const ScanFilter& scan_filter) {
   if (base::ContainsKey(scan_filters_, scan_filter)) {
     PA_LOG(ERROR) << "BleScanner::AddScanFilter(): Tried to add a scan filter "
                   << "which already existed. Filter: " << scan_filter;
@@ -27,7 +27,7 @@ void BleScanner::AddScanFilter(const DeviceIdPair& scan_filter) {
   HandleScanFilterChange();
 }
 
-void BleScanner::RemoveScanFilter(const DeviceIdPair& scan_filter) {
+void BleScanner::RemoveScanFilter(const ScanFilter& scan_filter) {
   if (!base::ContainsKey(scan_filters_, scan_filter)) {
     PA_LOG(ERROR) << "BleScanner::RemoveScanFilter(): Tried to remove a scan "
                   << "filter which was not present. Filter: " << scan_filter;
@@ -38,16 +38,30 @@ void BleScanner::RemoveScanFilter(const DeviceIdPair& scan_filter) {
   HandleScanFilterChange();
 }
 
-bool BleScanner::HasScanFilter(const DeviceIdPair& scan_filter) {
+bool BleScanner::HasScanFilter(const ScanFilter& scan_filter) {
   return base::ContainsKey(scan_filters_, scan_filter);
 }
 
+DeviceIdPairSet BleScanner::GetAllDeviceIdPairs() {
+  DeviceIdPairSet set;
+  for (const auto& scan_filter : scan_filters_)
+    set.insert(scan_filter.first);
+  return set;
+}
+
 void BleScanner::NotifyReceivedAdvertisementFromDevice(
     const cryptauth::RemoteDeviceRef& remote_device,
     device::BluetoothDevice* bluetooth_device,
-    bool is_background_advertisement) {
+    ConnectionRole connection_role) {
   delegate_->OnReceivedAdvertisement(remote_device, bluetooth_device,
-                                     is_background_advertisement);
+                                     connection_role);
+}
+
+std::ostream& operator<<(std::ostream& stream,
+                         const BleScanner::ScanFilter& scan_filter) {
+  stream << "{device_id_pair: " << scan_filter.first
+         << ", connection_role: " << scan_filter.second << "}";
+  return stream;
 }
 
 }  // namespace secure_channel

chromeos/services/secure_channel/ble_scanner.h

@@ -5,9 +5,12 @@
 #ifndef CHROMEOS_SERVICES_SECURE_CHANNEL_BLE_SCANNER_H_
 #define CHROMEOS_SERVICES_SECURE_CHANNEL_BLE_SCANNER_H_
 
-#include <unordered_set>
+#include <ostream>
+#include <utility>
 
+#include "base/containers/flat_set.h"
 #include "base/macros.h"
+#include "chromeos/services/secure_channel/connection_role.h"
 #include "chromeos/services/secure_channel/device_id_pair.h"
 #include "components/cryptauth/remote_device_ref.h"
 
@@ -29,22 +32,24 @@ class BleScanner {
     virtual void OnReceivedAdvertisement(
         cryptauth::RemoteDeviceRef remote_device,
         device::BluetoothDevice* bluetooth_device,
-        bool is_background_advertisement) = 0;
+        ConnectionRole connection_role) = 0;
   };
 
   virtual ~BleScanner();
 
-  // Adds a scan filter for the provided DeviceIdPair. If no scan filters were
+  using ScanFilter = std::pair<DeviceIdPair, ConnectionRole>;
+
+  // Adds a scan filter for the provided ScanFilter. If no scan filters were
   // previously present, adding a scan filter will start a BLE discovery session
   // and attempt to create a connection.
-  void AddScanFilter(const DeviceIdPair& scan_filter);
+  void AddScanFilter(const ScanFilter& scan_filter);
 
-  // Removes a scan filter for the provided DeviceIdPair. If this function
+  // Removes a scan filter for the provided ScanFilter. If this function
   // removes the only remaining filter, the ongoing BLE discovery session will
   // stop.
-  void RemoveScanFilter(const DeviceIdPair& scan_filter);
+  void RemoveScanFilter(const ScanFilter& scan_filter);
 
-  bool HasScanFilter(const DeviceIdPair& scan_filter);
+  bool HasScanFilter(const ScanFilter& scan_filter);
 
  protected:
   BleScanner(Delegate* delegate);
@@ -52,21 +57,25 @@ class BleScanner {
   virtual void HandleScanFilterChange() = 0;
 
   bool should_discovery_session_be_active() { return !scan_filters_.empty(); }
-  const DeviceIdPairSet& scan_filters() { return scan_filters_; }
+  const base::flat_set<ScanFilter>& scan_filters() { return scan_filters_; }
+  DeviceIdPairSet GetAllDeviceIdPairs();
 
   void NotifyReceivedAdvertisementFromDevice(
       const cryptauth::RemoteDeviceRef& remote_device,
       device::BluetoothDevice* bluetooth_device,
-      bool is_background_advertisement);
+      ConnectionRole connection_role);
 
  private:
   Delegate* delegate_;
 
-  DeviceIdPairSet scan_filters_;
+  base::flat_set<ScanFilter> scan_filters_;
 
   DISALLOW_COPY_AND_ASSIGN(BleScanner);
 };
 
+std::ostream& operator<<(std::ostream& stream,
+                         const BleScanner::ScanFilter& scan_filter);
+
 }  // namespace secure_channel
 
 }  // namespace chromeos

chromeos/services/secure_channel/ble_scanner_impl.cc

@@ -13,7 +13,6 @@
 #include "base/strings/string_util.h"
 #include "chromeos/components/proximity_auth/logging/logging.h"
 #include "chromeos/services/secure_channel/ble_constants.h"
-#include "chromeos/services/secure_channel/ble_service_data_helper.h"
 #include "chromeos/services/secure_channel/ble_synchronizer_base.h"
 #include "components/cryptauth/proto/cryptauth_api.pb.h"
 #include "components/cryptauth/remote_device_ref.h"
@@ -214,7 +213,7 @@ void BleScannerImpl::HandleDeviceUpdated(
   memcpy(string_contents_ptr, service_data->data(), service_data->size());
 
   auto potential_result = service_data_helper_->IdentifyRemoteDevice(
-      service_data_str, scan_filters());
+      service_data_str, GetAllDeviceIdPairs());
 
   // There was service data for the ProximityAuth UUID, but it did not apply to
   // any active scan filters. The advertisement was likely from a nearby device
@@ -222,21 +221,62 @@ void BleScannerImpl::HandleDeviceUpdated(
   if (!potential_result)
     return;
 
-  // Prepare a hex string of |service_data_str|.
+  HandlePotentialScanResult(service_data_str, *potential_result,
+                            bluetooth_device);
+}
+
+void BleScannerImpl::HandlePotentialScanResult(
+    const std::string& service_data,
+    const BleServiceDataHelper::DeviceWithBackgroundBool& potential_result,
+    device::BluetoothDevice* bluetooth_device) {
+  // Background advertisements correspond to the listener role; foreground
+  // advertisements correspond to the initiator role.
+  ConnectionRole connection_role = potential_result.second
+                                       ? ConnectionRole::kListenerRole
+                                       : ConnectionRole::kInitiatorRole;
+
+  // Check to see if a corresponding scan filter exists. At this point, it is
+  // possible that a scan result was received for the correct DeviceIdPair but
+  // incorrect ConnectionRole.
+  bool does_corresponding_scan_filter_exist = false;
+  for (const auto& scan_filter : scan_filters()) {
+    if (scan_filter.first.remote_device_id() !=
+        potential_result.first.GetDeviceId()) {
+      continue;
+    }
+
+    if (scan_filter.second == connection_role) {
+      does_corresponding_scan_filter_exist = true;
+      break;
+    }
+  }
+
+  // Prepare a hex string of |service_data|.
   std::stringstream ss;
   ss << "0x" << std::hex;
-  for (const auto& character : service_data_str)
+  for (const auto& character : service_data)
     ss << static_cast<uint32_t>(character);
 
+  if (!does_corresponding_scan_filter_exist) {
+    PA_LOG(WARNING) << "BleScannerImpl::HandleDeviceUpdated(): Received scan "
+                    << "result from device with ID \""
+                    << potential_result.first.GetTruncatedDeviceIdForLogs()
+                    << "\", but it did not correspond to an active scan "
+                    << "filter. Service data: " << ss.str()
+                    << ", Background advertisement: "
+                    << (potential_result.second ? "true" : "false");
+    return;
+  }
+
   PA_LOG(INFO) << "BleScannerImpl::HandleDeviceUpdated(): Received scan result "
                << "from device with ID \""
-               << potential_result->first.GetTruncatedDeviceIdForLogs() << "\""
+               << potential_result.first.GetTruncatedDeviceIdForLogs() << "\""
                << ". Service data: " << ss.str()
                << ", Background advertisement: "
-               << (potential_result->second ? "true" : "false");
+               << (potential_result.second ? "true" : "false");
 
-  NotifyReceivedAdvertisementFromDevice(
-      potential_result->first, bluetooth_device, potential_result->second);
+  NotifyReceivedAdvertisementFromDevice(potential_result.first,
+                                        bluetooth_device, connection_role);
 }
 
 void BleScannerImpl::SetServiceDataProviderForTesting(

chromeos/services/secure_channel/ble_scanner_impl.h

@@ -12,6 +12,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "chromeos/services/secure_channel/ble_scanner.h"
+#include "chromeos/services/secure_channel/ble_service_data_helper.h"
 #include "device/bluetooth/bluetooth_adapter.h"
 
 namespace device {
@@ -87,6 +88,10 @@ class BleScannerImpl : public BleScanner,
   void OnStopDiscoverySessionError();
 
   void HandleDeviceUpdated(device::BluetoothDevice* bluetooth_device);
+  void HandlePotentialScanResult(
+      const std::string& service_data,
+      const BleServiceDataHelper::DeviceWithBackgroundBool& potential_result,
+      device::BluetoothDevice* bluetooth_device);
 
   void SetServiceDataProviderForTesting(
       std::unique_ptr<ServiceDataProvider> service_data_provider);

chromeos/services/secure_channel/fake_ble_scanner.cc

@@ -12,11 +12,12 @@ FakeBleScanner::FakeBleScanner(Delegate* delegate) : BleScanner(delegate) {}
 
 FakeBleScanner::~FakeBleScanner() = default;
 
-std::vector<DeviceIdPair> FakeBleScanner::GetAllScanFiltersForRemoteDevice(
+std::vector<BleScanner::ScanFilter>
+FakeBleScanner::GetAllScanFiltersForRemoteDevice(
     const std::string& remote_device_id) {
-  std::vector<DeviceIdPair> all_scan_filters_for_remote_device;
+  std::vector<ScanFilter> all_scan_filters_for_remote_device;
   for (const auto& scan_filter : scan_filters()) {
-    if (scan_filter.remote_device_id() == remote_device_id)
+    if (scan_filter.first.remote_device_id() == remote_device_id)
       all_scan_filters_for_remote_device.push_back(scan_filter);
   }
   return all_scan_filters_for_remote_device;
@@ -33,9 +34,9 @@ FakeBleScannerDelegate::~FakeBleScannerDelegate() = default;
 void FakeBleScannerDelegate::OnReceivedAdvertisement(
     cryptauth::RemoteDeviceRef remote_device,
     device::BluetoothDevice* bluetooth_device,
-    bool is_background_advertisement) {
-  handled_scan_results_.push_back(std::make_tuple(
-      remote_device, bluetooth_device, is_background_advertisement));
+    ConnectionRole connection_role) {
+  handled_scan_results_.push_back(
+      std::make_tuple(remote_device, bluetooth_device, connection_role));
 }
 
 }  // namespace secure_channel

chromeos/services/secure_channel/fake_ble_scanner.h

@@ -26,7 +26,7 @@ class FakeBleScanner : public BleScanner {
     return num_scan_filter_changes_handled_;
   }
 
-  std::vector<DeviceIdPair> GetAllScanFiltersForRemoteDevice(
+  std::vector<ScanFilter> GetAllScanFiltersForRemoteDevice(
       const std::string& remote_device_id);
 
   // Public for testing.
@@ -47,8 +47,9 @@ class FakeBleScannerDelegate : public BleScanner::Delegate {
   FakeBleScannerDelegate();
   ~FakeBleScannerDelegate() override;
 
-  using ScannedResultList = std::vector<
-      std::tuple<cryptauth::RemoteDeviceRef, device::BluetoothDevice*, bool>>;
+  using ScannedResultList = std::vector<std::tuple<cryptauth::RemoteDeviceRef,
+                                                   device::BluetoothDevice*,
+                                                   ConnectionRole>>;
 
   const ScannedResultList& handled_scan_results() const {
     return handled_scan_results_;
@@ -57,7 +58,7 @@ class FakeBleScannerDelegate : public BleScanner::Delegate {
  private:
   void OnReceivedAdvertisement(cryptauth::RemoteDeviceRef remote_device,
                                device::BluetoothDevice* bluetooth_device,
-                               bool is_background_advertisement) override;
+                               ConnectionRole connection_role) override;
 
   ScannedResultList handled_scan_results_;