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 }