Fill on account select in the password manager behind a flag

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}

[jww, 2014-12-01 21:57:08]

jww@chromium.org changed reviewers:
+ gcasto@chromium.org

[jww, 2014-12-01 21:57:08]

Please note that this CL is built on top of
https://codereview.chromium.org/767353002/ (and was originally split in two from
https://codereview.chromium.org/492043003/) and thus cannot be committed until
that CL has been approved and committed.

[jww, 2014-12-10 01:41:34]

Garrett, this is the actual implementation of fill-on-account-select behavior
behind a flag (with your prior comments addressed). Can you take a look? Thanks!

[Garrett Casto, 2014-12-10 06:26:58]

gcasto@chromium.org changed reviewers:
+ isherman@chromium.org

[Garrett Casto, 2014-12-10 06:26:59]

LGTM modulo minor comments. Adding Ilya to take a look at the changes to
AutofillAgent.

https://codereview.chromium.org/773573004/diff/120001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/773573004/diff/120001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:138: //
TODO(jww): Add experimental groups check here.
You can add this check now. It should just be one line. You can actually setup
the experiment later.

https://codereview.chromium.org/773573004/diff/120001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:712:
PasswordAutofillAgent::LoginToPasswordInfoMap::iterator
Given that we never actually need the bare iterator and just want one return
value, can you change this to return a bool if the value is found and have an
output parameter for the found value?

https://codereview.chromium.org/773573004/diff/120001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1186: &username,
&password, fill_data, false, true,
Nit: This shouldn't need to be changed.

[jww, 2014-12-10 22:56:03]

Thanks, Garrett. Nits addressed.

https://codereview.chromium.org/773573004/diff/120001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/773573004/diff/120001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:138: //
TODO(jww): Add experimental groups check here.
On 2014/12/10 06:26:59, Garrett Casto wrote:
> You can add this check now. It should just be one line. You can actually setup
> the experiment later.

Done.

https://codereview.chromium.org/773573004/diff/120001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:712:
PasswordAutofillAgent::LoginToPasswordInfoMap::iterator
On 2014/12/10 06:26:59, Garrett Casto wrote:
> Given that we never actually need the bare iterator and just want one return
> value, can you change this to return a bool if the value is found and have an
> output parameter for the found value?

Done.

https://codereview.chromium.org/773573004/diff/120001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1186: &username,
&password, fill_data, false, true,
On 2014/12/10 06:26:59, Garrett Casto wrote:
> Nit: This shouldn't need to be changed.

Done.

[Ilya Sherman, 2014-12-11 23:28:55]

https://codereview.chromium.org/773573004/diff/160001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/773573004/diff/160001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7557: +        Enable fill passwords on
account selection
nit: Since you have a default/enable/disable switch, it make sense to drop
"Enable" from these strings.

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:248: element_ = *element;
I honestly don't even remember why it's important to set the element here, given
that that's the last thing that happens in this method.  Could you please remind
me?

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:613: if
(!input_element->isTextField())
I am concerned that without the isPasswordField() check, we might try to offer
regular, non-password Autofill suggestions for password fields.  Could you
please either restore the guard, but possibly a little later on, so that it only
affects non-password Autofill; or else explain to me why this concern doesn't
apply?

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:143:
base::FieldTrialList::FindFullName(kFillOnAccountSelectFieldTrialName);
You should query the field trial first, before checking the flags.  This ensures
that the field trial data will be uploaded for users who are using the flags.

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:144: return
group_name.compare(kFillOnAccountSelectFieldTrialEnabledGroup) == 0;
Optional nit: I think operator== is clearer here.

[jww, 2014-12-12 00:17:44]

https://codereview.chromium.org/773573004/diff/160001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/773573004/diff/160001/chrome/app/generated_re...
chrome/app/generated_resources.grd:7557: +        Enable fill passwords on
account selection
On 2014/12/11 23:28:55, Ilya Sherman wrote:
> nit: Since you have a default/enable/disable switch, it make sense to drop
> "Enable" from these strings.

Done.

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:248: element_ = *element;
On 2014/12/11 23:28:55, Ilya Sherman wrote:
> I honestly don't even remember why it's important to set the element here,
given
> that that's the last thing that happens in this method.  Could you please
remind
> me?

I think it's as simple as the focused node has changed, and element_ tracks the
currently focused node. For example, AutofillAgent::AcceptDataListSuggestion
takes the currently focused element (element_) as the input element and fills it
with the suggested value, and thus after focus has changed, element_ needs to be
updated.

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:613: if
(!input_element->isTextField())
On 2014/12/11 23:28:55, Ilya Sherman wrote:
> I am concerned that without the isPasswordField() check, we might try to offer
> regular, non-password Autofill suggestions for password fields.  Could you
> please either restore the guard, but possibly a little later on, so that it
only
> affects non-password Autofill; or else explain to me why this concern doesn't
> apply?

Good point. I moved the guard further down, right after the potential call to
password_autofill_agent_->ShowSuggestions(), since password fields should *only*
go to the password autofill agent.

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:143:
base::FieldTrialList::FindFullName(kFillOnAccountSelectFieldTrialName);
On 2014/12/11 23:28:55, Ilya Sherman wrote:
> You should query the field trial first, before checking the flags.  This
ensures
> that the field trial data will be uploaded for users who are using the flags.

Don't we want the opposite of that though? Namely, if the user flips the flag,
we don't want to consider their field trial group (b/c their not getting the
behavior of the field trial).

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:144: return
group_name.compare(kFillOnAccountSelectFieldTrialEnabledGroup) == 0;
On 2014/12/11 23:28:55, Ilya Sherman wrote:
> Optional nit: I think operator== is clearer here.

Done.

[Ilya Sherman, 2014-12-12 00:25:50]

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:248: element_ = *element;
On 2014/12/12 00:17:43, jww wrote:
> On 2014/12/11 23:28:55, Ilya Sherman wrote:
> > I honestly don't even remember why it's important to set the element here,
> given
> > that that's the last thing that happens in this method.  Could you please
> remind
> > me?
> 
> I think it's as simple as the focused node has changed, and element_ tracks
the
> currently focused node. For example, AutofillAgent::AcceptDataListSuggestion
> takes the currently focused element (element_) as the input element and fills
it
> with the suggested value, and thus after focus has changed, element_ needs to
be
> updated.

AcceptDataListSuggestion should only be called after ShowSuggestions, which also
sets the element_.  So, I guess my question is: Are there any code paths that
rely on the element_ being set here, rather than in a different method?  My more
focused question is: Does your change really need the element_ to be set here?

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:143:
base::FieldTrialList::FindFullName(kFillOnAccountSelectFieldTrialName);
On 2014/12/12 00:17:43, jww wrote:
> On 2014/12/11 23:28:55, Ilya Sherman wrote:
> > You should query the field trial first, before checking the flags.  This
> ensures
> > that the field trial data will be uploaded for users who are using the
flags.
> 
> Don't we want the opposite of that though? Namely, if the user flips the flag,
> we don't want to consider their field trial group (b/c their not getting the
> behavior of the field trial).

The ordering should be

// Call FieldTrialList::FindFullName
// Check the disable flag.
// Check the enable flag.
// Check the field trial group.

By querying first, you cause users to be assigned into groups like
"Enabled_Forced" and "Disabled_Forced" (or whatever you've named them).  If
that's not clear, the "Forcing Flags" page at go/finch might help clarify.

[jww, 2014-12-12 00:54:56]

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:248: element_ = *element;
On 2014/12/12 00:25:50, Ilya Sherman wrote:
> On 2014/12/12 00:17:43, jww wrote:
> > On 2014/12/11 23:28:55, Ilya Sherman wrote:
> > > I honestly don't even remember why it's important to set the element here,
> > given
> > > that that's the last thing that happens in this method.  Could you please
> > remind
> > > me?
> > 
> > I think it's as simple as the focused node has changed, and element_ tracks
> the
> > currently focused node. For example, AutofillAgent::AcceptDataListSuggestion
> > takes the currently focused element (element_) as the input element and
fills
> it
> > with the suggested value, and thus after focus has changed, element_ needs
to
> be
> > updated.
> 
> AcceptDataListSuggestion should only be called after ShowSuggestions, which
also
> sets the element_.  So, I guess my question is: Are there any code paths that
> rely on the element_ being set here, rather than in a different method?  My
more
> focused question is: Does your change really need the element_ to be set here?

My change doesn't touch the assignment of element_, so no, it doesn't need it I
suppose, but I'd rather leave the removal of the assignment to a separate,
unrelated CL, if that's cool with you.

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:143:
base::FieldTrialList::FindFullName(kFillOnAccountSelectFieldTrialName);
On 2014/12/12 00:25:50, Ilya Sherman wrote:
> On 2014/12/12 00:17:43, jww wrote:
> > On 2014/12/11 23:28:55, Ilya Sherman wrote:
> > > You should query the field trial first, before checking the flags.  This
> > ensures
> > > that the field trial data will be uploaded for users who are using the
> flags.
> > 
> > Don't we want the opposite of that though? Namely, if the user flips the
flag,
> > we don't want to consider their field trial group (b/c their not getting the
> > behavior of the field trial).
> 
> The ordering should be
> 
> // Call FieldTrialList::FindFullName
> // Check the disable flag.
> // Check the enable flag.
> // Check the field trial group.
> 
> By querying first, you cause users to be assigned into groups like
> "Enabled_Forced" and "Disabled_Forced" (or whatever you've named them).  If
> that's not clear, the "Forcing Flags" page at go/finch might help clarify.

Done.

[Ilya Sherman, 2014-12-12 01:56:02]

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:248: element_ = *element;
On 2014/12/12 00:54:55, jww wrote:
> On 2014/12/12 00:25:50, Ilya Sherman wrote:
> > On 2014/12/12 00:17:43, jww wrote:
> > > On 2014/12/11 23:28:55, Ilya Sherman wrote:
> > > > I honestly don't even remember why it's important to set the element
here,
> > > given
> > > > that that's the last thing that happens in this method.  Could you
please
> > > remind
> > > > me?
> > > 
> > > I think it's as simple as the focused node has changed, and element_
tracks
> > the
> > > currently focused node. For example,
AutofillAgent::AcceptDataListSuggestion
> > > takes the currently focused element (element_) as the input element and
> fills
> > it
> > > with the suggested value, and thus after focus has changed, element_ needs
> to
> > be
> > > updated.
> > 
> > AcceptDataListSuggestion should only be called after ShowSuggestions, which
> also
> > sets the element_.  So, I guess my question is: Are there any code paths
that
> > rely on the element_ being set here, rather than in a different method?  My
> more
> > focused question is: Does your change really need the element_ to be set
here?
> 
> My change doesn't touch the assignment of element_, so no, it doesn't need it
I
> suppose, but I'd rather leave the removal of the assignment to a separate,
> unrelated CL, if that's cool with you.

I think you can still address my more narrow question, by reverting your change
to this method if it's not needed.

[jww, 2014-12-12 02:05:49]

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):

https://codereview.chromium.org/773573004/diff/160001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:248: element_ = *element;
On 2014/12/12 01:56:02, Ilya Sherman wrote:
> On 2014/12/12 00:54:55, jww wrote:
> > On 2014/12/12 00:25:50, Ilya Sherman wrote:
> > > On 2014/12/12 00:17:43, jww wrote:
> > > > On 2014/12/11 23:28:55, Ilya Sherman wrote:
> > > > > I honestly don't even remember why it's important to set the element
> here,
> > > > given
> > > > > that that's the last thing that happens in this method.  Could you
> please
> > > > remind
> > > > > me?
> > > > 
> > > > I think it's as simple as the focused node has changed, and element_
> tracks
> > > the
> > > > currently focused node. For example,
> AutofillAgent::AcceptDataListSuggestion
> > > > takes the currently focused element (element_) as the input element and
> > fills
> > > it
> > > > with the suggested value, and thus after focus has changed, element_
needs
> > to
> > > be
> > > > updated.
> > > 
> > > AcceptDataListSuggestion should only be called after ShowSuggestions,
which
> > also
> > > sets the element_.  So, I guess my question is: Are there any code paths
> that
> > > rely on the element_ being set here, rather than in a different method? 
My
> > more
> > > focused question is: Does your change really need the element_ to be set
> here?
> > 
> > My change doesn't touch the assignment of element_, so no, it doesn't need
it
> I
> > suppose, but I'd rather leave the removal of the assignment to a separate,
> > unrelated CL, if that's cool with you.
> 
> I think you can still address my more narrow question, by reverting your
change
> to this method if it's not needed.

Ah, got it. I misunderstood your request. Done.

[Ilya Sherman, 2014-12-12 02:12:41]

LGTM.  Thanks, Joel.

[jww, 2014-12-12 02:14:20]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-12-12 02:16:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/773573004/220001

[commit-bot: I haz the power, 2014-12-12 02:21:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-12 02:21:43]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jww, 2014-12-12 02:29:00]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-12-12 02:29:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/773573004/240001

[commit-bot: I haz the power, 2014-12-12 03:38:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-12 03:38:14]

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_tes...)

[jww, 2014-12-12 06:25:43]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-12-12 06:26:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/773573004/260001

[commit-bot: I haz the power, 2014-12-12 06:30:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-12 06:30:25]

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_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jww, 2014-12-12 17:17:56]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-12-12 17:18:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/773573004/260001

[commit-bot: I haz the power, 2014-12-12 18:24:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-12 18:24:56]

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_...)

[jww, 2014-12-12 19:25:41]

The CQ bit was checked by jww@chromium.org

[commit-bot: I haz the power, 2014-12-12 19:26:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/773573004/280001

[commit-bot: I haz the power, 2014-12-12 21:01:10]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2014-12-12 21:02:11]

Patchset 15 (id:??) landed as
https://crrev.com/203550726163c89d165b09f122aa74596634ed1a
Cr-Commit-Position: refs/heads/master@{#308159}

[vabr (Chromium), 2014-12-15 14:37:42]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org

[vabr (Chromium), 2014-12-15 14:37:44]

A http://crbug.com/442191 inspired drive-by. :)

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
If line 443 corresponds to this sentence, then the condition on line 443 should
be negated.

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))
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.

[jww, 2014-12-15 23:14:47]

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!