[Fast Pair] Remove BLE address mapping, set V1 classic address post-pair

Removes the BLE address mapping from the DeviceAddressMap, which allowed
images from devices to never be evicted from prefs. Now, devices are
only mapped by their classic address, which is stable. V1 devices never
set their classic addresses before, so sets the classic address after
pairing completes (V2 devices set classic address mid-pair during
handshake).

Test: Unit tests and tested manually with V1 and V2 devices.
Bug: b:235117226
Fixed: b:268674526
Change-Id: I7f0f257fc4211ffb2ef4a7b98691d27394eb68da
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4245660
Reviewed-by: Jack Shira <jackshira@google.com>
Reviewed-by: Juliet Lévesque <julietlevesque@google.com>
Commit-Queue: Daniel Classon <dclasson@google.com>
Cr-Commit-Position: refs/heads/main@{#1105352}

ash/quick_pair/common/device.h

@@ -44,7 +44,7 @@ class COMPONENT_EXPORT(QUICK_PAIR_COMMON) Device
     return classic_address_;
   }
 
-  void set_classic_address(const std::string& address) {
+  void set_classic_address(const absl::optional<std::string>& address) {
     classic_address_ = address;
   }
 

ash/quick_pair/keyed_service/quick_pair_mediator.cc

@@ -261,6 +261,14 @@ void Mediator::OnRetroactivePairFound(scoped_refptr<Device> device) {
   }
   device_currently_showing_notification_ = device;
   ui_broker_->ShowAssociateAccount(device);
+
+  // Try saving mac address to model ID mapping one more time.
+  // TODO(b/235117226): we aren't really fetching device images here,
+  // since the images are already saved. We just want to save the mapping
+  // from mac address to model ID, and for Retroactive Pair this is one
+  // of the first times we have mac address and model ID for a paired device.
+  fast_pair_repository_->FetchDeviceImages(device);
+  fast_pair_repository_->PersistDeviceImages(device);
 }
 
 void Mediator::SetFastPairState(bool is_enabled) {
@@ -290,6 +298,13 @@ void Mediator::OnDevicePaired(scoped_refptr<Device> device) {
   ui_broker_->RemoveNotifications();
   device_currently_showing_notification_ = nullptr;
   scanner_broker_->OnDevicePaired(device);
+
+  // Try saving mac address to model ID mapping one more time.
+  // TODO(b/235117226): we aren't really fetching device images here,
+  // since the images are already saved. We just want to save the mapping
+  // from mac address to model ID, and for Initial/Subsequent Pair this is one
+  // of the first times we have mac address and model ID for a paired device.
+  fast_pair_repository_->FetchDeviceImages(device);
   fast_pair_repository_->PersistDeviceImages(device);
 }
 

ash/quick_pair/keyed_service/quick_pair_mediator_unittest.cc

@@ -866,5 +866,43 @@ TEST_F(MediatorTest, DiscoveryBan_RetroactiveAvoidsBan) {
       retroactive_device_);
 }
 
+TEST_F(MediatorTest, PersistsDeviceImages_AfterRetroactivePairFound) {
+  feature_status_tracker_->SetIsFastPairEnabled(true);
+
+  // We should save mac address to model ID mapping and persist images
+  // once Retroactive Pair is found--in other words, a device was just
+  // classic paired and we have images for that device we want to
+  // display in Bluetooth Settings, even if the user is offline/logged
+  // out/etc.
+  EXPECT_CALL(*mock_fast_pair_repository_, FetchDeviceImages).Times(1);
+  EXPECT_CALL(*mock_fast_pair_repository_, PersistDeviceImages).Times(1);
+  fake_retroactive_pairing_detector_->NotifyRetroactivePairFound(
+      retroactive_device_);
+}
+
+TEST_F(MediatorTest, PersistsDeviceImages_AfterDeviceInitialPaired) {
+  feature_status_tracker_->SetIsFastPairEnabled(true);
+
+  // We should save mac address to model ID mapping and persist images
+  // once a device is paired. We have images for the paired device we want to
+  // display in Bluetooth Settings, even if the user is offline/logged
+  // out/etc.
+  EXPECT_CALL(*mock_fast_pair_repository_, FetchDeviceImages).Times(1);
+  EXPECT_CALL(*mock_fast_pair_repository_, PersistDeviceImages).Times(1);
+  mock_pairer_broker_->NotifyDevicePaired(initial_device_);
+}
+
+TEST_F(MediatorTest, PersistsDeviceImages_AfterDeviceSubsequentPaired) {
+  feature_status_tracker_->SetIsFastPairEnabled(true);
+
+  // We should save mac address to model ID mapping and persist images
+  // once a device is paired. We have images for the paired device we want to
+  // display in Bluetooth Settings, even if the user is offline/logged
+  // out/etc.
+  EXPECT_CALL(*mock_fast_pair_repository_, FetchDeviceImages).Times(1);
+  EXPECT_CALL(*mock_fast_pair_repository_, PersistDeviceImages).Times(1);
+  mock_pairer_broker_->NotifyDevicePaired(subsequent_device_);
+}
+
 }  // namespace quick_pair
 }  // namespace ash

ash/quick_pair/pairing/fast_pair/fast_pair_pairer_impl.cc

@@ -266,9 +266,6 @@ void FastPairPairerImpl::OnConnectDevice(device::BluetoothDevice* device) {
   RecordProtocolPairingStep(FastPairProtocolPairingSteps::kDeviceConnected,
                             *device_);
   RecordConnectDeviceResult(/*success=*/true);
-  // The device ID can change between device discovery and connection, so
-  // ensure that device images are mapped to the current device ID.
-  FastPairRepository::Get()->FetchDeviceImages(device_);
 }
 
 void FastPairPairerImpl::OnConnectError(const std::string& error_message) {
@@ -662,13 +659,25 @@ void FastPairPairerImpl::DevicePairedChanged(device::BluetoothAdapter* adapter,
     return;
   }
 
-  if (device->GetAddress() == device_->ble_address() ||
-      device->GetAddress() == device_->classic_address()) {
+  if ((device_->classic_address().has_value() &&
+       device->GetAddress() == device_->classic_address().value()) ||
+      device->GetAddress() == device_->ble_address()) {
     QP_LOG(INFO) << __func__ << ": Completing pairing procedure " << device_;
 
     RecordProtocolPairingStep(FastPairProtocolPairingSteps::kPairingComplete,
                               *device_);
 
+    // V1 devices do not set the classic_address() field anywhere else, which is
+    // needed to map device addresses to persisted device images. Set the
+    // classic address here, which has to happen before paired_callback_ is
+    // fired. V2 devices can also set a missing classic address here, although
+    // that is not expected to happen.
+    if (!device_->classic_address() &&
+        device->GetAddressType() ==
+            device::BluetoothDevice::AddressType::ADDR_TYPE_PUBLIC) {
+      device_->set_classic_address(device->GetAddress());
+    }
+
     std::move(paired_callback_).Run(device_);
 
     // For V2 devices we still need to remove the Pairing Delegate and write the
@@ -784,10 +793,6 @@ void FastPairPairerImpl::OnConnected(
   RecordProtocolPairingStep(FastPairProtocolPairingSteps::kDeviceConnected,
                             *device_);
 
-  // The device ID can change between device discovery and connection, so
-  // ensure that device images are mapped to the current device ID.
-  FastPairRepository::Get()->FetchDeviceImages(device_);
-
   QP_LOG(INFO) << __func__ << ": starting account key write for `Pair` flow";
   adapter_->RemovePairingDelegate(this);
   AttemptSendAccountKey();

ash/quick_pair/pairing/fast_pair/fast_pair_pairer_impl_unittest.cc

@@ -157,6 +157,12 @@ class FakeBluetoothDevice
     std::move(callback).Run(absl::nullopt);
   }
 
+  // This method is called in DevicePairedChanged to ensure we are setting the
+  // classic address only if the device's address has the correct type (public).
+  device::BluetoothDevice::AddressType GetAddressType() const override {
+    return device::BluetoothDevice::AddressType::ADDR_TYPE_PUBLIC;
+  }
+
   void SetPairFailure() { pair_failure_ = true; }
 
   void SetPairTimeout() { pair_timeout_ = true; }
@@ -250,6 +256,8 @@ class FastPairPairerImplTest : public AshTestBase {
   void CreateMockDevice(DeviceFastPairVersion version, Protocol protocol) {
     device_ = base::MakeRefCounted<Device>(
         kMetadataId, kBluetoothCanonicalizedAddress, protocol);
+    // TODO(b/268722180): classic address should be set in the handshake, which
+    // is only for V2 devices. Should refactor that for unit tests.
     device_->set_classic_address(kBluetoothCanonicalizedAddress);
 
     device_->set_version(version);
@@ -2061,6 +2069,22 @@ TEST_F(FastPairPairerImplTest, FastPairVersionOne_DevicePaired) {
             1);
 }
 
+TEST_F(FastPairPairerImplTest,
+       FastPairVersionOne_SetsClassicAddressAfterPairing) {
+  Login(user_manager::UserType::USER_TYPE_REGULAR);
+  CreateDevice(DeviceFastPairVersion::kV1);
+  // V1 devices don't have classic addresses set during handshake.
+  device_->set_classic_address(absl::nullopt);
+  EXPECT_CALL(paired_callback_, Run);
+  EXPECT_CALL(pairing_procedure_complete_, Run);
+  EXPECT_EQ(DeviceFastPairVersion::kV1, device_->version().value());
+  DevicePaired();
+
+  // After pairing, classic address should be set.
+  EXPECT_TRUE(device_->classic_address());
+  EXPECT_EQ(device_->classic_address(), kBluetoothCanonicalizedAddress);
+}
+
 TEST_F(FastPairPairerImplTest, FastPairVersionOne_DeviceUnpaired) {
   Login(user_manager::UserType::USER_TYPE_REGULAR);
 

ash/quick_pair/repository/fast_pair/device_address_map.cc

@@ -26,17 +26,8 @@ DeviceAddressMap::DeviceAddressMap() = default;
 DeviceAddressMap::~DeviceAddressMap() = default;
 
 bool DeviceAddressMap::SaveModelIdForDevice(scoped_refptr<Device> device) {
-  // In some cases, BLE and classic address can map to different devices (with
-  // the same model ID) so we want to capture both mac address -> model ID
-  // records.
-  bool did_save = false;
-  if (!device->ble_address().empty()) {
-    did_save = true;
-    mac_address_to_model_id_[device->ble_address()] = device->metadata_id();
-  }
-
   if (!device->classic_address()) {
-    return did_save;
+    return false;
   }
 
   mac_address_to_model_id_[device->classic_address().value()] =
@@ -45,21 +36,11 @@ bool DeviceAddressMap::SaveModelIdForDevice(scoped_refptr<Device> device) {
 }
 
 bool DeviceAddressMap::PersistRecordsForDevice(scoped_refptr<Device> device) {
-  // TODO(235117226): See if we really need to persist for both addresses.
-  // In some cases, BLE and classic address can map to different devices (with
-  // the same model ID) so we want to capture both mac address -> model ID
-  // records.
-  bool did_persist = false;
-  if (!device->ble_address().empty()) {
-    did_persist = PersistMacAddressRecord(device->ble_address());
-  }
-
   if (!device->classic_address()) {
-    return did_persist;
+    return false;
   }
 
-  return PersistMacAddressRecord(device->classic_address().value()) ||
-         did_persist;
+  return PersistMacAddressRecord(device->classic_address().value());
 }
 
 bool DeviceAddressMap::PersistMacAddressRecord(const std::string& mac_address) {

ash/quick_pair/repository/fast_pair/device_address_map.h

@@ -57,19 +57,25 @@ class DeviceAddressMap {
   // local state prefs, false otherwise.
   bool HasPersistedRecordsForModelId(const std::string& model_id);
 
-  // Clears the in-memory map and reloads from prefs.
-  void RefreshCacheForTest();
-
  private:
+  FRIEND_TEST_ALL_PREFIXES(FastPairRepositoryImplTest, PersistDeviceImages);
+  FRIEND_TEST_ALL_PREFIXES(FastPairRepositoryImplTest,
+                           PersistDeviceImagesNoMacAddress);
+  FRIEND_TEST_ALL_PREFIXES(FastPairRepositoryImplTest, EvictDeviceImages);
+
   // Persists the |mac_address| -> model ID record in mac_address_to_model_id_
   // to local state prefs. Returns true if the record was persisted, false
   // if no record exists for |mac_address| or there was an error when
   // persisting.
   bool PersistMacAddressRecord(const std::string& mac_address);
+
   // Loads mac address -> model ID records persisted in prefs to
   // mac_address_to_model_id_.
   void LoadPersistedRecordsFromPrefs();
 
+  // Clears the in-memory map and reloads from prefs. Used by tests.
+  void RefreshCacheForTest();
+
   // Used to lazily load saved records from prefs.
   bool loaded_records_from_prefs_ = false;
   base::flat_map<std::string, std::string> mac_address_to_model_id_;