diff options
author | vabr <vabr@chromium.org> | 2014-09-24 02:19:06 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2014-09-24 09:19:24 +0000 |
commit | 4327ad3ce3504c5ae369855e24883079a67af811 (patch) | |
tree | bf5a403127a4c355f87138fbeed0acc7b8051190 | |
parent | 09f5d3bdbb0a65f6c89318da6adcda2fdfd65a90 (diff) | |
download | chromium_src-4327ad3ce3504c5ae369855e24883079a67af811.zip chromium_src-4327ad3ce3504c5ae369855e24883079a67af811.tar.gz chromium_src-4327ad3ce3504c5ae369855e24883079a67af811.tar.bz2 |
Remove Finch kill switch for PSL matching
Following up on http://crbug.com/338289#c13, this CL removes the kill switch for PSL matching. Apart from the linked check with our PM, the consideration is that PSL matching has been in stable for a couple of releases now on all platforms (except for KDE, see below), and the bugs seen so far were of low severity, exposing problems in password manager rather than with PSL matching itself.
It also replaces a couple of callsites of IsMatchingEnabled with ShouldPSLDomainMatchingApply -- the former did not take sites with a unified login origin into consideration, and on the affected callsites it seemed like it should.
One caveat: PSL matching has not been added to KDE on Linux yet (it's planned, just not a top priority). Given that PSL is live on other Linux environments, and all other platforms, the concerns about needing the kill switch for KDE are low.
As a follow-up to this CL, the (never submitted) internal CL with the corresponding Finch config will be closed.
BUG=338289
Review URL: https://codereview.chromium.org/508143002
Cr-Commit-Position: refs/heads/master@{#296371}
9 files changed, 68 insertions, 156 deletions
diff --git a/chrome/browser/password_manager/native_backend_gnome_x.cc b/chrome/browser/password_manager/native_backend_gnome_x.cc index 7500545..1035e95 100644 --- a/chrome/browser/password_manager/native_backend_gnome_x.cc +++ b/chrome/browser/password_manager/native_backend_gnome_x.cc @@ -30,7 +30,6 @@ using autofill::PasswordForm; using base::UTF8ToUTF16; using base::UTF16ToUTF8; using content::BrowserThread; -using password_manager::PSLMatchingHelper; #define GNOME_KEYRING_DEFINE_POINTER(name) \ typeof(&::gnome_keyring_##name) GnomeKeyringLoader::gnome_keyring_##name; @@ -151,17 +150,16 @@ scoped_ptr<PasswordForm> FormFromAttributes(GnomeKeyringAttributeList* attrs) { // Parse all the results from the given GList into a PasswordFormList, and free // the GList. PasswordForms are allocated on the heap, and should be deleted by // the consumer. If not NULL, |lookup_form| is used to filter out results -- -// only credentials with signon realms passing the PSL matching (done by -// |helper|) against |lookup_form->signon_realm| will be kept. PSL matched -// results get their signon_realm, origin, and action rewritten to those of -// |lookup_form_|, with the original signon_realm saved into the result's -// original_signon_realm data member. +// only credentials with signon realms passing the PSL matching against +// |lookup_form->signon_realm| will be kept. PSL matched results get their +// signon_realm, origin, and action rewritten to those of |lookup_form_|, with +// the original signon_realm saved into the result's original_signon_realm data +// member. void ConvertFormList(GList* found, const PasswordForm* lookup_form, - const PSLMatchingHelper& helper, NativeBackendGnome::PasswordFormList* forms) { - PSLMatchingHelper::PSLDomainMatchMetric psl_domain_match_metric = - PSLMatchingHelper::PSL_DOMAIN_MATCH_NONE; + password_manager::PSLDomainMatchMetric psl_domain_match_metric = + password_manager::PSL_DOMAIN_MATCH_NONE; for (GList* element = g_list_first(found); element != NULL; element = g_list_next(element)) { GnomeKeyringFound* data = static_cast<GnomeKeyringFound*>(element->data); @@ -173,11 +171,11 @@ void ConvertFormList(GList* found, // This is not an exact match, we try PSL matching. if (lookup_form->scheme != PasswordForm::SCHEME_HTML || form->scheme != PasswordForm::SCHEME_HTML || - !(PSLMatchingHelper::IsPublicSuffixDomainMatch( + !(password_manager::IsPublicSuffixDomainMatch( lookup_form->signon_realm, form->signon_realm))) { continue; } - psl_domain_match_metric = PSLMatchingHelper::PSL_DOMAIN_MATCH_FOUND; + psl_domain_match_metric = password_manager::PSL_DOMAIN_MATCH_FOUND; form->original_signon_realm = form->signon_realm; form->signon_realm = lookup_form->signon_realm; form->origin = lookup_form->origin; @@ -194,12 +192,15 @@ void ConvertFormList(GList* found, } } if (lookup_form) { + const GURL signon_realm(lookup_form->signon_realm); + std::string registered_domain = + password_manager::GetRegistryControlledDomain(signon_realm); UMA_HISTOGRAM_ENUMERATION( "PasswordManager.PslDomainMatchTriggering", - helper.IsMatchingEnabled() + password_manager::ShouldPSLDomainMatchingApply(registered_domain) ? psl_domain_match_metric - : PSLMatchingHelper::PSL_DOMAIN_MATCH_DISABLED, - PSLMatchingHelper::PSL_DOMAIN_MATCH_COUNT); + : password_manager::PSL_DOMAIN_MATCH_NOT_USED, + password_manager::PSL_DOMAIN_MATCH_COUNT); } } @@ -314,7 +315,6 @@ class GKRMethod : public GnomeKeyringLoader { // found logins to those which indeed PSL-match the look-up. And finally, // |lookup_form_| set to NULL means that PSL matching is not required. scoped_ptr<PasswordForm> lookup_form_; - const PSLMatchingHelper helper_; }; void GKRMethod::AddLogin(const PasswordForm& form, const char* app_string) { @@ -418,9 +418,9 @@ void GKRMethod::GetLogins(const PasswordForm& form, const char* app_string) { lookup_form_.reset(new PasswordForm(form)); // Search GNOME Keyring for matching passwords. ScopedAttributeList attrs(gnome_keyring_attribute_list_new()); - if (!helper_.ShouldPSLDomainMatchingApply( - PSLMatchingHelper::GetRegistryControlledDomain( - GURL(form.signon_realm)))) { + if (!password_manager::ShouldPSLDomainMatchingApply( + password_manager::GetRegistryControlledDomain( + GURL(form.signon_realm)))) { AppendString(&attrs, "signon_realm", form.signon_realm); } AppendString(&attrs, "application", app_string); @@ -515,8 +515,7 @@ void GKRMethod::OnOperationGetList(GnomeKeyringResult result, GList* list, method->result_ = result; method->forms_.clear(); // |list| will be freed after this callback returns, so convert it now. - ConvertFormList( - list, method->lookup_form_.get(), method->helper_, &method->forms_); + ConvertFormList(list, method->lookup_form_.get(), &method->forms_); method->lookup_form_.reset(NULL); method->event_.Signal(); } diff --git a/chrome/browser/password_manager/native_backend_gnome_x_unittest.cc b/chrome/browser/password_manager/native_backend_gnome_x_unittest.cc index c5f28c4..723d494 100644 --- a/chrome/browser/password_manager/native_backend_gnome_x_unittest.cc +++ b/chrome/browser/password_manager/native_backend_gnome_x_unittest.cc @@ -533,9 +533,6 @@ class NativeBackendGnomeTest : public testing::Test { // m.facebook.com password should not get updated. Depending on the argument, // the credential update is done via UpdateLogin or AddLogin. void CheckPSLUpdate(UpdateType update_type) { - password_manager::PSLMatchingHelper helper; - ASSERT_TRUE(helper.IsMatchingEnabled()); - NativeBackendGnome backend(321); backend.Init(); @@ -800,8 +797,6 @@ TEST_F(NativeBackendGnomeTest, BasicListLogins) { TEST_F(NativeBackendGnomeTest, PSLMatchingPositive) { PasswordForm result; const GURL kMobileURL("http://m.facebook.com/"); - password_manager::PSLMatchingHelper helper; - ASSERT_TRUE(helper.IsMatchingEnabled()); EXPECT_TRUE(CheckCredentialAvailability( form_facebook_, kMobileURL, PasswordForm::SCHEME_HTML, &result)); EXPECT_EQ(kMobileURL, result.origin); @@ -811,8 +806,6 @@ TEST_F(NativeBackendGnomeTest, PSLMatchingPositive) { // Save a password for www.facebook.com and see it not suggested for // m-facebook.com. TEST_F(NativeBackendGnomeTest, PSLMatchingNegativeDomainMismatch) { - password_manager::PSLMatchingHelper helper; - ASSERT_TRUE(helper.IsMatchingEnabled()); EXPECT_FALSE(CheckCredentialAvailability( form_facebook_, GURL("http://m-facebook.com/"), PasswordForm::SCHEME_HTML, NULL)); @@ -820,8 +813,6 @@ TEST_F(NativeBackendGnomeTest, PSLMatchingNegativeDomainMismatch) { // Test PSL matching is off for domains excluded from it. TEST_F(NativeBackendGnomeTest, PSLMatchingDisabledDomains) { - password_manager::PSLMatchingHelper helper; - ASSERT_TRUE(helper.IsMatchingEnabled()); EXPECT_FALSE(CheckCredentialAvailability( form_google_, GURL("http://one.google.com/"), PasswordForm::SCHEME_HTML, NULL)); @@ -829,9 +820,6 @@ TEST_F(NativeBackendGnomeTest, PSLMatchingDisabledDomains) { // Make sure PSL matches aren't available for non-HTML forms. TEST_F(NativeBackendGnomeTest, PSLMatchingDisabledForNonHTMLForms) { - password_manager::PSLMatchingHelper helper; - ASSERT_TRUE(helper.IsMatchingEnabled()); - CheckMatchingWithScheme(PasswordForm::SCHEME_BASIC); CheckMatchingWithScheme(PasswordForm::SCHEME_DIGEST); CheckMatchingWithScheme(PasswordForm::SCHEME_OTHER); diff --git a/components/password_manager/core/browser/login_database.cc b/components/password_manager/core/browser/login_database.cc index d9e6f09..e2ebfc5 100644 --- a/components/password_manager/core/browser/login_database.cc +++ b/components/password_manager/core/browser/login_database.cc @@ -577,13 +577,12 @@ bool LoginDatabase::GetLogins(const PasswordForm& form, "federation_url, is_zero_click FROM logins WHERE signon_realm == ? "; sql::Statement s; const GURL signon_realm(form.signon_realm); - std::string registered_domain = - PSLMatchingHelper::GetRegistryControlledDomain(signon_realm); - PSLMatchingHelper::PSLDomainMatchMetric psl_domain_match_metric = - PSLMatchingHelper::PSL_DOMAIN_MATCH_NONE; + std::string registered_domain = GetRegistryControlledDomain(signon_realm); + PSLDomainMatchMetric psl_domain_match_metric = PSL_DOMAIN_MATCH_NONE; + const bool should_PSL_matching_apply = + ShouldPSLDomainMatchingApply(registered_domain); // PSL matching only applies to HTML forms. - if (form.scheme == PasswordForm::SCHEME_HTML && - psl_helper_.ShouldPSLDomainMatchingApply(registered_domain)) { + if (form.scheme == PasswordForm::SCHEME_HTML && should_PSL_matching_apply) { // We are extending the original SQL query with one that includes more // possible matches based on public suffix domain matching. Using a regexp // here is just an optimization to not have to parse all the stored entries @@ -613,7 +612,7 @@ bool LoginDatabase::GetLogins(const PasswordForm& form, s.BindString(0, form.signon_realm); s.BindString(1, regexp); } else { - psl_domain_match_metric = PSLMatchingHelper::PSL_DOMAIN_MATCH_DISABLED; + psl_domain_match_metric = PSL_DOMAIN_MATCH_NOT_USED; s.Assign(db_.GetCachedStatement(SQL_FROM_HERE, sql_query.c_str())); s.BindString(0, form.signon_realm); } @@ -626,9 +625,9 @@ bool LoginDatabase::GetLogins(const PasswordForm& form, if (result == ENCRYPTION_RESULT_ITEM_FAILURE) continue; DCHECK(result == ENCRYPTION_RESULT_SUCCESS); - if (psl_helper_.IsMatchingEnabled()) { - if (!PSLMatchingHelper::IsPublicSuffixDomainMatch(new_form->signon_realm, - form.signon_realm)) { + if (should_PSL_matching_apply) { + if (!IsPublicSuffixDomainMatch(new_form->signon_realm, + form.signon_realm)) { // The database returned results that should not match. Skipping result. continue; } @@ -637,7 +636,7 @@ bool LoginDatabase::GetLogins(const PasswordForm& form, if (new_form->scheme != PasswordForm::SCHEME_HTML) continue; - psl_domain_match_metric = PSLMatchingHelper::PSL_DOMAIN_MATCH_FOUND; + psl_domain_match_metric = PSL_DOMAIN_MATCH_FOUND; // This is not a perfect match, so we need to create a new valid result. // We do this by copying over origin, signon realm and action from the // observed form and setting the original signon realm to what we found @@ -654,7 +653,7 @@ bool LoginDatabase::GetLogins(const PasswordForm& form, } UMA_HISTOGRAM_ENUMERATION("PasswordManager.PslDomainMatchTriggering", psl_domain_match_metric, - PSLMatchingHelper::PSL_DOMAIN_MATCH_COUNT); + PSL_DOMAIN_MATCH_COUNT); return s.Succeeded(); } diff --git a/components/password_manager/core/browser/login_database.h b/components/password_manager/core/browser/login_database.h index 9c9f273..0ea86e5 100644 --- a/components/password_manager/core/browser/login_database.h +++ b/components/password_manager/core/browser/login_database.h @@ -147,8 +147,6 @@ class LoginDatabase { mutable sql::Connection db_; sql::MetaTable meta_table_; - PSLMatchingHelper psl_helper_; - DISALLOW_COPY_AND_ASSIGN(LoginDatabase); }; diff --git a/components/password_manager/core/browser/login_database_unittest.cc b/components/password_manager/core/browser/login_database_unittest.cc index 2943b7e..4dd83f8 100644 --- a/components/password_manager/core/browser/login_database_unittest.cc +++ b/components/password_manager/core/browser/login_database_unittest.cc @@ -257,7 +257,6 @@ TEST_F(LoginDatabaseTest, Logins) { } TEST_F(LoginDatabaseTest, TestPublicSuffixDomainMatching) { - PSLMatchingHelper::EnablePublicSuffixDomainMatchingForTesting(); std::vector<PasswordForm*> result; // Verify the database is empty. @@ -307,15 +306,12 @@ TEST_F(LoginDatabaseTest, TestPublicSuffixDomainMatching) { } TEST_F(LoginDatabaseTest, TestPublicSuffixDisabledForNonHTMLForms) { - PSLMatchingHelper::EnablePublicSuffixDomainMatchingForTesting(); - TestNonHTMLFormPSLMatching(PasswordForm::SCHEME_BASIC); TestNonHTMLFormPSLMatching(PasswordForm::SCHEME_DIGEST); TestNonHTMLFormPSLMatching(PasswordForm::SCHEME_OTHER); } TEST_F(LoginDatabaseTest, TestPublicSuffixDomainMatchingShouldMatchingApply) { - PSLMatchingHelper::EnablePublicSuffixDomainMatchingForTesting(); std::vector<PasswordForm*> result; // Verify the database is empty. @@ -365,7 +361,6 @@ TEST_F(LoginDatabaseTest, TestPublicSuffixDomainMatchingShouldMatchingApply) { // instead of GetUniqueStatement, since REGEXP is in use. See // http://crbug.com/248608. TEST_F(LoginDatabaseTest, TestPublicSuffixDomainMatchingDifferentSites) { - PSLMatchingHelper::EnablePublicSuffixDomainMatchingForTesting(); std::vector<PasswordForm*> result; // Verify the database is empty. @@ -459,7 +454,6 @@ PasswordForm GetFormWithNewSignonRealm(PasswordForm form, } TEST_F(LoginDatabaseTest, TestPublicSuffixDomainMatchingRegexp) { - PSLMatchingHelper::EnablePublicSuffixDomainMatchingForTesting(); std::vector<PasswordForm*> result; // Verify the database is empty. diff --git a/components/password_manager/core/browser/psl_matching_helper.cc b/components/password_manager/core/browser/psl_matching_helper.cc index 484ca18..69244fa 100644 --- a/components/password_manager/core/browser/psl_matching_helper.cc +++ b/components/password_manager/core/browser/psl_matching_helper.cc @@ -7,7 +7,6 @@ #include "base/command_line.h" #include "base/logging.h" #include "base/memory/scoped_ptr.h" -#include "base/metrics/field_trial.h" #include "components/autofill/core/common/password_form.h" #include "components/password_manager/core/common/password_manager_switches.h" #include "net/base/registry_controlled_domains/registry_controlled_domain.h" @@ -17,33 +16,13 @@ using autofill::PasswordForm; namespace password_manager { -#if !defined(OS_IOS) -namespace { - -const char kPSLMatchingDesktopFieldTrialName[] = "PSLMatchingDesktop"; -const char kPSLMatchingDesktopFieldTrialDisabledGroupName[] = "Disabled"; - -} // namespace -#endif - -bool PSLMatchingHelper::psl_enabled_override_ = false; - -PSLMatchingHelper::PSLMatchingHelper() : psl_enabled_(DeterminePSLEnabled()) {} - -PSLMatchingHelper::~PSLMatchingHelper() {} - -bool PSLMatchingHelper::IsMatchingEnabled() const { - return psl_enabled_override_ || psl_enabled_; +bool ShouldPSLDomainMatchingApply( + const std::string& registry_controlled_domain) { + return registry_controlled_domain != "google.com"; } -bool PSLMatchingHelper::ShouldPSLDomainMatchingApply( - const std::string& registry_controlled_domain) const { - return IsMatchingEnabled() && registry_controlled_domain != "google.com"; -} - -// static -bool PSLMatchingHelper::IsPublicSuffixDomainMatch(const std::string& url1, - const std::string& url2) { +bool IsPublicSuffixDomainMatch(const std::string& url1, + const std::string& url2) { GURL gurl1(url1); GURL gurl2(url2); @@ -56,29 +35,10 @@ bool PSLMatchingHelper::IsPublicSuffixDomainMatch(const std::string& url1, gurl1.port() == gurl2.port(); } -// static -std::string PSLMatchingHelper::GetRegistryControlledDomain( - const GURL& signon_realm) { +std::string GetRegistryControlledDomain(const GURL& signon_realm) { return net::registry_controlled_domains::GetDomainAndRegistry( signon_realm, net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES); } -// static -void PSLMatchingHelper::EnablePublicSuffixDomainMatchingForTesting() { - psl_enabled_override_ = true; -} - -// static -bool PSLMatchingHelper::DeterminePSLEnabled() { - bool enabled = true; -#if !defined(OS_IOS) - if (base::FieldTrialList::FindFullName(kPSLMatchingDesktopFieldTrialName) == - kPSLMatchingDesktopFieldTrialDisabledGroupName) { - enabled = false; - } -#endif - return enabled; -} - } // namespace password_manager diff --git a/components/password_manager/core/browser/psl_matching_helper.h b/components/password_manager/core/browser/psl_matching_helper.h index 7213e06..e4e6ac1 100644 --- a/components/password_manager/core/browser/psl_matching_helper.h +++ b/components/password_manager/core/browser/psl_matching_helper.h @@ -17,63 +17,38 @@ struct PasswordForm; namespace password_manager { -class PSLMatchingHelper { - public: - // Enum used for histogram tracking PSL Domain triggering. - // New entries should only be added to the end of the enum (before *_COUNT) so - // as to not disrupt existing data. - enum PSLDomainMatchMetric { - PSL_DOMAIN_MATCH_DISABLED = 0, - PSL_DOMAIN_MATCH_NONE, - PSL_DOMAIN_MATCH_FOUND, - PSL_DOMAIN_MATCH_COUNT - }; - - PSLMatchingHelper(); - ~PSLMatchingHelper(); - - bool IsMatchingEnabled() const; - - // Using the public suffix list for matching the origin is only needed for - // websites that do not have a single hostname for entering credentials. It - // would be better for their users if they did, but until then we help them - // find - // credentials across different hostnames. We know that accounts.google.com is - // the only hostname we should be accepting credentials on for any domain - // under - // google.com, so we can apply a tighter policy for that domain. - // For owners of domains where a single hostname is always used when your - // users are entering their credentials, please contact palmer@chromium.org, - // nyquist@chromium.org or file a bug at http://crbug.com/ to be added here. - bool ShouldPSLDomainMatchingApply( - const std::string& registry_controlled_domain) const; - - // Two URLs are considered a Public Suffix Domain match if they have the same - // scheme, ports, and their registry controlled domains are equal. If one or - // both arguments do not describe valid URLs, returns false. - static bool IsPublicSuffixDomainMatch(const std::string& url1, - const std::string& url2); - - // Two hosts are considered to belong to the same website when they share the - // registry-controlled domain part. - static std::string GetRegistryControlledDomain(const GURL& signon_realm); - - // This overrides both the command line flags and platform restrictions. This - // function is not thread safe, and should be called before any other methods - // of |PSLMatchingHelper| are called. - static void EnablePublicSuffixDomainMatchingForTesting(); - - private: - static bool DeterminePSLEnabled(); - - const bool psl_enabled_; - - // Default is false, once set to true, overrides |psl_enabled_|. - static bool psl_enabled_override_; - - DISALLOW_COPY_AND_ASSIGN(PSLMatchingHelper); +// Enum used for histogram tracking PSL Domain triggering. +// New entries should only be added to the end of the enum (before *_COUNT) so +// as to not disrupt existing data. +enum PSLDomainMatchMetric { + PSL_DOMAIN_MATCH_NOT_USED = 0, + PSL_DOMAIN_MATCH_NONE, + PSL_DOMAIN_MATCH_FOUND, + PSL_DOMAIN_MATCH_COUNT }; +// Using the public suffix list for matching the origin is only needed for +// websites that do not have a single hostname for entering credentials. It +// would be better for their users if they did, but until then we help them find +// credentials across different hostnames. We know that accounts.google.com is +// the only hostname we should be accepting credentials on for any domain under +// google.com, so we can apply a tighter policy for that domain. For owners of +// domains where a single hostname is always used when your users are entering +// their credentials, please contact palmer@chromium.org, nyquist@chromium.org +// or file a bug at http://crbug.com/ to be added here. +bool ShouldPSLDomainMatchingApply( + const std::string& registry_controlled_domain); + +// Two URLs are considered a Public Suffix Domain match if they have the same +// scheme, ports, and their registry controlled domains are equal. If one or +// both arguments do not describe valid URLs, returns false. +bool IsPublicSuffixDomainMatch(const std::string& url1, + const std::string& url2); + +// Two hosts are considered to belong to the same website when they share the +// registry-controlled domain part. +std::string GetRegistryControlledDomain(const GURL& signon_realm); + } // namespace password_manager #endif // COMPONENTS_PASSWORD_MANAGER_CORE_BROWSER_PSL_MATCHING_HELPER_H_ diff --git a/components/password_manager/core/browser/psl_matching_helper_unittest.cc b/components/password_manager/core/browser/psl_matching_helper_unittest.cc index 4f282a3..fdc4708 100644 --- a/components/password_manager/core/browser/psl_matching_helper_unittest.cc +++ b/components/password_manager/core/browser/psl_matching_helper_unittest.cc @@ -48,8 +48,7 @@ TEST(PSLMatchingUtilsTest, IsPublicSuffixDomainMatch) { autofill::PasswordForm form2; form2.signon_realm = pairs[i].url2; EXPECT_EQ(pairs[i].should_match, - PSLMatchingHelper::IsPublicSuffixDomainMatch(form1.signon_realm, - form2.signon_realm)) + IsPublicSuffixDomainMatch(form1.signon_realm, form2.signon_realm)) << "First URL = " << pairs[i].url1 << ", second URL = " << pairs[i].url2; } diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index 9211214..10b2c10 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -48177,7 +48177,7 @@ To add a new entry, add it with any value and run test to compute valid value. The value indicates whether an entry returned by password autofill contains a value that was found by matching against the public suffix list. </summary> - <int value="0" label="Matching disabled"/> + <int value="0" label="Matching not used"/> <int value="1" label="No match"/> <int value="2" label="Match"/> </enum> |