[Passwords] Move credentials that exist in account and profile stores

In such case, it's enough to only remove the profile copy without
changing the account store.

Bug: 1032992
Change-Id: I75a3e8ebdcbf3c7bb096850cb031b06c1364e4b7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2091348
Commit-Queue: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#747799}

components/password_manager/core/browser/multi_store_password_save_manager.cc

@@ -40,6 +40,18 @@ std::vector<const PasswordForm*> ProfileStoreMatches(
   return MatchesInStore(matches, PasswordForm::Store::kProfileStore);
 }
 
+bool AccountStoreMatchesContainForm(
+    const std::vector<const PasswordForm*>& matches,
+    const PasswordForm& form) {
+  PasswordForm form_in_account_store(form);
+  form_in_account_store.in_store = PasswordForm::Store::kAccountStore;
+  for (const PasswordForm* match : matches) {
+    if (form_in_account_store == *match)
+      return true;
+  }
+  return false;
+}
+
 }  // namespace
 
 MultiStorePasswordSaveManager::MultiStorePasswordSaveManager(
@@ -136,10 +148,9 @@ bool MultiStorePasswordSaveManager::IsAccountStoreEnabled() {
 void MultiStorePasswordSaveManager::MoveCredentialsToAccountStore() {
   // TODO(crbug.com/1032992): There are other rare corner cases that should
   // still be handled:
-  // 1. Credentials exist in both stores.
-  // 2. Credential exists only in the profile store but a PSL matched one exists
+  // 1. Credential exists only in the profile store but a PSL matched one exists
   // in both profile and account store.
-  // 3. Moving credentials upon an update. FormFetch will have an outdated
+  // 2. Moving credentials upon an update. FormFetch will have an outdated
   // credentials. Fix it if this turns out to be a product requirement.
 
   std::vector<const PasswordForm*> account_store_matches =
@@ -164,8 +175,13 @@ void MultiStorePasswordSaveManager::MoveCredentialsToAccountStore() {
     if (match->username_value != pending_credentials_.username_value)
       continue;
 
-    account_store_form_saver_->Save(*match, account_store_matches,
-                                    /*old_password=*/base::string16());
+    // Don't call Save() if the credential already exists in the account
+    // store, 1) to avoid unnecessary sync cycles, 2) to avoid potential
+    // last_used_date update.
+    if (!AccountStoreMatchesContainForm(account_store_matches, *match)) {
+      account_store_form_saver_->Save(*match, account_store_matches,
+                                      /*old_password=*/base::string16());
+    }
     form_saver_->Remove(*match);
   }
 }

components/password_manager/core/browser/multi_store_password_save_manager_unittest.cc

@@ -556,4 +556,25 @@ TEST_F(MultiStorePasswordSaveManagerTest,
 
   password_save_manager()->MoveCredentialsToAccountStore();
 }
+
+TEST_F(MultiStorePasswordSaveManagerTest,
+       MoveCredentialsFromProfileToAccountStoreWhenExistsInBothStores) {
+  PasswordForm saved_match_in_profile_store(saved_match_);
+  saved_match_in_profile_store.in_store = PasswordForm::Store::kProfileStore;
+  PasswordForm saved_match_in_account_store(saved_match_);
+  saved_match_in_account_store.in_store = PasswordForm::Store::kAccountStore;
+  SetNonFederatedAndNotifyFetchCompleted(
+      {&saved_match_in_profile_store, &saved_match_in_account_store});
+
+  password_save_manager()->CreatePendingCredentials(
+      saved_match_in_profile_store, observed_form_, submitted_form_,
+      /*is_http_auth=*/false,
+      /*is_credential_api_save=*/false);
+
+  EXPECT_CALL(*mock_profile_form_saver(), Remove(saved_match_in_profile_store));
+  EXPECT_CALL(*mock_account_form_saver(), Save).Times(0);
+
+  password_save_manager()->MoveCredentialsToAccountStore();
+}
+
 }  // namespace password_manager