Updating of all entries in PasswordManager of the same credentials on password update

components/password_manager/core/browser/password_form_manager_unittest.cc

Index: components/password_manager/core/browser/password_form_manager_unittest.cc
diff --git a/components/password_manager/core/browser/password_form_manager_unittest.cc b/components/password_manager/core/browser/password_form_manager_unittest.cc
index a3e0339c253ed5f39724ee725801e4be4a591647..a471b8979a974d5606711d5c1087291d87671de4 100644
--- a/components/password_manager/core/browser/password_form_manager_unittest.cc
+++ b/components/password_manager/core/browser/password_form_manager_unittest.cc
@@ -184,7 +184,12 @@ class PasswordFormManagerTest : public testing::Test {
 
   // Types of possible outcomes of simulated matching, see
   // SimulateMatchingPhase.
-  enum ResultOfSimulatedMatching { RESULT_MATCH_FOUND, RESULT_NO_MATCH };
+  enum ResultOfSimulatedMatching {
+    RESULT_MATCH_FOUND,
+    RESULT_NO_MATCH,
+    RESULT_PSL_FOUND,
+    RESULT_FULL_AND_PSL_FOUND

[vabr (Chromium), 2015/09/24 15:17:33]

nit: Maybe it is time to convert this to a bit mask?
enum ResultOfSimulatedMatching {
  RESULT_SAVED_MATCH = 1 << 0,  // Include saved_match_ in store results.
  RESULT_PSL_MATCH = 1 << 1,  // Include psl_saved_match_ in store results.
}

[dvadym, 2015/09/25 10:19:52]

Thanks, it makes sense, done

+  };
 
   void SetUp() override {
     observed_form_.origin = GURL("http://accounts.google.com/a/LoginAuth");
@@ -203,6 +208,11 @@ class PasswordFormManagerTest : public testing::Test {
     saved_match_.other_possible_usernames.push_back(
         ASCIIToUTF16("test2@gmail.com"));
 
+    psl_saved_match_ = saved_match_;
+    psl_saved_match_.is_public_suffix_match = true;
+    psl_saved_match_.origin =
+        GURL("http://m.accounts.google.com/a/ServiceLoginAuth");
+
     autofill::FormFieldData field;
     field.label = ASCIIToUTF16("full_name");
     field.name = ASCIIToUTF16("full_name");
@@ -245,9 +255,16 @@ class PasswordFormManagerTest : public testing::Test {
       return;
     }
 
-    scoped_ptr<PasswordForm> match(new PasswordForm(saved_match_));
+    scoped_ptr<PasswordForm> match;
     ScopedVector<PasswordForm> result_form;
-    result_form.push_back(match.Pass());
+    if (result != RESULT_PSL_FOUND) {
+      match.reset(new PasswordForm(saved_match_));
+      result_form.push_back(match.Pass());
+    }
+    if (result != RESULT_MATCH_FOUND) {
+      match.reset(new PasswordForm(psl_saved_match_));
+      result_form.push_back(match.Pass());
+    }
     p->OnGetPasswordStoreResults(result_form.Pass());
   }
 
@@ -311,6 +328,7 @@ class PasswordFormManagerTest : public testing::Test {
 
   PasswordForm* observed_form() { return &observed_form_; }
   PasswordForm* saved_match() { return &saved_match_; }
+  PasswordForm* psl_saved_match() { return &psl_saved_match_; }
   PasswordForm* CreateSavedMatch(bool blacklisted) {
     // Owned by the caller of this method.
     PasswordForm* match = new PasswordForm(saved_match_);
@@ -333,6 +351,7 @@ class PasswordFormManagerTest : public testing::Test {
 
   PasswordForm observed_form_;
   PasswordForm saved_match_;
+  PasswordForm psl_saved_match_;
   scoped_refptr<NiceMock<MockPasswordStore>> mock_store_;
   scoped_ptr<TestPasswordManagerClient> client_;
   scoped_ptr<PasswordManager> password_manager_;
@@ -442,7 +461,8 @@ TEST_F(PasswordFormManagerTest, TestBlacklistMatching) {
 
   // Doesn't match because of PSL.
   PasswordForm blacklisted_psl = *observed_form();
-  blacklisted_psl.original_signon_realm = "http://m.accounts.google.com";
+  blacklisted_psl.signon_realm = "http://m.accounts.google.com";
+  blacklisted_psl.is_public_suffix_match = true;
   blacklisted_psl.blacklisted_by_user = true;
 
   // Doesn't match because of different origin.
@@ -508,7 +528,7 @@ TEST_F(PasswordFormManagerTest, AutofillBlacklisted) {
 TEST_F(PasswordFormManagerTest,
        OverriddenPSLMatchedCredentialsNotMarkedAsPSLMatched) {
   // The suggestion needs to be PSL-matched.
-  saved_match()->original_signon_realm = "www.example.org";
+  saved_match()->is_public_suffix_match = true;
   SimulateMatchingPhase(form_manager(), RESULT_MATCH_FOUND);
 
   // User modifies the suggested password and submits the form.
@@ -525,7 +545,7 @@ TEST_F(PasswordFormManagerTest,
 
 TEST_F(PasswordFormManagerTest, PSLMatchedCredentialsMetadataUpdated) {
   // The suggestion needs to be PSL-matched.
-  saved_match()->original_signon_realm = "www.example.org";
+  saved_match()->is_public_suffix_match = true;
   SimulateMatchingPhase(form_manager(), RESULT_MATCH_FOUND);
 
   PasswordForm submitted_form(*observed_form());
@@ -539,9 +559,9 @@ TEST_F(PasswordFormManagerTest, PSLMatchedCredentialsMetadataUpdated) {
   expected_saved_form.times_used = 1;
   expected_saved_form.other_possible_usernames.clear();
   expected_saved_form.form_data = saved_match()->form_data;
-  expected_saved_form.origin = saved_match()->origin;
-  expected_saved_form.original_signon_realm =
-      saved_match()->original_signon_realm;
+  expected_saved_form.origin = observed_form()->origin;
+  expected_saved_form.is_public_suffix_match =
+      saved_match()->is_public_suffix_match;
   PasswordForm actual_saved_form;
 
   EXPECT_CALL(*(client()->mock_driver()->mock_autofill_manager()),
@@ -1056,10 +1076,10 @@ TEST_F(PasswordFormManagerTest,
   ScopedVector<PasswordForm> simulated_results;
   simulated_results.push_back(CreateSavedMatch(false));
   simulated_results.push_back(CreateSavedMatch(false));
-  simulated_results[1]->username_value = ASCIIToUTF16("other@gmail.com");
-  simulated_results[1]->password_element = ASCIIToUTF16("signup_password");
-  simulated_results[1]->username_element = ASCIIToUTF16("signup_username");
-  simulated_results[1]->type = PasswordForm::TYPE_GENERATED;
+  simulated_results[0]->username_value = ASCIIToUTF16("other@gmail.com");

[vabr (Chromium), 2015/09/24 15:17:33]

Why this change (1->0)?

[dvadym, 2015/09/25 10:19:52]

Now we take the first form from the ones that matches the best, before we took
the last one. For details see password_form_manager.cc:460 in this CL.

+  simulated_results[0]->password_element = ASCIIToUTF16("signup_password");
+  simulated_results[0]->username_element = ASCIIToUTF16("signup_username");
+  simulated_results[0]->type = PasswordForm::TYPE_GENERATED;
   form_manager()->SimulateFetchMatchingLoginsFromPasswordStore();
 
   autofill::PasswordFormFillData fill_data;
@@ -1202,12 +1222,12 @@ TEST_F(PasswordFormManagerTest, TestScoringPublicSuffixMatch) {
   // Simulate having two matches for this form, first comes from different
   // signon realm, but reports the same origin and action as matched form.
   // Second candidate has the same signon realm as the form, but has a different
-  // origin and action. Public suffix match is the most important criterion so
-  // the second candidate should be selected.
+  // origin and action. Public suffix match is not taken into consideration

[vabr (Chromium), 2015/09/24 15:17:33]

How come? Line 988 in ScoreResult in your patch still contains the bonus points
for not being a PSL match.

[dvadym, 2015/09/25 10:19:52]

I meant that the property to be public suffix match gives 0 to score. That I
believe is not correct, but it's another story. But I agree that
!IsPublicSuffixMatch gives results. So I've returned back previous comment.

+  // during scoring. So the second candidate is taken.
   ScopedVector<PasswordForm> simulated_results;
   simulated_results.push_back(CreateSavedMatch(false));
   simulated_results.push_back(CreateSavedMatch(false));
-  simulated_results[0]->original_signon_realm = "http://accounts2.google.com";
+  simulated_results[0]->is_public_suffix_match = true;
   simulated_results[1]->origin =
       GURL("http://accounts.google.com/a/ServiceLoginAuth2");
   simulated_results[1]->action =
@@ -1221,10 +1241,8 @@ TEST_F(PasswordFormManagerTest, TestScoringPublicSuffixMatch) {
   form_manager()->OnGetPasswordStoreResults(simulated_results.Pass());
   EXPECT_TRUE(fill_data.additional_logins.empty());
   EXPECT_EQ(1u, form_manager()->best_matches().size());
-  EXPECT_TRUE(form_manager()
-                  ->best_matches()
-                  .begin()
-                  ->second->original_signon_realm.empty());
+  EXPECT_TRUE(
+      !form_manager()->best_matches().begin()->second->IsPublicSuffixMatch());
 }
 
 TEST_F(PasswordFormManagerTest, AndroidCredentialsAreAutofilled) {
@@ -1235,9 +1253,7 @@ TEST_F(PasswordFormManagerTest, AndroidCredentialsAreAutofilled) {
   // filled on username-select.
   ScopedVector<PasswordForm> simulated_results;
   simulated_results.push_back(new PasswordForm());
-  simulated_results[0]->signon_realm = observed_form()->signon_realm;
-  simulated_results[0]->original_signon_realm =
-      "android://hash@com.google.android";
+  simulated_results[0]->signon_realm = "android://hash@com.google.android";
   simulated_results[0]->origin = observed_form()->origin;
   simulated_results[0]->username_value = saved_match()->username_value;
   simulated_results[0]->password_value = saved_match()->password_value;
@@ -1381,9 +1397,9 @@ TEST_F(PasswordFormManagerTest, CorrectlyUpdatePasswordsWithSameUsername) {
   form_manager()->SimulateFetchMatchingLoginsFromPasswordStore();
   form_manager()->OnGetPasswordStoreResults(result.Pass());
 
-  // We always take the last credential with a particular username, regardless
+  // We always take the first credential with a particular username, regardless
   // of which ones are labeled preferred.
-  EXPECT_EQ(ASCIIToUTF16("second"),
+  EXPECT_EQ(ASCIIToUTF16("first"),
             form_manager()->preferred_match()->password_value);
 
   PasswordForm login(*observed_form());
@@ -1924,7 +1940,7 @@ TEST_F(PasswordFormManagerTest, RemoveNoUsernameAccounts) {
 TEST_F(PasswordFormManagerTest, NotRemovePSLNoUsernameAccounts) {
   PasswordForm saved_form = *saved_match();
   saved_form.username_value.clear();
-  saved_form.original_signon_realm = "www.example.org";
+  saved_form.is_public_suffix_match = true;
   ScopedVector<PasswordForm> result;
   result.push_back(new PasswordForm(saved_form));
   form_manager()->SimulateFetchMatchingLoginsFromPasswordStore();
@@ -2202,4 +2218,158 @@ TEST_F(PasswordFormManagerTest, UpdateFormManagers_IsCalled) {
   form_manager()->Save();
 }
 
+TEST_F(PasswordFormManagerTest, TestUpdatePSLMatchedCredentials) {
+  PasswordFormManager form_manager(password_manager(), client(),
+                                   client()->driver(), *observed_form(), false);
+  SimulateMatchingPhase(&form_manager, RESULT_FULL_AND_PSL_FOUND);
+
+  // User submits current and new credentials to the observed form.

[vabr (Chromium), 2015/09/24 15:17:33]

What do you mean by "current and new credentials"?

[dvadym, 2015/09/25 10:19:52]

Thanks, it's left from the test I've copied.

+  PasswordForm credentials(*observed_form());
+  credentials.username_value = saved_match()->username_value;
+  credentials.password_value =
+      saved_match()->password_value + ASCIIToUTF16("1");
+  credentials.preferred = true;
+  form_manager.ProvisionallySave(
+      credentials, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
+
+  // Successful login. The PasswordManager would instruct PasswordFormManager
+  // to save, and since this is an update, it should know not to save as a new
+  // login.
+  EXPECT_FALSE(form_manager.IsNewLogin());
+
+  // Trigger saving to exercise some special case handling in UpdateLogin().
+  PasswordForm new_credentials[2];
+  EXPECT_CALL(*mock_store(), UpdateLogin(_))
+      .WillOnce(testing::SaveArg<0>(&new_credentials[0]))
+      .WillOnce(testing::SaveArg<0>(&new_credentials[1]));
+
+  form_manager.Save();
+  Mock::VerifyAndClearExpectations(mock_store());
+
+  // No meta-information should be updated, only the password.
+  EXPECT_EQ(credentials.password_value, new_credentials[0].password_value);
+  EXPECT_EQ(saved_match()->username_value, new_credentials[0].username_value);
+  EXPECT_EQ(saved_match()->username_element,
+            new_credentials[0].username_element);
+  EXPECT_EQ(saved_match()->password_element,
+            new_credentials[0].password_element);
+  EXPECT_EQ(saved_match()->origin, new_credentials[0].origin);
+
+  EXPECT_EQ(credentials.password_value, new_credentials[1].password_value);
+  EXPECT_EQ(psl_saved_match()->username_element,
+            new_credentials[1].username_element);
+  EXPECT_EQ(psl_saved_match()->username_element,
+            new_credentials[1].username_element);
+  EXPECT_EQ(psl_saved_match()->password_element,
+            new_credentials[1].password_element);
+  EXPECT_EQ(psl_saved_match()->origin, new_credentials[1].origin);
+}
+
+TEST_F(PasswordFormManagerTest,
+       TestNotUpdatePSLMatchedCredentialsWithAnotherUsername) {
+  PasswordFormManager form_manager(password_manager(), client(),
+                                   client()->driver(), *observed_form(), false);
+  psl_saved_match()->username_value += ASCIIToUTF16("1");
+  SimulateMatchingPhase(&form_manager, RESULT_FULL_AND_PSL_FOUND);
+
+  // User submits current and new credentials to the observed form.
+  PasswordForm credentials(*observed_form());
+  credentials.username_value = saved_match()->username_value;
+  credentials.password_value =
+      saved_match()->password_value + ASCIIToUTF16("1");
+  credentials.preferred = true;
+  form_manager.ProvisionallySave(
+      credentials, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
+
+  // Successful login. The PasswordManager would instruct PasswordFormManager
+  // to save, and since this is an update, it should know not to save as a new
+  // login.
+  EXPECT_FALSE(form_manager.IsNewLogin());
+
+  // Trigger saving to exercise some special case handling in UpdateLogin().
+  PasswordForm new_credentials;
+  EXPECT_CALL(*mock_store(), UpdateLogin(_))
+      .WillOnce(testing::SaveArg<0>(&new_credentials));
+
+  form_manager.Save();
+  Mock::VerifyAndClearExpectations(mock_store());
+
+  // No meta-information should be updated, only the password.
+  EXPECT_EQ(credentials.password_value, new_credentials.password_value);
+  EXPECT_EQ(saved_match()->username_value, new_credentials.username_value);
+  EXPECT_EQ(saved_match()->password_element, new_credentials.password_element);
+  EXPECT_EQ(saved_match()->username_element, new_credentials.username_element);
+  EXPECT_EQ(saved_match()->origin, new_credentials.origin);
+}
+
+TEST_F(PasswordFormManagerTest,
+       TestNotUpdatePSLMatchedCredentialsWithAnotherPassword) {
+  PasswordFormManager form_manager(password_manager(), client(),
+                                   client()->driver(), *observed_form(), false);
+  psl_saved_match()->password_value += ASCIIToUTF16("2");
+  SimulateMatchingPhase(&form_manager, RESULT_FULL_AND_PSL_FOUND);
+
+  // User submits current and new credentials to the observed form.
+  PasswordForm credentials(*observed_form());
+  credentials.username_value = saved_match()->username_value;
+  credentials.password_value =
+      saved_match()->password_value + ASCIIToUTF16("1");
+  credentials.preferred = true;
+  form_manager.ProvisionallySave(
+      credentials, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
+
+  // Successful login. The PasswordManager would instruct PasswordFormManager
+  // to save, and since this is an update, it should know not to save as a new
+  // login.
+  EXPECT_FALSE(form_manager.IsNewLogin());
+
+  // Trigger saving to exercise some special case handling in UpdateLogin().
+  PasswordForm new_credentials;
+  EXPECT_CALL(*mock_store(), UpdateLogin(_))
+      .WillOnce(testing::SaveArg<0>(&new_credentials));
+
+  form_manager.Save();
+  Mock::VerifyAndClearExpectations(mock_store());
+
+  // No meta-information should be updated, only the password.
+  EXPECT_EQ(credentials.password_value, new_credentials.password_value);
+  EXPECT_EQ(saved_match()->username_value, new_credentials.username_value);
+  EXPECT_EQ(saved_match()->password_element, new_credentials.password_element);
+  EXPECT_EQ(saved_match()->username_element, new_credentials.username_element);
+  EXPECT_EQ(saved_match()->origin, new_credentials.origin);
+}
+
+TEST_F(PasswordFormManagerTest, TestNotUpdateWhenOnlyPSLMatched) {
+  PasswordFormManager form_manager(password_manager(), client(),
+                                   client()->driver(), *observed_form(), false);
+  SimulateMatchingPhase(&form_manager, RESULT_PSL_FOUND);
+
+  // User submits current and new credentials to the observed form.
+  PasswordForm credentials(*observed_form());
+  credentials.username_value = saved_match()->username_value;
+  credentials.password_value =
+      saved_match()->password_value + ASCIIToUTF16("1");
+  credentials.preferred = true;
+  form_manager.ProvisionallySave(
+      credentials, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
+
+  EXPECT_TRUE(form_manager.IsNewLogin());
+
+  // PSL matched credential should not be updated, since we are not sure that
+  // this is the same credential as submitted one.
+  PasswordForm new_credentials;
+  EXPECT_CALL(*mock_store(), UpdateLogin(_)).Times(0);
+  EXPECT_CALL(*mock_store(), AddLogin(_))
+      .WillOnce(testing::SaveArg<0>(&new_credentials));
+
+  form_manager.Save();
+  Mock::VerifyAndClearExpectations(mock_store());
+
+  EXPECT_EQ(credentials.password_value, new_credentials.password_value);
+  EXPECT_EQ(credentials.username_value, new_credentials.username_value);
+  EXPECT_EQ(credentials.password_element, new_credentials.password_element);
+  EXPECT_EQ(credentials.username_element, new_credentials.username_element);
+  EXPECT_EQ(credentials.origin, new_credentials.origin);
+}
+
 }  // namespace password_manager