542259 - not prompting for new smtp password in some cases, e.g., when server drops the connection on bad authentication

Status: RESOLVED FIXED

(MailNews Core :: Networking: SMTP, defect)

Description

[David :Bienvenu]

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]

blocking

Comment 2

[Ben Bucksch (:BenB)]

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]

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)]

xref bug 533006 (connection drop, POP3)

Comment 5

[Michael A. Pasek]

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]

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]

(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]

Attachment: 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]

Attachment: 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]

Attachment: 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)]

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]

Attachment: 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]

Attachment: 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]

Attachment: 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]

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]

(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]

(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)]

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)]

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)]

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)]

Might be related to bug 554044.

Comment 22

[David :Bienvenu]

this test also passes for me once I fix the bit-rot. I'll try it on mac next.

Comment 23

[David :Bienvenu]

Attachment: de-bit-rotted patch

Comment 24

[David :Bienvenu]

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]

test also worked with mac release build.

Comment 26

[Mark Banner (:standard8)]

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]

fix checked in.

Comment 28

[David :Bienvenu]

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]

Attachment: 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]

fixed for 3.0.5