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

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

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

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/mock_certificate_importer.h"
+#include "chromeos/network/onc/onc_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,11 +71,16 @@ 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 {
@@ -87,6 +92,56 @@ 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[] =
@@ -145,8 +200,7 @@ ACTION_P(SetCertificateList, list) {
 
 class NetworkConfigurationUpdaterTest : public testing::Test {
  protected:
-  NetworkConfigurationUpdaterTest() {
-  }
+  NetworkConfigurationUpdaterTest() : certificate_importer_(NULL) {}
 
   virtual void SetUp() OVERRIDE {
     EXPECT_CALL(provider_, IsInitializationComplete(_))
@@ -174,8 +228,7 @@ class NetworkConfigurationUpdaterTest : public testing::Test {
         onc::toplevel_config::kCertificates, &certs);
     AppendAll(*certs, &fake_certificates_);
 
-    certificate_importer_ =
-        new StrictMock<chromeos::onc::MockCertificateImporter>();
+    certificate_importer_ = new FakeCertificateImporter;
     certificate_importer_owned_.reset(certificate_importer_);
   }
 
@@ -243,7 +296,7 @@ class NetworkConfigurationUpdaterTest : public testing::Test {
   // NetworkConfigurationUpdater. When that happens, |certificate_importer_|
   // continues to point to that instance but |certificate_importer_owned_| is
   // released.
-  StrictMock<chromeos::onc::MockCertificateImporter>* certificate_importer_;
+  FakeCertificateImporter* certificate_importer_;
   scoped_ptr<chromeos::onc::CertificateImporter> certificate_importer_owned_;
 
   StrictMock<MockConfigurationPolicyProvider> provider_;
@@ -316,13 +369,13 @@ TEST_F(NetworkConfigurationUpdaterTest, PolicyIsValidatedAndRepaired) {
                         _,
                         IsEqualTo(network_configs_repaired),
                         IsEqualTo(global_config_repaired)));
-  EXPECT_CALL(*certificate_importer_,
-              ImportCertificates(_, onc::ONC_SOURCE_USER_POLICY,  _));
+  certificate_importer_->SetExpectedONCSource(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,
@@ -336,8 +389,7 @@ TEST_F(NetworkConfigurationUpdaterTest,
 
   EXPECT_CALL(network_config_handler_,
               SetPolicy(onc::ONC_SOURCE_USER_POLICY, _, _, _));
-  EXPECT_CALL(*certificate_importer_, ImportCertificates(_, _, _))
-      .WillRepeatedly(SetCertificateList(cert_list));
+  certificate_importer_->SetTrustedCertificatesResult(cert_list);
 
   UserNetworkConfigurationUpdater* updater =
       CreateNetworkConfigurationUpdaterForUserPolicy(
@@ -373,9 +425,8 @@ TEST_F(NetworkConfigurationUpdaterTest,
                                          net::X509Certificate::FORMAT_AUTO);
   ASSERT_EQ(1u, cert_list.size());
 
-  EXPECT_CALL(*certificate_importer_,
-              ImportCertificates(_, onc::ONC_SOURCE_USER_POLICY, _))
-      .WillRepeatedly(SetCertificateList(cert_list));
+  certificate_importer_->SetExpectedONCSource(onc::ONC_SOURCE_USER_POLICY);
+  certificate_importer_->SetTrustedCertificatesResult(cert_list);
 
   UserNetworkConfigurationUpdater* updater =
       CreateNetworkConfigurationUpdaterForUserPolicy(
@@ -398,10 +449,6 @@ 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 */,
@@ -414,7 +461,6 @@ 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);
@@ -429,10 +475,7 @@ TEST_F(NetworkConfigurationUpdaterTest,
                                          "ok_cert.pem",
                                          net::X509Certificate::FORMAT_AUTO);
   ASSERT_EQ(1u, cert_list.size());
-
-  EXPECT_CALL(*certificate_importer_,
-              ImportCertificates(_, onc::ONC_SOURCE_USER_POLICY, _))
-      .WillOnce(SetCertificateList(cert_list));
+  certificate_importer_->SetTrustedCertificatesResult(cert_list);
 
   // Change to any non-empty policy, so that updates are triggered. The actual
   // content of the policy is irrelevant.
@@ -469,7 +512,6 @@ TEST_F(NetworkConfigurationUpdaterTest,
                         kFakeUsernameHash,
                         IsEqualTo(&fake_network_configs_),
                         IsEqualTo(&fake_global_network_config_)));
-  EXPECT_CALL(*certificate_importer_, ImportCertificates(_, _ , _)).Times(0);
 
   UserNetworkConfigurationUpdater* updater =
       CreateNetworkConfigurationUpdaterForUserPolicy(
@@ -478,21 +520,14 @@ TEST_F(NetworkConfigurationUpdaterTest,
   MarkPolicyProviderInitialized();
 
   Mock::VerifyAndClearExpectations(&network_config_handler_);
-  Mock::VerifyAndClearExpectations(certificate_importer_);
+  EXPECT_EQ(0u, certificate_importer_->GetAndResetImportCount());
 
-  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,
-                                  _));
+  certificate_importer_->SetExpectedONCCertificates(fake_certificates_);
+  certificate_importer_->SetExpectedONCSource(onc::ONC_SOURCE_USER_POLICY);
 
   ASSERT_TRUE(certificate_importer_owned_);
   updater->SetCertificateImporterForTest(certificate_importer_owned_.Pass());
+  EXPECT_EQ(1u, certificate_importer_->GetAndResetImportCount());
 }
 
 class NetworkConfigurationUpdaterTestWithParam
@@ -543,14 +578,13 @@ TEST_P(NetworkConfigurationUpdaterTestWithParam, InitialUpdates) {
                         ExpectedUsernameHash(),
                         IsEqualTo(&fake_network_configs_),
                         IsEqualTo(&fake_global_network_config_)));
-  EXPECT_CALL(*certificate_importer_,
-              ImportCertificates(IsEqualTo(&fake_certificates_),
-                                 CurrentONCSource(),
-                                  _))
-      .Times(ExpectedImportCertificatesCallCount());
+  certificate_importer_->SetExpectedONCCertificates(fake_certificates_);
+  certificate_importer_->SetExpectedONCSource(CurrentONCSource());
 
   CreateNetworkConfigurationUpdater();
   MarkPolicyProviderInitialized();
+  EXPECT_EQ(ExpectedImportCertificatesCallCount(),
+            certificate_importer_->GetAndResetImportCount());
 }
 
 TEST_P(NetworkConfigurationUpdaterTestWithParam,
@@ -560,35 +594,22 @@ 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_);
-  Mock::VerifyAndClearExpectations(certificate_importer_);
+  EXPECT_EQ(0u, certificate_importer_->GetAndResetImportCount());
 
   EXPECT_CALL(network_config_handler_,
               SetPolicy(CurrentONCSource(),
                         ExpectedUsernameHash(),
                         IsEqualTo(&fake_network_configs_),
                         IsEqualTo(&fake_global_network_config_)));
-  EXPECT_CALL(*certificate_importer_,
-              ImportCertificates(IsEqualTo(&fake_certificates_),
-                                 CurrentONCSource(),
-                                  _))
-      .Times(ExpectedImportCertificatesCallCount());
+  certificate_importer_->SetExpectedONCSource(CurrentONCSource());
+  certificate_importer_->SetExpectedONCCertificates(fake_certificates_);
 
   MarkPolicyProviderInitialized();
+  EXPECT_EQ(ExpectedImportCertificatesCallCount(),
+            certificate_importer_->GetAndResetImportCount());
 }
 
 TEST_P(NetworkConfigurationUpdaterTestWithParam,
@@ -605,26 +626,25 @@ TEST_P(NetworkConfigurationUpdaterTestWithParam,
                         ExpectedUsernameHash(),
                         IsEqualTo(&fake_network_configs_),
                         IsEqualTo(&fake_global_network_config_)));
-  EXPECT_CALL(*certificate_importer_,
-              ImportCertificates(IsEqualTo(&fake_certificates_),
-                                 CurrentONCSource(),
-                                  _))
-      .Times(ExpectedImportCertificatesCallCount());
+  certificate_importer_->SetExpectedONCSource(CurrentONCSource());
+  certificate_importer_->SetExpectedONCCertificates(fake_certificates_);
 
   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_);
-  Mock::VerifyAndClearExpectations(certificate_importer_);
+  EXPECT_LE(ExpectedImportCertificatesCallCount(),
+            certificate_importer_->GetAndResetImportCount());
 
   // The Updater should update if policy changes.
   EXPECT_CALL(network_config_handler_,
@@ -632,28 +652,26 @@ TEST_P(NetworkConfigurationUpdaterTestWithParam, PolicyChange) {
                         _,
                         IsEqualTo(&fake_network_configs_),
                         IsEqualTo(&fake_global_network_config_)));
-  EXPECT_CALL(*certificate_importer_,
-              ImportCertificates(IsEqualTo(&fake_certificates_),
-                                 CurrentONCSource(),
-                                  _))
-      .Times(ExpectedImportCertificatesCallCount());
+  certificate_importer_->SetExpectedONCSource(CurrentONCSource());
+  certificate_importer_->SetExpectedONCCertificates(fake_certificates_);
 
   PolicyMap policy;
   policy.Set(GetParam(), POLICY_LEVEL_MANDATORY, POLICY_SCOPE_USER,
              new base::StringValue(kFakeONC), NULL);
   UpdateProviderPolicy(policy);
   Mock::VerifyAndClearExpectations(&network_config_handler_);
-  Mock::VerifyAndClearExpectations(certificate_importer_);
+  EXPECT_EQ(ExpectedImportCertificatesCallCount(),
+            certificate_importer_->GetAndResetImportCount());
 
   // Another update is expected if the policy goes away.
   EXPECT_CALL(network_config_handler_,
               SetPolicy(CurrentONCSource(), _, IsEmpty(), IsEmpty()));
-  EXPECT_CALL(*certificate_importer_,
-              ImportCertificates(IsEmpty(), CurrentONCSource(), _))
-      .Times(ExpectedImportCertificatesCallCount());
+  certificate_importer_->SetExpectedONCCertificates(base::ListValue());
 
   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,6 +15,7 @@
 #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"
@@ -84,6 +85,15 @@ 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
@@ -93,13 +103,11 @@ void UserNetworkConfigurationUpdater::ImportCertificates(
     return;
   }
 
-  web_trust_certs_.clear();
   certificate_importer_->ImportCertificates(
       certificates_onc,
       onc_source_,
-      allow_trusted_certificates_from_policy_ ? &web_trust_certs_ : NULL);
-
-  NotifyTrustAnchorsChanged();
+      base::Bind(&UserNetworkConfigurationUpdater::OnCertificatesImported,
+                 base::Unretained(this)));
 }
 
 void UserNetworkConfigurationUpdater::ApplyNetworkPolicy(
@@ -132,7 +140,10 @@ void UserNetworkConfigurationUpdater::CreateAndSetCertificateImporter(
     net::NSSCertDatabase* database) {
   DCHECK(database);
   SetCertificateImporter(scoped_ptr<chromeos::onc::CertificateImporter>(
-      new chromeos::onc::CertificateImporterImpl(database)));
+      new chromeos::onc::CertificateImporterImpl(
+          content::BrowserThread::GetMessageLoopProxyForThread(
+              content::BrowserThread::IO),
+          database)));
 }
 
 void UserNetworkConfigurationUpdater::SetCertificateImporter(

chrome/browser/chromeos/policy/user_network_configuration_updater.h

@@ -95,6 +95,11 @@ 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,6 +257,13 @@ 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:
@@ -1407,38 +1414,52 @@ 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) {
-    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. ";
+  if (!user) {
+    std::string error = "User not found.";
+    SendJavascriptCommand("receivedONCFileParse", new base::StringValue(error));
+    return;
+  }
 
-    std::string network_error;
-    chromeos::onc::ImportNetworksForUser(user, network_configs, &network_error);
-    if (!network_error.empty())
-      error += network_error;
-  } else {
-    error = "User not found.";
+  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. ";
   }
 
-  LOG_IF(ERROR, !error.empty()) << error;
+  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. ";
+
   SendJavascriptCommand("receivedONCFileParse", new base::StringValue(error));
 }
 

chromeos/chromeos.gyp

@@ -475,8 +475,6 @@
         '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,6 +6,7 @@
 #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"
@@ -19,20 +20,25 @@ namespace onc {
 
 class CHROMEOS_EXPORT CertificateImporter {
  public:
+  typedef base::Callback<
+      void(bool success, const net::CertificateList& onc_trusted_certificates)>
+      DoneCallback;
+
   CertificateImporter() {}
   virtual ~CertificateImporter() {}
 
-  // 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;
+  // 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;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(CertificateImporter);

chromeos/network/onc/onc_certificate_importer_impl.cc

@@ -9,7 +9,14 @@
 #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"
@@ -20,64 +27,94 @@
 #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)
-    : target_nssdb_(target_nssdb) {
+    : io_task_runner_(io_task_runner),
+      target_nssdb_(target_nssdb),
+      weak_factory_(this) {
   CHECK(target_nssdb);
 }
 
-bool CertificateImporterImpl::ImportCertificates(
+CertificateImporterImpl::~CertificateImporterImpl() {
+}
+
+void CertificateImporterImpl::ImportCertificates(
     const base::ListValue& certificates,
     ::onc::ONCSource source,
-    net::CertificateList* onc_trusted_certificates) {
+    const DoneCallback& done_callback) {
   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;
-  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) {
+  net::CertificateList onc_trusted_certificates;
   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,
-                                  onc_trusted_certificates,
-                                  imported_server_and_ca_certs)) {
+                                  nssdb,
+                                  &onc_trusted_certificates)) {
       success = false;
-      ONC_LOG_ERROR(
-          base::StringPrintf("Cannot parse certificate at index %zu", i));
+      LOG(ERROR) << "Cannot parse certificate at index " << i;
     } else {
       VLOG(2) << "Successfully imported certificate at index " << i;
     }
   }
-  return success;
+
+  done_callback.Run(success, onc_trusted_certificates);
 }
 
 // static
@@ -143,11 +180,20 @@ 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::CertificateList* onc_trusted_certificates,
-    CertsByGUID* imported_server_and_ca_certs) {
+    net::NSSCertDatabase* nssdb,
+    net::CertificateList* onc_trusted_certificates) {
   // Get out the attributes of the given certificate.
   std::string guid;
   certificate.GetStringWithoutPathExpansion(::onc::certificate::kGUID, &guid);
@@ -156,8 +202,8 @@ bool CertificateImporterImpl::ParseAndStoreCertificate(
   bool remove = false;
   if (certificate.GetBooleanWithoutPathExpansion(::onc::kRemove, &remove) &&
       remove) {
-    if (!DeleteCertAndKeyByNickname(guid, target_nssdb_)) {
-      ONC_LOG_ERROR("Unable to delete certificate");
+    if (!DeleteCertAndKeyByNickname(guid, nssdb)) {
+      LOG(ERROR) << "Unable to delete certificate";
       return false;
     } else {
       return true;
@@ -174,10 +220,10 @@ bool CertificateImporterImpl::ParseAndStoreCertificate(
                                       cert_type,
                                       guid,
                                       certificate,
-                                      onc_trusted_certificates,
-                                      imported_server_and_ca_certs);
+                                      nssdb,
+                                      onc_trusted_certificates);
   } else if (cert_type == ::onc::certificate::kClient) {
-    return ParseClientCertificate(guid, certificate);
+    return ParseClientCertificate(guid, certificate, nssdb);
   }
 
   NOTREACHED();
@@ -189,8 +235,8 @@ bool CertificateImporterImpl::ParseServerOrCaCertificate(
     const std::string& cert_type,
     const std::string& guid,
     const base::DictionaryValue& certificate,
-    net::CertificateList* onc_trusted_certificates,
-    CertsByGUID* imported_server_and_ca_certs) {
+    net::NSSCertDatabase* nssdb,
+    net::CertificateList* onc_trusted_certificates) {
   bool web_trust_flag = false;
   const base::ListValue* trust_list = NULL;
   if (certificate.GetListWithoutPathExpansion(::onc::certificate::kTrustBits,
@@ -208,8 +254,8 @@ bool CertificateImporterImpl::ParseServerOrCaCertificate(
       } else {
         // Trust bits should only increase trust and never restrict. Thus,
         // ignoring unknown bits should be safe.
-        ONC_LOG_WARNING("Certificate contains unknown trust type " +
-                        trust_type);
+        LOG(WARNING) << "Certificate contains unknown trust type "
+                     << trust_type;
       }
     }
   }
@@ -217,7 +263,7 @@ bool CertificateImporterImpl::ParseServerOrCaCertificate(
   bool import_with_ssl_trust = false;
   if (web_trust_flag) {
     if (!allow_trust_imports)
-      ONC_LOG_WARNING("Web trust not granted for certificate: " + guid);
+      LOG(WARNING) << "Web trust not granted for certificate: " << guid;
     else
       import_with_ssl_trust = true;
   }
@@ -226,17 +272,16 @@ bool CertificateImporterImpl::ParseServerOrCaCertificate(
   if (!certificate.GetStringWithoutPathExpansion(::onc::certificate::kX509,
                                                  &x509_data) ||
       x509_data.empty()) {
-    ONC_LOG_ERROR(
-        "Certificate missing appropriate certificate data for type: " +
-        cert_type);
+    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()) {
-    ONC_LOG_ERROR("Unable to create certificate from PEM encoding, type: " +
-                  cert_type);
+    LOG(ERROR) << "Unable to create certificate from PEM encoding, type: "
+               << cert_type;
     return false;
   }
 
@@ -250,21 +295,19 @@ bool CertificateImporterImpl::ParseServerOrCaCertificate(
                                                  : net::CA_CERT;
     VLOG(1) << "Certificate is already installed.";
     net::NSSCertDatabase::TrustBits missing_trust_bits =
-        trust & ~target_nssdb_->GetCertTrust(x509_cert.get(), net_cert_type);
+        trust & ~nssdb->GetCertTrust(x509_cert.get(), net_cert_type);
     if (missing_trust_bits) {
       std::string error_reason;
       bool success = false;
-      if (target_nssdb_->IsReadOnly(x509_cert.get())) {
+      if (nssdb->IsReadOnly(x509_cert.get())) {
         error_reason = " Certificate is stored read-only.";
       } else {
-        success = target_nssdb_->SetCertTrust(x509_cert.get(),
-                                              net_cert_type,
-                                              trust);
+        success = nssdb->SetCertTrust(x509_cert.get(), net_cert_type, trust);
       }
       if (!success) {
-        ONC_LOG_ERROR("Certificate of type " + cert_type +
-                      " was already present, but trust couldn't be set." +
-                      error_reason);
+        LOG(ERROR) << "Certificate of type " << cert_type
+                   << " was already present, but trust couldn't be set."
+                   << error_reason;
       }
     }
   } else {
@@ -273,21 +316,19 @@ bool CertificateImporterImpl::ParseServerOrCaCertificate(
     net::NSSCertDatabase::ImportCertFailureList failures;
     bool success = false;
     if (cert_type == ::onc::certificate::kServer)
-      success = target_nssdb_->ImportServerCert(cert_list, trust, &failures);
+      success = nssdb->ImportServerCert(cert_list, trust, &failures);
     else  // Authority cert
-      success = target_nssdb_->ImportCACerts(cert_list, trust, &failures);
+      success = nssdb->ImportCACerts(cert_list, trust, &failures);
 
     if (!failures.empty()) {
       std::string error_string = net::ErrorToString(failures[0].net_error);
-      ONC_LOG_ERROR(
-          base::StringPrintf("Error ( %s ) importing %s certificate",
-                             error_string.c_str(),
-                             cert_type.c_str()));
+      LOG(ERROR) << "Error ( " << error_string
+                 << " ) importing certificate of type " << cert_type;
       return false;
     }
 
     if (!success) {
-      ONC_LOG_ERROR("Unknown error importing " + cert_type + " certificate.");
+      LOG(ERROR) << "Unknown error importing " << cert_type << " certificate.";
       return false;
     }
   }
@@ -295,56 +336,53 @@ 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) {
+    const base::DictionaryValue& certificate,
+    net::NSSCertDatabase* nssdb) {
   std::string pkcs12_data;
   if (!certificate.GetStringWithoutPathExpansion(::onc::certificate::kPKCS12,
                                                  &pkcs12_data) ||
       pkcs12_data.empty()) {
-    ONC_LOG_ERROR("PKCS12 data is missing for client certificate.");
+    LOG(ERROR) << "PKCS12 data is missing for client certificate.";
     return false;
   }
 
   std::string decoded_pkcs12;
   if (!base::Base64Decode(pkcs12_data, &decoded_pkcs12)) {
-    ONC_LOG_ERROR(
-        "Unable to base64 decode PKCS#12 data: \"" + pkcs12_data + "\".");
+    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(target_nssdb_->GetPrivateSlot());
+  crypto::ScopedPK11Slot private_slot(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 = target_nssdb_->ImportFromPKCS12(
+  int import_result = nssdb->ImportFromPKCS12(
       module.get(), decoded_pkcs12, base::string16(), false, &imported_certs);
   if (import_result != net::OK) {
     std::string error_string = net::ErrorToString(import_result);
-    ONC_LOG_ERROR(
-        base::StringPrintf("Unable to import client certificate (error %s)",
-                           error_string.c_str()));
+    LOG(ERROR) << "Unable to import client certificate, error: "
+               << error_string;
     return false;
   }
 
   if (imported_certs.size() == 0) {
-    ONC_LOG_WARNING("PKCS12 data contains no importable certificates.");
+    LOG(WARNING) << "PKCS12 data contains no importable certificates.";
     return true;
   }
 
   if (imported_certs.size() != 1) {
-    ONC_LOG_WARNING("ONC File: PKCS12 data contains more than one certificate. "
-                    "Only the first one will be imported.");
+    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];
@@ -359,7 +397,7 @@ bool CertificateImporterImpl::ParseClientCertificate(
     PK11_SetPrivateKeyNickname(private_key, const_cast<char*>(guid.c_str()));
     SECKEY_DestroyPrivateKey(private_key);
   } else {
-    ONC_LOG_WARNING("Unable to find private key for certificate.");
+    LOG(WARNING) << "Unable to find private key for certificate.";
   }
   return true;
 }

chromeos/network/onc/onc_certificate_importer_impl.h

@@ -12,6 +12,7 @@
 #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"
@@ -19,6 +20,8 @@
 namespace base {
 class DictionaryValue;
 class ListValue;
+class SequencedTaskRunner;
+class SingleThreadTaskRunner;
 }
 
 namespace net {
@@ -31,34 +34,36 @@ namespace chromeos {
 namespace onc {
 
 // This class handles certificate imports from ONC (both policy and user
-// imports) into the certificate store. The GUID of Client certificates is
-// stored together with the certificate as Nickname. In contrast, Server and CA
+// imports) into a 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:
-  typedef std::map<std::string, scoped_refptr<net::X509Certificate> >
-      CertsByGUID;
-
-  explicit CertificateImporterImpl(net::NSSCertDatabase* target_nssdb_);
+  // |io_task_runner| will be used for NSSCertDatabase accesses.
+  CertificateImporterImpl(
+      const scoped_refptr<base::SequencedTaskRunner>& io_task_runner,
+      net::NSSCertDatabase* target_nssdb_);
+  virtual ~CertificateImporterImpl();
 
   // CertificateImporter overrides
-  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);
+  virtual void ImportCertificates(const base::ListValue& certificates,
+                                  ::onc::ONCSource source,
+                                  const DoneCallback& done_callback) OVERRIDE;
 
  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,
@@ -72,30 +77,36 @@ class CHROMEOS_EXPORT CertificateImporterImpl : public CertificateImporter {
 
   // Parses and stores/removes |certificate| in/from the certificate
   // store. Returns true if the operation succeeded.
-  bool ParseAndStoreCertificate(
+  static bool ParseAndStoreCertificate(
       bool allow_trust_imports,
       const base::DictionaryValue& certificate,
-      net::CertificateList* onc_trusted_certificates,
-      CertsByGUID* imported_server_and_ca_certs);
+      net::NSSCertDatabase* nssdb,
+      net::CertificateList* onc_trusted_certificates);
 
   // 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.
-  bool ParseServerOrCaCertificate(
+  static bool ParseServerOrCaCertificate(
       bool allow_trust_imports,
       const std::string& cert_type,
       const std::string& guid,
       const base::DictionaryValue& certificate,
-      net::CertificateList* onc_trusted_certificates,
-      CertsByGUID* imported_server_and_ca_certs);
+      net::NSSCertDatabase* nssdb,
+      net::CertificateList* onc_trusted_certificates);
 
-  bool ParseClientCertificate(const std::string& guid,
-                              const base::DictionaryValue& certificate);
+  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_;
 
   // 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,11 +13,12 @@
 #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/nss_util_internal.h"
-#include "crypto/scoped_test_nss_chromeos_user.h"
+#include "crypto/scoped_test_nss_db.h"
 #include "net/base/crypto_module.h"
 #include "net/cert/cert_type.h"
 #include "net/cert/nss_cert_database_chromeos.h"
@@ -63,30 +64,39 @@ net::CertType GetCertType(net::X509Certificate::OSCertHandle cert) {
 
 class ONCCertificateImporterImplTest : public testing::Test {
  public:
-  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.
+  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_));
+
     test_nssdb_.reset(new net::NSSCertDatabaseChromeOS(
-        crypto::GetPublicSlotForChromeOSUser(user_.username_hash()),
-        crypto::GetPublicSlotForChromeOSUser(private_user_.username_hash())));
+        crypto::ScopedPK11Slot(public_nssdb_.slot()),
+        crypto::ScopedPK11Slot(private_nssdb_.slot())));
 
     // Test db should be empty at start of test.
     EXPECT_TRUE(ListCertsInPublicSlot().empty());
     EXPECT_TRUE(ListCertsInPrivateSlot().empty());
   }
 
-  virtual ~ONCCertificateImporterImplTest() {}
+  virtual void TearDown() OVERRIDE {
+    thread_task_runner_handle_.reset();
+    task_runner_ = NULL;
+  }
 
  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);
@@ -98,14 +108,15 @@ class ONCCertificateImporterImplTest : public testing::Test {
     onc_certificates_.reset(certificates);
 
     web_trust_certificates_.clear();
-    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_));
+    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();
 
     public_list_ = ListCertsInPublicSlot();
     private_list_ = ListCertsInPrivateSlot();
@@ -119,25 +130,29 @@ class ONCCertificateImporterImplTest : public testing::Test {
       guid = &guid_temporary;
 
     AddCertificatesFromFile(filename, true);
-    ASSERT_EQ(1ul, public_list_.size() + private_list_.size());
-    if (!public_list_.empty())
+
+    if (expected_type == net::SERVER_CERT || expected_type == net::CA_CERT) {
+      ASSERT_EQ(1u, public_list_.size());
       EXPECT_EQ(expected_type, GetCertType(public_list_[0]->os_cert_handle()));
-    if (!private_list_.empty())
+      EXPECT_TRUE(private_list_.empty());
+    } else {  // net::USER_CERT
+      EXPECT_TRUE(public_list_.empty());
+      ASSERT_EQ(1u, private_list_.size());
       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);
-
-    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());
-    }
   }
 
+  // Certificates and the NSSCertDatabase depend on these test DBs. Destroy them
+  // last.
+  crypto::ScopedTestNSSDB public_nssdb_;
+  crypto::ScopedTestNSSDB private_nssdb_;
+
+  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.
@@ -145,15 +160,14 @@ 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(test_nssdb_->GetPublicSlot().get());
+    return ListCertsInSlot(public_nssdb_.slot());
   }
 
   net::CertificateList ListCertsInPrivateSlot() {
-    return ListCertsInSlot(test_nssdb_->GetPrivateSlot().get());
+    return ListCertsInSlot(private_nssdb_.slot());
   }
 
   net::CertificateList ListCertsInSlot(PK11SlotInfo* slot) {
@@ -171,16 +185,13 @@ 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, imported_server_and_ca_certs_.size());
+  EXPECT_EQ(2ul, public_list_.size());
 }
 
 TEST_F(ONCCertificateImporterImplTest, MultipleCertificatesWithFailures) {
@@ -188,7 +199,6 @@ 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) {
@@ -199,7 +209,7 @@ TEST_F(ONCCertificateImporterImplTest, AddClientCertificate) {
   EXPECT_TRUE(public_list_.empty());
 
   SECKEYPrivateKeyList* privkey_list =
-      PK11_ListPrivKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL, NULL);
+      PK11_ListPrivKeysInSlot(private_nssdb_.slot(), NULL, NULL);
   EXPECT_TRUE(privkey_list);
   if (privkey_list) {
     SECKEYPrivateKeyListNode* node = PRIVKEY_LIST_HEAD(privkey_list);
@@ -216,7 +226,7 @@ TEST_F(ONCCertificateImporterImplTest, AddClientCertificate) {
   }
 
   SECKEYPublicKeyList* pubkey_list =
-      PK11_ListPublicKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL);
+      PK11_ListPublicKeysInSlot(private_nssdb_.slot(), NULL);
   EXPECT_TRUE(pubkey_list);
   if (pubkey_list) {
     SECKEYPublicKeyListNode* node = PUBKEY_LIST_HEAD(pubkey_list);
@@ -234,11 +244,11 @@ TEST_F(ONCCertificateImporterImplTest, AddServerCertificateWithWebTrust) {
   AddCertificateFromFile("certificate-server.onc", net::SERVER_CERT, NULL);
 
   SECKEYPrivateKeyList* privkey_list =
-      PK11_ListPrivKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL, NULL);
+      PK11_ListPrivKeysInSlot(private_nssdb_.slot(), NULL, NULL);
   EXPECT_FALSE(privkey_list);
 
   SECKEYPublicKeyList* pubkey_list =
-      PK11_ListPublicKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL);
+      PK11_ListPublicKeysInSlot(private_nssdb_.slot(), NULL);
   EXPECT_FALSE(pubkey_list);
 
   ASSERT_EQ(1u, web_trust_certificates_.size());
@@ -252,11 +262,11 @@ TEST_F(ONCCertificateImporterImplTest, AddWebAuthorityCertificateWithWebTrust) {
   AddCertificateFromFile("certificate-web-authority.onc", net::CA_CERT, NULL);
 
   SECKEYPrivateKeyList* privkey_list =
-      PK11_ListPrivKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL, NULL);
+      PK11_ListPrivKeysInSlot(private_nssdb_.slot(), NULL, NULL);
   EXPECT_FALSE(privkey_list);
 
   SECKEYPublicKeyList* pubkey_list =
-      PK11_ListPublicKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL);
+      PK11_ListPublicKeysInSlot(private_nssdb_.slot(), NULL);
   EXPECT_FALSE(pubkey_list);
 
   ASSERT_EQ(1u, web_trust_certificates_.size());
@@ -271,11 +281,11 @@ TEST_F(ONCCertificateImporterImplTest, AddAuthorityCertificateWithoutWebTrust) {
   EXPECT_TRUE(web_trust_certificates_.empty());
 
   SECKEYPrivateKeyList* privkey_list =
-      PK11_ListPrivKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL, NULL);
+      PK11_ListPrivKeysInSlot(private_nssdb_.slot(), NULL, NULL);
   EXPECT_FALSE(privkey_list);
 
   SECKEYPublicKeyList* pubkey_list =
-      PK11_ListPublicKeysInSlot(test_nssdb_->GetPrivateSlot().get(), NULL);
+      PK11_ListPublicKeysInSlot(private_nssdb_.slot(), NULL);
   EXPECT_FALSE(pubkey_list);
 }