-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Check Google group membership with hasMember and get. #224
Conversation
This PR is an enhancement built on oauth2-proxy#160. That PR reduces the number of calls to the Google Admin API and simplifies the code by using the hasMember method. It also supports checking membership in nested groups. However, the above message doesn't handle members who are not a part of the domain. The hasMember API returns a 400 for that case. As a fallback, when the API returns a 400, this change will try using the `get` API which works as expected for members who aren't a part of the domain. Supporting members who belong to the Google group but aren't part of the domain is a requested feature from oauth2-proxy#95. https://developers.google.com/admin-sdk/directory/v1/reference/members/get Note that nested members who are not a part of the domain will not be correctly detected with this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 @jpalpant. Thanks for this PR!
Generally looks great, just a couple of things to look at.
LGTM 👍 - although I think it would be good to add a comment in the code to explain #224 (comment) |
I've added a few comments around the different APIs and when they are used - how does that look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
How should I go about getting this merged? I can resolve the conflict with the changelog, but is there another reviewer we need to check in on it? |
I'll do the honours! |
Support templating of `extraVolumes`
Description
This PR is an enhancement built on #160. That PR reduces the number of calls to the Google Admin API and simplifies the code by using the hasMember method. It also supports checking membership in nested groups.
However, the above message doesn't handle members who are not a part of the domain. The hasMember API returns a 400 for that case. As a fallback, when the API returns a 400, this change will try using the
get
API which works as expected for members who aren't a part of the domain. Supporting members who belong to the Google group but aren't part of the domain is a requested feature from #95.https://developers.google.com/admin-sdk/directory/v1/reference/members/get
Note that nested members who are not a part of the domain will not be correctly detected with this change.
Motivation and Context
This PR continues the work of #160 to improve the Google auth support when using a Google group as a filter. It reduces the complexity of the code, number of API calls, and handles members of nested groups. Additionally it maintains the functionality of checking for group members who do not have the same domain as the target user.
How Has This Been Tested?
I have tested this manually against my own Google groups by running
oauth2_proxy
using the arguments-email-domain=* -google-group=$MYGROUP -provider=google -google-admin-email=$ADMIN_EMAIL -google-service-account-json=/path/to/serviceaccount.json
. Within that context I have checked that authorization works as expected for:I have not tested other configurations yet, such as Google authentication without the groups filter. However I don't believe I've touched those parts of the code. I have not tested multiple-groups.
I modified the unit tests for the Google provider. Since the function makes different requests, I have written new tests and modified the dummy server to have responses for each request that will be made.
Checklist: