Autoconnect policy for CrOS.

Autoconnect policy for CrOS.

This adds an autoconnect policy that disables autoconnect of unmanaged networks. As a device policy this applies to all shared networks. As a user policy it applies to all networks of this user.

With this commit the policy is applied on each restart and login. UI lockdown is still missing.

Configurations affecting several networks were not supported previously by ONC. Therefore, this commit adds the new toplevel section "GlobalNetworkConfiguration" to ONC.

BUG=280146
For API change:

R=bartfab@chromium.org
TBR=eroman@chromium.org

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

[pneubeck (no reviews), 2013-10-15 10:08:03]

Please take a look. Thanks!

[pneubeck (no reviews), 2013-10-16 12:38:19]

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/policy_a...
File chromeos/network/policy_applicator.cc (right):

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/policy_a...
chromeos/network/policy_applicator.cc:264:
entry_properties.GetBooleanWithoutPathExpansion(shill::kAutoConnectProperty,
if entry doesn't exist, we have to set to false explicitly. Otherwise Shill
might silently switch it to true.

[bartfab (slow), 2013-10-16 12:40:30]

https://codereview.chromium.org/23526016/diff/86001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/configuration_policy_handler_chromeos.cc
(right):

https://codereview.chromium.org/23526016/diff/86001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/configuration_policy_handler_chromeos.cc:116: //
Currently, only the per network configuration is stored in a pref. Ignore
Nit: s/per network/per-network/

https://codereview.chromium.org/23526016/diff/86001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_configuration_updater.h (right):

https://codereview.chromium.org/23526016/diff/86001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater.h:17: class Value;
Nit: Forward-declare ListValue and DictionaryValue.

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/client_c...
File chromeos/network/client_cert_resolver_unittest.cc (right):

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/client_c...
chromeos/network/client_cert_resolver_unittest.cc:193: base::DictionaryValue()
/* no global network config */);
Nit: #include "base/values.h"

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.h (right):

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.h:110: virtual void
SetPolicy(
Nit: All other parameters are documented. For consistency, document
|global_network_config| as well.

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_impl.cc:110: ~Policies()
{ STLDeleteValues(&per_network_config); }
The style guide says:

"Simple accessors should generally be the only inline functions."

By my reading, that applies to structs as well, forbidding inline destructors.

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_impl.cc:441:
policies->per_network_config,
Since you have a struct that groups per-network configs and global network
config, would it make sense to pass around that struct instead of its
constituent members?

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler_impl.h (right):

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_impl.h:22: class
DictionaryValue;
Nit: This forward-declaration is no longer needed.

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_validator.h (right):

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.h:223: bool
GlobalNetworkConfigInUserImport(const base::DictionaryValue& onc_object);
Nit: Could these methods that return a bool be prefixed with "Is" or "Has"?

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/policy_a...
File chromeos/network/policy_applicator.cc (right):

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/policy_a...
chromeos/network/policy_applicator.cc:247: void
PolicyApplicator::GetPropertiesForUnmanagedEntry(
Could you comment this method a bit? It was not clear to me how it works until
we discussed it offline. I am also not sure whether
!properties_to_update.empty() is the best way to signal that an update is
necessary. Maybe the method could return a bool to explicitly indicate an update
is needed?

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/policy_a...
chromeos/network/policy_applicator.cc:255: return; // Autoconnect for Ethernet
cannot be configured.
Nit: There should be two spaces before a trailing comment.

[pneubeck (no reviews), 2013-10-17 10:22:26]

https://codereview.chromium.org/23526016/diff/86001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/configuration_policy_handler_chromeos.cc
(right):

https://codereview.chromium.org/23526016/diff/86001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/configuration_policy_handler_chromeos.cc:116: //
Currently, only the per network configuration is stored in a pref. Ignore
On 2013/10/16 12:40:31, bartfab wrote:
> Nit: s/per network/per-network/

Done.

https://codereview.chromium.org/23526016/diff/86001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_configuration_updater.h (right):

https://codereview.chromium.org/23526016/diff/86001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater.h:17: class Value;
On 2013/10/16 12:40:31, bartfab wrote:
> Nit: Forward-declare ListValue and DictionaryValue.

Done.

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/client_c...
File chromeos/network/client_cert_resolver_unittest.cc (right):

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/client_c...
chromeos/network/client_cert_resolver_unittest.cc:193: base::DictionaryValue()
/* no global network config */);
On 2013/10/16 12:40:31, bartfab wrote:
> Nit: #include "base/values.h"

Done.

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.h (right):

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.h:110: virtual void
SetPolicy(
On 2013/10/16 12:40:31, bartfab wrote:
> Nit: All other parameters are documented. For consistency, document
> |global_network_config| as well.

Done.

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_impl.cc:110: ~Policies()
{ STLDeleteValues(&per_network_config); }
On 2013/10/16 12:40:31, bartfab wrote:
> The style guide says:
> 
> "Simple accessors should generally be the only inline functions."
> 
> By my reading, that applies to structs as well, forbidding inline destructors.

Hm. Whether this applies to .cc local classes as well?
Done.

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_impl.cc:441:
policies->per_network_config,
On 2013/10/16 12:40:31, bartfab wrote:
> Since you have a struct that groups per-network configs and global network
> config, would it make sense to pass around that struct instead of its
> constituent members?

I think it's not worth it for two arguments only, because then the question of
where to declare that struct arises.

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler_impl.h (right):

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_impl.h:22: class
DictionaryValue;
On 2013/10/16 12:40:31, bartfab wrote:
> Nit: This forward-declaration is no longer needed.

used at least in line 116, which is not an OVERRIDE.

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_validator.h (right):

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_validator.h:223: bool
GlobalNetworkConfigInUserImport(const base::DictionaryValue& onc_object);
On 2013/10/16 12:40:31, bartfab wrote:
> Nit: Could these methods that return a bool be prefixed with "Is" or "Has"?

Done.

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/policy_a...
File chromeos/network/policy_applicator.cc (right):

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/policy_a...
chromeos/network/policy_applicator.cc:247: void
PolicyApplicator::GetPropertiesForUnmanagedEntry(
On 2013/10/16 12:40:31, bartfab wrote:
> Could you comment this method a bit? It was not clear to me how it works until
> we discussed it offline. I am also not sure whether
> !properties_to_update.empty() is the best way to signal that an update is
> necessary. Maybe the method could return a bool to explicitly indicate an
update
> is needed?

Done.

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/policy_a...
chromeos/network/policy_applicator.cc:255: return; // Autoconnect for Ethernet
cannot be configured.
On 2013/10/16 12:40:31, bartfab wrote:
> Nit: There should be two spaces before a trailing comment.

Done.

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/policy_a...
chromeos/network/policy_applicator.cc:264:
entry_properties.GetBooleanWithoutPathExpansion(shill::kAutoConnectProperty,
On 2013/10/16 12:38:20, pneubeck wrote:
> if entry doesn't exist, we have to set to false explicitly. Otherwise Shill
> might silently switch it to true.

Done.

[pneubeck (no reviews), 2013-10-17 11:48:14]

@David, FYI

[bartfab (slow), 2013-10-17 11:57:58]

lgtm with one more nit

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/23526016/diff/86001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler_impl.cc:110: ~Policies()
{ STLDeleteValues(&per_network_config); }
On 2013/10/17 10:22:26, pneubeck wrote:
> On 2013/10/16 12:40:31, bartfab wrote:
> > The style guide says:
> > 
> > "Simple accessors should generally be the only inline functions."
> > 
> > By my reading, that applies to structs as well, forbidding inline
destructors.
> 
> Hm. Whether this applies to .cc local classes as well?
> Done.

Yes. There was a discussion about this on chromium-dev@ recently. Someone asked
why we have huge classes full of inline methods in tests. After some discussion,
the conclusion was that the style guide prohibits this and nobody should be
doing it. The style guide applies to all code, not just header files.

https://codereview.chromium.org/23526016/diff/108001/chromeos/network/policy_...
File chromeos/network/policy_applicator.h (right):

https://codereview.chromium.org/23526016/diff/108001/chromeos/network/policy_...
chromeos/network/policy_applicator.h:80: // networks by the global network
config of the policy. |entry_properties| are
Nit: The comment talks about "unmanaged networks" in the plural but the
properties it returns are actually applicable to the specific network whose
|entry_properties| were provided as an input.

[David Roche, 2013-10-17 18:52:26]

https://codereview.chromium.org/23526016/diff/108001/chromeos/test/data/netwo...
File chromeos/test/data/network/repaired_toplevel_partially_invalid.onc (right):

https://codereview.chromium.org/23526016/diff/108001/chromeos/test/data/netwo...
chromeos/test/data/network/repaired_toplevel_partially_invalid.onc:51:
"AllowAutoconnect": false
Is the name of this field accurate?  The ONC-defined networks can still be
auto-connected to, right? Instead define this field as an enum, with the values
ALL, ONC_ONLY, NEVER?

[pneubeck (no reviews), 2013-10-18 08:01:26]

On 2013/10/17 18:52:26, David Roche wrote:
>
https://codereview.chromium.org/23526016/diff/108001/chromeos/test/data/netwo...
> File chromeos/test/data/network/repaired_toplevel_partially_invalid.onc
(right):
> 
>
https://codereview.chromium.org/23526016/diff/108001/chromeos/test/data/netwo...
> chromeos/test/data/network/repaired_toplevel_partially_invalid.onc:51:
> "AllowAutoconnect": false
> Is the name of this field accurate?  The ONC-defined networks can still be
> auto-connected to, right? Instead define this field as an enum, with the
values
> ALL, ONC_ONLY, NEVER?

What's the use of overwriting policy defined autoconnect? At least this doesn't
add any expressiveness and is pure "syntactic sugar" which could be implemented
e.g. in CPanel if it were really required (I didn't here of any request of this
kind).

We could change the name to "AllowUserNetworksAutoconnect".
Or instead extend the ONC spec with a clarifying description. I'm still unsure
whether we should put these policy/enterprise only features into the current ONC
spec or into a separate document. I have a slight preference of the former.

[pneubeck (no reviews), 2013-10-18 19:53:23]

On 2013/10/18 08:01:26, pneubeck wrote:
> On 2013/10/17 18:52:26, David Roche wrote:
> >
>
https://codereview.chromium.org/23526016/diff/108001/chromeos/test/data/netwo...
> > File chromeos/test/data/network/repaired_toplevel_partially_invalid.onc
> (right):
> > 
> >
>
https://codereview.chromium.org/23526016/diff/108001/chromeos/test/data/netwo...
> > chromeos/test/data/network/repaired_toplevel_partially_invalid.onc:51:
> > "AllowAutoconnect": false
> > Is the name of this field accurate?  The ONC-defined networks can still be
> > auto-connected to, right? Instead define this field as an enum, with the
> values
> > ALL, ONC_ONLY, NEVER?
> 
> What's the use of overwriting policy defined autoconnect? At least this
doesn't
> add any expressiveness and is pure "syntactic sugar" which could be
implemented
> e.g. in CPanel if it were really required (I didn't here of any request of
this
> kind).
> 
> We could change the name to "AllowUserNetworksAutoconnect".
> Or instead extend the ONC spec with a clarifying description. I'm still unsure
> whether we should put these policy/enterprise only features into the current
ONC
> spec or into a separate document. I have a slight preference of the former.

ping.

[David Roche, 2013-10-19 01:09:21]

On 2013/10/18 19:53:23, pneubeck wrote:
> On 2013/10/18 08:01:26, pneubeck wrote:
> > On 2013/10/17 18:52:26, David Roche wrote:
> > >
> >
>
https://codereview.chromium.org/23526016/diff/108001/chromeos/test/data/netwo...
> > > File chromeos/test/data/network/repaired_toplevel_partially_invalid.onc
> > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/23526016/diff/108001/chromeos/test/data/netwo...
> > > chromeos/test/data/network/repaired_toplevel_partially_invalid.onc:51:
> > > "AllowAutoconnect": false
> > > Is the name of this field accurate?  The ONC-defined networks can still be
> > > auto-connected to, right? Instead define this field as an enum, with the
> > values
> > > ALL, ONC_ONLY, NEVER?
> > 
> > What's the use of overwriting policy defined autoconnect? At least this
> doesn't
> > add any expressiveness and is pure "syntactic sugar" which could be
> implemented
> > e.g. in CPanel if it were really required (I didn't here of any request of
> this
> > kind).
> > 
> > We could change the name to "AllowUserNetworksAutoconnect".
> > Or instead extend the ONC spec with a clarifying description. I'm still
unsure
> > whether we should put these policy/enterprise only features into the current
> ONC
> > spec or into a separate document. I have a slight preference of the former.
> 
> ping.

The boolean field that was discussed in chat is fine instead of the enum
proposal.

It looks like the tweaks we discussed haven't been made yet?  The test ONC files
don't seem to be updated with the field name that was agreed.

[commit-bot: I haz the power, 2013-10-21 18:33:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/23526016/197001

[commit-bot: I haz the power, 2013-10-21 19:29:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pneubeck@chromium.org/23526016/197001

[pneubeck (no reviews), 2013-10-21 21:21:15]

Committed patchset #4 manually as r229910 (presubmit successful).