[Passwords] Preserve moving blacklist when updating credentials

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

components/password_manager/core/browser/multi_store_password_save_manager.cc

@@ -147,13 +147,17 @@ void MultiStorePasswordSaveManager::SavePendingToStoreImpl(
                         old_profile_password);
       break;
     case PendingCredentialsState::UPDATE:
-    case PendingCredentialsState::EQUAL_TO_SAVED_MATCH:
-      // TODO(crbug.com/1012203): Make to preserve the moving_blocked_for_list
-      // in the profile store in case |pending_credentials_| is coming from the
-      // account store.
-      form_saver_->Update(pending_credentials_, profile_matches,
+    case PendingCredentialsState::EQUAL_TO_SAVED_MATCH: {
+      // If the submitted credentials exists in both stores,
+      // |pending_credentials_| might be from the account store (and thus not
+      // have a moving_blocked_for_list). We need to preserve any existing list,
+      // so explicitly copy it over from the profile store match.
+      PasswordForm form_to_update(pending_credentials_);
+      form_to_update.moving_blocked_for_list =
+          states.similar_saved_form_from_profile_store->moving_blocked_for_list;
+      form_saver_->Update(form_to_update, profile_matches,
                           old_profile_password);
-      break;
+    } break;
     // The NEW_LOGIN case was already handled separately above.
     case PendingCredentialsState::NEW_LOGIN:
     case PendingCredentialsState::NONE:
@@ -170,10 +174,16 @@ void MultiStorePasswordSaveManager::SavePendingToStoreImpl(
                                         old_account_password);
         break;
       case PendingCredentialsState::UPDATE:
-      case PendingCredentialsState::EQUAL_TO_SAVED_MATCH:
-        account_store_form_saver_->Update(pending_credentials_, account_matches,
+      case PendingCredentialsState::EQUAL_TO_SAVED_MATCH: {
+        // If the submitted credentials exists in both stores,
+        // .|pending_credentials_| might be from the profile store (and thus
+        // has a moving_blocked_for_list). We need to clear it before storing to
+        // the account store.
+        PasswordForm form_to_update(pending_credentials_);
+        form_to_update.moving_blocked_for_list.clear();
+        account_store_form_saver_->Update(form_to_update, account_matches,
                                           old_account_password);
-        break;
+      } break;
       // The NEW_LOGIN case was already handled separately above.
       case PendingCredentialsState::NEW_LOGIN:
       case PendingCredentialsState::NONE:
@@ -267,7 +277,6 @@ void MultiStorePasswordSaveManager::BlockMovingToAccountStoreFor(
   // might be from the account store (and thus not have a
   // moving_blocked_for_list). We need to preserve any existing list, so
   // explicitly copy it over from the profile store match.
-
   PasswordForm form_to_block(pending_credentials_);
   form_to_block.moving_blocked_for_list =
       states.similar_saved_form_from_profile_store->moving_blocked_for_list;

components/password_manager/core/browser/multi_store_password_save_manager_unittest.cc

@@ -345,14 +345,25 @@ TEST_F(MultiStorePasswordSaveManagerTest, UpdateInProfileStoreOnly) {
 }
 
 TEST_F(MultiStorePasswordSaveManagerTest, UpdateInBothStores) {
+  // This test assumes that all fields of the PasswordForm in both stores are
+  // equal except the |moving_blocked_for_list|. The reason for that is:
+  // 1. |moving_blocked_for_list| is the most probable field to have different
+  //    values since it's always empty in the account store.
+  // 2. Other fields (e.g. |times_used|) are less critical and should be fine if
+  //    the value in one store overrides the value in the other one.
+
   SetAccountStoreEnabled(/*is_enabled=*/true);
 
-  PasswordForm saved_match_in_profile_store(saved_match_);
-  saved_match_in_profile_store.username_value =
+  PasswordForm saved_match_in_account_store(saved_match_);
+  saved_match_in_account_store.username_value =
       parsed_submitted_form_.username_value;
-  saved_match_in_profile_store.in_store = PasswordForm::Store::kProfileStore;
-  PasswordForm saved_match_in_account_store(saved_match_in_profile_store);
   saved_match_in_account_store.in_store = PasswordForm::Store::kAccountStore;
+  PasswordForm saved_match_in_profile_store(saved_match_in_account_store);
+  saved_match_in_profile_store.in_store = PasswordForm::Store::kProfileStore;
+  autofill::GaiaIdHash user_id_hash =
+      autofill::GaiaIdHash::FromGaiaId("user@gmail.com");
+  saved_match_in_profile_store.moving_blocked_for_list.push_back(user_id_hash);
+
   SetNonFederatedAndNotifyFetchCompleted(
       {&saved_match_in_profile_store, &saved_match_in_account_store});
 
@@ -365,8 +376,34 @@ TEST_F(MultiStorePasswordSaveManagerTest, UpdateInBothStores) {
   // An update prompt should be shown.
   EXPECT_TRUE(password_save_manager()->IsPasswordUpdate());
 
-  EXPECT_CALL(*mock_profile_form_saver(), Update(_, _, _));
-  EXPECT_CALL(*mock_account_form_saver(), Update(_, _, _));
+  // Both stores should be updated in the following ways:
+  // 1. |password_value| is updated.
+  // 2. |times_used| is incremented.
+  // 3. |date_last_used| is updated.
+  // 4. |in_store| field is irrelevant since it's not persisted.
+  // 5. The rest of fields are taken aribtrary from one store.
+  PasswordForm expected_profile_updated_form(saved_match_in_profile_store);
+  expected_profile_updated_form.password_value =
+      parsed_submitted_form_.password_value;
+  expected_profile_updated_form.times_used++;
+  expected_profile_updated_form.date_last_used =
+      password_save_manager()->GetPendingCredentials().date_last_used;
+  expected_profile_updated_form.in_store =
+      password_save_manager()->GetPendingCredentials().in_store;
+
+  PasswordForm expected_account_updated_form(saved_match_in_account_store);
+  expected_account_updated_form.password_value =
+      parsed_submitted_form_.password_value;
+  expected_account_updated_form.times_used++;
+  expected_account_updated_form.date_last_used =
+      password_save_manager()->GetPendingCredentials().date_last_used;
+  expected_account_updated_form.in_store =
+      password_save_manager()->GetPendingCredentials().in_store;
+
+  EXPECT_CALL(*mock_profile_form_saver(),
+              Update(expected_profile_updated_form, _, _));
+  EXPECT_CALL(*mock_account_form_saver(),
+              Update(expected_account_updated_form, _, _));
 
   password_save_manager()->Save(observed_form_, parsed_submitted_form_);
 }