BugFix:fast_pair_handshake_impl_test

PiperOrigin-RevId: 548765785

fastpair/handshake/BUILD

@@ -137,9 +137,10 @@ cc_test(
     shard_count = 16,
     deps = [
         ":handshake",
+        ":test_support",
         "//fastpair/common",
+        "//fastpair/crypto",
         "//fastpair/proto:fastpair_cc_proto",
-        "//fastpair/repository:test_support",
         "//internal/platform:base",
         "//internal/platform:test_util",
         "//internal/platform:types",
@@ -163,6 +164,7 @@ cc_test(
         ":handshake",
         "//fastpair/common",
         "//fastpair/internal/mediums",
+        "//internal/platform:logging",
         "//internal/platform:test_util",
         "//internal/platform:types",
         "//internal/platform/implementation/g3",  # build_cleaner: keep

fastpair/handshake/fast_pair_gatt_service_client_impl.cc

@@ -124,19 +124,13 @@ void FastPairGattServiceClientImpl::InitializeGattConnection(
 
 void FastPairGattServiceClientImpl::AttemptGattConnection() {
   NEARBY_LOGS(INFO) << __func__ << ": Attempt to connect to the device.";
-
-  if (gatt_client_) {
-    NEARBY_LOGS(INFO) << __func__
-                      << ": Disconnecting previous connections before attempt";
-    gatt_client_->Stop();
-    // Destroying gatt client may block if something went wrong.
-    defunct_gatt_client_ = std::move(gatt_client_);
-  }
+  ClearCurrentState();
   CreateGattConnection();
 }
 
 void FastPairGattServiceClientImpl::CreateGattConnection() {
   NEARBY_LOGS(INFO) << __func__ << " : Create Gatt Connection to the device.";
+  MutexLock lock(&mutex_);
   if (mediums_.GetBluetoothRadio().Enable() &&
       mediums_.GetBleV2().IsAvailable()) {
     gatt_client_ = mediums_.GetBleV2().ConnectToGattServer(
@@ -253,7 +247,7 @@ void FastPairGattServiceClientImpl::WriteRequestAsync(
 void FastPairGattServiceClientImpl::WriteKeyBasedCharacteristic(
     absl::string_view request) {
   NEARBY_LOGS(INFO) << __func__ << " :Start to write keybased characteristic.";
-
+  MutexLock lock(&mutex_);
   gatt_client_->CallRemoteFunction(
       kKeyBasedCharacteristicIndex, request,
       [this](absl::StatusOr<absl::string_view> response) {
@@ -295,6 +289,7 @@ void FastPairGattServiceClientImpl::WritePasskeyAsync(
 
 void FastPairGattServiceClientImpl::WritePasskeyCharacteristic(
     absl::string_view request) {
+    MutexLock lock(&mutex_);
     gatt_client_->CallRemoteFunction(
         kPasskeyCharacteristicIndex, request,
         [this](absl::StatusOr<absl::string_view> response) {
@@ -318,31 +313,32 @@ void FastPairGattServiceClientImpl::WritePasskeyCharacteristic(
 void FastPairGattServiceClientImpl::WriteAccountKey(
     const FastPairDataEncryptor& fast_pair_data_encryptor,
     WriteAccountkeyCallback write_accountkey_callback) {
-  CHECK(is_initialized_);
-  account_key_write_callback_ = std::move(write_accountkey_callback);
-  std::array<uint8_t, kAesBlockByteSize> raw_account_key =
-      CreateAccountKeyBlock();
-  const std::array<uint8_t, kAesBlockByteSize> data_to_write =
-      fast_pair_data_encryptor.EncryptBytes(raw_account_key);
-  gatt_client_->WriteCharacteristic(
-      kAccountKeyCharacteristicIndex,
-      std::string(data_to_write.begin(), data_to_write.end()),
-      api::ble_v2::GattClient::WriteType::kWithResponse,
-      [this, account_key =
-                 std::string(raw_account_key.begin(), raw_account_key.end())](
-          absl::Status status) {
-        if (status.ok()) {
-          NEARBY_LOGS(INFO)
-              << __func__
-              << ": Successfully write the accoutkey characteristic.";
-          NotifyWriteAccountKeyResult(AccountKey(account_key));
-        } else {
-          NEARBY_LOGS(INFO)
-              << __func__ << ": Failed to write the passkey characteristic ";
-          NotifyWriteAccountKeyError(
-              PairFailure::kAccountKeyCharacteristicWrite);
-        }
-      });
+    MutexLock lock(&mutex_);
+    CHECK(is_initialized_);
+    account_key_write_callback_ = std::move(write_accountkey_callback);
+    std::array<uint8_t, kAesBlockByteSize> raw_account_key =
+        CreateAccountKeyBlock();
+    const std::array<uint8_t, kAesBlockByteSize> data_to_write =
+        fast_pair_data_encryptor.EncryptBytes(raw_account_key);
+    gatt_client_->WriteCharacteristic(
+        kAccountKeyCharacteristicIndex,
+        std::string(data_to_write.begin(), data_to_write.end()),
+        api::ble_v2::GattClient::WriteType::kWithResponse,
+        [this, account_key =
+                   std::string(raw_account_key.begin(), raw_account_key.end())](
+            absl::Status status) {
+          if (status.ok()) {
+            NEARBY_LOGS(INFO)
+                << __func__
+                << ": Successfully write the accoutkey characteristic.";
+            NotifyWriteAccountKeyResult(AccountKey(account_key));
+          } else {
+            NEARBY_LOGS(INFO)
+                << __func__ << ": Failed to write the passkey characteristic ";
+            NotifyWriteAccountKeyError(
+                PairFailure::kAccountKeyCharacteristicWrite);
+          }
+        });
 }
 
 void FastPairGattServiceClientImpl::NotifyInitializedError(
@@ -391,10 +387,14 @@ void FastPairGattServiceClientImpl::NotifyWriteAccountKeyResult(
 }
 
 void FastPairGattServiceClientImpl::ClearCurrentState() {
-  if (gatt_client_ != nullptr) {
-    gatt_client_->Stop();
-    defunct_gatt_client_ = std::move(gatt_client_);
-  }
+  MutexLock lock(&mutex_);
+  executor_->Execute("clear-current-state",
+                    [this, gatt_client = std::move(gatt_client_)]() mutable {
+                      if (gatt_client != nullptr) {
+                        gatt_client->Stop();
+                        defunct_gatt_client_ = std::move(gatt_client);
+                      }
+                    });
 }
 
 }  // namespace fastpair

fastpair/handshake/fast_pair_gatt_service_client_impl.h

@@ -27,6 +27,7 @@
 #include "fastpair/handshake/fast_pair_gatt_service_client.h"
 #include "fastpair/internal/mediums/mediums.h"
 #include "fastpair/internal/mediums/robust_gatt_client.h"
+#include "internal/platform/mutex.h"
 #include "internal/platform/single_thread_executor.h"
 
 namespace nearby {
@@ -148,9 +149,10 @@ class FastPairGattServiceClientImpl : public FastPairGattServiceClient {
   WriteResponseCallback passkey_write_response_callback_;
   WriteAccountkeyCallback account_key_write_callback_;
 
+  Mutex mutex_;
   bool is_initialized_ = false;
   std::string device_address_;
-  std::unique_ptr<RobustGattClient> gatt_client_;
+  std::unique_ptr<RobustGattClient> gatt_client_ ABSL_GUARDED_BY(mutex_);
   std::unique_ptr<RobustGattClient> defunct_gatt_client_;
   RobustGattClient::ConnectionParams gatt_connection_params_;
   Mediums& mediums_;

fastpair/handshake/fast_pair_handshake_impl_test.cc

@@ -14,25 +14,22 @@
 
 #include "fastpair/handshake/fast_pair_handshake_impl.h"
 
-#include <algorithm>
 #include <array>
 #include <memory>
 #include <optional>
 #include <string>
 #include <utility>
 
-#include "gmock/gmock.h"
-#include "protobuf-matchers/protocol-buffer-matchers.h"
 #include "gtest/gtest.h"
 #include "absl/functional/any_invocable.h"
 #include "absl/status/status.h"
-#include "absl/strings/escaping.h"
-#include "fastpair/common/constant.h"
-#include "fastpair/common/device_metadata.h"
 #include "fastpair/common/fast_pair_device.h"
 #include "fastpair/common/pair_failure.h"
 #include "fastpair/common/protocol.h"
-#include "fastpair/proto/fastpair_rpcs.proto.h"
+#include "fastpair/crypto/decrypted_response.h"
+#include "fastpair/handshake/fake_fast_pair_data_encryptor.h"
+#include "fastpair/handshake/fast_pair_data_encryptor.h"
+#include "fastpair/handshake/fast_pair_data_encryptor_impl.h"
 #include "internal/platform/byte_array.h"
 #include "internal/platform/count_down_latch.h"
 #include "internal/platform/medium_environment.h"
@@ -50,13 +47,6 @@ constexpr absl::string_view kMetadataId("718c17");
 constexpr absl::string_view kPublicAddress("5E:3F:45:61:C3:32");
 constexpr absl::string_view kKeyBasedResponse("keybasedresponse");
 constexpr absl::string_view kWrongResponse("wrongresponse");
-constexpr absl::string_view kPublicAntiSpoof =
-    "Wuyr48lD3txnUhGiMF1IfzlTwRxxe+wMB1HLzP+"
-    "0wVcljfT3XPoiy1fntlneziyLD5knDVAJSE+RM/zlPRP/Jg==";
-constexpr char kInvalidPublicAntiSpoof[] = "InvalidPublicAntiSpoof";
-constexpr std::array<uint8_t, kAesBlockByteSize> kRawResponseBytes = {
-    0x01, 0x5E, 0x3F, 0x45, 0x61, 0xC3, 0x32, 0x1D,
-    0xA0, 0xBA, 0xF0, 0xBB, 0x95, 0x1F, 0xF7, 0xB6};
 constexpr Uuid kFastPairServiceUuid(0x0000FE2C00001000, 0x800000805F9B34FB);
 constexpr Uuid kKeyBasedCharacteristicUuidV2(0xFE2C123483664814,
                                              0x8EB001DE32100BEA);
@@ -64,11 +54,11 @@ constexpr Uuid kPasskeyCharacteristicUuidV2(0xFE2C123583664814,
                                             0x8EB001DE32100BEA);
 constexpr Uuid kAccountKeyCharacteristicUuidV2(0xFE2C123683664814,
                                                0x8EB001DE32100BEA);
-// Length of advertisement byte should be 16
-constexpr absl::string_view kKeyBasedCharacteristicAdvertisementByte =
-    "keyBasedCharacte";
-constexpr absl::string_view kPasskeyharacteristicAdvertisementByte =
-    "passkeyCharacter";
+constexpr std::array<uint8_t, 6> address_bytes = {0x5E, 0x3F, 0x45,
+                                                  0x61, 0xC3, 0x32};
+
+constexpr std::array<uint8_t, 9> salt = {0x08, 0x09, 0x0A, 0x0B, 0x0C,
+                                         0x0D, 0x0E, 0x0F, 0x00};
 }  // namespace
 
 class MediumEnvironmentStarter {
@@ -77,13 +67,51 @@ class MediumEnvironmentStarter {
   ~MediumEnvironmentStarter() { MediumEnvironment::Instance().Stop(); }
 };
 
+class FastPairFakeDataEncryptorImplFactory
+    : public FastPairDataEncryptorImpl::Factory {
+ public:
+  void CreateInstance(
+      const FastPairDevice& device,
+      absl::AnyInvocable<void(std::unique_ptr<FastPairDataEncryptor>)>
+          on_get_instance_callback) override {
+    if (!successful_retrieval_) {
+      std::move(on_get_instance_callback)(nullptr);
+      return;
+    }
+
+    auto data_encryptor = std::make_unique<FakeFastPairDataEncryptor>();
+    data_encryptor_ = data_encryptor.get();
+    data_encryptor->SetResponse(response_);
+    std::move(on_get_instance_callback)(std::move(data_encryptor));
+  }
+
+  FakeFastPairDataEncryptor* data_encryptor() { return data_encryptor_; }
+
+  void SetFailedRetrieval() { successful_retrieval_ = false; }
+
+  void SetResponse(std::optional<DecryptedResponse> response) {
+    response_ = std::move(response);
+  }
+
+ private:
+  FakeFastPairDataEncryptor* data_encryptor_ = nullptr;
+  bool successful_retrieval_ = true;
+  std::optional<DecryptedResponse> response_;
+};
+
 struct CharacteristicData {
   // Write result returned to the gatt client.
   absl::Status write_result;
+  std::optional<std::string> notify_response;
 };
 
 class FastPairHandshakeImplTest : public testing::Test {
  public:
+  FastPairHandshakeImplTest() {
+    FastPairDataEncryptorImpl::Factory::SetFactoryForTesting(
+        &fake_data_encryptor_factory_);
+  }
+
   void TearDown() override {
     executor_.Shutdown();
     key_based_characteristic_ = std::nullopt;
@@ -93,33 +121,33 @@ class FastPairHandshakeImplTest : public testing::Test {
     gatt_server_.reset();
   }
 
-  void StartGattServer(
-      absl::AnyInvocable<void()> trigger_keybase_value_change) {
+  void StartGattServer() {
     gatt_server_ =
         provider_ble_.StartGattServer(/*ServerGattConnectionCallback=*/{
             .on_characteristic_write_cb =
-                [&, trigger_keybase_value_change =
-                        std::move(trigger_keybase_value_change)](
-                    const api::ble_v2::BlePeripheral& remote_device,
+                [&](const api::ble_v2::BlePeripheral& remote_device,
                     const api::ble_v2::GattCharacteristic& characteristic,
                     int offset, absl::string_view data,
                     BleV2Medium::ServerGattConnectionCallback::
                         WriteValueCallback callback) mutable {
+                  MutexLock lock(&mutex_);
                   auto it = characteristics_.find(characteristic);
                   if (it == characteristics_.end()) {
                     callback(absl::NotFoundError("characteristic not found"));
                     return;
                   }
                   callback(it->second.write_result);
-                  if (it->second.write_result.ok() &&
-                      characteristic == *key_based_characteristic_) {
-                    trigger_keybase_value_change();
+                  if (it->second.notify_response.has_value()) {
+                    auto ignored = gatt_server_->NotifyCharacteristicChanged(
+                        characteristic, false,
+                        ByteArray(*it->second.notify_response));
                   }
                 }});
     provider_address_ = *gatt_server_->GetBlePeripheral().GetAddress();
   }
 
   void InsertCorrectGattCharacteristics() {
+    MutexLock lock(&mutex_);
     key_based_characteristic_ = gatt_server_->CreateCharacteristic(
         kFastPairServiceUuid, kKeyBasedCharacteristicUuidV2, permissions_,
         properties_);
@@ -138,50 +166,22 @@ class FastPairHandshakeImplTest : public testing::Test {
         absl::OkStatus();
   }
 
-  void SetUpValidAntiSpoofingKey(FastPairDevice* device) {
-    proto::GetObservedDeviceResponse response;
-    std::string decoded_key;
-    absl::Base64Unescape(kPublicAntiSpoof, &decoded_key);
-    response.mutable_device()->mutable_anti_spoofing_key_pair()->set_public_key(
-        decoded_key);
-    device->SetMetadata(DeviceMetadata(response));
+  void SetNotifyResponse(GattCharacteristic characteristic,
+                         absl::string_view response) {
+    MutexLock lock(&mutex_);
+    CHECK(characteristics_.find(characteristic) != characteristics_.end());
+    characteristics_[characteristic].notify_response = response;
   }
 
-  void SetUpInvalidAntiSpoofingKey(FastPairDevice* device) {
-    proto::GetObservedDeviceResponse response;
-    std::string decoded_key;
-    absl::Base64Unescape(kInvalidPublicAntiSpoof, &decoded_key);
-    response.mutable_device()->mutable_anti_spoofing_key_pair()->set_public_key(
-        decoded_key);
-    device->SetMetadata(DeviceMetadata(response));
-  }
-
-  absl::Status TriggerKeyBasedGattChanged() {
-    FastPairDataEncryptor* fast_pair_data_encryptor_ =
-        handshake_->fast_pair_data_encryptor();
-    std::array<uint8_t, kAesBlockByteSize> encryptedResponse =
-        fast_pair_data_encryptor_->EncryptBytes(kRawResponseBytes);
-    std::array<char, kAesBlockByteSize> response;
-    std::copy(encryptedResponse.begin(), encryptedResponse.end(),
-              response.begin());
-    return gatt_server_->NotifyCharacteristicChanged(
-        key_based_characteristic_.value(), false, ByteArray(response));
-  }
-
-  absl::Status TriggerKeyBasedGattChangedWithWrongResponse() {
-    return gatt_server_->NotifyCharacteristicChanged(
-        key_based_characteristic_.value(), false,
-        ByteArray(std::string(kKeyBasedResponse)));
-  }
-
-  absl::Status TriggerKeyBasedGattChangedWithWrongSizeResponse() {
-    return gatt_server_->NotifyCharacteristicChanged(
-        key_based_characteristic_.value(), false,
-        ByteArray(std::string(kWrongResponse)));
+  void SetDecryptedResponse() {
+    DecryptedResponse decrypted_response(
+        FastPairMessageType::kKeyBasedPairingResponse, address_bytes, salt);
+    fake_data_encryptor_factory_.SetResponse(std::move(decrypted_response));
   }
 
  protected:
   MediumEnvironmentStarter env_;
+  Mutex mutex_;
   SingleThreadExecutor executor_;
   std::unique_ptr<FastPairHandshake> handshake_;
   BluetoothAdapter provider_adapter_;
@@ -189,27 +189,26 @@ class FastPairHandshakeImplTest : public testing::Test {
   std::string provider_address_;
   Mediums mediums_;
   std::unique_ptr<FastPairDevice> fast_pair_device_;
-
- private:
-  absl::flat_hash_map<GattCharacteristic, CharacteristicData> characteristics_;
-  std::unique_ptr<GattServer> gatt_server_;
+  absl::flat_hash_map<GattCharacteristic, CharacteristicData> characteristics_
+      ABSL_GUARDED_BY(mutex_);
   std::optional<GattCharacteristic> key_based_characteristic_;
   std::optional<GattCharacteristic> passkey_characteristic_;
   std::optional<GattCharacteristic> accountkey_characteristic_;
+  FastPairFakeDataEncryptorImplFactory fake_data_encryptor_factory_;
+
+ private:
+  std::unique_ptr<GattServer> gatt_server_;
   Property properties_ = Property::kWrite | Property::kNotify;
   Permission permissions_ = Permission::kWrite;
 };
 
 TEST_F(FastPairHandshakeImplTest, Success) {
-  bool notified = false;
-  StartGattServer([&]() {
-    notified = true;
-    EXPECT_OK(TriggerKeyBasedGattChanged());
-  });
+  StartGattServer();
   InsertCorrectGattCharacteristics();
+  SetNotifyResponse(*key_based_characteristic_, kKeyBasedResponse);
   fast_pair_device_ = std::make_unique<FastPairDevice>(
       kMetadataId, provider_address_, Protocol::kFastPairInitialPairing);
-  SetUpValidAntiSpoofingKey(fast_pair_device_.get());
+  SetDecryptedResponse();
   CountDownLatch latch(1);
   handshake_ = std::make_unique<FastPairHandshakeImpl>(
       *fast_pair_device_, mediums_,
@@ -221,19 +220,14 @@ TEST_F(FastPairHandshakeImplTest, Success) {
       },
       &executor_);
   latch.Await();
-  EXPECT_TRUE(notified);
   EXPECT_TRUE(handshake_->completed_successfully());
 }
 
 TEST_F(FastPairHandshakeImplTest, GattError) {
-  bool notified = false;
-  StartGattServer([&]() {
-    notified = true;
-    EXPECT_OK(TriggerKeyBasedGattChanged());
-  });
+  StartGattServer();
   fast_pair_device_ = std::make_unique<FastPairDevice>(
       kMetadataId, provider_address_, Protocol::kFastPairInitialPairing);
-  SetUpValidAntiSpoofingKey(fast_pair_device_.get());
+  SetDecryptedResponse();
   CountDownLatch latch(1);
   handshake_ = std::make_unique<FastPairHandshakeImpl>(
       *fast_pair_device_, mediums_,
@@ -244,20 +238,16 @@ TEST_F(FastPairHandshakeImplTest, GattError) {
       },
       &executor_);
   latch.Await();
-  EXPECT_FALSE(notified);
   EXPECT_FALSE(handshake_->completed_successfully());
 }
 
 TEST_F(FastPairHandshakeImplTest, DataEncryptorCreateError) {
-  bool notified = false;
-  StartGattServer([&]() {
-    notified = true;
-    EXPECT_OK(TriggerKeyBasedGattChanged());
-  });
+  StartGattServer();
   InsertCorrectGattCharacteristics();
+  SetNotifyResponse(*key_based_characteristic_, kKeyBasedResponse);
   fast_pair_device_ = std::make_unique<FastPairDevice>(
       kMetadataId, provider_address_, Protocol::kFastPairInitialPairing);
-  SetUpInvalidAntiSpoofingKey(fast_pair_device_.get());
+  fake_data_encryptor_factory_.SetFailedRetrieval();
   CountDownLatch latch(1);
   handshake_ = std::make_unique<FastPairHandshakeImpl>(
       *fast_pair_device_, mediums_,
@@ -268,16 +258,15 @@ TEST_F(FastPairHandshakeImplTest, DataEncryptorCreateError) {
       },
       &executor_);
   latch.Await();
-  EXPECT_FALSE(notified);
   EXPECT_FALSE(handshake_->completed_successfully());
 }
 
 TEST_F(FastPairHandshakeImplTest, WriteResponseError) {
-  StartGattServer([]() {});
+  StartGattServer();
   InsertCorrectGattCharacteristics();
   fast_pair_device_ = std::make_unique<FastPairDevice>(
       kMetadataId, provider_address_, Protocol::kFastPairInitialPairing);
-  SetUpValidAntiSpoofingKey(fast_pair_device_.get());
+  SetDecryptedResponse();
   CountDownLatch latch(1);
   handshake_ = std::make_unique<FastPairHandshakeImpl>(
       *fast_pair_device_, mediums_,
@@ -293,15 +282,12 @@ TEST_F(FastPairHandshakeImplTest, WriteResponseError) {
 }
 
 TEST_F(FastPairHandshakeImplTest, WriteResponseWrongSize) {
-  bool notified = false;
-  StartGattServer([&]() {
-    notified = true;
-    EXPECT_OK(TriggerKeyBasedGattChangedWithWrongSizeResponse());
-  });
+  StartGattServer();
   InsertCorrectGattCharacteristics();
+  SetNotifyResponse(*key_based_characteristic_, kWrongResponse);
   fast_pair_device_ = std::make_unique<FastPairDevice>(
       kMetadataId, provider_address_, Protocol::kFastPairInitialPairing);
-  SetUpValidAntiSpoofingKey(fast_pair_device_.get());
+  SetDecryptedResponse();
   CountDownLatch latch(1);
   handshake_ = std::make_unique<FastPairHandshakeImpl>(
       *fast_pair_device_, mediums_,
@@ -313,20 +299,15 @@ TEST_F(FastPairHandshakeImplTest, WriteResponseWrongSize) {
       },
       &executor_);
   latch.Await();
-  EXPECT_TRUE(notified);
   EXPECT_FALSE(handshake_->completed_successfully());
 }
 
 TEST_F(FastPairHandshakeImplTest, ParseResponseError) {
-  bool notified = false;
-  StartGattServer([&]() {
-    notified = true;
-    EXPECT_OK(TriggerKeyBasedGattChangedWithWrongResponse());
-  });
+  StartGattServer();
   InsertCorrectGattCharacteristics();
+  SetNotifyResponse(*key_based_characteristic_, kKeyBasedResponse);
   fast_pair_device_ = std::make_unique<FastPairDevice>(
       kMetadataId, provider_address_, Protocol::kFastPairInitialPairing);
-  SetUpValidAntiSpoofingKey(fast_pair_device_.get());
   CountDownLatch latch(1);
   handshake_ = std::make_unique<FastPairHandshakeImpl>(
       *fast_pair_device_, mediums_,
@@ -338,7 +319,6 @@ TEST_F(FastPairHandshakeImplTest, ParseResponseError) {
       },
       &executor_);
   latch.Await();
-  EXPECT_TRUE(notified);
   EXPECT_FALSE(handshake_->completed_successfully());
 }
 }  // namespace fastpair