feat: support for multiple github orgs (oauth2-proxy#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 <[email protected]>
Co-authored-by: Jan Larwig <[email protected]>

Author: daniel-mersch

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

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,38 +24,52 @@ 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 <org>:<slug>, 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 
 [token](https://github.com/settings/tokens) for a user that has write access to the repository. The token must be 
 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:

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++

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, "[email protected]", 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}`},