Enable the PIN keyboard on the lockscreen.

Enable the PIN keyboard on the lockscreen.

The PIN keyboard will now show up if a user has the PIN keyboard enabled and has authenticated against the cryptohome recently (currently 24 hours).

The PIN keyboard is shown per-user, so this approach will scale to multiple user pods; however, the per-pod UI has not been completed yet.

The PIN keyboard is hidden after there are too many incorrect attempts.

BUG=603217
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/a09c41969d298d918f6f0824f619617ad9587b9e
Cr-Commit-Position: refs/heads/master@{#400058}

[jdufault, 2016-05-27 21:29:59]

Description was changed from

==========
Better lockscreen PIN keyboard integration.

The PIN keyboard will now show up if a user has the PIN keyboard enabled and has
logged in with their password within the past 12 hours.

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

to

==========
Better lockscreen PIN keyboard integration.

The PIN keyboard will now show up if a user has the PIN keyboard enabled and has
logged in with their password within the past 12 hours.

BUG=603217
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[jdufault, 2016-05-27 21:30:48]

Description was changed from

==========
Better lockscreen PIN keyboard integration.

The PIN keyboard will now show up if a user has the PIN keyboard enabled and has
logged in with their password within the past 12 hours.

BUG=603217
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Dynamically display the PIN keyboard on the lockscreen.

The PIN keyboard will now show up if a user has the PIN keyboard enabled and has
logged in with their password within the past 12 hours.

BUG=603217
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[jdufault, 2016-05-31 21:55:25]

Patchset #1 (id:1) has been deleted

[jdufault, 2016-06-01 17:16:55]

Patchset #1 (id:20001) has been deleted

[jdufault, 2016-06-01 17:20:27]

Description was changed from

==========
Dynamically display the PIN keyboard on the lockscreen.

The PIN keyboard will now show up if a user has the PIN keyboard enabled and has
logged in with their password within the past 12 hours.

BUG=603217
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Enable the PIN keyboard on the lockscreen.

The PIN keyboard will now show up if a user has the PIN keyboard enabled and has
authenticated against the cryptohome recently (currently 24 hours).

The PIN keyboard is shown per-user, so this approach will scale to multiple user
pods; however, the per-pod UI has not been completed yet.

The PIN keyboard is hidden after there are too many incorrect attempts.

BUG=603217
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[jdufault, 2016-06-01 17:27:40]

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

[jdufault, 2016-06-01 17:27:54]

Achuith/Pavel, PTAL. Thanks!

[achuithb, 2016-06-02 23:34:45]

Looks good overall

https://codereview.chromium.org/2015413002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/2015413002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:301: std::string pin =
user_context.GetKey()->GetSecret();
const

https://codereview.chromium.org/2015413002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:302: if (IsLikelyPIN(pin)) {
Not sure if this deserves a separate function - it's only 2 lines, right?

https://codereview.chromium.org/2015413002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:306: NOTREACHED() <<
"Missing PinStorage when trying to authenticate PIN";
Is this logging important? Why not 
if (pin_storage && pin_storage->TryAuthenticatePin(pin)) {

https://codereview.chromium.org/2015413002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:324: base::Unretained(this),
user_context)));
Why base::Unretained? Also, should this be updated_context?

https://codereview.chromium.org/2015413002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:334: base::Unretained(this),
user_context)));
base::Unretained?

https://codereview.chromium.org/2015413002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:547: bool
ScreenLocker::IsUserLoggedIn(const AccountId& account_id) {
is it painful to make this function const?

[jdufault, 2016-06-03 23:39:49]

https://codereview.chromium.org/2015413002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/2015413002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:301: std::string pin =
user_context.GetKey()->GetSecret();
On 2016/06/02 23:34:45, achuithb wrote:
> const

Done.

https://codereview.chromium.org/2015413002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:302: if (IsLikelyPIN(pin)) {
On 2016/06/02 23:34:45, achuithb wrote:
> Not sure if this deserves a separate function - it's only 2 lines, right?

Done. Yea, it's iffy. Inlining introduces an extra unused variable.

https://codereview.chromium.org/2015413002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:306: NOTREACHED() <<
"Missing PinStorage when trying to authenticate PIN";
On 2016/06/02 23:34:45, achuithb wrote:
> Is this logging important? Why not 
> if (pin_storage && pin_storage->TryAuthenticatePin(pin)) { 

Done.

https://codereview.chromium.org/2015413002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:324: base::Unretained(this),
user_context)));
On 2016/06/02 23:34:45, achuithb wrote:
> Why base::Unretained? Also, should this be updated_context?

Done on updated_context.

https://codereview.chromium.org/2015413002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:334: base::Unretained(this),
user_context)));
On 2016/06/02 23:34:45, achuithb wrote:
> base::Unretained?

I've converted this to use a weak_ptr, but I believe this is safe because
AuthenticateToCheck already calls back into the object, and the newly added
callback will get invoked in the same call hierarchy.

If OnPasswordAuthSuccess doesn't get called but OnAuthSuccess does on a password
authentication, PIN will be permanently disabled after it times out, so this
feels like an error that should be fixed (base::Unretained) instead of silently
ignored (weak_ptr).

https://codereview.chromium.org/2015413002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:547: bool
ScreenLocker::IsUserLoggedIn(const AccountId& account_id) {
On 2016/06/02 23:34:45, achuithb wrote:
> is it painful to make this function const?

Done.

[achuithb, 2016-06-04 00:32:00]

https://codereview.chromium.org/2015413002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/2015413002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:334: base::Unretained(this),
user_context)));
On 2016/06/03 23:39:49, jdufault wrote:
> On 2016/06/02 23:34:45, achuithb wrote:
> > base::Unretained?
> 
> I've converted this to use a weak_ptr, but I believe this is safe because
> AuthenticateToCheck already calls back into the object, and the newly added
> callback will get invoked in the same call hierarchy.
> 
> If OnPasswordAuthSuccess doesn't get called but OnAuthSuccess does on a
password
> authentication, PIN will be permanently disabled after it times out, so this
> feels like an error that should be fixed (base::Unretained) instead of
silently
> ignored (weak_ptr).

If you feel strongly that base::Unretained is the right construct, you should
add a comment.

In general, I think we try to avoid use of base::Unretained because of the
security risk of accessing freed memory, especially if this code gets
re-factored to behave differently in the future, such as posting on the message
loop, etc.

[jdufault, 2016-06-08 18:40:50]

https://codereview.chromium.org/2015413002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/2015413002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:334: base::Unretained(this),
user_context)));
On 2016/06/04 00:32:00, achuithb wrote:
> On 2016/06/03 23:39:49, jdufault wrote:
> > On 2016/06/02 23:34:45, achuithb wrote:
> > > base::Unretained?
> > 
> > I've converted this to use a weak_ptr, but I believe this is safe because
> > AuthenticateToCheck already calls back into the object, and the newly added
> > callback will get invoked in the same call hierarchy.
> > 
> > If OnPasswordAuthSuccess doesn't get called but OnAuthSuccess does on a
> password
> > authentication, PIN will be permanently disabled after it times out, so this
> > feels like an error that should be fixed (base::Unretained) instead of
> silently
> > ignored (weak_ptr).
> 
> If you feel strongly that base::Unretained is the right construct, you should
> add a comment.
> 
> In general, I think we try to avoid use of base::Unretained because of the
> security risk of accessing freed memory, especially if this code gets
> re-factored to behave differently in the future, such as posting on the
message
> loop, etc.

sg; the current CL is using the weak_ptr approach.

[achuithb, 2016-06-14 17:46:48]

lgtm

[jdufault, 2016-06-14 21:51:01]

The CQ bit was checked by jdufault@chromium.org

[commit-bot: I haz the power, 2016-06-14 21:52:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2015413002/60001

[commit-bot: I haz the power, 2016-06-14 23:53:43]

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

[commit-bot: I haz the power, 2016-06-14 23:53:44]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
  ios-device-gn on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)

[jdufault, 2016-06-15 23:14:06]

The CQ bit was checked by jdufault@chromium.org

[commit-bot: I haz the power, 2016-06-15 23:14:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2015413002/60001

[commit-bot: I haz the power, 2016-06-16 00:19:58]

Description was changed from

==========
Enable the PIN keyboard on the lockscreen.

The PIN keyboard will now show up if a user has the PIN keyboard enabled and has
authenticated against the cryptohome recently (currently 24 hours).

The PIN keyboard is shown per-user, so this approach will scale to multiple user
pods; however, the per-pod UI has not been completed yet.

The PIN keyboard is hidden after there are too many incorrect attempts.

BUG=603217
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Enable the PIN keyboard on the lockscreen.

The PIN keyboard will now show up if a user has the PIN keyboard enabled and has
authenticated against the cryptohome recently (currently 24 hours).

The PIN keyboard is shown per-user, so this approach will scale to multiple user
pods; however, the per-pod UI has not been completed yet.

The PIN keyboard is hidden after there are too many incorrect attempts.

BUG=603217
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-06-16 00:19:59]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-06-16 00:20:16]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-16 00:21:41]

Description was changed from

==========
Enable the PIN keyboard on the lockscreen.

The PIN keyboard will now show up if a user has the PIN keyboard enabled and has
authenticated against the cryptohome recently (currently 24 hours).

The PIN keyboard is shown per-user, so this approach will scale to multiple user
pods; however, the per-pod UI has not been completed yet.

The PIN keyboard is hidden after there are too many incorrect attempts.

BUG=603217
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
==========

to

==========
Enable the PIN keyboard on the lockscreen.

The PIN keyboard will now show up if a user has the PIN keyboard enabled and has
authenticated against the cryptohome recently (currently 24 hours).

The PIN keyboard is shown per-user, so this approach will scale to multiple user
pods; however, the per-pod UI has not been completed yet.

The PIN keyboard is hidden after there are too many incorrect attempts.

BUG=603217
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/a09c41969d298d918f6f0824f619617ad9587b9e
Cr-Commit-Position: refs/heads/master@{#400058}
==========

[commit-bot: I haz the power, 2016-06-16 00:21:43]

Patchset 2 (id:??) landed as
https://crrev.com/a09c41969d298d918f6f0824f619617ad9587b9e
Cr-Commit-Position: refs/heads/master@{#400058}