[PasswordManager] Improve detection of ignorable change password forms.

components/password_manager/core/browser/password_form_manager.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/password_manager/core/browser/password_form_manager.h"
 
 #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"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #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"
 #include "components/password_manager/core/browser/password_manager_client.h"
 #include "components/password_manager/core/browser/password_manager_driver.h"
 #include "components/password_manager/core/browser/password_store.h"
 
 using autofill::FormStructure;
 using autofill::PasswordForm;
 using autofill::PasswordFormMap;
@@ skipping 33 matching lines @@
                             event, SUBMISSION_EVENT_ENUM_COUNT);
 }
 
 PasswordForm CopyAndModifySSLValidity(const PasswordForm& orig,
                                       bool ssl_valid) {
   PasswordForm result(orig);
   result.ssl_valid = ssl_valid;
   return result;
 }
 
+bool IsChangePasswordForm(const PasswordForm& candidate) {
+  return !candidate.new_password_element.empty() &&
+         !candidate.password_element.empty();
+}
+
 }  // namespace
 
 PasswordFormManager::PasswordFormManager(
     PasswordManager* password_manager,
     PasswordManagerClient* client,
     const base::WeakPtr<PasswordManagerDriver>& driver,
     const PasswordForm& observed_form,
     bool ssl_valid)
     : best_matches_deleter_(&best_matches_),
       observed_form_(CopyAndModifySSLValidity(observed_form, ssl_valid)),
@@ skipping 158 matching lines @@
   DCHECK_NE(RESULT_NO_MATCH, DoesManage(credentials));
 
   // If this was a sign-up or change password form, we want to persist the new
   // password; if this was a login form, then the current password (which might
   // still be "new" in the sense that we see these credentials for the first
   // time, or that the user manually entered his actual password to overwrite an
   // obsolete password we had in the store).
   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 =
         pending_credentials_.password_value != password_to_save;
     if (IsPendingCredentialsPublicSuffixMatch()) {
       // If the autofilled credentials were only a PSL match, store a copy with
       // the current origin and signon realm. This ensures that on the next
       // visit, a precise match is found.
       is_new_login_ = true;
       user_action_ = password_changed ? kUserActionChoosePslMatch
@@ skipping 30 matching lines @@
       // infrequently, and the inconvenience put on the user by asking them is
       // not significant, so we are fine with asking here again.
       if (password_changed) {
         pending_credentials_.original_signon_realm.clear();
         DCHECK(!IsPendingCredentialsPublicSuffixMatch());
       }
     } else {  // Not a PSL match.
       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(
                  credentials.username_value)) {
     // |pending_credentials_| is now set. Note we don't update
     // |pending_credentials_.username_value| to |credentials.username_value|
     // yet because we need to keep the original username to modify the stored
     // credential.
     selected_username_ = credentials.username_value;
     is_new_login_ = false;
   } else {
     // User typed in a new, unknown username.
     user_action_ = kUserActionOverrideUsernameAndPassword;
     pending_credentials_ = observed_form_;
     pending_credentials_.username_value = credentials.username_value;
     pending_credentials_.other_possible_usernames =
         credentials.other_possible_usernames;
 
     // The password value will be filled in later, remove any garbage for now.
     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();
     }
   }
 
   pending_credentials_.action = credentials.action;
   // If the user selected credentials we autofilled from a PasswordForm
   // that contained no action URL (IE6/7 imported passwords, for example),
   // bless it with the action URL from the observed form. See bug 1107719.
@@ skipping 29 matching lines @@
   state_ = MATCHING_PHASE;
 
   scoped_ptr<BrowserSavePasswordProgressLogger> logger;
   if (client_->IsLoggingActive()) {
     logger.reset(new BrowserSavePasswordProgressLogger(client_));
     logger->LogMessage(Logger::STRING_FETCH_LOGINS_METHOD);
   }
 
   // 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();
     // There is no point in looking for the credentials in the store when they
     // won't be autofilled, so pretend there were none.
     std::vector<autofill::PasswordForm*> dummy_results;
     OnGetPasswordStoreResults(dummy_results);
     return;
   }
 
   PasswordStore* password_store = client_->GetPasswordStore();
   if (!password_store) {
     if (logger)
       logger->LogMessage(Logger::STRING_NO_STORE);
     NOTREACHED();
     return;
   }
   password_store->GetLogins(observed_form_, prompt_policy, this);
 }
 
 bool PasswordFormManager::HasCompletedMatching() const {
   return state_ == POST_MATCHING_PHASE;
 }
 
 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(
     const std::vector<PasswordForm*>& logins_result) {
   // Note that the result gets deleted after this call completes, but we own
   // the PasswordForm objects pointed to by the result vector, thus we keep
   // copies to a minimum here.
 
   scoped_ptr<BrowserSavePasswordProgressLogger> logger;
   if (client_->IsLoggingActive()) {
@@ skipping 440 matching lines @@
   if (has_generated_password_)
     LogPasswordGenerationSubmissionEvent(PASSWORD_SUBMITTED);
 }
 
 void PasswordFormManager::SubmitFailed() {
   submit_result_ = kSubmitResultFailed;
   if (has_generated_password_)
     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