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

[Bug]: Azure AD (Entra ID) documentation recommends unnecessarily wide permission #2772

Open
d3-jshaffer opened this issue Sep 10, 2024 · 2 comments

Comments

@d3-jshaffer
Copy link

OAuth2-Proxy Version

7.6.0

Provider

azure

Expected Behaviour

The documentation should recommend the DELEGATED permission GroupMember.Read.All which will allow the application to read the group membership of only the user currently signing in. This grants access to much less data: "Allows the app to list groups, read basic group properties and read membership of all groups the signed-in user has access to." We tested this internally and confirmed that the permission allows oauth2-proxy to verify group memberships.

Current Behaviour

The documentation recommends the application permission Group.Read.All which grants access to a lot more data than necessary: "Allows the app to read group properties and memberships, and read conversations for all groups, without a signed-in user." My understanding is it also grants access to all files and calendars in those groups as well (see: https://learn.microsoft.com/en-us/graph/permissions-reference#groupreadall).

Steps To Reproduce

The documentation is Step 3 on https://oauth2-proxy.github.io/oauth2-proxy/configuration/providers/azure

Possible Solutions

No response

Configuration details or additional information

No response

@d-led
Copy link

d-led commented Nov 22, 2024

Most other solutions only need the User.Read permission. Group.Read.All allows reading "other" users, which is not necessary

@tuunit
Copy link
Member

tuunit commented Nov 22, 2024

This will not be changed for the current azure ad implementation as it is going to be deprecated. This issue has been addressed in #2390 and will soon be merged and released

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

No branches or pull requests

3 participants