Handle empty or invalid github API responses

Co-authored-by: Joshua Casey <joshuatcasey@gmail.com>

internal/githubclient/githubclient.go

@@ -2,6 +2,7 @@ package githubclient
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 
@@ -10,7 +11,10 @@ import (
 	"k8s.io/apimachinery/pkg/util/sets"
 )
 
-const emptyUserMeansTheAuthenticatedUser = ""
+const (
+	emptyUserMeansTheAuthenticatedUser = ""
+	pageSize                           = 100
+)
 
 type UserInfo struct {
 	ID    string
@@ -25,8 +29,8 @@ type TeamInfo struct {
 
 type GitHubInterface interface {
 	GetUserInfo(ctx context.Context) (*UserInfo, error)
-	GetOrgMembership(ctx context.Context) ([]string, error)
-	GetTeamMembership(ctx context.Context, allowedOrganizations sets.Set[string]) ([]TeamInfo, error)
+	GetOrgMembership(ctx context.Context) (sets.Set[string], error)
+	GetTeamMembership(ctx context.Context, allowedOrganizations sets.Set[string]) ([]*TeamInfo, error)
 }
 
 type githubClient struct {
@@ -63,47 +67,46 @@ func NewGitHubClient(httpClient *http.Client, apiBaseURL, token string) (GitHubI
 }
 
 // GetUserInfo returns the "Login" and "ID" attributes of the logged-in user.
-// TODO: should we check ID and Login for nil?
 func (g *githubClient) GetUserInfo(ctx context.Context) (*UserInfo, error) {
-	user, response, err := g.client.Users.Get(ctx, emptyUserMeansTheAuthenticatedUser)
+	const errorPrefix = "error fetching authenticated user"
+
+	user, _, err := g.client.Users.Get(ctx, emptyUserMeansTheAuthenticatedUser)
 	if err != nil {
-		return nil, fmt.Errorf("error fetching authenticated user: %w", err)
+		return nil, fmt.Errorf("%s: %w", errorPrefix, err)
 	}
 	if user == nil { // untested
-		return nil, fmt.Errorf("error fetching authenticated user: user is nil")
-	}
-	if response == nil { // untested
-		return nil, fmt.Errorf("error fetching authenticated user: response is nil")
-	}
-	if user.ID == nil {
-		return nil, fmt.Errorf(`the "ID" attribute is missing for authenticated user`)
-	}
-	if user.Login == nil {
-		return nil, fmt.Errorf(`the "login" attribute is missing for authenticated user`)
+		return nil, fmt.Errorf("%s: user is nil", errorPrefix)
 	}
 
-	return &UserInfo{
+	userInfo := &UserInfo{
 		Login: user.GetLogin(),
 		ID:    fmt.Sprintf("%d", user.GetID()),
-	}, nil
+	}
+	if userInfo.ID == "0" {
+		return nil, fmt.Errorf(`%s: the "id" attribute is missing`, errorPrefix)
+	}
+	if userInfo.Login == "" {
+		return nil, fmt.Errorf(`%s: the "login" attribute is missing`, errorPrefix)
+	}
+	return userInfo, nil
 }
 
 // GetOrgMembership returns an array of the "Login" attributes for all organizations to which the authenticated user belongs.
-// TODO: what happens if login is nil?
-// TODO: what is a good page size?
-func (g *githubClient) GetOrgMembership(ctx context.Context) ([]string, error) {
-	organizationLoginStrings := make([]string, 0)
+func (g *githubClient) GetOrgMembership(ctx context.Context) (sets.Set[string], error) {
+	const errorPrefix = "error fetching organizations for authenticated user"
 
-	opt := &github.ListOptions{PerPage: 10}
+	organizationLogins := sets.New[string]()
+
+	opt := &github.ListOptions{PerPage: pageSize}
 	// get all pages of results
 	for {
 		organizationResults, response, err := g.client.Organizations.List(ctx, emptyUserMeansTheAuthenticatedUser, opt)
 		if err != nil {
-			return nil, fmt.Errorf("error fetching organizations for authenticated user: %w", err)
+			return nil, fmt.Errorf("%s: %w", errorPrefix, err)
 		}
 
 		for _, organization := range organizationResults {
-			organizationLoginStrings = append(organizationLoginStrings, organization.GetLogin())
+			organizationLogins.Insert(organization.GetLogin())
 		}
 		if response.NextPage == 0 {
 			break
@@ -111,54 +114,79 @@ func (g *githubClient) GetOrgMembership(ctx context.Context) ([]string, error) {
 		opt.Page = response.NextPage
 	}
 
-	return organizationLoginStrings, nil
+	if organizationLogins.Has("") {
+		return nil, fmt.Errorf(`%s: one or more organizations is missing the "login" attribute`, errorPrefix)
+	}
+
+	return organizationLogins, nil
 }
 
 func isOrgAllowed(allowedOrganizations sets.Set[string], login string) bool {
 	return len(allowedOrganizations) == 0 || allowedOrganizations.Has(login)
 }
 
+func buildAndValidateTeam(githubTeam *github.Team) (*TeamInfo, error) {
+	if githubTeam.GetOrganization() == nil {
+		return nil, errors.New(`missing the "organization" attribute for a team`)
+	}
+	organizationLogin := githubTeam.GetOrganization().GetLogin()
+	if organizationLogin == "" {
+		return nil, errors.New(`missing the organization's "login" attribute for a team`)
+	}
+
+	teamInfo := &TeamInfo{
+		Name: githubTeam.GetName(),
+		Slug: githubTeam.GetSlug(),
+		Org:  organizationLogin,
+	}
+	if teamInfo.Name == "" {
+		return nil, errors.New(`the "name" attribute is missing for a team`)
+	}
+	if teamInfo.Slug == "" {
+		return nil, errors.New(`the "slug" attribute is missing for a team`)
+	}
+	return teamInfo, nil
+}
+
 // GetTeamMembership returns a description of each team to which the authenticated user belongs.
 // If allowedOrganizations is not empty, will filter the results to only those teams which belong to the allowed organizations.
 // Parent teams will also be returned.
-// TODO: what happens if org or login or id are nil?
-// TODO: what is a good page size?
-func (g *githubClient) GetTeamMembership(ctx context.Context, allowedOrganizations sets.Set[string]) ([]TeamInfo, error) {
-	teamInfos := make([]TeamInfo, 0)
+func (g *githubClient) GetTeamMembership(ctx context.Context, allowedOrganizations sets.Set[string]) ([]*TeamInfo, error) {
+	const errorPrefix = "error fetching team membership for authenticated user"
+	teamInfos := make([]*TeamInfo, 0)
 
-	opt := &github.ListOptions{PerPage: 10}
+	opt := &github.ListOptions{PerPage: pageSize}
 	// get all pages of results
 	for {
 		teamsResults, response, err := g.client.Teams.ListUserTeams(ctx, opt)
 		if err != nil {
-			return nil, fmt.Errorf("error fetching team membership for authenticated user: %w", err)
+			return nil, fmt.Errorf("%s: %w", errorPrefix, err)
 		}
 
 		for _, team := range teamsResults {
-			org := team.GetOrganization().GetLogin()
+			teamInfo, err := buildAndValidateTeam(team)
+			if err != nil {
+				return nil, fmt.Errorf("%s: %w", errorPrefix, err)
+			}
 
-			if !isOrgAllowed(allowedOrganizations, org) {
+			if !isOrgAllowed(allowedOrganizations, teamInfo.Org) {
 				continue
 			}
 
-			teamInfos = append(teamInfos, TeamInfo{
-				Name: team.GetName(),
-				Slug: team.GetSlug(),
-				Org:  org,
-			})
+			teamInfos = append(teamInfos, teamInfo)
 
 			parent := team.GetParent()
 			if parent != nil {
-				parentOrg := parent.GetOrganization().GetLogin()
-				if !isOrgAllowed(allowedOrganizations, parentOrg) {
+				teamInfo, err := buildAndValidateTeam(parent)
+				if err != nil {
+					return nil, fmt.Errorf("%s: %w", errorPrefix, err)
+				}
+
+				if !isOrgAllowed(allowedOrganizations, teamInfo.Org) {
 					continue
 				}
 
-				teamInfos = append(teamInfos, TeamInfo{
-					Name: parent.GetName(),
-					Slug: parent.GetSlug(),
-					Org:  parentOrg,
-				})
+				teamInfos = append(teamInfos, teamInfo)
 			}
 		}
 		if response.NextPage == 0 {