Thunderbird gives wrong error message for mail_max_userip_connections

ASSIGNED
Assigned to

Status

MailNews Core
Networking: IMAP
ASSIGNED
5 years ago
3 years ago

People

(Reporter: Matthieu Pupat, Assigned: dlech, NeedInfo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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)

Updated

5 years ago
Component: Untriaged → Networking: IMAP
Product: Thunderbird → MailNews Core
(Assignee)

Comment 1

5 years ago
Created attachment 677298 [details] [diff] [review]
patch v1

This seems to do the trick.
Assignee: nobody → david
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #677298 - Flags: review?(irving)
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
(Assignee)

Comment 3

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

Comment 4

5 years ago
Created attachment 681283 [details] [diff] [review]
patch v2

Simplified error message handling.
Attachment #681283 - Flags: review?(irving)
(Assignee)

Updated

5 years ago
OS: Linux → All
Hardware: x86_64 → All
Version: 16 → Trunk
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
Attachment #681283 - Flags: ui-review?(mconley)
Attachment #681283 - Flags: review?(irving)
Attachment #681283 - Flags: review+
(Assignee)

Updated

5 years ago
Attachment #677298 - Attachment is obsolete: true
Attachment #677298 - Flags: review?(irving)
(Assignee)

Comment 6

5 years ago
Created attachment 682608 [details]
for UI review

This is what the error message will look like using the actual message from the bug reporter.
Attachment #682608 - Flags: ui-review?(mconley)
(Assignee)

Comment 7

5 years ago
Created attachment 682610 [details]
for UI review

If the server does not include an explanation, users will still see at least this.
Attachment #682610 - Flags: review?(mconley)
(Assignee)

Updated

5 years ago
Attachment #682610 - Flags: review?(mconley) → ui-review?(mconley)
(Assignee)

Comment 8

5 years ago
Created attachment 682626 [details] [diff] [review]
patch v3

fixed comments, r=irving from previous patch
Attachment #681283 - Attachment is obsolete: true
Attachment #681283 - Flags: ui-review?(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?
Attachment #682608 - Flags: ui-review?(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?
Attachment #682610 - Flags: ui-review?(mconley)
(Assignee)

Comment 11

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

Comment 12

5 years ago
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

5 years ago
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

3 years ago
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

3 years ago
The patch is very severely bitrotted. But can we move forward with this?
Flags: needinfo?(david)
You need to log in before you can comment on or make changes to this bug.