Adopt new commit infrastructure for keystore Nigori initialization

The newly introduced mechanism around PendingLocalNigoriCommit allows
implementing NigoriSyncBridgeImpl::MergeSyncData(),
responsible for keystore Nigori initialization (in case it receives
a default NigoriSpecifics), in a way that:
a) Success is only reported when the sync server acks the commit.
b) Conflict-resolution becomes simple.
c) Logic is factored out from NigoriSyncBridgeImpl.

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

components/sync/nigori/nigori_local_change_processor.h

@@ -69,6 +69,12 @@ class NigoriLocalChangeProcessor {
   virtual base::WeakPtr<ModelTypeControllerDelegate>
   GetControllerDelegate() = 0;
 
+  // Returns a boolean representing whether the processor's metadata is
+  // currently up to date and accurately tracking the model type's data. If
+  // false, and ModelReadyToSync() has already been called, then Put and Delete
+  // will no-op and can be omitted by bridge.
+  virtual bool IsTrackingMetadata() = 0;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(NigoriLocalChangeProcessor);
 };

components/sync/nigori/nigori_model_type_processor.h

@@ -54,12 +54,11 @@ class NigoriModelTypeProcessor : public ModelTypeProcessor,
   NigoriMetadataBatch GetMetadata() override;
   void ReportError(const ModelError& error) override;
   base::WeakPtr<ModelTypeControllerDelegate> GetControllerDelegate() override;
+  bool IsTrackingMetadata() override;
 
   bool IsConnectedForTest() const;
 
  private:
-  bool IsTrackingMetadata();
-
   // Returns true if the handshake with sync thread is complete.
   bool IsConnected() const;
 

components/sync/nigori/nigori_sync_bridge_impl.cc

@@ -33,29 +33,6 @@ using sync_pb::NigoriSpecifics;
 
 const char kNigoriNonUniqueName[] = "Nigori";
 
-// Creates keystore Nigori specifics given |keystore_keys_cryptographer|.
-// Returns NigoriSpecifics that contain:
-// 1. passphrase_type = KEYSTORE_PASSPHRASE.
-// 2. encryption_keybag contains all keystore keys and encrypted with the
-// latest keystore key.
-// 3. keystore_decryptor_token contains latest keystore key encrypted with
-// itself.
-// 4. keybag_is_frozen = true.
-// 5. keystore_migration_time is current time.
-// 6. Other fields are default.
-NigoriSpecifics MakeDefaultKeystoreNigori(
-    const KeystoreKeysCryptographer& keystore_keys_cryptographer) {
-  DCHECK(!keystore_keys_cryptographer.IsEmpty());
-
-  NigoriState state;
-  state.passphrase_type = NigoriSpecifics::KEYSTORE_PASSPHRASE;
-  state.keystore_migration_time = base::Time::Now();
-  state.cryptographer = keystore_keys_cryptographer.ToCryptographerImpl();
-  state.keystore_keys_cryptographer = keystore_keys_cryptographer.Clone();
-
-  return state.ToSpecificsProto();
-}
-
 KeyDerivationMethod GetKeyDerivationMethodFromSpecifics(
     const sync_pb::NigoriSpecifics& specifics) {
   KeyDerivationMethod key_derivation_method = ProtoKeyDerivationMethodToEnum(
@@ -175,8 +152,9 @@ bool IsValidPassphraseTransition(
   }
   switch (old_passphrase_type) {
     case NigoriSpecifics::UNKNOWN:
-      // This can happen iff we have not synced local state yet, so we accept
-      // any valid passphrase type (invalid filtered before).
+      // This can happen iff we have not synced local state yet or synced with
+      // default NigoriSpecifics, so we accept any valid passphrase type
+      // (invalid filtered before).
     case NigoriSpecifics::IMPLICIT_PASSPHRASE:
       return true;
     case NigoriSpecifics::KEYSTORE_PASSPHRASE:
@@ -700,20 +678,14 @@ base::Optional<ModelError> NigoriSyncBridgeImpl::MergeSyncData(
   }
   // We received uninitialized Nigori and need to initialize it as default
   // keystore Nigori.
-  // TODO(crbug.com/922900): Adopt QueuePendingLocalCommit().
-  NigoriSpecifics initialized_specifics =
-      MakeDefaultKeystoreNigori(*state_.keystore_keys_cryptographer);
-  DCHECK(IsValidNigoriSpecifics(initialized_specifics));
-
-  *data->specifics.mutable_nigori() = initialized_specifics;
-  processor_->Put(std::make_unique<EntityData>(std::move(*data)));
-  return UpdateLocalState(initialized_specifics);
+  QueuePendingLocalCommit(
+      PendingLocalNigoriCommit::ForKeystoreInitialization());
+  return base::nullopt;
 }
 
 base::Optional<ModelError> NigoriSyncBridgeImpl::ApplySyncChanges(
     base::Optional<EntityData> data) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK_NE(state_.passphrase_type, NigoriSpecifics::UNKNOWN);
 
   if (data) {
     DCHECK(data->specifics.has_nigori());
@@ -1126,11 +1098,7 @@ sync_pb::NigoriLocalData NigoriSyncBridgeImpl::SerializeAsNigoriLocalData()
 
 void NigoriSyncBridgeImpl::QueuePendingLocalCommit(
     std::unique_ptr<PendingLocalNigoriCommit> local_commit) {
-  NigoriState tmp_state = state_.Clone();
-  if (state_.passphrase_type == NigoriSpecifics::UNKNOWN) {
-    local_commit->OnFailure(broadcasting_observer_.get());
-    return;
-  }
+  DCHECK(processor_->IsTrackingMetadata());
 
   pending_local_commit_queue_.push_back(std::move(local_commit));
 

components/sync/nigori/nigori_sync_bridge_impl_unittest.cc

@@ -297,6 +297,7 @@ class MockNigoriLocalChangeProcessor : public NigoriLocalChangeProcessor {
   MOCK_METHOD1(ReportError, void(const ModelError&));
   MOCK_METHOD0(GetControllerDelegate,
                base::WeakPtr<ModelTypeControllerDelegate>());
+  MOCK_METHOD0(IsTrackingMetadata, bool());
 };
 
 class MockObserver : public SyncEncryptionHandler::Observer {
@@ -340,6 +341,7 @@ class NigoriSyncBridgeImplTest : public testing::Test {
 
     auto processor =
         std::make_unique<testing::NiceMock<MockNigoriLocalChangeProcessor>>();
+    ON_CALL(*processor, IsTrackingMetadata()).WillByDefault(Return(true));
     processor_ = processor.get();
     auto storage = std::make_unique<testing::NiceMock<MockNigoriStorage>>();
     storage_ = storage.get();
@@ -591,10 +593,15 @@ TEST_F(NigoriSyncBridgeImplTest,
 
   // We don't verify entire NigoriSpecifics here, because it requires too
   // complex matcher (NigoriSpecifics is not determenistic).
+  // Calling MergeSyncData() triggers a commit cycle but doesn't immediately
+  // expose the new state, until the commit completes.
   EXPECT_CALL(*processor(), Put(HasKeystoreNigori()));
   EXPECT_THAT(bridge()->MergeSyncData(std::move(default_entity_data)),
               Eq(base::nullopt));
   EXPECT_THAT(bridge()->GetData(), HasKeystoreNigori());
+
+  EXPECT_THAT(bridge()->ApplySyncChanges(base::nullopt), Eq(base::nullopt));
+  EXPECT_THAT(bridge()->GetData(), HasKeystoreNigori());
   EXPECT_THAT(bridge()->GetKeystoreMigrationTime(), Not(NullTime()));
   EXPECT_EQ(bridge()->GetPassphraseTypeForTesting(),
             sync_pb::NigoriSpecifics::KEYSTORE_PASSPHRASE);
@@ -887,6 +894,7 @@ TEST_F(NigoriSyncBridgeImplTest,
   ASSERT_THAT(bridge()->MergeSyncData(std::move(default_entity_data)),
               Eq(base::nullopt));
   ASSERT_THAT(bridge()->GetData(), Not(HasCustomPassphraseNigori()));
+  EXPECT_THAT(bridge()->ApplySyncChanges(base::nullopt), Eq(base::nullopt));
 
   // Calling SetEncryptionPassphrase() triggers a commit cycle but doesn't
   // immediately expose the new state, until the commit completes.

components/sync/nigori/pending_local_nigori_commit.cc

@@ -10,6 +10,7 @@
 #include "components/sync/base/sync_base_switches.h"
 #include "components/sync/engine/sync_engine_switches.h"
 #include "components/sync/nigori/cryptographer_impl.h"
+#include "components/sync/nigori/keystore_keys_cryptographer.h"
 #include "components/sync/nigori/nigori_state.h"
 
 namespace syncer {
@@ -129,6 +130,39 @@ class CustomPassphraseSetter : public PendingLocalNigoriCommit {
   DISALLOW_COPY_AND_ASSIGN(CustomPassphraseSetter);
 };
 
+class KeystoreInitializer : public PendingLocalNigoriCommit {
+ public:
+  KeystoreInitializer() = default;
+  ~KeystoreInitializer() override = default;
+
+  bool TryApply(NigoriState* state) const override {
+    DCHECK(!state->keystore_keys_cryptographer->IsEmpty());
+    if (state->passphrase_type != NigoriSpecifics::UNKNOWN) {
+      return false;
+    }
+
+    state->passphrase_type = NigoriSpecifics::KEYSTORE_PASSPHRASE;
+    state->keystore_migration_time = base::Time::Now();
+    state->cryptographer =
+        state->keystore_keys_cryptographer->ToCryptographerImpl();
+    return true;
+  }
+
+  void OnSuccess(const NigoriState& state,
+                 SyncEncryptionHandler::Observer* observer) override {
+    // Note: |passphrase_time| isn't populated for keystore passphrase.
+    observer->OnPassphraseTypeChanged(PassphraseType::kKeystorePassphrase,
+                                      /*passphrase_time=*/base::Time());
+    observer->OnCryptographerStateChanged(state.cryptographer.get(),
+                                          /*has_pending_keys=*/false);
+  }
+
+  void OnFailure(SyncEncryptionHandler::Observer* observer) override {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(KeystoreInitializer);
+};
+
 }  // namespace
 
 // static
@@ -140,4 +174,10 @@ PendingLocalNigoriCommit::ForSetCustomPassphrase(
                                                   random_salt_generator);
 }
 
+// static
+std::unique_ptr<PendingLocalNigoriCommit>
+PendingLocalNigoriCommit::ForKeystoreInitialization() {
+  return std::make_unique<KeystoreInitializer>();
+}
+
 }  // namespace syncer

components/sync/nigori/pending_local_nigori_commit.h

@@ -24,6 +24,8 @@ class PendingLocalNigoriCommit {
       const std::string& passphrase,
       const base::RepeatingCallback<std::string()>& random_salt_generator);
 
+  static std::unique_ptr<PendingLocalNigoriCommit> ForKeystoreInitialization();
+
   PendingLocalNigoriCommit() = default;
   virtual ~PendingLocalNigoriCommit() = default;