382437 - nsLoginManagerPrompter.js needs to implement nsIAuthPrompt

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P3)

Description

[Wolfgang Rosenauer [:wolfiR]]

nsLoginManagerPrompter.js needs to implement nsIAuthPrompt

I've tried to convert mailnews to use the new login manager but I can't easily switch it to nsIAuthPrompt2 and according to biesi the login manager prompter should (or has to) implement nsIAuthPrompt in addition to nsIAuthPrompt2 (please correct me if I'm wrong)

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

More specifically, it should have an implementation of nsIAuthPromptWrapper under the contract ID @mozilla.org/wallet/single-sign-on-prompt;1

Comment 2

[Justin Dolske [:Dolske]]

Attachment: WIP diff to nsLoginManagerPrompter.js

Here's a untested, incomplete WIP for implementing nsIAuthPrompt. While the prompts should be enough for using authentication, more code would need to be added to actually save logins (and prefill the prompts with info from existing logins).

I'm not sure I understand how/if nsIAuthPromptWrapper needs to be involved here. Looking at the 1.8 code, all that seems to so is add a setPromptDialogs() method, which is more or less what the current code does by using nsIPromptService. It seems just be just adding another layer of indirection, so my inclination would be to rip all that stuff in 1.9. Maybe I don't understand what it's doing, though.

Comment 3

[Mark Banner (:standard8)]

I've just been looking at the patch wrt SeaMonkey. I've attached a patch to bug 239131 (attachment 281872 [details] [diff] [review]) that will allow building SeaMonkey (and hopefully Thunderbird) with at least some (if not all) of the required changes to use toolkit's login manager instead of wallet.

In the prompt and promptUsernameAndPassword I did notice that:

var checkBox = { value : false }:

should have a semi-colon on the end, not the colon.

In the quick test that I did (with attachment 279233 [details] [diff] [review] and 281872 applied), it seemed like the mailnews password dialogs always came up even when the password was stored, and in neither case did they have the save password to password manager option.

I haven't yet had time to look at why this could be in any detail. For some pointers as to what the code is trying to do see the diff in attachment 281872 [details] [diff] [review] for nsMsgIncomingServer.cpp, and also http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/addrbook/src/nsAbLDAPListenerBase.cpp&rev=1.6&mark=184-246#184 for what I believe are the main two examples of how these are accessed.

Hopefully, I'll get chance to look in the next few days and see if its a problem with toolkit's pm, or how SeaMonkey/Thunderbird are picking it up.

Comment 4

[Mark Banner (:standard8)]

I've just had another look at this. Both instances in mailnews that I've looked at (see comment 3) use the window watcher to get the auth prompt. Digging into the code a little I've found the following two functions:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/embedding/components/windowwatcher/src/nsPrompt.cpp&rev=1.30&mark=80-143#80

NS_NewAuthPrompter and NS_NewAuthPrompter2 differ. NS_NewAuthPrompter2 seems to be using the password manager if its present, but NS_NewAuthPrompter tries to do it itself partially, or uses wallet's version instead...

I'm going to try copying the password manager "selection" code from the start of NS_NewAuthPrompter2 to the start of NS_NewAuthPrompter and see if this helps the problem.

Comment 5

[Mark Banner (:standard8)]

Attachment: Patch for nsPrompt.cpp for NS_NewAuthPrompter

This patch fixes NS_NewAuthPrompter to be like NS_NewAuthPrompter2 and hence will try to use the password manager's auth prompt factory if it exists.

This means we now get the save password check boxes in mailnews. They don't work at the moment because of the missing code in the nsLoginManagerPrompter patch (see the XXX items), but its a good start.

Comment 6

[Mark Banner (:standard8)]

Attachment: Patch for nsPrompt.cpp for NS_NewAuthPrompter v2

Better patch for nsPrompt.cpp - allows to work with older password managers (e.g. wallet) that don't support the GetPrompt interface with the nsIAuthPrompt interface.

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 281944 [details] [diff] [review]
Patch for nsPrompt.cpp for NS_NewAuthPrompter v2

Bug 397403 now deals with this change to nsPrompt.cpp. This bug will deal exclusively with the prompter changes.

Comment 8

[Mark Banner (:standard8)]

Attachment: WIP diff to nsLoginManagerPrompter.js

With the work I've been doing in the other dependent/blocking bugs, and Justin's original patch. I've now been able to get the promptPassword function working for at least LDAP address books.

The promptPassword will put the ldap url into hostname, and then extract the hostname and put that in httpRealm. Password manager requires that at least one of httpRealm or formSubmitURL is filled in, so I thought that httpRealm was best.

Currently if the promptPassword finds a password for the url passed in, then it will return immediately with the password. This mimics the current mailnews behaviour.

I'm also not dealing with the info bar as I'm not sure that's appropriate for nsIAuthPrompt implementations.

Any comments will be gratefully received.

Comment 9

[Justin Dolske [:Dolske]]

Comment on attachment 282606 [details] [diff] [review]
WIP diff to nsLoginManagerPrompter.js

Drive-by!


>+    /*
>+     * promptPassword
>+     *
>+     * Displays a prompt with a password field and ok/cancel buttons.
>+     */
>+    promptPassword : function (dialogTitle, text, passwordRealm,
>+                                         savePassword, pwd) {

Arg names here should be "aFoo" style instead of "foo". [aDialogTitle, aText, etc].

Also, "aPassword" instead of "pwd". Add comments noting expected format/value.

>+        if (!pwd || pwd != "") {

This expression doesn't make sense to me, isn't it always true?

>+            // Look for existing logins.
>+            var foundLogins = this._pwmgr.findLogins({}, passwordRealm, null,
>+                                                     host);

Variables names should be changed so the arguments are are not confusing. The last arg is a "realm" and the second arg is a "host", so passing in variables with these would look wrong at first glance.

In a change I'm making for another bug, I'm using realm == hostname if there isn't a more appropriate realm value available.

>+            // XXX Like the original code, we can't deal with multiple
>+            // account selection. (bug 227632)
>+            if (foundLogins.length > 0) {
>+                // We've got a login, so just return straight away like the old
>+                // wallet did.
>+                pwd.value = foundLogins[0].password;
>+                return true;
>+            }
>+        }

This seems wrong. If the caller just wants to automatically log in, without prompting the user, it should be calling password manager directly instead of promptPassword.

>+        if (ok && checkbox.value) {
>+            var newLogin = Cc["@mozilla.org/login-manager/loginInfo;1"]
>+                                .createInstance(Ci.nsILoginInfo);

Formatting style nit:

.            var newLogin = Cc["@mozilla.org/login-manager/loginInfo;1"].
.                           createInstance(Ci.nsILoginInfo);


>+            newLogin.init(passwordRealm,
>+                            null, host,
>+                            "", pwd.value,
>+                            "", "");

Same comment about variable names applies here.

Also, does the caller not actually not know a username at this point?


>+    /**
>+     * Extracts a hostname from a string based URI via a standard URI
>+     * implementation.
>+     */
>+    _getHost: function (aURIString) {
>+        var uri = Cc["@mozilla.org/network/standard-url;1"]
>+            .createInstance(Ci.nsIURI);
>+
>+        try {
>+          uri.spec = aURIString;
> 
>+          return uri.host;
>+        }
>+        catch (ex) {
>+          return "";
>+        }
>+    }

This should probably just use this._ioService.newURI(uriString, null, null), as in nsLoginManager.js. Not sure about the failure modes here, and if "" is the right thing to return.

Comment 10

[Mark Banner (:standard8)]

(In reply to comment #9)
> (From update of attachment 282606 [details] [diff] [review])
> Drive-by!
Thanks.

> >+        if (!pwd || pwd != "") {
> 
> This expression doesn't make sense to me, isn't it always true?

Yes, I was looking at the wrong interface definition. The nsIPrompt interface implies inout for pwd, whereas the nsIAuthPrompt doesn't.

> >+            // Look for existing logins.
> >+            var foundLogins = this._pwmgr.findLogins({}, passwordRealm, null,
> >+                                                     host);
> 
> Variables names should be changed so the arguments are are not confusing. The
> last arg is a "realm" and the second arg is a "host", so passing in variables
> with these would look wrong at first glance.
...
> In a change I'm making for another bug, I'm using realm == hostname if there
> isn't a more appropriate realm value available.

In which case should we update the idl file as well? At the moment the argument names used reflect those in the idl file, so its going to be confusing to users/editors if we don't.

> >+            // XXX Like the original code, we can't deal with multiple
> >+            // account selection. (bug 227632)
> >+            if (foundLogins.length > 0) {
> >+                // We've got a login, so just return straight away like the old
> >+                // wallet did.
> >+                pwd.value = foundLogins[0].password;
> >+                return true;
> >+            }
> >+        }
> 
> This seems wrong. If the caller just wants to automatically log in, without
> prompting the user, it should be calling password manager directly instead of
> promptPassword.

The existing "wallet" password manager implements the same interfaces, and handles them in this manner. If we're going to change it, then I don't know how we handle it from an interface perspective. Though I do agree the current documentation is unclear as to what exactly happens with the prompts.

If password manager doesn't return here, then I'd be tempted to remove its check for a log in completely. The reason being is that if the password doesn't exist, we're going to be making the check for a password twice which just seems silly, and we're going to have to duplicate more code wherever we throw up a dialog prompt to first check for an existing password.

> 
> >+        if (ok && checkbox.value) {
> >+            var newLogin = Cc["@mozilla.org/login-manager/loginInfo;1"]
> >+                                .createInstance(Ci.nsILoginInfo);
> 
> Formatting style nit:
> 
> .            var newLogin = Cc["@mozilla.org/login-manager/loginInfo;1"].
> .                           createInstance(Ci.nsILoginInfo);
> 
I was following a similar style to the rest of the nsLoginManagerPrompter code... but I'll change it anyway.

> >+            newLogin.init(passwordRealm,
> >+                            null, host,
> >+                            "", pwd.value,
> >+                            "", "");
> 
...
> Also, does the caller not actually not know a username at this point?

No, the way mailnews has always worked is to not put a username into the prompt dialog/password mananger (its typically stored in prefs). Additionally, according to the interface: http://mxr.mozilla.org/seamonkey/source/netwerk/base/public/nsIAuthPrompt.idl there is no provision for a username to be provided in this function.

Comment 11

[Justin Dolske [:Dolske]]

(In reply to comment #10)

> In which case should we update the idl file as well? At the moment the argument
> names used reflect those in the idl file, so its going to be confusing to
> users/editors if we don't.

Maybe, I'm mostly just trying to stop the spread of ambiguous "realm" usage. Fixing any existing usage is bonus.

> The existing "wallet" password manager implements the same interfaces, and
> handles them in this manner. If we're going to change it, then I don't know how
> we handle it from an interface perspective.

The interface isn't frozen, so I think it's fine to change the behavior for 1.9 to do the sensible thing (always prompt the user).

Hmm. So, I think what we actually need here is a fix for bug 223636. This would allow for automatic (non-prompted) logins, which is what I think the intention really is here. So, it's probably best to leave this as you have it, and we'll change the behavior with 223636.

[This might also help with the problem of saving a password before you know if it works or not.]

> > Also, does the caller not actually not know a username at this point?
> 
> No, the way mailnews has always worked is to not put a username into the

This seems like a problem. What if I want to check mail in two different Gmail accounts? The stored logins would both have blank username fields.

Comment 12

[Mark Banner (:standard8)]

(In reply to comment #9)
> >+            // Look for existing logins.
> >+            var foundLogins = this._pwmgr.findLogins({}, passwordRealm, null,
> >+                                                     host);
> 
> Variables names should be changed so the arguments are are not confusing. The
> last arg is a "realm" and the second arg is a "host", so passing in variables
> with these would look wrong at first glance.

I can answer this now. I intentionally swapped the arguments round.

The way mailnews and wallet currently works, is to store the url in what login manager sees as the "hostname". For example, the "Site" for an ldap directory will look like:

ldap://localhost:389/dc=test??sub?(objectclass=*)

For a "normal" pop3 (at least) mailbox, it will look like:

mailbox://bugzilla@localhost

So if I pass in the hostname in the "correct" place, then we'll end up with

localhost (mailbox://bugzilla@localhost)
localhost (ldap://localhost:389/dc=test??sub?(objectclass=*)

stored in the password manager.

This gives two problems:

1) findLogins() with the same parameters doesn't find a login.
2) migrated logins will not match for mailnews.

I'll attach a patch in a mo which has a commented out version that I tried.

(In reply to comment #11)
> > > Also, does the caller not actually not know a username at this point?
> > 
> > No, the way mailnews has always worked is to not put a username into the
> 
> This seems like a problem. What if I want to check mail in two different Gmail
> accounts? The stored logins would both have blank username fields.

You can probably pick this up from the above, but the general way mailnews works is to store which account has the password. The username is stored alongside the account details, and sometimes (but not always) in the password manager.

Comment 13

[Mark Banner (:standard8)]

Attachment: WIP diff to nsLoginManagerPrompter.js (v2a)

Slightly updated patch.

I've added comments about what the functions are going to do (based on wallet's current implementation, not its comments).

I've also started to address some of Justin's comments, but I've not completed the ones we've discussed so far.

This is mainly so I could continue the discussion about which login parameters should be used where.

Comment 14

[Justin Dolske [:Dolske]]

(In reply to comment #12)

> The way mailnews and wallet currently works, is to store the url in what login
> manager sees as the "hostname". For example, the "Site" for an ldap directory
> will look like:
> 
> ldap://localhost:389/dc=test??sub?(objectclass=*)

Where's "localhost" coming from? Or are you just using this as an example host name (that's confusing).

> This gives two problems:
> 
> 1) findLogins() with the same parameters doesn't find a login.
> 2) migrated logins will not match for mailnews.

#1 shouldn't be a problem as long as you're using the same format everywhere, unless I'm not understanding the problem you're describing. 

#2 can be fixed along with bug 379111 (ie, it can piggyback along on the code for that fix). Not storing the scheme for a login can potentially be a security issue.

> You can probably pick this up from the above, but the general way mailnews
> works is to store which account has the password. The username is stored
> alongside the account details, and sometimes (but not always) in the password
> manager.
> 

Comment 15

[Mark Banner (:standard8)]

(In reply to comment #14)
> (In reply to comment #12)
> 
> > The way mailnews and wallet currently works, is to store the url in what login
> > manager sees as the "hostname". For example, the "Site" for an ldap directory
> > will look like:
> > 
> > ldap://localhost:389/dc=test??sub?(objectclass=*)
> 
> Where's "localhost" coming from? Or are you just using this as an example host
> name (that's confusing).

I was using localhost as an example host. So

ldap://mozilla.com:389/dc=test??sub?(objectclass=*)

could be a valid one as well (if there was a server there ;-) ). Note that the ldap url will change if you change any of the settings.

What I have also just realised is that if you create an account in mailnews for demon (for example) you'd get a password mananger login for:

mailbox://[e-mail]

you later switch server to somewhere else and mailnews continues using the "mailbox://[e-mail]" login from the password manager (I'm not quite sure how this all fits together or even works, but it appears to be what we're doing).

This mailbox handling does seem a bit of a bug in itself/more confusion than necessary.

Comment 16

[Mark Banner (:standard8)]

(In reply to comment #15)
> This mailbox handling does seem a bit of a bug in itself/more confusion than
> necessary.

Sorry for the additional message.

I'm just starting to think that we should perhaps be doing something sensible here. i.e. store the scheme and hostname in the normal place, along with the username and password - use the expected format.

So, for instance, if you change server or username, then the password manager won't find the login and look for a new one. It'd probably also be clearer for the user when looking in the password manager about what is going on where.

We could then fix mailnews to cope with the new/correct format.

The problem here would be the migration from current versions to the latest. We'd either have to have extra code in mailnews for falling back to the old way (initially at least), or do something in toolkit's password mananger for the changing entries.

The first way might not be too bad, I'd just prefer not to break and annoy existing users that are upgrading by saying, "your password is still there, just go find it in password manager and enter it again".

Comment 17

[Mark Banner (:standard8)]

Attachment: WIP diff to nsLoginManagerPrompter.js (v3)

The latest version. This should almost be complete. I'm currently working on a way of testing these separate to mailnews as unit/mochitests can't cope with new windows. I think it may end up being a "manual" mochitest, but at least it'll let us satisfy ourselves that the bug is working.

Some notes about this patch:

- usernames for mailnews will be stored in the username field not the url.
- promptPassword dissects the url a bit, I came to the conclusion that to support the possibility of multiple users per server and the way in which these could be set up in mailnews, then the promptPassword has to be able to take the username from the url.

Comment 18

[Mark Banner (:standard8)]

Attachment: WIP diff to nsLoginManagerPrompter.js (v4)

This version works a lot better and fixes some bugs from v3 which stopped prompt() and promptUsernameAndPassword() from working.

I've got a basic (non-automatic) test suite up and running (I'll attach it in a mo). Just need to compare it with the current wallet version which I'll hopefully be able to do tomorrow.

Comment 19

[Mark Banner (:standard8)]

Attachment: Manual Test Suite

This is the test suite I've been using.

Since mochitests and our other tests aren't able to hook into dialog display correctly at the moment, I've used the mochitest framework to do manual based tests.

To run, (after applying the patches and building) enter <objdir>/_tests/testing/mochitest and run "perl runtest.pl". This will start up SeaMonkey/Firefox and pre-load a html page into it with all the tests.

Scroll down to very near the end, and look for the passwordmgr set of tests.

Then run (select) "test_0init.html" (This must be done every time you start-up the test suite).

Next, select one of the test_prompt* files. The prompts take you through what you should enter to make the tests pass. Of course, you can do the wrong thing and make the tests fail.

You should be able to run multiple test_prompt* files multiple times per start-up. If you get problems, you may need to remove the entry for ftp://localhost/abc manually, but hopefully each test will automatically remove that.

As this isn't an automated test suite (and hence can't be checked in), I've only done the minimum which is why it is also good to make the tests fail. Its better than nothing and gives me confidence that it'll work correctly.

Comment 20

[Mark Banner (:standard8)]

Attachment: Implement nsIAuthPrompt for nsLoginManagerPrompter.js

I'm happy this patch is effectively working the same way as wallet used to wrt displaying/not display prompts and handling of promptPassword urls.

I haven't reimplemented all of mailnews yet, but I'm fairly sure this will be acceptable for mailnews.

I'll attach a new manual test suite in a mo.

Comment 21

[Mark Banner (:standard8)]

Attachment: Manual Test Suite v2

Comment 22

[Mark Banner (:standard8)]

Comment on attachment 288876 [details] [diff] [review]
Implement nsIAuthPrompt for nsLoginManagerPrompter.js

biesi, are you happy to review this first? (I'll get gavin to look at it afterwards as well).

The code is mainly modelled around providing the same functionality as wallet, with the exception that usernames are split from the url.

Comment 19 has some info about the "manual" test suite.

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 288876 [details] [diff] [review]
Implement nsIAuthPrompt for nsLoginManagerPrompter.js

Asking Gavin for review as well to try and speed things up by doing them in parallel...

Comment 24

[Mark Banner (:standard8)]

Comment on attachment 288876 [details] [diff] [review]
Implement nsIAuthPrompt for nsLoginManagerPrompter.js

Apologies, I'll get the right flag this time.

Comment 25

[Mark Banner (:standard8)]

Requesting blocking FF 3 (I would prefer blocking gecko 1.9 but that's not available here).

Reasons:

1) SeaMonkey needs to drop wallet to help enable a clean l10n build for its 2.x builds.
2) Thunderbird also wants to drop wallet.
3) It will reduce the complexity of ifdefs in mozilla/toolkit/components.
4) FF 2 had the nsIAuthPrompt interface, FF 3 hasn't. Although core FF 3 doesn't need it, it is essentially a regression.

Notes:
Has a patch, just needs review, and I'm willing to update it if necessary.

Comment 26

[Justin Dolske [:Dolske]]

Comment on attachment 288876 [details] [diff] [review]
Implement nsIAuthPrompt for nsLoginManagerPrompter.js

Drive-by! :)

>+    /* ---------- nsIAuthPrompt prompts ---------- */

Do SM and TB use all of these, or just some?

>+    prompt : function (aDialogTitle, aText, aPasswordRealm,
>+                       aSavePassword, aDefaultText, aResult) {

I think it would be useful to document here (or in the nsIAuthPrompt IDL) what these args are expected to be. aPasswordRealm and aSavePassword, specifically. 

>+        this.log("===== prompt called =====");

"prompt()", just to be clear it's the actual function name.

>+        if (!aResult || aResult != "") {

That's the same as |if (true)|??? I guess this should be looking at aResult.value instead? It's an outparam, so it shouldn't be non-null anyway?

>+            // Look for existing logins.
>+            var foundLogins = this._pwmgr.findLogins({}, hostname, null,
>+
>+                                                     aPasswordRealm);

As above, what's the expected formated of aPasswordRealm here? Just seems odd to parse out the hostname, but then feed the whole thing in as the realm.

>+    /*
>+     * promptUsernameAndPassword
>+     *
>+     * Looks up a username and password in the database. Will prompt the user
>+     * with a dialog, even if a username and password are found.
>+     */
>+    promptUsernameAndPassword : function (aDialogTitle, aText, aPasswordRealm,
>+                                         aSavePassword, aUsername, aPassword) {

As above, these args should be documented too.

>+        if (aSavePassword == Ci.nsIAuthPrompt.SAVE_PASSWORD_FOR_SESSION)
>+            throw Components.results.NS_ERROR_NOT_IMPLEMENTED;

Should prompt() have this too?

>+        if (!aPassword || aPassword != "") {

Same thing as above.


>+    promptPassword : function (aDialogTitle, aText, aPasswordRealm,
>+                                         aSavePassword, aPassword) {

Docs, again.

>+        if (!aPassword || aPassword != "") {

See above.

>+    _getHost: function (aURIString) {

There's now a _getFormattedHostname() function, which should be used instead of this. It currently takes a URI; might be clever to have it check |instanceof| its arg, and convert incoming strings to URIs.

>+    _decomposeURI: function (aURIString) {

This should make use of _getFormattedHostname().

>+            // Remove the username component from the URI
>+            var reducedURI = hostname;

Move that into an else-clause?

>+            if (username != "") {

if (!username)

>+                reducedURI = uri.spec;

Hmm. This code is really just appending the path part of the URL to the hostname (from getFormattedHostname), right? Why only if there was a username specified? Do you really want a path here?

It should probably do something like |reducedURI = hostname + uri.path| instead of using the full spec.

>+                // Spec gives us a string with a "/" on the end even if
>+                // there is nothing else after the host name. We don't
>+                // want it, as it'll confuse the display.
>+                if (reducedURI[reducedURI.length - 1] == '/' &&
>+                    hostname == reducedURI.substring(0, reducedURI.length - 1)) {
>+                    reducedURI = hostname;
>+                }

Err, this is confusing too. The comment makes it sound like you just want to trim a trailing '/', but then |hostname| makes an appearance? What exactly is the intended difference between hostname and reducedURI?

Comment 27

[Mark Banner (:standard8)]

(In reply to comment #26)
> (From update of attachment 288876 [details] [diff] [review])
> Drive-by! :)
> 
> >+    /* ---------- nsIAuthPrompt prompts ---------- */
> 
> Do SM and TB use all of these, or just some?

If I remember correct, SM and TB use promptUsernameAndPassword and promptPassword. I may be wrong here, will confirm in the next day or so.

> >+    prompt : function (aDialogTitle, aText, aPasswordRealm,
> >+                       aSavePassword, aDefaultText, aResult) {
> 
> I think it would be useful to document here (or in the nsIAuthPrompt IDL) what
> these args are expected to be. aPasswordRealm and aSavePassword, specifically.

aPasswordRealm is the full url: e.g. ftp://localhost/abc
aSavePassword is one of the options from nsIAuthPrompt: http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/netwerk/base/public/nsIAuthPrompt.idl&rev=1.5&mark=48-50#42

> 
> >+        if (!aResult || aResult != "") {
> 
> That's the same as |if (true)|??? I guess this should be looking at
> aResult.value instead? It's an outparam, so it shouldn't be non-null anyway?

There's evidence to suggest the mailnews (and wallet) worked in this (incorrect) manner:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/base/util/nsMsgIncomingServer.cpp&rev=1.260&mark=733-738,741-743#727

I probably need to look more closely at that case and see if we either need to change the param to inout in nsIAuthPrompt, or rework mailnews somehow.

> >+            // Look for existing logins.
> >+            var foundLogins = this._pwmgr.findLogins({}, hostname, null,
> >+
> >+                                                     aPasswordRealm);
> 
> As above, what's the expected formated of aPasswordRealm here? Just seems odd
> to parse out the hostname, but then feed the whole thing in as the realm.

Like I said above, the aPasswordRealm will be a uri. This is probably more for ldap cases where we have a case where ldap://localhost/dc=test can have the same username, but different password to ldap://localhost/dc=test1

As the host is just required to be a host name (according to the idl), that's why I separate it out.

Comment 28

[Justin Dolske [:Dolske]]

(In reply to comment #27)

> > >+    prompt : function (aDialogTitle, aText, aPasswordRealm,
> > >+                       aSavePassword, aDefaultText, aResult) {
...
> aPasswordRealm is the full url: e.g. ftp://localhost/abc

Hmm, I'm still reluctant to be adding a new URL format, especially after having worked to get rid of the previously differing formats (http://foo vs foo:80). I just want to make sure I understand what the extra path bits are needed for.

> > >+        if (!aResult || aResult != "") {
...
> I probably need to look more closely at that case and see if we either need to
> change the param to inout in nsIAuthPrompt, or rework mailnews somehow.

Yeah, it really seems to be wanting an inout here. Although, hmm, when does the caller have a password? The nsIAuthPrompt2 code doesn't have a way for a caller to prefill the username or password... Why prompt the user at all if you already have login info?

Comment 29

[Mark Banner (:standard8)]

(In reply to comment #28)
> (In reply to comment #27)
> 
> > > >+    prompt : function (aDialogTitle, aText, aPasswordRealm,
> > > >+                       aSavePassword, aDefaultText, aResult) {
> ...
> > aPasswordRealm is the full url: e.g. ftp://localhost/abc
> 
> Hmm, I'm still reluctant to be adding a new URL format, especially after having
> worked to get rid of the previously differing formats (http://foo vs foo:80). I
> just want to make sure I understand what the extra path bits are needed for.

As I said at the end of comment 27:

> This is probably more for
> ldap cases where we have a case where ldap://localhost/dc=test can have the
> same username, but different password to ldap://localhost/dc=test1
> 
> As the host is just required to be a host name (according to the idl), that's
> why I separate it out.

So, in other words, you can have more than one login per host via different urls for LDAP. I can't see a way around this otherwise.

I'm not quite sure why you think this is an issue. The hostname part will still be just the reduced bit like you have now.

Comment 30

[Justin Dolske [:Dolske]]

Ok, I guess the LDAP thing makes sense, although it's hard to do once-a-month context-switching. :)

Can you update the patch to address the review comments above? If we can iterate quickly here, this mgiht be able to make Beta 4. [My understanding is that general stuff like this can still get in for the first week or two after the tree reopens, but after than approvals will be hard to get.]

Comment 31

[Mark Banner (:standard8)]

(In reply to comment #30)
> Ok, I guess the LDAP thing makes sense, although it's hard to do once-a-month
> context-switching. :)

Yeah sorry.

> Can you update the patch to address the review comments above? If we can
> iterate quickly here, this mgiht be able to make Beta 4. [My understanding is
> that general stuff like this can still get in for the first week or two after
> the tree reopens, but after than approvals will be hard to get.]

If I don't get it done this evening (sunday) I'll get it done on Monday.

Comment 32

[Mark Banner (:standard8)]

(In reply to comment #28)
> > > >+        if (!aResult || aResult != "") {
> ...
> > I probably need to look more closely at that case and see if we either need to
> > change the param to inout in nsIAuthPrompt, or rework mailnews somehow.
> 
> Yeah, it really seems to be wanting an inout here. Although, hmm, when does the
> caller have a password? The nsIAuthPrompt2 code doesn't have a way for a caller
> to prefill the username or password... Why prompt the user at all if you
> already have login info?
> 
Just thought I'd better respond to this one. Mailnews logins can sometimes fail, although the stored login is valid. So we reprompt with the same password just in case... See bug 249240 comment 42 ish for the background. Its something I really don't want to change under this bug, so I'm going for changing the parameter to inout - it won't actually affect any callers, but it'll be clearer from the documentation aspect.

Comment 33

[Justin Dolske [:Dolske]]

I'm more just concerned with the intention of the conditional, |if (!aResult || aResult != "")| doesn't make sense...

There's three possible inputs:

1. aResult == null
2. aResult == empty string
3. aResult == non-empty string

The conditional evaluates to true in all 3 cases.

Comment 34

[Mark Banner (:standard8)]

Attachment: Implement nsIAuthPrompt for nsLoginManagerPrompter.js v2

Updated patch that I think addresses all the comments. Sorry for the delay, I had problems with the mailnews side of things that were getting in the way.

Comment 35

[Justin Dolske [:Dolske]]

Comment on attachment 303860 [details] [diff] [review]
Implement nsIAuthPrompt for nsLoginManagerPrompter.js v2

>Index: netwerk/base/public/nsIAuthPrompt.idl
>===================================================================
>RCS file: /cvsroot/mozilla/netwerk/base/public/nsIAuthPrompt.idl,v

>     /**
>      * Puts up a text input dialog with OK and Cancel buttons.
>+     * @param  dialogText    The title for the dialog.
>+     * @param  text          The text to display in the dialog.
>+     * @param  passwordRealm The "realm" the password belongs to: e.g.
>+     *                       ldap://localhost/dc=test
>+     * @param  savePassword  One of the SAVE_PASSWORD_* options above.
>+     * @param  defaultText   The default text to display in the text input box.
>+     * @param  result        The password entered by the user if OK was
>+     *                       selected.

@result here isn't a password, it's just a text value.

Might be worth adding a note that prompt() uses separate args for the "in" and "out" values of the input field, where the other functions (now) use a single "inout" arg.



>Index: toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js
>===================================================================

>+ * Implements interfaces for prompting the user to enter/save/chane auth info.

s/chane/change/ :-)


>+ * nsIAuthPrompt2: Used by Firefox, invoked by a channel for protocol-based
>+ * authentication (eg HTTP Authenticate, FTP login).

No need to call out the app here, since nsIAuthPrompt2 is actually used by everything.


>+    prompt : function (aDialogTitle, aText, aPasswordRealm,
>+                       aSavePassword, aDefaultText, aResult) {
...
>+        if (aDefaultText != "") {
>+            aResult.value = aDefaultText;
>+        }
>+        else {


Just test for |if (aDefaultText)|.

Style nit: "} else {" on one line.

>+        var ok = this._promptService.prompt(this._window,
>+                aDialogTitle, aText, aResult, checkBoxLabel, checkBox);
>+
>+        if (ok && checkBox.value) {
>+            var newLogin = Cc["@mozilla.org/login-manager/loginInfo;1"].
>+                           createInstance(Ci.nsILoginInfo);
>+            newLogin.init(hostname, null, aPasswordRealm,
>+                          "", aResult.value,
>+                          "", "");
>+
>+            this.log("New login seen for " + aPasswordRealm);
>+
>+            this._pwmgr.addLogin(newLogin);
>+        }

*headscratch*

So, I don't get this. prompt() is just prompting for a plain text value, not a password. So, at the least this is putting aResult.value into the wrong param to newLogin.init(). But why would it even be saving anything at this point? All it has is a hostname and username. The checkbox's text ("Use Password Manager to remember this password.") won't even make sense.

As a general rule, I don't think Login Manager should be storing usernames without passwords. [I'd have to check if that breaks assumptions in the code.] I think what ought to happen here is that the prompt shouldn't save anything (no checkbox), and the saving can be handled by promptPassword() which is presumably going to be used next by the caller. [Which makes me wonder why promptUsernameAndPassword() isn't just being used?]

>+    promptUsernameAndPassword : function (aDialogTitle, aText, aPasswordRealm,
>+                                         aSavePassword, aUsername, aPassword) {
...
>+        if (!aPassword || !aPassword.value || aPassword.value == "") {

|aPassword| is an outparam, so it shouldn't ever be null. The 3rd condition is already taken care of by the 2nd. So, this can just be |if (!aPassword.value)|.

Hmm, what if the user specifies just a username? |if (!aPassword.value && !aUsername.value)| perhaps?


>+    promptPassword : function (aDialogTitle, aText, aPasswordRealm,
>+                                         aSavePassword, aPassword) {
...
>+        var [hostname, username, reducedURI] = this._decomposeURI(aPasswordRealm);

Change reducedURI to "pathname" (in name and value), for consistency with bug 403790. Then |var newRealm = hostname + pathname|.


>+        if (!aPassword || !aPassword.value || aPassword.value == "") {

Same comment as above... Just |if (!aPassword.value)|.


>     _getFormattedHostname : function (aURI) {
...
>+        if (aURI instanceof Ci.nsIURI) {
>+            uri = aURI;
>+        }
>+        else {

Formatting nit again: |} else {|.

>+            var ioService = Cc["@mozilla.org/network/io-service;1"].
>+                           getService(Ci.nsIIOService);

Hmm. Seems like there are enough places using ioService now, that we should just turn it into a |this._ioService| getter, ala the other service getters at the top of the file. All the new (and existing callers) can then just do |var foo = this._ioService.newURI(xxx)|.


>+    _decomposeURI: function (aURIString) {
...
>+            if (username == "")
>+                reducedURI = hostname;
>+            else {
>+                reducedURI = hostname + uri.path;
>+                // Spec gives us a string with a "/" on the end even if
>+                // there is nothing else after the host name. We don't
>+                // want it, as it'll confuse the display.
>+                var len = reducedURI.length;
>+                if (reducedURI[reducedURI.length - 1] == '/')
>+                    reducedURI = reducedURI.substring(0, len - 1);
>+            }

This part should basically be the same as cleanupURL() from bug 403790. See also previous comment.


>+        catch (ex) {
>+            throw Cr.NS_ERROR_MALFORMED_URI;
>+        }

Is this needed? If you try to do ioService.newURI(garbage), what's it throw?

Comment 36

[Mark Banner (:standard8)]

(In reply to comment #35)
> >+        var ok = this._promptService.prompt(this._window,
> >+                aDialogTitle, aText, aResult, checkBoxLabel, checkBox);
> >+
> >+        if (ok && checkBox.value) {
> >+            var newLogin = Cc["@mozilla.org/login-manager/loginInfo;1"].
> >+                           createInstance(Ci.nsILoginInfo);
> >+            newLogin.init(hostname, null, aPasswordRealm,
> >+                          "", aResult.value,
> >+                          "", "");
> >+
> >+            this.log("New login seen for " + aPasswordRealm);
> >+
> >+            this._pwmgr.addLogin(newLogin);
> >+        }
> 
> *headscratch*
> 
> So, I don't get this. prompt() is just prompting for a plain text value, not a
> password. So, at the least this is putting aResult.value into the wrong param
> to newLogin.init(). But why would it even be saving anything at this point? All
> it has is a hostname and username. The checkbox's text ("Use Password Manager
> to remember this password.") won't even make sense.
> 
> As a general rule, I don't think Login Manager should be storing usernames
> without passwords. [I'd have to check if that breaks assumptions in the code.]
> I think what ought to happen here is that the prompt shouldn't save anything
> (no checkbox), and the saving can be handled by promptPassword() which is
> presumably going to be used next by the caller. [Which makes me wonder why
> promptUsernameAndPassword() isn't just being used?]

As always, there's an exception. It appears the newsgroup protocol can allow a login to require a username but not a password. You can't tell if a password is required until you've sent the username.

So mailnews uses prompt() to get the username, and saves it in a login with a url like news://testserver/#username, and the username in the password field, then if required it uses promptPassword() to get the password and saves it in a second login with a url like news://testserver/#password with the password in the password field.

Given that I believe that login manager requires a password to be stored, then I think we have to go with this format still?

The checkbox text I can change easily, there's a rememberValue string already defined.

Comment 37

[Robert Kaiser]

Hmm, can't we just prompt for both at once but ignore the password (user can leave it empty) as long as the server doesn't request it?

Comment 38

[Justin Dolske [:Dolske]]

(In reply to comment #37)
> Hmm, can't we just prompt for both at once but ignore the password (user can
> leave it empty) as long as the server doesn't request it?

That would be better from a UI point of view... The normal case is that both are going to be needed, which would mean 2 dialogs. Better to just show one, and if the protocol doesn't request the password just ignore it.

This is what FTP is doing currently (at least on FF), and the FTP protocol also allows for login sequences where a USER command is requested (and sent), but no PASS command is requested (or sent). The browser just connects, sees a username is requested, and puts up a normal user+pass authentication prompt.

This is probably a tangent, though. New bug? No matter how the prompting is done, the important end state is if a username-only login should be saved.


(In reply to comment #36)

> It appears the newsgroup protocol can allow a
> login to require a username but not a password. You can't tell if a
> password is required until you've sent the username.

So, like FTP.

The reason I don't think Login Manager should save logins without a password is that such logins are poor security practice (and seem to be very rare). Login Manager has contained code in addLogin() to prohibit this condition since the initial checkin a year ago, and only 1 bug has been filed on it (bug 399703).

[Contrast to the original design of not allowing password-only logins, which was quickly noticed at a regression by multiple people.]

> So mailnews uses prompt() to get the username, and saves it in a login with a
> url like news://testserver/#username, and the username in the password field,
> then if required it uses promptPassword() to get the password and saves it in a
> second login with a url like news://testserver/#password with the password in
> the password field.

*shudder* That's awful.

This should all just use a single nsILoginInfo (even if we should ultimately be forced to allow username-only logins).

Comment 39

[Mark Banner (:standard8)]

Attachment: Implement nsIAuthPrompt for nsLoginManagerPrompter.js v3

I'd pretty much decided that mailnews should redo its dialog in some manner a couple of hours ago.

This is the new patch, prompt() is now basically just a wrapper. I think its probably acceptable given that these interfaces need reworking in Moz 2 (Justin's already mentioned this to me), also the caller can always do manual checking/saving in this case.

Also addressed the rest of the comments.

Comment 40

[Justin Dolske [:Dolske]]

Comment on attachment 304295 [details] [diff] [review]
Implement nsIAuthPrompt for nsLoginManagerPrompter.js v3

>Index: toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js
>===================================================================
>+    __ioService: null, // IO service for string -> nsIURI conversion
>+    get _ioService() {
>+        if (!this.__ioService)
>+            this.__ioService = Cc["@mozilla.org/network/io-service;1"]
>+                                    .getService(Ci.nsIIOService);

Nit: align the Cc and getService as in storage-Legacy.js.

You probably cut'n'pasted fron nsLoginManger.js, and the idiot who wrote that code indented it wrong. *cough* :-)

>+    /* ---------- nsIAuthPrompt prompts ---------- */
>+
>+
>+    /*
>+     * prompt
>+     *
>+     * If a password is found in the database for the password realm, it is
>+     * returned straight away without displaying a dialog.
>+     *
>+     * If a password is not found in the database, the user will be prompted
>+     * with a dialog with a text field and ok/cancel buttons. If the user
>+     * allows it, then the password will be saved in the database.
>+     */

Comment nit: needs updating.

With the prompting change you mentioned, does mailnews ever call this at all? Perhaps the function should just always throw NS_ERROR_NOT_IMPLEMENTED. Hmm, just as well either way, I suppose.

>+    /**
>+     * Extracts a hostname, username and a reduced URI (with no username) from
>+     * a string based URI via a standard URI implementation.
>+     */

Comment nit: returns a pathname, not a reduced URI.

>+    _decomposeURI: function (aURIString) {
...
>+        // Remove the username component from the URI
>+        var pathname = "";

Comment nit: The comment's not relevant now, and can be removed.

Comment 41

[Mark Banner (:standard8)]

Attachment: Implement nsIAuthPrompt for nsLoginManagerPrompter.js v4

Updated patch to address Justin's comments. Requesting r from gavin (for both parts) and sr from dveditz for the nsIAuthPrompt changes.

Comment 42

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 304981 [details] [diff] [review]
Implement nsIAuthPrompt for nsLoginManagerPrompter.js v4

>Index: toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js

>+    prompt : function (aDialogTitle, aText, aPasswordRealm,
>+                       aSavePassword, aDefaultText, aResult) {

>+        if (!aDefaultText) {
>+            aResult.value = aDefaultText;
>+        }

This should be "if (aDefaultText)", right?

>+        return this._promptService.prompt(this._window,
>+               aDialogTitle, aText, aResult, null, checkBox);

No point in passing "checkBox", an empty object will do since you're passing null for aCheckMsg.

>+    promptUsernameAndPassword : function (aDialogTitle, aText, aPasswordRealm,
>+                                         aSavePassword, aUsername, aPassword) {

>+        if (ok && checkBox.value) {
>+            var newLogin = Cc["@mozilla.org/login-manager/loginInfo;1"].
>+                           createInstance(Ci.nsILoginInfo);
>+            newLogin.init(hostname, null, aPasswordRealm,
>+                          aUsername.value, aPassword.value,
>+                          "", "");
>+
>+            this.log("New login seen for " + aPasswordRealm);
>+
>+            this._pwmgr.addLogin(newLogin);

Don't you need to check whether this is the same as an existing login (like the one you used to fill out the defaults earlier in this function), like promptAuth does at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js&rev=1.24&mark=248-269#240 ?

>+    promptPassword : function (aDialogTitle, aText, aPasswordRealm,
>+                                         aSavePassword, aPassword) {

>+        if (ok && checkBox.value) {
>+            var newLogin = Cc["@mozilla.org/login-manager/loginInfo;1"].
>+                           createInstance(Ci.nsILoginInfo);
>+            newLogin.init(hostname, null, newRealm, username,
>+                          aPassword.value, "", "");

I guess this doesn't have the same problem, because if we already had a login with this username we wouldn't have prompted.

r=me with those addressed.

Comment 43

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #42)
> Don't you need to check whether this is the same as an existing login

Hmm, and now that I think of it, even the existing promptAuth code seems like it would break if you had multiple logins stored and happened to fill in a username/password that matches a login other other than the one we used to pre-fill the dialog (since it only compares the new values to selectedLogin before calling addLogin).

I'll file a followup bug on this so that we can address it in the code you'll add as well as the existing code, no need to worry about it for this patch.

Comment 44

[:Gavin Sharp [email: [e-mail]]]

I filed bug 419081 for that issue.

Comment 45

[Daniel Veditz [:dveditz]]

Comment on attachment 304981 [details] [diff] [review]
Implement nsIAuthPrompt for nsLoginManagerPrompter.js v4

sr=dveditz

Comment 46

[Justin Dolske [:Dolske]]

As Gavin mentioned in comment #42, promptUsernameAndPassword() never checks to see if the values entered in the prompt are the same values it prefilled. It just always calls addLogin() (which will throw because the login already exists). That and the issue fixed by 419081 can probably just be fixed at the same time.

[Only mentioning this because the case 419081 fixes isn't likely to happen often, where as the existing patch here will make addLogin() throw all the time, if you had even just 1 stored login.]

Comment 47

[Mark Banner (:standard8)]

Attachment: [checked in] Implement nsIAuthPrompt for nsLoginManagerPrompter.js v5

Addressed Gavin's comments (apart from the ones that will be covered in bug 419081), carrying forward r and sr, requesting approval 1.9b4.

This patch alters one interface in a way that shouldn't affect other apps, and also implements nsIAuthPrompt functionality that FF doesn't use but is required by other apps. Extensions may also use it, so it'd be good to get in for the beta in case they wish to use it.

Comment 48

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 306505 [details] [diff] [review]
[checked in] Implement nsIAuthPrompt for nsLoginManagerPrompter.js v5

a1.9b4=beltzner

Comment 49

[Justin Dolske [:Dolske]]

I don't think this is really a "late-compat" bug... The existing interface that's being tweaked doesn't really actually change how existing code would work. It's actually just fixing the IDL to describe how it was being used anyway.

Comment 50

[Philip Chee]

> It's actually just fixing the IDL to describe how it was being used
> anyway.

dev-doc-needed ?

Comment 51

[Mark Banner (:standard8)]

Comment on attachment 306505 [details] [diff] [review]
[checked in] Implement nsIAuthPrompt for nsLoginManagerPrompter.js v5

Checked in earlier today.

Comment 52

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #42)
> Don't you need to check whether this is the same as an existing login (like the
> one you used to fill out the defaults earlier in this function)

This comment wasn't addressed. Perhaps I was unclear - I think you should have brought this code to the same bug-level as the current promptAuth code (comparing against the pre-filled password, at least) before landing, otherwise you'll have a rather busted dialog when users just accept the pre-filled password. The less-common case of checking against *all* saved passwords rather than just the one used to pre-fill was what I suggested putting off to another bug (bug 419081, or another given that that one already has a patch for the existing code).

Comment 53

[Mark Banner (:standard8)]

Attachment: [checked in] Fix saving passwords

Gavin, apologies I misunderstood your comment. These are the changes I think need to be done, I'm currently in the process of rebuilding to test them, though I'm fairly sure they will work. I've only changed promptUsernameAndPassword because promptPassword returns straight away if it finds a login.

Given we're not actually using the code at the moment, are you happy that we just put this patch through, or would you rather I back out in the intermediate time?

Comment 54

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 306885 [details] [diff] [review]
[checked in] Fix saving passwords

A bit odd to be using selectedLogin/foundLogins[0] interchangeably, but can address that with the followup to properly check against all logins, I guess. I think you should just go ahead and land this patch (with previous approval still valid).

Comment 55

[Mark Banner (:standard8)]

This is fixed now, the remaining issue covered by bug 419081