Closed Bug 469807 Opened 11 years ago Closed 11 years ago

SMTP protocol needs protocol handlers for toolkit password manager.

Categories

(MailNews Core :: Networking: SMTP, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b2

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached patch The fix (obsolete) — Splinter Review
The toolkit password manager assumes that for the passwords saved in it, it can call IOService.newURI on it.

The SMTP protocol currently doesn't have any protocols registered for it, this means the IOService returns a simple URI, which doesn't do everything we need and hence the toolkit password manager doesn't work for it. The patch I'm attaching registers the SMTP and SMTPS protocols. Although we're not currently going to be using SMTPS I don't think it hurts to have it registered if we decide to use it later.

The only downside I can see to having these registered is that smtp:// links in emails will get linked but they won't do anything when clicked. I haven't found a way to disable them getting linked - I guess there probably isn't a way.

This patch also has basic tests to check we get the protocol ok, and the basic url is working as we expect. If you were to build with the toolkit password manager you'd also find that test_smtpPassword.js (which tests using passwords from the current wallet file) passes with this patch.
Attachment #353202 - Flags: superreview?(dmose)
Attachment #353202 - Flags: review?(neil)
Comment on attachment 353202 [details] [diff] [review]
The fix

>+    classDescription: aProtocol + " Protocol Handler",
Care to throw in a .toUpperCase()?

>+      nsIProtocolHandler.URI_NOAUTH |
I don't think this one fits, it means "no user or host"...

>+function nsSMTPProtocolHandler() {}
>+
>+nsSMTPProtocolHandler.prototype =
>+  makeProtocolHandler("smtp",
>+                      Components.interfaces.nsISmtpUrl.DEFAULT_SMTP_PORT,
>+                      "b14c2b67-8680-4c11-8d63-9403c7d4f757");
>+
>+function nsSMTPSProtocolHandler() {}
>+
>+nsSMTPSProtocolHandler.prototype =
>+  makeProtocolHandler("smtps",
>+                      Components.interfaces.nsISmtpUrl.DEFAULT_SMTPS_PORT,
>+                      "057d0997-9e3a-411e-b4ee-2602f53fe05f");
>+
>+function NSGetModule(compMgr, fileSpec) {
>+  return XPCOMUtils.generateModule([nsSMTPProtocolHandler,
>+                                    nsSMTPSProtocolHandler]);
>+}
Ugh, generateModule is really lame here :-(
(In reply to comment #1)
> Ugh, generateModule is really lame here :-(

Well I've changed it slightly but I don't see a better way :-(
Attached patch The fix v2Splinter Review
Revised patch to address Neil's comments.
Attachment #353202 - Attachment is obsolete: true
Attachment #353396 - Flags: superreview?(dmose)
Attachment #353396 - Flags: review?(neil)
Attachment #353202 - Flags: superreview?(dmose)
Attachment #353202 - Flags: review?(neil)
Attachment #353396 - Flags: review?(neil) → review+
Attachment #353396 - Flags: superreview?(dmose) → superreview?(bienvenu)
Attachment #353396 - Flags: superreview?(bienvenu) → superreview+
Patch checked in: http://hg.mozilla.org/comm-central/rev/b8799ccd6a51
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b3
Thankfully I took a look at the build logs today and realised we were missing nsSMTPProtocolHandler.js from the packages files. Otherwise we could have had a few broken passwords...
Attachment #355932 - Flags: review?(neil)
Attachment #355932 - Flags: review?(bienvenu)
Correct patch this time...
Attachment #355932 - Attachment is obsolete: true
Attachment #355933 - Flags: review?(neil)
Attachment #355933 - Flags: review?(bienvenu)
Attachment #355932 - Flags: review?(neil)
Attachment #355932 - Flags: review?(bienvenu)
Attachment #355933 - Flags: review?(bienvenu) → review+
Attachment #355933 - Flags: review?(neil) → review+
Product: Core → MailNews Core
Target Milestone: mozilla1.9.1b3 → Thunderbird 3.0b2
You need to log in before you can comment on or make changes to this bug.