Introduce SuppressedHTTPSFormFetcher.

components/password_manager/core/browser/form_fetcher_impl_unittest.cc

Index: components/password_manager/core/browser/form_fetcher_impl_unittest.cc
diff --git a/components/password_manager/core/browser/form_fetcher_impl_unittest.cc b/components/password_manager/core/browser/form_fetcher_impl_unittest.cc
index 90c36a90dbf9ad13e8a29391c28639b217517182..83a0290900cc96f8a41100496a2911fad61a2518 100644
--- a/components/password_manager/core/browser/form_fetcher_impl_unittest.cc
+++ b/components/password_manager/core/browser/form_fetcher_impl_unittest.cc
@@ -44,6 +44,12 @@ namespace password_manager {
 
 namespace {
 
+const char kTestHttpRealm[] = "http://accounts.google.com/";
+const char kTestHttpURL[] = "http://accounts.google.com/a/LoginAuth";
+
+const char kTestHttpsRealm[] = "https://accounts.google.com/";
+const char kTestHttpsURL[] = "https://accounts.google.com/a/LoginAuth";
+
 class MockConsumer : public FormFetcher::Consumer {
  public:
   MOCK_METHOD2(ProcessMatches,
@@ -128,7 +134,7 @@ PasswordForm CreateHTTPNonFederated() {
 PasswordForm CreateFederated() {
   PasswordForm form = CreateNonFederated();
   form.password_value.clear();
-  form.federation_origin = url::Origin(GURL("https://accounts.google.com/"));
+  form.federation_origin = url::Origin(GURL(kTestHttpsRealm));
   return form;
 }
 
@@ -162,13 +168,14 @@ class FormFetcherImplTest : public testing::Test {
  public:
   FormFetcherImplTest()
       : form_digest_(PasswordForm::SCHEME_HTML,
-                     "http://accounts.google.com",
-                     GURL("http://accounts.google.com/a/LoginAuth")) {
+                     kTestHttpRealm,
+                     GURL(kTestHttpURL)) {
     mock_store_ = new MockPasswordStore();
     client_.set_store(mock_store_.get());
 
     form_fetcher_ = base::MakeUnique<FormFetcherImpl>(
-        form_digest_, &client_, /* should_migrate_http_passwords */ false);
+        form_digest_, &client_, false /* should_migrate_http_passwords */,
+        false /* should_query_suppressed_https_forms */);
   }
 
   ~FormFetcherImplTest() override { mock_store_->ShutdownOnUIThread(); }
@@ -399,7 +406,8 @@ TEST_F(FormFetcherImplTest, DoNotTryToMigrateHTTPPasswordsOnHTTPSites) {
   // A new form fetcher is created to be able to set the form digest and
   // migration flag.
   form_fetcher_ = base::MakeUnique<FormFetcherImpl>(
-      form_digest_, &client_, /* should_migrate_http_passwords */ true);
+      form_digest_, &client_, true /* should_migrate_http_passwords */,
+      false /* should_query_suppressed_https_forms */);
   EXPECT_CALL(consumer_, ProcessMatches(IsEmpty(), 0u));
   form_fetcher_->AddConsumer(&consumer_);
 
@@ -441,7 +449,8 @@ TEST_F(FormFetcherImplTest, TryToMigrateHTTPPasswordsOnHTTPSSites) {
   // A new form fetcher is created to be able to set the form digest and
   // migration flag.
   form_fetcher_ = base::MakeUnique<FormFetcherImpl>(
-      form_digest_, &client_, /* should_migrate_http_passwords */ true);
+      form_digest_, &client_, true /* should_migrate_http_passwords */,
+      false /* should_query_suppressed_https_forms */);
   EXPECT_CALL(consumer_, ProcessMatches(IsEmpty(), 0u));
   form_fetcher_->AddConsumer(&consumer_);
 
@@ -510,7 +519,8 @@ TEST_F(FormFetcherImplTest, StateIsWaitingDuringMigration) {
   // A new form fetcher is created to be able to set the form digest and
   // migration flag.
   form_fetcher_ = base::MakeUnique<FormFetcherImpl>(
-      form_digest_, &client_, /* should_migrate_http_passwords */ true);
+      form_digest_, &client_, true /* should_migrate_http_passwords */,
+      false /* should_query_suppressed_https_forms */);
 
   PasswordForm https_form = CreateNonFederated();
 
@@ -551,6 +561,181 @@ TEST_F(FormFetcherImplTest, StateIsWaitingDuringMigration) {
   EXPECT_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_->GetState());
 }
 
+TEST_F(FormFetcherImplTest, SuppressedHTTPSForms_QueriedForHTTPOrigins) {
+  form_fetcher_ = base::MakeUnique<FormFetcherImpl>(
+      form_digest_, &client_, false /* should_migrate_http_passwords */,
+      true /* should_query_suppressed_https_forms */);
+  EXPECT_CALL(consumer_, ProcessMatches(IsEmpty(), 0u));
+  form_fetcher_->AddConsumer(&consumer_);
+  testing::Mock::VerifyAndClearExpectations(&consumer_);
+
+  Fetch();
+
+  // The matching PasswordStore results coming in should trigger another
+  // GetLogins request to fetcht the suppressed HTTPS forms.
+
+  const PasswordStore::FormDigest https_version_of_form_digest(
+      PasswordForm::SCHEME_HTML, kTestHttpsRealm, GURL(kTestHttpsURL));
+  PasswordStoreConsumer* https_form_fetcher_ptr = nullptr;
+  EXPECT_CALL(*mock_store_, GetLogins(https_version_of_form_digest, _))
+      .WillOnce(::testing::SaveArg<1>(&https_form_fetcher_ptr));
+  EXPECT_CALL(consumer_, ProcessMatches(IsEmpty(), 0u));
+
+  form_fetcher_->OnGetPasswordStoreResults(
+      MakeResults(std::vector<PasswordForm>()));
+
+  testing::Mock::VerifyAndClearExpectations(&consumer_);
+  testing::Mock::VerifyAndClearExpectations(mock_store_.get());
+  EXPECT_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_->GetState());
+  EXPECT_FALSE(form_fetcher_->DidCompleteQueryingSuppressedHTTPSForms());
+  ASSERT_TRUE(https_form_fetcher_ptr);
+
+  https_form_fetcher_ptr->OnGetPasswordStoreResults(
+      MakeResults(std::vector<PasswordForm>()));
+
+  EXPECT_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_->GetState());
+  EXPECT_TRUE(form_fetcher_->DidCompleteQueryingSuppressedHTTPSForms());
+  EXPECT_THAT(form_fetcher_->GetSuppressedHTTPSForms(), IsEmpty());
+

[kolos1, 2017/05/22 12:35:07]

Could we split this test into 3 test?

[engedy, 2017/05/22 20:20:19]

Done. I also introduced two helper methods to cut down the redundancy here.

+  // Another call to Fetch() should refetch the list of suppressed HTTPS
+  // credentials too. It should not matter that there are matching HTTP forms.
+
+  Fetch();
+
+  const PasswordForm matching_http_form = CreateHTTPNonFederated();
+  const PasswordForm suppressed_https_form1 = CreateNonFederated();
+  const PasswordForm suppressed_https_form2 = CreateFederated();
+  https_form_fetcher_ptr = nullptr;
+  EXPECT_CALL(*mock_store_, GetLogins(https_version_of_form_digest, _))
+      .WillOnce(::testing::SaveArg<1>(&https_form_fetcher_ptr));
+  EXPECT_CALL(
+      consumer_,
+      ProcessMatches(UnorderedElementsAre(Pointee(matching_http_form)), 0u));
+
+  form_fetcher_->OnGetPasswordStoreResults(MakeResults({matching_http_form}));
+
+  testing::Mock::VerifyAndClearExpectations(&consumer_);
+  testing::Mock::VerifyAndClearExpectations(mock_store_.get());
+  EXPECT_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_->GetState());
+  EXPECT_TRUE(form_fetcher_->DidCompleteQueryingSuppressedHTTPSForms());
+  ASSERT_TRUE(https_form_fetcher_ptr);
+
+  https_form_fetcher_ptr->OnGetPasswordStoreResults(
+      MakeResults({suppressed_https_form1, suppressed_https_form2}));
+
+  EXPECT_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_->GetState());
+  EXPECT_TRUE(form_fetcher_->DidCompleteQueryingSuppressedHTTPSForms());
+  EXPECT_THAT(form_fetcher_->GetSuppressedHTTPSForms(),
+              UnorderedElementsAre(Pointee(suppressed_https_form1),
+                                   Pointee(suppressed_https_form2)));
+
+  // Ensure that calling Fetch() does not wipe (even temporarily) the previously
+  // fetched list of suppressed HTTPS credentials. Stale is better than nothing.
+
+  Fetch();
+
+  EXPECT_TRUE(form_fetcher_->DidCompleteQueryingSuppressedHTTPSForms());
+  EXPECT_EQ(FormFetcher::State::WAITING, form_fetcher_->GetState());
+  EXPECT_THAT(form_fetcher_->GetSuppressedHTTPSForms(),
+              UnorderedElementsAre(Pointee(suppressed_https_form1),
+                                   Pointee(suppressed_https_form2)));
+
+  EXPECT_CALL(*mock_store_, GetLogins(https_version_of_form_digest, _));
+  EXPECT_CALL(consumer_, ProcessMatches(IsEmpty(), 0u));
+
+  form_fetcher_->OnGetPasswordStoreResults(
+      MakeResults(std::vector<PasswordForm>()));
+
+  EXPECT_TRUE(form_fetcher_->DidCompleteQueryingSuppressedHTTPSForms());
+  EXPECT_THAT(form_fetcher_->GetSuppressedHTTPSForms(),
+              UnorderedElementsAre(Pointee(suppressed_https_form1),
+                                   Pointee(suppressed_https_form2)));
+
+  testing::Mock::VerifyAndClearExpectations(&consumer_);
+  testing::Mock::VerifyAndClearExpectations(mock_store_.get());
+
+  // Destroy FormFetcher while SuppressedHTTPSFormFetcher is busy.
+  form_fetcher_.reset();
+}
+
+// Exercise the scenario where querying the suppressed HTTPS logins takes so
+// long that in the meantime there is another call to Fetch(), which completes,
+// and triggers fetching HTTPS logins yet again. In this case, the first
+// SuppressedHTTPSFormFetcher is destroyed and its query cancelled.
+TEST_F(FormFetcherImplTest, SuppressedHTTPSForms_SimultaneousQueries) {
+  form_fetcher_ = base::MakeUnique<FormFetcherImpl>(
+      form_digest_, &client_, false /* should_migrate_http_passwords */,
+      true /* should_query_suppressed_https_forms */);
+  EXPECT_CALL(consumer_, ProcessMatches(IsEmpty(), 0u));
+  form_fetcher_->AddConsumer(&consumer_);
+  testing::Mock::VerifyAndClearExpectations(&consumer_);
+
+  Fetch();
+
+  const PasswordStore::FormDigest https_version_of_form_digest(
+      PasswordForm::SCHEME_HTML, kTestHttpsRealm, GURL(kTestHttpsURL));
+  base::WeakPtr<PasswordStoreConsumer> https_form_fetcher_ptr1;
+  EXPECT_CALL(*mock_store_, GetLogins(https_version_of_form_digest, _))
+      .WillOnce(::WithArg<1>(GetAndAssignWeakPtr(&https_form_fetcher_ptr1)));
+  EXPECT_CALL(consumer_, ProcessMatches(IsEmpty(), 0u));
+
+  form_fetcher_->OnGetPasswordStoreResults(
+      MakeResults(std::vector<PasswordForm>()));
+
+  testing::Mock::VerifyAndClearExpectations(&consumer_);
+  testing::Mock::VerifyAndClearExpectations(mock_store_.get());
+  EXPECT_TRUE(https_form_fetcher_ptr1);
+
+  Fetch();
+
+  PasswordStoreConsumer* https_form_fetcher_ptr2 = nullptr;
+  EXPECT_CALL(*mock_store_, GetLogins(https_version_of_form_digest, _))
+      .WillOnce(::testing::SaveArg<1>(&https_form_fetcher_ptr2));
+  EXPECT_CALL(consumer_, ProcessMatches(IsEmpty(), 0u));
+
+  form_fetcher_->OnGetPasswordStoreResults(
+      MakeResults(std::vector<PasswordForm>()));
+
+  testing::Mock::VerifyAndClearExpectations(&consumer_);
+  testing::Mock::VerifyAndClearExpectations(mock_store_.get());
+  EXPECT_FALSE(https_form_fetcher_ptr1);
+  ASSERT_TRUE(https_form_fetcher_ptr2);
+  EXPECT_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_->GetState());
+
+  EXPECT_FALSE(form_fetcher_->DidCompleteQueryingSuppressedHTTPSForms());
+  EXPECT_THAT(form_fetcher_->GetSuppressedHTTPSForms(), IsEmpty());
+
+  const PasswordForm suppressed_https_form = CreateNonFederated();
+  https_form_fetcher_ptr2->OnGetPasswordStoreResults(
+      MakeResults({suppressed_https_form}));
+
+  EXPECT_TRUE(form_fetcher_->DidCompleteQueryingSuppressedHTTPSForms());
+  EXPECT_THAT(form_fetcher_->GetSuppressedHTTPSForms(),
+              UnorderedElementsAre(Pointee(suppressed_https_form)));
+}
+
+TEST_F(FormFetcherImplTest, SuppressedHTTPSForms_NotQueriedForHTTPSOrigins) {
+  form_digest_ = PasswordStore::FormDigest(
+      PasswordForm::SCHEME_HTML, kTestHttpsRealm, GURL(kTestHttpsURL));
+  form_fetcher_ = base::MakeUnique<FormFetcherImpl>(
+      form_digest_, &client_, false /* should_migrate_http_passwords */,
+      true /* should_query_suppressed_https_forms */);
+  EXPECT_CALL(consumer_, ProcessMatches(IsEmpty(), 0u));
+  form_fetcher_->AddConsumer(&consumer_);
+  testing::Mock::VerifyAndClearExpectations(&consumer_);
+
+  Fetch();
+
+  EXPECT_CALL(*mock_store_, GetLogins(_, _)).Times(0);
+  EXPECT_CALL(consumer_, ProcessMatches(IsEmpty(), 0u));
+
+  form_fetcher_->OnGetPasswordStoreResults(
+      MakeResults(std::vector<PasswordForm>()));
+
+  EXPECT_EQ(FormFetcher::State::NOT_WAITING, form_fetcher_->GetState());
+  EXPECT_FALSE(form_fetcher_->DidCompleteQueryingSuppressedHTTPSForms());
+}
+
 // Cloning a FormFetcherImpl with empty results should result in an
 // instance with empty results.
 TEST_F(FormFetcherImplTest, Clone_EmptyResults) {
@@ -564,6 +749,7 @@ TEST_F(FormFetcherImplTest, Clone_EmptyResults) {
   EXPECT_EQ(FormFetcher::State::NOT_WAITING, clone->GetState());
   EXPECT_THAT(clone->GetInteractionsStats(), IsEmpty());
   EXPECT_THAT(clone->GetFederatedMatches(), IsEmpty());
+  EXPECT_THAT(clone->GetSuppressedHTTPSForms(), IsEmpty());
   MockConsumer consumer;
   EXPECT_CALL(consumer, ProcessMatches(IsEmpty(), 0u));
   clone->AddConsumer(&consumer);
@@ -615,6 +801,19 @@ TEST_F(FormFetcherImplTest, Clone_Stats) {
   EXPECT_EQ(1u, clone->GetInteractionsStats().size());
 }
 
+// Cloning a FormFetcherImpl with some suppressed HTTPS credentials should
+// result in an instance with the same suppressed credentials.
+TEST_F(FormFetcherImplTest, Clone_SuppressedHTTPSCredentials) {
+  Fetch();
+  form_fetcher_->OnGetPasswordStoreResults(
+      std::vector<std::unique_ptr<PasswordForm>>());
+  form_fetcher_->ProcessSuppressedHTTPSForms(
+      MakeResults({CreateNonFederated()}));
+
+  auto clone = form_fetcher_->Clone();
+  EXPECT_EQ(1u, clone->GetSuppressedHTTPSForms().size());
+}
+
 // Check that removing consumers stops them from receiving store updates.
 TEST_F(FormFetcherImplTest, RemoveConsumer) {
   Fetch();