Enable the PIN keyboard on the lockscreen.

chrome/browser/chromeos/login/lock/screen_locker.cc

Index: chrome/browser/chromeos/login/lock/screen_locker.cc
diff --git a/chrome/browser/chromeos/login/lock/screen_locker.cc b/chrome/browser/chromeos/login/lock/screen_locker.cc
index 866c04530c4a2b246ad914dfbca5317223a419af..09aee07075e9bdf2906d7da8ad32bb198070891c 100644
--- a/chrome/browser/chromeos/login/lock/screen_locker.cc
+++ b/chrome/browser/chromeos/login/lock/screen_locker.cc
@@ -27,6 +27,8 @@
 #include "base/strings/string_util.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/login/lock/webui_screen_locker.h"
+#include "chrome/browser/chromeos/login/quick_unlock/pin_storage.h"
+#include "chrome/browser/chromeos/login/quick_unlock/pin_storage_factory.h"
 #include "chrome/browser/chromeos/login/session/user_session_manager.h"
 #include "chrome/browser/chromeos/login/supervised/supervised_user_authentication.h"
 #include "chrome/browser/chromeos/login/ui/user_adding_screen.h"
@@ -119,6 +121,13 @@ class ScreenLockObserver : public SessionManagerClient::StubDelegate,
   DISALLOW_COPY_AND_ASSIGN(ScreenLockObserver);
 };
 
+bool IsLikelyPIN(const std::string& input) {
+  // Check if |input| looks like a number; the actual numeric value doesn't
+  // matter.
+  int num;
+  return base::StringToInt(input, &num);
+}
+
 ScreenLockObserver* g_screen_lock_observer = NULL;
 
 }  // namespace
@@ -215,6 +224,21 @@ void ScreenLocker::OnAuthSuccess(const UserContext& user_context) {
       user_manager::UserManager::Get()->SwitchActiveUser(
           user_context.GetAccountId());
     }
+
+    // Reset the number of PIN attempts available to the user. We always do this
+    // because:
+    // 1. If the user signed in with a PIN, that means they should be able to
+    //    continue signing in with a PIN.
+    // 2. If the user signed in with cryptohome keys, then the PIN timeout is
+    //    going to be reset as well, so it is safe to reset the unlock attempt
+    //    count.
+    PinStorage* pin_storage = PinStorageFactory::GetForUser(user);
+    if (pin_storage) {
+      pin_storage->ResetUnlockAttemptCount();
+    } else {
+      NOTREACHED() << "PinStorage not available for user";
+    }
+
     UserSessionManager::GetInstance()->UpdateEasyUnlockKeys(user_context);
   } else {
     NOTREACHED() << "Logged in user not found.";
@@ -233,6 +257,18 @@ void ScreenLocker::OnAuthSuccess(const UserContext& user_context) {
   delegate_->AnimateAuthenticationSuccess();
 }
 
+void ScreenLocker::OnPasswordAuthSuccess(const UserContext& user_context) {
+  // The user has signed in using their password, so reset the PIN timeout.
+  PinStorage* pin_storage =
+      PinStorageFactory::GetForAccountId(user_context.GetAccountId());
+
+  if (pin_storage) {
+    pin_storage->MarkStrongAuth();
+  } else {
+    NOTREACHED() << "Missing PinStorage for account";
+  }
+}
+
 void ScreenLocker::UnlockOnLoginSuccess() {
   DCHECK(base::MessageLoopForUI::IsCurrent());
   if (!authentication_capture_.get()) {
@@ -252,30 +288,40 @@ void ScreenLocker::UnlockOnLoginSuccess() {
 }
 
 void ScreenLocker::Authenticate(const UserContext& user_context) {
-  LOG_ASSERT(IsUserLoggedIn(user_context.GetAccountId().GetUserEmail()))
+  LOG_ASSERT(IsUserLoggedIn(user_context.GetAccountId()))
       << "Invalid user trying to unlock.";
 
   authentication_start_time_ = base::Time::Now();
   delegate_->SetInputEnabled(false);
   delegate_->OnAuthenticate();
 
-  // Special case: supervised users. Use special authenticator.
-  if (const user_manager::User* user =
-          FindUnlockUser(user_context.GetAccountId().GetUserEmail())) {
+  const user_manager::User* user = FindUnlockUser(user_context.GetAccountId());
+  if (user) {
+    // Check to see if the user submitted a PIN and it is valid.
+    std::string pin = user_context.GetKey()->GetSecret();

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

const

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

Done.

+    if (IsLikelyPIN(pin)) {

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

Not sure if this deserves a separate function - it's only 2 lines, right?

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

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

+      chromeos::PinStorage* pin_storage =
+          chromeos::PinStorageFactory::GetForUser(user);
+      if (!pin_storage) {
+        NOTREACHED() << "Missing PinStorage when trying to authenticate PIN";

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

Is this logging important? Why not 
if (pin_storage && pin_storage->TryAuthenticatePin(pin)) { 

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

Done.

+      } else if (pin_storage->TryAuthenticatePin(pin)) {
+        OnAuthSuccess(user_context);
+        return;
+      }
+    }
+
+    // Special case: supervised users. Use special authenticator.
     if (user->GetType() == user_manager::USER_TYPE_SUPERVISED) {
       UserContext updated_context = ChromeUserManager::Get()
                                         ->GetSupervisedUserManager()
                                         ->GetAuthentication()
                                         ->TransformKey(user_context);
-      // TODO(antrim) : replace empty closure with explicit method.
-      // http://crbug.com/351268
       BrowserThread::PostTask(
-          BrowserThread::UI,
-          FROM_HERE,
+          BrowserThread::UI, FROM_HERE,
           base::Bind(&ExtendedAuthenticator::AuthenticateToCheck,
-                     extended_authenticator_.get(),
-                     updated_context,
-                     base::Closure()));
+                     extended_authenticator_.get(), updated_context,
+                     base::Bind(&ScreenLocker::OnPasswordAuthSuccess,
+                                base::Unretained(this), user_context)));

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

Why base::Unretained? Also, should this be updated_context?

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

Done on updated_context.

       return;
     }
   }
@@ -283,23 +329,18 @@ void ScreenLocker::Authenticate(const UserContext& user_context) {
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&ExtendedAuthenticator::AuthenticateToCheck,
-                 extended_authenticator_.get(),
-                 user_context,
-                 base::Closure()));
+                 extended_authenticator_.get(), user_context,
+                 base::Bind(&ScreenLocker::OnPasswordAuthSuccess,
+                            base::Unretained(this), user_context)));

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

base::Unretained?

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

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

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

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]

sg; the current CL is using the weak_ptr approach.

 }
 
 const user_manager::User* ScreenLocker::FindUnlockUser(
-    const std::string& user_id) {
-  const user_manager::User* unlock_user = NULL;
-  for (user_manager::UserList::const_iterator it = users_.begin();
-       it != users_.end();
-       ++it) {
-    if ((*it)->email() == user_id) {
-      unlock_user = *it;
-      break;
-    }
+    const AccountId& account_id) {
+  for (const user_manager::User* user : users_) {
+    if (user->GetAccountId() == account_id)
+      return user;
   }
-  return unlock_user;
+  return nullptr;
 }
 
 void ScreenLocker::ClearErrors() {
@@ -503,11 +544,9 @@ content::WebUI* ScreenLocker::GetAssociatedWebUI() {
   return delegate_->GetAssociatedWebUI();
 }
 
-bool ScreenLocker::IsUserLoggedIn(const std::string& username) {
-  for (user_manager::UserList::const_iterator it = users_.begin();
-       it != users_.end();
-       ++it) {
-    if ((*it)->email() == username)
+bool ScreenLocker::IsUserLoggedIn(const AccountId& account_id) {

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

is it painful to make this function const?

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

Done.

+  for (user_manager::User* user : users_) {
+    if (user->GetAccountId() == account_id)
       return true;
   }
   return false;