1288113 - When Authenticating to POP3 or IMAP accounts, failure messages are woefully uninformative

Status: RESOLVED FIXED

(MailNews Core :: Networking, defect)

Description

[TREMBLAY,Bernard]

Attachment: Server-identification-failed-01a.jpg

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160604131506

Steps to reproduce:

Let automatic update from pop3 or imap accounts operate


Actual results:

Message : 
<identification on server "..." failed>
<retry> <get new password> <cancel>

Lack of useful information, see expected


Expected results:

Message : 
<identification on server "ssl..." - account "xyz..." failed>
<retry> <get new password> <cancel>

Explanation : the user can have several mails accounts with separate passwords on the same server, then the message don't give any useful information in such case.

Comment 1

[:aceman]

Hi, wouldn't adding "username" be better to know which password is needed? I assume you have multiple accounts on the same server and it is not seen which one failed.

Comment 2

[TREMBLAY,Bernard]

Hi,
Exactly, this is the solution.

During updating the content of the accounts, there are several reasons to reach this message :
1- Process error : this is another problem that I have not referenced. It seems that the number of accounts (near 30) makes that, because of somewhere timing, identifications errors occurs and in such case the retry is (seems) successful
2- True errors for invalidated accounts or password changed which case need the "changed account password" or "cancel" action.

The first step is to know exactly which account (user address on the server).

For now each update takes more than ten minutes and sometimes twenty actions of validations and retries, often ended by a full new general update, sometimes with repeated errors on accounts when password have been changed. Then for these cases the true error can be detected only when messages are not received (need a test)...

Comment 3

[Wayne Mery (:wsmwk)]

(In reply to :aceman from comment #1)
> Hi, wouldn't adding "username" be better to know which password is needed? I
> assume you have multiple accounts on the same server and it is not seen
> which one failed.

not a duplicate?

Comment 4

[:aceman]

I don't see this as dupe of any of the password management bugs. This is about the password prompt dialog display.

But when the password is incorrect, I get this dialog:
"Sending of password for user XXX did not succeed. Mail server YYY responded: invalid user/password, bye"

In that one, server and user is clearly shown. However, there is another one as in the attached picture, which does not contain the username.

Comment 5

[:aceman]

Attachment: patch

This could do it.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 8816698 [details] [diff] [review]
patch

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

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +695,5 @@
> +# "%S" is the account name
> +mailServerLoginFailedTitleWithAccount=Login failed to account "%S"
> +# LOCALIZATION NOTE (mailServerLoginFailed1):
> +# %1$S is the host name of the server, %2$S is the user name
> +mailServerLoginFailed1=Login to server %1$S with username %2$S failed.

nit: adding 2 to strings is better, since 1 is so alike to l.

::: mailnews/base/util/nsMsgUtils.h
@@ +264,5 @@
>   */
>  NS_MSG_BASE nsresult MsgPromptLoginFailed(nsIMsgWindow *aMsgWindow,
> +                                          const nsACString &aHostname,
> +                                          const nsACString &aUsername,
> +                                          const nsAString &aAccountname,

not very nice to have mixed string type arguments with little reason. make theme all nsACString?

Comment 7

[:aceman]

(In reply to Magnus Melin from comment #6)
> nit: adding 2 to strings is better, since 1 is so alike to l.

OK

> ::: mailnews/base/util/nsMsgUtils.h
> @@ +264,5 @@
> >   */
> >  NS_MSG_BASE nsresult MsgPromptLoginFailed(nsIMsgWindow *aMsgWindow,
> > +                                          const nsACString &aHostname,
> > +                                          const nsACString &aUsername,
> > +                                          const nsAString &aAccountname,
> 
> not very nice to have mixed string type arguments with little reason. make
> theme all nsACString?

I didn't want to pass in a nsCString when the accountname is a nsString (UTF16) attribute. Or do you want to it to downconvert it at the callers? Even if that isn't lossy, all the passed in strings need to be upconverted back to utf16 inside MsgPromptLoginFailed().

Comment 8

[TREMBLAY,Bernard]

Comment on attachment 8816698 [details] [diff] [review]
patch

Hi,

seems good for user information.

The original problem come from the fact that the "user name" is often <my_name>@<name associated with site name hosted by server : the domain hosted>
The "user" displays only the <my_name>;
Then several <my_name> can be hosted on same server (sample ssl0.ovh.net>
while <name associated with site name hosted by server : the domain hosted> is missing.

This is allowed with multi-domain option on hosts (OVH for example). While in other cases there is bi-univocity between server and the @x.

I write this for complementary explanation of the origin of the problem

Comment 9

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #7)
> I didn't want to pass in a nsCString when the accountname is a nsString
> (UTF16) attribute. Or do you want to it to downconvert it at the callers?
> Even if that isn't lossy, all the passed in strings need to be upconverted
> back to utf16 inside MsgPromptLoginFailed().

Oh well, in one of the callers you're converting to utf16 as is. 
I'll leave it up to you if you want to change anything or not. Overall it's a very sad story that there's two type of strings at all :(

Comment 10

[:aceman]

(In reply to Magnus Melin from comment #9)
> Oh well, in one of the callers you're converting to utf16 as is. 

Yes and then I can save the upconverting inside the function. If the function took nsCString, the caller would not upconvert in this single call place but the function would. Then all the other callers would have to downconvert first and then the function upconvert again. I don't see in which your proposal would be faster, or easier to the callers.

> I'll leave it up to you if you want to change anything or not. Overall it's
> a very sad story that there's two type of strings at all :(

That's true. I still don't understand why there need to be multiple types, when it seems all of them can represent all internationalized strings, the difference is just whether it is UTF-8 or UTF-16.

Comment 11

[:aceman]

Attachment: patch v1.1

Comment 12

[Wayne Mery (:wsmwk)]

Can someone improve the bug summary?
(I don't understand from the summary what this bug is about, and at the very least is awkwardly worded)

Comment 13

[Philip Chee]

Comment on attachment 8817898 [details] [diff] [review]
patch v1.1

Review of suite strings:

> -mailServerLoginFailed=Login to server %S failed.
> +# LOCALIZATION NOTE (mailServerLoginFailedTitleWithAccount):
> +# "%S" is the account name
Terminate complete sentences with "." ...is the account name.

> +mailServerLoginFailedTitleWithAccount=Login failed to account "%S"
I think - Login to account "%S" failed. - sounds better in English.

> +# LOCALIZATION NOTE (mailServerLoginFailed2):
> +# %1$S is the host name of the server, %2$S is the user name
Terminate complete sentences with "." ...is the user name.

C++ style nit:

> +    const char16_t *argumentsTitle[] = { aAccountname.BeginReading() };
> +    rv = bundle->FormatStringFromName(
> +      u"mailServerLoginFailedTitleWithAccount",
> +      argumentsTitle, 1, getter_Copies(title));

The prevailing style in mailnews naming appears to be "formatString" or "fooFormatString" or "fooBarFormatString"
So perhaps titleFormatString ?

Comment 14

[:aceman]

Attachment: patch v1.2

I think "formatStrings" makes no sense, because it does not contain the format string, but the arguments that should be replaced instead of the placeholders in the format string which is retrieved elsewhere. But OK, let's be consistent.

Comment 15

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/af9716e8c0a97e99f5a542fac039c12c8ac3dac1