[PasswordManager] Improve detection of ignorable change password forms.

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 30b7e325daeac7c635ff8627f91dfc9ba0776955..c71aeed23e06c406b0e95a07b2d6922c556bceae 100644
--- a/components/password_manager/core/browser/password_form_manager_unittest.cc
+++ b/components/password_manager/core/browser/password_form_manager_unittest.cc
@@ -392,6 +392,7 @@ TEST_F(PasswordFormManagerTest, TestNewLoginFromNewPasswordElement) {
   // Add a new password field to the test form. The PasswordFormManager should
   // save the password from this field, instead of the current password field.
   observed_form()->new_password_element = ASCIIToUTF16("NewPasswd");
+  observed_form()->username_marked_by_site = true;
 
   PasswordFormManager manager(NULL, client(), kNoDriver, *observed_form(),
                               false);
@@ -1343,4 +1344,101 @@ TEST_F(PasswordFormManagerTest, DriverDeletedBeforeStoreDone) {
   form_manager.OnGetPasswordStoreResults(results);
 }
 
+TEST_F(PasswordFormManagerTest,
+       SubmitIngnorableChangePasswordForm_MatchingUsernameAndPassword) {
+  observed_form()->new_password_element =
+      base::ASCIIToUTF16("new_password_field");
+
+  TestPasswordManagerClient client_with_store(mock_store());
+  PasswordFormManager manager(NULL, &client_with_store,

[vabr (Chromium), 2015/02/10 18:54:57]

NULL -> nullptr
Also below.
(See http://chromium-cpp.appspot.com/.)

[Pritam Nikam, 2015/02/19 11:18:48]

Done.

+                              client_with_store.driver(), *observed_form(),
+                              false);
+  EXPECT_CALL(*client_with_store.mock_driver(), IsOffTheRecord())

[vabr (Chromium), 2015/02/10 18:54:57]

Does the test really expect that call?
If this is only about returning false, let's use
ON_CALL(...).WillByDefault(Return(false));

Also in the new tests below.

(I know that this is just a copy of these lines from other parts of the
unittest, but let's leave cleaning the rest up for different CLs, and only make
sure what we add is right.)

[Pritam Nikam, 2015/02/19 11:18:48]

Done.

+      .WillRepeatedly(Return(false));
+  SimulateMatchingPhase(&manager, RESULT_MATCH_FOUND);
+
+  // User submits ignorable change-password form (i.e. without

[vabr (Chromium), 2015/02/10 18:54:57]

Suggested rewording, because "ignorable" has unclear meaning here:

The user submits a password on a change-password form, which does not use the
"autocomplete=username" mark-up (therefore Chrome had to guess what is the
username), but the user-typed credentials match something already stored (which
confirms that the guess was right).

[Pritam Nikam, 2015/02/19 11:18:48]

Done.

+  // *autocomplete=username* markup) having matching username and password to
+  // the stored form.
+  PasswordForm credentials(*observed_form());
+  credentials.username_value = saved_match()->username_value;
+  credentials.password_value = saved_match()->password_value;
+  credentials.new_password_value = ASCIIToUTF16("NewPassword");
+  credentials.preferred = true;

[vabr (Chromium), 2015/02/10 18:54:57]

Does the test actually need |credentials| to be preferred?

[Pritam Nikam, 2015/02/19 11:18:48]

Done.

+  manager.ProvisionallySave(
+      credentials, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
+
+  // Successful match found. The PasswordManager should instruct
+  // PasswordFormManager to overwrite the password for stored form.
+  EXPECT_FALSE(manager.IsNewLogin());
+
+  EXPECT_EQ(credentials.signon_realm,
+            GetPendingCredentials(&manager)->signon_realm);
+  EXPECT_EQ(credentials.username_value,
+            GetPendingCredentials(&manager)->username_value);
+
+  // By this point, the PasswordFormManager should have overwritten the new
+  // password value to be the current password.
+  EXPECT_EQ(credentials.new_password_value,
+            GetPendingCredentials(&manager)->password_value);
+}
+
+TEST_F(PasswordFormManagerTest,
+       SubmitIngnorableChangePasswordForm_NotMatchingPassword) {
+  observed_form()->new_password_element =
+      base::ASCIIToUTF16("new_password_field");
+
+  TestPasswordManagerClient client_with_store(mock_store());
+  PasswordFormManager manager(NULL, &client_with_store,
+                              client_with_store.driver(), *observed_form(),
+                              false);
+  EXPECT_CALL(*client_with_store.mock_driver(), IsOffTheRecord())
+      .WillRepeatedly(Return(false));
+  SimulateMatchingPhase(&manager, RESULT_MATCH_FOUND);
+
+  // User submits ignorable change-password form (i.e. without

[vabr (Chromium), 2015/02/10 18:54:57]

Suggested rewording:

The user submits a password on a change-password form, which does not use the
"autocomplete=username" mark-up (therefore Chrome had to guess what is the
username), and the user-typed password do not match anything already stored.
There is not much confidence in the guess being right, so the password should
not be stored.

[Pritam Nikam, 2015/02/19 11:18:48]

Done.

+  // *autocomplete=username* markup) having non-matching password to the stored
+  // form.
+  PasswordForm credentials(*observed_form());
+  credentials.username_value = saved_match()->username_value;
+  credentials.password_value = ASCIIToUTF16("DifferntPassword");
+  credentials.new_password_value = ASCIIToUTF16("NewPassword");
+  credentials.preferred = true;
+  manager.ProvisionallySave(
+      credentials, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
+
+  // No match found. Password manager shall not trigger the save or update the

[vabr (Chromium), 2015/02/10 18:54:57]

Actually, I would be surprised if this test worked (once you fix the EXPECT_CALL
as outlined below).
That's because the decision not to save the password is done in PasswordManager,
not PasswordFormManager. Normally, the call from line 1407 would not happen. But
once it does happen, the expectations below will not be met.

I'm afraid you'll need to find a way to move all these three tests to password
manager unittest, because it is actually PasswordManager we are testing here.

[Pritam Nikam, 2015/02/19 11:18:48]

Instead we will test the public function IsIgnorableChangePasswordForm() and if
it is returning false i.e. its not an ignorable credentials; we will go ahead
and test ProvisionallySave().

+  // credentials.
+  EXPECT_FALSE(manager.IsNewLogin());

[vabr (Chromium), 2015/02/10 18:54:57]

I suggest dropping this line. It seems to just test that the default value for
IsNewLogin is false, which is not what this test should do.

[Pritam Nikam, 2015/02/19 11:18:48]

Done.

+  EXPECT_CALL(*mock_store(), UpdateLogin(_)).Times(testing::Exactly(0));

[vabr (Chromium), 2015/02/10 18:54:57]

You need to put this expectation before the ProvisionallySave call, and also you
need to run RunAllPendingTasks() after ProvisionallySave, to be sure that the
password store query would have been run.

[Pritam Nikam, 2015/02/19 11:18:48]

Done.

+}
+
+TEST_F(PasswordFormManagerTest,
+       SubmitIngnorableChangePasswordForm_NotMatchingUsername) {
+  observed_form()->new_password_element =
+      base::ASCIIToUTF16("new_password_field");
+
+  TestPasswordManagerClient client_with_store(mock_store());
+  PasswordFormManager manager(NULL, &client_with_store,
+                              client_with_store.driver(), *observed_form(),
+                              false);
+  EXPECT_CALL(*client_with_store.mock_driver(), IsOffTheRecord())
+      .WillRepeatedly(Return(false));
+  SimulateMatchingPhase(&manager, RESULT_MATCH_FOUND);
+
+  // User submits ignorable change-password form (i.e. without

[vabr (Chromium), 2015/02/10 18:54:57]

Suggested rewording:

The user submits a password on a change-password form, which does not use the
"autocomplete=username" mark-up (therefore Chrome had to guess what is the
username), and the user-typed username does not match anything already stored.
There is not much confidence in the guess being right, so the password should
not be stored.

[Pritam Nikam, 2015/02/19 11:18:48]

Done.

+  // *autocomplete=username* markup) having non-matching username to the stored
+  // form.
+  PasswordForm credentials(*observed_form());
+  credentials.username_value = ASCIIToUTF16("DifferntUsername");
+  credentials.password_value = saved_match()->password_value;
+  credentials.new_password_value = ASCIIToUTF16("NewPassword");
+  credentials.preferred = true;
+  manager.ProvisionallySave(
+      credentials, PasswordFormManager::IGNORE_OTHER_POSSIBLE_USERNAMES);
+
+  // No matching username found. User typed in a new, unknown username.
+  EXPECT_TRUE(manager.IsNewLogin());
+}
+
 }  // namespace password_manager