Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
414013003:
Password autofill should not override explicitly typed password (Closed)
|
Created:
6 years, 5 months ago by vabr (Chromium) Modified:
6 years, 3 months ago CC:
chromium-reviews, benquan, jam, browser-components-watch_chromium.org, darin-cc_chromium.org, Dane Wallinga, dyu1, rouslan+autofillwatch_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org, Ilya Sherman, mkwst+watchlist_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionPassword autofill should not override explicitly typed password
Imagine the following:
(1) A user types a username, which matches saved credentials, so the password is autofilled.
(2) The user overwrites the suggested password in the password field.
(3) The user enters the username field again, but leaves it without changes.
Currently, the autofilled password after step 3 replaces the user-typed password from step 2.
This CL makes sure, that the password from step 2 stays.
BUG=385619
Committed: https://crrev.com/d7b0f8c76f05dcb04523e68cf8b1258f85e7c417
Cr-Commit-Position: refs/heads/master@{#291938}
Patch Set 1 #Patch Set 2 : Fix compilation #Patch Set 3 : Don't give up autocompletion completely #
Total comments: 4
Patch Set 4 : set->bit in PasswordInfo #Patch Set 5 : Removed unused #includes #
Total comments: 5
Patch Set 6 : Change all the signatures #Patch Set 7 : Rebased on ToT #Patch Set 8 : Further corrections #
Total comments: 8
Patch Set 9 : Just rebased #Patch Set 10 : Comments addressed #
Total comments: 20
Patch Set 11 : Just rebased #Patch Set 12 : Comments addressed #
Messages
Total messages: 27 (0 generated)
Balázs, PTAL. Garrett, Does the change in behaviour in the CL description sound good to you? Cheers, Vaclav
On 2014/07/24 13:47:02, vabr (Chromium) wrote: > Balázs, PTAL. > > Garrett, > Does the change in behaviour in the CL description sound good to you? > > Cheers, > Vaclav New behavior SGTM.
On 2014/07/24 21:17:11, Garrett Casto wrote: > On 2014/07/24 13:47:02, vabr (Chromium) wrote: > > Balázs, PTAL. > > > > Garrett, > > Does the change in behaviour in the CL description sound good to you? > > > > Cheers, > > Vaclav > > New behavior SGTM. Thanks, Garrett. Balázs, if you find time for this one as well, PTAL. Cheers, Vaclav
Hi Balázs, A friendly ping about this review. Thanks! Vaclav
On 2014/07/28 11:29:39, vabr (Chromium) wrote: > Hi Balázs, > > A friendly ping about this review. > > Thanks! > Vaclav Sorry for the delay. My overall impression is that your current solution might be a tad too aggressive. IIUC, it effectively kills password suggestions and autofilling for the current impression of the form. I think we should tread more carefully here, and find a good trade-off between: 1.) Not overriding the explicitly typed password in cases where the username field changes in non-meaningful way (focus change, selection change, some validation JS erases the field, etc.). 2.) Still offering suggestions and autofill functionality in cases when the user interacts with the username field in a meaningful way (types a new username for which we have a saved password, or selects an autocomplete suggestion, etc.). I understand that there might be a lot more cases inbetween, I am happy to discuss further.
Thanks, Balázs. I softened the change, PTAL. Cheers, Vaclav
https://codereview.chromium.org/414013003/diff/60001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/414013003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:939: if (ContainsKey(temporary_disabled_password_fill_, *username_element)) As discussed offline, many of these helper functions seem not to be using any member variables apart from the |gatekeeper_|. We should look into refactoring this. https://codereview.chromium.org/414013003/diff/60001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/414013003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.h:232: // The set of all username elements, for which the corresponding password As discussed offline, we should just add this to PasswordInfo.
Thanks, Balázs, PTAL. Cheers, Vaclav https://codereview.chromium.org/414013003/diff/60001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/414013003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:939: if (ContainsKey(temporary_disabled_password_fill_, *username_element)) On 2014/07/29 14:43:54, engedy wrote: > As discussed offline, many of these helper functions seem not to be using any > member variables apart from the |gatekeeper_|. We should look into refactoring > this. Done: the knowledge of this particular bit removed completely, the refactoring bug filed at: http://crbug.com/398436 https://codereview.chromium.org/414013003/diff/60001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/414013003/diff/60001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.h:232: // The set of all username elements, for which the corresponding password On 2014/07/29 14:43:54, engedy wrote: > As discussed offline, we should just add this to PasswordInfo. Done.
https://codereview.chromium.org/414013003/diff/100001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/414013003/diff/100001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:276: if (!element->isNull() && element->value().isNull() && Could you please double-check that we first erase any previously auto-filled passwords in cases where the new one gets auto-filled through the gatekeeper? I think TextFieldDidEndEditing() will need to be updated, but I am not entire sure there are not more places. https://codereview.chromium.org/414013003/diff/100001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:303: if (iter->second.user_changed_password_more_recently_than_username) I have put some more though into this, I *think* the ideal behavior *for me* would be the following: After the user overrides the password (we check that it actually ends up being different), we essentially fall back into "wait_for_username" mode, and a bit more. More precisely, we only re-fill the password field when: 1.) The user selects a username suggestion from the drop-down, a username that is not the one that corresponds to the edited password. 2.) The user edits the username, and ends editing in the text field, leaving a username there for which we have a password stored, and that is not the username that corresponds to the edited password. WDYT?
Hi Balázs, Could you PTAL? Sorry that this CL got substantially bigger -- addressing your suggestion and fixing some bugs in my previous implementation required a couple of unexpected changes. I'm happy to talk about this in person as well, if it helped. Cheers, Vaclav https://codereview.chromium.org/414013003/diff/100001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/414013003/diff/100001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:276: if (!element->isNull() && element->value().isNull() && On 2014/07/29 17:47:56, engedy wrote: > Could you please double-check that we first erase any previously auto-filled > passwords in cases where the new one gets auto-filled through the gatekeeper? > > I think TextFieldDidEndEditing() will need to be updated, but I am not entire > sure there are not more places. Great catch. Indeed, I broke this. It should be now fixed and I added a test for that. https://codereview.chromium.org/414013003/diff/100001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:279: element->setSuggestedValue(blink::WebString()); I just discovered that setValue clears the suggested value, so I'm dropping this line again. https://codereview.chromium.org/414013003/diff/100001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:303: if (iter->second.user_changed_password_more_recently_than_username) On 2014/07/29 17:47:56, engedy wrote: > I have put some more though into this, I *think* the ideal behavior *for me* > would be the following: > > After the user overrides the password (we check that it actually ends up being > different), we essentially fall back into "wait_for_username" mode, and a bit > more. More precisely, we only re-fill the password field when: > 1.) The user selects a username suggestion from the drop-down, a username that > is not the one that corresponds to the edited password. > 2.) The user edits the username, and ends editing in the text field, leaving a > username there for which we have a password stored, and that is not the username > that corresponds to the edited password. > > WDYT? Your semantics sounds reasonable to me. I tried to accommodate it. The missing piece seemed to be handling FillSuggestion calls from AutofillAgent. One piece of clarification: If the user does: 1) autofill credentials for username/password 2) edits password 3) edits username to something which is not among suggestions 4) edits username again, making it a prefix of the username of step 1), and triggering the inline autocomplete for that then I consider it fair to overwrite the password with the suggestion again. Do you agree?
https://codereview.chromium.org/414013003/diff/160001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/414013003/diff/160001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:270: if (!(*it)->wait_for_username_change) 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. https://codereview.chromium.org/414013003/diff/160001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:292: if (iter == login_to_password_info_.end()) nit: I would personally add an alias here and in TextDidChangeInTextField() to increase readability, like: PasswordInfo* password_info = iter->second; https://codereview.chromium.org/414013003/diff/160001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:337: login_to_password_info_[iter->second]->wait_for_username_change = true; 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. https://codereview.chromium.org/414013003/diff/160001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:351: iter->second->wait_for_username_change = false; Hmm, let us discuss this logic tomorrow in person, I am not sure I understand.
Thanks, Balázs, For your comments, and fruitful discussion last week. Once you are back, PTAL. I was able to revert the changes on the gatekeeper, and did a minor fix to the test as well. Cheers, Vaclav https://codereview.chromium.org/414013003/diff/160001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/414013003/diff/160001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:270: if (!(*it)->wait_for_username_change) On 2014/07/31 17:14:46, engedy wrote: > 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. 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. https://codereview.chromium.org/414013003/diff/160001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:292: if (iter == login_to_password_info_.end()) On 2014/07/31 17:14:46, engedy wrote: > nit: I would personally add an alias here and in TextDidChangeInTextField() to > increase readability, like: > > PasswordInfo* password_info = iter->second; Done. https://codereview.chromium.org/414013003/diff/160001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:337: login_to_password_info_[iter->second]->wait_for_username_change = true; On 2014/07/31 17:14:46, engedy wrote: > 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. 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. https://codereview.chromium.org/414013003/diff/160001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:351: iter->second->wait_for_username_change = false; On 2014/07/31 17:14:46, engedy wrote: > Hmm, let us discuss this logic tomorrow in person, I am not sure I understand. Acknowledged.
Hi Balázs, A gentle ping. Thanks! Vaclav
On 2014/08/06 07:54:29, vabr (Chromium) wrote: > Hi Balázs, > > A gentle ping. > > Thanks! > Vaclav I am sorry, I am a terrible person. I will take a look right away!
LGTM modulo comments. https://codereview.chromium.org/414013003/diff/200001/chrome/renderer/autofil... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/414013003/diff/200001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1570: // test checks that the overwritten password is not reverted back by the user nit: s/by/if/ https://codereview.chromium.org/414013003/diff/200001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1571: // triggering autofill through focusing (but not changing) the username again. nit: s/triggering/triggers/ https://codereview.chromium.org/414013003/diff/200001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1572: TEST_F(PasswordAutofillAgentTest, PasswordNotOverwritten) { nit: How about NoopEditingDoesNotOverwriteManuallyEditedPassword? https://codereview.chromium.org/414013003/diff/200001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1606: // Tests that inline autocompletion overwrites the previous password value when nit: I would personally remove this comment, I do not think it adds to much value. https://codereview.chromium.org/414013003/diff/200001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1608: TEST_F(PasswordAutofillAgentTest, InlineAutocompleteOverwritesValue) { nit: How about InlineAutocompleteOverwritesManuallyEditedPassword? https://codereview.chromium.org/414013003/diff/200001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/414013003/diff/200001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:331: // The user changed the password after it was autofilled. Flipping the nit: I would personally drop this comment, and integrate a summary into the comment on new line 288. https://codereview.chromium.org/414013003/diff/200001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:334: login_to_password_info_[iter->second].wait_for_username_change = true; I think this is not really needed, but for clarity, we might consider resetting the |suggested_value| here. https://codereview.chromium.org/414013003/diff/200001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:415: if (username_element.value() != username_element.value()) If we are overwriting the password nevertheless, we might as well just reset the flag too unconditionally. And I think we should do both: This seems like a very explicit user action, and I think the user expectation would be to re-autofill an accidentally edited password. https://codereview.chromium.org/414013003/diff/200001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/414013003/diff/200001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:103: bool wait_for_username_change; To be consistent with |backspace_pressed_last|, I would name this flag from the "what happened" side and say |password_was_edited_last|. I would also shorten the comment like: // The most recent <<insert antonym for NOOP her>> interaction with the form was the user manually editing the password value. https://codereview.chromium.org/414013003/diff/200001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:221: // Maps password elements to the corresponding username elements, good for nit: I would personally also consider shortening or dropping this comment.
Thanks for your review, Balázs. I addressed all your comments, in a straightforward way. Given your previous approval, I'm going to queue this for commit. Happy to do a follow-up CL in case you are not happy with some of my comment formulations. Cheers, Vaclav https://codereview.chromium.org/414013003/diff/200001/chrome/renderer/autofil... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/414013003/diff/200001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1570: // test checks that the overwritten password is not reverted back by the user On 2014/08/06 10:18:17, engedy (OOO) wrote: > nit: s/by/if/ Done. https://codereview.chromium.org/414013003/diff/200001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1571: // triggering autofill through focusing (but not changing) the username again. On 2014/08/06 10:18:17, engedy (OOO) wrote: > nit: s/triggering/triggers/ Done. https://codereview.chromium.org/414013003/diff/200001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1572: TEST_F(PasswordAutofillAgentTest, PasswordNotOverwritten) { On 2014/08/06 10:18:17, engedy (OOO) wrote: > nit: How about NoopEditingDoesNotOverwriteManuallyEditedPassword? Done. https://codereview.chromium.org/414013003/diff/200001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1606: // Tests that inline autocompletion overwrites the previous password value when On 2014/08/06 10:18:17, engedy (OOO) wrote: > nit: I would personally remove this comment, I do not think it adds to much > value. Done. https://codereview.chromium.org/414013003/diff/200001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1608: TEST_F(PasswordAutofillAgentTest, InlineAutocompleteOverwritesValue) { On 2014/08/06 10:18:17, engedy (OOO) wrote: > nit: How about InlineAutocompleteOverwritesManuallyEditedPassword? Done. https://codereview.chromium.org/414013003/diff/200001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/414013003/diff/200001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:331: // The user changed the password after it was autofilled. Flipping the On 2014/08/06 10:18:17, engedy (OOO) wrote: > nit: I would personally drop this comment, and integrate a summary into the > comment on new line 288. Comment dropped. The summary came out as a duplicate of the definition comment of |password_was_edited_last|, so I dropped that as well. https://codereview.chromium.org/414013003/diff/200001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:334: login_to_password_info_[iter->second].wait_for_username_change = true; On 2014/08/06 10:18:17, engedy (OOO) wrote: > I think this is not really needed, but for clarity, we might consider resetting > the |suggested_value| here. Only made a comment to that effect. The suggested value is already reset in HTMLInputElement::setValue. https://codereview.chromium.org/414013003/diff/200001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:415: if (username_element.value() != username_element.value()) On 2014/08/06 10:18:17, engedy (OOO) wrote: > If we are overwriting the password nevertheless, we might as well just reset the > flag too unconditionally. > > And I think we should do both: This seems like a very explicit user action, and > I think the user expectation would be to re-autofill an accidentally edited > password. Good catch! Agreed and done. https://codereview.chromium.org/414013003/diff/200001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/414013003/diff/200001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:103: bool wait_for_username_change; On 2014/08/06 10:18:17, engedy (OOO) wrote: > To be consistent with |backspace_pressed_last|, I would name this flag from the > "what happened" side and say |password_was_edited_last|. Done. > > I would also shorten the comment like: > > // The most recent <<insert antonym for NOOP her>> interaction with the form > was the user manually editing the password value. That's not entirely true if the form has more editable fields than the username and password. So I reformulated your suggestion a little. https://codereview.chromium.org/414013003/diff/200001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:221: // Maps password elements to the corresponding username elements, good for On 2014/08/06 10:18:17, engedy (OOO) wrote: > nit: I would personally also consider shortening or dropping this comment. Done.
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/414013003/240001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Garrett, Could you please do an OWNERS rubberstamp? Cheers, Vaclav
The CQ bit was checked by gcasto@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vabr@chromium.org/414013003/240001
Message was sent while issue was closed.
Committed patchset #12 (240001) as bbb03f1c6a4ff97ad0c27d12db399dd30c55ba11
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/d7b0f8c76f05dcb04523e68cf8b1258f85e7c417 Cr-Commit-Position: refs/heads/master@{#291938} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
