Use per-user nssdb in onc certificate importer

chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc

Index: chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc
diff --git a/chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc b/chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc
index fed3c77197ef7f193150a2eda0bbd2d8ebbe1304..94db23ccbc264018a67d947a588f376dc2ab9ef0 100644
--- a/chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc
+++ b/chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc
@@ -4,11 +4,15 @@
 
 #include "chrome/browser/chromeos/policy/user_network_configuration_updater_factory.h"
 
+#include "base/bind.h"
+#include "base/location.h"
 #include "base/memory/singleton.h"
+#include "base/message_loop/message_loop_proxy.h"
 #include "chrome/browser/chromeos/login/user.h"
 #include "chrome/browser/chromeos/login/user_manager.h"
 #include "chrome/browser/chromeos/policy/user_network_configuration_updater.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/net/nss_context.h"
 #include "chrome/browser/policy/profile_policy_connector.h"
 #include "chrome/browser/policy/profile_policy_connector_factory.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
@@ -21,6 +25,42 @@
 
 namespace policy {
 
+namespace {
+
+bool skip_certificate_importer_creation_for_test = false;
+
+// Callback for getting the users certificate database.
+// Initializes onc::CertificateImporter for |updater|.
+void OnDatabaseForImporter(
+    UserNetworkConfigurationUpdater* updater,
+    net::NSSCertDatabase* cert_database) {
+  updater->SetCertificateImporter(
+      scoped_ptr<chromeos::onc::CertificateImporter>(
+          new chromeos::onc::CertificateImporterImpl(cert_database)));
+}
+
+// Fetches the user's NSSCertDatabase so it could be user to creating the
+// |updater|'s certificate importer.
+void CreateAndSetCertificateImporterForService(
+    Profile* profile,
+    UserNetworkConfigurationUpdater* updater) {
+  // |GetNSSCertDatabaseForProfile| should not be called before the profile's
+  // ProfileIOData is initialized, which happens in ProfileImpl::DoFinalInit.
+  // Unfortunately, this is not the case here. Services created with the browser
+  // context (one of which is UserNetworkConfigurationUpdater) are created
+  // before profile's final initialization, but during the same message loop
+  // task. Going async here should make callign GetNSSCertDatabaseForProfile
+  // safe.
+  base::MessageLoopProxy::current()->PostTask(

[pneubeck (no reviews), 2014/02/06 09:37:46]

I think this is too fragile. If the initialization process changes at any time
this will potentially break.

Please wait for chrome::NOTIFICATION_PROFILE_ADDED instead (e.g. like in
user_policy_signin_service_base.cc line 41).
Also the callback from OnDatabaseForImporter has to be bound to a WeakPtr, I
think.

The following should work:

class User..Factory {
  content::NotificationRegistrar registrar_;
}

..Factory::BuildServiceInstanceFor(context) {
  ...
  if (!skip_certificate_importer_creation_for_test) {
    registrar_.Add(this,
                   chrome::NOTIFICATION_PROFILE_ADDED,
                   content::Source<Profile>(profile));
  }
  return updater.release();
}

..Factory::Observe(
    int type,
    const content::NotificationSource& source,
    const content::NotificationDetails& details) {
  DCHECK_EQ(type, PROFILE_ADDED);
  Profile* profile = content::Source<Profile>(source).ptr();
  DCHECK(profile);
  UserNetworkConfigurationUpdater* updater = GetForProfile(profile);
  DCHECK(updater);
  GetNSSCertDatabaseForProfile(profile,
    base::Bind(&OnDatabaseForImporter, weak_ptr_factory_.GetWeakPtr(),
updater));
}

[tbarzic, 2014/02/06 23:02:36]

Good point, though, I think it would better to observe the notification in the
service rather than factory.

+      FROM_HERE,
+      base::Bind(
+          &GetNSSCertDatabaseForProfile,
+          profile,
+          base::Bind(&OnDatabaseForImporter, updater)));
+}
+
+}  // namespace
+
 // static
 UserNetworkConfigurationUpdater*
 UserNetworkConfigurationUpdaterFactory::GetForProfile(Profile* profile) {
@@ -34,6 +74,12 @@ UserNetworkConfigurationUpdaterFactory::GetInstance() {
   return Singleton<UserNetworkConfigurationUpdaterFactory>::get();
 }
 
+// static
+void UserNetworkConfigurationUpdaterFactory::
+SetSkipCertificateImporterCreationForTest(bool skip) {
+  skip_certificate_importer_creation_for_test = skip;
+}
+
 UserNetworkConfigurationUpdaterFactory::UserNetworkConfigurationUpdaterFactory()
     : BrowserContextKeyedServiceFactory(
           "UserNetworkConfigurationUpdater",
@@ -81,14 +127,20 @@ UserNetworkConfigurationUpdaterFactory::BuildServiceInstanceFor(
   ProfilePolicyConnector* profile_connector =
       ProfilePolicyConnectorFactory::GetForProfile(profile);
 
-  return UserNetworkConfigurationUpdater::CreateForUserPolicy(
-      allow_trusted_certs_from_policy,
-      *user,
-      scoped_ptr<chromeos::onc::CertificateImporter>(
-          new chromeos::onc::CertificateImporterImpl),
-      profile_connector->policy_service(),
-      chromeos::NetworkHandler::Get()->managed_network_configuration_handler())
-      .release();
+  scoped_ptr<UserNetworkConfigurationUpdater> updater(
+      UserNetworkConfigurationUpdater::CreateForUserPolicy(
+          allow_trusted_certs_from_policy,
+          *user,
+          profile_connector->policy_service(),
+          chromeos::NetworkHandler::Get()->
+              managed_network_configuration_handler()));
+
+  // The certificate importer is created asynchronously and passed to the
+  // updater.
+  if (!skip_certificate_importer_creation_for_test)
+    CreateAndSetCertificateImporterForService(profile, updater.get());
+
+  return updater.release();
 }
 
 }  // namespace policy