[Sync:USS] Expose Nigori-related interfaces via SyncEncryptionHandler

In order to simplify initialization of SyncEncryptionHandler and its
dependencies and upcoming transfer of SyncEncryptionHandler ownership,
SynEncryptionHandler now exposes Cryptographer, NigoriHandler and
KeystoreKeysHandler. USS implementation of SyncEncryptionHandler (aka
NigoriSyncBridgeImpl) returns nullptrs as Cryptographer and
NigoriHandler, since they are needed only in Directory world.

Bug: 922900
Change-Id: I519d4f682ab990fdefadfdfedda713df14fea2a0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1645779
Commit-Queue: Maksim Moskvitin <mmoskvitin@google.com>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#667094}

components/sync/engine/sync_encryption_handler.h

@@ -16,8 +16,15 @@
 namespace syncer {
 
 class Cryptographer;
+class KeystoreKeysHandler;
 enum class PassphraseType;
 
+namespace syncable {
+
+class NigoriHandler;
+
+}  // namespace syncable
+
 // Reasons due to which Cryptographer might require a passphrase.
 enum PassphraseRequiredReason {
   REASON_PASSPHRASE_NOT_REQUIRED = 0,  // Initial value.
@@ -173,6 +180,24 @@ class SyncEncryptionHandler {
   // base::Time() in case migration isn't completed.
   virtual base::Time GetKeystoreMigrationTime() const = 0;
 
+  // Unsafe getter. Use only if sync is not up and running and there is no risk
+  // of other threads calling this. Returns the original cryptographer. This
+  // Cryptographer will always reflect the actual state of
+  // SyncEncryptionHandler (no needs to call this method again in case
+  // cryptographer state was changed).
+  // TODO(crbug.com/970213): remove this method or replace with normal getter
+  // for const Cryptographer.
+  virtual Cryptographer* GetCryptographerUnsafe() = 0;
+
+  // Returns KeystoreKeysHandler, allowing to pass new keystore keys and to
+  // check whether keystore keys need to be requested from the server.
+  virtual KeystoreKeysHandler* GetKeystoreKeysHandler() = 0;
+
+  // Returns NigoriHandler, allowing directory-specific interaction with
+  // Nigori. Returns nullptr iff USS implementation of Nigori is enabled.
+  // TODO(crbug.com/970213): remove this method.
+  virtual syncable::NigoriHandler* GetNigoriHandler() = 0;
+
   // The set of types that are always encrypted.
   static ModelTypeSet SensitiveTypes();
 };

components/sync/engine_impl/sync_encryption_handler_impl.cc

@@ -743,6 +743,21 @@ base::Time SyncEncryptionHandlerImpl::GetKeystoreMigrationTime() const {
   return keystore_migration_time_;
 }
 
+Cryptographer* SyncEncryptionHandlerImpl::GetCryptographerUnsafe() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  return &vault_unsafe_.cryptographer;
+}
+
+KeystoreKeysHandler* SyncEncryptionHandlerImpl::GetKeystoreKeysHandler() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  return this;
+}
+
+syncable::NigoriHandler* SyncEncryptionHandlerImpl::GetNigoriHandler() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  return this;
+}
+
 // Note: this is called from within a syncable transaction, so we need to post
 // tasks if we want to do any work that creates a new sync_api transaction.
 void SyncEncryptionHandlerImpl::ApplyNigoriUpdate(
@@ -867,11 +882,6 @@ PassphraseType SyncEncryptionHandlerImpl::GetPassphraseType(
   return UnlockVault(trans).passphrase_type;
 }
 
-Cryptographer* SyncEncryptionHandlerImpl::GetCryptographerUnsafe() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return &vault_unsafe_.cryptographer;
-}
-
 ModelTypeSet SyncEncryptionHandlerImpl::GetEncryptedTypesUnsafe() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return vault_unsafe_.encrypted_types;

components/sync/engine_impl/sync_encryption_handler_impl.h

@@ -69,6 +69,9 @@ class SyncEncryptionHandlerImpl : public KeystoreKeysHandler,
   void EnableEncryptEverything() override;
   bool IsEncryptEverythingEnabled() const override;
   base::Time GetKeystoreMigrationTime() const override;
+  Cryptographer* GetCryptographerUnsafe() override;
+  KeystoreKeysHandler* GetKeystoreKeysHandler() override;
+  syncable::NigoriHandler* GetNigoriHandler() override;
 
   // NigoriHandler implementation.
   // Note: all methods are invoked while the caller holds a transaction.
@@ -89,7 +92,7 @@ class SyncEncryptionHandlerImpl : public KeystoreKeysHandler,
 
   // Unsafe getters. Use only if sync is not up and running and there is no risk
   // of other threads calling this.
-  Cryptographer* GetCryptographerUnsafe();
+
   ModelTypeSet GetEncryptedTypesUnsafe();
 
   bool MigratedToKeystore();

components/sync/engine_impl/sync_manager_impl.cc

@@ -299,25 +299,14 @@ void SyncManagerImpl::Init(InitArgs* args) {
   allstatus_.SetHasKeystoreKey(
       !args->restored_keystore_key_for_bootstrapping.empty());
 
-  Cryptographer* cryptographer_for_directory = nullptr;
-  syncable::NigoriHandler* nigori_handler = nullptr;
-  KeystoreKeysHandler* keystore_keys_handler = nullptr;
   if (base::FeatureList::IsEnabled(switches::kSyncUSSNigori)) {
-    auto nigori_sync_bridge_impl = std::make_unique<NigoriSyncBridgeImpl>(
+    sync_encryption_handler_ = std::make_unique<NigoriSyncBridgeImpl>(
         std::make_unique<NigoriModelTypeProcessor>(), args->encryptor);
-    keystore_keys_handler = nigori_sync_bridge_impl.get();
-    sync_encryption_handler_ = std::move(nigori_sync_bridge_impl);
   } else {
-    auto sync_encryption_handler_impl =
-        std::make_unique<SyncEncryptionHandlerImpl>(
-            &share_, args->encryptor, args->restored_key_for_bootstrapping,
-            args->restored_keystore_key_for_bootstrapping,
-            base::BindRepeating(&Nigori::GenerateScryptSalt));
-    cryptographer_for_directory =
-        sync_encryption_handler_impl->GetCryptographerUnsafe();
-    nigori_handler = sync_encryption_handler_impl.get();
-    keystore_keys_handler = sync_encryption_handler_impl.get();
-    sync_encryption_handler_ = std::move(sync_encryption_handler_impl);
+    sync_encryption_handler_ = std::make_unique<SyncEncryptionHandlerImpl>(
+        &share_, args->encryptor, args->restored_key_for_bootstrapping,
+        args->restored_keystore_key_for_bootstrapping,
+        base::BindRepeating(&Nigori::GenerateScryptSalt));
   }
 
   // Register for encryption related changes now. We have to do this before
@@ -356,12 +345,13 @@ void SyncManagerImpl::Init(InitArgs* args) {
 
   DCHECK(backing_store);
 
-  // Note: |nigori_handler| and |cryptographer_for_directory| are nullptrs iff
-  // kSyncUSSNigori is enabled.
+  // Note: NigoriHandler and Cryptographer passed to Directory are nullptrs iff
+  // USS implementation of Nigori is enabled.
   share_.directory = std::make_unique<syncable::Directory>(
       std::move(backing_store), args->unrecoverable_error_handler,
-      report_unrecoverable_error_function_, nigori_handler,
-      cryptographer_for_directory);
+      report_unrecoverable_error_function_,
+      sync_encryption_handler_->GetNigoriHandler(),
+      sync_encryption_handler_->GetCryptographerUnsafe());
 
   DVLOG(1) << "AccountId: " << args->authenticated_account_id;
   if (!OpenDirectory(args)) {
@@ -395,7 +385,8 @@ void SyncManagerImpl::Init(InitArgs* args) {
 
   model_type_registry_ = std::make_unique<ModelTypeRegistry>(
       args->workers, &share_, this, base::Bind(&MigrateDirectoryData),
-      args->cancelation_signal, keystore_keys_handler);
+      args->cancelation_signal,
+      sync_encryption_handler_->GetKeystoreKeysHandler());
   sync_encryption_handler_->AddObserver(model_type_registry_.get());
 
   // Build a SyncCycleContext and store the worker in it.

components/sync/nigori/nigori_sync_bridge_impl.cc

@@ -314,6 +314,25 @@ base::Time NigoriSyncBridgeImpl::GetKeystoreMigrationTime() const {
   return base::Time();
 }
 
+Cryptographer* NigoriSyncBridgeImpl::GetCryptographerUnsafe() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  // This method exposes Cryptographer to Directory, and it's redundant in case
+  // USS implementation is enabled.
+  return nullptr;
+}
+
+KeystoreKeysHandler* NigoriSyncBridgeImpl::GetKeystoreKeysHandler() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  return this;
+}
+
+syncable::NigoriHandler* NigoriSyncBridgeImpl::GetNigoriHandler() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  // Note: GetNigoriHandler() is a workaround for coexistence with Directory
+  // implementation, returning nullptr here is expected behavior.
+  return nullptr;
+}
+
 bool NigoriSyncBridgeImpl::NeedKeystoreKey() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   // We explicitly ask the server for keystore keys iff it's first-time sync,

components/sync/nigori/nigori_sync_bridge_impl.h

@@ -53,6 +53,9 @@ class NigoriSyncBridgeImpl : public KeystoreKeysHandler,
   void EnableEncryptEverything() override;
   bool IsEncryptEverythingEnabled() const override;
   base::Time GetKeystoreMigrationTime() const override;
+  Cryptographer* GetCryptographerUnsafe() override;
+  KeystoreKeysHandler* GetKeystoreKeysHandler() override;
+  syncable::NigoriHandler* GetNigoriHandler() override;
 
   // KeystoreKeysHandler implementation.
   bool NeedKeystoreKey() const override;

components/sync/test/engine/test_directory_setter_upper.cc

@@ -35,7 +35,7 @@ void TestDirectorySetterUpper::SetUp() {
           name_, base::BindRepeating(
                      []() -> std::string { return "kTestCacheGuid"; })),
       MakeWeakHandle(handler_.GetWeakPtr()), base::Closure(),
-      &encryption_handler_, encryption_handler_.cryptographer());
+      &encryption_handler_, encryption_handler_.GetCryptographerUnsafe());
   ASSERT_EQ(syncable::OPENED_NEW,
             directory_->Open(name_, &delegate_, transaction_observer));
   directory_->set_cache_guid("kTestCacheGuid");
@@ -52,7 +52,7 @@ void TestDirectorySetterUpper::SetUpWith(
   directory_ = std::make_unique<syncable::Directory>(
       std::move(directory_store), MakeWeakHandle(handler_.GetWeakPtr()),
       base::Closure(), &encryption_handler_,
-      encryption_handler_.cryptographer());
+      encryption_handler_.GetCryptographerUnsafe());
   ASSERT_EQ(syncable::OPENED_EXISTING,
             directory_->Open(name_, &delegate_, transaction_observer));
   directory_->set_cache_guid("kTestCacheGuid");

components/sync/test/fake_sync_encryption_handler.cc

@@ -128,4 +128,16 @@ base::Time FakeSyncEncryptionHandler::GetKeystoreMigrationTime() const {
   return base::Time();
 }
 
+Cryptographer* FakeSyncEncryptionHandler::GetCryptographerUnsafe() {
+  return &cryptographer_;
+}
+
+KeystoreKeysHandler* FakeSyncEncryptionHandler::GetKeystoreKeysHandler() {
+  return this;
+}
+
+syncable::NigoriHandler* FakeSyncEncryptionHandler::GetNigoriHandler() {
+  return this;
+}
+
 }  // namespace syncer

components/sync/test/fake_sync_encryption_handler.h

@@ -43,6 +43,9 @@ class FakeSyncEncryptionHandler : public KeystoreKeysHandler,
   PassphraseType GetPassphraseType(
       syncable::BaseTransaction* const trans) const override;
   base::Time GetKeystoreMigrationTime() const override;
+  Cryptographer* GetCryptographerUnsafe() override;
+  KeystoreKeysHandler* GetKeystoreKeysHandler() override;
+  syncable::NigoriHandler* GetNigoriHandler() override;
 
   // NigoriHandler implemenation.
   void ApplyNigoriUpdate(const sync_pb::NigoriSpecifics& nigori,
@@ -57,8 +60,6 @@ class FakeSyncEncryptionHandler : public KeystoreKeysHandler,
   bool NeedKeystoreKey() const override;
   bool SetKeystoreKeys(const std::vector<std::string>& keys) override;
 
-  Cryptographer* cryptographer() { return &cryptographer_; }
-
  private:
   base::ObserverList<SyncEncryptionHandler::Observer>::Unchecked observers_;
   ModelTypeSet encrypted_types_;