Add GitHub groups (orgs/teams) support #1928
Conversation
08273ac to
125ead6
Compare
|
@JoelSpeed ready for first review. |
|
@JoelSpeed I was thinking about replacing the restriction on orgs and teams by a single flag containing either orgs or org:teams --github-groups="Org1,Org2,Org3:Team1,Org3:Team2" Furthermore, the original and current implementation only allows to restrict on a single org with multiple teams but not multiple orgs. What do you think about this proposal? (Could be part of a second PR) |
| @@ -31,7 +32,8 @@ var _ Provider = (*GitHubProvider)(nil) | |||
|
|
|||
| const ( | |||
| githubProviderName = "GitHub" | |||
| githubDefaultScope = "user:email" | |||
| githubDefaultScope = "user:email read:org" | |||
| orgTeamSeparator = ":" | |||
There was a problem hiding this comment.
I'm still considering "/" as the orgTeamSeparator
There was a problem hiding this comment.
@JoelSpeed ":" vs. "/" ? do you have a clear preference?
Or should it be configurable 🤔
| // <https://api.github.com/user/teams?page=3&per_page=10>; rel="prev", <https://api.github.com/user/teams?page=1&per_page=10>; rel="first" | ||
|
|
||
| link := result.Headers().Get("Link") | ||
| rep1 := regexp.MustCompile(`(?s).*\<https://api.github.com/user/teams\?page=(.)&per_page=[0-9]+\>; rel="last".*`) |
There was a problem hiding this comment.
I haven't had the chance to check this. But I don't think this will work on an enterprise server instance with a custom domain as the api url would be different. As this isn't part of the new implementation and has been in the code base for a while. I would leave it as is for now.
There was a problem hiding this comment.
I'll create a PR, already fixed it locally to work with Github Enterprise Server.
|
@JoelSpeed any thoughts on this PR? Or should I ask someone else from the maintainer list regarding the review? |
…rove overall code quality
59b1ed6 to
6489cc6
Compare
Any updates regarding the review? 😄 |
|
I would really like to see this merged soon! Would simply our setup at work a lot. |
|
I did a quick test with GitHub Enterprise Server 3.5.2 using /auth endpoint and it works very well. Thank you @tuunit! |
|
@JoelSpeed @steakunderscore @desaintmartin @NickMeves can I help you to review this and some of the other open pull requests? As there are currently 44 open PRs and 98 issues. |
| // <https://api.github.com/user/teams?page=3&per_page=10>; rel="prev", <https://api.github.com/user/teams?page=1&per_page=10>; rel="first" | ||
|
|
||
| link := result.Headers().Get("Link") | ||
| rep1 := regexp.MustCompile(`(?s).*\<https://api.github.com/user/teams\?page=(.)&per_page=[0-9]+\>; rel="last".*`) |
Check failure
Code scanning / CodeQL
Incomplete regular expression for hostnames
|
This pull request has been inactive for 60 days. If the pull request is still relevant please comment to re-activate the pull request. If no action is taken within 7 days, the pull request will be marked closed. |
|
@JoelSpeed please reopen :) |
|
The branch has since been force pushed so I can't reopen it, will need a fresh PR |
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: