Add device policy definition for local accounts.

Add device policy definition for local accounts.

BUG=chromium-os:35715
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168236

[pastarmovj, 2012-11-06 12:42:14]

I'm good in general. Some comments inline before I stamp it.

http://codereview.chromium.org/11358113/diff/1/chrome/app/policy/policy_templ...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/11358113/diff/1/chrome/app/policy/policy_templ...
chrome/app/policy/policy_templates.json:3045: # TODO(joaodasilva): Make this the
default and deprecate the other proxy
proxy policies?

http://codereview.chromium.org/11358113/diff/1/chrome/browser/chromeos/settin...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

http://codereview.chromium.org/11358113/diff/1/chrome/browser/chromeos/settin...
chrome/browser/chromeos/settings/device_settings_provider.cc:216:
em::LocalAccountsProto * local_accounts =
one space too much.

http://codereview.chromium.org/11358113/diff/1/chrome/browser/chromeos/settin...
chrome/browser/chromeos/settings/device_settings_provider.cc:225: if
(!(*entry)->GetAsDictionary(&entry_dict) ||
Isn't the logic inverted here?
If not read then set in the proto_entry else not reached is how i read it now?

[Mattias Nissler (ping if slow), 2012-11-06 13:55:09]

You correctly figured I didn't even try this ;) I'm working at this moment on
follow-up CLs that will actually make use of this. I mainly uploaded it now for
discussing with Bin and Raymond.

On 2012/11/06 12:42:14, pastarmovj wrote:
> I'm good in general. Some comments inline before I stamp it.
> 
>
http://codereview.chromium.org/11358113/diff/1/chrome/app/policy/policy_templ...
> File chrome/app/policy/policy_templates.json (right):
> 
>
http://codereview.chromium.org/11358113/diff/1/chrome/app/policy/policy_templ...
> chrome/app/policy/policy_templates.json:3045: # TODO(joaodasilva): Make this
the
> default and deprecate the other proxy
> proxy policies?
> 
>
http://codereview.chromium.org/11358113/diff/1/chrome/browser/chromeos/settin...
> File chrome/browser/chromeos/settings/device_settings_provider.cc (right):
> 
>
http://codereview.chromium.org/11358113/diff/1/chrome/browser/chromeos/settin...
> chrome/browser/chromeos/settings/device_settings_provider.cc:216:
> em::LocalAccountsProto * local_accounts =
> one space too much.
> 
>
http://codereview.chromium.org/11358113/diff/1/chrome/browser/chromeos/settin...
> chrome/browser/chromeos/settings/device_settings_provider.cc:225: if
> (!(*entry)->GetAsDictionary(&entry_dict) ||
> Isn't the logic inverted here?
> If not read then set in the proto_entry else not reached is how i read it now?

[Bin, 2012-11-07 19:17:03]

http://codereview.chromium.org/11358113/diff/1/chrome/app/policy/policy_templ...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/11358113/diff/1/chrome/app/policy/policy_templ...
chrome/app/policy/policy_templates.json:3049: 'name': 'DeviceLocalAccounts',
What's this for? I assume it is a device-level policy. But that's are defined in
chrome_device_policy.proto

[Mattias Nissler (ping if slow), 2012-11-12 15:50:51]

http://codereview.chromium.org/11358113/diff/1/chrome/app/policy/policy_templ...
File chrome/app/policy/policy_templates.json (right):

http://codereview.chromium.org/11358113/diff/1/chrome/app/policy/policy_templ...
chrome/app/policy/policy_templates.json:3045: # TODO(joaodasilva): Make this the
default and deprecate the other proxy
On 2012/11/06 12:42:15, pastarmovj wrote:
> proxy policies?

Done.

http://codereview.chromium.org/11358113/diff/1/chrome/app/policy/policy_templ...
chrome/app/policy/policy_templates.json:3049: 'name': 'DeviceLocalAccounts',
On 2012/11/07 19:17:04, binzhao1 wrote:
> What's this for? I assume it is a device-level policy. But that's are defined
in
> chrome_device_policy.proto

This is just for allocating a policy constant (which is necessary to have stuff
show up in about:policy).

http://codereview.chromium.org/11358113/diff/1/chrome/browser/chromeos/settin...
File chrome/browser/chromeos/settings/device_settings_provider.cc (right):

http://codereview.chromium.org/11358113/diff/1/chrome/browser/chromeos/settin...
chrome/browser/chromeos/settings/device_settings_provider.cc:216:
em::LocalAccountsProto * local_accounts =
On 2012/11/06 12:42:15, pastarmovj wrote:
> one space too much.

Done.

http://codereview.chromium.org/11358113/diff/1/chrome/browser/chromeos/settin...
chrome/browser/chromeos/settings/device_settings_provider.cc:225: if
(!(*entry)->GetAsDictionary(&entry_dict) ||
On 2012/11/06 12:42:15, pastarmovj wrote:
> Isn't the logic inverted here?
> If not read then set in the proto_entry else not reached is how i read it now?

Done.

[pastarmovj, 2012-11-12 15:55:23]

lgtm

[Mattias Nissler (ping if slow), 2012-11-13 17:32:24]

So, I've updated the names per our discussions and getting more serious with
actually intending to commit this. Bin, Julian, please take another look.

[Bin, 2012-11-13 20:03:49]

http://codereview.chromium.org/11358113/diff/9/chrome/browser/policy/proto/ch...
File chrome/browser/policy/proto/chrome_device_policy.proto (right):

http://codereview.chromium.org/11358113/diff/9/chrome/browser/policy/proto/ch...
chrome/browser/policy/proto/chrome_device_policy.proto:227: // e.g.
user@example.com. Policy-configured device-local accounts are on the
account_id will be in the format of email address, but it may contains illegal
characters for email address (e.g enterprise-configured public account). 

Can you add this to the comment to make it clear?

http://codereview.chromium.org/11358113/diff/9/chrome/browser/policy/proto/ch...
chrome/browser/policy/proto/chrome_device_policy.proto:229: repeated string
account_id = 1;
Should we define a singular DeviceLocalAccountProto here and defined a repeated
DeviceLocalAccountProto in ChromeDeviceSettingsProto? I am okay if we wrap
another layer on top of repeated DeviceLocalAccountProto.

If we ever want to put any additional fields into one single DeviceLocalAccount,
then it is hard to add in the current scheme.

[Mattias Nissler (ping if slow), 2012-11-14 13:42:48]

Bin, please take another look.

https://codereview.chromium.org/11358113/diff/9/chrome/browser/policy/proto/c...
File chrome/browser/policy/proto/chrome_device_policy.proto (right):

https://codereview.chromium.org/11358113/diff/9/chrome/browser/policy/proto/c...
chrome/browser/policy/proto/chrome_device_policy.proto:227: // e.g.
user@example.com. Policy-configured device-local accounts are on the
On 2012/11/13 20:03:49, binzhao1 wrote:
> account_id will be in the format of email address, but it may contains illegal
> characters for email address (e.g enterprise-configured public account). 
> 
> Can you add this to the comment to make it clear?

Done.

https://codereview.chromium.org/11358113/diff/9/chrome/browser/policy/proto/c...
chrome/browser/policy/proto/chrome_device_policy.proto:229: repeated string
account_id = 1;
On 2012/11/13 20:03:49, binzhao1 wrote:
> Should we define a singular DeviceLocalAccountProto here and defined a
repeated
> DeviceLocalAccountProto in ChromeDeviceSettingsProto? I am okay if we wrap
> another layer on top of repeated DeviceLocalAccountProto.
> 
> If we ever want to put any additional fields into one single
DeviceLocalAccount,
> then it is hard to add in the current scheme.

I prefer not to make ChromeDeviceSettingsProto.device_local_accounts repeated,
because there are probably going to be a few global device-local account
settings (auto-login for example).

Regarding adding more fields, you have a point. I figured that a simple repeated
string field would be easier to handle when I adjusted this yesterday. Then
again, better safe than sorry. Restored the old version.

[pastarmovj, 2012-11-14 13:59:30]

I spotted two small issues please fix them or correct me if i am wrong.

https://codereview.chromium.org/11358113/diff/12003/chrome/app/policy/policy_...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/11358113/diff/12003/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:3073: 'example_value': [ { "Id":
"demo@example.com", "Name": "Demo session" } ],
This example is not accurate anymore please update it.

https://codereview.chromium.org/11358113/diff/12003/chrome/browser/policy/dev...
File chrome/browser/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/11358113/diff/12003/chrome/browser/policy/dev...
chrome/browser/policy/device_policy_decoder_chromeos.cc:119: if
(!policy.has_device_local_accounts()) {
if NOT policy has field ?!

[Bin, 2012-11-14 21:30:07]

lgtm

LGTM on the proto change.

[Mattias Nissler (ping if slow), 2012-11-16 14:54:31]

Julian, I fixed the mistakes. I can haz a stamp please?

https://codereview.chromium.org/11358113/diff/12003/chrome/app/policy/policy_...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/11358113/diff/12003/chrome/app/policy/policy_...
chrome/app/policy/policy_templates.json:3073: 'example_value': [ { "Id":
"demo@example.com", "Name": "Demo session" } ],
On 2012/11/14 13:59:30, pastarmovj wrote:
> This example is not accurate anymore please update it.

Good catch. Done.

https://codereview.chromium.org/11358113/diff/12003/chrome/browser/policy/dev...
File chrome/browser/policy/device_policy_decoder_chromeos.cc (right):

https://codereview.chromium.org/11358113/diff/12003/chrome/browser/policy/dev...
chrome/browser/policy/device_policy_decoder_chromeos.cc:119: if
(!policy.has_device_local_accounts()) {
On 2012/11/14 13:59:30, pastarmovj wrote:
> if NOT policy has field ?!

Done. It's just for the about:policy UI ;) I've verified now that it works as
intended.

[pastarmovj, 2012-11-16 15:27:25]

Cool thanks for fixing it! LGTM

[commit-bot: I haz the power, 2012-11-16 15:29:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnissler@chromium.org/11358113/14001

[commit-bot: I haz the power, 2012-11-16 17:36:44]

Change committed as 168236