Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(9237)

Unified Diff: components/password_manager/core/browser/form_fetcher_impl_unittest.cc

Issue 2878463003: Introduce SuppressedHTTPSFormFetcher. (Closed)
Patch Set: Polish, test for simultaneous behavior. Created 3 years, 7 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
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
+ // 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();

Powered by Google App Engine
This is Rietveld 408576698