Matthew N. [:MattN] Assignee
	Description • 8 years ago

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.

	Johann Hofmann [:johannh]
	Comment 3 • 8 years ago
mozreview-review

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

	Matthew N. [:MattN] Assignee
	Comment 4 • 8 years ago
mozreview-review-reply

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.

	Pulsebot
	Comment 5 • 8 years ago

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

	Phil Ringnalda (:philor)
	Comment 6 • 8 years ago
bugherder

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