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

Feature: Add GitHub groups (orgs/teams) support #2196

Merged

Conversation

tuunit
Copy link
Member

@tuunit tuunit commented Aug 27, 2023

Motivation and Context

GitHub's hierarchy is structured by organization and teams. As of now, in version 7.4.0 this hierarchy can only be used for restricting access but the details about a users membership are not passed through to the underlying applications behind the oauth2-proxy. Therefore this PRs aim is to extend the GitHub provider with the functionality to expose the users membership. As the session storage already provides the functionality to store "groups" and forward them as headers, this is a valid feature / extension and in line with other providers.

Description of the implementation details

I refactored the GitHub provider quite extensively. To ensure backwards compatibility all the restrictions and checks are done as before. The only logical change is that instead of just fetching the org and team information when an organisation or team restriction is set, the membership information is always fetched from GitHub and added to the sessions group storage.

How Has This Been Tested?

All existing GitHub tests have been corrected to accommodate the new code structure. Manual testing has been done and I already use this version of the implementation in a production environment.

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.

@tuunit tuunit requested a review from a team as a code owner August 27, 2023 10:04
@tuunit tuunit force-pushed the feature/github-orgs-and-teams-as-groups branch from f87a1a7 to a9c31a5 Compare August 27, 2023 10:05
@tuunit
Copy link
Member Author

tuunit commented Aug 27, 2023

@JoelSpeed new PR for #1928

@hoax I would highly appreciate a code review from your side and especially check that I didn't miss anything when rebasing our commits.

@tuunit tuunit changed the title Feature/GitHub orgs and teams as groups Feature: Add GitHub groups (orgs/teams) support Aug 27, 2023
@tuunit tuunit added this to the v8.0.0 milestone Sep 13, 2023
@tuunit tuunit modified the milestones: v8.0.0, v7.6.0 Sep 14, 2023
@hoax
Copy link
Contributor

hoax commented Sep 23, 2023

@tuunit, looks good, but did not test it yet

Copy link
Member Author

@tuunit tuunit left a comment

Choose a reason for hiding this comment

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

Reminder to myself: Rebase and fix merge conflicts

@tuunit tuunit force-pushed the feature/github-orgs-and-teams-as-groups branch from 50fbfab to e09b152 Compare October 15, 2023 08:37
@tuunit
Copy link
Member Author

tuunit commented Oct 25, 2023

@kvanzuijlen can you give this a review? :)

Copy link
Member

@kvanzuijlen kvanzuijlen left a comment

Choose a reason for hiding this comment

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

LGTM

@kvanzuijlen kvanzuijlen added the LGTM PR is ready to merge label Nov 7, 2023
@tuunit tuunit force-pushed the feature/github-orgs-and-teams-as-groups branch 2 times, most recently from 7c5248a to 8d2c360 Compare November 20, 2023 22:12
@tuunit tuunit force-pushed the feature/github-orgs-and-teams-as-groups branch from 0599724 to dbe7a0c Compare December 17, 2023 10:49
tuunit and others added 5 commits December 18, 2023 10:35
* add nightly build and push

* add date based nightly build tags

* only keep single multiarch image build and push

* add changelog

* add images to internal docs static files

* add docu for nightly builds

* remove unnecessary spaces

* update nightly repository
miguelborges99 and others added 7 commits December 18, 2023 10:35
…oxy#1949)

* Issue 978: Fix Custom cookie name breaks redis for session (see oauth2-proxy#978)

* Issue 978: Fix Custom cookie name breaks redis for session (see oauth2-proxy#978)

* Update CHANGELOG.md

* Issue 978: Fix Custom cookie name breaks redis for session

* Issue 978: Fix Custom cookie name breaks redis for session

* Issue 978: Fix Custom cookie name breaks redis for session

* Issue 978: Fix Custom cookie name breaks redis for session

* Issue 978: Fix Custom cookie name breaks redis for session

* Issue 978: Fix Custom cookie name breaks redis for session

* Update CHANGELOG.md

---------

Co-authored-by: Nuno Borges <Nuno.Borges@ctw.bmwgroup.com>
Co-authored-by: Joel Speed <Joel.speed@hotmail.co.uk>
* Support http.AllowQuerySemicolons

* Docs

* Make it clear we are overriding the handler

* Update documentation for allow-query-semicolons

* Fix changelog format

* Fix formatting

---------

Co-authored-by: MickMake <github@mickmake.com>
@tuunit tuunit force-pushed the feature/github-orgs-and-teams-as-groups branch from dbe7a0c to 8a2d360 Compare December 18, 2023 09:35
Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Is this implemented in a backwards compatible way?

@JoelSpeed JoelSpeed merged commit 52ad317 into oauth2-proxy:master Dec 18, 2023
8 checks passed
@jacek-jablonski
Copy link

Is this implemented in a backwards compatible way?

Nope, it blew away our production.

@tuunit
Copy link
Member Author

tuunit commented Feb 21, 2024

Is this implemented in a backwards compatible way?

Nope, it blew away our production.

I've tested it with 3 different scenarios and another user tested the feature branch as well so I'm a bit lost what happened on your end.

Can you please share your config and which errors you encountered?

@jacek-jablonski
Copy link

Hi,
config by args:

--provider=github --scope="read:user" --email-domain=* --github-team="xxx/devs" --upstream=http://argo-server:2746 --http-address=0.0.0.0:4180 --reverse-proxy

OAUTH2_PROXY_CLIENT_ID and OAUTH2_PROXY_CLIENT_SECRET set by env vars.

Error was:

[2024/02/21 14:18:43] [oauthproxy.go:888] Error creating session during OAuth2 callback: unexpected status "403": {"message":"You need at least read:org scope or user scope to list your organizations.","documentation_url":"https://docs.github.com/rest/orgs/orgs#list-organizations-for-the-authenticated-user"}

@etho201
Copy link

etho201 commented Feb 21, 2024

Hi, config by args:

--provider=github --scope="read:user" --email-domain=* --github-team="xxx/devs" --upstream=http://argo-server:2746 --http-address=0.0.0.0:4180 --reverse-proxy

OAUTH2_PROXY_CLIENT_ID and OAUTH2_PROXY_CLIENT_SECRET set by env vars.

Error was:

[2024/02/21 14:18:43] [oauthproxy.go:888] Error creating session during OAuth2 callback: unexpected status "403": {"message":"You need at least read:org scope or user scope to list your organizations.","documentation_url":"https://docs.github.com/rest/orgs/orgs#list-organizations-for-the-authenticated-user"}

I had the same problem. It's a breaking change if/when a user specified the scope. For example, I was using this flag --scope=user:email and this broke things after updating to 7.6.0. To fix this, I had to either remove my manual setting of the flag or add the new setting user:email read:org. If you don't specify, the default value is now user:email read:org. See here for more: https://github.com/oauth2-proxy/oauth2-proxy/pull/2196/files#r1492008964

@tuunit
Copy link
Member Author

tuunit commented Feb 22, 2024

@etho201 thanks for sharing. Indeed that is the issue we will update it in the CHANGELOG and I'll talk to @JoelSpeed if we can get a bugfix released.

@tuunit
Copy link
Member Author

tuunit commented Feb 22, 2024

@etho201 @jacek-jablonski
I'm sorry for the inconvenience :/

@jacek-jablonski
Copy link

@etho201 @jacek-jablonski

I'm sorry for the inconvenience :/

No problem at all. Thanks for the great work 🙂

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

Successfully merging this pull request may close these issues.

None yet

8 participants