Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2374303002:
cros: Added a new function to quick unlock api for checking unfinished pins. (Closed)
|
Created:
4 years, 2 months ago by sammiequon Modified:
4 years ago CC:
chromium-reviews, extensions-reviews_chromium.org, oshima+watch_chromium.org, asvitkine+watch_chromium.org, chromium-apps-reviews_chromium.org, tnagel+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncros: Added a new function to quick unlock api for checking unfinished pins.
This function will check pins against user policies. Currently the checking logic is in javascript code, but we want to move it to C++.
BUG=612271
TEST=unit_tests --gtest_filter="QuickUnlockPrivateUnitTest.*"
Committed: https://crrev.com/a5173075c1a5c6abc3aafeff4116bf2f57d8a644
Cr-Commit-Position: refs/heads/master@{#437552}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Changed the function output. #
Total comments: 45
Patch Set 3 : Added policy to enable/disable easy pins. #Patch Set 4 : Fixed patch set 2 errors. #
Total comments: 28
Patch Set 5 : Rebased. #Patch Set 6 : Fixed patch set 4 errors. #
Total comments: 10
Patch Set 7 : Rebased. #Patch Set 8 : Fixed patch set 6 errors. #
Total comments: 2
Patch Set 9 : Rebased. #Patch Set 10 : Split up problems into warnings and errors. #
Total comments: 64
Patch Set 11 : Fixed patch set 10 errors. #
Total comments: 58
Patch Set 12 : Fixed patch set 11 errors. #
Total comments: 18
Patch Set 13 : Fixed patch set 12 errors. #Patch Set 14 : Rebased. #
Total comments: 4
Patch Set 15 : Fixed patch set 14 errors. #Patch Set 16 : Rebased. #
Total comments: 18
Patch Set 17 : Rebased. #Patch Set 18 : Fixed patch set 15 errors. #
Total comments: 25
Patch Set 19 : Rebased. #Patch Set 20 : Fixed patch set 18 errors. #Patch Set 21 : Rebased. #
Total comments: 18
Patch Set 22 : Fixed patch set 21 errors. #
Total comments: 18
Patch Set 23 : Fixed patch set 22 errors. #
Total comments: 10
Patch Set 24 : Fixed patch set 23 errors. #
Total comments: 4
Patch Set 25 : Rebased. #Patch Set 26 : Fixed patch set 24 errors. #Patch Set 27 : Rebased. #
Total comments: 10
Patch Set 28 : Fixed patch set 27 errors. #Messages
Total messages: 119 (76 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== cros: Added a new function to quick unlock api for checking unfinished pins. This function will check pins against user policies. Currently the checking logic is in javascript code, but we want to move it to C++. BUG=612271 TEST=unit_tests --gtest_filter="QuickUnlockPrivateUnitTest.VerifyPinsAgainstPreferences" ========== to ========== cros: Added a new function to quick unlock api for checking unfinished pins. This function will check pins against user policies. Currently the checking logic is in javascript code, but we want to move it to C++. BUG=612271 TEST=unit_tests --gtest_filter="QuickUnlockPrivateUnitTest.VerifyPinsAgainstPreferences" ==========
sammiequon@chromium.org changed reviewers: + jdufault@chromium.org
On 2016/09/29 17:24:43, sammiequon wrote: > mailto:sammiequon@chromium.org changed reviewers: > + mailto:jdufault@chromium.org jdufault@- Please take a look. Thanks!
https://codereview.chromium.org/2374303002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:110: params_ = GetUsablePin::Params::Create(*args_); setModes needs to validate these checks as well. https://codereview.chromium.org/2374303002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/2374303002/diff/20001/chrome/common/extension... chrome/common/extensions/api/quick_unlock_private.idl:45: static void getUsablePin(DOMString attemptedPassword, Note the difference between how password and credential are used in this file. This API needs to account for two additional things: - Adding a new quick unlock mode - Exposing information to the user (such as minimum length). We should reach out to UI folks and decide if we need to expose specific information, because that will change the shape of the API. For example, it may be okay to say "This PIN is too short" instead of "Enter a PIN between 4 and 6 digits". What about something along these lines? We could drop CredentialRequirement if we don't need the specific policy information. enum CredentialRequirementFailure { TOO_SHORT, TOO_LONG, TOO_WEAK }; dictionary CredentialRequirement { CredentialRequirementFailure[] failures; Number? minLength; Number? maxLength; }; callback CredentialRequirementCallback = void (CredentialRequirement requirement); static void isCredentialUsable( QuickUnlockMode mode, DOMString credential, CredentialRequirementCallback onComplete);
https://codereview.chromium.org/2374303002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:110: params_ = GetUsablePin::Params::Create(*args_); On 2016/09/29 19:36:52, jdufault wrote: > setModes needs to validate these checks as well. Done. https://codereview.chromium.org/2374303002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/2374303002/diff/20001/chrome/common/extension... chrome/common/extensions/api/quick_unlock_private.idl:45: static void getUsablePin(DOMString attemptedPassword, On 2016/09/29 19:36:52, jdufault wrote: > Note the difference between how password and credential are used in this file. > > This API needs to account for two additional things: > > - Adding a new quick unlock mode > - Exposing information to the user (such as minimum length). > > We should reach out to UI folks and decide if we need to expose specific > information, because that will change the shape of the API. For example, it may > be okay to say "This PIN is too short" instead of "Enter a PIN between 4 and 6 > digits". > > What about something along these lines? We could drop CredentialRequirement if > we don't need the specific policy information. > > enum CredentialRequirementFailure { > TOO_SHORT, > TOO_LONG, > TOO_WEAK > }; > > dictionary CredentialRequirement { > CredentialRequirementFailure[] failures; > Number? minLength; > Number? maxLength; > }; > > callback CredentialRequirementCallback = > void (CredentialRequirement requirement); > > static void isCredentialUsable( > QuickUnlockMode mode, > DOMString credential, > CredentialRequirementCallback onComplete); Done.
https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:65: bool CheckPinInvalid(const std::string& pin, 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(...)) { /* ... */ } https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:67: CredentialRequirement* result) { Document that |result| is optional. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:69: PrefService* prefservice = profile->GetPrefs(); nit: pref_service or just prefs https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:73: result->min_length.reset(new int(minimum_length)); base::MakeUnique(minimum_length) https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:82: CREDENTIAL_REQUIREMENT_FAILURE_TOO_SHORT); Did you run git cl format? This looks strange. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:96: return is_invalid; nit: newline above https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:150: std::unique_ptr<CredentialRequirement> result = auto* https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:153: QuickUnlockMode mode = params_->mode; I don't think you need this local if you're only using it once. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:167: const int pin_check_easy_threshold = 2; Is this an optimization? If so, I don't think it is worthwhile - the overhead of calling this function will dwarf any savings. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:170: bool is_same = true, is_increasing = true; Separate these into different lines. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:172: for (int j = 0; j < int{tried_password.size()}; j++) { Iterate over chars directly for (char c : tried_password) { ... } https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:178: is_increasing = is_increasing && (tried_password[j] == last_char + 1); This algorithm looks different from the JS one. For example, I think this will consider 4321 as strong. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:228: if (CheckPinInvalid(credential, chrome_details_.GetProfile(), nullptr)) This assumes that every credential is a PIN, which is not necessarily true. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc (right): https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:183: auto params = base::MakeUnique<base::ListValue>(); auto* for pointers https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:185: params->AppendString(ToString(pin_mode)); Move pin_mode below so params_ initialization stays together. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:330: QuickUnlockModeList{QuickUnlockMode::QUICK_UNLOCK_MODE_PIN}, {"1111"})); By default policy should not be active, so these should still be allowed. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:390: CheckPins(std::vector<std::string>{"1243", "24569", "584934353422432"}, These are extremely hard to read because it's just a long sequence of numbers. I think the test would be significantly more readable if it was written like this EXPECT_EQ({}, CheckPin("1234")); EXPECT_EQ({TOO_SHORT, WEAK_PIN}, CheckPin("11")); https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:393: // Verify that now if the minimum is set to 3, pins of length 3 are accepted. What happens if pin "asdf" is checked? https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile.cc (right): https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile.cc:174: registry->RegisterIntegerPref(prefs::kPinUnlockMinimumLength, 4); 0 default? https://codereview.chromium.org/2374303002/diff/40001/chrome/common/extension... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/2374303002/diff/40001/chrome/common/extension... chrome/common/extensions/api/quick_unlock_private.idl:51: DOMString attemptedPassword, attemptedPassword => credential https://codereview.chromium.org/2374303002/diff/40001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2374303002/diff/40001/chrome/common/pref_name... chrome/common/pref_names.cc:903: const char kPinUnlockMinimumLength[] = "pin_unlock_minimum_length"; It looks like this needs a comment and should be separate from the previous section.
https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:65: bool CheckPinInvalid(const std::string& pin, On 2016/09/30 01:05:50, jdufault wrote: > 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(...)) { /* ... */ } Done. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:67: CredentialRequirement* result) { On 2016/09/30 01:05:50, jdufault wrote: > Document that |result| is optional. Done. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:69: PrefService* prefservice = profile->GetPrefs(); On 2016/09/30 01:05:50, jdufault wrote: > nit: pref_service or just prefs Done. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:73: result->min_length.reset(new int(minimum_length)); On 2016/09/30 01:05:50, jdufault wrote: > base::MakeUnique(minimum_length) Done. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:82: CREDENTIAL_REQUIREMENT_FAILURE_TOO_SHORT); On 2016/09/30 01:05:50, jdufault wrote: > Did you run git cl format? This looks strange. Yes I did. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:96: return is_invalid; On 2016/09/30 01:05:50, jdufault wrote: > nit: newline above Done. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:150: std::unique_ptr<CredentialRequirement> result = On 2016/09/30 01:05:50, jdufault wrote: > auto* auto* does not work but auto does. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:153: QuickUnlockMode mode = params_->mode; On 2016/09/30 01:05:50, jdufault wrote: > I don't think you need this local if you're only using it once. Done. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:167: const int pin_check_easy_threshold = 2; On 2016/09/30 01:05:50, jdufault wrote: > Is this an optimization? If so, I don't think it is worthwhile - the overhead of > calling this function will dwarf any savings. The const int? It's just to show what the 2 stands for. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:170: bool is_same = true, is_increasing = true; On 2016/09/30 01:05:50, jdufault wrote: > Separate these into different lines. Done. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:172: for (int j = 0; j < int{tried_password.size()}; j++) { On 2016/09/30 01:05:50, jdufault wrote: > Iterate over chars directly > > for (char c : tried_password) { ... } Done. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:178: is_increasing = is_increasing && (tried_password[j] == last_char + 1); On 2016/09/30 01:05:50, jdufault wrote: > This algorithm looks different from the JS one. For example, I think this will > consider 4321 as strong. Yes the google doc only says increasing. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:228: if (CheckPinInvalid(credential, chrome_details_.GetProfile(), nullptr)) On 2016/09/30 01:05:50, jdufault wrote: > This assumes that every credential is a PIN, which is not necessarily true. Done. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc (right): https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:183: auto params = base::MakeUnique<base::ListValue>(); On 2016/09/30 01:05:51, jdufault wrote: > auto* for pointers I don't think auto* works for std::unique_ptr<T>. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:185: params->AppendString(ToString(pin_mode)); On 2016/09/30 01:05:50, jdufault wrote: > Move pin_mode below so params_ initialization stays together. Done. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:330: QuickUnlockModeList{QuickUnlockMode::QUICK_UNLOCK_MODE_PIN}, {"1111"})); On 2016/09/30 01:05:50, jdufault wrote: > By default policy should not be active, so these should still be allowed. Done. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:390: CheckPins(std::vector<std::string>{"1243", "24569", "584934353422432"}, On 2016/09/30 01:05:50, jdufault wrote: > These are extremely hard to read because it's just a long sequence of numbers. I > think the test would be significantly more readable if it was written like this > > EXPECT_EQ({}, CheckPin("1234")); > EXPECT_EQ({TOO_SHORT, WEAK_PIN}, CheckPin("11")); Done. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:393: // Verify that now if the minimum is set to 3, pins of length 3 are accepted. On 2016/09/30 01:05:51, jdufault wrote: > What happens if pin "asdf" is checked? This should never be called. Should we add the digit checker into the function and add a new failure? https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile.cc (right): https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile.cc:174: registry->RegisterIntegerPref(prefs::kPinUnlockMinimumLength, 4); On 2016/09/30 01:05:51, jdufault wrote: > 0 default? Done. https://codereview.chromium.org/2374303002/diff/40001/chrome/common/extension... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/2374303002/diff/40001/chrome/common/extension... chrome/common/extensions/api/quick_unlock_private.idl:51: DOMString attemptedPassword, On 2016/09/30 01:05:51, jdufault wrote: > attemptedPassword => credential Done. https://codereview.chromium.org/2374303002/diff/40001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2374303002/diff/40001/chrome/common/pref_name... chrome/common/pref_names.cc:903: const char kPinUnlockMinimumLength[] = "pin_unlock_minimum_length"; On 2016/09/30 01:05:51, jdufault wrote: > It looks like this needs a comment and should be separate from the previous > section. Done.
https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:178: is_increasing = is_increasing && (tried_password[j] == last_char + 1); On 2016/09/30 18:38:35, sammiequon wrote: > On 2016/09/30 01:05:50, jdufault wrote: > > This algorithm looks different from the JS one. For example, I think this will > > consider 4321 as strong. > > Yes the google doc only says increasing. I think we should also consider 4321 weak. https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc (right): https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:393: // Verify that now if the minimum is set to 3, pins of length 3 are accepted. On 2016/09/30 18:38:35, sammiequon wrote: > On 2016/09/30 01:05:51, jdufault wrote: > > What happens if pin "asdf" is checked? > > This should never be called. Should we add the digit checker into the function > and add a new failure? Up to you. We should at least do a sanity check here to make sure we don't crash. https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:67: // nullptr for |result| if all we want to do is check the validity of |pin|. nit: 'a nullptr' => 'null' null is preferred inside of comments. https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:73: int minimum_length = pref_service->GetInteger(prefs::kPinUnlockMinimumLength); Name these min_length, max_length? https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:91: // longer than the maximum specified length. 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. https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:105: // nullptr for |result| if all we want to do is check the validity of |pin|. nullptr => null (see above comment) https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:106: bool IsPinDifficultEnough(const std::string& pin, 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. https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:116: const int pin_check_easy_threshold = 2; Move this constant the top of the file and fix the naming style. https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:118: return allow_easy; Shouldn't this always return true? https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:125: for (const char& c : pin) { 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); } https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc (right): https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:60: PIN_GOOD = 0x01, What about using 1 << 0, 1 << 1, 1 << 2, 1 << 3 instead? https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:392: CheckPin(PIN_GOOD, "1112"); These are much easier to read, thanks. https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile.cc (right): https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile.cc:179: registry->RegisterBooleanPref(prefs::kPinUnlockAllowEasyPins, true); Please keep naming consistent (replace Easy with Weak), ie, CredentialRequirementFailure::TOO_WEAK. https://codereview.chromium.org/2374303002/diff/80001/chrome/common/extension... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/2374303002/diff/80001/chrome/common/extension... chrome/common/extensions/api/quick_unlock_private.idl:32: boolean? allowEasyPins; Why does the UI need to know the policy value for allowing weak pins? https://codereview.chromium.org/2374303002/diff/80001/chrome/common/extension... chrome/common/extensions/api/quick_unlock_private.idl:49: // Returns a set of problems if any with the give mode and credential. What about: // Checks if the given credential can be used for the given unlock mode. Enterprise policy can change credential requirements.
Patchset #6 (id:120001) has been deleted
https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:178: is_increasing = is_increasing && (tried_password[j] == last_char + 1); On 2016/10/04 22:57:27, jdufault wrote: > On 2016/09/30 18:38:35, sammiequon wrote: > > On 2016/09/30 01:05:50, jdufault wrote: > > > This algorithm looks different from the JS one. For example, I think this > will > > > consider 4321 as strong. > > > > Yes the google doc only says increasing. > > I think we should also consider 4321 weak. Done. https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:67: // nullptr for |result| if all we want to do is check the validity of |pin|. On 2016/10/04 22:57:27, jdufault wrote: > nit: 'a nullptr' => 'null' > > null is preferred inside of comments. Done. https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:73: int minimum_length = pref_service->GetInteger(prefs::kPinUnlockMinimumLength); On 2016/10/04 22:57:27, jdufault wrote: > Name these min_length, max_length? Done. https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:91: // longer than the maximum specified length. On 2016/10/04 22:57:27, jdufault wrote: > 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. Done. https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:105: // nullptr for |result| if all we want to do is check the validity of |pin|. On 2016/10/04 22:57:27, jdufault wrote: > nullptr => null (see above comment) Done. https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:106: bool IsPinDifficultEnough(const std::string& pin, On 2016/10/04 22:57:28, jdufault wrote: > 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. Done. https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:116: const int pin_check_easy_threshold = 2; On 2016/10/04 22:57:27, jdufault wrote: > Move this constant the top of the file and fix the naming style. Done. https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:118: return allow_easy; On 2016/10/04 22:57:28, jdufault wrote: > Shouldn't this always return true? Done. https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:125: for (const char& c : pin) { On 2016/10/04 22:57:28, jdufault wrote: > 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); > } Done. https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc (right): https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:60: PIN_GOOD = 0x01, On 2016/10/04 22:57:28, jdufault wrote: > What about using 1 << 0, 1 << 1, 1 << 2, 1 << 3 instead? Done. https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:392: CheckPin(PIN_GOOD, "1112"); On 2016/10/04 22:57:28, jdufault wrote: > These are much easier to read, thanks. :) https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile.cc (right): https://codereview.chromium.org/2374303002/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile.cc:179: registry->RegisterBooleanPref(prefs::kPinUnlockAllowEasyPins, true); On 2016/10/04 22:57:28, jdufault wrote: > Please keep naming consistent (replace Easy with Weak), ie, > CredentialRequirementFailure::TOO_WEAK. Done. https://codereview.chromium.org/2374303002/diff/80001/chrome/common/extension... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/2374303002/diff/80001/chrome/common/extension... chrome/common/extensions/api/quick_unlock_private.idl:32: boolean? allowEasyPins; On 2016/10/04 22:57:28, jdufault wrote: > Why does the UI need to know the policy value for allowing weak pins? I thought they will display a warning instead of error if weak pins are not allowed. https://codereview.chromium.org/2374303002/diff/80001/chrome/common/extension... chrome/common/extensions/api/quick_unlock_private.idl:49: // Returns a set of problems if any with the give mode and credential. On 2016/10/04 22:57:28, jdufault wrote: > What about: > > // Checks if the given credential can be used for the given unlock mode. > Enterprise policy can change credential requirements. Done.
I'll take a look at the code again when the IDL changes are made. https://codereview.chromium.org/2374303002/diff/80001/chrome/common/extension... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/2374303002/diff/80001/chrome/common/extension... chrome/common/extensions/api/quick_unlock_private.idl:32: boolean? allowEasyPins; On 2016/10/05 19:48:11, sammiequon wrote: > On 2016/10/04 22:57:28, jdufault wrote: > > Why does the UI need to know the policy value for allowing weak pins? > > I thought they will display a warning instead of error if weak pins are not > allowed. This means that some of the failures are not actually failures. That's very confusing. Maybe we should: - rename CredentialRequirementFailure to CredentialProblem - add a CredentialProblem[] warnings entry to CredentialRequirement - CredentialCheck is probably a better name for CredentialRequirement as well. - Maybe checkCredential instead of isCredentialUsable as well? https://codereview.chromium.org/2374303002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:70: // null for |result| if all we want to do is check the validity of |pin|. Update comment https://codereview.chromium.org/2374303002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:86: if (int{pin.size()} < min_length) { Compilation fails without this cast? What if you do if (min_length >= pin.size()) // ... https://codereview.chromium.org/2374303002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:91: // If the maximum lenght is nonzero and shorter than the minimum length, the lenght -> length https://codereview.chromium.org/2374303002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:93: if (max_length > 0 && max_length < min_length) This check should happen before assigning out_* params. It should probably be != 0 instead of > 0 as well. https://codereview.chromium.org/2374303002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:107: // |result| is a valid pointer, the failures are added to |result|. Pass a Update comment
Patchset #8 (id:180001) has been deleted
https://codereview.chromium.org/2374303002/diff/80001/chrome/common/extension... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/2374303002/diff/80001/chrome/common/extension... chrome/common/extensions/api/quick_unlock_private.idl:32: boolean? allowEasyPins; On 2016/10/05 21:47:37, jdufault wrote: > On 2016/10/05 19:48:11, sammiequon wrote: > > On 2016/10/04 22:57:28, jdufault wrote: > > > Why does the UI need to know the policy value for allowing weak pins? > > > > I thought they will display a warning instead of error if weak pins are not > > allowed. > > This means that some of the failures are not actually failures. That's very > confusing. Maybe we should: > > - rename CredentialRequirementFailure to CredentialProblem > - add a CredentialProblem[] warnings entry to CredentialRequirement > - CredentialCheck is probably a better name for CredentialRequirement as well. > - Maybe checkCredential instead of isCredentialUsable as well? Done. https://codereview.chromium.org/2374303002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:70: // null for |result| if all we want to do is check the validity of |pin|. On 2016/10/05 21:47:38, jdufault wrote: > Update comment Done. https://codereview.chromium.org/2374303002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:86: if (int{pin.size()} < min_length) { On 2016/10/05 21:47:37, jdufault wrote: > Compilation fails without this cast? What if you do > > if (min_length >= pin.size()) > // ... Yeah it doesn't compile without the cast both ways. https://codereview.chromium.org/2374303002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:91: // If the maximum lenght is nonzero and shorter than the minimum length, the On 2016/10/05 21:47:38, jdufault wrote: > lenght -> length Done. https://codereview.chromium.org/2374303002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:93: if (max_length > 0 && max_length < min_length) On 2016/10/05 21:47:38, jdufault wrote: > This check should happen before assigning out_* params. > > It should probably be != 0 instead of > 0 as well. Done. https://codereview.chromium.org/2374303002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:107: // |result| is a valid pointer, the failures are added to |result|. Pass a On 2016/10/05 21:47:38, jdufault wrote: > Update comment Done.
https://codereview.chromium.org/2374303002/diff/200001/chrome/common/extensio... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/2374303002/diff/200001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:32: boolean? allowWeakPins; I think this API will be more general and flexible if instead of exposing just |problems|, we split it up into |errors| and |warnings|. This means that the |allowWeakPins| will not be needed. For example, if in the future we add a new problem type, PREVIOUSLY_USED, then we would have to add another credential boolean allowReusedPin if we did not have the errors/warnings split.
Patchset #10 (id:240001) has been deleted
https://codereview.chromium.org/2374303002/diff/200001/chrome/common/extensio... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/2374303002/diff/200001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:32: boolean? allowWeakPins; On 2016/10/14 21:15:53, jdufault wrote: > I think this API will be more general and flexible if instead of exposing just > |problems|, we split it up into |errors| and |warnings|. > > This means that the |allowWeakPins| will not be needed. > > For example, if in the future we add a new problem type, PREVIOUSLY_USED, then > we would have to add another credential boolean allowReusedPin if we did not > have the errors/warnings split. Done.
Pretty close, most comments are minor. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:80: if (max_length != 0 && max_length < min_length) Should this be <= 0? What behavior do we want for, say, max_length = -1? https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:87: declare problems here https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:89: if (int{pin.size()} < min_length) { nit: drop {} https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:95: if (max_length != 0 && int{pin.size()} > max_length) { nit: drop {} https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:104: std::vector<CredentialProblem> IsPinDifficultEnough(const std::string& pin) { Maybe return base::Optional<CredentialProblem> instead? Up to you. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:106: // If the pin length is |kPinCheckWeakThreshold| or less, there is no need to nit: newline above https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:125: if ((is_same || is_increasing || is_decreasing)) { drop extraneous () https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:126: problems.push_back(CredentialProblem::CREDENTIAL_PROBLEM_TOO_WEAK); drop {} https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:192: std::string tried_password = params_->credential; 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). https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:194: return RespondNow(ArgumentList(CheckCredential::Results::Create(*result))); This should probably report an error that the input contains non-digit characters? https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:199: auto length_problems = Only use auto when the type of the variable is evident from the expression defining it. (Remove auto) https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:200: IsPinLengthValid(tried_password, profile, result->min_length.get(), 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. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:202: auto weak_problems = IsPinDifficultEnough(tried_password); Specify type instead of auto https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:206: result->warnings.insert(result->warnings.end(), weak_problems.begin(), Maybe you want to add a helper method? AppendToList(result->errors, length_problems); AppendToList(allow_weak ? result->warnings : result->errors, weak_problems) https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:245: for (const std::string& credential : params_->credentials) { If this is made a problem the check can be merged with the one below. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:255: (!IsPinLengthValid(params_->credentials[j], This if statement is pretty challenging to read. You should 1. Use some temporaries 2. Use multiple ifs https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc (right): https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:155: auto result = CheckCredentialUsingPin(pin); Only use auto when type is evident from the initializer expression. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:156: auto errors = result.errors; Specify init type https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:157: auto warnings = result.warnings; Specify init type https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:160: EXPECT_EQ(expected_outcome & PIN_GOOD ? true : false, HasFlag(expected_outcome, PIN_GOOD)? https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:184: CredentialCheck function_result; What about just result? https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:328: QuickUnlockModeList{QuickUnlockMode::QUICK_UNLOCK_MODE_PIN}, {"0000"})); Revert, "" means that the mode is not changed. For example, if there are two quick unlock modes, PIN and SWIPE, then SetModes({ PIN, SWIPE }, { "", "1234" }) will leave PIN with the current value and set SWIPE to 1234. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:387: CheckPin(PIN_SHORT, "55"); Try to reuse the same numeric value when you can - only make them different when it is important to the test. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:424: CheckPin(PIN_SHORT | PIN_WEAK, "111"); Just to verify, if you have just CheckPin(PIN_SHORT, "111") then the test will fail? https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:428: pref_service->SetInteger(prefs::kPinUnlockMinimumLength, 6); This seems like a separate test. https://codereview.chromium.org/2374303002/diff/260001/chrome/test/data/polic... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/2374303002/diff/260001/chrome/test/data/polic... chrome/test/data/policy/policy_test_cases.json:2656: "test_policy": { "PinUnlockAllowWeakPins": false }, We want this to default to false? https://codereview.chromium.org/2374303002/diff/260001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2374303002/diff/260001/components/policy/reso... components/policy/resources/policy_templates.json:9090: 'dynamic_refresh': False, All of these seem like they should support dynamic refresh. https://codereview.chromium.org/2374303002/diff/260001/components/policy/reso... components/policy/resources/policy_templates.json:9097: 'desc': '''Pins created for the lock screen must have a length equal or greater than this value. ''', Drop extra space at end. https://codereview.chromium.org/2374303002/diff/260001/components/policy/reso... components/policy/resources/policy_templates.json:9112: 'desc': '''Pins created for the lock screen must have a length equal or lesser than this value. If this value is 0, or is less than PinUnlockMinimumLength, pins will have no maximum. ''', Drop extra space at end. https://codereview.chromium.org/2374303002/diff/260001/components/policy/reso... components/policy/resources/policy_templates.json:9127: 'desc': '''If this is set to true users cannot set weak pins for the lock screen pin. Such pins include pins whose digits are all the same number, or are increasing. ''', Drop extra space at end.
https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:80: if (max_length != 0 && max_length < min_length) On 2016/10/31 22:03:07, jdufault wrote: > Should this be <= 0? > > What behavior do we want for, say, max_length = -1? 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? https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:87: On 2016/10/31 22:03:07, jdufault wrote: > declare problems here Done. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:89: if (int{pin.size()} < min_length) { On 2016/10/31 22:03:07, jdufault wrote: > nit: drop {} Done. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:95: if (max_length != 0 && int{pin.size()} > max_length) { On 2016/10/31 22:03:07, jdufault wrote: > nit: drop {} Done. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:104: std::vector<CredentialProblem> IsPinDifficultEnough(const std::string& pin) { On 2016/10/31 22:03:07, jdufault wrote: > Maybe return base::Optional<CredentialProblem> instead? Up to you. Would this be an base::Optional<vector<CredentialProblem>>? In case we want to add extra cases later? https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:106: // If the pin length is |kPinCheckWeakThreshold| or less, there is no need to On 2016/10/31 22:03:07, jdufault wrote: > nit: newline above Done. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:125: if ((is_same || is_increasing || is_decreasing)) { On 2016/10/31 22:03:07, jdufault wrote: > drop extraneous () Done. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:126: problems.push_back(CredentialProblem::CREDENTIAL_PROBLEM_TOO_WEAK); On 2016/10/31 22:03:07, jdufault wrote: > drop {} Done. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:192: std::string tried_password = params_->credential; On 2016/10/31 22:03:07, jdufault wrote: > 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). Done. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:194: return RespondNow(ArgumentList(CheckCredential::Results::Create(*result))); On 2016/10/31 22:03:07, jdufault wrote: > This should probably report an error that the input contains non-digit > characters? Done. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:199: auto length_problems = On 2016/10/31 22:03:07, jdufault wrote: > Only use auto when the type of the variable is evident from the expression > defining it. > > (Remove auto) Done. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:200: IsPinLengthValid(tried_password, profile, result->min_length.get(), On 2016/10/31 22:03:07, jdufault wrote: > 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. Done. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:202: auto weak_problems = IsPinDifficultEnough(tried_password); On 2016/10/31 22:03:07, jdufault wrote: > Specify type instead of auto Done. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:206: result->warnings.insert(result->warnings.end(), weak_problems.begin(), On 2016/10/31 22:03:07, jdufault wrote: > Maybe you want to add a helper method? > > AppendToList(result->errors, length_problems); > AppendToList(allow_weak ? result->warnings : result->errors, weak_problems) Done. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:245: for (const std::string& credential : params_->credentials) { On 2016/10/31 22:03:07, jdufault wrote: > If this is made a problem the check can be merged with the one below. Done. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:255: (!IsPinLengthValid(params_->credentials[j], On 2016/10/31 22:03:08, jdufault wrote: > This if statement is pretty challenging to read. You should > > 1. Use some temporaries > 2. Use multiple ifs Done. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc (right): https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:155: auto result = CheckCredentialUsingPin(pin); On 2016/10/31 22:03:08, jdufault wrote: > Only use auto when type is evident from the initializer expression. Done. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:156: auto errors = result.errors; On 2016/10/31 22:03:08, jdufault wrote: > Specify init type Done. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:157: auto warnings = result.warnings; On 2016/10/31 22:03:08, jdufault wrote: > Specify init type Done. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:160: EXPECT_EQ(expected_outcome & PIN_GOOD ? true : false, On 2016/10/31 22:03:08, jdufault wrote: > HasFlag(expected_outcome, PIN_GOOD)? Done. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:184: CredentialCheck function_result; On 2016/10/31 22:03:08, jdufault wrote: > What about just result? result is used in line 181. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:328: QuickUnlockModeList{QuickUnlockMode::QUICK_UNLOCK_MODE_PIN}, {"0000"})); On 2016/10/31 22:03:08, jdufault wrote: > Revert, "" means that the mode is not changed. > > For example, if there are two quick unlock modes, PIN and SWIPE, then > > SetModes({ PIN, SWIPE }, { "", "1234" }) > > will leave PIN with the current value and set SWIPE to 1234. Done. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:387: CheckPin(PIN_SHORT, "55"); On 2016/10/31 22:03:08, jdufault wrote: > Try to reuse the same numeric value when you can - only make them different when > it is important to the test. Done. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:424: CheckPin(PIN_SHORT | PIN_WEAK, "111"); On 2016/10/31 22:03:08, jdufault wrote: > Just to verify, if you have just CheckPin(PIN_SHORT, "111") then the test will > fail? That is correct. This test checks that all problems are reported. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:428: pref_service->SetInteger(prefs::kPinUnlockMinimumLength, 6); On 2016/10/31 22:03:08, jdufault wrote: > This seems like a separate test. Done. https://codereview.chromium.org/2374303002/diff/260001/chrome/test/data/polic... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/2374303002/diff/260001/chrome/test/data/polic... chrome/test/data/policy/policy_test_cases.json:2656: "test_policy": { "PinUnlockAllowWeakPins": false }, On 2016/10/31 22:03:08, jdufault wrote: > We want this to default to false? Done. https://codereview.chromium.org/2374303002/diff/260001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2374303002/diff/260001/components/policy/reso... components/policy/resources/policy_templates.json:9090: 'dynamic_refresh': False, On 2016/10/31 22:03:08, jdufault wrote: > All of these seem like they should support dynamic refresh. Done. https://codereview.chromium.org/2374303002/diff/260001/components/policy/reso... components/policy/resources/policy_templates.json:9097: 'desc': '''Pins created for the lock screen must have a length equal or greater than this value. ''', On 2016/10/31 22:03:08, jdufault wrote: > Drop extra space at end. Done. https://codereview.chromium.org/2374303002/diff/260001/components/policy/reso... components/policy/resources/policy_templates.json:9112: 'desc': '''Pins created for the lock screen must have a length equal or lesser than this value. If this value is 0, or is less than PinUnlockMinimumLength, pins will have no maximum. ''', On 2016/10/31 22:03:08, jdufault wrote: > Drop extra space at end. Done. https://codereview.chromium.org/2374303002/diff/260001/components/policy/reso... components/policy/resources/policy_templates.json:9127: 'desc': '''If this is set to true users cannot set weak pins for the lock screen pin. Such pins include pins whose digits are all the same number, or are increasing. ''', On 2016/10/31 22:03:08, jdufault wrote: > Drop extra space at end. Done.
https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:80: if (max_length != 0 && max_length < min_length) On 2016/11/01 18:08:31, sammiequon wrote: > On 2016/10/31 22:03:07, jdufault wrote: > > Should this be <= 0? > > > > What behavior do we want for, say, max_length = -1? > > 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? 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. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:104: std::vector<CredentialProblem> IsPinDifficultEnough(const std::string& pin) { On 2016/11/01 18:08:31, sammiequon wrote: > On 2016/10/31 22:03:07, jdufault wrote: > > Maybe return base::Optional<CredentialProblem> instead? Up to you. > > Would this be an base::Optional<vector<CredentialProblem>>? In case we want to > add extra cases later? 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. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:67: // Returns a list of problems (if any) for a given |pin|. Update name/comment to match impl. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:77: // pointers, return the profile min/max length as well. Update the comments for all of these methods to describe what type of problem the method returns. Make the comment less generic. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:96: // Check if the pin is shorter than the minimum specified length. nit: newline above https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:119: bool is_same = true; We should be warning for the top-10 most common PINs. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:214: std::vector<CredentialProblem> length_problems = IsPinLengthValid( // Note: This function call writes values to |result->min_length| and |result->max_length|. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:222: AppendToProblemList(result->errors, validity_problems); 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. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:260: if (!IsPinValid(credential).empty()) 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? https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:271: if (params_->modes[j] != QuickUnlockMode::QUICK_UNLOCK_MODE_PIN) 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. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:274: // Check if there any errors with the length of the pin. Drop comment; Generally, comments should call out something non-obvious, explain why, or provide insight into what a large block of code does. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:280: // Check if there any errors with the difficulty of the pin. Drop comment https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h (right): https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h:60: : public UIThreadExtensionFunction { Is this the output from `git cl format`? https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc (right): https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:58: enum TestPinState { ExpectedPinState? https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:58: enum TestPinState { enum class https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:146: bool CredentialChecksContains( What about renaming this to HasProblem? https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:146: bool CredentialChecksContains( Add comment, // Returns true if |problem| is contained in |problems|. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:147: CredentialProblem problem_to_check, Rename to |problem|? https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:148: const std::vector<CredentialProblem> list_of_problems) { Rename to |problems|? https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:159: std::vector<CredentialProblem> errors = result.errors; Use ctor? Also add const. const std::vector<CredentialProblem> errors(result.errors); https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:160: std::vector<CredentialProblem> warnings = result.warnings; Ditto https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:383: // Verifies that unsubmitted pins give the right message to users. What about // Validates PIN error checking in conjunction with policy-related prefs. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:384: TEST_F(QuickUnlockPrivateUnitTest, VerifyPinsAgainstPreferences) { What about the name CheckCredentialProblemReporting https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:438: TEST_F(QuickUnlockPrivateUnitTest, VerifyPinsPreferencesData) { What about the name CheckCredentialGivesCorrectMinMaxPinLength https://codereview.chromium.org/2374303002/diff/280001/chrome/common/extensio... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/2374303002/diff/280001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:32: long? minLength; API implementation is currently always providing a value for these options. It seems like minLength is always defined, so make it non-nullable? If we have an infinite maxLength, this should be null. https://codereview.chromium.org/2374303002/diff/280001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2374303002/diff/280001/components/policy/reso... components/policy/resources/policy_templates.json:9097: 'desc': '''Pins created for the lock screen must have a length equal or greater than this value.''', Pins => PINs https://codereview.chromium.org/2374303002/diff/280001/components/policy/reso... components/policy/resources/policy_templates.json:9112: 'desc': '''Pins created for the lock screen must have a length equal or lesser than this value. If this value is 0, or is less than PinUnlockMinimumLength, pins will have no maximum.''', Pins => PINs https://codereview.chromium.org/2374303002/diff/280001/components/policy/reso... components/policy/resources/policy_templates.json:9112: 'desc': '''Pins created for the lock screen must have a length equal or lesser than this value. If this value is 0, or is less than PinUnlockMinimumLength, pins will have no maximum.''', a length equal or lesser than => a length equal to or less than https://codereview.chromium.org/2374303002/diff/280001/components/policy/reso... components/policy/resources/policy_templates.json:9112: 'desc': '''Pins created for the lock screen must have a length equal or lesser than this value. If this value is 0, or is less than PinUnlockMinimumLength, pins will have no maximum.''', If this value is 0, or is less than => If this value is less than https://codereview.chromium.org/2374303002/diff/280001/components/policy/reso... components/policy/resources/policy_templates.json:9127: 'desc': '''If this is set to true users cannot set weak pins for the lock screen pin. Such pins include pins whose digits are all the same number, or are increasing.''', pins => PINs https://codereview.chromium.org/2374303002/diff/280001/components/policy/reso... components/policy/resources/policy_templates.json:9127: 'desc': '''If this is set to true users cannot set weak pins for the lock screen pin. Such pins include pins whose digits are all the same number, or are increasing.''', I wonder if these strings need to get approved by a PM or similar.
Description was changed from ========== cros: Added a new function to quick unlock api for checking unfinished pins. This function will check pins against user policies. Currently the checking logic is in javascript code, but we want to move it to C++. BUG=612271 TEST=unit_tests --gtest_filter="QuickUnlockPrivateUnitTest.VerifyPinsAgainstPreferences" ========== to ========== cros: Added a new function to quick unlock api for checking unfinished pins. This function will check pins against user policies. Currently the checking logic is in javascript code, but we want to move it to C++. BUG=612271 TEST=unit_tests --gtest_filter="QuickUnlockPrivateUnitTest.CheckCredential*" ==========
https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:80: if (max_length != 0 && max_length < min_length) On 2016/11/01 18:56:43, jdufault wrote: > On 2016/11/01 18:08:31, sammiequon wrote: > > On 2016/10/31 22:03:07, jdufault wrote: > > > Should this be <= 0? > > > > > > What behavior do we want for, say, max_length = -1? > > > > 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? > > 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. Done. https://codereview.chromium.org/2374303002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:104: std::vector<CredentialProblem> IsPinDifficultEnough(const std::string& pin) { On 2016/11/01 18:56:43, jdufault wrote: > On 2016/11/01 18:08:31, sammiequon wrote: > > On 2016/10/31 22:03:07, jdufault wrote: > > > Maybe return base::Optional<CredentialProblem> instead? Up to you. > > > > Would this be an base::Optional<vector<CredentialProblem>>? In case we want to > > add extra cases later? > > 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. 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. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:67: // Returns a list of problems (if any) for a given |pin|. On 2016/11/01 18:56:44, jdufault wrote: > Update name/comment to match impl. Done. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:77: // pointers, return the profile min/max length as well. On 2016/11/01 18:56:43, jdufault wrote: > Update the comments for all of these methods to describe what type of problem > the method returns. Make the comment less generic. Done. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:96: // Check if the pin is shorter than the minimum specified length. On 2016/11/01 18:56:44, jdufault wrote: > nit: newline above Done. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:119: bool is_same = true; On 2016/11/01 18:56:43, jdufault wrote: > We should be warning for the top-10 most common PINs. I added a TODO. I don't think adding this would cause much delays but just to be safe. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:214: std::vector<CredentialProblem> length_problems = IsPinLengthValid( On 2016/11/01 18:56:43, jdufault wrote: > // Note: This function call writes values to |result->min_length| and > |result->max_length|. Done. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:222: AppendToProblemList(result->errors, validity_problems); On 2016/11/01 18:56:43, jdufault wrote: > 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. I think this make it harder to read because of writing min/max length. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:260: if (!IsPinValid(credential).empty()) On 2016/11/01 18:56:43, jdufault wrote: > 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? Done. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:271: if (params_->modes[j] != QuickUnlockMode::QUICK_UNLOCK_MODE_PIN) On 2016/11/01 18:56:43, jdufault wrote: > 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. Done. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:274: // Check if there any errors with the length of the pin. On 2016/11/01 18:56:43, jdufault wrote: > Drop comment; > > Generally, comments should call out something non-obvious, explain why, or > provide insight into what a large block of code does. Done. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:280: // Check if there any errors with the difficulty of the pin. On 2016/11/01 18:56:43, jdufault wrote: > Drop comment Done. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h (right): https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.h:60: : public UIThreadExtensionFunction { On 2016/11/01 18:56:44, jdufault wrote: > Is this the output from `git cl format`? Yes. Confirmed by running it again. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc (right): https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:58: enum TestPinState { On 2016/11/01 18:56:44, jdufault wrote: > ExpectedPinState? Done. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:58: enum TestPinState { On 2016/11/01 18:56:44, jdufault wrote: > enum class According to stack overflow, bitmask with enum class require & and | overloading or adding a macro through a header. If that is true, I do not think it is necessary. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:146: bool CredentialChecksContains( On 2016/11/01 18:56:44, jdufault wrote: > What about renaming this to HasProblem? Done. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:146: bool CredentialChecksContains( On 2016/11/01 18:56:44, jdufault wrote: > Add comment, > > // Returns true if |problem| is contained in |problems|. Done. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:147: CredentialProblem problem_to_check, On 2016/11/01 18:56:44, jdufault wrote: > Rename to |problem|? Done. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:148: const std::vector<CredentialProblem> list_of_problems) { On 2016/11/01 18:56:44, jdufault wrote: > Rename to |problems|? Done. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:159: std::vector<CredentialProblem> errors = result.errors; On 2016/11/01 18:56:44, jdufault wrote: > Use ctor? Also add const. > > const std::vector<CredentialProblem> errors(result.errors); Done. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:160: std::vector<CredentialProblem> warnings = result.warnings; On 2016/11/01 18:56:44, jdufault wrote: > Ditto Done. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:383: // Verifies that unsubmitted pins give the right message to users. On 2016/11/01 18:56:44, jdufault wrote: > What about > > // Validates PIN error checking in conjunction with policy-related prefs. Done. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:384: TEST_F(QuickUnlockPrivateUnitTest, VerifyPinsAgainstPreferences) { On 2016/11/01 18:56:44, jdufault wrote: > What about the name > > CheckCredentialProblemReporting Done. https://codereview.chromium.org/2374303002/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:438: TEST_F(QuickUnlockPrivateUnitTest, VerifyPinsPreferencesData) { On 2016/11/01 18:56:44, jdufault wrote: > What about the name > > CheckCredentialGivesCorrectMinMaxPinLength Done. https://codereview.chromium.org/2374303002/diff/280001/chrome/common/extensio... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/2374303002/diff/280001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:32: long? minLength; On 2016/11/01 18:56:44, jdufault wrote: > API implementation is currently always providing a value for these options. > > It seems like minLength is always defined, so make it non-nullable? > > If we have an infinite maxLength, this should be null. 0 stands for infinite, so this should non-nullable as well methink. https://codereview.chromium.org/2374303002/diff/280001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2374303002/diff/280001/components/policy/reso... components/policy/resources/policy_templates.json:9097: 'desc': '''Pins created for the lock screen must have a length equal or greater than this value.''', On 2016/11/01 18:56:45, jdufault wrote: > Pins => PINs Done. https://codereview.chromium.org/2374303002/diff/280001/components/policy/reso... components/policy/resources/policy_templates.json:9112: 'desc': '''Pins created for the lock screen must have a length equal or lesser than this value. If this value is 0, or is less than PinUnlockMinimumLength, pins will have no maximum.''', On 2016/11/01 18:56:45, jdufault wrote: > a length equal or lesser than => a length equal to or less than Done. https://codereview.chromium.org/2374303002/diff/280001/components/policy/reso... components/policy/resources/policy_templates.json:9112: 'desc': '''Pins created for the lock screen must have a length equal or lesser than this value. If this value is 0, or is less than PinUnlockMinimumLength, pins will have no maximum.''', On 2016/11/01 18:56:45, jdufault wrote: > Pins => PINs Done. https://codereview.chromium.org/2374303002/diff/280001/components/policy/reso... components/policy/resources/policy_templates.json:9112: 'desc': '''Pins created for the lock screen must have a length equal or lesser than this value. If this value is 0, or is less than PinUnlockMinimumLength, pins will have no maximum.''', On 2016/11/01 18:56:45, jdufault wrote: > If this value is 0, or is less than => If this value is less than Changed the description a bit. https://codereview.chromium.org/2374303002/diff/280001/components/policy/reso... components/policy/resources/policy_templates.json:9127: 'desc': '''If this is set to true users cannot set weak pins for the lock screen pin. Such pins include pins whose digits are all the same number, or are increasing.''', On 2016/11/01 18:56:45, jdufault wrote: > pins => PINs Done. https://codereview.chromium.org/2374303002/diff/280001/components/policy/reso... components/policy/resources/policy_templates.json:9127: 'desc': '''If this is set to true users cannot set weak pins for the lock screen pin. Such pins include pins whose digits are all the same number, or are increasing.''', On 2016/11/01 18:56:45, jdufault wrote: > I wonder if these strings need to get approved by a PM or similar. Messaged Tom.
https://codereview.chromium.org/2374303002/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:35: // Pins greater in length than |kPinCheckWeakThreshold| will be checked for nit: Pins => PINs https://codereview.chromium.org/2374303002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:37: const int kPinCheckWeakThreshold = 2; Using Threshold as a name is a bit confusing here. It could mean any number of things, like some threshold heuristic? I'd go with something more explicit, ie, kMinPinLengthForWeakCheck, kMinWeakPinLength, or kMinLengthForWeakPin. https://codereview.chromium.org/2374303002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:87: if (max_length != 0 && max_length < min_length) This should happen after the sanitization check. https://codereview.chromium.org/2374303002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:120: // crbug.com/661649. I don't think adding the check will be much work, can you do it in this CL? https://codereview.chromium.org/2374303002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:140: // Pin is considered weak if either of these is met. nit: either => any nit: Pin => PIN https://codereview.chromium.org/2374303002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:147: void AppendToProblemList(std::vector<CredentialProblem>& list_to_append_to, What about a standard src/dest name? AppendToProblemList(vector dest, vector src) src first feels more natural to me, but I'm not sure if we have standardized upon anything so up to you. https://codereview.chromium.org/2374303002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:208: return RespondNow(ArgumentList(CheckCredential::Results::Create(*result))); nit: drop {} https://codereview.chromium.org/2374303002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:218: // Note: This function call writes values to |result->min_length| and This comment should be above the IsPinLengthValid function call. https://codereview.chromium.org/2374303002/diff/300001/chrome/common/extensio... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/2374303002/diff/300001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:33: long maxLength; // Add a comment that if maxLength == 0 there is no max PIN length.
Patchset #13 (id:320001) has been deleted
https://codereview.chromium.org/2374303002/diff/300001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:35: // Pins greater in length than |kPinCheckWeakThreshold| will be checked for On 2016/11/02 19:22:59, jdufault wrote: > nit: Pins => PINs Done. https://codereview.chromium.org/2374303002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:37: const int kPinCheckWeakThreshold = 2; On 2016/11/02 19:22:59, jdufault wrote: > Using Threshold as a name is a bit confusing here. It could mean any number of > things, like some threshold heuristic? > > I'd go with something more explicit, ie, kMinPinLengthForWeakCheck, > kMinWeakPinLength, or kMinLengthForWeakPin. Done. https://codereview.chromium.org/2374303002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:87: if (max_length != 0 && max_length < min_length) On 2016/11/02 19:22:59, jdufault wrote: > This should happen after the sanitization check. Done. https://codereview.chromium.org/2374303002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:120: // crbug.com/661649. On 2016/11/02 19:22:59, jdufault wrote: > I don't think adding the check will be much work, can you do it in this CL? Done. https://codereview.chromium.org/2374303002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:140: // Pin is considered weak if either of these is met. On 2016/11/02 19:22:59, jdufault wrote: > nit: either => any > nit: Pin => PIN Done. https://codereview.chromium.org/2374303002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:147: void AppendToProblemList(std::vector<CredentialProblem>& list_to_append_to, On 2016/11/02 19:22:59, jdufault wrote: > What about a standard src/dest name? > > AppendToProblemList(vector dest, vector src) > > src first feels more natural to me, but I'm not sure if we have standardized > upon anything so up to you. Done. https://codereview.chromium.org/2374303002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:208: return RespondNow(ArgumentList(CheckCredential::Results::Create(*result))); On 2016/11/02 19:22:59, jdufault wrote: > nit: drop {} Done. https://codereview.chromium.org/2374303002/diff/300001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:218: // Note: This function call writes values to |result->min_length| and On 2016/11/02 19:22:59, jdufault wrote: > This comment should be above the IsPinLengthValid function call. Done. https://codereview.chromium.org/2374303002/diff/300001/chrome/common/extensio... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/2374303002/diff/300001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:33: long maxLength; On 2016/11/02 19:22:59, jdufault wrote: > // Add a comment that if maxLength == 0 there is no max PIN length. Done.
https://codereview.chromium.org/2374303002/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:131: kMostCommonPins + sizeof(kMostCommonPins) / sizeof(kMostCommonPins[0]); You should be able to use std::end(kMostCommonPins) https://codereview.chromium.org/2374303002/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:132: bool is_too_common = std::find(kMostCommonPins, end, pin) != end; why the too? What about is_too_common => is_common?
https://codereview.chromium.org/2374303002/diff/360001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:131: kMostCommonPins + sizeof(kMostCommonPins) / sizeof(kMostCommonPins[0]); On 2016/11/08 23:39:56, jdufault wrote: > You should be able to use std::end(kMostCommonPins) Done. https://codereview.chromium.org/2374303002/diff/360001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:132: bool is_too_common = std::find(kMostCommonPins, end, pin) != end; On 2016/11/08 23:39:56, jdufault wrote: > why the too? What about is_too_common => is_common? Done.
lgtm https://codereview.chromium.org/2374303002/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc (right): https://codereview.chromium.org/2374303002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:401: // Verify that now if the minimum is set to 3, PINs of length 3 are accepted. nit: minimum => minimum length https://codereview.chromium.org/2374303002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:403: CheckPin(PIN_WEAK_BUT_OK, "111"); What about renaming PIN_WEAK/PIN_WEAK_BUT_OK? PIN_WEAK => PIN_WEAK_ERROR PIN_WEAK_BUT_OK => PIN_WEAK_WARNING I think it will make it clearer that weak can be either a warning or error. https://codereview.chromium.org/2374303002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:407: pref_service->SetInteger(prefs::kPinUnlockMaximumLength, 2); You should also test when min and max length are equal. https://codereview.chromium.org/2374303002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:410: CheckPin(PIN_SHORT, "112"); PIN_TOO_SHORT, PIN_TOO_LONG will probably be easier to read as it makes it clearer that they are errors. https://codereview.chromium.org/2374303002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:414: // considered as long PINs. nit: are considered as long PINs => are considered too long and cannot be used. https://codereview.chromium.org/2374303002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:438: CheckPin(PIN_WEAK, "1111"); I'm not sure the difference between the "1111" and the "11111111" test case. I would remove the "11111111" test case. https://codereview.chromium.org/2374303002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:447: CheckPin(PIN_WEAK, "1212"); I think you only need one of these cases (making sure it doesn't match any of the other weak filters). https://codereview.chromium.org/2374303002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:452: CheckPin(PIN_SHORT | PIN_WEAK, "111"); Comment or remove these test cases. https://codereview.chromium.org/2374303002/diff/400001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2374303002/diff/400001/components/policy/reso... components/policy/resources/policy_templates.json:9303: If this setting is less than PinUnlockMinimum but greater than 0, the maximum length is the same as the minimum length. PinUnlockMinimum => PinUnlockMinimumLength
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
https://codereview.chromium.org/2374303002/diff/400001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc (right): https://codereview.chromium.org/2374303002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:401: // Verify that now if the minimum is set to 3, PINs of length 3 are accepted. On 2016/11/17 17:50:33, jdufault wrote: > nit: minimum => minimum length Done. https://codereview.chromium.org/2374303002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:403: CheckPin(PIN_WEAK_BUT_OK, "111"); On 2016/11/17 17:50:33, jdufault wrote: > What about renaming PIN_WEAK/PIN_WEAK_BUT_OK? > > PIN_WEAK => PIN_WEAK_ERROR > PIN_WEAK_BUT_OK => PIN_WEAK_WARNING > > I think it will make it clearer that weak can be either a warning or error. Done. https://codereview.chromium.org/2374303002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:407: pref_service->SetInteger(prefs::kPinUnlockMaximumLength, 2); On 2016/11/17 17:50:33, jdufault wrote: > You should also test when min and max length are equal. Done. https://codereview.chromium.org/2374303002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:410: CheckPin(PIN_SHORT, "112"); On 2016/11/17 17:50:33, jdufault wrote: > PIN_TOO_SHORT, PIN_TOO_LONG will probably be easier to read as it makes it > clearer that they are errors. Done. https://codereview.chromium.org/2374303002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:414: // considered as long PINs. On 2016/11/17 17:50:33, jdufault wrote: > nit: are considered as long PINs => are considered too long and cannot be used. Done. https://codereview.chromium.org/2374303002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:438: CheckPin(PIN_WEAK, "1111"); On 2016/11/17 17:50:33, jdufault wrote: > I'm not sure the difference between the "1111" and the "11111111" test case. I > would remove the "11111111" test case. Done. https://codereview.chromium.org/2374303002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:447: CheckPin(PIN_WEAK, "1212"); On 2016/11/17 17:50:33, jdufault wrote: > I think you only need one of these cases (making sure it doesn't match any of > the other weak filters). Done. https://codereview.chromium.org/2374303002/diff/400001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:452: CheckPin(PIN_SHORT | PIN_WEAK, "111"); On 2016/11/17 17:50:33, jdufault wrote: > Comment or remove these test cases. Done. https://codereview.chromium.org/2374303002/diff/400001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2374303002/diff/400001/components/policy/reso... components/policy/resources/policy_templates.json:9303: If this setting is less than PinUnlockMinimum but greater than 0, the maximum length is the same as the minimum length. On 2016/11/17 17:50:34, jdufault wrote: > PinUnlockMinimum => PinUnlockMinimumLength Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sammiequon@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
rdevlin.cronin@ - Please take a look at c/b/c/extensions/*. Thanks!
Nice! A few nits, but all pretty small and overall this looks great. https://codereview.chromium.org/2374303002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:83: Profile* profile, Maybe just pass in prefs here, rather than the full profile? https://codereview.chromium.org/2374303002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:109: if (int{pin.size()} < min_length) prefer static_cast<int> here (here and below) https://codereview.chromium.org/2374303002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:131: bool is_common = std::find(kMostCommonPins, end, pin) != end; just inline std::end() here. https://codereview.chromium.org/2374303002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:143: is_decreasing = is_decreasing && (current == previous - 1); do we have any kind of data for this? It strikes me as a little weird that 23456789 isn't allowed but 124 is. https://codereview.chromium.org/2374303002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:217: Profile* profile = chrome_details_.GetProfile(); Prefer to just use Profile::FromBrowserContext(browser_context()). https://codereview.chromium.org/2374303002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:222: std::vector<CredentialProblem> validity_problems = IsPinNumeric(credential); This seems overly complex. Each of these only returns a single problem, and all except pin length return a constant problem. Maybe: if (!IsPinNumeric()) problems.push_back(not numeric); CredentialProblem length_problem; if (!IsPinLengthValid(&length_problem)) problems.push_back(length_problem); if (!IsPinDifficultEnough()) problems.push_back(too easy); Then we get rid of AppendToProblemList, returning vectors in all cases, etc. https://codereview.chromium.org/2374303002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:272: for (size_t j = 0; j < params_->modes.size(); ++j) { range-based for loop? https://codereview.chromium.org/2374303002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc (right): https://codereview.chromium.org/2374303002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:440: // a 9 is not considered decreasing. Ah, this answers my question. :) I think this would fit better with the verification code, and then have a comment here saying "// See FooMethod for the description of a weak pin", but I don't feel terribly strongly. https://codereview.chromium.org/2374303002/diff/440001/chrome/common/extensio... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/2374303002/diff/440001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:22: enum CredentialProblem { comment https://codereview.chromium.org/2374303002/diff/440001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:30: CredentialProblem[] errors; Ideally, we should have comments over each of these. https://codereview.chromium.org/2374303002/diff/440001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:33: // If |maxLength| is 0, there is no PIN max length. Prefer something like: // The maximum allowed length for a pin. A value of 0 indicates no maximum length. https://codereview.chromium.org/2374303002/diff/440001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:51: // Enterprise policy can change credential requirements. Document each param that goes in (similar to setModes below)
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #20 (id:480001) has been deleted
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
https://codereview.chromium.org/2374303002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:109: if (int{pin.size()} < min_length) On 2016/11/23 18:22:49, Devlin (OOO through nov28) wrote: > prefer static_cast<int> here (here and below) The style guide says that int{} style casts are preferred when casting arithmetic types (https://google.github.io/styleguide/cppguide.html#Casting).
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, no build URL)
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2374303002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:83: Profile* profile, On 2016/11/23 18:22:49, Devlin wrote: > Maybe just pass in prefs here, rather than the full profile? Done. https://codereview.chromium.org/2374303002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:109: if (int{pin.size()} < min_length) On 2016/11/23 18:22:49, Devlin wrote: > prefer static_cast<int> here (here and below) Done. https://codereview.chromium.org/2374303002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:131: bool is_common = std::find(kMostCommonPins, end, pin) != end; On 2016/11/23 18:22:48, Devlin wrote: > just inline std::end() here. Done. https://codereview.chromium.org/2374303002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:143: is_decreasing = is_decreasing && (current == previous - 1); On 2016/11/23 18:22:48, Devlin wrote: > do we have any kind of data for this? It strikes me as a little weird that > 23456789 isn't allowed but 124 is. I agree. Let me check with the PM if we should hold up on the increase/decrease if the pin is sufficiently long. https://codereview.chromium.org/2374303002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:217: Profile* profile = chrome_details_.GetProfile(); On 2016/11/23 18:22:49, Devlin wrote: > Prefer to just use Profile::FromBrowserContext(browser_context()). Done. https://codereview.chromium.org/2374303002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:222: std::vector<CredentialProblem> validity_problems = IsPinNumeric(credential); On 2016/11/23 18:22:49, Devlin wrote: > This seems overly complex. Each of these only returns a single problem, and all > except pin length return a constant problem. Maybe: > if (!IsPinNumeric()) > problems.push_back(not numeric); > CredentialProblem length_problem; > if (!IsPinLengthValid(&length_problem)) > problems.push_back(length_problem); > if (!IsPinDifficultEnough()) > problems.push_back(too easy); > > Then we get rid of AppendToProblemList, returning vectors in all cases, etc. Done. This is a lot cleaner. Thanks! https://codereview.chromium.org/2374303002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:272: for (size_t j = 0; j < params_->modes.size(); ++j) { On 2016/11/23 18:22:48, Devlin wrote: > range-based for loop? params->modes & params->credentials are always the same size and i am using both of their elements inside the for loop. https://codereview.chromium.org/2374303002/diff/440001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc (right): https://codereview.chromium.org/2374303002/diff/440001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc:440: // a 9 is not considered decreasing. On 2016/11/23 18:22:49, Devlin wrote: > Ah, this answers my question. :) I think this would fit better with the > verification code, and then have a comment here saying "// See FooMethod for the > description of a weak pin", but I don't feel terribly strongly. Done. https://codereview.chromium.org/2374303002/diff/440001/chrome/common/extensio... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/2374303002/diff/440001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:22: enum CredentialProblem { On 2016/11/23 18:22:49, Devlin wrote: > comment Done. https://codereview.chromium.org/2374303002/diff/440001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:30: CredentialProblem[] errors; On 2016/11/23 18:22:49, Devlin wrote: > Ideally, we should have comments over each of these. Done. https://codereview.chromium.org/2374303002/diff/440001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:33: // If |maxLength| is 0, there is no PIN max length. On 2016/11/23 18:22:49, Devlin wrote: > Prefer something like: > // The maximum allowed length for a pin. A value of 0 indicates no maximum > length. Done. https://codereview.chromium.org/2374303002/diff/440001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:51: // Enterprise policy can change credential requirements. On 2016/11/23 18:22:49, Devlin wrote: > Document each param that goes in (similar to setModes below) Done.
Patchset #22 (id:540001) has been deleted
a few more small nits, but otherwise lgtm https://codereview.chromium.org/2374303002/diff/520001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/520001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:105: *length_problem = CredentialProblem::CREDENTIAL_PROBLEM_TOO_SHORT; need to check this for null (here and on line 112) https://codereview.chromium.org/2374303002/diff/520001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:135: std::end(kMostCommonPins); nit: just return here: if (std::find(...) != std::end(...)) return false; https://codereview.chromium.org/2374303002/diff/520001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:199: : chrome_details_(this) {} no longer needed, right? https://codereview.chromium.org/2374303002/diff/520001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:206: params_ = CheckCredential::Params::Create(*args_); given params is only used here, let's have it be local to this function, rather than a private member https://codereview.chromium.org/2374303002/diff/520001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:207: EXTENSION_FUNCTION_VALIDATE(params_.get()); nit: params_.get() is redundant (unique_ptr has a bool operator). Prefer just EXTENSION_FUNCTION_VALIDATE(params_) https://codereview.chromium.org/2374303002/diff/520001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:215: std::string credential = params_->credential; const std::string& https://codereview.chromium.org/2374303002/diff/520001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:232: if (length_problem != CredentialProblem::CREDENTIAL_PROBLEM_NONE) When would we return false to IsPinLengthValid(), but not have a length problem? Seems like, if anything, this should be a DCHECK_NE() https://codereview.chromium.org/2374303002/diff/520001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:277: bool allow_weak = Profile::FromBrowserContext(browser_context()) nit: cache Profile so we can re-use it on line 290. https://codereview.chromium.org/2374303002/diff/520001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:280: for (size_t j = 0; j < params_->modes.size(); ++j) { This loop seems unnecessary given the check on line 273 - do we anticipate various modes being accepted later?
https://codereview.chromium.org/2374303002/diff/520001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/520001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:105: *length_problem = CredentialProblem::CREDENTIAL_PROBLEM_TOO_SHORT; On 2016/11/29 21:35:17, Devlin wrote: > need to check this for null (here and on line 112) Done. https://codereview.chromium.org/2374303002/diff/520001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:135: std::end(kMostCommonPins); On 2016/11/29 21:35:17, Devlin wrote: > nit: just return here: > if (std::find(...) != std::end(...)) > return false; Done. https://codereview.chromium.org/2374303002/diff/520001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:199: : chrome_details_(this) {} On 2016/11/29 21:35:17, Devlin wrote: > no longer needed, right? Right. https://codereview.chromium.org/2374303002/diff/520001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:206: params_ = CheckCredential::Params::Create(*args_); On 2016/11/29 21:35:17, Devlin wrote: > given params is only used here, let's have it be local to this function, rather > than a private member Done. https://codereview.chromium.org/2374303002/diff/520001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:207: EXTENSION_FUNCTION_VALIDATE(params_.get()); On 2016/11/29 21:35:17, Devlin wrote: > nit: params_.get() is redundant (unique_ptr has a bool operator). Prefer just > EXTENSION_FUNCTION_VALIDATE(params_) Done. https://codereview.chromium.org/2374303002/diff/520001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:215: std::string credential = params_->credential; On 2016/11/29 21:35:17, Devlin wrote: > const std::string& Done. https://codereview.chromium.org/2374303002/diff/520001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:232: if (length_problem != CredentialProblem::CREDENTIAL_PROBLEM_NONE) On 2016/11/29 21:35:17, Devlin wrote: > When would we return false to IsPinLengthValid(), but not have a length problem? > Seems like, if anything, this should be a DCHECK_NE() Done. https://codereview.chromium.org/2374303002/diff/520001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:277: bool allow_weak = Profile::FromBrowserContext(browser_context()) On 2016/11/29 21:35:17, Devlin wrote: > nit: cache Profile so we can re-use it on line 290. Done. https://codereview.chromium.org/2374303002/diff/520001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:280: for (size_t j = 0; j < params_->modes.size(); ++j) { On 2016/11/29 21:35:17, Devlin wrote: > This loop seems unnecessary given the check on line 273 - do we anticipate > various modes being accepted later? Yes, there are other modes planned to be added later.
stevenjb@ - Please take a look at c/b/c/*. Thanks!
sammiequon@chromium.org changed reviewers: + stevenjb@chromium.org
stevenjb@ - Please take a look at c/b/c/*. Thanks!
https://codereview.chromium.org/2374303002/diff/560001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/560001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:37: const int kMinLengthForWeakPin = 2; This should really be kMinLengthForNonWeakPin, or just kMinPinLength. https://codereview.chromium.org/2374303002/diff/560001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:41: "1004", "2000", "4444", "2222", "6969"}; Aren't 1234, 1111, 0000, 7777, 4444, and 2222 already considered weak? https://codereview.chromium.org/2374303002/diff/560001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:94: // maximum length is the minimum length. nit: comment is unnecessary https://codereview.chromium.org/2374303002/diff/560001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:101: *out_max_length = max_length; This method seems a bit over complicated. Could we split it up into a method that retrieves and validates the min/max length, and a separate on that validates the pin? https://codereview.chromium.org/2374303002/diff/560001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:131: // check for same character and increasing pin. It would be nice if these tests matched the order in the comment above (or vice versa, or just integrate those comments with the code). https://codereview.chromium.org/2374303002/diff/560001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:141: // Check for same digits, increasing PIN simutaneously. simultaneously https://codereview.chromium.org/2374303002/diff/560001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:235: &result->max_length, &length_problem)) { This is a file local function, right? Why not just pass result.get()? https://codereview.chromium.org/2374303002/diff/560001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:289: if (params_->modes[j] == QuickUnlockMode::QUICK_UNLOCK_MODE_PIN) { invert and continue https://codereview.chromium.org/2374303002/diff/560001/chrome/common/extensio... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/2374303002/diff/560001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:41: }; It seems a little awkward to me that we return the min/max pin length along with the credential check result. It seems like the UI might want to request that information independently (e.g. to prevent submitting an invalid pin). Maybe add a getCredentialRequirements method instead and just provide the min/max length there?
Patchset #23 (id:580001) has been deleted
Description was changed from ========== cros: Added a new function to quick unlock api for checking unfinished pins. This function will check pins against user policies. Currently the checking logic is in javascript code, but we want to move it to C++. BUG=612271 TEST=unit_tests --gtest_filter="QuickUnlockPrivateUnitTest.CheckCredential*" ========== to ========== cros: Added a new function to quick unlock api for checking unfinished pins. This function will check pins against user policies. Currently the checking logic is in javascript code, but we want to move it to C++. BUG=612271 TEST=unit_tests --gtest_filter="QuickUnlockPrivateUnitTest.*" ==========
Sorry forgot to rebase. https://codereview.chromium.org/2374303002/diff/560001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/560001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:37: const int kMinLengthForWeakPin = 2; On 2016/11/30 18:13:58, stevenjb wrote: > This should really be kMinLengthForNonWeakPin, or just kMinPinLength. Done. https://codereview.chromium.org/2374303002/diff/560001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:41: "1004", "2000", "4444", "2222", "6969"}; On 2016/11/30 18:13:58, stevenjb wrote: > Aren't 1234, 1111, 0000, 7777, 4444, and 2222 already considered weak? Done. https://codereview.chromium.org/2374303002/diff/560001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:94: // maximum length is the minimum length. On 2016/11/30 18:13:58, stevenjb wrote: > nit: comment is unnecessary Done. https://codereview.chromium.org/2374303002/diff/560001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:101: *out_max_length = max_length; On 2016/11/30 18:13:58, stevenjb wrote: > This method seems a bit over complicated. Could we split it up into a method > that retrieves and validates the min/max length, and a separate on that > validates the pin? Done. https://codereview.chromium.org/2374303002/diff/560001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:131: // check for same character and increasing pin. On 2016/11/30 18:13:58, stevenjb wrote: > It would be nice if these tests matched the order in the comment above (or vice > versa, or just integrate those comments with the code). Done. https://codereview.chromium.org/2374303002/diff/560001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:141: // Check for same digits, increasing PIN simutaneously. On 2016/11/30 18:13:58, stevenjb wrote: > simultaneously Done. https://codereview.chromium.org/2374303002/diff/560001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:235: &result->max_length, &length_problem)) { On 2016/11/30 18:13:58, stevenjb wrote: > This is a file local function, right? Why not just pass result.get()? Function changed. https://codereview.chromium.org/2374303002/diff/560001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:289: if (params_->modes[j] == QuickUnlockMode::QUICK_UNLOCK_MODE_PIN) { On 2016/11/30 18:13:58, stevenjb wrote: > invert and continue Done. https://codereview.chromium.org/2374303002/diff/560001/chrome/common/extensio... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/2374303002/diff/560001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:41: }; On 2016/11/30 18:13:58, stevenjb wrote: > It seems a little awkward to me that we return the min/max pin length along with > the credential check result. It seems like the UI might want to request that > information independently (e.g. to prevent submitting an invalid pin). > > Maybe add a getCredentialRequirements method instead and just provide the > min/max length there? Done.
Thanks, lgtm w/ nits https://codereview.chromium.org/2374303002/diff/600001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/600001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:94: DCHECK(out_min_length && out_max_length); nit: DCHECK input requirements at top of function. https://codereview.chromium.org/2374303002/diff/600001/chrome/common/extensio... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/2374303002/diff/600001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:50: void (CredentialRequirements requirements); indent 4 spaces from start of previous line https://codereview.chromium.org/2374303002/diff/600001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:66: // |credential| has (if any). Indent comment 4 spaces (this looks like |credential| is described twice). Also, elaborate '(if any)', e.g. 'or an empty list if there are none' https://codereview.chromium.org/2374303002/diff/600001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:73: // |onComplete|: Called with the credential requirements of the given |mode|. Line too long https://codereview.chromium.org/2374303002/diff/600001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:75: QuickUnlockMode mode, CredentialRequirementsCallback onComplete); indent
sammiequon@chromium.org changed reviewers: + isherman@chromium.org, pastarmovj@chromium.org
isherman@ - Please take a look at t/m/h/histograms.xml and e/b/extension_function_histogram_value.h. Thanks! pastarmovj@ - Please take a look at c/bp/configuration_policy_handler_list_factory.cc and c/pr/policy_templates.json. Thanks! https://codereview.chromium.org/2374303002/diff/600001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc (right): https://codereview.chromium.org/2374303002/diff/600001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api.cc:94: DCHECK(out_min_length && out_max_length); On 2016/12/01 17:32:13, stevenjb wrote: > nit: DCHECK input requirements at top of function. Done. https://codereview.chromium.org/2374303002/diff/600001/chrome/common/extensio... File chrome/common/extensions/api/quick_unlock_private.idl (right): https://codereview.chromium.org/2374303002/diff/600001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:50: void (CredentialRequirements requirements); On 2016/12/01 17:32:13, stevenjb wrote: > indent 4 spaces from start of previous line Done. https://codereview.chromium.org/2374303002/diff/600001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:66: // |credential| has (if any). On 2016/12/01 17:32:13, stevenjb wrote: > Indent comment 4 spaces (this looks like |credential| is described twice). > Also, elaborate '(if any)', e.g. 'or an empty list if there are none' Done. https://codereview.chromium.org/2374303002/diff/600001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:73: // |onComplete|: Called with the credential requirements of the given |mode|. On 2016/12/01 17:32:13, stevenjb wrote: > Line too long Done. https://codereview.chromium.org/2374303002/diff/600001/chrome/common/extensio... chrome/common/extensions/api/quick_unlock_private.idl:75: QuickUnlockMode mode, CredentialRequirementsCallback onComplete); On 2016/12/01 17:32:13, stevenjb wrote: > indent Done.
histograms.xml lgtm
https://codereview.chromium.org/2374303002/diff/620001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2374303002/diff/620001/components/policy/reso... components/policy/resources/policy_templates.json:9204: 'name': 'QuickUnlockModeWhitelist', Consider grouping all PIN related policies in a group. https://codereview.chromium.org/2374303002/diff/620001/components/policy/reso... components/policy/resources/policy_templates.json:9318: 'name': 'PinUnlockAllowWeakPins', To be consistent (at least with the recently added policies) please rename this one to PinUnlockWeakPinsAllowed. Also in this case the True value should mean weak pins are allowed and False that they are not. Your documentation currently states the opposite.
https://codereview.chromium.org/2374303002/diff/620001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2374303002/diff/620001/components/policy/reso... components/policy/resources/policy_templates.json:9204: 'name': 'QuickUnlockModeWhitelist', On 2016/12/02 08:26:12, pastarmovj wrote: > Consider grouping all PIN related policies in a group. Done. https://codereview.chromium.org/2374303002/diff/620001/components/policy/reso... components/policy/resources/policy_templates.json:9318: 'name': 'PinUnlockAllowWeakPins', On 2016/12/02 08:26:12, pastarmovj wrote: > To be consistent (at least with the recently added policies) please rename this > one to PinUnlockWeakPinsAllowed. > > Also in this case the True value should mean weak pins are allowed and False > that they are not. Your documentation currently states the opposite. Done.
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
Patchset #27 (id:680001) has been deleted
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
Patchset #27 (id:700001) has been deleted
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/12/02 18:48:59, sammiequon wrote: > https://codereview.chromium.org/2374303002/diff/620001/components/policy/reso... > File components/policy/resources/policy_templates.json (right): > > https://codereview.chromium.org/2374303002/diff/620001/components/policy/reso... > components/policy/resources/policy_templates.json:9204: 'name': > 'QuickUnlockModeWhitelist', > On 2016/12/02 08:26:12, pastarmovj wrote: > > Consider grouping all PIN related policies in a group. > > Done. > > https://codereview.chromium.org/2374303002/diff/620001/components/policy/reso... > components/policy/resources/policy_templates.json:9318: 'name': > 'PinUnlockAllowWeakPins', > On 2016/12/02 08:26:12, pastarmovj wrote: > > To be consistent (at least with the recently added policies) please rename > this > > one to PinUnlockWeakPinsAllowed. > > > > Also in this case the True value should mean weak pins are allowed and False > > that they are not. Your documentation currently states the opposite. > > Done. pastarmovj@ - Ping/Please take a look. Thanks!
Sorry for the delay. A couple of nits and one last question and we are good to go. https://codereview.chromium.org/2374303002/diff/720001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2374303002/diff/720001/components/policy/reso... components/policy/resources/policy_templates.json:9268: 'supported_on': ['chrome_os:56-'], nit: please bump up to 57 unless you are planning to merge (highly unlikely given that this is UI change). https://codereview.chromium.org/2374303002/diff/720001/components/policy/reso... components/policy/resources/policy_templates.json:9287: 'supported_on': ['chrome_os:56-'], nit:ditto. https://codereview.chromium.org/2374303002/diff/720001/components/policy/reso... components/policy/resources/policy_templates.json:9290: 'per_profile': False, Just a question here. Why are these policies below not per profile while the top one is? https://codereview.chromium.org/2374303002/diff/720001/components/policy/reso... components/policy/resources/policy_templates.json:9306: 'supported_on': ['chrome_os:56-'], nit:ditto. https://codereview.chromium.org/2374303002/diff/720001/components/policy/reso... components/policy/resources/policy_templates.json:9327: 'supported_on': ['chrome_os:56-'], nit: ditto.
https://codereview.chromium.org/2374303002/diff/720001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2374303002/diff/720001/components/policy/reso... components/policy/resources/policy_templates.json:9268: 'supported_on': ['chrome_os:56-'], On 2016/12/08 09:02:19, pastarmovj wrote: > nit: please bump up to 57 unless you are planning to merge (highly unlikely > given that this is UI change). Done. https://codereview.chromium.org/2374303002/diff/720001/components/policy/reso... components/policy/resources/policy_templates.json:9287: 'supported_on': ['chrome_os:56-'], On 2016/12/08 09:02:19, pastarmovj wrote: > nit:ditto. Done. https://codereview.chromium.org/2374303002/diff/720001/components/policy/reso... components/policy/resources/policy_templates.json:9290: 'per_profile': False, On 2016/12/08 09:02:19, pastarmovj wrote: > Just a question here. Why are these policies below not per profile while the top > one is? That is a mistake they should all be true. Done. https://codereview.chromium.org/2374303002/diff/720001/components/policy/reso... components/policy/resources/policy_templates.json:9306: 'supported_on': ['chrome_os:56-'], On 2016/12/08 09:02:19, pastarmovj wrote: > nit:ditto. Done. https://codereview.chromium.org/2374303002/diff/720001/components/policy/reso... components/policy/resources/policy_templates.json:9327: 'supported_on': ['chrome_os:56-'], On 2016/12/08 09:02:19, pastarmovj wrote: > nit: ditto. Done.
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by sammiequon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jdufault@chromium.org, rdevlin.cronin@chromium.org, stevenjb@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2374303002/#ps740001 (title: "Fixed patch set 27 errors.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 740001, "attempt_start_ts": 1481298046828010,
"parent_rev": "419df0e9daa0617dd28a8d0ca9c94f67f0f0372f", "commit_rev":
"d28c15b857f42bf548aac9c561318967c56ef958"}
Message was sent while issue was closed.
Description was changed from ========== cros: Added a new function to quick unlock api for checking unfinished pins. This function will check pins against user policies. Currently the checking logic is in javascript code, but we want to move it to C++. BUG=612271 TEST=unit_tests --gtest_filter="QuickUnlockPrivateUnitTest.*" ========== to ========== cros: Added a new function to quick unlock api for checking unfinished pins. This function will check pins against user policies. Currently the checking logic is in javascript code, but we want to move it to C++. BUG=612271 TEST=unit_tests --gtest_filter="QuickUnlockPrivateUnitTest.*" Review-Url: https://codereview.chromium.org/2374303002 ==========
Message was sent while issue was closed.
Committed patchset #28 (id:740001)
Message was sent while issue was closed.
Description was changed from ========== cros: Added a new function to quick unlock api for checking unfinished pins. This function will check pins against user policies. Currently the checking logic is in javascript code, but we want to move it to C++. BUG=612271 TEST=unit_tests --gtest_filter="QuickUnlockPrivateUnitTest.*" Review-Url: https://codereview.chromium.org/2374303002 ========== to ========== cros: Added a new function to quick unlock api for checking unfinished pins. This function will check pins against user policies. Currently the checking logic is in javascript code, but we want to move it to C++. BUG=612271 TEST=unit_tests --gtest_filter="QuickUnlockPrivateUnitTest.*" Committed: https://crrev.com/a5173075c1a5c6abc3aafeff4116bf2f57d8a644 Cr-Commit-Position: refs/heads/master@{#437552} ==========
Message was sent while issue was closed.
Patchset 28 (id:??) landed as https://crrev.com/a5173075c1a5c6abc3aafeff4116bf2f57d8a644 Cr-Commit-Position: refs/heads/master@{#437552} |
