dex
dex copied to clipboard
dexidp
Adjust scopes to be in line with the Microsoft Identity Platform v2.
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 https://github.com/dexidp/dex/issues/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
dear @nabokihms
can I support you for the review? Or is there anything missing from my side?
thanks,
christian
@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 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.
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: https://github.com/dexidp/dex/blob/ee50c093130b92d43905ad85134c506080456ee0/connector/microsoft/microsoft.go#L376 See the API documentation: https://docs.microsoft.com/de-de/graph/api/user-getmembergroups?view=graph-rest-1.0&tabs=http
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.
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.
Unfortunately, I'm no Azure AD admin and lack the permissions in our company to test this. Would you mind doing that?
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.
Any status on this PR? Microsoft is deprecating support for their older identity platform calls this year.
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.
I think I can take a look on it. Since I have no access to Microsoft IdP v2, it can take some time.
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 ?
