Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2627083002:
cros: Ensure quick unlock is disabled by default for enterprise policy. (Closed)
Descriptioncros: Ensure quick unlock is disabled by default for enterprise policy.
BUG=680161
Review-Url: https://codereview.chromium.org/2627083002
Cr-Commit-Position: refs/heads/master@{#446480}
Committed: https://chromium.googlesource.com/chromium/src/+/5064664f98bd980318e9233e2d52a534e1dd2f4e
Patch Set 1 #Patch Set 2 : Add policy generation support #
Total comments: 10
Patch Set 3 : Generalize default_for_enterprise_users to support all lists. #
Dependent Patchsets: Messages
Total messages: 29 (17 generated)
Description was changed from ========== cros: Ensure quick unlock is disabled by default for enterprise policy. BUG=680161 ========== to ========== cros: Ensure quick unlock is disabled by default for enterprise policy. BUG=680161 ==========
jdufault@chromium.org changed reviewers: + tnagel@chromium.org
jdufault@chromium.org changed reviewers: + sammiequon@chromium.org
tnagel@, sammiequon@, PTAL. Small change that ensures quick unlock is disabled by default.
On 2017/01/11 17:16:27, jdufault wrote: > tnagel@, sammiequon@, PTAL. Small change that ensures quick unlock is disabled > by default. * on enterprise enrolled devices
The CQ bit was checked by jdufault@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/2627083002/diff/20001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2627083002/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:9270: 'default_for_enterprise_users': [], Should this be a boolean value?
https://codereview.chromium.org/2627083002/diff/20001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2627083002/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:9270: 'default_for_enterprise_users': [], On 2017/01/12 20:01:20, sammiequon wrote: > Should this be a boolean value? Example value is a list? It looks like the C++ code which handles it also assumes it is a list[1]. 1: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/quick_unlo...
lgtm Btw, pastarmovj@ was the reviewer for the other quick unlock policy CLs. https://codereview.chromium.org/2627083002/diff/20001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2627083002/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:9270: 'default_for_enterprise_users': [], On 2017/01/12 20:38:47, jdufault wrote: > On 2017/01/12 20:01:20, sammiequon wrote: > > Should this be a boolean value? > > Example value is a list? It looks like the C++ code which handles it also > assumes it is a list[1]. > > 1: > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/quick_unlo... Ah, that's how it works, I just saw all the other instances in this file had a boolean.
jdufault@chromium.org changed reviewers: + pastarmovj@chromium.org, sammiequon@google.com - sammiequon@chromium.org, tnagel@chromium.org
pastarmovj@ PTAL; this will need to get merged into m57. (tnagel@ to CC)
https://codereview.chromium.org/2627083002/diff/20001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2627083002/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:9270: 'default_for_enterprise_users': [], On 2017/01/12 20:42:29, sammiequon wrote: > On 2017/01/12 20:38:47, jdufault wrote: > > On 2017/01/12 20:01:20, sammiequon wrote: > > > Should this be a boolean value? > > > > Example value is a list? It looks like the C++ code which handles it also > > assumes it is a list[1]. > > > > 1: > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/quick_unlo... > > Ah, that's how it works, I just saw all the other instances in this file had a > boolean. Just making sure you understand the implications of this line is that the user will not be able to enable any of the quick unlock modes if the device is enrolled. It's not just the initial state but the only state. If this is what you want you are good. :) https://codereview.chromium.org/2627083002/diff/20001/components/policy/tools... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/2627083002/diff/20001/components/policy/tools... components/policy/tools/generate_policy_source.py:749: elif (policy.policy_type == 'Type::LIST' and If I get this right this code will only work for empty lists right? Any other case is not handled. Since you are adding this case I would appreciate if it covers non-empty lists too. I know it will be more involved than the other cases above but I think we should strive for correctness here. Also in the spirit of the results of the last Googlegeist I shall ask you to write some tests for this code especially since your case will be more complex than the trivial ones above. :)
tnagel@chromium.org changed reviewers: + tnagel@chromium.org
https://codereview.chromium.org/2627083002/diff/20001/components/policy/tools... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/2627083002/diff/20001/components/policy/tools... components/policy/tools/generate_policy_source.py:749: elif (policy.policy_type == 'Type::LIST' and +1 to that.
The CQ bit was checked by jdufault@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/2627083002/diff/20001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2627083002/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:9270: 'default_for_enterprise_users': [], On 2017/01/23 23:38:12, pastarmovj wrote: > On 2017/01/12 20:42:29, sammiequon wrote: > > On 2017/01/12 20:38:47, jdufault wrote: > > > On 2017/01/12 20:01:20, sammiequon wrote: > > > > Should this be a boolean value? > > > > > > Example value is a list? It looks like the C++ code which handles it also > > > assumes it is a list[1]. > > > > > > 1: > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/quick_unlo... > > > > Ah, that's how it works, I just saw all the other instances in this file had a > > boolean. > > Just making sure you understand the implications of this line is that the user > will not be able to enable any of the quick unlock modes if the device is > enrolled. It's not just the initial state but the only state. If this is what > you want you are good. :) Here's the desired behavior: the user is enterprise enrolled, admin console has not been updated/no prefs have been pulled. All quick unlock modes are disabled. For enterprise users, quick unlock needs to be explicitly enabled by the admin. https://codereview.chromium.org/2627083002/diff/20001/components/policy/tools... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/2627083002/diff/20001/components/policy/tools... components/policy/tools/generate_policy_source.py:749: elif (policy.policy_type == 'Type::LIST' and I've extended the logic, but I don't see a relatively reasonable way to test this. The existing tests just load up existing policy (generated via the normal compile cycle) and check if the default value is set (ie, e2e test). I don't see any support for explicitly invoking this python file and giving it arbitrary input. While testing would be nice, it looks like a much bigger task than what this CL should encompass.
lgtm https://codereview.chromium.org/2627083002/diff/20001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2627083002/diff/20001/components/policy/resou... components/policy/resources/policy_templates.json:9270: 'default_for_enterprise_users': [], On 2017/01/25 01:32:43, jdufault wrote: > On 2017/01/23 23:38:12, pastarmovj wrote: > > On 2017/01/12 20:42:29, sammiequon wrote: > > > On 2017/01/12 20:38:47, jdufault wrote: > > > > On 2017/01/12 20:01:20, sammiequon wrote: > > > > > Should this be a boolean value? > > > > > > > > Example value is a list? It looks like the C++ code which handles it also > > > > assumes it is a list[1]. > > > > > > > > 1: > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/quick_unlo... > > > > > > Ah, that's how it works, I just saw all the other instances in this file had > a > > > boolean. > > > > Just making sure you understand the implications of this line is that the user > > will not be able to enable any of the quick unlock modes if the device is > > enrolled. It's not just the initial state but the only state. If this is what > > you want you are good. :) > > Here's the desired behavior: the user is enterprise enrolled, admin console has > not been updated/no prefs have been pulled. All quick unlock modes are disabled. > > For enterprise users, quick unlock needs to be explicitly enabled by the admin. Acknowledged. https://codereview.chromium.org/2627083002/diff/20001/components/policy/tools... File components/policy/tools/generate_policy_source.py (right): https://codereview.chromium.org/2627083002/diff/20001/components/policy/tools... components/policy/tools/generate_policy_source.py:749: elif (policy.policy_type == 'Type::LIST' and On 2017/01/25 01:32:43, jdufault wrote: > I've extended the logic, but I don't see a relatively reasonable way to test > this. The existing tests just load up existing policy (generated via the normal > compile cycle) and check if the default value is set (ie, e2e test). I don't see > any support for explicitly invoking this python file and giving it arbitrary > input. > > While testing would be nice, it looks like a much bigger task than what this CL > should encompass. I think the easiest (and complete) way to test this is to write a python test file similar to the PRESUBMIT_test.py in src/ that will invoke this script pass it a minimal json file and verify the output matches the expected result. I think it is indeed a good idea to do this in a separate CL (optimally tracked by a separate bug). However I will appreciate it if you would bootstrap this effort now that you have spent some time with this code.
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sammiequon@chromium.org Link to the patchset: https://codereview.chromium.org/2627083002/#ps40001 (title: "Generalize default_for_enterprise_users to support all lists.")
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": 40001, "attempt_start_ts": 1485467322371980,
"parent_rev": "d7de40d188cb377011ad81c9b82b814752c9124a", "commit_rev":
"5064664f98bd980318e9233e2d52a534e1dd2f4e"}
Message was sent while issue was closed.
Description was changed from ========== cros: Ensure quick unlock is disabled by default for enterprise policy. BUG=680161 ========== to ========== cros: Ensure quick unlock is disabled by default for enterprise policy. BUG=680161 Review-Url: https://codereview.chromium.org/2627083002 Cr-Commit-Position: refs/heads/master@{#446480} Committed: https://chromium.googlesource.com/chromium/src/+/5064664f98bd980318e9233e2d52... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/5064664f98bd980318e9233e2d52... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
