The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 3.1b2

Status

MailNews Core
Networking: SMTP
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Bienvenu, Assigned: Bienvenu)

Tracking

unspecified
Thunderbird 3.1b2
x86
Mac OS X
Bug Flags:
in-testsuite +

Thunderbird Tracking Flags

(blocking-thunderbird3.1 beta2+, thunderbird3.1 beta2-fixed, thunderbird3.0 .5-fixed)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

7 years ago
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?
Blocks: 543957
(Assignee)

Comment 1

7 years ago
blocking
Flags: blocking-thunderbird3.1? → blocking-thunderbird3.1+

Updated

7 years ago
blocking-thunderbird3.1: --- → needed
Flags: blocking-thunderbird3.1+

Comment 2

7 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

7 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

7 years ago
blocking-thunderbird3.1: needed → beta2+

Comment 4

7 years ago
xref bug 533006 (connection drop, POP3)

Comment 5

7 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

7 years ago
Whiteboard: [will look at this tomorrow - probably won't take more than a day or 2]
(Assignee)

Comment 6

7 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

7 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

7 years ago
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.
(Assignee)

Comment 9

7 years ago
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.
Attachment #431520 - Attachment is obsolete: true
(Assignee)

Comment 10

7 years ago
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)
Attachment #431641 - Attachment is obsolete: true
(Assignee)

Updated

7 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

7 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

7 years ago
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.
Attachment #431712 - Attachment is obsolete: true
(Assignee)

Comment 13

7 years ago
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.
Attachment #431733 - Attachment is obsolete: true
(Assignee)

Comment 14

7 years ago
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.
Attachment #431739 - Attachment is obsolete: true
Attachment #431751 - Flags: superreview?(bugzilla)
Attachment #431751 - Flags: review?(bugzilla)
(Assignee)

Updated

7 years ago
Whiteboard: [have working patch; working on unit test] → [have patch with unit test for r/sr standard8]
(Assignee)

Comment 15

7 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

7 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

7 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

7 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.
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 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

7 years ago
Might be related to bug 554044.
(Assignee)

Comment 22

7 years ago
this test also passes for me once I fix the bit-rot. I'll try it on mac next.
(Assignee)

Comment 23

7 years ago
Created attachment 434657 [details] [diff] [review]
de-bit-rotted patch
Attachment #431751 - Attachment is obsolete: true
(Assignee)

Comment 24

7 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

7 years ago
test also worked with mac release build.
Attachment #434657 - Flags: superreview?(bugzilla)
Attachment #434657 - Flags: superreview+
Attachment #434657 - Flags: review?(bugzilla)
Attachment #434657 - Flags: review+
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.
Whiteboard: [have patch with unit test for r/sr standard8] → [has review][needs minor tweak to patch]
(Assignee)

Comment 27

7 years ago
fix checked in.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
status-thunderbird3.1: --- → beta2-fixed
Flags: in-testsuite+
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Whiteboard: [has review][needs minor tweak to patch]
Attachment #434657 - Flags: approval-thunderbird3.0.5+
(Assignee)

Comment 28

7 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

7 years ago
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.
(Assignee)

Comment 30

7 years ago
fixed for 3.0.5
status-thunderbird3.0: wanted → .5-fixed
blocking-thunderbird3.0: ? → ---
You need to log in before you can comment on or make changes to this bug.