Set HexSSID in network config before matching it against policies

Set HexSSID in network config before matching it against policies

When creating a new network configuration, the config is matched
against existing, policy configured configs. If the network is already
configured, network configuration should fail. The problem is that
policy matching compares network SSIDs using HexSSID property, which
was not required in the new network config (network configuration
creation would succeed with only SSID set). This CL ensures that the
HexSSID property gets populated before matching the configuration
against existing policies. Also, it adds a step for verifying the
user defined oncconfiguration (in particular that property types are
properly set).

BUG=705024
Review-Url: https://codereview.chromium.org/2767253006
Cr-Commit-Position: refs/heads/master@{#460018}
Committed: https://chromium.googlesource.com/chromium/src/+/8000869bbcac8cfd3e24de549d8cf07547d152a5

[tbarzic, 2017-03-24 19:29:36]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 19:30:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbarzic, 2017-03-24 19:35:17]

tbarzic@chromium.org changed reviewers:
+ stevenjb@chromium.org

[tbarzic, 2017-03-24 19:35:18]

https://codereview.chromium.org/2767253006/diff/60001/chromeos/network/manage...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/2767253006/diff/60001/chromeos/network/manage...
chromeos/network/managed_network_configuration_handler_impl.cc:398: policies =
GetPoliciesForUser("");
I'm not 100% sure this is right, but not allowing a user config to override
device policy provided config (which would be in the shared profile) seems
reasonable to me.

[stevenjb, 2017-03-24 20:23:06]

This seems reasonable, but I think we need to add coverage to
networking_private_chromeos_apitest.

Specifically, I would like to make sure that we can modify the autoconnect,
nameserver, IP, and proxy settings of a policy network where allowed (and not
when prohibited).

We may already have at least some coverage for that, but I'm not certain, and I
really don't want to break that. I realize it is kind of a pain to test it.
Welcome to working on network configuration :)

https://codereview.chromium.org/2767253006/diff/80001/chromeos/network/manage...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/2767253006/diff/80001/chromeos/network/manage...
chromeos/network/managed_network_configuration_handler_impl.cc:384: // Make user
the network is not configured through a user policy.
"Make sure"

[tbarzic, 2017-03-24 21:01:16]

On 2017/03/24 20:23:06, stevenjb wrote:
> This seems reasonable, but I think we need to add coverage to
> networking_private_chromeos_apitest.
> 
> Specifically, I would like to make sure that we can modify the autoconnect,
> nameserver, IP, and proxy settings of a policy network where allowed (and not
> when prohibited).
> 
> We may already have at least some coverage for that, but I'm not certain, and
I
> really don't want to break that. I realize it is kind of a pain to test it.
> Welcome to working on network configuration :)
> 
>
https://codereview.chromium.org/2767253006/diff/80001/chromeos/network/manage...
> File chromeos/network/managed_network_configuration_handler_impl.cc (right):
> 
>
https://codereview.chromium.org/2767253006/diff/80001/chromeos/network/manage...
> chromeos/network/managed_network_configuration_handler_impl.cc:384: // Make
user
> the network is not configured through a user policy.
> "Make sure"

OK, I'll double-check if there are tests for setting configured network's
properties. Though, networkingPrivate.setProperties behavior (which should be
used to change existing configuration) should not be affected by this change.

[stevenjb, 2017-03-24 21:18:58]

On 2017/03/24 21:01:16, tbarzic wrote:
> On 2017/03/24 20:23:06, stevenjb wrote:
> > This seems reasonable, but I think we need to add coverage to
> > networking_private_chromeos_apitest.
> > 
> > Specifically, I would like to make sure that we can modify the autoconnect,
> > nameserver, IP, and proxy settings of a policy network where allowed (and
not
> > when prohibited).
> > 
> > We may already have at least some coverage for that, but I'm not certain,
and
> I
> > really don't want to break that. I realize it is kind of a pain to test it.
> > Welcome to working on network configuration :)
> > 
> >
>
https://codereview.chromium.org/2767253006/diff/80001/chromeos/network/manage...
> > File chromeos/network/managed_network_configuration_handler_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2767253006/diff/80001/chromeos/network/manage...
> > chromeos/network/managed_network_configuration_handler_impl.cc:384: // Make
> user
> > the network is not configured through a user policy.
> > "Make sure"
> 
> OK, I'll double-check if there are tests for setting configured network's
> properties. Though, networkingPrivate.setProperties behavior (which should be
> used to change existing configuration) should not be affected by this change.

Oh, right, this is just for creating a configuration. Sorry... gardening and
distracted.

This should be fine then, but we probably should still add a createNetwork test
that attempts to overwrite an existing policy network (which should now fail).

[tbarzic, 2017-03-24 23:58:04]

On 2017/03/24 21:18:58, stevenjb wrote:
> On 2017/03/24 21:01:16, tbarzic wrote:
> > On 2017/03/24 20:23:06, stevenjb wrote:
> > > This seems reasonable, but I think we need to add coverage to
> > > networking_private_chromeos_apitest.
> > > 
> > > Specifically, I would like to make sure that we can modify the
autoconnect,
> > > nameserver, IP, and proxy settings of a policy network where allowed (and
> not
> > > when prohibited).
> > > 
> > > We may already have at least some coverage for that, but I'm not certain,
> and
> > I
> > > really don't want to break that. I realize it is kind of a pain to test
it.
> > > Welcome to working on network configuration :)
> > > 
> > >
> >
>
https://codereview.chromium.org/2767253006/diff/80001/chromeos/network/manage...
> > > File chromeos/network/managed_network_configuration_handler_impl.cc
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2767253006/diff/80001/chromeos/network/manage...
> > > chromeos/network/managed_network_configuration_handler_impl.cc:384: //
Make
> > user
> > > the network is not configured through a user policy.
> > > "Make sure"
> > 
> > OK, I'll double-check if there are tests for setting configured network's
> > properties. Though, networkingPrivate.setProperties behavior (which should
be
> > used to change existing configuration) should not be affected by this
change.
> 
> Oh, right, this is just for creating a configuration. Sorry... gardening and
> distracted.
> 
> This should be fine then, but we probably should still add a createNetwork
test
> that attempts to overwrite an existing policy network (which should now fail).

This is covered by unittests I've added, but I guess I can add a browser test,
too.

[tbarzic, 2017-03-27 17:50:38]

On 2017/03/24 23:58:04, tbarzic wrote:
> On 2017/03/24 21:18:58, stevenjb wrote:
> > On 2017/03/24 21:01:16, tbarzic wrote:
> > > On 2017/03/24 20:23:06, stevenjb wrote:
> > > > This seems reasonable, but I think we need to add coverage to
> > > > networking_private_chromeos_apitest.
> > > > 
> > > > Specifically, I would like to make sure that we can modify the
> autoconnect,
> > > > nameserver, IP, and proxy settings of a policy network where allowed
(and
> > not
> > > > when prohibited).
> > > > 
> > > > We may already have at least some coverage for that, but I'm not
certain,
> > and
> > > I
> > > > really don't want to break that. I realize it is kind of a pain to test
> it.
> > > > Welcome to working on network configuration :)
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2767253006/diff/80001/chromeos/network/manage...
> > > > File chromeos/network/managed_network_configuration_handler_impl.cc
> (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2767253006/diff/80001/chromeos/network/manage...
> > > > chromeos/network/managed_network_configuration_handler_impl.cc:384: //
> Make
> > > user
> > > > the network is not configured through a user policy.
> > > > "Make sure"
> > > 
> > > OK, I'll double-check if there are tests for setting configured network's
> > > properties. Though, networkingPrivate.setProperties behavior (which should
> be
> > > used to change existing configuration) should not be affected by this
> change.
> > 
> > Oh, right, this is just for creating a configuration. Sorry... gardening and
> > distracted.
> > 
> > This should be fine then, but we probably should still add a createNetwork
> test
> > that attempts to overwrite an existing policy network (which should now
fail).
> 
> This is covered by unittests I've added, but I guess I can add a browser test,
> too.

Added a browser tests, and few more unittests

[stevenjb, 2017-03-27 18:01:34]

Thanks for the extra tests! lgtm.

https://codereview.chromium.org/2767253006/diff/160001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/networking_private/chromeos/test.js
(right):

https://codereview.chromium.org/2767253006/diff/160001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:904: },
nit: move this immediately after 'createNetwork()' so that we can see that both
success and failure are tested (and maybe name this createNetworkFor... to be
consistent).

[tbarzic, 2017-03-27 18:32:33]

https://codereview.chromium.org/2767253006/diff/80001/chromeos/network/manage...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/2767253006/diff/80001/chromeos/network/manage...
chromeos/network/managed_network_configuration_handler_impl.cc:384: // Make user
the network is not configured through a user policy.
On 2017/03/24 20:23:06, stevenjb wrote:
> "Make sure"

Done.

https://codereview.chromium.org/2767253006/diff/160001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/networking_private/chromeos/test.js
(right):

https://codereview.chromium.org/2767253006/diff/160001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/networking_private/chromeos/test.js:904: },
On 2017/03/27 18:01:34, stevenjb wrote:
> nit: move this immediately after 'createNetwork()' so that we can see that
both
> success and failure are tested (and maybe name this createNetworkFor... to be
> consistent).

Done.

[tbarzic, 2017-03-27 18:36:24]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 18:37:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 18:52:52]

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

[commit-bot: I haz the power, 2017-03-27 18:52:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tbarzic, 2017-03-27 19:03:56]

The CQ bit was checked by tbarzic@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 19:05:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbarzic, 2017-03-27 19:12:43]

tbarzic@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[tbarzic, 2017-03-27 19:12:44]

Had to add a (direct) dependency for unit_tests -> components/onc to make
trybots pass, so + rdevlin.cronin for BUILD.gn

[commit-bot: I haz the power, 2017-03-27 20:30:17]

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

[commit-bot: I haz the power, 2017-03-27 20:30:18]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-03-28 01:34:40]

extensions/browser/BUILD.gn lgtm

https://codereview.chromium.org/2767253006/diff/240001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc
(right):

https://codereview.chromium.org/2767253006/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc:556:
"{ \"NetworkConfigurations\": ["
Optional drive-by: This seems like a delightful time to use raw string literals.
:)

(#6 of http://en.cppreference.com/w/cpp/language/string_literal, e.g.
R"({
      "NetworkConfigurations": [{
        "GUID": "stub_wifi2",
...
   })"

https://codereview.chromium.org/2767253006/diff/240001/extensions/browser/BUI...
File extensions/browser/BUILD.gn (right):

https://codereview.chromium.org/2767253006/diff/240001/extensions/browser/BUI...
extensions/browser/BUILD.gn:475: "//components/onc:onc",
nit: s/onc:onc/onc

[tbarzic, 2017-03-28 04:00:22]

https://codereview.chromium.org/2767253006/diff/240001/chrome/browser/extensi...
File
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc
(right):

https://codereview.chromium.org/2767253006/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/api/networking_private/networking_private_chromeos_apitest.cc:556:
"{ \"NetworkConfigurations\": ["
On 2017/03/28 01:34:40, Devlin wrote:
> Optional drive-by: This seems like a delightful time to use raw string
literals.
> :)
> 
> (#6 of http://en.cppreference.com/w/cpp/language/string_literal, e.g.
> R"({
>       "NetworkConfigurations": [{
>         "GUID": "stub_wifi2",
> ...
>    })"

Done.

https://codereview.chromium.org/2767253006/diff/240001/extensions/browser/BUI...
File extensions/browser/BUILD.gn (right):

https://codereview.chromium.org/2767253006/diff/240001/extensions/browser/BUI...
extensions/browser/BUILD.gn:475: "//components/onc:onc",
On 2017/03/28 01:34:40, Devlin wrote:
> nit: s/onc:onc/onc

Done.

[tbarzic, 2017-03-28 04:37:08]

The CQ bit was checked by tbarzic@chromium.org

[tbarzic, 2017-03-28 04:37:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2767253006/#ps330001
(title: ".")

[commit-bot: I haz the power, 2017-03-28 04:37:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 05:41:26]

CQ is committing da patch.

Bot data: {"patchset_id": 330001, "attempt_start_ts": 1490675828676200,
"parent_rev": "a548ad0c0108bc06d5e622acfc89777bcb00ee23", "commit_rev":
"8000869bbcac8cfd3e24de549d8cf07547d152a5"}

[commit-bot: I haz the power, 2017-03-28 05:42:57]

Description was changed from

==========
Set HexSSID in network config before matching it against policies

When creating a new network configuration, the config is matched
against existing, policy configured configs. If the network is already
configured, network configuration should fail. The problem is that
policy matching compares network SSIDs using HexSSID property, which
was not required in the new network config (network configuration
creation would succeed with only SSID set). This CL ensures that the
HexSSID property gets populated before matching the configuration
against existing policies. Also, it adds a step for verifying the
user defined oncconfiguration (in particular that property types are
properly set).

BUG=705024
==========

to

==========
Set HexSSID in network config before matching it against policies

When creating a new network configuration, the config is matched
against existing, policy configured configs. If the network is already
configured, network configuration should fail. The problem is that
policy matching compares network SSIDs using HexSSID property, which
was not required in the new network config (network configuration
creation would succeed with only SSID set). This CL ensures that the
HexSSID property gets populated before matching the configuration
against existing policies. Also, it adds a step for verifying the
user defined oncconfiguration (in particular that property types are
properly set).

BUG=705024

Review-Url: https://codereview.chromium.org/2767253006
Cr-Commit-Position: refs/heads/master@{#460018}
Committed:
https://chromium.googlesource.com/chromium/src/+/8000869bbcac8cfd3e24de549d8c...
==========

[commit-bot: I haz the power, 2017-03-28 05:42:58]

Committed patchset #18 (id:330001) as
https://chromium.googlesource.com/chromium/src/+/8000869bbcac8cfd3e24de549d8c...

[mdjones, 2017-03-28 16:39:43]

A revert of this CL (patchset #18 id:330001) has been created in
https://codereview.chromium.org/2776403002/ by mdjones@chromium.org.

The reason for reverting is: Breaking content_browsertests on Marshmallow 64 bit
tester:
https://uberchromegw.corp.google.com/i/chromium.android/builders/Marshmallow%....