470439 - Migration from signons.txt loses newsgroup username/password information.

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Mark Banner (:standard8)]

Due to the way that MailNews stores newsgroup logins in signons.txt, we need to keep the path information.

Currently mailnews stores:

news://localhost/#password
news://localhost/#username

Although these aren't ideal, until we do a further uplift in mailnews to fix what we do, I'm not going to change them. In any case - when we upgrade, they currently come out as:

news://localhost/

hence we don't have enough info.

I have a patch for this, just need to extend the unit tests.

Comment 1

[Justin Dolske [:Dolske]]

(In reply to comment #0)

> Currently mailnews stores:
> 
> news://localhost/#password
> news://localhost/#username

Does this mean 2 logins? Is that literally "#username" or is it actual logins data (eg "#dolske")? Is localhost just an example hostname here?

Comment 2

[Mark Banner (:standard8)]

(In reply to comment #1)
> (In reply to comment #0)
> 
> > Currently mailnews stores:
> > 
> > news://localhost/#password
> > news://localhost/#username
> 
> Does this mean 2 logins? Is that literally "#username" or is it actual logins
> data (eg "#dolske")? Is localhost just an example hostname here?

localhost is just an example hostname. It is literally just "#username" and "#password". MailNews currently stores them like this because newsgroup servers don't have to have a username and password, the protocol allows for just a username.

Comment 3

[Mark Banner (:standard8)]

Attachment: Proposed Fix

This is the proposed fix, I need to write some more unit tests first.

Comment 4

[Justin Dolske [:Dolske]]

Looks ok.

Comment 5

[Mark Banner (:standard8)]

Attachment: The fix

Here's the fix with unit tests. I've checked these and the rest of the password manager tests work on Firefox and Thunderbird.

Comment 6

[Justin Dolske [:Dolske]]

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

>+// News is set up as an external protocol in Firefox's prefs, for this test
>+// to work, we need it to be internal.
>+var prefBranch = Cc["@mozilla.org/preferences-service;1"].
>+                 getService(Ci.nsIPrefBranch);
>+prefBranch.setBoolPref("network.protocol-handler.external.news", false);

What happens if this pref isn't flipped (and/or if NewsProtocolFactory isn't registered)?

Seems like a Firefox bug if it can't understand standard URI types, and possibly a login manager bug depending on what happens.


>@@ -107,7 +122,13 @@ dummyuser3.init("mailbox://localhost", n
> dummyuser3.init("mailbox://localhost", null, "mailbox://localhost",
>     "test+pop3", "pop3test", "", "");
> 
>-dummyuser4.init("http://dummyhost.mozilla.org", "", null,
>+dummyuser4.init("news://localhost", null, "news://localhost/#password",
>+    "", "newstest", "", "");
>+
>+dummyuser5.init("news://localhost", null, "news://localhost/#username",
>+    "", "testnews", "", "");
>+
>+dummyuserlast.init("http://dummyhost.mozilla.org", "", null,
>     "testuser1", "testpass1", "put_user_here", "put_pw_here");

Why "dummyuserlast"? There's no ordering guaranteed in the pwmgr results. Just add new dummyuser5 and dummyuser6 entries unless there's some reason for this.

Comment 7

[Mark Banner (:standard8)]

(In reply to comment #6)
> (From update of attachment 354212 [details] [diff] [review])
> >+// News is set up as an external protocol in Firefox's prefs, for this test
> >+// to work, we need it to be internal.
> >+var prefBranch = Cc["@mozilla.org/preferences-service;1"].
> >+                 getService(Ci.nsIPrefBranch);
> >+prefBranch.setBoolPref("network.protocol-handler.external.news", false);
> 
> What happens if this pref isn't flipped (and/or if NewsProtocolFactory isn't
> registered)?

If this pref isn't flipped, then Firefox builds will revert to trying to use the external URI handler for the protocol, which reverts it to using nsSimpleURI (which is the same in the non *ProtocolFactory cases). nsSimpleURI can't parse URIs, nsStandardURL can, hence for Firefox we define the protocol factories to 

> Seems like a Firefox bug if it can't understand standard URI types, and
> possibly a login manager bug depending on what happens.

Well it can understand standard URI types, it just can't parse them (or do things like get default port).

Comment 8

[Mark Banner (:standard8)]

Patch checked in: http://hg.mozilla.org/mozilla-central/rev/30d34ca12778

Comment 9

[Mark Banner (:standard8)]

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

Requesting 1.9.1 approval for this minor fix to importing/upgrading password files containing "news" protocol. This is needed for Thunderbird and SeaMonkey migration to toolkit password manager.

Should be low risk to Firefox as FF doesn't use news protocols, and this is in a migration path that has a reasonable number of unit tests as well.

Comment 10

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

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

a191=beltzner

Comment 11

[Mark Banner (:standard8)]

Checked into 1.9.1 branch:

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0da822e765f3