Implement origin-based deletion for passwords in PasswordDefaultStore

components/password_manager/core/browser/password_store_default_unittest.cc

Index: components/password_manager/core/browser/password_store_default_unittest.cc
diff --git a/components/password_manager/core/browser/password_store_default_unittest.cc b/components/password_manager/core/browser/password_store_default_unittest.cc
index 8cdf4b93b5a8c96f4c0b9f9757d87f3d8aee657e..66db938927f64548d9fb77a63396599f026b1f7c 100644
--- a/components/password_manager/core/browser/password_store_default_unittest.cc
+++ b/components/password_manager/core/browser/password_store_default_unittest.cc
@@ -8,6 +8,7 @@
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/prefs/pref_service.h"
+#include "base/run_loop.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -19,10 +20,14 @@
 #include "components/password_manager/core/browser/password_store_default.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "url/origin.h"
 
 using autofill::PasswordForm;
+using testing::ElementsAre;
 using testing::ElementsAreArray;
 using testing::IsEmpty;
+using testing::SizeIs;

[vasilii, 2015/10/12 17:01:29]

unused

[Timo Reimann, 2015/10/12 22:08:06]

Thanks, another leftover.

cpplint doesn't seem to catch unused using statements. If there's a tool/option
to detect such issues, I'd be happy to know.

+using testing::_;
 
 namespace password_manager {
 
@@ -75,6 +80,23 @@ PasswordFormData CreateTestPasswordFormData() {
   return data;
 }
 
+PasswordFormData CreateTestPasswordFormDataByOrigin(
+    const std::string& origin_url) {

[vasilii, 2015/10/12 17:01:30]

I'm worried about the lifetime of origin_url. You save raw pointers to its
internals in PasswordFormData. If somebody calls
CreateTestPasswordFormDataByOrigin("hello") then the test will crash.
You need to pass const char*.

[Timo Reimann, 2015/10/12 22:08:06]

It didn't crash on me when I passed in a string literal; I guess I was in the
happy path of undefined behavior.

Done.

[vasilii, 2015/10/13 13:48:20]

Because your string was alive during the test.

+  PasswordFormData data = {PasswordForm::SCHEME_HTML,
+                           origin_url.c_str(),
+                           origin_url.c_str(),
+                           origin_url.c_str(),
+                           L"submit_element",
+                           L"username_element",
+                           L"password_element",
+                           L"username_value",
+                           L"password_value",
+                           true,
+                           false,
+                           base::Time::Now().ToDoubleT()};
+  return data;
+}
+
 }  // anonymous namespace
 
 class PasswordStoreDefaultTest : public testing::Test {
@@ -89,6 +111,16 @@ class PasswordStoreDefaultTest : public testing::Test {
     return temp_dir_.path().Append(FILE_PATH_LITERAL("login_test"));
   }
 
+  scoped_refptr<PasswordStoreDefault> CreateInitializedStore() {
+    LoginDatabase* database = new LoginDatabase(test_login_db_file_path());
+    scoped_refptr<PasswordStoreDefault> store(new PasswordStoreDefault(
+        base::ThreadTaskRunnerHandle::Get(),
+        base::ThreadTaskRunnerHandle::Get(), make_scoped_ptr(database)));

[vasilii, 2015/10/12 17:01:30]

Why not creating the database here?

[Timo Reimann, 2015/10/12 22:08:06]

Personally I found it harder to read due to the deep level of nested calls
involved (store initialized with PasswordStoreDefault initialized with
make_scoped_ptr initialized with a LoginDatabase instance initialized with a
file path).

I made the change nevertheless; if you prefer this approach I'd be happy to
stick to it.

+    store->Init(syncer::SyncableService::StartSyncFlare());
+
+    return store;
+  }
+
   base::MessageLoopForUI message_loop_;
   base::ScopedTempDir temp_dir_;
 };
@@ -138,7 +170,6 @@ TEST_F(PasswordStoreDefaultTest, NonASCIIData) {
   base::MessageLoop::current()->RunUntilIdle();
 
   store->Shutdown();
-  base::MessageLoop::current()->RunUntilIdle();
 }
 
 TEST_F(PasswordStoreDefaultTest, Notifications) {
@@ -191,7 +222,6 @@ TEST_F(PasswordStoreDefaultTest, Notifications) {
 
   store->RemoveObserver(&observer);
   store->Shutdown();
-  base::MessageLoop::current()->RunUntilIdle();
 }
 
 // Verify that operations on a PasswordStore with a bad database cause no
@@ -256,7 +286,84 @@ TEST_F(PasswordStoreDefaultTest, OperationsOnABadDatabaseSilentlyFail) {
   // Ensure no notifications and no explosions during shutdown either.
   bad_store->RemoveObserver(&mock_observer);
   bad_store->Shutdown();
+}
+
+TEST_F(PasswordStoreDefaultTest,
+       RemoveLoginsByOriginAndTimeImpl_FittingOriginAndTime) {
+  scoped_refptr<PasswordStoreDefault> store = CreateInitializedStore();
+
+  const std::string origin_url = "http://foo.example.com";

[vasilii, 2015/10/12 17:01:30]

const char[]

[Timo Reimann, 2015/10/12 22:08:06]

Done for all origin_url instances across all new tests.

+  scoped_ptr<autofill::PasswordForm> form =
+      CreatePasswordFormFromDataForTesting(
+          CreateTestPasswordFormDataByOrigin(origin_url));
+  store->AddLogin(*form);
   base::MessageLoop::current()->RunUntilIdle();
+
+  MockPasswordStoreObserver observer;
+  store->AddObserver(&observer);
+  EXPECT_CALL(observer, OnLoginsChanged(ElementsAre(PasswordStoreChange(
+                            PasswordStoreChange::REMOVE, *form))));
+
+  const url::Origin origin((GURL(origin_url)));
+  base::RunLoop run_loop;
+  store->RemoveLoginsByOriginAndTime(origin, base::Time::Time(),
+                                     base::Time::Max(), run_loop.QuitClosure());
+  base::MessageLoop::current()->RunUntilIdle();

[vasilii, 2015/10/12 17:01:30]

Nope. RunLoop::Run should be called here. That's how you test that the callback
is actually called.
Same below.

[Timo Reimann, 2015/10/12 22:08:06]

Gotcha. Done again for this and all other new tests.

+
+  store->RemoveObserver(&observer);
+  store->Shutdown();
+}
+
+TEST_F(PasswordStoreDefaultTest,
+       RemoveLoginsByOriginAndTimeImpl_NonMatchingOrigin) {
+  scoped_refptr<PasswordStoreDefault> store = CreateInitializedStore();
+
+  const std::string origin_url = "http://foo.example.com";
+  scoped_ptr<autofill::PasswordForm> form =
+      CreatePasswordFormFromDataForTesting(
+          CreateTestPasswordFormDataByOrigin(origin_url));
+  store->AddLogin(*form);
+  base::MessageLoop::current()->RunUntilIdle();
+
+  MockPasswordStoreObserver observer;
+  store->AddObserver(&observer);
+  EXPECT_CALL(observer, OnLoginsChanged(_)).Times(0);
+
+  const url::Origin other_origin(GURL("http://bar.example.com"));
+  base::RunLoop run_loop;
+  store->RemoveLoginsByOriginAndTime(other_origin, base::Time::Time(),
+                                     base::Time::Max(), run_loop.QuitClosure());
+  base::MessageLoop::current()->RunUntilIdle();
+
+  store->RemoveObserver(&observer);
+  store->Shutdown();
+}
+
+TEST_F(PasswordStoreDefaultTest,
+       RemoveLoginsByOriginAndTimeImpl_NotWithinTimeInterval) {
+  scoped_refptr<PasswordStoreDefault> store = CreateInitializedStore();
+
+  const std::string origin_url = "http://foo.example.com";
+  scoped_ptr<autofill::PasswordForm> form =
+      CreatePasswordFormFromDataForTesting(
+          CreateTestPasswordFormDataByOrigin(origin_url));
+  store->AddLogin(*form);
+  base::MessageLoop::current()->RunUntilIdle();
+
+  MockPasswordStoreObserver observer;
+  store->AddObserver(&observer);
+  EXPECT_CALL(observer, OnLoginsChanged(_)).Times(0);
+
+  const url::Origin origin((GURL(origin_url)));
+  base::Time time_after_creation_date =
+      form->date_created + base::TimeDelta::FromDays(1);
+  base::RunLoop run_loop;
+  store->RemoveLoginsByOriginAndTime(origin, time_after_creation_date,
+                                     base::Time::Max(), run_loop.QuitClosure());
+  base::MessageLoop::current()->RunUntilIdle();
+
+  store->RemoveObserver(&observer);
+  store->Shutdown();
 }
 
 }  // namespace password_manager