cros: Implement cryptohome backend for pin.

cros: Implement cryptohome backend for pin.

The actual cryptohome APIs we will use are not ready yet, and some
refactoring may be required when they are ready, but this gets us most
of the way there.

BUG=623344

[jdufault, 2017-04-12 19:00:09]

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

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

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-12 20:46:51]

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

[commit-bot: I haz the power, 2017-04-12 20:46:52]

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

[jdufault, 2017-04-12 22:48:57]

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

[commit-bot: I haz the power, 2017-04-12 22:49:43]

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-12 23:52:46]

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

[commit-bot: I haz the power, 2017-04-12 23:52:48]

Dry run: This issue passed the CQ dry run.

[jdufault, 2017-04-15 00:10:27]

Patchset #1 (id:1) has been deleted

[jdufault, 2017-04-15 00:12:26]

Description was changed from

==========
cros: Implement cryptohome backend for pin.

The actual cryptohome APIs we will use are not ready yet, and some
refactoring may be required when they are ready, but this gets us most
of the way there.

BUG=623344
==========

to

==========
cros: Implement cryptohome backend for pin.

The actual cryptohome APIs we will use are not ready yet, and some
refactoring may be required when they are ready, but this gets us most
of the way there.

BUG=623344
==========

[jdufault, 2017-04-15 00:12:27]

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

[jdufault, 2017-04-15 00:12:55]

achuith@ PTAL. Sorry for the huge CL - I've tried to break it smaller into
chunks where feasible.

[jdufault, 2017-05-11 18:34:10]

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

[commit-bot: I haz the power, 2017-05-11 18:35:34]

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-11 21:54:48]

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

[commit-bot: I haz the power, 2017-05-11 21:54:51]

Dry run: This issue passed the CQ dry run.

[jdufault, 2017-05-12 16:49:27]

achuith@ PTAL when you get the chance.

[Alexander Alekseev, 2017-05-12 22:40:39]

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1304: 
Please, add TODO  https://crbug.com/721938 to optimize this.

[achuithb, 2017-05-13 01:01:58]

Sorry for the delay. I need to take a second pass through this code to make sure
I understand it.

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
File
chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc
(right):

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:440:
return newly_active_modes;
So that can only be empty or have the QUICK_UNLOCK_MODE_PIN?

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
File
chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h
(right):

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h:139:
std::vector<api::quick_unlock_private::QuickUnlockMode> ApplyModeChange(
Is it worthwhile using a typedef for this?

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
File
chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc
(right):

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:334:
bool IsPinSetInBackend() {
nit const

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:335:
AccountId account_id = AccountId::FromUserEmail(kTestUserEmail);
nit const

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:376:
void DoAddKeyCallback(const cryptohome::KeyDefinition& key,
let's add some newlines between methods

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

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_backend.cc:53:
base::WeakPtrFactory<CryptohomeBackend> weak_factory_;
You can do weak_factory_{this};
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/chrome_views_del...

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_backend.cc:68: auto on_key_data =
Let's maybe pull this out into a named callback? I haven't seen this many
lambdas in one CL.

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_backend.cc:100:
cryptohome::Identification id(user_context.GetAccountId());
nit all const

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_backend.cc:112: for
(std::vector<base::Closure>::const_iterator it =
Does range-based loop not work here?

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_backend.cc:130:
cryptohome::Identification id(user_context.GetAccountId());
nit const

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_backend.cc:141: return
g_cryptohome_backend_;
it's ok to leak this? 

Isn't there a singleton class we can use here instead?

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

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_backend.h:45: // Try to
authenticate.
nit newline

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

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage_prefs_unittest.cc:29:
quick_unlock::PinStoragePrefs* PinStorage() const {
Why not make this a protected data member?

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

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/quick_unlock_storage.h:27: using
BoolCallback = base::Callback<void(bool)>;
Where's this used?

[jdufault, 2017-06-05 19:29:18]

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

[commit-bot: I haz the power, 2017-06-05 19:30:04]

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-06-05 23:07:59]

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

[commit-bot: I haz the power, 2017-06-05 23:08:00]

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...)

[jdufault, 2017-06-06 18:17:06]

Sorry about taking so long to address the comments. Thanks for taking a look!

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
File
chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc
(right):

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:440:
return newly_active_modes;
On 2017/05/13 01:01:57, achuithb wrote:
> So that can only be empty or have the QUICK_UNLOCK_MODE_PIN?

Yep.

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
File
chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h
(right):

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h:139:
std::vector<api::quick_unlock_private::QuickUnlockMode> ApplyModeChange(
On 2017/05/13 01:01:57, achuithb wrote:
> Is it worthwhile using a typedef for this?

Done.

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
File
chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc
(right):

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:334:
bool IsPinSetInBackend() {
On 2017/05/13 01:01:58, achuithb wrote:
> nit const

This calls PumpRunLoop() which cannot be constant.

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:335:
AccountId account_id = AccountId::FromUserEmail(kTestUserEmail);
On 2017/05/13 01:01:57, achuithb wrote:
> nit const

Done.

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:376:
void DoAddKeyCallback(const cryptohome::KeyDefinition& key,
On 2017/05/13 01:01:58, achuithb wrote:
> let's add some newlines between methods

Done.

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

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_backend.cc:53:
base::WeakPtrFactory<CryptohomeBackend> weak_factory_;
On 2017/05/13 01:01:58, achuithb wrote:
> You can do weak_factory_{this};
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/chrome_views_del...
> 

Done - that's a nice pattern.

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_backend.cc:68: auto on_key_data =
On 2017/05/13 01:01:58, achuithb wrote:
> Let's maybe pull this out into a named callback? I haven't seen this many
> lambdas in one CL.

Done.

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_backend.cc:100:
cryptohome::Identification id(user_context.GetAccountId());
On 2017/05/13 01:01:58, achuithb wrote:
> nit all const

Done.

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_backend.cc:112: for
(std::vector<base::Closure>::const_iterator it =
On 2017/05/13 01:01:58, achuithb wrote:
> Does range-based loop not work here?

Done - not sure why I wasn't using it.

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_backend.cc:130:
cryptohome::Identification id(user_context.GetAccountId());
On 2017/05/13 01:01:58, achuithb wrote:
> nit const

Done.

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_backend.cc:141: return
g_cryptohome_backend_;
On 2017/05/13 01:01:58, achuithb wrote:
> it's ok to leak this? 
> 
> Isn't there a singleton class we can use here instead?

I tried converting to a leaky base::Singleton instance. For tests, we need the
ability to reset/destroy the singleton which base::Singleton does not appear to
support.

I think leaking is fine.

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

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_backend.h:45: // Try to
authenticate.
On 2017/05/13 01:01:58, achuithb wrote:
> nit newline

Done.

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

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/pin_storage_prefs_unittest.cc:29:
quick_unlock::PinStoragePrefs* PinStorage() const {
On 2017/05/13 01:01:58, achuithb wrote:
> Why not make this a protected data member?

Do you mean caching it? I try to avoid caching state as it can get out of sync.

This is under a protected: access modifier.

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

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/quick_unlock/quick_unlock_storage.h:27: using
BoolCallback = base::Callback<void(bool)>;
On 2017/05/13 01:01:58, achuithb wrote:
> Where's this used?

Removed.

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right):

https://codereview.chromium.org/2809993004/diff/40001/chrome/browser/ui/webui...
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:1304: 
On 2017/05/12 22:40:39, Alexander Alekseev wrote:
> Please, add TODO  https://crbug.com/721938 to optimize this.

Done.