555448 - [autoconfig] Wrong error msg: Says "Username or password invalid", although it could be any kind of error (verifyConfig)

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect, P1)

Description

[Ben Bucksch (:BenB)]

Reproduction:
1. File | New | Mail account...
2. Enter [e-mail] and continue
3. Disconnect network, or any other error
4. Click on "Create account"

Actual result:
Dialog says "username and/or password invalid"

Expected result:
The actual error is is shown, whatever was the cause for the failed verification.
There are thousands of reasons why the check may have failed: network down, server down, server malfunctioning, our network code malfunctioning, the dialog code having some exception (just add "throw 'foo';" somewhere in verifyConfig.js).
"username and/or password invalid" MUST NOT be shown unless we are certain that it was the username or password, or at least that the server refused the authentication. If the server gave an error message (even as response to authentication), that message must be shown. Sometimes, authentication fails simply because only one POP check within 15 minutes is allowed, and the server then says that as error message in response to the login.

Importance:
Misleading error messages which tell the *wrong* cause are worse than none. They make users try to fix the wrong thing (here: try other username permutations or hunt for the right password, maybe it was changed?) and therefore cause severe problems.

Comment 1

[Ben Bucksch (:BenB)]

Note: this may well need new strings.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

This would be great to get, but we wouldn't block on it unless we had reason to believe that it was being experienced by an extremely significant percentage of our users, and they weren't able to recover from it.  Adding folks who might know whether that's true, they're welcome to renominate.

Comment 3

[Ben Bucksch (:BenB)]

Attachment: Add error strings only, v1

This patch adds only the needed error messages to the locale, before the string freeze.

I think this bug is serious (see initial description), so I'd like to fix it in 3.1, if I can. If we don't add the strings now, I can't fix it later anymore, though. Just adding the strings shouldn't do much harm, just a tiny little bit more work for our translators.

Please review before the beta2 deadline (with enough time for me to still check it in)!

Comment 4

[Ben Bucksch (:BenB)]

Attachment: Add error strings only, v2

Add "unexpectedly"

Comment 5

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 440032 [details] [diff] [review]
Add error strings only, v2

>+++ b/mail/locales/en-US/chrome/messenger/accountCreationUtil.properties
>@@ -13,8 +13,14 @@ boolean.error=Not a boolean
>+#verifyConfig.js
>+auth_failed_generic.error=Login failed. Are username/email address and password correct?
>+auth_failed_with_reason.error=Login failed. The server said: %S

Needs a translation note.

>+verification_failed.error=Login verification failed for an unknown reason.
>+verification_failed_with_exception.error=Login verification failed unexpectedly: %S

Needs a translation note.  Also, I'ld like that to read a little better, like "Login verification failed unexpectedly with message: %S"

Other than those two, r=me.

Comment 6

[Ben Bucksch (:BenB)]

Attachment: Add error strings only, v3

Comment 7

[Mark Banner (:standard8)]

(In reply to comment #3)
> If we don't add the strings now, I can't fix it later anymore,
> though. Just adding the strings shouldn't do much harm, just a tiny little bit
> more work for our translators.
> 
> Please review before the beta2 deadline (with enough time for me to still check
> it in)!

Localisers hate this type of string landing, and it is actively discouraged and we've been complained at before for doing this. Generally, I believe, it is because they are unable to test the strings although they have to translate them.

I'm cc'ing the l10n guys here. The only option may be to land it with a couple of l10n postings. This isn't a blocker, but very much wanted.

Comment 8

[Ben Bucksch (:BenB)]

> Generally, I believe, it is because they are unable to test the strings

These are just error strings. Generally, they couldn't even test with a patch. Same in other bugs.

Comment 9

[Simon Paquet [:sipaq]]

We can do this, but *only* if we give localisers enough backgroud information. 
That would include:

- a much more descriptive localization note. Basically a short form of the 
  original bug issue together with a short explanation on how to test and 
  if testing isn't possible, why it is not.
- A post to mozilla.dev.l10n with a short heads-up for localizers when you 
  check this in.

Comment 10

[Ben Bucksch (:BenB)]

Sure, I can add descriptions, no problem. Thanks.

Comment 11

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 440097 [details] [diff] [review]
Add error strings only, v3

I don't usually like using the word "server" in messages to the user but I'm not sure what else to replace it with.  If we had the host name that would be a lot better.  Is that possible?

In general it looks fine, so I'll give a ui-r+ but will look for comments on that.

Comment 12

[Ben Bucksch (:BenB)]

Yes, I think I can add the hostname. I'd still prefer to keep the word "server" in there. I modeled it after these msgs:
http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties#78

An error occurred while sending mail. The mail server responded:  %s

(and similar msgs in IMAP, POP3 etc.) The msgs here are very similar.

Comment 13

[Ben Bucksch (:BenB)]

Attachment: Add error strings only, v4 - commited

Here's an example of all 4 errors:
Login failed. Are username/email address and password correct?
Login failed. The server imap.web.de said: Only one login per 15 minutes
Login verification failed for an unknown reason.
Login verification failed with message: TCP timeout

Comment 14

[Ben Bucksch (:BenB)]

Commited as <http://hg.mozilla.org/comm-central/rev/b9de88d28d49>
(with one duplicated line in v4 removed)

Comment 15

[Simon Paquet [:sipaq]]

Comment on attachment 440307 [details] [diff] [review]
Add error strings only, v4 - commited

Looks great (with the duplicate removed). Thanks for your work.

Comment 16

[Tom Metro]

Related bugs:
Bug 224032
Bug 435306

Comment 17

[Ben Bucksch (:BenB)]

Not related, this bug is in the new account creation wizard.

Comment 18

[Ben Bucksch (:BenB)]

I just noticed that we published a bad config for tiscali.it: IMAP, port 143, normal SSL. This will surely go wrong. The user sees a message: "Username/password invalid!" next to the password. Very bad! :-((( This is sure to horribly confuse users and waste tons of their time, we must fix this.

Comment 19

[Ben Bucksch (:BenB)]

(If anybody wants to take and fix this bug, be my guest. I likely won't have time in the near future.)

Comment 20

[Vincent (caméléon)]

Hi gents,

The work here seems great, it is too bad if it cannot be finished...

We have a lot of user reporting this kind of error on GSFN, see https://getsatisfaction.com/mozilla_messaging/tags/username_or_password_wrong

However, it doesn't only happened during autoconfiguration. Should we open a new bug for this kind of problems, which certainly need a similar fix?

Comment 21

[Ben Bucksch (:BenB)]

I agree this is very important, but I won't get to it in the mid-term future.

bienvenu, could you please fix this?

Comment 22

[Ben Bucksch (:BenB)]

Attachment: [autoconfig] Show real error message during account verification

The account creation dialog verifies the password before creating the account, by contacting the real server and attempting a login. The bug is that it always says "Verify username or password", it always blames the password, no matter what error there was. This is not only wrong and misleading for end users, because they hunt the wrong end. It's also dangerous, because they will re-attempt the password multiple times, then try other passwords, thereby exposing other passwords to the server.

This patch shows the real error message.

More specifically: If it's a wrong username or password, we maintain the same error message as before. If it's any other error, we show the real error message.

Comment 23

[Ben Bucksch (:BenB)]

Attachment: Fix, with commit message

Ready to land

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: autoconfig-verify-errors-555448-6.diff

We have most of the C++ source reformatted now. I've fixed a few issues here.

BTW, it should be r=Neil, no?

Comment 25

[Jorg K (CEST = GMT+2)]

Actually, before we land this, can you explain why you added errorParameters when that's not used.

Comment 26

[Ben Bucksch (:BenB)]

can you explain why you added errorParameters when that's not used.

This allows to carry on additional error information from the server. It can be logged to help diagnosis. I didn't add the logging yet, but it's structurally important to have a place for extra information.

Comment 27

[Ben Bucksch (:BenB)]

nsACString& aErrorCode -> nsACString &aErrorCode

Who decided on that code style change? That's just wrong. The "*" and the "&" goes with the type, not with the variable.

If somebody just now reformatted our codebase for the & to go with the variable instead of the type, they better revert that change.

See Mozilla code style guide https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
SomeMap::GetValue(const nsString& key, nsString& value);
GetStringValue(nsAWritableCString& aResult)
const nsACstring& aStr,
mozilla::UniquePtr<const char*>&& aBuffer,
void DoSomething(nsIContent* aContent)
char* warning = GetStringValue();
nsISupports* aOptionalThing = nullptr)
Foo** aResult

Comment 28

[Ben Bucksch (:BenB)]

Re *&, see bug 1546364 comment 28.

Comment 29

[Ben Bucksch (:BenB)]

Attachment: Fix, v9, with commit message

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/164c011c8985
[autoconfig] Show real error message during account verification. r=Neil