mirror of
https://github.com/coder/coder.git
synced 2025-07-09 11:45:56 +00:00
fix: Fetch GitHub teams by name for performance (#2955)
In large organizations with thousands of teams, looping took >5s. This fetches organizations by team name, which should be very fast!
This commit is contained in:
@ -770,24 +770,9 @@ func configureGithubOAuth2(accessURL *url.URL, clientID, clientSecret string, al
|
|||||||
})
|
})
|
||||||
return memberships, err
|
return memberships, err
|
||||||
},
|
},
|
||||||
ListTeams: func(ctx context.Context, client *http.Client, org string) ([]*github.Team, error) {
|
Team: func(ctx context.Context, client *http.Client, org, teamSlug string) (*github.Team, error) {
|
||||||
opt := &github.ListOptions{
|
team, _, err := github.NewClient(client).Teams.GetTeamBySlug(ctx, org, teamSlug)
|
||||||
// This is the maximum amount per-page that GitHub allows.
|
return team, err
|
||||||
PerPage: 100,
|
|
||||||
}
|
|
||||||
var allTeams []*github.Team
|
|
||||||
for {
|
|
||||||
teams, resp, err := github.NewClient(client).Teams.ListTeams(ctx, org, opt)
|
|
||||||
if err != nil {
|
|
||||||
return nil, err
|
|
||||||
}
|
|
||||||
allTeams = append(allTeams, teams...)
|
|
||||||
if resp.NextPage == 0 {
|
|
||||||
break
|
|
||||||
}
|
|
||||||
opt.Page = resp.NextPage
|
|
||||||
}
|
|
||||||
return allTeams, nil
|
|
||||||
},
|
},
|
||||||
}, nil
|
}, nil
|
||||||
}
|
}
|
||||||
|
@ -29,7 +29,7 @@ type GithubOAuth2Config struct {
|
|||||||
AuthenticatedUser func(ctx context.Context, client *http.Client) (*github.User, error)
|
AuthenticatedUser func(ctx context.Context, client *http.Client) (*github.User, error)
|
||||||
ListEmails func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error)
|
ListEmails func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error)
|
||||||
ListOrganizationMemberships func(ctx context.Context, client *http.Client) ([]*github.Membership, error)
|
ListOrganizationMemberships func(ctx context.Context, client *http.Client) ([]*github.Membership, error)
|
||||||
ListTeams func(ctx context.Context, client *http.Client, org string) ([]*github.Team, error)
|
Team func(ctx context.Context, client *http.Client, org, team string) (*github.Team, error)
|
||||||
|
|
||||||
AllowSignups bool
|
AllowSignups bool
|
||||||
AllowOrganizations []string
|
AllowOrganizations []string
|
||||||
@ -74,31 +74,20 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
|
|||||||
|
|
||||||
// The default if no teams are specified is to allow all.
|
// The default if no teams are specified is to allow all.
|
||||||
if len(api.GithubOAuth2Config.AllowTeams) > 0 {
|
if len(api.GithubOAuth2Config.AllowTeams) > 0 {
|
||||||
teams, err := api.GithubOAuth2Config.ListTeams(r.Context(), oauthClient, *selectedMembership.Organization.Login)
|
|
||||||
if err != nil {
|
|
||||||
httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{
|
|
||||||
Message: "Failed to fetch teams from GitHub.",
|
|
||||||
Detail: err.Error(),
|
|
||||||
})
|
|
||||||
return
|
|
||||||
}
|
|
||||||
|
|
||||||
var allowedTeam *github.Team
|
var allowedTeam *github.Team
|
||||||
for _, team := range teams {
|
for _, allowTeam := range api.GithubOAuth2Config.AllowTeams {
|
||||||
for _, allowTeam := range api.GithubOAuth2Config.AllowTeams {
|
if allowTeam.Organization != *selectedMembership.Organization.Login {
|
||||||
if allowTeam.Organization != *selectedMembership.Organization.Login {
|
// This needs to continue because multiple organizations
|
||||||
// This needs to continue because multiple organizations
|
// could exist in the allow/team listings.
|
||||||
// could exist in the allow/team listings.
|
continue
|
||||||
continue
|
}
|
||||||
}
|
|
||||||
if allowTeam.Slug != *team.Slug {
|
allowedTeam, err = api.GithubOAuth2Config.Team(r.Context(), oauthClient, allowTeam.Organization, allowTeam.Slug)
|
||||||
continue
|
// The calling user may not have permission to the requested team!
|
||||||
}
|
if err != nil {
|
||||||
allowedTeam = team
|
continue
|
||||||
break
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
if allowedTeam == nil {
|
if allowedTeam == nil {
|
||||||
httpapi.Write(rw, http.StatusUnauthorized, codersdk.Response{
|
httpapi.Write(rw, http.StatusUnauthorized, codersdk.Response{
|
||||||
Message: fmt.Sprintf("You aren't a member of an authorized team in the %s Github organization!", *selectedMembership.Organization.Login),
|
Message: fmt.Sprintf("You aren't a member of an authorized team in the %s Github organization!", *selectedMembership.Organization.Login),
|
||||||
|
@ -9,6 +9,7 @@ import (
|
|||||||
"github.com/google/go-github/v43/github"
|
"github.com/google/go-github/v43/github"
|
||||||
"github.com/stretchr/testify/require"
|
"github.com/stretchr/testify/require"
|
||||||
"golang.org/x/oauth2"
|
"golang.org/x/oauth2"
|
||||||
|
"golang.org/x/xerrors"
|
||||||
|
|
||||||
"github.com/coder/coder/coderd"
|
"github.com/coder/coder/coderd"
|
||||||
"github.com/coder/coder/coderd/coderdtest"
|
"github.com/coder/coder/coderd/coderdtest"
|
||||||
@ -87,10 +88,8 @@ func TestUserOAuth2Github(t *testing.T) {
|
|||||||
},
|
},
|
||||||
}}, nil
|
}}, nil
|
||||||
},
|
},
|
||||||
ListTeams: func(ctx context.Context, client *http.Client, org string) ([]*github.Team, error) {
|
Team: func(ctx context.Context, client *http.Client, org, team string) (*github.Team, error) {
|
||||||
return []*github.Team{{
|
return nil, xerrors.New("no perms")
|
||||||
Slug: github.String("nope"),
|
|
||||||
}}, nil
|
|
||||||
},
|
},
|
||||||
},
|
},
|
||||||
})
|
})
|
||||||
@ -223,10 +222,8 @@ func TestUserOAuth2Github(t *testing.T) {
|
|||||||
},
|
},
|
||||||
}}, nil
|
}}, nil
|
||||||
},
|
},
|
||||||
ListTeams: func(ctx context.Context, client *http.Client, org string) ([]*github.Team, error) {
|
Team: func(ctx context.Context, client *http.Client, org, team string) (*github.Team, error) {
|
||||||
return []*github.Team{{
|
return &github.Team{}, nil
|
||||||
Slug: github.String("frontend"),
|
|
||||||
}}, nil
|
|
||||||
},
|
},
|
||||||
AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
|
AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
|
||||||
return &github.User{
|
return &github.User{
|
||||||
|
6
go.sum
6
go.sum
@ -1880,8 +1880,6 @@ github.com/zclconf/go-cty v1.10.0 h1:mp9ZXQeIcN8kAwuqorjH+Q+njbJKjLrvB2yIh4q7U+0
|
|||||||
github.com/zclconf/go-cty v1.10.0/go.mod h1:vVKLxnk3puL4qRAv72AO+W99LUD4da90g3uUAzyuvAk=
|
github.com/zclconf/go-cty v1.10.0/go.mod h1:vVKLxnk3puL4qRAv72AO+W99LUD4da90g3uUAzyuvAk=
|
||||||
github.com/zclconf/go-cty-debug v0.0.0-20191215020915-b22d67c1ba0b/go.mod h1:ZRKQfBXbGkpdV6QMzT3rU1kSTAnfu1dO8dPKjYprgj8=
|
github.com/zclconf/go-cty-debug v0.0.0-20191215020915-b22d67c1ba0b/go.mod h1:ZRKQfBXbGkpdV6QMzT3rU1kSTAnfu1dO8dPKjYprgj8=
|
||||||
github.com/zeebo/assert v1.3.0 h1:g7C04CbJuIDKNPFHmsk4hwZDO5O+kntRxzaUoNXj+IQ=
|
github.com/zeebo/assert v1.3.0 h1:g7C04CbJuIDKNPFHmsk4hwZDO5O+kntRxzaUoNXj+IQ=
|
||||||
github.com/zeebo/errs v1.2.2 h1:5NFypMTuSdoySVTqlNs1dEoU21QVamMQJxW/Fii5O7g=
|
|
||||||
github.com/zeebo/errs v1.2.2/go.mod h1:sgbWHsvVuTPHcqJJGQ1WhI5KbWlHYz+2+2C/LSEtCw4=
|
|
||||||
github.com/zeebo/errs v1.3.0 h1:hmiaKqgYZzcVgRL1Vkc1Mn2914BbzB0IBxs+ebeutGs=
|
github.com/zeebo/errs v1.3.0 h1:hmiaKqgYZzcVgRL1Vkc1Mn2914BbzB0IBxs+ebeutGs=
|
||||||
github.com/zeebo/errs v1.3.0/go.mod h1:sgbWHsvVuTPHcqJJGQ1WhI5KbWlHYz+2+2C/LSEtCw4=
|
github.com/zeebo/errs v1.3.0/go.mod h1:sgbWHsvVuTPHcqJJGQ1WhI5KbWlHYz+2+2C/LSEtCw4=
|
||||||
github.com/zenazn/goji v0.9.0/go.mod h1:7S9M489iMyHBNxwZnk9/EHS098H4/F6TATF2mIxtB1Q=
|
github.com/zenazn/goji v0.9.0/go.mod h1:7S9M489iMyHBNxwZnk9/EHS098H4/F6TATF2mIxtB1Q=
|
||||||
@ -2912,10 +2910,6 @@ sigs.k8s.io/structured-merge-diff/v4 v4.1.2/go.mod h1:j/nl6xW8vLS49O8YvXW1ocPhZa
|
|||||||
sigs.k8s.io/yaml v1.1.0/go.mod h1:UJmg0vDUVViEyp3mgSv9WPwZCDxu4rQW1olrI1uml+o=
|
sigs.k8s.io/yaml v1.1.0/go.mod h1:UJmg0vDUVViEyp3mgSv9WPwZCDxu4rQW1olrI1uml+o=
|
||||||
sigs.k8s.io/yaml v1.2.0/go.mod h1:yfXDCHCao9+ENCvLSE62v9VSji2MKu5jeNfTrofGhJc=
|
sigs.k8s.io/yaml v1.2.0/go.mod h1:yfXDCHCao9+ENCvLSE62v9VSji2MKu5jeNfTrofGhJc=
|
||||||
software.sslmate.com/src/go-pkcs12 v0.0.0-20210415151418-c5206de65a78 h1:SqYE5+A2qvRhErbsXFfUEUmpWEKxxRSMgGLkvRAFOV4=
|
software.sslmate.com/src/go-pkcs12 v0.0.0-20210415151418-c5206de65a78 h1:SqYE5+A2qvRhErbsXFfUEUmpWEKxxRSMgGLkvRAFOV4=
|
||||||
storj.io/drpc v0.0.30 h1:jqPe4T9KEu3CDBI05A2hCMgMSHLtd/E0N0yTF9QreIE=
|
|
||||||
storj.io/drpc v0.0.30/go.mod h1:6rcOyR/QQkSTX/9L5ZGtlZaE2PtXTTZl8d+ulSeeYEg=
|
|
||||||
storj.io/drpc v0.0.32 h1:5p5ZwsK/VOgapaCu+oxaPVwO6UwIs+iwdMiD50+R4PI=
|
|
||||||
storj.io/drpc v0.0.32/go.mod h1:6rcOyR/QQkSTX/9L5ZGtlZaE2PtXTTZl8d+ulSeeYEg=
|
|
||||||
storj.io/drpc v0.0.33-0.20220622181519-9206537a4db7 h1:6jIp39oQGZMjfrG3kiafK2tcL0Fbprh2kvaoJNfhvuM=
|
storj.io/drpc v0.0.33-0.20220622181519-9206537a4db7 h1:6jIp39oQGZMjfrG3kiafK2tcL0Fbprh2kvaoJNfhvuM=
|
||||||
storj.io/drpc v0.0.33-0.20220622181519-9206537a4db7/go.mod h1:6rcOyR/QQkSTX/9L5ZGtlZaE2PtXTTZl8d+ulSeeYEg=
|
storj.io/drpc v0.0.33-0.20220622181519-9206537a4db7/go.mod h1:6rcOyR/QQkSTX/9L5ZGtlZaE2PtXTTZl8d+ulSeeYEg=
|
||||||
tailscale.com v1.26.2 h1:EBR0DXblI2Rx3mPe/YU29oZbQLnC8BtJYUTufmEygUY=
|
tailscale.com v1.26.2 h1:EBR0DXblI2Rx3mPe/YU29oZbQLnC8BtJYUTufmEygUY=
|
||||||
|
Reference in New Issue
Block a user