PasswordForm: move from current/old password scheme to current/new.

components/autofill/content/renderer/password_form_conversion_utils.cc

 // Copyright 2013 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/autofill/content/renderer/password_form_conversion_utils.h"
 
 #include "base/strings/string_util.h"
 #include "components/autofill/content/renderer/form_autofill_util.h"
 #include "components/autofill/core/common/password_form.h"
 #include "third_party/WebKit/public/platform/WebString.h"
@@ skipping 16 matching lines @@
 static const size_t kMaxPasswords = 3;
 
 // Checks in a case-insensitive way if the autocomplete attribute for the given
 // |element| is present and has the specified |value_in_lowercase|.
 bool HasAutocompleteAttributeValue(const WebInputElement* element,
                                    const char* value_in_lowercase) {
   return LowerCaseEqualsASCII(element->getAttribute("autocomplete"),
                               value_in_lowercase);
 }
 
-// Helper to determine which password is the main one, and which is
-// an old password (e.g on a "make new password" form), if any.
+// Helper to determine which password is the main (current) one, and which is
+// the new password (e.g., on a sign-up or change password form), if any.
 bool LocateSpecificPasswords(std::vector<WebInputElement> passwords,
                              WebInputElement* password,
-                             WebInputElement* old_password) {
+                             WebInputElement* new_password) {
   switch (passwords.size()) {
     case 1:
       // Single password, easy.
       *password = passwords[0];
       break;
     case 2:
       if (passwords[0].value() == passwords[1].value()) {
-        // Treat two identical passwords as a single password.
-        *password = passwords[0];
+        // Two identical passwords: assume we are seeing a new password with a
+        // confirmation. This can be either a sign-up form or a password change
+        // form that does not ask for the old password.
+        *new_password = passwords[0];
       } else {
         // Assume first is old password, second is new (no choice but to guess).
-        *old_password = passwords[0];
-        *password = passwords[1];
+        *password = passwords[0];
+        *new_password = passwords[1];
       }
       break;
     case 3:
-      if (passwords[0].value() == passwords[1].value() &&
-          passwords[0].value() == passwords[2].value()) {
-        // All three passwords the same? Just treat as one and hope.
+      if (!passwords[0].value().isEmpty() &&

[engedy, 2014/07/03 13:34:19]

We have found a problem here that the set of elements that are marked as
|password_element| and/or |new_password_element| is dependent on whether the
password fields are filled with values or not.

Given that PasswordFormManager's |observed_form_| is parsed before the user
fills in the values, but the |form| received in
PasswordManager::ProvisionallySavePassword is parsed after, they might have a
different layout in terms of |password_element| and |new_password_element|
fields; and we might have the following problems:

 0.) In theory, the dependent code will make different conclusions, in the two
cases, as to whether or not we are dealing with a "login form" or "sign-up or
change password" form -- as this is based on whether or not we are seeing a
|new_password_element|.

 1.) More concretely, PasswordFormManager::DoesManage() will return false if the
|password_element|s differ, therefore the PasswordManager will not find the
PasswordFormManager originally created for |observed_form_|, and will not save
the password in the end.

 2.) Or, if we are lucky enough to get as far as
PasswordFormManager::UpdateLogin(), we might try to update missing element names
in the stored credentials based on an |observed_form_| that is not really a
"login form" -- although we could have told this already once we have seen
|form| with the filled in password values.

After having had a long discussion with Vaclav, we decided the change behavior
here somewhat to address the issues.

In a nutshell, when there is a chance that we might identify a form as a having
a |new_password_element| once we we have seen the password values, we wanted to
make sure that we will always speculatively select a |new_password| field
(perhaps the wrong one) even before the values are input into the password
fields.

We hope that the proposed changes address the most common scenarios, and that
they will not cause breakages with any other important use case.

+          (passwords[0].value() == passwords[1].value() &&
+           passwords[0].value() == passwords[2].value())) {
+        // All three passwords are the same and non-empty? This does not make
+        // any sense, give up.
+        return false;
+      } else if (passwords[1].value() == passwords[2].value()) {
+        // New password is the duplicated one, and comes second; or empty form
+        // with 3 password fields, in which case we will assume this layout.
         *password = passwords[0];
+        *new_password = passwords[1];
       } else if (passwords[0].value() == passwords[1].value()) {

[engedy, 2014/07/03 13:34:19]

Note that this is also a behavior change.

-        // Two the same and one different -> old password is duplicated one.
-        *old_password = passwords[0];
+        // It is strange that the new password comes first, but trust more which
+        // fields are duplicated than the ordering of fields.
         *password = passwords[2];
-      } else if (passwords[1].value() == passwords[2].value()) {
-        *old_password = passwords[0];
-        *password = passwords[1];
+        *new_password = passwords[0];
       } else {
         // Three different passwords, or first and last match with middle
         // different. No idea which is which, so no luck.
         return false;
       }
       break;
     default:
       return false;
   }
   return true;
 }
 
-// Get information about a login form that encapsulated in the
-// PasswordForm struct.
+// Get information about a login form encapsulated in a PasswordForm struct.
 void GetPasswordForm(const WebFormElement& form, PasswordForm* password_form) {
   WebInputElement latest_input_element;
   WebInputElement username_element;
   // Caches whether |username_element| is marked with autocomplete='username'.
   // Needed for performance reasons to avoid recalculating this multiple times.
   bool has_seen_element_with_autocomplete_username_before = false;
   std::vector<WebInputElement> passwords;
   std::vector<base::string16> other_possible_usernames;
 
   WebVector<WebFormControlElement> control_elements;
@@ skipping 68 matching lines @@
         }
       }
     }
   }
 
   if (!username_element.isNull()) {
     password_form->username_element = username_element.nameForAutofill();
     password_form->username_value = username_element.value();
   }
 
+  if (!username_element.isNull()) {
+    password_form->username_element = username_element.nameForAutofill();
+    password_form->username_value = username_element.value();
+  }
+
   // Get the document URL
   GURL full_origin(form.document().url());
 
   // Calculate the canonical action URL
   WebString action = form.action();
   if (action.isNull())
     action = WebString(""); // missing 'action' attribute implies current URL
   GURL full_action(form.document().completeURL(action));
   if (!full_action.is_valid())
     return;
 
   WebInputElement password;
-  WebInputElement old_password;
-  if (!LocateSpecificPasswords(passwords, &password, &old_password))
+  WebInputElement new_password;
+  if (!LocateSpecificPasswords(passwords, &password, &new_password))
     return;
 
   // We want to keep the path but strip any authentication data, as well as
   // query and ref portions of URL, for the form action and form origin.
   GURL::Replacements rep;
   rep.ClearUsername();
   rep.ClearPassword();
   rep.ClearQuery();
   rep.ClearRef();
   password_form->action = full_action.ReplaceComponents(rep);
   password_form->origin = full_origin.ReplaceComponents(rep);
 
   rep.SetPathStr("");
   password_form->signon_realm = full_origin.ReplaceComponents(rep).spec();
 
   password_form->other_possible_usernames.swap(other_possible_usernames);
 
   if (!password.isNull()) {
     password_form->password_element = password.nameForAutofill();
     password_form->password_value = password.value();
     password_form->password_autocomplete_set = password.autoComplete();
   }
-  if (!old_password.isNull()) {
-    password_form->old_password_element = old_password.nameForAutofill();
-    password_form->old_password_value = old_password.value();
+  if (!new_password.isNull()) {
+    password_form->new_password_element = new_password.nameForAutofill();
+    password_form->new_password_value = new_password.value();
   }
 
   password_form->scheme = PasswordForm::SCHEME_HTML;
   password_form->ssl_valid = false;
   password_form->preferred = false;
   password_form->blacklisted_by_user = false;
   password_form->type = PasswordForm::TYPE_MANUAL;
   password_form->use_additional_authentication = false;
 }
 
@@ skipping 13 matching lines @@
                            blink::WebFormControlElement(),
                            REQUIRE_NONE,
                            EXTRACT_NONE,
                            &password_form->form_data,
                            NULL /* FormFieldData */);
 
   return password_form.Pass();
 }
 
 }  // namespace autofill