Implementation of sync password reuse checking.

Implementation of sync password reuse checking.

Comparison of hashes of all suffixes of input with the sync password hash is implemented.

BUG=657041
Review-Url: https://codereview.chromium.org/2847743002
Cr-Commit-Position: refs/heads/master@{#467970}
Committed: https://chromium.googlesource.com/chromium/src/+/b62c659f2d62011b43e5930f07ac6bb57254331c

[dvadym, 2017-04-27 13:53:55]

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

[dvadym, 2017-04-27 13:53:56]

Hi Vaclav,

could you please review this CL?

Regards,
Vadym

[dvadym, 2017-04-27 13:58:20]

Description was changed from

==========
Implementation of sync password reuse checking

BUG=657041
==========

to

==========
Implementation of sync password reuse checking.

Implementation of comparison hashes of all suffixes of input with hash of the
sync password.

BUG=657041
==========

[dvadym, 2017-04-27 13:59:19]

Description was changed from

==========
Implementation of sync password reuse checking.

Implementation of comparison hashes of all suffixes of input with hash of the
sync password.

BUG=657041
==========

to

==========
Implementation of sync password reuse checking.

Comparison of hashes of all suffixes of input with the sync password hash is
implemented.

BUG=657041
==========

[vabr (Chromium), 2017-04-27 14:17:49]

LGTM with nits.

Cheers,
Vaclav

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

https://codereview.chromium.org/2847743002/diff/20001/components/password_man...
components/password_manager/core/browser/password_reuse_detector.cc:71: if
(CheckSyncPasswordReuse(input, domain, consumer))
nit: Do you want to document in a comment that the order of first checking the
sync password and then the others is important? Or is it not important?

https://codereview.chromium.org/2847743002/diff/20001/components/password_man...
components/password_manager/core/browser/password_reuse_detector.cc:130: //
TODO(crbug.com/657041) Implement saving of sync password hash.
nit: "saving" is not completely clear. Does it mean saving on disk, or is it
saving in memory (and hence obsolete)?

https://codereview.chromium.org/2847743002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_reuse_detector.h (right):

https://codereview.chromium.org/2847743002/diff/20001/components/password_man...
components/password_manager/core/browser/password_reuse_detector.h:93:
base::Optional<uint64_t> sync_password_hash_;
#include <stdint.h> for uint64_t

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

https://codereview.chromium.org/2847743002/diff/20001/components/password_man...
components/password_manager/core/browser/password_reuse_detector_unittest.cc:187:
TEST(PasswordReuseDetectorTest, CheckingSyncPassword) {
Please split this to particular cases (google.com, prefix, wrong domain). This
will add some boiler-plate, but also remove the VerifyAndChelarExpectations()
and will keep the tests focused, small and fast/parallelisable.

https://codereview.chromium.org/2847743002/diff/20001/components/password_man...
components/password_manager/core/browser/password_reuse_detector_unittest.cc:203:
"accounts.google.com", 1, 0));
The declaration comment for this argument in password_reuse_detector_consumer.h
is:
"|saved_domain| is the domain on which |password| is saved."

That's now incorrect. It is the domain where the password use is legitimate.
Please fix that comment and also the argument name.

https://codereview.chromium.org/2847743002/diff/20001/components/password_man...
components/password_manager/core/browser/password_reuse_detector_unittest.cc:208:
// Check that saved password reuse also works.
This is already covered in TypingPasswordOnDifferentSite.

[dvadym, 2017-04-27 16:40:50]

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

[dvadym, 2017-04-27 16:40:54]

Thanks Vaclav for review! I've addressed you comments

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

https://codereview.chromium.org/2847743002/diff/20001/components/password_man...
components/password_manager/core/browser/password_reuse_detector.cc:71: if
(CheckSyncPasswordReuse(input, domain, consumer))
On 2017/04/27 14:17:49, vabr (Chromium) wrote:
> nit: Do you want to document in a comment that the order of first checking the
> sync password and then the others is important? Or is it not important?

I don't think that order matter, so I've written:
  // It seems that the checking order does not really matter, let us check the
  // sync password first.

https://codereview.chromium.org/2847743002/diff/20001/components/password_man...
components/password_manager/core/browser/password_reuse_detector.cc:130: //
TODO(crbug.com/657041) Implement saving of sync password hash.
On 2017/04/27 14:17:49, vabr (Chromium) wrote:
> nit: "saving" is not completely clear. Does it mean saving on disk, or is it
> saving in memory (and hence obsolete)?

Saving into preferences, updated.

https://codereview.chromium.org/2847743002/diff/20001/components/password_man...
File components/password_manager/core/browser/password_reuse_detector.h (right):

https://codereview.chromium.org/2847743002/diff/20001/components/password_man...
components/password_manager/core/browser/password_reuse_detector.h:93:
base::Optional<uint64_t> sync_password_hash_;
On 2017/04/27 14:17:49, vabr (Chromium) wrote:
> #include <stdint.h> for uint64_t

Done.

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

https://codereview.chromium.org/2847743002/diff/20001/components/password_man...
components/password_manager/core/browser/password_reuse_detector_unittest.cc:187:
TEST(PasswordReuseDetectorTest, CheckingSyncPassword) {
On 2017/04/27 14:17:49, vabr (Chromium) wrote:
> Please split this to particular cases (http://google.com, prefix, wrong
domain). This
> will add some boiler-plate, but also remove the VerifyAndChelarExpectations()
> and will keep the tests focused, small and fast/parallelisable.

Done.

https://codereview.chromium.org/2847743002/diff/20001/components/password_man...
components/password_manager/core/browser/password_reuse_detector_unittest.cc:203:
"accounts.google.com", 1, 0));
On 2017/04/27 14:17:49, vabr (Chromium) wrote:
> The declaration comment for this argument in
password_reuse_detector_consumer.h
> is:
> "|saved_domain| is the domain on which |password| is saved."
> 
> That's now incorrect. It is the domain where the password use is legitimate.
> Please fix that comment and also the argument name.

Done.

https://codereview.chromium.org/2847743002/diff/20001/components/password_man...
components/password_manager/core/browser/password_reuse_detector_unittest.cc:208:
// Check that saved password reuse also works.
On 2017/04/27 14:17:49, vabr (Chromium) wrote:
> This is already covered in TypingPasswordOnDifferentSite.

Yes, but in that test no sync password is available. This test verifies that
both checking work together.

[commit-bot: I haz the power, 2017-04-27 16:41:26]

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-04-27 17:01:15]

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

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

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[vabr (Chromium), 2017-04-28 07:35:51]

LGTM with two more nits.

Thanks,
Vaclav

https://codereview.chromium.org/2847743002/diff/40001/components/password_man...
File components/password_manager/core/browser/password_reuse_detector.cc
(right):

https://codereview.chromium.org/2847743002/diff/40001/components/password_man...
components/password_manager/core/browser/password_reuse_detector.cc:71: // It
seems that the checking order does not really matter, let us check the
Because the order is not important, the comment can be deleted. The purpose of
the comment was to preserve the order if important, but there is nothing to
preserve.

Also in general, please do not use "we" / "us" in the comments (go/avoidwe),
because it is vague and does not help.

https://codereview.chromium.org/2847743002/diff/40001/components/password_man...
File
components/password_manager/core/browser/password_reuse_detector_unittest.cc
(right):

https://codereview.chromium.org/2847743002/diff/40001/components/password_man...
components/password_manager/core/browser/password_reuse_detector_unittest.cc:215:
TEST(PasswordReuseDetectorTest, SavedPasswordsReuseSyncPasswordAvailable) {
I get your point about combining the sync password hash and checking reuse of
saved passwords. But please explain that in the code (the test name does not
help here). Something like adding the following line before line 215:
// Check that reuse of saved passwords is detected also if the hash of a sync
password is saved.

[dvadym, 2017-04-28 11:54:22]

Thanks for review!

https://codereview.chromium.org/2847743002/diff/40001/components/password_man...
File components/password_manager/core/browser/password_reuse_detector.cc
(right):

https://codereview.chromium.org/2847743002/diff/40001/components/password_man...
components/password_manager/core/browser/password_reuse_detector.cc:71: // It
seems that the checking order does not really matter, let us check the
On 2017/04/28 07:35:51, vabr (Chromium) wrote:
> Because the order is not important, the comment can be deleted. The purpose of
> the comment was to preserve the order if important, but there is nothing to
> preserve.
> 
> Also in general, please do not use "we" / "us" in the comments (go/avoidwe),
> because it is vague and does not help.

Done.

https://codereview.chromium.org/2847743002/diff/40001/components/password_man...
File
components/password_manager/core/browser/password_reuse_detector_unittest.cc
(right):

https://codereview.chromium.org/2847743002/diff/40001/components/password_man...
components/password_manager/core/browser/password_reuse_detector_unittest.cc:215:
TEST(PasswordReuseDetectorTest, SavedPasswordsReuseSyncPasswordAvailable) {
On 2017/04/28 07:35:51, vabr (Chromium) wrote:
> I get your point about combining the sync password hash and checking reuse of
> saved passwords. But please explain that in the code (the test name does not
> help here). Something like adding the following line before line 215:
> // Check that reuse of saved passwords is detected also if the hash of a sync
> password is saved.

Thanks it makes sense

[dvadym, 2017-04-28 11:54:30]

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

[commit-bot: I haz the power, 2017-04-28 11:54:55]

Dry run: CQ is trying da patch.

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

[vabr (Chromium), 2017-04-28 12:19:29]

LGTM unconditionally. :)
Vaclav

[commit-bot: I haz the power, 2017-04-28 12:43:18]

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

[commit-bot: I haz the power, 2017-04-28 12:43:19]

Dry run: This issue passed the CQ dry run.

[dvadym, 2017-04-28 12:47:25]

The CQ bit was checked by dvadym@chromium.org

[commit-bot: I haz the power, 2017-04-28 12:47:39]

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-04-28 12:53:24]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493383645179420,
"parent_rev": "aa95dd2a139633f9e3a03f59b6a8d6468d8c9e28", "commit_rev":
"b62c659f2d62011b43e5930f07ac6bb57254331c"}

[commit-bot: I haz the power, 2017-04-28 12:53:34]

Description was changed from

==========
Implementation of sync password reuse checking.

Comparison of hashes of all suffixes of input with the sync password hash is
implemented.

BUG=657041
==========

to

==========
Implementation of sync password reuse checking.

Comparison of hashes of all suffixes of input with the sync password hash is
implemented.

BUG=657041

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

[commit-bot: I haz the power, 2017-04-28 12:53:35]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/b62c659f2d62011b43e5930f07ac...