Closed Bug 1120129 Opened 9 years ago Closed 9 years ago

Allow per-site recipes to adjust the username/password field detection for filling

Categories

(Toolkit :: Password Manager, defect, P1)

defect
Points:
8

Tracking

()

RESOLVED FIXED
mozilla39
Iteration:
39.2 - 23 Mar
Tracking Status
firefox38 --- affected
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

from bug 1119454 comment 0:
* explicitly specifying the username or password field for a site (when our heuristics identify the wrong one)
* explicitly specifying what is *not* a username or password field (similar problem to above, but a different solution)
Blocks: 1120130
Cool. This is probably the most useful recipe.
See Also: → 1120874
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Iteration: --- → 38.3 - 23 Feb
Points: --- → 8
Flags: firefox-backlog+
Priority: -- → P1
Flags: qe-verify?
Flags: qe-verify? → qe-verify-
Iteration: 38.3 - 23 Feb → 39.1 - 9 Mar
Attached file MozReview Request: bz://1120129/MattN (obsolete) —
/r/4887 - Bug 1120129 - Allow per-site recipes to adjust the username/password field detection.

Pull down this commit:

hg pull review -r e2350fc85abe34a601b68944a9a89e2a12d6c30d
Comment on attachment 8573633 [details]
MozReview Request: bz://1120129/MattN

Working patch for feedback while I make tests
Attachment #8573633 - Flags: feedback?(dolske)
Iteration: 39.1 - 9 Mar → 39.2 - 23 Mar
Comment on attachment 8573633 [details]
MozReview Request: bz://1120129/MattN

/r/4887 - Bug 1120129 - Allow per-site recipes to adjust the username/password field detection. r=dolske

Pull down this commit:

hg pull review -r 558981cc3d07843229272dd32779300549bb7b7f
Attachment #8573633 - Flags: feedback?(dolske) → review?(dolske)
I'm still working on a m-bc test but I added unit tests already.
Attachment #8573633 - Flags: review?(dolske)
Comment on attachment 8573633 [details]
MozReview Request: bz://1120129/MattN

https://reviewboard.mozilla.org/r/4885/#review4139

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
(Diff revision 1)
> +    if (!pwFields) {

*ponder* This fallback makes sense, but otoh if we have a recipee that doesn't apply to a page, maybe we'd want that to be an explicit fail. Not sure if that's very compelling, though.

Also, we'll probably want to do something for telemetry to let us know that a recipee isn't working.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
(Diff revision 2)
> -      let parent = new LoginRecipesParent();
> +      let parent = new LoginRecipesParent(/* loadDefaults = */true);

This is a little gross (both having a boolean arg, and having to set it to get default behavior).

I suggest either a string (filename) or JS object (recipees), which specifies what to initialize with instead of the defaults. I suppose both could be supported concurrently by typechecking.

::: toolkit/components/passwordmgr/LoginRecipes.jsm
(Diff revision 2)
> -const OPTIONAL_KEYS = ["description", "pathRegex"];
> +const OPTIONAL_KEYS = ["description", "passwordSelector", "pathRegex", "usernameSelector"];

Hmm, I can already see REQUIRED/OPTIONAL keys having dubious value, seems like we're going to have few of the former and everything in the latter. We might want to just bake this into the logic of add() or a verification helper(). We'll see.

::: toolkit/components/passwordmgr/LoginRecipes.jsm
(Diff revision 2)
> +    if (recipeErrors) {

Why count, instead of just immediately failing?

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
(Diff revision 2)
> +      for (let pwField of pwOverrideFields) {

Seems like this should just be pwOverrideFields[0] (assuming it finds anything). I'm not sure how the extra fields from querySelectorAll would be useful... I think right now we will just want the single password field for fill/capture. Maybe in the future we'll want the ability to specify the extra fields involved in changing a password, but I'd suspect we'd want explicit equivalents for that. (eg passwordSelector + oldPasswordSelector + newPasswordVerifySelector).

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
(Diff revision 2)
> +    if (!pwFields) {

We should probably log something if we had a recipe (and this expected to find something), but didn't actually get the expected field.

The usual-path fallback is smart, but we'd want to know if something isn't working as designed.

(Might be worth a spinoff to consider how we could add this to telemetry, since ideally we'd like to know this without a developer having to enable logging, visiting a site, and staring at the console.)(

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
(Diff revision 2)
> +      var unOverrideField = form.ownerDocument.querySelector(

userOverrideField. And maybe passOverrideField.

unOverrideField is ungood.

::: toolkit/components/passwordmgr/LoginRecipes.jsm
(Diff revision 2)
> +  _filterRecipesForForm(aRecipes, aForm) {

s/ForForm/ForPage/?

We know we want to support formless logins at some point, so we can future-proof a bit here.

::: toolkit/components/passwordmgr/LoginRecipes.jsm
(Diff revision 2)
> +      "description": "okta uses a hidden password field to trick us",

Nitpicky: maybe we should try and keep descriptions a little more neutral. s/trick/disable saving passwords/?

::: toolkit/components/passwordmgr/test/unit/head.js
(Diff revision 2)
> +    let document = new Proxy(parsedDoc, {

Clever, although I'm a little wary of faking things out this way, since we're not really testing how code actually runs in the wild.

Maybe we could split the difference and test with a real (but blank) document loaded from the site(s) in our test framework, but inject the actual content we want instead of having static files?

Or maybe it's just not a big deal and I'm being overly paranoid.
Depends on: 1144456
https://reviewboard.mozilla.org/r/4885/#review4425

> Clever, although I'm a little wary of faking things out this way, since we're not really testing how code actually runs in the wild.
> 
> Maybe we could split the difference and test with a real (but blank) document loaded from the site(s) in our test framework, but inject the actual content we want instead of having static files?
> 
> Or maybe it's just not a big deal and I'm being overly paranoid.

I did try to use a real document and fetch it via XHR but those documents also have a null document.location. It's nice to be able to test this headless without a browser and that's why I went with this. I think mocking only document.location seems very low risk.

> Why count, instead of just immediately failing?

I was trying to avoid having one recipe blowing everything up e.g. if the client doesn't support a newer recipe rule. I think having the promise consistently resolve/reject when it's done loading is nice and useful for testing and waiting for the load to complete.

> *ponder* This fallback makes sense, but otoh if we have a recipee that doesn't apply to a page, maybe we'd want that to be an explicit fail. Not sure if that's very compelling, though.
> 
> Also, we'll probably want to do something for telemetry to let us know that a recipee isn't working.

Yeah, I was thinking we may want to add an option to recipes to require usage of the selectors in the future. I think in general falling back to regular heuristics makes sense in case the page changes and the selectors no longer match. I can easily add telemetry to know about any failures but it's harder if we want to correlate to the recipe that is failing.

> Hmm, I can already see REQUIRED/OPTIONAL keys having dubious value, seems like we're going to have few of the former and everything in the latter. We might want to just bake this into the logic of add() or a verification helper(). We'll see.

Yeah, we probably don't need REQUIRED_KEYS if there will only be one/two. I think I'll leave it for now since I don't think it hurts as a JSM-local const.

> This is a little gross (both having a boolean arg, and having to set it to get default behavior).
> 
> I suggest either a string (filename) or JS object (recipees), which specifies what to initialize with instead of the defaults. I suppose both could be supported concurrently by typechecking.

I originally was using an un-named object with destructuring but we don't support default values for those yet. I'll switch back to using a regular object. I can also switch the default if you prefer but like you say, I think this may change to a file name.

> We should probably log something if we had a recipe (and this expected to find something), but didn't actually get the expected field.
> 
> The usual-path fallback is smart, but we'd want to know if something isn't working as designed.
> 
> (Might be worth a spinoff to consider how we could add this to telemetry, since ideally we'd like to know this without a developer having to enable logging, visiting a site, and staring at the console.)(

Filed bug 1144456

> s/ForForm/ForPage/?
> 
> We know we want to support formless logins at some point, so we can future-proof a bit here.

We will still likely have a concept of forms (in the user sense) so I don't think this is a problem. I imagine we will have a form-like wrapper that acts similar to a <form>. i.e I don't think we're going to move to searching the whole page AFAIK.
Attachment #8573633 - Flags: review?(dolske)
Comment on attachment 8573633 [details]
MozReview Request: bz://1120129/MattN

/r/4887 - Bug 1120129 - Allow per-site recipes to adjust the username/password field detection for autofill. r=dolske

Pull down this commit:

hg pull review -r a58d7c21525526ecf2ab2f30af9be7a27bb97e53
Attachment #8573633 - Flags: review?(dolske) → review+
Comment on attachment 8573633 [details]
MozReview Request: bz://1120129/MattN

https://reviewboard.mozilla.org/r/4885/#review4583

Ship It!

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
(Diff revisions 1 - 3)
>          }

pwFields is always null at this point. So instead of [] + .push() you can just set it to pwFields = [{index : .... }].

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
(Diff revision 3)
> -      let parent = new LoginRecipesParent();
> +      let parent = new LoginRecipesParent({ defaults: true });

Yeah, I still think we should initialize with defaults without needing to specify any arguments.

|let parent = new LRP();| should just work.

::: toolkit/components/passwordmgr/LoginRecipes.jsm
(Diff revisions 1 - 3)
> +        throw new Error("'" + prop + "' must be a string");

What, no string template here? :)

::: toolkit/components/passwordmgr/LoginRecipes.jsm
(Diff revisions 2 - 3)
>   */

That seems backwards. Shouldn't it default to, well, defaults = true?
Hmm. Apparently intradiff comments don't get published usefully into Bugzilla.
Summary: Allow per-site recipes to adjust the username/password field detection → Allow per-site recipes to adjust the username/password field detection for filling
https://hg.mozilla.org/integration/fx-team/rev/4f28f9c87ba6
Flags: in-testsuite+
Whiteboard: [fixed-in-fx-team]
Blocks: 1101026
https://hg.mozilla.org/mozilla-central/rev/4f28f9c87ba6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla39
Attachment #8573633 - Attachment is obsolete: true
Attachment #8619099 - Flags: review+
You need to log in before you can comment on or make changes to this bug.