Last Comment Bug 542259 - not prompting for new smtp password in some cases, e.g., when server drops the connection on bad authentication
: not prompting for new smtp password in some cases, e.g., when server drops th...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Networking: SMTP (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: Thunderbird 3.1b2
Assigned To: David :Bienvenu
:
Mentors:
Depends on:
Blocks: 543957
  Show dependency treegraph
 
Reported: 2010-01-26 09:35 PST by David :Bienvenu
Modified: 2010-05-07 11:28 PDT (History)
4 users (show)
mozilla: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
beta2+
beta2-fixed
.5-fixed


Attachments
wip (7.25 KB, patch)
2010-03-09 17:50 PST, David :Bienvenu
no flags Details | Diff | Splinter Review
wip 2 (7.75 KB, patch)
2010-03-10 08:59 PST, David :Bienvenu
no flags Details | Diff | Splinter Review
working patch (8.78 KB, patch)
2010-03-10 14:16 PST, David :Bienvenu
no flags Details | Diff | Splinter Review
simpler patch (6.04 KB, patch)
2010-03-10 15:10 PST, David :Bienvenu
no flags Details | Diff | Splinter Review
even simpler, and now passes existing unit tests... (4.12 KB, patch)
2010-03-10 15:38 PST, David :Bienvenu
no flags Details | Diff | Splinter Review
proposed fix with unit test (12.88 KB, patch)
2010-03-10 16:42 PST, David :Bienvenu
standard8: review-
Details | Diff | Splinter Review
de-bit-rotted patch (12.29 KB, patch)
2010-03-24 13:57 PDT, David :Bienvenu
standard8: review+
standard8: superreview+
standard8: approval‑thunderbird3.0.5+
Details | Diff | Splinter Review
3.0.x branch patch (11.77 KB, patch)
2010-05-07 08:24 PDT, David :Bienvenu
no flags Details | Diff | Splinter Review

Description David :Bienvenu 2010-01-26 09:35:23 PST
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.
Comment 1 David :Bienvenu 2010-02-09 09:33:15 PST
blocking
Comment 2 Ben Bucksch (:BenB) 2010-03-01 11:57:13 PST
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.
Comment 3 David :Bienvenu 2010-03-01 12:09:43 PST
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...
Comment 4 Ben Bucksch (:BenB) 2010-03-05 12:30:50 PST
xref bug 533006 (connection drop, POP3)
Comment 5 Michael A. Pasek 2010-03-06 16:28:24 PST
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".
Comment 6 David :Bienvenu 2010-03-09 08:49:02 PST
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 Michael A. Pasek 2010-03-09 13:31:33 PST
(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.
Comment 8 David :Bienvenu 2010-03-09 17:50:45 PST
Created attachment 431520 [details] [diff] [review]
wip

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.
Comment 9 David :Bienvenu 2010-03-10 08:59:08 PST
Created attachment 431641 [details] [diff] [review]
wip 2

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.
Comment 10 David :Bienvenu 2010-03-10 14:16:12 PST
Created attachment 431712 [details] [diff] [review]
working patch

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)
Comment 11 Ben Bucksch (:BenB) 2010-03-10 14:31:36 PST
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
Comment 12 David :Bienvenu 2010-03-10 15:10:37 PST
Created attachment 431733 [details] [diff] [review]
simpler patch

since LoadUrl is calling Initialize, I don't need to break out InitTransport, which makes the patch quite a bit smaller and less scary.
Comment 13 David :Bienvenu 2010-03-10 15:38:49 PST
Created attachment 431739 [details] [diff] [review]
even simpler, and now passes existing unit tests...

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.
Comment 14 David :Bienvenu 2010-03-10 16:42:45 PST
Created attachment 431751 [details] [diff] [review]
proposed fix with unit test

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.
Comment 15 David :Bienvenu 2010-03-10 17:01:20 PST
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 Michael A. Pasek 2010-03-11 07:06:12 PST
(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 ?
Comment 17 David :Bienvenu 2010-03-11 07:10:46 PST
(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 Ben Bucksch (:BenB) 2010-03-11 09:21:12 PST
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 Mark Banner (:standard8) (afk until 26th July) 2010-03-22 13:01:58 PDT
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 Mark Banner (:standard8) (afk until 26th July) 2010-03-22 13:03:42 PDT
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.
Comment 21 Ben Bucksch (:BenB) 2010-03-22 13:19:35 PDT
Might be related to bug 554044.
Comment 22 David :Bienvenu 2010-03-24 13:41:19 PDT
this test also passes for me once I fix the bit-rot. I'll try it on mac next.
Comment 23 David :Bienvenu 2010-03-24 13:57:27 PDT
Created attachment 434657 [details] [diff] [review]
de-bit-rotted patch
Comment 24 David :Bienvenu 2010-03-24 14:39:02 PDT
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.
Comment 25 David :Bienvenu 2010-03-24 16:18:18 PDT
test also worked with mac release build.
Comment 26 Mark Banner (:standard8) (afk until 26th July) 2010-04-08 05:35:32 PDT
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.
Comment 27 David :Bienvenu 2010-04-08 15:29:31 PDT
fix checked in.
Comment 28 David :Bienvenu 2010-05-06 08:22:26 PDT
having a challenging time getting the test back-ported to 3.0.x - will try to get it working and landed this morning.
Comment 29 David :Bienvenu 2010-05-07 08:24:35 PDT
Created attachment 444093 [details] [diff] [review]
3.0.x branch patch

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.
Comment 30 David :Bienvenu 2010-05-07 08:25:59 PDT
fixed for 3.0.5

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