Fix for no password save when action URL is modified

chrome/browser/password_manager/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 "chrome/browser/password_manager/password_form_manager.h"
 
 #include <algorithm>
 
 #include "base/metrics/histogram.h"
 #include "base/string_split.h"
@@ skipping 40 matching lines @@
                             GetActionsTaken(),
                             kMaxNumActionsTaken);
 }
 
 int PasswordFormManager::GetActionsTaken() {
   return user_action_ + kUserActionMax * (manager_action_ +
          kManagerActionMax * submit_result_);
 };
 
 // TODO(timsteele): use a hash of some sort in the future?
-bool PasswordFormManager::DoesManage(const PasswordForm& form) const {
+bool PasswordFormManager::DoesManage(const PasswordForm& form,
+                                     ActionMatch action_match) const {
   if (form.scheme != PasswordForm::SCHEME_HTML)
       return observed_form_.signon_realm == form.signon_realm;
 
   // HTML form case.
   // At a minimum, username and password element must match.
   if (!((form.username_element == observed_form_.username_element) &&
         (form.password_element == observed_form_.password_element))) {
     return false;
   }
 
-  // The action URL must also match, but the form is allowed to have an empty
-  // action URL (See bug 1107719).
-  if (form.action.is_valid() && (form.action != observed_form_.action))
-    return false;
+  // When action match is required, the action URL must match, but
+  // the form is allowed to have an empty action URL (See bug 1107719).
+  // Otherwise ignore action URL, this is to allow saving password form with
+  // dynamically changed action URL (See bug 27246).
+  if (form.action.is_valid() && (form.action != observed_form_.action)) {
+    if (action_match == kActionMatchRequired)

[tim (not reviewing), 2012/09/05 21:40:59]

nit - chromium style diverges from Google style here and prefers MACRO_STYLE for
enum values. (see http://dev.chromium.org/developers/coding-style)

[guohui, 2012/09/17 13:28:55]

Done.

+      return false;
+  }
 
   // If this is a replay of the same form in the case a user entered an invalid
   // password, the origin of the new form may equal the action of the "first"
   // form.
   if (!((form.origin == observed_form_.origin) ||
         (form.origin == observed_form_.action))) {
     if (form.origin.SchemeIsSecure() &&
         !observed_form_.origin.SchemeIsSecure()) {
       // Compare origins, ignoring scheme. There is no easy way to do this
       // with GURL because clearing the scheme would result in an invalid url.
@@ skipping 78 matching lines @@
   // 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_.username_element.empty() &&
       !observed_form_.password_element.empty();
 }
 
 void PasswordFormManager::ProvisionallySave(const PasswordForm& credentials) {
   DCHECK_EQ(state_, POST_MATCHING_PHASE);
-  DCHECK(DoesManage(credentials));
+  DCHECK(DoesManage(credentials, kActionMatchNotRequired));
 
   // 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()) {
     // The user signed in with a login we autofilled.
     pending_credentials_ = *it->second;
     is_new_login_ = false;
-    // 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.
-    if (pending_credentials_.action.is_empty())
-      pending_credentials_.action = observed_form_.action;
 
     // Check to see if we're using a known username but a new password.
     if (pending_credentials_.password_value != credentials.password_value)
       user_action_ = kUserActionOverride;
   } else {
     // User typed in a new, unknown username.
     user_action_ = kUserActionOverride;
     pending_credentials_ = observed_form_;
     pending_credentials_.username_value = credentials.username_value;
   }
 
+  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.
+  if (pending_credentials_.action.is_empty())
+    pending_credentials_.action = observed_form_.action;

[tim (not reviewing), 2012/09/05 21:40:59]

Could we add a test that verifies this gets updated on the "new username" case
as well? See TestEmptyAction.

[guohui, 2012/09/17 13:28:55]

Updated TestNewLogin to verify the action gets populated correctly on the new
username case. Also created a new test case TestUpdateAction to verify the
action get updated on the case when username and password are the same as saved
but action URL has changed.

+
   pending_credentials_.password_value = credentials.password_value;
   pending_credentials_.preferred = credentials.preferred;
 }
 
 void PasswordFormManager::Save() {
   DCHECK_EQ(state_, POST_MATCHING_PHASE);
   DCHECK(!profile_->IsOffTheRecord());
 
   if (IsNewLogin())
     SaveAsNewLogin(true);
@@ skipping 293 matching lines @@
 
 void PasswordFormManager::SubmitFailed() {
   submit_result_ = kSubmitResultFailed;
 }
 
 void PasswordFormManager::SendNotBlacklistedToRenderer() {
   content::RenderViewHost* host = web_contents_->GetRenderViewHost();
   host->Send(new AutofillMsg_FormNotBlacklisted(host->GetRoutingID(),
                                                  observed_form_));
 }