Support conflict resolution for Nigori

From processor perspective, we always choose remote update in case of
conflict. This should work, because bridge aware of not-commited local
changes. Pending local commits reapplied on top of incoming changes
after processing conflicting remote update as regular update.

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

components/sync/nigori/DEPS

 include_rules = [
   "+components/sync/base",
   "+components/sync/engine",
+  "+components/sync/engine_impl",
   "+components/sync/model",
   "+components/sync/model_impl",
   "+components/sync/protocol",

components/sync/nigori/nigori_model_type_processor.cc

@@ -4,11 +4,13 @@
 
 #include "components/sync/nigori/nigori_model_type_processor.h"
 
+#include "base/metrics/histogram_macros.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "components/sync/base/client_tag_hash.h"
 #include "components/sync/base/data_type_histogram.h"
 #include "components/sync/base/time.h"
 #include "components/sync/engine/commit_queue.h"
+#include "components/sync/engine_impl/conflict_resolver.h"
 #include "components/sync/model_impl/processor_entity.h"
 #include "components/sync/nigori/forwarding_model_type_processor.h"
 #include "components/sync/nigori/nigori_sync_bridge.h"
@@ -160,8 +162,16 @@ void NigoriModelTypeProcessor::OnUpdateReceived(
   }
 
   if (entity_->IsUnsynced()) {
-    // TODO(mamir): conflict resolution
-    NOTIMPLEMENTED();
+    // Remote update always win in case of conflict, because bridge takes care
+    // of reapplying pending local changes after processing the remote update.
+    entity_->RecordForcedUpdate(*updates[0]);
+    error = bridge_->ApplySyncChanges(std::move(*updates[0]->entity));
+    UMA_HISTOGRAM_ENUMERATION("Sync.ResolveConflict",
+                              ConflictResolution::kUseRemote,
+                              ConflictResolution::kTypeSize);
+    UMA_HISTOGRAM_ENUMERATION("Sync.ResolveSimpleConflict",
+                              ConflictResolver::NIGORI_MERGE,
+                              ConflictResolver::CONFLICT_RESOLUTION_SIZE);
   } else if (!entity_->MatchesData(*updates[0]->entity)) {
     // Inform the bridge of the new or updated data.
     entity_->RecordAcceptedUpdate(*updates[0]);

components/sync/nigori/nigori_sync_bridge.h

@@ -36,11 +36,6 @@ class NigoriSyncBridge {
   // Retrieve Nigori sync data.
   virtual std::unique_ptr<EntityData> GetData() = 0;
 
-  // Resolve a conflict between the client and server versions of data. They are
-  // guaranteed not to match (both be deleted or have identical specifics).
-  virtual ConflictResolution ResolveConflict(const EntityData& local_data,
-                                             const EntityData& remote_data) = 0;
-
   // Informs the bridge that sync has been disabed. The bridge is responsible
   // for deleting all data and metadata upon disabling sync.
   virtual void ApplyDisableSyncChanges() = 0;

components/sync/nigori/nigori_sync_bridge_impl.cc

@@ -794,14 +794,9 @@ base::Optional<ModelError> NigoriSyncBridgeImpl::UpdateLocalState(
 
   MaybeNotifyOfPendingKeys();
 
-  // TODO(crbug.com/): Instead of issuing errors for local commits, call
-  // PutNextApplicablePendingLocalCommit() to verify pending local changes
-  // (relevant for the conflict case). Issue failures if they are no longer
-  // applicable, or call Put() otherwise.
-  for (const auto& pending_local_commit : pending_local_commit_queue_) {
-    pending_local_commit->OnFailure(broadcasting_observer_.get());
-  }
-  pending_local_commit_queue_.clear();
+  // There might be pending local commits, so make attempt to apply them on top
+  // of new |state_|.
+  PutNextApplicablePendingLocalCommit();
 
   storage_->StoreData(SerializeAsNigoriLocalData());
 
@@ -937,14 +932,6 @@ std::unique_ptr<EntityData> NigoriSyncBridgeImpl::GetData() {
   return entity_data;
 }
 
-ConflictResolution NigoriSyncBridgeImpl::ResolveConflict(
-    const EntityData& local_data,
-    const EntityData& remote_data) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  NOTIMPLEMENTED();
-  return ConflictResolution::kUseLocal;
-}
-
 void NigoriSyncBridgeImpl::ApplyDisableSyncChanges() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   // The user intended to disable sync, so we need to clear all the data, except

components/sync/nigori/nigori_sync_bridge_impl.h

@@ -80,8 +80,6 @@ class NigoriSyncBridgeImpl : public KeystoreKeysHandler,
   base::Optional<ModelError> ApplySyncChanges(
       base::Optional<EntityData> data) override;
   std::unique_ptr<EntityData> GetData() override;
-  ConflictResolution ResolveConflict(const EntityData& local_data,
-                                     const EntityData& remote_data) override;
   void ApplyDisableSyncChanges() override;
 
   // TODO(crbug.com/922900): investigate whether we need this getter outside of

components/sync/nigori/nigori_sync_bridge_impl_unittest.cc

@@ -921,6 +921,68 @@ TEST_F(NigoriSyncBridgeImplTest,
   // salt to check expectations about cryptographer state.
 }
 
+// Tests that pending local change with setting custom passphrase is applied,
+// when there was a conflicting remote update and remote update is respected.
+TEST_F(NigoriSyncBridgeImplTest,
+       ShouldSetCustomPassphraseAfterConflictingUpdates) {
+  // Start with simple keystore Nigori.
+  const std::string kRawKeystoreKey1 = "keystore_key1";
+  const KeyParams kKeystoreKeyParams1 = KeystoreKeyParams(kRawKeystoreKey1);
+  EntityData simple_keystore_entity_data;
+  *simple_keystore_entity_data.specifics.mutable_nigori() =
+      BuildKeystoreNigoriSpecifics(
+          /*keybag_keys_params=*/{kKeystoreKeyParams1},
+          /*keystore_decryptor_params=*/kKeystoreKeyParams1,
+          /*keystore_key_params=*/kKeystoreKeyParams1);
+  bridge()->SetKeystoreKeys({kRawKeystoreKey1});
+  ASSERT_THAT(bridge()->MergeSyncData(std::move(simple_keystore_entity_data)),
+              Eq(base::nullopt));
+
+  // Set up custom passphrase locally, but don't emulate commit completion.
+  const std::string kCustomPassphrase = "custom_passphrase";
+  bridge()->SetEncryptionPassphrase(kCustomPassphrase);
+
+  // Emulate conflict with rotated keystore Nigori.
+  const std::string kRawKeystoreKey2 = "keystore_key2";
+  const KeyParams kKeystoreKeyParams2 = KeystoreKeyParams(kRawKeystoreKey2);
+  EntityData rotated_keystore_entity_data;
+  *rotated_keystore_entity_data.specifics.mutable_nigori() =
+      BuildKeystoreNigoriSpecifics(
+          /*keybag_keys_params=*/{kKeystoreKeyParams1, kKeystoreKeyParams2},
+          /*keystore_decryptor_params=*/kKeystoreKeyParams2,
+          /*keystore_key_params=*/kKeystoreKeyParams2);
+  bridge()->SetKeystoreKeys({kRawKeystoreKey1, kRawKeystoreKey2});
+
+  // Verify that custom passphrase is set on top of
+  // |rotated_keystore_entity_data|.
+  EXPECT_CALL(*processor(), Put(HasCustomPassphraseNigori()));
+  EXPECT_THAT(
+      bridge()->ApplySyncChanges(std::move(rotated_keystore_entity_data)),
+      Eq(base::nullopt));
+  EXPECT_THAT(bridge()->GetData(), HasCustomPassphraseNigori());
+
+  // Mimic commit completion.
+  EXPECT_CALL(*observer(), OnPassphraseAccepted());
+  EXPECT_CALL(*observer(), OnEncryptedTypesChanged(
+                               /*encrypted_types=*/EncryptableUserTypes(),
+                               /*encrypt_everything=*/true));
+  EXPECT_CALL(*observer(), OnCryptographerStateChanged(
+                               NotNull(), /*has_pending_keys=*/false));
+  EXPECT_CALL(*observer(),
+              OnPassphraseTypeChanged(PassphraseType::kCustomPassphrase,
+                                      /*passphrase_time=*/Not(NullTime())));
+  EXPECT_CALL(*observer(), OnBootstrapTokenUpdated(Ne(std::string()),
+                                                   PASSPHRASE_BOOTSTRAP_TOKEN));
+  EXPECT_THAT(bridge()->ApplySyncChanges(base::nullopt), Eq(base::nullopt));
+  EXPECT_THAT(bridge()->GetData(), HasCustomPassphraseNigori());
+
+  const Cryptographer& cryptographer = bridge()->GetCryptographerForTesting();
+  EXPECT_THAT(cryptographer, CanDecryptWith(kKeystoreKeyParams1));
+  EXPECT_THAT(cryptographer, CanDecryptWith(kKeystoreKeyParams2));
+  // TODO(crbug.com/922900): find a good way to get key derivation method and
+  // salt to check expectations about cryptographer state.
+}
+
 // Tests that SetEncryptionPassphrase() call doesn't lead to custom passphrase
 // change in case we already have one.
 TEST_F(NigoriSyncBridgeImplTest, ShouldNotAllowCustomPassphraseChange) {