Show Form-Not-Secure warning on page load

Show Form-Not-Secure warning on page load

Currently, the Form-Not-Secure warning shows up in the autofill dropdown
when the autofill suggestions are shown, but it should also show up on page
load when a password form is autofilled.

This CL adds a ShowNotSecureWarning method that roughly mirrors the
Fill-On-Account-Select flow. When a password form is autofilled on page load,
and when the PasswordFormFillData indicates that the warning should be shown,
the renderer calls ShowNotSecureWarning() to tell the browser to show the
autofill popup with the form-not-secure warning (and no autofill suggestions).

BUG=672668
TEST=Save a password on http://rsolomakhin.github.io/autofill/ in the
Name/Password form. Load the page again and observe that the field
is autofilled with a "Login not secure" autofill dropdown.

Committed: https://crrev.com/50592c0e5e6c21d5b11f3a5f1515d9afcd5fe1a7
Cr-Commit-Position: refs/heads/master@{#441289}

[estark, 2016-12-29 16:27:16]

Patchset #5 (id:80001) has been deleted

[estark, 2016-12-29 16:27:30]

Patchset #4 (id:60001) has been deleted

[estark, 2016-12-29 16:27:37]

Patchset #3 (id:40001) has been deleted

[estark, 2016-12-29 16:27:43]

Patchset #2 (id:20001) has been deleted

[estark, 2016-12-29 16:27:50]

Patchset #1 (id:1) has been deleted

[estark, 2016-12-29 23:15:46]

Patchset #1 (id:100001) has been deleted

[estark, 2016-12-30 00:09:05]

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

[commit-bot: I haz the power, 2016-12-30 00:09:19]

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

[estark, 2016-12-30 00:52:37]

Description was changed from

==========
checkpoint: show FNS on page load

BUG=672668
==========

to

==========
Show Form-Not-Secure warning on page load

Currently, the Form-Not-Secure warning shows up in the autofill dropdown
when the autofill suggestions are shown, but it should also show up on page
load when a password form is autofilled.

This CL adds a ShowNotSecureWarning method that roughly mirrors the
Fill-On-Account-Select flow. When a password form is autofilled on page load,
and when the PasswordFormFillData indicates that the warning should be shown,
the renderer calls ShowNotSecureWarning() to tell the browser to show the
autofill popup with the form-not-secure warning (and no autofill suggestions).

BUG=672668
TEST=Save a password on http://rsolomakhin.github.io/autofill/ in the
Name/Password form. Load the page again and observe that the field
is autofilled with a "Login not secure" autofill dropdown.
==========

[commit-bot: I haz the power, 2016-12-30 00:57:59]

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

[commit-bot: I haz the power, 2016-12-30 00:57:59]

Dry run: This issue passed the CQ dry run.

[estark, 2016-12-30 00:59:52]

estark@chromium.org changed reviewers:
+ engedy@chromium.org, sebsg@chromium.org

[estark, 2016-12-30 00:59:53]

sebsg, can you please take a look at components/autofill, and engedy for
components/password_manager?

The specs for this are at go/fns-ui-spec; this CL is the "On page load" column
of the Password specs.

Note that this implementation suffers from a bug that fill-on-account-select
also has, which means the the popup sometimes doesn't show up on page load:
https://crbug.com/677144. I'll look into that separately.

Thanks!

[vabr (Chromium), 2017-01-02 10:01:35]

On 2016/12/30 00:59:53, estark wrote:
> sebsg, can you please take a look at components/autofill, and engedy for
> components/password_manager?
> 
> The specs for this are at go/fns-ui-spec; this CL is the "On page load" column
> of the Password specs.
> 
> Note that this implementation suffers from a bug that fill-on-account-select
> also has, which means the the popup sometimes doesn't show up on page load:
> https://crbug.com/677144. I'll look into that separately.
> 
> Thanks!

I'm looking at both autofill and password_manager now.
Cheers,
Vaclav

[vabr (Chromium), 2017-01-02 10:15:04]

LGTM, thanks!
Vaclav

https://codereview.chromium.org/2604453003/diff/140001/chrome/renderer/autofi...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/2604453003/diff/140001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1373:
SetHttpFormWarning();
optional: It might be a good idea to also test that ShowNotSecureWarning is not
called if kHttpFormWarningFeature is not enabled. Such test could have a TODO
linked to the HTTP warning launch bug to delete this test once
kHttpFormWarningFeature is the default behaviour.

On the other hand, launching this UI by accident puts the user in no danger, so
I'm also fine with you not testing for that, as long as it is a conscious
decision.

[engedy, 2017-01-02 10:35:36]

engedy@chromium.org changed reviewers:
- engedy@chromium.org

[engedy, 2017-01-02 10:35:49]

Thanks, Vaclav. :)

[estark, 2017-01-03 16:06:29]

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

[commit-bot: I haz the power, 2017-01-03 16:06:35]

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

[estark, 2017-01-03 16:08:36]

estark@chromium.org changed reviewers:
+ jochen@chromium.org, meacer@chromium.org

[estark, 2017-01-03 16:08:36]

jochen: can you please review the chrome/renderer/DEPS change?
meacer: can you please review the IPC changes in
components/autofill/content/common?
Thanks!

https://codereview.chromium.org/2604453003/diff/140001/chrome/renderer/autofi...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/2604453003/diff/140001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1373:
SetHttpFormWarning();
On 2017/01/02 10:15:04, vabr (mostly OOO until Feb) wrote:
> optional: It might be a good idea to also test that ShowNotSecureWarning is
not
> called if kHttpFormWarningFeature is not enabled. Such test could have a TODO
> linked to the HTTP warning launch bug to delete this test once
> kHttpFormWarningFeature is the default behaviour.
> 
> On the other hand, launching this UI by accident puts the user in no danger,
so
> I'm also fine with you not testing for that, as long as it is a conscious
> decision.

Done. Added some additional tests too.

Thanks!

[commit-bot: I haz the power, 2017-01-03 16:56:49]

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

[commit-bot: I haz the power, 2017-01-03 16:56:50]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-01-03 17:12:16]

lgtm either way

https://codereview.chromium.org/2604453003/diff/160001/chrome/renderer/DEPS
File chrome/renderer/DEPS (right):

https://codereview.chromium.org/2604453003/diff/160001/chrome/renderer/DEPS#n...
chrome/renderer/DEPS:28: "+components/security_state/core",
Will security_state/core eventually contain UI code or similar that can't go in
the renderer?

Would it make sense to split out the common code into core/common?

[meacer, 2017-01-04 00:03:11]

mojo lgtm

https://codereview.chromium.org/2604453003/diff/160001/components/autofill/co...
File components/autofill/content/common/autofill_driver.mojom (right):

https://codereview.chromium.org/2604453003/diff/160001/components/autofill/co...
components/autofill/content/common/autofill_driver.mojom:103: 
nit: Extra blank line?

https://codereview.chromium.org/2604453003/diff/160001/components/password_ma...
File
components/password_manager/core/browser/password_autofill_manager_unittest.cc
(right):

https://codereview.chromium.org/2604453003/diff/160001/components/password_ma...
components/password_manager/core/browser/password_autofill_manager_unittest.cc:609:
base::string16 warning_message =
nit: const

[estark, 2017-01-04 01:03:14]

https://codereview.chromium.org/2604453003/diff/160001/chrome/renderer/DEPS
File chrome/renderer/DEPS (right):

https://codereview.chromium.org/2604453003/diff/160001/chrome/renderer/DEPS#n...
chrome/renderer/DEPS:28: "+components/security_state/core",
On 2017/01/03 17:12:16, jochen wrote:
> Will security_state/core eventually contain UI code or similar that can't go
in
> the renderer?
> 
> Would it make sense to split out the common code into core/common?

I'll keep this in mind for future, though I don't think it makes sense to do it
right now just for the sake of using a Feature in a test. (I don't anticipate
the addition of code that can't go in the renderer at the moment.) In fact, I
think I'll move this dep into specific_include_rules in
chrome/renderer/autofill/DEPS since it's only needed for the one test.

https://codereview.chromium.org/2604453003/diff/160001/components/autofill/co...
File components/autofill/content/common/autofill_driver.mojom (right):

https://codereview.chromium.org/2604453003/diff/160001/components/autofill/co...
components/autofill/content/common/autofill_driver.mojom:103: 
On 2017/01/04 00:03:11, Mustafa Emre Acer wrote:
> nit: Extra blank line?

Done.

https://codereview.chromium.org/2604453003/diff/160001/components/password_ma...
File
components/password_manager/core/browser/password_autofill_manager_unittest.cc
(right):

https://codereview.chromium.org/2604453003/diff/160001/components/password_ma...
components/password_manager/core/browser/password_autofill_manager_unittest.cc:609:
base::string16 warning_message =
On 2017/01/04 00:03:11, Mustafa Emre Acer wrote:
> nit: const

Done.

[estark, 2017-01-04 01:03:54]

The CQ bit was checked by estark@chromium.org

[estark, 2017-01-04 01:03:54]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
jochen@chromium.org, meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/2604453003/#ps180001
(title: "meacer, jochen comments")

[commit-bot: I haz the power, 2017-01-04 01:04:20]

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-01-04 01:52:44]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1483491834339800,
"parent_rev": "1b4f834c0bab04a2f6a8f5345637b1e96e072738", "commit_rev":
"7dfa2dac2e56f0f01ab51a775fbefc31f9ad9820"}

[commit-bot: I haz the power, 2017-01-04 01:53:16]

Description was changed from

==========
Show Form-Not-Secure warning on page load

Currently, the Form-Not-Secure warning shows up in the autofill dropdown
when the autofill suggestions are shown, but it should also show up on page
load when a password form is autofilled.

This CL adds a ShowNotSecureWarning method that roughly mirrors the
Fill-On-Account-Select flow. When a password form is autofilled on page load,
and when the PasswordFormFillData indicates that the warning should be shown,
the renderer calls ShowNotSecureWarning() to tell the browser to show the
autofill popup with the form-not-secure warning (and no autofill suggestions).

BUG=672668
TEST=Save a password on http://rsolomakhin.github.io/autofill/ in the
Name/Password form. Load the page again and observe that the field
is autofilled with a "Login not secure" autofill dropdown.
==========

to

==========
Show Form-Not-Secure warning on page load

Currently, the Form-Not-Secure warning shows up in the autofill dropdown
when the autofill suggestions are shown, but it should also show up on page
load when a password form is autofilled.

This CL adds a ShowNotSecureWarning method that roughly mirrors the
Fill-On-Account-Select flow. When a password form is autofilled on page load,
and when the PasswordFormFillData indicates that the warning should be shown,
the renderer calls ShowNotSecureWarning() to tell the browser to show the
autofill popup with the form-not-secure warning (and no autofill suggestions).

BUG=672668
TEST=Save a password on http://rsolomakhin.github.io/autofill/ in the
Name/Password form. Load the page again and observe that the field
is autofilled with a "Login not secure" autofill dropdown.

Review-Url: https://codereview.chromium.org/2604453003
==========

[commit-bot: I haz the power, 2017-01-04 01:53:19]

Committed patchset #4 (id:180001)

[commit-bot: I haz the power, 2017-01-04 01:55:20]

Description was changed from

==========
Show Form-Not-Secure warning on page load

Currently, the Form-Not-Secure warning shows up in the autofill dropdown
when the autofill suggestions are shown, but it should also show up on page
load when a password form is autofilled.

This CL adds a ShowNotSecureWarning method that roughly mirrors the
Fill-On-Account-Select flow. When a password form is autofilled on page load,
and when the PasswordFormFillData indicates that the warning should be shown,
the renderer calls ShowNotSecureWarning() to tell the browser to show the
autofill popup with the form-not-secure warning (and no autofill suggestions).

BUG=672668
TEST=Save a password on http://rsolomakhin.github.io/autofill/ in the
Name/Password form. Load the page again and observe that the field
is autofilled with a "Login not secure" autofill dropdown.

Review-Url: https://codereview.chromium.org/2604453003
==========

to

==========
Show Form-Not-Secure warning on page load

Currently, the Form-Not-Secure warning shows up in the autofill dropdown
when the autofill suggestions are shown, but it should also show up on page
load when a password form is autofilled.

This CL adds a ShowNotSecureWarning method that roughly mirrors the
Fill-On-Account-Select flow. When a password form is autofilled on page load,
and when the PasswordFormFillData indicates that the warning should be shown,
the renderer calls ShowNotSecureWarning() to tell the browser to show the
autofill popup with the form-not-secure warning (and no autofill suggestions).

BUG=672668
TEST=Save a password on http://rsolomakhin.github.io/autofill/ in the
Name/Password form. Load the page again and observe that the field
is autofilled with a "Login not secure" autofill dropdown.

Committed: https://crrev.com/50592c0e5e6c21d5b11f3a5f1515d9afcd5fe1a7
Cr-Commit-Position: refs/heads/master@{#441289}
==========

[commit-bot: I haz the power, 2017-01-04 01:55:21]

Patchset 4 (id:??) landed as
https://crrev.com/50592c0e5e6c21d5b11f3a5f1515d9afcd5fe1a7
Cr-Commit-Position: refs/heads/master@{#441289}