Show Login Not Secure on username field even without Autocomplete attribute

Show Login Not Secure on all form fields if a password field is visible

Previously, the "Login Not Secure" notice was only shown for fields in
forms if the input type was Password or if the text field contained an
AutoComplete="username" attribute. This change enhances the warning to
show "Login Not Secure" notice for any text field in a form containing
a visible password field.

BUG=688612
TEST=Enable FNS, visit http://webdbg.com/test/forms/findlogins.htm

Review-Url: https://codereview.chromium.org/2682473002
Cr-Commit-Position: refs/heads/master@{#451025}
Committed: https://chromium.googlesource.com/chromium/src/+/e4310b09c5b49b6ce2825021919049704a8ff4a6

[elawrence, 2017-02-06 20:20:15]

Patchset #1 (id:1) has been deleted

[elawrence, 2017-02-06 20:39:28]

The CQ bit was checked by elawrence@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 20:40:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-06 21:47:27]

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

[commit-bot: I haz the power, 2017-02-06 21:47:28]

Dry run: This issue passed the CQ dry run.

[elawrence, 2017-02-09 22:36:36]

The CQ bit was checked by elawrence@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 22:37:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-09 23:46:01]

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

[commit-bot: I haz the power, 2017-02-09 23:46:01]

Dry run: This issue passed the CQ dry run.

[elawrence, 2017-02-10 16:15:03]

elawrence@chromium.org changed reviewers:
+ dvadym@chromium.org, sebsg@chromium.org

[elawrence, 2017-02-10 16:15:04]

sebsg@chromium.org: Please review changes in components/autofill/

dvadym@chromium.org: Please review changes in chrome/browser/password_manager/
and chrome/test/data/password/password_form.html

Thanks!

[dvadym, 2017-02-10 16:35:47]

LGTM from password manager point of view, i.e. files
  components/autofill/content/renderer/password_autofill_agent.cc
  chrome/browser/password_manager/password_manager_browsertest.cc
  chrome/test/data/password/password_form.html

Please keep in mind that a password field might be not a password field but for
example a CVV code field.

[sebsg, 2017-02-10 19:15:21]

The CVC is a important case like Vadym said. There are a bunch of sites that set
that field as being a password (had to work around that in heuristics a while
back).

Do you think you could check if Autofill predicts this field as being a CVC and
not show it in that case?

Thanks!

[sebsg, 2017-02-10 19:48:38]

For example, you could get that information by checking the Type of a field in
that way:

field->Type().GetStorableType() !=  CREDIT_CARD_VERIFICATION_CODE

After the heuristics have run and after getting the server predictions. 

Don't hesitate to ping me if you have questions :)

[elawrence, 2017-02-10 20:05:02]

> For example, you could get that information by checking the Type of a field in
> that way:
> 
> field->Type().GetStorableType() !=  CREDIT_CARD_VERIFICATION_CODE
> 
> After the heuristics have run and after getting the server predictions. 

Thanks!

Our original goal was to show the warning on just the Username field. The
problem we had was that we didn't know it was a username field in the
ShowSuggestions() call if the user doesn't have any saved passwords (because
FindPasswordInfoForElement returns false). So the proposal was to instead invoke
GetPasswordFormFromWebForm() to determine whether we're in a username field. My
worry there was that this was a pretty complicated and lengthy bit of code to be
running on every keystroke in every edit box. So my proposal was to simply
detect whether ANY field in the form was a password field, and if so, display
the warning for all other fields.

In my initial testing, I recognized that a signup form might have both a credit
card field AND a login password field and I decided that this case was
sufficiently obscure that it wasn't worth trying to do anything special.
(Largely because HTTP forms with password fields appear to be ~700x more common
than HTTP forms with credit card forms). 

I had overlooked the issue with CVV fields, and that does seem more interesting.


If I wanted to run the heuristics, can you point me to how I'd go about doing
that? Or should I pursue the GetPasswordFormFromWebForm approach? (Or is that
the same thing?)

thanks!

[elawrence, 2017-02-10 20:08:57]

Oops, I meant CreatePasswordFormFromWebForm(), not GetPasswordFormFromWebForm().


Perhaps it would make sense to do a hybrid, whereby we first do the cheaper
check to see if a form has any password inputs, and only if that returns true do
we call CreatePasswordFormFromWebForm ?

[elawrence, 2017-02-10 22:10:49]

Patchset #3 (id:60001) has been deleted

[elawrence, 2017-02-10 22:20:17]

Patchset #3 (id:80001) has been deleted

[elawrence, 2017-02-10 22:23:46]

Okay, I've tried a different approach to this using the CreatePasswordFormFrom*
functions. This seems to work well in my tests, but I'm not sure if the
&field_value_and_properties_map_ and &form_predictions_ arguments are
appropriate for use at this point in the code?

[elawrence, 2017-02-13 15:26:18]

Description was changed from

==========
Show Login Not Secure on all form fields if a password field is visible

Previously, the "Login Not Secure" notice was only shown for fields in
forms if the input type was Password or if the text field contained an
AutoComplete="username" attribute. This change enhances the warning to
show "Login Not Secure" notice for any text field in a form containing
a visible password field.

BUG=688612
TEST=Enable FNS, visit http://webdbg.com/test/forms/findlogins.htm
==========

to

==========
Show Login Not Secure on all form fields if a password field is visible

Previously, the "Login Not Secure" notice was only shown for fields in
forms if the input type was Password or if the text field contained an
AutoComplete="username" attribute. This change enhances the warning to
show "Login Not Secure" notice for any text field in a form containing
a visible password field.

BUG=688612
TEST=Enable FNS, visit http://webdbg.com/test/forms/findlogins.htm
==========

[dvadym, 2017-02-13 16:52:11]

On 2017/02/10 22:23:46, elawrence wrote:
> Okay, I've tried a different approach to this using the
CreatePasswordFormFrom*
> functions. This seems to work well in my tests, but I'm not sure if the
> &field_value_and_properties_map_ and &form_predictions_ arguments are
> appropriate for use at this point in the code?

It's ok to use field_value_and_properties_map_ and form_predictions_ as
arguments for CreatePasswordFormFrom.

Still LGTM from password manager point of view. 

I'll just add some clarification of how it works in order that it would be more
clear tradeoffs between this and the first solution:

1.Now CreatePasswordFormFrom uses simple heuristics to find username field,
namely username field is the last visible text field before the first visible
password field. 
2.Checking of visibility of the field is just heuristics, if it says yes, the
field could be still invisible.
3.So it means if the site adds some invisible text field that according to our
heuristics is visible between username and passowrd fields, we would never show
warning on a username field.
4.Also it means that it anyway can show warning on a payment form, because
CreatePasswordFormFrom has no idea about CVV codes.
5.But in the first solution the warning will be shown on all fields (that also
might be not ideal).

CreatePasswordFormFromWebForm is more expensive that checking only visibility of
fields, but not so much expensive. We call it on any typing in a password field,
ShowSuggestions is called only on click, so it shouldn't be any problems.

[sebsg, 2017-02-13 18:53:56]

LGTM % comment

https://codereview.chromium.org/2682473002/diff/120001/chrome/browser/passwor...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/2682473002/diff/120001/chrome/browser/passwor...
chrome/browser/password_manager/password_manager_browsertest.cc:1868: }
Can you add a test for when a user clicks on another field when a password field
is present?

[elawrence, 2017-02-15 21:26:00]

> Can you add a test for when a user clicks on another field when a password
field
> is present?

@sebsg: Done. Can you please take another look at
password_manager_browsertest.cc and password_form.html to verify that this is
what you asked for? It was not entirely clear how to verify that a dialog
*isn't* shown, but the approach I used (trigger action, execute script, run
message loop until idle) appears to work correctly.

(Please ignore the other two files; they were simply rebased).

Thanks!

[sebsg, 2017-02-15 21:28:34]

Nice, thanks for adding this. LGTM

[elawrence, 2017-02-15 21:32:43]

The CQ bit was checked by elawrence@chromium.org

[elawrence, 2017-02-15 21:32:43]

The patchset sent to the CQ was uploaded after l-g-t-m from dvadym@chromium.org
Link to the patchset: https://codereview.chromium.org/2682473002/#ps130001
(title: "Add negative test for non-username field")

[commit-bot: I haz the power, 2017-02-15 21:35:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-15 22:24:45]

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

[commit-bot: I haz the power, 2017-02-15 22:24:46]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[elawrence, 2017-02-15 23:28:55]

The CQ bit was checked by elawrence@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 23:30:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 01:34:47]

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

[commit-bot: I haz the power, 2017-02-16 01:34:48]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[elawrence, 2017-02-16 02:14:39]

The CQ bit was checked by elawrence@chromium.org

[elawrence, 2017-02-16 02:14:39]

The patchset sent to the CQ was uploaded after l-g-t-m from sebsg@chromium.org,
dvadym@chromium.org
Link to the patchset: https://codereview.chromium.org/2682473002/#ps150001
(title: "Correct ordering of private fields")

[commit-bot: I haz the power, 2017-02-16 02:15:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 04:08:02]

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

[commit-bot: I haz the power, 2017-02-16 04:08:03]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[elawrence, 2017-02-16 18:39:28]

The CQ bit was checked by elawrence@chromium.org

[commit-bot: I haz the power, 2017-02-16 18:40:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 18:47:33]

CQ is committing da patch.

Bot data: {"patchset_id": 150001, "attempt_start_ts": 1487270368305470,
"parent_rev": "8429cafc9f883a9715eeaad60d2216ba3b1cf138", "commit_rev":
"e4310b09c5b49b6ce2825021919049704a8ff4a6"}

[commit-bot: I haz the power, 2017-02-16 18:48:07]

Description was changed from

==========
Show Login Not Secure on all form fields if a password field is visible

Previously, the "Login Not Secure" notice was only shown for fields in
forms if the input type was Password or if the text field contained an
AutoComplete="username" attribute. This change enhances the warning to
show "Login Not Secure" notice for any text field in a form containing
a visible password field.

BUG=688612
TEST=Enable FNS, visit http://webdbg.com/test/forms/findlogins.htm
==========

to

==========
Show Login Not Secure on all form fields if a password field is visible

Previously, the "Login Not Secure" notice was only shown for fields in
forms if the input type was Password or if the text field contained an
AutoComplete="username" attribute. This change enhances the warning to
show "Login Not Secure" notice for any text field in a form containing
a visible password field.

BUG=688612
TEST=Enable FNS, visit http://webdbg.com/test/forms/findlogins.htm

Review-Url: https://codereview.chromium.org/2682473002
Cr-Commit-Position: refs/heads/master@{#451025}
Committed:
https://chromium.googlesource.com/chromium/src/+/e4310b09c5b49b6ce28250219190...
==========

[commit-bot: I haz the power, 2017-02-16 18:48:09]

Committed patchset #6 (id:150001) as
https://chromium.googlesource.com/chromium/src/+/e4310b09c5b49b6ce28250219190...