Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 773573004: Fill on account select in the password manager behind a flag (Closed)

Created:
6 years, 1 month ago by jww
Modified:
6 years, 1 month ago
CC:
chromium-reviews, benquan, browser-components-watch_chromium.org, jam, darin-cc_chromium.org, Dane Wallinga, dyu1, mkwst+moarreviews-renderer_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org, mkwst+watchlist-passwords_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src@master
Project:
chromium
Visibility:
Public.

Description

Fill on account select in the password manager behind a flag This implements, behind a flag, fill on account select in the password manager. When the --enable-fill-on-account-select flag is used (or set in chrome://flags), instead of autofilling on page load, the password manager will mark fields as "autofilled" that it believes it can appropriately autofill, but it will wait until the user has selected those fields manually and chooses the appropriate account before filling them in. There are two main advantages to this approach. The first is that it raises the bar for attackers with XSS's to sites in harvesting passwords. These attackers will not be able to rely on a mere user gesture before the field is filled; the user now must explicitly choose the account before it fills. Secondly, it has the possibility of improving the browsing experience on sites where the password manager fails to fill correctly, as it won't actually fill until the user chooses to do so, so if it tries to fill a wrong field, they user will simply not choose an account. The most basic part of this CL is quite straightforward in that if the flag is enabled, it simply stops, marks the fields as autofilled, and does not actually fill the fields with the credentials. However, there is a more complicated corner case: forms where the username is not editable, but the password field is, such as google.com/accounts after a user has logged in once. For these cases, it was necessary to add additional logic so that the *password* field can be clicked and the credentials can be selected. This requires a new IPC and some new autofill plumbing to change the dropdown menu, which was added in a previous CL, while this CL sets the appropriate option in the IPC. BUG=410963 Committed: https://crrev.com/203550726163c89d165b09f122aa74596634ed1a Cr-Commit-Position: refs/heads/master@{#308159}

Patch Set 1 #

Patch Set 2 : Rebase on top of autofill manager changes #

Patch Set 3 : Rebase on autofill manager changes #

Patch Set 4 : Rebase on ToT #

Patch Set 5 : Rebase again #

Patch Set 6 : Updated DEPS #

Patch Set 7 : Rebase on ToT #

Total comments: 6

Patch Set 8 : gcasto nit fixes #

Patch Set 9 : Rebase on ToT #

Total comments: 16

Patch Set 10 : Address comments from isherman #

Patch Set 11 : Addressing more comments from isherman #

Patch Set 12 : Restore isPasswordField check to FocusedNodeChange #

Patch Set 13 : Rebase on ToT #

Patch Set 14 : Updated histograms.xml #

Patch Set 15 : Fix browser_test crashes #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+286 lines, -36 lines) Patch
M chrome/app/generated_resources.grd View 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +8 lines, -0 lines 0 comments Download
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/renderer/autofill/password_autofill_agent_browsertest.cc View 1 2 3 4 5 6 4 chunks +123 lines, -2 lines 0 comments Download
M components/autofill/content/renderer/autofill_agent.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +6 lines, -1 line 0 comments Download
M components/autofill/content/renderer/password_autofill_agent.h View 1 2 3 4 5 6 7 2 chunks +15 lines, -3 lines 0 comments Download
M components/autofill/content/renderer/password_autofill_agent.cc View 1 2 3 4 5 6 7 8 9 10 16 chunks +114 lines, -30 lines 4 comments Download
M components/autofill/core/common/autofill_switches.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -0 lines 0 comments Download
M components/autofill/core/common/autofill_switches.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +8 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 36 (12 generated)
jww
Please note that this CL is built on top of https://codereview.chromium.org/767353002/ (and was originally split ...
6 years, 1 month ago (2014-12-01 21:57:08 UTC) #2
jww
Garrett, this is the actual implementation of fill-on-account-select behavior behind a flag (with your prior ...
6 years, 1 month ago (2014-12-10 01:41:34 UTC) #3
Garrett Casto
LGTM modulo minor comments. Adding Ilya to take a look at the changes to AutofillAgent. ...
6 years, 1 month ago (2014-12-10 06:26:59 UTC) #5
jww
Thanks, Garrett. Nits addressed. https://codereview.chromium.org/773573004/diff/120001/components/autofill/content/renderer/password_autofill_agent.cc File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/773573004/diff/120001/components/autofill/content/renderer/password_autofill_agent.cc#newcode138 components/autofill/content/renderer/password_autofill_agent.cc:138: // TODO(jww): Add experimental groups ...
6 years, 1 month ago (2014-12-10 22:56:03 UTC) #6
Ilya Sherman
https://codereview.chromium.org/773573004/diff/160001/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/773573004/diff/160001/chrome/app/generated_resources.grd#newcode7557 chrome/app/generated_resources.grd:7557: + Enable fill passwords on account selection nit: Since ...
6 years, 1 month ago (2014-12-11 23:28:55 UTC) #7
jww
https://codereview.chromium.org/773573004/diff/160001/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/773573004/diff/160001/chrome/app/generated_resources.grd#newcode7557 chrome/app/generated_resources.grd:7557: + Enable fill passwords on account selection On 2014/12/11 ...
6 years, 1 month ago (2014-12-12 00:17:44 UTC) #8
Ilya Sherman
https://codereview.chromium.org/773573004/diff/160001/components/autofill/content/renderer/autofill_agent.cc File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/773573004/diff/160001/components/autofill/content/renderer/autofill_agent.cc#newcode248 components/autofill/content/renderer/autofill_agent.cc:248: element_ = *element; On 2014/12/12 00:17:43, jww wrote: > ...
6 years, 1 month ago (2014-12-12 00:25:50 UTC) #9
jww
https://codereview.chromium.org/773573004/diff/160001/components/autofill/content/renderer/autofill_agent.cc File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/773573004/diff/160001/components/autofill/content/renderer/autofill_agent.cc#newcode248 components/autofill/content/renderer/autofill_agent.cc:248: element_ = *element; On 2014/12/12 00:25:50, Ilya Sherman wrote: ...
6 years, 1 month ago (2014-12-12 00:54:56 UTC) #10
Ilya Sherman
https://codereview.chromium.org/773573004/diff/160001/components/autofill/content/renderer/autofill_agent.cc File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/773573004/diff/160001/components/autofill/content/renderer/autofill_agent.cc#newcode248 components/autofill/content/renderer/autofill_agent.cc:248: element_ = *element; On 2014/12/12 00:54:55, jww wrote: > ...
6 years, 1 month ago (2014-12-12 01:56:02 UTC) #11
jww
https://codereview.chromium.org/773573004/diff/160001/components/autofill/content/renderer/autofill_agent.cc File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/773573004/diff/160001/components/autofill/content/renderer/autofill_agent.cc#newcode248 components/autofill/content/renderer/autofill_agent.cc:248: element_ = *element; On 2014/12/12 01:56:02, Ilya Sherman wrote: ...
6 years, 1 month ago (2014-12-12 02:05:49 UTC) #12
Ilya Sherman
LGTM. Thanks, Joel.
6 years, 1 month ago (2014-12-12 02:12:41 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/773573004/220001
6 years, 1 month ago (2014-12-12 02:16:12 UTC) #15
commit-bot: I haz the power
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/103295) ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator/builds/41374) ios_rel_device_ninja ...
6 years, 1 month ago (2014-12-12 02:21:43 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/773573004/240001
6 years, 1 month ago (2014-12-12 02:29:45 UTC) #19
commit-bot: I haz the power
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tests_recipe/builds/36881)
6 years, 1 month ago (2014-12-12 03:38:14 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/773573004/260001
6 years, 1 month ago (2014-12-12 06:26:38 UTC) #23
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_compile_dbg_ng/builds/6144) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_gn_rel/builds/42273)
6 years, 1 month ago (2014-12-12 06:30:25 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/773573004/260001
6 years, 1 month ago (2014-12-12 17:18:43 UTC) #27
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/14234)
6 years, 1 month ago (2014-12-12 18:24:56 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/773573004/280001
6 years, 1 month ago (2014-12-12 19:26:20 UTC) #31
commit-bot: I haz the power
Committed patchset #15 (id:280001)
6 years, 1 month ago (2014-12-12 21:01:10 UTC) #32
commit-bot: I haz the power
Patchset 15 (id:??) landed as https://crrev.com/203550726163c89d165b09f122aa74596634ed1a Cr-Commit-Position: refs/heads/master@{#308159}
6 years, 1 month ago (2014-12-12 21:02:11 UTC) #33
vabr (Chromium)
A http://crbug.com/442191 inspired drive-by. :) https://codereview.chromium.org/773573004/diff/280001/components/autofill/content/renderer/password_autofill_agent.cc File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/773573004/diff/280001/components/autofill/content/renderer/password_autofill_agent.cc#newcode428 components/autofill/content/renderer/password_autofill_agent.cc:428: // If (a) is ...
6 years, 1 month ago (2014-12-15 14:37:44 UTC) #35
jww
6 years, 1 month ago (2014-12-15 23:14:47 UTC) #36
Message was sent while issue was closed.
https://codereview.chromium.org/773573004/diff/280001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/773573004/diff/280001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:428: // If (a)
is false but (b) is true, then the username element should not be
On 2014/12/15 14:37:44, vabr (OOO soon) wrote:
> If line 443 corresponds to this sentence, then the condition on line 443
should
> be negated.
Yikes, good catch! The comment is wrong. It should read "If (a) is false and (b)
is false..." I'll upload a CL in a sec.

https://codereview.chromium.org/773573004/diff/280001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:761: if
(element.isPasswordField() && IsElementEditable(*username_element))
On 2014/12/15 14:37:43, vabr (OOO soon) wrote:
> username_element.isNull() will still be true for forms like on
> http://kamsnimi.wz.cz, which do not have a username at all.
> 
> I'm working on a patch to make those forms work nicely with fill on request.
> I'll Cc you on there.

Awesome, thanks!

Powered by Google App Engine
This is Rietveld 408576698