Adjust scopes to be in line with the Microsoft Identity Platform v2.

[schuhu]

Overview

This PR adds the openid scope necessary for the Microsoft Identity Platform v2 as well as changes the user and group scopes to the ones now supported on the Microsoft graph API.
This PR was created together with Microsoft backend engineers and fixes #1855

What this PR does / why we need it

The current Microsoft connector just works. But if you enabled features like 2fa on your Microsoft Azure AD Application this is not enforced, because the scopes used trigger the Azure App in a different way than intended.

So, to be able to use 2fa on a Microsoft Azure AD App, you need those changes.

Special notes for your reviewer

I tested this change locally with group sync and refresh token. Same behaviour as before, except that policies like 2fa applied to the Azure AD App now are enforced by Microsoft.

Does this PR introduce a user-facing change?

NONE

[schuhu]

dear @nabokihms

can I support you for the review? Or is there anything missing from my side?

thanks,

christian

[nabokihms]

@schuhu Hello, thanks for submitting the PR. According to the description, it might be a good improvement. Unfortunately, we have no integration tests for this connector, so it will take us some time to review the changes.

[schuhu]

@schuhu Hello, thanks for submitting the PR. According to the description, it might be a good improvement. Unfortunately, we have no integration tests for this connector, so it will take us some time to review the changes.

no problem, just tell me how I can assist.

[wiegandf]

Is Directory.Read.All still required or would we need to adapt https://github.com/dexidp/website/blob/main/content/docs/connectors/microsoft.md?

I am wondering because with scopeDefault = "https://graph.microsoft.com/.default", dex was still trying to get the Directory.Read.All scope (added to my application but not granted consent yet) and it failed for me. When I added user.read scope instead of scopeDefault, everything seems to work fine. But maybe I am overseeing an issue here?

Isn't User.Read enough for this call:

dex/connector/microsoft/microsoft.go

Line 376 in ee50c09

reqURL := c.graphURL + "/v1.0/me/getMemberGroups"

See the API documentation: https://docs.microsoft.com/de-de/graph/api/user-getmembergroups?view=graph-rest-1.0&tabs=http

[schuhu]

According to Microsoft, you still need to give the app the Directory.Read.All permission. Unfortunately I don't know if this is only necessary if you sync groups or if this is a basic requirement.

[wiegandf]

I am positive that it's not required for this call https://docs.microsoft.com/de-de/graph/api/user-getmembergroups, as dex is using delegated permissions only (that's what's also documented in https://github.com/dexidp/website/blob/main/content/docs/connectors/microsoft.md#caveats).

With the default scopes it should be fine and we would just need to change the documentation which scopes are really required in the application.

[schuhu]

Unfortunately, I'm no Azure AD admin and lack the permissions in our company to test this. Would you mind doing that?

[schuhu]

@wiegandf do you expect this to be merged sometime soon?

[wiegandf]

Unfortunately, I also have no means to test this. And I am also not one of the dex maintainers, so I can't merge it.

[brokenjacobs]

Any status on this PR? Microsoft is deprecating support for their older identity platform calls this year.

[schuhu]

Any status on this PR? Microsoft is deprecating support for their older identity platform calls this year.

tbh, because dex did not yet merge it, we run our own fork with this connector for over a year now in production.

we use dex to bridge the gap between istio/oauth2-proxy and aad for dozens of apps, hundreds of users and, thousands of logins a day.

[netsplit]

@nabokihms can we somehow bring this forward?

[nabokihms]

I think I can take a look on it. Since I have no access to Microsoft IdP v2, it can take some time.

[pdf]

The PR is a bit messy (merge commits and 6407711 should not be included here), but it would also solve #2442. I'll try to find some time to test against our AzureAD tenant this week if that would be useful @nabokihms ?