1679724 - Invalid OAuth2 settings in config for Office 365 when using Exchange Autodiscover service

Status: RESOLVED WONTFIX

(Thunderbird :: Account Manager, defect)

Description

[David Ward]

When support for the Exchange Autodiscover service was implemented in Thunderbird, it produced a config with OAuth2 settings, using the URI for either OWA or EWS as the OAuth2 scope.

This is not valid for Office 365, where the required scopes are specifically defined. For any other Exchange server, OAuth 2.0 cannot be used since the registration details (client ID, client secret, authorization endpoint, and token endpoint) are not known to Thunderbird; in this case, OAuth 2.0 settings should not be added here at all.

Comment 1

[David Ward]

Attachment: Bug 1679724 - Part 1: Refactor code iterating over Exchange Autodiscover response.

This allows common code for applying certain settings to each protocol,
such as auth method and username. This also ensures that if an exception
occurs, only complete protocol settings are returned in the config.

Each Exchange-specific protocol ("WEB", "EXHTTP", or "EXCH") is returned
as a separate alternative server in the config.

Comment 2

[David Ward]

Attachment: Bug 1679724 - Part 2: Apply known OAuth2 settings when using Exchange Autodiscover.

This uses the correct provider settings for Office 365. Otherwise OAuth2
is disabled since these settings are unknown.

Depends on D98131

Comment 3

[Ben Bucksch (:BenB)]

Hey David,

thanks for the patches. I'll have to wrap my head around the fixes.

Bug 1679724 - Part 2: Apply known OAuth2 settings when using Exchange Autodiscover.

I understand that you're now correctly populating the scope and issuer fields. That's great, thanks for that.

However, please keep the original code logic. It added both authentication methods as alternatives, and that was intentional. If password auth is available, we want to know that, and we actually prefer that. As it is, the patch is risking to break quite a lot downstream.

So, please keep the code logic exactly as it is, but simply set the scope and issuer as from the OAuth module, as you do.

Comment 4

[Ben Bucksch (:BenB)]

Comment on attachment 9190206 [details]
Bug 1679724 - Part 1: Refactor code iterating over Exchange Autodiscover response.

Bug 1679724 - Part 1: Refactor code iterating over Exchange Autodiscover response.

Sorry, I fail to understand the need for this patch.

Also, if I'm reading the patch correctly, it creates an invalid AccountConfig object, because the config.incoming properties are not populated, and everything are alternatives. The AccountConfig object has one config.incoming primary config, and 0 or more alternatives.

Each Exchange-specific protocol ("WEB", "EXHTTP", or "EXCH") is returned as a separate alternative server in the config.

Have you actually tested this, for all cases? I don't think this is going to work properly.

r- on part 1

Comment 5

[Ben Bucksch (:BenB)]

Comment on attachment 9190207 [details]
Bug 1679724 - Part 2: Apply known OAuth2 settings when using Exchange Autodiscover.

r- for the logic changes

The scope change is good. Please make a new patch with only that.

Comment 6

[Ben Bucksch (:BenB)]

Attachment: Fix scope, v3

Hey David, this is the actual fix from your patches.
Could you please test whether that fixes the issue for you? I haven't tested them yet.

Comment 7

[Ben Bucksch (:BenB)]

Attachment: Fix OAuth2 scope, v4

Fix the on-premise Exchange case where there is no OAuth2 config.

Comment 8

[David Ward]

Hi Ben,

Thanks for the comments. The changes are marked in Phabricator as draft and are not submitted for review yet.

It added both authentication methods as alternatives, and that was intentional. If password auth is available, we want to know that, and we actually prefer that.

• In the existing code, SPA is not an alternative auth method to cleartext — or vice versa. If SPA is enabled, it simply overwrites the current auth method.
• In Thunderbird, OAuth2 should only be set as an auth method (or alternative auth method) if the specific OAuth2 provider details are found here. So this change is necessary.

Also, if I'm reading the patch correctly, it creates an invalid AccountConfig object, because the config.incoming properties are not populated, and everything are alternatives.

config.incoming is populated unless it already exists; otherwise it adds the protocol to config.incomingAlternatives:

        if (!config.incoming.hostname) {
          config.incoming = server;
        } else {
          config.incomingAlternatives.push(server);
        }

See next comment:

Sorry, I fail to understand the need for this patch.

What happens if IMAP or POP3 setting appear before the others in the XML file, rather than after? Wouldn't those protocol settings be clobbered by the EWS or OWA settings, or is something enforcing an order when iterating over the protocols that I am missing? With that and the comment below in mind, it would be helpful to make the behavior of this code a bit more explicit.

Have you actually tested this, for all cases? I don't think this is going to work properly.

This part is unclear to me, and I don't have the ability to test directly (part of why this remains a draft). I can see where a single Exchange protocol in the configuration might be needed instead. But what is the preference order among protocols? Should settings for EXHTTP taken first, followed by EXCH, etc.? Right now the settings are simply overwritten each time it iterates over another protocol.

Ultimately, we need a way to consistently apply both the OAuth2 method, and the issuer/scope, for each protocol — if we have server support for it. This is a starting point. If you could help provide an understanding of what needs to be handled differently when obtaining the EWS/OWA-specific protocol configuration, that would be helpful.

Comment 9

[David Ward]

Comment on attachment 9190302 [details] [diff] [review]
Fix OAuth2 scope, v4

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

No, the OAuth2 method and issuer/scope should not be set unless we know them. See previous comment.