New plumbing of incoming sync trusted vault keys

The API is refactored to isolate the flow for incoming encryption keys.
This better separates the API from other functions that are intended to
be used by actual UI (e.g. user-entered passphrases), and sets the
foundation for triggering fetches initiated by the TrustedVaultClient
implementation itself (useful on Android).

There is a behavioral difference as a result of rewriting the logic
in SyncServiceCrypto, which is arguably desirable: trusted vault keys
are not propagated to the sync engine (and hence the Nigori keybag
unless there is an actual need to do so, which excludes, most
importantly, users that are not in the relevant passphrase type).

Bug: 1012659
Change-Id: I009e02ebd0370eb98e95c50368299b0bf947cfb1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1973823
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#726535}

chrome/browser/sync/sync_encryption_keys_tab_helper.cc

@@ -13,7 +13,6 @@
 #include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/common/sync_encryption_keys_extension.mojom.h"
 #include "components/sync/driver/sync_service.h"
-#include "components/sync/driver/sync_user_settings.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_receiver_set.h"
@@ -76,7 +75,7 @@ class SyncEncryptionKeysTabHelper::EncryptionKeyApi
     CHECK_EQ(receivers_.GetCurrentTargetFrame()->GetLastCommittedOrigin(),
              GetAllowedOrigin());
 
-    sync_service_->GetUserSettings()->AddTrustedVaultDecryptionKeys(
+    sync_service_->AddTrustedVaultDecryptionKeysFromWeb(
         gaia_id, EncryptionKeysAsStrings(encryption_keys));
     std::move(callback).Run();
   }

chrome/browser/sync/trusted_vault_client_android.cc

@@ -53,6 +53,12 @@ void TrustedVaultClientAndroid::FetchKeysCompleted(
   std::move(cb).Run(converted_keys);
 }
 
+std::unique_ptr<TrustedVaultClientAndroid::Subscription>
+TrustedVaultClientAndroid::AddKeysChangedObserver(
+    const base::RepeatingClosure& cb) {
+  return observer_list_.Add(cb);
+}
+
 void TrustedVaultClientAndroid::FetchKeys(
     const std::string& gaia_id,
     base::OnceCallback<void(const std::vector<std::string>&)> cb) {

chrome/browser/sync/trusted_vault_client_android.h

@@ -37,6 +37,8 @@ class TrustedVaultClientAndroid : public syncer::TrustedVaultClient {
       const base::android::JavaParamRef<jobjectArray>& keys);
 
   // TrustedVaultClient implementation.
+  std::unique_ptr<Subscription> AddKeysChangedObserver(
+      const base::RepeatingClosure& cb) override;
   void FetchKeys(
       const std::string& gaia_id,
       base::OnceCallback<void(const std::vector<std::string>&)> cb) override;
@@ -57,6 +59,8 @@ class TrustedVaultClientAndroid : public syncer::TrustedVaultClient {
 
   // Null if no in-flight FetchKeys().
   std::unique_ptr<OngoingFetchKeys> ongoing_fetch_keys_;
+
+  CallbackList observer_list_;
 };
 
 #endif  // CHROME_BROWSER_SYNC_TRUSTED_VAULT_CLIENT_ANDROID_H_

components/sync/driver/fake_sync_service.cc

@@ -163,6 +163,10 @@ void FakeSyncService::GetAllNodesForDebugging(
 
 void FakeSyncService::SetInvalidationsForSessionsEnabled(bool enabled) {}
 
+void FakeSyncService::AddTrustedVaultDecryptionKeysFromWeb(
+    const std::string& gaia_id,
+    const std::vector<std::string>& keys) {}
+
 UserDemographicsResult FakeSyncService::GetUserNoisedBirthYearAndGender(
     base::Time now) {
   return UserDemographicsResult::ForStatus(

components/sync/driver/fake_sync_service.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 #include <string>
+#include <vector>
 
 #include "components/sync/driver/sync_service.h"
 #include "google_apis/gaia/google_service_auth_error.h"
@@ -67,6 +68,9 @@ class FakeSyncService : public SyncService {
       base::OnceCallback<void(std::unique_ptr<base::ListValue>)> callback)
       override;
   void SetInvalidationsForSessionsEnabled(bool enabled) override;
+  void AddTrustedVaultDecryptionKeysFromWeb(
+      const std::string& gaia_id,
+      const std::vector<std::string>& keys) override;
   UserDemographicsResult GetUserNoisedBirthYearAndGender(
       base::Time now) override;
 

components/sync/driver/file_based_trusted_vault_client.cc

@@ -123,6 +123,12 @@ FileBasedTrustedVaultClient::FileBasedTrustedVaultClient(
 
 FileBasedTrustedVaultClient::~FileBasedTrustedVaultClient() = default;
 
+std::unique_ptr<FileBasedTrustedVaultClient::Subscription>
+FileBasedTrustedVaultClient::AddKeysChangedObserver(
+    const base::RepeatingClosure& cb) {
+  return observer_list_.Add(cb);
+}
+
 void FileBasedTrustedVaultClient::FetchKeys(
     const std::string& gaia_id,
     base::OnceCallback<void(const std::vector<std::string>&)> cb) {
@@ -138,6 +144,7 @@ void FileBasedTrustedVaultClient::StoreKeys(
   TriggerLazyInitializationIfNeeded();
   backend_task_runner_->PostTask(
       FROM_HERE, base::BindOnce(&Backend::StoreKeys, backend_, gaia_id, keys));
+  observer_list_.Notify();
 }
 
 void FileBasedTrustedVaultClient::WaitForFlushForTesting(

components/sync/driver/file_based_trusted_vault_client.h

@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_SYNC_DRIVER_FILE_BASED_TRUSTED_VAULT_CLIENT_H_
 #define COMPONENTS_SYNC_DRIVER_FILE_BASED_TRUSTED_VAULT_CLIENT_H_
 
+#include <memory>
 #include <string>
 #include <vector>
 
@@ -28,6 +29,8 @@ class FileBasedTrustedVaultClient : public TrustedVaultClient {
   ~FileBasedTrustedVaultClient() override;
 
   // TrustedVaultClient implementation.
+  std::unique_ptr<Subscription> AddKeysChangedObserver(
+      const base::RepeatingClosure& cb) override;
   void FetchKeys(
       const std::string& gaia_id,
       base::OnceCallback<void(const std::vector<std::string>&)> cb) override;
@@ -44,6 +47,8 @@ class FileBasedTrustedVaultClient : public TrustedVaultClient {
   const base::FilePath file_path_;
   const scoped_refptr<base::SequencedTaskRunner> backend_task_runner_;
 
+  CallbackList observer_list_;
+
   // Backend constructed lazily in the UI thread, used in |backend_task_runner_|
   // and destroyed (refcounted) on any thread.
   class Backend;

components/sync/driver/mock_sync_service.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 #include <string>
+#include <vector>
 
 #include "base/values.h"
 #include "components/signin/public/identity_manager/account_info.h"
@@ -43,7 +44,6 @@ class MockSyncService : public SyncService {
   MOCK_CONST_METHOD0(RequiresClientUpgrade, bool());
   MOCK_CONST_METHOD0(GetExperimentalAuthenticationKey,
                      std::unique_ptr<crypto::ECPrivateKey>());
-
   MOCK_METHOD0(GetSetupInProgressHandle,
                std::unique_ptr<SyncSetupInProgressHandle>());
   MOCK_CONST_METHOD0(IsSetupInProgress, bool());
@@ -57,6 +57,9 @@ class MockSyncService : public SyncService {
   MOCK_METHOD1(TriggerRefresh, void(const ModelTypeSet& types));
   MOCK_METHOD1(DataTypePreconditionChanged, void(syncer::ModelType type));
   MOCK_METHOD1(SetInvalidationsForSessionsEnabled, void(bool enabled));
+  MOCK_METHOD2(AddTrustedVaultDecryptionKeysFromWeb,
+               void(const std::string& gaia_id,
+                    const std::vector<std::string>& keys));
   MOCK_METHOD1(GetUserNoisedBirthYearAndGender,
                UserDemographicsResult(base::Time now));
 

components/sync/driver/profile_sync_service.cc

@@ -1779,6 +1779,12 @@ void ProfileSyncService::SetInvalidationsForSessionsEnabled(bool enabled) {
   }
 }
 
+void ProfileSyncService::AddTrustedVaultDecryptionKeysFromWeb(
+    const std::string& gaia_id,
+    const std::vector<std::string>& keys) {
+  sync_client_->GetTrustedVaultClient()->StoreKeys(gaia_id, keys);
+}
+
 UserDemographicsResult ProfileSyncService::GetUserNoisedBirthYearAndGender(
     base::Time now) {
   // Do not provide the synced user’s birth year and gender when sync is

components/sync/driver/profile_sync_service.h

@@ -137,6 +137,9 @@ class ProfileSyncService : public SyncService,
   void TriggerRefresh(const ModelTypeSet& types) override;
   void DataTypePreconditionChanged(ModelType type) override;
   void SetInvalidationsForSessionsEnabled(bool enabled) override;
+  void AddTrustedVaultDecryptionKeysFromWeb(
+      const std::string& gaia_id,
+      const std::vector<std::string>& keys) override;
   UserDemographicsResult GetUserNoisedBirthYearAndGender(
       base::Time now) override;
   void AddObserver(SyncServiceObserver* observer) override;

components/sync/driver/sync_service.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 #include <string>
+#include <vector>
 
 #include "base/callback.h"
 #include "base/location.h"
@@ -368,6 +369,12 @@ class SyncService : public KeyedService {
   // page is opened.
   virtual void SetInvalidationsForSessionsEnabled(bool enabled) = 0;
 
+  // Processes trusted vault encryption keys retrieved from the web. Unused and
+  // ignored on platforms where keys are retrieved by other means.
+  virtual void AddTrustedVaultDecryptionKeysFromWeb(
+      const std::string& gaia_id,
+      const std::vector<std::string>& keys) = 0;
+
   //////////////////////////////////////////////////////////////////////////////
   // USER DEMOGRAPHICS
   //////////////////////////////////////////////////////////////////////////////

components/sync/driver/sync_service_crypto.cc

@@ -17,7 +17,6 @@
 #include "components/sync/base/sync_prefs.h"
 #include "components/sync/driver/sync_driver_switches.h"
 #include "components/sync/driver/sync_service.h"
-#include "components/sync/driver/trusted_vault_client.h"
 #include "components/sync/engine/sync_string_conversions.h"
 #include "components/sync/nigori/nigori.h"
 
@@ -31,7 +30,12 @@ class EmptyTrustedVaultClient : public TrustedVaultClient {
   EmptyTrustedVaultClient() = default;
   ~EmptyTrustedVaultClient() override = default;
 
-  // TrustedVaultClient implementatio.
+  // TrustedVaultClient implementation.
+  std::unique_ptr<Subscription> AddKeysChangedObserver(
+      const base::RepeatingClosure& cb) override {
+    return nullptr;
+  }
+
   void FetchKeys(
       const std::string& gaia_id,
       base::OnceCallback<void(const std::vector<std::string>&)> cb) override {
@@ -39,7 +43,10 @@ class EmptyTrustedVaultClient : public TrustedVaultClient {
   }
 
   void StoreKeys(const std::string& gaia_id,
-                 const std::vector<std::string>& keys) override {}
+                 const std::vector<std::string>& keys) override {
+    // Never invoked by SyncServiceCrypto.
+    NOTREACHED();
+  }
 };
 
 // A SyncEncryptionHandler::Observer implementation that simply posts all calls
@@ -186,6 +193,11 @@ SyncServiceCrypto::SyncServiceCrypto(
   DCHECK(reconfigure_);
   DCHECK(sync_prefs_);
   DCHECK(trusted_vault_client_);
+
+  trusted_vault_client_subscription_ =
+      trusted_vault_client_->AddKeysChangedObserver(base::BindRepeating(
+          &SyncServiceCrypto::OnTrustedVaultClientKeysChanged,
+          weak_factory_.GetWeakPtr()));
 }
 
 SyncServiceCrypto::~SyncServiceCrypto() = default;
@@ -206,6 +218,7 @@ bool SyncServiceCrypto::IsPassphraseRequired() const {
     case RequiredUserAction::kNone:
     case RequiredUserAction::kFetchingTrustedVaultKeys:
     case RequiredUserAction::kTrustedVaultKeyRequired:
+    case RequiredUserAction::kTrustedVaultKeyRequiredButFetching:
       return false;
     case RequiredUserAction::kPassphraseRequiredForDecryption:
     case RequiredUserAction::kPassphraseRequiredForEncryption:
@@ -224,7 +237,9 @@ bool SyncServiceCrypto::IsUsingSecondaryPassphrase() const {
 bool SyncServiceCrypto::IsTrustedVaultKeyRequired() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return state_.required_user_action ==
-         RequiredUserAction::kTrustedVaultKeyRequired;
+             RequiredUserAction::kTrustedVaultKeyRequired ||
+         state_.required_user_action ==
+             RequiredUserAction::kTrustedVaultKeyRequiredButFetching;
 }
 
 void SyncServiceCrypto::EnableEncryptEverything() {
@@ -256,6 +271,7 @@ void SyncServiceCrypto::SetEncryptionPassphrase(const std::string& passphrase) {
     case RequiredUserAction::kPassphraseRequiredForDecryption:
     case RequiredUserAction::kFetchingTrustedVaultKeys:
     case RequiredUserAction::kTrustedVaultKeyRequired:
+    case RequiredUserAction::kTrustedVaultKeyRequiredButFetching:
       // Cryptographer has pending keys.
       NOTREACHED()
           << "Can not set explicit passphrase when decryption is needed.";
@@ -323,16 +339,6 @@ bool SyncServiceCrypto::SetDecryptionPassphrase(const std::string& passphrase) {
   return true;
 }
 
-void SyncServiceCrypto::AddTrustedVaultDecryptionKeys(
-    const std::string& gaia_id,
-    const std::vector<std::string>& keys) {
-  trusted_vault_client_->StoreKeys(gaia_id, keys);
-
-  if (state_.engine && state_.account_info.gaia == gaia_id) {
-    state_.engine->AddTrustedVaultDecryptionKeys(keys, base::DoNothing());
-  }
-}
-
 PassphraseType SyncServiceCrypto::GetPassphraseType() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return state_.cached_passphrase_type;
@@ -359,6 +365,7 @@ bool SyncServiceCrypto::HasCryptoError() const {
       return false;
     case RequiredUserAction::kFetchingTrustedVaultKeys:
     case RequiredUserAction::kTrustedVaultKeyRequired:
+    case RequiredUserAction::kTrustedVaultKeyRequiredButFetching:
     case RequiredUserAction::kPassphraseRequiredForDecryption:
     case RequiredUserAction::kPassphraseRequiredForEncryption:
       return true;
@@ -447,6 +454,7 @@ void SyncServiceCrypto::OnTrustedVaultKeyAccepted() {
       return;
     case RequiredUserAction::kFetchingTrustedVaultKeys:
     case RequiredUserAction::kTrustedVaultKeyRequired:
+    case RequiredUserAction::kTrustedVaultKeyRequiredButFetching:
       break;
   }
 
@@ -532,10 +540,40 @@ SyncServiceCrypto::GetEncryptionObserverProxy() {
       weak_factory_.GetWeakPtr(), base::SequencedTaskRunnerHandle::Get());
 }
 
+void SyncServiceCrypto::OnTrustedVaultClientKeysChanged() {
+  switch (state_.required_user_action) {
+    case RequiredUserAction::kNone:
+    case RequiredUserAction::kPassphraseRequiredForDecryption:
+    case RequiredUserAction::kPassphraseRequiredForEncryption:
+      // If no trusted vault keys are required, there's nothing to do. If they
+      // later are required, a fetch will be triggered in
+      // OnTrustedVaultKeyRequired().
+      return;
+    case RequiredUserAction::kFetchingTrustedVaultKeys:
+    case RequiredUserAction::kTrustedVaultKeyRequiredButFetching:
+      // If there's an ongoing fetch, FetchKeys() cannot be issued immediately
+      // since that violates the function precondition. However, the in-flight
+      // FetchKeys() may end up returning stale keys, so let's make sure
+      // FetchKeys() is invoked again once it becomes possible.
+      state_.deferred_trusted_vault_fetch_keys_pending = true;
+      return;
+    case RequiredUserAction::kTrustedVaultKeyRequired:
+      state_.required_user_action =
+          RequiredUserAction::kTrustedVaultKeyRequiredButFetching;
+      break;
+  }
+
+  FetchTrustedVaultKeys();
+}
+
 void SyncServiceCrypto::FetchTrustedVaultKeys() {
   DCHECK(state_.engine);
-  DCHECK_EQ(state_.required_user_action,
-            RequiredUserAction::kFetchingTrustedVaultKeys);
+  DCHECK(state_.required_user_action ==
+             RequiredUserAction::kFetchingTrustedVaultKeys ||
+         state_.required_user_action ==
+             RequiredUserAction::kTrustedVaultKeyRequiredButFetching);
+
+  state_.deferred_trusted_vault_fetch_keys_pending = false;
 
   trusted_vault_client_->FetchKeys(
       state_.account_info.gaia,
@@ -545,11 +583,15 @@ void SyncServiceCrypto::FetchTrustedVaultKeys() {
 
 void SyncServiceCrypto::TrustedVaultKeysFetched(
     const std::vector<std::string>& keys) {
-  // The engine could have been shut down while keys were being fetched.
-  if (!state_.engine) {
+  if (state_.required_user_action !=
+          RequiredUserAction::kFetchingTrustedVaultKeys &&
+      state_.required_user_action !=
+          RequiredUserAction::kTrustedVaultKeyRequiredButFetching) {
     return;
   }
 
+  DCHECK(state_.engine);
+
   state_.engine->AddTrustedVaultDecryptionKeys(
       keys, base::BindOnce(&SyncServiceCrypto::TrustedVaultKeysAdded,
                            weak_factory_.GetWeakPtr()));
@@ -557,7 +599,16 @@ void SyncServiceCrypto::TrustedVaultKeysFetched(
 
 void SyncServiceCrypto::TrustedVaultKeysAdded() {
   if (state_.required_user_action !=
-      RequiredUserAction::kFetchingTrustedVaultKeys) {
+          RequiredUserAction::kFetchingTrustedVaultKeys &&
+      state_.required_user_action !=
+          RequiredUserAction::kTrustedVaultKeyRequiredButFetching) {
+    return;
+  }
+
+  // If FetchKeys() was intended to be called during an already existing ongoing
+  // FetchKeys(), it needs to be invoked now that it's possible.
+  if (state_.deferred_trusted_vault_fetch_keys_pending) {
+    FetchTrustedVaultKeys();
     return;
   }
 

components/sync/driver/sync_service_crypto.h

@@ -15,6 +15,7 @@
 #include "components/signin/public/identity_manager/account_info.h"
 #include "components/sync/base/model_type.h"
 #include "components/sync/driver/data_type_encryption_handler.h"
+#include "components/sync/driver/trusted_vault_client.h"
 #include "components/sync/engine/configure_reason.h"
 #include "components/sync/engine/sync_encryption_handler.h"
 #include "components/sync/engine/sync_engine.h"
@@ -22,7 +23,6 @@
 namespace syncer {
 
 class CryptoSyncPrefs;
-class TrustedVaultClient;
 
 // This class functions as mostly independent component of SyncService that
 // handles things related to encryption, including holding lots of state and
@@ -51,8 +51,6 @@ class SyncServiceCrypto : public SyncEncryptionHandler::Observer,
   bool IsEncryptEverythingEnabled() const;
   void SetEncryptionPassphrase(const std::string& passphrase);
   bool SetDecryptionPassphrase(const std::string& passphrase);
-  void AddTrustedVaultDecryptionKeys(const std::string& gaia_id,
-                                     const std::vector<std::string>& keys);
 
   // Returns the actual passphrase type being used for encryption.
   PassphraseType GetPassphraseType() const;
@@ -98,8 +96,15 @@ class SyncServiceCrypto : public SyncEncryptionHandler::Observer,
     // Silent attempt is completed and user action is definitely required to
     // retrieve trusted vault keys.
     kTrustedVaultKeyRequired,
+    // The need for user action has already been surfaced to upper layers (UI)
+    // via IsTrustedVaultKeyRequired() but there's an ongoing fetch that may
+    // resolve the issue.
+    kTrustedVaultKeyRequiredButFetching,
   };
 
+  // Observer method invoked by TrustedVaultClient when its content changes.
+  void OnTrustedVaultClientKeysChanged();
+
   // Reads trusted vault keys from the client and feeds them to the sync engine.
   void FetchTrustedVaultKeys();
 
@@ -120,6 +125,10 @@ class SyncServiceCrypto : public SyncEncryptionHandler::Observer,
   // Never null and guaranteed to outlive us.
   TrustedVaultClient* const trusted_vault_client_;
 
+  // Subscription to observe changes in |*trusted_vault_client_|.
+  std::unique_ptr<TrustedVaultClient::Subscription>
+      trusted_vault_client_subscription_;
+
   // All the mutable state is wrapped in a struct so that it can be easily
   // reset to its default values.
   struct State {
@@ -175,6 +184,10 @@ class SyncServiceCrypto : public SyncEncryptionHandler::Observer,
     // If an explicit passphrase is in use, the time at which the passphrase was
     // first set (if available).
     base::Time cached_explicit_passphrase_time;
+
+    // Set to true when FetchKeys() should be issued again once an ongoing
+    // fetch-and-add procedure completes.
+    bool deferred_trusted_vault_fetch_keys_pending = false;
   } state_;
 
   SEQUENCE_CHECKER(sequence_checker_);

components/sync/driver/sync_service_crypto_unittest.cc

@@ -4,6 +4,8 @@
 
 #include "components/sync/driver/sync_service_crypto.h"
 
+#include <list>
+#include <map>
 #include <utility>
 
 #include "base/bind_helpers.h"
@@ -21,6 +23,7 @@ namespace syncer {
 namespace {
 
 using testing::_;
+using testing::Eq;
 
 sync_pb::EncryptedData MakeEncryptedData(
     const std::string& passphrase,
@@ -56,18 +59,52 @@ class MockCryptoSyncPrefs : public CryptoSyncPrefs {
   MOCK_METHOD1(SetKeystoreEncryptionBootstrapToken, void(const std::string&));
 };
 
-class MockTrustedVaultClient : public TrustedVaultClient {
+// Simple in-memory implementation of TrustedVaultClient.
+class TestTrustedVaultClient : public TrustedVaultClient {
  public:
-  MockTrustedVaultClient() = default;
-  ~MockTrustedVaultClient() override = default;
-
-  MOCK_METHOD2(
-      FetchKeys,
-      void(const std::string& gaia_id,
-           base::OnceCallback<void(const std::vector<std::string>&)> cb));
-  MOCK_METHOD2(StoreKeys,
-               void(const std::string& gaia_id,
-                    const std::vector<std::string>& keys));
+  TestTrustedVaultClient() = default;
+  ~TestTrustedVaultClient() override = default;
+
+  // Exposes the total number of calls to FetchKeys().
+  int fetch_count() const { return fetch_count_; }
+
+  // Mimics the completion of the next (FIFO) FetchKeys() request.
+  bool CompleteFetchKeysRequest() {
+    if (pending_responses_.empty()) {
+      return false;
+    }
+
+    base::OnceClosure cb = std::move(pending_responses_.front());
+    pending_responses_.pop_front();
+    std::move(cb).Run();
+    return true;
+  }
+
+  // TrustedVaultClient implementation.
+  std::unique_ptr<Subscription> AddKeysChangedObserver(
+      const base::RepeatingClosure& cb) override {
+    return observer_list_.Add(cb);
+  }
+
+  void FetchKeys(
+      const std::string& gaia_id,
+      base::OnceCallback<void(const std::vector<std::string>&)> cb) override {
+    ++fetch_count_;
+    pending_responses_.push_back(
+        base::BindOnce(std::move(cb), gaia_id_to_keys_[gaia_id]));
+  }
+
+  void StoreKeys(const std::string& gaia_id,
+                 const std::vector<std::string>& keys) override {
+    gaia_id_to_keys_[gaia_id] = keys;
+    observer_list_.Notify();
+  }
+
+ private:
+  std::map<std::string, std::vector<std::string>> gaia_id_to_keys_;
+  CallbackList observer_list_;
+  int fetch_count_ = 0;
+  std::list<base::OnceClosure> pending_responses_;
 };
 
 class SyncServiceCryptoTest : public testing::Test {
@@ -93,7 +130,7 @@ class SyncServiceCryptoTest : public testing::Test {
       base::MockCallback<base::RepeatingCallback<void(ConfigureReason)>>>
       reconfigure_cb_;
   testing::NiceMock<MockCryptoSyncPrefs> prefs_;
-  testing::NiceMock<MockTrustedVaultClient> trusted_vault_client_;
+  testing::NiceMock<TestTrustedVaultClient> trusted_vault_client_;
   testing::NiceMock<MockSyncEngine> engine_;
   SyncServiceCrypto crypto_;
 };
@@ -103,6 +140,7 @@ TEST_F(SyncServiceCryptoTest, ShouldExposePassphraseRequired) {
 
   crypto_.SetSyncEngine(CoreAccountInfo(), &engine_);
   ASSERT_FALSE(crypto_.IsPassphraseRequired());
+  ASSERT_THAT(trusted_vault_client_.fetch_count(), Eq(0));
 
   // Mimic the engine determining that a passphrase is required.
   EXPECT_CALL(reconfigure_cb_, Run(CONFIGURE_REASON_CRYPTO));
@@ -130,38 +168,6 @@ TEST_F(SyncServiceCryptoTest, ShouldExposePassphraseRequired) {
   EXPECT_FALSE(crypto_.IsPassphraseRequired());
 }
 
-TEST_F(SyncServiceCryptoTest,
-       ShouldStoreTrustedVaultKeysBeforeEngineInitialization) {
-  const std::string kAccount = "account1";
-  const std::vector<std::string> kKeys = {"key1"};
-  EXPECT_CALL(trusted_vault_client_, StoreKeys("account1", kKeys));
-  crypto_.AddTrustedVaultDecryptionKeys(kAccount, kKeys);
-}
-
-TEST_F(SyncServiceCryptoTest,
-       ShouldStoreTrustedVaultKeysAfterEngineInitialization) {
-  const CoreAccountInfo kSyncingAccount =
-      MakeAccountInfoWithGaia("syncingaccount");
-  const CoreAccountInfo kOtherAccount = MakeAccountInfoWithGaia("otheraccount");
-  const std::vector<std::string> kSyncingAccountKeys = {"key1"};
-  const std::vector<std::string> kOtherAccountKeys = {"key2"};
-
-  crypto_.SetSyncEngine(kSyncingAccount, &engine_);
-
-  EXPECT_CALL(trusted_vault_client_,
-              StoreKeys(kOtherAccount.gaia, kOtherAccountKeys));
-  EXPECT_CALL(trusted_vault_client_,
-              StoreKeys(kSyncingAccount.gaia, kSyncingAccountKeys));
-
-  // Only the sync-ing account should be propagated to the engine.
-  EXPECT_CALL(engine_, AddTrustedVaultDecryptionKeys(kOtherAccountKeys, _))
-      .Times(0);
-  EXPECT_CALL(engine_, AddTrustedVaultDecryptionKeys(kSyncingAccountKeys, _));
-  crypto_.AddTrustedVaultDecryptionKeys(kOtherAccount.gaia, kOtherAccountKeys);
-  crypto_.AddTrustedVaultDecryptionKeys(kSyncingAccount.gaia,
-                                        kSyncingAccountKeys);
-}
-
 TEST_F(SyncServiceCryptoTest,
        ShouldReadValidTrustedVaultKeysFromClientBeforeInitialization) {
   const CoreAccountInfo kSyncingAccount =
@@ -173,24 +179,17 @@ TEST_F(SyncServiceCryptoTest,
 
   // OnTrustedVaultKeyRequired() called during initialization of the sync
   // engine (i.e. before SetSyncEngine()).
-  EXPECT_CALL(trusted_vault_client_, FetchKeys(_, _)).Times(0);
   crypto_.OnTrustedVaultKeyRequired();
 
-  base::OnceCallback<void(const std::vector<std::string>&)> fetch_keys_cb;
-  EXPECT_CALL(trusted_vault_client_, FetchKeys(kSyncingAccount.gaia, _))
-      .WillOnce(
-          [&](const std::string& gaia_id,
-              base::OnceCallback<void(const std::vector<std::string>&)> cb) {
-            fetch_keys_cb = std::move(cb);
-          });
+  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kFetchedKeys);
 
   // Trusted vault keys should be fetched only after the engine initialization
   // is completed.
+  ASSERT_THAT(trusted_vault_client_.fetch_count(), Eq(0));
   crypto_.SetSyncEngine(kSyncingAccount, &engine_);
-  VerifyAndClearExpectations();
 
   // While there is an ongoing fetch, there should be no user action required.
-  ASSERT_TRUE(fetch_keys_cb);
+  EXPECT_THAT(trusted_vault_client_.fetch_count(), Eq(1));
   EXPECT_FALSE(crypto_.IsTrustedVaultKeyRequired());
 
   base::OnceClosure add_keys_cb;
@@ -201,7 +200,7 @@ TEST_F(SyncServiceCryptoTest,
           });
 
   // Mimic completion of the fetch.
-  std::move(fetch_keys_cb).Run(kFetchedKeys);
+  ASSERT_TRUE(trusted_vault_client_.CompleteFetchKeysRequest());
   ASSERT_TRUE(add_keys_cb);
   EXPECT_FALSE(crypto_.IsTrustedVaultKeyRequired());
 
@@ -221,21 +220,16 @@ TEST_F(SyncServiceCryptoTest,
   EXPECT_CALL(reconfigure_cb_, Run(_)).Times(0);
   ASSERT_FALSE(crypto_.IsTrustedVaultKeyRequired());
 
-  base::OnceCallback<void(const std::vector<std::string>&)> fetch_keys_cb;
-  EXPECT_CALL(trusted_vault_client_, FetchKeys(kSyncingAccount.gaia, _))
-      .WillOnce(
-          [&](const std::string& gaia_id,
-              base::OnceCallback<void(const std::vector<std::string>&)> cb) {
-            fetch_keys_cb = std::move(cb);
-          });
+  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kFetchedKeys);
 
   // Mimic the engine determining that trusted vault keys are required.
   crypto_.SetSyncEngine(kSyncingAccount, &engine_);
+  ASSERT_THAT(trusted_vault_client_.fetch_count(), Eq(0));
+
   crypto_.OnTrustedVaultKeyRequired();
-  VerifyAndClearExpectations();
 
   // While there is an ongoing fetch, there should be no user action required.
-  ASSERT_TRUE(fetch_keys_cb);
+  EXPECT_THAT(trusted_vault_client_.fetch_count(), Eq(1));
   EXPECT_FALSE(crypto_.IsTrustedVaultKeyRequired());
 
   base::OnceClosure add_keys_cb;
@@ -246,7 +240,7 @@ TEST_F(SyncServiceCryptoTest,
           });
 
   // Mimic completion of the fetch.
-  std::move(fetch_keys_cb).Run(kFetchedKeys);
+  ASSERT_TRUE(trusted_vault_client_.CompleteFetchKeysRequest());
   ASSERT_TRUE(add_keys_cb);
   EXPECT_FALSE(crypto_.IsTrustedVaultKeyRequired());
 
@@ -264,21 +258,16 @@ TEST_F(SyncServiceCryptoTest, ShouldReadInvalidTrustedVaultKeysFromClient) {
 
   ASSERT_FALSE(crypto_.IsTrustedVaultKeyRequired());
 
-  base::OnceCallback<void(const std::vector<std::string>&)> fetch_keys_cb;
-  EXPECT_CALL(trusted_vault_client_, FetchKeys(kSyncingAccount.gaia, _))
-      .WillOnce(
-          [&](const std::string& gaia_id,
-              base::OnceCallback<void(const std::vector<std::string>&)> cb) {
-            fetch_keys_cb = std::move(cb);
-          });
+  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kFetchedKeys);
 
   // Mimic the engine determining that trusted vault keys are required.
   crypto_.SetSyncEngine(kSyncingAccount, &engine_);
+  ASSERT_THAT(trusted_vault_client_.fetch_count(), Eq(0));
+
   crypto_.OnTrustedVaultKeyRequired();
-  VerifyAndClearExpectations();
 
   // While there is an ongoing fetch, there should be no user action required.
-  ASSERT_TRUE(fetch_keys_cb);
+  EXPECT_THAT(trusted_vault_client_.fetch_count(), Eq(1));
   EXPECT_FALSE(crypto_.IsTrustedVaultKeyRequired());
 
   base::OnceClosure add_keys_cb;
@@ -289,7 +278,7 @@ TEST_F(SyncServiceCryptoTest, ShouldReadInvalidTrustedVaultKeysFromClient) {
           });
 
   // Mimic completion of the client.
-  std::move(fetch_keys_cb).Run(kFetchedKeys);
+  ASSERT_TRUE(trusted_vault_client_.CompleteFetchKeysRequest());
   ASSERT_TRUE(add_keys_cb);
   EXPECT_FALSE(crypto_.IsTrustedVaultKeyRequired());
 
@@ -299,6 +288,157 @@ TEST_F(SyncServiceCryptoTest, ShouldReadInvalidTrustedVaultKeysFromClient) {
   EXPECT_TRUE(crypto_.IsTrustedVaultKeyRequired());
 }
 
+// Similar to ShouldReadInvalidTrustedVaultKeysFromClient: the vault
+// initially has no valid keys, leading to IsTrustedVaultKeyRequired().
+// Later, the vault gets populated with the keys, which should trigger
+// a fetch and eventually resolve the encryption issue.
+TEST_F(SyncServiceCryptoTest, ShouldRefetchTrustedVaultKeysWhenChangeObserved) {
+  const CoreAccountInfo kSyncingAccount =
+      MakeAccountInfoWithGaia("syncingaccount");
+  const std::vector<std::string> kInitialKeys = {"key1"};
+  const std::vector<std::string> kNewKeys = {"key1", "key2"};
+
+  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kInitialKeys);
+
+  // The engine replies with OnTrustedVaultKeyAccepted() only if |kNewKeys| are
+  // provided.
+  ON_CALL(engine_, AddTrustedVaultDecryptionKeys(_, _))
+      .WillByDefault(
+          [&](const std::vector<std::string>& keys, base::OnceClosure done_cb) {
+            if (keys == kNewKeys) {
+              crypto_.OnTrustedVaultKeyAccepted();
+            }
+            std::move(done_cb).Run();
+          });
+
+  // Mimic initialization of the engine where trusted vault keys are needed and
+  // |kInitialKeys| are fetched, which are insufficient, and hence
+  // IsTrustedVaultKeyRequired() is exposed.
+  crypto_.SetSyncEngine(kSyncingAccount, &engine_);
+  crypto_.OnTrustedVaultKeyRequired();
+  ASSERT_THAT(trusted_vault_client_.fetch_count(), Eq(1));
+  ASSERT_TRUE(trusted_vault_client_.CompleteFetchKeysRequest());
+  ASSERT_TRUE(crypto_.IsTrustedVaultKeyRequired());
+
+  // Mimic keys being added to the vault, which triggers a notification to
+  // observers (namely |crypto_|), leading to a second fetch.
+  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kNewKeys);
+  EXPECT_THAT(trusted_vault_client_.fetch_count(), Eq(2));
+  EXPECT_CALL(reconfigure_cb_, Run(CONFIGURE_REASON_CRYPTO));
+  EXPECT_TRUE(trusted_vault_client_.CompleteFetchKeysRequest());
+  EXPECT_FALSE(crypto_.IsTrustedVaultKeyRequired());
+}
+
+// Same as above but the new keys become available during an ongoing FetchKeys()
+// request.
+TEST_F(SyncServiceCryptoTest,
+       ShouldDeferTrustedVaultKeyFetchingWhenChangeObservedWhileOngoingFetch) {
+  const CoreAccountInfo kSyncingAccount =
+      MakeAccountInfoWithGaia("syncingaccount");
+  const std::vector<std::string> kInitialKeys = {"key1"};
+  const std::vector<std::string> kNewKeys = {"key1", "key2"};
+
+  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kInitialKeys);
+
+  // The engine replies with OnTrustedVaultKeyAccepted() only if |kNewKeys| are
+  // provided.
+  ON_CALL(engine_, AddTrustedVaultDecryptionKeys(_, _))
+      .WillByDefault(
+          [&](const std::vector<std::string>& keys, base::OnceClosure done_cb) {
+            if (keys == kNewKeys) {
+              crypto_.OnTrustedVaultKeyAccepted();
+            }
+            std::move(done_cb).Run();
+          });
+
+  // Mimic initialization of the engine where trusted vault keys are needed and
+  // |kInitialKeys| are in the process of being fetched.
+  crypto_.SetSyncEngine(kSyncingAccount, &engine_);
+  crypto_.OnTrustedVaultKeyRequired();
+  ASSERT_THAT(trusted_vault_client_.fetch_count(), Eq(1));
+  ASSERT_FALSE(crypto_.IsTrustedVaultKeyRequired());
+
+  // While there is an ongoing fetch, mimic keys being added to the vault, which
+  // triggers a notification to observers (namely |crypto_|).
+  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kNewKeys);
+
+  // Because there's already an ongoing fetch, a second one should not have been
+  // triggered yet and should be deferred instead.
+  EXPECT_THAT(trusted_vault_client_.fetch_count(), Eq(1));
+
+  // As soon as the first fetch completes, the second one (deferred) should be
+  // started.
+  EXPECT_TRUE(trusted_vault_client_.CompleteFetchKeysRequest());
+  EXPECT_THAT(trusted_vault_client_.fetch_count(), Eq(2));
+  EXPECT_FALSE(crypto_.IsTrustedVaultKeyRequired());
+
+  // The completion of the second fetch should resolve the encryption issue.
+  EXPECT_CALL(reconfigure_cb_, Run(CONFIGURE_REASON_CRYPTO));
+  EXPECT_TRUE(trusted_vault_client_.CompleteFetchKeysRequest());
+  EXPECT_THAT(trusted_vault_client_.fetch_count(), Eq(2));
+  EXPECT_FALSE(crypto_.IsTrustedVaultKeyRequired());
+}
+
+// The engine gets initialized and the vault initially has insufficient keys,
+// leading to IsTrustedVaultKeyRequired(). Later, keys are added to the vault
+// *twice*, where the later event should be handled as a deferred fetch.
+TEST_F(
+    SyncServiceCryptoTest,
+    ShouldDeferTrustedVaultKeyFetchingWhenChangeObservedWhileOngoingRefetch) {
+  const CoreAccountInfo kSyncingAccount =
+      MakeAccountInfoWithGaia("syncingaccount");
+  const std::vector<std::string> kInitialKeys = {"key1"};
+  const std::vector<std::string> kIntermediateKeys = {"key1", "key2"};
+  const std::vector<std::string> kLatestKeys = {"key1", "key2", "key3"};
+
+  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kInitialKeys);
+
+  // The engine replies with OnTrustedVaultKeyAccepted() only if |kLatestKeys|
+  // are provided.
+  ON_CALL(engine_, AddTrustedVaultDecryptionKeys(_, _))
+      .WillByDefault(
+          [&](const std::vector<std::string>& keys, base::OnceClosure done_cb) {
+            if (keys == kLatestKeys) {
+              crypto_.OnTrustedVaultKeyAccepted();
+            }
+            std::move(done_cb).Run();
+          });
+
+  // Mimic initialization of the engine where trusted vault keys are needed and
+  // |kInitialKeys| are fetched, which are insufficient, and hence
+  // IsTrustedVaultKeyRequired() is exposed.
+  crypto_.SetSyncEngine(kSyncingAccount, &engine_);
+  crypto_.OnTrustedVaultKeyRequired();
+  ASSERT_THAT(trusted_vault_client_.fetch_count(), Eq(1));
+  ASSERT_TRUE(trusted_vault_client_.CompleteFetchKeysRequest());
+  ASSERT_TRUE(crypto_.IsTrustedVaultKeyRequired());
+
+  // Mimic keys being added to the vault, which triggers a notification to
+  // observers (namely |crypto_|), leading to a second fetch.
+  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kIntermediateKeys);
+  EXPECT_THAT(trusted_vault_client_.fetch_count(), Eq(2));
+
+  // While the second fetch is ongoing, mimic more keys being added to the
+  // vault, which triggers a notification to observers (namely |crypto_|).
+  trusted_vault_client_.StoreKeys(kSyncingAccount.gaia, kLatestKeys);
+
+  // Because there's already an ongoing fetch, a third one should not have been
+  // triggered yet and should be deferred instead.
+  EXPECT_THAT(trusted_vault_client_.fetch_count(), Eq(2));
+
+  // As soon as the second fetch completes, the third one (deferred) should be
+  // started.
+  EXPECT_TRUE(trusted_vault_client_.CompleteFetchKeysRequest());
+  EXPECT_THAT(trusted_vault_client_.fetch_count(), Eq(3));
+  EXPECT_TRUE(crypto_.IsTrustedVaultKeyRequired());
+
+  // The completion of the third fetch should resolve the encryption issue.
+  EXPECT_CALL(reconfigure_cb_, Run(CONFIGURE_REASON_CRYPTO));
+  EXPECT_TRUE(trusted_vault_client_.CompleteFetchKeysRequest());
+  EXPECT_THAT(trusted_vault_client_.fetch_count(), Eq(3));
+  EXPECT_FALSE(crypto_.IsTrustedVaultKeyRequired());
+}
+
 }  // namespace
 
 }  // namespace syncer

components/sync/driver/sync_user_settings.h

@@ -6,7 +6,6 @@
 #define COMPONENTS_SYNC_DRIVER_SYNC_USER_SETTINGS_H_
 
 #include <string>
-#include <vector>
 
 #include "base/compiler_specific.h"
 #include "base/time/time.h"
@@ -124,12 +123,6 @@ class SyncUserSettings {
   // copy of encrypted keys; returns true otherwise.
   virtual bool SetDecryptionPassphrase(const std::string& passphrase)
       WARN_UNUSED_RESULT = 0;
-  // Analogous to SetDecryptionPassphrase but specifically for
-  // TRUSTED_VAULT_PASSPHRASE: it provides new decryption keys that could
-  // allow decrypting pending Nigori keys.
-  virtual void AddTrustedVaultDecryptionKeys(
-      const std::string& gaia_id,
-      const std::vector<std::string>& keys) = 0;
 };
 
 }  // namespace syncer

components/sync/driver/sync_user_settings_impl.cc

@@ -224,13 +224,6 @@ bool SyncUserSettingsImpl::SetDecryptionPassphrase(
   return result;
 }
 
-void SyncUserSettingsImpl::AddTrustedVaultDecryptionKeys(
-    const std::string& gaia_id,
-    const std::vector<std::string>& keys) {
-  DVLOG(1) << "Adding trusted vault decryption keys.";
-  crypto_->AddTrustedVaultDecryptionKeys(gaia_id, keys);
-}
-
 void SyncUserSettingsImpl::SetSyncRequestedIfNotSetExplicitly() {
   prefs_->SetSyncRequestedIfNotSetExplicitly();
 }

components/sync/driver/sync_user_settings_impl.h

@@ -6,7 +6,6 @@
 #define COMPONENTS_SYNC_DRIVER_SYNC_USER_SETTINGS_IMPL_H_
 
 #include <string>
-#include <vector>
 
 #include "base/callback.h"
 #include "components/sync/base/model_type.h"
@@ -72,9 +71,6 @@ class SyncUserSettingsImpl : public SyncUserSettings {
 
   void SetEncryptionPassphrase(const std::string& passphrase) override;
   bool SetDecryptionPassphrase(const std::string& passphrase) override;
-  void AddTrustedVaultDecryptionKeys(
-      const std::string& gaia_id,
-      const std::vector<std::string>& keys) override;
 
   void SetSyncRequestedIfNotSetExplicitly();
 

components/sync/driver/sync_user_settings_mock.h

@@ -56,8 +56,6 @@ class SyncUserSettingsMock : public SyncUserSettings {
 
   MOCK_METHOD1(SetEncryptionPassphrase, void(const std::string&));
   MOCK_METHOD1(SetDecryptionPassphrase, bool(const std::string&));
-  MOCK_METHOD2(AddTrustedVaultDecryptionKeys,
-               void(const std::string&, const std::vector<std::string>&));
 };
 
 }  // namespace syncer

components/sync/driver/test_sync_service.cc

@@ -304,6 +304,10 @@ void TestSyncService::GetAllNodesForDebugging(
 
 void TestSyncService::SetInvalidationsForSessionsEnabled(bool enabled) {}
 
+void TestSyncService::AddTrustedVaultDecryptionKeysFromWeb(
+    const std::string& gaia_id,
+    const std::vector<std::string>& keys) {}
+
 UserDemographicsResult TestSyncService::GetUserNoisedBirthYearAndGender(
     base::Time now) {
   return user_demographics_result_;

components/sync/driver/test_sync_service.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 #include <string>
+#include <vector>
 
 #include "base/macros.h"
 #include "base/observer_list.h"
@@ -42,7 +43,6 @@ class TestSyncService : public SyncService {
       const UserDemographicsResult& user_demographics_result);
   void SetExperimentalAuthenticationKey(
       std::unique_ptr<crypto::ECPrivateKey> experimental_authentication_key);
-
   // Convenience versions of the above, for when the caller doesn't care about
   // the particular values in the snapshot, just whether there is one.
   void SetEmptyLastCycleSnapshot();
@@ -106,6 +106,9 @@ class TestSyncService : public SyncService {
       base::OnceCallback<void(std::unique_ptr<base::ListValue>)> callback)
       override;
   void SetInvalidationsForSessionsEnabled(bool enabled) override;
+  void AddTrustedVaultDecryptionKeysFromWeb(
+      const std::string& gaia_id,
+      const std::vector<std::string>& keys) override;
   UserDemographicsResult GetUserNoisedBirthYearAndGender(
       base::Time now) override;
 

components/sync/driver/test_sync_user_settings.cc

@@ -196,10 +196,6 @@ bool TestSyncUserSettings::SetDecryptionPassphrase(
   return false;
 }
 
-void TestSyncUserSettings::AddTrustedVaultDecryptionKeys(
-    const std::string& gaia_id,
-    const std::vector<std::string>& keys) {}
-
 void TestSyncUserSettings::SetFirstSetupComplete() {
   first_setup_complete_ = true;
 }

components/sync/driver/test_sync_user_settings.h

@@ -6,7 +6,6 @@
 #define COMPONENTS_SYNC_DRIVER_TEST_SYNC_USER_SETTINGS_H_
 
 #include <string>
-#include <vector>
 
 #include "components/sync/driver/sync_user_settings.h"
 
@@ -61,9 +60,6 @@ class TestSyncUserSettings : public SyncUserSettings {
 
   void SetEncryptionPassphrase(const std::string& passphrase) override;
   bool SetDecryptionPassphrase(const std::string& passphrase) override;
-  void AddTrustedVaultDecryptionKeys(
-      const std::string& gaia_id,
-      const std::vector<std::string>& keys) override;
 
   void SetFirstSetupComplete();
   void ClearFirstSetupComplete();

components/sync/driver/trusted_vault_client.h

@@ -5,10 +5,12 @@
 #ifndef COMPONENTS_SYNC_DRIVER_TRUSTED_VAULT_CLIENT_H_
 #define COMPONENTS_SYNC_DRIVER_TRUSTED_VAULT_CLIENT_H_
 
+#include <memory>
 #include <string>
 #include <vector>
 
 #include "base/callback_forward.h"
+#include "base/callback_list.h"
 #include "base/macros.h"
 
 namespace syncer {
@@ -20,9 +22,20 @@ class TrustedVaultClient {
   TrustedVaultClient() = default;
   virtual ~TrustedVaultClient() = default;
 
+  using CallbackList = base::CallbackList<void()>;
+  using Subscription = CallbackList::Subscription;
+
+  // Registers an observer-like callback that will be invoked when the content
+  // of the vault has changed (e.g. new keys added). The subscription must not
+  // outlive |*this|.
+  virtual std::unique_ptr<Subscription> AddKeysChangedObserver(
+      const base::RepeatingClosure& cb) = 0;
+
   // Attempts to fetch decryption keys, required by sync to resume.
   // Implementations are expected to NOT prompt the user for actions. |cb| is
   // called on completion with known keys or an empty list if none known.
+  // Concurrent calls to FetchKeys() must not be issued since implementations
+  // may not support them.
   virtual void FetchKeys(
       const std::string& gaia_id,
       base::OnceCallback<void(const std::vector<std::string>&)> cb) = 0;