[PasswordManager] Improve detection of ignorable change password forms.

components/password_manager/core/browser/password_form_manager.cc

Index: components/password_manager/core/browser/password_form_manager.cc
diff --git a/components/password_manager/core/browser/password_form_manager.cc b/components/password_manager/core/browser/password_form_manager.cc
index 096257b8d1a3114a819949087cb7350fd5c5c466..d99fe6d61ced64efa4269b750ef35d038f3d7632 100644
--- a/components/password_manager/core/browser/password_form_manager.cc
+++ b/components/password_manager/core/browser/password_form_manager.cc
@@ -7,6 +7,7 @@
 #include <algorithm>
 #include <set>
 
+#include "base/command_line.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/user_metrics.h"
 #include "base/strings/string_split.h"
@@ -15,6 +16,7 @@
 #include "components/autofill/core/browser/autofill_manager.h"
 #include "components/autofill/core/browser/form_structure.h"
 #include "components/autofill/core/browser/validation.h"
+#include "components/autofill/core/common/autofill_switches.h"
 #include "components/autofill/core/common/password_form.h"
 #include "components/password_manager/core/browser/browser_save_password_progress_logger.h"
 #include "components/password_manager/core/browser/password_manager.h"
@@ -68,6 +70,11 @@ PasswordForm CopyAndModifySSLValidity(const PasswordForm& orig,
   return result;
 }
 
+bool IsChangePasswordForm(const PasswordForm& candidate) {
+  return !candidate.new_password_element.empty() &&
+         !candidate.password_element.empty();
+}
+
 }  // namespace
 
 PasswordFormManager::PasswordFormManager(
@@ -246,13 +253,15 @@ void PasswordFormManager::ProvisionallySave(
   base::string16 password_to_save(credentials.new_password_element.empty() ?
       credentials.password_value : credentials.new_password_value);
 
+  bool is_username_certainly_correct = credentials.username_marked_by_site;

[vabr (Chromium), 2015/02/02 14:18:49]

nit: Because credentials.username_marked_by_site is not much longer than
is_username_certainly_correct, and you use that bool only once here anyway,
let's just get rid of the bool and use credentials.username_marked_by_site in
its place instead.

[Pritam Nikam, 2015/02/05 06:12:07]

Done.

+
   // Make sure the important fields stay the same as the initially observed or
   // autofilled ones, as they may have changed if the user experienced a login
   // failure.
   // Look for these credentials in the list containing auto-fill entries.
   PasswordFormMap::const_iterator it =
       best_matches_.find(credentials.username_value);
-  if (it != best_matches_.end()) {
+  if (!is_username_certainly_correct && it != best_matches_.end()) {

[vabr (Chromium), 2015/02/02 14:18:49]

Don't narrow the condition this way -- observe that you are changing the
behaviour for all types of forms (not just change-password forms), as long as
they have the autocomplete=username markup.

[Pritam Nikam, 2015/02/05 06:12:06]

Done.

     // The user signed in with a login we autofilled.
     pending_credentials_ = *it->second;
     bool password_changed =
@@ -303,6 +312,16 @@ void PasswordFormManager::ProvisionallySave(
       is_new_login_ = false;
       if (password_changed)
         user_action_ = kUserActionOverridePassword;
+
+      // Check whether its a change-password form with matching credentials in
+      // stored form.
+      PasswordForm matching_form;
+      if (IsChangePasswordForm(credentials) &&
+          HasMatchingCredentialsInStore(credentials, &matching_form)) {

[vabr (Chromium), 2015/02/02 14:18:49]

This is actually unnecessary -- you already know that the single form among
best_matches_, which matches credentials' username is *it->second, already
stored as pending_crendentials_ on line 266. So just check the password.

[Pritam Nikam, 2015/02/05 06:12:07]

Done.

+        pending_credentials_ = matching_form;

[vabr (Chromium), 2015/02/02 14:18:50]

Save the copy -- just pass pending_credentials_ to HasMatchingCredentialsInStore
and get rid of matching_form. 

[vabr (Chromium), 2015/02/02 14:18:50]

Given the other comments, the modification needed to perform on
pending_credentials_ boils down to updating the password. But that is done on
line 365 below.

[Pritam Nikam, 2015/02/05 06:12:06]

Done.

[Pritam Nikam, 2015/02/05 06:12:06]

Done.

+        selected_username_ = matching_form.username_value;

[vabr (Chromium), 2015/02/02 14:18:50]

Why do you set this? According to password_form_manager.h, this is set "if the
user has selected one of the other possible usernames". However, line 263 tells
me that the username of |credentials| matches one suggested in |best_matches_|.

[Pritam Nikam, 2015/02/05 06:12:06]

Done.

+        user_action_ = kUserActionOverridePassword;

[vabr (Chromium), 2015/02/02 14:18:49]

How do you know the password is overridden? Shouldn't you check that?

[Pritam Nikam, 2015/02/05 06:12:07]

Done.

+      }
     }
   } else if (action == ALLOW_OTHER_POSSIBLE_USERNAMES &&
              UpdatePendingCredentialsIfOtherPossibleUsername(
@@ -325,10 +344,11 @@ void PasswordFormManager::ProvisionallySave(
     pending_credentials_.password_value.clear();
     pending_credentials_.new_password_value.clear();
 
-    // If this was a sign-up or change password form, the names of the elements
-    // are likely different than those on a login form, so do not bother saving
-    // them. We will fill them with meaningful values in UpdateLogin() when the
-    // user goes onto a real login form for the first time.
+    // If this was a sign-up or change password form, the names of the
+    // elements are likely different than those on a login form, so do not

[vabr (Chromium), 2015/02/02 14:18:49]

Please do not just reformat the comments, if you don't change them.

[Pritam Nikam, 2015/02/05 06:12:06]

Done.

+    // bother saving them. We will fill them with meaningful values in
+    // UpdateLogin() when the user goes onto a real login form for the first
+    // time.
     if (!credentials.new_password_element.empty()) {
       pending_credentials_.password_element.clear();
       pending_credentials_.new_password_element.clear();
@@ -378,7 +398,9 @@ void PasswordFormManager::FetchMatchingLoginsFromPasswordStore(
 
   // Do not autofill on sign-up or change password forms (until we have some
   // working change password functionality).
-  if (!observed_form_.new_password_element.empty()) {
+  if (!observed_form_.new_password_element.empty() &&
+      !base::CommandLine::ForCurrentProcess()->HasSwitch(

[vabr (Chromium), 2015/02/02 14:18:49]

This change looks like a bad merge, and nothing to do with your CL?
Also applies to the 2 new #includes at the beginning of this file.

[Pritam Nikam, 2015/02/05 06:12:06]

Done.

+          autofill::switches::kEnablePasswordSaveOnInPageNavigation)) {
     if (logger)
       logger->LogMessage(Logger::STRING_FORM_NOT_AUTOFILLED);
     client_->AutofillResultsComputed();
@@ -404,10 +426,8 @@ bool PasswordFormManager::HasCompletedMatching() const {
 }
 
 bool PasswordFormManager::IsIgnorableChangePasswordForm() const {
-  bool is_change_password_form = !observed_form_.new_password_element.empty() &&
-                                 !observed_form_.password_element.empty();
   bool is_username_certainly_correct = observed_form_.username_marked_by_site;
-  return is_change_password_form && !is_username_certainly_correct;
+  return IsChangePasswordForm(observed_form_) && !is_username_certainly_correct;
 }
 
 void PasswordFormManager::OnRequestDone(
@@ -868,4 +888,21 @@ void PasswordFormManager::SubmitFailed() {
     LogPasswordGenerationSubmissionEvent(PASSWORD_SUBMISSION_FAILED);
 }
 
+bool PasswordFormManager::HasMatchingCredentialsInStore(
+    const autofill::PasswordForm& candidate,
+    autofill::PasswordForm* matching_form) const {
+  bool match_found = false;
+  for (const auto& match : best_matches_) {
+    if (match.second->username_value == candidate.username_value &&
+        match.second->password_value == candidate.password_value) {
+      if (matching_form)

[vabr (Chromium), 2015/02/02 14:18:49]

There should also be a check that the candidate has the same realm as the forms
in best_matches_. You can compare the realms with IsPublicSuffixDomainMatch from
components/password_manager/core/browser/psl_matching_helper.h.

Alternatively, because this is a private function, and it can be reasoned that
it is always applied to forms with the same realm (because of all forms being
related to the same PasswordFormManager instance), you can make that check a
DCHECK, and add a comment with the expectation about dandidate.signon_realm
expectation at the method declaration.

[Pritam Nikam, 2015/02/05 06:12:06]

Done. Removed this function, instead performed in-place check for password
value.

+        *matching_form = *(match.second);
+      match_found = true;
+      break;
+    }
+  }
+
+  return match_found;
+}
+
 }  // namespace password_manager