Clean Obsolete HTTP Data from the Password Store

chrome/browser/password_manager/password_manager_util_unittest.cc

Index: chrome/browser/password_manager/password_manager_util_unittest.cc
diff --git a/chrome/browser/password_manager/password_manager_util_unittest.cc b/chrome/browser/password_manager/password_manager_util_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..af2bf821de80851dc3872622ac7468fb702690de
--- /dev/null
+++ b/chrome/browser/password_manager/password_manager_util_unittest.cc
@@ -0,0 +1,306 @@
+// 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 "chrome/browser/password_manager/password_manager_util.h"
+
+#include <initializer_list>
+#include <memory>
+
+#include "base/memory/ptr_util.h"
+#include "base/run_loop.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "chrome/browser/password_manager/password_store_factory.h"
+#include "chrome/test/base/testing_profile.h"
+#include "components/password_manager/core/browser/mock_password_store.h"
+#include "components/password_manager/core/browser/password_manager_test_utils.h"
+#include "components/password_manager/core/common/password_manager_pref_names.h"
+#include "components/prefs/pref_service.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "net/http/transport_security_state.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+using autofill::PasswordForm;
+using password_manager::InteractionsStats;
+using testing::Invoke;
+using testing::Mock;
+using testing::NiceMock;
+using testing::_;
+
+namespace password_manager_util {
+
+namespace {
+
+constexpr char kTestHttpURL[] = "http://example.org/";
+constexpr char kTestHttpsURL[] = "https://example.org/";
+
+PasswordForm CreateTestHTTPForm() {
+  PasswordForm form;
+  form.origin = GURL(kTestHttpURL);
+  form.signon_realm = form.origin.spec();
+  form.action = form.origin;
+  form.username_value = base::ASCIIToUTF16("user");
+  form.password_value = base::ASCIIToUTF16("password");
+  return form;
+}
+
+PasswordForm CreateTestHTTPSForm() {
+  PasswordForm form;
+  form.origin = GURL(kTestHttpsURL);
+  form.signon_realm = form.origin.spec();
+  form.action = form.origin;
+  form.username_value = base::ASCIIToUTF16("user");
+  form.password_value = base::ASCIIToUTF16("password");
+  return form;
+}
+
+InteractionsStats CreateTestHTTPStats() {
+  InteractionsStats stats;
+  stats.origin_domain = GURL(kTestHttpURL);
+  stats.username_value = base::ASCIIToUTF16("user");
+  return stats;
+}
+
+InteractionsStats CreateTestHTTPSStats() {
+  InteractionsStats stats;
+  stats.origin_domain = GURL(kTestHttpsURL);
+  stats.username_value = base::ASCIIToUTF16("user");
+  return stats;
+}
+
+std::vector<std::unique_ptr<PasswordForm>> MakeResults(
+    const std::vector<PasswordForm>& forms) {
+  std::vector<std::unique_ptr<PasswordForm>> results;
+  results.reserve(forms.size());
+  for (const auto& form : forms)
+    results.push_back(base::MakeUnique<PasswordForm>(form));
+  return results;
+}
+
+// Auxiliary class to automatically set and reset the HSTS state for a given
+// host.
+class HSTSStateManager {
+ public:
+  HSTSStateManager(net::TransportSecurityState* state,
+                   bool is_hsts,
+                   const std::string& host);
+  ~HSTSStateManager();
+
+ private:
+  net::TransportSecurityState* state_;
+  const bool is_hsts_;
+  const std::string host_;
+};
+
+HSTSStateManager::HSTSStateManager(net::TransportSecurityState* state,
+                                   bool is_hsts,
+                                   const std::string& host)
+    : state_(state), is_hsts_(is_hsts), host_(host) {
+  if (is_hsts_) {
+    base::Time expiry = base::Time::Max();
+    bool include_subdomains = false;
+    state_->AddHSTS(host_, expiry, include_subdomains);
+  }
+}
+
+HSTSStateManager::~HSTSStateManager() {
+  if (is_hsts_)
+    state_->DeleteDynamicDataForHost(host_);
+}
+
+}  // namespace
+
+class PasswordManagerUtilTest : public testing::Test {
+ public:
+  PasswordManagerUtilTest();
+
+  TestingProfile& profile() { return profile_; }
+  password_manager::MockPasswordStore* store() { return store_; }
+  net::TransportSecurityState* GetTransportSecurityState();
+
+ private:
+  content::TestBrowserThreadBundle thread_bundle_;
+  TestingProfile profile_;
+  password_manager::MockPasswordStore* store_;
+};
+
+PasswordManagerUtilTest::PasswordManagerUtilTest() {
+  PasswordStoreFactory::GetInstance()->SetTestingFactory(
+      &profile_, password_manager::BuildPasswordStore<
+                     content::BrowserContext,
+                     NiceMock<password_manager::MockPasswordStore>>);
+
+  store_ = static_cast<password_manager::MockPasswordStore*>(
+      PasswordStoreFactory::GetForProfile(&profile_,
+                                          ServiceAccessType::EXPLICIT_ACCESS)

[vasilii, 2017/03/23 17:39:26]

Implicit access here

[jdoerrie, 2017/03/24 14:08:32]

Done.

+          .get());
+}
+
+net::TransportSecurityState*
+PasswordManagerUtilTest::GetTransportSecurityState() {
+  return profile()
+      .GetRequestContext()
+      ->GetURLRequestContext()
+      ->transport_security_state();
+}
+
+TEST_F(PasswordManagerUtilTest, TestPostHSTSQueryForHostAndProfile) {
+  const GURL test_origin(kTestHttpsURL);
+  for (bool is_hsts : {false, true}) {
+    SCOPED_TRACE(testing::Message()
+                 << std::boolalpha << "is_hsts: " << is_hsts);
+
+    HSTSStateManager manager(GetTransportSecurityState(), is_hsts,
+                             test_origin.host());
+    // Post query and ensure callback gets run.

[vasilii, 2017/03/23 17:39:26]

Would ba cool to verify that the callback was executed. The function which does
nothing will also pass the test currently.

[jdoerrie, 2017/03/24 14:08:31]

Done.

+    PostHSTSQueryForHostAndProfile(
+        test_origin, &profile(),
+        base::Bind([](bool expectation,
+                      bool result) { EXPECT_EQ(expectation, result); },
+                   is_hsts));
+    base::RunLoop().RunUntilIdle();
+  }
+}
+
+TEST_F(PasswordManagerUtilTest, TestBlacklistDeletion) {
+  for (bool is_http : {false, true}) {
+    for (bool is_blacklisted : {false, true}) {

[vasilii, 2017/03/23 17:39:26]

I didn't get the value of the test for is_blacklisted == false. Especially
confusing is the part where FillBlacklistLogins returns a non blacklisted form
as it's impossible.
I suggest you drop this loop.

[jdoerrie, 2017/03/24 14:08:31]

Done.

+      for (bool is_hsts : {false, true}) {
+        SCOPED_TRACE(testing::Message()
+                     << std::boolalpha
+                     << "(is_http, is_blacklisted, is_hsts): (" << is_http
+                     << ", " << is_blacklisted << ", " << is_hsts << ")");
+
+        const bool should_be_deleted = is_http && is_blacklisted && is_hsts;
+
+        PasswordForm form =
+            is_http ? CreateTestHTTPForm() : CreateTestHTTPSForm();
+        form.blacklisted_by_user = is_blacklisted;
+
+        HSTSStateManager manager(GetTransportSecurityState(), is_hsts,
+                                 form.origin.host());
+
+        EXPECT_CALL(*store(), FillBlacklistLogins(_))
+            .WillOnce(Invoke(
+                [&form](std::vector<std::unique_ptr<autofill::PasswordForm>>*
+                            forms) {
+                  *forms = MakeResults({form});
+                  return true;
+                }));
+
+        EXPECT_CALL(*store(), RemoveLogin(form)).Times(should_be_deleted);
+
+        // Initiate clean up and make sure all aync tasks are run until
+        // completion.
+        DelayCleanObsoleteHttpDataForProfile(&profile(), 0);
+        base::RunLoop().RunUntilIdle();
+
+        // Verify and clear all expectations as well as the preference.
+        Mock::VerifyAndClearExpectations(store());
+        profile().GetPrefs()->SetBoolean(
+            password_manager::prefs::kWasObsoleteHttpDataCleaned, false);

[vasilii, 2017/03/23 17:39:26]

Do you want to check the value of the pref? Same below.

[jdoerrie, 2017/03/24 14:08:31]

Done.

+      }
+    }
+  }
+}
+
+TEST_F(PasswordManagerUtilTest, TestAutofillableDeletion) {
+  for (bool is_hsts : {false, true}) {
+    for (bool same_host : {false, true}) {
+      for (bool same_user : {false, true}) {
+        for (bool same_pass : {false, true}) {
+          SCOPED_TRACE(testing::Message()
+                       << std::boolalpha
+                       << "(is_hsts, same_host, same_user, same_pass): ("
+                       << is_hsts << ", " << same_host << ", " << same_user
+                       << ", " << same_pass);
+
+          const bool should_be_deleted =
+              is_hsts && same_host && same_user && same_pass;
+
+          PasswordForm http_form = CreateTestHTTPForm();
+          PasswordForm https_form = CreateTestHTTPSForm();
+
+          if (!same_host) {
+            GURL::Replacements rep;
+            rep.SetHostStr("a-totally-different-host");
+            http_form.origin = http_form.origin.ReplaceComponents(rep);
+          }
+
+          if (!same_user) {
+            http_form.username_value =
+                https_form.username_value + base::ASCIIToUTF16("-different");

[vasilii, 2017/03/23 17:39:26]

Optional: simple assignment would be more readable and save a line.

[jdoerrie, 2017/03/24 14:08:31]

Done.

+          }
+
+          if (!same_pass) {
+            http_form.password_value =
+                https_form.password_value + base::ASCIIToUTF16("-different");
+          }
+
+          HSTSStateManager manager(GetTransportSecurityState(), is_hsts,
+                                   https_form.origin.host());
+
+          EXPECT_CALL(*store(), FillAutofillableLogins(_))
+              .WillOnce(Invoke(
+                  [&http_form, &https_form](
+                      std::vector<std::unique_ptr<autofill::PasswordForm>>*
+                          forms) {
+                    *forms = MakeResults({http_form, https_form});
+                    return true;
+                  }));
+
+          EXPECT_CALL(*store(), RemoveLogin(http_form))
+              .Times(should_be_deleted);
+
+          // Initiate clean up and make sure all aync tasks are run until
+          // completion.
+          DelayCleanObsoleteHttpDataForProfile(&profile(), 0);
+          base::RunLoop().RunUntilIdle();
+
+          // Verify and clear all expectations as well as the preference.
+          Mock::VerifyAndClearExpectations(store());
+          profile().GetPrefs()->SetBoolean(
+              password_manager::prefs::kWasObsoleteHttpDataCleaned, false);
+        }
+      }
+    }
+  }
+}
+
+TEST_F(PasswordManagerUtilTest, TestSiteStatsDeletion) {
+  for (bool is_http : {false, true}) {
+    for (bool is_hsts : {false, true}) {
+      SCOPED_TRACE(testing::Message()
+                   << std::boolalpha << "(is_http, is_hsts): (" << is_http
+                   << ", " << is_hsts);
+
+      const bool should_be_deleted = is_http && is_hsts;
+
+      InteractionsStats stats =
+          is_http ? CreateTestHTTPStats() : CreateTestHTTPSStats();
+
+      HSTSStateManager manager(GetTransportSecurityState(), is_hsts,
+                               stats.origin_domain.host());
+
+      EXPECT_CALL(*store(), GetAllSiteStatsImpl()).WillOnce(Invoke([&stats]() {
+        return std::vector<InteractionsStats>({stats});
+      }));
+      EXPECT_CALL(*store(), RemoveSiteStatsImpl(stats.origin_domain))
+          .Times(should_be_deleted);
+
+      // Initiate clean up and make sure all aync tasks are run until
+      // completion.
+      DelayCleanObsoleteHttpDataForProfile(&profile(), 0);
+      base::RunLoop().RunUntilIdle();
+
+      // Verify and clear all expectations as well as the preference.
+      Mock::VerifyAndClearExpectations(store());
+      profile().GetPrefs()->SetBoolean(
+          password_manager::prefs::kWasObsoleteHttpDataCleaned, false);
+    }
+  }
+}
+
+}  // namespace password_manager_util