Instantiation of OwnerKeyUtil is delegated to OwnerSettingsServiceFactory.

chrome/browser/chromeos/ownership/owner_settings_service.cc

 // Copyright 2014 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/ownership/owner_settings_service.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
-#include "base/path_service.h"
 #include "base/prefs/pref_service.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/chromeos/ownership/owner_settings_service_factory.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "chrome/browser/chromeos/settings/session_manager_operation.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chromeos/chromeos_paths.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
-#include "components/ownership/owner_key_util_impl.h"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/common/content_switches.h"
 #include "crypto/nss_util.h"
 #include "crypto/nss_util_internal.h"
 #include "crypto/rsa_private_key.h"
 #include "crypto/scoped_nss_types.h"
 #include "crypto/signature_creator.h"
 
 namespace em = enterprise_management;
 
 using content::BrowserThread;
 using ownership::OwnerKeyUtil;
 using ownership::PrivateKey;
 using ownership::PublicKey;
 
 namespace chromeos {
 
 namespace {
 
-scoped_refptr<OwnerKeyUtil>* g_owner_key_util_for_testing = NULL;
 DeviceSettingsService* g_device_settings_service_for_testing = NULL;
 
 bool IsOwnerInTests(const std::string& user_id) {
   if (user_id.empty() ||
       !CommandLine::ForCurrentProcess()->HasSwitch(::switches::kTestType) ||
       !CrosSettings::IsInitialized()) {
     return false;
   }
   const base::Value* value = CrosSettings::Get()->GetPref(kDeviceOwner);
   if (!value || value->GetType() != base::Value::TYPE_STRING)
@@ skipping 115 matching lines @@
   scoped_ptr<crypto::RSAPrivateKey> key(
       crypto::RSAPrivateKey::FindFromPublicKeyInfo(public_key));
   bool is_owner = key.get() != NULL;
   return is_owner;
 }
 
 // Checks whether NSS slots with private key are mounted or
 // not. Responds via |callback|.
 void DoesPrivateKeyExistAsync(
     const OwnerSettingsService::IsOwnerCallback& callback) {
-  scoped_refptr<OwnerKeyUtil> owner_key_util;
-  if (g_owner_key_util_for_testing)
-    owner_key_util = *g_owner_key_util_for_testing;
-  else
-    owner_key_util = OwnerSettingsService::MakeOwnerKeyUtil();
+  scoped_refptr<OwnerKeyUtil> owner_key_util =
+      OwnerSettingsServiceFactory::GetInstance()->GetOwnerKeyUtil();

[erikwright (departed), 2014/09/02 19:13:56]

This circular dependency between the factory and the service seems pretty
undesirable. It's called by a static method, IsOwnerForSafeModeAsync, which is
called by ChromeCryptohomeAuthenticator.

IsOwnerForSafeModeAsync needs to use ProfileHelper, which is presumably going to
be a barrier to componentization anyway.

I think IsOwnerForSafeModeAsync should be an instance method and
ChromeCryptohomeAuthenticator should be responsible for looking up the instance
of OwnerSettingsService for a given Profile.

[ygorshenin1, 2014/09/03 10:14:58]

I agree with you that we need to get rid of the circular dependency here.
Unfortunately this method is called when only in the safe mode and when profile
corresponding to user_id is not ready yet. For now I propose to pass an
OwnerKeyUtil instance as an argument to
OwnerSettingsService::IsOwnerForSafeModeAsync(). When OwnerSettingsService will
be moved to components/ownership/* all Chrome-specific things like
IsOwnerForSafeModeAsync() will remain in an OwnerSettingsServiceChrome.

On 2014/09/02 19:13:56, erikwright wrote:

[erikwright (departed), 2014/09/03 14:07:16]

In my first reading I thought you were using a Profile, now I see you are using
a profile FilePath. I'm happy with the removal of the circular dependency for
now. Thanks.

+
   if (!owner_key_util) {
     callback.Run(false);
     return;
   }
   scoped_refptr<base::TaskRunner> task_runner =
       content::BrowserThread::GetBlockingPool()
           ->GetTaskRunnerWithShutdownBehavior(
               base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
   base::PostTaskAndReplyWithResult(
       task_runner.get(),
@@ skipping 37 matching lines @@
       // For consumer management unenrollment.
       return new_mode == em::PolicyData::NOT_MANAGED;
   }
 
   NOTREACHED();
   return false;
 }
 
 }  // namespace
 
-OwnerSettingsService::OwnerSettingsService(Profile* profile)
+OwnerSettingsService::OwnerSettingsService(
+    Profile* profile,
+    const scoped_refptr<OwnerKeyUtil>& owner_key_util)
     : profile_(profile),
-      owner_key_util_(MakeOwnerKeyUtil()),
+      owner_key_util_(owner_key_util),
       waiting_for_profile_creation_(true),
       waiting_for_tpm_token_(true),
       weak_factory_(this) {
   if (TPMTokenLoader::IsInitialized()) {
     waiting_for_tpm_token_ = !TPMTokenLoader::Get()->IsTPMTokenReady();
     TPMTokenLoader::Get()->AddObserver(this);
   }
 
   if (DBusThreadManager::IsInitialized() &&
       DBusThreadManager::Get()->GetSessionManagerClient()) {
@@ skipping 137 matching lines @@
       BrowserThread::IO,
       FROM_HERE,
       base::Bind(base::IgnoreResult(&crypto::InitializeNSSForChromeOSUser),
                  user_id,
                  user_hash,
                  ProfileHelper::GetProfilePathByUserIdHash(user_hash)),
       base::Bind(&DoesPrivateKeyExistAsync, callback));
 }
 
 // static
-scoped_refptr<ownership::OwnerKeyUtil>
-OwnerSettingsService::MakeOwnerKeyUtil() {
-  base::FilePath public_key_path;
-  if (!PathService::Get(chromeos::FILE_OWNER_KEY, &public_key_path))
-    return NULL;
-  return new ownership::OwnerKeyUtilImpl(public_key_path);
-}
-
-// static
-void OwnerSettingsService::SetOwnerKeyUtilForTesting(
-    const scoped_refptr<OwnerKeyUtil>& owner_key_util) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  if (g_owner_key_util_for_testing) {
-    delete g_owner_key_util_for_testing;
-    g_owner_key_util_for_testing = NULL;
-  }
-  if (owner_key_util.get()) {
-    g_owner_key_util_for_testing = new scoped_refptr<OwnerKeyUtil>();
-    *g_owner_key_util_for_testing = owner_key_util;
-  }
-}
-
-// static
 void OwnerSettingsService::SetDeviceSettingsServiceForTesting(
     DeviceSettingsService* device_settings_service) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   g_device_settings_service_for_testing = device_settings_service;
 }
 
 void OwnerSettingsService::ReloadPrivateKey() {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (waiting_for_profile_creation_ || waiting_for_tpm_token_)
     return;
@@ skipping 88 matching lines @@
 void OwnerSettingsService::HandleError(DeviceSettingsService::Status status,
                                        const base::Closure& callback) {
   LOG(ERROR) << "Session manager operation failed: " << status;
   GetDeviceSettingsService()->OnSignAndStoreOperationCompleted(status);
   if (!callback.is_null())
     callback.Run();
 }
 
 scoped_refptr<OwnerKeyUtil> OwnerSettingsService::GetOwnerKeyUtil() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (g_owner_key_util_for_testing)
-    return *g_owner_key_util_for_testing;
   return owner_key_util_;
 }
 
 DeviceSettingsService* OwnerSettingsService::GetDeviceSettingsService() {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (g_device_settings_service_for_testing)
     return g_device_settings_service_for_testing;
   if (DeviceSettingsService::IsInitialized())
     return DeviceSettingsService::Get();
   return NULL;
 }
 
 }  // namespace chromeos