403790 - Password manager needs to be able to migrate mailnews logins.

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P2)

Description

[Mark Banner (:standard8)]

Spun off (slightly) from bug 396316.

Toolkit's password manager needs to be able to migrate logins where the site url is of the forms:

mailbox://user@localhost
ldap://localhost:389/dc=test

Once the work in bug 396316 is complete, we can do this as well. Though I'll probably do the test case before then.

Comment 1

[Mark Banner (:standard8)]

Here's what we've got to convert (site/username):

imap://username@host            username
ldap://localhost:389/dc=test    <>
mailbox://username@host         username
smtp://username@host            <>
news://localhost:119/#password  <>
news://localhost:119/#username  <>

into (site/httpRealm/username):

imap://host       imap://host              username
ldap://localhost  ldap://localhost/dc=test <>
mailbox://host    mailbox://host           username
smtp://host       smtp://host              username
news://localhost/ news://localhost/        username

note: "<>" means an empty value

The first patch I'm attaching won't deal with the newsgroups case, I may put that into a different bug. I'm mainly interested in getting the main ones migrating properly for now.

Comment 2

[Mark Banner (:standard8)]

Attachment: Possible Fix

Possible fix (see also previous comments).

Not requesting reviews yet as the tests don't work on Thunderbird (it doesn't build with the ftp protocol :-( and they don't run tests yet anyway), and I'm currently rebuilding SeaMonkey due to the latest nsIAuthPrompt updates.

Should be fully functional apart from the newsgroups migration if it works.

Comment 3

[Mark Banner (:standard8)]

Attachment: The fix

A few fixes from the previous version, caused by some incorrect assumptions by myself. This now passes all the existing tests and will successfully migrate imap/pop/ldap logins (which may need a couple of mailnews fixes from my tree).

I think smtp logins require a channel/uri to be set up for them, I'll need to think about news in more detail later, but that's minor compared to getting the rest of this going.

Note that I've used ftp in the unit test as both SM & FF build with that protocol in place, hence its an easy way of testing the reformatting without having to supply extra code that would essentially only be required for FF.

Comment 4

[Justin Dolske [:Dolske]]

Comment on attachment 301580 [details] [diff] [review]
The fix

>Index: toolkit/components/passwordmgr/src/storage-Legacy.js
>===================================================================

>+
>+                // Blank out the username so that we can elimante it from the
>+                // URL if necessary.
>+                uri.username = "";
>+
>+                // If the uri spec now isn't the same as the username, save it
>+                // so that it can be used later as it is one of the special ones
>+                // that mailnews used to have.
>+                if (uri.spec != aURL) {
>+                    URLNoUsername = uri.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 (URLNoUsername[URLNoUsername.length - 1] == '/' &&
>+                        newURL ==
>+                        URLNoUsername.substring(0, URLNoUsername.length - 1)) {
>+                        URLNoUsername = newURL;
>+                    }
>+                }

We've already got the parts for a cleaned up URL by this point, can this just be:

if (username) {
    URLNoUsername = hostname + uri.path;
    // remove trailing '/' if present
    var len = URLNoUsername.length;
    if (URLNoUsername[len - 1] == '/')
        URLNoUsername = URLNoUsername.substring(0, len - 1)
}


>+        // This could be one of the special mailnews logins, if so we'll handle
>+        // it here.
>+        else if (aLogin.usernameField == "\\=username=\\" &&
>+            aLogin.passwordField == "\\=password=\\") {

This check should be stricter, random web logins could be using these field names.

EG, Extract the protocol, and check |if (protocol in ["imap", "ldaps", ...])|

>Index: toolkit/components/passwordmgr/test/unit/data/signons-403790.txt
>===================================================================
>+ftp://bugzilla@localhost
>+\=username=\
>+~
>+*\=password=\

Were FTP logins stored in that format? I thought it was just the mail/news protocols (imap://, ldaps://, mailbox:// smtp://).

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #4)
> We've already got the parts for a cleaned up URL by this point, can this just
> be:
> if (username) {
>     URLNoUsername = hostname + uri.path;
>     // remove trailing '/' if present
>     var len = URLNoUsername.length;
>     if (URLNoUsername[len - 1] == '/')
>         URLNoUsername = URLNoUsername.substring(0, len - 1)
> }

Yes, though I think it'll be URLNoUsername = newURL + uri.path as we'll need the port for non-standard port set ups.

> >+        // This could be one of the special mailnews logins, if so we'll handle
> >+        // it here.
> >+        else if (aLogin.usernameField == "\\=username=\\" &&
> >+            aLogin.passwordField == "\\=password=\\") {
> This check should be stricter, random web logins could be using these field
> names.
> EG, Extract the protocol, and check |if (protocol in ["imap", "ldaps", ...])|

ok, I can add the check.

> >Index: toolkit/components/passwordmgr/test/unit/data/signons-403790.txt
> >===================================================================
> >+ftp://bugzilla@localhost
> >+\=username=\
> >+~
> >+*\=password=\
> Were FTP logins stored in that format? I thought it was just the mail/news
> protocols (imap://, ldaps://, mailbox:// smtp://).

FTP logins weren't stored in that format (if I remember correctly), it just seemed most convenient to test it that way. I'll have to create some additional protocol handlers in the test suite so that FF/xulrunner will pass correctly for this test. Shouldn't be too hard, though I'll branch all the mailnews related tests into a separate file.

Comment 6

[Justin Dolske [:Dolske]]

(In reply to comment #5)
> (In reply to comment #4)

> Yes, though I think it'll be URLNoUsername = newURL + uri.path as we'll need
> the port for non-standard port set ups.

Oops, yes. The variable is |newURL| here, but callers refer to it as |hostname|.

> I'll have to create some additional
> protocol handlers in the test suite so that FF/xulrunner will pass correctly
> for this test. Shouldn't be too hard, though I'll branch all the mailnews
> related tests into a separate file.

Oh? Is it because the default port isn't known?

Comment 7

[Mark Banner (:standard8)]

Attachment: The fix v2

Updated patch. Addresses Justin's comments. Includes protocol handlers for the tests so that they will pass in all normal configurations of sm/ff/tb (note, tb fails on test_storage_legacy_1.js, but that's a different story...).

This should get us going with migration of passwords (once I've fixed mailnews a little).

Comment 8

[Justin Dolske [:Dolske]]

Comment on attachment 301990 [details] [diff] [review]
The fix v2

Ok, just a couple more small things:

>+                // If we have a username, or spec isn't the same as the before,
>+                // save it, so that it can be used later as it is one of the
>+                // special ones that mailnews used to have.
>+                if (username || uri.spec != aURL) {

What is the |uri.spec != aURL| test doing? |uri| is never modified after being created from aURL, so this just seems odd. Did we talk about this on IRC? It seems familiar but I can't remember why. :)


>+                    reducedURL = newURL + 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 = reducedURL.length;
>+                    if (reducedURL[len - 1] == '/')
>+                        reducedURL = reducedURL.substring(0, len - 1);

Comment referring to "Spec" is outdated now.

Hmm, shouldn't the trailing-slash stripping be limited to an empty path? We want "foo://host" instead of "foo://host/", but "foo://host/bar/" should not be changed to "foo://host/bar".

EG:
  if (uri.path == "/")
      reducedURL = newURL;
  else
      reducedURL = newURL + uri.path


>         const isHTTP = /^https?:\/\//;
>         if (!isHTTP.test(aLogin.hostname) && !isFormLogin) {
>@@ -743,6 +753,25 @@ LoginManagerStorage_legacy.prototype = {
>             aLogin.formSubmitURL = null;
>             this.log("2E upgrade: set empty realm to " + aLogin.httpRealm);
>         }
>+        // This could be one of the special mailnews logins, if so we'll handle
>+        // it here.
>+        else if (aLogin.usernameField == "\\=username=\\" &&
>+                 aLogin.passwordField == "\\=password=\\" &&
>+                 aLogin.hostname.match(/^ldap|^smtp|^imap|^news|^mailbox/)) {

Just to be extra paranoid, I think the whole scheme should be exactly matched. EG, these checks would match "newsmurf://".

EG
  const isMailNews = /^(ldaps?|smtp|etc):\/\//;


>Index: toolkit/components/passwordmgr/test/unit/test_storage_legacy_4.js
>===================================================================

>+ * Test suite for storage-Legacy.js -- various bug fixes.

s/various bug fixes/mailnews specific tests/

I just kind of skimmed through the rest of the test, but it looks fine.

Comment 9

[Mark Banner (:standard8)]

(In reply to comment #8)
> (From update of attachment 301990 [details] [diff] [review])
> Ok, just a couple more small things:
> 
> >+                // If we have a username, or spec isn't the same as the before,
> >+                // save it, so that it can be used later as it is one of the
> >+                // special ones that mailnews used to have.
> >+                if (username || uri.spec != aURL) {
> 
> What is the |uri.spec != aURL| test doing? |uri| is never modified after being
> created from aURL, so this just seems odd. Did we talk about this on IRC? It
> seems familiar but I can't remember why. :)

I've just looked at this again, it is checking this case:

aURL = ldap://localhost1:389/dc=test
uri.spec = ldap://localhost1/dc=test (because 389 is the default).

LDAP doesn't use a hostname currently... Maybe it would be better to just generate reducedURL by default?

I've addressed the rest of the comments and I'm currently waiting my build to complete so I can test the changes before I post the new patch. 

Comment 10

[Mark Banner (:standard8)]

Attachment: The fix v3

Revised patch addressing Justin's comments, see also comment 9.

Comment 11

[Justin Dolske [:Dolske]]

(In reply to comment #9)

> aURL = ldap://localhost1:389/dc=test
> uri.spec = ldap://localhost1/dc=test (because 389 is the default).
> 
> LDAP doesn't use a hostname currently... Maybe it would be better to just
> generate reducedURL by default?

Err... What? In Thunderbird my saved LDAP password is for "ldaps://ldap.mozilla.org:636/dc=mozilla..." If something is storing passwords, it had better be using an appropriate hostname in the stored login so it doesn't end up sending the wrong thing somewhere.

Argh, now that I'm looking at cleanupURL() again, this just doesn't seem right. The path-as-realm is only needed for ldap:// entries, right? It should be explicitly constrained that way, and the path string just always returned by cleanupURL(). Callers can ignore it unless they want it.

Basically, something like:

function cleanupURL() {
    // ...get newURL...
    // ...get username...
    pathname = "";
    if (uri.path != "/")
        pathname = uri.path;
    return [newURL, username, pathname];
}
...
if (blah && isMailNews.test(...)) {
    ...
    if (isLDAP.test(...))
        aLogin.httpRealm = hostname + pathname;
    else
        aLogin.httpRealm = hostname; // ???
    ...
}


One more thing:

+            // and because these aren't form logins, ensure the username
+            // extracted from the URL is stored.
+            var [encUsername, userCanceled] = this._encrypt(username);
+            if (!userCanceled)
+                aLogin.wrappedJSObject.encryptedUsername = encUsername;

Shouldn't this already be happening in the existing code, about 35 lines up?

        // If a non-HTTP URL contained a username, it wasn't stored in the
        // encrypted username field (which contains an encrypted empty value)
        // (Don't do this if it's a form login, though.)
        if (username && !isFormLogin) {
            var [encUsername, userCanceled] = this._encrypt(username);
            if (!userCanceled)
                aLogin.wrappedJSObject.encryptedUsername = encUsername;
        }

Does something think mailnews logins are form logins (isFormLogin == true)? I guess it must, because otherwise the |if (!isHTTP.test(aLogin.hostname) && !isFormLogin)| case would be getting called, and the |else {}| case you added wouldn't be. Gah. I suppose the best fix here would be to add a |&& !isMailNews()| check to the conditions for isFormLogin, and move the mailnew |else| block inside the |!isHttp| block as just an |if| test.

Is my handwaving clear? Maybe I should change this section and attach it. :)

Comment 12

[Mark Banner (:standard8)]

Attachment: The fix v4

> aURL = ldap://localhost1:389/dc=test

"localhost1" was just something I used as a testcase, I would normally expect a proper hostname here.

I think I've addressed everything else Justin wanted with this patch. Its a bit simpler now as well :-)

Comment 13

[Justin Dolske [:Dolske]]

Comment on attachment 303125 [details] [diff] [review]
The fix v4

>+        const isMailNews = /^(ldaps?|smtp|imap|news|mailbox):\/\//;
>+
>         var isFormLogin = (aLogin.formSubmitURL ||
>                            aLogin.usernameField ||
>-                           aLogin.passwordField);
>+                           aLogin.passwordField) &&
>+                          !isMailNews.test(aLogin.hostname);

It's worth a comment here to mention why mailnews logins are an exception to the isFormLogin conditional. [Namely, that they're protocol logins with a user/pass field name set]

Did all these isMailNews logins have the unneeded field names set?

>+            // Null out the form items because these aren't form logins (which
>+            // at least mailnews logins had).
>+            aLogin.usernameField = "";
>+            aLogin.passwordField = "";

Hmm. In theory these shouldn't ever be set for other logins. I kinda want to just always clear these on principle, but OTOH I don't want to break migrating logins for some weird extension this late in the cycle... We should probably just go ahead and wrap this in a if(isMailNews) check.

Comment 14

[Mark Banner (:standard8)]

(In reply to comment #13)
> Did all these isMailNews logins have the unneeded field names set?

I hadn't checked news up until now, but yes, they all had these unneeded field names set.

Comment 15

[Mark Banner (:standard8)]

Attachment: The fix v5

Adds a comment, and limits username/password field name blanking to mailnews.

Comment 16

[Mark Banner (:standard8)]

Requesting blocking FF 3 (though this is really blocking gecko 1.9). TB & SM need this to be able to migrate to the password manager for their next major releases. This bug goes together with bug 382437 which is also blocking FF 3. Has patch, just needs final review.

Although we may not be able to get it right with the first patch, I'd like to be able to at least ensure we get something in before the gecko 1.9 release, so that there is something there, and minor improvements etc are just bug fixes.

Comment 17

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

Blocking for platform compatibility.

Comment 18

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

Comment on attachment 303772 [details] [diff] [review]
The fix v5

>Index: toolkit/components/passwordmgr/src/storage-Legacy.js

>+            // Null out the form items because these aren't form logins (which
>+            // at least mailnews logins had).

I don't understand this comment, can you re-phrase?

Comment 19

[Justin Dolske [:Dolske]]

Mark: how about we shoot to land this for B4? It should be low-risk, but lest there be weird mailnews-like logins stored by FF extensions or something, it might be nice to have it vetted by a beta.

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 303772 [details] [diff] [review]
The fix v5

Requesting Approval 1.9b4. Should be low - medium risk, and passes all the unit tests, however we want to get it in for testing during the beta as it may affect extensions (unlikely, but you never know).

Comment 21

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

Comment on attachment 303772 [details] [diff] [review]
The fix v5

"Might be nice" doesn't warrant the risk this late in the b4 cycle, we'll wait for the next milestone on this.

Comment 22

[Reed Loden [:reed]]

Comment on attachment 303772 [details] [diff] [review]
The fix v5

This is a blocker. No approval1.9 needed.

Comment 23

[Mike Connor [:mconnor]]

Mark, is this ready to land?  Can you land ASAP?

Comment 24

[Mark Banner (:standard8)]

(In reply to comment #23)
> Mark, is this ready to land?  Can you land ASAP?

I updated the patch last night, but couldn't land it because I didn't have time and the tree was burning. I will try and land it again tonight (UTC).

Comment 25

[Mark Banner (:standard8)]

Patch checked in. Fixed. I'll deal with any follow up issues in mailnews in other bugs (because I don't know what they are till I write the rest of the code).