Last Comment Bug 470439 - Migration from signons.txt loses newsgroup username/password information.
: Migration from signons.txt loses newsgroup username/password information.
Status: RESOLVED FIXED
[tb3needs]
: fixed1.9.1
Product: Toolkit
Classification: Components
Component: Password Manager (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9.1b3
Assigned To: Mark Banner (:standard8)
:
Mentors:
Depends on:
Blocks: 433316
  Show dependency treegraph
 
Reported: 2008-12-19 09:00 PST by Mark Banner (:standard8)
Modified: 2008-12-30 02:14 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed Fix (884 bytes, patch)
2008-12-19 14:11 PST, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
The fix (9.72 KB, patch)
2008-12-22 13:11 PST, Mark Banner (:standard8)
dolske: review+
mbeltzner: approval1.9.1+
Details | Diff | Splinter Review

Description Mark Banner (:standard8) 2008-12-19 09:00:33 PST
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] 2008-12-19 13:17:48 PST
(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) 2008-12-19 13:55:28 PST
(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) 2008-12-19 14:11:44 PST
Created attachment 353888 [details] [diff] [review]
Proposed Fix

This is the proposed fix, I need to write some more unit tests first.
Comment 4 Justin Dolske [:Dolske] 2008-12-19 17:43:27 PST
Looks ok.
Comment 5 Mark Banner (:standard8) 2008-12-22 13:11:04 PST
Created attachment 354212 [details] [diff] [review]
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] 2008-12-23 14:01:48 PST
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) 2008-12-23 14:26:43 PST
(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) 2008-12-28 07:03:18 PST
Patch checked in: http://hg.mozilla.org/mozilla-central/rev/30d34ca12778
Comment 9 Mark Banner (:standard8) 2008-12-28 11:37:44 PST
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] 2008-12-29 23:02:15 PST
Comment on attachment 354212 [details] [diff] [review]
The fix

a191=beltzner
Comment 11 Mark Banner (:standard8) 2008-12-30 02:14:58 PST
Checked into 1.9.1 branch:

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

Note You need to log in before you can comment on or make changes to this bug.