From e92f38f38f211967ef40d23e03de569b7a30f01e Mon Sep 17 00:00:00 2001 From: Eric Stroczynski Date: Thu, 17 Aug 2017 10:13:00 -0700 Subject: [PATCH 1/2] 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 } From ce9ac761a6fbf56109bcfc3bb79c04575e5502cf Mon Sep 17 00:00:00 2001 From: Eric Stroczynski Date: Fri, 18 Aug 2017 11:04:12 -0700 Subject: [PATCH 2/2] connector/github: abstract scope check and group getter --- connector/github/github.go | 84 +++++++++++++++++++------------------- 1 file changed, 43 insertions(+), 41 deletions(-) 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 {