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

Check Google group membership with hasMember and get. #224

Merged
merged 5 commits into from
Aug 6, 2019

Conversation

jpalpant
Copy link
Contributor

@jpalpant jpalpant commented Jul 24, 2019

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:

  • (allowed) a member of the group with the group domain
  • (allowed) a member of the group with an external domain
  • (disallowed) nonmember of the group with the group domain
  • (disallowed) nonmember of the group with an external domain
  • (allowed) member of a subgroup where subgroup and user have the group domain

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:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

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.
@JoelSpeed JoelSpeed requested a review from a team July 24, 2019 08:21
Copy link
Contributor

@steakunderscore steakunderscore left a 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.

providers/google.go Show resolved Hide resolved
providers/google_test.go Outdated Show resolved Hide resolved
@jpalpant jpalpant requested a review from loshz as a code owner July 30, 2019 09:02
@loshz
Copy link
Contributor

loshz commented Jul 30, 2019

LGTM 👍 - although I think it would be good to add a comment in the code to explain #224 (comment)

@jpalpant
Copy link
Contributor Author

I've added a few comments around the different APIs and when they are used - how does that look?

Copy link
Contributor

@steakunderscore steakunderscore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@loshz loshz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@jpalpant
Copy link
Contributor Author

jpalpant commented Aug 5, 2019

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?

@loshz
Copy link
Contributor

loshz commented Aug 6, 2019

I'll do the honours!

@loshz loshz merged commit 7d910c0 into oauth2-proxy:master Aug 6, 2019
Jing-ze pushed a commit to Jing-ze/oauth2-proxy that referenced this pull request Nov 19, 2024
T-vK pushed a commit to T-vK/oauth2-proxy that referenced this pull request Nov 20, 2024
Support templating of `extraVolumes`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants