Closed
Bug 542259
Opened 15 years ago
Closed 15 years ago
not prompting for new smtp password in some cases, e.g., when server drops the connection on bad authentication
Categories
(MailNews Core :: Networking: SMTP, defect)
Tracking
(blocking-thunderbird3.1 beta2+, thunderbird3.1 beta2-fixed, thunderbird3.0 .5-fixed)
RESOLVED
FIXED
Thunderbird 3.1b2
Tracking | Status | |
---|---|---|
blocking-thunderbird3.1 | --- | beta2+ |
thunderbird3.1 | --- | beta2-fixed |
thunderbird3.0 | --- | .5-fixed |
People
(Reporter: Bienvenu, Assigned: Bienvenu)
References
Details
Attachments
(2 files, 6 obsolete files)
12.29 KB,
patch
|
standard8
:
review+
standard8
:
superreview+
standard8
:
approval-thunderbird3.0.5+
|
Details | Diff | Splinter Review |
11.77 KB,
patch
|
Details | Diff | Splinter Review |
We've had reports of users unable to send mail. It turned out that removing the saved password from the password manager enabled them to send mail, which strongly implies that we're not reprompting for a password in some logon failures, and the most likely issue is that the server is dropping the connection on the failed logon, and we're not getting to the password reprompt code on connection errors.
this should block the smtp issues tracking bug.
Flags: blocking-thunderbird3.1?
Assignee | ||
Comment 1•15 years ago
|
||
blocking
Flags: blocking-thunderbird3.1? → blocking-thunderbird3.1+
Updated•15 years ago
|
blocking-thunderbird3.1: --- → needed
Flags: blocking-thunderbird3.1+
Comment 2•15 years ago
|
||
bienvenu told me that SMTP server serdan.ro:25 is one of the offenders here.
bienvenu can't test, because his ISP doesn't let him out on port 25.
I tested serdan.ro:25 and entered nonsense after "AUTH PLAIN" and "AUTH LOGIN", and the server always responded properly with a nice "535 Error: authentication failed", no connection drop.
It could of course be that you need a valid username and wrong password to trigger the issue, and although I can't rule it out, I doubt that, given that AUTH PLAIN has username and password in one token. It could be a server-internal issue, which triggers with valid username and wrong password, but I think that case would have been tested. So, in conclusion, I don't think serdan.ro:25 shows that "connection drop" misbehaviour.
Of course I can't speak for other servers, but we'd need to see testcases.
Assignee | ||
Comment 3•15 years ago
|
||
the case we were looking at, iirc, is that cram-md5 drops the connection after a correct username and password, so you do need the correct info...
Updated•15 years ago
|
blocking-thunderbird3.1: needed → beta2+
Comment 4•15 years ago
|
||
xref bug 533006 (connection drop, POP3)
Comment 5•15 years ago
|
||
What I've seen working with some users on GetSatisfaction is that if you have a saved password which is (now) incorrect, certain servers (yahoo/hotmail) will close the connection after the first AUTH attempt, so TB never gets a chance to try the other method (LOGIN/PLAIN) -- which would presumably then give the "failed password" prompt and allow you to enter a new password. Instead, the send fails with a "connection lost in the middle of the transaction" (i.e., you never get told that authentication failed, and you never get a chance to change your password).
This can also occur when the user name is incorrect (which was the case for many new TB users); you don't get notified that the authentication failed, only that the "connection was lost in the middle of the transaction".
Assignee | ||
Updated•15 years ago
|
Whiteboard: [will look at this tomorrow - probably won't take more than a day or 2]
Assignee | ||
Comment 6•15 years ago
|
||
I debugged this a bit this morning. When the server drops the connection on a bad password, nsSmtpProtocol::OnStopRequest is called, and we don't get back into nsSmtpProtocol::ProcessProtocolState. The reprompting for a password happens in AuthLoginResponse, which is called from the SMTP_AUTH_EXTERNAL_RESPONSE and SMTP_AUTH_LOGIN_RESPONSE states of ProcessProtocolState.
Because the server has dropped the connection, we can't really continue to use the nsSmtpProtocol object, which means retrying won't work, even if we do call AuthLoginResponse() in this case. So we have a couple choices - allow protocol objects to be re-used after connection failure, which would involve teaching them how to reconnect, or add retry logic including password reprompting to smtp urls. One advantage of re-using protocol objects would be that fallback between auth mechanisms would be easier.
SMTP actually has pretty well-defined error codes so we have a reasonable chance of recognizing authentication errors.
Comment 7•15 years ago
|
||
(In reply to comment #6)
> [...] The reprompting for a password [...]
> [...] add retry logic including password reprompting [...]
Note that, as in many cases I saw, it could be the user name that's bad, not the password. Notifying the user that the authentication failed, due to either "invalid user name" or "incorrect password", would then alert them that one of the two is wrong.
Assignee | ||
Comment 8•15 years ago
|
||
this makes it so connection drops on auth failures makes us reprompt for a password, and retry the url if the user asks. I haven't tested this patch with a server that drops the connection, but I will try to do so tomorrow.
Assignee | ||
Comment 9•15 years ago
|
||
this is closer to working - the compose window doesn't close after the send, probably because we lost the url listener, but the send does happen after reprompting for password. We still put up the server disconnected error, which I should suppress.
Attachment #431520 -
Attachment is obsolete: true
Assignee | ||
Comment 10•15 years ago
|
||
this patch works reasonably well for me. I'm going to generate try server builds for folks to try, and work on the unit tests (I've extended the smtp fake server to optionally drop the connection on auth failures, and started working on a test that tests that, but it's not done)
Attachment #431641 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Whiteboard: [will look at this tomorrow - probably won't take more than a day or 2] → [have working patch; working on unit test]
Target Milestone: --- → Thunderbird 3.1b2
Comment 11•15 years ago
|
||
You could probably easily adapt my test_smtpAuthMethod.js test for that.
As for the connection drop, I implemented that as well in the test_pop3ServerBrokenCRAMDisconnect.js test, you can use that as example.
http://hg.mozilla.org/users/mozilla.BenB_bucksch.org/mail-auth/file/a533c15fa9d3/mailnews/compose/test/unit/test_smtpAuthMethods.js
http://hg.mozilla.org/users/mozilla.BenB_bucksch.org/mail-auth/file/a533c15fa9d3/mailnews/local/test/unit/test_pop3ServerBrokenCRAMDisconnect.js
Assignee | ||
Comment 12•15 years ago
|
||
since LoadUrl is calling Initialize, I don't need to break out InitTransport, which makes the patch quite a bit smaller and less scary.
Attachment #431712 -
Attachment is obsolete: true
Assignee | ||
Comment 13•15 years ago
|
||
turned out not to be a good idea to try to convert to nsresults, because that broke normal password failure, and made the unit tests fail.
Attachment #431733 -
Attachment is obsolete: true
Assignee | ||
Comment 14•15 years ago
|
||
This adds a unit test to the bug fix, and tweaks the fake smtp server to optionally drop the connection on an authentication error.
The core patch checks for auth errors in OnStopRequest, and calls code that puts up the password reprompt dialog. If the user doesn't cancel, we rerun the url. I moved initialization of the smtp url into the LoadUrl call, which means LoadUrl twice will do re-initialization of the url.
Attachment #431739 -
Attachment is obsolete: true
Attachment #431751 -
Flags: superreview?(bugzilla)
Attachment #431751 -
Flags: review?(bugzilla)
Assignee | ||
Updated•15 years ago
|
Whiteboard: [have working patch; working on unit test] → [have patch with unit test for r/sr standard8]
Assignee | ||
Comment 15•15 years ago
|
||
I've submitted a try server request for a 3.04 pre build with this patch. Michael, I think this will also handle auth failures for bad user names, though it will do so by reprompting for the password, which I think is the same thing we do if the server gives an auth failure but doesn't drop the connection.
Comment 16•15 years ago
|
||
(In reply to comment #15)
> [...] handle auth failures for bad user names [...] reprompting for the password [...] same thing
> we do if the server gives an auth failure but doesn't drop the connection.
True. This should make some Yahoo/Hotmail user's experiences much less confusing when they've configured an invalid username/password.
I'm curious about one part of the fix:
> if (NS_FAILED(rv) && m_prefTrySSL == PREF_SECURE_TRY_STARTTLS)
> {
> m_prefTrySSL = PREF_SECURE_NEVER;
> rv = OpenNetworkSocket(aURL, nsnull, callbacks);
> }
Isn't "Try SSL" a holdover from 2.x (3.x is explicit "SSL/TLS") ? Is it advisable to "fall back" to an insecure connection method if the user configured "SSL/TLS" ? Or is there a prefs option to indicate "Try SSL" (vs. "SSL/TLS") in 3.x ?
Assignee | ||
Comment 17•15 years ago
|
||
(In reply to comment #16)
> (In reply to comment #15)
> > [...] handle auth failures for bad user names [...] reprompting for the password [...] same thing
> > we do if the server gives an auth failure but doesn't drop the connection.
>
> True. This should make some Yahoo/Hotmail user's experiences much less
> confusing when they've configured an invalid username/password.
>
> I'm curious about one part of the fix:
> > if (NS_FAILED(rv) && m_prefTrySSL == PREF_SECURE_TRY_STARTTLS)
> > {
> > m_prefTrySSL = PREF_SECURE_NEVER;
> > rv = OpenNetworkSocket(aURL, nsnull, callbacks);
> > }
>
You're looking at an older patch - I'm not changing that code for this fix (I never was; older patches just moved that code...). BenB might very well be reworking that in his larger auth patch.
Comment 18•15 years ago
|
||
Michael, in addition to PREF_SECURE_TRY_STARTTLS, there's also an PREF_SECURE_ALWAYS_STARTTLS, as you can easily see in the source. The former is no longer exposed in the 3.x UI.
Comment 19•15 years ago
|
||
I tweaked the test to work with Ben's changes on trunk:
diff --git a/mailnews/compose/test/unit/test_smtpPasswordFailure3.js b/mailnews/compose/test/unit/test_smtpPasswordFailure3.js
--- a/mailnews/compose/test/unit/test_smtpPasswordFailure3.js
+++ b/mailnews/compose/test/unit/test_smtpPasswordFailure3.js
@@ -120,10 +120,8 @@ function run_test() {
// This time with auth
test = "Auth sendMailMessage";
- smtpServer.authMethod = 1;
- smtpServer.useSecAuth = false;
- smtpServer.trySecAuth = false;
- smtpServer.trySSL = false;
+ smtpServer.authMethod = Ci.nsMsgAuthMethod.passwordCleartext;
+ smtpServer.socketType = Ci.nsMsgSocketType.plain;
smtpServer.username = kUsername;
dump("Send\n");
However, I then got:
TEST-UNEXPECTED-FAIL | /Users/moztest/comm/main/tb/mozilla/_tests/xpcshell/test_compose/unit/test_smtpPasswordFailure3.js | 2152398868 == 0 - See following stack:
JS frame :: /Users/moztest/comm/main/src/mozilla/testing/xpcshell/head.js :: do_throw :: line 257
JS frame :: /Users/moztest/comm/main/src/mozilla/testing/xpcshell/head.js :: do_check_eq :: line 287
JS frame :: /Users/moztest/comm/main/tb/mozilla/_tests/xpcshell/test_compose/unit/test_smtpPasswordFailure3.js :: anonymous :: line 149
That error code equates to NS_ERROR_NET_RESET. So I guess we're now not handling something we should be.
Comment 20•15 years ago
|
||
Comment on attachment 431751 [details] [diff] [review]
proposed fix with unit test
Denying review until we sort out the test fixes. The basic idea does look reasonable though.
Attachment #431751 -
Flags: superreview?(bugzilla)
Attachment #431751 -
Flags: review?(bugzilla)
Attachment #431751 -
Flags: review-
Comment 21•15 years ago
|
||
Might be related to bug 554044.
Assignee | ||
Comment 22•15 years ago
|
||
this test also passes for me once I fix the bit-rot. I'll try it on mac next.
Assignee | ||
Comment 23•15 years ago
|
||
Attachment #431751 -
Attachment is obsolete: true
Assignee | ||
Comment 24•15 years ago
|
||
Comment on attachment 434657 [details] [diff] [review]
de-bit-rotted patch
this also works on the mac. I guess I can try a release build as well to see if it's one of those weird tests that works in debug but not release mode.
Attachment #434657 -
Flags: superreview?(bugzilla)
Attachment #434657 -
Flags: review?(bugzilla)
Assignee | ||
Comment 25•15 years ago
|
||
test also worked with mac release build.
Updated•15 years ago
|
Attachment #434657 -
Flags: superreview?(bugzilla)
Attachment #434657 -
Flags: superreview+
Attachment #434657 -
Flags: review?(bugzilla)
Attachment #434657 -
Flags: review+
Comment 26•15 years ago
|
||
Comment on attachment 434657 [details] [diff] [review]
de-bit-rotted patch
> /**
> * This handler implements the bare minimum required by RFC 2821.
> * @see RFC 2821
> */
> function SMTP_RFC2821_handler(daemon) {
> this._daemon = daemon;
> this.closing = false;
>+ this.dropOnAuthFailure = false;
nit: please add some documentation for this in the comment above the start of the handler, so that readers know this handler supports it and what it does.
Updated•15 years ago
|
Whiteboard: [have patch with unit test for r/sr standard8] → [has review][needs minor tweak to patch]
Assignee | ||
Comment 27•15 years ago
|
||
fix checked in.
Status: NEW → RESOLVED
Closed: 15 years ago
status-thunderbird3.1:
--- → beta2-fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Assignee | ||
Updated•15 years ago
|
Whiteboard: [has review][needs minor tweak to patch]
Updated•15 years ago
|
Attachment #434657 -
Flags: approval-thunderbird3.0.5+
Assignee | ||
Comment 28•15 years ago
|
||
having a challenging time getting the test back-ported to 3.0.x - will try to get it working and landed this morning.
Assignee | ||
Comment 29•15 years ago
|
||
this is what I'll land on the branch. The smtp test infrastructure changed quite a bit, so it took me a while to get the test working again.
Assignee | ||
Comment 30•15 years ago
|
||
fixed for 3.0.5
Updated•15 years ago
|
blocking-thunderbird3.0: ? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•