Empty allowedOrganizations will return all teams

Co-authored-by: Ryan Richard <richardry@vmware.com>

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,
 				})
 			}
 		}

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)