Interface for sync password saving.

Interface for sync password saving.

This CL has only interface for saving sync passwords in PasswordStore and passing this call to PasswordReuseDetector. Implementation is complex and probably will consist of many CLs, but having interface unblocks other parts.

Tests will be added with implementation.

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

[dvadym, 2017-04-27 10:06:40]

Description was changed from

==========
Interface for sync password saving

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

to

==========
Interface for sync password saving.

This CL has only interface for saving sync passwords in PasswordStore and
pathing this call to PasswordReuseDetector. Implementation is complex and
probably will consist in many CLs, but having interface unblocks other parts.

Tests will be added with implementation.

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

[dvadym, 2017-04-27 10:06:41]

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

[dvadym, 2017-04-27 10:06:49]

Description was changed from

==========
Interface for sync password saving.

This CL has only interface for saving sync passwords in PasswordStore and
pathing this call to PasswordReuseDetector. Implementation is complex and
probably will consist in many CLs, but having interface unblocks other parts.

Tests will be added with implementation.

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

to

==========
Interface for sync password saving.

This CL has only interface for saving sync passwords in PasswordStore and
passing this call to PasswordReuseDetector. Implementation is complex and
probably will consist in many CLs, but having interface unblocks other parts.

Tests will be added with implementation.

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

[dvadym, 2017-04-27 10:08:09]

Description was changed from

==========
Interface for sync password saving.

This CL has only interface for saving sync passwords in PasswordStore and
passing this call to PasswordReuseDetector. Implementation is complex and
probably will consist in many CLs, but having interface unblocks other parts.

Tests will be added with implementation.

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

to

==========
Interface for sync password saving.

This CL has only interface for saving sync passwords in PasswordStore and
passing this call to PasswordReuseDetector. Implementation is complex and
probably will consist of many CLs, but having interface unblocks other parts.

Tests will be added with implementation.

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

[dvadym, 2017-04-27 10:09:04]

Hi Vaclav,

could you please review this CL?

Regards,
Vadym

[vabr (Chromium), 2017-04-27 11:12:29]

Hi Vadym,

Once concern I have here is that the protection of the sync password is not a
password manager feature, it is a sync feature. In particular, I understand that
it can be enabled even if password saving is disabled.

Therefore it seems unfortunate to pass this through PasswordManager. Isn't there
a better-suited sync service for that?

Cheers,
Vaclav

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

https://codereview.chromium.org/2846823002/diff/60001/components/password_man...
components/password_manager/core/browser/password_reuse_detector.cc:84: //
todo(crbug.com/657041) Implement saving of sync password hash.
nit: Capitalise "todo" to "TODO".

[dvadym, 2017-04-27 11:24:19]

Thanks for comments!

1.If Password saving is disabled, PasswordStore is created anyway (for filling),
so it looks no problem with this. 
2.A hash of sync password has to be in PasswordReuseDetector (because this is
the place where checking of keystrokes is done), putting checking of a sync
password in a sync component means duplicating all infrastructure that currently
built for PasswordReuseDetector. 

So it looks that PasswordReuseDetector is better place, despite this is not
fully Password Manager feature.

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

https://codereview.chromium.org/2846823002/diff/60001/components/password_man...
components/password_manager/core/browser/password_reuse_detector.cc:84: //
todo(crbug.com/657041) Implement saving of sync password hash.
On 2017/04/27 11:12:29, vabr (Chromium) wrote:
> nit: Capitalise "todo" to "TODO".

Done.

[dvadym, 2017-04-27 11:25:18]

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

[commit-bot: I haz the power, 2017-04-27 11:25:26]

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-27 11:38:42]

On 2017/04/27 11:24:19, dvadym wrote:
> Thanks for comments!
> 
> 1.If Password saving is disabled, PasswordStore is created anyway (for
filling),
> so it looks no problem with this. 
> 2.A hash of sync password has to be in PasswordReuseDetector (because this is
> the place where checking of keystrokes is done), putting checking of a sync
> password in a sync component means duplicating all infrastructure that
currently
> built for PasswordReuseDetector. 
> 
> So it looks that PasswordReuseDetector is better place, despite this is not
> fully Password Manager feature.

Thanks for explaining, I overlooked that PasswordReuseDetector is owned by
PasswordStore.

My concerns still stand -- the fact that PasswordStore is created anyway is a
coincidence and I expect it to go away with servicification. At that point we'll
have some technical debt to pay for the coupling of PasswordReuseDetector is
owned by PasswordStore.

I no longer think this CL introduces the issue, it just makes it more likely to
hit us later. Therefore the CL itself LGTM, but I would appreciate a bug
tracking decoupling keystrokes checking from PasswordStore.

Cheers,
Vaclav

[dvadym, 2017-04-27 11:45:34]

Thanks for review Vaclav! I've created a bug
https://bugs.chromium.org/p/chromium/issues/detail?id=715987

[dvadym, 2017-04-27 11:45:43]

The CQ bit was unchecked by dvadym@chromium.org

[dvadym, 2017-04-27 11:45:43]

The CQ bit was checked by dvadym@chromium.org

[commit-bot: I haz the power, 2017-04-27 11:45:50]

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 12:11:53]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493293543669300,
"parent_rev": "15e8fb8c185a6137e42c895401c127972249d26f", "commit_rev":
"b7503b4cee7f355c9e51bfc5318a1809bfe64637"}

[commit-bot: I haz the power, 2017-04-27 12:12:08]

Description was changed from

==========
Interface for sync password saving.

This CL has only interface for saving sync passwords in PasswordStore and
passing this call to PasswordReuseDetector. Implementation is complex and
probably will consist of many CLs, but having interface unblocks other parts.

Tests will be added with implementation.

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

to

==========
Interface for sync password saving.

This CL has only interface for saving sync passwords in PasswordStore and
passing this call to PasswordReuseDetector. Implementation is complex and
probably will consist of many CLs, but having interface unblocks other parts.

Tests will be added with implementation.

BUG=657041

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

[commit-bot: I haz the power, 2017-04-27 12:12:09]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/b7503b4cee7f355c9e51bfc5318a...