Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2387253002:
cros: Added policies for screen unlock. (Closed)
|
Created:
4 years, 2 months ago by sammiequon Modified:
4 years, 1 month ago CC:
chromium-reviews, alemate+watch_chromium.org, oshima+watch_chromium.org, achuith+watch_chromium.org, asvitkine+watch_chromium.org, tnagel+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncros: Added policies for screen unlock.
Added two policies for screen unlock. The first one is to restrict which screen unlock methods can be used. The second one is to choose the frequency at which a user has to enter their password to continue using quick unlock.
BUG=612271
TEST=unit_tests --gtest_filter="PinStorageUnitTest.*"
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/2502899929baa186318f39e1394701974d7acec5
Cr-Commit-Position: refs/heads/master@{#430541}
Patch Set 1 #
Total comments: 25
Patch Set 2 : Rebased. #Patch Set 3 : Rebased. #Patch Set 4 : Reparented to LKGR. #Patch Set 5 : Fixed patch set 1 errors. #
Total comments: 13
Patch Set 6 : Rebased. #Patch Set 7 : Fixed patch set 5 errors. #
Total comments: 12
Patch Set 8 : Fixed patch set 7 errors. #
Total comments: 20
Patch Set 9 : Fixed patch set 8 errors. #Patch Set 10 : Rebased. #Patch Set 11 : Fixed rebase error. #
Total comments: 20
Patch Set 12 : Fixed patch set 11 errors. #
Total comments: 9
Patch Set 13 : Fixed patch set 12 errors. #
Total comments: 9
Patch Set 14 : Fixed patch set 13 errors. #Patch Set 15 : Rebased. #
Total comments: 6
Patch Set 16 : Fixed patch set 15 errors. #
Total comments: 16
Patch Set 17 : Fixed patch set 16 errors. #Patch Set 18 : Rebased. #Messages
Total messages: 88 (56 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== cros: Added policies for screen unlock. Added two policies for screen unlock. The first one is to restrict which screen unlock methods can be used. The second one is to choose the frequency at which a user has to enter their password to continue using quick unlock. BUG=612271 TEST=unit_tests --gtest_filter="PinStorageUnitTest.*" ========== to ========== cros: Added policies for screen unlock. Added two policies for screen unlock. The first one is to restrict which screen unlock methods can be used. The second one is to choose the frequency at which a user has to enter their password to continue using quick unlock. BUG=612271 TEST=unit_tests --gtest_filter="PinStorageUnitTest.*" ==========
sammiequon@chromium.org changed reviewers: + jdufault@chromium.org
On 2016/10/03 21:46:55, sammiequon wrote: > mailto:sammiequon@chromium.org changed reviewers: > + mailto:jdufault@chromium.org jdufault@ - Please take a look. Thanks!
I haven't had a chance to go through everything just yet. https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/quick_unlock/pin_storage.cc (right): https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:70: switch (strong_auth_interval) { Have a helper function that converts PasswordConfirmation enums to base::TimeDelta values and call that helper function instead of embedding the switch statement. https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:121: bool PinStorage::IsPinUnlockEnabled() const { Implement this as part of IsQuickUnlockEnabled(). You may need to pass the function some additional params. https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:124: auto all_value = base::StringValue("all"); Is this how other policy code handles logic like this? https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/quick_unlock/pin_storage.h (right): https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/pin_storage.h:27: static const int kMaximumUnlockAttempts = 3; This value should be changeable using policy. https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/pin_storage.h:40: bool HasStrongAuth() const; Why did you decide to define NeedsStrongAuth separately from HasStrongAuth? It seems reasonable to put all of the NeedsStrongAuth logic into HasStrongAuth. Right now, it is very confusing for a reader because HasStrongAuth and NeedsStrongAuth are only subtly different. This makes the API confusing. https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc (right): https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:28: update->Append(base::MakeUnique<base::StringValue>("pin")); If the device is *not* enterprise enrolled, PIN should be allowed. However, the default policy value may be to disallow PIN - but this seems like a server-side change and not a client-side one. https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:128: // Verifies altering the Password confirmation preference produces the expected Password => password https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:156: static_cast<int>(chromeos::PasswordConfirmation::SIX_HOURS)); int{}
https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h (right): https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h:12: enum class PasswordConfirmation { This should also be named PasswordConfirmationFrequency; it's confusing to read if Frequency is not present. https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile.cc (right): https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile.cc:188: static_cast<int>(chromeos::PasswordConfirmation::DAY)); int{} https://codereview.chromium.org/2387253002/diff/20001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2387253002/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:9044: 'example_value': ['password'], Is 'password' a valid entry here? Should these policies be named QuickUnlockModeWhitelist = ['PIN', 'PATTERN', ...] QuickUnlockTimeout = 'SixHours' QuickUnlockAttempts = 3
https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/quick_unlock/pin_storage.cc (right): https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:70: switch (strong_auth_interval) { On 2016/10/04 17:55:06, jdufault wrote: > Have a helper function that converts PasswordConfirmation enums to > base::TimeDelta values and call that helper function instead of embedding the > switch statement. Done. https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:121: bool PinStorage::IsPinUnlockEnabled() const { On 2016/10/04 17:55:05, jdufault wrote: > Implement this as part of IsQuickUnlockEnabled(). You may need to pass the > function some additional params. Aren't these seperate, like quick unlock could be enabled but pin unlock disabled and say, pattern unlock enabled. https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:124: auto all_value = base::StringValue("all"); On 2016/10/04 17:55:05, jdufault wrote: > Is this how other policy code handles logic like this? https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/enterprise... A couple do. https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/quick_unlock/pin_storage.h (right): https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/pin_storage.h:27: static const int kMaximumUnlockAttempts = 3; On 2016/10/04 17:55:06, jdufault wrote: > This value should be changeable using policy. I did not see this on the doc, but I will do it in a separate CL. https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/pin_storage.h:40: bool HasStrongAuth() const; On 2016/10/04 17:55:06, jdufault wrote: > Why did you decide to define NeedsStrongAuth separately from HasStrongAuth? It > seems reasonable to put all of the NeedsStrongAuth logic into HasStrongAuth. > > Right now, it is very confusing for a reader because HasStrongAuth and > NeedsStrongAuth are only subtly different. This makes the API confusing. I thought HasStrongAuth would be needed separately on some cases. A lot tests check to see if last_strong_auth is null. https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc (right): https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:128: // Verifies altering the Password confirmation preference produces the expected On 2016/10/04 17:55:06, jdufault wrote: > Password => password Done. https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:156: static_cast<int>(chromeos::PasswordConfirmation::SIX_HOURS)); On 2016/10/04 17:55:06, jdufault wrote: > int{} int{} will cause a compiler error. https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h (right): https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h:12: enum class PasswordConfirmation { On 2016/10/04 23:26:21, jdufault wrote: > This should also be named PasswordConfirmationFrequency; it's confusing to read > if Frequency is not present. Done. https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile.cc (right): https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile.cc:188: static_cast<int>(chromeos::PasswordConfirmation::DAY)); On 2016/10/04 23:26:21, jdufault wrote: > int{} int{} will cause compiler error. https://codereview.chromium.org/2387253002/diff/20001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2387253002/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:9044: 'example_value': ['password'], On 2016/10/04 23:26:21, jdufault wrote: > Is 'password' a valid entry here? Should these policies be named > > QuickUnlockModeWhitelist = ['PIN', 'PATTERN', ...] > QuickUnlockTimeout = 'SixHours' > QuickUnlockAttempts = 3 Yeah password was on the doc. Done the renaming.
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
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: 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_...)
https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/quick_unlock/pin_storage.cc (right): https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:121: bool PinStorage::IsPinUnlockEnabled() const { On 2016/10/18 22:47:49, sammiequon wrote: > On 2016/10/04 17:55:05, jdufault wrote: > > Implement this as part of IsQuickUnlockEnabled(). You may need to pass the > > function some additional params. > > Aren't these seperate, like quick unlock could be enabled but pin unlock > disabled and say, pattern unlock enabled. There's more code than just PinStorage that needs to know if PIN is enabled/disabled by policy (ie, settings). We can refactor IsQuickUnlockEnabled down the line to support multiple quick unlock modes when needed. We already support multiple modes in the private API only because it is significantly harder to make changes, so we wanted to nail the final design down immediately. https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/quick_unlock/pin_storage.h (right): https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/pin_storage.h:40: bool HasStrongAuth() const; On 2016/10/18 22:47:49, sammiequon wrote: > On 2016/10/04 17:55:06, jdufault wrote: > > Why did you decide to define NeedsStrongAuth separately from HasStrongAuth? It > > seems reasonable to put all of the NeedsStrongAuth logic into HasStrongAuth. > > > > Right now, it is very confusing for a reader because HasStrongAuth and > > NeedsStrongAuth are only subtly different. This makes the API confusing. > > I thought HasStrongAuth would be needed separately on some cases. A lot tests > check to see if last_strong_auth is null. The API is very confusing right now, I think it's worth adding such a method to a test utility class instead of here. FWIW, it should generally not be possible to have a PinStorage instance where there is no last_strong_auth_ time. https://codereview.chromium.org/2387253002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/pin_storage.cc (right): https://codereview.chromium.org/2387253002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:45: time_delta = base::TimeDelta::FromHours(6); Return the value directly and remove the |time_delta| temporary. https://codereview.chromium.org/2387253002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:93: ; Remove extra ; https://codereview.chromium.org/2387253002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:131: auto all_value = base::StringValue("all"); base::StringValue all_value("all")? https://codereview.chromium.org/2387253002/diff/100001/chrome/test/data/polic... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/2387253002/diff/100001/chrome/test/data/polic... chrome/test/data/policy/policy_test_cases.json:2640: "test_policy": { "QuickUnlockModeWhitelist": ["password"] }, I don't think "password" should be related to quick unlock? https://codereview.chromium.org/2387253002/diff/100001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2387253002/diff/100001/components/policy/reso... components/policy/resources/policy_templates.json:9096: 'example_value': ['password'], Use PIN instead of password for an example. https://codereview.chromium.org/2387253002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2387253002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:79812: + <int value="346" label="Enables force sign in for Google Chrome."/> Looks like you have a merge conflict here.
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...
https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/quick_unlock/pin_storage.cc (right): https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:121: bool PinStorage::IsPinUnlockEnabled() const { On 2016/10/21 19:03:44, jdufault wrote: > On 2016/10/18 22:47:49, sammiequon wrote: > > On 2016/10/04 17:55:05, jdufault wrote: > > > Implement this as part of IsQuickUnlockEnabled(). You may need to pass the > > > function some additional params. > > > > Aren't these seperate, like quick unlock could be enabled but pin unlock > > disabled and say, pattern unlock enabled. > > There's more code than just PinStorage that needs to know if PIN is > enabled/disabled by policy (ie, settings). We can refactor IsQuickUnlockEnabled > down the line to support multiple quick unlock modes when needed. > > We already support multiple modes in the private API only because it is > significantly harder to make changes, so we wanted to nail the final design down > immediately. Done. https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/quick_unlock/pin_storage.h (right): https://codereview.chromium.org/2387253002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/quick_unlock/pin_storage.h:40: bool HasStrongAuth() const; On 2016/10/21 19:03:44, jdufault wrote: > On 2016/10/18 22:47:49, sammiequon wrote: > > On 2016/10/04 17:55:06, jdufault wrote: > > > Why did you decide to define NeedsStrongAuth separately from HasStrongAuth? > It > > > seems reasonable to put all of the NeedsStrongAuth logic into HasStrongAuth. > > > > > > Right now, it is very confusing for a reader because HasStrongAuth and > > > NeedsStrongAuth are only subtly different. This makes the API confusing. > > > > I thought HasStrongAuth would be needed separately on some cases. A lot tests > > check to see if last_strong_auth is null. > > The API is very confusing right now, I think it's worth adding such a method to > a test utility class instead of here. > > FWIW, it should generally not be possible to have a PinStorage instance where > there is no last_strong_auth_ time. Done. https://codereview.chromium.org/2387253002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/pin_storage.cc (right): https://codereview.chromium.org/2387253002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:45: time_delta = base::TimeDelta::FromHours(6); On 2016/10/21 19:03:45, jdufault wrote: > Return the value directly and remove the |time_delta| temporary. Done. https://codereview.chromium.org/2387253002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:93: ; On 2016/10/21 19:03:44, jdufault wrote: > Remove extra ; Done. https://codereview.chromium.org/2387253002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:131: auto all_value = base::StringValue("all"); On 2016/10/21 19:03:45, jdufault wrote: > base::StringValue all_value("all")? Done. https://codereview.chromium.org/2387253002/diff/100001/chrome/test/data/polic... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/2387253002/diff/100001/chrome/test/data/polic... chrome/test/data/policy/policy_test_cases.json:2640: "test_policy": { "QuickUnlockModeWhitelist": ["password"] }, On 2016/10/21 19:03:45, jdufault wrote: > I don't think "password" should be related to quick unlock? Should I ask tbuckley@ to remove it then? I put "PIN" for now. https://codereview.chromium.org/2387253002/diff/100001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2387253002/diff/100001/components/policy/reso... components/policy/resources/policy_templates.json:9096: 'example_value': ['password'], On 2016/10/21 19:03:45, jdufault wrote: > Use PIN instead of password for an example. Done. https://codereview.chromium.org/2387253002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2387253002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:79812: + <int value="346" label="Enables force sign in for Google Chrome."/> On 2016/10/21 19:03:45, jdufault wrote: > Looks like you have a merge conflict here. I think it was because of rebase.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2387253002/diff/100001/chrome/test/data/polic... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/2387253002/diff/100001/chrome/test/data/polic... chrome/test/data/policy/policy_test_cases.json:2640: "test_policy": { "QuickUnlockModeWhitelist": ["password"] }, On 2016/10/21 23:49:25, sammiequon wrote: > On 2016/10/21 19:03:45, jdufault wrote: > > I don't think "password" should be related to quick unlock? > > Should I ask tbuckley@ to remove it then? I put "PIN" for now. The doc has been updated, password is no longer related. https://codereview.chromium.org/2387253002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/pin_storage.cc (right): https://codereview.chromium.org/2387253002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:51: default: Remove default case. If you leave it there, I believe the compiler will not give a warning about a missing entry. https://codereview.chromium.org/2387253002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:55: return base::TimeDelta(); Add NOTREACHED() above this line. https://codereview.chromium.org/2387253002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:133: const bool has_strong_auth = HasStrongAuth(); Remove the has_strong_auth variable. https://codereview.chromium.org/2387253002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc (right): https://codereview.chromium.org/2387253002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:26: // Pin is not allowed by default so we need to update it for this test. nit: Pin => PIN What about // PIN is not enabled by default. Enable it for the test. But PIN should be enabled by default if the device is not enterprise enrolled, so this should not be needed. https://codereview.chromium.org/2387253002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2387253002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:80008: + <int value="347" label="Always Open PDF files externally"/> Looks like there are still merge/rebase conflicts here. I would expect the delta to only be + <int value="350" ...> + <int value="351" ...>
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...
https://codereview.chromium.org/2387253002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/pin_storage.cc (right): https://codereview.chromium.org/2387253002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:51: default: On 2016/10/25 17:39:50, jdufault wrote: > Remove default case. If you leave it there, I believe the compiler will not give > a warning about a missing entry. Done. https://codereview.chromium.org/2387253002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:55: return base::TimeDelta(); On 2016/10/25 17:39:50, jdufault wrote: > Add NOTREACHED() above this line. Done. https://codereview.chromium.org/2387253002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:133: const bool has_strong_auth = HasStrongAuth(); On 2016/10/25 17:39:50, jdufault wrote: > Remove the has_strong_auth variable. Done. https://codereview.chromium.org/2387253002/diff/140001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc (right): https://codereview.chromium.org/2387253002/diff/140001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:26: // Pin is not allowed by default so we need to update it for this test. On 2016/10/25 17:39:50, jdufault wrote: > nit: Pin => PIN > > What about > > // PIN is not enabled by default. Enable it for the test. > > But PIN should be enabled by default if the device is not enterprise enrolled, > so this should not be needed. Done. https://codereview.chromium.org/2387253002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2387253002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:80008: + <int value="347" label="Always Open PDF files externally"/> On 2016/10/25 17:39:51, jdufault wrote: > Looks like there are still merge/rebase conflicts here. I would expect the delta > to only be > > + <int value="350" ...> > + <int value="351" ...> I'm not sure where these came from. Perhaps since these are generated, the author forgot to run the python script to change the labels? Not sure if the bots check for these. Should i manually revert?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2387253002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2387253002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:80008: + <int value="347" label="Always Open PDF files externally"/> On 2016/10/25 19:18:41, sammiequon wrote: > On 2016/10/25 17:39:51, jdufault wrote: > > Looks like there are still merge/rebase conflicts here. I would expect the > delta > > to only be > > > > + <int value="350" ...> > > + <int value="351" ...> > > I'm not sure where these came from. Perhaps since these are generated, the > author forgot to run the python script to change the labels? Not sure if the > bots check for these. Should i manually revert? If you checkout this file and regenerate it, all of these changes show up? https://codereview.chromium.org/2387253002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc (right): https://codereview.chromium.org/2387253002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:27: ListPrefUpdate update(profile_->GetPrefs(), PIN should be enabled by default? https://codereview.chromium.org/2387253002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:52: bool IsLastStrongAuthAvaliable() { What about the name `HasStrongAuthInfo`? https://codereview.chromium.org/2387253002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h (right): https://codereview.chromium.org/2387253002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h:23: bool IsQuickUnlockEnabled(PrefService* pref_service); If you add IsPinUnlockEnabled, then I'm not sure what IsQuickUnlockEnabled means, especially in relation to IsPinUnlockEnabled. Quick unlock by itself doesn't stand for anything. I think you should merge the two functions together and use the name IsPinUnlockEnabled. https://codereview.chromium.org/2387253002/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile.cc (right): https://codereview.chromium.org/2387253002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile.cc:177: base::ListValue screen_unlock_whitelist_default; screen_unlock_whitelist_default => quick_unlock_default_whitelist https://codereview.chromium.org/2387253002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile.cc:177: base::ListValue screen_unlock_whitelist_default; Is this the best location to register these prefs? It seems like a lot of cros-specific things are registered similar to WallpaperManagerBase::RegisterPrefs[1]. 1: https://cs.chromium.org/chromium/src/components/wallpaper/wallpaper_manager_b... https://codereview.chromium.org/2387253002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile.cc:178: screen_unlock_whitelist_default.AppendString("password"); Remove password https://codereview.chromium.org/2387253002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/md_settings_ui.cc (right): https://codereview.chromium.org/2387253002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/md_settings_ui.cc:130: profile->GetPrefs())); Is wrapping needed? https://codereview.chromium.org/2387253002/diff/160001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2387253002/diff/160001/components/policy/reso... components/policy/resources/policy_templates.json:9123: 'caption': '''Enter password every day to continue using quick unlock.''', every day (24 hours) https://codereview.chromium.org/2387253002/diff/160001/components/policy/reso... components/policy/resources/policy_templates.json:9128: 'caption': '''Enter password every week to continue using quick unlock.''', every week (168 hours) https://codereview.chromium.org/2387253002/diff/160001/components/policy/reso... components/policy/resources/policy_templates.json:9140: 'desc': ''' Users have to enter the password into the lock screen every once in a while to continue using quick unlock. This policy sents the interval at which the lock screen will request the users password. ''', the password => their password
https://codereview.chromium.org/2387253002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2387253002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:80008: + <int value="347" label="Always Open PDF files externally"/> On 2016/10/27 19:40:04, jdufault wrote: > On 2016/10/25 19:18:41, sammiequon wrote: > > On 2016/10/25 17:39:51, jdufault wrote: > > > Looks like there are still merge/rebase conflicts here. I would expect the > > delta > > > to only be > > > > > > + <int value="350" ...> > > > + <int value="351" ...> > > > > I'm not sure where these came from. Perhaps since these are generated, the > > author forgot to run the python script to change the labels? Not sure if the > > bots check for these. Should i manually revert? > > If you checkout this file and regenerate it, all of these changes show up? Yes. https://codereview.chromium.org/2387253002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc (right): https://codereview.chromium.org/2387253002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:27: ListPrefUpdate update(profile_->GetPrefs(), On 2016/10/27 19:40:04, jdufault wrote: > PIN should be enabled by default? Done. https://codereview.chromium.org/2387253002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:52: bool IsLastStrongAuthAvaliable() { On 2016/10/27 19:40:04, jdufault wrote: > What about the name `HasStrongAuthInfo`? Done. https://codereview.chromium.org/2387253002/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h (right): https://codereview.chromium.org/2387253002/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h:23: bool IsQuickUnlockEnabled(PrefService* pref_service); On 2016/10/27 19:40:04, jdufault wrote: > If you add IsPinUnlockEnabled, then I'm not sure what IsQuickUnlockEnabled > means, especially in relation to IsPinUnlockEnabled. Quick unlock by itself > doesn't stand for anything. > > I think you should merge the two functions together and use the name > IsPinUnlockEnabled. Done. https://codereview.chromium.org/2387253002/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile.cc (right): https://codereview.chromium.org/2387253002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile.cc:177: base::ListValue screen_unlock_whitelist_default; On 2016/10/27 19:40:04, jdufault wrote: > screen_unlock_whitelist_default => quick_unlock_default_whitelist Done. https://codereview.chromium.org/2387253002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile.cc:177: base::ListValue screen_unlock_whitelist_default; On 2016/10/27 19:40:04, jdufault wrote: > Is this the best location to register these prefs? It seems like a lot of > cros-specific things are registered similar to > WallpaperManagerBase::RegisterPrefs[1]. > > 1: > https://cs.chromium.org/chromium/src/components/wallpaper/wallpaper_manager_b... Done. https://codereview.chromium.org/2387253002/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile.cc:178: screen_unlock_whitelist_default.AppendString("password"); On 2016/10/27 19:40:04, jdufault wrote: > Remove password Done. https://codereview.chromium.org/2387253002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/md_settings_ui.cc (right): https://codereview.chromium.org/2387253002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/md_settings_ui.cc:130: profile->GetPrefs())); On 2016/10/27 19:40:04, jdufault wrote: > Is wrapping needed? Done. https://codereview.chromium.org/2387253002/diff/160001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2387253002/diff/160001/components/policy/reso... components/policy/resources/policy_templates.json:9123: 'caption': '''Enter password every day to continue using quick unlock.''', On 2016/10/27 19:40:04, jdufault wrote: > every day (24 hours) Done. https://codereview.chromium.org/2387253002/diff/160001/components/policy/reso... components/policy/resources/policy_templates.json:9128: 'caption': '''Enter password every week to continue using quick unlock.''', On 2016/10/27 19:40:04, jdufault wrote: > every week (168 hours) Done. https://codereview.chromium.org/2387253002/diff/160001/components/policy/reso... components/policy/resources/policy_templates.json:9140: 'desc': ''' Users have to enter the password into the lock screen every once in a while to continue using quick unlock. This policy sents the interval at which the lock screen will request the users password. ''', On 2016/10/27 19:40:04, jdufault wrote: > the password => their password 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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_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.
https://codereview.chromium.org/2387253002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc (right): https://codereview.chromium.org/2387253002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:129: // results. What are the expected results? https://codereview.chromium.org/2387253002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:154: pref_service->SetInteger( Write a helper method to do this. SetConfirmationFrequency(PasswordConfirmationFrequency::SIX_HOURS) https://codereview.chromium.org/2387253002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:173: pin_storage_test.ReduceRemainingStrongAuthTimeBy( What about renaming pin_storage_test to test_api? https://codereview.chromium.org/2387253002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:179: // one week, moving interval by 3 days will not trigger a request for strong This is the same test four times over. I would eliminate 3 of them and keep your favorite. I think there are some other interesting test cases: - Current auth time at 8 hours elapsed, 12 hour timeout. Policy changes to 4 hours (so now timeout has happened from policy change). What happens? - Same thing in reverse (8 hour elapsed, 4 hour timeout, timeout changes to 12 hours). https://codereview.chromium.org/2387253002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc (right): https://codereview.chromium.org/2387253002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc:38: const base::ListValue* screen_lock_whitelist = PIN is still enabled for non-enterprise enrolled users, right? https://codereview.chromium.org/2387253002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h (right): https://codereview.chromium.org/2387253002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h:12: namespace quick_unlock { Remove namespace https://codereview.chromium.org/2387253002/diff/220001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2387253002/diff/220001/components/policy/reso... components/policy/resources/policy_templates.json:9100: 'desc': ''' This policy sets which methods can be used to unlock the lock screen. All means all avaliable methods can be used, otherwise only the specified chosen methods may be used. ''', Drop trailing space. https://codereview.chromium.org/2387253002/diff/220001/components/policy/reso... components/policy/resources/policy_templates.json:9113: 'caption': '''Enter password every six hours to continue using quick unlock.''', What about "Password entry is required every six hours" https://codereview.chromium.org/2387253002/diff/220001/components/policy/reso... components/policy/resources/policy_templates.json:9140: 'desc': ''' Users have to enter their password into the lock screen every once in a while to continue using quick unlock. This policy sents the interval at which the lock screen will request the users password. ''', Drop starting and trailing whitespace.
https://codereview.chromium.org/2387253002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc (right): https://codereview.chromium.org/2387253002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:129: // results. On 2016/10/31 22:22:01, jdufault wrote: > What are the expected results? Done. https://codereview.chromium.org/2387253002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:154: pref_service->SetInteger( On 2016/10/31 22:22:02, jdufault wrote: > Write a helper method to do this. > > SetConfirmationFrequency(PasswordConfirmationFrequency::SIX_HOURS) Done. https://codereview.chromium.org/2387253002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:173: pin_storage_test.ReduceRemainingStrongAuthTimeBy( On 2016/10/31 22:22:01, jdufault wrote: > What about renaming pin_storage_test to test_api? Done. https://codereview.chromium.org/2387253002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:179: // one week, moving interval by 3 days will not trigger a request for strong On 2016/10/31 22:22:01, jdufault wrote: > This is the same test four times over. I would eliminate 3 of them and keep your > favorite. > > I think there are some other interesting test cases: > > - Current auth time at 8 hours elapsed, 12 hour timeout. Policy changes to 4 > hours (so now timeout has happened from policy change). What happens? > > - Same thing in reverse (8 hour elapsed, 4 hour timeout, timeout changes to 12 > hours). Done. https://codereview.chromium.org/2387253002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc (right): https://codereview.chromium.org/2387253002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc:38: const base::ListValue* screen_lock_whitelist = On 2016/10/31 22:22:02, jdufault wrote: > PIN is still enabled for non-enterprise enrolled users, right? You mean as a default, yeah it is. https://codereview.chromium.org/2387253002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h (right): https://codereview.chromium.org/2387253002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h:12: namespace quick_unlock { On 2016/10/31 22:22:02, jdufault wrote: > Remove namespace If I remove this, should I change the name of RegisterProfilePrefs. https://codereview.chromium.org/2387253002/diff/220001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2387253002/diff/220001/components/policy/reso... components/policy/resources/policy_templates.json:9100: 'desc': ''' This policy sets which methods can be used to unlock the lock screen. All means all avaliable methods can be used, otherwise only the specified chosen methods may be used. ''', On 2016/10/31 22:22:02, jdufault wrote: > Drop trailing space. Done. https://codereview.chromium.org/2387253002/diff/220001/components/policy/reso... components/policy/resources/policy_templates.json:9113: 'caption': '''Enter password every six hours to continue using quick unlock.''', On 2016/10/31 22:22:02, jdufault wrote: > What about > > "Password entry is required every six hours" Done. https://codereview.chromium.org/2387253002/diff/220001/components/policy/reso... components/policy/resources/policy_templates.json:9140: 'desc': ''' Users have to enter their password into the lock screen every once in a while to continue using quick unlock. This policy sents the interval at which the lock screen will request the users password. ''', On 2016/10/31 22:22:02, jdufault wrote: > Drop starting and trailing whitespace. Done.
https://codereview.chromium.org/2387253002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h (right): https://codereview.chromium.org/2387253002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h:12: namespace quick_unlock { On 2016/11/01 04:33:18, sammiequon wrote: > On 2016/10/31 22:22:02, jdufault wrote: > > Remove namespace > > If I remove this, should I change the name of RegisterProfilePrefs. Sure, RegisterQuickUnlockProfilePrefs(). https://codereview.chromium.org/2387253002/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/pin_storage.h (right): https://codereview.chromium.org/2387253002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage.h:26: // TODO(sammiequon): Pull this value in from policy. See crbug.com/612271. Why not implement this now, so that we don't have to come back to it in the future? https://codereview.chromium.org/2387253002/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc (right): https://codereview.chromium.org/2387253002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc:25: quick_unlock_whitelist_default.AppendString("PIN"); Is there some shared constant this can use instead of "PIN"? If not, then pull the strings out into constants defined in the anonymous namespace ("all", "PIN"). https://codereview.chromium.org/2387253002/diff/240001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2387253002/diff/240001/components/policy/reso... components/policy/resources/policy_templates.json:9100: 'desc': ''' This policy sets which methods can be used to unlock the lock screen. All means all avaliable methods can be used, otherwise only the specified chosen methods may be used.''', Remove starting whitespace. https://codereview.chromium.org/2387253002/diff/240001/components/policy/reso... components/policy/resources/policy_templates.json:9100: 'desc': ''' This policy sets which methods can be used to unlock the lock screen. All means all avaliable methods can be used, otherwise only the specified chosen methods may be used.''', Run these strings by Tom as well?
https://codereview.chromium.org/2387253002/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h (right): https://codereview.chromium.org/2387253002/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h:12: namespace quick_unlock { On 2016/11/02 18:44:02, jdufault wrote: > On 2016/11/01 04:33:18, sammiequon wrote: > > On 2016/10/31 22:22:02, jdufault wrote: > > > Remove namespace > > > > If I remove this, should I change the name of RegisterProfilePrefs. > > Sure, RegisterQuickUnlockProfilePrefs(). Done. https://codereview.chromium.org/2387253002/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/pin_storage.h (right): https://codereview.chromium.org/2387253002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage.h:26: // TODO(sammiequon): Pull this value in from policy. See crbug.com/612271. On 2016/11/02 18:44:02, jdufault wrote: > Why not implement this now, so that we don't have to come back to it in the > future? I don't see this policy on the doc, do I have to run this by Tom first? Also I'd prefer to get this CL out quicker since it's already been a month but I can work on this right away in a dependent CL. https://codereview.chromium.org/2387253002/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc (right): https://codereview.chromium.org/2387253002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc:25: quick_unlock_whitelist_default.AppendString("PIN"); On 2016/11/02 18:44:02, jdufault wrote: > Is there some shared constant this can use instead of "PIN"? > > If not, then pull the strings out into constants defined in the anonymous > namespace ("all", "PIN"). Done. https://codereview.chromium.org/2387253002/diff/240001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2387253002/diff/240001/components/policy/reso... components/policy/resources/policy_templates.json:9100: 'desc': ''' This policy sets which methods can be used to unlock the lock screen. All means all avaliable methods can be used, otherwise only the specified chosen methods may be used.''', On 2016/11/02 18:44:03, jdufault wrote: > Remove starting whitespace. Done. https://codereview.chromium.org/2387253002/diff/240001/components/policy/reso... components/policy/resources/policy_templates.json:9100: 'desc': ''' This policy sets which methods can be used to unlock the lock screen. All means all avaliable methods can be used, otherwise only the specified chosen methods may be used.''', On 2016/11/02 18:44:02, jdufault wrote: > Run these strings by Tom as well? Done.
lgtm https://codereview.chromium.org/2387253002/diff/240001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/pin_storage.h (right): https://codereview.chromium.org/2387253002/diff/240001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage.h:26: // TODO(sammiequon): Pull this value in from policy. See crbug.com/612271. On 2016/11/02 22:00:29, sammiequon wrote: > On 2016/11/02 18:44:02, jdufault wrote: > > Why not implement this now, so that we don't have to come back to it in the > > future? > > I don't see this policy on the doc, do I have to run this by Tom first? Also I'd > prefer to get this CL out quicker since it's already been a month but I can work > on this right away in a dependent CL. The reason I suggest doing it in this CL is to reduce churn for OWNERs, since you're going to be touching a lot of the same files as this CL. Feel free to run it past Tom first. I'm fine if you implement it in a different CL. https://codereview.chromium.org/2387253002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc (right): https://codereview.chromium.org/2387253002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:163: // value of the new frequency persists. In this case, the frequency shortened How about // A valid strong auth becomes invalid if the confirmation frequency is // shortened to less than the expiration time. https://codereview.chromium.org/2387253002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:179: // frequency) so password confirmation is not required. How about // An expired strong auth becomes usable if the confirmation frequency // gets extended past the expiration time. https://codereview.chromium.org/2387253002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:234: base::TimeDelta::FromDays(10)); This is a little ugly but it is probably not worth the additional code to make it better. https://codereview.chromium.org/2387253002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h (right): https://codereview.chromium.org/2387253002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h:14: // (password) is required before continuing to use quick unlock. drop 'before continuing' https://codereview.chromium.org/2387253002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h:25: // Returns true if pin unlock is allowed by policy and the feature flag is nit: pin => PIN
https://codereview.chromium.org/2387253002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc (right): https://codereview.chromium.org/2387253002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:163: // value of the new frequency persists. In this case, the frequency shortened On 2016/11/02 22:52:59, jdufault wrote: > How about > > // A valid strong auth becomes invalid if the confirmation frequency is > // shortened to less than the expiration time. Done. https://codereview.chromium.org/2387253002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:179: // frequency) so password confirmation is not required. On 2016/11/02 22:52:59, jdufault wrote: > How about > > // An expired strong auth becomes usable if the confirmation frequency > // gets extended past the expiration time. Done. https://codereview.chromium.org/2387253002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h (right): https://codereview.chromium.org/2387253002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h:14: // (password) is required before continuing to use quick unlock. On 2016/11/02 22:52:59, jdufault wrote: > drop 'before continuing' Done. https://codereview.chromium.org/2387253002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h:25: // Returns true if pin unlock is allowed by policy and the feature flag is On 2016/11/02 22:52:59, jdufault wrote: > nit: pin => PIN Done.
Patchset #14 (id:280001) 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...
sammiequon@chromium.org changed reviewers: + stevenjb@chromium.org
On 2016/11/03 00:45:53, sammiequon wrote: > https://codereview.chromium.org/2387253002/diff/260001/chrome/browser/chromeo... > File chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc (right): > > https://codereview.chromium.org/2387253002/diff/260001/chrome/browser/chromeo... > chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:163: // value > of the new frequency persists. In this case, the frequency shortened > On 2016/11/02 22:52:59, jdufault wrote: > > How about > > > > // A valid strong auth becomes invalid if the confirmation frequency is > > // shortened to less than the expiration time. > > Done. > > https://codereview.chromium.org/2387253002/diff/260001/chrome/browser/chromeo... > chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:179: // > frequency) so password confirmation is not required. > On 2016/11/02 22:52:59, jdufault wrote: > > How about > > > > // An expired strong auth becomes usable if the confirmation frequency > > // gets extended past the expiration time. > > Done. > > https://codereview.chromium.org/2387253002/diff/260001/chrome/browser/chromeo... > File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h (right): > > https://codereview.chromium.org/2387253002/diff/260001/chrome/browser/chromeo... > chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h:14: // > (password) is required before continuing to use quick unlock. > On 2016/11/02 22:52:59, jdufault wrote: > > drop 'before continuing' > > Done. > > https://codereview.chromium.org/2387253002/diff/260001/chrome/browser/chromeo... > chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h:25: // Returns > true if pin unlock is allowed by policy and the feature flag is > On 2016/11/02 22:52:59, jdufault wrote: > > nit: pin => PIN > > Done. stevenjb@ - Please take a look at chrome/browser/chromeos/* & chrome/browser/ui/webui/*. Thanks!
https://codereview.chromium.org/2387253002/diff/320001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h (right): https://codereview.chromium.org/2387253002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h:14: // (password) is required to use quick unlock. We should add a comment that these values are used in policy_templates.json for QuickUnlockTimeout so that if they change or are appended to, the policy template will also get updated. Also, this should have a better name (or all of these should be namespaced). https://codereview.chromium.org/2387253002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h:25: // Returns true if PIN unlock is allowed by policy and the feature flag is 'and the quick unlock feature flag is present' https://codereview.chromium.org/2387253002/diff/320001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/md_settings_ui.cc (right): https://codereview.chromium.org/2387253002/diff/320001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/md_settings_ui.cc:128: html_source->AddBoolean("quickUnlockEnabled", pinUnlockEnabled
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== cros: Added policies for screen unlock. Added two policies for screen unlock. The first one is to restrict which screen unlock methods can be used. The second one is to choose the frequency at which a user has to enter their password to continue using quick unlock. BUG=612271 TEST=unit_tests --gtest_filter="PinStorageUnitTest.*" ========== to ========== cros: Added policies for screen unlock. Added two policies for screen unlock. The first one is to restrict which screen unlock methods can be used. The second one is to choose the frequency at which a user has to enter their password to continue using quick unlock. BUG=612271 TEST=unit_tests --gtest_filter="PinStorageUnitTest.*" CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2387253002/diff/320001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h (right): https://codereview.chromium.org/2387253002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h:14: // (password) is required to use quick unlock. On 2016/11/03 18:23:03, stevenjb wrote: > We should add a comment that these values are used in policy_templates.json for > QuickUnlockTimeout so that if they change or are appended to, the policy > template will also get updated. > > Also, this should have a better name (or all of these should be namespaced). Done. https://codereview.chromium.org/2387253002/diff/320001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.h:25: // Returns true if PIN unlock is allowed by policy and the feature flag is On 2016/11/03 18:23:03, stevenjb wrote: > 'and the quick unlock feature flag is present' Done. https://codereview.chromium.org/2387253002/diff/320001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/md_settings_ui.cc (right): https://codereview.chromium.org/2387253002/diff/320001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/md_settings_ui.cc:128: html_source->AddBoolean("quickUnlockEnabled", On 2016/11/03 18:23:03, stevenjb wrote: > pinUnlockEnabled Done.
LGTM FWIW, I would recommend (in a separate CL) namespacing everything in login/quick_unlock/ with quick_unlock:: and simplify the names within that namespace.
Description was changed from ========== cros: Added policies for screen unlock. Added two policies for screen unlock. The first one is to restrict which screen unlock methods can be used. The second one is to choose the frequency at which a user has to enter their password to continue using quick unlock. BUG=612271 TEST=unit_tests --gtest_filter="PinStorageUnitTest.*" CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== cros: Added policies for screen unlock. Added two policies for screen unlock. The first one is to restrict which screen unlock methods can be used. The second one is to choose the frequency at which a user has to enter their password to continue using quick unlock. BUG=612271 TEST=unit_tests --gtest_filter="PinStorageUnitTest.*" CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
sammiequon@chromium.org changed reviewers: + battre@chromium.org, isherman@chromium.org, pastarmovj@chromium.org
On 2016/11/03 21:48:30, stevenjb wrote: > LGTM > > FWIW, I would recommend (in a separate CL) namespacing everything in > login/quick_unlock/ with quick_unlock:: and simplify the names within that > namespace. battre@ - Please take a look at c/b/p/browser_prefs.cc. Thanks! isherman@ - Please take a look at t/m/h/histograms.xml. Thanks! pastarmovj@ - Please take a look at c/b/p/configuration_policy_handler_list_factory.cc and c/p/r/policy_templates.json. Thanks!
histograms.xml lgtm
https://codereview.chromium.org/2387253002/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/pin_storage.cc (right): https://codereview.chromium.org/2387253002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:40: base::TimeDelta QuickUnlockPasswordConfirmationFrequencyFrequencyToTimeDelta( nit: Do you need the word Frequency twice in this name? https://codereview.chromium.org/2387253002/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc (right): https://codereview.chromium.org/2387253002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:141: // The default is one day, so verify moving time back 13 hours should not nit: isn't this moving time forward instead of back? :) https://codereview.chromium.org/2387253002/diff/340001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2387253002/diff/340001/components/policy/reso... components/policy/resources/policy_templates.json:9159: 'type': 'array', Please consult the "ExtensionAllowedTypes" policy to see how to document the possible values for a string enum. Also in the description section write the possible values and what do they mean. Also describe the default behavior when the policy is not set. https://codereview.chromium.org/2387253002/diff/340001/components/policy/reso... components/policy/resources/policy_templates.json:9211: 'desc': '''Users have to enter their password into the lock screen every once in a while to continue using quick unlock. This policy sents the interval at which the lock screen will request the users password.''', Again document the default behavior here. https://codereview.chromium.org/2387253002/diff/340001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2387253002/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:80751: + <int value="346" label="Enables force sign in for Google Chrome."/> Did you change the labels for policies 346-350 or is this some rebase artifact?
c/b/p/browser_prefs.cc LGTM https://codereview.chromium.org/2387253002/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc (right): https://codereview.chromium.org/2387253002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc:32: static_cast<int>(QuickUnlockPasswordConfirmationFrequency::DAY)); I did not research the context. Please be sure that you are not enabling PIN unlock by default while the PIN is set to a default value of 0000 on a factory reset device. https://codereview.chromium.org/2387253002/diff/340001/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2387253002/diff/340001/chrome/common/pref_nam... chrome/common/pref_names.cc:908: const char kQuickUnlockTimeout[] = "quick_unlock_timeout"; Add descriptions to these here? https://codereview.chromium.org/2387253002/diff/340001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2387253002/diff/340001/components/policy/reso... components/policy/resources/policy_templates.json:9211: 'desc': '''Users have to enter their password into the lock screen every once in a while to continue using quick unlock. This policy sents the interval at which the lock screen will request the users password.''', This description is unclear to me. Let's say I pick 6 hours. Does that mean that I have to enter the password (as opposed to quick unlock) - if the Chromebook showed a lock screen for 6 hours? - if the last quick unlock has been 6 hours ago, but I used the Chromebook during that time? - if the last password has been entered 6 hours ago, but I used the Chromebook during that time?
https://codereview.chromium.org/2387253002/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc (right): https://codereview.chromium.org/2387253002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc:32: static_cast<int>(QuickUnlockPasswordConfirmationFrequency::DAY)); On 2016/11/04 10:59:35, battre wrote: > I did not research the context. Please be sure that you are not enabling PIN > unlock by default while the PIN is set to a default value of 0000 on a factory > reset device. Where are you getting the 0000 default value from?
https://codereview.chromium.org/2387253002/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/pin_storage.cc (right): https://codereview.chromium.org/2387253002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage.cc:40: base::TimeDelta QuickUnlockPasswordConfirmationFrequencyFrequencyToTimeDelta( On 2016/11/04 10:33:03, pastarmovj wrote: > nit: Do you need the word Frequency twice in this name? Done. https://codereview.chromium.org/2387253002/diff/340001/chrome/browser/chromeo... File chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc (right): https://codereview.chromium.org/2387253002/diff/340001/chrome/browser/chromeo... chrome/browser/chromeos/login/quick_unlock/pin_storage_unittest.cc:141: // The default is one day, so verify moving time back 13 hours should not On 2016/11/04 10:33:03, pastarmovj wrote: > nit: isn't this moving time forward instead of back? :) Yes. It actually moves the last seen time back which is kind of moving time forward. I made the comment more descriptive. Done. https://codereview.chromium.org/2387253002/diff/340001/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2387253002/diff/340001/chrome/common/pref_nam... chrome/common/pref_names.cc:908: const char kQuickUnlockTimeout[] = "quick_unlock_timeout"; On 2016/11/04 10:59:36, battre wrote: > Add descriptions to these here? Done. https://codereview.chromium.org/2387253002/diff/340001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2387253002/diff/340001/components/policy/reso... components/policy/resources/policy_templates.json:9159: 'type': 'array', On 2016/11/04 10:33:03, pastarmovj wrote: > Please consult the "ExtensionAllowedTypes" policy to see how to document the > possible values for a string enum. > > Also in the description section write the possible values and what do they mean. > Also describe the default behavior when the policy is not set. Done. https://codereview.chromium.org/2387253002/diff/340001/components/policy/reso... components/policy/resources/policy_templates.json:9211: 'desc': '''Users have to enter their password into the lock screen every once in a while to continue using quick unlock. This policy sents the interval at which the lock screen will request the users password.''', On 2016/11/04 10:59:36, battre wrote: > This description is unclear to me. Let's say I pick 6 hours. Does that mean that > I have to enter the password (as opposed to quick unlock) > - if the Chromebook showed a lock screen for 6 hours? > - if the last quick unlock has been 6 hours ago, but I used the Chromebook > during that time? > - if the last password has been entered 6 hours ago, but I used the Chromebook > during that time? Done. https://codereview.chromium.org/2387253002/diff/340001/components/policy/reso... components/policy/resources/policy_templates.json:9211: 'desc': '''Users have to enter their password into the lock screen every once in a while to continue using quick unlock. This policy sents the interval at which the lock screen will request the users password.''', On 2016/11/04 10:33:03, pastarmovj wrote: > Again document the default behavior here. Done. https://codereview.chromium.org/2387253002/diff/340001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2387253002/diff/340001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:80751: + <int value="346" label="Enables force sign in for Google Chrome."/> On 2016/11/04 10:33:03, pastarmovj wrote: > Did you change the labels for policies 346-350 or is this some rebase artifact? These changed when I ran t/m/h/update_policies.py.
On Fri, Nov 4, 2016 at 2:18 PM, <jdufault@chromium.org> wrote: > > https://codereview.chromium.org/2387253002/diff/340001/ > chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc > File chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc > (right): > > https://codereview.chromium.org/2387253002/diff/340001/ > chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc#newcode32 > chrome/browser/chromeos/login/quick_unlock/quick_unlock_utils.cc:32: > static_cast<int>(QuickUnlockPasswordConfirmationFrequency::DAY)); > On 2016/11/04 10:59:35, battre wrote: > > I did not research the context. Please be sure that you are not > enabling PIN > > unlock by default while the PIN is set to a default value of 0000 on a > factory > > reset device. > > Where are you getting the 0000 default value from? > > https://codereview.chromium.org/2387253002/ > That's the part about not researching the context. If there is no default value, we are good. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) 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.
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, isherman@chromium.org, stevenjb@chromium.org, battre@chromium.org, pastarmovj@chromium.org Link to the patchset: https://codereview.chromium.org/2387253002/#ps380001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== cros: Added policies for screen unlock. Added two policies for screen unlock. The first one is to restrict which screen unlock methods can be used. The second one is to choose the frequency at which a user has to enter their password to continue using quick unlock. BUG=612271 TEST=unit_tests --gtest_filter="PinStorageUnitTest.*" CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== cros: Added policies for screen unlock. Added two policies for screen unlock. The first one is to restrict which screen unlock methods can be used. The second one is to choose the frequency at which a user has to enter their password to continue using quick unlock. BUG=612271 TEST=unit_tests --gtest_filter="PinStorageUnitTest.*" CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #18 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== cros: Added policies for screen unlock. Added two policies for screen unlock. The first one is to restrict which screen unlock methods can be used. The second one is to choose the frequency at which a user has to enter their password to continue using quick unlock. BUG=612271 TEST=unit_tests --gtest_filter="PinStorageUnitTest.*" CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== cros: Added policies for screen unlock. Added two policies for screen unlock. The first one is to restrict which screen unlock methods can be used. The second one is to choose the frequency at which a user has to enter their password to continue using quick unlock. BUG=612271 TEST=unit_tests --gtest_filter="PinStorageUnitTest.*" CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/2502899929baa186318f39e1394701974d7acec5 Cr-Commit-Position: refs/heads/master@{#430541} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/2502899929baa186318f39e1394701974d7acec5 Cr-Commit-Position: refs/heads/master@{#430541} |
