Revert of Make ONCCertificateImporter async. (patchset #1 id:1 of...

Revert of Make ONCCertificateImporter async. (patchset #1 id:1 of https://codereview.chromium.org/582413002/)

Reason for revert:
Use-after-free.
https://code.google.com/p/chromium/issues/detail?id=415916

Original issue's description:
> Make ONCCertificateImporter async.
>
> This prepares for the new CertDatabase keyed service, which will have stricter threading restrictions. https://codereview.chromium.org/419013003/
> Before, ONCCertificateImporter accessed the NSSCertDatabase from the UI thread and blocked on certificate store operations.
> Now, the import itself is asychronous and calls back on completion.
>
> While there, this also removes the GMock of the importer.
>
> This is a reland of f08303014b165f6013fe33198cd798ebd9a4e925
>   refs/heads/master@{#295534}
> with the fixed destruction order in ONCCertificateImporterImplTest.
>
> The fix was reviewed in
>   https://codereview.chromium.org/589443002/
>
> TBR=eroman@chromium.org
> BUG=413219
>
> Committed: https://crrev.com/3b4ba221657f6b27e2156818bc445c885d87fc0e
> Cr-Commit-Position: refs/heads/master@{#295687}

TBR=joaodasilva@chromium.org,eroman@chromium.org,pneubeck@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=413219

Review URL: https://codereview.chromium.org/584923002

Cr-Commit-Position: refs/heads/master@{#295702}

chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc

@@ -17,7 +17,7 @@
 #include "chrome/test/base/testing_profile.h"
 #include "chromeos/network/fake_network_device_handler.h"
 #include "chromeos/network/mock_managed_network_configuration_handler.h"
-#include "chromeos/network/onc/onc_certificate_importer.h"
+#include "chromeos/network/onc/mock_certificate_importer.h"
 #include "chromeos/network/onc/onc_test_utils.h"
 #include "chromeos/network/onc/onc_utils.h"
 #include "components/onc/onc_constants.h"
@@ -71,16 +71,11 @@ class FakeUser : public user_manager::User {
 class FakeWebTrustedCertsObserver
     : public UserNetworkConfigurationUpdater::WebTrustedCertsObserver {
  public:
-  FakeWebTrustedCertsObserver() {}
-
   virtual void OnTrustAnchorsChanged(
       const net::CertificateList& trust_anchors) OVERRIDE {
     trust_anchors_ = trust_anchors;
   }
   net::CertificateList trust_anchors_;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(FakeWebTrustedCertsObserver);
 };
 
 class FakeNetworkDeviceHandler : public chromeos::FakeNetworkDeviceHandler {
@@ -92,56 +87,6 @@ class FakeNetworkDeviceHandler : public chromeos::FakeNetworkDeviceHandler {
    }
 
    bool allow_roaming_;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(FakeNetworkDeviceHandler);
-};
-
-class FakeCertificateImporter : public chromeos::onc::CertificateImporter {
- public:
-  FakeCertificateImporter()
-      : expected_onc_source_(::onc::ONC_SOURCE_UNKNOWN), call_count_(0) {}
-  virtual ~FakeCertificateImporter() {}
-
-  void SetTrustedCertificatesResult(
-      net::CertificateList onc_trusted_certificates) {
-    onc_trusted_certificates_ = onc_trusted_certificates;
-  }
-
-  void SetExpectedONCCertificates(const base::ListValue& certificates) {
-    expected_onc_certificates_.reset(certificates.DeepCopy());
-  }
-
-  void SetExpectedONCSource(::onc::ONCSource source) {
-    expected_onc_source_ = source;
-  }
-
-  unsigned int GetAndResetImportCount() {
-    unsigned int count = call_count_;
-    call_count_ = 0;
-    return count;
-  }
-
-  virtual void ImportCertificates(const base::ListValue& certificates,
-                                  ::onc::ONCSource source,
-                                  const DoneCallback& done_callback) OVERRIDE {
-    if (expected_onc_source_ != ::onc::ONC_SOURCE_UNKNOWN)
-      EXPECT_EQ(expected_onc_source_, source);
-    if (expected_onc_certificates_) {
-      EXPECT_TRUE(chromeos::onc::test_utils::Equals(
-          expected_onc_certificates_.get(), &certificates));
-    }
-    ++call_count_;
-    done_callback.Run(true, onc_trusted_certificates_);
-  }
-
- private:
-  ::onc::ONCSource expected_onc_source_;
-  scoped_ptr<base::ListValue> expected_onc_certificates_;
-  net::CertificateList onc_trusted_certificates_;
-  unsigned int call_count_;
-
-  DISALLOW_COPY_AND_ASSIGN(FakeCertificateImporter);
 };
 
 const char kFakeONC[] =
@@ -200,7 +145,8 @@ ACTION_P(SetCertificateList, list) {
 
 class NetworkConfigurationUpdaterTest : public testing::Test {
  protected:
-  NetworkConfigurationUpdaterTest() : certificate_importer_(NULL) {}
+  NetworkConfigurationUpdaterTest() {
+  }
 
   virtual void SetUp() OVERRIDE {
     EXPECT_CALL(provider_, IsInitializationComplete(_))
@@ -228,7 +174,8 @@ class NetworkConfigurationUpdaterTest : public testing::Test {
         onc::toplevel_config::kCertificates, &certs);
     AppendAll(*certs, &fake_certificates_);
 
-    certificate_importer_ = new FakeCertificateImporter;
+    certificate_importer_ =
+        new StrictMock<chromeos::onc::MockCertificateImporter>();
     certificate_importer_owned_.reset(certificate_importer_);
   }
 
@@ -296,7 +243,7 @@ class NetworkConfigurationUpdaterTest : public testing::Test {
   // NetworkConfigurationUpdater. When that happens, |certificate_importer_|
   // continues to point to that instance but |certificate_importer_owned_| is
   // released.
-  FakeCertificateImporter* certificate_importer_;
+  StrictMock<chromeos::onc::MockCertificateImporter>* certificate_importer_;
   scoped_ptr<chromeos::onc::CertificateImporter> certificate_importer_owned_;
 
   StrictMock<MockConfigurationPolicyProvider> provider_;
@@ -369,13 +316,13 @@ TEST_F(NetworkConfigurationUpdaterTest, PolicyIsValidatedAndRepaired) {
                         _,
                         IsEqualTo(network_configs_repaired),
                         IsEqualTo(global_config_repaired)));
-  certificate_importer_->SetExpectedONCSource(onc::ONC_SOURCE_USER_POLICY);
+  EXPECT_CALL(*certificate_importer_,
+              ImportCertificates(_, onc::ONC_SOURCE_USER_POLICY,  _));
 
   CreateNetworkConfigurationUpdaterForUserPolicy(
       false /* do not allow trusted certs from policy */,
       true /* set certificate importer */);
   MarkPolicyProviderInitialized();
-  EXPECT_EQ(1u, certificate_importer_->GetAndResetImportCount());
 }
 
 TEST_F(NetworkConfigurationUpdaterTest,
@@ -389,7 +336,8 @@ TEST_F(NetworkConfigurationUpdaterTest,
 
   EXPECT_CALL(network_config_handler_,
               SetPolicy(onc::ONC_SOURCE_USER_POLICY, _, _, _));
-  certificate_importer_->SetTrustedCertificatesResult(cert_list);
+  EXPECT_CALL(*certificate_importer_, ImportCertificates(_, _, _))
+      .WillRepeatedly(SetCertificateList(cert_list));
 
   UserNetworkConfigurationUpdater* updater =
       CreateNetworkConfigurationUpdaterForUserPolicy(
@@ -425,8 +373,9 @@ TEST_F(NetworkConfigurationUpdaterTest,
                                          net::X509Certificate::FORMAT_AUTO);
   ASSERT_EQ(1u, cert_list.size());
 
-  certificate_importer_->SetExpectedONCSource(onc::ONC_SOURCE_USER_POLICY);
-  certificate_importer_->SetTrustedCertificatesResult(cert_list);
+  EXPECT_CALL(*certificate_importer_,
+              ImportCertificates(_, onc::ONC_SOURCE_USER_POLICY, _))
+      .WillRepeatedly(SetCertificateList(cert_list));
 
   UserNetworkConfigurationUpdater* updater =
       CreateNetworkConfigurationUpdaterForUserPolicy(
@@ -449,6 +398,10 @@ TEST_F(NetworkConfigurationUpdaterTest,
       .Times(AnyNumber());
 
   // Start with an empty certificate list.
+  EXPECT_CALL(*certificate_importer_,
+              ImportCertificates(_, onc::ONC_SOURCE_USER_POLICY, _))
+      .WillRepeatedly(SetCertificateList(net::CertificateList()));
+
   UserNetworkConfigurationUpdater* updater =
       CreateNetworkConfigurationUpdaterForUserPolicy(
           true /* allow trusted certs from policy */,
@@ -461,6 +414,7 @@ TEST_F(NetworkConfigurationUpdaterTest,
   base::RunLoop().RunUntilIdle();
 
   // Verify that the returned certificate list is empty.
+  Mock::VerifyAndClearExpectations(certificate_importer_);
   {
     net::CertificateList trust_anchors;
     updater->GetWebTrustedCertificates(&trust_anchors);
@@ -475,7 +429,10 @@ TEST_F(NetworkConfigurationUpdaterTest,
                                          "ok_cert.pem",
                                          net::X509Certificate::FORMAT_AUTO);
   ASSERT_EQ(1u, cert_list.size());
-  certificate_importer_->SetTrustedCertificatesResult(cert_list);
+
+  EXPECT_CALL(*certificate_importer_,
+              ImportCertificates(_, onc::ONC_SOURCE_USER_POLICY, _))
+      .WillOnce(SetCertificateList(cert_list));
 
   // Change to any non-empty policy, so that updates are triggered. The actual
   // content of the policy is irrelevant.
@@ -512,6 +469,7 @@ TEST_F(NetworkConfigurationUpdaterTest,
                         kFakeUsernameHash,
                         IsEqualTo(&fake_network_configs_),
                         IsEqualTo(&fake_global_network_config_)));
+  EXPECT_CALL(*certificate_importer_, ImportCertificates(_, _ , _)).Times(0);
 
   UserNetworkConfigurationUpdater* updater =
       CreateNetworkConfigurationUpdaterForUserPolicy(
@@ -520,14 +478,21 @@ TEST_F(NetworkConfigurationUpdaterTest,
   MarkPolicyProviderInitialized();
 
   Mock::VerifyAndClearExpectations(&network_config_handler_);
-  EXPECT_EQ(0u, certificate_importer_->GetAndResetImportCount());
+  Mock::VerifyAndClearExpectations(certificate_importer_);
 
-  certificate_importer_->SetExpectedONCCertificates(fake_certificates_);
-  certificate_importer_->SetExpectedONCSource(onc::ONC_SOURCE_USER_POLICY);
+  EXPECT_CALL(network_config_handler_,
+              SetPolicy(onc::ONC_SOURCE_USER_POLICY,
+                        kFakeUsernameHash,
+                        IsEqualTo(&fake_network_configs_),
+                        IsEqualTo(&fake_global_network_config_)))
+      .Times(0);
+  EXPECT_CALL(*certificate_importer_,
+              ImportCertificates(IsEqualTo(&fake_certificates_),
+                                 onc::ONC_SOURCE_USER_POLICY,
+                                  _));
 
   ASSERT_TRUE(certificate_importer_owned_);
   updater->SetCertificateImporterForTest(certificate_importer_owned_.Pass());
-  EXPECT_EQ(1u, certificate_importer_->GetAndResetImportCount());
 }
 
 class NetworkConfigurationUpdaterTestWithParam
@@ -578,13 +543,14 @@ TEST_P(NetworkConfigurationUpdaterTestWithParam, InitialUpdates) {
                         ExpectedUsernameHash(),
                         IsEqualTo(&fake_network_configs_),
                         IsEqualTo(&fake_global_network_config_)));
-  certificate_importer_->SetExpectedONCCertificates(fake_certificates_);
-  certificate_importer_->SetExpectedONCSource(CurrentONCSource());
+  EXPECT_CALL(*certificate_importer_,
+              ImportCertificates(IsEqualTo(&fake_certificates_),
+                                 CurrentONCSource(),
+                                  _))
+      .Times(ExpectedImportCertificatesCallCount());
 
   CreateNetworkConfigurationUpdater();
   MarkPolicyProviderInitialized();
-  EXPECT_EQ(ExpectedImportCertificatesCallCount(),
-            certificate_importer_->GetAndResetImportCount());
 }
 
 TEST_P(NetworkConfigurationUpdaterTestWithParam,
@@ -594,22 +560,35 @@ TEST_P(NetworkConfigurationUpdaterTestWithParam,
              new base::StringValue(kFakeONC), NULL);
   UpdateProviderPolicy(policy);
 
+  EXPECT_CALL(network_config_handler_,
+              SetPolicy(CurrentONCSource(),
+                        ExpectedUsernameHash(),
+                        IsEqualTo(&fake_network_configs_),
+                        IsEqualTo(&fake_global_network_config_)))
+      .Times(0);
+  EXPECT_CALL(*certificate_importer_,
+              ImportCertificates(IsEqualTo(&fake_certificates_),
+                                 CurrentONCSource(),
+                                  _))
+      .Times(0);
+
   CreateNetworkConfigurationUpdater();
 
   Mock::VerifyAndClearExpectations(&network_config_handler_);
-  EXPECT_EQ(0u, certificate_importer_->GetAndResetImportCount());
+  Mock::VerifyAndClearExpectations(certificate_importer_);
 
   EXPECT_CALL(network_config_handler_,
               SetPolicy(CurrentONCSource(),
                         ExpectedUsernameHash(),
                         IsEqualTo(&fake_network_configs_),
                         IsEqualTo(&fake_global_network_config_)));
-  certificate_importer_->SetExpectedONCSource(CurrentONCSource());
-  certificate_importer_->SetExpectedONCCertificates(fake_certificates_);
+  EXPECT_CALL(*certificate_importer_,
+              ImportCertificates(IsEqualTo(&fake_certificates_),
+                                 CurrentONCSource(),
+                                  _))
+      .Times(ExpectedImportCertificatesCallCount());
 
   MarkPolicyProviderInitialized();
-  EXPECT_EQ(ExpectedImportCertificatesCallCount(),
-            certificate_importer_->GetAndResetImportCount());
 }
 
 TEST_P(NetworkConfigurationUpdaterTestWithParam,
@@ -626,25 +605,26 @@ TEST_P(NetworkConfigurationUpdaterTestWithParam,
                         ExpectedUsernameHash(),
                         IsEqualTo(&fake_network_configs_),
                         IsEqualTo(&fake_global_network_config_)));
-  certificate_importer_->SetExpectedONCSource(CurrentONCSource());
-  certificate_importer_->SetExpectedONCCertificates(fake_certificates_);
+  EXPECT_CALL(*certificate_importer_,
+              ImportCertificates(IsEqualTo(&fake_certificates_),
+                                 CurrentONCSource(),
+                                  _))
+      .Times(ExpectedImportCertificatesCallCount());
 
   CreateNetworkConfigurationUpdater();
-
-  EXPECT_EQ(ExpectedImportCertificatesCallCount(),
-            certificate_importer_->GetAndResetImportCount());
 }
 
 TEST_P(NetworkConfigurationUpdaterTestWithParam, PolicyChange) {
   // Ignore the initial updates.
   EXPECT_CALL(network_config_handler_, SetPolicy(_, _, _, _)).Times(AtLeast(1));
+  EXPECT_CALL(*certificate_importer_, ImportCertificates(_, _, _))
+      .Times(AtLeast(ExpectedImportCertificatesCallCount()));
 
   CreateNetworkConfigurationUpdater();
   MarkPolicyProviderInitialized();
 
   Mock::VerifyAndClearExpectations(&network_config_handler_);
-  EXPECT_LE(ExpectedImportCertificatesCallCount(),
-            certificate_importer_->GetAndResetImportCount());
+  Mock::VerifyAndClearExpectations(certificate_importer_);
 
   // The Updater should update if policy changes.
   EXPECT_CALL(network_config_handler_,
@@ -652,26 +632,28 @@ TEST_P(NetworkConfigurationUpdaterTestWithParam, PolicyChange) {
                         _,
                         IsEqualTo(&fake_network_configs_),
                         IsEqualTo(&fake_global_network_config_)));
-  certificate_importer_->SetExpectedONCSource(CurrentONCSource());
-  certificate_importer_->SetExpectedONCCertificates(fake_certificates_);
+  EXPECT_CALL(*certificate_importer_,
+              ImportCertificates(IsEqualTo(&fake_certificates_),
+                                 CurrentONCSource(),
+                                  _))
+      .Times(ExpectedImportCertificatesCallCount());
 
   PolicyMap policy;
   policy.Set(GetParam(), POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
              new base::StringValue(kFakeONC), NULL);
   UpdateProviderPolicy(policy);
   Mock::VerifyAndClearExpectations(&network_config_handler_);
-  EXPECT_EQ(ExpectedImportCertificatesCallCount(),
-            certificate_importer_->GetAndResetImportCount());
+  Mock::VerifyAndClearExpectations(certificate_importer_);
 
   // Another update is expected if the policy goes away.
   EXPECT_CALL(network_config_handler_,
               SetPolicy(CurrentONCSource(), _, IsEmpty(), IsEmpty()));
-  certificate_importer_->SetExpectedONCCertificates(base::ListValue());
+  EXPECT_CALL(*certificate_importer_,
+              ImportCertificates(IsEmpty(), CurrentONCSource(), _))
+      .Times(ExpectedImportCertificatesCallCount());
 
   policy.Erase(GetParam());
   UpdateProviderPolicy(policy);
-  EXPECT_EQ(ExpectedImportCertificatesCallCount(),
-            certificate_importer_->GetAndResetImportCount());
 }
 
 INSTANTIATE_TEST_CASE_P(NetworkConfigurationUpdaterTestWithParamInstance,

chrome/browser/chromeos/policy/user_network_configuration_updater.cc

@@ -15,7 +15,6 @@
 #include "chromeos/network/managed_network_configuration_handler.h"
 #include "chromeos/network/onc/onc_certificate_importer_impl.h"
 #include "components/user_manager/user.h"
-#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_source.h"
 #include "net/cert/x509_certificate.h"
 #include "policy/policy_constants.h"
@@ -85,15 +84,6 @@ void UserNetworkConfigurationUpdater::GetWebTrustedCertificates(
   *certs = web_trust_certs_;
 }
 
-void UserNetworkConfigurationUpdater::OnCertificatesImported(
-    bool /* unused success */,
-    const net::CertificateList& onc_trusted_certificates) {
-  web_trust_certs_.clear();
-  if (allow_trusted_certificates_from_policy_)
-    web_trust_certs_ = onc_trusted_certificates;
-  NotifyTrustAnchorsChanged();
-}
-
 void UserNetworkConfigurationUpdater::ImportCertificates(
     const base::ListValue& certificates_onc) {
   // If certificate importer is not yet set, cache the certificate onc. It will
@@ -103,11 +93,13 @@ void UserNetworkConfigurationUpdater::ImportCertificates(
     return;
   }
 
+  web_trust_certs_.clear();
   certificate_importer_->ImportCertificates(
       certificates_onc,
       onc_source_,
-      base::Bind(&UserNetworkConfigurationUpdater::OnCertificatesImported,
-                 base::Unretained(this)));
+      allow_trusted_certificates_from_policy_ ? &web_trust_certs_ : NULL);
+
+  NotifyTrustAnchorsChanged();
 }
 
 void UserNetworkConfigurationUpdater::ApplyNetworkPolicy(
@@ -140,10 +132,7 @@ void UserNetworkConfigurationUpdater::CreateAndSetCertificateImporter(
     net::NSSCertDatabase* database) {
   DCHECK(database);
   SetCertificateImporter(scoped_ptr<chromeos::onc::CertificateImporter>(
-      new chromeos::onc::CertificateImporterImpl(
-          content::BrowserThread::GetMessageLoopProxyForThread(
-              content::BrowserThread::IO),
-          database)));
+      new chromeos::onc::CertificateImporterImpl(database)));
 }
 
 void UserNetworkConfigurationUpdater::SetCertificateImporter(

chrome/browser/chromeos/policy/user_network_configuration_updater.h

@@ -95,11 +95,6 @@ class UserNetworkConfigurationUpdater : public NetworkConfigurationUpdater,
       PolicyService* policy_service,
       chromeos::ManagedNetworkConfigurationHandler* network_config_handler);
 
-  // Called by the CertificateImporter when an import finished.
-  void OnCertificatesImported(
-      bool success,
-      const net::CertificateList& onc_trusted_certificates);
-
   // NetworkConfigurationUpdater:
   virtual void ImportCertificates(
       const base::ListValue& certificates_onc) OVERRIDE;

chrome/browser/ui/webui/net_internals/net_internals_ui.cc

@@ -257,13 +257,6 @@ class NetInternalsMessageHandler
   void ImportONCFileToNSSDB(const std::string& onc_blob,
                             const std::string& passcode,
                             net::NSSCertDatabase* nssdb);
-
-  // Called back by the CertificateImporter when a certificate import finished.
-  // |previous_error| contains earlier errors during this import.
-  void OnCertificatesImported(
-      const std::string& previous_error,
-      bool success,
-      const net::CertificateList& onc_trusted_certificates);
 #endif
 
  private:
@@ -1414,52 +1407,38 @@ void NetInternalsMessageHandler::ImportONCFileToNSSDB(
     const std::string& onc_blob,
     const std::string& passcode,
     net::NSSCertDatabase* nssdb) {
+  std::string error;
   user_manager::User* user = chromeos::ProfileHelper::Get()->GetUserByProfile(
       Profile::FromWebUI(web_ui()));
 
-  if (!user) {
-    std::string error = "User not found.";
-    SendJavascriptCommand("receivedONCFileParse", new base::StringValue(error));
-    return;
-  }
+  if (user) {
+    onc::ONCSource onc_source = onc::ONC_SOURCE_USER_IMPORT;
+
+    base::ListValue network_configs;
+    base::DictionaryValue global_network_config;
+    base::ListValue certificates;
+    if (!chromeos::onc::ParseAndValidateOncForImport(onc_blob,
+                                                     onc_source,
+                                                     passcode,
+                                                     &network_configs,
+                                                     &global_network_config,
+                                                     &certificates)) {
+      error = "Errors occurred during the ONC parsing. ";
+    }
 
-  std::string error;
-  onc::ONCSource onc_source = onc::ONC_SOURCE_USER_IMPORT;
-  base::ListValue network_configs;
-  base::DictionaryValue global_network_config;
-  base::ListValue certificates;
-  if (!chromeos::onc::ParseAndValidateOncForImport(onc_blob,
-                                                   onc_source,
-                                                   passcode,
-                                                   &network_configs,
-                                                   &global_network_config,
-                                                   &certificates)) {
-    error = "Errors occurred during the ONC parsing. ";
-  }
+    chromeos::onc::CertificateImporterImpl cert_importer(nssdb);
+    if (!cert_importer.ImportCertificates(certificates, onc_source, NULL))
+      error += "Some certificates couldn't be imported. ";
 
-  std::string network_error;
-  chromeos::onc::ImportNetworksForUser(user, network_configs, &network_error);
-  if (!network_error.empty())
-    error += network_error;
-
-  chromeos::onc::CertificateImporterImpl cert_importer(
-      BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO), nssdb);
-  cert_importer.ImportCertificates(
-      certificates,
-      onc_source,
-      base::Bind(&NetInternalsMessageHandler::OnCertificatesImported,
-                 AsWeakPtr(),
-                 error));
-}
-
-void NetInternalsMessageHandler::OnCertificatesImported(
-    const std::string& previous_error,
-    bool success,
-    const net::CertificateList& /* unused onc_trusted_certificates */) {
-  std::string error = previous_error;
-  if (!success)
-    error += "Some certificates couldn't be imported. ";
+    std::string network_error;
+    chromeos::onc::ImportNetworksForUser(user, network_configs, &network_error);
+    if (!network_error.empty())
+      error += network_error;
+  } else {
+    error = "User not found.";
+  }
 
+  LOG_IF(ERROR, !error.empty()) << error;
   SendJavascriptCommand("receivedONCFileParse", new base::StringValue(error));
 }
 

chromeos/chromeos.gyp

@@ -475,6 +475,8 @@
         'network/fake_network_device_handler.h',
         'network/mock_managed_network_configuration_handler.cc',
         'network/mock_managed_network_configuration_handler.h',
+        'network/onc/mock_certificate_importer.cc',
+        'network/onc/mock_certificate_importer.h',
         'network/onc/onc_test_utils.cc',
         'network/onc/onc_test_utils.h',
         'system/mock_statistics_provider.cc',

chromeos/network/onc/mock_certificate_importer.cc

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chromeos/network/onc/mock_certificate_importer.h"
+
+namespace chromeos {
+namespace onc {
+
+MockCertificateImporter::MockCertificateImporter() {
+}
+
+MockCertificateImporter::~MockCertificateImporter() {
+}
+
+}  // namespace onc
+}  // namespace chromeos

chromeos/network/onc/mock_certificate_importer.h

+// Copyright 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_NETWORK_ONC_MOCK_CERTIFICATE_IMPORTER_H_
+#define CHROMEOS_NETWORK_ONC_MOCK_CERTIFICATE_IMPORTER_H_
+
+#include "base/basictypes.h"
+#include "base/values.h"
+#include "chromeos/chromeos_export.h"
+#include "chromeos/network/onc/onc_certificate_importer.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+namespace chromeos {
+namespace onc {
+
+class CHROMEOS_EXPORT MockCertificateImporter : public CertificateImporter {
+ public:
+  MockCertificateImporter();
+  virtual ~MockCertificateImporter();
+  MOCK_METHOD3(ImportCertificates, bool(const base::ListValue&,
+                                        ::onc::ONCSource,
+                                        net::CertificateList*));
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockCertificateImporter);
+};
+
+}  // namespace onc
+}  // namespace chromeos
+
+#endif  // CHROMEOS_NETWORK_ONC_MOCK_CERTIFICATE_IMPORTER_H_

chromeos/network/onc/onc_certificate_importer.h

@@ -6,7 +6,6 @@
 #define CHROMEOS_NETWORK_ONC_ONC_CERTIFICATE_IMPORTER_H_
 
 #include "base/basictypes.h"
-#include "base/callback_forward.h"
 #include "chromeos/chromeos_export.h"
 #include "components/onc/onc_constants.h"
 #include "net/cert/x509_certificate.h"
@@ -20,25 +19,20 @@ namespace onc {
 
 class CHROMEOS_EXPORT CertificateImporter {
  public:
-  typedef base::Callback<
-      void(bool success, const net::CertificateList& onc_trusted_certificates)>
-      DoneCallback;
-
   CertificateImporter() {}
   virtual ~CertificateImporter() {}
 
-  // Import |certificates|, which must be a list of ONC Certificate objects.
-  // Certificates are only imported with web trust for user imports. If the
-  // "Remove" field of a certificate is enabled, then removes the certificate
-  // from the store instead of importing.
-  // When the import is completed, |done_callback| will be called with |success|
-  // equal to true if all certificates were imported successfully.
-  // |onc_trusted_certificates| will contain the list of certificates that
-  // were imported and requested the TrustBit "Web".
-  // Never calls |done_callback| after this importer is destructed.
-  virtual void ImportCertificates(const base::ListValue& certificates,
-                                  ::onc::ONCSource source,
-                                  const DoneCallback& done_callback) = 0;
+  // Import the |certificates|, which must be a list of ONC Certificate objects.
+  // Certificates are only imported with web trust for user imports. If
+  // |onc_trusted_certificates| is not NULL, it will be filled with the list
+  // of certificates that requested the TrustBit "Web". If the "Remove" field of
+  // a certificate is enabled, then removes the certificate from the store
+  // instead of importing. Returns true if all certificates were imported
+  // successfully.
+  virtual bool ImportCertificates(
+      const base::ListValue& certificates,
+      ::onc::ONCSource source,
+      net::CertificateList* onc_trusted_certificates) = 0;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(CertificateImporter);

chromeos/network/onc/onc_certificate_importer_impl.cc

@@ -9,14 +9,7 @@
 #include <pk11pub.h>
 
 #include "base/base64.h"
-#include "base/bind.h"
-#include "base/bind_helpers.h"
-#include "base/callback.h"
-#include "base/location.h"
 #include "base/logging.h"
-#include "base/sequenced_task_runner.h"
-#include "base/single_thread_task_runner.h"
-#include "base/thread_task_runner_handle.h"
 #include "base/values.h"
 #include "chromeos/network/network_event_log.h"
 #include "chromeos/network/onc/onc_utils.h"
@@ -27,94 +20,64 @@
 #include "net/cert/nss_cert_database.h"
 #include "net/cert/x509_certificate.h"
 
+#define ONC_LOG_WARNING(message)                                \
+  NET_LOG_DEBUG("ONC Certificate Import Warning", message)
+#define ONC_LOG_ERROR(message)                                  \
+  NET_LOG_ERROR("ONC Certificate Import Error", message)
+
 namespace chromeos {
 namespace onc {
 
-namespace {
-
-void CallBackOnOriginLoop(
-    const scoped_refptr<base::SingleThreadTaskRunner>& origin_loop,
-    const CertificateImporter::DoneCallback& callback,
-    bool success,
-    const net::CertificateList& onc_trusted_certificates) {
-  origin_loop->PostTask(
-      FROM_HERE, base::Bind(callback, success, onc_trusted_certificates));
-}
-
-}  // namespace
-
 CertificateImporterImpl::CertificateImporterImpl(
-    const scoped_refptr<base::SequencedTaskRunner>& io_task_runner,
     net::NSSCertDatabase* target_nssdb)
-    : io_task_runner_(io_task_runner),
-      target_nssdb_(target_nssdb),
-      weak_factory_(this) {
+    : target_nssdb_(target_nssdb) {
   CHECK(target_nssdb);
 }
 
-CertificateImporterImpl::~CertificateImporterImpl() {
-}
-
-void CertificateImporterImpl::ImportCertificates(
+bool CertificateImporterImpl::ImportCertificates(
     const base::ListValue& certificates,
     ::onc::ONCSource source,
-    const DoneCallback& done_callback) {
+    net::CertificateList* onc_trusted_certificates) {
   VLOG(2) << "ONC file has " << certificates.GetSize() << " certificates";
-  // |done_callback| must only be called as long as |this| still exists.
-  // Thereforce, call back to |this|. This check of |this| must happen last and
-  // on the origin thread.
-  DoneCallback callback_to_this =
-      base::Bind(&CertificateImporterImpl::RunDoneCallback,
-                 weak_factory_.GetWeakPtr(),
-                 done_callback);
-
-  // |done_callback| must be called on the origin thread.
-  DoneCallback callback_on_origin_loop =
-      base::Bind(&CallBackOnOriginLoop,
-                 base::ThreadTaskRunnerHandle::Get(),
-                 callback_to_this);
-
-  // This is the actual function that imports the certificates.
-  base::Closure import_certs_callback =
-      base::Bind(&ParseAndStoreCertificates,
-                 source,
-                 callback_on_origin_loop,
-                 base::Owned(certificates.DeepCopy()),
-                 target_nssdb_);
-
-  // The NSSCertDatabase must be accessed on |io_task_runner_|
-  io_task_runner_->PostTask(FROM_HERE, import_certs_callback);
-}
 
-// static
-void CertificateImporterImpl::ParseAndStoreCertificates(
-    ::onc::ONCSource source,
-    const DoneCallback& done_callback,
-    base::ListValue* certificates,
-    net::NSSCertDatabase* nssdb) {
   // Web trust is only granted to certificates imported by the user.
   bool allow_trust_imports = source == ::onc::ONC_SOURCE_USER_IMPORT;
-  net::CertificateList onc_trusted_certificates;
+  if (!ParseAndStoreCertificates(allow_trust_imports,
+                                 certificates,
+                                 onc_trusted_certificates,
+                                 NULL)) {
+    LOG(ERROR) << "Cannot parse some of the certificates in the ONC from "
+               << onc::GetSourceAsString(source);
+    return false;
+  }
+  return true;
+}
+
+bool CertificateImporterImpl::ParseAndStoreCertificates(
+    bool allow_trust_imports,
+    const base::ListValue& certificates,
+    net::CertificateList* onc_trusted_certificates,
+    CertsByGUID* imported_server_and_ca_certs) {
   bool success = true;
-  for (size_t i = 0; i < certificates->GetSize(); ++i) {
+  for (size_t i = 0; i < certificates.GetSize(); ++i) {
     const base::DictionaryValue* certificate = NULL;
-    certificates->GetDictionary(i, &certificate);
+    certificates.GetDictionary(i, &certificate);
     DCHECK(certificate != NULL);
 
     VLOG(2) << "Parsing certificate at index " << i << ": " << *certificate;
 
     if (!ParseAndStoreCertificate(allow_trust_imports,
                                   *certificate,
-                                  nssdb,
-                                  &onc_trusted_certificates)) {
+                                  onc_trusted_certificates,
+                                  imported_server_and_ca_certs)) {
       success = false;
-      LOG(ERROR) << "Cannot parse certificate at index " << i;
+      ONC_LOG_ERROR(
+          base::StringPrintf("Cannot parse certificate at index %zu", i));
     } else {
       VLOG(2) << "Successfully imported certificate at index " << i;
     }
   }
-
-  done_callback.Run(success, onc_trusted_certificates);
+  return success;
 }
 
 // static
@@ -180,20 +143,11 @@ bool CertificateImporterImpl::DeleteCertAndKeyByNickname(
   return result;
 }
 
-void CertificateImporterImpl::RunDoneCallback(
-    const CertificateImporter::DoneCallback& callback,
-    bool success,
-    const net::CertificateList& onc_trusted_certificates) {
-  if (!success)
-    NET_LOG_ERROR("ONC Certificate Import Error", "");
-  callback.Run(success, onc_trusted_certificates);
-}
-
 bool CertificateImporterImpl::ParseAndStoreCertificate(
     bool allow_trust_imports,
     const base::DictionaryValue& certificate,
-    net::NSSCertDatabase* nssdb,
-    net::CertificateList* onc_trusted_certificates) {
+    net::CertificateList* onc_trusted_certificates,
+    CertsByGUID* imported_server_and_ca_certs) {
   // Get out the attributes of the given certificate.
   std::string guid;
   certificate.GetStringWithoutPathExpansion(::onc::certificate::kGUID, &guid);
@@ -202,8 +156,8 @@ bool CertificateImporterImpl::ParseAndStoreCertificate(
   bool remove = false;
   if (certificate.GetBooleanWithoutPathExpansion(::onc::kRemove, &remove) &&
       remove) {
-    if (!DeleteCertAndKeyByNickname(guid, nssdb)) {
-      LOG(ERROR) << "Unable to delete certificate";
+    if (!DeleteCertAndKeyByNickname(guid, target_nssdb_)) {
+      ONC_LOG_ERROR("Unable to delete certificate");
       return false;
     } else {
       return true;
@@ -220,10 +174,10 @@ bool CertificateImporterImpl::ParseAndStoreCertificate(
                                       cert_type,
                                       guid,
                                       certificate,
-                                      nssdb,
-                                      onc_trusted_certificates);
+                                      onc_trusted_certificates,
+                                      imported_server_and_ca_certs);
   } else if (cert_type == ::onc::certificate::kClient) {
-    return ParseClientCertificate(guid, certificate, nssdb);
+    return ParseClientCertificate(guid, certificate);
   }
 
   NOTREACHED();
@@ -235,8 +189,8 @@ bool CertificateImporterImpl::ParseServerOrCaCertificate(
     const std::string& cert_type,
     const std::string& guid,
     const base::DictionaryValue& certificate,
-    net::NSSCertDatabase* nssdb,
-    net::CertificateList* onc_trusted_certificates) {
+    net::CertificateList* onc_trusted_certificates,
+    CertsByGUID* imported_server_and_ca_certs) {
   bool web_trust_flag = false;
   const base::ListValue* trust_list = NULL;
   if (certificate.GetListWithoutPathExpansion(::onc::certificate::kTrustBits,
@@ -254,8 +208,8 @@ bool CertificateImporterImpl::ParseServerOrCaCertificate(
       } else {
         // Trust bits should only increase trust and never restrict. Thus,
         // ignoring unknown bits should be safe.
-        LOG(WARNING) << "Certificate contains unknown trust type "
-                     << trust_type;
+        ONC_LOG_WARNING("Certificate contains unknown trust type " +
+                        trust_type);
       }
     }
   }
@@ -263,7 +217,7 @@ bool CertificateImporterImpl::ParseServerOrCaCertificate(
   bool import_with_ssl_trust = false;
   if (web_trust_flag) {
     if (!allow_trust_imports)
-      LOG(WARNING) << "Web trust not granted for certificate: " << guid;
+      ONC_LOG_WARNING("Web trust not granted for certificate: " + guid);
     else
       import_with_ssl_trust = true;
   }
@@ -272,16 +226,17 @@ bool CertificateImporterImpl::ParseServerOrCaCertificate(
   if (!certificate.GetStringWithoutPathExpansion(::onc::certificate::kX509,
                                                  &x509_data) ||
       x509_data.empty()) {
-    LOG(ERROR) << "Certificate missing appropriate certificate data for type: "
-               << cert_type;
+    ONC_LOG_ERROR(
+        "Certificate missing appropriate certificate data for type: " +
+        cert_type);
     return false;
   }
 
   scoped_refptr<net::X509Certificate> x509_cert =
       DecodePEMCertificate(x509_data);
   if (!x509_cert.get()) {
-    LOG(ERROR) << "Unable to create certificate from PEM encoding, type: "
-               << cert_type;
+    ONC_LOG_ERROR("Unable to create certificate from PEM encoding, type: " +
+                  cert_type);
     return false;
   }
 
@@ -295,19 +250,21 @@ bool CertificateImporterImpl::ParseServerOrCaCertificate(
                                                  : net::CA_CERT;
     VLOG(1) << "Certificate is already installed.";
     net::NSSCertDatabase::TrustBits missing_trust_bits =
-        trust & ~nssdb->GetCertTrust(x509_cert.get(), net_cert_type);
+        trust & ~target_nssdb_->GetCertTrust(x509_cert.get(), net_cert_type);
     if (missing_trust_bits) {
       std::string error_reason;
       bool success = false;
-      if (nssdb->IsReadOnly(x509_cert.get())) {
+      if (target_nssdb_->IsReadOnly(x509_cert.get())) {
         error_reason = " Certificate is stored read-only.";
       } else {
-        success = nssdb->SetCertTrust(x509_cert.get(), net_cert_type, trust);
+        success = target_nssdb_->SetCertTrust(x509_cert.get(),
+                                              net_cert_type,
+                                              trust);
       }
       if (!success) {
-        LOG(ERROR) << "Certificate of type " << cert_type
-                   << " was already present, but trust couldn't be set."
-                   << error_reason;
+        ONC_LOG_ERROR("Certificate of type " + cert_type +
+                      " was already present, but trust couldn't be set." +
+                      error_reason);
       }
     }
   } else {
@@ -316,19 +273,21 @@ bool CertificateImporterImpl::ParseServerOrCaCertificate(
     net::NSSCertDatabase::ImportCertFailureList failures;
     bool success = false;
     if (cert_type == ::onc::certificate::kServer)
-      success = nssdb->ImportServerCert(cert_list, trust, &failures);
+      success = target_nssdb_->ImportServerCert(cert_list, trust, &failures);
     else  // Authority cert
-      success = nssdb->ImportCACerts(cert_list, trust, &failures);
+      success = target_nssdb_->ImportCACerts(cert_list, trust, &failures);
 
     if (!failures.empty()) {
       std::string error_string = net::ErrorToString(failures[0].net_error);
-      LOG(ERROR) << "Error ( " << error_string
-                 << " ) importing certificate of type " << cert_type;
+      ONC_LOG_ERROR(
+          base::StringPrintf("Error ( %s ) importing %s certificate",
+                             error_string.c_str(),
+                             cert_type.c_str()));
       return false;
     }
 
     if (!success) {
-      LOG(ERROR) << "Unknown error importing " << cert_type << " certificate.";
+      ONC_LOG_ERROR("Unknown error importing " + cert_type + " certificate.");
       return false;
     }
   }
@@ -336,53 +295,56 @@ bool CertificateImporterImpl::ParseServerOrCaCertificate(
   if (web_trust_flag && onc_trusted_certificates)
     onc_trusted_certificates->push_back(x509_cert);
 
+  if (imported_server_and_ca_certs)
+    (*imported_server_and_ca_certs)[guid] = x509_cert;
+
   return true;
 }
 
 bool CertificateImporterImpl::ParseClientCertificate(
     const std::string& guid,
-    const base::DictionaryValue& certificate,
-    net::NSSCertDatabase* nssdb) {
+    const base::DictionaryValue& certificate) {
   std::string pkcs12_data;
   if (!certificate.GetStringWithoutPathExpansion(::onc::certificate::kPKCS12,
                                                  &pkcs12_data) ||
       pkcs12_data.empty()) {
-    LOG(ERROR) << "PKCS12 data is missing for client certificate.";
+    ONC_LOG_ERROR("PKCS12 data is missing for client certificate.");
     return false;
   }
 
   std::string decoded_pkcs12;
   if (!base::Base64Decode(pkcs12_data, &decoded_pkcs12)) {
-    LOG(ERROR) << "Unable to base64 decode PKCS#12 data: \"" << pkcs12_data
-               << "\".";
+    ONC_LOG_ERROR(
+        "Unable to base64 decode PKCS#12 data: \"" + pkcs12_data + "\".");
     return false;
   }
 
   // Since this has a private key, always use the private module.
-  crypto::ScopedPK11Slot private_slot(nssdb->GetPrivateSlot());
+  crypto::ScopedPK11Slot private_slot(target_nssdb_->GetPrivateSlot());
   if (!private_slot)
     return false;
   scoped_refptr<net::CryptoModule> module(
       net::CryptoModule::CreateFromHandle(private_slot.get()));
   net::CertificateList imported_certs;
 
-  int import_result = nssdb->ImportFromPKCS12(
+  int import_result = target_nssdb_->ImportFromPKCS12(
       module.get(), decoded_pkcs12, base::string16(), false, &imported_certs);
   if (import_result != net::OK) {
     std::string error_string = net::ErrorToString(import_result);
-    LOG(ERROR) << "Unable to import client certificate, error: "
-               << error_string;
+    ONC_LOG_ERROR(
+        base::StringPrintf("Unable to import client certificate (error %s)",
+                           error_string.c_str()));
     return false;
   }
 
   if (imported_certs.size() == 0) {
-    LOG(WARNING) << "PKCS12 data contains no importable certificates.";
+    ONC_LOG_WARNING("PKCS12 data contains no importable certificates.");
     return true;
   }
 
   if (imported_certs.size() != 1) {
-    LOG(WARNING) << "ONC File: PKCS12 data contains more than one certificate. "
-                    "Only the first one will be imported.";
+    ONC_LOG_WARNING("ONC File: PKCS12 data contains more than one certificate. "
+                    "Only the first one will be imported.");
   }
 
   scoped_refptr<net::X509Certificate> cert_result = imported_certs[0];
@@ -397,7 +359,7 @@ bool CertificateImporterImpl::ParseClientCertificate(
     PK11_SetPrivateKeyNickname(private_key, const_cast<char*>(guid.c_str()));
     SECKEY_DestroyPrivateKey(private_key);
   } else {
-    LOG(WARNING) << "Unable to find private key for certificate.";
+    ONC_LOG_WARNING("Unable to find private key for certificate.");
   }
   return true;
 }

chromeos/network/onc/onc_certificate_importer_impl.h

@@ -12,7 +12,6 @@
 #include "base/basictypes.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/memory/weak_ptr.h"
 #include "chromeos/chromeos_export.h"
 #include "chromeos/network/onc/onc_certificate_importer.h"
 #include "components/onc/onc_constants.h"
@@ -20,8 +19,6 @@
 namespace base {
 class DictionaryValue;
 class ListValue;
-class SequencedTaskRunner;
-class SingleThreadTaskRunner;
 }
 
 namespace net {
@@ -34,36 +31,34 @@ namespace chromeos {
 namespace onc {
 
 // This class handles certificate imports from ONC (both policy and user
-// imports) into a certificate store. The GUID of Client certificates is stored
-// together with the certificate as Nickname. In contrast, Server and CA
+// imports) into the certificate store. The GUID of Client certificates is
+// stored together with the certificate as Nickname. In contrast, Server and CA
 // certificates are identified by their PEM and not by GUID.
 // TODO(pneubeck): Replace Nickname by PEM for Client
 // certificates. http://crbug.com/252119
 class CHROMEOS_EXPORT CertificateImporterImpl : public CertificateImporter {
  public:
-  // |io_task_runner| will be used for NSSCertDatabase accesses.
-  CertificateImporterImpl(
-      const scoped_refptr<base::SequencedTaskRunner>& io_task_runner,
-      net::NSSCertDatabase* target_nssdb_);
-  virtual ~CertificateImporterImpl();
+  typedef std::map<std::string, scoped_refptr<net::X509Certificate> >
+      CertsByGUID;
+
+  explicit CertificateImporterImpl(net::NSSCertDatabase* target_nssdb_);
 
   // CertificateImporter overrides
-  virtual void ImportCertificates(const base::ListValue& certificates,
-                                  ::onc::ONCSource source,
-                                  const DoneCallback& done_callback) OVERRIDE;
+  virtual bool ImportCertificates(
+      const base::ListValue& certificates,
+      ::onc::ONCSource source,
+      net::CertificateList* onc_trusted_certificates) OVERRIDE;
+
+  // This implements ImportCertificates. Additionally, if
+  // |imported_server_and_ca_certs| is not NULL, it will be filled with the
+  // (GUID, Certificate) pairs of all succesfully imported Server and CA
+  // certificates.
+  bool ParseAndStoreCertificates(bool allow_trust_imports,
+                                 const base::ListValue& onc_certificates,
+                                 net::CertificateList* onc_trusted_certificates,
+                                 CertsByGUID* imported_server_and_ca_certs);
 
  private:
-  void RunDoneCallback(const CertificateImporter::DoneCallback& callback,
-                       bool success,
-                       const net::CertificateList& onc_trusted_certificates);
-
-  // This is the synchronous implementation of ImportCertificates. It is
-  // executed on the given |io_task_runner_|.
-  static void ParseAndStoreCertificates(::onc::ONCSource source,
-                                        const DoneCallback& done_callback,
-                                        base::ListValue* certificates,
-                                        net::NSSCertDatabase* nssdb);
-
   // Lists the certificates that have the string |label| as their certificate
   // nickname (exact match).
   static void ListCertsWithNickname(const std::string& label,
@@ -77,36 +72,30 @@ class CHROMEOS_EXPORT CertificateImporterImpl : public CertificateImporter {
 
   // Parses and stores/removes |certificate| in/from the certificate
   // store. Returns true if the operation succeeded.
-  static bool ParseAndStoreCertificate(
+  bool ParseAndStoreCertificate(
       bool allow_trust_imports,
       const base::DictionaryValue& certificate,
-      net::NSSCertDatabase* nssdb,
-      net::CertificateList* onc_trusted_certificates);
+      net::CertificateList* onc_trusted_certificates,
+      CertsByGUID* imported_server_and_ca_certs);
 
   // Imports the Server or CA certificate |certificate|. Web trust is only
   // applied if the certificate requests the TrustBits attribute "Web" and if
   // the |allow_trust_imports| permission is granted, otherwise the attribute is
   // ignored.
-  static bool ParseServerOrCaCertificate(
+  bool ParseServerOrCaCertificate(
       bool allow_trust_imports,
       const std::string& cert_type,
       const std::string& guid,
       const base::DictionaryValue& certificate,
-      net::NSSCertDatabase* nssdb,
-      net::CertificateList* onc_trusted_certificates);
+      net::CertificateList* onc_trusted_certificates,
+      CertsByGUID* imported_server_and_ca_certs);
 
-  static bool ParseClientCertificate(const std::string& guid,
-                                     const base::DictionaryValue& certificate,
-                                     net::NSSCertDatabase* nssdb);
-
-  // The task runner to use for NSSCertDatabase accesses.
-  scoped_refptr<base::SequencedTaskRunner> io_task_runner_;
+  bool ParseClientCertificate(const std::string& guid,
+                              const base::DictionaryValue& certificate);
 
   // The certificate database to which certificates are imported.
   net::NSSCertDatabase* target_nssdb_;
 
-  base::WeakPtrFactory<CertificateImporterImpl> weak_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(CertificateImporterImpl);
 };
 

chromeos/network/onc/onc_certificate_importer_impl_unittest.cc

@@ -13,12 +13,11 @@
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
-#include "base/test/test_simple_task_runner.h"
-#include "base/thread_task_runner_handle.h"
 #include "base/values.h"
 #include "chromeos/network/onc/onc_test_utils.h"
 #include "components/onc/onc_constants.h"
-#include "crypto/scoped_test_nss_db.h"
+#include "crypto/nss_util_internal.h"
+#include "crypto/scoped_test_nss_chromeos_user.h"
 #include "net/base/crypto_module.h"
 #include "net/cert/cert_type.h"
 #include "net/cert/nss_cert_database_chromeos.h"
@@ -64,39 +63,30 @@ net::CertType GetCertType(net::X509Certificate::OSCertHandle cert) {
 
 class ONCCertificateImporterImplTest : public testing::Test {
  public:
-  ONCCertificateImporterImplTest() {}
-  virtual ~ONCCertificateImporterImplTest() {}
-
-  virtual void SetUp() OVERRIDE {
-    ASSERT_TRUE(public_nssdb_.is_open());
-    ASSERT_TRUE(private_nssdb_.is_open());
-
-    task_runner_ = new base::TestSimpleTaskRunner();
-    thread_task_runner_handle_.reset(
-        new base::ThreadTaskRunnerHandle(task_runner_));
-
+  ONCCertificateImporterImplTest() : user_("username_hash"),
+                                     private_user_("private_user_hash") {}
+
+  virtual void SetUp() {
+    ASSERT_TRUE(user_.constructed_successfully());
+    ASSERT_TRUE(private_user_.constructed_successfully());
+
+    // By default test user will have the same public and private slot.
+    // Unfortunatelly, ONC importer should care about which slot certificates
+    // get imported to. To work around this, we create another NSS user whose
+    // public slot will act as the private slot.
+    // TODO(tbarzic): See if there's a better way to achieve this.
     test_nssdb_.reset(new net::NSSCertDatabaseChromeOS(
-        crypto::ScopedPK11Slot(public_nssdb_.slot()),
-        crypto::ScopedPK11Slot(private_nssdb_.slot())));
+        crypto::GetPublicSlotForChromeOSUser(user_.username_hash()),
+        crypto::GetPublicSlotForChromeOSUser(private_user_.username_hash())));
 
     // Test db should be empty at start of test.
     EXPECT_TRUE(ListCertsInPublicSlot().empty());
     EXPECT_TRUE(ListCertsInPrivateSlot().empty());
   }
 
-  virtual void TearDown() OVERRIDE {
-    thread_task_runner_handle_.reset();
-    task_runner_ = NULL;
-  }
+  virtual ~ONCCertificateImporterImplTest() {}
 
  protected:
-  void OnImportCompleted(bool expected_success,
-                         bool success,
-                         const net::CertificateList& onc_trusted_certificates) {
-    EXPECT_EQ(expected_success, success);
-    web_trust_certificates_ = onc_trusted_certificates;
-  }
-
   void AddCertificatesFromFile(std::string filename, bool expected_success) {
     scoped_ptr<base::DictionaryValue> onc =
         test_utils::ReadTestDictionary(filename);
@@ -108,15 +98,14 @@ class ONCCertificateImporterImplTest : public testing::Test {
     onc_certificates_.reset(certificates);
 
     web_trust_certificates_.clear();
-    CertificateImporterImpl importer(task_runner_, test_nssdb_.get());
-    importer.ImportCertificates(
-        *certificates,
-        ::onc::ONC_SOURCE_USER_IMPORT,  // allow web trust
-        base::Bind(&ONCCertificateImporterImplTest::OnImportCompleted,
-                   base::Unretained(this),
-                   expected_success));
-
-    task_runner_->RunUntilIdle();
+    imported_server_and_ca_certs_.clear();
+    CertificateImporterImpl importer(test_nssdb_.get());
+    EXPECT_EQ(
+        expected_success,
+        importer.ParseAndStoreCertificates(true,  // allow web trust
+                                           *certificates,
+                                           &web_trust_certificates_,
+                                           &imported_server_and_ca_certs_));
 
     public_list_ = ListCertsInPublicSlot();
     private_list_ = ListCertsInPrivateSlot();
@@ -130,29 +119,25 @@ class ONCCertificateImporterImplTest : public testing::Test {
       guid = &guid_temporary;
 
     AddCertificatesFromFile(filename, true);
-
-    if (expected_type == net::SERVER_CERT || expected_type == net::CA_CERT) {
-      ASSERT_EQ(1u, public_list_.size());
+    ASSERT_EQ(1ul, public_list_.size() + private_list_.size());
+    if (!public_list_.empty())
       EXPECT_EQ(expected_type, GetCertType(public_list_[0]->os_cert_handle()));
-      EXPECT_TRUE(private_list_.empty());
-    } else {  // net::USER_CERT
-      EXPECT_TRUE(public_list_.empty());
-      ASSERT_EQ(1u, private_list_.size());
+    if (!private_list_.empty())
       EXPECT_EQ(expected_type, GetCertType(private_list_[0]->os_cert_handle()));
-    }
 
     base::DictionaryValue* certificate = NULL;
     onc_certificates_->GetDictionary(0, &certificate);
     certificate->GetStringWithoutPathExpansion(::onc::certificate::kGUID, guid);
-  }
 
-  // Certificates and the NSSCertDatabase depend on these test DBs. Destroy them
-  // last.
-  crypto::ScopedTestNSSDB public_nssdb_;
-  crypto::ScopedTestNSSDB private_nssdb_;
+    if (expected_type == net::SERVER_CERT || expected_type == net::CA_CERT) {
+      EXPECT_EQ(1u, imported_server_and_ca_certs_.size());
+      EXPECT_TRUE(
+          imported_server_and_ca_certs_[*guid]->Equals(public_list_[0].get()));
+    } else {  // net::USER_CERT
+      EXPECT_TRUE(imported_server_and_ca_certs_.empty());
+    }
+  }
 
-  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
-  scoped_ptr<base::ThreadTaskRunnerHandle> thread_task_runner_handle_;
   scoped_ptr<net::NSSCertDatabaseChromeOS> test_nssdb_;
   scoped_ptr<base::ListValue> onc_certificates_;
   // List of certs in the nssdb's public slot.
@@ -160,14 +145,15 @@ class ONCCertificateImporterImplTest : public testing::Test {
   // List of certs in the nssdb's "private" slot.
   net::CertificateList private_list_;
   net::CertificateList web_trust_certificates_;
+  CertificateImporterImpl::CertsByGUID imported_server_and_ca_certs_;
 
  private:
   net::CertificateList ListCertsInPublicSlot() {
-    return ListCertsInSlot(public_nssdb_.slot());
+    return ListCertsInSlot(test_nssdb_->GetPublicSlot().get());
   }
 
   net::CertificateList ListCertsInPrivateSlot() {
-    return ListCertsInSlot(private_nssdb_.slot());
+    return ListCertsInSlot(test_nssdb_->GetPrivateSlot().get());
   }
 
   net::CertificateList ListCertsInSlot(PK11SlotInfo* slot) {
@@ -185,13 +171,16 @@ class ONCCertificateImporterImplTest : public testing::Test {
     std::sort(result.begin(), result.end(), net::X509Certificate::LessThan());
     return result;
   }
+
+  crypto::ScopedTestNSSChromeOSUser user_;
+  crypto::ScopedTestNSSChromeOSUser private_user_;
 };
 
 TEST_F(ONCCertificateImporterImplTest, MultipleCertificates) {
   AddCertificatesFromFile("managed_toplevel2.onc", true);
   EXPECT_EQ(onc_certificates_->GetSize(), public_list_.size());
   EXPECT_TRUE(private_list_.empty());
-  EXPECT_EQ(2ul, public_list_.size());
+  EXPECT_EQ(2ul, imported_server_and_ca_certs_.size());
 }
 
 TEST_F(ONCCertificateImporterImplTest, MultipleCertificatesWithFailures) {
@@ -199,6 +188,7 @@ TEST_F(ONCCertificateImporterImplTest, MultipleCertificatesWithFailures) {
   EXPECT_EQ(3ul, onc_certificates_->GetSize());
   EXPECT_EQ(1ul, private_list_.size());
   EXPECT_TRUE(public_list_.empty());
+  EXPECT_TRUE(imported_server_and_ca_certs_.empty());
 }
 
 TEST_F(ONCCertificateImporterImplTest, AddClientCertificate) {
@@ -209,7 +199,7 @@ TEST_F(ONCCertificateImporterImplTest, AddClientCertificate) {
   EXPECT_TRUE(public_list_.empty());
 
   SECKEYPrivateKeyList* privkey_list =
-      PK11_ListPrivKeysInSlot(private_nssdb_.slot(), NULL, NULL);
+      PK11_ListPrivKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL, NULL);
   EXPECT_TRUE(privkey_list);
   if (privkey_list) {
     SECKEYPrivateKeyListNode* node = PRIVKEY_LIST_HEAD(privkey_list);
@@ -226,7 +216,7 @@ TEST_F(ONCCertificateImporterImplTest, AddClientCertificate) {
   }
 
   SECKEYPublicKeyList* pubkey_list =
-      PK11_ListPublicKeysInSlot(private_nssdb_.slot(), NULL);
+      PK11_ListPublicKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL);
   EXPECT_TRUE(pubkey_list);
   if (pubkey_list) {
     SECKEYPublicKeyListNode* node = PUBKEY_LIST_HEAD(pubkey_list);
@@ -244,11 +234,11 @@ TEST_F(ONCCertificateImporterImplTest, AddServerCertificateWithWebTrust) {
   AddCertificateFromFile("certificate-server.onc", net::SERVER_CERT, NULL);
 
   SECKEYPrivateKeyList* privkey_list =
-      PK11_ListPrivKeysInSlot(private_nssdb_.slot(), NULL, NULL);
+      PK11_ListPrivKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL, NULL);
   EXPECT_FALSE(privkey_list);
 
   SECKEYPublicKeyList* pubkey_list =
-      PK11_ListPublicKeysInSlot(private_nssdb_.slot(), NULL);
+      PK11_ListPublicKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL);
   EXPECT_FALSE(pubkey_list);
 
   ASSERT_EQ(1u, web_trust_certificates_.size());
@@ -262,11 +252,11 @@ TEST_F(ONCCertificateImporterImplTest, AddWebAuthorityCertificateWithWebTrust) {
   AddCertificateFromFile("certificate-web-authority.onc", net::CA_CERT, NULL);
 
   SECKEYPrivateKeyList* privkey_list =
-      PK11_ListPrivKeysInSlot(private_nssdb_.slot(), NULL, NULL);
+      PK11_ListPrivKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL, NULL);
   EXPECT_FALSE(privkey_list);
 
   SECKEYPublicKeyList* pubkey_list =
-      PK11_ListPublicKeysInSlot(private_nssdb_.slot(), NULL);
+      PK11_ListPublicKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL);
   EXPECT_FALSE(pubkey_list);
 
   ASSERT_EQ(1u, web_trust_certificates_.size());
@@ -281,11 +271,11 @@ TEST_F(ONCCertificateImporterImplTest, AddAuthorityCertificateWithoutWebTrust) {
   EXPECT_TRUE(web_trust_certificates_.empty());
 
   SECKEYPrivateKeyList* privkey_list =
-      PK11_ListPrivKeysInSlot(private_nssdb_.slot(), NULL, NULL);
+      PK11_ListPrivKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL, NULL);
   EXPECT_FALSE(privkey_list);
 
   SECKEYPublicKeyList* pubkey_list =
-      PK11_ListPublicKeysInSlot(private_nssdb_.slot(), NULL);
+      PK11_ListPublicKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL);
   EXPECT_FALSE(pubkey_list);
 }