1859656 - Implement creation of Exchange accounts using Autodiscover
Closed Bug 1859656 Opened 1 year ago Closed 8 months ago

Implement creation of Exchange accounts using Autodiscover

Categories

(Thunderbird :: Account Manager, enhancement)

enhancement

Tracking

(thunderbird126 fixed)

RESOLVED FIXED
126 Branch
Tracking Status
thunderbird126 --- fixed

People

(Reporter: leftmostcat, Assigned: leftmostcat, NeedInfo)

References

(Blocks 2 open bugs, Regressed 1 open bug)

Details

Attachments

(1 file)

The Autodiscover Rust service needs to be wired through the account manager to allow users to initiate account creation and successfully authenticate with EWS. Initially, the resulting incoming server should be a stub of some form and will not be functional.

As this is a user-facing change, it should be gated behind the mail.protocol.ews.enable preference.

As mentioned in bug 1859654, we're already implementing autodiscover in JS in the account creation dialog. Creating it in Rust would create significant overhead for the communication between the JS and Rust layers.

  • It would add significant complexity to the code,
  • take engineering time,
  • would likely also be slower, because of the context changes, transmission between the layers, and
  • speed is network-bound anyway.

If you're lacking functionality or information, you could extend the existing implementation by the bits that you lack, instead of re-writing everything.

Agreed, what's missing from the current autodiscovery?
Autodiscovery is responsible for finding out the details, which to use to then create an actual account.

As linked from the bug dependency, there are several known issues with the current Autodiscover implementation. It's limited to Basic auth over plain HTTP—which even in isolation is a situation we shouldn't be comfortable with—and, partly as a result of that limitation, does not offer full support for redirects. While these issues could be remediated in the existing code, it has several architectural issues which we deemed sufficient to make it an undesirable base on which to build:

  • The request body is built as a string and this is prone to being brittle, especially given that the sanitization of inputs happens elsewhere in the code.
  • Actual fetch and parsing happens across several files with several idiosyncratic types and interfaces involved, and with the issues with JavaScript tooling in our code which I've discussed elsewhere, this makes maintenance and extension of the code a burden.
  • It's reliant on making requests as XHR for deserialization, and there are explicit comments in the code from you, Ben, about the problems that causes.
  • It's also reliant on our idiosyncratic notion of "JXON", the sole documentation of which is a dead link to MDN.
  • We don't have robust verification that the response is in the shape we expect, and building this with JS objects would be a lot of boilerplate and still not as robust as we'd like.

As Ben points out, the network is the speed bottleneck here, not the XPCOM overhead, and overall we feel that an implementation in a strongly-typed language with good XML parsing support will improve maintainability and allow us to address the limitations and architectural issues with the current implementation.

Blocks: 1860335

It's limited to Basic auth over plain HTTP

First off, we do Basic auth only over HTTPS, not "plain HTTP".

As far as I'm aware, Autodiscover 1.0 with XML only supports HTTP auth (Basic/Digest/NTLM/Negotiate).
Mozilla (the code that we use) can do NTLM. We found that "Negotiate" caused problems and had to disable it, IIRC.

FYI, Microsoft eventually realized that authentication for AutoDiscover was a mistake (due to the inherent chicken and egg problem of finding the right config), and newer versions don't have any authentication at all.

Actual fetch and parsing happens across several files

Actual fetch and parsing for AutoDiscover happens only in one file:
https://searchfox.org/comm-central/source/mail/components/accountcreation/modules/ExchangeAutoDiscover.sys.mjs
It contains all code related to AutoDiscover. It returns the config data in a struct common to all autoconfig methods. Unless you want to rewrite the entire account configuation dialog logic (which would be a huge effort and difficult to get right), you will need to return that struct from your new code anyways.

We don't have robust verification [of] the response

The current code validates the response parts that we use. More importantly, it validates each value that we use, with its specific content. E.g. email addresses are not only validated as strings, but actually as email address.

It is intentional that we ignore response parts that we don't need, given the revisions and variations of the protocol over 1-2 decades, and the wide variety of server implementations, some of them non-Microsoft servers which imitate Exchange or AutoDiscover.

I still don't know:

  1. What exactly is missing in the current implementation? What exactly do you need to do, which you cannot do with the current implementation?
  2. Why is that missing piece important? Given that the current code works to create Exchange acccounts, since years.
  3. And if there is indeed a feature missing: Why can you not add it to the current implementation? ("I don't like the current code" is not sufficient motivation, by itself.)
Flags: needinfo?(leftmostcat)
Assignee: nobody → leftmostcat
Attachment #9393166 - Attachment description: WIP: Bug 1859656 - Ensure we present Exchange as an option regardless of priority. → Bug 1859656 - Ensure we present Exchange as an option regardless of priority. r=aleca,freaktechnik,vineet
Status: NEW → ASSIGNED
No longer depends on: 1859654

Note for landing: there are other patches in this stack that don't belong to this bug.

Pushed by martin@humanoids.be:
https://hg.mozilla.org/comm-central/rev/071dab0a4445
Ensure we present Exchange as an option regardless of priority. r=aleca,freaktechnik,vineet

Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → 126 Branch
Regressions: 1907443
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: