diff --git a/internal/githubclient/githubclient.go b/internal/githubclient/githubclient.go index 0832e1b83..04e978507 100644 --- a/internal/githubclient/githubclient.go +++ b/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 { diff --git a/internal/githubclient/githubclient_test.go b/internal/githubclient/githubclient_test.go index 7e2c6987c..e9214e7c8 100644 --- a/internal/githubclient/githubclient_test.go +++ b/internal/githubclient/githubclient_test.go @@ -178,7 +178,7 @@ func TestGetUser(t *testing.T) { ), ), token: "does-this-token-work", - wantErr: `the "login" attribute is missing for authenticated user`, + wantErr: `error fetching authenticated user: the "login" attribute is missing`, }, { name: "handles missing ID", @@ -191,7 +191,7 @@ func TestGetUser(t *testing.T) { ), ), token: "does-this-token-work", - wantErr: `the "ID" attribute is missing for authenticated user`, + wantErr: `error fetching authenticated user: the "id" attribute is missing`, }, { name: "passes the context parameter into the API call", @@ -312,6 +312,21 @@ func TestGetOrgMembership(t *testing.T) { token: "does-this-token-work", wantOrgs: []string{"some-org-to-which-the-authenticated-user-belongs"}, }, + { + name: "errors when a Login field is empty", + httpClient: mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetUserOrgs, + []github.Organization{ + {Login: github.String("page1-org1")}, + {Login: nil}, + {Login: github.String("page1-org3")}, + }, + ), + ), + token: "some-token", + wantErr: `error fetching organizations for authenticated user: one or more organizations is missing the "login" attribute`, + }, { name: "passes the context parameter into the API call", token: "some-token", @@ -338,7 +353,7 @@ func TestGetOrgMembership(t *testing.T) { ), ), token: "some-token", - wantErr: "error fetching organizations for authenticated user: GET {SERVER_URL}/user/orgs?per_page=10: 424 some random client error []", + wantErr: "error fetching organizations for authenticated user: GET {SERVER_URL}/user/orgs?per_page=100: 424 some random client error []", }, } for _, test := range tests { @@ -364,7 +379,8 @@ func TestGetOrgMembership(t *testing.T) { } require.NotNil(t, actual) - require.Equal(t, test.wantOrgs, actual) + require.Equal(t, len(actual), len(test.wantOrgs)) + require.True(t, actual.HasAll(test.wantOrgs...)) }) } } @@ -379,7 +395,7 @@ func TestGetTeamMembership(t *testing.T) { ctx context.Context allowedOrganizations []string wantErr string - wantTeams []TeamInfo + wantTeams []*TeamInfo }{ { name: "happy path", @@ -420,7 +436,7 @@ func TestGetTeamMembership(t *testing.T) { ), token: "some-token", allowedOrganizations: []string{"alpha", "beta"}, - wantTeams: []TeamInfo{ + wantTeams: []*TeamInfo{ { Name: "orgAlpha-team1-name", Slug: "orgAlpha-team1-slug", @@ -475,7 +491,7 @@ func TestGetTeamMembership(t *testing.T) { ), token: "some-token", allowedOrganizations: []string{"alpha", "gamma"}, - wantTeams: []TeamInfo{ + wantTeams: []*TeamInfo{ { Name: "team1-name", Slug: "team1-slug", @@ -526,7 +542,7 @@ func TestGetTeamMembership(t *testing.T) { ), ), token: "some-token", - wantTeams: []TeamInfo{ + wantTeams: []*TeamInfo{ { Name: "team1-name", Slug: "team1-slug", @@ -599,7 +615,7 @@ func TestGetTeamMembership(t *testing.T) { "parent-team-org-that-in-reality-can-never-be-different-than-child-team-org", "beta", }, - wantTeams: []TeamInfo{ + wantTeams: []*TeamInfo{ { Name: "team-name-with-parent", Slug: "team-slug-with-parent", @@ -649,7 +665,7 @@ func TestGetTeamMembership(t *testing.T) { ), token: "some-token", allowedOrganizations: []string{"page1-org-name", "page2-org-name"}, - wantTeams: []TeamInfo{ + wantTeams: []*TeamInfo{ { Name: "page1-team-name", Slug: "page1-team-slug", @@ -662,6 +678,164 @@ func TestGetTeamMembership(t *testing.T) { }, }, }, + { + name: "missing organization attribute returns an error", + httpClient: mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetUserTeams, + []github.Team{ + { + Name: github.String("team-name"), + Slug: github.String("team-slug"), + }, + }, + ), + ), + wantErr: `error fetching team membership for authenticated user: missing the "organization" attribute for a team`, + }, + { + name: "missing organization's login attribute returns an error", + httpClient: mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetUserTeams, + []github.Team{ + { + Name: github.String("team-name"), + Slug: github.String("team-slug"), + Organization: &github.Organization{}, + }, + }, + ), + ), + wantErr: `error fetching team membership for authenticated user: missing the organization's "login" attribute for a team`, + }, + { + name: "missing the name attribute for a team returns an error", + httpClient: mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetUserTeams, + []github.Team{ + { + Slug: github.String("team-slug"), + Organization: &github.Organization{ + Login: github.String("some-org"), + }, + }, + }, + ), + ), + wantErr: `error fetching team membership for authenticated user: the "name" attribute is missing for a team`, + }, + { + name: "missing the slug attribute for a team returns an error", + httpClient: mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetUserTeams, + []github.Team{ + { + Name: github.String("team-name"), + Organization: &github.Organization{ + Login: github.String("some-org"), + }, + }, + }, + ), + ), + wantErr: `error fetching team membership for authenticated user: the "slug" attribute is missing for a team`, + }, + { + name: "missing parent's organization attribute returns an error", + httpClient: mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetUserTeams, + []github.Team{ + { + Name: github.String("team-name-with-parent"), + Slug: github.String("team-slug-with-parent"), + Parent: &github.Team{ + Name: github.String("parent-team-name"), + Slug: github.String("parent-team-slug"), + }, + Organization: &github.Organization{ + Login: github.String("some-org"), + }, + }, + }, + ), + ), + wantErr: `error fetching team membership for authenticated user: missing the "organization" attribute for a team`, + }, + { + name: "missing parent's organization's login attribute returns an error", + httpClient: mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetUserTeams, + []github.Team{ + { + Name: github.String("team-name-with-parent"), + Slug: github.String("team-slug-with-parent"), + Parent: &github.Team{ + Name: github.String("parent-team-name"), + Slug: github.String("parent-team-slug"), + Organization: &github.Organization{}, + }, + Organization: &github.Organization{ + Login: github.String("some-org"), + }, + }, + }, + ), + ), + wantErr: `error fetching team membership for authenticated user: missing the organization's "login" attribute for a team`, + }, + { + name: "missing the name attribute for a parent team returns an error", + httpClient: mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetUserTeams, + []github.Team{ + { + Name: github.String("team-name-with-parent"), + Slug: github.String("team-slug-with-parent"), + Parent: &github.Team{ + Slug: github.String("parent-team-slug"), + Organization: &github.Organization{ + Login: github.String("some-org"), + }, + }, + Organization: &github.Organization{ + Login: github.String("some-org"), + }, + }, + }, + ), + ), + wantErr: `error fetching team membership for authenticated user: the "name" attribute is missing for a team`, + }, + { + name: "missing the slug attribute for a parent team returns an error", + httpClient: mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetUserTeams, + []github.Team{ + { + Name: github.String("team-name-with-parent"), + Slug: github.String("team-slug-with-parent"), + Parent: &github.Team{ + Name: github.String("parent-team-name"), + Organization: &github.Organization{ + Login: github.String("some-org"), + }, + }, + Organization: &github.Organization{ + Login: github.String("some-org"), + }, + }, + }, + ), + ), + wantErr: `error fetching team membership for authenticated user: the "slug" attribute is missing for a team`, + }, { name: "the token is added in the Authorization header", httpClient: mock.NewMockedHTTPClient( @@ -677,7 +851,7 @@ func TestGetTeamMembership(t *testing.T) { ), token: "does-this-token-work", allowedOrganizations: []string{"org-login"}, - wantTeams: []TeamInfo{ + wantTeams: []*TeamInfo{ { Name: "team1-name", Slug: "team1-slug", @@ -711,7 +885,7 @@ func TestGetTeamMembership(t *testing.T) { ), ), token: "some-token", - wantErr: "error fetching team membership for authenticated user: GET {SERVER_URL}/user/teams?per_page=10: 424 some random client error []", + wantErr: "error fetching team membership for authenticated user: GET {SERVER_URL}/user/teams?per_page=100: 424 some random client error []", }, } for _, test := range tests {