If we have credentials with the empty username and the user uses them for...

If we have credentials with the empty username and the user uses them for sign-in with non-empty username then delete no-username credentials. These no-username credentials most probably were stored on change password or reset forms, and we don't need them anymore.

BUG=359315

Review URL: https://codereview.chromium.org/1272613002

Cr-Commit-Position: refs/heads/master@{#342360}

components/password_manager/core/browser/password_form_manager.cc

@@ -289,9 +289,10 @@ void PasswordFormManager::Save() {
     LogPasswordGenerationSubmissionEvent(PASSWORD_USED);
   }
 
-  if (IsNewLogin())
+  if (IsNewLogin()) {
     SaveAsNewLogin(true);
-  else
+    DeleteEmptyUsernameCredentials();
+  } else
     UpdateLogin();
 }
 
@@ -996,6 +997,22 @@ int PasswordFormManager::ScoreResult(const PasswordForm& candidate) const {
   return score;
 }
 
+void PasswordFormManager::DeleteEmptyUsernameCredentials() {
+  if (best_matches_.empty() || pending_credentials_.username_value.empty())
+    return;
+  PasswordStore* password_store = client_->GetPasswordStore();
+  if (!password_store) {
+    NOTREACHED();
+    return;
+  }
+  for (auto iter = best_matches_.begin(); iter != best_matches_.end(); ++iter) {
+    PasswordForm* form = iter->second;
+    if (!form->IsPublicSuffixMatch() && form->username_value.empty() &&
+        form->password_value == pending_credentials_.password_value)
+      password_store->RemoveLogin(*form);
+  }
+}
+
 PasswordForm* PasswordFormManager::FindBestMatchForUpdatePassword(
     const base::string16& password) const {
   if (best_matches_.size() == 1) {

components/password_manager/core/browser/password_form_manager.h

@@ -362,6 +362,12 @@ class PasswordFormManager : public PasswordStoreConsumer {
   // from password store.
   void CreatePendingCredentials();
 
+  // If |pending_credentials_.username_value| is not empty, iterates over all
+  // forms from |best_matches_| and deletes from the password store all which
+  // are not PSL-matched, have an empty username, and a password equal to
+  // |pending_credentials_.password_value|.
+  void DeleteEmptyUsernameCredentials();
+
   // If |best_matches| contains only one entry then return this entry. Otherwise
   // for empty |password| return nullptr and for non-empty |password| returns
   // the unique entry in |best_matches_| with the same password, if it exists,

components/password_manager/core/browser/password_form_manager_unittest.cc

@@ -54,6 +54,10 @@ ACTION_P4(InvokeConsumer, form1, form2, form3, form4) {
   arg0->OnGetPasswordStoreResults(result.Pass());
 }
 
+MATCHER_P(CheckUsername, username_value, "Username incorrect") {
+  return arg.username_value == username_value;
+}
+
 void RunAllPendingTasks() {
   base::RunLoop run_loop;
   base::ThreadTaskRunnerHandle::Get()->PostTask(
@@ -1750,4 +1754,175 @@ TEST_F(PasswordFormManagerTest, WipeStoreCopyIfOutdated_Outdated) {
                                       1);
 }
 
+TEST_F(PasswordFormManagerTest, RemoveNoUsernameAccounts) {
+  TestPasswordManagerClient client_with_store(mock_store());
+  TestPasswordManager password_manager(&client_with_store);
+  PasswordFormManager form_manager(&password_manager, &client_with_store,
+                                   client_with_store.driver(), *observed_form(),
+                                   false);
+
+  PasswordForm saved_form = *saved_match();
+  saved_form.username_value.clear();
+  ScopedVector<PasswordForm> result;
+  result.push_back(new PasswordForm(saved_form));
+  form_manager.SimulateFetchMatchingLoginsFromPasswordStore();
+  form_manager.OnGetPasswordStoreResults(result.Pass());
+
+  PasswordForm submitted_form(*observed_form());
+  submitted_form.preferred = true;
+  submitted_form.username_value = saved_match()->username_value;
+  submitted_form.password_value = saved_match()->password_value;
+
+  saved_form.preferred = false;
+  EXPECT_CALL(*mock_store(),
+              AddLogin(CheckUsername(saved_match()->username_value)));
+  EXPECT_CALL(*mock_store(), RemoveLogin(saved_form));
+
+  form_manager.ProvisionallySave(
+      submitted_form, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
+  form_manager.Save();
+}
+
+TEST_F(PasswordFormManagerTest, NotRemovePSLNoUsernameAccounts) {
+  TestPasswordManagerClient client_with_store(mock_store());
+  TestPasswordManager password_manager(&client_with_store);
+  PasswordFormManager form_manager(&password_manager, &client_with_store,
+                                   client_with_store.driver(), *observed_form(),
+                                   false);
+
+  PasswordForm saved_form = *saved_match();
+  saved_form.username_value.clear();
+  saved_form.original_signon_realm = "www.example.org";
+  ScopedVector<PasswordForm> result;
+  result.push_back(new PasswordForm(saved_form));
+  form_manager.SimulateFetchMatchingLoginsFromPasswordStore();
+  form_manager.OnGetPasswordStoreResults(result.Pass());
+
+  PasswordForm submitted_form(*observed_form());
+  submitted_form.preferred = true;
+  submitted_form.username_value = saved_match()->username_value;
+  submitted_form.password_value = saved_match()->password_value;
+
+  EXPECT_CALL(*mock_store(),
+              AddLogin(CheckUsername(saved_match()->username_value)));
+  EXPECT_CALL(*mock_store(), RemoveLogin(_)).Times(0);
+
+  form_manager.ProvisionallySave(
+      submitted_form, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
+  form_manager.Save();
+}
+
+TEST_F(PasswordFormManagerTest, NotRemoveCredentialsWithUsername) {
+  TestPasswordManagerClient client_with_store(mock_store());
+  TestPasswordManager password_manager(&client_with_store);
+  PasswordFormManager form_manager(&password_manager, &client_with_store,
+                                   client_with_store.driver(), *observed_form(),
+                                   false);
+
+  PasswordForm saved_form = *saved_match();
+  ASSERT_FALSE(saved_form.username_value.empty());
+  ScopedVector<PasswordForm> result;
+  result.push_back(new PasswordForm(saved_form));
+  form_manager.SimulateFetchMatchingLoginsFromPasswordStore();
+  form_manager.OnGetPasswordStoreResults(result.Pass());
+
+  PasswordForm submitted_form(*observed_form());
+  submitted_form.preferred = true;
+  base::string16 username = saved_form.username_value + ASCIIToUTF16("1");
+  submitted_form.username_value = username;
+  submitted_form.password_value = saved_match()->password_value;
+
+  EXPECT_CALL(*mock_store(), AddLogin(CheckUsername(username)));
+  EXPECT_CALL(*mock_store(), RemoveLogin(_)).Times(0);
+
+  form_manager.ProvisionallySave(
+      submitted_form, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
+  form_manager.Save();
+}
+
+TEST_F(PasswordFormManagerTest, NotRemoveCredentialsWithDiferrentPassword) {
+  TestPasswordManagerClient client_with_store(mock_store());
+  TestPasswordManager password_manager(&client_with_store);
+  PasswordFormManager form_manager(&password_manager, &client_with_store,
+                                   client_with_store.driver(), *observed_form(),
+                                   false);
+
+  PasswordForm saved_form = *saved_match();
+  saved_form.username_value.clear();
+  ScopedVector<PasswordForm> result;
+  result.push_back(new PasswordForm(saved_form));
+  form_manager.SimulateFetchMatchingLoginsFromPasswordStore();
+  form_manager.OnGetPasswordStoreResults(result.Pass());
+
+  PasswordForm submitted_form(*observed_form());
+  submitted_form.preferred = true;
+  submitted_form.username_value = saved_match()->username_value;
+  submitted_form.password_value = saved_form.password_value + ASCIIToUTF16("1");
+
+  EXPECT_CALL(*mock_store(),
+              AddLogin(CheckUsername(saved_match()->username_value)));
+  EXPECT_CALL(*mock_store(), RemoveLogin(_)).Times(0);
+
+  form_manager.ProvisionallySave(
+      submitted_form, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
+  form_manager.Save();
+}
+
+TEST_F(PasswordFormManagerTest, SaveNoUsernameEvenIfWithUsernamePresent) {
+  TestPasswordManagerClient client_with_store(mock_store());
+  TestPasswordManager password_manager(&client_with_store);
+  PasswordFormManager form_manager(&password_manager, &client_with_store,
+                                   client_with_store.driver(), *observed_form(),
+                                   false);
+
+  PasswordForm* saved_form = saved_match();
+  ASSERT_FALSE(saved_match()->username_value.empty());
+  ScopedVector<PasswordForm> result;
+  result.push_back(new PasswordForm(*saved_form));
+  form_manager.SimulateFetchMatchingLoginsFromPasswordStore();
+  form_manager.OnGetPasswordStoreResults(result.Pass());
+
+  PasswordForm submitted_form(*observed_form());
+  submitted_form.preferred = true;
+  submitted_form.username_value.clear();
+
+  EXPECT_CALL(*mock_store(), AddLogin(CheckUsername(base::string16())));
+  EXPECT_CALL(*mock_store(), RemoveLogin(_)).Times(0);
+
+  form_manager.ProvisionallySave(
+      submitted_form, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
+  form_manager.Save();
+}
+
+TEST_F(PasswordFormManagerTest, NotRemoveOnUpdate) {
+  TestPasswordManagerClient client_with_store(mock_store());
+  TestPasswordManager password_manager(&client_with_store);
+  PasswordFormManager form_manager(&password_manager, &client_with_store,
+                                   client_with_store.driver(), *observed_form(),
+                                   false);
+
+  ScopedVector<PasswordForm> result;
+  PasswordForm saved_form = *saved_match();
+  ASSERT_FALSE(saved_form.username_value.empty());
+  result.push_back(new PasswordForm(saved_form));
+  saved_form.username_value.clear();
+  saved_form.preferred = false;
+  result.push_back(new PasswordForm(saved_form));
+  form_manager.SimulateFetchMatchingLoginsFromPasswordStore();
+  form_manager.OnGetPasswordStoreResults(result.Pass());
+
+  PasswordForm submitted_form(*observed_form());
+  submitted_form.preferred = true;
+  submitted_form.username_value = saved_match()->username_value;
+  submitted_form.password_value = saved_form.password_value + ASCIIToUTF16("1");
+
+  EXPECT_CALL(*mock_store(),
+              UpdateLogin(CheckUsername(saved_match()->username_value)));
+  EXPECT_CALL(*mock_store(), RemoveLogin(_)).Times(0);
+
+  form_manager.ProvisionallySave(
+      submitted_form, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
+  form_manager.Save();
+}
+
 }  // namespace password_manager