Implement PasswordSyncBridge::MergeSyncData()

Bug: 902349
Change-Id: I4b01ff184fc57d68a7944421aaace1d475df89a7
Reviewed-on: https://chromium-review.googlesource.com/c/1437184
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#626495}

components/password_manager/core/browser/sync/password_sync_bridge.cc

@@ -4,6 +4,8 @@
 
 #include "components/password_manager/core/browser/sync/password_sync_bridge.h"
 
+#include <unordered_set>
+
 #include "base/auto_reset.h"
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
@@ -189,7 +191,97 @@ PasswordSyncBridge::CreateMetadataChangeList() {
 base::Optional<syncer::ModelError> PasswordSyncBridge::MergeSyncData(
     std::unique_ptr<syncer::MetadataChangeList> metadata_change_list,
     syncer::EntityChangeList entity_data) {
-  NOTIMPLEMENTED();
+  base::AutoReset<bool> processing_changes(&is_processing_remote_sync_changes_,
+                                           true);
+  // Read all local passwords.
+  PrimaryKeyToFormMap key_to_local_form_map;
+  if (!password_store_sync_->ReadAllLogins(&key_to_local_form_map)) {
+    return syncer::ModelError(FROM_HERE,
+                              "Failed to load entries from password store.");
+  }
+
+  // Collect the client tags of remote passwords.  Note that |entity_data| only
+  // contains client tag *hashes*.
+  std::unordered_set<std::string> client_tags_of_remote_passwords;
+  for (const syncer::EntityChange& entity_change : entity_data) {
+    client_tags_of_remote_passwords.insert(GetClientTag(entity_change.data()));
+  }
+
+  // This is used to keep track of all the changes applied to the password
+  // store to notify other observers of the password store.
+  PasswordStoreChangeList password_store_changes;
+  base::Optional<syncer::ModelError> error;
+  {
+    ScopedStoreTransaction transaction(password_store_sync_);
+    // For any local password that doesn't exist in the remote passwords, issue
+    // a change_processor()->Put(). Password comparison is done by comparing the
+    // client tags. In addition, collect the client tags of local passwords.
+    std::unordered_set<std::string> client_tags_of_local_passwords;
+    for (const auto& pair : key_to_local_form_map) {
+      std::unique_ptr<syncer::EntityData> entity_data =
+          CreateEntityData(/*password_form=*/*pair.second);
+      const std::string client_tag_of_local_password =
+          GetClientTag(*entity_data);
+      client_tags_of_local_passwords.insert(client_tag_of_local_password);
+      if (client_tags_of_remote_passwords.count(client_tag_of_local_password) ==
+          0) {
+        change_processor()->Put(
+            /*storage_key=*/base::NumberToString(pair.first),
+            std::move(entity_data), metadata_change_list.get());
+      }
+    }
+
+    // For any remote password that doesn't exist in the local passwords, issue
+    // a password_store_sync_->AddLoginSync() and for those that exist in the
+    // local passwords, issue a password_store_sync_->UpdateLoginSync().
+    // Password comparison is done by comparing the client tags. In both cases,
+    // invoke the change_processor()->UpdateStorageKey().
+    const base::Time time_now = base::Time::Now();
+    for (const syncer::EntityChange& entity_change : entity_data) {
+      const std::string client_tag_of_remote_password =
+          GetClientTag(entity_change.data());
+      PasswordStoreChangeList changes;
+      if (client_tags_of_local_passwords.count(client_tag_of_remote_password) ==
+          0) {
+        changes = password_store_sync_->AddLoginSync(
+            PasswordFromEntityChange(entity_change, /*sync_time=*/time_now));
+        DCHECK_LE(1U, changes.size());
+      } else {
+        changes = password_store_sync_->UpdateLoginSync(
+            PasswordFromEntityChange(entity_change, /*sync_time=*/time_now));
+        DCHECK_LE(1U, changes.size());
+      }
+      if (changes.empty()) {
+        return syncer::ModelError(
+            FROM_HERE, "Failed to add/update an entry in the password store.");
+      }
+      change_processor()->UpdateStorageKey(
+          entity_change.data(),
+          /*storage_key=*/
+          base::NumberToString(changes[0].primary_key()),
+          metadata_change_list.get());
+      password_store_changes.push_back(changes[0]);
+    }
+
+    // Persist the metadata changes.
+    // TODO(mamir): add some test coverage for the metadata persistence.
+    syncer::SyncMetadataStoreChangeList sync_metadata_store_change_list(
+        password_store_sync_->GetMetadataStore(), syncer::PASSWORDS);
+    // |metadata_change_list| must have been created via
+    // CreateMetadataChangeList() so downcasting is safe.
+    static_cast<syncer::InMemoryMetadataChangeList*>(metadata_change_list.get())
+        ->TransferChangesTo(&sync_metadata_store_change_list);
+    error = sync_metadata_store_change_list.TakeError();
+  }  // End of scoped transaction.
+
+  if (!password_store_changes.empty()) {
+    // It could be the case that there are no remote passwords. In such case,
+    // there would be no changes to the password store other than the sync
+    // metadata changes, and no need to notify observers since they aren't
+    // interested in changes to sync metadata.
+    password_store_sync_->NotifyLoginsChanged(password_store_changes);
+  }
+
   return base::nullopt;
 }
 
@@ -255,8 +347,7 @@ base::Optional<syncer::ModelError> PasswordSyncBridge::ApplySyncChanges(
           break;
         }
       }
-      password_store_changes.insert(password_store_changes.end(),
-                                    changes.begin(), changes.end());
+      password_store_changes.push_back(changes[0]);
     }
 
     // Persist the metadata changes.
@@ -287,7 +378,7 @@ void PasswordSyncBridge::GetData(StorageKeyList storage_keys,
   PrimaryKeyToFormMap key_to_form_map;
   if (!password_store_sync_->ReadAllLogins(&key_to_form_map)) {
     change_processor()->ReportError(
-        {FROM_HERE, "Failed to load entries from table."});
+        {FROM_HERE, "Failed to load entries from the password store."});
     return;
   }
 

components/password_manager/core/browser/sync/password_sync_bridge_unittest.cc

@@ -30,6 +30,7 @@ using testing::Eq;
 using testing::Invoke;
 using testing::NotNull;
 using testing::Return;
+using testing::UnorderedElementsAre;
 
 constexpr char kSignonRealm1[] = "abc";
 constexpr char kSignonRealm2[] = "def";
@@ -47,9 +48,9 @@ MATCHER_P(FormHasSignonRealm, expected_signon_realm, "") {
   return arg.signon_realm == expected_signon_realm;
 }
 
-// |*arg| must be of type PasswordStoreChangeList.
+// |*arg| must be of type PasswordStoreChange.
 MATCHER_P(ChangeHasPrimaryKey, expected_primary_key, "") {
-  return arg[0].primary_key() == expected_primary_key;
+  return arg.primary_key() == expected_primary_key;
 }
 
 // |*arg| must be of type SyncMetadataStoreChangeList.
@@ -378,8 +379,9 @@ TEST_F(PasswordSyncBridgeTest, ShouldApplyRemoteCreation) {
               AddLoginSync(FormHasSignonRealm(kSignonRealm1)));
   EXPECT_CALL(mock_processor(), UpdateStorageKey(_, kStorageKey, _));
   EXPECT_CALL(*mock_password_store_sync(), CommitTransaction());
-  EXPECT_CALL(*mock_password_store_sync(),
-              NotifyLoginsChanged(ChangeHasPrimaryKey(1)));
+  EXPECT_CALL(
+      *mock_password_store_sync(),
+      NotifyLoginsChanged(UnorderedElementsAre(ChangeHasPrimaryKey(1))));
 
   // Processor shouldn't be notified about remote changes.
   EXPECT_CALL(mock_processor(), Put(_, _, _)).Times(0);
@@ -407,7 +409,8 @@ TEST_F(PasswordSyncBridgeTest, ShouldApplyRemoteUpdate) {
               UpdateLoginSync(FormHasSignonRealm(kSignonRealm1)));
   EXPECT_CALL(*mock_password_store_sync(), CommitTransaction());
   EXPECT_CALL(*mock_password_store_sync(),
-              NotifyLoginsChanged(ChangeHasPrimaryKey(kPrimaryKey)));
+              NotifyLoginsChanged(
+                  UnorderedElementsAre(ChangeHasPrimaryKey(kPrimaryKey))));
 
   // Processor shouldn't be notified about remote changes.
   EXPECT_CALL(mock_processor(), Put(_, _, _)).Times(0);
@@ -433,7 +436,8 @@ TEST_F(PasswordSyncBridgeTest, ShouldApplyRemoteDeletion) {
               RemoveLoginByPrimaryKeySync(kPrimaryKey));
   EXPECT_CALL(*mock_password_store_sync(), CommitTransaction());
   EXPECT_CALL(*mock_password_store_sync(),
-              NotifyLoginsChanged(ChangeHasPrimaryKey(kPrimaryKey)));
+              NotifyLoginsChanged(
+                  UnorderedElementsAre(ChangeHasPrimaryKey(kPrimaryKey))));
 
   // Processor shouldn't be notified about remote changes.
   EXPECT_CALL(mock_processor(), Delete(_, _)).Times(0);
@@ -476,4 +480,78 @@ TEST_F(PasswordSyncBridgeTest, ShouldNotGetDataForNonExistingStorageKey) {
   EXPECT_FALSE(optional_specifics.has_value());
 }
 
+TEST_F(PasswordSyncBridgeTest, ShouldMergeSyncRemoteAndLocalPasswords) {
+  ON_CALL(mock_processor(), IsTrackingMetadata()).WillByDefault(Return(true));
+  // Setup the test to have Form 1 and Form 2 stored locally, and Form 2 and
+  // Form 3 coming as remote changes. We will assign primary keys for Form 1 and
+  // Form 2. Form 3 will arrive as remote creation, and FakeDatabase will assign
+  // it primary key 1.
+  const int kPrimaryKey1 = 1000;
+  const int kPrimaryKey2 = 1001;
+  const int kExpectedPrimaryKey3 = 1;
+  const std::string kPrimaryKeyStr1 = "1000";
+  const std::string kPrimaryKeyStr2 = "1001";
+  const std::string kExpectedPrimaryKeyStr3 = "1";
+  autofill::PasswordForm form1 = MakePasswordForm(kSignonRealm1);
+  autofill::PasswordForm form2 = MakePasswordForm(kSignonRealm2);
+  autofill::PasswordForm form3 = MakePasswordForm(kSignonRealm3);
+  sync_pb::PasswordSpecifics specifics1 =
+      CreateSpecificsWithSignonRealm(kSignonRealm1);
+  sync_pb::PasswordSpecifics specifics2 =
+      CreateSpecificsWithSignonRealm(kSignonRealm2);
+  sync_pb::PasswordSpecifics specifics3 =
+      CreateSpecificsWithSignonRealm(kSignonRealm3);
+
+  fake_db()->AddLoginForPrimaryKey(kPrimaryKey1, form1);
+  fake_db()->AddLoginForPrimaryKey(kPrimaryKey2, form2);
+
+  // Form 1 will be added to the change processor, Form 2 will be updated in the
+  // password sync store, and Form 3 will be added to the password store sync.
+
+  // Interactions should happen in this order:
+  //           +--> Put(1) ------------------------------------+
+  //           |                                               |
+  // Begin() --|--> UpdateLoginSync(2) --> UpdateStorageKey(2)-|--> Commit()
+  //           |                                               |
+  //           +--> AddLoginSync (3)   --> UpdateStorageKey(3)-+
+
+  testing::Sequence s1, s2, s3;
+  EXPECT_CALL(*mock_password_store_sync(), BeginTransaction())
+      .InSequence(s1, s2, s3);
+  EXPECT_CALL(mock_processor(),
+              Put(kPrimaryKeyStr1, EntityDataHasSignonRealm(kSignonRealm1), _))
+      .InSequence(s1);
+  EXPECT_CALL(*mock_password_store_sync(),
+              UpdateLoginSync(FormHasSignonRealm(kSignonRealm2)))
+      .InSequence(s2);
+  EXPECT_CALL(*mock_password_store_sync(),
+              AddLoginSync(FormHasSignonRealm(kSignonRealm3)))
+      .InSequence(s3);
+
+  EXPECT_CALL(mock_processor(), UpdateStorageKey(_, kPrimaryKeyStr2, _))
+      .InSequence(s2);
+  EXPECT_CALL(mock_processor(), UpdateStorageKey(_, kExpectedPrimaryKeyStr3, _))
+      .InSequence(s3);
+  EXPECT_CALL(*mock_password_store_sync(), CommitTransaction())
+      .InSequence(s1, s2, s3);
+
+  EXPECT_CALL(*mock_password_store_sync(),
+              NotifyLoginsChanged(UnorderedElementsAre(
+                  ChangeHasPrimaryKey(kPrimaryKey2),
+                  ChangeHasPrimaryKey(kExpectedPrimaryKey3))))
+      .InSequence(s1, s2, s3);
+
+  // Processor shouldn't be informed about Form 2 or Form 3.
+  EXPECT_CALL(mock_processor(), Put(kPrimaryKeyStr2, _, _)).Times(0);
+  EXPECT_CALL(mock_processor(), Put(kExpectedPrimaryKeyStr3, _, _)).Times(0);
+
+  base::Optional<syncer::ModelError> error = bridge()->MergeSyncData(
+      bridge()->CreateMetadataChangeList(),
+      {syncer::EntityChange::CreateAdd(
+           /*storage_key=*/"", SpecificsToEntity(specifics2)),
+       syncer::EntityChange::CreateAdd(
+           /*storage_key=*/"", SpecificsToEntity(specifics3))});
+  EXPECT_FALSE(error);
+}
+
 }  // namespace password_manager