[Sync::USS] Implement NigoriModelTypeProcessor::OnUpdateReceived()

Bug: 922900
Change-Id: Ic0c140f62697c5a6cdae2ec56204aadae8c3e3cb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1575938
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#653620}

components/sync/nigori/nigori_model_type_processor.cc

@@ -96,7 +96,71 @@ void NigoriModelTypeProcessor::OnUpdateReceived(
     const sync_pb::ModelTypeState& type_state,
     UpdateResponseDataList updates) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  NOTIMPLEMENTED();
+  DCHECK(model_ready_to_sync_);
+  // If there is a model error, it must have been reported already but hasn't
+  // reached the sync engine yet. In this case return directly to avoid
+  // interactions with the bridge.
+  if (model_error_) {
+    return;
+  }
+
+  base::Optional<ModelError> error;
+
+  bool is_initial_sync = !model_type_state_.initial_sync_done();
+  model_type_state_ = type_state;
+
+  if (is_initial_sync) {
+    DCHECK(!entity_);
+    if (updates.empty()) {
+      error = bridge_->MergeSyncData(base::nullopt);
+    } else {
+      DCHECK(!updates[0]->entity->is_deleted());
+      entity_ = ProcessorEntity::CreateNew(
+          kNigoriStorageKey, kNigoriClientTagHash, updates[0]->entity->id,
+          updates[0]->entity->creation_time);
+      entity_->RecordAcceptedUpdate(*updates[0]);
+      error = bridge_->MergeSyncData(std::move(*updates[0]->entity));
+    }
+    if (error) {
+      // TODO(mamir): ReportError(*error);
+      NOTIMPLEMENTED();
+    }
+    return;
+  }
+
+  if (updates.empty()) {
+    bridge_->ApplySyncChanges(/*data=*/base::nullopt);
+    return;
+  }
+
+  DCHECK(entity_);
+  // We assume the bridge will issue errors in case of deletions. Therefore, we
+  // are adding the following DCHECK to simplify the code.
+  DCHECK(!updates[0]->entity->is_deleted());
+
+  if (entity_->UpdateIsReflection(updates[0]->response_version)) {
+    // Seen this update before; just ignore it.
+    bridge_->ApplySyncChanges(/*data=*/base::nullopt);
+    return;
+  }
+
+  if (entity_->IsUnsynced()) {
+    // TODO(mamir): conflict resolution
+    NOTIMPLEMENTED();
+  } else if (!entity_->MatchesData(*updates[0]->entity)) {
+    // Inform the bridge of the new or updated data.
+    entity_->RecordAcceptedUpdate(*updates[0]);
+    error = bridge_->ApplySyncChanges(std::move(*updates[0]->entity));
+  }
+
+  if (error) {
+    // TODO(mamir): ReportError(*error);
+    NOTIMPLEMENTED();
+    return;
+  }
+
+  // There may be new reasons to commit by the time this function is done.
+  NudgeForCommitIfNeeded();
 }
 
 void NigoriModelTypeProcessor::OnSyncStarting(

components/sync/nigori/nigori_model_type_processor_unittest.cc

@@ -21,18 +21,52 @@ namespace syncer {
 
 namespace {
 
+using testing::_;
 using testing::Eq;
 using testing::Ne;
 using testing::NotNull;
 
 const char kNigoriNonUniqueName[] = "nigori";
+const char kNigoriServerId[] = "nigori_server_id";
 const char kCacheGuid[] = "generated_id";
 
+// |*arg| must be of type base::Optional<EntityData>.
+MATCHER_P(OptionalEntityDataHasDecryptorTokenKeyName, expected_key_name, "") {
+  return arg->specifics.nigori().keystore_decryptor_token().key_name() ==
+         expected_key_name;
+}
+
 void CaptureCommitRequest(CommitRequestDataList* dst,
                           CommitRequestDataList&& src) {
   *dst = std::move(src);
 }
 
+sync_pb::ModelTypeState CreateDummyModelTypeState() {
+  sync_pb::ModelTypeState model_type_state;
+  model_type_state.set_cache_guid(kCacheGuid);
+  model_type_state.set_initial_sync_done(true);
+  return model_type_state;
+}
+
+// Creates a dummy Nigori UpdateResponseData that has the keystore decryptor
+// token key name set.
+std::unique_ptr<syncer::UpdateResponseData> CreateDummyNigoriUpdateResponseData(
+    const std::string keystore_decryptor_token_key_name,
+    int response_version) {
+  auto entity_data = std::make_unique<syncer::EntityData>();
+  entity_data->id = kNigoriServerId;
+  sync_pb::NigoriSpecifics* nigori_specifics =
+      entity_data->specifics.mutable_nigori();
+  nigori_specifics->mutable_keystore_decryptor_token()->set_key_name(
+      keystore_decryptor_token_key_name);
+  entity_data->non_unique_name = kNigoriNonUniqueName;
+
+  auto response_data = std::make_unique<syncer::UpdateResponseData>();
+  response_data->entity = std::move(entity_data);
+  response_data->response_version = response_version;
+  return response_data;
+}
+
 class MockNigoriSyncBridge : public NigoriSyncBridge {
  public:
   MockNigoriSyncBridge() = default;
@@ -41,8 +75,9 @@ class MockNigoriSyncBridge : public NigoriSyncBridge {
   MOCK_METHOD1(
       MergeSyncData,
       base::Optional<ModelError>(const base::Optional<EntityData>& data));
-  MOCK_METHOD1(ApplySyncChanges,
-               base::Optional<ModelError>(const EntityData& data));
+  MOCK_METHOD1(
+      ApplySyncChanges,
+      base::Optional<ModelError>(const base::Optional<EntityData>& data));
   MOCK_METHOD0(GetData, std::unique_ptr<EntityData>());
   MOCK_METHOD2(ResolveConflict,
                ConflictResolution(const EntityData& local_data,
@@ -65,18 +100,24 @@ class NigoriModelTypeProcessorTest : public testing::Test {
     mock_commit_queue_ptr_ = mock_commit_queue_.get();
   }
 
-  void SimulateModelReadyToSyncWithInitialSyncDone() {
+  void SimulateModelReadyToSync(bool initial_sync_done, int server_version) {
     NigoriMetadataBatch nigori_metadata_batch;
-    nigori_metadata_batch.model_type_state.set_initial_sync_done(true);
+    nigori_metadata_batch.model_type_state.set_initial_sync_done(
+        initial_sync_done);
     nigori_metadata_batch.entity_metadata = sync_pb::EntityMetadata();
     nigori_metadata_batch.entity_metadata->set_creation_time(
         TimeToProtoTime(base::Time::Now()));
     nigori_metadata_batch.entity_metadata->set_sequence_number(0);
     nigori_metadata_batch.entity_metadata->set_acked_sequence_number(0);
+    nigori_metadata_batch.entity_metadata->set_server_version(server_version);
     processor_.ModelReadyToSync(mock_nigori_sync_bridge(),
                                 std::move(nigori_metadata_batch));
   }
 
+  void SimulateModelReadyToSync(bool initial_sync_done) {
+    SimulateModelReadyToSync(initial_sync_done, /*server_version=*/1);
+  }
+
   void SimulateConnectSync() {
     processor_.ConnectSync(std::move(mock_commit_queue_));
   }
@@ -132,7 +173,7 @@ TEST_F(NigoriModelTypeProcessorTest,
 }
 
 TEST_F(NigoriModelTypeProcessorTest, ShouldIncrementSequenceNumberWhenPut) {
-  SimulateModelReadyToSyncWithInitialSyncDone();
+  SimulateModelReadyToSync(/*initial_sync_done=*/true);
   base::Optional<sync_pb::EntityMetadata> entity_metadata1 =
       processor()->GetMetadata().entity_metadata;
   ASSERT_TRUE(entity_metadata1);
@@ -152,7 +193,7 @@ TEST_F(NigoriModelTypeProcessorTest, ShouldIncrementSequenceNumberWhenPut) {
 }
 
 TEST_F(NigoriModelTypeProcessorTest, ShouldGetEmptyLocalChanges) {
-  SimulateModelReadyToSyncWithInitialSyncDone();
+  SimulateModelReadyToSync(/*initial_sync_done=*/true);
   CommitRequestDataList commit_request;
   processor()->GetLocalChanges(
       /*max_entries=*/10,
@@ -161,7 +202,7 @@ TEST_F(NigoriModelTypeProcessorTest, ShouldGetEmptyLocalChanges) {
 }
 
 TEST_F(NigoriModelTypeProcessorTest, ShouldGetLocalChangesWhenPut) {
-  SimulateModelReadyToSyncWithInitialSyncDone();
+  SimulateModelReadyToSync(/*initial_sync_done=*/true);
 
   auto entity_data = std::make_unique<syncer::EntityData>();
   entity_data->specifics.mutable_nigori();
@@ -178,7 +219,7 @@ TEST_F(NigoriModelTypeProcessorTest, ShouldGetLocalChangesWhenPut) {
 
 TEST_F(NigoriModelTypeProcessorTest,
        ShouldNudgeForCommitUponConnectSyncIfReadyToSyncAndLocalChanges) {
-  SimulateModelReadyToSyncWithInitialSyncDone();
+  SimulateModelReadyToSync(/*initial_sync_done=*/true);
 
   auto entity_data = std::make_unique<syncer::EntityData>();
   entity_data->specifics.mutable_nigori();
@@ -191,7 +232,7 @@ TEST_F(NigoriModelTypeProcessorTest,
 }
 
 TEST_F(NigoriModelTypeProcessorTest, ShouldNudgeForCommitUponPutIfReadyToSync) {
-  SimulateModelReadyToSyncWithInitialSyncDone();
+  SimulateModelReadyToSync(/*initial_sync_done=*/true);
   SimulateConnectSync();
 
   auto entity_data = std::make_unique<syncer::EntityData>();
@@ -203,7 +244,7 @@ TEST_F(NigoriModelTypeProcessorTest, ShouldNudgeForCommitUponPutIfReadyToSync) {
 }
 
 TEST_F(NigoriModelTypeProcessorTest, ShouldInvokeSyncStartCallback) {
-  SimulateModelReadyToSyncWithInitialSyncDone();
+  SimulateModelReadyToSync(/*initial_sync_done=*/true);
 
   syncer::DataTypeActivationRequest request;
   request.error_handler = base::DoNothing();
@@ -231,6 +272,66 @@ TEST_F(NigoriModelTypeProcessorTest, ShouldInvokeSyncStartCallback) {
   EXPECT_TRUE(processor()->IsConnectedForTest());
 }
 
+TEST_F(NigoriModelTypeProcessorTest, ShouldMergeSyncData) {
+  SimulateModelReadyToSync(/*initial_sync_done=*/false);
+
+  const std::string kDecryptorTokenKeyName = "key_name";
+  UpdateResponseDataList updates;
+  updates.push_back(CreateDummyNigoriUpdateResponseData(kDecryptorTokenKeyName,
+                                                        /*server_version=*/1));
+
+  EXPECT_CALL(*mock_nigori_sync_bridge(),
+              MergeSyncData(OptionalEntityDataHasDecryptorTokenKeyName(
+                  kDecryptorTokenKeyName)));
+
+  processor()->OnUpdateReceived(CreateDummyModelTypeState(),
+                                std::move(updates));
+}
+
+TEST_F(NigoriModelTypeProcessorTest, ShouldApplySyncChanges) {
+  SimulateModelReadyToSync(/*initial_sync_done=*/true, /*server_version=*/1);
+
+  const std::string kDecryptorTokenKeyName = "key_name";
+  UpdateResponseDataList updates;
+  updates.push_back(CreateDummyNigoriUpdateResponseData(kDecryptorTokenKeyName,
+                                                        /*server_version=*/2));
+
+  EXPECT_CALL(*mock_nigori_sync_bridge(),
+              ApplySyncChanges(OptionalEntityDataHasDecryptorTokenKeyName(
+                  kDecryptorTokenKeyName)));
+
+  processor()->OnUpdateReceived(CreateDummyModelTypeState(),
+                                std::move(updates));
+}
+
+TEST_F(NigoriModelTypeProcessorTest, ShouldApplySyncChangesWhenEmptyUpdates) {
+  const int kServerVersion = 1;
+  SimulateModelReadyToSync(/*initial_sync_done=*/true, kServerVersion);
+
+  // ApplySyncChanges() should still be called to trigger persistence of the
+  // metadata.
+  EXPECT_CALL(*mock_nigori_sync_bridge(), ApplySyncChanges(Eq(base::nullopt)));
+
+  processor()->OnUpdateReceived(CreateDummyModelTypeState(),
+                                UpdateResponseDataList());
+}
+
+TEST_F(NigoriModelTypeProcessorTest, ShouldApplySyncChangesWhenReflection) {
+  const int kServerVersion = 1;
+  SimulateModelReadyToSync(/*initial_sync_done=*/true, kServerVersion);
+
+  UpdateResponseDataList updates;
+  updates.push_back(CreateDummyNigoriUpdateResponseData(
+      /*keystore_decryptor_token_key_name=*/"key_name", kServerVersion));
+
+  // ApplySyncChanges() should still be called to trigger persistence of the
+  // metadata.
+  EXPECT_CALL(*mock_nigori_sync_bridge(), ApplySyncChanges(Eq(base::nullopt)));
+
+  processor()->OnUpdateReceived(CreateDummyModelTypeState(),
+                                std::move(updates));
+}
+
 }  // namespace
 
 }  // namespace syncer

components/sync/nigori/nigori_sync_bridge.h

@@ -31,7 +31,7 @@ class NigoriSyncBridge {
 
   // Apply changes from the sync server locally.
   virtual base::Optional<ModelError> ApplySyncChanges(
-      const EntityData& data) = 0;
+      const base::Optional<EntityData>& data) = 0;
 
   // Retrieve Nigori sync data.
   virtual std::unique_ptr<EntityData> GetData() = 0;

components/sync/nigori/nigori_sync_bridge_impl.cc

@@ -149,41 +149,43 @@ base::Optional<ModelError> NigoriSyncBridgeImpl::MergeSyncData(
                       "Received empty EntityData during initial "
                       "sync of Nigori.");
   }
-  return ApplySyncChanges(*data);
+  return ApplySyncChanges(data);
 }
 
 base::Optional<ModelError> NigoriSyncBridgeImpl::ApplySyncChanges(
-    const EntityData& data) {
+    const base::Optional<EntityData>& data) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(data.specifics.has_nigori());
-  const sync_pb::NigoriSpecifics& nigori = data.specifics.nigori();
-
-  // TODO(crbug.com/922900): support other passphrase types.
-  if (ProtoPassphraseTypeToEnum(nigori.passphrase_type()) !=
-      PassphraseType::KEYSTORE_PASSPHRASE) {
-    NOTIMPLEMENTED();
-    return ModelError(FROM_HERE, "Only keystore Nigori node is supported.");
-  }
+  if (data) {
+    DCHECK(data->specifics.has_nigori());
+    const sync_pb::NigoriSpecifics& nigori = data->specifics.nigori();
+
+    // TODO(crbug.com/922900): support other passphrase types.
+    if (ProtoPassphraseTypeToEnum(nigori.passphrase_type()) !=
+        PassphraseType::KEYSTORE_PASSPHRASE) {
+      NOTIMPLEMENTED();
+      return ModelError(FROM_HERE, "Only keystore Nigori node is supported.");
+    }
 
-  DCHECK(!keystore_keys_.empty());
-  // TODO(crbug.com/922900): verify that we don't need to check that
-  // nigori.encryption_keybag() and nigori.keystore_decryptor_token() are not
-  // empty.
-  sync_pb::EncryptedData keybag = nigori.encryption_keybag();
-  if (cryptographer_.CanDecrypt(keybag)) {
-    cryptographer_.InstallKeys(keybag);
-  } else {
-    cryptographer_.SetPendingKeys(keybag);
-    base::Optional<std::string> serialized_keystore_decryptor =
-        DecryptKeystoreDecryptor(keystore_keys_,
-                                 nigori.keystore_decryptor_token(),
-                                 cryptographer_.encryptor());
-    if (!serialized_keystore_decryptor ||
-        !cryptographer_.ImportNigoriKey(*serialized_keystore_decryptor) ||
-        !cryptographer_.is_ready()) {
-      return ModelError(FROM_HERE,
-                        "Failed to decrypt pending keys using the keystore "
-                        "decryptor token.");
+    DCHECK(!keystore_keys_.empty());
+    // TODO(crbug.com/922900): verify that we don't need to check that
+    // nigori.encryption_keybag() and nigori.keystore_decryptor_token() are not
+    // empty.
+    sync_pb::EncryptedData keybag = nigori.encryption_keybag();
+    if (cryptographer_.CanDecrypt(keybag)) {
+      cryptographer_.InstallKeys(keybag);
+    } else {
+      cryptographer_.SetPendingKeys(keybag);
+      base::Optional<std::string> serialized_keystore_decryptor =
+          DecryptKeystoreDecryptor(keystore_keys_,
+                                   nigori.keystore_decryptor_token(),
+                                   cryptographer_.encryptor());
+      if (!serialized_keystore_decryptor ||
+          !cryptographer_.ImportNigoriKey(*serialized_keystore_decryptor) ||
+          !cryptographer_.is_ready()) {
+        return ModelError(FROM_HERE,
+                          "Failed to decrypt pending keys using the keystore "
+                          "decryptor token.");
+      }
     }
   }
   // TODO(crbug.com/922900): implement updates of other data fields (e.g.

components/sync/nigori/nigori_sync_bridge_impl.h

@@ -56,7 +56,8 @@ class NigoriSyncBridgeImpl : public KeystoreKeysHandler,
   // NigoriSyncBridge implementation.
   base::Optional<ModelError> MergeSyncData(
       const base::Optional<EntityData>& data) override;
-  base::Optional<ModelError> ApplySyncChanges(const EntityData& data) override;
+  base::Optional<ModelError> ApplySyncChanges(
+      const base::Optional<EntityData>& data) override;
   std::unique_ptr<EntityData> GetData() override;
   ConflictResolution ResolveConflict(const EntityData& local_data,
                                      const EntityData& remote_data) override;