Teach PasswordAutofillAgent sometimes match prefixes of usernames

components/autofill/content/renderer/password_autofill_agent.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_autofill_agent.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <memory>
@@ skipping 250 matching lines @@
   FormInputElementMap unowned_elements_map;
   if (FindFormInputElements(control_elements, data, ambiguous_or_empty_names,
                             &unowned_elements_map))
     results->push_back(unowned_elements_map);
 }
 
 bool IsElementEditable(const blink::WebInputElement& element) {
   return element.IsEnabled() && !element.IsReadOnly();
 }
 
-bool DoUsernamesMatch(const base::string16& username1,
-                      const base::string16& username2,
+bool DoUsernamesMatch(const base::string16& potential_suggestion,
+                      const base::string16& current_username,
                       bool exact_match) {
-  if (exact_match)
-    return username1 == username2;
-  return FieldIsSuggestionSubstringStartingOnTokenBoundary(username1, username2,
-                                                           true);
+  bool match = (potential_suggestion == current_username);

[vabr (Chromium), 2017/06/12 18:56:58]

optional nit: I am a bit concerned that we have two bool variables called
*match, but their logical type is different (one is a directive about how strict
matching to use, the other captures whether the two strings match).

A little more concise way to write this is by pulling the match out of the
branch, and it gets around the above issue by not naming |match|.

if (potential_suggestion == current_username)
  return true;
return !exact_match && IsPrefixEndingOnTokenBoundary(...)

[melandory, 2017/06/19 10:53:48]

Done.

+  return exact_match ? match
+                     : (match || IsPrefixEndingOnTokenBoundary(
+                                     current_username, potential_suggestion));
 }
 
 // Returns whether the given |element| is editable.
 bool IsElementAutocompletable(const blink::WebInputElement& element) {
   return IsElementEditable(element);
 }
 
 // Returns whether the |username_element| is allowed to be autofilled.
 //
 // Note that if the user interacts with the |password_field| and the
@@ skipping 78 matching lines @@
     if (value)
       it->second.first.reset(new base::string16(*value));
     it->second.second |= added_flags;
   } else {
     (*field_value_and_properties_map)[element] = std::make_pair(
         value ? base::MakeUnique<base::string16>(*value) : nullptr,
         added_flags);
   }
 }
 
+void FindMatchesByUsername(const PasswordFormFillData& fill_data,

[vabr (Chromium), 2017/06/12 18:56:58]

nit: Please comment on what the function does and what is the meaning of the
arguments.

[melandory, 2017/06/19 10:53:48]

Done.

+                           const base::string16& current_username,
+                           bool exact_username_match,
+                           RendererSavePasswordProgressLogger* logger,
+                           base::string16* username,
+                           base::string16* password) {
+  // Look for any suitable matches to current field text.
+  if (DoUsernamesMatch(fill_data.username_field.value, current_username,
+                       exact_username_match)) {
+    *username = fill_data.username_field.value;
+    *password = fill_data.password_field.value;
+    if (logger)
+      logger->LogMessage(Logger::STRING_USERNAMES_MATCH);
+  } else {
+    // Scan additional logins for a match.
+    for (const auto& it : fill_data.additional_logins) {
+      if (DoUsernamesMatch(it.first, current_username, exact_username_match)) {
+        *username = it.first;
+        *password = it.second.password;
+        break;
+      }
+    }
+    if (logger) {
+      logger->LogBoolean(Logger::STRING_MATCH_IN_ADDITIONAL,
+                         !(username->empty() && password->empty()));
+    }
+
+    // Check possible usernames.
+    if (username->empty() && password->empty()) {
+      for (const auto& it : fill_data.other_possible_usernames) {
+        for (size_t i = 0; i < it.second.size(); ++i) {
+          if (DoUsernamesMatch(it.second[i], current_username,
+                               exact_username_match)) {
+            *username = it.second[i];
+            *password = it.first.password;
+            break;
+          }
+        }
+        if (!username->empty() && !password->empty())

[vabr (Chromium), 2017/06/12 18:56:58]

I know that this is just the old code moved around, but I think it is slightly
wrong here. I would just drop the !username->empty() part -- the username could
be empty.

The pre-condition of the cycle implies that the password is empty, and whenever
a username is assigned inside it, password is also. So even restricted to
non-empty usernames, just checking !password->empty() should be equivalent.

[melandory, 2017/06/19 10:53:48]

Done.

+          break;
+      }
+    }
+  }
+}
+
 // This function attempts to fill |username_element| and |password_element|
 // with values from |fill_data|. The |password_element| will only have the
 // suggestedValue set, and will be registered for copying that to the real
 // value through |registration_callback|. If a match is found, return true and
 // |field_value_and_properties_map| will be modified with the autofilled
 // credentials and |FieldPropertiesFlags::AUTOFILLED| flag.
 bool FillUserNameAndPassword(
     blink::WebInputElement* username_element,
     blink::WebInputElement* password_element,
     const PasswordFormFillData& fill_data,
@@ skipping 11 matching lines @@
 
   base::string16 current_username;
   if (!username_element->IsNull()) {
     current_username = username_element->Value().Utf16();
   }
 
   // username and password will contain the match found if any.
   base::string16 username;
   base::string16 password;
 
-  // Look for any suitable matches to current field text.
-  if (DoUsernamesMatch(fill_data.username_field.value, current_username,
-                       exact_username_match)) {
-    username = fill_data.username_field.value;
-    password = fill_data.password_field.value;
-    if (logger)
-      logger->LogMessage(Logger::STRING_USERNAMES_MATCH);
-  } else {
-    // Scan additional logins for a match.
-    for (const auto& it : fill_data.additional_logins) {
-      if (DoUsernamesMatch(it.first, current_username, exact_username_match)) {
-        username = it.first;
-        password = it.second.password;
-        break;
-      }
-    }
-    if (logger) {
-      logger->LogBoolean(Logger::STRING_MATCH_IN_ADDITIONAL,
-                         !(username.empty() && password.empty()));
-    }
+  FindMatchesByUsername(fill_data, current_username, exact_username_match,
+                        logger, &username, &password);
 
-    // Check possible usernames.
-    if (username.empty() && password.empty()) {
-      for (const auto& it : fill_data.other_possible_usernames) {
-        for (size_t i = 0; i < it.second.size(); ++i) {
-          if (DoUsernamesMatch(
-                  it.second[i], current_username, exact_username_match)) {
-            username = it.second[i];
-            password = it.first.password;
-            break;
-          }
-        }
-        if (!username.empty() && !password.empty())
-          break;
-      }
-    }
-  }
   if (password.empty())
     return false;
 
   // TODO(tkent): Check maxlength and pattern for both username and password
   // fields.
 
   // Input matches the username, fill in required values.
   if (!username_element->IsNull() &&
       IsElementAutocompletable(*username_element)) {
     // TODO(crbug.com/507714): Why not setSuggestedValue?
     username_element->SetAutofillValue(blink::WebString::FromUTF16(username));
     UpdateFieldValueAndPropertiesMaskMap(*username_element, &username,
                                          FieldPropertiesFlags::AUTOFILLED,
                                          field_value_and_properties_map);
     username_element->SetAutofilled(true);
     if (logger)
       logger->LogElementName(Logger::STRING_USERNAME_FILLED, *username_element);
     if (set_selection) {
       form_util::PreviewSuggestion(username, current_username,
                                    username_element);
     }
-  } else if (current_username != username) {
+  } else if (exact_username_match && current_username != username) {

[vabr (Chromium), 2017/06/12 18:56:58]

There is not code path to satisfy this condition: exact_username_match means
that either username was set equal to current_username above, or the password
stayed empty, in which case this function exited on line 456. Please drop the
branch completely.

[melandory, 2017/06/19 10:53:48]

Done.

     // If the username can't be filled and it doesn't match a saved password
-    // as is, don't autofill a password.
+    // as is, autofill the password only in case the username element id read
+    // only.
     return false;
   }
 
   // Wait to fill in the password until a user gesture occurs. This is to make
   // sure that we do not fill in the DOM with a password until we believe the
   // user is intentionally interacting with the page.
   password_element->SetSuggestedValue(blink::WebString::FromUTF16(password));
   UpdateFieldValueAndPropertiesMaskMap(*password_element, &password,
                                        FieldPropertiesFlags::AUTOFILLED,
                                        field_value_and_properties_map);
@@ skipping 65 matching lines @@
   // In all other cases, do nothing.
   bool form_has_fillable_username = !username_field_name.empty() &&
                                     IsElementAutocompletable(username_element);
 
   if (form_has_fillable_username && username_element.Value().IsEmpty()) {
     // TODO(tkent): Check maxlength and pattern.
     username_element.SetAutofillValue(
         blink::WebString::FromUTF16(fill_data.username_field.value));
   }
 
-  // Fill if we have an exact match for the username. Note that this sets
-  // username to autofilled.
+  bool exact_username_match =
+      username_element.IsNull() || IsElementEditable(username_element);
+  // User the exact match for the editable username fields and allow prefix

[vabr (Chromium), 2017/06/12 18:56:58]

nit: User -> Use

[melandory, 2017/06/19 10:53:48]

Done.

+  // match for read-only username fields.
+  // Note that this sets username to autofilled.

[vabr (Chromium), 2017/06/12 18:56:58]

nit: The latter sentence no longer makes sense ("this" referred to "Fill...",
which was deleted). Also, I suspect that it is not true for read-only fields. My
suggestion is to drop this sentence.

[melandory, 2017/06/19 10:53:48]

Done.

   return FillUserNameAndPassword(
-      &username_element, &password_element, fill_data,
-      true /* exact_username_match */, false /* set_selection */,
-      field_value_and_properties_map, registration_callback, logger);
+      &username_element, &password_element, fill_data, exact_username_match,
+      false /* set_selection */, field_value_and_properties_map,
+      registration_callback, logger);
 }
 
 // Annotate |forms| with form and field signatures as HTML attributes.
 void AnnotateFormsWithSignatures(
     blink::WebVector<blink::WebFormElement> forms) {
   for (blink::WebFormElement form : forms) {
     std::unique_ptr<PasswordForm> password_form(
         CreatePasswordFormFromWebForm(form, nullptr, nullptr));
     if (password_form) {
       form.SetAttribute(
@@ skipping 1203 matching lines @@
 PasswordAutofillAgent::GetPasswordManagerDriver() {
   if (!password_manager_driver_) {
     render_frame()->GetRemoteInterfaces()->GetInterface(
         mojo::MakeRequest(&password_manager_driver_));
   }
 
   return password_manager_driver_;
 }
 
 }  // namespace autofill