[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/metrics/histogram.h"
@@ skipping 214 matching lines @@
 bool PasswordFormManager::HasValidPasswordForm() const {
   DCHECK_EQ(state_, POST_MATCHING_PHASE);
   // Non-HTML password forms (primarily HTTP and FTP autentication)
   // do not contain username_element and password_element values.
   if (observed_form_.scheme != PasswordForm::SCHEME_HTML)
     return true;
   return !observed_form_.password_element.empty() ||
          !observed_form_.new_password_element.empty();
 }
 
-void PasswordFormManager::ProvisionallySave(
+bool PasswordFormManager::ProvisionallySave(
     const PasswordForm& credentials,
     OtherPossibleUsernamesAction action) {
   DCHECK_EQ(state_, POST_MATCHING_PHASE);
   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).
@@ skipping 50 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;
+
+      // For ignorable change-password form, bail out early if usernames are

[vabr (Chromium), 2015/02/06 16:27:07]

Why should change password forms not be ignored in the presence of PSL matches?

[Pritam Nikam, 2015/02/09 15:48:17]

Done.

+      // matching but passwords are not matching.
+      if (IsIgnorableChangePasswordForm() &&
+          pending_credentials_.password_value != credentials.password_value) {
+        return false;
+      }
     }
   } 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.
+    // For ignorable change-password form, bail out early if usernames are not
+    // matching.
+    if (IsIgnorableChangePasswordForm())
+      return false;
+
     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();
 
@@ skipping 18 matching lines @@
   pending_credentials_.preferred = credentials.preferred;
 
   if (user_action_ == kUserActionOverridePassword &&
       pending_credentials_.type == PasswordForm::TYPE_GENERATED &&
       !has_generated_password_) {
     LogPasswordGenerationSubmissionEvent(PASSWORD_OVERRIDDEN);
   }
 
   if (has_generated_password_)
     pending_credentials_.type = PasswordForm::TYPE_GENERATED;
+
+  return true;
 }
 
 void PasswordFormManager::Save() {
   DCHECK_EQ(state_, POST_MATCHING_PHASE);
   DCHECK(!client_->IsOffTheRecord());
 
   if (IsNewLogin())
     SaveAsNewLogin(true);
   else
     UpdateLogin();
 }
 
 void PasswordFormManager::FetchMatchingLoginsFromPasswordStore(
     PasswordStore::AuthorizationPromptPolicy prompt_policy) {
   DCHECK_EQ(state_, PRE_MATCHING_PHASE);
   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

[vabr (Chromium), 2015/02/06 16:27:07]

The goal of http://crbug.com/448351 is not at all to support autofilling on
sign-up and change password forms. Revert this deletion.

[Pritam Nikam, 2015/02/09 15:48:17]

In my opinion, this is needed to fetch stored forms and identify genuine
ignorable change-password form cases. We can skip autofilling for all
change-password form in later part.

-  // working change password functionality).
-  if (!observed_form_.new_password_element.empty()) {
-    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;
-}
-
 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()) {
     logger.reset(new BrowserSavePasswordProgressLogger(client_));
     logger->LogMessage(Logger::STRING_ON_REQUEST_DONE_METHOD);
@@ skipping 121 matching lines @@
   // (1) we are in Incognito mode, (2) the ACTION paths don't match,
   // or (3) if it matched using public suffix domain matching.
   bool wait_for_username = client_->IsOffTheRecord() ||
                            observed_form_.action.GetWithEmptyPath() !=
                                preferred_match_->action.GetWithEmptyPath() ||
                            preferred_match_->IsPublicSuffixMatch();
   if (wait_for_username)
     manager_action_ = kManagerActionNone;
   else
     manager_action_ = kManagerActionAutofilled;
-  password_manager_->Autofill(driver.get(), observed_form_, best_matches_,
-                              *preferred_match_, wait_for_username);
+
+  // 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()) {

[vabr (Chromium), 2015/02/06 16:27:07]

What's the advantage of moving the check to so late?

[Pritam Nikam, 2015/02/09 15:48:17]

We can skip autofilling for all change-password form here.

+    password_manager_->Autofill(driver.get(), observed_form_, best_matches_,
+                                *preferred_match_, wait_for_username);
+  }
 }
 
 void PasswordFormManager::OnGetPasswordStoreResults(
     const std::vector<autofill::PasswordForm*>& results) {
   DCHECK_EQ(state_, MATCHING_PHASE);
 
   scoped_ptr<BrowserSavePasswordProgressLogger> logger;
   if (client_->IsLoggingActive()) {
     logger.reset(new BrowserSavePasswordProgressLogger(client_));
     logger->LogMessage(Logger::STRING_ON_GET_STORE_RESULTS_METHOD);
@@ skipping 295 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::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;
+}
+
 }  // namespace password_manager