Password autofill should not override explicitly typed password

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 "base/bind.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
@@ skipping 228 matching lines @@
 PasswordAutofillAgent::~PasswordAutofillAgent() {
 }
 
 PasswordAutofillAgent::PasswordValueGatekeeper::PasswordValueGatekeeper()
     : was_user_gesture_seen_(false) {
 }
 
 PasswordAutofillAgent::PasswordValueGatekeeper::~PasswordValueGatekeeper() {
 }
 
-void PasswordAutofillAgent::PasswordValueGatekeeper::RegisterElement(
-    blink::WebInputElement* element) {
-  if (was_user_gesture_seen_)
-    ShowValue(element);
-  else
-    elements_.push_back(*element);
+void PasswordAutofillAgent::PasswordValueGatekeeper::RegisterElementInfo(
+    PasswordInfo* element_info) {
+  DCHECK(element_info);
+  if (was_user_gesture_seen_) {
+    ShowValue(&element_info->password_field);
+  } else {
+    elements_info_.insert(element_info);
+  }
+}
+
+void PasswordAutofillAgent::PasswordValueGatekeeper::UnregisterElementInfo(
+    PasswordInfo* element_info) {
+  elements_info_.erase(element_info);
 }
 
 void PasswordAutofillAgent::PasswordValueGatekeeper::OnUserGesture() {
   was_user_gesture_seen_ = true;
 
-  for (std::vector<blink::WebInputElement>::iterator it = elements_.begin();
-       it != elements_.end();
+  for (std::set<PasswordInfo*>::iterator it = elements_info_.begin();
+       it != elements_info_.end();
        ++it) {
-    ShowValue(&(*it));
+    if (!(*it)->wait_for_username_change)

[engedy, 2014/07/31 17:14:46]

I would suggest keeping this logic out of PasswordValueGatekeeper.

In my understanding, what gets assigned to |suggested_value|, should
semantically be considered the "value" of the element already, it is just not
exposed to |value| so as to protect it from JS.

I think it should be the callers' responsibility not to latch a non-empty value
to |suggested_value| in the first place if they do not intend to overwrite it --
then we only need to make sure not to clear |value| on ShowValue() if
|suggested_value| is empty.

[vabr (Chromium), 2014/08/04 15:10:07]

I agree. I also realised that the callers, which need to care, already seem to
guard on wait_for_username_change, so I can just drop this check (and added
wiring).

Done.

+      ShowValue(&(*it)->password_field);
   }
 
-  elements_.clear();
+  elements_info_.clear();
 }
 
 void PasswordAutofillAgent::PasswordValueGatekeeper::Reset() {
   was_user_gesture_seen_ = false;
-  elements_.clear();
+  elements_info_.clear();
 }
 
 void PasswordAutofillAgent::PasswordValueGatekeeper::ShowValue(
     blink::WebInputElement* element) {
-  if (!element->isNull() && !element->suggestedValue().isNull())
+  if (!element->isNull() && !element->suggestedValue().isEmpty())
     element->setValue(element->suggestedValue(), true);
 }
 
 bool PasswordAutofillAgent::TextFieldDidEndEditing(
     const blink::WebInputElement& element) {
   LoginToPasswordInfoMap::const_iterator iter =
       login_to_password_info_.find(element);
   if (iter == login_to_password_info_.end())

[engedy, 2014/07/31 17:14:46]

nit: I would personally add an alias here and in TextDidChangeInTextField() to
increase readability, like:

PasswordInfo* password_info = iter->second;

[vabr (Chromium), 2014/08/04 15:10:07]

Done.

     return false;
 
-  const PasswordFormFillData& fill_data = iter->second.fill_data;
+  // Don't let autofill overwrite an explicit change made by the user.
+  if (iter->second->wait_for_username_change)
+    return false;
 
   // If wait_for_username is false, we should have filled when the text changed.
-  if (!fill_data.wait_for_username)
+  if (!iter->second->fill_data.wait_for_username)
     return false;
 
-  blink::WebInputElement password = iter->second.password_field;
-  if (!IsElementEditable(password))
+  if (!IsElementEditable(iter->second->password_field))
     return false;
 
   blink::WebInputElement username = element;  // We need a non-const.
 
   // Do not set selection when ending an editing session, otherwise it can
   // mess with focus.
   FillUserNameAndPassword(&username,
-                          &password,
-                          fill_data,
+                          iter->second,
                           true /* exact_username_match */,
                           false /* set_selection */);
   return true;
 }
 
 bool PasswordAutofillAgent::TextDidChangeInTextField(
     const blink::WebInputElement& element) {
+  // TODO(vabr): Get a mutable argument instead. http://crbug.com/397083
+  blink::WebInputElement mutable_element = element;  // We need a non-const.
+
   if (element.isPasswordField()) {
     // Some login forms have event handlers that put a hash of the password into
     // a hidden field and then clear the password (http://crbug.com/28910,
     // http://crbug.com/391693). This method gets called before any of those
     // handlers run, so save away a copy of the password in case it gets lost.
     // To honor the user having explicitly cleared the password, even an empty
     // password will be saved here.
     ProvisionallySavePassword(
         element.document().frame(), element.form(), RESTRICTION_NONE);
+
+    PasswordToLoginMap::iterator iter = password_to_username_.find(element);
+    if (iter != password_to_username_.end()) {
+      // The user changed the password after it was autofilled. Flipping the
+      // flag below to true makes sure it is not overwritten by autofill
+      // re-firing for the same username.
+      login_to_password_info_[iter->second]->wait_for_username_change = true;

[engedy, 2014/07/31 17:14:46]

Perhaps this would be caught by tests, but we should double-check that this
event handler is not called when it is us who autofilled the password.

[vabr (Chromium), 2014/08/04 15:10:06]

I'm reasonably certain that TextDidChangeInTextField is called when the user
types in the input field:

Tracing this hook back to Blink leads me to
TextFieldInputType::didSetValueByUserEdit, which has a rather suggestive name.

+      mutable_element.setAutofilled(false);
+    }
     return false;
   }
 
   LoginToPasswordInfoMap::const_iterator iter =
       login_to_password_info_.find(element);
   if (iter == login_to_password_info_.end())
     return false;
 
   // The input text is being changed, so any autofilled password is now
   // outdated.
-  blink::WebInputElement username = element;  // We need a non-const.
-  username.setAutofilled(false);
+  mutable_element.setAutofilled(false);
+  iter->second->wait_for_username_change = false;

[engedy, 2014/07/31 17:14:46]

Hmm, let us discuss this logic tomorrow in person, I am not sure I understand.

[vabr (Chromium), 2014/08/04 15:10:07]

Acknowledged.

 
-  blink::WebInputElement password = iter->second.password_field;
+  blink::WebInputElement password = iter->second->password_field;
   if (password.isAutofilled()) {
     password.setValue(base::string16(), true);
     password.setAutofilled(false);
   }
 
   // If wait_for_username is true we will fill when the username loses focus.
-  if (iter->second.fill_data.wait_for_username)
+  if (iter->second->fill_data.wait_for_username)
     return false;
 
   if (!element.isText() || !IsElementAutocompletable(element) ||
       !IsElementAutocompletable(password)) {
     return false;
   }
 
   // Don't inline autocomplete if the user is deleting, that would be confusing.
   // But refresh the popup.  Note, since this is ours, return true to signal
   // no further processing is required.
-  if (iter->second.backspace_pressed_last) {
-    ShowSuggestionPopup(iter->second.fill_data, username, false);
+  if (iter->second->backspace_pressed_last) {
+    ShowSuggestionPopup(iter->second->fill_data, element, false);
     return true;
   }
 
   blink::WebString name = element.nameForAutofill();
   if (name.isEmpty())
     return false;  // If the field has no name, then we won't have values.
 
   // Don't attempt to autofill with values that are too large.
   if (element.value().length() > kMaximumTextSizeForAutocomplete)
     return false;
 
   // The caret position should have already been updated.
-  PerformInlineAutocomplete(element, password, iter->second.fill_data);
+  PerformInlineAutocomplete(&mutable_element, iter->second);
   return true;
 }
 
 bool PasswordAutofillAgent::TextFieldHandlingKeyDown(
     const blink::WebInputElement& element,
     const blink::WebKeyboardEvent& event) {
   // If using the new Autofill UI that lives in the browser, it will handle
   // keypresses before this function. This is not currently an issue but if
   // the keys handled there or here change, this issue may appear.
 
   LoginToPasswordInfoMap::iterator iter = login_to_password_info_.find(element);
   if (iter == login_to_password_info_.end())
     return false;
 
   int win_key_code = event.windowsKeyCode;
-  iter->second.backspace_pressed_last =
+  iter->second->backspace_pressed_last =
       (win_key_code == ui::VKEY_BACK || win_key_code == ui::VKEY_DELETE);
   return true;
 }
 
 bool PasswordAutofillAgent::FillSuggestion(
     const blink::WebNode& node,
     const blink::WebString& username,
     const blink::WebString& password) {
   blink::WebInputElement username_element;
-  PasswordInfo password_info;
+  PasswordInfo* password_info;
 
   if (!FindLoginInfo(node, &username_element, &password_info) ||
       !IsElementAutocompletable(username_element) ||
-      !IsElementAutocompletable(password_info.password_field)) {
+      !IsElementAutocompletable(password_info->password_field)) {
     return false;
   }
 
-  base::string16 current_username = username_element.value();
+  if (username_element.value() != username_element.value())
+    password_info->wait_for_username_change = false;
   username_element.setValue(username, true);
   username_element.setAutofilled(true);
   username_element.setSelectionRange(username.length(), username.length());
 
-  password_info.password_field.setValue(password, true);
-  password_info.password_field.setAutofilled(true);
+  password_info->password_field.setValue(password, true);
+  password_info->password_field.setAutofilled(true);
 
   return true;
 }
 
 bool PasswordAutofillAgent::PreviewSuggestion(
     const blink::WebNode& node,
     const blink::WebString& username,
     const blink::WebString& password) {
   blink::WebInputElement username_element;
-  PasswordInfo password_info;
+  PasswordInfo* password_info;
 
   if (!FindLoginInfo(node, &username_element, &password_info) ||
       !IsElementAutocompletable(username_element) ||
-      !IsElementAutocompletable(password_info.password_field)) {
+      !IsElementAutocompletable(password_info->password_field)) {
     return false;
   }
 
   was_username_autofilled_ = username_element.isAutofilled();
   username_selection_start_ = username_element.selectionStart();
   username_element.setSuggestedValue(username);
   username_element.setAutofilled(true);
   username_element.setSelectionRange(
       username_selection_start_,
       username_element.suggestedValue().length());
 
-  was_password_autofilled_ = password_info.password_field.isAutofilled();
-  password_info.password_field.setSuggestedValue(password);
-  password_info.password_field.setAutofilled(true);
+  was_password_autofilled_ = password_info->password_field.isAutofilled();
+  password_info->password_field.setSuggestedValue(password);
+  password_info->password_field.setAutofilled(true);
 
   return true;
 }
 
 bool PasswordAutofillAgent::DidClearAutofillSelection(
     const blink::WebNode& node) {
   blink::WebInputElement username_element;
-  PasswordInfo password_info;
+  PasswordInfo* password_info;
   if (!FindLoginInfo(node, &username_element, &password_info))
     return false;
 
-  ClearPreview(&username_element, &password_info.password_field);
+  ClearPreview(&username_element, &password_info->password_field);
   return true;
 }
 
 bool PasswordAutofillAgent::ShowSuggestions(
     const blink::WebInputElement& element,
     bool show_all) {
   LoginToPasswordInfoMap::const_iterator iter =
       login_to_password_info_.find(element);
   if (iter == login_to_password_info_.end())
     return false;
 
   // If autocomplete='off' is set on the form elements, no suggestion dialog
   // should be shown. However, return |true| to indicate that this is a known
   // password form and that the request to show suggestions has been handled (as
   // a no-op).
   if (!IsElementAutocompletable(element) ||
-      !IsElementAutocompletable(iter->second.password_field))
+      !IsElementAutocompletable(iter->second->password_field))
     return true;
 
-  return ShowSuggestionPopup(iter->second.fill_data, element, show_all);
+  return ShowSuggestionPopup(iter->second->fill_data, element, show_all);
 }
 
 bool PasswordAutofillAgent::OriginCanAccessPasswordManager(
     const blink::WebSecurityOrigin& origin) {
   return origin.canAccessPasswordManager();
 }
 
 void PasswordAutofillAgent::OnDynamicFormsSeen(blink::WebFrame* frame) {
   SendPasswordForms(frame, false /* only_visible */);
 }
@@ skipping 313 matching lines @@
     // Attach autocomplete listener to enable selecting alternate logins.
     // First, get pointers to username element.
     blink::WebInputElement username_element =
         form_elements->input_elements[form_data.basic_data.fields[0].name];
 
     // Get pointer to password element. (We currently only support single
     // password forms).
     blink::WebInputElement password_element =
         form_elements->input_elements[form_data.basic_data.fields[1].name];
 
-    // If wait_for_username is true, we don't want to initially fill the form
-    // until the user types in a valid username.
-    if (!form_data.wait_for_username)
-      FillFormOnPasswordRecieved(form_data, username_element, password_element);
-
     // We might have already filled this form if there are two <form> elements
     // with identical markup.
     if (login_to_password_info_.find(username_element) !=
         login_to_password_info_.end())
       continue;
 
-    PasswordInfo password_info;
-    password_info.fill_data = form_data;
-    password_info.password_field = password_element;
-    login_to_password_info_[username_element] = password_info;
+    scoped_ptr<PasswordInfo> password_info(new PasswordInfo);
+    password_info->fill_data = form_data;
+    password_info->password_field = password_element;
+    login_to_password_info_[username_element] = password_info.get();
+    password_to_username_[password_element] = username_element;
+    // If wait_for_username is true, we don't want to initially fill the form
+    // until the user types in a valid username.
+    if (!form_data.wait_for_username)
+      FillFormOnPasswordRecieved(&username_element, password_info.get());
+    password_infos_[username_element.document().frame()].push_back(
+        make_linked_ptr(password_info.release()));
 
     FormData form;
     FormFieldData field;
     FindFormAndFieldForFormControlElement(
         username_element, &form, &field, REQUIRE_NONE);
     Send(new AutofillHostMsg_AddPasswordFormMapping(
         routing_id(), field, form_data));
   }
 }
 
 void PasswordAutofillAgent::OnSetLoggingState(bool active) {
   logging_state_active_ = active;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // PasswordAutofillAgent, private:
 
+PasswordAutofillAgent::PasswordInfo::PasswordInfo()
+    : backspace_pressed_last(false), wait_for_username_change(false) {
+}
+
 void PasswordAutofillAgent::GetSuggestions(
     const PasswordFormFillData& fill_data,
     const base::string16& input,
     std::vector<base::string16>* suggestions,
     std::vector<base::string16>* realms,
     bool show_all) {
   if (show_all ||
       StartsWith(fill_data.basic_data.fields[0].value, input, false)) {
     suggestions->push_back(fill_data.basic_data.fields[0].value);
     realms->push_back(base::UTF8ToUTF16(fill_data.preferred_realm));
@@ skipping 53 matching lines @@
   gfx::RectF bounding_box_scaled(bounding_box.x() * scale,
                                  bounding_box.y() * scale,
                                  bounding_box.width() * scale,
                                  bounding_box.height() * scale);
   Send(new AutofillHostMsg_ShowPasswordSuggestions(
       routing_id(), field, bounding_box_scaled, suggestions, realms));
   return !suggestions.empty();
 }
 
 void PasswordAutofillAgent::FillFormOnPasswordRecieved(
-    const PasswordFormFillData& fill_data,
-    blink::WebInputElement username_element,
-    blink::WebInputElement password_element) {
+    blink::WebInputElement* username_element,
+    PasswordInfo* password_info) {
+  blink::WebInputElement* password_element = &password_info->password_field;
   // Do not fill if the password field is in an iframe.
-  DCHECK(password_element.document().frame());
-  if (password_element.document().frame()->parent())
+  DCHECK(password_element->document().frame());
+  if (password_element->document().frame()->parent())
     return;
 
   if (!ShouldIgnoreAutocompleteOffForPasswordFields() &&
-      !username_element.form().autoComplete())
+      !username_element->form().autoComplete())
     return;
 
   // If we can't modify the password, don't try to set the username
-  if (!IsElementAutocompletable(password_element))
+  if (!IsElementAutocompletable(*password_element))
     return;
 
   // Try to set the username to the preferred name, but only if the field
   // can be set and isn't prefilled.
-  if (IsElementAutocompletable(username_element) &&
-      username_element.value().isEmpty()) {
+  if (IsElementAutocompletable(*username_element) &&
+      username_element->value().isEmpty()) {
     // TODO(tkent): Check maxlength and pattern.
-    username_element.setValue(fill_data.basic_data.fields[0].value, true);
+    username_element->setValue(
+        password_info->fill_data.basic_data.fields[0].value, true);
   }
 
   // Fill if we have an exact match for the username. Note that this sets
   // username to autofilled.
-  FillUserNameAndPassword(&username_element,
-                          &password_element,
-                          fill_data,
+  FillUserNameAndPassword(username_element,
+                          password_info,
                           true /* exact_username_match */,
                           false /* set_selection */);
 }
 
 bool PasswordAutofillAgent::FillUserNameAndPassword(
     blink::WebInputElement* username_element,
-    blink::WebInputElement* password_element,
-    const PasswordFormFillData& fill_data,
+    PasswordInfo* password_info,
     bool exact_username_match,
     bool set_selection) {
+  blink::WebInputElement password_element = password_info->password_field;
+  const PasswordFormFillData& fill_data = password_info->fill_data;
   base::string16 current_username = username_element->value();
   // 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.basic_data.fields[0].value,
                        current_username,
                        exact_username_match)) {
     username = fill_data.basic_data.fields[0].value;
@@ skipping 32 matching lines @@
       }
     }
   }
   if (password.empty())
     return false;  // No match was found.
 
   // TODO(tkent): Check maxlength and pattern for both username and password
   // fields.
 
   // Don't fill username if password can't be set.
-  if (!IsElementAutocompletable(*password_element)) {
+  if (!IsElementAutocompletable(password_element)) {
     return false;
   }
 
   // Input matches the username, fill in required values.
   if (IsElementAutocompletable(*username_element)) {
     username_element->setValue(username, true);
     username_element->setAutofilled(true);
 
     if (set_selection) {
       username_element->setSelectionRange(current_username.length(),
                                           username.length());
     }
   } else if (current_username != username) {
     // If the username can't be filled and it doesn't match a saved password
     // as is, don't autofill a password.
     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(password);
-  gatekeeper_.RegisterElement(password_element);
+  password_element.setSuggestedValue(password);
+  gatekeeper_.RegisterElementInfo(password_info);
 
-  password_element->setAutofilled(true);
+  password_element.setAutofilled(true);
   return true;
 }
 
 void PasswordAutofillAgent::PerformInlineAutocomplete(
-    const blink::WebInputElement& username_input,
-    const blink::WebInputElement& password_input,
-    const PasswordFormFillData& fill_data) {
-  DCHECK(!fill_data.wait_for_username);
+    blink::WebInputElement* username,
+    PasswordInfo* password_info) {
+  DCHECK(!password_info->fill_data.wait_for_username);
 
-  // We need non-const versions of the username and password inputs.
-  blink::WebInputElement username = username_input;
-  blink::WebInputElement password = password_input;
+  blink::WebInputElement password = password_info->password_field;
 
   // Don't inline autocomplete if the caret is not at the end.
   // TODO(jcivelli): is there a better way to test the caret location?
-  if (username.selectionStart() != username.selectionEnd() ||
-      username.selectionEnd() != static_cast<int>(username.value().length())) {
+  if (username->selectionStart() != username->selectionEnd() ||
+      username->selectionEnd() !=
+          static_cast<int>(username->value().length())) {
     return;
   }
 
   // Show the popup with the list of available usernames.
-  ShowSuggestionPopup(fill_data, username, false);
+  ShowSuggestionPopup(password_info->fill_data, *username, false);
 
 #if !defined(OS_ANDROID)
   // Fill the user and password field with the most relevant match. Android
   // only fills in the fields after the user clicks on the suggestion popup.
-  FillUserNameAndPassword(&username,
-                          &password,
-                          fill_data,
+  FillUserNameAndPassword(username,
+                          password_info,
                           false /* exact_username_match */,
                           true /* set_selection */);
 #endif
 }
 
 void PasswordAutofillAgent::FrameClosing(const blink::WebFrame* frame) {
   for (LoginToPasswordInfoMap::iterator iter = login_to_password_info_.begin();
        iter != login_to_password_info_.end();) {
-    if (iter->first.document().frame() == frame)
+    if (iter->first.document().frame() == frame) {
+      password_to_username_.erase(iter->second->password_field);
+      gatekeeper_.UnregisterElementInfo(iter->second);
       login_to_password_info_.erase(iter++);
-    else
+    } else {
       ++iter;
+    }
   }
+  password_infos_.erase(frame);
   for (FrameToPasswordFormMap::iterator iter =
            provisionally_saved_forms_.begin();
        iter != provisionally_saved_forms_.end();) {
     if (iter->first == frame)
       provisionally_saved_forms_.erase(iter++);
     else
       ++iter;
   }
 }
 
 bool PasswordAutofillAgent::FindLoginInfo(const blink::WebNode& node,
                                           blink::WebInputElement* found_input,
-                                          PasswordInfo* found_password) {
+                                          PasswordInfo** found_password) {
   if (!node.isElementNode())
     return false;
 
   blink::WebElement element = node.toConst<blink::WebElement>();
   if (!element.hasHTMLTagName("input"))
     return false;
 
   blink::WebInputElement input = element.to<blink::WebInputElement>();
   LoginToPasswordInfoMap::iterator iter = login_to_password_info_.find(input);
   if (iter == login_to_password_info_.end())
@@ skipping 26 matching lines @@
   scoped_ptr<PasswordForm> password_form(CreatePasswordForm(form));
   if (!password_form || (restriction == RESTRICTION_NON_EMPTY_PASSWORD &&
                          password_form->password_value.empty() &&
                          password_form->new_password_value.empty())) {
     return;
   }
   provisionally_saved_forms_[frame].reset(password_form.release());
 }
 
 }  // namespace autofill