From 7731437af49c2767dac211694b2eec70f71b799c Mon Sep 17 00:00:00 2001 From: Daniel Mersch <94058351+daniel-mersch@users.noreply.github.com> Date: Thu, 29 May 2025 18:11:07 +0200 Subject: [PATCH] feat: support for multiple github orgs (#3072) * fix for github teams * Update github.go * added errorhandling * Update github.md * refactored GitHub provider refactored hasOrg, hasOrgAndTeams and hasTeam into hasAccess to stay within function limit * reverted Refactoring * refactored github.go - joined hasOrgAndTeamAccess into checkRestrictions * refactored github.go - reduced number of returns of function checkRestrictions to 4 * updated GitHub provider to accept legacy team ids * GoFmt and golangci-lint Formatted with GoFmt and followed recommendations of GoLint * added Tests added Tests for checkRestrictions. * refactored in maintainer feedback * Removed code, documentation and tests for legacy ids * add changelog and update docs --------- Signed-off-by: Jan Larwig Co-authored-by: Jan Larwig --- CHANGELOG.md | 2 + docs/docs/configuration/providers/github.md | 34 ++++-- providers/github.go | 62 +++++++--- providers/github_test.go | 129 ++++++++++++++++++++ 4 files changed, 201 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dca9694a..eb7b3e2b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ ## Changes since v7.9.0 +- [#3072](https://github.com/oauth2-proxy/oauth2-proxy/pull/3072) feat: support for multiple github orgs #3072 (@daniel-mersch) + # V7.9.0 ## Release Highlights diff --git a/docs/docs/configuration/providers/github.md b/docs/docs/configuration/providers/github.md index 04c3a4ef..cebca314 100644 --- a/docs/docs/configuration/providers/github.md +++ b/docs/docs/configuration/providers/github.md @@ -8,7 +8,7 @@ title: GitHub | Flag | Toml Field | Type | Description | Default | | ---------------- | -------------- | -------------- | ------------------------------------------------------------------------------------------------------------- | ------- | | `--github-org` | `github_org` | string | restrict logins to members of this organisation | | -| `--github-team` | `github_team` | string | restrict logins to members of any of these teams (slug), separated by a comma | | +| `--github-team` | `github_team` | string | restrict logins to members of any of these teams (slug) or (org:team), comma separated | | | `--github-repo` | `github_repo` | string | restrict logins to collaborators of this repository formatted as `orgname/repo` | | | `--github-token` | `github_token` | string | the token to use when verifying repository collaborators (must have push access to the repository) | | | `--github-user` | `github_users` | string \| list | To allow users to login by username even if they do not belong to the specified org and team or collaborators | | @@ -24,23 +24,36 @@ team level access, or to collaborators of a repository. Restricting by these opt NOTE: When `--github-user` is set, the specified users are allowed to log in even if they do not belong to the specified org and team or collaborators. -To restrict by organization only, include the following flag: +To restrict access to your organization: ```shell - --github-org="" # restrict logins to members of this organisation + # restrict logins to members of this organisation + --github-org="your-org" ``` -To restrict within an organization to specific teams, include the following flag in addition to `-github-org`: +To restrict access to specific teams within an organization: ```shell - --github-team="" # restrict logins to members of any of these teams (slug), separated by a comma + --github-org="your-org" + # restrict logins to members of any of these teams (slug), comma separated + --github-team="team1,team2,team3" +``` + +To restrict to teams within different organizations, keep the organization flag empty and use `--github-team` like so: + +```shell + # keep empty + --github-org="" + # restrict logins to members to any of the following teams (format :, like octo:team1), comma separated + --github-team="org1:team1,org2:team1,org3:team42,octo:cat" ``` If you would rather restrict access to collaborators of a repository, those users must either have push access to a public repository or any access to a private repository: ```shell - --github-repo="" # restrict logins to collaborators of this repository formatted as orgname/repo + # restrict logins to collaborators of this repository formatted as orgname/repo + --github-repo="" ``` If you'd like to allow access to users with **read only** access to a **public** repository you will need to provide a @@ -48,14 +61,15 @@ If you'd like to allow access to users with **read only** access to a **public** created with at least the `public_repo` scope: ```shell - --github-token="" # the token to use when verifying repository collaborators + # the token to use when verifying repository collaborators + --github-token="" ``` -To allow a user to log in with their username even if they do not belong to the specified org and team or collaborators, -separated by a comma +To allow a user to log in with their username even if they do not belong to the specified org and team or collaborators: ```shell - --github-user="" #allow logins by username, separated by a comma + # allow logins by username, comma separated + --github-user="" ``` If you are using GitHub enterprise, make sure you set the following to the appropriate url: diff --git a/providers/github.go b/providers/github.go index f85578fc..749fcc03 100644 --- a/providers/github.go +++ b/providers/github.go @@ -200,6 +200,7 @@ func (p *GitHubProvider) hasOrgAndTeam(s *sessions.SessionState) error { if strings.EqualFold(p.Org, ot.Org) { hasOrg = true + teams := strings.Split(p.Team, ",") for _, team := range teams { if strings.EqualFold(strings.TrimSpace(team), ot.Team) { @@ -220,6 +221,37 @@ func (p *GitHubProvider) hasOrgAndTeam(s *sessions.SessionState) error { return errors.New("user is missing required organization") } +func (p *GitHubProvider) hasTeam(s *sessions.SessionState) error { + var teams []string + + for _, group := range s.Groups { + if strings.Contains(group, orgTeamSeparator) { + teams = append(teams, strings.TrimSpace(group)) + } + } + + var presentTeams = make([]string, 0, len(teams)) + + for _, ot := range teams { + allowedTeams := strings.Split(p.Team, ",") + for _, team := range allowedTeams { + if !strings.Contains(team, orgTeamSeparator) { + logger.Printf("Please use fully qualified team names (org:team-slug) if you omit the organisation. Current Team name: %s", team) + return errors.New("team name is invalid") + } + + if strings.EqualFold(strings.TrimSpace(team), ot) { + logger.Printf("Found Github Organization/Team:%s", ot) + return nil + } + } + presentTeams = append(presentTeams, ot) + } + + logger.Printf("Missing Team:%q in teams: %v", p.Team, presentTeams) + return errors.New("user is missing required team") +} + func (p *GitHubProvider) hasRepoAccess(ctx context.Context, accessToken string) error { // https://developer.github.com/v3/repos/#get-a-repository @@ -378,12 +410,22 @@ func (p *GitHubProvider) checkRestrictions(ctx context.Context, s *sessions.Sess return err } - if err := p.hasOrgAndTeamAccess(s); err != nil { + var err error + switch { + case p.Org != "" && p.Team != "": + err = p.hasOrgAndTeam(s) + case p.Org != "": + err = p.hasOrg(s) + case p.Team != "": + err = p.hasTeam(s) + } + + if err != nil { return err } if p.Org == "" && p.Repo != "" && p.Token == "" { - // If we have a token we'll do the collaborator check in GetUserName + // If we have a token we'll do the collaborator check return p.hasRepoAccess(ctx, s.AccessToken) } @@ -408,18 +450,6 @@ func (p *GitHubProvider) checkUserRestriction(ctx context.Context, s *sessions.S return verifiedUser, nil } -func (p *GitHubProvider) hasOrgAndTeamAccess(s *sessions.SessionState) error { - if p.Org != "" && p.Team != "" { - return p.hasOrgAndTeam(s) - } - - if p.Org != "" { - return p.hasOrg(s) - } - - return nil -} - func (p *GitHubProvider) getOrgAndTeam(ctx context.Context, s *sessions.SessionState) error { err := p.getOrgs(ctx, s) if err != nil { @@ -503,8 +533,8 @@ func (p *GitHubProvider) getTeams(ctx context.Context, s *sessions.SessionState) } for _, team := range teams { - logger.Printf("Member of Github Organization/Team:%q/%q", team.Org.Login, team.Slug) - s.Groups = append(s.Groups, team.Org.Login+orgTeamSeparator+team.Slug) + logger.Printf("Member of Github Organization/Team: %q/%q", team.Org.Login, team.Slug) + s.Groups = append(s.Groups, fmt.Sprintf("%s%s%s", team.Org.Login, orgTeamSeparator, team.Slug)) } pn++ diff --git a/providers/github_test.go b/providers/github_test.go index 8dfb943f..e86f5d7b 100644 --- a/providers/github_test.go +++ b/providers/github_test.go @@ -39,6 +39,7 @@ func testGitHubBackend(payloads map[string][]string) *httptest.Server { "/repos/oauth2-proxy/oauth2-proxy/collaborators/mbland": {""}, "/user": {""}, "/user/emails": {""}, + "/user/teams": {"page=1&per_page=100", "page=2&per_page=100", "page=3&per_page=100"}, "/user/orgs": {"page=1&per_page=100", "page=2&per_page=100", "page=3&per_page=100"}, // GitHub Enterprise Server API "/api/v3": {""}, @@ -168,6 +169,134 @@ func TestGitHubProvider_getEmailWithOrg(t *testing.T) { assert.Equal(t, "michael.bland@gsa.gov", session.Email) } +func TestGitHubProvider_checkRestrictionsOrg(t *testing.T) { + b := testGitHubBackend(map[string][]string{ + "/user/emails": {`[ {"email": "john.doe@example", "verified": true, "primary": true} ]`}, + "/user/orgs": { + `[ { "login": "test-org-1" } ]`, + `[ { "login": "test-org-2" } ]`, + `[ ]`, + }, + "/user/teams": { + `[ { "name":"test-team-1", "slug":"test-team-1", "organization": { "login": "test-org-1" } } ]`, + `[ { "name":"test-team-2", "slug":"test-team-2", "organization": { "login": "test-org-2" } } ]`, + `[ ]`, + }, + }) + defer b.Close() + + // This test should succeed, because of the valid organization. + bURL, _ := url.Parse(b.URL) + p := testGitHubProvider(bURL.Host, + options.GitHubOptions{ + Org: "test-org-1", + }, + ) + + var err error + session := CreateAuthorizedSession() + err = p.getOrgAndTeam(context.Background(), session) + assert.NoError(t, err) + err = p.checkRestrictions(context.Background(), session) + assert.NoError(t, err) + + // This part should fail, because user is not part of the organization + p = testGitHubProvider(bURL.Host, + options.GitHubOptions{ + Org: "test-org-1-fail", + }, + ) + + err = p.checkRestrictions(context.Background(), session) + assert.Error(t, err) +} + +func TestGitHubProvider_checkRestrictionsOrgTeam(t *testing.T) { + b := testGitHubBackend(map[string][]string{ + "/user/emails": {`[ {"email": "john.doe@example", "verified": true, "primary": true} ]`}, + "/user/orgs": { + `[ { "login": "test-org-1" } ]`, + `[ { "login": "test-org-2" } ]`, + `[ ]`, + }, + "/user/teams": { + `[ { "name":"test-team-1", "slug":"test-team-1", "organization": { "login": "test-org-1" } } ]`, + `[ { "name":"test-team-2", "slug":"test-team-2", "organization": { "login": "test-org-2" } } ]`, + `[ ]`, + }, + }) + defer b.Close() + + // This test should succeed, because of the valid Org and Team combination. + bURL, _ := url.Parse(b.URL) + p := testGitHubProvider(bURL.Host, + options.GitHubOptions{ + Org: "test-org-1", + Team: "test-team-1", + }, + ) + + var err error + session := CreateAuthorizedSession() + err = p.getOrgAndTeam(context.Background(), session) + assert.NoError(t, err) + err = p.checkRestrictions(context.Background(), session) + assert.NoError(t, err) + + // This part should fail, because user is not part of the organization and the team + p = testGitHubProvider(bURL.Host, + options.GitHubOptions{ + Org: "test-org-1-fail", + Team: "test-team-1-fail", + }, + ) + + err = p.checkRestrictions(context.Background(), session) + assert.Error(t, err) +} + +func TestGitHubProvider_checkRestrictionsTeam(t *testing.T) { + b := testGitHubBackend(map[string][]string{ + "/user/emails": {`[ {"email": "john.doe@example", "verified": true, "primary": true} ]`}, + "/user/orgs": { + `[ { "login": "test-org-1" } ]`, + `[ { "login": "test-org-2" } ]`, + `[ ]`, + }, + "/user/teams": { + `[ { "name":"test-team-1", "slug":"test-team-1", "organization": { "login": "test-org-1" } } ]`, + `[ { "name":"test-team-2", "slug":"test-team-2", "organization": { "login": "test-org-2" } } ]`, + `[ ]`, + }, + }) + defer b.Close() + + // This test should succeed, because of the valid org:slug combination. + bURL, _ := url.Parse(b.URL) + p := testGitHubProvider(bURL.Host, + options.GitHubOptions{ + Team: "test-org-1:test-team-1, test-org-2:test-team-2-fail", + }, + ) + + var err error + session := CreateAuthorizedSession() + err = p.getOrgAndTeam(context.Background(), session) + assert.NoError(t, err) + err = p.checkRestrictions(context.Background(), session) + assert.NoError(t, err) + + // This part should fail, because user is not part of the organization:team combination + p = testGitHubProvider(bURL.Host, + options.GitHubOptions{ + Team: "test-org-1-fail:test-team-1-fail, test-org-2-fail:test-team-2-fail", + }, + ) + + err = p.checkRestrictions(context.Background(), session) + assert.Error(t, err) +} + func TestGitHubProvider_getEmailWithWriteAccessToPublicRepo(t *testing.T) { b := testGitHubBackend(map[string][]string{ "/repo/oauth2-proxy/oauth2-proxy": {`{"permissions": {"pull": true, "push": true}, "private": false}`},