1176347 - IMAP uses thread-unsafe NTLM authentication on the IMAP thread

Status: NEW

(MailNews Core :: Networking: IMAP, defect)

Description

[Kent James (:rkent)]

In doing NTLM testing for bug 1174159 I get various assertions in a debug build, because IMAP is using the NTLM authentication module on the IMAP thread, while it is only designed for main thread.

Comment 1

[Wayne Mery (:wsmwk)]

Are we still doing this wrong?

Comment 2

[gene smith]

FWIW, haven't seen any bug reports for imap users logging in with NTLM. I assume one would need an Exchange server to test this?

Comment 3

[Ben Campbell]

I don't have an exchange server (or a windows build) to check on this, but tracing through the code by hand it does look like the nsIAuthModule functions (init() and getNextToken()) are being called from the IMAP thread.
I don't know the specifics on how M-C views accessing it's auth modules when not on main thread, but if the debug asserts in the original description are still happening, then that'd suggest M-C views it rather dimly :-)

The general flow of execution is:

nsIMapProtocol::ImapThreadMainLoop()
nsIMapProtocol::ProcessCurrentUrl()
nsIMapProtocol::TryToLogon()
nsIMapProtocol::AuthLogin()
nsMsgProtocol::DoNtlmStep1()/DoNtlmStep2()
nsIAuthModule.init()/getNextToken()

I guess the obvious simplest fix is to wrap the nsIAuthModule calls (in DoNtlmStepN()) with:
NS_DispatchToMainThread( ... , NS_DISPATCH_SYNC);

Looks like GSSAPI auth would also suffer from the same problem - it also use an nsIAuthModule.
(See nsMsgProtocol::DoGSSAPIStep1()/DoGSSAPIStep2()).
I think all the other IMAP auth implementations do their own thing.

It looks like the nsMsgProtocol NTLM and GSSAPI DoNtlm/GSSAPIStepN() helper functions could be merged... (after all, they both use nsIAuthModule, which is supposed to magic away the details!)

Comment 4

[Wayne Mery (:wsmwk)]

It looks like the nsMsgProtocol NTLM and GSSAPI DoNtlm/GSSAPIStepN() helper functions could be merged... (after all, they both use nsIAuthModule, which is supposed to magic away the details!)

Should we do that first? In a separate bug?