[sync] Add passkey sync observer

Add an observer class to the passkey sync entities. This is useful to
notify the chrome password manager of newly downloaded or updated
passkeys.

Bug: 1428655
Change-Id: I008949272a74d08fe414200c3c958da7390b9a27
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4571385
Reviewed-by: Martin Kreichgauer <martinkr@google.com>
Commit-Queue: Nina Satragno <nsatragno@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1153980}

chrome/browser/sync/test/integration/single_client_webauthn_credentials_sync_test.cc

@@ -31,6 +31,7 @@ using testing::UnorderedElementsAre;
 
 using webauthn_credentials_helper::EntityHasSyncId;
 using webauthn_credentials_helper::LocalPasskeysMatchChecker;
+using webauthn_credentials_helper::MockPasskeyModelObserver;
 using webauthn_credentials_helper::NewPasskey;
 using webauthn_credentials_helper::PasskeyHasSyncId;
 using webauthn_credentials_helper::PasskeySyncActiveChecker;
@@ -127,6 +128,8 @@ IN_PROC_BROWSER_TEST_F(SingleClientWebAuthnCredentialsSyncTest,
       ServerPasskeysMatchChecker(UnorderedElementsAre(EntityHasSyncId(sync_id)))
           .Wait());
 
+  MockPasskeyModelObserver observer(&GetModel());
+  EXPECT_CALL(observer, OnPasskeysChanged);
   GetModel().DeletePasskey(passkey.credential_id());
   EXPECT_TRUE(ServerPasskeysMatchChecker(IsEmpty()).Wait());
 }
@@ -155,6 +158,8 @@ IN_PROC_BROWSER_TEST_F(SingleClientWebAuthnCredentialsSyncTest,
       ServerPasskeysMatchChecker(UnorderedElementsAre(EntityHasSyncId(sync_id)))
           .Wait());
 
+  MockPasskeyModelObserver observer(&GetModel());
+  EXPECT_CALL(observer, OnPasskeysChanged).Times(0);
   EXPECT_FALSE(GetModel().DeletePasskey("non existing id"));
   EXPECT_TRUE(
       ServerPasskeysMatchChecker(UnorderedElementsAre(EntityHasSyncId(sync_id)))

chrome/browser/sync/test/integration/webauthn_credentials_helper.cc

@@ -55,9 +55,9 @@ bool PasskeySyncActiveChecker::IsExitConditionSatisfied(std::ostream* os) {
 
 LocalPasskeysMatchChecker::LocalPasskeysMatchChecker(int profile,
                                                      Matcher matcher)
-    : SingleClientStatusChangeChecker(test()->GetSyncService(profile)),
-      profile_(profile),
-      matcher_(matcher) {}
+    : profile_(profile), matcher_(matcher) {
+  observation_.Observe(&GetModel(profile_));
+}
 
 LocalPasskeysMatchChecker::~LocalPasskeysMatchChecker() = default;
 
@@ -70,8 +70,7 @@ bool LocalPasskeysMatchChecker::IsExitConditionSatisfied(std::ostream* os) {
   return matches;
 }
 
-void LocalPasskeysMatchChecker::OnSyncCycleCompleted(
-    syncer::SyncService* sync) {
+void LocalPasskeysMatchChecker::OnPasskeysChanged() {
   CheckExitCondition();
 }
 
@@ -91,6 +90,12 @@ bool ServerPasskeysMatchChecker::IsExitConditionSatisfied(std::ostream* os) {
   return matches;
 }
 
+MockPasskeyModelObserver::MockPasskeyModelObserver(PasskeyModel* model) {
+  observation_.Observe(model);
+}
+
+MockPasskeyModelObserver::~MockPasskeyModelObserver() = default;
+
 PasskeyModel& GetModel(int profile_idx) {
   return *PasskeyModelFactory::GetForProfile(test()->GetProfile(profile_idx));
 }

chrome/browser/sync/test/integration/webauthn_credentials_helper.h

@@ -7,8 +7,10 @@
 
 #include <vector>
 
+#include "base/scoped_observation.h"
 #include "chrome/browser/sync/test/integration/fake_server_match_status_checker.h"
 #include "chrome/browser/sync/test/integration/single_client_status_change_checker.h"
+#include "chrome/browser/sync/test/integration/status_change_checker.h"
 #include "components/webauthn/core/browser/passkey_model.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -30,7 +32,8 @@ class PasskeySyncActiveChecker : public SingleClientStatusChangeChecker {
   bool IsExitConditionSatisfied(std::ostream* os) override;
 };
 
-class LocalPasskeysMatchChecker : public SingleClientStatusChangeChecker {
+class LocalPasskeysMatchChecker : public StatusChangeChecker,
+                                  public PasskeyModel::Observer {
  public:
   using Matcher =
       testing::Matcher<std::vector<sync_pb::WebauthnCredentialSpecifics>>;
@@ -40,11 +43,15 @@ class LocalPasskeysMatchChecker : public SingleClientStatusChangeChecker {
 
   // SingleClientStatusChangeChecker:
   bool IsExitConditionSatisfied(std::ostream* os) override;
-  void OnSyncCycleCompleted(syncer::SyncService* sync) override;
+
+  // PasskeyModel::Observer:
+  void OnPasskeysChanged() override;
 
  private:
   const int profile_;
   const Matcher matcher_;
+  base::ScopedObservation<PasskeyModel, PasskeyModel::Observer> observation_{
+      this};
 };
 
 class ServerPasskeysMatchChecker
@@ -62,6 +69,18 @@ class ServerPasskeysMatchChecker
   const Matcher matcher_;
 };
 
+class MockPasskeyModelObserver : public PasskeyModel::Observer {
+ public:
+  explicit MockPasskeyModelObserver(PasskeyModel* model);
+  ~MockPasskeyModelObserver() override;
+
+  MOCK_METHOD(void, OnPasskeysChanged, (), (override));
+
+ private:
+  base::ScopedObservation<PasskeyModel, PasskeyModel::Observer> observation_{
+      this};
+};
+
 PasskeyModel& GetModel(int profile_idx);
 
 bool AwaitAllModelsMatch();

components/webauthn/core/browser/BUILD.gn

@@ -33,8 +33,6 @@ static_library("test_support") {
   sources = [
     "mock_internal_authenticator.cc",
     "mock_internal_authenticator.h",
-    "test_passkey_model.cc",
-    "test_passkey_model.h",
   ]
   deps = [
     ":browser",
@@ -44,4 +42,10 @@ static_library("test_support") {
     "//testing/gmock",
     "//testing/gtest",
   ]
+  if (!is_ios && !is_android) {
+    sources += [
+      "test_passkey_model.cc",
+      "test_passkey_model.h",
+    ]
+  }
 }

components/webauthn/core/browser/passkey_model.h

@@ -10,6 +10,7 @@
 
 #include "base/containers/flat_set.h"
 #include "base/memory/weak_ptr.h"
+#include "base/observer_list_types.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/sync/model/model_type_store.h"
 
@@ -35,6 +36,16 @@ class ModelTypeControllerDelegate;
 // accessible though PasskeyModel.
 class PasskeyModel : public KeyedService {
  public:
+  class Observer : public base::CheckedObserver {
+   public:
+    // Notifies the observer that passkeys have changed, e.g. because a new one
+    // was downloaded or deleted.
+    virtual void OnPasskeysChanged() = 0;
+  };
+
+  virtual void AddObserver(Observer* observer) = 0;
+  virtual void RemoveObserver(Observer* observer) = 0;
+
   // Returns the sync ModelTypeControllerDelegate for the WEBAUTHN_CREDENTIAL
   // data type.
   virtual base::WeakPtr<syncer::ModelTypeControllerDelegate>

components/webauthn/core/browser/passkey_sync_bridge.cc

@@ -118,6 +118,14 @@ PasskeySyncBridge::PasskeySyncBridge(
 
 PasskeySyncBridge::~PasskeySyncBridge() = default;
 
+void PasskeySyncBridge::AddObserver(Observer* observer) {
+  observers_.AddObserver(observer);
+}
+
+void PasskeySyncBridge::RemoveObserver(Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
 std::unique_ptr<syncer::MetadataChangeList>
 PasskeySyncBridge::CreateMetadataChangeList() {
   return syncer::ModelTypeStore::WriteBatch::CreateMetadataChangeList();
@@ -150,6 +158,7 @@ absl::optional<syncer::ModelError> PasskeySyncBridge::MergeFullSyncData(
       std::move(write_batch),
       base::BindOnce(&PasskeySyncBridge::OnStoreCommitWriteBatch,
                      weak_ptr_factory_.GetWeakPtr()));
+  NotifyPasskeysChanged();
   return absl::nullopt;
 }
 
@@ -184,6 +193,9 @@ PasskeySyncBridge::ApplyIncrementalSyncChanges(
       std::move(write_batch),
       base::BindOnce(&PasskeySyncBridge::OnStoreCommitWriteBatch,
                      weak_ptr_factory_.GetWeakPtr()));
+  if (!entity_changes.empty()) {
+    NotifyPasskeysChanged();
+  }
   return absl::nullopt;
 }
 
@@ -228,6 +240,7 @@ void PasskeySyncBridge::ApplyDisableSyncChanges(
   CHECK(store_);
   store_->DeleteAllDataAndMetadata(base::DoNothing());
   data_.clear();
+  NotifyPasskeysChanged();
 }
 
 base::WeakPtr<syncer::ModelTypeControllerDelegate>
@@ -296,6 +309,7 @@ bool PasskeySyncBridge::DeletePasskey(const std::string& credential_id) {
       std::move(write_batch),
       base::BindOnce(&PasskeySyncBridge::OnStoreCommitWriteBatch,
                      weak_ptr_factory_.GetWeakPtr()));
+  NotifyPasskeysChanged();
   return true;
 }
 
@@ -316,6 +330,7 @@ std::string PasskeySyncBridge::AddNewPasskeyForTesting(
       base::BindOnce(&PasskeySyncBridge::OnStoreCommitWriteBatch,
                      weak_ptr_factory_.GetWeakPtr()));
   data_[sync_id] = std::move(specifics);
+  NotifyPasskeysChanged();
   return sync_id;
 }
 
@@ -363,6 +378,7 @@ void PasskeySyncBridge::OnStoreReadAllMetadata(
     std::string storage_key = specifics.sync_id();
     data_[std::move(storage_key)] = std::move(specifics);
   }
+  NotifyPasskeysChanged();
 }
 
 void PasskeySyncBridge::OnStoreCommitWriteBatch(
@@ -372,3 +388,9 @@ void PasskeySyncBridge::OnStoreCommitWriteBatch(
     return;
   }
 }
+
+void PasskeySyncBridge::NotifyPasskeysChanged() {
+  for (auto& observer : observers_) {
+    observer.OnPasskeysChanged();
+  }
+}

components/webauthn/core/browser/passkey_sync_bridge.h

@@ -10,6 +10,7 @@
 
 #include "base/containers/flat_set.h"
 #include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
 #include "components/sync/model/model_type_store.h"
 #include "components/sync/model/model_type_sync_bridge.h"
 #include "components/sync/protocol/webauthn_credential_specifics.pb.h"
@@ -48,6 +49,8 @@ class PasskeySyncBridge : public syncer::ModelTypeSyncBridge,
                                    delete_metadata_change_list) override;
 
   // PasskeyModel:
+  void AddObserver(Observer* observer) override;
+  void RemoveObserver(Observer* observer) override;
   base::WeakPtr<syncer::ModelTypeControllerDelegate>
   GetModelTypeControllerDelegate() override;
   base::flat_set<std::string> GetAllSyncIds() const override;
@@ -68,13 +71,16 @@ class PasskeySyncBridge : public syncer::ModelTypeSyncBridge,
       const absl::optional<syncer::ModelError>& error,
       std::unique_ptr<syncer::MetadataBatch> metadata_batch);
   void OnStoreCommitWriteBatch(const absl::optional<syncer::ModelError>& error);
+  void NotifyPasskeysChanged();
 
   // Local view of the stored data. Indexes specifics protos by storage key.
   std::map<std::string, sync_pb::WebauthnCredentialSpecifics> data_;
 
   // Passkeys are stored locally in leveldb.
   std::unique_ptr<syncer::ModelTypeStore> store_;
 
+  base::ObserverList<Observer> observers_;
+
   base::WeakPtrFactory<PasskeySyncBridge> weak_ptr_factory_{this};
 };
 

components/webauthn/core/browser/test_passkey_model.cc

@@ -12,6 +12,14 @@
 TestPasskeyModel::TestPasskeyModel() = default;
 TestPasskeyModel::~TestPasskeyModel() = default;
 
+void TestPasskeyModel::AddObserver(Observer* observer) {
+  observers_.AddObserver(observer);
+}
+
+void TestPasskeyModel::RemoveObserver(Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
 base::WeakPtr<syncer::ModelTypeControllerDelegate>
 TestPasskeyModel::GetModelTypeControllerDelegate() {
   NOTIMPLEMENTED();
@@ -34,13 +42,23 @@ TestPasskeyModel::GetAllPasskeys() const {
 std::string TestPasskeyModel::AddNewPasskeyForTesting(
     sync_pb::WebauthnCredentialSpecifics passkey) {
   credentials_.push_back(std::move(passkey));
+  NotifyPasskeysChanged();
   return credentials_.back().credential_id();
 }
 
 bool TestPasskeyModel::DeletePasskey(const std::string& credential_id) {
   // Don't implement the shadow chain deletion logic. Instead, remove the
   // credential with the matching id.
-  return std::erase_if(credentials_, [&credential_id](const auto& credential) {
-           return credential.credential_id() == credential_id;
-         }) > 0;
+  bool removed =
+      std::erase_if(credentials_, [&credential_id](const auto& credential) {
+        return credential.credential_id() == credential_id;
+      }) > 0;
+  NotifyPasskeysChanged();
+  return removed;
+}
+
+void TestPasskeyModel::NotifyPasskeysChanged() {
+  for (auto& observer : observers_) {
+    observer.OnPasskeysChanged();
+  }
 }

components/webauthn/core/browser/test_passkey_model.h

@@ -7,6 +7,7 @@
 
 #include <string>
 
+#include "base/observer_list.h"
 #include "components/sync/protocol/webauthn_credential_specifics.pb.h"
 #include "components/webauthn/core/browser/passkey_model.h"
 
@@ -16,6 +17,8 @@ class TestPasskeyModel : public PasskeyModel {
   ~TestPasskeyModel() override;
 
   // PasskeyModel:
+  void AddObserver(Observer* observer) override;
+  void RemoveObserver(Observer* observer) override;
   base::WeakPtr<syncer::ModelTypeControllerDelegate>
   GetModelTypeControllerDelegate() override;
   base::flat_set<std::string> GetAllSyncIds() const override;
@@ -26,7 +29,10 @@ class TestPasskeyModel : public PasskeyModel {
   bool DeletePasskey(const std::string& credential_id) override;
 
  private:
+  void NotifyPasskeysChanged();
+
   std::vector<sync_pb::WebauthnCredentialSpecifics> credentials_;
+  base::ObserverList<Observer> observers_;
 };
 
 #endif  // COMPONENTS_WEBAUTHN_CORE_BROWSER__TEST_PASSKEY_MODEL_H_