Add ManagementMode enum and GetManagementMode().

Add ManagementMode enum and GetManagementMode().

GetManagementMode() checks PolicyData to see if the device is enterprise-managed, etc. based on the management_mode field and request_token if management_mode is not set.

Also, refactor EnrollmentHandlerChromeOS to use ManagementMode.

BUG=chromium:436026
TEST=unit_tests

Committed: https://crrev.com/8edac011a427f0c0180c17a5437b642a3ad016c5
Cr-Commit-Position: refs/heads/master@{#306124}

[davidyu, 2014-11-26 03:36:28]

davidyu@chromium.org changed reviewers:
+ bartfab@chromium.org, mnissler@chromium.org

[davidyu, 2014-11-26 03:36:28]

I found three places that check the existence of request token to decide if the
device is enterprise-managed. All three places get PolicyData from different
sources: one from DeviceCloudPolicyStore; one from DeviceSettingsService; and
the other one simply gets it from somewhere else. So I add a new class
ProfileUtil so that it can be used in all three places.

There are other places that check the existence of request token, but in those
places the value of the token is required and it has nothing to do with the
management mode, so I didn't touch those code.

[Mattias Nissler (ping if slow), 2014-11-26 13:52:42]

https://codereview.chromium.org/742513006/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/policy_util.h (right):

https://codereview.chromium.org/742513006/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_util.h:16: class PolicyUtil {
We should just make helper function outside a class for this (unless I've missed
a style discussion on the mailing list that established a dummy class is the
only true way now).

https://codereview.chromium.org/742513006/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_util.h:21: static bool IsConsumerManaged(
Can we make an enum and have a single GetManagementMode() function instead? If
we do so, all the code should probably also move to
cloud_policy_constants.{h,cc} as that contains similar functionality.

[davidyu, 2014-11-27 04:00:35]

https://codereview.chromium.org/742513006/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/policy_util.h (right):

https://codereview.chromium.org/742513006/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_util.h:16: class PolicyUtil {
On 2014/11/26 13:52:42, Mattias Nissler wrote:
> We should just make helper function outside a class for this (unless I've
missed
> a style discussion on the mailing list that established a dummy class is the
> only true way now).

Done.

https://codereview.chromium.org/742513006/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/policy_util.h:21: static bool IsConsumerManaged(
On 2014/11/26 13:52:42, Mattias Nissler wrote:
> Can we make an enum and have a single GetManagementMode() function instead? If
> we do so, all the code should probably also move to
> cloud_policy_constants.{h,cc} as that contains similar functionality.

Done.

[Mattias Nissler (ping if slow), 2014-11-27 08:37:31]

https://codereview.chromium.org/742513006/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/policy_util.h (right):

https://codereview.chromium.org/742513006/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/policy_util.h:16: GetManagementMode(const
enterprise_management::PolicyData& policy_data);
Two suggestions:
1. I think moving this over to cloud_policy_constants.h would be better. This
isn't a constant of course, but that file already contains similar helpers, so
people would look there I guess.
2. Using the fully-qualified ManagementMode type from the proto is rather
unwieldy - how about putting an alias / copy in cloud_policy_constants, so I can
just write MANAGEMENT_MODE_LOCAL_OWNER within the policy namespace?

[davidyu, 2014-11-27 10:04:09]

https://codereview.chromium.org/742513006/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/policy_util.h (right):

https://codereview.chromium.org/742513006/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/policy_util.h:16: GetManagementMode(const
enterprise_management::PolicyData& policy_data);
On 2014/11/27 08:37:31, Mattias Nissler wrote:
> Two suggestions:
> 1. I think moving this over to cloud_policy_constants.h would be better. This
> isn't a constant of course, but that file already contains similar helpers, so
> people would look there I guess.
> 2. Using the fully-qualified ManagementMode type from the proto is rather
> unwieldy - how about putting an alias / copy in cloud_policy_constants, so I
can
> just write MANAGEMENT_MODE_LOCAL_OWNER within the policy namespace?

Ok. I added a new enum. I also refactored EnrollmentHandlerChromeOS to use this
new enum.

[Mattias Nissler (ping if slow), 2014-11-28 09:21:18]

LGTM w/ nits.

https://codereview.chromium.org/742513006/diff/40001/components/policy/core/c...
File components/policy/core/common/cloud/cloud_policy_constants.h (right):

https://codereview.chromium.org/742513006/diff/40001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_constants.h:138: // An enum
that indicates if a device that is local owned, enterprise-managed,
suggestion: s/that is local owned,/has a local owner, is/

https://codereview.chromium.org/742513006/diff/40001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_constants.h:141:
MANAGEMENT_MODE_LOCAL = 0,
for consistency, can we rename this to MANAGEMENT_MODE_LOCAL_OWNER?

[bartfab (slow), 2014-11-28 13:56:54]

lgtm

https://codereview.chromium.org/742513006/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc (right):

https://codereview.chromium.org/742513006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:233:
DeviceManagementService* service = NULL;
Nit: s/NULL/nullptr/

https://codereview.chromium.org/742513006/diff/40001/components/policy/core/c...
File components/policy/core/common/cloud/cloud_policy_constants.h (right):

https://codereview.chromium.org/742513006/diff/40001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_constants.h:138: // An enum
that indicates if a device that is local owned, enterprise-managed,
On 2014/11/28 09:21:18, Mattias Nissler wrote:
> suggestion: s/that is local owned,/has a local owner, is/

I think that naming will still be wrong. Consumer management does not preclude a
local owner. In fact, it requires it. It should be more something like:

* Enterprise ownership with remote management
* Consumer ownership with remote management
* Consumer ownership with local management only

The same goes for the constants, really.

https://codereview.chromium.org/742513006/diff/40001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_constants.h:146: // If
|management_mode| is set in |policy_data|, it simply returns it. For
Nit: s/it/this method/

https://codereview.chromium.org/742513006/diff/40001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_constants.h:147: // legacy
policy data which don't have |management_mode|, it checks if
Nit 1: s/policy data/|policy_data|/
Nit 2: s/don't/doesn't/
Nit 3: s/|management_mode|/|management_mode| set/
Nit 4: s/it/this method/

https://codereview.chromium.org/742513006/diff/40001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_constants.h:148: //
|request_token| is set and returns a mode for backward-compatiblity.
Nit: This comment goes into a lot of detail but then falls short of explaining
the full logic. Either expand the comment to cover the full logic or shorten the
comment to not talk about it at all.

https://codereview.chromium.org/742513006/diff/40001/components/policy/core/c...
File components/policy/core/common/cloud/cloud_policy_constants_unittest.cc
(right):

https://codereview.chromium.org/742513006/diff/40001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_constants_unittest.cc:23: //
PolicyData with no |request_token| is local owned.
Nit: s/local/locally/

[davidyu, 2014-11-29 01:50:14]

https://codereview.chromium.org/742513006/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc (right):

https://codereview.chromium.org/742513006/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/device_cloud_policy_initializer.cc:233:
DeviceManagementService* service = NULL;
On 2014/11/28 13:56:53, bartfab wrote:
> Nit: s/NULL/nullptr/

Done.

https://codereview.chromium.org/742513006/diff/40001/components/policy/core/c...
File components/policy/core/common/cloud/cloud_policy_constants.h (right):

https://codereview.chromium.org/742513006/diff/40001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_constants.h:138: // An enum
that indicates if a device that is local owned, enterprise-managed,
On 2014/11/28 09:21:18, Mattias Nissler wrote:
> suggestion: s/that is local owned,/has a local owner, is/

Done.

https://codereview.chromium.org/742513006/diff/40001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_constants.h:138: // An enum
that indicates if a device that is local owned, enterprise-managed,
On 2014/11/28 13:56:54, bartfab wrote:
> On 2014/11/28 09:21:18, Mattias Nissler wrote:
> > suggestion: s/that is local owned,/has a local owner, is/
> 
> I think that naming will still be wrong. Consumer management does not preclude
a
> local owner. In fact, it requires it. It should be more something like:
> 
> * Enterprise ownership with remote management
> * Consumer ownership with remote management
> * Consumer ownership with local management only
> 
> The same goes for the constants, really.

I think it's hard to have a precise name while keeping it short. We have some
comments in the protocol buffer. I added a reference here to that proto file so
that readers can find the comments if they want to know what each mode means.

https://codereview.chromium.org/742513006/diff/40001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_constants.h:141:
MANAGEMENT_MODE_LOCAL = 0,
On 2014/11/28 09:21:18, Mattias Nissler wrote:
> for consistency, can we rename this to MANAGEMENT_MODE_LOCAL_OWNER?

Done.

https://codereview.chromium.org/742513006/diff/40001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_constants.h:146: // If
|management_mode| is set in |policy_data|, it simply returns it. For
On 2014/11/28 13:56:53, bartfab wrote:
> Nit: s/it/this method/

Done.

https://codereview.chromium.org/742513006/diff/40001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_constants.h:147: // legacy
policy data which don't have |management_mode|, it checks if
On 2014/11/28 13:56:54, bartfab wrote:
> Nit 1: s/policy data/|policy_data|/
> Nit 2: s/don't/doesn't/
> Nit 3: s/|management_mode|/|management_mode| set/
> Nit 4: s/it/this method/

Done.

https://codereview.chromium.org/742513006/diff/40001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_constants.h:148: //
|request_token| is set and returns a mode for backward-compatiblity.
On 2014/11/28 13:56:53, bartfab wrote:
> Nit: This comment goes into a lot of detail but then falls short of explaining
> the full logic. Either expand the comment to cover the full logic or shorten
the
> comment to not talk about it at all.

Done.

https://codereview.chromium.org/742513006/diff/40001/components/policy/core/c...
File components/policy/core/common/cloud/cloud_policy_constants_unittest.cc
(right):

https://codereview.chromium.org/742513006/diff/40001/components/policy/core/c...
components/policy/core/common/cloud/cloud_policy_constants_unittest.cc:23: //
PolicyData with no |request_token| is local owned.
On 2014/11/28 13:56:54, bartfab wrote:
> Nit: s/local/locally/

Done.

[davidyu, 2014-11-29 01:52:23]

The CQ bit was checked by davidyu@chromium.org

[commit-bot: I haz the power, 2014-11-29 01:53:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/742513006/60001

[commit-bot: I haz the power, 2014-11-29 01:56:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-29 01:56:42]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/87795)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[davidyu, 2014-11-29 02:37:00]

The CQ bit was checked by davidyu@chromium.org

[commit-bot: I haz the power, 2014-11-29 02:37:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/742513006/80001

[commit-bot: I haz the power, 2014-11-29 02:54:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-29 02:54:49]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[davidyu, 2014-11-29 14:03:10]

The CQ bit was checked by davidyu@chromium.org

[commit-bot: I haz the power, 2014-11-29 14:04:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/742513006/100001

[commit-bot: I haz the power, 2014-11-29 14:45:34]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-11-29 14:46:41]

Patchset 6 (id:??) landed as
https://crrev.com/8edac011a427f0c0180c17a5437b642a3ad016c5
Cr-Commit-Position: refs/heads/master@{#306124}