From e0f868e9bb4f5ca574529d5fbc550381f6ec9db9 Mon Sep 17 00:00:00 2001
From: vabr <vabr@chromium.org>
Date: Wed, 21 Jan 2015 06:10:24 -0800
Subject: [PATCH] NativeBackendKWallet::GetAllLogins should not ignore errors

Currently, if KWallet failes to respond during credential retrieval, NativeBackendKWallet::GetAllLogins just silently ignores that, unlike NativeBackendKWallet::GetLoginsList, and unlike GetAllLogins itself during presence checking.

This CL makes GetAllLogins return false whenever KWallet fails to respond, to match the rest of the NativeBackendKWallet code.

BUG=324291

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

Cr-Commit-Position: refs/heads/master@{#312344}
---
 .../password_manager/native_backend_kwallet_x.cc   |  4 +--
 .../native_backend_kwallet_x_unittest.cc           | 37 ++++++++++++++++++++++
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/chrome/browser/password_manager/native_backend_kwallet_x.cc b/chrome/browser/password_manager/native_backend_kwallet_x.cc
index 0c73602f0844..5244c6a604c9 100644
--- a/chrome/browser/password_manager/native_backend_kwallet_x.cc
+++ b/chrome/browser/password_manager/native_backend_kwallet_x.cc
@@ -522,7 +522,7 @@ bool NativeBackendKWallet::GetAllLogins(PasswordFormList* forms,
             &method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT));
     if (!response.get()) {
       LOG(ERROR) << "Error contacting kwalletd (readEntry)";
-      continue;
+      return false;
     }
     dbus::MessageReader reader(response.get());
     const uint8_t* bytes = NULL;
@@ -530,7 +530,7 @@ bool NativeBackendKWallet::GetAllLogins(PasswordFormList* forms,
     if (!reader.PopArrayOfBytes(&bytes, &length)) {
       LOG(ERROR) << "Error reading response from kwalletd (readEntry): "
                  << response->ToString();
-      continue;
+      return false;
     }
     if (!bytes || !CheckSerializedValue(bytes, length, signon_realm))
       continue;
diff --git a/chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc b/chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc
index 5960c132c9e0..4b2fd0837205 100644
--- a/chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc
+++ b/chrome/browser/password_manager/native_backend_kwallet_x_unittest.cc
@@ -4,6 +4,7 @@
 
 #include <algorithm>
 #include <map>
+#include <set>
 #include <string>
 #include <vector>
 
@@ -119,6 +120,14 @@ class TestKWallet {
   // No need to disallow copy and assign. This class is safe to copy and assign.
 };
 
+// Runs |backend->GetAutofillableLogins(forms)| and expects that the return
+// value is false.
+void CheckGetAutofillableLoginsFails(
+    PasswordStoreX::NativeBackend* backend,
+    PasswordStoreX::NativeBackend::PasswordFormList* forms) {
+  EXPECT_FALSE(backend->GetAutofillableLogins(forms));
+}
+
 }  // anonymous namespace
 
 // Obscure magic: we need to declare storage for this constant because we use it
@@ -289,6 +298,10 @@ class NativeBackendKWalletTest : public NativeBackendKWalletTestBase {
 
   TestKWallet wallet_;
 
+  // For all method names contained in |failing_methods_|, the mocked KWallet
+  // will return a null response.
+  std::set<std::string> failing_methods_;
+
  private:
   dbus::Response* KLauncherMethodCall(
       dbus::MethodCall* method_call, testing::Unused);
@@ -460,6 +473,8 @@ dbus::Response* NativeBackendKWalletTest::KWalletMethodCall(
     return NULL;
   EXPECT_EQ("org.kde.KWallet", method_call->GetInterface());
 
+  if (ContainsKey(failing_methods_, method_call->GetMember()))
+    return nullptr;
   scoped_ptr<dbus::Response> response;
   if (method_call->GetMember() == "isEnabled") {
     response = dbus::Response::CreateEmpty();
@@ -1047,6 +1062,28 @@ void NativeBackendKWalletPickleTest::CheckVersion0Pickle(
   STLDeleteElements(&form_list);
 }
 
+// Check that if KWallet fails to respond, the backend propagates the error.
+TEST_F(NativeBackendKWalletTest, GetAllLoginsErrorHandling) {
+  NativeBackendKWalletStub backend(42);
+  EXPECT_TRUE(backend.InitWithBus(mock_session_bus_));
+  // Make KWallet fail on calling readEntry.
+  failing_methods_.insert("readEntry");
+
+  // Store some non-blacklisted logins to be potentially returned.
+  BrowserThread::PostTask(
+      BrowserThread::DB, FROM_HERE,
+      base::Bind(base::IgnoreResult(&NativeBackendKWalletStub::AddLogin),
+                 base::Unretained(&backend), form_google_));
+
+  // Verify that nothing is in fact returned, because KWallet fails to respond.
+  std::vector<PasswordForm*> form_list;
+  BrowserThread::PostTask(BrowserThread::DB, FROM_HERE,
+                          base::Bind(&CheckGetAutofillableLoginsFails,
+                                     base::Unretained(&backend), &form_list));
+  RunDBThread();
+  EXPECT_EQ(0u, form_list.size());
+}
+
 // We try both SCHEME_HTML and SCHEME_BASIC since the scheme is stored right
 // after the size in the pickle, so it's what gets read as part of the count
 // when reading 32-bit pickles on 64-bit systems. SCHEME_HTML is 0 (so we'll
-- 
2.11.4.GIT