1518155 - [autoconfig] Exchange AutoDiscover: Ask for username, only if necessary

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, enhancement, P2)

Description

[Ben Bucksch (:BenB)]

The account creation dialog has the philosophy that it needs to work seamlessly for the least computer-savvy end user. It needs to ask the absolute minimum amount of information to set up an account, which is the email address and password (and the user's real name, unless we can get it from the OS). That works for many accounts.

Bug 1500105 added Exchange AutoDiscover support. The Exchange AutoDiscover protocol as designed by Microsoft has the very unfortunate property that it requires a login before even returning a config. That's a bad chicken and egg problem, because the login username format (which is not necessarily the same as the email address) is exactly part of the config that we are trying to find out. Formats can differ wildly:
* [e-mail]
* foo
* EXAMPLE\foo
etc.. Any one of them might be required, and the others will likely not work. Finding out which exact username format is expected by the server is part of the frustration of setting up an account. *1 The account creation dialog tried to take that away. Unfortunately, Exchange AutoDiscover doesn't let us. I consider that a serious design fault of the protocol.

Luckily, a good deal of the servers accept the email address as login username now. For them, bug 1500105 already works. Unfortunately, a good deal of servers still does not accept the email address as login username. For them, bug 1500105 does not work.

What happens is that we try to log in to the AutoDiscover server with the email address as username. The server doesn't recognize the username, and sends us "Authentication failed". We consequently ignore AutoDiscover.

Instead, we should detect this specific situation. If the AutoDiscover server sends us "Authentication failed", then we should additionally ask the end user for the username. Most likely, this is a Windows domain login of the form
* EXAMPLE\foo
so we need to show that as a hint to the end user, so that he knows which username form to use. Affected users will likely recognize this as domain login and know which username is meant.

It's important that we do this only in this specific case. We do *not* want to bother users who are not affected by these subpar environments with manually entering a username, given that 95% of users won't need to enter it.

This bug tracks the implementation of this feature tweak.

Comment 1

[Ben Bucksch (:BenB)]

Attachment: Fix, v1

This implements the feature.

I tested that the new username text field appears, and that the value entered in the username field is accepted and used as username.
I could not test with an account that actually needs this configuration. Somebody promised me one, I'll test with that account once I receive it.

Comment 2

[Ben Bucksch (:BenB)]

Attachment: Fix, v2

Comment 3

[Ben Bucksch (:BenB)]

I've received the test account of a university which does not accept email addresses as login usernames, but requires the Windows domain\username as login, including for AutoDiscover.
And it works with that account. We get a HTTP 401, we detect this situation, you get the additional username prompt, you enter your Windows domain login username, and click Continue, and we get the AutoDiscover response, a correct server, we offer Owl addon, and we set up the Owl account.

Comment 4

[[e-mail]]

Comment on attachment 9034879 [details] [diff] [review]
Fix, v2

•      console.error(e);
  

•      // must call error callback in any case, to stop the discover mode
  

Do you need to log all errors here?

• let url1 = "https://" + sanitize.hostname(domain) +

•         "/autodiscover/autodiscover.xml";
  

• let url1 = "http://autodiscover." + sanitize.hostname(domain) +

•         "/autodiscover/autodiscover.xml"; // needed by email hosters
  

• let url3 = "http://autodiscover." + sanitize.hostname(domain) +

•         "/autodiscover/autodiscover.xml"; // needed by email hosters
  

• let url3 = "https://" + sanitize.hostname(domain) +

•         "/autodiscover/autodiscover.xml";
  

Why the swap?

<!ENTITY password.text "Optional, will only be used to validate the username">
<!ENTITY rememberPassword.label "Remember password">
<!ENTITY rememberPassword.accesskey "m">
+<!ENTITY username.label "Your login:">
+<!ENTITY username.accesskey "l">
+<!ENTITY username.placeholder "YOURDOMAIN\yourusername">
+<!ENTITY username.text "Domain login, e.g. YOURDOMAIN\yourusername">

<!ENTITY imapLong.label "IMAP (remote folders)">
<!ENTITY pop3Long.label "POP3 (keep mail on your computer)">
Nit: Strange indent.

+please_enter_username=Please enter your user name, e.g. YOURDOMAIN\yourusername
Unused?

Comment 5

[[e-mail]]

Comment on attachment 9034879 [details] [diff] [review]
Fix, v2

+          console.error(e);
+          // must call error callback in any case, to stop the discover mode

Do you need to log all errors here?

-  let url1 = "https://" + sanitize.hostname(domain) +
-             "/autodiscover/autodiscover.xml";
+  let url1 = "http://autodiscover." + sanitize.hostname(domain) +
+             "/autodiscover/autodiscover.xml"; // needed by email hosters
  let url2 = "https://autodiscover." + sanitize.hostname(domain) +
             "/autodiscover/autodiscover.xml";
-  let url3 = "http://autodiscover." + sanitize.hostname(domain) +
-             "/autodiscover/autodiscover.xml"; // needed by email hosters
+  let url3 = "https://" + sanitize.hostname(domain) +
+             "/autodiscover/autodiscover.xml";

Why the swap?

<!ENTITY password.text                   "Optional, will only be used to validate the username">
<!ENTITY rememberPassword.label          "Remember password">
<!ENTITY rememberPassword.accesskey      "m">
+<!ENTITY username.label                      "Your login:">
+<!ENTITY username.accesskey                  "l">
+<!ENTITY username.placeholder                "YOURDOMAIN\yourusername">
+<!ENTITY username.text                       "Domain login, e.g. YOURDOMAIN\yourusername">

<!ENTITY imapLong.label                  "IMAP (remote folders)">
<!ENTITY pop3Long.label                  "POP3 (keep mail on your computer)">

Nit: Strange indent

+please_enter_username=Please enter your user name, e.g. YOURDOMAIN\yourusername

Unused?

Comment 6

[Ben Bucksch (:BenB)]

Do you need to log all errors here?

I'd like to see whether there are cases where out status = 401 detection fails. If a user reports that we don't recognize AutoDiscover although it exists, I can ask him to send me the longs, and this output would give me clues. So, this wasn't just for my own dev debugging, but I need this for debugging end users.
I guess I should expand it, actually, because console.log by default collapses the object and a simply copy&paste will not suffice. So, I should make a more explicit log line.

Nit: Strange indent

Thanks.

Why the swap?

That's a subtle one. The PriorityOrderAbortable class, once all fail, reports the exception of one of these calls as representative the error for all of them. It takes the error of the first one, because that's by definition (of the class) the preferred one. In this case, the servers return different errors, because https://example.com/ is just the website and often returns 404 or "host not found" for email domains, while autodiscover returns 401. We need that error code here.

Comment 7

[Ben Bucksch (:BenB)]

Attachment: Fix, v3

• Review requests fixed
• I didn't need the logging after all, because the errors are already logged as-is
• I've put https://autodiscover.example.com as first URL
• Neil had a good point that I should check all calls for a 401 errors, not just the first one. I've implemented that.

Comment 8

[Ben Bucksch (:BenB)]

Attachment: Fix, v4

Comment 9

[Ben Bucksch (:BenB)]

Attachment: Fix, v5

Reduced explanation text. Added l10n note.

Comment 10

[Ben Bucksch (:BenB)]

Attachment: Fix, v6

Logging tweak: Add error code to already existing debug output. For network errors (e.g. timeouts, DNS errors etc.), now gives "failed with 0 at <" instead of "failed with at <".

Comment 11

[[e-mail]]

Comment on attachment 9035403 [details] [diff] [review]
Fix, v6

-  let url1 = "https://" + sanitize.hostname(domain) +
+  let url1 = "https://autodiscover." + sanitize.hostname(domain) +
             "/autodiscover/autodiscover.xml";
-  let url2 = "https://autodiscover." + sanitize.hostname(domain) +
+  let url2 = "http://autodiscover." + sanitize.hostname(domain) +
+             "/autodiscover/autodiscover.xml";
+  let url3 = "https://" + sanitize.hostname(domain) +
             "/autodiscover/autodiscover.xml";
-  let url3 = "http://autodiscover." + sanitize.hostname(domain) +
-             "/autodiscover/autodiscover.xml"; // needed by email hosters

These can go back in their original order now, right? r=me with that fixed.

+      ddump("call " + this.position + " took " + (Date.now() - this._time) +
+          "ms and failed with " + e.code + " " + e +
          (this.callerAbortable && this.callerAbortable._url ? " at <" + this.callerAbortable._url + ">" : ""));

Nit: e.code might be undefined, which looks odd in the Error Console.

Comment 12

[Ben Bucksch (:BenB)]

These can go back in their original order now, right?

No. They still have an order of priority (class PriorityOrderAbortable).
If several of these return a result, we want to prefer the one from https, not the http one. That's the current change.

Nit: e.code might be undefined, which looks odd in the Error Console.

Right, I can put a ? in there.

Comment 13

[Ben Bucksch (:BenB)]

Attachment: Fix, v7

• Debug output fixed
• URL order as in v6

Comment 14

[Ben Bucksch (:BenB)]

Attachment: Fix, v8 - first land

Comment 15

[Ben Bucksch (:BenB)]

Impact if declined:
Users with Exchange hosts that do not accept email addresses as usernames will not be detected and bug 1500105 will not have any positive effect for them.
This appears to be a sizable portion of self-hosted Exchange servers.

Comment 16

[Ben Bucksch (:BenB)]

Risk:
The new username field appears only if the Exchange AutoDiscover server responsed and failed with error code 401 Unauthorized.

Comment 17

[Jorg K (CEST = GMT+2)]

I don't know what the tracking flags are for. This will land now on TB 66, so why track it. If you want beta uplift, please request it on the patch. However, there are string changes, so not a good idea. TB 66 beta comes out at the end of January.

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 9035647 [details] [diff] [review]
Fix, v8 - first land

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

::: mail/components/accountcreation/content/emailWizard.js
@@ +617,5 @@
> +      fetch = fetchConfigFromExchange(domain,
> +        emailAddress, this._exchangeUsername, this._password,
> +        call.successCallback(),
> +        (e, allErrors) => {
> +          // Must call error callback in any case, to stop the discover mode

Please make sure all comments adhere to coding standards. Full stop missing.

@@ +620,5 @@
> +        (e, allErrors) => {
> +          // Must call error callback in any case, to stop the discover mode
> +          call.errorCallback()(e); // ()(e) is correct
> +          if (e.code == 401 || allErrors && allErrors.find(e => e.code == 401)) { // Auth failed
> +            // ask user for username

Upper case and full stop. I'll fix it.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/130e9b3087d7
[autoconfig] Exchange AutoDiscover: Ask for username only if necessary. r=Neil

Comment 20

[Ben Bucksch (:BenB)]

Please make sure all comments adhere to coding standards. Full stop missing. ... Upper case

FWIW, I have not found such a coding standard in https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style (I searched for "comment" on this page).

Existing Thunderbird code does not adhere to these rules at all. See e.g.
https://searchfox.org/comm-central/source/mailnews/imap/src/nsImapIncomingSerer.cpp#807
// do nothing; return NS_OK; for queuing
...
rv = NS_BINDING_ABORTED; // user cancelled
...
else // cannot get anyone to handle the url queue it
...
// caller will queue the url
and many many other examples like it.

The rule I use is: Full stop only to separate sentences. If there are not at least 2 sentences (i.e. only one phrase, or not even a phrase), I do not finish with a full stop. This is similar to user interfaces, which generally do not use full stop, either, unless there's a paragraph with several sentences.

Which rules are you referring to?

Comment 21

[Jorg K (CEST = GMT+2)]

Well, there has been much discussion about inline comments. The comments in question here were full line comments:

// Must call error callback in any case, to stop the discover mode
// ask user for username

There we prefer full English sentences starting with uppercase and ending with a full-stop.

Good English and punctuation are always very welcome ;-)

Comment 22

[Onno Ekker [:nONoNonO UTC+1]]

username.label now occurs twice in the file mail/locales/en-US/chrome/messenger/accountCreation.dtd, on line 21 and on line 33.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/389458d07470
Follow-up: Rename some strings to avoid duplication. r=me DONTBUILD

Comment 24

[Jorg K (CEST = GMT+2)]

Ben, to avoid a backout here, I went ahead and fixed the issue. I also renamed a two IDs of elements, however, usernametextEx appears to be unused, so why have it?

Comment 25

[Ben Bucksch (:BenB)]

username.label now occurs twice

Arg! :-(
Thanks for catching that.

Comment 26

[Jorg K (CEST = GMT+2)]

You've checked https://hg.mozilla.org/comm-central/rev/389458d07470, right?

Comment 27

[Ben Bucksch (:BenB)]

Yes, I have reviewed it after the fact. r=BenB

Comment 28

[Ben Bucksch (:BenB)]

TODO: Make patch without string changes for TB beta and TB 60.

Comment 29

[Jorg K (CEST = GMT+2)]

(In reply to Ben Bucksch (:BenB) from comment #28)

TODO: Make patch without string changes for TB beta and TB 60.

Coming up?

Comment 30

[Ben Bucksch (:BenB)]

Yes, but this will be Wednesday probably. I wanted to do it today, but didn't get to it, sorry.

Comment 31

[Ben Bucksch (:BenB)]

I'll do this ASAP

Comment 32

[Ben Bucksch (:BenB)]

Attachment: Avoid duplicated labels and element IDs - second land

This is the same patch that was landed as fixup. Attached just for the record.

Comment 33

[Ben Bucksch (:BenB)]

Attachment: Fix, v9 - combined

This is the combination of Fix v8 and Avoid duplication
It is the complete fix for this bug.

Comment 34

[Ben Bucksch (:BenB)]

Attachment: Fix, v9, new locale strings removed, for TB 60

Comment 35

[Ben Bucksch (:BenB)]

Comment on attachment 9036926 [details] [diff] [review]
Fix, v9, new locale strings removed, for TB 60

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
Bug 1500105 will not work for users whose server requires a Windows domain username (instead of the email address). I think this concerns a large percentage of on-premise Exchange servers.

Testing completed (on c-c, etc.):
TB trunk

Risk to taking this patch (and alternatives if risky):

Comment 36

[Jorg K (CEST = GMT+2)]

TB 65 beta 3:
https://hg.mozilla.org/releases/comm-beta/rev/d90827241325c5d38974e01ea61bdac3c7cd1fbb

Please check this, I had to rebase the ESR60 patch which didn't cleanly apply to C-B:
$ hg qpush
applying autoconfig-exchange-username-1518155-9-tb60.diff
patching file mail/components/accountcreation/content/emailWizard.js
Hunk #2 FAILED at 455
Hunk #3 FAILED at 626
2 out of 4 hunks FAILED -- saving rejects to file mail/components/accountcreation/content/emailWizard.js.rej
patching file mail/components/accountcreation/content/exchangeAutoDiscover.js
Hunk #2 succeeded at 117 with fuzz 1 (offset 0 lines).
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh autoconfig-exchange-username-1518155-9-tb60.diff

Comment 37

[Ben Bucksch (:BenB)]

Hey Jörg, Neil did a try build based on TB 60:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a141ab4f4e83bdaba447e4a0a330fc61311821b3
He had to fix up a few patches. If you apply the patches from that try push, in the same order, they should work.
I meant to attach them to the bugs, but didn't get around to it yet. Sorry about that.

Comment 38

[Ben Bucksch (:BenB)]

Please check this, I had to rebase the ESR60 patch which didn't cleanly apply to C-B:

Ah, sorry, you're talking about 65 beta, not TB 60.
I manually compared your TB 65 beta commit with my TB 60 patch and it looks OK to me.

Comment 39

[Jorg K (CEST = GMT+2)]

I did my best, but four eyes are always better. Yes, this is for the beta.

Comment 40

[Jorg K (CEST = GMT+2)]

(In reply to Ben Bucksch (:BenB) from comment #37)

Hey Jörg, Neil did a try build based on TB 60:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a141ab4f4e83bdaba447e4a0a330fc61311821b3
He had to fix up a few patches. If you apply the patches from that try push, in the same order, they should work.
I meant to attach them to the bugs, but didn't get around to it yet. Sorry about that.

I see. Well, the seven patches in the try are all on "approval-comm-esr60?" Knowing that, I can get them from the try if you want. Let me know.

Comment 41

[Ben Bucksch (:BenB)]

Yes, please get them from that try build. I think that's easiest for all parties. It also shows the order of the patches.

Just the last one there, the patch for this bug here, has a bad commit message:
7396bd09b5ccBB Remove 'Windows username', too
is wrong and my mistake. Instead, the commit message should be:

Bug 1518155 - [autoconfig] Exchange AutoDiscover: Ask for username only if necessary. r=Neil

Comment 42

[Jorg K (CEST = GMT+2)]

Comment on attachment 9036926 [details] [diff] [review]
Fix, v9, new locale strings removed, for TB 60

Same as Neil's try.

Comment 43

[Jorg K (CEST = GMT+2)]

TB 60.5.0 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/858bcc0951038087b8c87ecd790cb123c64fdb86