cros: Added a new function to quick unlock api for checking unfinished pins.

chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.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/profiles/profile_helper.h"
+#include "chrome/common/pref_names.h"
 #include "chromeos/login/auth/extended_authenticator.h"
 #include "chromeos/login/auth/user_context.h"
+#include "components/prefs/pref_service.h"
 #include "extensions/browser/event_router.h"
 
 namespace extensions {
 
 namespace quick_unlock_private = api::quick_unlock_private;
 namespace SetModes = quick_unlock_private::SetModes;
 namespace GetActiveModes = quick_unlock_private::GetActiveModes;
+namespace CheckCredential = quick_unlock_private::CheckCredential;
 namespace GetAvailableModes = quick_unlock_private::GetAvailableModes;
 namespace OnActiveModesChanged = quick_unlock_private::OnActiveModesChanged;
+using CredentialProblem = quick_unlock_private::CredentialProblem;
+using CredentialCheck = quick_unlock_private::CredentialCheck;
 using QuickUnlockMode = quick_unlock_private::QuickUnlockMode;
 using QuickUnlockModeList = std::vector<QuickUnlockMode>;
 
 namespace {
 
 const char kModesAndCredentialsLengthMismatch[] =
     "|modes| and |credentials| must have the same number of elements";
 const char kMultipleModesNotSupported[] =
     "At most one quick unlock mode can be active.";
+// Pins greater in length than |kPinCheckWeakThreshold| will be checked for
+// weakness.
+const int kPinCheckWeakThreshold = 2;
 
 // Returns the active set of quick unlock modes.
 QuickUnlockModeList ComputeActiveModes(Profile* profile) {
   QuickUnlockModeList modes;
 
   chromeos::PinStorage* pin_storage =
       chromeos::PinStorageFactory::GetForProfile(profile);
   if (pin_storage && pin_storage->IsPinSet())
     modes.push_back(quick_unlock_private::QUICK_UNLOCK_MODE_PIN);
 
   return modes;
 }
 
 // Returns true if |a| and |b| contain the same elements. The elements do not
 // need to be in the same order.
 bool AreModesEqual(const QuickUnlockModeList& a, const QuickUnlockModeList& b) {
   if (a.size() != b.size())
     return false;
 
   // This is a slow comparison algorithm, but the number of entries in |a| and
   // |b| will always be very low (0-3 items) so it doesn't matter.
   for (size_t i = 0; i < a.size(); ++i) {
     if (std::find(b.begin(), b.end(), a[i]) == b.end())
       return false;
   }
 
   return true;
 }
 
+// Returns a list of problems (if any) for a given |pin|.

[jdufault, 2016/11/01 18:56:44]

Update name/comment to match impl.

[sammiequon, 2016/11/02 18:05:25]

Done.

+std::vector<CredentialProblem> IsPinValid(const std::string& pin) {
+  std::vector<CredentialProblem> problems;
+  if (!std::all_of(pin.begin(), pin.end(), ::isdigit))
+    problems.push_back(CredentialProblem::CREDENTIAL_PROBLEM_CONTAINS_NONDIGIT);
+  return problems;
+}
+
+// Returns a list of problems (if any) for a given |pin| given the policies in
+// the |profile|. If |out_min_length| and/or |out_max_length| are valid
+// pointers, return the profile min/max length as well.

[jdufault, 2016/11/01 18:56:43]

Update the comments for all of these methods to describe what type of problem
the method returns. Make the comment less generic.

[sammiequon, 2016/11/02 18:05:24]

Done.

+std::vector<CredentialProblem> IsPinLengthValid(const std::string& pin,
+                                                Profile* profile,
+                                                int* out_min_length,
+                                                int* out_max_length) {
+  PrefService* pref_service = profile->GetPrefs();
+  int min_length = pref_service->GetInteger(prefs::kPinUnlockMinimumLength);
+  int max_length = pref_service->GetInteger(prefs::kPinUnlockMaximumLength);
+  // If the maximum length is nonzero and shorter than the minimum length, the
+  // maximum length is the minimum length.
+  if (max_length != 0 && max_length < min_length)
+    max_length = min_length;
+
+  if (out_min_length)
+    *out_min_length = min_length;
+  if (out_max_length)
+    *out_max_length = max_length;
+
+  std::vector<CredentialProblem> problems;
+  // Check if the pin is shorter than the minimum specified length.

[jdufault, 2016/11/01 18:56:44]

nit: newline above

[sammiequon, 2016/11/02 18:05:25]

Done.

+  if (int{pin.size()} < min_length)
+    problems.push_back(CredentialProblem::CREDENTIAL_PROBLEM_TOO_SHORT);
+
+  // If the maximum specified length is zero, there is no maximum length.
+  // Otherwise check if the pin is longer than the maximum specified length.
+  if (max_length != 0 && int{pin.size()} > max_length)
+    problems.push_back(CredentialProblem::CREDENTIAL_PROBLEM_TOO_LONG);
+
+  return problems;
+}
+
+// Returns a list of problems (if any) for a given |pin| given the policies in
+// the |profile|.
+std::vector<CredentialProblem> IsPinDifficultEnough(const std::string& pin) {
+  std::vector<CredentialProblem> problems;
+
+  // If the pin length is |kPinCheckWeakThreshold| or less, there is no need to
+  // check for same character and increasing pin.
+  if (int{pin.size()} <= kPinCheckWeakThreshold)
+    return problems;
+
+  // Check for same digits, increasing pin simutaneously.
+  bool is_same = true;

[jdufault, 2016/11/01 18:56:43]

We should be warning for the top-10 most common PINs.

[sammiequon, 2016/11/02 18:05:24]

I added a TODO. I don't think adding this would cause much delays but just to be
safe.

+  bool is_increasing = true;
+  bool is_decreasing = true;
+  for (int i = 1; i < int{pin.length()}; ++i) {
+    const char previous = pin[i - 1];
+    const char current = pin[i];
+
+    is_same = is_same && (current == previous);
+    is_increasing = is_increasing && (current == previous + 1);
+    is_decreasing = is_decreasing && (current == previous - 1);
+  }
+
+  // Pin is considered weak if either of these is met.
+  if (is_same || is_increasing || is_decreasing)
+    problems.push_back(CredentialProblem::CREDENTIAL_PROBLEM_TOO_WEAK);
+
+  return problems;
+}
+
+void AppendToProblemList(std::vector<CredentialProblem>& list_to_append_to,
+                         const std::vector<CredentialProblem>& list_to_append) {
+  list_to_append_to.insert(list_to_append_to.end(), list_to_append.begin(),
+                           list_to_append.end());
+}
+
 }  // namespace
 
 // quickUnlockPrivate.getAvailableModes
 
 QuickUnlockPrivateGetAvailableModesFunction::
     QuickUnlockPrivateGetAvailableModesFunction()
     : chrome_details_(this) {}
 
 QuickUnlockPrivateGetAvailableModesFunction::
     ~QuickUnlockPrivateGetAvailableModesFunction() {}
@@ skipping 17 matching lines @@
 QuickUnlockPrivateGetActiveModesFunction::
     ~QuickUnlockPrivateGetActiveModesFunction() {}
 
 ExtensionFunction::ResponseAction
 QuickUnlockPrivateGetActiveModesFunction::Run() {
   const QuickUnlockModeList modes =
       ComputeActiveModes(chrome_details_.GetProfile());
   return RespondNow(ArgumentList(GetActiveModes::Results::Create(modes)));
 }
 
+// quickUnlockPrivate.checkCredential
+
+QuickUnlockPrivateCheckCredentialFunction::
+    QuickUnlockPrivateCheckCredentialFunction()
+    : chrome_details_(this) {}
+
+QuickUnlockPrivateCheckCredentialFunction::
+    ~QuickUnlockPrivateCheckCredentialFunction() {}
+
+ExtensionFunction::ResponseAction
+QuickUnlockPrivateCheckCredentialFunction::Run() {
+  params_ = CheckCredential::Params::Create(*args_);
+  EXTENSION_FUNCTION_VALIDATE(params_.get());
+
+  auto result = base::MakeUnique<CredentialCheck>();
+  result->min_length = base::MakeUnique<int>();
+  result->max_length = base::MakeUnique<int>();
+
+  // Only handles pins for now.
+  if (params_->mode != QuickUnlockMode::QUICK_UNLOCK_MODE_PIN) {
+    return RespondNow(ArgumentList(CheckCredential::Results::Create(*result)));
+  }
+
+  std::string credential = params_->credential;
+
+  Profile* profile = chrome_details_.GetProfile();
+  PrefService* pref_service = profile->GetPrefs();
+  bool allow_weak = pref_service->GetBoolean(prefs::kPinUnlockAllowWeakPins);
+
+  // Check and return the problems. |result->min_length| and
+  // |result->max_length| also get the minimum length and maximum length from
+  // policy here.
+  std::vector<CredentialProblem> validity_problems = IsPinValid(credential);
+  std::vector<CredentialProblem> length_problems = IsPinLengthValid(

[jdufault, 2016/11/01 18:56:43]

// Note: This function call writes values to |result->min_length| and
|result->max_length|.

[sammiequon, 2016/11/02 18:05:25]

Done.

+      credential, profile, result->min_length.get(), result->max_length.get());
+  std::vector<CredentialProblem> weak_problems =
+      IsPinDifficultEnough(credential);
+
+  // Append the results from various checks to the appropriate list in |result|.
+  // |validity_problems| and |length_problems| are always errors, while
+  // |weak_problems| are errors or warnings depending on the policy.
+  AppendToProblemList(result->errors, validity_problems);

[jdufault, 2016/11/01 18:56:43]

What about dropping the temporaries and inlining the IsPin* calls directly?

It might make the code harder to read, so please use judgement to decide if the
change is worthwhile.

[sammiequon, 2016/11/02 18:05:24]

I think this make it harder to read because of writing min/max length.

+  AppendToProblemList(result->errors, length_problems);
+  AppendToProblemList(allow_weak ? result->warnings : result->errors,
+                      weak_problems);
+
+  return RespondNow(ArgumentList(CheckCredential::Results::Create(*result)));
+}
+
 // quickUnlockPrivate.setModes
 
 QuickUnlockPrivateSetModesFunction::QuickUnlockPrivateSetModesFunction()
     : chrome_details_(this) {}
 
 QuickUnlockPrivateSetModesFunction::~QuickUnlockPrivateSetModesFunction() {}
 
 void QuickUnlockPrivateSetModesFunction::SetAuthenticatorAllocatorForTesting(
     const QuickUnlockPrivateSetModesFunction::AuthenticatorAllocator&
         allocator) {
@@ skipping 10 matching lines @@
   EXTENSION_FUNCTION_VALIDATE(params_.get());
 
   if (params_->modes.size() != params_->credentials.size())
     return RespondNow(Error(kModesAndCredentialsLengthMismatch));
 
   if (params_->modes.size() > 1)
     return RespondNow(Error(kMultipleModesNotSupported));
 
   // Verify every credential is numeric.
   for (const std::string& credential : params_->credentials) {
-    if (!std::all_of(credential.begin(), credential.end(), ::isdigit))
+    if (!IsPinValid(credential).empty())

[jdufault, 2016/11/01 18:56:43]

This should be in the loop below and happening on PIN only.

Also, if the method were here the name is very confusing - why are you calling
IsPinValid on credentials that are not related to PIN?

[sammiequon, 2016/11/02 18:05:25]

Done.

       return RespondNow(ArgumentList(SetModes::Results::Create(false)));
   }
 
+  // Verify every credential is valid based on policies.
+  bool allow_weak = chrome_details_.GetProfile()->GetPrefs()->GetBoolean(
+      prefs::kPinUnlockAllowWeakPins);
+  for (size_t j = 0; j < params_->modes.size(); ++j) {
+    if (params_->credentials[j].empty())
+      continue;
+
+    if (params_->modes[j] != QuickUnlockMode::QUICK_UNLOCK_MODE_PIN)

[jdufault, 2016/11/01 18:56:43]

I would invert the if (not do a continue), ie,

  if (...) {
    // ...
  }

since this will be a logical place to add checks for other quick unlock modes.

[sammiequon, 2016/11/02 18:05:24]

Done.

+      continue;
+
+    // Check if there any errors with the length of the pin.

[jdufault, 2016/11/01 18:56:43]

Drop comment;

Generally, comments should call out something non-obvious, explain why, or
provide insight into what a large block of code does.

[sammiequon, 2016/11/02 18:05:24]

Done.

+    if (!IsPinLengthValid(params_->credentials[j], chrome_details_.GetProfile(),
+                          nullptr, nullptr)
+             .empty()) {
+      return RespondNow(ArgumentList(SetModes::Results::Create(false)));
+    }
+    // Check if there any errors with the difficulty of the pin.

[jdufault, 2016/11/01 18:56:43]

Drop comment

[sammiequon, 2016/11/02 18:05:24]

Done.

+    if (!allow_weak && !IsPinDifficultEnough(params_->credentials[j]).empty())
+      return RespondNow(ArgumentList(SetModes::Results::Create(false)));
+  }
+
   user_manager::User* user = chromeos::ProfileHelper::Get()->GetUserByProfile(
       chrome_details_.GetProfile());
   chromeos::UserContext user_context(user->GetAccountId());
   user_context.SetKey(chromeos::Key(params_->account_password));
 
   // Lazily allocate the authenticator. We do this here, instead of in the ctor,
   // so that tests can install a fake.
   if (authenticator_allocator_.is_null())
     extended_authenticator_ = chromeos::ExtendedAuthenticator::Create(this);
   else
@@ skipping 83 matching lines @@
   }
 
   std::unique_ptr<base::ListValue> args = OnActiveModesChanged::Create(modes);
   std::unique_ptr<Event> event(
       new Event(events::QUICK_UNLOCK_PRIVATE_ON_ACTIVE_MODES_CHANGED,
                 OnActiveModesChanged::kEventName, std::move(args)));
   EventRouter::Get(browser_context())->BroadcastEvent(std::move(event));
 }
 
 }  // namespace extensions