Add support for kiosk mode on the client. Make sure the settings are written in the lockbox.

Add support for kiosk mode on the client. Make sure the settings are written in the lockbox.

Propagates the registration mode through the policy subsystem and lock it down in the lockbox.
Provides interface to query the values using the EnterpriseInstallAttributes class.

BUG=chromium-os:26246
TEST=unit_tests: *Enterprise*,*Policy*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=123471

[pastarmovj, 2012-02-15 14:43:52]

@Mattias: Can you please review this CL.

@Bin,Joao: FYI.

[Mattias Nissler (ping if slow), 2012-02-15 15:32:03]

http://codereview.chromium.org/9403010/diff/5001/chrome/browser/policy/enterp...
File chrome/browser/policy/enterprise_install_attributes.cc (right):

http://codereview.chromium.org/9403010/diff/5001/chrome/browser/policy/enterp...
chrome/browser/policy/enterprise_install_attributes.cc:23: if (separator_pos !=
email.npos && separator_pos < email.length()-1)
spaces around -

http://codereview.chromium.org/9403010/diff/5001/chrome/browser/policy/enterp...
chrome/browser/policy/enterprise_install_attributes.cc:26: NOTREACHED() <<
"|user| is not a proper email address.";
Change this to "Not a proper email: " << email;

http://codereview.chromium.org/9403010/diff/5001/chrome/browser/policy/enterp...
chrome/browser/policy/enterprise_install_attributes.cc:82: NOTREACHED() <<
"Unknown device mode: " << device_mode;
a function for translating to the string would be better, since then we can have
bot the NOTREACHED and no default branch, so we get both the compile time error
and the NOTREACHED() at run time.

http://codereview.chromium.org/9403010/diff/5001/chrome/browser/policy/enterp...
chrome/browser/policy/enterprise_install_attributes.cc:135: VLOG(1) << "Calling
GetMode on non-enrolled device";
VLOG_IF

http://codereview.chromium.org/9403010/diff/5001/chrome/browser/policy/enterp...
chrome/browser/policy/enterprise_install_attributes.cc:177: NOTREACHED() <<
"Unknown enterprise mode : " << enterprise_mode;
same here, use a function for translating.

http://codereview.chromium.org/9403010/diff/5001/chrome/browser/policy/enterp...
File chrome/browser/policy/enterprise_install_attributes.h (right):

http://codereview.chromium.org/9403010/diff/5001/chrome/browser/policy/enterp...
chrome/browser/policy/enterprise_install_attributes.h:13: #include
"chrome/browser/policy/proto/device_management_backend.pb.h"
We shouldn't depend on the proto file here. I feel this is a layering violation,
as the locking the devices should be possible without knowing about all the rest
of the enterprise stuff. Can you declare an enum here to represent the mode?
Then, we should also use that in the data store and translate immediately when
we get it from the registration response.

http://codereview.chromium.org/9403010/diff/5001/chrome/browser/policy/enterp...
chrome/browser/policy/enterprise_install_attributes.h:64: // the output of this
function!
Maybe bool GetMode(DeviceMode* mode) would be a better API? Or alternatively, if
you declare the enum here, you could add a constant for consumer devices that
you return if the device is not enterprise.

[pastarmovj, 2012-02-15 17:40:14]

Addressed your comments and added a few more tests to ensure we handle migration
from old devices properly.
I would love to run some try jobs for that but it seems the CrOS try servers are
a bit overwhelmed right now. Will still queue some try jobs for those now.

http://codereview.chromium.org/9403010/diff/5001/chrome/browser/policy/enterp...
File chrome/browser/policy/enterprise_install_attributes.cc (right):

http://codereview.chromium.org/9403010/diff/5001/chrome/browser/policy/enterp...
chrome/browser/policy/enterprise_install_attributes.cc:23: if (separator_pos !=
email.npos && separator_pos < email.length()-1)
On 2012/02/15 15:32:03, Mattias Nissler wrote:
> spaces around -

Done.

http://codereview.chromium.org/9403010/diff/5001/chrome/browser/policy/enterp...
chrome/browser/policy/enterprise_install_attributes.cc:26: NOTREACHED() <<
"|user| is not a proper email address.";
On 2012/02/15 15:32:03, Mattias Nissler wrote:
> Change this to "Not a proper email: " << email;

Done.

http://codereview.chromium.org/9403010/diff/5001/chrome/browser/policy/enterp...
chrome/browser/policy/enterprise_install_attributes.cc:82: NOTREACHED() <<
"Unknown device mode: " << device_mode;
On 2012/02/15 15:32:03, Mattias Nissler wrote:
> a function for translating to the string would be better, since then we can
have
> bot the NOTREACHED and no default branch, so we get both the compile time
error
> and the NOTREACHED() at run time.

Done.

http://codereview.chromium.org/9403010/diff/5001/chrome/browser/policy/enterp...
chrome/browser/policy/enterprise_install_attributes.cc:135: VLOG(1) << "Calling
GetMode on non-enrolled device";
On 2012/02/15 15:32:03, Mattias Nissler wrote:
> VLOG_IF

Not needed.

http://codereview.chromium.org/9403010/diff/5001/chrome/browser/policy/enterp...
chrome/browser/policy/enterprise_install_attributes.cc:177: NOTREACHED() <<
"Unknown enterprise mode : " << enterprise_mode;
On 2012/02/15 15:32:03, Mattias Nissler wrote:
> same here, use a function for translating.

Done.

http://codereview.chromium.org/9403010/diff/5001/chrome/browser/policy/enterp...
File chrome/browser/policy/enterprise_install_attributes.h (right):

http://codereview.chromium.org/9403010/diff/5001/chrome/browser/policy/enterp...
chrome/browser/policy/enterprise_install_attributes.h:13: #include
"chrome/browser/policy/proto/device_management_backend.pb.h"
On 2012/02/15 15:32:03, Mattias Nissler wrote:
> We shouldn't depend on the proto file here. I feel this is a layering
violation,
> as the locking the devices should be possible without knowing about all the
rest
> of the enterprise stuff. Can you declare an enum here to represent the mode?
> Then, we should also use that in the data store and translate immediately when
> we get it from the registration response.

Done.

http://codereview.chromium.org/9403010/diff/5001/chrome/browser/policy/enterp...
chrome/browser/policy/enterprise_install_attributes.h:64: // the output of this
function!
On 2012/02/15 15:32:03, Mattias Nissler wrote:
> Maybe bool GetMode(DeviceMode* mode) would be a better API? Or alternatively,
if
> you declare the enum here, you could add a constant for consumer devices that
> you return if the device is not enterprise.

Using the local enum I can have a UNKNOWN_DEVICE value to represent not
initialized or unknown mode as well as END_USER_DEVICE mode.

[Mattias Nissler (ping if slow), 2012-02-16 10:36:27]

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/device_t...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/device_t...
chrome/browser/policy/device_token_fetcher.cc:201: }
we should do the right thing when there is a value in the field that we don't
understand. Probably the safest choice would be to report an error, since we're
probably seeing a new mode that the code doesn't support.

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
File chrome/browser/policy/enterprise_install_attributes.cc (right):

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
chrome/browser/policy/enterprise_install_attributes.cc:16: const char
kEndUserDeviceMode[] = "end_user";
s/end user/consumer/

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
chrome/browser/policy/enterprise_install_attributes.cc:50: }
put a NOTREACHED here. In fact, just move the UNKNOWN code here and fall
through.

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
chrome/browser/policy/enterprise_install_attributes.cc:77: const std::string&
device_id) {
I think we should CHECK here for device_mode != UNKNOWN

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
chrome/browser/policy/enterprise_install_attributes.cc:156: //
|registration_user_| is empty on end user devices.
s/end user/consumer/

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
File chrome/browser/policy/enterprise_install_attributes.h (right):

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
chrome/browser/policy/enterprise_install_attributes.h:32: 
comment

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
chrome/browser/policy/enterprise_install_attributes.h:34: END_USER_DEVICE,
Put a common prefix, e.g. DEVICE_MODE_ENTERPRISE etc.

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
chrome/browser/policy/enterprise_install_attributes.h:34: END_USER_DEVICE,
s/END_USER/CONSUMER/, since enterprise users are also end users :)

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
chrome/browser/policy/enterprise_install_attributes.h:45: LockResult LockDevice(
this line break should no longer be necessary

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
chrome/browser/policy/enterprise_install_attributes.h:68: // the output of this
function!
This comment is outdated, you may want to change it to indicate that this may
return UNKNOWN_DEVICE if the device is non-enterprise

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
File chrome/browser/policy/enterprise_install_attributes_unittest.cc (right):

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
chrome/browser/policy/enterprise_install_attributes_unittest.cc:102:
TEST_F(EnterpriseInstallAttributesTest, EndUserDevice) {
consumer

[pastarmovj, 2012-02-17 13:59:47]

PTAL. 
Please note that I introduced a new error state for the case of missing/unknown
enrollment mode. Another CL is in the queue that makes the UI aware of that
state - until it lands the UI will get stuck if this state is triggered.

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/device_t...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/device_t...
chrome/browser/policy/device_token_fetcher.cc:201: }
On 2012/02/16 10:36:27, Mattias Nissler wrote:
> we should do the right thing when there is a value in the field that we don't
> understand. Probably the safest choice would be to report an error, since
we're
> probably seeing a new mode that the code doesn't support.

Done.

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
File chrome/browser/policy/enterprise_install_attributes.cc (right):

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
chrome/browser/policy/enterprise_install_attributes.cc:16: const char
kEndUserDeviceMode[] = "end_user";
On 2012/02/16 10:36:27, Mattias Nissler wrote:
> s/end user/consumer/

Done.

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
chrome/browser/policy/enterprise_install_attributes.cc:50: }
On 2012/02/16 10:36:27, Mattias Nissler wrote:
> put a NOTREACHED here. In fact, just move the UNKNOWN code here and fall
> through.

Done.

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
chrome/browser/policy/enterprise_install_attributes.cc:77: const std::string&
device_id) {
On 2012/02/16 10:36:27, Mattias Nissler wrote:
> I think we should CHECK here for device_mode != UNKNOWN

Done.

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
chrome/browser/policy/enterprise_install_attributes.cc:156: //
|registration_user_| is empty on end user devices.
On 2012/02/16 10:36:27, Mattias Nissler wrote:
> s/end user/consumer/

Done.

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
File chrome/browser/policy/enterprise_install_attributes.h (right):

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
chrome/browser/policy/enterprise_install_attributes.h:32: 
On 2012/02/16 10:36:27, Mattias Nissler wrote:
> comment

Done.

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
chrome/browser/policy/enterprise_install_attributes.h:34: END_USER_DEVICE,
On 2012/02/16 10:36:27, Mattias Nissler wrote:
> Put a common prefix, e.g. DEVICE_MODE_ENTERPRISE etc.

Done.

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
chrome/browser/policy/enterprise_install_attributes.h:34: END_USER_DEVICE,
On 2012/02/16 10:36:27, Mattias Nissler wrote:
> s/END_USER/CONSUMER/, since enterprise users are also end users :)

Done.

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
chrome/browser/policy/enterprise_install_attributes.h:45: LockResult LockDevice(
On 2012/02/16 10:36:27, Mattias Nissler wrote:
> this line break should no longer be necessary

Done.

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
chrome/browser/policy/enterprise_install_attributes.h:68: // the output of this
function!
On 2012/02/16 10:36:27, Mattias Nissler wrote:
> This comment is outdated, you may want to change it to indicate that this may
> return UNKNOWN_DEVICE if the device is non-enterprise

Done. Actually for non-enterprise devices it will correctly return
DEVICE_TYPE_CONSUMER. DEVICE_TYPE_UNKNOWN means the device has not been locked
yet.

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
File chrome/browser/policy/enterprise_install_attributes_unittest.cc (right):

http://codereview.chromium.org/9403010/diff/12/chrome/browser/policy/enterpri...
chrome/browser/policy/enterprise_install_attributes_unittest.cc:102:
TEST_F(EnterpriseInstallAttributesTest, EndUserDevice) {
On 2012/02/16 10:36:27, Mattias Nissler wrote:
> consumer

Done.

[Mattias Nissler (ping if slow), 2012-02-20 12:22:54]

http://codereview.chromium.org/9403010/diff/9002/chrome/browser/policy/enterp...
File chrome/browser/policy/enterprise_install_attributes.cc (right):

http://codereview.chromium.org/9403010/diff/9002/chrome/browser/policy/enterp...
chrome/browser/policy/enterprise_install_attributes.cc:79: CHECK(device_mode !=
DEVICE_MODE_UNKNOWN);
CHECK_NE

http://codereview.chromium.org/9403010/diff/9002/chrome/browser/policy/enterp...
chrome/browser/policy/enterprise_install_attributes.cc:161: return
DEVICE_MODE_CONSUMER;
shouldn't we rather put this logic in ReadImmutableAttributes below for the case
when the attributes are actually not present?

http://codereview.chromium.org/9403010/diff/9002/chrome/browser/policy/enterp...
File chrome/browser/policy/enterprise_install_attributes.h (right):

http://codereview.chromium.org/9403010/diff/9002/chrome/browser/policy/enterp...
chrome/browser/policy/enterprise_install_attributes.h:38: DEVICE_MODE_UNKNOWN
Unknown should probably go first. We probably also want to put short comments
here indicating what the different modes mean.

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils_browsertest.cc:259:
connector_->GetDeviceCloudPolicyDataStore());
can't you just call connector_->LockDevice()?

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_data_store.h (right):

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_data_store.h:42: enum DeviceMode {
It might be a good idea to move this to cloud_policy_constants.h, given that
it's used both here and in EnterpriseInstallAttributes

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_data_store.h:43: DEVICE_MODE_CONSUMER,
short comments on what the modes mean?

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:207:
em::DeviceRegisterRequest::DEVICE) {
indent by 4

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:208: // TODO(pastarmovj): Default
to DEVICE_MODE_UNKNOWN once the DM server
s/the//

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.h (right):

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.h:84: // the client
nit: missing period.

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_install_attributes.cc (right):

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_install_attributes.cc:50: NOTREACHED() <<
"Invalid device mode.";
Add << mode to the message.

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_install_attributes.cc:157:
CloudPolicyDataStore::DeviceMode EnterpriseInstallAttributes::GetMode() {
As commented before, the code in this function should probably rather live in
ReadImmutableAttributes. Or is there a point doing this fixup over and over
again?

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_install_attributes.h (right):

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_install_attributes.h:13: #include
"chrome/browser/policy/cloud_policy_data_store.h"
Let's move the DeviceMode enum to policy constants, so we don't depend on the
data store.

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_install_attributes.h:14: #include
"chrome/browser/policy/proto/device_management_backend.pb.h"
needed? I hope not.

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/polic...
File chrome/browser/policy/policy_notifier.cc (right):

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/polic...
chrome/browser/policy/policy_notifier.cc:53: e[TOKEN_FETCHER] ==
CloudPolicySubsystem::BAD_ENROLLMENT_MODE)) {
+1 space indentation

[pastarmovj, 2012-02-20 14:26:30]

PTAL.
1. Addressed your comments.
2. Added two new tests to the DeviceTokenFetcher suite to verify enterprise vs.
user registration behavior.
3. Cleaned up stale includes.

http://codereview.chromium.org/9403010/diff/9002/chrome/browser/policy/enterp...
File chrome/browser/policy/enterprise_install_attributes.cc (right):

http://codereview.chromium.org/9403010/diff/9002/chrome/browser/policy/enterp...
chrome/browser/policy/enterprise_install_attributes.cc:79: CHECK(device_mode !=
DEVICE_MODE_UNKNOWN);
On 2012/02/20 12:22:54, Mattias Nissler wrote:
> CHECK_NE

Done.

http://codereview.chromium.org/9403010/diff/9002/chrome/browser/policy/enterp...
chrome/browser/policy/enterprise_install_attributes.cc:161: return
DEVICE_MODE_CONSUMER;
On 2012/02/20 12:22:54, Mattias Nissler wrote:
> shouldn't we rather put this logic in ReadImmutableAttributes below for the
case
> when the attributes are actually not present?

Done.

http://codereview.chromium.org/9403010/diff/9002/chrome/browser/policy/enterp...
File chrome/browser/policy/enterprise_install_attributes.h (right):

http://codereview.chromium.org/9403010/diff/9002/chrome/browser/policy/enterp...
chrome/browser/policy/enterprise_install_attributes.h:38: DEVICE_MODE_UNKNOWN
On 2012/02/20 12:22:54, Mattias Nissler wrote:
> Unknown should probably go first. We probably also want to put short comments
> here indicating what the different modes mean.

Done.

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils_browsertest.cc (right):

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils_browsertest.cc:259:
connector_->GetDeviceCloudPolicyDataStore());
On 2012/02/20 12:22:54, Mattias Nissler wrote:
> can't you just call connector_->LockDevice()?

Done. As discussed offline made the accessors non-const.

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/cloud...
File chrome/browser/policy/cloud_policy_data_store.h (right):

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_data_store.h:42: enum DeviceMode {
On 2012/02/20 12:22:54, Mattias Nissler wrote:
> It might be a good idea to move this to cloud_policy_constants.h, given that
> it's used both here and in EnterpriseInstallAttributes

Done.

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/cloud...
chrome/browser/policy/cloud_policy_data_store.h:43: DEVICE_MODE_CONSUMER,
On 2012/02/20 12:22:54, Mattias Nissler wrote:
> short comments on what the modes mean?

Done.

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:207:
em::DeviceRegisterRequest::DEVICE) {
On 2012/02/20 12:22:54, Mattias Nissler wrote:
> indent by 4

Done.

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:208: // TODO(pastarmovj): Default
to DEVICE_MODE_UNKNOWN once the DM server
On 2012/02/20 12:22:54, Mattias Nissler wrote:
> s/the//

Done.

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.h (right):

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.h:84: // the client
On 2012/02/20 12:22:54, Mattias Nissler wrote:
> nit: missing period.

Done.

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_install_attributes.cc (right):

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_install_attributes.cc:50: NOTREACHED() <<
"Invalid device mode.";
On 2012/02/20 12:22:54, Mattias Nissler wrote:
> Add << mode to the message.

Done.

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_install_attributes.cc:157:
CloudPolicyDataStore::DeviceMode EnterpriseInstallAttributes::GetMode() {
On 2012/02/20 12:22:54, Mattias Nissler wrote:
> As commented before, the code in this function should probably rather live in
> ReadImmutableAttributes. Or is there a point doing this fixup over and over
> again?

Done.

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_install_attributes.h (right):

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_install_attributes.h:13: #include
"chrome/browser/policy/cloud_policy_data_store.h"
On 2012/02/20 12:22:54, Mattias Nissler wrote:
> Let's move the DeviceMode enum to policy constants, so we don't depend on the
> data store.

Done.

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_install_attributes.h:14: #include
"chrome/browser/policy/proto/device_management_backend.pb.h"
On 2012/02/20 12:22:54, Mattias Nissler wrote:
> needed? I hope not.

Done.

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/polic...
File chrome/browser/policy/policy_notifier.cc (right):

http://codereview.chromium.org/9403010/diff/18004/chrome/browser/policy/polic...
chrome/browser/policy/policy_notifier.cc:53: e[TOKEN_FETCHER] ==
CloudPolicySubsystem::BAD_ENROLLMENT_MODE)) {
On 2012/02/20 12:22:54, Mattias Nissler wrote:
> +1 space indentation

Done.

[Mattias Nissler (ping if slow), 2012-02-21 10:37:45]

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/brows...
File chrome/browser/policy/browser_policy_connector.h (right):

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.h:126: 
Didn't you want to put a comment here saying that access to these is read-only
except for tests?

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:77: // internally throughout
ChromeOS to represent different device modes.
remove "throughout ChromeOS", since this code is compiled on all platforms.

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:136: // User token
fetches does not have to carry enrollment type.
That comment is misleading, since the test is really about device registrations.
Say something like "For device registrations, the fetcher needs to determine
device mode."

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:141:
data_store->AddObserver(&observer_);
do you actually need the observer for this test? I guess not.

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:168:
data_store->AddObserver(&observer_);
ditto

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:179: "fake_auth_token",
true);
It looks like this setup code is now duplicated 3 times for the most part. Any
chance to consolidate?

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_install_attributes.cc (right):

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_install_attributes.cc:24: const char
kAttrEnterpriseDeviceId[] = "enterprise.device_id";
alphabetize?

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_install_attributes.cc:157: 
nit: I'd remove this blank line.

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_install_attributes.cc:178: registration_user_ =
enterprise_user;
nit: blank line before comment?

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_install_attributes.cc:181: registration_mode_ =
DEVICE_MODE_ENTERPRISE;
nit: blank line before comment?

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_install_attributes.cc:184: // present.
add something like "which may happen for devices that have been enrolled in
pre-R19 revisions of the code when not all of the current attributes were
written."

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_install_attributes.cc:198: registration_mode_ =
DEVICE_MODE_CONSUMER;
now we return CONSUMER even the InstallAttributesGet() call fails. I guess we
should split the condition above to first read the attrs. If that fails, do
nothing. If successful, look at the attrs to determine whether we're enterprise
or consumer.

[pastarmovj, 2012-02-21 14:57:07]

Addressed comments.

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/brows...
File chrome/browser/policy/browser_policy_connector.h (right):

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/brows...
chrome/browser/policy/browser_policy_connector.h:126: 
On 2012/02/21 10:37:46, Mattias Nissler wrote:
> Didn't you want to put a comment here saying that access to these is read-only
> except for tests?

I was kind of unsure whether putting there this comment would make much sense.
As we discussed offline - we should rather fix the general design here than
putting access control in comments. :) I put some comment in here however.

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher.cc:77: // internally throughout
ChromeOS to represent different device modes.
On 2012/02/21 10:37:46, Mattias Nissler wrote:
> remove "throughout ChromeOS", since this code is compiled on all platforms.

Done.

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/devic...
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:136: // User token
fetches does not have to carry enrollment type.
On 2012/02/21 10:37:46, Mattias Nissler wrote:
> That comment is misleading, since the test is really about device
registrations.
> Say something like "For device registrations, the fetcher needs to determine
> device mode."

Yep. I removed the comment here and put it down at the point I EXPECT that the
mode has been set.

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:141:
data_store->AddObserver(&observer_);
On 2012/02/21 10:37:46, Mattias Nissler wrote:
> do you actually need the observer for this test? I guess not.

Done.

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:168:
data_store->AddObserver(&observer_);
On 2012/02/21 10:37:46, Mattias Nissler wrote:
> ditto

Done.

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/devic...
chrome/browser/policy/device_token_fetcher_unittest.cc:179: "fake_auth_token",
true);
On 2012/02/21 10:37:46, Mattias Nissler wrote:
> It looks like this setup code is now duplicated 3 times for the most part. Any
> chance to consolidate?

Did some consolidation. Could push it further but I think this will make future
changes in single tests rather harder.

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_install_attributes.cc (right):

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_install_attributes.cc:24: const char
kAttrEnterpriseDeviceId[] = "enterprise.device_id";
On 2012/02/21 10:37:46, Mattias Nissler wrote:
> alphabetize?

Done.

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_install_attributes.cc:157: 
On 2012/02/21 10:37:46, Mattias Nissler wrote:
> nit: I'd remove this blank line.

Done.

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_install_attributes.cc:178: registration_user_ =
enterprise_user;
On 2012/02/21 10:37:46, Mattias Nissler wrote:
> nit: blank line before comment?

Done.

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_install_attributes.cc:181: registration_mode_ =
DEVICE_MODE_ENTERPRISE;
On 2012/02/21 10:37:46, Mattias Nissler wrote:
> nit: blank line before comment?

Done.

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_install_attributes.cc:184: // present.
On 2012/02/21 10:37:46, Mattias Nissler wrote:
> add something like "which may happen for devices that have been enrolled in
> pre-R19 revisions of the code when not all of the current attributes were
> written."

Done.

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_install_attributes.cc:198: registration_mode_ =
DEVICE_MODE_CONSUMER;
On 2012/02/21 10:37:46, Mattias Nissler wrote:
> now we return CONSUMER even the InstallAttributesGet() call fails. I guess we
> should split the condition above to first read the attrs. If that fails, do
> nothing. If successful, look at the attrs to determine whether we're
enterprise
> or consumer.

We lock the lock box empty for consumer devices so this call will fail anyways.
If we are here however we know that we have properly locked, valid lockbox so
missing username is what we have always used to distinguish consumer from
enterprise devices (see EnterpriseInstallAttributes::IsEnterpriseDevice()). 

Of course we could further verify that the |enterprise_owned| string is not
"true" and I will add this check but it is not going to improve our confidence
significantly.

[Mattias Nissler (ping if slow), 2012-02-23 10:57:26]

LGTM with nits.

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_install_attributes.cc (right):

http://codereview.chromium.org/9403010/diff/21005/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_install_attributes.cc:198: registration_mode_ =
DEVICE_MODE_CONSUMER;
On 2012/02/21 14:57:08, pastarmovj wrote:
> On 2012/02/21 10:37:46, Mattias Nissler wrote:
> > now we return CONSUMER even the InstallAttributesGet() call fails. I guess
we
> > should split the condition above to first read the attrs. If that fails, do
> > nothing. If successful, look at the attrs to determine whether we're
> enterprise
> > or consumer.
> 
> We lock the lock box empty for consumer devices so this call will fail
anyways.
> If we are here however we know that we have properly locked, valid lockbox so
> missing username is what we have always used to distinguish consumer from
> enterprise devices (see EnterpriseInstallAttributes::IsEnterpriseDevice()). 
> 
> Of course we could further verify that the |enterprise_owned| string is not
> "true" and I will add this check but it is not going to improve our confidence
> significantly.

I was missing the detail that InstallAttributesGet will return false for missing
attributes, so this is OK.

http://codereview.chromium.org/9403010/diff/29005/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_install_attributes.cc (right):

http://codereview.chromium.org/9403010/diff/29005/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_install_attributes.cc:186: // the code, when
those new attributes were added.
s/when/before/

Also s/those/these/ (twice)

[pastarmovj, 2012-02-24 12:47:50]

Fixed the nitty nit and proceeding to commit.

http://codereview.chromium.org/9403010/diff/29005/chrome/browser/policy/enter...
File chrome/browser/policy/enterprise_install_attributes.cc (right):

http://codereview.chromium.org/9403010/diff/29005/chrome/browser/policy/enter...
chrome/browser/policy/enterprise_install_attributes.cc:186: // the code, when
those new attributes were added.
On 2012/02/23 10:57:27, Mattias Nissler wrote:
> s/when/before/
> 
> Also s/those/these/ (twice)

Done.