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| 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.
+std::vector<CredentialProblem> IsPinLengthValid(const std::string& pin,
+                                                Profile* profile,
+                                                int* out_min_length,
+                                                int* out_max_length) {
+  std::vector<CredentialProblem> problems;
+  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)

[jdufault, 2016/10/31 22:03:07]

Should this be <= 0?

What behavior do we want for, say, max_length = -1?

[sammiequon, 2016/11/01 18:08:31]

Would people be allowed to enter negative numbers, for max_length we could make
negatives do the same thing as zero. What about min_length, should we cap the
minimum at 2?

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

Policy UI hopefully will sanitize input, but we should still program defensively
in this case because the input data may not come from the policy server.

I'd cap min_length at >= 1 (keep existing behavior, but handle negative
min_lengths) and treat a negative max_length as unspecified max_length. We can
consider negative max_length and negative min_length as unspecified behavior.

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

Done.

+    max_length = min_length;
+
+  if (out_min_length)
+    *out_min_length = min_length;
+  if (out_max_length)
+    *out_max_length = max_length;
+

[jdufault, 2016/10/31 22:03:07]

declare problems here

[sammiequon, 2016/11/01 18:08:31]

Done.

+  // Check if the pin is shorter than the minimum specified length.
+  if (int{pin.size()} < min_length) {

[jdufault, 2016/10/31 22:03:07]

nit: drop {}

[sammiequon, 2016/11/01 18:08:31]

Done.

+    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) {

[jdufault, 2016/10/31 22:03:07]

nit: drop {}

[sammiequon, 2016/11/01 18:08:31]

Done.

+    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) {

[jdufault, 2016/10/31 22:03:07]

Maybe return base::Optional<CredentialProblem> instead? Up to you.

[sammiequon, 2016/11/01 18:08:31]

Would this be an base::Optional<vector<CredentialProblem>>? In case we want to
add extra cases later?

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

base::Optional<std::vector<CredentialProblem>> would be a strange type, since
the empty optional could be represented just as well with an empty vector. At
least in this case, there is no semantic different between an empty problem
vector and an empty option.

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

I see, I decided to stick with the vector implementation because I want to keep
them consistent, even if one of these only ever returns one or no values.

+  std::vector<CredentialProblem> problems;
+  // If the pin length is |kPinCheckWeakThreshold| or less, there is no need to

[jdufault, 2016/10/31 22:03:07]

nit: newline above

[sammiequon, 2016/11/01 18:08:31]

Done.

+  // 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;
+  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)) {

[jdufault, 2016/10/31 22:03:07]

drop extraneous ()

[sammiequon, 2016/11/01 18:08:31]

Done.

+    problems.push_back(CredentialProblem::CREDENTIAL_PROBLEM_TOO_WEAK);

[jdufault, 2016/10/31 22:03:07]

drop {}

[sammiequon, 2016/11/01 18:08:31]

Done.

+  }
+
+  return problems;
+}
+
 }  // 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 tried_password = params_->credential;

[jdufault, 2016/10/31 22:03:07]

What about reusing the name credential instead of tried_password?

You should stay consistent with the existing terminology though (so
tried_credential instead of tried_password).

[sammiequon, 2016/11/01 18:08:31]

Done.

+  if (!std::all_of(tried_password.begin(), tried_password.end(), ::isdigit))
+    return RespondNow(ArgumentList(CheckCredential::Results::Create(*result)));

[jdufault, 2016/10/31 22:03:07]

This should probably report an error that the input contains non-digit
characters?

[sammiequon, 2016/11/01 18:08:31]

Done.

+
+  Profile* profile = chrome_details_.GetProfile();
+  PrefService* pref_service = profile->GetPrefs();
+  bool allow_weak = pref_service->GetBoolean(prefs::kPinUnlockAllowWeakPins);
+  auto length_problems =

[jdufault, 2016/10/31 22:03:07]

Only use auto when the type of the variable is evident from the expression
defining it.

(Remove auto)

[sammiequon, 2016/11/01 18:08:31]

Done.

+      IsPinLengthValid(tried_password, profile, result->min_length.get(),

[jdufault, 2016/10/31 22:03:07]

It's not clear that min_length and max_length are getting written to here. You
should add a comment or find another way to write the code so that is expressed.

[sammiequon, 2016/11/01 18:08:31]

Done.

+                       result->max_length.get());
+  auto weak_problems = IsPinDifficultEnough(tried_password);

[jdufault, 2016/10/31 22:03:07]

Specify type instead of auto

[sammiequon, 2016/11/01 18:08:31]

Done.

+  result->errors.insert(result->errors.end(), length_problems.begin(),
+                        length_problems.end());
+  if (allow_weak) {
+    result->warnings.insert(result->warnings.end(), weak_problems.begin(),

[jdufault, 2016/10/31 22:03:07]

Maybe you want to add a helper method?

  AppendToList(result->errors, length_problems);
  AppendToList(allow_weak ? result->warnings : result->errors, weak_problems)

[sammiequon, 2016/11/01 18:08:31]

Done.

+                            weak_problems.end());
+  } else {
+    result->errors.insert(result->errors.end(), weak_problems.begin(),
+                          weak_problems.end());
+  }
+
+  return RespondNow(ArgumentList(CheckCredential::Results::Create(*result)));
+}
+
 // quickUnlockPrivate.setModes
 
 QuickUnlockPrivateSetModesFunction::QuickUnlockPrivateSetModesFunction()
     : chrome_details_(this) {}
 
 QuickUnlockPrivateSetModesFunction::~QuickUnlockPrivateSetModesFunction() {}
 
 void QuickUnlockPrivateSetModesFunction::SetAuthenticatorAllocatorForTesting(
     const QuickUnlockPrivateSetModesFunction::AuthenticatorAllocator&
         allocator) {
   authenticator_allocator_ = allocator;
 }
 
 void QuickUnlockPrivateSetModesFunction::SetModesChangedEventHandlerForTesting(
     const ModesChangedEventHandler& handler) {
   modes_changed_handler_ = handler;
 }
 
 ExtensionFunction::ResponseAction QuickUnlockPrivateSetModesFunction::Run() {
   params_ = SetModes::Params::Create(*args_);
   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) {

[jdufault, 2016/10/31 22:03:07]

If this is made a problem the check can be merged with the one below.

[sammiequon, 2016/11/01 18:08:31]

Done.

     if (!std::all_of(credential.begin(), credential.end(), ::isdigit))
       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_->modes[j] == QuickUnlockMode::QUICK_UNLOCK_MODE_PIN &&
+        (!IsPinLengthValid(params_->credentials[j],

[jdufault, 2016/10/31 22:03:08]

This if statement is pretty challenging to read. You should

1. Use some temporaries
2. Use multiple ifs

[sammiequon, 2016/11/01 18:08:31]

Done.

+                           chrome_details_.GetProfile(), nullptr, nullptr)
+              .empty() ||
+         (!IsPinDifficultEnough(params_->credentials[j]).empty() &&
+          !allow_weak))) {
+      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