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

Index: chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc
diff --git a/chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc b/chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc
index 50bdfb92b393bb1028d07ee2ed4c58c2058f0e0c..e2f3987b8933b45d73c1453528cb5b689b8037aa 100644
--- a/chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc
+++ b/chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc
@@ -7,8 +7,10 @@
 #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 {
@@ -16,8 +18,12 @@ namespace extensions {
 namespace quick_unlock_private = api::quick_unlock_private;
 namespace SetModes = quick_unlock_private::SetModes;
 namespace GetActiveModes = quick_unlock_private::GetActiveModes;
+namespace IsCredentialUsable = quick_unlock_private::IsCredentialUsable;
 namespace GetAvailableModes = quick_unlock_private::GetAvailableModes;
 namespace OnActiveModesChanged = quick_unlock_private::OnActiveModesChanged;
+using CredentialRequirementFailure =
+    quick_unlock_private::CredentialRequirementFailure;
+using CredentialRequirement = quick_unlock_private::CredentialRequirement;
 using QuickUnlockMode = quick_unlock_private::QuickUnlockMode;
 using QuickUnlockModeList = std::vector<QuickUnlockMode>;
 
@@ -56,6 +62,86 @@ bool AreModesEqual(const QuickUnlockModeList& a, const QuickUnlockModeList& b) {
   return true;
 }
 
+// Check if a given |pin| is valid given the policies in the |profile|. If
+// |result| is a valid pointer, the failures are added to |result|. Pass a
+// nullptr for |result| if all we want to do is check the validity of |pin|.

[jdufault, 2016/10/04 22:57:27]

nit: 'a nullptr' => 'null'

null is preferred inside of comments.

[sammiequon, 2016/10/05 19:48:11]

Done.

+bool IsPinLengthValid(const std::string& pin,
+                      Profile* profile,
+                      CredentialRequirement* result) {
+  bool is_valid = true;
+  PrefService* pref_service = profile->GetPrefs();
+  int minimum_length = pref_service->GetInteger(prefs::kPinUnlockMinimumLength);

[jdufault, 2016/10/04 22:57:27]

Name these min_length, max_length?

[sammiequon, 2016/10/05 19:48:11]

Done.

+  int maximum_length = pref_service->GetInteger(prefs::kPinUnlockMaximumLength);
+  if (result) {
+    result->min_length = base::MakeUnique<int>(minimum_length);
+    result->max_length = base::MakeUnique<int>(maximum_length);
+  }
+
+  // Check if the pin is shorter than the minimum specified length.
+  if (int{pin.size()} < minimum_length) {
+    is_valid = false;
+    if (result) {
+      result->failures.push_back(CredentialRequirementFailure::
+                                     CREDENTIAL_REQUIREMENT_FAILURE_TOO_SHORT);
+    }
+  }
+
+  // If the maximum specified length is shorter or equal to the minimum
+  // specified length, there is no maximum length. Otherwise check if the pin is
+  // longer than the maximum specified length.

[jdufault, 2016/10/04 22:57:27]

I think if the maximum length is shorter than the minimum, we should set the
maximum to the minimum. For example,

min: 4, max: 1 => pin of exactly 4 digits

For the unlimited case, we could hard code that to 0.

It seems that with the current logic we cannot set policy to require a PIN of
exactly 4 digits.

[sammiequon, 2016/10/05 19:48:11]

Done.

+  if (maximum_length > minimum_length && int{pin.size()} > maximum_length) {
+    is_valid = true;
+    if (result) {
+      result->failures.push_back(CredentialRequirementFailure::
+                                     CREDENTIAL_REQUIREMENT_FAILURE_TOO_LONG);
+    }
+  }
+
+  return is_valid;
+}
+
+// Check if a given |pin| is valid given the policies in the |profile|. If
+// |result| is a valid pointer, the failures are added to |result|. Pass a
+// nullptr for |result| if all we want to do is check the validity of |pin|.

[jdufault, 2016/10/04 22:57:27]

nullptr => null (see above comment)

[sammiequon, 2016/10/05 19:48:11]

Done.

+bool IsPinDifficultEnough(const std::string& pin,

[jdufault, 2016/10/04 22:57:28]

If result->allow_easy_pins is eliminated, this function can be simplified by
returning the set of failures.

  std::vector<CredentialRequirementFailure> CheckPinDifficulty(
      const std::string& pin, Profile* profile);

I think applying a similar logic to IsPinLengthValid would make it more readable
as well.

  std::vector<CredentialRequirementFailure> CheckPinLength(
      const std::string& pin, Profile* profile,
      int* out_min_length, int* out_max_length) {
    // ...
    if (out_min_length)
      *out_min_length = min_length;
    // ...
  }

This would eliminate all of the if (result) blocks which make the functions
painful to read and complicate the logic.

[sammiequon, 2016/10/05 19:48:11]

Done.

+                          Profile* profile,
+                          CredentialRequirement* result) {
+  PrefService* pref_service = profile->GetPrefs();
+  bool allow_easy = pref_service->GetBoolean(prefs::kPinUnlockAllowEasyPins);
+  if (result)
+    result->allow_easy_pins = base::MakeUnique<bool>(allow_easy);
+
+  // If the pin length is two or less, there is no need to check for same
+  // character and increasing pin.
+  const int pin_check_easy_threshold = 2;

[jdufault, 2016/10/04 22:57:27]

Move this constant the top of the file and fix the naming style.

[sammiequon, 2016/10/05 19:48:11]

Done.

+  if (int{pin.size()} <= pin_check_easy_threshold)
+    return allow_easy;

[jdufault, 2016/10/04 22:57:28]

Shouldn't this always return true?

[sammiequon, 2016/10/05 19:48:11]

Done.

+
+  // Check for same digits, increasing pin simutaneously.
+  bool is_same = true;
+  bool is_increasing = true;
+  bool first = true;
+  char last_char;
+  for (const char& c : pin) {

[jdufault, 2016/10/04 22:57:28]

I'd remove the const&, the pointer is bigger than the actual value.

I'd switch this to a normal for loop since there are quite a few lines dedicated
to tracking the previous value.

for (size_t i = 1; i < 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);
}

[sammiequon, 2016/10/05 19:48:11]

Done.

+    if (first) {
+      first = false;
+      last_char = c;
+      continue;
+    }
+    is_same = is_same && (c == last_char);
+    is_increasing = is_increasing && (c == last_char + 1);
+    last_char = c;
+  }
+
+  // Pin is considered weak if either of these is met.
+  if ((is_same || is_increasing) && result) {
+    result->failures.push_back(
+        CredentialRequirementFailure::CREDENTIAL_REQUIREMENT_FAILURE_TOO_WEAK);
+  }
+
+  return is_same || is_increasing;
+}
+
 }  // namespace
 
 // quickUnlockPrivate.getAvailableModes
@@ -93,6 +179,37 @@ QuickUnlockPrivateGetActiveModesFunction::Run() {
   return RespondNow(ArgumentList(GetActiveModes::Results::Create(modes)));
 }
 
+// quickUnlockPrivate.isCredentialUsable
+
+QuickUnlockPrivateIsCredentialUsableFunction::
+    QuickUnlockPrivateIsCredentialUsableFunction()
+    : chrome_details_(this) {}
+
+QuickUnlockPrivateIsCredentialUsableFunction::
+    ~QuickUnlockPrivateIsCredentialUsableFunction() {}
+
+ExtensionFunction::ResponseAction
+QuickUnlockPrivateIsCredentialUsableFunction::Run() {
+  params_ = IsCredentialUsable::Params::Create(*args_);
+  EXTENSION_FUNCTION_VALIDATE(params_.get());
+
+  auto result = base::MakeUnique<CredentialRequirement>();
+
+  // Only handles pins for now.
+  if (params_->mode != QuickUnlockMode::QUICK_UNLOCK_MODE_PIN) {
+    return RespondNow(
+        ArgumentList(IsCredentialUsable::Results::Create(*result)));
+  }
+
+  std::string tried_password = params_->credential;
+
+  Profile* profile = chrome_details_.GetProfile();
+  IsPinLengthValid(tried_password, profile, result.get());
+  IsPinDifficultEnough(tried_password, profile, result.get());
+
+  return RespondNow(ArgumentList(IsCredentialUsable::Results::Create(*result)));
+}
+
 // quickUnlockPrivate.setModes
 
 QuickUnlockPrivateSetModesFunction::QuickUnlockPrivateSetModesFunction()
@@ -127,6 +244,17 @@ ExtensionFunction::ResponseAction QuickUnlockPrivateSetModesFunction::Run() {
       return RespondNow(ArgumentList(SetModes::Results::Create(false)));
   }
 
+  // Verify every credential is valid based on policies.
+  for (size_t j = 0; j < params_->modes.size(); ++j) {
+    if (params_->modes[j] == QuickUnlockMode::QUICK_UNLOCK_MODE_PIN &&
+        !IsPinLengthValid(params_->credentials[j], chrome_details_.GetProfile(),
+                          nullptr) &&
+        !IsPinDifficultEnough(params_->credentials[j],
+                              chrome_details_.GetProfile(), nullptr)) {
+      return RespondNow(ArgumentList(SetModes::Results::Create(false)));
+    }
+  }
+
   user_manager::User* user = chromeos::ProfileHelper::Get()->GetUserByProfile(
       chrome_details_.GetProfile());
   chromeos::UserContext user_context(user->GetAccountId());