From e92f38f38f211967ef40d23e03de569b7a30f01e Mon Sep 17 00:00:00 2001
From: Eric Stroczynski <ericstroczynski@gmail.com>
Date: Thu, 17 Aug 2017 10:13:00 -0700
Subject: [PATCH] connector/github: error if no groups scope without orgs

We should always check if a user is in any orgs or teams specified
in config, and whether the groups scope is also included in client
requests. If not, return an error, because dex wouldn't have required
permissions to do the request anyway (need read:org).
---
 connector/github/github.go | 77 +++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 42 deletions(-)

diff --git a/connector/github/github.go b/connector/github/github.go
index 83281aec..462786df 100644
--- a/connector/github/github.go
+++ b/connector/github/github.go
@@ -134,15 +134,14 @@ type githubConnector struct {
 }
 
 func (c *githubConnector) oauth2Config(scopes connector.Scopes) *oauth2.Config {
-	var githubScopes []string
-	if scopes.Groups {
-		githubScopes = []string{scopeEmail, scopeOrgs}
-	} else {
-		githubScopes = []string{scopeEmail}
+	// The GitHub API requires requires read:org scope to ensure users are members
+	// of orgs and teams provided in configs.
+	githubScopes := []string{scopeEmail}
+	if len(c.orgs) > 0 || c.org != "" {
+		githubScopes = append(githubScopes, scopeOrgs)
 	}
 
 	endpoint := github.Endpoint
-
 	// case when it is a GitHub Enterprise account.
 	if c.hostName != "" {
 		endpoint = oauth2.Endpoint{
@@ -207,6 +206,22 @@ func newHTTPClient(rootCA string) (*http.Client, error) {
 	}, nil
 }
 
+// getGroups retrieves GitHub orgs and teams a user is in, if any.
+func (c *githubConnector) getGroups(ctx context.Context, client *http.Client, groupScope bool, userLogin string) (groups []string, err error) {
+	// Org and team membership should always be checked if specified by config.
+	if len(c.orgs) > 0 {
+		if groups, err = c.listGroups(ctx, client, userLogin); err != nil {
+			return groups, err
+		}
+	} else if groupScope && c.org != "" {
+		// Do not check org membership here to preserve legacy behavior.
+		if groups, err = c.teams(ctx, client, c.org); err != nil {
+			return groups, fmt.Errorf("github: get teams: %v", err)
+		}
+	}
+	return groups, nil
+}
+
 func (c *githubConnector) HandleCallback(s connector.Scopes, r *http.Request) (identity connector.Identity, err error) {
 	q := r.URL.Query()
 	if errType := q.Get("error"); errType != "" {
@@ -244,24 +259,13 @@ func (c *githubConnector) HandleCallback(s connector.Scopes, r *http.Request) (i
 		EmailVerified: true,
 	}
 
-	if s.Groups {
-		var groups []string
-		if len(c.orgs) > 0 {
-			if groups, err = c.listGroups(ctx, client, user.Login); err != nil {
-				return identity, err
-			}
-		} else if c.org != "" {
-			inOrg, err := c.userInOrg(ctx, client, user.Login, c.org)
-			if err != nil {
-				return identity, err
-			}
-			if !inOrg {
-				return identity, fmt.Errorf("github: user %q not a member of org %q", user.Login, c.org)
-			}
-			if groups, err = c.teams(ctx, client, c.org); err != nil {
-				return identity, fmt.Errorf("github: get teams: %v", err)
-			}
-		}
+	groups, err := c.getGroups(ctx, client, s.Groups, user.Login)
+	if err != nil {
+		return identity, err
+	}
+	// Preserve behavior of only setting identity.Groups if 'orgs' or groups scope
+	// and 'org' are specified.
+	if len(c.orgs) > 0 || (s.Groups && c.org != "") {
 		identity.Groups = groups
 	}
 
@@ -300,24 +304,13 @@ func (c *githubConnector) Refresh(ctx context.Context, s connector.Scopes, ident
 	identity.Username = username
 	identity.Email = user.Email
 
-	if s.Groups {
-		var groups []string
-		if len(c.orgs) > 0 {
-			if groups, err = c.listGroups(ctx, client, user.Login); err != nil {
-				return identity, err
-			}
-		} else if c.org != "" {
-			inOrg, err := c.userInOrg(ctx, client, user.Login, c.org)
-			if err != nil {
-				return identity, err
-			}
-			if !inOrg {
-				return identity, fmt.Errorf("github: user %q not a member of org %q", user.Login, c.org)
-			}
-			if groups, err = c.teams(ctx, client, c.org); err != nil {
-				return identity, fmt.Errorf("github: get teams: %v", err)
-			}
-		}
+	groups, err := c.getGroups(ctx, client, s.Groups, user.Login)
+	if err != nil {
+		return identity, err
+	}
+	// Preserve behavior of only setting identity.Groups if 'orgs' or groups scope
+	// and 'org' are specified.
+	if len(c.orgs) > 0 || (s.Groups && c.org != "") {
 		identity.Groups = groups
 	}