fix: Use membership endpoint to ensure user exists in team (coder#3129)

kylecarbs · web-flow · commit fd4954b4e5ab · 2022-07-22T13:54:08.000-05:00
This was using the incorrect GitHub endpoint prior, which fetched a team by slug. Any user in a GitHub organization can view all teams, so this didn't block signups like intended. I've verified this API returns an error when the calling user is not a member of the team requested. Fixes coder#3105.
diff --git a/cli/server.go b/cli/server.go
@@ -796,8 +796,8 @@ func configureGithubOAuth2(accessURL *url.URL, clientID, clientSecret string, al
 			})
 			return memberships, err
 		},
-		Team: func(ctx context.Context, client *http.Client, org, teamSlug string) (*github.Team, error) {
-			team, _, err := github.NewClient(client).Teams.GetTeamBySlug(ctx, org, teamSlug)
+		TeamMembership: func(ctx context.Context, client *http.Client, org, teamSlug, username string) (*github.Membership, error) {
+			team, _, err := github.NewClient(client).Teams.GetTeamMembershipBySlug(ctx, org, teamSlug, username)
 			return team, err
 		},
 	}, nil
diff --git a/coderd/userauth.go b/coderd/userauth.go
@@ -29,7 +29,7 @@ type GithubOAuth2Config struct {
 	AuthenticatedUser           func(ctx context.Context, client *http.Client) (*github.User, error)
 	ListEmails                  func(ctx context.Context, client *http.Client) ([]*github.UserEmail, error)
 	ListOrganizationMemberships func(ctx context.Context, client *http.Client) ([]*github.Membership, error)
-	Team                        func(ctx context.Context, client *http.Client, org, team string) (*github.Team, error)
+	TeamMembership              func(ctx context.Context, client *http.Client, org, team, username string) (*github.Membership, error)
 
 	AllowSignups       bool
 	AllowOrganizations []string
@@ -72,17 +72,26 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	ghUser, err := api.GithubOAuth2Config.AuthenticatedUser(r.Context(), oauthClient)
+	if err != nil {
+		httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{
+			Message: "Internal error fetching authenticated Github user.",
+			Detail:  err.Error(),
+		})
+		return
+	}
+
 	// The default if no teams are specified is to allow all.
 	if len(api.GithubOAuth2Config.AllowTeams) > 0 {
-		var allowedTeam *github.Team
+		var allowedTeam *github.Membership
 		for _, allowTeam := range api.GithubOAuth2Config.AllowTeams {
 			if allowTeam.Organization != *selectedMembership.Organization.Login {
 				// This needs to continue because multiple organizations
 				// could exist in the allow/team listings.
 				continue
 			}
 
-			allowedTeam, err = api.GithubOAuth2Config.Team(r.Context(), oauthClient, allowTeam.Organization, allowTeam.Slug)
+			allowedTeam, err = api.GithubOAuth2Config.TeamMembership(r.Context(), oauthClient, allowTeam.Organization, allowTeam.Slug, *ghUser.Login)
 			// The calling user may not have permission to the requested team!
 			if err != nil {
 				continue
@@ -151,14 +160,6 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
 			// email to organization.
 			organizationID = organizations[0].ID
 		}
-		ghUser, err := api.GithubOAuth2Config.AuthenticatedUser(r.Context(), oauthClient)
-		if err != nil {
-			httpapi.Write(rw, http.StatusInternalServerError, codersdk.Response{
-				Message: "Internal error fetching authenticated Github user.",
-				Detail:  err.Error(),
-			})
-			return
-		}
 		var verifiedEmail *github.UserEmail
 		for _, email := range emails {
 			if !email.GetPrimary() || !email.GetVerified() {
diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go
@@ -88,7 +88,12 @@ func TestUserOAuth2Github(t *testing.T) {
 						},
 					}}, nil
 				},
-				Team: func(ctx context.Context, client *http.Client, org, team string) (*github.Team, error) {
+				AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
+					return &github.User{
+						Login: github.String("kyle"),
+					}, nil
+				},
+				TeamMembership: func(ctx context.Context, client *http.Client, org, team, username string) (*github.Membership, error) {
 					return nil, xerrors.New("no perms")
 				},
 			},
@@ -222,8 +227,8 @@ func TestUserOAuth2Github(t *testing.T) {
 						},
 					}}, nil
 				},
-				Team: func(ctx context.Context, client *http.Client, org, team string) (*github.Team, error) {
-					return &github.Team{}, nil
+				TeamMembership: func(ctx context.Context, client *http.Client, org, team, username string) (*github.Membership, error) {
+					return &github.Membership{}, nil
 				},
 				AuthenticatedUser: func(ctx context.Context, client *http.Client) (*github.User, error) {
 					return &github.User{
