Closed Bug 1330829 Opened 3 years ago Closed 3 years ago

Login Manager recipe to support ignoring specific password fields

Categories

(Toolkit :: Password Manager, enhancement)

52 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

(Blocks 1 open bug)

Details

(Whiteboard: [passwords:recipes])

Attachments

(1 file)

Some sites use useless type=password fields to throw off password manager, other sites use type=password fields for things that aren't password fields e.g. credit cards or one-time use pins which the user doesn't want to save in password manager.

I propose adding a notPasswordSelector recipe option that works like notUsernameSelector to filter out these fields we don't want to consider.
Comment on attachment 8826425 [details]
Bug 1330829 - Login Manager recipe support for ignoring specific password fields.

https://reviewboard.mozilla.org/r/104368/#review105134

Seems pretty straightforward. dholbert has tried it out (https://bugzilla.mozilla.org/show_bug.cgi?id=1330810#c6) and I'll rely on him to know that this works in practice.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:592
(Diff revision 2)
>     * @param {FormLike} form - the FormLike to look for password fields in.
> -   * @param {bool} [skipEmptyFields=false] - Whether to ignore password fields with no value.
> -   *                                         Used at capture time since saving empty values isn't
> -   *                                         useful.
> +   * @param {Object} options
> +   * @param {bool} [options.skipEmptyFields=false] - Whether to ignore password fields with no value.
> +   *                                                 Used at capture time since saving empty values isn't
> +   *                                                 useful.
> +   * @param {Object} [options.fieldOverrideRecipe=null] - A relevant field override recipe to use.

I really like that options documentation style. We should use it in more places.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:614
(Diff revision 2)
>  
> +      // Exclude ones matching a `notPasswordSelector`, if specified.
> +      if (fieldOverrideRecipe && fieldOverrideRecipe.notPasswordSelector &&
> +          element.matches(fieldOverrideRecipe.notPasswordSelector)) {
> +        log("skipping password field (id/name is", element.id, " / ",
> +            element.name + ") due to recipe:", fieldOverrideRecipe);

nit: The combination of "," and "+" concatenating looks a bit random here, can't you replace the single "+" with a ","?

::: toolkit/components/passwordmgr/LoginRecipes.jsm:17
(Diff revision 2)
> +  "description",
> +  "notPasswordSelector",
> +  "notUsernameSelector",
> +  "passwordSelector",
> +  "pathRegex",
> +  "usernameSelector"

nit: Since you've added trailing commas in most other parts of your patch, I assume you forgot to add one here :)

::: toolkit/components/passwordmgr/test/mochitest/test_recipe_login_fields.html:141
(Diff revision 2)
> +add_task(function* loadNotPasswordSelectorRecipes() {
> +  yield resetRecipes();
> +  yield loadRecipes({
> +    siteRecipes: [{
> +      hosts: ["mochi.test:8888"],
> +      notPasswordSelector: "input[name='not_pword'], input[name='not_pword2']"

nit: trailing comma
Attachment #8826425 - Flags: review?(jhofmann) → review+
Comment on attachment 8826425 [details]
Bug 1330829 - Login Manager recipe support for ignoring specific password fields.

https://reviewboard.mozilla.org/r/104368/#review105134

> nit: The combination of "," and "+" concatenating looks a bit random here, can't you replace the single "+" with a ","?

The problem is that I didn't want a space before the ")" and spaces get added between arguments.

> nit: Since you've added trailing commas in most other parts of your patch, I assume you forgot to add one here :)

Yep, I like trailing commas.
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc4d5c2cade3
Login Manager recipe support for ignoring specific password fields. r=johannh
https://hg.mozilla.org/mozilla-central/rev/fc4d5c2cade3
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.