[OSCrypt] KWallet/dbus implementation should use dbus' thread

Dbus, which we use to communicate with KWallet, requires to be called
on a dedicated thread.

Bug: 782851
Change-Id: I7c55c0c6cec03d3cf688b6f1ad60326e4d5f417f
Reviewed-on: https://chromium-review.googlesource.com/803481
Commit-Queue: Christos Froussios <cfroussios@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520980}

chrome/browser/chrome_browser_main_linux.cc

@@ -26,6 +26,7 @@
 #if !defined(OS_CHROMEOS)
 #include "base/command_line.h"
 #include "base/linux_util.h"
+#include "chrome/browser/dbus/dbus_thread_linux.h"
 #include "chrome/common/chrome_paths_internal.h"
 #include "chrome/common/chrome_switches.h"
 #include "components/os_crypt/key_storage_config_linux.h"
@@ -70,9 +71,10 @@ void ChromeBrowserMainPartsLinux::PreProfileInit() {
       parsed_command_line().GetSwitchValueASCII(switches::kPasswordStore);
   // Forward the product name
   config->product_name = l10n_util::GetStringUTF8(IDS_PRODUCT_NAME);
-  // OSCrypt may target keyring, which requires calls from the main thread.
+  // OSCrypt may target backends, which require calls from specific threads.
   config->main_thread_runner = content::BrowserThread::GetTaskRunnerForThread(
       content::BrowserThread::UI);
+  config->dbus_task_runner = chrome::GetDBusTaskRunner();
   // OSCrypt can be disabled in a special settings file.
   config->should_use_preference =
       parsed_command_line().HasSwitch(switches::kEnableEncryptionSelection);

components/os_crypt/key_storage_config_linux.h

@@ -33,6 +33,9 @@ struct Config {
   bool should_use_preference;
   // Preferences are stored in a separate file in the user data directory.
   base::FilePath user_data_path;
+  // Communication with the backend via dbus needs to happen on a specific
+  // thread. Currently, only KWallet needs to use dbus.
+  scoped_refptr<base::SequencedTaskRunner> dbus_task_runner;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(Config);

components/os_crypt/key_storage_kwallet.cc

@@ -8,12 +8,18 @@
 
 #include "base/base64.h"
 #include "base/rand_util.h"
+#include "base/sequenced_task_runner.h"
 #include "components/os_crypt/kwallet_dbus.h"
 #include "dbus/bus.h"
 
-KeyStorageKWallet::KeyStorageKWallet(base::nix::DesktopEnvironment desktop_env,
-                                     std::string app_name)
-    : desktop_env_(desktop_env), handle_(-1), app_name_(std::move(app_name)) {}
+KeyStorageKWallet::KeyStorageKWallet(
+    base::nix::DesktopEnvironment desktop_env,
+    std::string app_name,
+    scoped_refptr<base::SequencedTaskRunner> dbus_task_runner)
+    : desktop_env_(desktop_env),
+      handle_(-1),
+      app_name_(std::move(app_name)),
+      dbus_task_runner_(dbus_task_runner) {}
 
 KeyStorageKWallet::~KeyStorageKWallet() {
   // The handle is shared between programs that are using the same wallet.
@@ -23,7 +29,12 @@ KeyStorageKWallet::~KeyStorageKWallet() {
   kwallet_dbus_->GetSessionBus()->ShutdownAndBlock();
 }
 
+base::SequencedTaskRunner* KeyStorageKWallet::GetTaskRunner() {
+  return dbus_task_runner_.get();
+}
+
 bool KeyStorageKWallet::Init() {
+  DCHECK(dbus_task_runner_->RunsTasksInCurrentSequence());
   // Initialize using the production KWalletDBus.
   return InitWithKWalletDBus(nullptr);
 }
@@ -80,6 +91,8 @@ KeyStorageKWallet::InitResult KeyStorageKWallet::InitWallet() {
 }
 
 std::string KeyStorageKWallet::GetKeyImpl() {
+  DCHECK(dbus_task_runner_->RunsTasksInCurrentSequence());
+
   // Get handle
   KWalletDBus::Error error =
       kwallet_dbus_->Open(wallet_name_, app_name_, &handle_);

components/os_crypt/key_storage_kwallet.h

@@ -12,10 +12,15 @@
 #include "components/os_crypt/key_storage_linux.h"
 #include "components/os_crypt/kwallet_dbus.h"
 
+namespace base {
+class SequencedTaskRunner;
+}
+
 class KeyStorageKWallet : public KeyStorageLinux {
  public:
   KeyStorageKWallet(base::nix::DesktopEnvironment desktop_env,
-                    std::string app_name);
+                    std::string app_name,
+                    scoped_refptr<base::SequencedTaskRunner> dbus_task_runner);
   ~KeyStorageKWallet() override;
 
   // Initialize using an optional KWalletDBus mock.
@@ -25,6 +30,7 @@ class KeyStorageKWallet : public KeyStorageLinux {
 
  protected:
   // KeyStorageLinux
+  base::SequencedTaskRunner* GetTaskRunner() override;
   bool Init() override;
   std::string GetKeyImpl() override;
 
@@ -46,6 +52,7 @@ class KeyStorageKWallet : public KeyStorageLinux {
   std::string wallet_name_;
   const std::string app_name_;
   std::unique_ptr<KWalletDBus> kwallet_dbus_;
+  scoped_refptr<base::SequencedTaskRunner> dbus_task_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(KeyStorageKWallet);
 };

components/os_crypt/key_storage_kwallet_unittest.cc

@@ -6,6 +6,7 @@
 
 #include "base/logging.h"
 #include "base/nix/xdg_util.h"
+#include "base/test/test_simple_task_runner.h"
 #include "dbus/message.h"
 #include "dbus/mock_bus.h"
 #include "dbus/mock_object_proxy.h"
@@ -95,7 +96,9 @@ class MockKWalletDBus : public KWalletDBus {
 
 class KeyStorageKWalletTest : public testing::Test {
  public:
-  KeyStorageKWalletTest() : key_storage_kwallet_(kDesktopEnv, "test-app") {}
+  KeyStorageKWalletTest()
+      : task_runner_(base::MakeRefCounted<base::TestSimpleTaskRunner>()),
+        key_storage_kwallet_(kDesktopEnv, "test-app", task_runner_) {}
 
   void SetUp() override {
     kwallet_dbus_mock_ = new StrictMock<MockKWalletDBus>();
@@ -120,6 +123,7 @@ class KeyStorageKWalletTest : public testing::Test {
 
  protected:
   StrictMock<MockKWalletDBus>* kwallet_dbus_mock_;
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
   KeyStorageKWallet key_storage_kwallet_;
   const std::string wallet_name_ = "mollet";
 
@@ -230,7 +234,8 @@ class KeyStorageKWalletFailuresTest
     : public testing::TestWithParam<KWalletDBus::Error> {
  public:
   KeyStorageKWalletFailuresTest()
-      : key_storage_kwallet_(kDesktopEnv, "test-app") {}
+      : task_runner_(new base::TestSimpleTaskRunner()),
+        key_storage_kwallet_(kDesktopEnv, "test-app", task_runner_) {}
 
   void SetUp() override {
     // |key_storage_kwallet_| will take ownership of |kwallet_dbus_mock_|.
@@ -255,6 +260,7 @@ class KeyStorageKWalletFailuresTest
 
  protected:
   StrictMock<MockKWalletDBus>* kwallet_dbus_mock_;
+  scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
   KeyStorageKWallet key_storage_kwallet_;
   const std::string wallet_name_ = "mollet";
 

components/os_crypt/key_storage_linux.cc

@@ -80,7 +80,7 @@ std::unique_ptr<KeyStorageLinux> KeyStorageLinux::CreateService(
 #if defined(USE_LIBSECRET)
   if (selected_backend == os_crypt::SelectedLinuxBackend::GNOME_ANY ||
       selected_backend == os_crypt::SelectedLinuxBackend::GNOME_LIBSECRET) {
-    key_storage.reset(new KeyStorageLibsecret());
+    key_storage = std::make_unique<KeyStorageLibsecret>();
     if (key_storage->WaitForInitOnTaskRunner()) {
       VLOG(1) << "OSCrypt using Libsecret as backend.";
       return key_storage;
@@ -91,7 +91,8 @@ 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(config.main_thread_runner));
+    key_storage =
+        std::make_unique<KeyStorageKeyring>(config.main_thread_runner);
     if (key_storage->WaitForInitOnTaskRunner()) {
       VLOG(1) << "OSCrypt using Keyring as backend.";
       return key_storage;
@@ -107,8 +108,8 @@ std::unique_ptr<KeyStorageLinux> KeyStorageLinux::CreateService(
         selected_backend == os_crypt::SelectedLinuxBackend::KWALLET
             ? base::nix::DESKTOP_ENVIRONMENT_KDE4
             : base::nix::DESKTOP_ENVIRONMENT_KDE5;
-    key_storage.reset(
-        new KeyStorageKWallet(used_desktop_env, config.product_name));
+    key_storage = std::make_unique<KeyStorageKWallet>(
+        used_desktop_env, config.product_name, config.dbus_task_runner);
     if (key_storage->WaitForInitOnTaskRunner()) {
       VLOG(1) << "OSCrypt using KWallet as backend.";
       return key_storage;