Implement pin storage backend.

Implement pin storage backend.

This provides the backend C++ api for managing and authenticating different PINs. It will be used by the lock screen and by the settings.

BUG=603217
Committed: https://crrev.com/489f430f0a9a61984f4f114bbed9bd6ce1e14c77
Cr-Commit-Position: refs/heads/master@{#395156}

[jdufault, 2016-05-13 19:43:51]

Description was changed from

==========
wip

BUG=
==========

to

==========
Implement pin storage backend.

This provides the backend C++ api for managing and authenticating different
PINs. It will be used by the lock screen and by the settings.

BUG=603217
==========

[jdufault, 2016-05-13 19:44:13]

jdufault@chromium.org changed reviewers:
+ achuith@chromium.org, stevenjb@chromium.org

[jdufault, 2016-05-13 20:04:53]

Achuith or Steven, PTAL.

I wanted keep pin storage and pin authentication separate concepts, so I put
them into separate files. I'm fine merging pin_authentication into pin_storage
if you prefer, though.

PinStorage is currently an interface. I opted for this to hide the
BrowserContextedKeyedAPI boilerplate and to make it easier to write a fake
PinStorage implementation, but I'm not sure that we will end up needing the fake
PinStorage.

There are still some big open questions about how the lock screen with multiple
users will work; depending on the answers to those questions, we can split up
the unlock attempt count and the time since the last strong authentication from
the per-profile storage.

Thanks!

[jdufault, 2016-05-13 20:17:48]

Patchset #1 (id:1) has been deleted

[achuithb, 2016-05-13 23:34:57]

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/quick_unlock/pin_authentication.cc (right):

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_authentication.cc:14: //
TODO(jdufault): Pull these values in from policy.
Reference a bug

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/quick_unlock/pin_authentication.h (right):

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_authentication.h:15: bool
IsPinAvailable(PinStorage* storage);
These should be in a new namespace nested in chromeos, I believe.

Why are these not just methods of PinStorage?

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/quick_unlock/pin_storage.cc (right):

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:27: ~PinStorageImpl()
override;
Can this be protected?

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:29: // PinStorage
implementation:
Everything below here can be private, right?

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:35: int
UnlockAttemptCount() override;
const

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:58: base::Time
last_strong_auth_;
should this be initialized to something?

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:85: attempt_count_(0)
{}
move to class

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:114: void
PinStorageImpl::SetPin(const std::string& raw_pin) {
This method is probably the most significant bit of code in this CL. Is there a
way to unittest this?

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:115: // Compute salt.
base64 encode it so it is a UTF8 string, which is required by
Generate random salt.

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:125: std::string
secret = key.GetSecret();
const

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:138: std::string
PinStorageImpl::pin_salt() const {
PinSalt?

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:142: std::string
PinStorageImpl::pin_secret() const {
PinSecret?

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:156:
user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
Do we want this to be syncable?

[stevenjb, 2016-05-16 16:33:34]

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/quick_unlock/pin_authentication.cc (right):

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_authentication.cc:34: }
These both look like they should be PinStorage member functions.

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/quick_unlock/pin_authentication.h (right):

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_authentication.h:15: bool
IsPinAvailable(PinStorage* storage);
On 2016/05/13 23:34:56, achuithb wrote:
> These should be in a new namespace nested in chromeos, I believe.
> 
> Why are these not just methods of PinStorage?

+1

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/quick_unlock/pin_storage.h (right):

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.h:26: static PinStorage*
GetInstance(Profile* profile);
We should use a BrowserContextKeyedServiceFactory for this. E.g. see
CaptivePortalService and CaptivePortalServiceFactory.

[jdufault, 2016-05-16 22:08:48]

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/quick_unlock/pin_authentication.cc (right):

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_authentication.cc:14: //
TODO(jdufault): Pull these values in from policy.
On 2016/05/13 23:34:56, achuithb wrote:
> Reference a bug

Done.

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_authentication.cc:34: }
On 2016/05/16 16:33:34, stevenjb wrote:
> These both look like they should be PinStorage member functions.

Done.

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/quick_unlock/pin_authentication.h (right):

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_authentication.h:15: bool
IsPinAvailable(PinStorage* storage);
On 2016/05/16 16:33:34, stevenjb wrote:
> On 2016/05/13 23:34:56, achuithb wrote:
> > These should be in a new namespace nested in chromeos, I believe.
> > 
> > Why are these not just methods of PinStorage?
> 
> +1

I wanted to keep them separate because conceptually storing and authenticating
against the pin are separate concepts, but I suppose the storage code already
deals with generating the salt/etc, so I've merged the two files.

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/quick_unlock/pin_storage.cc (right):

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:27: ~PinStorageImpl()
override;
On 2016/05/13 23:34:56, achuithb wrote:
> Can this be protected?

Done.

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:29: // PinStorage
implementation:
On 2016/05/13 23:34:56, achuithb wrote:
> Everything below here can be private, right?

Done.

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:35: int
UnlockAttemptCount() override;
On 2016/05/13 23:34:56, achuithb wrote:
> const

Done.

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:58: base::Time
last_strong_auth_;
On 2016/05/13 23:34:57, achuithb wrote:
> should this be initialized to something?

It's default constructed to the "null" time (IsPinAvailable needs to check for
this - done in the next patch).

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:85: attempt_count_(0)
{}
On 2016/05/13 23:34:56, achuithb wrote:
> move to class

Done.

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:114: void
PinStorageImpl::SetPin(const std::string& raw_pin) {
On 2016/05/13 23:34:56, achuithb wrote:
> This method is probably the most significant bit of code in this CL. Is there
a
> way to unittest this?

I haven't been able to figure out any tests that would add value.

The best I can think of is making the salt/secret generation done via a
delegate, and then overloading that delegate in a test, and then verifying the
set prefs are equal to the values returned by the delegate.

Note that there is a test verifying that the pref service receives some values.

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:115: // Compute salt.
base64 encode it so it is a UTF8 string, which is required by
On 2016/05/13 23:34:57, achuithb wrote:
> Generate random salt.

Done.

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:125: std::string
secret = key.GetSecret();
On 2016/05/13 23:34:57, achuithb wrote:
> const

Done.

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:138: std::string
PinStorageImpl::pin_salt() const {
On 2016/05/13 23:34:56, achuithb wrote:
> PinSalt?

Done.

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:142: std::string
PinStorageImpl::pin_secret() const {
On 2016/05/13 23:34:57, achuithb wrote:
> PinSecret?

Done.

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:156:
user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
On 2016/05/13 23:34:57, achuithb wrote:
> Do we want this to be syncable?

Security is fine with it, but we're still not sure which way we want to go. I
prefer syncable so that's what I went with for now, but it may change. Either
way, it should be easy to swap out, since both the syncable and non-syncable
approaches use the pref system.

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/quick_unlock/pin_storage.h (right):

https://codereview.chromium.org/1977923002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.h:26: static PinStorage*
GetInstance(Profile* profile);
On 2016/05/16 16:33:34, stevenjb wrote:
> We should use a BrowserContextKeyedServiceFactory for this. E.g. see
> CaptivePortalService and CaptivePortalServiceFactory.

Ah, the factory separation cleans things up quite a bit. Thanks.

[stevenjb, 2016-05-16 22:19:08]

https://codereview.chromium.org/1977923002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/quick_unlock/pin_storage.cc (right):

https://codereview.chromium.org/1977923002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:71: }
This could be attempt_count() inlined in the header (or unloc_attemt_count() and
the member renamed).

https://codereview.chromium.org/1977923002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/quick_unlock/pin_storage.h (right):

https://codereview.chromium.org/1977923002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.h:47: bool
HasStrongAuth() const;
Each public method should have its own comment, describing any args or return
value. Exceptions are trivial getters/setters, e.g. foo() or set_foo(). (The
member should be sufficiently commented instead).

[achuithb, 2016-05-16 23:05:10]

looks good overall

https://codereview.chromium.org/1977923002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/quick_unlock/pin_storage.cc (right):

https://codereview.chromium.org/1977923002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:21: std::string
ComputeSecret(std::string pin, std::string salt) {
Input params should be const std::string&. You might as well pass in
std::string* secret as an out param and have the return type be void.

https://codereview.chromium.org/1977923002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:81:
crypto::RandBytes(base::WriteInto(&salt, kSaltByteSize + 1), kSaltByteSize);
nit: Also make salt generation an anonymous function?

https://codereview.chromium.org/1977923002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:116: return secret ==
PinSecret();
Inline ComputeSecret? (assuming you don't use the outparam)

https://codereview.chromium.org/1977923002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc (right):

https://codereview.chromium.org/1977923002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:130:
pin_storage->last_strong_auth_ -=
Let's use a setter for testing here

[jdufault, 2016-05-17 19:58:26]

https://codereview.chromium.org/1977923002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/quick_unlock/pin_storage.cc (right):

https://codereview.chromium.org/1977923002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:21: std::string
ComputeSecret(std::string pin, std::string salt) {
On 2016/05/16 23:05:10, achuithb wrote:
> Input params should be const std::string&. You might as well pass in
> std::string* secret as an out param and have the return type be void.

Converted input params to references.

Is there a good reason to use an out param instead? I find using the return
value to be more readable at the call-site.

https://codereview.chromium.org/1977923002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:71: }
On 2016/05/16 22:19:08, stevenjb wrote:
> This could be attempt_count() inlined in the header (or unloc_attemt_count()
and
> the member renamed).

Done.

https://codereview.chromium.org/1977923002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:81:
crypto::RandBytes(base::WriteInto(&salt, kSaltByteSize + 1), kSaltByteSize);
On 2016/05/16 23:05:10, achuithb wrote:
> nit: Also make salt generation an anonymous function?

Done.

https://codereview.chromium.org/1977923002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:116: return secret ==
PinSecret();
On 2016/05/16 23:05:10, achuithb wrote:
> Inline ComputeSecret? (assuming you don't use the outparam)

Done. I initially did this, but thought it was a bit harder to read.

https://codereview.chromium.org/1977923002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/quick_unlock/pin_storage.h (right):

https://codereview.chromium.org/1977923002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.h:47: bool
HasStrongAuth() const;
On 2016/05/16 22:19:08, stevenjb wrote:
> Each public method should have its own comment, describing any args or return
> value. Exceptions are trivial getters/setters, e.g. foo() or set_foo(). (The
> member should be sufficiently commented instead).
> 

Done.

https://codereview.chromium.org/1977923002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc (right):

https://codereview.chromium.org/1977923002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:130:
pin_storage->last_strong_auth_ -=
On 2016/05/16 23:05:10, achuithb wrote:
> Let's use a setter for testing here

Replaced with a custom test API.

[achuithb, 2016-05-17 20:13:44]

https://codereview.chromium.org/1977923002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/quick_unlock/pin_storage.cc (right):

https://codereview.chromium.org/1977923002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:21: std::string
ComputeSecret(std::string pin, std::string salt) {
On 2016/05/17 19:58:26, jdufault wrote:
> On 2016/05/16 23:05:10, achuithb wrote:
> > Input params should be const std::string&. You might as well pass in
> > std::string* secret as an out param and have the return type be void.
> 
> Converted input params to references.
> 
> Is there a good reason to use an out param instead? I find using the return
> value to be more readable at the call-site.

It saves a copy, and it's generally preferred for non built-in types. These are
not compelling reasons if the return value works better at call-sites, which it
does for the test api class anyway

https://codereview.chromium.org/1977923002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc (right):

https://codereview.chromium.org/1977923002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:34: void
ReduceRemainingStrongAuthTimeBy(base::TimeDelta time_delta) {
const base::TimeDelta&

https://codereview.chromium.org/1977923002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:44:
DISALLOW_COPY_AND_ASSIGN(PinStorageTestApi);
newline before this line

[achuithb, 2016-05-17 20:14:06]

lgtm from me assuming you take care of the nits in the previous comment

[jdufault, 2016-05-20 19:29:44]

https://codereview.chromium.org/1977923002/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc (right):

https://codereview.chromium.org/1977923002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:34: void
ReduceRemainingStrongAuthTimeBy(base::TimeDelta time_delta) {
On 2016/05/17 20:13:44, achuithb wrote:
> const base::TimeDelta&

Done.

https://codereview.chromium.org/1977923002/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:44:
DISALLOW_COPY_AND_ASSIGN(PinStorageTestApi);
On 2016/05/17 20:13:44, achuithb wrote:
> newline before this line

Done.

[jdufault, 2016-05-20 19:31:21]

jdufault@chromium.org changed reviewers:
+ battre@chromium.org, bauerb@chromium.org, gab@chromium.org

[jdufault, 2016-05-20 19:31:58]

+gab@, bauerb@, or battre@ for chrome/browser/prefs/browser_prefs.cc.

[gab, 2016-05-20 19:54:55]

On 2016/05/20 19:31:58, jdufault wrote:
> +gab@, bauerb@, or battre@ for chrome/browser/prefs/browser_prefs.cc.

Please do not mass email OWNERS, pick one, if urgent ping, if no response
*switch* but mass emailing owners is not respectful of their time (and honestly
will probably generally get you a slower response per lack of specificity).

That being said, I'll take this one (and remove others).

[gab, 2016-05-20 19:55:05]

gab@chromium.org changed reviewers:
- battre@chromium.org, bauerb@chromium.org

[gab, 2016-05-20 19:56:07]

c/b/prefs lgtm

[jdufault, 2016-05-20 19:59:49]

The CQ bit was checked by jdufault@chromium.org

[jdufault, 2016-05-20 19:59:49]

The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/1977923002/#ps90001
(title: "Rebase")

[jdufault, 2016-05-20 20:00:07]

On 2016/05/20 19:54:55, gab wrote:
> On 2016/05/20 19:31:58, jdufault wrote:
> > +gab@, bauerb@, or battre@ for chrome/browser/prefs/browser_prefs.cc.
> 
> Please do not mass email OWNERS, pick one, if urgent ping, if no response
> *switch* but mass emailing owners is not respectful of their time (and
honestly
> will probably generally get you a slower response per lack of specificity).
> 
> That being said, I'll take this one (and remove others).

Will do. Thanks

[commit-bot: I haz the power, 2016-05-20 20:00:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1977923002/90001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1977923002/90001

[commit-bot: I haz the power, 2016-05-20 20:34:41]

Description was changed from

==========
Implement pin storage backend.

This provides the backend C++ api for managing and authenticating different
PINs. It will be used by the lock screen and by the settings.

BUG=603217
==========

to

==========
Implement pin storage backend.

This provides the backend C++ api for managing and authenticating different
PINs. It will be used by the lock screen and by the settings.

BUG=603217
==========

[commit-bot: I haz the power, 2016-05-20 20:34:42]

Committed patchset #5 (id:90001)

[commit-bot: I haz the power, 2016-05-20 20:36:24]

Description was changed from

==========
Implement pin storage backend.

This provides the backend C++ api for managing and authenticating different
PINs. It will be used by the lock screen and by the settings.

BUG=603217
==========

to

==========
Implement pin storage backend.

This provides the backend C++ api for managing and authenticating different
PINs. It will be used by the lock screen and by the settings.

BUG=603217

Committed: https://crrev.com/489f430f0a9a61984f4f114bbed9bd6ce1e14c77
Cr-Commit-Position: refs/heads/master@{#395156}
==========

[commit-bot: I haz the power, 2016-05-20 20:36:25]

Patchset 5 (id:??) landed as
https://crrev.com/489f430f0a9a61984f4f114bbed9bd6ce1e14c77
Cr-Commit-Position: refs/heads/master@{#395156}