diff --git a/connector/github/github.go b/connector/github/github.go index 462786df..9471f2fa 100644 --- a/connector/github/github.go +++ b/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 {