Consolidate config parameters for OSCrypt

The individual setters for each parameter required for OSCrypt's
initialisation are replaced by a single setter for a Config struct.
The Config definition is moved to a separate file.

The result is less code, especially in the top level files of OSCrypt.

Bug:709096

Change-Id: If6f05129879fcae86b1b2bf032641b05418fe8ee
Reviewed-on: https://chromium-review.googlesource.com/565567Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Commit-Queue: Christos Froussios <cfroussios@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486717}

chrome/browser/chrome_browser_main_linux.cc

@@ -6,7 +6,9 @@
 
 #include <fontconfig/fontconfig.h>
 
+#include <memory>
 #include <string>
+#include <utility>
 
 #include "base/files/file_path.h"
 #include "base/single_thread_task_runner.h"
@@ -26,6 +28,7 @@
 #include "base/linux_util.h"
 #include "chrome/common/chrome_paths_internal.h"
 #include "chrome/common/chrome_switches.h"
+#include "components/os_crypt/key_storage_config_linux.h"
 #include "components/os_crypt/os_crypt.h"
 #include "content/public/browser/browser_thread.h"
 #endif
@@ -61,22 +64,20 @@ void ChromeBrowserMainPartsLinux::PreProfileInit() {
       l10n_util::GetStringUTF8(IDS_SHORT_PRODUCT_NAME));
 
 #if !defined(OS_CHROMEOS)
+  std::unique_ptr<os_crypt::Config> config(new os_crypt::Config());
   // Forward to os_crypt the flag to use a specific password store.
-  OSCrypt::SetStore(
-      parsed_command_line().GetSwitchValueASCII(switches::kPasswordStore));
+  config->store =
+      parsed_command_line().GetSwitchValueASCII(switches::kPasswordStore);
   // Forward the product name
-  OSCrypt::SetProductName(l10n_util::GetStringUTF8(IDS_PRODUCT_NAME));
+  config->product_name = l10n_util::GetStringUTF8(IDS_PRODUCT_NAME);
   // OSCrypt may target keyring, which requires calls from the main thread.
-  scoped_refptr<base::SingleThreadTaskRunner> main_thread_runner(
-      content::BrowserThread::GetTaskRunnerForThread(
-          content::BrowserThread::UI));
-  OSCrypt::SetMainThreadRunner(main_thread_runner);
+  config->main_thread_runner = content::BrowserThread::GetTaskRunnerForThread(
+      content::BrowserThread::UI);
   // OSCrypt can be disabled in a special settings file.
-  OSCrypt::ShouldUsePreference(
-      parsed_command_line().HasSwitch(switches::kEnableEncryptionSelection));
-  base::FilePath user_data_dir;
-  chrome::GetDefaultUserDataDirectory(&user_data_dir);
-  OSCrypt::SetUserDataPath(user_data_dir);
+  config->should_use_preference =
+      parsed_command_line().HasSwitch(switches::kEnableEncryptionSelection);
+  chrome::GetDefaultUserDataDirectory(&config->user_data_path);
+  OSCrypt::SetConfig(std::move(config));
 #endif
 
   ChromeBrowserMainPartsPosix::PreProfileInit();

components/os_crypt/BUILD.gn

@@ -72,6 +72,8 @@ static_library("os_crypt") {
   if (is_desktop_linux) {
     sources -= [ "os_crypt_posix.cc" ]
     sources += [
+      "key_storage_config_linux.cc",
+      "key_storage_config_linux.h",
       "key_storage_linux.cc",
       "key_storage_linux.h",
       "key_storage_util_linux.cc",

components/os_crypt/key_storage_config_linux.cc

+// Copyright 2017 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 "components/os_crypt/key_storage_config_linux.h"
+
+namespace os_crypt {
+
+Config::Config() = default;
+Config::~Config() = default;
+
+}  // namespace os_crypt

components/os_crypt/key_storage_config_linux.h

+// Copyright 2017 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 COMPONENTS_OS_CRYPT_KEY_STORAGE_CONFIG_LINUX_H_
+#define COMPONENTS_OS_CRYPT_KEY_STORAGE_CONFIG_LINUX_H_
+
+#include <memory>
+#include <string>
+
+#include "base/files/file_path.h"
+#include "base/macros.h"
+#include "base/memory/ref_counted.h"
+#include "base/single_thread_task_runner.h"
+
+namespace os_crypt {
+
+// A container for all the initialisation parameters for OSCrypt.
+struct Config {
+ public:
+  Config();
+  ~Config();
+
+  // Force OSCrypt to use a specific linux password store.
+  std::string store;
+  // The product name to use for permission prompts.
+  std::string product_name;
+  // A runner on the main thread for gnome-keyring to be called from.
+  // TODO(crbug/466975): Libsecret and KWallet don't need this. We can remove
+  // this when we stop supporting keyring.
+  scoped_refptr<base::SingleThreadTaskRunner> main_thread_runner;
+  // Controls whether preference on using or ignoring backends is used.
+  bool should_use_preference;
+  // Preferences are stored in a separate file in the user data directory.
+  base::FilePath user_data_path;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(Config);
+};
+
+}  // namespace os_crypt
+
+#endif  // COMPONENTS_OS_CRYPT_KEY_STORAGE_CONFIG_LINUX_H_

components/os_crypt/key_storage_keyring.h

@@ -8,6 +8,7 @@
 #include <string>
 
 #include "base/macros.h"
+#include "base/memory/ref_counted.h"
 #include "components/os_crypt/key_storage_linux.h"
 
 namespace base {

components/os_crypt/key_storage_linux.cc

@@ -5,13 +5,9 @@
 #include "components/os_crypt/key_storage_linux.h"
 
 #include "base/environment.h"
-#include "base/files/file.h"
-#include "base/files/file_path.h"
-#include "base/files/file_util.h"
-#include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/nix/xdg_util.h"
-#include "base/single_thread_task_runner.h"
+#include "components/os_crypt/key_storage_config_linux.h"
 #include "components/os_crypt/key_storage_util_linux.h"
 
 #if defined(USE_LIBSECRET)
@@ -32,61 +28,18 @@ const char KeyStorageLinux::kFolderName[] = "Chromium Keys";
 const char KeyStorageLinux::kKey[] = "Chromium Safe Storage";
 #endif
 
-namespace {
-
-// Parameters to OSCrypt, which are set before the first call to OSCrypt, are
-// stored here.
-struct Configuration {
-  std::string store;
-  std::string product_name;
-  scoped_refptr<base::SingleThreadTaskRunner> main_thread_runner;
-  bool should_use_preference;
-  base::FilePath user_data_path;
-};
-
-base::LazyInstance<Configuration>::DestructorAtExit g_config =
-    LAZY_INSTANCE_INITIALIZER;
-
-}  // namespace
-
-// static
-void KeyStorageLinux::SetStore(const std::string& store_type) {
-  g_config.Get().store = store_type;
-  VLOG(1) << "OSCrypt store set to " << store_type;
-}
-
-// static
-void KeyStorageLinux::SetProductName(const std::string& product_name) {
-  g_config.Get().product_name = product_name;
-}
-
-// static
-void KeyStorageLinux::SetMainThreadRunner(
-    scoped_refptr<base::SingleThreadTaskRunner> main_thread_runner) {
-  g_config.Get().main_thread_runner = main_thread_runner;
-}
-
-// static
-void KeyStorageLinux::ShouldUsePreference(bool should_use_preference) {
-  g_config.Get().should_use_preference = should_use_preference;
-}
-
-// static
-void KeyStorageLinux::SetUserDataPath(const base::FilePath& path) {
-  g_config.Get().user_data_path = path;
-}
-
 // static
-std::unique_ptr<KeyStorageLinux> KeyStorageLinux::CreateService() {
+std::unique_ptr<KeyStorageLinux> KeyStorageLinux::CreateService(
+    const os_crypt::Config& config) {
 #if defined(USE_LIBSECRET) || defined(USE_KEYRING) || defined(USE_KWALLET)
   // Select a backend.
-  bool use_backend = !g_config.Get().should_use_preference ||
-                     os_crypt::GetBackendUse(g_config.Get().user_data_path);
+  bool use_backend = !config.should_use_preference ||
+                     os_crypt::GetBackendUse(config.user_data_path);
   std::unique_ptr<base::Environment> env(base::Environment::Create());
   base::nix::DesktopEnvironment desktop_env =
       base::nix::GetDesktopEnvironment(env.get());
   os_crypt::SelectedLinuxBackend selected_backend =
-      os_crypt::SelectBackend(g_config.Get().store, use_backend, desktop_env);
+      os_crypt::SelectBackend(config.store, use_backend, desktop_env);
 
   // Try initializing the selected backend.
   // In case of GNOME_ANY, prefer Libsecret
@@ -106,7 +59,7 @@ std::unique_ptr<KeyStorageLinux> KeyStorageLinux::CreateService() {
 #if defined(USE_KEYRING)
   if (selected_backend == os_crypt::SelectedLinuxBackend::GNOME_ANY ||
       selected_backend == os_crypt::SelectedLinuxBackend::GNOME_KEYRING) {
-    key_storage.reset(new KeyStorageKeyring(g_config.Get().main_thread_runner));
+    key_storage.reset(new KeyStorageKeyring(config.main_thread_runner));
     if (key_storage->Init()) {
       VLOG(1) << "OSCrypt using Keyring as backend.";
       return key_storage;
@@ -117,13 +70,13 @@ std::unique_ptr<KeyStorageLinux> KeyStorageLinux::CreateService() {
 #if defined(USE_KWALLET)
   if (selected_backend == os_crypt::SelectedLinuxBackend::KWALLET ||
       selected_backend == os_crypt::SelectedLinuxBackend::KWALLET5) {
-    DCHECK(!g_config.Get().product_name.empty());
+    DCHECK(!config.product_name.empty());
     base::nix::DesktopEnvironment used_desktop_env =
         selected_backend == os_crypt::SelectedLinuxBackend::KWALLET
             ? base::nix::DESKTOP_ENVIRONMENT_KDE4
             : base::nix::DESKTOP_ENVIRONMENT_KDE5;
     key_storage.reset(
-        new KeyStorageKWallet(used_desktop_env, g_config.Get().product_name));
+        new KeyStorageKWallet(used_desktop_env, config.product_name));
     if (key_storage->Init()) {
       VLOG(1) << "OSCrypt using KWallet as backend.";
       return key_storage;

components/os_crypt/key_storage_linux.h

@@ -9,12 +9,10 @@
 #include <string>
 
 #include "base/macros.h"
-#include "base/memory/ref_counted.h"
 
-namespace base {
-class FilePath;
-class SingleThreadTaskRunner;
-}  // namespace base
+namespace os_crypt {
+struct Config;
+}
 
 // An API for retrieving OSCrypt's password from the system's password storage
 // service.
@@ -23,26 +21,9 @@ class KeyStorageLinux {
   KeyStorageLinux() = default;
   virtual ~KeyStorageLinux() = default;
 
-  // Force OSCrypt to use a specific linux password store.
-  static void SetStore(const std::string& store_type);
-
-  // The product name to use for permission prompts.
-  static void SetProductName(const std::string& product_name);
-
-  // A runner on the main thread for gnome-keyring to be called from.
-  // TODO(crbug/466975): Libsecret and KWallet don't need this. We can remove
-  // this when we stop supporting keyring.
-  static void SetMainThreadRunner(
-      scoped_refptr<base::SingleThreadTaskRunner> main_thread_runner);
-
-  // Controls whether preference on using or ignoring backends is used.
-  static void ShouldUsePreference(bool should_use_preference);
-
-  // Preferences are stored in a separate file in the user data directory.
-  static void SetUserDataPath(const base::FilePath& path);
-
   // Tries to load the appropriate key storage. Returns null if none succeed.
-  static std::unique_ptr<KeyStorageLinux> CreateService();
+  static std::unique_ptr<KeyStorageLinux> CreateService(
+      const os_crypt::Config& config);
 
   // Gets the encryption key from the OS password-managing library. If a key is
   // not found, a new key will be generated, stored and returned.

components/os_crypt/os_crypt.h

@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_OS_CRYPT_OS_CRYPT_H_
 #define COMPONENTS_OS_CRYPT_OS_CRYPT_H_
 
+#include <memory>
 #include <string>
 
 #include "base/macros.h"
@@ -13,12 +14,12 @@
 #include "base/strings/string16.h"
 #include "build/build_config.h"
 
-#if defined(OS_LINUX) && !defined(OS_CHROMEOS)
-#include "components/os_crypt/key_storage_linux.h"
-#endif  // defined(OS_LINUX) && !defined(OS_CHROMEOS)
+#if defined(OS_LINUX) && !defined(OS_CHROMEOS) && defined(UNIT_TEST)
+class KeyStorageLinux;
+#endif  // defined(OS_LINUX) && !defined(OS_CHROMEOS) && defined(UNIT_TEST)
 
-namespace base {
-class FilePath;
+namespace os_crypt {
+struct Config;
 }
 
 // The OSCrypt class gives access to simple encryption and decryption of
@@ -28,27 +29,8 @@ class FilePath;
 class OSCrypt {
  public:
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
-  // If |store_type| is a known password store, we will attempt to use it.
-  // In any other case, we default to auto-detecting the store.
-  // This should not be changed after OSCrypt has been used.
-  static void SetStore(const std::string& store_type);
-
-  // Some password stores may prompt the user for permission and show the
-  // application name.
-  static void SetProductName(const std::string& product_name);
-
-  // The gnome-keyring implementation requires calls from the main thread.
-  // TODO(crbug/466975): Libsecret and KWallet don't need this. We can remove
-  // this when we stop supporting keyring.
-  static void SetMainThreadRunner(
-      scoped_refptr<base::SingleThreadTaskRunner> main_thread_runner);
-
-  // Enable the feature where we determine if we should try a backend via a
-  // preference file.
-  static void ShouldUsePreference(bool should_use_preference);
-
-  // Set the folder, where OSCrypt will check for its preference file.
-  static void SetUserDataPath(const base::FilePath& path);
+  // Set the configuration of OSCrypt.
+  static void SetConfig(std::unique_ptr<os_crypt::Config> config);
 
   // Returns true iff the real secret key (not hardcoded one) is available.
   static bool IsEncryptionAvailable();

components/os_crypt/os_crypt_linux.cc

@@ -8,7 +8,6 @@
 
 #include <algorithm>
 #include <iterator>
-#include <memory>
 
 #include "base/lazy_instance.h"
 #include "base/logging.h"
@@ -16,6 +15,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/synchronization/lock.h"
+#include "components/os_crypt/key_storage_config_linux.h"
 #include "components/os_crypt/key_storage_linux.h"
 #include "crypto/encryptor.h"
 #include "crypto/symmetric_key.h"
@@ -56,6 +56,7 @@ struct Cache {
   std::unique_ptr<std::string> password_v11_cache;
   bool is_key_storage_cached;
   bool is_password_v11_cached;
+  std::unique_ptr<os_crypt::Config> config;
   // Guards access to |g_cache|, making lazy initialization of individual parts
   // thread safe.
   base::Lock lock;
@@ -67,8 +68,10 @@ base::LazyInstance<Cache>::Leaky g_cache = LAZY_INSTANCE_INITIALIZER;
 // found.
 KeyStorageLinux* GetKeyStorage() {
   if (!g_cache.Get().is_key_storage_cached) {
+    DCHECK(g_cache.Get().config);
     g_cache.Get().is_key_storage_cached = true;
-    g_cache.Get().key_storage_cache = KeyStorageLinux::CreateService();
+    g_cache.Get().key_storage_cache =
+        KeyStorageLinux::CreateService(*g_cache.Get().config);
   }
   return g_cache.Get().key_storage_cache.get();
 }
@@ -225,44 +228,10 @@ bool OSCrypt::DecryptString(const std::string& ciphertext,
 }
 
 // static
-void OSCrypt::SetStore(const std::string& store_type) {
-  // Changing the targeted password store makes no sense after initializing.
-  DCHECK(!g_cache.Get().is_key_storage_cached);
-
-  KeyStorageLinux::SetStore(store_type);
-}
-
-// static
-void OSCrypt::SetProductName(const std::string& product_name) {
-  // Setting the product name makes no sense after initializing.
-  DCHECK(!g_cache.Get().is_key_storage_cached);
-
-  KeyStorageLinux::SetProductName(product_name);
-}
-
-// static
-void OSCrypt::SetMainThreadRunner(
-    scoped_refptr<base::SingleThreadTaskRunner> main_thread_runner) {
-  // Setting the task runner makes no sense after initializing.
-  DCHECK(!g_cache.Get().is_key_storage_cached);
-
-  KeyStorageLinux::SetMainThreadRunner(main_thread_runner);
-}
-
-// static
-void OSCrypt::ShouldUsePreference(bool should_use_preference) {
+void OSCrypt::SetConfig(std::unique_ptr<os_crypt::Config> config) {
   // Setting initialisation parameters makes no sense after initializing.
   DCHECK(!g_cache.Get().is_key_storage_cached);
-
-  KeyStorageLinux::ShouldUsePreference(should_use_preference);
-}
-
-// static
-void OSCrypt::SetUserDataPath(const base::FilePath& path) {
-  // Setting initialisation parameters makes no sense after initializing.
-  DCHECK(!g_cache.Get().is_key_storage_cached);
-
-  KeyStorageLinux::SetUserDataPath(path);
+  g_cache.Get().config = std::move(config);
 }
 
 // static

components/os_crypt/os_crypt_mocker_linux.cc

@@ -4,9 +4,13 @@
 
 #include "components/os_crypt/os_crypt_mocker_linux.h"
 
+#include <memory>
+
 #include "base/base64.h"
 #include "base/lazy_instance.h"
+#include "base/memory/ptr_util.h"
 #include "base/rand_util.h"
+#include "components/os_crypt/key_storage_config_linux.h"
 #include "components/os_crypt/os_crypt.h"
 
 namespace {
@@ -50,6 +54,7 @@ OSCryptMockerLinux* OSCryptMockerLinux::GetInstance() {
 // static
 void OSCryptMockerLinux::SetUpWithSingleton() {
   UseMockKeyStorageForTesting(&GetKeyStorage, &GetPassword);
+  OSCrypt::SetConfig(base::MakeUnique<os_crypt::Config>());
 }
 
 // static

components/password_manager/core/browser/hash_password_manager_unittest.cc

@@ -24,13 +24,13 @@ class HashPasswordManagerTest : public testing::Test {
     prefs_.registry()->RegisterStringPref(prefs::kSyncPasswordLengthAndHashSalt,
                                           std::string(),
                                           PrefRegistry::NO_REGISTRATION_FLAGS);
-#if defined(OS_MACOSX)
-    // Mock Keychain. There is a call to Keychain on initializling
+    // Mock OSCrypt. There is a call to OSCrypt on initializling
     // PasswordReuseDetector, so it should be mocked.
     OSCryptMocker::SetUpWithSingleton();
-#endif
   }
 
+  ~HashPasswordManagerTest() override { OSCryptMocker::TearDown(); }
+
  protected:
   TestingPrefServiceSimple prefs_;
 };

components/password_manager/core/browser/password_store_unittest.cc

@@ -24,6 +24,7 @@
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
+#include "components/os_crypt/os_crypt_mocker.h"
 #include "components/password_manager/core/browser/android_affiliation/affiliated_match_helper.h"
 #include "components/password_manager/core/browser/android_affiliation/affiliation_service.h"
 #include "components/password_manager/core/browser/android_affiliation/mock_affiliated_match_helper.h"
@@ -38,10 +39,6 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-#if defined(OS_MACOSX)
-#include "components/os_crypt/os_crypt_mocker.h"
-#endif
-
 using autofill::PasswordForm;
 using base::WaitableEvent;
 using testing::_;
@@ -120,18 +117,14 @@ class PasswordStoreTest : public testing::Test {
 
   void SetUp() override {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
-#if defined(OS_MACOSX)
-    // Mock Keychain. There is a call to Keychain on initializling
+    // Mock OSCrypt. There is a call to OSCrypt on initializling
     // PasswordReuseDetector, so it should be mocked.
     OSCryptMocker::SetUpWithSingleton();
-#endif
   }
 
   void TearDown() override {
     ASSERT_TRUE(temp_dir_.Delete());
-#if defined(OS_MACOSX)
     OSCryptMocker::TearDown();
-#endif
   }
 
   base::FilePath test_login_db_file_path() const {