Add an Ethernet EAP policy.

Add an Ethernet EAP policy.

This is the last change required to enable the Ethernet EAP policy on ChromeOS.

BUG=126870
(For preferences_browsertest.cc)
TBR=nkostylev@chromium.org

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

[pneubeck (no reviews), 2013-09-16 12:13:31]

This depends on the other CLs 23510004, 23506040 and 23609047.

Bartosz, please take again a look at 
  chrome/browser/**
  chromeos/network/{policy_*, managed_network_configuration_handler_unittest.cc}

Steven, ptal at
  chromeos/network/{network_state*, device_state*}

[stevenjb, 2013-09-16 23:53:24]

https://codereview.chromium.org/23451044/diff/7009/chromeos/network/network_s...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/23451044/diff/7009/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:368: const NetworkStateBase* ethernet)
const {
I don't especially like passing a NetworkState* back to NSH. We should just pass
the service_path and look up the NetworkState.

https://codereview.chromium.org/23451044/diff/7009/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:611: if
(ethernet_service->update_received() ||
If this is false, then there should be an update in progress, and we should wait
for that update to trigger any notifications. What change to NetworkState are we
intending to signal here?

[bartfab (slow), 2013-09-17 08:45:54]

chrome/browser/*
chromeos/network/policy_*
chromeos/network/managed_network_configuration_handler_unittest.cc

LGTM

https://codereview.chromium.org/23451044/diff/7009/chromeos/network/managed_n...
File chromeos/network/managed_network_configuration_handler_unittest.cc (right):

https://codereview.chromium.org/23451044/diff/7009/chromeos/network/managed_n...
chromeos/network/managed_network_configuration_handler_unittest.cc:318:
scoped_ptr<base::DictionaryValue> expected_shill_properties =
Nit: #include "base/memory/scoped_ptr.h"

https://codereview.chromium.org/23451044/diff/7009/chromeos/network/policy_ut...
File chromeos/network/policy_util.cc (right):

https://codereview.chromium.org/23451044/diff/7009/chromeos/network/policy_ut...
chromeos/network/policy_util.cc:75: // identification. For compatibility, we
currently should stick to Shill's
Just to make sure nothing is overlooked: I presume this updates is in sync with
Shill and Shill is able to match ONC for Ethernet as well now?

https://codereview.chromium.org/23451044/diff/7009/chromeos/network/policy_ut...
chromeos/network/policy_util.cc:89: const base::DictionaryValue* policy_ethernet
= NULL;
Nit: #include "base/values.h"

https://codereview.chromium.org/23451044/diff/7009/chromeos/network/policy_ut...
chromeos/network/policy_util.cc:104: return (policy_auth == actual_auth);
Nit: No need for brackets here.

[pneubeck (no reviews), 2013-09-17 12:48:35]

https://codereview.chromium.org/23451044/diff/7009/chromeos/network/managed_n...
File chromeos/network/managed_network_configuration_handler_unittest.cc (right):

https://codereview.chromium.org/23451044/diff/7009/chromeos/network/managed_n...
chromeos/network/managed_network_configuration_handler_unittest.cc:318:
scoped_ptr<base::DictionaryValue> expected_shill_properties =
On 2013/09/17 08:45:54, bartfab wrote:
> Nit: #include "base/memory/scoped_ptr.h"

Done.

https://codereview.chromium.org/23451044/diff/7009/chromeos/network/network_s...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/23451044/diff/7009/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:368: const NetworkStateBase* ethernet)
const {
On 2013/09/16 23:53:24, stevenjb wrote:
> I don't especially like passing a NetworkState* back to NSH. We should just
pass
> the service_path and look up the NetworkState.

Done.

https://codereview.chromium.org/23451044/diff/7009/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:611: if
(ethernet_service->update_received() ||
On 2013/09/16 23:53:24, stevenjb wrote:
> If this is false, then there should be an update in progress, and we should
wait
> for that update to trigger any notifications. What change to NetworkState are
we
> intending to signal here?

The intention here is:

Sometime after you plugin the ethernet cable, Shill successfully authenticated
at the EAP server and sets this flag to true.
After that, e.g. the proxy settings have to be updated or any other connection
dependent services.

This additional code here is might not be necessary if Shill always changes the
connection status after the EAP authentication. I'm not sure about that but will
investigate further.

https://codereview.chromium.org/23451044/diff/7009/chromeos/network/policy_ut...
File chromeos/network/policy_util.cc (right):

https://codereview.chromium.org/23451044/diff/7009/chromeos/network/policy_ut...
chromeos/network/policy_util.cc:75: // identification. For compatibility, we
currently should stick to Shill's
On 2013/09/17 08:45:54, bartfab wrote:
> Just to make sure nothing is overlooked: I presume this updates is in sync
with
> Shill and Shill is able to match ONC for Ethernet as well now?

This function isn't directly Shill related because Shill doesn't know anything
about policies.

I assume, you mean if Shill accepts Ethernet configurations. Yes, at least the
ethernet eap settings are correctly handled by Shill.
Other ethernet settings that would have to be written to Shill (like IP
addresses) don't exist yet.
Note: proxy settings are not sent to Shill anymore and work for both Ethernet
with or without EAP.

https://codereview.chromium.org/23451044/diff/7009/chromeos/network/policy_ut...
chromeos/network/policy_util.cc:89: const base::DictionaryValue* policy_ethernet
= NULL;
On 2013/09/17 08:45:54, bartfab wrote:
> Nit: #include "base/values.h"

Done.

https://codereview.chromium.org/23451044/diff/7009/chromeos/network/policy_ut...
chromeos/network/policy_util.cc:104: return (policy_auth == actual_auth);
On 2013/09/17 08:45:54, bartfab wrote:
> Nit: No need for brackets here.

Done.

[stevenjb, 2013-09-17 16:22:59]

https://codereview.chromium.org/23451044/diff/7009/chromeos/network/network_s...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/23451044/diff/7009/chromeos/network/network_s...
chromeos/network/network_state_handler.cc:611: if
(ethernet_service->update_received() ||
On 2013/09/17 12:48:36, pneubeck wrote:
> On 2013/09/16 23:53:24, stevenjb wrote:
> > If this is false, then there should be an update in progress, and we should
> wait
> > for that update to trigger any notifications. What change to NetworkState
are
> we
> > intending to signal here?
> 
> The intention here is:
> 
> Sometime after you plugin the ethernet cable, Shill successfully authenticated
> at the EAP server and sets this flag to true.
> After that, e.g. the proxy settings have to be updated or any other connection
> dependent services.
> 
> This additional code here is might not be necessary if Shill always changes
the
> connection status after the EAP authentication. I'm not sure about that but
will
> investigate further.

In that case we should call
RequestUpdateForNetwork(ethernet_service->service_path), which will notify
observers if/when the changed properties have been received.

https://codereview.chromium.org/23451044/diff/29001/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/onc_utils.cc (right):

https://codereview.chromium.org/23451044/diff/29001/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/onc_utils.cc:273: (*it)->GetAsDictionary(&network);
if (!GetAsDictionary()) { ERROR, continue }

https://codereview.chromium.org/23451044/diff/29001/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/onc_utils.cc:282: &ethernet);
if (!GetDictionary...) { ERROR, continue }

https://codereview.chromium.org/23451044/diff/29001/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/onc_utils.cc:296: const std::string& network_guid) {
I really think all of these should just take either 'service_path' (or even
'guid') and look up FavoriteState here where we need it. I'd much rather do an
extra lookup then pass both 'network' and 'network_guid'. Also that way we only
need one GetPolicyForNetwork() function.

https://codereview.chromium.org/23451044/diff/29001/chromeos/network/network_...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/23451044/diff/29001/chromeos/network/network_...
chromeos/network/network_state_handler.cc:373: base::StringPrintf("Unknown
service path %s", service_path.c_str()));
nit: + instead of StringPrintf ("foo" + bar where bar is a string is supported)

[stevenjb, 2013-09-28 00:37:45]

lgtm

https://codereview.chromium.org/23451044/diff/82001/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/onc_utils.cc (right):

https://codereview.chromium.org/23451044/diff/82001/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/onc_utils.cc:284: &ethernet);
if (!ethernet) handle error

https://codereview.chromium.org/23451044/diff/82001/chromeos/network/network_...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/23451044/diff/82001/chromeos/network/network_...
chromeos/network/network_state_handler.cc:374: if (network->type() !=
flimflam::kTypeEthernet ||
Shouldn't this trigger an error?

[pneubeck (no reviews), 2013-09-30 20:08:30]

@Steven, please take a last look at the last changes to network_state_handler.cc
Thanks!

https://codereview.chromium.org/23451044/diff/29001/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/onc_utils.cc (right):

https://codereview.chromium.org/23451044/diff/29001/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/onc_utils.cc:273: (*it)->GetAsDictionary(&network);
On 2013/09/17 16:22:59, stevenjb wrote:
> if (!GetAsDictionary()) { ERROR, continue }

Done.

https://codereview.chromium.org/23451044/diff/29001/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/onc_utils.cc:282: &ethernet);
On 2013/09/17 16:22:59, stevenjb wrote:
> if (!GetDictionary...) { ERROR, continue }

Done.

https://codereview.chromium.org/23451044/diff/29001/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/onc_utils.cc:296: const std::string& network_guid) {
On 2013/09/17 16:22:59, stevenjb wrote:
> I really think all of these should just take either 'service_path' (or even
> 'guid') and look up FavoriteState here where we need it. I'd much rather do an
> extra lookup then pass both 'network' and 'network_guid'. Also that way we
only
> need one GetPolicyForNetwork() function.

Done.

https://codereview.chromium.org/23451044/diff/29001/chromeos/network/network_...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/23451044/diff/29001/chromeos/network/network_...
chromeos/network/network_state_handler.cc:373: base::StringPrintf("Unknown
service path %s", service_path.c_str()));
On 2013/09/17 16:22:59, stevenjb wrote:
> nit: + instead of StringPrintf ("foo" + bar where bar is a string is
supported)

Done.

https://codereview.chromium.org/23451044/diff/82001/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/onc_utils.cc (right):

https://codereview.chromium.org/23451044/diff/82001/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/onc_utils.cc:284: &ethernet);
On 2013/09/28 00:37:45, stevenjb wrote:
> if (!ethernet) handle error
The dictionary comes from policy, where it is validated/repaired before
application.
Thus, a DCHECK would be correct but not helpful because it would crash anyways
with the next access...

https://codereview.chromium.org/23451044/diff/82001/chromeos/network/network_...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/23451044/diff/82001/chromeos/network/network_...
chromeos/network/network_state_handler.cc:374: if (network->type() !=
flimflam::kTypeEthernet ||
On 2013/09/28 00:37:45, stevenjb wrote:
> Shouldn't this trigger an error?

Done.

[pneubeck (no reviews), 2013-09-30 20:08:30]

@Steven, please take a last look at the last changes to network_state_handler.cc
Thanks!

https://codereview.chromium.org/23451044/diff/29001/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/onc_utils.cc (right):

https://codereview.chromium.org/23451044/diff/29001/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/onc_utils.cc:273: (*it)->GetAsDictionary(&network);
On 2013/09/17 16:22:59, stevenjb wrote:
> if (!GetAsDictionary()) { ERROR, continue }

Done.

https://codereview.chromium.org/23451044/diff/29001/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/onc_utils.cc:282: &ethernet);
On 2013/09/17 16:22:59, stevenjb wrote:
> if (!GetDictionary...) { ERROR, continue }

Done.

https://codereview.chromium.org/23451044/diff/29001/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/onc_utils.cc:296: const std::string& network_guid) {
On 2013/09/17 16:22:59, stevenjb wrote:
> I really think all of these should just take either 'service_path' (or even
> 'guid') and look up FavoriteState here where we need it. I'd much rather do an
> extra lookup then pass both 'network' and 'network_guid'. Also that way we
only
> need one GetPolicyForNetwork() function.

Done.

https://codereview.chromium.org/23451044/diff/29001/chromeos/network/network_...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/23451044/diff/29001/chromeos/network/network_...
chromeos/network/network_state_handler.cc:373: base::StringPrintf("Unknown
service path %s", service_path.c_str()));
On 2013/09/17 16:22:59, stevenjb wrote:
> nit: + instead of StringPrintf ("foo" + bar where bar is a string is
supported)

Done.

https://codereview.chromium.org/23451044/diff/82001/chrome/browser/chromeos/n...
File chrome/browser/chromeos/net/onc_utils.cc (right):

https://codereview.chromium.org/23451044/diff/82001/chrome/browser/chromeos/n...
chrome/browser/chromeos/net/onc_utils.cc:284: &ethernet);
On 2013/09/28 00:37:45, stevenjb wrote:
> if (!ethernet) handle error
The dictionary comes from policy, where it is validated/repaired before
application.
Thus, a DCHECK would be correct but not helpful because it would crash anyways
with the next access...

https://codereview.chromium.org/23451044/diff/82001/chromeos/network/network_...
File chromeos/network/network_state_handler.cc (right):

https://codereview.chromium.org/23451044/diff/82001/chromeos/network/network_...
chromeos/network/network_state_handler.cc:374: if (network->type() !=
flimflam::kTypeEthernet ||
On 2013/09/28 00:37:45, stevenjb wrote:
> Shouldn't this trigger an error?

Done.

[stevenjb, 2013-09-30 22:07:28]

lgtm

[commit-bot: I haz the power, 2013-10-01 16:03:25]

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

[commit-bot: I haz the power, 2013-10-01 16:20:45]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-10-01 19:08:18]

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

[commit-bot: I haz the power, 2013-10-02 01:54:04]

Retried try job too often on win7_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...

[commit-bot: I haz the power, 2013-10-02 10:12:56]

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

[commit-bot: I haz the power, 2013-10-02 10:20:18]

Change committed as 226444