Closed Bug 555448 Opened 15 years ago Closed 5 years ago

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

Categories

(Thunderbird :: Account Manager, defect, P1)

Product:
Thunderbird
Component:
Account Manager
Type:
defect

Tracking

(blocking-thunderbird3.1 -)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 68.0
Tracking Flags:
Tracking Status
blocking-thunderbird3.1 --- -

People

(Reporter: BenB, Assigned: BenB)

References

(
URL
)

Details

(Whiteboard: [GS])

Attachments

(1 file, 7 obsolete files)

Fix, v9, with commit message
(deleted), patch
BenB
: review+
Details | Diff | Splinter Review
Reproduction: 1. File | New | Mail account... 2. Enter foo@gmx.com 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.
Priority: -- → P2
blocking-thunderbird3.1: --- → ?
Note: this may well need new strings.
Summary: Account creation wizard says "username and/or password invalid", although it could be any kind of error → Account creation wizard says "Username or password invalid", although it could be any kind of error
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.
blocking-thunderbird3.1: ? → -
Flags: wanted-thunderbird+
Priority: P2 → P1
Attached patch Add error strings only, v1 (obsolete) (deleted) — — Splinter Review
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)!
Attachment #440029 - Flags: ui-review?(clarkbw)
Attachment #440029 - Flags: review?(bwinton)
Status: NEW → ASSIGNED
Summary: Account creation wizard says "Username or password invalid", although it could be any kind of error → [Account creation] Says "Username or password invalid", although it could be any kind of error (verifyConfig)
Attached patch Add error strings only, v2 (obsolete) (deleted) — — Splinter Review
Add "unexpectedly"
Attachment #440029 - Attachment is obsolete: true
Attachment #440032 - Flags: ui-review?(clarkbw)
Attachment #440032 - Flags: review?(bwinton)
Attachment #440029 - Flags: ui-review?(clarkbw)
Attachment #440029 - Flags: review?(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.
Attachment #440032 - Flags: review?(bwinton) → review+
Attached patch Add error strings only, v3 (obsolete) (deleted) — — Splinter Review
Attachment #440097 - Flags: ui-review?(clarkbw)
Attachment #440097 - Flags: review+
Attachment #440032 - Attachment is obsolete: true
Attachment #440032 - Flags: ui-review?(clarkbw)
(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.
> 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.
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.
Sure, I can add descriptions, no problem. Thanks.
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.
Attachment #440097 - Flags: ui-review?(clarkbw) → ui-review+
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.
Attached patch Add error strings only, v4 - commited (obsolete) (deleted) — — Splinter Review
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
Attachment #440097 - Attachment is obsolete: true
Attachment #440307 - Flags: ui-review+
Attachment #440307 - Flags: review+
Attachment #440307 - Flags: feedback?(bugzilla)
Commited as <http://hg.mozilla.org/comm-central/rev/b9de88d28d49> (with one duplicated line in v4 removed)
Attachment #440307 - Attachment description: Add error strings only, v4 → Add error strings only, v4 - commited
Comment on attachment 440307 [details] [diff] [review] Add error strings only, v4 - commited Looks great (with the duplicate removed). Thanks for your work.
Attachment #440307 - Flags: feedback?(bugzilla) → feedback+
Related bugs: Bug 224032 Bug 435306
Not related, this bug is in the new account creation wizard.
Summary: [Account creation] Says "Username or password invalid", although it could be any kind of error (verifyConfig) → [autoconfig] Wrong error msg: Says "Username or password invalid", although it could be any kind of error (verifyConfig)
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.
Severity: normal → major
(If anybody wants to take and fix this bug, be my guest. I likely won't have time in the near future.)
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?
I agree this is very important, but I won't get to it in the mid-term future. bienvenu, could you please fix this?
Assignee: ben.bucksch → dbienvenu
Assignee: mozilla → nobody
Status: ASSIGNED → NEW
Assignee: nobody → ben.bucksch
Attachment #440307 - Attachment is obsolete: true

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.

Attachment #9064233 - Flags: review?(neil)
Target Milestone: --- → Thunderbird 68.0
Attached patch Fix, with commit message (obsolete) (deleted) — — Splinter Review

Ready to land

Attachment #9064593 - Flags: review+
Keywords: checkin-needed
Attachment #9064233 - Flags: review?(neil) → review+
Attached patch autoconfig-verify-errors-555448-6.diff (obsolete) (deleted) — — Splinter Review

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

BTW, it should be r=Neil, no?

Attachment #9064593 - Attachment is obsolete: true
Attachment #9064622 - Flags: review+

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

Flags: needinfo?(ben.bucksch)
Keywords: checkin-needed

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.

Flags: needinfo?(ben.bucksch)

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

Re *&, see bug 1546364 comment 28.

Attached patch Fix, v9, with commit message (deleted) — — Splinter Review
Attachment #9064622 - Attachment is obsolete: true
Attachment #9064775 - Flags: review+

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

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Attachment #9064233 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: