Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2902183002:
Improve determination of managed state in DeviceSettingsProvider (Closed)
|
Created:
3 years, 7 months ago by Thiemo Nagel Modified:
3 years, 6 months ago CC:
chromium-reviews, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImprove determination of managed state in DeviceSettingsProvider
Use PolicyData::management_mode to determine whether the device has a
local owner and only fall back to DM token in case management_mode is
unset. That way, the correct determination is guaranteed for both
cloud and Active Directory management. (Currently, the code is not
broken because AD policy doesn't include a user name, but that might
change in the future.)
Also update the PolicyData::management_mode documentation to include
Active Directory.
BUG=722799
Review-Url: https://codereview.chromium.org/2902183002
Cr-Commit-Position: refs/heads/master@{#476239}
Committed: https://chromium.googlesource.com/chromium/src/+/92db6b2c14f28d8339590ea4c6a0186d9bbf08c5
Patch Set 1 #Patch Set 2 : Improved proto docs #Patch Set 3 : Minor improvements #
Total comments: 12
Patch Set 4 : Addressed comments #
Total comments: 2
Patch Set 5 : Refactor logic to make it more readable #
Messages
Total messages: 32 (21 generated)
The CQ bit was checked by tnagel@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...
Description was changed from ========== Improve determination of managed state in DeviceSettingsProvider Use PolicyData::management_mode to determine whether the device has a local owner and only fall back to DM token in case management_mode is unset. That way, the correct determination is guaranteed for both cloud and Active Directory management. (Currently, the code is not broken because AD policy doesn't include a user name, but that might change in the future.) BUG=722799 ========== to ========== Improve determination of managed state in DeviceSettingsProvider Use PolicyData::management_mode to determine whether the device has a local owner and only fall back to DM token in case management_mode is unset. That way, the correct determination is guaranteed for both cloud and Active Directory management. (Currently, the code is not broken because AD policy doesn't include a user name, but that might change in the future.) Also update the PolicyData::management_mode documentation to include Active Directory. BUG=722799 ==========
tnagel@chromium.org changed reviewers: + ljusten@chromium.org
Hey Lutz, could you please take a look? Thank you, Thiemo
The CQ bit was checked by tnagel@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 with nits (but I'm not a reviewer). https://codereview.chromium.org/2902183002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/2902183002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/settings/device_settings_provider.cc:745: // Determine whether device is unmanaged. See PolicyData::management_mode Nit: Remove one space. I don't think it's common to have 2 spaces after a full stop. https://codereview.chromium.org/2902183002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/settings/device_settings_provider.cc:747: bool unmanaged = false; Nit: Split this off into a separate IsEnterpriseManaged function and reverse logic to match other code (i.e. check for managed, not unmanaged). https://codereview.chromium.org/2902183002/diff/40001/components/policy/proto... File components/policy/proto/device_management_backend.proto (right): https://codereview.chromium.org/2902183002/diff/40001/components/policy/proto... components/policy/proto/device_management_backend.proto:374: // The device is enterprise-managed (either via cloud or through Active Nit: I guess at some point we might support Azure AD servers as well, so I'd suggest to refer to DM Server instead of 'cloud'.
The CQ bit was checked by tnagel@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...
On 2017/05/29 12:43:02, ljusten (instantaneous) wrote: > LGTM with nits Thank you! > (but I'm not a reviewer). Not a committer.
tnagel@chromium.org changed reviewers: + rsorokin@chromium.org
Hey Roman, could you please take a look? Thank you! Thiemo https://codereview.chromium.org/2902183002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/2902183002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/settings/device_settings_provider.cc:745: // Determine whether device is unmanaged. See PolicyData::management_mode On 2017/05/29 12:43:01, ljusten (instantaneous) wrote: > Nit: Remove one space. I don't think it's common to have 2 spaces after a full > stop. Agreed. There is some usage across the codebase, but not in this file. https://codereview.chromium.org/2902183002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/settings/device_settings_provider.cc:747: bool unmanaged = false; On 2017/05/29 12:43:01, ljusten (instantaneous) wrote: > Nit: Split this off into a separate IsEnterpriseManaged function and reverse > logic to match other code (i.e. check for managed, not unmanaged). Which other code would that match? I don't think there's any other use of management_mode. https://codereview.chromium.org/2902183002/diff/40001/components/policy/proto... File components/policy/proto/device_management_backend.proto (right): https://codereview.chromium.org/2902183002/diff/40001/components/policy/proto... components/policy/proto/device_management_backend.proto:374: // The device is enterprise-managed (either via cloud or through Active On 2017/05/29 12:43:01, ljusten (instantaneous) wrote: > Nit: I guess at some point we might support Azure AD servers as well, so I'd > suggest to refer to DM Server instead of 'cloud'. Valid point, I guess. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2902183002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/2902183002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/settings/device_settings_provider.cc:747: bool unmanaged = false; why not "managed = true" ? Negative name is confusing :)
https://codereview.chromium.org/2902183002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/2902183002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/settings/device_settings_provider.cc:745: // Determine whether device is unmanaged. See PolicyData::management_mode On 2017/05/29 15:07:16, Thiemo Nagel wrote: > On 2017/05/29 12:43:01, ljusten (instantaneous) wrote: > > Nit: Remove one space. I don't think it's common to have 2 spaces after a full > > stop. > > Agreed. There is some usage across the codebase, but not in this file. Aaaarggghhhh! https://codereview.chromium.org/2902183002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/settings/device_settings_provider.cc:747: bool unmanaged = false; On 2017/05/29 15:07:16, Thiemo Nagel wrote: > On 2017/05/29 12:43:01, ljusten (instantaneous) wrote: > > Nit: Split this off into a separate IsEnterpriseManaged function and reverse > > logic to match other code (i.e. check for managed, not unmanaged). > > Which other code would that match? I don't think there's any other use of > management_mode. CL:518045/CL:506159 for instance. Granted, not the same repo, but we should be consistent while we're at it. https://codereview.chromium.org/2902183002/diff/40001/components/policy/proto... File components/policy/proto/device_management_backend.proto (right): https://codereview.chromium.org/2902183002/diff/40001/components/policy/proto... components/policy/proto/device_management_backend.proto:374: // The device is enterprise-managed (either via cloud or through Active On 2017/05/29 15:07:16, Thiemo Nagel wrote: > On 2017/05/29 12:43:01, ljusten (instantaneous) wrote: > > Nit: I guess at some point we might support Azure AD servers as well, so I'd > > suggest to refer to DM Server instead of 'cloud'. > > Valid point, I guess. Thanks. IMHO, we should rename cloud policy to user policy everywhere. It confused the hell out of me that we always refer to it as user policy, but it's called cloud policy. Device policy, which is also coming from the cloud, is not called cloud policy...
The CQ bit was checked by tnagel@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...
Roman, Lutz, thank you for your comments. I've tried to make the code more readable. Could you please take another look? Thank you! Thiemo https://codereview.chromium.org/2902183002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/2902183002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/settings/device_settings_provider.cc:745: // Determine whether device is unmanaged. See PolicyData::management_mode On 2017/05/30 08:55:10, ljusten (instantaneous) wrote: > On 2017/05/29 15:07:16, Thiemo Nagel wrote: > > On 2017/05/29 12:43:01, ljusten (instantaneous) wrote: > > > Nit: Remove one space. I don't think it's common to have 2 spaces after a > full > > > stop. > > > > Agreed. There is some usage across the codebase, but not in this file. > > Aaaarggghhhh! Acknowledged. https://codereview.chromium.org/2902183002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/settings/device_settings_provider.cc:747: bool unmanaged = false; On 2017/05/30 08:55:10, ljusten (instantaneous) wrote: > On 2017/05/29 15:07:16, Thiemo Nagel wrote: > > On 2017/05/29 12:43:01, ljusten (instantaneous) wrote: > > > Nit: Split this off into a separate IsEnterpriseManaged function and reverse > > > logic to match other code (i.e. check for managed, not unmanaged). > > > > Which other code would that match? I don't think there's any other use of > > management_mode. > > CL:518045/CL:506159 for instance. Granted, not the same repo, but we should be > consistent while we're at it. Done. https://codereview.chromium.org/2902183002/diff/40001/components/policy/proto... File components/policy/proto/device_management_backend.proto (right): https://codereview.chromium.org/2902183002/diff/40001/components/policy/proto... components/policy/proto/device_management_backend.proto:374: // The device is enterprise-managed (either via cloud or through Active On 2017/05/30 08:55:10, ljusten (instantaneous) wrote: > On 2017/05/29 15:07:16, Thiemo Nagel wrote: > > On 2017/05/29 12:43:01, ljusten (instantaneous) wrote: > > > Nit: I guess at some point we might support Azure AD servers as well, so I'd > > > suggest to refer to DM Server instead of 'cloud'. > > > > Valid point, I guess. Thanks. > > IMHO, we should rename cloud policy to user policy everywhere. It confused the > hell out of me that we always refer to it as user policy, but it's called cloud > policy. Device policy, which is also coming from the cloud, is not called cloud > policy... Note that it's possible to have non-cloud user policy on non-CrOS platforms, e.g. GPO on Windows, JSON on Linux, ... If there's something you'd like to clarify, I'm happy to review CLs. :) https://codereview.chromium.org/2902183002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/2902183002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/settings/device_settings_provider.cc:747: bool unmanaged = false; On 2017/05/29 16:19:53, Roman Sorokin (ftl) wrote: > why not "managed = true" ? > > Negative name is confusing :) Done.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/30 12:29:58, Roman Sorokin (ftl) wrote: > lgtm Thank you!
The CQ bit was checked by tnagel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ljusten@chromium.org Link to the patchset: https://codereview.chromium.org/2902183002/#ps80001 (title: "Refactor logic to make it more readable")
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": 80001, "attempt_start_ts": 1496307362094530,
"parent_rev": "2c935aff0c6cfb6c1863d3f875f46ba6100031ae", "commit_rev":
"92db6b2c14f28d8339590ea4c6a0186d9bbf08c5"}
Message was sent while issue was closed.
Description was changed from ========== Improve determination of managed state in DeviceSettingsProvider Use PolicyData::management_mode to determine whether the device has a local owner and only fall back to DM token in case management_mode is unset. That way, the correct determination is guaranteed for both cloud and Active Directory management. (Currently, the code is not broken because AD policy doesn't include a user name, but that might change in the future.) Also update the PolicyData::management_mode documentation to include Active Directory. BUG=722799 ========== to ========== Improve determination of managed state in DeviceSettingsProvider Use PolicyData::management_mode to determine whether the device has a local owner and only fall back to DM token in case management_mode is unset. That way, the correct determination is guaranteed for both cloud and Active Directory management. (Currently, the code is not broken because AD policy doesn't include a user name, but that might change in the future.) Also update the PolicyData::management_mode documentation to include Active Directory. BUG=722799 Review-Url: https://codereview.chromium.org/2902183002 Cr-Commit-Position: refs/heads/master@{#476239} Committed: https://chromium.googlesource.com/chromium/src/+/92db6b2c14f28d8339590ea4c6a0... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/92db6b2c14f28d8339590ea4c6a0... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
