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 "components/autofill/content/renderer/form_autofill_util.h"
 #include "components/autofill/core/common/password_form.h"
 #include "third_party/WebKit/public/platform/WebString.h"
 #include "third_party/WebKit/public/web/WebDocument.h"
 #include "third_party/WebKit/public/web/WebFormControlElement.h"
 #include "third_party/WebKit/public/web/WebInputElement.h"
 
 using blink::WebDocument;
 using blink::WebFormControlElement;
 using blink::WebFormElement;
 using blink::WebInputElement;
 using blink::WebString;
 using blink::WebVector;
 
 namespace autofill {
 namespace {
 
 // Maximum number of password fields we will observe before throwing our
 // hands in the air and giving up with a given form.
 static const size_t kMaxPasswords = 3;
 
-// 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 one, and which is the new

[Ilya Sherman, 2014/07/01 03:08:47]

Hmm, it's not entirely clear what a "main" password is -- probably worth
clarifying that bit of the comment.

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

Done.

+// 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

[vabr (Chromium), 2014/06/30 10:07:20]

Did you consider what happens when the values are equal because they are both
empty?

In that case it's not clear what the new password should be, so choosing one at
random might be the best we can do, but in that case we should at least put that
in a comment, especially since it is not consistent with the guess we make in
the else branch.

Or maybe the distinction only matters when we are about to save the password, in
which case it's reasonable to expect that the value is not empty?

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

We have discussed this with Vaclav to great length. There are two cases here:

 1) A form with two new password fields (sign-up form). The solution currently
implemented by this CL will result in correct behavior here.

 2) A change password form, with one field for the old, and one for the new
password. Here, we will initially decide that password_element == "" and
new_password_element != "" when we first see the form parsed/rendered. But
later, when the form is finally submitted with the two fields having different
values, we will have password_element != "". This will cause
PasswordManager::ProvisionallySavePassword not to be able to match the submitted
form to any of the PasswordFormManagers, so we will not save the credentials.

We consider (1) to be a more reasonable case to handle correctly, and (2) is
handled incorrectly by the old implementation already in the same way, so at
least we are not worse off. But perhaps we will need to revisit this later.

Perhaps we should take these cases into account when we refactor
PasswordFormManager::DoesManage(), and be on the lookout for matches between
re-purposed fields there.

[vabr (Chromium), 2014/07/03 14:42:56]

Thinking about (2) from a bigger distance, I think we should and can fix that.
Not in this CL, though, let's keep it more refactoring than changing behaviour.
LocateSpecificPasswords should have a way to signal it is unsure about the
choice of new_password_element, and DoesManage should be able to tolerate a
specific mismatch (observed new password == submitted current password) in that
case.
Balázs, would you mind filing a bug for fixing (2)?

[engedy, 2014/07/03 18:47:36]

Done.

+        // 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.
         *password = passwords[0];
       } else if (passwords[0].value() == passwords[1].value()) {
-        // Two the same and one different -> old password is duplicated one.
-        *old_password = passwords[0];
+        // Strange that the new password comes first, but trust more in that the
+        // field is duplicated more rather than in the ordering of fields.

[vabr (Chromium), 2014/06/30 10:07:20]

(typo?) second "more"

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

Done.

         *password = passwords[2];
+        *new_password = passwords[0];
       } else if (passwords[1].value() == passwords[2].value()) {
-        *old_password = passwords[0];
-        *password = passwords[1];
+        // New password is the duplicated one, and comes second. Makes sense.
+        *password = passwords[0];
+        *new_password = passwords[1];
       } 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;
   std::vector<WebInputElement> passwords;
   std::vector<base::string16> other_possible_usernames;
 
   WebVector<WebFormControlElement> control_elements;
   form.getFormControlElements(control_elements);
 
   for (size_t i = 0; i < control_elements.size(); ++i) {
     WebFormControlElement control_element = control_elements[i];
@@ skipping 36 matching lines @@
 
   // 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