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..77d6b6c17f948f7211dce0aea820435614be37c6 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,40 @@ bool AreModesEqual(const QuickUnlockModeList& a, const QuickUnlockModeList& b) {
   return true;
 }
 
+bool CheckPinInvalid(const std::string& pin,

[jdufault, 2016/09/30 01:05:50]

I think it would be easier to read if this was stated positively, because if we
want to negate it there will be two double-negatives. So what about IsPinValid?

For example, this is confusing to read:

  if (!CheckPinInvalid(...)) { /* ... */ }

[sammiequon, 2016/09/30 18:38:35]

Done.

+                     Profile* profile,
+                     CredentialRequirement* result) {

[jdufault, 2016/09/30 01:05:50]

Document that |result| is optional.

[sammiequon, 2016/09/30 18:38:35]

Done.

+  bool is_invalid = false;
+  PrefService* prefservice = profile->GetPrefs();

[jdufault, 2016/09/30 01:05:50]

nit: pref_service or just prefs

[sammiequon, 2016/09/30 18:38:35]

Done.

+  int minimum_length = prefservice->GetInteger(prefs::kPinUnlockMinimumLength);
+  int maximum_length = prefservice->GetInteger(prefs::kPinUnlockMaximumLength);
+  if (result) {
+    result->min_length.reset(new int(minimum_length));

[jdufault, 2016/09/30 01:05:50]

base::MakeUnique(minimum_length)

[sammiequon, 2016/09/30 18:38:35]

Done.

+    result->max_length.reset(new int(maximum_length));
+  }
+
+  // Check if the pin is shorter than the minimum specified length.
+  if (int{pin.size()} < minimum_length) {
+    is_invalid = true;
+    if (result) {
+      result->failures.push_back(CredentialRequirementFailure::
+                                     CREDENTIAL_REQUIREMENT_FAILURE_TOO_SHORT);

[jdufault, 2016/09/30 01:05:50]

Did you run git cl format? This looks strange.

[sammiequon, 2016/09/30 18:38:35]

Yes I did.

+    }
+  }
+
+  // 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.
+  if (maximum_length > minimum_length && int{pin.size()} > maximum_length) {
+    is_invalid = true;
+    if (result) {
+      result->failures.push_back(CredentialRequirementFailure::
+                                     CREDENTIAL_REQUIREMENT_FAILURE_TOO_LONG);
+    }
+  }
+  return is_invalid;

[jdufault, 2016/09/30 01:05:50]

nit: newline above

[sammiequon, 2016/09/30 18:38:35]

Done.

+}
+
 }  // namespace
 
 // quickUnlockPrivate.getAvailableModes
@@ -93,6 +133,62 @@ 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());
+
+  std::unique_ptr<CredentialRequirement> result =

[jdufault, 2016/09/30 01:05:50]

auto*

[sammiequon, 2016/09/30 18:38:35]

auto* does not work but auto does.

+      base::MakeUnique<CredentialRequirement>();
+
+  QuickUnlockMode mode = params_->mode;

[jdufault, 2016/09/30 01:05:50]

I don't think you need this local if you're only using it once.

[sammiequon, 2016/09/30 18:38:35]

Done.

+  // Only handles pins for now.
+  if (mode != QuickUnlockMode::QUICK_UNLOCK_MODE_PIN) {
+    return RespondNow(
+        ArgumentList(IsCredentialUsable::Results::Create(*result)));
+  }
+
+  std::string tried_password = params_->attempted_password;
+
+  Profile* profile = chrome_details_.GetProfile();
+  CheckPinInvalid(tried_password, profile, result.get());
+
+  // 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/09/30 01:05:50]

Is this an optimization? If so, I don't think it is worthwhile - the overhead of
calling this function will dwarf any savings.

[sammiequon, 2016/09/30 18:38:35]

The const int? It's just to show what the 2 stands for.

+  if (int{tried_password.size()} > pin_check_easy_threshold) {
+    // Check for same digits, increasing pin simutaneously.
+    bool is_same = true, is_increasing = true;

[jdufault, 2016/09/30 01:05:50]

Separate these into different lines.

[sammiequon, 2016/09/30 18:38:35]

Done.

+    char last_char;
+    for (int j = 0; j < int{tried_password.size()}; j++) {

[jdufault, 2016/09/30 01:05:50]

Iterate over chars directly

  for (char c : tried_password) { ... }

[sammiequon, 2016/09/30 18:38:35]

Done.

+      if (j == 0) {
+        last_char = tried_password[j];
+        continue;
+      }
+      is_same = is_same && (tried_password[j] == last_char);
+      is_increasing = is_increasing && (tried_password[j] == last_char + 1);

[jdufault, 2016/09/30 01:05:50]

This algorithm looks different from the JS one. For example, I think this will
consider 4321 as strong.

[sammiequon, 2016/09/30 18:38:35]

Yes the google doc only says increasing.

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

I think we should also consider 4321 weak.

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

Done.

+      last_char = tried_password[j];
+    }
+
+    // Pin is considered weak if either of these is met.
+    if (is_same || is_increasing) {
+      result->failures.push_back(CredentialRequirementFailure::
+                                     CREDENTIAL_REQUIREMENT_FAILURE_TOO_WEAK);
+    }
+  }
+
+  return RespondNow(ArgumentList(IsCredentialUsable::Results::Create(*result)));
+}
+
 // quickUnlockPrivate.setModes
 
 QuickUnlockPrivateSetModesFunction::QuickUnlockPrivateSetModesFunction()
@@ -127,6 +223,12 @@ ExtensionFunction::ResponseAction QuickUnlockPrivateSetModesFunction::Run() {
       return RespondNow(ArgumentList(SetModes::Results::Create(false)));
   }
 
+  // Verify every credential is valid based on policies.
+  for (const std::string& credential : params_->credentials) {
+    if (CheckPinInvalid(credential, chrome_details_.GetProfile(), nullptr))

[jdufault, 2016/09/30 01:05:50]

This assumes that every credential is a PIN, which is not necessarily true.

[sammiequon, 2016/09/30 18:38:35]

Done.

+      return RespondNow(ArgumentList(SetModes::Results::Create(false)));
+  }
+
   user_manager::User* user = chromeos::ProfileHelper::Get()->GetUserByProfile(
       chrome_details_.GetProfile());
   chromeos::UserContext user_context(user->GetAccountId());