diff --git a/hack/header.txt b/hack/header.txt index c45500693..649ffa377 100644 --- a/hack/header.txt +++ b/hack/header.txt @@ -1,2 +1,2 @@ -Copyright 2020-2025 the Pinniped contributors. All Rights Reserved. +Copyright 2020-2026 the Pinniped contributors. All Rights Reserved. SPDX-License-Identifier: Apache-2.0 diff --git a/internal/federationdomain/endpoints/token/token_handler_test.go b/internal/federationdomain/endpoints/token/token_handler_test.go index 091ca93a9..9e2a0cb39 100644 --- a/internal/federationdomain/endpoints/token/token_handler_test.go +++ b/internal/federationdomain/endpoints/token/token_handler_test.go @@ -2149,9 +2149,10 @@ func TestRefreshGrant(t *testing.T) { return &expectedGithubUpstreamRefresh{ performedByUpstreamName: githubUpstreamName, args: &oidctestutil.GetUserArgs{ - Ctx: nil, // this will be filled in with the actual request context by the test below - AccessToken: githubUpstreamAccessToken, - IDPDisplayName: githubUpstreamName, + Ctx: nil, // this will be filled in with the actual request context by the test below + AccessToken: githubUpstreamAccessToken, + IDPDisplayName: githubUpstreamName, + RetryOnUnauthorized: upstreamprovider.DoNotRetryOnUnauthorized, }, } } diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go index 79ccc08f8..f2dd5a6b7 100644 --- a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go +++ b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go @@ -1,4 +1,4 @@ -// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2026 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package resolvedgithub @@ -108,7 +108,7 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) LoginFromCallback( ) } - user, err := p.Provider.GetUser(ctx, accessToken, p.GetDisplayName()) + user, err := p.Provider.GetUser(ctx, accessToken, p.GetDisplayName(), upstreamprovider.RetryOnUnauthorized) if errors.As(err, &upstreamprovider.GitHubLoginDeniedError{}) { // We specifically want errors of type GitHubLoginDeniedError to have a user-displayed message. @@ -153,7 +153,11 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) UpstreamRefresh( } // Get the user's GitHub identity and groups again using the cached access token. - refreshedUserInfo, err := p.Provider.GetUser(ctx, githubSessionData.UpstreamAccessToken, p.GetDisplayName()) + refreshedUserInfo, err := p.Provider.GetUser(ctx, + githubSessionData.UpstreamAccessToken, + p.GetDisplayName(), + upstreamprovider.DoNotRetryOnUnauthorized, + ) if err != nil { return nil, p.refreshErr(err) } diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go index 82e8980a3..f62941af5 100644 --- a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go +++ b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go @@ -153,9 +153,10 @@ func TestLoginFromCallback(t *testing.T) { }, wantGetUserCall: true, wantGetUserArgs: &oidctestutil.GetUserArgs{ - Ctx: uniqueCtx, - AccessToken: "fake-access-token", - IDPDisplayName: "fake-display-name", + Ctx: uniqueCtx, + AccessToken: "fake-access-token", + IDPDisplayName: "fake-display-name", + RetryOnUnauthorized: upstreamprovider.RetryOnUnauthorized, }, wantIdentity: &resolvedprovider.Identity{ UpstreamUsername: "fake-username", @@ -205,9 +206,10 @@ func TestLoginFromCallback(t *testing.T) { }, wantGetUserCall: true, wantGetUserArgs: &oidctestutil.GetUserArgs{ - Ctx: uniqueCtx, - AccessToken: "fake-access-token", - IDPDisplayName: "fake-display-name", + Ctx: uniqueCtx, + AccessToken: "fake-access-token", + IDPDisplayName: "fake-display-name", + RetryOnUnauthorized: upstreamprovider.RetryOnUnauthorized, }, wantIdentity: nil, wantExtras: nil, @@ -232,9 +234,10 @@ func TestLoginFromCallback(t *testing.T) { }, wantGetUserCall: true, wantGetUserArgs: &oidctestutil.GetUserArgs{ - Ctx: uniqueCtx, - AccessToken: "fake-access-token", - IDPDisplayName: "fake-display-name", + Ctx: uniqueCtx, + AccessToken: "fake-access-token", + IDPDisplayName: "fake-display-name", + RetryOnUnauthorized: upstreamprovider.RetryOnUnauthorized, }, wantIdentity: nil, wantExtras: nil, @@ -324,9 +327,10 @@ func TestUpstreamRefresh(t *testing.T) { idpDisplayName: "fake-display-name", wantGetUserCall: true, wantGetUserArgs: &oidctestutil.GetUserArgs{ - Ctx: uniqueCtx, - AccessToken: "fake-access-token", - IDPDisplayName: "fake-display-name", + Ctx: uniqueCtx, + AccessToken: "fake-access-token", + IDPDisplayName: "fake-display-name", + RetryOnUnauthorized: upstreamprovider.DoNotRetryOnUnauthorized, }, wantRefreshedIdentity: &resolvedprovider.RefreshedIdentity{ UpstreamUsername: "refreshed-username", @@ -349,9 +353,10 @@ func TestUpstreamRefresh(t *testing.T) { idpDisplayName: "fake-display-name", wantGetUserCall: true, wantGetUserArgs: &oidctestutil.GetUserArgs{ - Ctx: uniqueCtx, - AccessToken: "fake-access-token", - IDPDisplayName: "fake-display-name", + Ctx: uniqueCtx, + AccessToken: "fake-access-token", + IDPDisplayName: "fake-display-name", + RetryOnUnauthorized: upstreamprovider.DoNotRetryOnUnauthorized, }, wantRefreshedIdentity: nil, wantWrappedErr: "fake github GetUser error message", @@ -407,9 +412,10 @@ func TestUpstreamRefresh(t *testing.T) { idpDisplayName: "fake-display-name", wantGetUserCall: true, wantGetUserArgs: &oidctestutil.GetUserArgs{ - Ctx: uniqueCtx, - AccessToken: "fake-access-token", - IDPDisplayName: "fake-display-name", + Ctx: uniqueCtx, + AccessToken: "fake-access-token", + IDPDisplayName: "fake-display-name", + RetryOnUnauthorized: upstreamprovider.DoNotRetryOnUnauthorized, }, wantRefreshedIdentity: nil, wantWrappedErr: `user's calculated downstream subject at initial login was "https://fake-downstream-subject" ` + diff --git a/internal/federationdomain/upstreamprovider/upstream_provider.go b/internal/federationdomain/upstreamprovider/upstream_provider.go index 070de4594..4a1743272 100644 --- a/internal/federationdomain/upstreamprovider/upstream_provider.go +++ b/internal/federationdomain/upstreamprovider/upstream_provider.go @@ -1,4 +1,4 @@ -// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2026 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package upstreamprovider @@ -26,6 +26,18 @@ const ( AccessTokenType RevocableTokenType = "access_token" ) +// UnauthorizedRetryBehavior controls whether GetUser retries after a 401 from the GitHub user API. +type UnauthorizedRetryBehavior bool + +const ( + // RetryOnUnauthorized retries a bounded number of times after a short delay, since GitHub + // occasionally returns a transient 401 for a token that was just issued. + RetryOnUnauthorized UnauthorizedRetryBehavior = true + // DoNotRetryOnUnauthorized fails immediately on a 401 (e.g. during a refresh, where a 401 + // indicates a genuinely revoked or invalid token). + DoNotRetryOnUnauthorized UnauthorizedRetryBehavior = false +) + // LDAPRefreshAttributes contains information about the user from the original login request // and previous refreshes to be used during an LDAP session refresh. type LDAPRefreshAttributes struct { @@ -189,5 +201,8 @@ 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. // 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) + // If retryOnUnauthorized is true, a 401 from the user API will be retried a bounded number of + // times after a short delay, to work around GitHub occasionally returning a transient 401 for a + // token that was just issued. + GetUser(ctx context.Context, accessToken string, idpDisplayName string, retryOnUnauthorized UnauthorizedRetryBehavior) (*GitHubUser, error) } diff --git a/internal/githubclient/githubclient.go b/internal/githubclient/githubclient.go index 046638162..4f1f16f22 100644 --- a/internal/githubclient/githubclient.go +++ b/internal/githubclient/githubclient.go @@ -11,6 +11,7 @@ import ( "net/url" "slices" "strings" + "time" "github.com/google/go-github/v87/github" "k8s.io/apimachinery/pkg/util/sets" @@ -22,6 +23,13 @@ import ( const ( emptyUserMeansTheAuthenticatedUser = "" pageSize = 100 + + // defaultUnauthorizedRetryDelay and defaultUnauthorizedMaxRetries control how GetUserInfo + // retries when the GitHub user API returns a 401 for a token that was just issued. GitHub + // sometimes returns a transient 401 for a valid, freshly issued token before it has fully + // propagated; hopefully waiting briefly and retrying will resolve it. + defaultUnauthorizedRetryDelay = 1 * time.Second + defaultUnauthorizedMaxRetries = 2 ) type UserInfo struct { @@ -36,13 +44,18 @@ type TeamInfo struct { } type GitHubInterface interface { - GetUserInfo(ctx context.Context) (*UserInfo, error) + GetUserInfo(ctx context.Context, retryOnUnauthorized bool) (*UserInfo, error) GetOrgMembership(ctx context.Context) ([]string, error) GetTeamMembership(ctx context.Context, allowedOrganizations *setutil.CaseInsensitiveSet) ([]TeamInfo, error) } type githubClient struct { client *github.Client + + // unauthorizedRetryDelay and unauthorizedMaxRetries configure GetUserInfo's retry-on-401 + // behavior. They are fields so that unit tests can adjust them. + unauthorizedRetryDelay time.Duration + unauthorizedMaxRetries int } var _ GitHubInterface = (*githubClient)(nil) @@ -85,15 +98,43 @@ func NewGitHubClient(httpClient *http.Client, apiBaseURL, token string) (GitHubI } return &githubClient{ - client: client, + client: client, + unauthorizedRetryDelay: defaultUnauthorizedRetryDelay, + unauthorizedMaxRetries: defaultUnauthorizedMaxRetries, }, nil } -// GetUserInfo returns the "Login" and "ID" attributes of the logged-in user. -func (g *githubClient) GetUserInfo(ctx context.Context) (*UserInfo, error) { +// isUnauthorized returns true only when err is a *github.ErrorResponse for an HTTP 401. GitHub's +// rate-limiting conditions are surfaced by go-github as the distinct *github.RateLimitError and +// *github.AbuseRateLimitError types (HTTP 403/429), so this check can never match those and cannot +// interfere with go-github's rate-limit handling. +func isUnauthorized(err error) bool { + var errResp *github.ErrorResponse + return errors.As(err, &errResp) && + errResp.Response != nil && + errResp.Response.StatusCode == http.StatusUnauthorized +} + +// GetUserInfo returns the "Login" and "ID" attributes of the logged-in user. If retryOnUnauthorized +// is true and GitHub responds with a 401, the request is retried a bounded number of times after a +// short delay, since GitHub sometimes returns a transient 401 for a token that was just issued. +func (g *githubClient) GetUserInfo(ctx context.Context, retryOnUnauthorized bool) (*UserInfo, error) { const errorPrefix = "error fetching authenticated user" - user, _, err := g.client.Users.Get(ctx, emptyUserMeansTheAuthenticatedUser) + var user *github.User + var err error + for attempt := 0; ; attempt++ { + user, _, err = g.client.Users.Get(ctx, emptyUserMeansTheAuthenticatedUser) + if err == nil || !retryOnUnauthorized || !isUnauthorized(err) || attempt >= g.unauthorizedMaxRetries { + break + } + plog.Debug("got 401 from GitHub user endpoint", "attempt", attempt+1) + select { + case <-ctx.Done(): + return nil, fmt.Errorf("%s: %w", errorPrefix, ctx.Err()) + case <-time.After(g.unauthorizedRetryDelay): + } + } if err != nil { return nil, fmt.Errorf("%s: %w", errorPrefix, err) } diff --git a/internal/githubclient/githubclient_test.go b/internal/githubclient/githubclient_test.go index 3926c0cc0..c77648d47 100644 --- a/internal/githubclient/githubclient_test.go +++ b/internal/githubclient/githubclient_test.go @@ -8,6 +8,7 @@ import ( "net/http" "strings" "testing" + "time" "github.com/google/go-github/v87/github" "github.com/migueleliasweb/go-github-mock/src/mock" @@ -125,12 +126,13 @@ func TestGetUser(t *testing.T) { t.Parallel() tests := []struct { - name string - httpClient *http.Client - token string - ctx context.Context - wantErr string - wantUserInfo UserInfo + name string + httpClient *http.Client + token string + ctx context.Context + retryOnUnauthorized bool + wantErr string + wantUserInfo UserInfo }{ { name: "happy path", @@ -236,7 +238,7 @@ func TestGetUser(t *testing.T) { ctx = test.ctx } - actual, err := githubClient.GetUserInfo(ctx) + actual, err := githubClient.GetUserInfo(ctx, test.retryOnUnauthorized) if test.wantErr != "" { rt, ok := test.httpClient.Transport.(*mock.EnforceHostRoundTripper) require.True(t, ok) @@ -251,6 +253,115 @@ func TestGetUser(t *testing.T) { } } +func TestGetUserInfoRetryOnUnauthorized(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + retryOnUnauthorized bool + retryDelay time.Duration // zero means no artificial delay + ctxTimeout time.Duration // zero means context.Background() with no timeout + handlerStatus int + handlerMessage string + succeedOnAttempt int // returns success instead of handlerStatus after this many attempts; 0 means never succeed + wantErrContains string + wantErrIs error + wantUserInfo *UserInfo + wantAttempts int + }{ + { + name: "retries once on a 401 then succeeds, when retryOnUnauthorized is true", + retryOnUnauthorized: true, + handlerStatus: http.StatusUnauthorized, + handlerMessage: "bad credentials", + succeedOnAttempt: 2, + wantUserInfo: &UserInfo{Login: "some-username", ID: "12345678"}, + wantAttempts: 2, + }, + { + name: "exhausts retries and fails when every attempt returns a 401, when retryOnUnauthorized is true", + retryOnUnauthorized: true, + handlerStatus: http.StatusUnauthorized, + handlerMessage: "bad credentials", + wantErrContains: "401 bad credentials", + wantAttempts: 1 + defaultUnauthorizedMaxRetries, + }, + { + name: "does not retry a 401 when retryOnUnauthorized is false", + retryOnUnauthorized: false, + handlerStatus: http.StatusUnauthorized, + handlerMessage: "bad credentials", + wantErrContains: "401 bad credentials", + wantAttempts: 1, + }, + { + name: "does not retry a non-401 error even when retryOnUnauthorized is true", + retryOnUnauthorized: true, + handlerStatus: http.StatusForbidden, + handlerMessage: "rate limited", + wantErrContains: "403 rate limited", + wantAttempts: 1, + }, + { + name: "returns promptly when the context is canceled during the retry wait", + retryOnUnauthorized: true, + retryDelay: time.Hour, // long enough that a real wait would fail the test + ctxTimeout: 50 * time.Millisecond, + handlerStatus: http.StatusUnauthorized, + handlerMessage: "bad credentials", + wantErrIs: context.DeadlineExceeded, + wantAttempts: 1, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + attempts := new(int) + + httpClient := mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler(mock.GetUser, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + *attempts++ + if *attempts == test.succeedOnAttempt { + _, err := w.Write([]byte(`{"login":"some-username","id":12345678}`)) + require.NoError(t, err) + return + } + mock.WriteError(w, test.handlerStatus, test.handlerMessage) + })), + ) + + c, err := github.NewClient(github.WithHTTPClient(httpClient), github.WithAuthToken("some-token")) + require.NoError(t, err) + + githubClient := &githubClient{ + client: c, + unauthorizedRetryDelay: test.retryDelay, + unauthorizedMaxRetries: defaultUnauthorizedMaxRetries, + } + + ctx := context.Background() + if test.ctxTimeout != 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, test.ctxTimeout) + defer cancel() + } + + actual, err := githubClient.GetUserInfo(ctx, test.retryOnUnauthorized) + require.Equal(t, test.wantAttempts, *attempts) + switch { + case test.wantErrContains != "": + require.ErrorContains(t, err, test.wantErrContains) + case test.wantErrIs != nil: + require.ErrorIs(t, err, test.wantErrIs) + default: + require.NoError(t, err) + require.Equal(t, test.wantUserInfo, actual) + } + }) + } +} + func TestGetOrgMembership(t *testing.T) { t.Parallel() diff --git a/internal/mocks/mockcachevalue/mockcachevalue.go b/internal/mocks/mockcachevalue/mockcachevalue.go index 5aa817f7a..13dc68aa1 100644 --- a/internal/mocks/mockcachevalue/mockcachevalue.go +++ b/internal/mocks/mockcachevalue/mockcachevalue.go @@ -1,4 +1,4 @@ -// Copyright 2020-2025 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2026 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // diff --git a/internal/mocks/mockcredentialrequest/mockcredentialrequest.go b/internal/mocks/mockcredentialrequest/mockcredentialrequest.go index fbe51490b..31ff558ea 100644 --- a/internal/mocks/mockcredentialrequest/mockcredentialrequest.go +++ b/internal/mocks/mockcredentialrequest/mockcredentialrequest.go @@ -1,4 +1,4 @@ -// Copyright 2020-2025 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2026 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // diff --git a/internal/mocks/mockgithubclient/mockgithubclient.go b/internal/mocks/mockgithubclient/mockgithubclient.go index 7f7ee6f20..b8e42d8a1 100644 --- a/internal/mocks/mockgithubclient/mockgithubclient.go +++ b/internal/mocks/mockgithubclient/mockgithubclient.go @@ -1,4 +1,4 @@ -// Copyright 2020-2025 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2026 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // @@ -77,16 +77,16 @@ func (mr *MockGitHubInterfaceMockRecorder) GetTeamMembership(ctx, allowedOrganiz } // GetUserInfo mocks base method. -func (m *MockGitHubInterface) GetUserInfo(ctx context.Context) (*githubclient.UserInfo, error) { +func (m *MockGitHubInterface) GetUserInfo(ctx context.Context, retryOnUnauthorized bool) (*githubclient.UserInfo, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetUserInfo", ctx) + ret := m.ctrl.Call(m, "GetUserInfo", ctx, retryOnUnauthorized) ret0, _ := ret[0].(*githubclient.UserInfo) ret1, _ := ret[1].(error) return ret0, ret1 } // GetUserInfo indicates an expected call of GetUserInfo. -func (mr *MockGitHubInterfaceMockRecorder) GetUserInfo(ctx any) *gomock.Call { +func (mr *MockGitHubInterfaceMockRecorder) GetUserInfo(ctx, retryOnUnauthorized any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserInfo", reflect.TypeOf((*MockGitHubInterface)(nil).GetUserInfo), ctx) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUserInfo", reflect.TypeOf((*MockGitHubInterface)(nil).GetUserInfo), ctx, retryOnUnauthorized) } diff --git a/internal/mocks/mockissuer/mockissuer.go b/internal/mocks/mockissuer/mockissuer.go index 28f711fe7..594969aad 100644 --- a/internal/mocks/mockissuer/mockissuer.go +++ b/internal/mocks/mockissuer/mockissuer.go @@ -1,4 +1,4 @@ -// Copyright 2020-2025 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2026 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // diff --git a/internal/mocks/mockkeyset/mockkeyset.go b/internal/mocks/mockkeyset/mockkeyset.go index a444be834..a2d4dcfed 100644 --- a/internal/mocks/mockkeyset/mockkeyset.go +++ b/internal/mocks/mockkeyset/mockkeyset.go @@ -1,4 +1,4 @@ -// Copyright 2020-2025 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2026 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // diff --git a/internal/mocks/mockkubecertagent/mockdynamiccert.go b/internal/mocks/mockkubecertagent/mockdynamiccert.go index c417a4f74..a8456d22c 100644 --- a/internal/mocks/mockkubecertagent/mockdynamiccert.go +++ b/internal/mocks/mockkubecertagent/mockdynamiccert.go @@ -1,4 +1,4 @@ -// Copyright 2020-2025 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2026 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // diff --git a/internal/mocks/mockkubecertagent/mockpodcommandexecutor.go b/internal/mocks/mockkubecertagent/mockpodcommandexecutor.go index 66b4e93fb..206dcd5e8 100644 --- a/internal/mocks/mockkubecertagent/mockpodcommandexecutor.go +++ b/internal/mocks/mockkubecertagent/mockpodcommandexecutor.go @@ -1,4 +1,4 @@ -// Copyright 2020-2025 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2026 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // diff --git a/internal/mocks/mockldapconn/mockldapconn.go b/internal/mocks/mockldapconn/mockldapconn.go index 90a4e1264..599cdb816 100644 --- a/internal/mocks/mockldapconn/mockldapconn.go +++ b/internal/mocks/mockldapconn/mockldapconn.go @@ -1,4 +1,4 @@ -// Copyright 2020-2025 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2026 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // diff --git a/internal/mocks/mockoidcclientoptions/mockoidcclientoptions.go b/internal/mocks/mockoidcclientoptions/mockoidcclientoptions.go index ce4438664..4dc4356be 100644 --- a/internal/mocks/mockoidcclientoptions/mockoidcclientoptions.go +++ b/internal/mocks/mockoidcclientoptions/mockoidcclientoptions.go @@ -1,4 +1,4 @@ -// Copyright 2020-2025 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2026 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // diff --git a/internal/mocks/mockresponsewriter/mockresponsewriter.go b/internal/mocks/mockresponsewriter/mockresponsewriter.go index 1d14f37ac..e6f1edf71 100644 --- a/internal/mocks/mockresponsewriter/mockresponsewriter.go +++ b/internal/mocks/mockresponsewriter/mockresponsewriter.go @@ -1,4 +1,4 @@ -// Copyright 2020-2025 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2026 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // diff --git a/internal/mocks/mocksecrethelper/mocksecrethelper.go b/internal/mocks/mocksecrethelper/mocksecrethelper.go index 65acf06ca..c3c976016 100644 --- a/internal/mocks/mocksecrethelper/mocksecrethelper.go +++ b/internal/mocks/mocksecrethelper/mocksecrethelper.go @@ -1,4 +1,4 @@ -// Copyright 2020-2025 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2026 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // diff --git a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go index d41a01b6b..db009669d 100644 --- a/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go +++ b/internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go @@ -1,4 +1,4 @@ -// Copyright 2020-2025 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2026 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // diff --git a/internal/testutil/oidctestutil/testgithubprovider.go b/internal/testutil/oidctestutil/testgithubprovider.go index dea7fa9ff..7c8575afe 100644 --- a/internal/testutil/oidctestutil/testgithubprovider.go +++ b/internal/testutil/oidctestutil/testgithubprovider.go @@ -1,4 +1,4 @@ -// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2026 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package oidctestutil @@ -25,9 +25,10 @@ type ExchangeAuthcodeArgs struct { // GetUserArgs is used to spy on calls to // TestUpstreamGitHubIdentityProvider.GetUserFunc(). type GetUserArgs struct { - Ctx context.Context - AccessToken string - IDPDisplayName string + Ctx context.Context + AccessToken string + IDPDisplayName string + RetryOnUnauthorized upstreamprovider.UnauthorizedRetryBehavior } type TestUpstreamGitHubIdentityProviderBuilder struct { @@ -239,15 +240,16 @@ func (u *TestUpstreamGitHubIdentityProvider) ExchangeAuthcodeArgs(call int) *Exc return u.exchangeAuthcodeArgs[call] } -func (u *TestUpstreamGitHubIdentityProvider) GetUser(ctx context.Context, accessToken string, idpDisplayName string) (*upstreamprovider.GitHubUser, error) { +func (u *TestUpstreamGitHubIdentityProvider) GetUser(ctx context.Context, accessToken string, idpDisplayName string, retryOnUnauthorized upstreamprovider.UnauthorizedRetryBehavior) (*upstreamprovider.GitHubUser, error) { if u.getUserArgs == nil { u.getUserArgs = make([]*GetUserArgs, 0) } u.getUserCallCount++ u.getUserArgs = append(u.getUserArgs, &GetUserArgs{ - Ctx: ctx, - AccessToken: accessToken, - IDPDisplayName: idpDisplayName, + Ctx: ctx, + AccessToken: accessToken, + IDPDisplayName: idpDisplayName, + RetryOnUnauthorized: retryOnUnauthorized, }) return u.GetUserFunc(ctx, accessToken) } diff --git a/internal/upstreamgithub/upstreamgithub.go b/internal/upstreamgithub/upstreamgithub.go index dc8e2e56b..722bcc9aa 100644 --- a/internal/upstreamgithub/upstreamgithub.go +++ b/internal/upstreamgithub/upstreamgithub.go @@ -1,4 +1,4 @@ -// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2026 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package upstreamgithub implements an abstraction of upstream GitHub provider interactions. @@ -115,7 +115,7 @@ func (p *Provider) ExchangeAuthcode(ctx context.Context, authcode string, redire // 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. -func (p *Provider) GetUser(ctx context.Context, accessToken string, idpDisplayName string) (*upstreamprovider.GitHubUser, error) { +func (p *Provider) GetUser(ctx context.Context, accessToken string, idpDisplayName string, retryOnUnauthorized upstreamprovider.UnauthorizedRetryBehavior) (*upstreamprovider.GitHubUser, error) { githubClient, err := p.buildGitHubClient(p.c.HttpClient, p.c.APIBaseURL, accessToken) if err != nil { return nil, err @@ -123,7 +123,7 @@ func (p *Provider) GetUser(ctx context.Context, accessToken string, idpDisplayNa githubUser := upstreamprovider.GitHubUser{} - userInfo, err := githubClient.GetUserInfo(ctx) + userInfo, err := githubClient.GetUserInfo(ctx, bool(retryOnUnauthorized)) if err != nil { return nil, err } diff --git a/internal/upstreamgithub/upstreamgithub_test.go b/internal/upstreamgithub/upstreamgithub_test.go index 7b72fa4d1..391633c0d 100644 --- a/internal/upstreamgithub/upstreamgithub_test.go +++ b/internal/upstreamgithub/upstreamgithub_test.go @@ -207,6 +207,7 @@ func TestGetUser(t *testing.T) { tests := []struct { name string providerConfig ProviderConfig + retryOnUnauthorized upstreamprovider.UnauthorizedRetryBehavior buildGitHubClientError error buildMockResponses func(hubInterface *mockgithubclient.MockGitHubInterface) wantUser *upstreamprovider.GitHubUser @@ -221,7 +222,28 @@ func TestGetUser(t *testing.T) { UsernameAttribute: idpv1alpha1.GitHubUsernameLoginAndID, }, buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) { - mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{ + mockGitHubInterface.EXPECT().GetUserInfo(someContext, false).Return(&githubclient.UserInfo{ + Login: "some-github-login", + ID: "some-github-id", + }, nil) + mockGitHubInterface.EXPECT().GetOrgMembership(someContext).Return(nil, nil) + mockGitHubInterface.EXPECT().GetTeamMembership(someContext, gomock.Any()).Return(nil, nil) + }, + wantUser: &upstreamprovider.GitHubUser{ + Username: "some-github-login:some-github-id", + DownstreamSubject: fmt.Sprintf("https://some-url?idpName=%s&login=some-github-login&id=some-github-id", encodedIDPDisplayName), + }, + }, + { + name: "happy path with retryOnUnauthorized", + providerConfig: ProviderConfig{ + APIBaseURL: "https://some-url", + HttpClient: someHttpClient, + UsernameAttribute: idpv1alpha1.GitHubUsernameLoginAndID, + }, + retryOnUnauthorized: upstreamprovider.RetryOnUnauthorized, + buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) { + mockGitHubInterface.EXPECT().GetUserInfo(someContext, true).Return(&githubclient.UserInfo{ Login: "some-github-login", ID: "some-github-id", }, nil) @@ -241,7 +263,7 @@ func TestGetUser(t *testing.T) { UsernameAttribute: idpv1alpha1.GitHubUsernameLogin, }, buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) { - mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{ + mockGitHubInterface.EXPECT().GetUserInfo(someContext, false).Return(&githubclient.UserInfo{ Login: "some-github-login", ID: "some-github-id", }, nil) @@ -261,7 +283,7 @@ func TestGetUser(t *testing.T) { UsernameAttribute: idpv1alpha1.GitHubUsernameID, }, buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) { - mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{ + mockGitHubInterface.EXPECT().GetUserInfo(someContext, false).Return(&githubclient.UserInfo{ Login: "some-github-login", ID: "some-github-id", }, nil) @@ -282,7 +304,7 @@ func TestGetUser(t *testing.T) { AllowedOrganizations: setutil.NewCaseInsensitiveSet("ALLOWED-ORG1", "ALLOWED-ORG2"), }, buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) { - mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{ + mockGitHubInterface.EXPECT().GetUserInfo(someContext, false).Return(&githubclient.UserInfo{ Login: "some-github-login", ID: "some-github-id", }, nil) @@ -303,7 +325,7 @@ func TestGetUser(t *testing.T) { AllowedOrganizations: setutil.NewCaseInsensitiveSet("allowed-org"), }, buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) { - mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{ + mockGitHubInterface.EXPECT().GetUserInfo(someContext, false).Return(&githubclient.UserInfo{ Login: "some-github-login", ID: "some-github-id", }, nil) @@ -321,7 +343,7 @@ func TestGetUser(t *testing.T) { GroupNameAttribute: idpv1alpha1.GitHubUseTeamNameForGroupName, }, buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) { - mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{ + mockGitHubInterface.EXPECT().GetUserInfo(someContext, false).Return(&githubclient.UserInfo{ Login: "some-github-login", ID: "some-github-id", }, nil) @@ -360,7 +382,7 @@ func TestGetUser(t *testing.T) { GroupNameAttribute: idpv1alpha1.GitHubUseTeamSlugForGroupName, }, buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) { - mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{ + mockGitHubInterface.EXPECT().GetUserInfo(someContext, false).Return(&githubclient.UserInfo{ Login: "some-github-login", ID: "some-github-id", }, nil) @@ -405,7 +427,7 @@ func TestGetUser(t *testing.T) { HttpClient: someHttpClient, }, buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) { - mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(nil, errors.New("error from githubClient.GetUserInfo")) + mockGitHubInterface.EXPECT().GetUserInfo(someContext, false).Return(nil, errors.New("error from githubClient.GetUserInfo")) }, wantErrMsg: "error from githubClient.GetUserInfo", }, @@ -417,7 +439,7 @@ func TestGetUser(t *testing.T) { UsernameAttribute: idpv1alpha1.GitHubUsernameLoginAndID, }, buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) { - mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{}, nil) + mockGitHubInterface.EXPECT().GetUserInfo(someContext, false).Return(&githubclient.UserInfo{}, nil) mockGitHubInterface.EXPECT().GetOrgMembership(someContext).Return(nil, errors.New("error from githubClient.GetOrgMembership")) }, wantErrMsg: "error from githubClient.GetOrgMembership", @@ -430,7 +452,7 @@ func TestGetUser(t *testing.T) { UsernameAttribute: idpv1alpha1.GitHubUsernameLoginAndID, }, buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) { - mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{}, nil) + mockGitHubInterface.EXPECT().GetUserInfo(someContext, false).Return(&githubclient.UserInfo{}, nil) mockGitHubInterface.EXPECT().GetOrgMembership(someContext).Return(nil, nil) mockGitHubInterface.EXPECT().GetTeamMembership(someContext, gomock.Any()).Return(nil, errors.New("error from githubClient.GetTeamMembership")) }, @@ -444,7 +466,7 @@ func TestGetUser(t *testing.T) { UsernameAttribute: "this-is-not-legal-value-from-the-enum", }, buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) { - mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{ + mockGitHubInterface.EXPECT().GetUserInfo(someContext, false).Return(&githubclient.UserInfo{ Login: "some-github-login", ID: "some-github-id", }, nil) @@ -460,7 +482,7 @@ func TestGetUser(t *testing.T) { GroupNameAttribute: "this-is-not-legal-value-from-the-enum", }, buildMockResponses: func(mockGitHubInterface *mockgithubclient.MockGitHubInterface) { - mockGitHubInterface.EXPECT().GetUserInfo(someContext).Return(&githubclient.UserInfo{ + mockGitHubInterface.EXPECT().GetUserInfo(someContext, false).Return(&githubclient.UserInfo{ Login: "some-github-login", ID: "some-github-id", }, nil) @@ -498,7 +520,7 @@ func TestGetUser(t *testing.T) { return mockGitHubInterface, test.buildGitHubClientError } - actualUser, actualErr := p.GetUser(context.Background(), accessToken, idpDisplayName) + actualUser, actualErr := p.GetUser(context.Background(), accessToken, idpDisplayName, test.retryOnUnauthorized) switch { case test.wantErrMsg != "":