803994 - Thunderbird gives wrong error message for mail_max_userip_connections

Status: ASSIGNED

(MailNews Core :: Networking: IMAP, defect)

Description

[Matthieu Pupat]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20121011153927

Steps to reproduce:

Try to log to a dovecot IMAP server where mail_max_userip_connections was reached. Below is server dialog:

* OK [CAPABILITY IMAP4rev1 LITERAL+ SASL-IR LOGIN-REFERRALS ID ENABLE IDLE AUTH=PLAIN] Dovecot ready.
1 LOGIN username password
1 NO [UNAVAILABLE] Maximum number of connections from user+IP exceeded (mail_max_userip_connections=20)
closed



Actual results:

Thunderbird displayed a message that the password was incorrect and proposed to re-enter the password


Expected results:

Thunderbird should not say password is incorrect. It should display the error message given by the server: 
Maximum number of connections from user+IP exceeded (mail_max_userip_connections=20)

Comment 1

[David Lechner (:dlech)]

Attachment: patch v1

This seems to do the trick.

Comment 2

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 677298 [details] [diff] [review]
patch v1

Review of attachment 677298 [details] [diff] [review]:
-----------------------------------------------------------------

Before I go into too much detail with the review, is it possible to get nsImapServerResponseParser to follow the code path that leads to http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapServerResponseParser.cpp#268 in this case? That's the mechanism we use to display other BAD and NO response lines from the IMAP server, so I'd prefer to re-use that if it's not too difficult.

If it's not practical to use the fServerConnection.AlertUserEventFromServer(fCurrentLine) mechanism, then the existing patch is almost ready to use...

I'll leave the r? flag on this patch until we decide whether to change to this the AlertUserEventFromServer() approach.

::: mail/locales/en-US/chrome/messenger/imapMsgs.properties
@@ +418,5 @@
>  # Place the word %1$S in your translation where the name of the account should appear.
>  # Place the word %2$S in your translation where the alert from the server should appear.
>  5119=Alert from account %1$S: %2$S
> +
> +## @name IMAP_LOGIN_UNAVAILIBLE

spelling: unavailable (appears many places in the patch)

@@ +421,5 @@
> +
> +## @name IMAP_LOGIN_UNAVAILIBLE
> +## @loc None
> +# LOCALIZATION NOTE (5120): %S is the account name
> +5120=You cannot log in to %S because the server is currently unavailable. Please try again later.

We're trying to move away from using numeric codes for these - new messages should use string identifiers.

::: mailnews/imap/src/nsImapServerResponseParser.h
@@ +124,5 @@
>    bool            fCondStoreEnabled;  
>    bool            fUseModSeq;  // can use mod seq for currently selected folder
>    uint64_t        fHighestModSeq;
>  
> +  // used to notify when attempting authentication and server responds with [UNAVAIABLE]

spelling: UNAVAILABLE

Comment 3

[David Lechner (:dlech)]

(In reply to Irving Reid (:irving) from comment #2)
> Before I go into too much detail with the review, is it possible to get
> nsImapServerResponseParser to follow the code path that leads to
> http://mxr.mozilla.org/comm-central/source/mailnews/imap/src/
> nsImapServerResponseParser.cpp#268 in this case? 

It is indeed possible. I initially avoided doing this because virtually every imap command issued by Thunderbird expects to be in the selected state, which means that the error message will include the mailbox/folder name, which could be misleading since the actual error has nothing to do with a specific mailbox. It is much simpler this way though.


> > +## @name IMAP_LOGIN_UNAVAILIBLE
> > +## @loc None
> > +# LOCALIZATION NOTE (5120): %S is the account name
> > +5120=You cannot log in to %S because the server is currently unavailable. Please try again later.
> 
> We're trying to move away from using numeric codes for these - new messages
> should use string identifiers.

This is kind of a moot point because the new message is no longer needed per the suggestion above, but... If I change this to a string identifier, then I would have to either change all imap strings to have string identifiers or duplicate a number of functions to accept a string as a parameter in addition to an uint for the existing messages. I'd be glad to do this, but I think it needs a separate bug.

Comment 4

[David Lechner (:dlech)]

Attachment: patch v2

Simplified error message handling.

Comment 5

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 681283 [details] [diff] [review]
patch v2

Review of attachment 681283 [details] [diff] [review]:
-----------------------------------------------------------------

Code looks great; r=me with the comments fixed. Do you have an easy way to reproduce this problem from the UI, or can you attach a screen shot of what the error pop up looks like? I'd like to get Mike or Blake to do a quick UI review of that before we land.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +8353,5 @@
>        if (!loginSucceeded)
>        {
> +        // If server gave reason for authentication failure as [UNAVAILABLE]
> +        // then we skip authentication retries, etc.
> +        // User will be notified, but that is implemented elsewhere

Say where, in this comment - in this case, something like "the user is notified by the call to fServerConnection.AlertUserEventFromServer() near the end of nsImapServerResponseParser::ParseIMAPServerResponse()"

::: mailnews/imap/src/nsImapServerResponseParser.h
@@ +124,5 @@
>    bool            fCondStoreEnabled;  
>    bool            fUseModSeq;  // can use mod seq for currently selected folder
>    uint64_t        fHighestModSeq;
>  
> +  // used to notify when attempting authentication and server responds with [UNAVAIABLE]

Typo in comment - UNAVAILABLE

Comment 6

[David Lechner (:dlech)]

Attachment: for UI review

This is what the error message will look like using the actual message from the bug reporter.

Comment 7

[David Lechner (:dlech)]

Attachment: for UI review

If the server does not include an explanation, users will still see at least this.

Comment 8

[David Lechner (:dlech)]

Attachment: patch v3

fixed comments, r=irving from previous patch

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 682608 [details]
for UI review

Hrm. That's kind of a scary error message, and likely confusing to users who don't know what email servers are, or IP addresses.

What is the possibility of saying something a little higher level, but providing a link to low-level error information?

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 682610 [details]
for UI review

Surely we can translate this into something clearer - like

The current operation on 'Inbox' for account user@localhost did not succeed.

And then provide a link with more information?

Comment 11

[David Lechner (:dlech)]

(In reply to Mike Conley (:mconley) from comment #9)
> Comment on attachment 682608 [details]
> for UI review
> 
> Hrm. That's kind of a scary error message, and likely confusing to users who
> don't know what email servers are, or IP addresses.
> 
> What is the possibility of saying something a little higher level, but
> providing a link to low-level error information?

OK. I am just using an already existing message. All of the text before '[UNAVAILIBLE]' is shown in response to other server responses. Should I add a new message for only this particular case or fix up all cases. All cases would actually probably be easier, but warrant a new bug.

Comment 12

[David Lechner (:dlech)]

After thinking about this a bit, I think I should point out that these are the toaster messages that pop up just briefly. I don't think we want to add a 'more info' link to these. Users could not get the mouse there fast enough.

So, so we do a modal dialog with the error message or do we leave it as is or something else?

Comment 13

[Magnus Melin [:mkmelin]]

I'm inclined to think simplifying complex error situations is no real gain. You kind of need to know what the problem actually was to potentially do something about it (for a normal user: what to tell tech support, if it doesn't resolve by waiting). So even if the message is complex, its still better than using something thunderbird specific that doesn't say say exactly what the problem is.

Comment 14

[u520405]

I am still having this problem, tested on Ubuntu 12.04 LTS using Thunderbird 31.1.2.

It's quite a waste of time chasin the wrong problem, although logs clearly indicated the actual problem it took us some time to decide the error message was not coherent with the observed behavior.

Comment 15

[Magnus Melin [:mkmelin]]

The patch is very severely bitrotted. But can we move forward with this?

Comment 16

[Wayne Mery (:wsmwk)]

(In reply to Magnus Melin [:mkmelin] from comment #15)

The patch is very severely bitrotted. But can we move forward with this?

9 years later, "severely" may be an understatement. Is the bug (and patch) even still relevant?

Comment 18

[gene smith]

Attachment: Bug 803994 - If UNAVAILABLE response occurs at imap login, show server response & don't prompt for password. r=mkmelin

This rebases and updates the patch by David Lechner from 11 years ago using addition information from here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1602375#c6