Closed Bug 1611405 Opened 7 months ago Closed 5 months ago

account setup should prefer standard protocols over exchange

Categories

(Thunderbird :: Account Manager, defect)

defect
Not set
normal

Tracking

(thunderbird_esr68+ fixed, thunderbird75+ fixed)

RESOLVED FIXED
Thunderbird 76.0
Tracking Status
thunderbird_esr68 + fixed
thunderbird75 + fixed

People

(Reporter: mkmelin, Assigned: mkmelin)

References

(Blocks 1 open bug, Regression)

Details

Attachments

(3 files, 3 obsolete files)

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.

Summary: should prefer standard protocols over exchange → account setup should prefer standard protocols over exchange

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

I'll fix this.

Ben Bucksch

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

Attached patch Prefer IMAP result, v1 (obsolete) — Splinter Review

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.

Assignee: mkmelin+mozilla → ben.bucksch
Attachment #9126833 - Flags: review?(mkmelin+mozilla)
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
Attachment #9126833 - Flags: review?(mkmelin+mozilla)

This is the patch I had.

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

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.

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

Drop this line.

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

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

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

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...
Assignee: ben.bucksch → mkmelin+mozilla
Attachment #9126833 - Attachment is obsolete: true
Attachment #9127063 - Attachment is obsolete: true
Attachment #9134195 - Flags: review?(alessandro)
Attachment #9134195 - Flags: feedback?(ben.bucksch)

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 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.
Attachment #9134195 - Flags: review?(alessandro) → review+

Made the suggested changes.

Attachment #9134195 - Attachment is obsolete: true
Attachment #9134195 - Flags: feedback?(ben.bucksch)
Attachment #9134594 - Flags: review+
Attachment #9134594 - Flags: feedback?(ben.bucksch)

Going to land later today.

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.

So, did you?

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/0f20836e8455
account setup should prefer standard protocols over exchange. r=aleca

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 76.0
Attachment #9134594 - Flags: feedback?(ben.bucksch)

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 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
Attachment #9134594 - Flags: review-

(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.

Attachment #9134594 - Flags: approval-comm-esr68?
Attachment #9134594 - Flags: approval-comm-beta?
Comment on attachment 9134594 [details] [diff] [review]
bug1611405_exchange_prefer_standards.patch

Beta approved.
Will assess ESR after beta experience
Attachment #9134594 - Flags: approval-comm-beta? → approval-comm-beta+
Comment on attachment 9134594 [details] [diff] [review]
bug1611405_exchange_prefer_standards.patch

Approved for ESR
Attachment #9134594 - Flags: approval-comm-esr68? → approval-comm-esr68+
Keywords: regression

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

Keywords: regression
You need to log in before you can comment on or make changes to this bug.