From a12a5f387aeb304dfffd6171bef89b4276e61e5d Mon Sep 17 00:00:00 2001 From: Joshua Casey Date: Fri, 17 May 2024 13:24:37 -0500 Subject: [PATCH] Empty allowedOrganizations will return all teams Co-authored-by: Ryan Richard --- internal/githubclient/githubclient.go | 31 +++++-- internal/githubclient/githubclient_test.go | 97 +++++++++++++++++++++- 2 files changed, 115 insertions(+), 13 deletions(-) diff --git a/internal/githubclient/githubclient.go b/internal/githubclient/githubclient.go index 998b7d16b..62d1b00a9 100644 --- a/internal/githubclient/githubclient.go +++ b/internal/githubclient/githubclient.go @@ -4,9 +4,10 @@ import ( "context" "fmt" "net/http" - "slices" "github.com/google/go-github/v62/github" + + "k8s.io/apimachinery/pkg/util/sets" ) const emptyUserMeansTheAuthenticatedUser = "" @@ -25,7 +26,7 @@ type TeamInfo struct { type GitHubInterface interface { GetUserInfo() (*UserInfo, error) GetOrgMembership() ([]string, error) - GetTeamMembership(allowedOrganizations []string) ([]TeamInfo, error) + GetTeamMembership(allowedOrganizations sets.Set[string]) ([]TeamInfo, error) } type githubClient struct { @@ -90,8 +91,9 @@ func (g *githubClient) GetUserInfo() (*UserInfo, error) { // GetOrgMembership returns an array of the "Login" attributes for all organizations to which the authenticated user belongs. // TODO: where should context come from? // TODO: what happens if login is nil? +// TODO: what is a good page size? func (g *githubClient) GetOrgMembership() ([]string, error) { - organizationsAsStrings := make([]string, 0) + organizationLoginStrings := make([]string, 0) opt := &github.ListOptions{PerPage: 10} // get all pages of results @@ -102,7 +104,7 @@ func (g *githubClient) GetOrgMembership() ([]string, error) { } for _, organization := range organizationResults { - organizationsAsStrings = append(organizationsAsStrings, organization.GetLogin()) + organizationLoginStrings = append(organizationLoginStrings, organization.GetLogin()) } if response.NextPage == 0 { break @@ -110,14 +112,20 @@ func (g *githubClient) GetOrgMembership() ([]string, error) { opt.Page = response.NextPage } - return organizationsAsStrings, nil + return organizationLoginStrings, nil } -// GetTeamMembership returns a description of each team to which the authenticated user belongs, filtered by allowedOrganizations. +func isOrgAllowed(allowedOrganizations sets.Set[string], login string) bool { + return len(allowedOrganizations) == 0 || allowedOrganizations.Has(login) +} + +// 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: where should context come from? // TODO: what happens if org or login or id are nil? -func (g *githubClient) GetTeamMembership(allowedOrganizations []string) ([]TeamInfo, error) { +// TODO: what is a good page size? +func (g *githubClient) GetTeamMembership(allowedOrganizations sets.Set[string]) ([]TeamInfo, error) { teamInfos := make([]TeamInfo, 0) opt := &github.ListOptions{PerPage: 10} @@ -131,7 +139,7 @@ func (g *githubClient) GetTeamMembership(allowedOrganizations []string) ([]TeamI for _, team := range teamsResults { org := team.GetOrganization().GetLogin() - if !slices.Contains(allowedOrganizations, org) { + if !isOrgAllowed(allowedOrganizations, org) { continue } @@ -143,10 +151,15 @@ func (g *githubClient) GetTeamMembership(allowedOrganizations []string) ([]TeamI parent := team.GetParent() if parent != nil { + parentOrg := parent.GetOrganization().GetLogin() + if !isOrgAllowed(allowedOrganizations, parentOrg) { + continue + } + teamInfos = append(teamInfos, TeamInfo{ Name: parent.GetName(), Slug: parent.GetSlug(), - Org: org, + Org: parentOrg, }) } } diff --git a/internal/githubclient/githubclient_test.go b/internal/githubclient/githubclient_test.go index 7e0a0af26..716e3340d 100644 --- a/internal/githubclient/githubclient_test.go +++ b/internal/githubclient/githubclient_test.go @@ -8,6 +8,7 @@ import ( "github.com/google/go-github/v62/github" "github.com/migueleliasweb/go-github-mock/src/mock" "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/util/cert" "go.pinniped.dev/internal/net/phttp" @@ -151,6 +152,7 @@ func TestGetUser(t *testing.T) { mock.WithRequestMatchHandler( mock.GetUser, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Len(t, r.Header["Authorization"], 1) require.Equal(t, "Bearer does-this-token-work", r.Header.Get("Authorization")) _, err := w.Write([]byte(`{"login":"some-authenticated-username","id":999888}`)) require.NoError(t, err) @@ -280,6 +282,7 @@ func TestGetOrgMembership(t *testing.T) { mock.WithRequestMatchHandler( mock.GetUserOrgs, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Len(t, r.Header["Authorization"], 1) require.Equal(t, "Bearer does-this-token-work", r.Header.Get("Authorization")) _, err := w.Write([]byte(`[{"login":"some-org-to-which-the-authenticated-user-belongs"}]`)) require.NoError(t, err) @@ -447,6 +450,67 @@ func TestGetTeamMembership(t *testing.T) { }, }, }, + { + name: "when allowedOrganizations is empty, return all teams", + httpClient: mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetUserTeams, + []github.Team{ + { + Name: github.String("team1-name"), + Slug: github.String("team1-slug"), + Organization: &github.Organization{ + Login: github.String("alpha"), + }, + }, + { + Name: github.String("team2-name"), + Slug: github.String("team2-slug"), + Organization: &github.Organization{ + Login: github.String("beta"), + }, + }, + { + Name: github.String("team3-name"), + Slug: github.String("team3-slug"), + Parent: &github.Team{ + Name: github.String("delta-team-name"), + Slug: github.String("delta-team-slug"), + Organization: &github.Organization{ + Login: github.String("delta"), + }, + }, + Organization: &github.Organization{ + Login: github.String("gamma"), + }, + }, + }, + ), + ), + token: "some-token", + wantTeams: []TeamInfo{ + { + Name: "team1-name", + Slug: "team1-slug", + Org: "alpha", + }, + { + Name: "team2-name", + Slug: "team2-slug", + Org: "beta", + }, + { + Name: "team3-name", + Slug: "team3-slug", + Org: "gamma", + }, + { + Name: "delta-team-name", + Slug: "delta-team-slug", + Org: "delta", + }, + }, + }, { name: "includes parent team if present", httpClient: mock.NewMockedHTTPClient( @@ -474,11 +538,29 @@ func TestGetTeamMembership(t *testing.T) { Login: github.String("beta"), }, }, + { + Name: github.String("team-name-with-parent-in-disallowed-org"), + Slug: github.String("team-slug-with-parent-in-disallowed-org"), + Parent: &github.Team{ + Name: github.String("disallowed-parent-team-name"), + Slug: github.String("disallowed-parent-team-slug"), + Organization: &github.Organization{ + Login: github.String("disallowed-org"), + }, + }, + Organization: &github.Organization{ + Login: github.String("beta"), + }, + }, }, ), ), - token: "some-token", - allowedOrganizations: []string{"org-with-nested-teams", "beta"}, + token: "some-token", + allowedOrganizations: []string{ + "org-with-nested-teams", + "parent-team-org-that-in-reality-can-never-be-different-than-child-team-org", + "beta", + }, wantTeams: []TeamInfo{ { Name: "team-name-with-parent", @@ -488,13 +570,18 @@ func TestGetTeamMembership(t *testing.T) { { Name: "parent-team-name", Slug: "parent-team-slug", - Org: "org-with-nested-teams", + Org: "parent-team-org-that-in-reality-can-never-be-different-than-child-team-org", }, { Name: "team-name-without-parent", Slug: "team-slug-without-parent", Org: "beta", }, + { + Name: "team-name-with-parent-in-disallowed-org", + Slug: "team-slug-with-parent-in-disallowed-org", + Org: "beta", + }, }, }, { @@ -543,6 +630,7 @@ func TestGetTeamMembership(t *testing.T) { mock.WithRequestMatchHandler( mock.GetUserTeams, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Len(t, r.Header["Authorization"], 1) require.Equal(t, "Bearer does-this-token-work", r.Header.Get("Authorization")) _, err := w.Write([]byte(`[{"name":"team1-name","slug":"team1-slug","organization":{"login":"org-login"}}]`)) require.NoError(t, err) @@ -584,7 +672,8 @@ func TestGetTeamMembership(t *testing.T) { githubClient := &githubClient{ client: github.NewClient(test.httpClient).WithAuthToken(test.token), } - actual, err := githubClient.GetTeamMembership(test.allowedOrganizations) + + actual, err := githubClient.GetTeamMembership(sets.New[string](test.allowedOrganizations...)) if test.wantErr != "" { rt, ok := test.httpClient.Transport.(*mock.EnforceHostRoundTripper) require.True(t, ok)