1611405 - account setup should prefer standard protocols over exchange

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

If an exchange account type is detected via autodiscover.xml it will usually (always?) be the listed in the xml above the standard protocols. Therefore it will become the default in the config we use. This is clearly incorrect. If standard protocols are listed they should be preferred. In fact it was never the intention to even show exchange for that case.

Comment 1

[Ben Bucksch]

You mean if the AutoDiscover.xml lists both IMAP and Exchange options, then IMAP should be the default?

I'll fix this.

Ben Bucksch

Comment 2

[Magnus Melin [:mkmelin]]

I have a patch I just need to test it some more.

Comment 3

[Ben Bucksch (:BenB)]

Attachment: Prefer IMAP result, v1

If the AutoDiscover XML returns both Exchange and IMAP or POP3 configs, then prefer the IMAP config.

I've tested this with normal Office365, but I don't have a test case where this triggers. The code should do the right thing, though.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9126833 [details] [diff] [review]
Prefer IMAP result, v1

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

::: mail/components/accountcreation/content/exchangeAutoDiscover.js
@@ +363,5 @@
> +  // Prefer IMAP
> +  let imap = config.incomingAlternatives.find(alt => alt.type == "imap");
> +  let pop3 = config.incomingAlternatives.find(alt => alt.type == "pop3");
> +  if (imap || pop3) {
> +    config.incomingAlternatives.unshift(config.incoming);

this could still leave the order kind of odd, e.g. imap,exchange,pop3

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: bug1611405_exchange_prefer_standards.patch

This is the patch I had.

But I'd like to have a sample autodiscover xml file to test this with.

Comment 6

[Ben Bucksch (:BenB)]

Comment on attachment 9127063 [details] [diff] [review]
bug1611405_exchange_prefer_standards.patch

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

::: mail/components/accountcreation/content/accountConfig.js
@@ +329,5 @@
> +        return -1;
> +      }
> +      return 0;
> +    });
> +    this.alternatives = alternatives;

Drop this line.

@@ +330,5 @@
> +      }
> +      return 0;
> +    });
> +    this.alternatives = alternatives;
> +    this.incoming = alternatives.shift();

This does unnecessary work for the standard case (over 80%) where we have only an Exchange config. But it's marginal, so I don't mind too much.

Comment 7

[Magnus Melin [:mkmelin]]

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

Drop this line.

Without that it doesn't give you the sorted alternatives.

Comment 8

[Ben Bucksch (:BenB)]

Ah, I think you meant: this.incomingAlternatives = alternatives;

Comment 9

[Magnus Melin [:mkmelin]]

Attachment: autodiscover-w-imap-pop-smtp.xml

Sample response for when imap pop and smtp are set in the autodiscover result.

Comment 10

[Magnus Melin [:mkmelin]]

Attachment: bug1611405_exchange_prefer_standards.patch

Fixes some related problems too:

• looks like autodiscover.xml really uses Capitalized "On" or "Off"
• outgoing server auth need to be set to something, otherwise it won't succeed
• far from all callers pass the second argument allErrors, I notice...

Comment 11

[Magnus Melin [:mkmelin]]

Attachment: bug1611405_exchange_prefer_standards.debug.patch

To test: apply the debug patch too, and bug 1623398.
Try to set up an account for say foo@localhost - it will check the bugzilla attachment for a configuration.

Comment 12

[Alessandro Castellani [:aleca]]

Comment on attachment 9134195 [details] [diff] [review]
bug1611405_exchange_prefer_standards.patch

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

This works as expected for me, just a couple of code nits to change.
I haven't test this with those accounts that Ben provided us, the exchange accounts with disabled IMAP.
I just want to be sure these changes won't affect what we did in bug 1592258.

::: mail/components/accountcreation/content/accountConfig.js
@@ +320,5 @@
> +    alternatives.unshift(this.incoming);
> +    alternatives.sort((a, b) => {
> +      if (a.type == b.type) {
> +        return 0;
> +      }

Do we need this condition if we're always returning 0 if the other 2 conditions are not met?

::: mail/components/accountcreation/content/exchangeAutoDiscover.js
@@ +329,1 @@
>          }

Maybe we can define the server.auth before this condition in order to avoid the else callback.

Comment 13

[Magnus Melin [:mkmelin]]

Attachment: bug1611405_exchange_prefer_standards.patch

Made the suggested changes.

Comment 14

[Magnus Melin [:mkmelin]]

Going to land later today.

Comment 15

[Ben Bucksch (:BenB)]

I didn't have time to look at it yet, over the last 3 days, due to problems arising from Corona. I'll look at it.
Please give me a chance to review.

Comment 16

[Magnus Melin [:mkmelin]]

So, did you?

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0f20836e8455
account setup should prefer standard protocols over exchange. r=aleca

Comment 18

[Ben Bucksch (:BenB)]

I said in comment 15: Please give me a chance to review. I cannot give a 1-day response time right now, due to the Corona virus crisis.

Comment 19

[Ben Bucksch (:BenB)]

Comment on attachment 9134594 [details] [diff] [review]
bug1611405_exchange_prefer_standards.patch

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

::: mail/components/accountcreation/content/accountConfig.js
@@ +313,5 @@
> +  /**
> +   * Sort the config alternatives such that exchange is the last of the
> +   * alternatives.
> +   */
> +  preferStandardProtocols() {

This does not belong here. If you put it here, then make it generic, not about a specific protocol.

::: mail/components/accountcreation/content/emailWizard.js
@@ +697,5 @@
>          call.successCallback(),
>          (e, allErrors) => {
>            // Must call error callback in any case to stop the discover mode.
>            let errorCallback = call.errorCallback();
> +          if (allErrors && allErrors.some(e => e.code == 401)) {

Why is this check needed? What is calling this without the `allErrors`?

::: mail/components/accountcreation/content/exchangeAutoDiscover.js
@@ +297,5 @@
>          server.port = sanitize.integer(protocolX.Port);
>          server.socketType = 1; // plain
>          if (
>            "SSL" in protocolX &&
> +          protocolX.SSL.toLowerCase() == "on" // "On" or "Off"

The sanitize should stay. If there are values that are neither "on" nor "off" (in any capitalization), then the file is invalid and it should be rejected.

@@ +325,3 @@
>          ) {
>            // Secure Password Authentication = NTLM or GSSAPI/Kerberos
> +          server.auth = Ci.nsMsgAuthMethod.secure;

Good change

Comment 20

[Magnus Melin [:mkmelin]]

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

• preferStandardProtocols() {

This does not belong here. If you put it here, then make it generic, not
about a specific protocol.

Conceptually it belongs here, should there ever be any other non-standard protocols we could find. I don't really see that ever happening though.

•      if (allErrors && allErrors.some(e => e.code == 401)) {
  

Why is this check needed? What is calling this without the allErrors?

There are plenty of places it actually happens especially if you set prefs to disable some of the prefs, like https://searchfox.org/comm-central/rev/36361fb3e76ab8f11bc59524f444269ff6ae84d0/mail/components/accountcreation/content/exchangeAutoDiscover.js#57. I think ever having the second parameter is really the wrong thing to do: the standard way for error callbacks is to get one parameters, and the same goes for promise rejections.

::: mail/components/accountcreation/content/exchangeAutoDiscover.js
@@ +297,5 @@

     server.port = sanitize.integer(protocolX.Port);
     server.socketType = 1; // plain
     if (
       "SSL" in protocolX &&

•      protocolX.SSL.toLowerCase() == "on" // "On" or "Off"
  

The sanitize should stay. If there are values that are neither "on" nor
"off" (in any capitalization), then the file is invalid and it should be
rejected.

I don't see why. Even if sanitation failed we have data that could easily work. With the code we now ignore such invalid values.

Comment 21

[Wayne Mery (:wsmwk)]

Comment on attachment 9134594 [details] [diff] [review]
bug1611405_exchange_prefer_standards.patch

Beta approved.
Will assess ESR after beta experience

Comment 22

[Rob Lemley [:rjl]]

Thunderbird 75.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/e99cde50f9d3

Comment 23

[Wayne Mery (:wsmwk)]

Comment on attachment 9134594 [details] [diff] [review]
bug1611405_exchange_prefer_standards.patch

Approved for ESR

Comment 24

[Magnus Melin [:mkmelin]]

Thunderbird 68.7:
https://hg.mozilla.org/releases/comm-esr68/rev/f7099fd16d6f5dff22154eab3161674142501739

Comment 25

[Magnus Melin [:mkmelin]]

Not a regression per se, it was broken since initial exchange support.