Skip to content

Commit

Permalink
[Fast Pair] Don't create Gatt connection if already paired to Chromebook
Browse files Browse the repository at this point in the history
Exposes API in FastPairRepository to check the SavedDeviceRegistry for
an account key, and if the account key exists already, it implies that
the device has already been paired with this Chromebook. Uses this API
in the NotDiscoverableScanner to not create a Gatt connection to a
device if it has already been paired.

Test: verified on DUT
Change-Id: Iaf6cfc640ee5a7cdce49c2421e4bcc6444c51fa4
Fixed: b/227209345
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3557556
Reviewed-by: Shane Fitzpatrick <shanefitz@google.com>
Commit-Queue: Juliet Lévesque <julietlevesque@google.com>
Cr-Commit-Position: refs/heads/main@{#987196}
  • Loading branch information
julietlevesque authored and Chromium LUCI CQ committed Mar 30, 2022
1 parent 2af4ef9 commit da109a2
Show file tree
Hide file tree
Showing 12 changed files with 151 additions and 5 deletions.
9 changes: 7 additions & 2 deletions ash/quick_pair/repository/fake_fast_pair_repository.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ void FakeFastPairRepository::ClearFakeMetadata(

void FakeFastPairRepository::SetCheckAccountKeysResult(
absl::optional<PairingMetadata> result) {
check_account_key_result_ = result;
check_account_keys_result_ = result;
}

bool FakeFastPairRepository::HasKeyForDevice(const std::string& mac_address) {
Expand Down Expand Up @@ -61,7 +61,7 @@ void FakeFastPairRepository::GetDeviceMetadata(
void FakeFastPairRepository::CheckAccountKeys(
const AccountKeyFilter& account_key_filter,
CheckAccountKeysCallback callback) {
std::move(callback).Run(check_account_key_result_);
std::move(callback).Run(check_account_keys_result_);
}

void FakeFastPairRepository::AssociateAccountKey(
Expand Down Expand Up @@ -101,6 +101,11 @@ void FakeFastPairRepository::FetchDeviceImages(scoped_refptr<Device> device) {
return;
}

bool FakeFastPairRepository::IsAccountKeyPairedLocally(
const std::vector<uint8_t>& account_key) {
return is_account_key_paired_locally_;
}

// Unimplemented.
bool FakeFastPairRepository::PersistDeviceImages(scoped_refptr<Device> device) {
return true;
Expand Down
9 changes: 8 additions & 1 deletion ash/quick_pair/repository/fake_fast_pair_repository.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ class FakeFastPairRepository : public FastPairRepository {

void SetCheckAccountKeysResult(absl::optional<PairingMetadata> result);

void set_is_account_key_paired_locally(bool is_account_key_paired_locally) {
is_account_key_paired_locally_ = is_account_key_paired_locally;
}

bool HasKeyForDevice(const std::string& mac_address);

void set_is_network_connected(bool is_connected) {
Expand Down Expand Up @@ -71,16 +75,19 @@ class FakeFastPairRepository : public FastPairRepository {
const std::vector<uint8_t>& account_key,
DeleteAssociatedDeviceByAccountKeyCallback callback) override;
void GetSavedDevices(GetSavedDevicesCallback callback) override;
bool IsAccountKeyPairedLocally(
const std::vector<uint8_t>& account_key) override;

private:
static void SetInstance(FastPairRepository* instance);

nearby::fastpair::OptInStatus status_ =
nearby::fastpair::OptInStatus::STATUS_UNKNOWN;
bool is_network_connected_ = true;
bool is_account_key_paired_locally_ = true;
base::flat_map<std::string, std::unique_ptr<DeviceMetadata>> data_;
base::flat_map<std::string, std::vector<uint8_t>> saved_account_keys_;
absl::optional<PairingMetadata> check_account_key_result_;
absl::optional<PairingMetadata> check_account_keys_result_;
base::WeakPtrFactory<FakeFastPairRepository> weak_ptr_factory_{this};
};

Expand Down
28 changes: 28 additions & 0 deletions ash/quick_pair/repository/fast_pair/saved_device_registry.cc
Original file line number Diff line number Diff line change
Expand Up @@ -66,5 +66,33 @@ absl::optional<const std::vector<uint8_t>> SavedDeviceRegistry::GetAccountKey(
return std::vector<uint8_t>(decoded.begin(), decoded.end());
}

bool SavedDeviceRegistry::IsAccountKeySavedToRegistry(
const std::vector<uint8_t>& account_key) {
PrefService* pref_service =
QuickPairBrowserDelegate::Get()->GetActivePrefService();
if (!pref_service) {
QP_LOG(WARNING) << __func__ << ": No user pref service available.";
return false;
}

const base::Value* saved_devices =
pref_service->GetDictionary(kFastPairSavedDevicesPref);
if (!saved_devices) {
QP_LOG(WARNING) << __func__
<< ": No Fast Pair Saved Devices pref available.";
return false;
}

std::string encoded_key = base::Base64Encode(account_key);
for (const auto it : saved_devices->DictItems()) {
const std::string* value = it.second.GetIfString();
if (value && *value == encoded_key) {
return true;
}
}

return false;
}

} // namespace quick_pair
} // namespace ash
3 changes: 3 additions & 0 deletions ash/quick_pair/repository/fast_pair/saved_device_registry.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ class SavedDeviceRegistry {
// empty vector.
absl::optional<const std::vector<uint8_t>> GetAccountKey(
const std::string& mac_address);

// Checks if the account key is in the registry.
bool IsAccountKeySavedToRegistry(const std::vector<uint8_t>& account_key);
};

} // namespace quick_pair
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
#include "ash/quick_pair/repository/fast_pair/saved_device_registry.h"

#include "ash/quick_pair/common/mock_quick_pair_browser_delegate.h"
#include "components/prefs/pref_registry_simple.h"
#include "components/prefs/pref_service.h"
#include "components/prefs/scoped_user_pref_update.h"
#include "components/prefs/testing_pref_service.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
Expand Down Expand Up @@ -54,6 +57,11 @@ TEST_F(SavedDeviceRegistryTest, ValidLookup) {

ASSERT_EQ(kAccountKey1, *first);
ASSERT_EQ(kAccountKey2, *second);

EXPECT_TRUE(
saved_device_registry_->IsAccountKeySavedToRegistry(kAccountKey1));
EXPECT_TRUE(
saved_device_registry_->IsAccountKeySavedToRegistry(kAccountKey2));
}

TEST_F(SavedDeviceRegistryTest, InvalidLookup) {
Expand All @@ -62,6 +70,29 @@ TEST_F(SavedDeviceRegistryTest, InvalidLookup) {
auto invalid_result =
saved_device_registry_->GetAccountKey(kNotSavedMacAddress);
ASSERT_EQ(absl::nullopt, invalid_result);

EXPECT_TRUE(
saved_device_registry_->IsAccountKeySavedToRegistry(kAccountKey1));
EXPECT_FALSE(
saved_device_registry_->IsAccountKeySavedToRegistry(kAccountKey2));
}

TEST_F(SavedDeviceRegistryTest, MissingPrefService) {
ON_CALL(*browser_delegate_, GetActivePrefService())
.WillByDefault(testing::Return(nullptr));
saved_device_registry_->SaveAccountKey(kFirstSavedMacAddress, kAccountKey1);
saved_device_registry_->SaveAccountKey(kSecondSavedMacAddress, kAccountKey2);

auto first = saved_device_registry_->GetAccountKey(kFirstSavedMacAddress);
auto second = saved_device_registry_->GetAccountKey(kSecondSavedMacAddress);

ASSERT_EQ(absl::nullopt, first);
ASSERT_EQ(absl::nullopt, second);

EXPECT_FALSE(
saved_device_registry_->IsAccountKeySavedToRegistry(kAccountKey1));
EXPECT_FALSE(
saved_device_registry_->IsAccountKeySavedToRegistry(kAccountKey2));
}

} // namespace quick_pair
Expand Down
5 changes: 5 additions & 0 deletions ash/quick_pair/repository/fast_pair_repository.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ class FastPairRepository {
virtual void CheckAccountKeys(const AccountKeyFilter& account_key_filter,
CheckAccountKeysCallback callback) = 0;

// Checks account keys saved to the device registry for a match to
// |account_key|. Return true if a match is found and false otherwise.
virtual bool IsAccountKeyPairedLocally(
const std::vector<uint8_t>& account_key) = 0;

// Stores the given |account_key| for a |device| on the server.
virtual void AssociateAccountKey(scoped_refptr<Device> device,
const std::vector<uint8_t>& account_key) = 0;
Expand Down
8 changes: 6 additions & 2 deletions ash/quick_pair/repository/fast_pair_repository_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,11 @@ void FastPairRepositoryImpl::OnImageDecoded(
/*has_retryable_error=*/false);
}

bool FastPairRepositoryImpl::IsAccountKeyPairedLocally(
const std::vector<uint8_t>& account_key) {
return saved_device_registry_->IsAccountKeySavedToRegistry(account_key);
}

void FastPairRepositoryImpl::CheckAccountKeys(
const AccountKeyFilter& account_key_filter,
CheckAccountKeysCallback callback) {
Expand Down Expand Up @@ -325,9 +330,8 @@ bool FastPairRepositoryImpl::DeleteAssociatedDevice(
const device::BluetoothDevice* device) {
absl::optional<const std::vector<uint8_t>> account_key =
saved_device_registry_->GetAccountKey(device->GetAddress());
if (!account_key) {
if (!account_key)
return false;
}

QP_LOG(INFO) << __func__ << ": Removing device from Footprints.";
footprints_fetcher_->DeleteUserDevice(base::HexEncode(*account_key),
Expand Down
2 changes: 2 additions & 0 deletions ash/quick_pair/repository/fast_pair_repository_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ class FastPairRepositoryImpl : public FastPairRepository {
DeviceMetadataCallback callback) override;
void CheckAccountKeys(const AccountKeyFilter& account_key_filter,
CheckAccountKeysCallback callback) override;
bool IsAccountKeyPairedLocally(
const std::vector<uint8_t>& account_key) override;
void AssociateAccountKey(scoped_refptr<Device> device,
const std::vector<uint8_t>& account_key) override;
bool DeleteAssociatedDevice(const device::BluetoothDevice* device) override;
Expand Down
10 changes: 10 additions & 0 deletions ash/quick_pair/repository/fast_pair_repository_impl_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,13 @@ constexpr char kTestModelId[] = "test_model_id";
constexpr char kTestDeviceId[] = "test_ble_device_id";
constexpr char kTestBLEAddress[] = "test_ble_address";
constexpr char kTestClassicAddress[] = "test_classic_address";
constexpr char kFirstSavedMacAddress[] = "00:11:22:33:44";
const std::vector<uint8_t> kAccountKey1{0x11, 0x22, 0x33, 0x44, 0x55, 0x66,
0x77, 0x88, 0x99, 0x00, 0xAA, 0xBB,
0xCC, 0xDD, 0xEE, 0xFF};
const std::vector<uint8_t> kAccountKey2{0x11, 0x11, 0x22, 0x22, 0x33, 0x33,
0x44, 0x44, 0x55, 0x55, 0x66, 0x66,
0x77, 0x77, 0x88, 0x88};
const std::vector<uint8_t> kFilterBytes1{0x0A, 0x42, 0x88, 0x10};
const uint8_t salt = 0xC7;

Expand Down Expand Up @@ -512,5 +516,11 @@ TEST_F(FastPairRepositoryImplTest, GetSavedDevices_MissingResponse) {
EXPECT_EQ(0u, devices_.size());
}

TEST_F(FastPairRepositoryImplTest, IsAccountKeyPairedLocally) {
saved_device_registry_->SaveAccountKey(kFirstSavedMacAddress, kAccountKey1);
EXPECT_TRUE(fast_pair_repository_->IsAccountKeyPairedLocally(kAccountKey1));
EXPECT_FALSE(fast_pair_repository_->IsAccountKeyPairedLocally(kAccountKey2));
}

} // namespace quick_pair
} // namespace ash
4 changes: 4 additions & 0 deletions ash/quick_pair/repository/mock_fast_pair_repository.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ class MockFastPairRepository : public FastPairRepository {
GetSavedDevices,
(GetSavedDevicesCallback callback),
(override));
MOCK_METHOD(bool,
IsAccountKeyPairedLocally,
(const std::vector<uint8_t>& account_key),
(override));
};

} // namespace quick_pair
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,13 @@ void FastPairNotDiscoverableScannerImpl::OnAccountKeyFilterCheckResult(
if (!metadata || !metadata->device_metadata)
return;

if (FastPairRepository::Get()->IsAccountKeyPairedLocally(
metadata->account_key)) {
QP_LOG(INFO) << __func__
<< ": device already paired and saved to this Chromebook";
return;
}

auto& details = metadata->device_metadata->GetDetails();

// Convert the integer model id to uppercase hex string.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@ TEST_F(FastPairNotDiscoverableScannerImplTest,
PairingMetadata pairing_metadata(device_metadata.get(),
std::vector<uint8_t>());
repository_->SetCheckAccountKeysResult(pairing_metadata);
repository_->set_is_account_key_paired_locally(false);

EXPECT_CALL(found_device_callback_, Run).Times(0);
EXPECT_CALL(*process_manager_, GetProcessReference)
Expand Down Expand Up @@ -273,6 +274,7 @@ TEST_F(FastPairNotDiscoverableScannerImplTest,
PairingMetadata pairing_metadata(device_metadata.get(),
std::vector<uint8_t>());
repository_->SetCheckAccountKeysResult(pairing_metadata);
repository_->set_is_account_key_paired_locally(false);

EXPECT_CALL(found_device_callback_, Run).Times(0);
EXPECT_CALL(*process_manager_, GetProcessReference)
Expand Down Expand Up @@ -319,6 +321,7 @@ TEST_F(FastPairNotDiscoverableScannerImplTest,
PairingMetadata pairing_metadata(device_metadata.get(),
std::vector<uint8_t>());
repository_->SetCheckAccountKeysResult(pairing_metadata);
repository_->set_is_account_key_paired_locally(false);

EXPECT_CALL(found_device_callback_, Run).Times(0);
EXPECT_CALL(*process_manager_, GetProcessReference)
Expand Down Expand Up @@ -394,6 +397,7 @@ TEST_F(FastPairNotDiscoverableScannerImplTest, DeviceLostDuringParsing) {
PairingMetadata pairing_metadata(device_metadata.get(),
std::vector<uint8_t>());
repository_->SetCheckAccountKeysResult(pairing_metadata);
repository_->set_is_account_key_paired_locally(false);
EXPECT_CALL(*process_manager_, GetProcessReference)
.WillRepeatedly(
[&](QuickPairProcessManager::ProcessStoppedCallback callback) {
Expand All @@ -419,6 +423,7 @@ TEST_F(FastPairNotDiscoverableScannerImplTest, NoModelId) {
PairingMetadata pairing_metadata(device_metadata.get(),
std::vector<uint8_t>());
repository_->SetCheckAccountKeysResult(pairing_metadata);
repository_->set_is_account_key_paired_locally(false);

EXPECT_CALL(found_device_callback_, Run).Times(0);
scanner_->NotifyDeviceFound(device);
Expand All @@ -438,6 +443,7 @@ TEST_F(FastPairNotDiscoverableScannerImplTest, InvokesLostCallbackAfterFound) {
PairingMetadata pairing_metadata(device_metadata.get(),
std::vector<uint8_t>());
repository_->SetCheckAccountKeysResult(pairing_metadata);
repository_->set_is_account_key_paired_locally(false);
EXPECT_CALL(*process_manager_, GetProcessReference)
.WillRepeatedly(
[&](QuickPairProcessManager::ProcessStoppedCallback callback) {
Expand All @@ -461,6 +467,34 @@ TEST_F(FastPairNotDiscoverableScannerImplTest, InvokesLostCallbackAfterFound) {
base::RunLoop().RunUntilIdle();
}

TEST_F(FastPairNotDiscoverableScannerImplTest, AlreadySavedToChromebook) {
device::BluetoothDevice* device = GetDevice(GetAdvServicedata());

nearby::fastpair::GetObservedDeviceResponse response;
response.mutable_device()->set_id(kModelIdLong);
response.mutable_device()->set_trigger_distance(2);

auto device_metadata =
std::make_unique<DeviceMetadata>(std::move(response), gfx::Image());
PairingMetadata pairing_metadata(device_metadata.get(),
std::vector<uint8_t>());
repository_->SetCheckAccountKeysResult(pairing_metadata);
repository_->set_is_account_key_paired_locally(true);
EXPECT_CALL(*process_manager_, GetProcessReference)
.WillRepeatedly(
[&](QuickPairProcessManager::ProcessStoppedCallback callback) {
return std::make_unique<
QuickPairProcessManagerImpl::ProcessReferenceImpl>(
data_parser_remote_, base::DoNothing());
});

EXPECT_CALL(found_device_callback_, Run).Times(0);
EXPECT_CALL(lost_device_callback_, Run).Times(0);
scanner_->NotifyDeviceFound(device);

base::RunLoop().RunUntilIdle();
}

TEST_F(FastPairNotDiscoverableScannerImplTest, FactoryCreate) {
not_discoverable_scanner_.reset();
std::unique_ptr<FastPairNotDiscoverableScanner>
Expand All @@ -478,6 +512,7 @@ TEST_F(FastPairNotDiscoverableScannerImplTest, FactoryCreate) {
PairingMetadata pairing_metadata(device_metadata.get(),
std::vector<uint8_t>());
repository_->SetCheckAccountKeysResult(pairing_metadata);
repository_->set_is_account_key_paired_locally(false);
EXPECT_CALL(*process_manager_, GetProcessReference)
.WillRepeatedly(
[&](QuickPairProcessManager::ProcessStoppedCallback callback) {
Expand Down Expand Up @@ -522,6 +557,7 @@ TEST_F(FastPairNotDiscoverableScannerImplTest, SetBatteryInfo) {
PairingMetadata pairing_metadata(device_metadata.get(),
std::vector<uint8_t>());
repository_->SetCheckAccountKeysResult(pairing_metadata);
repository_->set_is_account_key_paired_locally(false);

EXPECT_CALL(found_device_callback_, Run).Times(1);
EXPECT_CALL(*process_manager_, GetProcessReference)
Expand Down Expand Up @@ -562,6 +598,7 @@ TEST_F(FastPairNotDiscoverableScannerImplTest, SetUnknownBatteryInfo) {
PairingMetadata pairing_metadata(device_metadata.get(),
std::vector<uint8_t>());
repository_->SetCheckAccountKeysResult(pairing_metadata);
repository_->set_is_account_key_paired_locally(false);

EXPECT_CALL(found_device_callback_, Run).Times(1);
EXPECT_CALL(*process_manager_, GetProcessReference)
Expand Down Expand Up @@ -603,6 +640,7 @@ TEST_F(FastPairNotDiscoverableScannerImplTest, SetInvalidPercentBatteryInfo) {
PairingMetadata pairing_metadata(device_metadata.get(),
std::vector<uint8_t>());
repository_->SetCheckAccountKeysResult(pairing_metadata);
repository_->set_is_account_key_paired_locally(false);

EXPECT_CALL(found_device_callback_, Run).Times(1);
EXPECT_CALL(*process_manager_, GetProcessReference)
Expand Down Expand Up @@ -646,6 +684,7 @@ TEST_F(FastPairNotDiscoverableScannerImplTest, HandshakeFailed) {
PairingMetadata pairing_metadata(device_metadata.get(),
std::vector<uint8_t>());
repository_->SetCheckAccountKeysResult(pairing_metadata);
repository_->set_is_account_key_paired_locally(false);
EXPECT_CALL(*process_manager_, GetProcessReference)
.WillRepeatedly(
[&](QuickPairProcessManager::ProcessStoppedCallback callback) {
Expand Down Expand Up @@ -680,6 +719,7 @@ TEST_F(FastPairNotDiscoverableScannerImplTest, AlreadyPaired) {
PairingMetadata pairing_metadata(device_metadata.get(),
std::vector<uint8_t>());
repository_->SetCheckAccountKeysResult(pairing_metadata);
repository_->set_is_account_key_paired_locally(false);
EXPECT_CALL(*process_manager_, GetProcessReference)
.WillRepeatedly(
[&](QuickPairProcessManager::ProcessStoppedCallback callback) {
Expand Down

0 comments on commit da109a2

Please sign in to comment.