Use per-user nssdb in onc certificate importer

chrome/browser/chromeos/policy/user_network_configuration_updater_factory.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 "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"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
 #include "chromeos/network/network_handler.h"
 #include "chromeos/network/onc/onc_certificate_importer_impl.h"
 #include "components/browser_context_keyed_service/browser_context_dependency_manager.h"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
 
 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) {
   return static_cast<UserNetworkConfigurationUpdater*>(
       GetInstance()->GetServiceForBrowserContext(profile, true));
 }
 
 // static
 UserNetworkConfigurationUpdaterFactory*
 UserNetworkConfigurationUpdaterFactory::GetInstance() {
   return Singleton<UserNetworkConfigurationUpdaterFactory>::get();
 }
 
+// static
+void UserNetworkConfigurationUpdaterFactory::
+SetSkipCertificateImporterCreationForTest(bool skip) {
+  skip_certificate_importer_creation_for_test = skip;
+}
+
 UserNetworkConfigurationUpdaterFactory::UserNetworkConfigurationUpdaterFactory()
     : BrowserContextKeyedServiceFactory(
           "UserNetworkConfigurationUpdater",
           BrowserContextDependencyManager::GetInstance()) {
   DependsOn(ProfilePolicyConnectorFactory::GetInstance());
 }
 
 UserNetworkConfigurationUpdaterFactory::
     ~UserNetworkConfigurationUpdaterFactory() {}
 
@@ skipping 27 matching lines @@
   // also http://crbug.com/310685 .
   if (user != user_manager->GetPrimaryUser())
     return NULL;
 
   const bool allow_trusted_certs_from_policy =
       user->GetType() == chromeos::User::USER_TYPE_REGULAR;
 
   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