Trigger protected password entry request on password reuse event.

Trigger protected password entry request on password reuse event.

BUG=698899
Review-Url: https://codereview.chromium.org/2878813002
Cr-Commit-Position: refs/heads/master@{#472828}
Committed: https://chromium.googlesource.com/chromium/src/+/31a45cd7736a6224fa8f7b6eafc091fd25d8b803

[Jialiu Lin, 2017-05-11 23:57:36]

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

[commit-bot: I haz the power, 2017-05-11 23:58:44]

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-05-12 01:42:11]

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

[commit-bot: I haz the power, 2017-05-12 01:42:11]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2017-05-12 17:38:04]

jialiul@chromium.org changed reviewers:
+ dvadym@chromium.org, nparker@chromium.org

[Jialiu Lin, 2017-05-12 17:38:05]

Hi nparker@, dvadym@, 

This is the CL to trigger protected password entry ping (a.k.a password reuse
request). Could you take a look?  Thanks!

[Nathan Parker, 2017-05-12 22:28:00]

https://codereview.chromium.org/2878813002/diff/20001/components/password_man...
File
components/password_manager/core/browser/password_reuse_detection_manager.cc
(right):

https://codereview.chromium.org/2878813002/diff/20001/components/password_man...
components/password_manager/core/browser/password_reuse_detection_manager.cc:72:
password_protection_service->MaybeStartProtectedPasswordEntryRequest(
Could the Record be called from within MaybeStart? That way it could be private
and not exposed here.

https://codereview.chromium.org/2878813002/diff/20001/components/safe_browsin...
File components/safe_browsing/password_protection/password_protection_request.cc
(right):

https://codereview.chromium.org/2878813002/diff/20001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_request.cc:87:
void PasswordProtectionRequest::FillRequestProto() {
Are there tests for this function?

https://codereview.chromium.org/2878813002/diff/20001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_request.cc:106:
main_frame->set_has_password_field(true);
I think all this form/frame info should be filled for both request types.  So
you could pull it out of the switch.

https://codereview.chromium.org/2878813002/diff/20001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_request.cc:112:
password_frame->set_has_password_field(true);
So do we need to set_has_password_field on both the password_frame and the
password_form?  It seems redundant.  Maybe the password_form doesn't need the
field at all, as it could be implied that the form has a password field. That
would work as long as we only collect forms that have password fields.

https://codereview.chromium.org/2878813002/diff/20001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_request.cc:125:
password_reuse->add_password_reused_original_origins(legitimate_domain_);
This one should only be filled for SBER.  That's the case now be default, but we
should protect this before we switch the finch config.

https://codereview.chromium.org/2878813002/diff/20001/components/safe_browsin...
File components/safe_browsing/password_protection/password_protection_request.h
(right):

https://codereview.chromium.org/2878813002/diff/20001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_request.h:64:
const std::string& legitinate_domain,
nit: spelling. legitimate_domain

https://codereview.chromium.org/2878813002/diff/20001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_request.h:132:
const std::string legitimate_domain_;
Could we call this "saved_domain" or "associated_domain"?  Or is
legitimate_domain used elsewhere already? I'm assuming eventually this'll be a
vector.

https://codereview.chromium.org/2878813002/diff/20001/components/safe_browsin...
File components/safe_browsing/password_protection/password_protection_service.cc
(right):

https://codereview.chromium.org/2878813002/diff/20001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_service.cc:290:
std::string(),
/* legitimate_domain: not used for this type */

[Jialiu Lin, 2017-05-13 00:23:11]

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

[commit-bot: I haz the power, 2017-05-13 00:23:52]

Dry run: CQ is trying da patch.

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

[Jialiu Lin, 2017-05-13 00:24:14]

Thanks!

https://codereview.chromium.org/2878813002/diff/20001/components/password_man...
File
components/password_manager/core/browser/password_reuse_detection_manager.cc
(right):

https://codereview.chromium.org/2878813002/diff/20001/components/password_man...
components/password_manager/core/browser/password_reuse_detection_manager.cc:72:
password_protection_service->MaybeStartProtectedPasswordEntryRequest(
On 2017/05/12 at 22:27:59, Nathan Parker wrote:
> Could the Record be called from within MaybeStart? That way it could be
private and not exposed here.

I was thinking about removing the RecordPasswordReuse function, since this
metrics (PasswordManager.PasswordReuse.MainFrameMatchCsdWhitelist) is kind of
redundant once  we started pinging.

If password_manager team is not actively using this metric, I would suggest
deprecate it. 

+dvadym, could you comment on this?

https://codereview.chromium.org/2878813002/diff/20001/components/safe_browsin...
File components/safe_browsing/password_protection/password_protection_request.cc
(right):

https://codereview.chromium.org/2878813002/diff/20001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_request.cc:87:
void PasswordProtectionRequest::FillRequestProto() {
On 2017/05/12 at 22:27:59, Nathan Parker wrote:
> Are there tests for this function?

Done. Added 2 unit tests to verify request proto.

https://codereview.chromium.org/2878813002/diff/20001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_request.cc:106:
main_frame->set_has_password_field(true);
On 2017/05/12 at 22:27:59, Nathan Parker wrote:
> I think all this form/frame info should be filled for both request types.  So
you could pull it out of the switch.

This is temporary. password form, password_form_frame_url, and sub frame info is
not available to password reuse ping yet.  I'll refactor this part when there is
more info to fill in.

https://codereview.chromium.org/2878813002/diff/20001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_request.cc:112:
password_frame->set_has_password_field(true);
On 2017/05/12 at 22:27:59, Nathan Parker wrote:
> So do we need to set_has_password_field on both the password_frame and the
password_form?  It seems redundant.  Maybe the password_form doesn't need the
field at all, as it could be implied that the form has a password field. That
would work as long as we only collect forms that have password fields.

Agree. It is redundant. remove the filling of has_password_field on
password_form.
I'll talk to weiningy to make some refine the proto definition.

https://codereview.chromium.org/2878813002/diff/20001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_request.cc:125:
password_reuse->add_password_reused_original_origins(legitimate_domain_);
On 2017/05/12 at 22:27:59, Nathan Parker wrote:
> This one should only be filled for SBER.  That's the case now be default, but
we should protect this before we switch the finch config.

Yes, thanks for catching this.

https://codereview.chromium.org/2878813002/diff/20001/components/safe_browsin...
File components/safe_browsing/password_protection/password_protection_request.h
(right):

https://codereview.chromium.org/2878813002/diff/20001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_request.h:64:
const std::string& legitinate_domain,
On 2017/05/12 at 22:27:59, Nathan Parker wrote:
> nit: spelling. legitimate_domain

Done.

https://codereview.chromium.org/2878813002/diff/20001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_request.h:132:
const std::string legitimate_domain_;
On 2017/05/12 at 22:27:59, Nathan Parker wrote:
> Could we call this "saved_domain" or "associated_domain"?  Or is
legitimate_domain used elsewhere already? I'm assuming eventually this'll be a
vector.

Rename to "saved_domain".
"legitimate_domain" is used by PasswordReuseDetectionManager::OnReuseFound(). 
"saved_domain" is more easier to understand. 
I'll do the string to vector change later when password_manager function is
ready to expose all saved domains.

https://codereview.chromium.org/2878813002/diff/20001/components/safe_browsin...
File components/safe_browsing/password_protection/password_protection_service.cc
(right):

https://codereview.chromium.org/2878813002/diff/20001/components/safe_browsin...
components/safe_browsing/password_protection/password_protection_service.cc:290:
std::string(),
On 2017/05/12 at 22:28:00, Nathan Parker wrote:
> /* legitimate_domain: not used for this type */

Done.

[commit-bot: I haz the power, 2017-05-13 01:26:25]

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

[commit-bot: I haz the power, 2017-05-13 01:26:26]

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

[Nathan Parker, 2017-05-15 17:56:46]

cool, LGTM (FYI Vadym was out Fri/Mon)

[Jialiu Lin, 2017-05-16 00:58:44]

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

[commit-bot: I haz the power, 2017-05-16 00:59:46]

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-05-16 02:59:59]

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

[commit-bot: I haz the power, 2017-05-16 03:00:00]

Dry run: This issue passed the CQ dry run.

[dvadym, 2017-05-16 12:01:58]

LGTM

[Jialiu Lin, 2017-05-16 16:24:27]

Thanks dvadym! A quick question for you

https://codereview.chromium.org/2878813002/diff/100001/components/password_ma...
File
components/password_manager/core/browser/password_reuse_detection_manager.cc
(right):

https://codereview.chromium.org/2878813002/diff/100001/components/password_ma...
components/password_manager/core/browser/password_reuse_detection_manager.cc:71:
password_protection_service->RecordPasswordReuse(main_frame_url_);
dvadym@, are you actively using the
"PasswordManager.PasswordReuse.MainFrameMatchCsdWhitelist" UMA metric?  It is
kind of duplicated with "PasswordProtection.RequestOutcome".  If you're not
using it, I'll remove the "RecordPasswordReuse()" function. WDYT?

[dvadym, 2017-05-17 16:25:41]

https://codereview.chromium.org/2878813002/diff/100001/components/password_ma...
File
components/password_manager/core/browser/password_reuse_detection_manager.cc
(right):

https://codereview.chromium.org/2878813002/diff/100001/components/password_ma...
components/password_manager/core/browser/password_reuse_detection_manager.cc:71:
password_protection_service->RecordPasswordReuse(main_frame_url_);
On 2017/05/16 16:24:27, Jialiu Lin wrote:
> dvadym@, are you actively using the
> "PasswordManager.PasswordReuse.MainFrameMatchCsdWhitelist" UMA metric?  It is
> kind of duplicated with "PasswordProtection.RequestOutcome".  If you're not
> using it, I'll remove the "RecordPasswordReuse()" function. WDYT?

Sure, if it measures the similar data. We don't use this metric. Thanks for
cleaning!

[Jialiu Lin, 2017-05-17 18:01:34]

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

[commit-bot: I haz the power, 2017-05-17 18:03:39]

Dry run: CQ is trying da patch.

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

[Jialiu Lin, 2017-05-17 18:45:43]

jialiul@chromium.org changed reviewers:
+ holte@chromium.org

[Jialiu Lin, 2017-05-17 18:45:44]

+holte@ for owner review of histograms.xml

Thanks!

[Steven Holte, 2017-05-17 23:17:17]

lgtm

[Jialiu Lin, 2017-05-17 23:21:06]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2017-05-17 23:21:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
nparker@chromium.org, dvadym@chromium.org
Link to the patchset: https://codereview.chromium.org/2878813002/#ps140001
(title: "nit")

[commit-bot: I haz the power, 2017-05-17 23:22:12]

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-05-18 04:50:54]

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

[commit-bot: I haz the power, 2017-05-18 04:50:56]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Jialiu Lin, 2017-05-18 15:05:56]

The CQ bit was checked by jialiul@chromium.org

[commit-bot: I haz the power, 2017-05-18 15:06:42]

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-05-18 16:05:02]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1495119956964830,
"parent_rev": "57598441ff56d8e8005c7d9547246dbe1f149bf5", "commit_rev":
"31a45cd7736a6224fa8f7b6eafc091fd25d8b803"}

[commit-bot: I haz the power, 2017-05-18 16:05:12]

Description was changed from

==========
Trigger protected password entry request on password reuse event.

BUG=698899
==========

to

==========
Trigger protected password entry request on password reuse event.

BUG=698899

Review-Url: https://codereview.chromium.org/2878813002
Cr-Commit-Position: refs/heads/master@{#472828}
Committed:
https://chromium.googlesource.com/chromium/src/+/31a45cd7736a6224fa8f7b6eafc0...
==========

[commit-bot: I haz the power, 2017-05-18 16:05:13]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/31a45cd7736a6224fa8f7b6eafc0...