Closed
Bug 463096
Opened 16 years ago
Closed 16 years ago
make NNTP use socket type, and msgIncomingServer.isSecure readonly
Categories
(MailNews Core :: Networking: NNTP, defect)
MailNews Core
Networking: NNTP
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9.1b3
People
(Reporter: mkmelin, Assigned: mkmelin)
Details
Attachments
(1 file, 1 obsolete file)
34.16 KB,
patch
|
jcranmer
:
review+
mkmelin
:
superreview+
|
Details | Diff | Splinter Review |
From bug 462045 comment 1. > I'm also tempted to suggest a followup bug to make NNTP use the socket > type throughout (making isSecure readonly), but I imagine that might > prove a headache for existing profiles.
Comment 1•16 years ago
|
||
I'm sure we want this on Trunk, not on 1.8 branch. Also, it might be nice to kick out isSecure altogether while we're at it.
Version: 1.8 Branch → Trunk
Assignee | ||
Comment 2•16 years ago
|
||
Yeah, certainly meant to file against Trunk. I'd like to keep isSecure (as readonly), since it's so convenient to use in js when you don't care about sockets and just want know if it's secure or not.
Assignee | ||
Comment 3•16 years ago
|
||
From rfc 4642 - xref bug 420262. In some existing implementations, TCP port 563 has been dedicated to NNTP over TLS. These implementations begin the TLS negotiation immediately upon connection and then continue with the initial steps of an NNTP session. This use of TLS on a separate port is discouraged for the reasons documented in Section 7 of "Using TLS with IMAP, POP3 and ACAP" [TLS-IMAPPOP]. So the special port and snews:// should only be set for SSL. When STARTTLS was implemented for mail (bug 60377) it was supposed to care of the migration. I haven't investigated fully but secnews servers get a socketType 0 pref written out - I think it's because in the prefs UI the socketType stuff is hidden, and then get's persisted as default... so the migration won't kick in for news.
Assignee | ||
Comment 4•16 years ago
|
||
Attachment #351272 -
Flags: superreview?(neil)
Attachment #351272 -
Flags: review?(Pidgeot18)
Comment 5•16 years ago
|
||
/me wonders how many of his mega-patches this bitrots. Nothing jumps out at me from the patch, but I'll need to test this first.
Status: NEW → ASSIGNED
Comment 6•16 years ago
|
||
Comment on attachment 351272 [details] [diff] [review] proposed fix >+ var url = (server.socketType == Components.interfaces.nsIMsgIncomingServer.useSSL) ? >+ "snews://" : "news://"; >+ url += server.hostName + ":" + server.port + "/" + hdr.messageId; So, this is in case we start supporting TLS for news? How forward-thinking of you :-) The patch did look somewhat bigger than I originally expected! >- m_rootFolder->NotifyBoolPropertyChanged(NS_NewAtom("isSecure"), isSecure, aIsSecure); So, we introduced a bug in the previous patch that changing the socket type of a mail server failed to notify the UI to update the icon :-( I guess this notification needs to be moved to SetSocketType. sr=me with that fixed. >+ PRInt32 socketType; >+ rv = server->GetSocketType(&socketType); >+ NS_ENSURE_SUCCESS(rv, rv); (See later.) >+ PRBool isSecure = PR_FALSE; >+ rv = server->GetIsSecure(&isSecure); >+ NS_ENSURE_SUCCESS(rv, rv); > > // When we are making a secure connection, we need to make sure that we > // pass an interface requestor down to the socket transport so that PSM can > // retrieve a nsIPrompt instance if needed. > nsCOMPtr<nsIInterfaceRequestor> ir; > if (isSecure && aMsgWindow) { > nsCOMPtr<nsIDocShell> docShell; > aMsgWindow->GetRootDocShell(getter_AddRefs(docShell)); > ir = do_QueryInterface(docShell); > } I have a feeling that this should work off the socket type too, for all non-default socket types. >+ PRInt32 socketType; >+ rv = server->GetSocketType(&socketType); >+ NS_ENSURE_SUCCESS(rv, rv); I wonder if it's worth getting the socket type earlier on, it looks as if it's used in at least two places. >- * H�kan Waara <hwaara@chello.se> >+ * Håkan Waara <hwaara@chello.se> Bah, is he still giving us grief? ;-) >- const char *newsScheme = (isSecure) ? SNEWS_SCHEME : NEWS_SCHEME; >+ const char *newsScheme = (socketType == nsIMsgIncomingServer::useSSL) ? >+ SNEWS_SCHEME : NEWS_SCHEME; Nit: you don't actually need to use those ()s if you don't want to ;-) >+ if (NS_SUCCEEDED(rv) && NS_SUCCEEDED(mPrefBranch->GetBoolPref("isSecure", &isSecure))) { I don't see the point of testing the old value here.
Attachment #351272 -
Flags: superreview?(neil) → superreview+
Assignee | ||
Comment 7•16 years ago
|
||
(In reply to comment #6) > (From update of attachment 351272 [details] [diff] [review]) > >+ var url = (server.socketType == Components.interfaces.nsIMsgIncomingServer.useSSL) ? > >+ "snews://" : "news://"; > >+ url += server.hostName + ":" + server.port + "/" + hdr.messageId; > So, this is in case we start supporting TLS for news? How forward-thinking of > you :-) The patch did look somewhat bigger than I originally expected! Yes, TLS for news looks like it might be fairly easy to implement. Don't know if any servers support it though. > >- m_rootFolder->NotifyBoolPropertyChanged(NS_NewAtom("isSecure"), isSecure, aIsSecure); > So, we introduced a bug in the previous patch that changing the socket type of > a mail server failed to notify the UI to update the icon :-( I guess this > notification needs to be moved to SetSocketType. sr=me with that fixed. Sure. It's not needed in thunderbird - due to the non-rdf folder pane? > >- const char *newsScheme = (isSecure) ? SNEWS_SCHEME : NEWS_SCHEME; > >+ const char *newsScheme = (socketType == nsIMsgIncomingServer::useSSL) ? > >+ SNEWS_SCHEME : NEWS_SCHEME; > Nit: you don't actually need to use those ()s if you don't want to ;-) I think it's a bit more readable with the braces. > >+ if (NS_SUCCEEDED(rv) && NS_SUCCEEDED(mPrefBranch->GetBoolPref("isSecure", &isSecure))) { > I don't see the point of testing the old value here. The point is that for new profiles there's no need to write out the isSecure pref. So here I check if the pref is written out - if it's not, let's not start here. When the pref isn't written out, the migration won't have to run for new profiles so there's a small win.
Assignee | ||
Comment 8•16 years ago
|
||
Carrying fwd sr=neil
Attachment #351272 -
Attachment is obsolete: true
Attachment #351713 -
Flags: superreview+
Attachment #351713 -
Flags: review?(Pidgeot18)
Attachment #351272 -
Flags: review?(Pidgeot18)
Updated•16 years ago
|
Attachment #351713 -
Flags: review?(Pidgeot18) → review+
Comment 9•16 years ago
|
||
Comment on attachment 351713 [details] [diff] [review] proposed fix, v2 >diff --git a/mailnews/news/src/nsNNTPProtocol.cpp b/mailnews/news/src/nsNNTPProtocol.cpp >@@ -419,34 +418,33 @@ NS_IMETHODIMP nsNNTPProtocol::Initialize > if (port<=0) { >- if (isSecure) { >+ if (socketType == nsIMsgIncomingServer::useSSL) > port = SECURE_NEWS_PORT; >- } >- else { >+ else > port = NEWS_PORT; >- } > } Why not a conditional statement here? >@@ -478,44 +476,45 @@ NS_IMETHODIMP nsNNTPProtocol::Initialize >- if (isSecure && aMsgWindow) { >+ if (socketType != nsIMsgIncomingServer::defaultSocket && aMsgWindow) { Nit: Since you're already here, change the { to be on the next line. >- nsCOMPtr<nsIMsgIncomingServer> server = do_QueryInterface(m_nntpServer); >- if (server) >- server->GetRealHostName(hostName); >+ >+ server->GetRealHostName(hostName); >+ NS_ENSURE_SUCCESS(rv, rv); Either put in an rv = in there or get rid of the NS_ENSURE_SUCCESS(rv, rv); >- * H�kan Waara <hwaara@chello.se> >+ * Håkan Waara <hwaara@chello.se> <aside>/me kicks vim for assuming an ISO 8859-1 charset</aside> >diff --git a/mailnews/news/src/nsNntpIncomingServer.cpp b/mailnews/news/src/nsNntpIncomingServer.cpp >+NS_IMETHODIMP >+nsNntpIncomingServer::SetSocketType(PRInt32 aSocketType) >+{ >+ if (!mPrefBranch) >+ return NS_ERROR_NOT_INITIALIZED; >+ nsresult rv = nsMsgIncomingServer::SetSocketType(aSocketType); >+ if (NS_SUCCEEDED(rv)) { Nit: { on next line, please. >+ PRBool isSecure = PR_FALSE; >+ if (NS_SUCCEEDED(mPrefBranch->GetBoolPref("isSecure", &isSecure))) { Ditto. >+ // Must keep isSecure in sync since we migrate based on it... if it's set. >+ rv = mPrefBranch->SetBoolPref("isSecure", aSocketType == nsIMsgIncomingServer::useSSL); I'd like to keep 80-line width, if reasonable (this seems to be the only egregious violation). >diff --git a/mailnews/news/src/nsNntpService.cpp b/mailnews/news/src/nsNntpService.cpp >- rv = (*aServer)->SetIsSecure(aIsSecure); >- if (NS_FAILED(rv)) return rv; >+ if (aUseSSL) { >+ rv = (*aServer)->SetSocketType(nsIMsgIncomingServer::useSSL); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } Nit: { on next line. I don't see any functional issues, so r+ modulo those nits.
Comment 10•16 years ago
|
||
(In reply to comment #7) > Yes, TLS for news looks like it might be fairly easy to implement. Don't know > if any servers support it though. Bug 420262. The original reporter is working on adding RFC 3977+ support to INN; news.trigofacile.com has this partial implementation, and I've verified via commandline that it supports STARTTLS (well, it doesn't return a 500 to STARTTLS).
Assignee | ||
Comment 11•16 years ago
|
||
Nits fixed, pushed. changeset: 1360:8dbdd7537b67 http://hg.mozilla.org/comm-central/rev/8dbdd7537b67 ->FIXED
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b3
Updated•15 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•