Changes in HttpCredentialCleaner

This CL introduces a few improvements to HttpCredentialCleaner:
(1) A fix in removing the protocol from signon_realm,
(2) refactoring the unittest to use parameters,
(3) minor style fixes.

More details about (1):
HttpCredentialCleaner removes the protocol (HTTP or HTTPS) form
the signon_realm in order to compare the signon_realm of HTTP
credentials with the signon_realm of HTTPS credentials. Until now,
a GURL was created from the signon_realm of the form and then the
protocol was extracted from that GURL, resulting the signon_realm
excluding protocol. This can cause problems when the auth realm contains
characters that are forbidden in an url. This will lead in creating an
invalid url, and the resulting signon_realm with protocol exluded will
be an empty string.
This CL will avoid conversion from the signon_realm string to the GURL
and use other way to remove the protocol from the signon_realm.

More details about (2):
Unitests for this class were changed from one single test into a
bunch of parametrised tests in order to make debug easier in case
of failing test in the future.

Bug: 871140
Change-Id: Ic606d250f50806ae3a3fa07a480fcb01f5551c97
Reviewed-on: https://chromium-review.googlesource.com/1249104
Commit-Queue: Narcis Gemene <gemene@google.com>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594826}

components/password_manager/core/browser/http_credentials_cleaner.cc

@@ -5,7 +5,6 @@
 #include "components/password_manager/core/browser/http_credentials_cleaner.h"
 
 #include "base/metrics/histogram_functions.h"
-#include "base/strings/string_piece.h"
 #include "components/password_manager/core/browser/password_manager_util.h"
 #include "url/gurl.h"
 
@@ -30,11 +29,10 @@ void HttpCredentialCleaner::OnGetPasswordStoreResults(
   });
 
   for (auto& form : results) {
-    // The next signon-realm has the protocol excluded. For example if original
-    // signon_realm is "https://google.com/". After excluding protocol it
-    // becomes "google.com/".
-    FormKey form_key({GURL(form->signon_realm).GetContent(), form->scheme,
-                      form->username_value});
+    FormKey form_key(
+        {std::string(
+             password_manager_util::GetSignonRealmWithProtocolExcluded(*form)),
+         form->scheme, form->username_value});
     if (form->origin.SchemeIs(url::kHttpScheme)) {
       PostHSTSQueryForHostAndNetworkContext(
           form->origin, network_context_getter_.Run(),

components/password_manager/core/browser/http_credentials_cleaner_unittest.cc

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "components/password_manager/core/browser/http_credentials_cleaner.h"
+
 #include "base/macros.h"
 #include "base/stl_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ -17,77 +19,171 @@
 
 namespace password_manager {
 
-TEST(HttpCredentialCleaner, ReportHttpMigrationMetrics) {
-  enum class HttpCredentialType { kNoMatching, kEquivalent, kConflicting };
-
-  struct TestCase {
-    bool is_hsts_enabled;
-    autofill::PasswordForm::Scheme http_form_scheme;
-    bool same_signon_realm;
-    bool same_scheme;
-    bool same_username;
-    bool same_password;
-    HttpCredentialType expected;
-  };
-
+namespace {
+
+enum class HttpCredentialType { kNoMatching, kEquivalent, kConflicting };
+
+struct TestCase {
+  bool is_hsts_enabled;
+  autofill::PasswordForm::Scheme http_form_scheme;
+  bool same_signon_realm;
+  bool same_scheme;
+  bool same_username;
+  bool same_password;
+  // |expected| == kNoMatching if:
+  // same_signon_realm & same_scheme & same_username = false
+  // Otherwise:
+  // |expected| == kEquivalent if:
+  // same_signon_realm & same_scheme & same_username & same_password = true
+  // Otherwise:
+  // |expected| == kConflicting if:
+  // same_signon_realm & same_scheme & same_username = true but same_password =
+  // false
+  HttpCredentialType expected;
+};
+
+constexpr static TestCase kCases[] = {
+
+    {true, autofill::PasswordForm::Scheme::SCHEME_HTML, false, true, true, true,
+     HttpCredentialType::kNoMatching},
+    {true, autofill::PasswordForm::Scheme::SCHEME_HTML, true, false, true, true,
+     HttpCredentialType::kNoMatching},
+    {true, autofill::PasswordForm::Scheme::SCHEME_HTML, true, true, false, true,
+     HttpCredentialType::kNoMatching},
+    {true, autofill::PasswordForm::Scheme::SCHEME_HTML, true, true, true, false,
+     HttpCredentialType::kConflicting},
+    {true, autofill::PasswordForm::Scheme::SCHEME_HTML, true, true, true, true,
+     HttpCredentialType::kEquivalent},
+
+    {false, autofill::PasswordForm::Scheme::SCHEME_HTML, false, true, true,
+     true, HttpCredentialType::kNoMatching},
+    {false, autofill::PasswordForm::Scheme::SCHEME_HTML, true, false, true,
+     true, HttpCredentialType::kNoMatching},
+    {false, autofill::PasswordForm::Scheme::SCHEME_HTML, true, true, false,
+     true, HttpCredentialType::kNoMatching},
+    {false, autofill::PasswordForm::Scheme::SCHEME_HTML, true, true, true,
+     false, HttpCredentialType::kConflicting},
+    {false, autofill::PasswordForm::Scheme::SCHEME_HTML, true, true, true, true,
+     HttpCredentialType::kEquivalent},
+
+    {true, autofill::PasswordForm::Scheme::SCHEME_BASIC, false, true, true,
+     true, HttpCredentialType::kNoMatching},
+    {true, autofill::PasswordForm::Scheme::SCHEME_BASIC, true, false, true,
+     true, HttpCredentialType::kNoMatching},
+    {true, autofill::PasswordForm::Scheme::SCHEME_BASIC, true, true, false,
+     true, HttpCredentialType::kNoMatching},
+    {true, autofill::PasswordForm::Scheme::SCHEME_BASIC, true, true, true,
+     false, HttpCredentialType::kConflicting},
+    {true, autofill::PasswordForm::Scheme::SCHEME_BASIC, true, true, true, true,
+     HttpCredentialType::kEquivalent},
+
+    {false, autofill::PasswordForm::Scheme::SCHEME_BASIC, false, true, true,
+     true, HttpCredentialType::kNoMatching},
+    {false, autofill::PasswordForm::Scheme::SCHEME_BASIC, true, false, true,
+     true, HttpCredentialType::kNoMatching},
+    {false, autofill::PasswordForm::Scheme::SCHEME_BASIC, true, true, false,
+     true, HttpCredentialType::kNoMatching},
+    {false, autofill::PasswordForm::Scheme::SCHEME_BASIC, true, true, true,
+     false, HttpCredentialType::kConflicting},
+    {false, autofill::PasswordForm::Scheme::SCHEME_BASIC, true, true, true,
+     true, HttpCredentialType::kEquivalent}};
+
+}  // namespace
+
+class HttpCredentialCleanerTest : public ::testing::TestWithParam<TestCase> {
+ public:
+  HttpCredentialCleanerTest() = default;
+
+  ~HttpCredentialCleanerTest() override = default;
+
+ protected:
+  scoped_refptr<TestPasswordStore> store_ =
+      base::MakeRefCounted<TestPasswordStore>();
+
+  DISALLOW_COPY_AND_ASSIGN(HttpCredentialCleanerTest);
+};
+
+TEST_P(HttpCredentialCleanerTest, ReportHttpMigrationMetrics) {
   struct Histogram {
     bool test_hsts_enabled;
     HttpCredentialType test_type;
     std::string histogram_name;
   };
 
-  constexpr static TestCase cases[] = {
-
-      {true, autofill::PasswordForm::Scheme::SCHEME_HTML, false, true, true,
-       true, HttpCredentialType::kNoMatching},
-      {true, autofill::PasswordForm::Scheme::SCHEME_HTML, true, false, true,
-       true, HttpCredentialType::kNoMatching},
-      {true, autofill::PasswordForm::Scheme::SCHEME_HTML, true, true, false,
-       true, HttpCredentialType::kNoMatching},
-      {true, autofill::PasswordForm::Scheme::SCHEME_HTML, true, true, true,
-       false, HttpCredentialType::kConflicting},
-      {true, autofill::PasswordForm::Scheme::SCHEME_HTML, true, true, true,
-       true, HttpCredentialType::kEquivalent},
-
-      {false, autofill::PasswordForm::Scheme::SCHEME_HTML, false, true, true,
-       true, HttpCredentialType::kNoMatching},
-      {false, autofill::PasswordForm::Scheme::SCHEME_HTML, true, false, true,
-       true, HttpCredentialType::kNoMatching},
-      {false, autofill::PasswordForm::Scheme::SCHEME_HTML, true, true, false,
-       true, HttpCredentialType::kNoMatching},
-      {false, autofill::PasswordForm::Scheme::SCHEME_HTML, true, true, true,
-       false, HttpCredentialType::kConflicting},
-      {false, autofill::PasswordForm::Scheme::SCHEME_HTML, true, true, true,
-       true, HttpCredentialType::kEquivalent},
-
-      {true, autofill::PasswordForm::Scheme::SCHEME_BASIC, false, true, true,
-       true, HttpCredentialType::kNoMatching},
-      {true, autofill::PasswordForm::Scheme::SCHEME_BASIC, true, false, true,
-       true, HttpCredentialType::kNoMatching},
-      {true, autofill::PasswordForm::Scheme::SCHEME_BASIC, true, true, false,
-       true, HttpCredentialType::kNoMatching},
-      {true, autofill::PasswordForm::Scheme::SCHEME_BASIC, true, true, true,
-       false, HttpCredentialType::kConflicting},
-      {true, autofill::PasswordForm::Scheme::SCHEME_BASIC, true, true, true,
-       true, HttpCredentialType::kEquivalent},
-
-      {false, autofill::PasswordForm::Scheme::SCHEME_BASIC, false, true, true,
-       true, HttpCredentialType::kNoMatching},
-      {false, autofill::PasswordForm::Scheme::SCHEME_BASIC, true, false, true,
-       true, HttpCredentialType::kNoMatching},
-      {false, autofill::PasswordForm::Scheme::SCHEME_BASIC, true, true, false,
-       true, HttpCredentialType::kNoMatching},
-      {false, autofill::PasswordForm::Scheme::SCHEME_BASIC, true, true, true,
-       false, HttpCredentialType::kConflicting},
-      {false, autofill::PasswordForm::Scheme::SCHEME_BASIC, true, true, true,
-       true, HttpCredentialType::kEquivalent}
-
-  };
-
-  const base::string16 username[2] = {base::ASCIIToUTF16("user0"),
-                                      base::ASCIIToUTF16("user1")};
-  const base::string16 password[2] = {base::ASCIIToUTF16("pass0"),
-                                      base::ASCIIToUTF16("pass1")};
+  static const std::string signon_realm[2] = {"https://example.org/realm/",
+                                              "https://example.org/"};
+
+  static const base::string16 username[2] = {base::ASCIIToUTF16("user0"),
+                                             base::ASCIIToUTF16("user1")};
+
+  static const base::string16 password[2] = {base::ASCIIToUTF16("pass0"),
+                                             base::ASCIIToUTF16("pass1")};
+
+  base::test::ScopedTaskEnvironment scoped_task_environment;
+  ASSERT_TRUE(store_->Init(syncer::SyncableService::StartSyncFlare(), nullptr));
+  TestCase test = GetParam();
+  SCOPED_TRACE(testing::Message()
+               << "is_hsts_enabled=" << test.is_hsts_enabled
+               << ", http_form_scheme="
+               << static_cast<int>(test.http_form_scheme)
+               << ", same_signon_realm=" << test.same_signon_realm
+               << ", same_scheme=" << test.same_scheme
+               << ", same_username=" << test.same_username
+               << ", same_password=" << test.same_password);
+
+  autofill::PasswordForm http_form;
+  http_form.origin = GURL("http://example.org/");
+  http_form.signon_realm = "http://example.org/";
+  http_form.scheme = test.http_form_scheme;
+  http_form.username_value = username[1];
+  http_form.password_value = password[1];
+  store_->AddLogin(http_form);
+
+  autofill::PasswordForm https_form;
+  https_form.origin = GURL("https://example.org/");
+  https_form.signon_realm = signon_realm[test.same_signon_realm];
+  https_form.username_value = username[test.same_username];
+  https_form.password_value = password[test.same_password];
+  https_form.scheme = test.http_form_scheme;
+  if (!test.same_scheme) {
+    https_form.scheme =
+        (http_form.scheme == autofill::PasswordForm::Scheme::SCHEME_BASIC
+             ? autofill::PasswordForm::Scheme::SCHEME_HTML
+             : autofill::PasswordForm::Scheme::SCHEME_BASIC);
+  }
+  store_->AddLogin(https_form);
+
+  auto request_context = base::MakeRefCounted<net::TestURLRequestContextGetter>(
+      base::ThreadTaskRunnerHandle::Get());
+  network::mojom::NetworkContextPtr network_context_pipe;
+  auto network_context = std::make_unique<network::NetworkContext>(
+      nullptr, mojo::MakeRequest(&network_context_pipe),
+      request_context->GetURLRequestContext());
+
+  if (test.is_hsts_enabled) {
+    network_context->AddHSTSForTesting(
+        http_form.origin.host(), base::Time::Max(),
+        false /*include_subdomains*/, base::DoNothing());
+  }
+  scoped_task_environment.RunUntilIdle();
+
+  const TestPasswordStore::PasswordMap passwords_before_cleaning =
+      store_->stored_passwords();
+
+  base::HistogramTester histogram_tester;
+
+  password_manager_util::ReportHttpMigrationMetrics(
+      store_,
+      base::BindLambdaForTesting([&]() -> network::mojom::NetworkContext* {
+        // This needs to be network_context_pipe.get() and
+        // not network_context.get() to make HSTS queries asynchronous, which
+        // is what the progress tracking logic in HttpMetricsMigrationReporter
+        // assumes.  This also matches reality, since
+        // StoragePartition::GetNetworkContext will return a mojo pipe
+        // even in the in-process case.
+        return network_context_pipe.get();
+      }));
+  scoped_task_environment.RunUntilIdle();
 
   std::vector<Histogram> histograms_to_test;
   for (bool test_hsts_enabled : {true, false}) {
@@ -106,87 +202,19 @@ TEST(HttpCredentialCleaner, ReportHttpMigrationMetrics) {
          "PasswordManager.HttpCredentialsWithConflictingHttpsCredential." +
              suffix});
   }
-  for (const auto& test : cases) {
-    SCOPED_TRACE(testing::Message()
-                 << "is_hsts_enabled=" << test.is_hsts_enabled
-                 << ", http_form_scheme="
-                 << static_cast<int>(test.http_form_scheme)
-                 << ", same_signon_realm=" << test.same_signon_realm
-                 << ", same_scheme=" << test.same_scheme
-                 << ", same_username=" << test.same_username
-                 << ", same_password=" << test.same_password);
-
-    base::test::ScopedTaskEnvironment scoped_task_environment;
-    auto request_context =
-        base::MakeRefCounted<net::TestURLRequestContextGetter>(
-            base::ThreadTaskRunnerHandle::Get());
-    network::mojom::NetworkContextPtr network_context_pipe;
-    auto network_context = std::make_unique<network::NetworkContext>(
-        nullptr, mojo::MakeRequest(&network_context_pipe),
-        request_context->GetURLRequestContext());
-
-    auto password_store =
-        base::MakeRefCounted<password_manager::TestPasswordStore>();
-    ASSERT_TRUE(password_store->Init(syncer::SyncableService::StartSyncFlare(),
-                                     nullptr));
-
-    autofill::PasswordForm http_form;
-    http_form.origin = GURL("http://example.org/");
-    http_form.signon_realm = http_form.origin.GetOrigin().spec();
-    http_form.scheme = test.http_form_scheme;
-    http_form.username_value = username[1];
-    http_form.password_value = password[1];
-    password_store->AddLogin(http_form);
-
-    autofill::PasswordForm https_form;
-    https_form.origin = GURL("https://example.org/");
-    https_form.scheme = test.http_form_scheme;
-    if (!test.same_scheme) {
-      if (https_form.scheme == autofill::PasswordForm::Scheme::SCHEME_BASIC)
-        https_form.scheme = autofill::PasswordForm::Scheme::SCHEME_HTML;
-      else
-        https_form.scheme = autofill::PasswordForm::Scheme::SCHEME_BASIC;
-    }
-
-    https_form.signon_realm = https_form.origin.GetOrigin().spec();
-    if (!test.same_signon_realm)
-      https_form.signon_realm += "different/";
-
-    https_form.username_value = username[test.same_username];
-    https_form.password_value = password[test.same_password];
-    password_store->AddLogin(https_form);
-
-    if (test.is_hsts_enabled) {
-      network_context->AddHSTSForTesting(
-          http_form.origin.host(), base::Time::Max(),
-          false /*include_subdomains*/, base::DoNothing());
-    }
-    scoped_task_environment.RunUntilIdle();
-
-    base::HistogramTester histogram_tester;
-    password_manager_util::ReportHttpMigrationMetrics(
-        password_store,
-        base::BindLambdaForTesting([&]() -> network::mojom::NetworkContext* {
-          // This needs to be network_context_pipe.get() and
-          // not network_context.get() to make HSTS queries asynchronous, which
-          // is what the progress tracking logic in HttpMetricsMigrationReporter
-          // assumes.  This also matches reality, since
-          // StoragePartition::GetNetworkContext will return a mojo pipe
-          // even in the in-process case.
-          return network_context_pipe.get();
-        }));
-    scoped_task_environment.RunUntilIdle();
-
-    for (const auto& histogram : histograms_to_test) {
-      int sample =
-          static_cast<int>(histogram.test_type == test.expected &&
-                           histogram.test_hsts_enabled == test.is_hsts_enabled);
-      histogram_tester.ExpectUniqueSample(histogram.histogram_name, sample, 1);
-    }
-
-    password_store->ShutdownOnUIThread();
-    scoped_task_environment.RunUntilIdle();
+  for (const auto& histogram : histograms_to_test) {
+    int sample =
+        static_cast<int>(histogram.test_type == test.expected &&
+                         histogram.test_hsts_enabled == test.is_hsts_enabled);
+    histogram_tester.ExpectUniqueSample(histogram.histogram_name, sample, 1);
   }
+
+  store_->ShutdownOnUIThread();
+  scoped_task_environment.RunUntilIdle();
 }
 
+INSTANTIATE_TEST_CASE_P(,
+                        HttpCredentialCleanerTest,
+                        ::testing::ValuesIn(kCases));
+
 }  // namespace password_manager
\ No newline at end of file

components/password_manager/core/browser/invalid_realm_credential_cleaner.cc

@@ -13,6 +13,7 @@
 
 #include "base/strings/string_piece.h"
 #include "components/autofill/core/common/password_form.h"
+#include "components/password_manager/core/browser/password_manager_util.h"
 #include "components/password_manager/core/browser/password_store.h"
 #include "components/password_manager/core/common/password_manager_pref_names.h"
 #include "components/prefs/pref_service.h"
@@ -198,12 +199,9 @@ void InvalidRealmCredentialCleaner::OnGetPasswordStoreResults(
   // HTTP forms to the expected signon_realm (excluding the protocol).
   std::map<FormKeyForHttpMatch, std::string> http_credentials_map;
   for (const auto& form : http_forms) {
-    base::StringPiece signon_realm = form->signon_realm;
-    // Find the web origin in the signon_realm and remove what is before it.
-    // This will result in removing the protocol ("http://").
-    signon_realm = signon_realm.substr(
-        signon_realm.find(form->origin.GetOrigin().GetContent()));
-    http_credentials_map.emplace(GetFormKeyForHttpMatch(*form), signon_realm);
+    http_credentials_map.emplace(
+        GetFormKeyForHttpMatch(*form),
+        password_manager_util::GetSignonRealmWithProtocolExcluded(*form));
   }
 
   // Separate HTML and non-HTML HTTPS credentials.

components/password_manager/core/browser/password_manager_util.cc

@@ -243,6 +243,20 @@ void RemoveUselessCredentials(
       base::TimeDelta::FromSeconds(delay_in_seconds));
 }
 
+base::StringPiece GetSignonRealmWithProtocolExcluded(const PasswordForm& form) {
+  base::StringPiece signon_realm_protocol_excluded = form.signon_realm;
+
+  // Find the web origin (with protocol excluded) in the signon_realm.
+  const size_t after_protocol =
+      signon_realm_protocol_excluded.find(form.origin.GetOrigin().GetContent());
+  DCHECK_NE(after_protocol, base::StringPiece::npos);
+
+  // Keep the string starting with position |after_protocol|.
+  signon_realm_protocol_excluded =
+      signon_realm_protocol_excluded.substr(after_protocol);
+  return signon_realm_protocol_excluded;
+}
+
 void FindBestMatches(
     std::vector<const PasswordForm*> matches,
     std::map<base::string16, const PasswordForm*>* best_matches,

components/password_manager/core/browser/password_manager_util.h

@@ -105,6 +105,14 @@ void RemoveUselessCredentials(
     PrefService* prefs,
     int delay_in_seconds);
 
+// Excluding protocol from a signon_realm means to remove from the signon_realm
+// what is before the web origin (with the protocol excluded as well). For
+// example if the signon_realm is "https://www.google.com/", after
+// excluding protocol it becomes "www.google.com/".
+// This assumes that the |form|'s origin is a substring of the signon_realm.
+base::StringPiece GetSignonRealmWithProtocolExcluded(
+    const autofill::PasswordForm& form);
+
 // Report metrics about HTTP to HTTPS migration process. This function cannot be
 // used on iOS platform because the HSTS query is not supported.
 // |network_context_getter| should return nullptr if it can't get the network

components/password_manager/core/browser/password_manager_util_unittest.cc

@@ -208,6 +208,18 @@ TEST(PasswordManagerUtil,
   }
 }
 
+TEST(PasswordManagerUtil, GetSignonRealmWithProtocolExcluded) {
+  autofill::PasswordForm http_form;
+  http_form.origin = GURL("http://www.google.com/page-1/");
+  http_form.signon_realm = "http://www.google.com/";
+  EXPECT_EQ(GetSignonRealmWithProtocolExcluded(http_form), "www.google.com/");
+
+  autofill::PasswordForm https_form;
+  https_form.origin = GURL("https://www.google.com/page-1/");
+  https_form.signon_realm = "https://www.google.com/";
+  EXPECT_EQ(GetSignonRealmWithProtocolExcluded(https_form), "www.google.com/");
+}
+
 TEST(PasswordManagerUtil, FindBestMatches) {
   const int kNotFound = -1;
   struct TestMatch {