Closed Bug 1188456 Opened 5 years ago Closed 5 years ago

Helper to deduplicate password manager logins

Categories

(Toolkit :: Password Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Iteration:
42.3 - Aug 10
Tracking Status
firefox42 --- fixed

People

(Reporter: rittme, Assigned: rittme)

References

Details

Attachments

(1 file)

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: nobody → bernardo
Blocks: 433238
Status: NEW → ASSIGNED
Flags: qe-verify-
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+
Iteration: --- → 42.3 - Aug 10
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog+
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)
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+
Keywords: checkin-needed
Please provide a Try link.
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d6b5e49c24b6
Status: ASSIGNED → RESOLVED
Closed: 5 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.