Use openAsync for KWallet in FreedesktopSecretKeyProvider

This CL switches FreedesktopSecretKeyProvider to use openAsync instead
of open when using KWallet. This is to avoid timing out (the default
DBus timeout is 25s) while KWallet prompts the user for a password.

The sync backend will continue to use open instead of openAsync since
it's going to be removed soon.

R=thestig

Fixed: 448853884
Change-Id: Iec000ce30f57fbcf757bb92e9ac2c0456ea38faf
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7056328
Reviewed-by: Lei Zhang <[email protected]>
Commit-Queue: Thomas Anderson <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1532378}

Author: tanderson-google

components/os_crypt/async/browser/freedesktop_secret_key_provider.cc

@@ -24,6 +24,7 @@
 #include "base/strings/string_util.h"
 #include "components/dbus/thread_linux/dbus_thread_linux.h"
 #include "components/dbus/utils/call_method.h"
+#include "components/dbus/utils/connect_to_signal.h"
 #include "components/dbus/utils/variant.h"
 #include "components/os_crypt/async/common/algorithm.mojom.h"
 #include "crypto/kdf.h"
@@ -171,8 +172,9 @@ class FreedesktopSecretKeyProvider::Prompter
   void StartPrompt() {
     auto* prompt_proxy = bus_->GetObjectProxy(
         FreedesktopSecretKeyProvider::kSecretServiceName, prompt_path_);
-    prompt_proxy->ConnectToSignal(
-        FreedesktopSecretKeyProvider::kSecretPromptInterface, "Completed",
+    dbus_utils::ConnectToSignal(
+        prompt_proxy, FreedesktopSecretKeyProvider::kSecretPromptInterface,
+        "Completed",
         base::BindRepeating(&Prompter::OnPromptCompletedSignal, this),
         base::BindOnce(&Prompter::OnSignalConnected, this));
     dbus_utils::CallMethod<"s", "">(
@@ -197,23 +199,22 @@ class FreedesktopSecretKeyProvider::Prompter
     }
   }
 
-  void OnPromptCompletedSignal(dbus::Signal* signal) {
-    dbus::MessageReader reader(signal);
-    auto dismissed = dbus_utils::ReadValue<bool>(reader);
-    auto variant = dbus_utils::ReadValue<dbus_utils::Variant>(reader);
-    if (!dismissed.has_value() || !variant.has_value() ||
-        reader.HasMoreData()) {
+  void OnPromptCompletedSignal(
+      dbus_utils::ConnectToSignalResult<bool, dbus_utils::Variant> result) {
+    if (!result.has_value()) {
       LOG(ERROR) << "Failed to read Prompt.Completed signal args.";
       Finish(base::unexpected(ErrorDetail::kInvalidSignalFormat));
       return;
     }
 
-    if (dismissed.value()) {
+    auto& [dismissed, variant] = result.value();
+
+    if (dismissed) {
       Finish(base::unexpected(ErrorDetail::kPromptDismissed));
       return;
     }
 
-    auto value = std::move(variant.value()).Take<T>();
+    auto value = std::move(variant).Take<T>();
     if (!value) {
       LOG(ERROR) << "Failed to parse prompt result.";
       Finish(base::unexpected(ErrorDetail::kInvalidVariantFormat));
@@ -268,6 +269,8 @@ void FreedesktopSecretKeyProvider::GetKey(KeyCallback callback) {
 
   // Reset state in case GetKey is called multiple times.
   kwallet_proxy_ = nullptr;
+  kwallet_handle_ = kKWalletInvalidHandle;
+  kwallet_transaction_id_ = kKWalletInvalidTransactionId;
   session_opened_ = false;
   session_proxy_ = nullptr;
   default_collection_proxy_ = nullptr;
@@ -580,23 +583,80 @@ void FreedesktopSecretKeyProvider::OnKWalletNetworkWallet(
                     DbusErrorToErrorDetail(wallet_name.error()));
     return;
   }
-  dbus_utils::CallMethod<"sxs", "i">(
-      kwallet_proxy_, kKWalletInterface, kKWalletMethodOpen,
-      base::BindOnce(&FreedesktopSecretKeyProvider::OnKWalletOpen,
+
+  dbus_utils::ConnectToSignal(
+      kwallet_proxy_, kKWalletInterface, kKWalletSignalWalletAsyncOpened,
+      base::BindRepeating(
+          &FreedesktopSecretKeyProvider::OnKWalletWalletAsyncOpened,
+          weak_ptr_factory_.GetWeakPtr()),
+      base::BindOnce(&FreedesktopSecretKeyProvider::OnSignalConnected,
+                     weak_ptr_factory_.GetWeakPtr()));
+
+  // handleSession = true means KWallet will take care of keeping the wallet
+  // open as long as the application is running.
+  constexpr bool kHandleSession = true;
+  dbus_utils::CallMethod<"sxsb", "i">(
+      kwallet_proxy_, kKWalletInterface, kKWalletMethodOpenAsync,
+      base::BindOnce(&FreedesktopSecretKeyProvider::OnKWalletOpenAsync,
                      weak_ptr_factory_.GetWeakPtr()),
-      std::get<0>(wallet_name.value()), 0, product_name_);
+      std::get<0>(wallet_name.value()), 0, product_name_, kHandleSession);
+}
+
+void FreedesktopSecretKeyProvider::OnKWalletOpenAsync(
+    dbus_utils::CallMethodResultSig<"i"> t_id) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  if (!t_id.has_value()) {
+    FinalizeFailure(InitStatus::kKWalletOpenFailed,
+                    DbusErrorToErrorDetail(t_id.error()));
+    return;
+  }
+
+  kwallet_transaction_id_ = std::get<0>(t_id.value());
+}
+
+void FreedesktopSecretKeyProvider::OnSignalConnected(
+    const std::string& interface_name,
+    const std::string& signal_name,
+    bool connected) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  if (!connected) {
+    LOG(ERROR) << "Failed to connect to " << signal_name << " signal.";
+    FinalizeFailure(InitStatus::kKWalletOpenFailed,
+                    ErrorDetail::kPromptFailedSignalConnection);
+  }
 }
 
-void FreedesktopSecretKeyProvider::OnKWalletOpen(
-    dbus_utils::CallMethodResultSig<"i"> handle_reply) {
+void FreedesktopSecretKeyProvider::OnKWalletWalletAsyncOpened(
+    dbus_utils::ConnectToSignalResult<int32_t, int32_t> result) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  if (!handle_reply.has_value()) {
+
+  if (kwallet_transaction_id_ == kKWalletInvalidTransactionId) {
+    return;
+  }
+
+  if (!result.has_value()) {
+    LOG(ERROR) << "Failed to read walletAsyncOpened signal args.";
     FinalizeFailure(InitStatus::kKWalletOpenFailed,
-                    DbusErrorToErrorDetail(handle_reply.error()));
+                    ErrorDetail::kInvalidSignalFormat);
     return;
   }
 
-  kwallet_handle_ = std::get<0>(handle_reply.value());
+  auto& [t_id, handle] = result.value();
+
+  if (t_id != kwallet_transaction_id_) {
+    return;
+  }
+
+  // Reset transaction ID to avoid processing the signal again.
+  kwallet_transaction_id_ = kKWalletInvalidTransactionId;
+
+  OnKWalletOpen(handle);
+}
+
+void FreedesktopSecretKeyProvider::OnKWalletOpen(int32_t handle) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  kwallet_handle_ = handle;
   if (kwallet_handle_ == kKWalletInvalidHandle) {
     FinalizeFailure(InitStatus::kKWalletOpenFailed,
                     ErrorDetail::kKWalletApiReturnedError);

components/os_crypt/async/browser/freedesktop_secret_key_provider.h

@@ -21,6 +21,7 @@
 #include "build/branding_buildflags.h"
 #include "components/dbus/utils/call_method.h"
 #include "components/dbus/utils/check_for_service_and_start.h"
+#include "components/dbus/utils/connect_to_signal.h"
 #include "components/dbus/utils/name_has_owner.h"
 #include "components/os_crypt/async/browser/key_provider.h"
 #include "dbus/bus.h"
@@ -130,14 +131,16 @@ class FreedesktopSecretKeyProvider : public KeyProvider {
   static constexpr char kKWalletInterface[] = "org.kde.KWallet";
   static constexpr char kKWalletMethodIsEnabled[] = "isEnabled";
   static constexpr char kKWalletMethodNetworkWallet[] = "networkWallet";
-  static constexpr char kKWalletMethodOpen[] = "open";
+  static constexpr char kKWalletMethodOpenAsync[] = "openAsync";
   static constexpr char kKWalletMethodReadPassword[] = "readPassword";
   static constexpr char kKWalletMethodClose[] = "close";
   static constexpr char kKWalletMethodHasFolder[] = "hasFolder";
   static constexpr char kKWalletMethodCreateFolder[] = "createFolder";
   static constexpr char kKWalletMethodHasEntry[] = "hasEntry";
   static constexpr char kKWalletMethodWritePassword[] = "writePassword";
 
+  static constexpr char kKWalletSignalWalletAsyncOpened[] = "walletAsyncOpened";
+
   static constexpr char kDefaultAlias[] = "default";
 
   // These constants are duplicated from the sync backend.
@@ -167,6 +170,7 @@ class FreedesktopSecretKeyProvider : public KeyProvider {
   static constexpr char kKWalletD6Path[] = "/modules/kwalletd6";
 
   static constexpr int kKWalletInvalidHandle = -1;
+  static constexpr int kKWalletInvalidTransactionId = -1;
 
 #if BUILDFLAG(GOOGLE_CHROME_BRANDING)
   static constexpr char kKWalletFolder[] = "Chrome Keys";
@@ -196,7 +200,13 @@ class FreedesktopSecretKeyProvider : public KeyProvider {
   void OnKWalletServiceStarted(std::optional<bool> has_owner);
   void OnKWalletIsEnabled(dbus_utils::CallMethodResultSig<"b"> is_enabled);
   void OnKWalletNetworkWallet(dbus_utils::CallMethodResultSig<"s"> wallet_name);
-  void OnKWalletOpen(dbus_utils::CallMethodResultSig<"i"> handle_reply);
+  void OnKWalletOpenAsync(dbus_utils::CallMethodResultSig<"i"> t_id);
+  void OnKWalletWalletAsyncOpened(
+      dbus_utils::ConnectToSignalResult<int32_t, int32_t> result);
+  void OnSignalConnected(const std::string& interface_name,
+                         const std::string& signal_name,
+                         bool connected);
+  void OnKWalletOpen(int32_t handle);
   void OnKWalletHasFolder(dbus_utils::CallMethodResultSig<"b"> has_folder);
   void OnKWalletCreateFolder(dbus_utils::CallMethodResultSig<"b"> success);
   void OnKWalletHasEntry(dbus_utils::CallMethodResultSig<"b"> has_entry);
@@ -224,6 +234,7 @@ class FreedesktopSecretKeyProvider : public KeyProvider {
   // For KWallet password storage
   raw_ptr<dbus::ObjectProxy> kwallet_proxy_ = nullptr;
   int32_t kwallet_handle_ = kKWalletInvalidHandle;
+  int32_t kwallet_transaction_id_ = kKWalletInvalidTransactionId;
 
   const std::string password_store_;
   const std::string product_name_;

components/os_crypt/async/browser/freedesktop_secret_key_provider_unittest.cc

@@ -52,6 +52,7 @@ constexpr char kItemPromptPath[] =
     "/org/freedesktop/secrets/prompt/item_prompt";
 constexpr char kNetworkWallet[] = "kdewallet";
 constexpr int32_t kKWalletHandle = 42;
+constexpr int32_t kTransactionId = 123;
 
 constexpr char kFakeSecret[] = "c3VwZXJfc2VjcmV0X2tleQ==";
 
@@ -601,12 +602,29 @@ TEST(FreedesktopSecretKeyProviderTest, KWallet) {
                    MatchArgs(), _))
       .WillOnce(RespondWith(std::string(kNetworkWallet)));
 
-  // open -> non-negative handle
+  // ConnectToSignal walletAsyncOpened
+  dbus::ObjectProxy::SignalCallback signal_callback;
+  EXPECT_CALL(
+      *mock_kwallet5_proxy,
+      ConnectToSignal(
+          FreedesktopSecretKeyProvider::kKWalletInterface,
+          FreedesktopSecretKeyProvider::kKWalletSignalWalletAsyncOpened, _, _))
+      .WillOnce([&](const std::string& interface_name,
+                    const std::string& signal_name,
+                    dbus::ObjectProxy::SignalCallback callback,
+                    dbus::ObjectProxy::OnConnectedCallback on_connected) {
+        std::move(on_connected).Run(interface_name, signal_name, true);
+        signal_callback = std::move(callback);
+      });
+
+  // openAsync -> transaction ID
   EXPECT_CALL(*mock_kwallet5_proxy,
               Call(FreedesktopSecretKeyProvider::kKWalletInterface,
-                   FreedesktopSecretKeyProvider::kKWalletMethodOpen,
-                   MatchArgs(kNetworkWallet, int64_t(0), kProductName), _))
-      .WillOnce(RespondWith(kKWalletHandle));
+                   FreedesktopSecretKeyProvider::kKWalletMethodOpenAsync,
+                   MatchArgs(kNetworkWallet, static_cast<int64_t>(0),
+                             kProductName, true),
+                   _))
+      .WillOnce(RespondWith(kTransactionId));
 
   // hasFolder -> true
   EXPECT_CALL(*mock_kwallet5_proxy,
@@ -657,6 +675,16 @@ TEST(FreedesktopSecretKeyProviderTest, KWallet) {
         key = std::move(returned_key.value());
       }));
 
+  // Simulate walletAsyncOpened signal
+  ASSERT_TRUE(signal_callback);
+  auto signal = dbus::Signal(
+      FreedesktopSecretKeyProvider::kKWalletInterface,
+      FreedesktopSecretKeyProvider::kKWalletSignalWalletAsyncOpened);
+  dbus::MessageWriter writer(&signal);
+  dbus_utils::WriteValue(writer, kTransactionId);
+  dbus_utils::WriteValue(writer, kKWalletHandle);
+  signal_callback.Run(&signal);
+
   EXPECT_EQ(tag, "v11");
   EXPECT_TRUE(key.has_value());
 }
@@ -701,12 +729,29 @@ TEST(FreedesktopSecretKeyProviderTest, KWalletCreateFolderAndPassword) {
                    MatchArgs(), _))
       .WillOnce(RespondWith(std::string(kNetworkWallet)));
 
-  // open -> non-negative handle
+  // ConnectToSignal walletAsyncOpened
+  dbus::ObjectProxy::SignalCallback signal_callback;
+  EXPECT_CALL(
+      *mock_kwallet6_proxy,
+      ConnectToSignal(
+          FreedesktopSecretKeyProvider::kKWalletInterface,
+          FreedesktopSecretKeyProvider::kKWalletSignalWalletAsyncOpened, _, _))
+      .WillOnce([&](const std::string& interface_name,
+                    const std::string& signal_name,
+                    dbus::ObjectProxy::SignalCallback callback,
+                    dbus::ObjectProxy::OnConnectedCallback on_connected) {
+        std::move(on_connected).Run(interface_name, signal_name, true);
+        signal_callback = std::move(callback);
+      });
+
+  // openAsync -> transaction ID
   EXPECT_CALL(*mock_kwallet6_proxy,
               Call(FreedesktopSecretKeyProvider::kKWalletInterface,
-                   FreedesktopSecretKeyProvider::kKWalletMethodOpen,
-                   MatchArgs(kNetworkWallet, int64_t(0), kProductName), _))
-      .WillOnce(RespondWith(kKWalletHandle));
+                   FreedesktopSecretKeyProvider::kKWalletMethodOpenAsync,
+                   MatchArgs(kNetworkWallet, static_cast<int64_t>(0),
+                             kProductName, true),
+                   _))
+      .WillOnce(RespondWith(kTransactionId));
 
   // hasFolder -> false
   EXPECT_CALL(*mock_kwallet6_proxy,
@@ -752,6 +797,16 @@ TEST(FreedesktopSecretKeyProviderTest, KWalletCreateFolderAndPassword) {
         key = std::move(returned_key.value());
       }));
 
+  // Simulate walletAsyncOpened signal
+  ASSERT_TRUE(signal_callback);
+  auto signal = dbus::Signal(
+      FreedesktopSecretKeyProvider::kKWalletInterface,
+      FreedesktopSecretKeyProvider::kKWalletSignalWalletAsyncOpened);
+  dbus::MessageWriter writer(&signal);
+  dbus_utils::WriteValue(writer, kTransactionId);
+  dbus_utils::WriteValue(writer, kKWalletHandle);
+  signal_callback.Run(&signal);
+
   EXPECT_EQ(tag, "v11");
   EXPECT_TRUE(key.has_value());
 }