Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dev/mail#79 - Fix MS Exchange/IMAP. Use OpenID Connect. #18951

Merged
merged 2 commits into from
Nov 10, 2020

Conversation

totten
Copy link
Member

@totten totten commented Nov 10, 2020

Overview

This addresses a bug (https://lab.civicrm.org/dev/mail/-/issues/79) where the OAuth2 implementation does not work with MS Exchange Online's IMAP.

Before

When requesting a token for the ms-exchange provider, Civi requests multiple permissions, including User.Read (to determine the default email address) and IMAP.AccessAsUser.All (to connect the relevant mailbox).

Thanks to @demeritcowboy's debugging, we can see there is a problem in Microsoft's services: it treats these permissions as mutually-exclusive. (If you have User.Read permission, then IMAP.AccessAsUser.All does not work.)

After

Stop requesting User.Read permission. Consequently, we can't read the default email address from https://graph.microsoft.com/v1.0/me. Instead, we can use OpenID Connect to determine the email address.

Comments

Tested in local builds of 5.32.beta with both:

At time of writing, there appears to be a problem in Microsoft's services:
if you request both `User.Read` and `IMAP.AccessAsUser.All`, then the token
does not actually work for IMAP access.

However, it is does work to combine `openid` and `IMAP.AccessAsUser.All`.

This patch to CiviGenericProvider makes it easier to get resource-owner
details via OpenID Connect's `id_token`.
At time of writing, there appears to be a problem in Microsoft's services:
if you request both `User.Read` and `IMAP.AccessAsUser.All`, then the token
does not actually work for IMAP access.

However, it is does work to combine `openid` and `IMAP.AccessAsUser.All`.

This patch revises the MS Exchange definition to get resource-owner details
via OpenID Connect's `id_token`.
@civibot
Copy link

civibot bot commented Nov 10, 2020

(Standard links)

@civibot civibot bot added the master label Nov 10, 2020
@seamuslee001
Copy link
Contributor

@totten should this be against the 5.32 branch?

@totten totten changed the base branch from master to 5.32 November 10, 2020 01:12
@civibot civibot bot added 5.32 and removed master labels Nov 10, 2020
@totten
Copy link
Member Author

totten commented Nov 10, 2020

@seamuslee001 Yes, thanks for catching that. Switched it over.

@demeritcowboy
Copy link
Contributor

  • Yay it works.
  • I'm still seeing User.Read showing up, and with the wrong scope, but since it comes at the end of the list and it's not being accessed it doesn't affect anything. I'm not sure where it's coming from - I also tried deleting the token/mail account and cleared caches and started again.
    • Oauth admin:
      scopes1
    • Inspect:
      scopes2
  • Also checked that google was still working (although still having refresh token acquisition problems for google but it's probably separate).
  • Just a note regarding outlook.office365.com vs outlook.office.com - it looks like this was changed in May but without any explanation. They seem to be equivalent but yes seems office.com is the more current.

@seamuslee001
Copy link
Contributor

I'm going to merge based on @demeritcowboy 's testing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants