Bug 1590622 - Prefer a domain match over a scheme match when deduping logins. r=sfoster
authorMatthew Noorenberghe <mozilla@noorenberghe.ca>
Wed, 13 Nov 2019 18:53:33 +0000
changeset 501776 07adfe0808dd4e04c53e8d641898fda21ccbdd24
parent 501775 f30f256f9b67880a1d934d2e1a4d0fd012d06818
child 501777 f65cd268b0e59b40ff7e6b1daa536c2e626e6bfe
push id114172
push userdluca@mozilla.com
push dateTue, 19 Nov 2019 11:31:10 +0000
treeherdermozilla-inbound@b5c5ba07d3db [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfoster
bugs1590622
milestone72.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1590622 - Prefer a domain match over a scheme match when deduping logins. r=sfoster On an https: page it's more important to use the login was from the same hostPort than to use an https: login from a different subdomain. This fixes autocomplete to show "From this website" and also fixes autofill in the event that there was a duplicate username+password combo saved on an different https: subdomain while previously saving that combo on the http: version of the same hostPort. Differential Revision: https://phabricator.services.mozilla.com/D52802
toolkit/components/passwordmgr/LoginManagerParent.jsm
toolkit/components/passwordmgr/test/mochitest/test_autofill_https_upgrade.html
toolkit/components/passwordmgr/test/unit/test_LoginManagerParent_searchAndDedupeLogins.js
toolkit/components/passwordmgr/test/unit/test_dedupeLogins.js
--- a/toolkit/components/passwordmgr/LoginManagerParent.jsm
+++ b/toolkit/components/passwordmgr/LoginManagerParent.jsm
@@ -172,19 +172,19 @@ class LoginManagerParent extends JSWindo
         return [];
       }
       throw e;
     }
 
     logins = LoginHelper.shadowHTTPLogins(logins);
 
     let resolveBy = [
+      "subdomain",
       "actionOrigin",
       "scheme",
-      "subdomain",
       "timePasswordChanged",
     ];
     return LoginHelper.dedupeLogins(
       logins,
       ["username", "password"],
       resolveBy,
       formOrigin,
       formActionOrigin
--- a/toolkit/components/passwordmgr/test/mochitest/test_autofill_https_upgrade.html
+++ b/toolkit/components/passwordmgr/test/mochitest/test_autofill_https_upgrade.html
@@ -41,17 +41,20 @@ async function prepareLoginsAndProcessFo
     await LoginManager.addLogin(login);
   }
 
   iframe.src = url;
   await promiseFormsProcessed();
 }
 
 add_task(async function setup() {
-  await SpecialPowers.pushPrefEnv({"set": [["signon.schemeUpgrades", true]]});
+  await SpecialPowers.pushPrefEnv({"set": [
+    ["signon.schemeUpgrades", true],
+    ["signon.includeOtherSubdomainsInLookup", true],
+  ]});
 });
 
 add_task(async function test_simpleNoDupesNoAction() {
   await prepareLoginsAndProcessForm("https://example.com" + MISSING_ACTION_PATH, [
     new nsLoginInfo("http://example.com", "http://example.com", null,
                     "name2", "pass2", "uname", "pword"),
   ]);
 
@@ -101,12 +104,45 @@ add_task(async function test_dedupe() {
     new nsLoginInfo("https://example.com", "http://example.com", null,
                     "name1", "passHTTPStoHTTP", "uname", "pword"),
   ]);
 
   await checkLoginFormInChildFrame(getIframeBrowsingContext(window, 0), "form-basic-username", "name1",
                                            "form-basic-password", "passHTTPStoHTTPS");
 });
 
+add_task(async function test_dedupe_subdomain() {
+  // subdomain match (should be autofilled)
+  let loginToFill = new nsLoginInfo("http://test1.example.com", "http://test1.example.com", null,
+                                    "name1", "pass1");
+  const loginToFillGUID = "subdomain-match"
+  // Assign a GUID to this login so we can ensure this is the login that gets
+  // filled later.
+  loginToFill.QueryInterface(SpecialPowers.Ci.nsILoginMetaInfo).guid = loginToFillGUID;
+
+  await prepareLoginsAndProcessForm("https://test1.example.com" + MISSING_ACTION_PATH, [
+    // All logins have the same username and password:
+    // https: (scheme match)
+    new nsLoginInfo("https://example.com", "https://example.com", null,
+                    "name1", "pass1"),
+    loginToFill,
+    // formActionOrigin match
+    new nsLoginInfo("http://example.com", "https://test1.example.com", null,
+                    "name1", "pass1"),
+  ]);
+
+  let bc = getIframeBrowsingContext(window, 0);
+  await checkLoginFormInChildFrame(bc, "form-basic-username", "name1",
+                                   "form-basic-password", "pass1");
+
+  let filledGUID = await SpecialPowers.spawn(bc, [], function getFilledGUID() {
+    let LMC = this.content.getWindowGlobalChild().getActor("LoginManager");
+    let doc = this.content.document;
+    let form = doc.getElementById("form-basic");
+    return LMC.stateForDocument(doc).fillsByRootElement.get(form).guid;
+  });
+  is(filledGUID, loginToFillGUID, "Check the correct login was filled");
+});
+
 </script>
 </pre>
 </body>
 </html>
--- a/toolkit/components/passwordmgr/test/unit/test_LoginManagerParent_searchAndDedupeLogins.js
+++ b/toolkit/components/passwordmgr/test/unit/test_LoginManagerParent_searchAndDedupeLogins.js
@@ -118,16 +118,23 @@ add_task(async function test_searchAndDe
     },
     {
       description: "HTTPS form, different origin, different scheme",
       formActionOrigin: DOMAIN1_HTTPS_ORIGIN,
       logins: [DOMAIN1_HTTPS_TO_HTTPS_U1_P1, DOMAIN2_HTTP_TO_HTTP_U1_P1],
       expected: [DOMAIN1_HTTPS_TO_HTTPS_U1_P1],
     },
     {
+      description:
+        "HTTPS form, different origin, different scheme, same password, same hostPort preferred",
+      formActionOrigin: DOMAIN1_HTTPS_ORIGIN,
+      logins: [DOMAIN1_HTTP_TO_HTTP_U1_P1, DOMAIN2_HTTPS_TO_HTTPS_U1_P1],
+      expected: [DOMAIN1_HTTP_TO_HTTP_U1_P1],
+    },
+    {
       description: "HTTP form, different origin, different scheme",
       formActionOrigin: DOMAIN1_HTTP_ORIGIN,
       logins: [DOMAIN1_HTTPS_TO_HTTPS_U1_P1, DOMAIN2_HTTP_TO_HTTP_U1_P1],
       expected: [DOMAIN2_HTTP_TO_HTTP_U1_P1],
     },
     {
       description: "HTTPS form, different username, different scheme",
       formActionOrigin: DOMAIN1_HTTPS_ORIGIN,
--- a/toolkit/components/passwordmgr/test/unit/test_dedupeLogins.js
+++ b/toolkit/components/passwordmgr/test/unit/test_dedupeLogins.js
@@ -347,30 +347,30 @@ add_task(async function test_dedupeLogin
         DOMAIN1_HTTPS_NONSTANDARD_PORT1,
         DOMAIN1_HTTPS_NONSTANDARD_PORT2,
       ],
       undefined,
       ["subdomain"],
       DOMAIN1_HTTPS_AUTH.origin,
     ],
     [
-      "resolveBy matching _searchAndDedupeLogins, prefer https: scheme over http: in primary and subdomains",
+      "resolveBy matching searchAndDedupeLogins, prefer domain matches then https: scheme over http:",
       // expected:
       [DOMAIN1_HTTPS_TO_HTTPS_U1_P1, DOMAIN2_HTTPS_TO_HTTPS_U2_P2],
       // logins:
       [
         DOMAIN1_HTTP_TO_HTTP_U1_P1,
         DOMAIN1_HTTPS_TO_HTTPS_U1_P1,
         DOMAIN2_HTTP_TO_HTTP_U2_P2,
         DOMAIN2_HTTPS_TO_HTTPS_U2_P2,
       ],
       // uniqueKeys:
       undefined,
       // resolveBy:
-      ["actionOrigin", "scheme", "subdomain", "timePasswordChanged"],
+      ["subdomain", "actionOrigin", "scheme", "timePasswordChanged"],
       // preferredOrigin:
       DOMAIN1_HTTPS_TO_HTTPS_U1_P1.origin,
     ],
   ];
 
   for (let tc of testcases) {
     let description = tc.shift();
     let expected = tc.shift();