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

NEW
Unassigned

Status

()

Toolkit
Password Manager
P2
enhancement
2 years ago
7 months ago

People

(Reporter: ckarlof, Unassigned, Mentored)

Tracking

(Depends on: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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.
Severity: normal → enhancement
Summary: Broaden search criteria for password fill suggestions shown in the context menu affordance "Fill Password" → Broaden search criteria for password fill suggestions shown in the context menu affordance "Fill Password" to include subdomains.
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 589628
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.
Assignee: nobody → saad
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Status: REOPENED → ASSIGNED
Priority: -- → P2
Depends on: 1294194
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

9 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
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?
Attachment #8784163 - Flags: review?(MattN+bmo)
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
Attachment #8784074 - Flags: review?(MattN+bmo)
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.
Assignee: saad → nobody
Mentor: MattN+bmo@mozilla.com
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.