Myk Melez [:myk] [@mykmelez] Assignee
	Description • 15 years ago

When the login manager doesn't fill a login form because autofill is off or the form is disabled, it notifies observers.  But when the login manager does fill a login form or doesn't fill a form because there are multiple possible logins for the form, it doesn't notify observers.

For some use cases, however, like Weave's new chrome auth integration, it would be useful to know about all those cases, since Weave's chrome auth integration generates a list of available logins whether or not the form ends up getting filled out.

Here's where the login manager decides to fill a login form:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/src/nsLoginManager.js#1158

Here's where it decides not to fill a form because there are multiple logins:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/src/nsLoginManager.js#1153

The ideal implementation would be a notification that provides all the information _fillForm was able to gather, including the username/password fields it detected, the set of found logins for the form, and the selected login.

A notification that only provides the form (as do the current notifications) might work, since Weave could use it to get logins via findLogins, although that wouldn't tell it which was the selected login (which is useful to know, since Weave can use it to pick a default login from the list) or what the username/password fields are (which Weave needs to know in order to fill out the form, and which are determined by complex heuristics within the login manager).

This would be really useful to have in Firefox 3.5, since it would obviate the need for Weave to ship a bunch of duplicate code from the login manager (and track fixes to that code, etc., i.e. all the pain that duplicate code entails).  Thus requesting blocking status for that release.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 1 • 15 years ago

Myk: sounds like you know what needs to be fixed, can you start working on a patch (that doesn't change interfaces)?

	Myk Melez [:myk] [@mykmelez] Assignee
	Comment 2 • 15 years ago

(In reply to comment #1)
> Myk: sounds like you know what needs to be fixed, can you start working on a
> patch (that doesn't change interfaces)?

Is it possible to add an interface so long as I don't change existing ones in the process?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 3 • 15 years ago

yep. In fact we're already doing this for nsILoginManager, see bug 489876.

	Myk Melez [:myk] [@mykmelez] Assignee
	Comment 4 • 15 years ago

Ok, I'll take this on and see if I can put together a patch that is low risk enough to land in 1.9.1.

The extra interface to which I am referring is a kind of "form info" interface that provides the extra information about the form that is described in comment 0.  However, it might be possible to provide this information without adding a new interface if we're willing to sacrifice the ability for native code to handle the notification.  I'll see what I can do.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 5 • 15 years ago

Hey Myk, I'm highly motivated to take this code, so please poke me when you need someone for reviews or whatnot, but I can't actually block the release on it (as much as I agree with its intent!)

	Myk Melez [:myk] [@mykmelez] Assignee
	Comment 6 • 15 years ago

Here's a patch that adds a "found logins" notification to the login manager to notify observers when the login manager finds one or more logins that can be used to fill a form, whether or not it actually fills the form with them.  Observers can then use that information to fill in the form themselves.

I've included MochiTest tests for the functionality, and I added a patch to bug 492725 that makes Weave use this notification instead of all the login manager code I had previously copied into it to make its chrome-based authentication feature work.

	Myk Melez [:myk] [@mykmelez] Assignee
	Comment 7 • 15 years ago

(In reply to comment #5)
> Hey Myk, I'm highly motivated to take this code, so please poke me when you
> need someone for reviews or whatnot, but I can't actually block the release on
> it (as much as I agree with its intent!)

FWIW, I figured as much and actually requested "blocking" because the last time I hacked on core Firefox code the way to request "wanted" was to request "blocking" and let drivers decide that the bug is "wanted" rather than "blocking".  That was a while ago, though; I suppose the process could be very different now.

	Justin Dolske [:Dolske]
	Comment 8 • 15 years ago

Comment on attachment 377124 [details] [diff] [review]
patch v1: adds "found logins" notification to login manager

Just a few small things to fix... Overall patch looks great.

>--- a/toolkit/components/passwordmgr/src/nsLoginManager.js
...
>             if (matchingLogins.length == 1)
>                 selectedLogin = matchingLogins[0];
>-            else
>+            else {
>+                didntFillReason = "multipleLogins";
>                 this.log("Multiple logins for form, so not filling any.");
>+            }

The 1-line if-block should get brackets now that the else-block does.

Also, I think you missed a case above this...

            if (matchingLogins.length)
                selectedLogin = matchingLogins[0];
            else
                this.log("Password not filled. None of the stored " +
                         "logins match the username already present.");

I think you want to set |didntFillReason = "existingUsername";| here. And a test for it. :)

>+        this._notifyFoundLogins(didntFillReason, usernameField, passwordField,
>+                                foundLogins, selectedLogin);

I'm slightly wary about passing around logins in a notification, but I think that's just unfounded paranoia. Anyone who can get the notification could just pull the data from pwmgr directly.

>+     * @param didntFillReason {String}
>+     *        the reason the login manager didn't fill the form, if any;
>+     *        otherwise null; possible values are:

This should probably be phrased more directly -- if didntFillReason is null, then it implies the form *was* filled.

>+      let formInfo = {
>+        wrappedJSObject: {
>+          didntFillReason: didntFillReason,
>+          usernameField:   usernameField,
>+          passwordField:   passwordField,
>+          foundLogins:     foundLogins,
>+          selectedLogin:   selectedLogin
>+        }
>+      };

Hmm. If the caller modifies the array with .splice(), login manager would see that change too? Which would probably be unintentional breakage... Clone the array like:

    foundLogins: foundLogins.concat()

Maybe I'm being overly paranoid, but it shouldn't hurt...


>+++ b/toolkit/components/passwordmgr/test/test_basic_form_observer_foundLogins.html
...
>+function startTest(){
>+  netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
>+
>+  // usernameField/passwordField comparisons have to use ok() rather than is()
>+  // because is() calls MochiKit::Base::repr() on its arguments, which does
>+  // something that causes Mozilla to throw NS_ERROR_XPC_SECURITY_MANAGER_VETO.

Huh. That's... Odd. Can you file an xpconnect bug with a smaller testcase? Sounds like the kind of thing mrbkap would be interested in. :)

>+  // Test notification of a form that wasn't filled because autocomplete is off.
>+  // These tests also mostly test the codepaths resulting in notifications about
>+  // "multipleLogins" and "noAutoFillForms".

Could you explicitly test these cases too? They share some common code now, but lest that ever change it's unlikely someone will remember to update this test.

	Justin Dolske [:Dolske]
	Comment 9 • 15 years ago

Also, how about just using a nsIPropertyBag[2] for the notification subject? That would avoid the .wrappedJSObject oddness, though at the cost of needing a little more XPCOM glue.

	Myk Melez [:myk] [@mykmelez] Assignee
	Comment 10 • 15 years ago

(In reply to comment #8)
> The 1-line if-block should get brackets now that the else-block does.

Ok, done.


> I think you want to set |didntFillReason = "existingUsername";| here. And a
> test for it. :)

Yup, I've added the code and test.


> >+        this._notifyFoundLogins(didntFillReason, usernameField, passwordField,
> >+                                foundLogins, selectedLogin);
> 
> I'm slightly wary about passing around logins in a notification, but I think
> that's just unfounded paranoia. Anyone who can get the notification could just
> pull the data from pwmgr directly.

Indeed!


> >+     * @param didntFillReason {String}
> >+     *        the reason the login manager didn't fill the form, if any;
> >+     *        otherwise null; possible values are:
> 
> This should probably be phrased more directly -- if didntFillReason is null,
> then it implies the form *was* filled.

Ok, I've phrased it more directly as "the reason the login manager didn't fill the form, if any; if the value of this parameter is null, then the form was filled."


> Hmm. If the caller modifies the array with .splice(), login manager would see
> that change too? Which would probably be unintentional breakage... Clone the
> array like:
> 
>     foundLogins: foundLogins.concat()
> 
> Maybe I'm being overly paranoid, but it shouldn't hurt...

Yup, the code now does so.


> Huh. That's... Odd. Can you file an xpconnect bug with a smaller testcase?
> Sounds like the kind of thing mrbkap would be interested in. :)

I shall do so as soon as I can reduce it.  My first attempt has failed, although it does cause a crash, which is almost as good.

In any case, the problem disappeared from the password manager tests once I switched to passing a property bag, so I've removed the comment from the tests.


> Could you explicitly test these cases too? They share some common code now, 
> but lest that ever change it's unlikely someone will remember to update this
> test.

Yup, the tests now test all cases independently.  The autofillForms case is in test_basic_form_observer_autofillForms.html, because it's much easier to implement it there, while the rest are in test_basic_form_observer_foundLogins.html.


(In reply to comment #9)
> Also, how about just using a nsIPropertyBag[2] for the notification subject?
> That would avoid the .wrappedJSObject oddness, though at the cost of needing a
> little more XPCOM glue.

Ok, the code now uses an nsI(Writable)PropertyBag2.

	Justin Dolske [:Dolske]
	Comment 11 • 15 years ago

Comment on attachment 378302 [details] [diff] [review]
patch v2: fixes review comments

>--- a/toolkit/components/passwordmgr/src/nsLoginManager.js
...
>+            }
>+            else {

Nit in a couple places: } else {

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 12 • 15 years ago

Comment on attachment 378302 [details] [diff] [review]
patch v2: fixes review comments

a191=beltzner

	Justin Dolske [:Dolske]
	Comment 13 • 15 years ago

Pushed http://hg.mozilla.org/mozilla-central/rev/21e21853891e

	Justin Dolske [:Dolske]
	Comment 14 • 15 years ago

Pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bab74c06bc10