From 8923704f3c00396929ed422b266e09ff3ebb2676 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Mon, 20 May 2024 16:36:31 -0700 Subject: [PATCH] Finish initial github login flow Also: - fix github teams query: fix bug and sort/unique the results - add IDP display name to github downstream subject - fix error types returned by LoginFromCallback - add trace logs to github API results - update e2e test - implement placeholder version of refresh for github --- .../downstreamsubject/downstream_subject.go | 3 +- .../resolved_github_provider.go | 27 +- .../resolved_github_provider_test.go | 37 +-- .../upstreamprovider/upsteam_provider.go | 3 +- internal/githubclient/githubclient.go | 49 +++- internal/githubclient/githubclient_test.go | 248 ++++++------------ .../mockgithubclient/mockgithubclient.go | 4 +- .../oidctestutil/testgithubprovider.go | 12 +- internal/upstreamgithub/upstreamgithub.go | 29 +- .../upstreamgithub/upstreamgithub_test.go | 177 ++++++++++++- test/integration/e2e_test.go | 44 ++-- test/testlib/browsertest/browsertest.go | 55 +++- test/testlib/env.go | 30 ++- test/testlib/skip.go | 10 +- 14 files changed, 453 insertions(+), 275 deletions(-) diff --git a/internal/federationdomain/downstreamsubject/downstream_subject.go b/internal/federationdomain/downstreamsubject/downstream_subject.go index d4c206ce7..345ec737e 100644 --- a/internal/federationdomain/downstreamsubject/downstream_subject.go +++ b/internal/federationdomain/downstreamsubject/downstream_subject.go @@ -28,6 +28,7 @@ func OIDC(upstreamIssuerAsString string, upstreamSubject string, idpDisplayName func GitHub(apiBaseURL, idpDisplayName, login, id string) string { return fmt.Sprintf("%s?%s=%s&login=%s&id=%s", apiBaseURL, oidc.IDTokenSubClaimIDPNameQueryParam, url.QueryEscape(idpDisplayName), - url.QueryEscape(login), url.QueryEscape(id), + url.QueryEscape(login), + url.QueryEscape(id), ) } diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go index c9de9036e..e0bec1ed1 100644 --- a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go +++ b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go @@ -7,13 +7,16 @@ import ( "context" "errors" "fmt" + "net/http" "golang.org/x/oauth2" "go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1" "go.pinniped.dev/internal/federationdomain/resolvedprovider" "go.pinniped.dev/internal/federationdomain/upstreamprovider" + "go.pinniped.dev/internal/httputil/httperr" "go.pinniped.dev/internal/idtransform" + "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/psession" "go.pinniped.dev/pkg/oidcclient/nonce" "go.pinniped.dev/pkg/oidcclient/pkce" @@ -99,12 +102,19 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) LoginFromCallback( ) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) { accessToken, err := p.Provider.ExchangeAuthcode(ctx, authCode, redirectURI) if err != nil { - return nil, nil, fmt.Errorf("failed to exchange auth code using GitHub API: %w", err) + plog.WarningErr("error exchanging GitHub authcode", err, "upstreamName", p.Provider.GetName()) + return nil, nil, httperr.Wrap(http.StatusBadGateway, + fmt.Sprintf("failed to exchange authcode using GitHub API: %s", err.Error()), + err, + ) } - user, err := p.Provider.GetUser(ctx, accessToken) + user, err := p.Provider.GetUser(ctx, accessToken, p.GetDisplayName()) if err != nil { - return nil, nil, fmt.Errorf("failed to get user info from GitHub API: %w", err) + return nil, nil, httperr.Wrap(http.StatusUnprocessableEntity, + fmt.Sprintf("failed to get user info from GitHub API: %s", err.Error()), + err, + ) } return &resolvedprovider.Identity{ @@ -124,7 +134,12 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) LoginFromCallback( func (p *FederationDomainResolvedGitHubIdentityProvider) UpstreamRefresh( _ context.Context, - _ *resolvedprovider.Identity, -) (refreshedIdentity *resolvedprovider.RefreshedIdentity, err error) { - return nil, errors.New("function UpstreamRefresh not yet implemented for GitHub IDP") + identity *resolvedprovider.Identity, +) (*resolvedprovider.RefreshedIdentity, error) { + // TODO: actually implement refresh. this is just a placeholder that will make refresh always succeed. + return &resolvedprovider.RefreshedIdentity{ + UpstreamUsername: identity.UpstreamUsername, + UpstreamGroups: identity.UpstreamGroups, + IDPSpecificSessionData: nil, // nil means that no update to the GitHub-specific portion of the session data is required + }, nil } diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go index 3f957ce37..7941c104f 100644 --- a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go +++ b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go @@ -15,6 +15,7 @@ import ( idpdiscoveryv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1" "go.pinniped.dev/internal/federationdomain/resolvedprovider" "go.pinniped.dev/internal/federationdomain/upstreamprovider" + "go.pinniped.dev/internal/httputil/httperr" "go.pinniped.dev/internal/psession" "go.pinniped.dev/internal/testutil/oidctestutil" "go.pinniped.dev/internal/testutil/transformtestutil" @@ -109,10 +110,11 @@ func TestLoginFromCallback(t *testing.T) { uniqueCtx := context.WithValue(context.Background(), "some-unique-key", "some-value") //nolint:staticcheck // okay to use string key for test tests := []struct { - name string - provider *oidctestutil.TestUpstreamGitHubIdentityProvider - authcode string - redirectURI string + name string + provider *oidctestutil.TestUpstreamGitHubIdentityProvider + idpDisplayName string + authcode string + redirectURI string wantExchangeAuthcodeCall bool wantExchangeAuthcodeArgs *oidctestutil.ExchangeAuthcodeArgs @@ -132,6 +134,7 @@ func TestLoginFromCallback(t *testing.T) { DownstreamSubject: "https://fake-downstream-subject", }). Build(), + idpDisplayName: "fake-display-name", authcode: "fake-authcode", redirectURI: "https://fake-redirect-uri", wantExchangeAuthcodeCall: true, @@ -142,8 +145,9 @@ func TestLoginFromCallback(t *testing.T) { }, wantGetUserCall: true, wantGetUserArgs: &oidctestutil.GetUserArgs{ - Ctx: uniqueCtx, - AccessToken: "fake-access-token", + Ctx: uniqueCtx, + AccessToken: "fake-access-token", + IDPDisplayName: "fake-display-name", }, wantIdentity: &resolvedprovider.Identity{ UpstreamUsername: "fake-username", @@ -160,6 +164,7 @@ func TestLoginFromCallback(t *testing.T) { provider: oidctestutil.NewTestUpstreamGitHubIdentityProviderBuilder(). WithAuthcodeExchangeError(errors.New("fake authcode exchange error")). Build(), + idpDisplayName: "fake-display-name", authcode: "fake-authcode", redirectURI: "https://fake-redirect-uri", wantExchangeAuthcodeCall: true, @@ -171,7 +176,7 @@ func TestLoginFromCallback(t *testing.T) { wantGetUserCall: false, wantIdentity: nil, wantExtras: nil, - wantErr: "failed to exchange auth code using GitHub API: fake authcode exchange error", + wantErr: "failed to exchange authcode using GitHub API: fake authcode exchange error: fake authcode exchange error", }, { name: "error while getting user info", @@ -179,6 +184,7 @@ func TestLoginFromCallback(t *testing.T) { WithAccessToken("fake-access-token"). WithGetUserError(errors.New("fake user info error")). Build(), + idpDisplayName: "fake-display-name", authcode: "fake-authcode", redirectURI: "https://fake-redirect-uri", wantExchangeAuthcodeCall: true, @@ -189,24 +195,23 @@ func TestLoginFromCallback(t *testing.T) { }, wantGetUserCall: true, wantGetUserArgs: &oidctestutil.GetUserArgs{ - Ctx: uniqueCtx, - AccessToken: "fake-access-token", + Ctx: uniqueCtx, + AccessToken: "fake-access-token", + IDPDisplayName: "fake-display-name", }, wantIdentity: nil, wantExtras: nil, - wantErr: "failed to get user info from GitHub API: fake user info error", + wantErr: "failed to get user info from GitHub API: fake user info error: fake user info error", }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - transforms := transformtestutil.NewRejectAllAuthPipeline(t) - subject := FederationDomainResolvedGitHubIdentityProvider{ - DisplayName: "fake-display-name", + DisplayName: test.idpDisplayName, Provider: test.provider, SessionProviderType: psession.ProviderTypeGitHub, - Transforms: transforms, + Transforms: transformtestutil.NewRejectAllAuthPipeline(t), } identity, loginExtras, err := subject.LoginFromCallback(uniqueCtx, @@ -233,7 +238,9 @@ func TestLoginFromCallback(t *testing.T) { if test.wantErr == "" { require.NoError(t, err) } else { - require.EqualError(t, err, test.wantErr) + errAsResponder, ok := err.(httperr.Responder) + require.True(t, ok) + require.EqualError(t, errAsResponder, test.wantErr) } require.Equal(t, test.wantExtras, loginExtras) require.Equal(t, test.wantIdentity, identity) diff --git a/internal/federationdomain/upstreamprovider/upsteam_provider.go b/internal/federationdomain/upstreamprovider/upsteam_provider.go index cb52dcedf..c05785417 100644 --- a/internal/federationdomain/upstreamprovider/upsteam_provider.go +++ b/internal/federationdomain/upstreamprovider/upsteam_provider.go @@ -171,5 +171,6 @@ type UpstreamGithubIdentityProviderI interface { // GetUser calls the user, orgs, and teams APIs of GitHub using the accessToken. // It validates any required org memberships. It returns a User or an error. - GetUser(ctx context.Context, accessToken string) (*GitHubUser, error) + // The IDP display name is passed to aid in building a suitable downstream subject string. + GetUser(ctx context.Context, accessToken string, idpDisplayName string) (*GitHubUser, error) } diff --git a/internal/githubclient/githubclient.go b/internal/githubclient/githubclient.go index 8fdaea6fc..c1889dcaa 100644 --- a/internal/githubclient/githubclient.go +++ b/internal/githubclient/githubclient.go @@ -9,11 +9,13 @@ import ( "fmt" "net/http" "net/url" + "slices" "strings" "github.com/google/go-github/v62/github" - "k8s.io/apimachinery/pkg/util/sets" + + "go.pinniped.dev/internal/plog" ) const ( @@ -35,7 +37,7 @@ type TeamInfo struct { type GitHubInterface interface { GetUserInfo(ctx context.Context) (*UserInfo, error) GetOrgMembership(ctx context.Context) (sets.Set[string], error) - GetTeamMembership(ctx context.Context, allowedOrganizations sets.Set[string]) ([]*TeamInfo, error) + GetTeamMembership(ctx context.Context, allowedOrganizations sets.Set[string]) ([]TeamInfo, error) } type githubClient struct { @@ -87,6 +89,7 @@ func (g *githubClient) GetUserInfo(ctx context.Context) (*UserInfo, error) { if user == nil { // untested return nil, fmt.Errorf("%s: user is nil", errorPrefix) } + plog.Trace("got raw GitHub API user results", "user", user) userInfo := &UserInfo{ Login: user.GetLogin(), @@ -98,6 +101,8 @@ func (g *githubClient) GetUserInfo(ctx context.Context) (*UserInfo, error) { if userInfo.Login == "" { return nil, fmt.Errorf(`%s: the "login" attribute is missing`, errorPrefix) } + + plog.Trace("calculated response from GitHub user endpoint", "user", userInfo) return userInfo, nil } @@ -114,6 +119,7 @@ func (g *githubClient) GetOrgMembership(ctx context.Context) (sets.Set[string], if err != nil { return nil, fmt.Errorf("%s: %w", errorPrefix, err) } + plog.Trace("got raw GitHub API org results", "orgs", organizationResults, "hasNextPage", response.NextPage) for _, organization := range organizationResults { organizationLogins.Insert(organization.GetLogin()) @@ -128,6 +134,7 @@ func (g *githubClient) GetOrgMembership(ctx context.Context) (sets.Set[string], return nil, fmt.Errorf(`%s: one or more organizations is missing the "login" attribute`, errorPrefix) } + plog.Trace("calculated response from GitHub org membership endpoint", "orgs", organizationLogins.UnsortedList()) return organizationLogins, nil } @@ -135,6 +142,10 @@ func isOrgAllowed(allowedOrganizations sets.Set[string], login string) bool { return len(allowedOrganizations) == 0 || allowedOrganizations.Has(login) } +func buildAndValidateParentTeam(githubTeam *github.Team, organizationLogin string) (*TeamInfo, error) { + return buildTeam(githubTeam, organizationLogin) +} + func buildAndValidateTeam(githubTeam *github.Team) (*TeamInfo, error) { if githubTeam.GetOrganization() == nil { return nil, errors.New(`missing the "organization" attribute for a team`) @@ -144,6 +155,10 @@ func buildAndValidateTeam(githubTeam *github.Team) (*TeamInfo, error) { return nil, errors.New(`missing the organization's "login" attribute for a team`) } + return buildTeam(githubTeam, organizationLogin) +} + +func buildTeam(githubTeam *github.Team, organizationLogin string) (*TeamInfo, error) { teamInfo := &TeamInfo{ Name: githubTeam.GetName(), Slug: githubTeam.GetSlug(), @@ -161,9 +176,9 @@ func buildAndValidateTeam(githubTeam *github.Team) (*TeamInfo, error) { // 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. -func (g *githubClient) GetTeamMembership(ctx context.Context, allowedOrganizations sets.Set[string]) ([]*TeamInfo, error) { +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) + teamInfos := sets.New[TeamInfo]() opt := &github.ListOptions{PerPage: pageSize} // get all pages of results @@ -172,6 +187,7 @@ func (g *githubClient) GetTeamMembership(ctx context.Context, allowedOrganizatio if err != nil { return nil, fmt.Errorf("%s: %w", errorPrefix, err) } + plog.Trace("got raw GitHub API team results", "teams", teamsResults, "hasNextPage", response.NextPage) for _, team := range teamsResults { teamInfo, err := buildAndValidateTeam(team) @@ -183,20 +199,18 @@ func (g *githubClient) GetTeamMembership(ctx context.Context, allowedOrganizatio continue } - teamInfos = append(teamInfos, teamInfo) + teamInfos.Insert(*teamInfo) parent := team.GetParent() if parent != nil { - teamInfo, err := buildAndValidateTeam(parent) + // The GitHub API does not return the Organization for the Parent of the team. + // Use the org of the child as the org of the parent, since they must come from the same org. + parentTeamInfo, err := buildAndValidateParentTeam(parent, teamInfo.Org) if err != nil { return nil, fmt.Errorf("%s: %w", errorPrefix, err) } - if !isOrgAllowed(allowedOrganizations, teamInfo.Org) { - continue - } - - teamInfos = append(teamInfos, teamInfo) + teamInfos.Insert(*parentTeamInfo) } } if response.NextPage == 0 { @@ -205,5 +219,16 @@ func (g *githubClient) GetTeamMembership(ctx context.Context, allowedOrganizatio opt.Page = response.NextPage } - return teamInfos, nil + // Sort by org and then by name, just so we always return teams in the same order. + sortedTeams := teamInfos.UnsortedList() + slices.SortStableFunc(sortedTeams, func(a, b TeamInfo) int { + orgsCompared := strings.Compare(a.Org, b.Org) + if orgsCompared == 0 { + return strings.Compare(a.Slug, b.Slug) + } + return orgsCompared + }) + + plog.Trace("calculated response from GitHub teams endpoint", "teams", sortedTeams) + return sortedTeams, nil } diff --git a/internal/githubclient/githubclient_test.go b/internal/githubclient/githubclient_test.go index 35ffc4e46..0ea8e7523 100644 --- a/internal/githubclient/githubclient_test.go +++ b/internal/githubclient/githubclient_test.go @@ -100,22 +100,23 @@ func TestNewGitHubClient(t *testing.T) { if test.wantErr != "" { require.EqualError(t, err, test.wantErr) - return + } else { + require.NoError(t, err) + + require.NotNil(t, actualI) + actual, ok := actualI.(*githubClient) + require.True(t, ok) + require.NotNil(t, actual.client.BaseURL) + require.Equal(t, test.wantBaseURL, actual.client.BaseURL.String()) + + // Force the githubClient's httpClient roundTrippers to run and add the Authorization header + + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, testServer.URL, nil) + require.NoError(t, err) + + _, err = actual.client.Client().Do(req) //nolint:bodyclose + require.NoError(t, err) } - - require.NotNil(t, actualI) - actual, ok := actualI.(*githubClient) - require.True(t, ok) - require.NotNil(t, actual.client.BaseURL) - require.Equal(t, test.wantBaseURL, actual.client.BaseURL.String()) - - // Force the githubClient's httpClient roundTrippers to run and add the Authorization header - - req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, testServer.URL, nil) - require.NoError(t, err) - - _, err = actual.client.Client().Do(req) //nolint:bodyclose - require.NoError(t, err) }) } } @@ -241,11 +242,11 @@ func TestGetUser(t *testing.T) { require.True(t, ok) test.wantErr = strings.ReplaceAll(test.wantErr, "{SERVER_URL}", rt.Host) require.EqualError(t, err, test.wantErr) - return + } else { + require.NoError(t, err) + require.NotNil(t, actual) + require.Equal(t, test.wantUserInfo, *actual) } - - require.NotNil(t, actual) - require.Equal(t, test.wantUserInfo, *actual) }) } } @@ -395,7 +396,7 @@ func TestGetTeamMembership(t *testing.T) { ctx context.Context allowedOrganizations []string wantErr string - wantTeams []*TeamInfo + wantTeams []TeamInfo }{ { name: "happy path", @@ -436,7 +437,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", @@ -491,7 +492,7 @@ func TestGetTeamMembership(t *testing.T) { ), token: "some-token", allowedOrganizations: []string{"alpha", "gamma"}, - wantTeams: []*TeamInfo{ + wantTeams: []TeamInfo{ { Name: "team1-name", Slug: "team1-slug", @@ -528,11 +529,9 @@ func TestGetTeamMembership(t *testing.T) { 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"), - }, + Name: github.String("delta-team-name"), + Slug: github.String("delta-team-slug"), + Organization: nil, // the real GitHub API does not return Org on "Parent" team. }, Organization: &github.Organization{ Login: github.String("gamma"), @@ -542,7 +541,7 @@ func TestGetTeamMembership(t *testing.T) { ), ), token: "some-token", - wantTeams: []*TeamInfo{ + wantTeams: []TeamInfo{ { Name: "team1-name", Slug: "team1-slug", @@ -553,20 +552,20 @@ func TestGetTeamMembership(t *testing.T) { Slug: "team2-slug", Org: "beta", }, + { + Name: "delta-team-name", + Slug: "delta-team-slug", + Org: "gamma", + }, { Name: "team3-name", Slug: "team3-slug", Org: "gamma", }, - { - Name: "delta-team-name", - Slug: "delta-team-slug", - Org: "delta", - }, }, }, { - name: "includes parent team if present", + name: "includes parent team in allowed orgs if present", httpClient: mock.NewMockedHTTPClient( mock.WithRequestMatch( mock.GetUserTeams, @@ -575,33 +574,48 @@ func TestGetTeamMembership(t *testing.T) { 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("parent-team-org-that-in-reality-can-never-be-different-than-child-team-org"), - }, + Name: github.String("parent-team-name"), + Slug: github.String("parent-team-slug"), + Organization: nil, // the real GitHub API does not return Org on "Parent" team. }, Organization: &github.Organization{ Login: github.String("org-with-nested-teams"), }, }, { - Name: github.String("team-name-without-parent"), - Slug: github.String("team-slug-without-parent"), + Name: github.String("team-name-with-same-parent-again"), + Slug: github.String("team-slug-with-same-parent-again"), + Parent: &github.Team{ + Name: github.String("parent-team-name"), + Slug: github.String("parent-team-slug"), + Organization: nil, // the real GitHub API does not return Org on "Parent" team. + }, Organization: &github.Organization{ - Login: github.String("beta"), + Login: github.String("org-with-nested-teams"), }, }, { - 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"), - }, + Name: github.String("parent-team-name"), + Slug: github.String("parent-team-slug"), + Organization: &github.Organization{ + Login: github.String("org-with-nested-teams"), }, + }, + { + Name: github.String("team-name-with-parent-from-disallowed-org"), + Slug: github.String("team-slug-with-parent-from-disallowed-org"), + Parent: &github.Team{ + Name: github.String("parent-team-name-from-disallowed-org"), + Slug: github.String("parent-team-slug-from-disallowed-org"), + Organization: nil, // the real GitHub API does not return Org on "Parent" team. + }, + Organization: &github.Organization{ + Login: github.String("disallowed-org"), + }, + }, + { + Name: github.String("team-name-without-parent"), + Slug: github.String("team-slug-without-parent"), Organization: &github.Organization{ Login: github.String("beta"), }, @@ -612,29 +626,28 @@ func TestGetTeamMembership(t *testing.T) { 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", - Slug: "team-slug-with-parent", - Org: "org-with-nested-teams", - }, - { - Name: "parent-team-name", - Slug: "parent-team-slug", - Org: "parent-team-org-that-in-reality-can-never-be-different-than-child-team-org", - }, + wantTeams: []TeamInfo{ { 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", + Name: "parent-team-name", + Slug: "parent-team-slug", + Org: "org-with-nested-teams", + }, + { + Name: "team-name-with-parent", + Slug: "team-slug-with-parent", + Org: "org-with-nested-teams", + }, + { + Name: "team-name-with-same-parent-again", + Slug: "team-slug-with-same-parent-again", + Org: "org-with-nested-teams", }, }, }, @@ -665,7 +678,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", @@ -743,99 +756,6 @@ func TestGetTeamMembership(t *testing.T) { ), 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( @@ -851,7 +771,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", @@ -907,11 +827,11 @@ func TestGetTeamMembership(t *testing.T) { require.True(t, ok) test.wantErr = strings.ReplaceAll(test.wantErr, "{SERVER_URL}", rt.Host) require.EqualError(t, err, test.wantErr) - return + } else { + require.NoError(t, err) + require.NotNil(t, actual) + require.Equal(t, test.wantTeams, actual) } - - require.NotNil(t, actual) - require.Equal(t, test.wantTeams, actual) }) } } diff --git a/internal/mocks/mockgithubclient/mockgithubclient.go b/internal/mocks/mockgithubclient/mockgithubclient.go index 6a1901c14..988031522 100644 --- a/internal/mocks/mockgithubclient/mockgithubclient.go +++ b/internal/mocks/mockgithubclient/mockgithubclient.go @@ -61,10 +61,10 @@ func (mr *MockGitHubInterfaceMockRecorder) GetOrgMembership(arg0 any) *gomock.Ca } // GetTeamMembership mocks base method. -func (m *MockGitHubInterface) GetTeamMembership(arg0 context.Context, arg1 sets.Set[string]) ([]*githubclient.TeamInfo, error) { +func (m *MockGitHubInterface) GetTeamMembership(arg0 context.Context, arg1 sets.Set[string]) ([]githubclient.TeamInfo, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetTeamMembership", arg0, arg1) - ret0, _ := ret[0].([]*githubclient.TeamInfo) + ret0, _ := ret[0].([]githubclient.TeamInfo) ret1, _ := ret[1].(error) return ret0, ret1 } diff --git a/internal/testutil/oidctestutil/testgithubprovider.go b/internal/testutil/oidctestutil/testgithubprovider.go index 1f793aeac..1eea5896c 100644 --- a/internal/testutil/oidctestutil/testgithubprovider.go +++ b/internal/testutil/oidctestutil/testgithubprovider.go @@ -24,8 +24,9 @@ type ExchangeAuthcodeArgs struct { // GetUserArgs is used to spy on calls to // TestUpstreamGitHubIdentityProvider.GetUserFunc(). type GetUserArgs struct { - Ctx context.Context - AccessToken string + Ctx context.Context + AccessToken string + IDPDisplayName string } type TestUpstreamGitHubIdentityProviderBuilder struct { @@ -232,14 +233,15 @@ func (u *TestUpstreamGitHubIdentityProvider) ExchangeAuthcodeArgs(call int) *Exc return u.exchangeAuthcodeArgs[call] } -func (u *TestUpstreamGitHubIdentityProvider) GetUser(ctx context.Context, accessToken string) (*upstreamprovider.GitHubUser, error) { +func (u *TestUpstreamGitHubIdentityProvider) GetUser(ctx context.Context, accessToken string, idpDisplayName string) (*upstreamprovider.GitHubUser, error) { if u.getUserArgs == nil { u.getUserArgs = make([]*GetUserArgs, 0) } u.getUserCallCount++ u.getUserArgs = append(u.getUserArgs, &GetUserArgs{ - Ctx: ctx, - AccessToken: accessToken, + Ctx: ctx, + AccessToken: accessToken, + IDPDisplayName: idpDisplayName, }) return u.GetUserFunc(ctx, accessToken) } diff --git a/internal/upstreamgithub/upstreamgithub.go b/internal/upstreamgithub/upstreamgithub.go index 6571c2fa7..7321ac35e 100644 --- a/internal/upstreamgithub/upstreamgithub.go +++ b/internal/upstreamgithub/upstreamgithub.go @@ -99,34 +99,23 @@ func (p *Provider) GetAuthorizationURL() string { } func (p *Provider) ExchangeAuthcode(ctx context.Context, authcode string, redirectURI string) (string, error) { - // TODO: write tests for this - panic("write some tests for this sketch of the implementation, maybe by running a test server in the unit tests") - //nolint:govet // this code is intentionally unreachable until we resolve the todos tok, err := p.c.OAuth2Config.Exchange( coreosoidc.ClientContext(ctx, p.c.HttpClient), authcode, oauth2.SetAuthURLParam("redirect_uri", redirectURI), ) if err != nil { - return "", err + return "", fmt.Errorf("error exchanging authorization code using GitHub API: %w", err) } return tok.AccessToken, nil } -// GetUser will use the provided configuration to make HTTPS calls to the GitHub API to find out who the logged-in user is, -// what organizations they belong to, and what teams they belong to. -// If the user's information meets the AllowedOrganization criteria specified on the GitHubIdentityProvider, they will be -// allowed to log in. +// GetUser will use the provided configuration to make HTTPS calls to the GitHub API to get the identity of the +// authenticated user and to discover their org and team memberships. +// If the user's information meets the AllowedOrganization criteria specified on the GitHubIdentityProvider, +// they will be allowed to log in. // Note that errors from the githubclient package already have helpful error prefixes, so there is no need for additional prefixes here. -// TODO: populate the IDP display name -// TODO: What should we do if the group or team name is outside of the enum? The controller would reject this. -// TODO: should we use the APIBaseURL or some other URL in the downstreamSubject -// -// Examples: -// -// "github.com" or "https://github.com" or "https://api.github.com"? -// "enterprise.tld" or "https://enterprise.tld" or "https://enterprise.tld/api/v3"? -func (p *Provider) GetUser(ctx context.Context, accessToken string) (*upstreamprovider.GitHubUser, error) { +func (p *Provider) GetUser(ctx context.Context, accessToken string, idpDisplayName string) (*upstreamprovider.GitHubUser, error) { githubClient, err := p.buildGitHubClient(p.c.HttpClient, p.c.APIBaseURL, accessToken) if err != nil { return nil, err @@ -139,7 +128,7 @@ func (p *Provider) GetUser(ctx context.Context, accessToken string) (*upstreampr return nil, err } - githubUser.DownstreamSubject = downstreamsubject.GitHub(p.c.APIBaseURL, "TODO_IDP_DISPLAY_NAME", userInfo.Login, userInfo.ID) + githubUser.DownstreamSubject = downstreamsubject.GitHub(p.c.APIBaseURL, idpDisplayName, userInfo.Login, userInfo.ID) switch p.c.UsernameAttribute { case supervisoridpv1alpha1.GitHubUsernameLoginAndID: @@ -148,6 +137,8 @@ func (p *Provider) GetUser(ctx context.Context, accessToken string) (*upstreampr githubUser.Username = userInfo.Login case supervisoridpv1alpha1.GitHubUsernameID: githubUser.Username = userInfo.ID + default: + return nil, fmt.Errorf("bad configuration: unknown GitHub username attribute: %s", p.c.UsernameAttribute) } orgMembership, err := githubClient.GetOrgMembership(ctx) @@ -174,6 +165,8 @@ func (p *Provider) GetUser(ctx context.Context, accessToken string) (*upstreampr downstreamGroup = fmt.Sprintf("%s/%s", team.Org, team.Name) case supervisoridpv1alpha1.GitHubUseTeamSlugForGroupName: downstreamGroup = fmt.Sprintf("%s/%s", team.Org, team.Slug) + default: + return nil, fmt.Errorf("bad configuration: unknown GitHub group name attribute: %s", p.c.GroupNameAttribute) } githubUser.Groups = append(githubUser.Groups, downstreamGroup) diff --git a/internal/upstreamgithub/upstreamgithub_test.go b/internal/upstreamgithub/upstreamgithub_test.go index 428872af6..5b8ae3c6d 100644 --- a/internal/upstreamgithub/upstreamgithub_test.go +++ b/internal/upstreamgithub/upstreamgithub_test.go @@ -5,9 +5,12 @@ package upstreamgithub import ( "context" + "crypto/tls" "errors" + "fmt" "net/http" "testing" + "time" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" @@ -15,11 +18,13 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/rand" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/client-go/util/cert" supervisoridpv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1" "go.pinniped.dev/internal/federationdomain/upstreamprovider" "go.pinniped.dev/internal/githubclient" "go.pinniped.dev/internal/mocks/mockgithubclient" + "go.pinniped.dev/internal/testutil/tlsserver" ) func TestGitHubProvider(t *testing.T) { @@ -82,7 +87,112 @@ func TestGitHubProvider(t *testing.T) { }, subject.GetConfig().HttpClient) } +func TestExchangeAuthcode(t *testing.T) { + const fakeGitHubAccessToken = "gho_16C7e42F292c6912E7710c838347Ae178B4a" //nolint:gosec // this is not a credential + + tests := []struct { + name string + tokenEndpointPath string + wantErr string + }{ + { + name: "happy path", + tokenEndpointPath: "/token", + }, + { + name: "when the GitHub token endpoint returns an error", + tokenEndpointPath: "/token-error", + wantErr: "error exchanging authorization code using GitHub API: oauth2: cannot fetch token: 401 Unauthorized\nResponse: some github error", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + testServer, testServerCA := tlsserver.TestServerIPv4(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // See documentation at https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/authorizing-oauth-apps + // GitHub docs say to use a POST. + require.Equal(t, http.MethodPost, r.Method) + + // The OAuth client library happens to choose to send these headers. Asserting here for our own understanding. + require.Len(t, r.Header, 4) + require.Equal(t, "application/x-www-form-urlencoded", r.Header.Get("Content-Type")) + require.Equal(t, "gzip", r.Header.Get("Accept-Encoding")) + require.NotEmpty(t, r.Header.Get("User-Agent")) + require.NotEmpty(t, r.Header.Get("Content-Length")) + + // Get the params. + err := r.ParseForm() + require.NoError(t, err) + params := r.PostForm + require.Len(t, params, 5) + // These four params are documented by GitHub. + require.Equal(t, "fake-client-id", params.Get("client_id")) + require.Equal(t, "fake-client-secret", params.Get("client_secret")) + require.Equal(t, "https://fake-redirect-url", params.Get("redirect_uri")) + require.Equal(t, "fake-authcode", params.Get("code")) + // This param is not documented by GitHub, but is standard OAuth2. GitHub should respect or ignore it. + require.Equal(t, "authorization_code", params.Get("grant_type")) + + // The GitHub docs say that it will return a URL encoded form by default, so I assume it would set this header. + w.Header().Set("content-type", "application/x-www-form-urlencoded") + + switch r.URL.Path { + case "/token": + // Example response from GitHub docs. + responseBody := "access_token=" + fakeGitHubAccessToken + "&scope=repo%2Cgist&token_type=bearer" + w.WriteHeader(http.StatusOK) + _, err = w.Write([]byte(responseBody)) + require.NoError(t, err) + case "/token-error": + responseBody := "some github error" + w.WriteHeader(http.StatusUnauthorized) + _, err = w.Write([]byte(responseBody)) + require.NoError(t, err) + default: + t.Fatalf("tried to call provider at unexpected endpoint: %s", r.URL.Path) + } + }), nil) + testServerPool, err := cert.NewPoolFromBytes(testServerCA) + require.NoError(t, err) + + subject := New(ProviderConfig{ + OAuth2Config: &oauth2.Config{ + ClientID: "fake-client-id", + ClientSecret: "fake-client-secret", + Scopes: []string{"scope1", "scope2"}, + Endpoint: oauth2.Endpoint{ + AuthURL: "https://fake-auth-url", + TokenURL: testServer.URL + test.tokenEndpointPath, + AuthStyle: oauth2.AuthStyleInParams, + }, + }, + HttpClient: &http.Client{ + Timeout: 10 * time.Second, + Transport: &http.Transport{TLSClientConfig: &tls.Config{ + MinVersion: tls.VersionTLS12, + RootCAs: testServerPool, + }}, + }, + }) + + accessToken, err := subject.ExchangeAuthcode(context.Background(), "fake-authcode", "https://fake-redirect-url") + if test.wantErr != "" { + require.EqualError(t, err, test.wantErr) + require.Empty(t, accessToken) + } else { + require.NoError(t, err) + require.Equal(t, fakeGitHubAccessToken, accessToken) + } + }) + } +} + func TestGetUser(t *testing.T) { + const idpDisplayName = "idp display name 😀" + const encodedIDPDisplayName = "idp+display+name+%F0%9F%98%80" + ctrl := gomock.NewController(t) t.Cleanup(ctrl.Finish) @@ -117,7 +227,7 @@ func TestGetUser(t *testing.T) { }, wantUser: &upstreamprovider.GitHubUser{ Username: "some-github-login:some-github-id", - DownstreamSubject: "https://some-url?idpName=TODO_IDP_DISPLAY_NAME&login=some-github-login&id=some-github-id", + DownstreamSubject: fmt.Sprintf("https://some-url?idpName=%s&login=some-github-login&id=some-github-id", encodedIDPDisplayName), }, }, { @@ -137,7 +247,7 @@ func TestGetUser(t *testing.T) { }, wantUser: &upstreamprovider.GitHubUser{ Username: "some-github-login", - DownstreamSubject: "https://some-url?idpName=TODO_IDP_DISPLAY_NAME&login=some-github-login&id=some-github-id", + DownstreamSubject: fmt.Sprintf("https://some-url?idpName=%s&login=some-github-login&id=some-github-id", encodedIDPDisplayName), }, }, { @@ -157,7 +267,7 @@ func TestGetUser(t *testing.T) { }, wantUser: &upstreamprovider.GitHubUser{ Username: "some-github-id", - DownstreamSubject: "https://some-url?idpName=TODO_IDP_DISPLAY_NAME&login=some-github-login&id=some-github-id", + DownstreamSubject: fmt.Sprintf("https://some-url?idpName=%s&login=some-github-login&id=some-github-id", encodedIDPDisplayName), }, }, { @@ -178,7 +288,7 @@ func TestGetUser(t *testing.T) { }, wantUser: &upstreamprovider.GitHubUser{ Username: "some-github-login:some-github-id", - DownstreamSubject: "https://some-url?idpName=TODO_IDP_DISPLAY_NAME&login=some-github-login&id=some-github-id", + DownstreamSubject: fmt.Sprintf("https://some-url?idpName=%s&login=some-github-login&id=some-github-id", encodedIDPDisplayName), }, }, { @@ -213,7 +323,7 @@ func TestGetUser(t *testing.T) { ID: "some-github-id", }, nil) mockGitHubInterface.EXPECT().GetOrgMembership(someContext).Return(sets.New[string]("allowed-org2"), nil) - mockGitHubInterface.EXPECT().GetTeamMembership(someContext, sets.New[string]("allowed-org1", "allowed-org2")).Return([]*githubclient.TeamInfo{ + mockGitHubInterface.EXPECT().GetTeamMembership(someContext, sets.New[string]("allowed-org1", "allowed-org2")).Return([]githubclient.TeamInfo{ { Name: "org1-team1-name", Slug: "org1-team1-slug", @@ -234,7 +344,7 @@ func TestGetUser(t *testing.T) { wantUser: &upstreamprovider.GitHubUser{ Username: "some-github-login:some-github-id", Groups: []string{"org1-name/org1-team1-name", "org1-name/org1-team2-name", "org2-name/org2-team1-name"}, - DownstreamSubject: "https://some-url?idpName=TODO_IDP_DISPLAY_NAME&login=some-github-login&id=some-github-id", + DownstreamSubject: fmt.Sprintf("https://some-url?idpName=%s&login=some-github-login&id=some-github-id", encodedIDPDisplayName), }, }, { @@ -252,7 +362,7 @@ func TestGetUser(t *testing.T) { ID: "some-github-id", }, nil) mockGitHubInterface.EXPECT().GetOrgMembership(someContext).Return(sets.New[string]("allowed-org2"), nil) - mockGitHubInterface.EXPECT().GetTeamMembership(someContext, sets.New[string]("allowed-org1", "allowed-org2")).Return([]*githubclient.TeamInfo{ + mockGitHubInterface.EXPECT().GetTeamMembership(someContext, sets.New[string]("allowed-org1", "allowed-org2")).Return([]githubclient.TeamInfo{ { Name: "org1-team1-name", Slug: "org1-team1-slug", @@ -273,7 +383,7 @@ func TestGetUser(t *testing.T) { wantUser: &upstreamprovider.GitHubUser{ Username: "some-github-login:some-github-id", Groups: []string{"org1-name/org1-team1-slug", "org1-name/org1-team2-slug", "org2-name/org2-team1-slug"}, - DownstreamSubject: "https://some-url?idpName=TODO_IDP_DISPLAY_NAME&login=some-github-login&id=some-github-id", + DownstreamSubject: fmt.Sprintf("https://some-url?idpName=%s&login=some-github-login&id=some-github-id", encodedIDPDisplayName), }, }, { @@ -299,8 +409,9 @@ func TestGetUser(t *testing.T) { { name: "returns errors from githubClient.GetOrgMembership()", providerConfig: ProviderConfig{ - APIBaseURL: "https://some-url", - HttpClient: someHttpClient, + APIBaseURL: "https://some-url", + HttpClient: someHttpClient, + UsernameAttribute: supervisoridpv1alpha1.GitHubUsernameLoginAndID, }, buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) { mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{}, nil) @@ -311,8 +422,9 @@ func TestGetUser(t *testing.T) { { name: "returns errors from githubClient.GetTeamMembership()", providerConfig: ProviderConfig{ - APIBaseURL: "https://some-url", - HttpClient: someHttpClient, + APIBaseURL: "https://some-url", + HttpClient: someHttpClient, + UsernameAttribute: supervisoridpv1alpha1.GitHubUsernameLoginAndID, }, buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) { mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{}, nil) @@ -321,6 +433,45 @@ func TestGetUser(t *testing.T) { }, wantErr: "error from githubClient.GetTeamMembership", }, + { + name: "bad configuration: UsernameAttribute", + providerConfig: ProviderConfig{ + APIBaseURL: "https://some-url", + HttpClient: someHttpClient, + UsernameAttribute: "this-is-not-legal-value-from-the-enum", + }, + buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) { + mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{ + Login: "some-github-login", + ID: "some-github-id", + }, nil) + }, + wantErr: "bad configuration: unknown GitHub username attribute: this-is-not-legal-value-from-the-enum", + }, + { + name: "bad configuration: GroupNameAttribute", + providerConfig: ProviderConfig{ + APIBaseURL: "https://some-url", + HttpClient: someHttpClient, + UsernameAttribute: supervisoridpv1alpha1.GitHubUsernameLoginAndID, + GroupNameAttribute: "this-is-not-legal-value-from-the-enum", + }, + buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) { + mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{ + Login: "some-github-login", + ID: "some-github-id", + }, nil) + mockGitHubInterface.EXPECT().GetOrgMembership(someContext).Return(nil, nil) + mockGitHubInterface.EXPECT().GetTeamMembership(someContext, sets.New[string]()).Return([]githubclient.TeamInfo{ + { + Name: "org1-team1-name", + Slug: "org1-team1-slug", + Org: "org1-name", + }, + }, nil) + }, + wantErr: "bad configuration: unknown GitHub group name attribute: this-is-not-legal-value-from-the-enum", + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { @@ -341,7 +492,7 @@ func TestGetUser(t *testing.T) { return mockGitHubInterface, test.buildGitHubClientError } - actualUser, actualErr := p.GetUser(context.Background(), accessToken) + actualUser, actualErr := p.GetUser(context.Background(), accessToken, idpDisplayName) if test.wantErr != "" { require.EqualError(t, actualErr, test.wantErr) require.Nil(t, actualUser) diff --git a/test/integration/e2e_test.go b/test/integration/e2e_test.go index 28fcc5f91..34e652a74 100644 --- a/test/integration/e2e_test.go +++ b/test/integration/e2e_test.go @@ -1210,8 +1210,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { }) t.Run("with Supervisor GitHub upstream IDP and browser flow with with form_post automatic authcode delivery to CLI", func(t *testing.T) { - // TODO only skip this test when the GitHub test env vars are not set - t.Skip("always skipping for now, this test is still a work in progress and it always fails at the moment") + testlib.SkipTestWhenGitHubIsUnavailable(t) testCtx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) t.Cleanup(cancel) @@ -1221,16 +1220,32 @@ func TestE2EFullIntegration_Browser(t *testing.T) { // Start a fresh browser driver because we don't want to share cookies between the various tests in this file. browser := browsertest.OpenBrowser(t) - // TODO create clusterrolebinding for expected user and WaitForUserToHaveAccess. doesn't matter until login fully works. + expectedUsername := env.SupervisorUpstreamGithub.TestUserUsername + ":" + env.SupervisorUpstreamGithub.TestUserID + expectedGroups := env.SupervisorUpstreamGithub.TestUserExpectedTeamSlugs + + // Create a ClusterRoleBinding to give our test user from the upstream read-only access to the cluster. + testlib.CreateTestClusterRoleBinding(t, + rbacv1.Subject{Kind: rbacv1.UserKind, APIGroup: rbacv1.GroupName, Name: expectedUsername}, + rbacv1.RoleRef{Kind: "ClusterRole", APIGroup: rbacv1.GroupName, Name: "view"}, + ) + testlib.WaitForUserToHaveAccess(t, expectedUsername, []string{}, &authorizationv1.ResourceAttributes{ + Verb: "get", + Group: "", + Version: "v1", + Resource: "namespaces", + }) // Create upstream GitHub provider and wait for it to become ready. - // TODO use return value when calling requireUserCanUseKubectlWithoutAuthenticatingAgain below - _ = testlib.CreateTestGitHubIdentityProvider(t, idpv1alpha1.GitHubIdentityProviderSpec{ + createdProvider := testlib.CreateTestGitHubIdentityProvider(t, idpv1alpha1.GitHubIdentityProviderSpec{ AllowAuthentication: idpv1alpha1.GitHubAllowAuthenticationSpec{ Organizations: idpv1alpha1.GitHubOrganizationsSpec{ Policy: ptr.To(idpv1alpha1.GitHubAllowedAuthOrganizationsPolicyAllGitHubUsers), }, }, + Claims: idpv1alpha1.GitHubClaims{ + Username: ptr.To(idpv1alpha1.GitHubUsernameLoginAndID), + Groups: ptr.To(idpv1alpha1.GitHubUseTeamSlugForGroupName), + }, Client: idpv1alpha1.GitHubClientSpec{ SecretName: testlib.CreateGitHubClientCredentialsSecret(t, env.SupervisorUpstreamGithub.GithubAppClientID, @@ -1262,8 +1277,7 @@ func TestE2EFullIntegration_Browser(t *testing.T) { kubectlCmd.Env = append(os.Environ(), env.ProxyEnv()...) // Run the kubectl command, wait for the Pinniped CLI to print the authorization URL, and open it in the browser. - // TODO use return value when calling requireKubectlGetNamespaceOutput below - _ = startKubectlAndOpenAuthorizationURLInBrowser(testCtx, t, kubectlCmd, browser) + kubectlOutputChan := startKubectlAndOpenAuthorizationURLInBrowser(testCtx, t, kubectlCmd, browser) // Confirm that we got to the upstream IDP's login page, fill out the form, and submit the form. browsertest.LoginToUpstreamGitHub(t, browser, env.SupervisorUpstreamGithub) @@ -1272,16 +1286,14 @@ func TestE2EFullIntegration_Browser(t *testing.T) { t.Logf("waiting for response page %s", federationDomain.Spec.Issuer) browser.WaitForURL(t, regexp.MustCompile(regexp.QuoteMeta(federationDomain.Spec.Issuer))) - // TODO When you turn off headless and watch this test run, - // the browser is indeed redirected back to the Supervisor at this point with a code, - // but the Supervisor's callback endpoint does not yet work for github IDPs so it returns an error page, - // and the Supervisor's form_post page is not loaded, so it does not automatically post the callback to the CLI's callback listener. - // The test eventually times out and fails at this point. + // The response page should have done the background fetch() and POST'ed to the CLI's callback. + // It should now be in the "success" state. + formpostExpectSuccessState(t, browser) - // TODO - // formpostExpectSuccessState - // requireKubectlGetNamespaceOutput - // requireUserCanUseKubectlWithoutAuthenticatingAgain + requireKubectlGetNamespaceOutput(t, env, waitForKubectlOutput(t, kubectlOutputChan)) + + requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, federationDomain, createdProvider.Name, kubeconfigPath, + sessionCachePath, pinnipedExe, expectedUsername, expectedGroups, allScopes) }) t.Run("with multiple IDPs: one OIDC and one LDAP", func(t *testing.T) { diff --git a/test/testlib/browsertest/browsertest.go b/test/testlib/browsertest/browsertest.go index 4cf1c3de8..2cf06a991 100644 --- a/test/testlib/browsertest/browsertest.go +++ b/test/testlib/browsertest/browsertest.go @@ -398,14 +398,27 @@ func LoginToUpstreamGitHub(t *testing.T, b *Browser, upstream testlib.TestGithub t.Logf("entering GitHub OTP code") b.SendKeysToFirstMatch(t, otpSelector, code) + // Keep looping until we get to a page that we do not know how to handle. Then return to allow the test to move on. + for handleOccasionalGithubLoginPage(t, b) { + continue + } +} + +// handleOccasionalGithubLoginPage handles the interstitial pages which GitHub might show during a login flow. +// None of these will always happen. +func handleOccasionalGithubLoginPage(t *testing.T, b *Browser) bool { + t.Helper() + t.Log("sleeping for 2 seconds before looking at page title") time.Sleep(2 * time.Second) pageTitle := b.Title(t) t.Logf("saw page title %q", pageTitle) + lowercaseTitle := strings.ToLower(pageTitle) - // Next Github might go to another page asking if you authorize the GitHub App to act on your behalf, - // if this user has never authorized this app. - if strings.HasPrefix(pageTitle, "Authorize ") { // the title is "Authorize " + switch { + case strings.HasPrefix(lowercaseTitle, "authorize "): // the title is "Authorize " + // Next Github might go to another page asking if you authorize the GitHub App to act on your behalf, + // if this user has never authorized this app. // Wait for the authorize app page to be rendered. t.Logf("waiting for GitHub authorize button") // There are unfortunately two very similar buttons on this page: @@ -415,16 +428,23 @@ func LoginToUpstreamGitHub(t *testing.T, b *Browser, upstream testlib.TestGithub b.WaitForVisibleElements(t, submitAuthorizeAppButtonSelector) t.Logf("clicking authorize button") b.ClickFirstMatch(t, submitAuthorizeAppButtonSelector) + return true - t.Log("sleeping for 2 seconds before looking at page title again") - time.Sleep(2 * time.Second) - pageTitle = b.Title(t) - t.Logf("saw page title %q", pageTitle) - } + case strings.HasPrefix(lowercaseTitle, "confirm your account recovery settings"): + // Next Github might occasionally as you to confirm your recovery settings. + // Wait for the page to be rendered. + t.Logf("waiting for GitHub confirm button") + // There are several buttons and links. We want to click this confirm button to confirm our settings: + // + submitConfirmButtonSelector := "button.btn-primary" + b.WaitForVisibleElements(t, submitConfirmButtonSelector) + t.Logf("clicking confirm button") + b.ClickFirstMatch(t, submitConfirmButtonSelector) + return true - // TODO I only saw this happen once, so I did not get a chance to finish this code. Not sure if it will happen again? - // Next GitHub might ask if we want to configure a passkey for auth. - if strings.HasPrefix(pageTitle, "Passkey TODO GET THIS PAGE TITLE") { + case strings.HasPrefix(lowercaseTitle, "configure passwordless authentication"): + // Next GitHub might occasionally ask if we want to configure a passkey for auth. + // The URL bar shows https://github.com/sessions/trusted-device for this page. // The link that we want to click looks like this: // dontAskAgainLinkSelector := `input[value="Don't ask again for this browser"]` @@ -434,6 +454,19 @@ func LoginToUpstreamGitHub(t *testing.T, b *Browser, upstream testlib.TestGithub // Tell it that we do not want to use a passkey. t.Logf("clicking don't ask again button") b.ClickFirstMatch(t, dontAskAgainLinkSelector) + return true + + case strings.HasPrefix(lowercaseTitle, "server error"): + // Sometimes this happens after the OTP page. Not sure why. The page has a cute cartoon, but no helpful information. + // The URL bar shows https://github.com/sessions/trusted-device for this error page, which is the URL that usually + // asks if you want to configure passwordless authentication (aka passkey). + t.Fatal("Got GitHub server error page during login flow. This is not expected, but is unfortunately unrecoverable.") + return false // we recognized the title, but we don't know how to handle this page because it has no buttons or other way forward + + default: + // We did not know how to handle the page given its title. + // Maybe we successfully got through all the interstitial pages and finished the login. + return false } } diff --git a/test/testlib/env.go b/test/testlib/env.go index a14bdca35..6b3615b81 100644 --- a/test/testlib/env.go +++ b/test/testlib/env.go @@ -112,11 +112,15 @@ type TestLDAPUpstream struct { } type TestGithubUpstream struct { - GithubAppClientID string `json:"githubAppClientId"` - GithubAppClientSecret string `json:"githubAppClientSecret"` - TestUserUsername string `json:"testUserUsername"` - TestUserPassword string `json:"testUserPassword"` - TestUserOTPSecret string `json:"testUserOTPSecret"` + GithubAppClientID string `json:"githubAppClientId"` + GithubAppClientSecret string `json:"githubAppClientSecret"` + TestUserUsername string `json:"testUserUsername"` // the "login" attribute value for the user + TestUserPassword string `json:"testUserPassword"` + TestUserOTPSecret string `json:"testUserOTPSecret"` + TestUserID string `json:"testUserID"` // the "id" attribute value for the user + TestUserOrganization string `json:"testUserOrganization"` // an org to which the user belongs + TestUserExpectedTeamNames []string `json:"testUserExpectedTeamNames"` + TestUserExpectedTeamSlugs []string `json:"testUserExpectedTeamSlugs"` } // ProxyEnv returns a set of environment variable strings (e.g., to combine with os.Environ()) which set up the configured test HTTP proxy. @@ -329,11 +333,15 @@ func loadEnvVars(t *testing.T, result *TestEnv) { } result.SupervisorUpstreamGithub = TestGithubUpstream{ - GithubAppClientID: wantEnv("PINNIPED_TEST_GITHUB_APP_CLIENT_ID", ""), - GithubAppClientSecret: wantEnv("PINNIPED_TEST_GITHUB_APP_CLIENT_SECRET", ""), - TestUserUsername: wantEnv("PINNIPED_TEST_GITHUB_USER_USERNAME", ""), - TestUserPassword: wantEnv("PINNIPED_TEST_GITHUB_USER_PASSWORD", ""), - TestUserOTPSecret: wantEnv("PINNIPED_TEST_GITHUB_USER_OTP_SECRET", ""), + GithubAppClientID: wantEnv("PINNIPED_TEST_GITHUB_APP_CLIENT_ID", ""), + GithubAppClientSecret: wantEnv("PINNIPED_TEST_GITHUB_APP_CLIENT_SECRET", ""), + TestUserUsername: wantEnv("PINNIPED_TEST_GITHUB_USER_USERNAME", ""), + TestUserPassword: wantEnv("PINNIPED_TEST_GITHUB_USER_PASSWORD", ""), + TestUserOTPSecret: wantEnv("PINNIPED_TEST_GITHUB_USER_OTP_SECRET", ""), + TestUserID: wantEnv("PINNIPED_TEST_GITHUB_USERID", ""), + TestUserOrganization: wantEnv("PINNIPED_TEST_GITHUB_ORG", ""), + TestUserExpectedTeamNames: filterEmpty(strings.Split(wantEnv("PINNIPED_TEST_GITHUB_EXPECTED_TEAM_NAMES", ""), ",")), + TestUserExpectedTeamSlugs: filterEmpty(strings.Split(wantEnv("PINNIPED_TEST_GITHUB_EXPECTED_TEAM_SLUGS", ""), ",")), } sort.Strings(result.SupervisorUpstreamLDAP.TestUserDirectGroupsCNs) @@ -341,6 +349,8 @@ func loadEnvVars(t *testing.T, result *TestEnv) { sort.Strings(result.SupervisorUpstreamActiveDirectory.TestUserDirectGroupsCNs) sort.Strings(result.SupervisorUpstreamActiveDirectory.TestUserDirectGroupsDNs) sort.Strings(result.SupervisorUpstreamActiveDirectory.TestUserIndirectGroupsSAMAccountNames) + sort.Strings(result.SupervisorUpstreamGithub.TestUserExpectedTeamNames) + sort.Strings(result.SupervisorUpstreamGithub.TestUserExpectedTeamSlugs) } func (e *TestEnv) HasCapability(cap Capability) bool { diff --git a/test/testlib/skip.go b/test/testlib/skip.go index 6fbc621f0..3ad283faf 100644 --- a/test/testlib/skip.go +++ b/test/testlib/skip.go @@ -1,4 +1,4 @@ -// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package testlib @@ -33,3 +33,11 @@ func SkipTestWhenActiveDirectoryIsUnavailable(t *testing.T, env *TestEnv) { t.Skip("Active Directory hostname not specified") } } + +func SkipTestWhenGitHubIsUnavailable(t *testing.T) { + t.Helper() + + if IntegrationEnv(t).SupervisorUpstreamGithub.GithubAppClientID == "" { + t.Skip("GitHub test env vars not specified") + } +}