Instantiation of OwnerKeyUtil is delegated to OwnerSettingsServiceFactory, as we're going to move as much as possible from OwnerSettingsService to components/ownership/* and because OwnerSettingsServiceFactory should do all platform-specific business.

BUG=398856
TEST=existing browser_tests and unit_tests

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

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

13 files changed, 71 insertions, 92 deletions

diff --git a/chrome/browser/chromeos/app_mode/kiosk_app_manager.cc b/chrome/browser/chromeos/app_mode/kiosk_app_manager.cc
index 61b1287..b6a6e5e 100644
--- a/chrome/browser/chromeos/app_mode/kiosk_app_manager.cc
+++ b/chrome/browser/chromeos/app_mode/kiosk_app_manager.cc
@@ -22,7 +22,7 @@
 #include "chrome/browser/chromeos/app_mode/kiosk_app_external_loader.h"
 #include "chrome/browser/chromeos/app_mode/kiosk_app_manager_observer.h"
 #include "chrome/browser/chromeos/app_mode/kiosk_external_updater.h"
-#include "chrome/browser/chromeos/ownership/owner_settings_service.h"
+#include "chrome/browser/chromeos/ownership/owner_settings_service_factory.h"
 #include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
 #include "chrome/browser/chromeos/policy/device_local_account.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
@@ -59,7 +59,7 @@ void OnRemoveAppCryptohomeComplete(const std::string& app,
 // Check for presence of machine owner public key file.
 void CheckOwnerFilePresence(bool *present) {
   scoped_refptr<ownership::OwnerKeyUtil> util =
-      OwnerSettingsService::MakeOwnerKeyUtil();
+      OwnerSettingsServiceFactory::GetInstance()->GetOwnerKeyUtil();
   *present = util.get() && util->IsPublicKeyPresent();
 }
 
diff --git a/chrome/browser/chromeos/chrome_browser_main_chromeos.cc b/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
index a04ebbd..77b6730 100644
--- a/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
+++ b/chrome/browser/chromeos/chrome_browser_main_chromeos.cc
@@ -55,7 +55,7 @@
 #include "chrome/browser/chromeos/memory/oom_priority_manager.h"
 #include "chrome/browser/chromeos/net/network_portal_detector_impl.h"
 #include "chrome/browser/chromeos/options/cert_library.h"
-#include "chrome/browser/chromeos/ownership/owner_settings_service.h"
+#include "chrome/browser/chromeos/ownership/owner_settings_service_factory.h"
 #include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
 #include "chrome/browser/chromeos/policy/device_local_account.h"
 #include "chrome/browser/chromeos/power/idle_action_warning_observer.h"
@@ -187,7 +187,7 @@ class DBusServices {
     DeviceSettingsService::Initialize();
     DeviceSettingsService::Get()->SetSessionManager(
         DBusThreadManager::Get()->GetSessionManagerClient(),
-        OwnerSettingsService::MakeOwnerKeyUtil());
+        OwnerSettingsServiceFactory::GetInstance()->GetOwnerKeyUtil());
   }
 
   ~DBusServices() {
diff --git a/chrome/browser/chromeos/login/auth/chrome_cryptohome_authenticator.cc b/chrome/browser/chromeos/login/auth/chrome_cryptohome_authenticator.cc
index ac7694a..e2d2ed2 100644
--- a/chrome/browser/chromeos/login/auth/chrome_cryptohome_authenticator.cc
+++ b/chrome/browser/chromeos/login/auth/chrome_cryptohome_authenticator.cc
@@ -6,6 +6,7 @@
 
 #include "base/thread_task_runner_handle.h"
 #include "chrome/browser/chromeos/ownership/owner_settings_service.h"
+#include "chrome/browser/chromeos/ownership/owner_settings_service_factory.h"
 #include "chrome/browser/chromeos/settings/cros_settings.h"
 #include "components/user_manager/user_manager.h"
 #include "content/public/browser/browser_thread.h"
@@ -43,7 +44,10 @@ void ChromeCryptohomeAuthenticator::CheckSafeModeOwnership(
   }
 
   OwnerSettingsService::IsOwnerForSafeModeAsync(
-      context.GetUserID(), context.GetUserIDHash(), callback);
+      context.GetUserID(),
+      context.GetUserIDHash(),
+      OwnerSettingsServiceFactory::GetInstance()->GetOwnerKeyUtil(),
+      callback);
 }
 
 }  // namespace chromeos
diff --git a/chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc b/chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc
index 454ffdc..afd8863 100644
--- a/chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc
+++ b/chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc
@@ -134,7 +134,9 @@ class CryptohomeAuthenticatorTest : public testing::Test {
         user_manager_enabler_(user_manager_),
         mock_caller_(NULL),
         mock_homedir_methods_(NULL),
-        owner_key_util_(new ownership::MockOwnerKeyUtil) {
+        owner_key_util_(new ownership::MockOwnerKeyUtil()) {
+    OwnerSettingsServiceFactory::GetInstance()->SetOwnerKeyUtilForTesting(
+        owner_key_util_);
     user_context_.SetKey(Key("fakepass"));
     user_context_.SetUserIDHash("me_nowhere_com_hash");
     const user_manager::User* user =
@@ -165,15 +167,12 @@ class CryptohomeAuthenticatorTest : public testing::Test {
 
     SystemSaltGetter::Initialize();
 
-    OwnerSettingsService::SetOwnerKeyUtilForTesting(owner_key_util_);
-
     auth_ = new ChromeCryptohomeAuthenticator(&consumer_);
     state_.reset(new TestAttemptState(user_context_, false));
   }
 
   // Tears down the test fixture.
   virtual void TearDown() {
-    OwnerSettingsService::SetOwnerKeyUtilForTesting(NULL);
     SystemSaltGetter::Shutdown();
     DBusThreadManager::Shutdown();
 
diff --git a/chrome/browser/chromeos/ownership/owner_settings_service.cc b/chrome/browser/chromeos/ownership/owner_settings_service.cc
index e2b3805..9246a05 100644
--- a/chrome/browser/chromeos/ownership/owner_settings_service.cc
+++ b/chrome/browser/chromeos/ownership/owner_settings_service.cc
@@ -9,17 +9,13 @@
 #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"
@@ -43,7 +39,6 @@ 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) {
@@ -178,12 +173,8 @@ bool DoesPrivateKeyExistAsyncHelper(
 // Checks whether NSS slots with private key are mounted or
 // not. Responds via |callback|.
 void DoesPrivateKeyExistAsync(
+    const scoped_refptr<OwnerKeyUtil>& owner_key_util,
     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();
   if (!owner_key_util) {
     callback.Run(false);
     return;
@@ -241,9 +232,11 @@ bool CheckManagementModeTransition(em::PolicyData::ManagementMode current_mode,
 
 }  // 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) {
@@ -385,6 +378,7 @@ void OwnerSettingsService::OwnerKeySet(bool success) {
 void OwnerSettingsService::IsOwnerForSafeModeAsync(
     const std::string& user_id,
     const std::string& user_hash,
+    const scoped_refptr<OwnerKeyUtil>& owner_key_util,
     const IsOwnerCallback& callback) {
   CHECK(chromeos::LoginState::Get()->IsInSafeMode());
 
@@ -397,30 +391,7 @@ void OwnerSettingsService::IsOwnerForSafeModeAsync(
                  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;
-  }
+      base::Bind(&DoesPrivateKeyExistAsync, owner_key_util, callback));
 }
 
 // static
@@ -532,8 +503,6 @@ void OwnerSettingsService::HandleError(DeviceSettingsService::Status status,
 
 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_;
 }
 
diff --git a/chrome/browser/chromeos/ownership/owner_settings_service.h b/chrome/browser/chromeos/ownership/owner_settings_service.h
index a86c084..3419d4f 100644
--- a/chrome/browser/chromeos/ownership/owner_settings_service.h
+++ b/chrome/browser/chromeos/ownership/owner_settings_service.h
@@ -71,14 +71,11 @@ class OwnerSettingsService : public DeviceSettingsService::PrivateKeyDelegate,
 
   // Checks if the user is the device owner, without the user profile having to
   // been initialized. Should be used only if login state is in safe mode.
-  static void IsOwnerForSafeModeAsync(const std::string& user_id,
-                                      const std::string& user_hash,
-                                      const IsOwnerCallback& callback);
-
-  static scoped_refptr<ownership::OwnerKeyUtil> MakeOwnerKeyUtil();
-
-  static void SetOwnerKeyUtilForTesting(
-      const scoped_refptr<ownership::OwnerKeyUtil>& owner_key_util);
+  static void IsOwnerForSafeModeAsync(
+      const std::string& user_id,
+      const std::string& user_hash,
+      const scoped_refptr<ownership::OwnerKeyUtil>& owner_key_util,
+      const IsOwnerCallback& callback);
 
   static void SetDeviceSettingsServiceForTesting(
       DeviceSettingsService* device_settings_service);
@@ -86,7 +83,9 @@ class OwnerSettingsService : public DeviceSettingsService::PrivateKeyDelegate,
  private:
   friend class OwnerSettingsServiceFactory;
 
-  explicit OwnerSettingsService(Profile* profile);
+  OwnerSettingsService(
+      Profile* profile,
+      const scoped_refptr<ownership::OwnerKeyUtil>& owner_key_util);
 
   // Reloads private key from profile's NSS slots. Responds via call
   // to OnPrivateKeyLoaded().
diff --git a/chrome/browser/chromeos/ownership/owner_settings_service_factory.cc b/chrome/browser/chromeos/ownership/owner_settings_service_factory.cc
index a307888..4f2e79d 100644
--- a/chrome/browser/chromeos/ownership/owner_settings_service_factory.cc
+++ b/chrome/browser/chromeos/ownership/owner_settings_service_factory.cc
@@ -4,10 +4,14 @@
 
 #include "chrome/browser/chromeos/ownership/owner_settings_service_factory.h"
 
+#include "base/path_service.h"
 #include "chrome/browser/chromeos/ownership/owner_settings_service.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chromeos/chromeos_paths.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "components/ownership/owner_key_util.h"
+#include "components/ownership/owner_key_util_impl.h"
 
 namespace chromeos {
 
@@ -32,13 +36,29 @@ OwnerSettingsServiceFactory* OwnerSettingsServiceFactory::GetInstance() {
   return Singleton<OwnerSettingsServiceFactory>::get();
 }
 
+scoped_refptr<ownership::OwnerKeyUtil>
+OwnerSettingsServiceFactory::GetOwnerKeyUtil() {
+  if (owner_key_util_.get())
+    return owner_key_util_;
+  base::FilePath public_key_path;
+  if (!PathService::Get(chromeos::FILE_OWNER_KEY, &public_key_path))
+    return NULL;
+  owner_key_util_ = new ownership::OwnerKeyUtilImpl(public_key_path);
+  return owner_key_util_;
+}
+
+void OwnerSettingsServiceFactory::SetOwnerKeyUtilForTesting(
+    const scoped_refptr<ownership::OwnerKeyUtil>& owner_key_util) {
+  owner_key_util_ = owner_key_util;
+}
+
 // static
 KeyedService* OwnerSettingsServiceFactory::BuildInstanceFor(
     content::BrowserContext* browser_context) {
   Profile* profile = static_cast<Profile*>(browser_context);
   if (profile->IsGuestSession() || ProfileHelper::IsSigninProfile(profile))
     return NULL;
-  return new OwnerSettingsService(profile);
+  return new OwnerSettingsService(profile, GetInstance()->GetOwnerKeyUtil());
 }
 
 bool OwnerSettingsServiceFactory::ServiceIsCreatedWithBrowserContext() const {
diff --git a/chrome/browser/chromeos/ownership/owner_settings_service_factory.h b/chrome/browser/chromeos/ownership/owner_settings_service_factory.h
index e98773a..597656b 100644
--- a/chrome/browser/chromeos/ownership/owner_settings_service_factory.h
+++ b/chrome/browser/chromeos/ownership/owner_settings_service_factory.h
@@ -9,12 +9,17 @@
 
 #include "base/compiler_specific.h"
 #include "base/macros.h"
+#include "base/memory/ref_counted.h"
 #include "base/memory/singleton.h"
 #include "components/keyed_service/content/browser_context_keyed_service_factory.h"
 
 class KeyedService;
 class Profile;
 
+namespace ownership {
+class OwnerKeyUtil;
+}
+
 namespace chromeos {
 
 class OwnerSettingsService;
@@ -25,6 +30,11 @@ class OwnerSettingsServiceFactory : public BrowserContextKeyedServiceFactory {
 
   static OwnerSettingsServiceFactory* GetInstance();
 
+  scoped_refptr<ownership::OwnerKeyUtil> GetOwnerKeyUtil();
+
+  void SetOwnerKeyUtilForTesting(
+      const scoped_refptr<ownership::OwnerKeyUtil>& owner_key_util);
+
  private:
   friend struct DefaultSingletonTraits<OwnerSettingsServiceFactory>;
 
@@ -40,6 +50,8 @@ class OwnerSettingsServiceFactory : public BrowserContextKeyedServiceFactory {
   virtual KeyedService* BuildServiceInstanceFor(
       content::BrowserContext* browser_context) const OVERRIDE;
 
+  scoped_refptr<ownership::OwnerKeyUtil> owner_key_util_;
+
   DISALLOW_COPY_AND_ASSIGN(OwnerSettingsServiceFactory);
 };
 
diff --git a/chrome/browser/chromeos/settings/device_settings_test_helper.cc b/chrome/browser/chromeos/settings/device_settings_test_helper.cc
index 19c82c3..06fd21c 100644
--- a/chrome/browser/chromeos/settings/device_settings_test_helper.cc
+++ b/chrome/browser/chromeos/settings/device_settings_test_helper.cc
@@ -207,6 +207,8 @@ DeviceSettingsTestBase::DeviceSettingsTestBase()
     : user_manager_(new FakeUserManager()),
       user_manager_enabler_(user_manager_),
       owner_key_util_(new ownership::MockOwnerKeyUtil()) {
+  OwnerSettingsServiceFactory::GetInstance()->SetOwnerKeyUtilForTesting(
+      owner_key_util_);
 }
 
 DeviceSettingsTestBase::~DeviceSettingsTestBase() {
@@ -226,14 +228,12 @@ void DeviceSettingsTestBase::SetUp() {
   device_settings_test_helper_.set_policy_blob(device_policy_.GetBlob());
   device_settings_service_.SetSessionManager(&device_settings_test_helper_,
                                              owner_key_util_);
-  OwnerSettingsService::SetOwnerKeyUtilForTesting(owner_key_util_.get());
   OwnerSettingsService::SetDeviceSettingsServiceForTesting(
       &device_settings_service_);
   profile_.reset(new TestingProfile());
 }
 
 void DeviceSettingsTestBase::TearDown() {
-  OwnerSettingsService::SetOwnerKeyUtilForTesting(NULL);
   OwnerSettingsService::SetDeviceSettingsServiceForTesting(NULL);
   FlushDeviceSettings();
   device_settings_service_.UnsetSessionManager();
diff --git a/chrome/browser/chromeos/settings/session_manager_operation_unittest.cc b/chrome/browser/chromeos/settings/session_manager_operation_unittest.cc
index 86568f5..7c5271d 100644
--- a/chrome/browser/chromeos/settings/session_manager_operation_unittest.cc
+++ b/chrome/browser/chromeos/settings/session_manager_operation_unittest.cc
@@ -39,19 +39,19 @@ class SessionManagerOperationTest : public testing::Test {
       : ui_thread_(content::BrowserThread::UI, &message_loop_),
         file_thread_(content::BrowserThread::FILE, &message_loop_),
         owner_key_util_(new ownership::MockOwnerKeyUtil()),
-        validated_(false) {}
+        validated_(false) {
+    OwnerSettingsServiceFactory::GetInstance()->SetOwnerKeyUtilForTesting(
+        owner_key_util_);
+  }
 
   virtual void SetUp() OVERRIDE {
     policy_.payload().mutable_pinned_apps()->add_app_id("fake-app");
     policy_.Build();
 
     profile_.reset(new TestingProfile());
-    OwnerSettingsService::SetOwnerKeyUtilForTesting(owner_key_util_);
     service_ = OwnerSettingsServiceFactory::GetForProfile(profile_.get());
   }
 
-  void TearDown() { OwnerSettingsService::SetOwnerKeyUtilForTesting(NULL); }
-
   MOCK_METHOD2(OnOperationCompleted,
                void(SessionManagerOperation*, DeviceSettingsService::Status));
 
diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi
index 7c8a256..28872fa 100644
--- a/chrome/chrome_tests_unit.gypi
+++ b/chrome/chrome_tests_unit.gypi
@@ -26,7 +26,7 @@
         '../components/components.gyp:invalidation_test_support',
         '../components/components.gyp:metrics_test_support',
         '../components/components.gyp:omnibox_test_support',
-        '../components/components.gyp:ownership_test_support',
+        '../components/components.gyp:ownership',
         '../components/components.gyp:password_manager_core_browser_test_support',
         '../components/components.gyp:pref_registry_test_support',
         '../components/components.gyp:search_engines_test_support',
diff --git a/components/ownership.gypi b/components/ownership.gypi
index fb6d41a..46a7a51 100644
--- a/components/ownership.gypi
+++ b/components/ownership.gypi
@@ -14,25 +14,12 @@
       'OWNERSHIP_IMPLEMENTATION',
     ],
     'sources': [
+      'ownership/mock_owner_key_util.cc',
+      'ownership/mock_owner_key_util.h',
       'ownership/owner_key_util.cc',
       'ownership/owner_key_util.h',
       'ownership/owner_key_util_impl.cc',
       'ownership/owner_key_util_impl.h',
      ],
-  },
-  { 'target_name': 'ownership_test_support',
-    'type': '<(component)',
-    'dependencies': [
-      '<(DEPTH)/base/base.gyp:base',
-      '<(DEPTH)/crypto/crypto.gyp:crypto',
-      'ownership',
-    ],
-    'defines': [
-      'OWNERSHIP_IMPLEMENTATION',
-    ],
-    'sources': [
-      'ownership/mock_owner_key_util.cc',
-      'ownership/mock_owner_key_util.h',
-    ],
   }],
 }
diff --git a/components/ownership/BUILD.gn b/components/ownership/BUILD.gn
index b63ca1b..067f71f 100644
--- a/components/ownership/BUILD.gn
+++ b/components/ownership/BUILD.gn
@@ -4,6 +4,8 @@
 
 component("ownership") {
   sources = [
+    "mock_owner_key_util.cc",
+    "mock_owner_key_util.h",
     "owner_key_util.cc",
     "owner_key_util.h",
     "owner_key_util_impl.cc",
@@ -20,19 +22,6 @@ component("ownership") {
   ]
 }
 
-component("test_support") {
-  sources = [
-    "mock_owner_key_util.cc",
-    "mock_owner_key_util.h",
-  ]
-
-  deps = [
-    ":ownership",
-    "//base",
-    "//crypto",
-  ]
-}
-
 source_set("unit_tests") {
   sources = ["owner_key_util_unittest.cc"]