1188456 - Helper to deduplicate password manager logins

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Bernardo Rittmeyer [:rittme]]

When getting every login for a host name we can have duplicate logins, because we don't use formSubmitURL, which is one of the keys for the search. We need a helper method to remove this duplicate logins.

Comment 1

[Bernardo Rittmeyer [:rittme]]

Attachment: MozReview Request: Bug 1188456 - Helper to deduplicate password manager logins. r=MattN

Bug 1188456 - Helper to deduplicate password manager logins. r?MattN

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8640856 [details]
MozReview Request: Bug 1188456 - Helper to deduplicate password manager logins. r=MattN

https://reviewboard.mozilla.org/r/14379/#review12999

::: toolkit/components/passwordmgr/LoginHelper.jsm:265
(Diff revision 1)
> +   * @param {Array} loginsList

Nit: @param {nsILoginInfo[]} logins

gives the type and I don't think "list" adds value (in fact, it could be confused with some "List" type).

::: toolkit/components/passwordmgr/LoginHelper.jsm:271
(Diff revision 1)
> +   * @returns {Array} list of unique logins.

{nsILoginInfo[]}

::: toolkit/components/passwordmgr/LoginHelper.jsm:276
(Diff revision 1)
> +    //Generate a unique key string from a login

Nit: missing space after //

::: toolkit/components/passwordmgr/LoginHelper.jsm:285
(Diff revision 1)
> +      // If an equivalent login exists and is older, we don't want to keep the current one.

I interpret this comment to mean something different than you intended because I interpret "current" to mean the one that we are currently looking at (aka `login`) but you mean the one currently in the Map. I think it may be clearer like:
If we find a more recently used login for the same key, replace the existing one.

::: toolkit/components/passwordmgr/LoginHelper.jsm:287
(Diff revision 1)
> +        let newDate = login.QueryInterface(Ci.nsILoginMetaInfo).timeLastUsed;

Using "new" as a variable substring in code dealing with dates where "new" isn't referring to the dates being compared isn't ideal. How about `loginDate`?

::: toolkit/components/passwordmgr/LoginHelper.jsm:268
(Diff revision 1)
> +   * @param {Array} uniqueKeys

Nit: {string[] = ["username", "password"]}

::: toolkit/components/passwordmgr/LoginHelper.jsm:273
(Diff revision 1)
> +   removeDuplicateLogins(loginsList, uniqueKeys = ["username", "password"]) {
> +    const KEY_DELIMITER = ":";

Indentation seems wrong here. It looks like 3 spaces.

::: toolkit/components/passwordmgr/LoginHelper.jsm:272
(Diff revision 1)
> +   */
> +   removeDuplicateLogins(loginsList, uniqueKeys = ["username", "password"]) {

Technically you're not removing the logins and are instead returning a new array. How about "dedupeLogins"?

::: toolkit/components/passwordmgr/test/unit/test_logins_change.js:66
(Diff revision 1)
> + */
> +function assertLoginsEqualsByKeys(loginA, loginB, keys) {

`keys` isn't documented

::: toolkit/components/passwordmgr/test/unit/test_logins_change.js:361
(Diff revision 1)
> +add_task(function test_deduplicate_keep_most_recent() {

Nit: s/keep/keeps/

::: toolkit/components/passwordmgr/test/unit/test_logins_change.js:358
(Diff revision 1)
> +/**
> + * Ensure that the login deduplication function keeps the most recent login
> + */
> +add_task(function test_deduplicate_keep_most_recent() {

This test doesn't really prove that the expected result wasn't just by chance of implemenation (e.g. returning the earlier or latest in the array). I think you should add checks in the same function that passing the same 2 logins (assign them to variables) in the opposite order also still gives you the most recent one. Assuming your function is deterministic, it gives assurance that the login that's kept isn't related to the order of the original array.

::: toolkit/components/passwordmgr/LoginHelper.jsm:296
(Diff revision 1)
> +    return [... loginsByKeys.values()];

Nit: we don't normally put a space after `...` for whatever reason. 219 vs. 5 in /browser/

::: toolkit/components/passwordmgr/test/unit/test_logins_change.js:67
(Diff revision 1)
> +function assertLoginsEqualsByKeys(loginA, loginB, keys) {

This function doesn't actually `Assert`, it only tests.

Comment 3

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8640856 [details]
MozReview Request: Bug 1188456 - Helper to deduplicate password manager logins. r=MattN

Bug 1188456 - Helper to deduplicate password manager logins. r=MattN

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8640856 [details]
MozReview Request: Bug 1188456 - Helper to deduplicate password manager logins. r=MattN

https://reviewboard.mozilla.org/r/14379/#review13017

Ship It!

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Please provide a Try link.

Comment 6

[Bernardo Rittmeyer [:rittme]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=57ec9e9afe83

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d6b5e49c24b6

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d6b5e49c24b6