Helper to deduplicate password manager logins

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: rittme, Assigned: rittme)

Tracking

Trunk
mozilla42
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

4 years ago
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.
Assignee

Updated

4 years ago
Assignee: nobody → bernardo
Blocks: 433238
Status: NEW → ASSIGNED
Flags: qe-verify-
Assignee

Comment 1

4 years ago
Bug 1188456 - Helper to deduplicate password manager logins. r?MattN
Attachment #8640856 - Flags: review?(MattN+bmo)
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.
Attachment #8640856 - Flags: review?(MattN+bmo) → review+
Assignee

Updated

4 years ago
Iteration: --- → 42.3 - Aug 10
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
Assignee

Updated

4 years ago
Attachment #8640856 - Attachment description: MozReview Request: Bug 1188456 - Helper to deduplicate password manager logins. r?MattN → MozReview Request: Bug 1188456 - Helper to deduplicate password manager logins. r=MattN
Attachment #8640856 - Flags: review+ → review?(MattN+bmo)
Assignee

Comment 3

4 years ago
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 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!
Attachment #8640856 - Flags: review?(MattN+bmo) → review+
Assignee

Updated

4 years ago
Keywords: checkin-needed
Please provide a Try link.
Keywords: checkin-needed
Assignee

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d6b5e49c24b6
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.