Extract certificate policy application from NetworkLibrary.

Extract certificate policy application from NetworkLibrary.

This is mostly a refactoring:
- Import of certificates is handled by a new CertifcateHandler, which will get more functionality like resolving CertificatePatterns in upcoming commits.
- Policy validation moved into NetworkConfigurationUpdater and net_internals, because it covers both certificates and networks.

The only functional change is that certificate policies should now also work if ManagedNetworkConfigurationHandler is used instead of NetworkLibrary.

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

[pneubeck (no reviews), 2013-04-22 08:07:22]

@thestig
  chrome/chrome_browser_chromeos.gypi

@nkostylev
  .../login

@eroman
  .../net_internals

@joaodasilva
  .../policy

@steven
  .../chromeos/cros
  chromeos/network

The CL is mostly large because one previous unit test of NetworkLibrary
(LoadOncNetworksWithInvalidConfig: importing a partially invalid ONC) is broken
into two parts:

- NetworkConfigurationUpdaterTest:PolicyIsValidatedAndRepaired
- ONCCertificateImporterTest:MultipleCertificatesWithFailures

[Nikita (slow), 2013-04-22 09:08:08]

chromeos/login lgtm

[Joao da Silva, 2013-04-22 10:38:09]

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/network_library_impl_base.cc (right):

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1087: if (true) {
?

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_configuration_updater.cc (right):

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater.cc:20: class
CrosTrustAnchorProvider : public net::CertTrustAnchorProvider {
Just a comment: once we have the ProfilePolicyConnector then we can move this
object there, and remove this class.

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_configuration_updater_impl.h
(right):

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater_impl.h:32: explicit
NetworkConfigurationUpdaterImpl(
No need to be explicit.

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/network_configuration_updater_impl_cros_unittest.cc
(right):

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater_impl_cros_unittest.cc:124:

nit: remove newline

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater_impl_cros_unittest.cc:186:
{
Extra {} scope not needed.

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater_impl_cros_unittest.cc:209:
: public  NetworkConfigurationUpdaterTest,
nit: single space after 'public'

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater_impl_cros_unittest.cc:244:
{
Extra {} scope not needed.

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater_impl_cros_unittest.cc:271:
{
The extra {} scope is not needed.

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater_impl_cros_unittest.cc:333:
{
Extra {} scope not needed.

https://codereview.chromium.org/14192017/diff/15001/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/14192017/diff/15001/chromeos/chromeos.gyp#new...
chromeos/chromeos.gyp:204: 'network/certificate_handler.h',
order

https://codereview.chromium.org/14192017/diff/15001/chromeos/chromeos.gyp#new...
chromeos/chromeos.gyp:379: 'network/mock_certificate_handler.h',
order

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/certific...
File chromeos/network/certificate_handler.cc (right):

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/certific...
chromeos/network/certificate_handler.cc:14: CertificateHandler*
g_certificate_handler_instance = NULL;
I generally singletons with static getters.

The CertificateHandler class doesn't have any state, so it could just be created
wherever it's used i.e. the NetworkConfigurationUpdater classes could get an
owned instance (scoped_ptr<CertificateHandler>) to use, so that tests can inject
the mock. Net internals UI can just create an instance when it needs one.

If the class grows in the future then the global instance could be owned by the
BrowserPolicyConnector or a similar class.

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_utils.h (right):

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_utils.h:84: 
nit: remove extra newline

[stevenjb, 2013-04-22 16:53:40]

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_configuration_updater.cc (right):

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater.cc:33:
trust_anchors_.swap(*trust_anchors);
Mixing scoped_ptr and direct pointer operations (swap) is confusing. I think
this would be more clear if trust_anchors_ were a scoped_ptr.

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

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater.h:54: // An
implementation of CertTrustAnchorProvider. Owned by the updater, but
s/the updater/this class/ (It wasn't immediately clear to me what 'the updater'
referred to).

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater.h:55: // lives on
the IO thread.
s/lives/runs (and is deleted)/ (or something more descriptive)

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_configuration_updater_impl.h
(right):

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater_impl.h:60:
chromeos::CertificateHandler* certificate_handler_;
Add a comment to the effect that these are pointers to a global instance or a
mock.

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/certific...
File chromeos/network/certificate_handler.cc (right):

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/certific...
chromeos/network/certificate_handler.cc:14: CertificateHandler*
g_certificate_handler_instance = NULL;
On 2013/04/22 10:38:09, Joao da Silva wrote:
> I generally singletons with static getters.
> 
> The CertificateHandler class doesn't have any state, so it could just be
created
> wherever it's used i.e. the NetworkConfigurationUpdater classes could get an
> owned instance (scoped_ptr<CertificateHandler>) to use, so that tests can
inject
> the mock. Net internals UI can just create an instance when it needs one.
> 
> If the class grows in the future then the global instance could be owned by
the
> BrowserPolicyConnector or a similar class.

I tend to agree. This is a useful model for classes with external dependencies
where construction and destruction order matters, but is overkill here and
should be avoided. It seems like and instance of this can just be owned by the
class(es) that use it.

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.cc (right):

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:827:
(*it)->GetAsDictionary(&network);
Need to handle if (!GetAsDictionary()) here.

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:829:
network->GetStringWithoutPathExpansion(onc::network_config::kGUID, &guid);
Not clear that the logic below will handle an empty guid correctly. You may want
to handle !GetStringWithoutPathExpansion() explicitly.

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:833: if (new_entry) {
This is headache inducing and relies on [] implicitly inserting a NULL entry.
I'd prefer something along the lines of 'if (policies->count(guid) > 0) {
LOG(ERROR)...; delete policies[guid]; }'

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:843: }
nit: no {}

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_utils.cc:275: network_configs->reset(new
base::ListValue);
Could be network_configs->Clear()

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_utils.cc:308: true,  // Fail on missing fields.
nit: align comments

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_utils.cc:351:
network_configs->reset(network_configs_listvalue);
Could be network_configs->Swap()

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_utils.h (right):

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_utils.h:82: scoped_ptr<base::ListValue>* certificates);
Is there a reason we need to pass a scoped_ptr<>* (confusing) instead of just a
ListValue*?

[pneubeck (no reviews), 2013-04-22 18:15:49]

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/certific...
File chromeos/network/certificate_handler.cc (right):

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/certific...
chromeos/network/certificate_handler.cc:14: CertificateHandler*
g_certificate_handler_instance = NULL;
Agree. This class was meant for several certificate tasks that require permanent
lifetime, but I can as well wait adding it until required.

FYI, in the future this class shall keep track
- of networks with CertificatePatterns, find matching Certificates and send them
to Shill.
  (notified by ManagedNetworkConfigurationHandler)
- of new / removal of certificates
  (notified by some certificate database)
- of certificate expiration
...

Certificate policies may (as in this patch) or may not be routed through this
class.

[eroman, 2013-04-22 20:02:16]

net_internals LGTM

[Lei Zhang, 2013-04-22 20:11:26]

chrome/chrome_browser_chromeos.gypi lgtm

[pneubeck (no reviews), 2013-04-23 18:05:25]

@Steven, @Joao, PTAL

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/network_library_impl_base.cc (right):

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1087: if (true) {
On 2013/04/22 10:38:09, Joao da Silva wrote:
> ?

Done.

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_configuration_updater.cc (right):

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater.cc:20: class
CrosTrustAnchorProvider : public net::CertTrustAnchorProvider {
On 2013/04/22 10:38:09, Joao da Silva wrote:
> Just a comment: once we have the ProfilePolicyConnector then we can move this
> object there, and remove this class.

Done.

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater.cc:33:
trust_anchors_.swap(*trust_anchors);
On 2013/04/22 16:53:41, stevenjb (chromium) wrote:
> Mixing scoped_ptr and direct pointer operations (swap) is confusing. I think
> this would be more clear if trust_anchors_ were a scoped_ptr.

@Joao, you added that code. Any comment?

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

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater.h:54: // An
implementation of CertTrustAnchorProvider. Owned by the updater, but
On 2013/04/22 16:53:41, stevenjb (chromium) wrote:
> s/the updater/this class/ (It wasn't immediately clear to me what 'the
updater'
> referred to).

Done.

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater.h:55: // lives on
the IO thread.
On 2013/04/22 16:53:41, stevenjb (chromium) wrote:
> s/lives/runs (and is deleted)/ (or something more descriptive)

Done.

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_configuration_updater_impl.h
(right):

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater_impl.h:32: explicit
NetworkConfigurationUpdaterImpl(
On 2013/04/22 10:38:09, Joao da Silva wrote:
> No need to be explicit.

Done.

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater_impl.h:60:
chromeos::CertificateHandler* certificate_handler_;
On 2013/04/22 16:53:41, stevenjb (chromium) wrote:
> Add a comment to the effect that these are pointers to a global instance or a
> mock.

Done.

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
File
chrome/browser/chromeos/policy/network_configuration_updater_impl_cros_unittest.cc
(right):

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater_impl_cros_unittest.cc:124:

On 2013/04/22 10:38:09, Joao da Silva wrote:
> nit: remove newline

Done.

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater_impl_cros_unittest.cc:186:
{
On 2013/04/22 10:38:09, Joao da Silva wrote:
> Extra {} scope not needed.

Done.

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater_impl_cros_unittest.cc:209:
: public  NetworkConfigurationUpdaterTest,
On 2013/04/22 10:38:09, Joao da Silva wrote:
> nit: single space after 'public'

Done.

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater_impl_cros_unittest.cc:244:
{
On 2013/04/22 10:38:09, Joao da Silva wrote:
> Extra {} scope not needed.

Done.

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater_impl_cros_unittest.cc:271:
{
On 2013/04/22 10:38:09, Joao da Silva wrote:
> The extra {} scope is not needed.

Done.

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater_impl_cros_unittest.cc:333:
{
On 2013/04/22 10:38:09, Joao da Silva wrote:
> Extra {} scope not needed.

Done.

https://codereview.chromium.org/14192017/diff/15001/chromeos/chromeos.gyp
File chromeos/chromeos.gyp (right):

https://codereview.chromium.org/14192017/diff/15001/chromeos/chromeos.gyp#new...
chromeos/chromeos.gyp:204: 'network/certificate_handler.h',
On 2013/04/22 10:38:09, Joao da Silva wrote:
> order

Done.

https://codereview.chromium.org/14192017/diff/15001/chromeos/chromeos.gyp#new...
chromeos/chromeos.gyp:379: 'network/mock_certificate_handler.h',
On 2013/04/22 10:38:09, Joao da Silva wrote:
> order

Done.

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.cc (right):

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:827:
(*it)->GetAsDictionary(&network);
On 2013/04/22 16:53:41, stevenjb (chromium) wrote:
> Need to handle if (!GetAsDictionary()) here.
The ONC must be validated before calling this function. If this call still
doesn't succeed, it will immediately segfault in the next statement.

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:829:
network->GetStringWithoutPathExpansion(onc::network_config::kGUID, &guid);
On 2013/04/22 16:53:41, stevenjb (chromium) wrote:
> Not clear that the logic below will handle an empty guid correctly. You may
want
> to handle !GetStringWithoutPathExpansion() explicitly.

Empty GUIDs are not allowed in ONC. Since this is a more implicit rule and
wouldn't lead to an immediate crash, I added a DCHECK.

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:833: if (new_entry) {
On 2013/04/22 16:53:41, stevenjb (chromium) wrote:
> This is headache inducing and relies on [] implicitly inserting a NULL entry.
> I'd prefer something along the lines of 'if (policies->count(guid) > 0) {
> LOG(ERROR)...; delete policies[guid]; }'

Done.

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:843: }
On 2013/04/22 16:53:41, stevenjb (chromium) wrote:
> nit: no {}

Done.

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_utils.cc:275: network_configs->reset(new
base::ListValue);
On 2013/04/22 16:53:41, stevenjb (chromium) wrote:
> Could be network_configs->Clear()

Done.

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_utils.cc:308: true,  // Fail on missing fields.
On 2013/04/22 16:53:41, stevenjb (chromium) wrote:
> nit: align comments

Done.

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_utils.cc:351:
network_configs->reset(network_configs_listvalue);
On 2013/04/22 16:53:41, stevenjb (chromium) wrote:
> Could be network_configs->Swap()

Done.

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/onc/onc_...
File chromeos/network/onc/onc_utils.h (right):

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_utils.h:82: scoped_ptr<base::ListValue>* certificates);
On 2013/04/22 16:53:41, stevenjb (chromium) wrote:
> Is there a reason we need to pass a scoped_ptr<>* (confusing) instead of just
a
> ListValue*?

Done.

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/onc/onc_...
chromeos/network/onc/onc_utils.h:84: 
On 2013/04/22 10:38:09, Joao da Silva wrote:
> nit: remove extra newline

Done.

[stevenjb, 2013-04-23 20:02:59]

Thanks. LGTM.

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.cc (right):

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:827:
(*it)->GetAsDictionary(&network);
On 2013/04/23 18:05:25, pneubeck wrote:
> On 2013/04/22 16:53:41, stevenjb (chromium) wrote:
> > Need to handle if (!GetAsDictionary()) here.
> The ONC must be validated before calling this function. If this call still
> doesn't succeed, it will immediately segfault in the next statement.
nit: add DCHECK(network) to document that.

[Joao da Silva, 2013-04-24 09:21:14]

lgtm

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_configuration_updater.cc (right):

https://codereview.chromium.org/14192017/diff/15001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater.cc:33:
trust_anchors_.swap(*trust_anchors);
On 2013/04/23 18:05:25, pneubeck wrote:
> On 2013/04/22 16:53:41, stevenjb (chromium) wrote:
> > Mixing scoped_ptr and direct pointer operations (swap) is confusing. I think
> > this would be more clear if trust_anchors_ were a scoped_ptr.
> 
> @Joao, you added that code. Any comment?

trust_anchors_ as a scoped_ptr SGTM.

[commit-bot: I haz the power, 2013-04-24 10:54:06]

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

[commit-bot: I haz the power, 2013-04-24 11:18:55]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[pneubeck (no reviews), 2013-04-24 11:25:10]

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/managed_...
File chromeos/network/managed_network_configuration_handler.cc (right):

https://codereview.chromium.org/14192017/diff/15001/chromeos/network/managed_...
chromeos/network/managed_network_configuration_handler.cc:827:
(*it)->GetAsDictionary(&network);
On 2013/04/23 20:02:59, stevenjb (chromium) wrote:
> On 2013/04/23 18:05:25, pneubeck wrote:
> > On 2013/04/22 16:53:41, stevenjb (chromium) wrote:
> > > Need to handle if (!GetAsDictionary()) here.
> > The ONC must be validated before calling this function. If this call still
> > doesn't succeed, it will immediately segfault in the next statement.
> nit: add DCHECK(network) to document that.

Done.

[commit-bot: I haz the power, 2013-04-24 11:25:23]

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

[commit-bot: I haz the power, 2013-04-24 12:12:36]

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

[commit-bot: I haz the power, 2013-04-25 08:35:05]

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

[commit-bot: I haz the power, 2013-04-25 08:50:30]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
cacheinvalidation_unittests, check_deps, chromedriver2_unittests,
components_unittests, content_browsertests, content_unittests, crypto_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, nacl_integration, net_unittests, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-04-26 08:28:03]

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

[commit-bot: I haz the power, 2013-04-26 13:51:10]

Failed to apply patch for
chrome/browser/chromeos/policy/network_configuration_updater_impl_cros.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/chromeos/policy/network_configuration_updater_impl_cros.cc
  Hunk #1 FAILED at 8.
  Hunk #2 succeeded at 81 (offset -2 lines).
  Hunk #3 succeeded at 94 (offset -2 lines).
  Hunk #4 FAILED at 123.
  2 out of 4 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/policy/network_configuration_updater_impl_cros.cc.rej

Patch:      
chrome/browser/chromeos/policy/network_configuration_updater_impl_cros.cc
Index: chrome/browser/chromeos/policy/network_configuration_updater_impl_cros.cc
diff --git
a/chrome/browser/chromeos/policy/network_configuration_updater_impl_cros.cc
b/chrome/browser/chromeos/policy/network_configuration_updater_impl_cros.cc
index
32bfc5a8a8a716f2448e8565c9243a52a83fdac1..cc26606585d1bf10fcd01016d7c4be8d2916917f
100644
--- a/chrome/browser/chromeos/policy/network_configuration_updater_impl_cros.cc
+++ b/chrome/browser/chromeos/policy/network_configuration_updater_impl_cros.cc
@@ -8,61 +8,27 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
-#include "base/command_line.h"
 #include "base/logging.h"
-#include "base/memory/scoped_ptr.h"
+#include "base/values.h"
 #include "chrome/browser/chromeos/cros/network_library.h"
 #include "chrome/browser/policy/policy_map.h"
-#include "chrome/common/chrome_switches.h"
+#include "chromeos/network/certificate_handler.h"
 #include "chromeos/network/onc/onc_constants.h"
 #include "chromeos/network/onc/onc_utils.h"
-#include "content/public/browser/browser_thread.h"
-#include "net/cert/cert_trust_anchor_provider.h"
-#include "net/cert/x509_certificate.h"
 #include "policy/policy_constants.h"
 
-using content::BrowserThread;
-
 namespace policy {
 
-namespace {
-
-// A simple implementation of net::CertTrustAnchorProvider that returns a list
-// of certificates that can be set by the owner of this object.
-class CrosTrustAnchorProvider : public net::CertTrustAnchorProvider {
- public:
-  CrosTrustAnchorProvider() {}
-  virtual ~CrosTrustAnchorProvider() {}
-
-  // CertTrustAnchorProvider overrides.
-  virtual const net::CertificateList& GetAdditionalTrustAnchors() OVERRIDE {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-    return trust_anchors_;
-  }
-
-  void SetTrustAnchors(scoped_ptr<net::CertificateList> trust_anchors) {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-    trust_anchors_.swap(*trust_anchors);
-  }
-
- private:
-  net::CertificateList trust_anchors_;
-
-  DISALLOW_COPY_AND_ASSIGN(CrosTrustAnchorProvider);
-};
-
-}  // namespace
-
 NetworkConfigurationUpdaterImplCros::NetworkConfigurationUpdaterImplCros(
     PolicyService* policy_service,
-    chromeos::NetworkLibrary* network_library)
+    chromeos::NetworkLibrary* network_library,
+    scoped_ptr<chromeos::CertificateHandler> certificate_handler)
     : policy_change_registrar_(
           policy_service, PolicyNamespace(POLICY_DOMAIN_CHROME,
std::string())),
       network_library_(network_library),
+      certificate_handler_(certificate_handler.Pass()),
       user_policy_initialized_(false),
-      allow_trusted_certificates_from_policy_(false),
-      policy_service_(policy_service),
-      cert_trust_provider_(new CrosTrustAnchorProvider()) {
+      policy_service_(policy_service) {
   DCHECK(network_library_);
   policy_change_registrar_.Observe(
       key::kDeviceOpenNetworkConfiguration,
@@ -83,10 +49,6 @@
NetworkConfigurationUpdaterImplCros::NetworkConfigurationUpdaterImplCros(
 
 NetworkConfigurationUpdaterImplCros::~NetworkConfigurationUpdaterImplCros() {
   network_library_->RemoveNetworkProfileObserver(this);
-  bool posted = BrowserThread::DeleteSoon(
-      BrowserThread::IO, FROM_HERE, cert_trust_provider_);
-  if (!posted)
-    delete cert_trust_provider_;
 }
 
 void NetworkConfigurationUpdaterImplCros::OnProfileListChanged() {
@@ -100,17 +62,6 @@ void
NetworkConfigurationUpdaterImplCros::OnUserPolicyInitialized() {
   ApplyNetworkConfigurations();
 }
 
-void NetworkConfigurationUpdaterImplCros::
-set_allow_trusted_certificates_from_policy(bool allow) {
-  allow_trusted_certificates_from_policy_ = allow;
-}
-
-net::CertTrustAnchorProvider*
-    NetworkConfigurationUpdaterImplCros::GetCertTrustAnchorProvider() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  return cert_trust_provider_;
-}
-
 void NetworkConfigurationUpdaterImplCros::OnPolicyChanged(
     chromeos::onc::ONCSource onc_source,
     const base::Value* previous,
@@ -138,39 +89,30 @@ void
NetworkConfigurationUpdaterImplCros::ApplyNetworkConfiguration(
       PolicyNamespace(POLICY_DOMAIN_CHROME, std::string()));
   const base::Value* policy_value = policies.GetValue(policy_key);
 
-  std::string new_network_config;
+  std::string onc_blob;
   if (policy_value != NULL) {
     // If the policy is not a string, we issue a warning, but still clear the
     // network configuration.
-    if (!policy_value->GetAsString(&new_network_config)) {
+    if (!policy_value->GetAsString(&onc_blob)) {
       LOG(WARNING) << "ONC policy for source "
                    << chromeos::onc::GetSourceAsString(onc_source)
                    << " is not a string value.";
     }
   }
 
-  // An empty string is not a valid ONC and generates warnings and
-  // errors. Replace by a valid empty configuration.
-  if (new_network_config.empty())
-    new_network_config = chromeos::onc::kEmptyUnencryptedConfiguration;
+  base::ListValue network_configs;
+  base::ListValue certificates;
+  ParseAndValidateOncForImport(
+      onc_blob, onc_source, "", &network_configs, &certificates);
 
-  scoped_ptr<net::CertificateList> web_trust_certs(new net::CertificateList());
-  if (!network_library_->LoadOncNetworks(new_network_config, "", onc_source,
-                                         web_trust_certs.get())) {
-    LOG(ERROR) << "Errors occurred during the ONC policy application.";
-  }
+  network_library_->LoadOncNetworks(network_configs, onc_source);
 
-  CommandLine* command_line = CommandLine::ForCurrentProcess();
-  if (onc_source == chromeos::onc::ONC_SOURCE_USER_POLICY &&
-      allow_trusted_certificates_from_policy_ &&
-      command_line->HasSwitch(switches::kEnableWebTrustCerts)) {
-    BrowserThread::PostTask(
-        BrowserThread::IO, FROM_HERE,
-        base::Bind(&CrosTrustAnchorProvider::SetTrustAnchors,
-                   base::Unretained(static_cast<CrosTrustAnchorProvider*>(
-                       cert_trust_provider_)),
-                   base::Passed(&web_trust_certs)));
-  }
+  scoped_ptr<net::CertificateList> web_trust_certs(new net::CertificateList);
+  certificate_handler_->ImportCertificates(
+      certificates, onc_source, web_trust_certs.get());
+
+  if (onc_source == chromeos::onc::ONC_SOURCE_USER_POLICY)
+    SetTrustAnchors(web_trust_certs.Pass());
 }
 
 }  // namespace policy

[pneubeck (no reviews), 2013-04-26 15:03:02]

Committed patchset #7 manually as r196735 (presubmit successful).