connector/github: abstract scope check and group getter

connector/github/github.go

@@ -24,9 +24,12 @@ import (
 )
 
 const (
-	apiURL     = "https://api.github.com"
+	apiURL = "https://api.github.com"
+	// GitHub requires this scope to access '/user' and '/user/emails' API endpoints.
 	scopeEmail = "user:email"
-	scopeOrgs  = "read:org"
+	// GitHub requires this scope to access '/user/teams' and '/orgs' API endpoints
+	// which are used when a client includes the 'groups' scope.
+	scopeOrgs = "read:org"
 )
 
 // Pagination URL patterns
@@ -133,11 +136,18 @@ type githubConnector struct {
 	httpClient *http.Client
 }
 
+// groupsRequired returns whether dex requires GitHub's 'read:org' scope. Dex
+// needs 'read:org' if 'orgs' or 'org' fields are populated in a config file.
+// Clients can require 'groups' scope without setting 'orgs'/'org'.
+func (c *githubConnector) groupsRequired(groupScope bool) bool {
+	return len(c.orgs) > 0 || c.org != "" || groupScope
+}
+
 func (c *githubConnector) oauth2Config(scopes connector.Scopes) *oauth2.Config {
-	// The GitHub API requires requires read:org scope to ensure users are members
-	// of orgs and teams provided in configs.
+	// 'read:org' scope is required by the GitHub API, and thus for dex to ensure
+	// a user is a member of orgs and teams provided in configs.
 	githubScopes := []string{scopeEmail}
-	if len(c.orgs) > 0 || c.org != "" {
+	if c.groupsRequired(scopes.Groups) {
 		githubScopes = append(githubScopes, scopeOrgs)
 	}
 
@@ -206,22 +216,6 @@ 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 != "" {
@@ -259,13 +253,12 @@ func (c *githubConnector) HandleCallback(s connector.Scopes, r *http.Request) (i
 		EmailVerified: true,
 	}
 
-	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 != "") {
+	// Only set identity.Groups if 'orgs', 'org', or 'groups' scope are specified.
+	if c.groupsRequired(s.Groups) {
+		groups, err := c.getGroups(ctx, client, s.Groups, user.Login)
+		if err != nil {
+			return identity, err
+		}
 		identity.Groups = groups
 	}
 
@@ -304,26 +297,35 @@ func (c *githubConnector) Refresh(ctx context.Context, s connector.Scopes, ident
 	identity.Username = username
 	identity.Email = user.Email
 
-	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 != "") {
+	// Only set identity.Groups if 'orgs', 'org', or 'groups' scope are specified.
+	if c.groupsRequired(s.Groups) {
+		groups, err := c.getGroups(ctx, client, s.Groups, user.Login)
+		if err != nil {
+			return identity, err
+		}
 		identity.Groups = groups
 	}
 
 	return identity, nil
 }
 
-// listGroups enforces org and team constraints on user authorization
+// 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) ([]string, error) {
+	if len(c.orgs) > 0 {
+		return c.groupsForOrgs(ctx, client, userLogin)
+	} else if c.org != "" {
+		return c.teamsForOrg(ctx, client, c.org)
+	}
+	return nil, nil
+}
+
+// groupsForOrgs enforces org and team constraints on user authorization
 // Cases in which user is authorized:
 // 	N orgs, no teams: user is member of at least 1 org
 // 	N orgs, M teams per org: user is member of any team from at least 1 org
 // 	N-1 orgs, M teams per org, 1 org with no teams: user is member of any team
 // from at least 1 org, or member of org with no teams
-func (c *githubConnector) listGroups(ctx context.Context, client *http.Client, userName string) (groups []string, err error) {
+func (c *githubConnector) groupsForOrgs(ctx context.Context, client *http.Client, userName string) (groups []string, err error) {
 	var inOrgNoTeams bool
 	for _, org := range c.orgs {
 		inOrg, err := c.userInOrg(ctx, client, userName, org.Name)
@@ -334,7 +336,7 @@ func (c *githubConnector) listGroups(ctx context.Context, client *http.Client, u
 			continue
 		}
 
-		teams, err := c.teams(ctx, client, org.Name)
+		teams, err := c.teamsForOrg(ctx, client, org.Name)
 		if err != nil {
 			return groups, err
 		}
@@ -551,11 +553,11 @@ type team struct {
 	} `json:"organization"`
 }
 
-// teams queries the GitHub API for team membership within a specific organization.
+// teamsForOrg queries the GitHub API for team membership within a specific organization.
 //
 // The HTTP passed client is expected to be constructed by the golang.org/x/oauth2 package,
 // which inserts a bearer token as part of the request.
-func (c *githubConnector) teams(ctx context.Context, client *http.Client, orgName string) ([]string, error) {
+func (c *githubConnector) teamsForOrg(ctx context.Context, client *http.Client, orgName string) ([]string, error) {
 	apiURL, groups := c.apiURL+"/user/teams", []string{}
 	for {
 		// https://developer.github.com/v3/orgs/teams/#list-user-teams
@@ -564,7 +566,7 @@ func (c *githubConnector) teams(ctx context.Context, client *http.Client, orgNam
 			err   error
 		)
 		if apiURL, err = get(ctx, client, apiURL, &teams); err != nil {
-			return nil, err
+			return nil, fmt.Errorf("github: get teams: %v", err)
 		}
 
 		for _, team := range teams {