1200472 - Broaden search criteria for password fill suggestions shown in the context menu affordance "Fill Password" to include subdomains.

Status: RESOLVED FIXED

(Toolkit :: Password Manager, enhancement, P1)

Description

[Chris Karlof [:ckarlof]]

I had less than ideal experience using the context menu to try to fill my BofA password this weekend.

I invoked the Fill Password affordance in the context menu on BofA to fill my password. It said "no suggestions". I was on www.bankofamerica.com, but the password was saved under secure.bankofamerica.com. 

I propose we should broaden search criteria shown in this suggestion list. Search second level domain (i.e., *.bankofamerica.com) or perhaps even ignore the scheme? We potentially have the opportunity to show the origin for each of the entries we suggest here, to minimize user error and confusion.

Comment 2

[Matthew N. [:MattN]]

I'm going to undupe this since bug 589628 was going to cover both autocomplete and the context menu but the autocomplete code is being refactored in bug 1189618 and I don't want to block on that.

Comment 3

[Saad Quadri [:saadq]]

Attachment: Bug 1200472 - Broaden search criteria to include subdomains in context menu.

Review commit: https://reviewboard.mozilla.org/r/73646/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73646/

Comment 4

[Saad Quadri [:saadq]]

Comment on attachment 8784074 [details]
Bug 1200472 - Broaden search criteria to include subdomains in context menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73646/diff/1-2/

Comment 5

[Saad Quadri [:saadq]]

I realized that in my first patch there was an issue where it would not provide logins if the `schemeUpgrades` options was enabled, but the schemes were the same. The new one should work.

Comment 6

[Saad Quadri [:saadq]]

Attachment: Bug 1200472 - Add test cases for includeETLDPlusOne search option.

Review commit: https://reviewboard.mozilla.org/r/73712/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/73712/

Comment 7

[Saad Quadri [:saadq]]

Comment on attachment 8784074 [details]
Bug 1200472 - Broaden search criteria to include subdomains in context menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/73646/diff/2-3/

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8784163 [details]
Bug 1200472 - Add test cases for includeETLDPlusOne search option.

https://reviewboard.mozilla.org/r/73712/#review71850

Good job on the many cases. I have a few more to add and some comments about port handling for the patch. Thanks

::: toolkit/components/passwordmgr/test/unit/test_isOriginMatching.js:34
(Diff revision 1)
>      [true, "https://example.com:8443", "https://example.com:8443", { schemeUpgrades: true }],
>      [false, "https://example.com", "http://example.com", { schemeUpgrades: true }], // downgrade
>      [false, "http://example.com:8080", "https://example.com", { schemeUpgrades: true }], // port mismatch
>      [false, "http://example.com", "https://example.com:8443", { schemeUpgrades: true }], // port mismatch
>      [false, "http://sub.example.com", "http://example.com", { schemeUpgrades: true }],
> +    [true, "http://sub.example.com", "http://example.com", { includeETLDPlusOne: true }],

Can you test cases where aSearchOrigin is for a subdomain of the loginOrigin?

::: toolkit/components/passwordmgr/test/unit/test_isOriginMatching.js:37
(Diff revision 1)
> +    [false, "http://sub.example.com:8080", "https://example.com", { includeETLDPlusOne: true }], // port mismatch
> +    [false, "http://example.com", "https://example.com:8443", { includeETLDPlusOne: true }], // port mismatch

a) These aren't only testing port mismatches because they don't have the same scheme and therefore these wouldn't be returned even if the ports matched.

b) I think these should be true (after making the schemes match) as I don't think we should take the port into account when doing eTLD+1 upgrades.

::: toolkit/components/passwordmgr/test/unit/test_isOriginMatching.js:43
(Diff revision 1)
> +    [false, "http://example.com", "https://example.com:8443", {
> +      includeETLDPlusOne: true,
> +      schemeUpgrades: true
> +    }],
> +    [false, "http://sub.example.com:8080", "https://example.com", {
> +      includeETLDPlusOne: true,
> +      schemeUpgrades: true
> +    }],

I also think these should be true as I don't think we should take the port into account when doing eTLD+1 upgrades.

Can you also add a case for bug 1266655 that is true when the schemes and baseDomain are the same but the port is different?

::: toolkit/components/passwordmgr/test/unit/test_isOriginMatching.js:51
(Diff revision 1)
> +    [false, "https://sub.example.com", "http://example.com", {
> +      includeETLDPlusOne: true,
> +      schemeUpgrades: true
> +    }],

Can you add "// downgrade" as a note about why this is false?

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8784074 [details]
Bug 1200472 - Broaden search criteria to include subdomains in context menu.

https://reviewboard.mozilla.org/r/73646/#review71842

This looks pretty good though I think the behaviour for ports should change and there are some testing comments. Thanks Saad.

::: toolkit/components/passwordmgr/LoginHelper.jsm:189
(Diff revision 3)
> -    if (!aOptions) {
> +    let { schemeUpgrades, includeETLDPlusOne } = aOptions;
> +
> +    if (!schemeUpgrades && !includeETLDPlusOne) {
>        return false;
>      }

Did no tests fail due to null being passed for `aOptions`? If not, I suppose removing the `!aOptions` check is okay.

::: toolkit/components/passwordmgr/LoginHelper.jsm:201
(Diff revision 3)
> +        let eTLDService = Cc["@mozilla.org/network/effective-tld-service;1"]
> +                     .getService(Ci.nsIEffectiveTLDService);

You can move this to the top of the file so we can save some work getting the service for each call.
```
XPCOMUtils.defineLazyServiceGetter(this, "eTLDService",
                                   "@mozilla.org/network/effective-tld-service;1",
                                   "nsIEffectiveTLDService");
```

::: toolkit/components/passwordmgr/LoginHelper.jsm:207
(Diff revision 3)
> +      let hostPortMatches = () => {
> +        return includeETLDPlusOne
> +          ? loginURI.port == searchURI.port && loginBase == searchBase
> +          : loginURI.hostPort == searchURI.hostPort;
> +      };

I find it a bit confusing that you're referencing `loginBase` and `searchBase` in a different block than they're assigned to so I think it would be a bit clearer if this method used regular `if` blocks and loginBase/searchBase were declared and assigned to in the `includeETLDPlusOne` block:
```
if (includeETLDPlusOne) {
  let loginBase = eTLDService.getBaseDomain(loginURI);
  let searchBase = eTLDService.getBaseDomain(searchURI);
  return loginBase == searchBase;
}

return loginURI.hostPort == searchURI.hostPort;
```

You can maybe even inline loginBase/searchBase if the line will be < 100 characters.

::: toolkit/components/passwordmgr/LoginHelper.jsm:209
(Diff revision 3)
> +        searchBase = eTLDService.getBaseDomain(searchURI);
> +      }
> +
> +      let hostPortMatches = () => {
> +        return includeETLDPlusOne
> +          ? loginURI.port == searchURI.port && loginBase == searchBase

I may not have mentioned this before but I think we should ignore the port for eTLD+1 searches. See bug 1266655.

::: toolkit/components/passwordmgr/test/unit/test_context_menu.js:32
(Diff revision 3)
>    let testHostnames = [
>      "http://www.example.com",
>      "http://www2.example.com",
>      "http://www3.example.com",
>      "http://empty.example.com",
>    ];

Since these all have the same eTLD+1 we're not testing the same cases anymore. For example, empty.example.com previous returned no results but now it will return all of them. Can you change empty.example.com to empty.mozilla.org to continue tesitng the empty case?

::: toolkit/components/passwordmgr/test/unit/test_context_menu.js:135
(Diff revision 3)
>  function getExpectedLogins(hostname) {
> -  return Services.logins.getAllLogins().filter(entry => entry["hostname"] === hostname);
> +  let uri = Services.io.newURI(hostname, null, null);
> +
> +  let searchParams = {
> +    hostname: uri.prePath,
> +    schemeUpgrades: LoginHelper.schemeUpgrades,
> +    includeETLDPlusOne: true
> +  };
> +
> +  return LoginHelper.searchLoginsWithObject(searchParams);
>  }

My concern with this approach is that this means that if searchLoginsWithObject or any of the functions it calls are wrong then we may no longer fail this test since both the expected and actual may both be wrong in the same way. Could you go back to the old way of doing the equivalent work in a different way like so:
```
return Services.logins.getAllLogins().filter(entry => entry["hostname"].endsWith(hostname.replace(/^.*\./, '')));
```

::: toolkit/components/passwordmgr/test/unit/test_context_menu.js:157
(Diff revision 3)
>        new LoginInfo("http://www2.example.com", "http://www.example.com", null,
>                      "username", "password",
>                      "form_field_username", "form_field_password"),
>        new LoginInfo("http://www2.example.com", "http://www2.example.com", null,
>                      "username", "password2",
>                      "form_field_username", "form_field_password"),
>        new LoginInfo("http://www2.example.com", "http://www2.example.com", null,
>                      "username2", "password2",
>                      "form_field_username", "form_field_password"),

Could you switch these from example.com to example.net (and the corresponding array entry above) so we're testing these separately still.

::: toolkit/components/passwordmgr/test/unit/test_context_menu.js:167
(Diff revision 3)
>        new LoginInfo("http://www3.example.com", "http://www.example.com", null,
>                      "", "password",
>                      "form_field_username", "form_field_password"),
>        new LoginInfo("http://www3.example.com", "http://www3.example.com", null,
>                      "", "password2",
>                      "form_field_username", "form_field_password"),

Ditto as example.org

Comment 10

[Matthew N. [:MattN]]

I don't think Saad is working on this anymore and I don't expect him to attach his follow-up patch at this point.

Comment 11

[Matthew N. [:MattN]]

Attachment: Bug 1200472 - Move includeOtherSubdomainsInLookup pref lookup to LoginHelper. r=sfoster

Comment 12

[Matthew N. [:MattN]]

Attachment: Bug 1200472 - Include subdomain login fill suggestions in the context menu. r=sfoster

Depends on D51353

Comment 13

[Matthew N. [:MattN]]

Attachment: macOS section heading style

I found that macOS uses grey text outdented into the gutter with a separator above for this type of UI so that is what I'm following there.

Comment 14

[Matthew N. [:MattN]]

Attachment: Bug 1200472 - Make LoginManagerParent._searchAndDedupeLogins public and static. r=sfoster

To allow easier access from LoginManagerContextMenu.jsm since it didn't rely on this anyways.

Depends on D51353

Comment 15

[Matthew N. [:MattN]]

Attachment: Bug 1200472 - Don't allow creating invalid logins with LTU.TestData helpers. r=sfoster

Reduce the change of accidentally creating invalid test cases.

Depends on D52629

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d38c2fbd306b
Move includeOtherSubdomainsInLookup pref lookup to LoginHelper. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/877779162b15
Make LoginManagerParent._searchAndDedupeLogins public and static. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/2e5826c18532
Don't allow creating invalid logins with LTU.TestData helpers. r=sfoster
https://hg.mozilla.org/integration/autoland/rev/c72321ba48b8
Include subdomain login fill suggestions in the context menu. r=sfoster

Comment 17

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/d38c2fbd306b
https://hg.mozilla.org/mozilla-central/rev/877779162b15
https://hg.mozilla.org/mozilla-central/rev/2e5826c18532
https://hg.mozilla.org/mozilla-central/rev/c72321ba48b8

Comment 18

[Matthew N. [:MattN]]

The UI addition will be reverted in bug 1599752 since bug 1595915 will remove the login submenu altogether. The majority of https://hg.mozilla.org/integration/autoland/rev/c72321ba48b8 is reverted while the other commits remain.