[Password Manager] Remove PSL matching for non-HTML forms

chrome/browser/password_manager/native_backend_gnome_x_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <stdarg.h>
 
 #include "base/basictypes.h"
 #include "base/prefs/pref_service.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
@@ skipping 306 matching lines @@
     form_facebook_.signon_realm = "http://www.facebook.com/";
 
     form_isc_.origin = GURL("http://www.isc.org/");
     form_isc_.action = GURL("http://www.isc.org/auth");
     form_isc_.username_element = UTF8ToUTF16("id");
     form_isc_.username_value = UTF8ToUTF16("janedoe");
     form_isc_.password_element = UTF8ToUTF16("passwd");
     form_isc_.password_value = UTF8ToUTF16("ihazabukkit");
     form_isc_.submit_element = UTF8ToUTF16("login");
     form_isc_.signon_realm = "http://www.isc.org/";
+
+    basic_auth_.origin = GURL("http://www.example.com/");
+    basic_auth_.username_value = UTF8ToUTF16("username");
+    basic_auth_.password_value = UTF8ToUTF16("pass");
+    basic_auth_.signon_realm = "http://www.example.com/Realm";
+    basic_auth_.scheme = PasswordForm::SCHEME_BASIC;
   }
 
   virtual void TearDown() {
     base::MessageLoop::current()->PostTask(FROM_HERE,
                                            base::MessageLoop::QuitClosure());
     base::MessageLoop::current()->Run();
     db_thread_.Stop();
   }
 
   void RunBothThreads() {
@@ skipping 63 matching lines @@
     CheckUint32Attribute(item, "scheme", form.scheme);
     CheckStringAttribute(item, "application", app_string);
   }
 
   // Saves |credentials| and then gets login for origin and realm |url|. Returns
   // true when something is found, and in such case copies the result to
   // |result| when |result| is not NULL. (Note that there can be max. 1 result,
   // derived from |credentials|.)
   bool CheckCredentialAvailability(const PasswordForm& credentials,
                                    const GURL& url,
+                                   const std::string& realm,

[vabr (Chromium), 2014/05/07 07:57:38]

Please explain the meaning of |realm| in the comment above. In particular the
connection to the SCHEME_BASIC is not obvious from its name.

[Garrett Casto, 2014/05/08 20:01:20]

Done.

                                    PasswordForm* result) {
     NativeBackendGnome backend(321);
     backend.Init();
 
     BrowserThread::PostTask(
         BrowserThread::DB,
         FROM_HERE,
         base::Bind(base::IgnoreResult(&NativeBackendGnome::AddLogin),
                    base::Unretained(&backend),
                    credentials));
 
     PasswordForm target_form;
     target_form.origin = url;
     target_form.signon_realm = url.spec();
+    if (!realm.empty()) {
+      target_form.signon_realm.append(realm);
+      target_form.scheme = PasswordForm::SCHEME_BASIC;
+    }
     std::vector<PasswordForm*> form_list;
     BrowserThread::PostTask(
         BrowserThread::DB,
         FROM_HERE,
         base::Bind(base::IgnoreResult(&NativeBackendGnome::GetLogins),
                    base::Unretained(&backend),
                    target_form,
                    &form_list));
 
     RunBothThreads();
 
     EXPECT_EQ(1u, mock_keyring_items.size());
     if (mock_keyring_items.size() > 0)
       CheckMockKeyringItem(&mock_keyring_items[0], credentials, "chrome-321");
+    mock_keyring_items.clear();
 
     if (form_list.empty())
       return false;
     EXPECT_EQ(1u, form_list.size());
     if (result)
       *result = *form_list[0];
     STLDeleteElements(&form_list);
     return true;
   }
 
@@ skipping 117 matching lines @@
   }
 
   base::MessageLoopForUI message_loop_;
   content::TestBrowserThread ui_thread_;
   content::TestBrowserThread db_thread_;
 
   // Provide some test forms to avoid having to set them up in each test.
   PasswordForm form_google_;
   PasswordForm form_facebook_;
   PasswordForm form_isc_;
+  PasswordForm basic_auth_;
 };
 
 TEST_F(NativeBackendGnomeTest, BasicAddLogin) {
   NativeBackendGnome backend(42);
   backend.Init();
 
   BrowserThread::PostTask(
       BrowserThread::DB, FROM_HERE,
       base::Bind(base::IgnoreResult(&NativeBackendGnome::AddLogin),
                  base::Unretained(&backend), form_google_));
@@ skipping 31 matching lines @@
   if (mock_keyring_items.size() > 0)
     CheckMockKeyringItem(&mock_keyring_items[0], form_google_, "chrome-42");
 }
 
 // Save a password for www.facebook.com and see it suggested for m.facebook.com.
 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, &result));
+  EXPECT_TRUE(CheckCredentialAvailability(
+      form_facebook_, kMobileURL, "", &result));
   EXPECT_EQ(kMobileURL, result.origin);
   EXPECT_EQ(kMobileURL.spec(), result.signon_realm);
 }
 
 // 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/"), NULL));
+      form_facebook_, GURL("http://m-facebook.com/"), "", NULL));
 }
 
 // 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/"), NULL));
+      form_google_, GURL("http://one.google.com/"), "", NULL));
+}
+
+// Make sure PSL matches aren't available for non-HTML forms.
+TEST_F(NativeBackendGnomeTest, PSLMatchingDisabledForNonHTMLForms) {

[vabr (Chromium), 2014/05/07 07:57:38]

The code change applies to all possible schemes, but the tests only care about
SCHEME_BASIC and SCHEME_HTML.

I'm not sure if it's worth the effort to add also tests for SCHEME_DIGEST and
SCHEME_OTHER. Here it would be an easy copy-paste of the added test. For the
LoginDatabase it would be probably longer.

My vote would be to cover all schemes with tests, but I don't insist on it if
you believe it's not worth it.

[Garrett Casto, 2014/05/08 20:01:20]

Done.

+  password_manager::PSLMatchingHelper helper;
+  ASSERT_TRUE(helper.IsMatchingEnabled());
+  // Don't match basic auth form with an HTML form.
+  EXPECT_FALSE(CheckCredentialAvailability(
+      basic_auth_, GURL("http://www.example.com"), "", NULL));
+  // Don't match an HTML form with a basic auth form.
+  EXPECT_FALSE(CheckCredentialAvailability(
+      form_google_, GURL("http://www.google.com/"), "Realm", NULL));
+  // Don't match two different basic auth forms with different origin.
+  EXPECT_FALSE(CheckCredentialAvailability(
+      basic_auth_, GURL("http://first.example.com"), "Realm", NULL));
 }
 
 TEST_F(NativeBackendGnomeTest, PSLUpdatingStrictUpdateLogin) {
   CheckPSLUpdate(UPDATE_BY_UPDATELOGIN);
 }
 
 TEST_F(NativeBackendGnomeTest, PSLUpdatingStrictAddLogin) {
   // TODO(vabr): if AddLogin becomes no longer valid for existing logins, then
   // just delete this test.
   CheckPSLUpdate(UPDATE_BY_ADDLOGIN);
@@ skipping 145 matching lines @@
   // Quick check that we got two results back.
   EXPECT_EQ(2u, form_list.size());
   STLDeleteElements(&form_list);
 
   EXPECT_EQ(1u, mock_keyring_items.size());
   if (mock_keyring_items.size() > 0)
     CheckMockKeyringItem(&mock_keyring_items[0], form_google_, "chrome-42");
 }
 
 // TODO(mdm): add more basic tests here at some point.