diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go index e0bec1ed1..3f53538a9 100644 --- a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go +++ b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go @@ -9,6 +9,7 @@ import ( "fmt" "net/http" + "github.com/ory/fosite" "golang.org/x/oauth2" "go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1" @@ -133,13 +134,41 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) LoginFromCallback( } func (p *FederationDomainResolvedGitHubIdentityProvider) UpstreamRefresh( - _ context.Context, + ctx context.Context, identity *resolvedprovider.Identity, ) (*resolvedprovider.RefreshedIdentity, error) { - // TODO: actually implement refresh. this is just a placeholder that will make refresh always succeed. + githubSessionData, ok := identity.IDPSpecificSessionData.(*psession.GitHubSessionData) + if !ok { + // This should not really happen. + return nil, p.refreshErr(errors.New("wrong data type found for IDPSpecificSessionData")) + } + if len(githubSessionData.UpstreamAccessToken) == 0 { + // This should not really happen. + return nil, p.refreshErr(errors.New("session is missing GitHub access token")) + } + + // Get the user's GitHub identity and groups again using the cached access token. + refreshedUserInfo, err := p.Provider.GetUser(ctx, githubSessionData.UpstreamAccessToken, p.GetDisplayName()) + if err != nil { + return nil, p.refreshErr(fmt.Errorf("failed to get user info from GitHub API: %w", err)) + } + + if refreshedUserInfo.DownstreamSubject != identity.DownstreamSubject { + // The user's upstream identity changed since the initial login in a surprising way. + return nil, p.refreshErr(fmt.Errorf("user's calculated downstream subject at initial login was %q but now is %q", + identity.DownstreamSubject, refreshedUserInfo.DownstreamSubject)) + } + return &resolvedprovider.RefreshedIdentity{ - UpstreamUsername: identity.UpstreamUsername, - UpstreamGroups: identity.UpstreamGroups, + UpstreamUsername: refreshedUserInfo.Username, + UpstreamGroups: refreshedUserInfo.Groups, IDPSpecificSessionData: nil, // nil means that no update to the GitHub-specific portion of the session data is required }, nil } + +func (p *FederationDomainResolvedGitHubIdentityProvider) refreshErr(err error) *fosite.RFC6749Error { + return resolvedprovider.ErrUpstreamRefreshError(). + WithHint("Upstream refresh failed."). + WithTrace(err). + WithDebugf("provider name: %q, provider type: %q", p.Provider.GetName(), p.GetSessionProviderType()) +} diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go index 5a2b63b6e..824b2bead 100644 --- a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go +++ b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go @@ -6,8 +6,10 @@ package resolvedgithub import ( "context" "errors" + "net/http" "testing" + "github.com/ory/fosite" "github.com/stretchr/testify/require" "golang.org/x/oauth2" @@ -248,3 +250,164 @@ func TestLoginFromCallback(t *testing.T) { }) } } + +func TestUpstreamRefresh(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 + idpDisplayName string + identity *resolvedprovider.Identity + + wantGetUserCall bool + wantGetUserArgs *oidctestutil.GetUserArgs + wantRefreshedIdentity *resolvedprovider.RefreshedIdentity + wantWrappedErr string + }{ + { + name: "happy path", + provider: oidctestutil.NewTestUpstreamGitHubIdentityProviderBuilder(). + WithUser(&upstreamprovider.GitHubUser{ + Username: "refreshed-username", + Groups: []string{"refreshed-group1", "refreshed-group2"}, + DownstreamSubject: "https://fake-downstream-subject", + }). + Build(), + identity: &resolvedprovider.Identity{ + UpstreamUsername: "initial-username", + UpstreamGroups: []string{"initial-group1", "initial-group2"}, + DownstreamSubject: "https://fake-downstream-subject", + IDPSpecificSessionData: &psession.GitHubSessionData{UpstreamAccessToken: "fake-access-token"}, + }, + idpDisplayName: "fake-display-name", + wantGetUserCall: true, + wantGetUserArgs: &oidctestutil.GetUserArgs{ + Ctx: uniqueCtx, + AccessToken: "fake-access-token", + IDPDisplayName: "fake-display-name", + }, + wantRefreshedIdentity: &resolvedprovider.RefreshedIdentity{ + UpstreamUsername: "refreshed-username", + UpstreamGroups: []string{"refreshed-group1", "refreshed-group2"}, + IDPSpecificSessionData: nil, + }, + }, + { + name: "error while getting user info", + provider: oidctestutil.NewTestUpstreamGitHubIdentityProviderBuilder(). + WithName("fake-provider-name"). + WithGetUserError(errors.New("fake user info error")). + Build(), + identity: &resolvedprovider.Identity{ + UpstreamUsername: "initial-username", + UpstreamGroups: []string{"initial-group1", "initial-group2"}, + DownstreamSubject: "https://fake-downstream-subject", + IDPSpecificSessionData: &psession.GitHubSessionData{UpstreamAccessToken: "fake-access-token"}, + }, + idpDisplayName: "fake-display-name", + wantGetUserCall: true, + wantGetUserArgs: &oidctestutil.GetUserArgs{ + Ctx: uniqueCtx, + AccessToken: "fake-access-token", + IDPDisplayName: "fake-display-name", + }, + wantRefreshedIdentity: nil, + wantWrappedErr: "failed to get user info from GitHub API: fake user info error", + }, + { + name: "wrong session data type, which should not really happen", + provider: oidctestutil.NewTestUpstreamGitHubIdentityProviderBuilder(). + WithName("fake-provider-name"). + Build(), + identity: &resolvedprovider.Identity{ + UpstreamUsername: "initial-username", + UpstreamGroups: []string{"initial-group1", "initial-group2"}, + DownstreamSubject: "https://fake-downstream-subject", + IDPSpecificSessionData: &psession.LDAPSessionData{}, // wrong type + }, + idpDisplayName: "fake-display-name", + wantGetUserCall: false, + wantRefreshedIdentity: nil, + wantWrappedErr: "wrong data type found for IDPSpecificSessionData", + }, + { + name: "session is missing github access token, which should not really happen", + provider: oidctestutil.NewTestUpstreamGitHubIdentityProviderBuilder(). + WithName("fake-provider-name"). + Build(), + identity: &resolvedprovider.Identity{ + UpstreamUsername: "initial-username", + UpstreamGroups: []string{"initial-group1", "initial-group2"}, + DownstreamSubject: "https://fake-downstream-subject", + IDPSpecificSessionData: &psession.GitHubSessionData{UpstreamAccessToken: ""}, // missing token + }, + idpDisplayName: "fake-display-name", + wantGetUserCall: false, + wantRefreshedIdentity: nil, + wantWrappedErr: "session is missing GitHub access token", + }, + { + name: "users downstream subject changes based on an unexpected change in the upstream identity", + provider: oidctestutil.NewTestUpstreamGitHubIdentityProviderBuilder(). + WithName("fake-provider-name"). + WithUser(&upstreamprovider.GitHubUser{ + Username: "refreshed-username", + Groups: []string{"refreshed-group1", "refreshed-group2"}, + DownstreamSubject: "https://unexpected-different-downstream-subject", // unexpected change in calculated subject during refresh + }). + Build(), + identity: &resolvedprovider.Identity{ + UpstreamUsername: "initial-username", + UpstreamGroups: []string{"initial-group1", "initial-group2"}, + DownstreamSubject: "https://fake-downstream-subject", + IDPSpecificSessionData: &psession.GitHubSessionData{UpstreamAccessToken: "fake-access-token"}, + }, + idpDisplayName: "fake-display-name", + wantGetUserCall: true, + wantGetUserArgs: &oidctestutil.GetUserArgs{ + Ctx: uniqueCtx, + AccessToken: "fake-access-token", + IDPDisplayName: "fake-display-name", + }, + wantRefreshedIdentity: nil, + wantWrappedErr: `user's calculated downstream subject at initial login was "https://fake-downstream-subject" ` + + `but now is "https://unexpected-different-downstream-subject"`, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + subject := FederationDomainResolvedGitHubIdentityProvider{ + DisplayName: test.idpDisplayName, + Provider: test.provider, + SessionProviderType: psession.ProviderTypeGitHub, + Transforms: transformtestutil.NewRejectAllAuthPipeline(t), + } + + refreshedIdentity, err := subject.UpstreamRefresh(uniqueCtx, test.identity) + + if test.wantGetUserCall { + require.Equal(t, 1, test.provider.GetUserCallCount()) + require.Equal(t, test.wantGetUserArgs, test.provider.GetUserArgs(0)) + } else { + require.Zero(t, test.provider.GetUserCallCount()) + } + + if test.wantWrappedErr == "" { + require.NoError(t, err) + } else { + require.NotNil(t, err, "expected to get an error but did not get one") + errAsFositeErr, ok := err.(*fosite.RFC6749Error) + require.True(t, ok) + require.EqualError(t, errAsFositeErr.Unwrap(), test.wantWrappedErr) + require.Equal(t, "error", errAsFositeErr.ErrorField) + require.Equal(t, "Error during upstream refresh.", errAsFositeErr.DescriptionField) + require.Equal(t, http.StatusUnauthorized, errAsFositeErr.CodeField) + require.Equal(t, `provider name: "fake-provider-name", provider type: "github"`, errAsFositeErr.DebugField) + } + + require.Equal(t, test.wantRefreshedIdentity, refreshedIdentity) + }) + } +} diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 5ab67d5af..d17c3ee49 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -516,7 +516,24 @@ func TestSupervisorLogin_Browser(t *testing.T) { createIDP: func(t *testing.T) string { return testlib.CreateTestGitHubIdentityProvider(t, basicGitHubIdentityProviderSpec(), idpv1alpha1.GitHubPhaseReady).Name }, - requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlowGitHub, + requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlowGitHub, + editRefreshSessionDataWithoutBreaking: func(t *testing.T, sessionData *psession.PinnipedSession, _, _ string) []string { + // Even if we update this group to some names that did not come from the GitHub API, + // we expect that it will return to the real groups from the GitHub API after we refresh. + initialGroupMembership := []string{"some-wrong-group", "some-other-group"} + sessionData.Custom.UpstreamGroups = initialGroupMembership // upstream group names in session + sessionData.Fosite.Claims.Extra["groups"] = initialGroupMembership // downstream group names in session + return env.SupervisorUpstreamGithub.TestUserExpectedTeamSlugs + }, + breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, _, _ string) { + // Pretend that the github access token was revoked or expired by changing it to an + // invalid access token in the user's session data. This should cause refresh to fail because + // during the refresh the GitHub API will not accept this bad access token. + customSessionData := pinnipedSession.Custom + require.Equal(t, psession.ProviderTypeGitHub, customSessionData.ProviderType) + require.NotEmpty(t, customSessionData.GitHub.UpstreamAccessToken) + customSessionData.GitHub.UpstreamAccessToken = "purposely-using-bad-access-token-during-an-automated-integration-test" + }, wantDownstreamIDTokenSubjectToMatch: expectedIDTokenSubjectRegexForUpstreamGitHub, wantDownstreamIDTokenUsernameToMatch: func(_ string) string { return "^" + regexp.QuoteMeta(env.SupervisorUpstreamGithub.TestUserUsername+":"+env.SupervisorUpstreamGithub.TestUserID) + "$" @@ -572,7 +589,7 @@ func TestSupervisorLogin_Browser(t *testing.T) { wantDownstreamIDTokenGroups: env.SupervisorUpstreamGithub.TestUserExpectedTeamNames, }, { - name: "github when user does not belong to any of the allowed orgs, should fail to exchange downstream authcode", + name: "github when user does not belong to any of the allowed orgs, should fail at the Supervisor callback endpoint", maybeSkip: skipGitHubTests, createIDP: func(t *testing.T) string { spec := basicGitHubIdentityProviderSpec() @@ -2836,6 +2853,7 @@ func testSupervisorLogin( // Should have got an error since the upstream refresh should have failed. require.Error(t, err) require.EqualError(t, err, `oauth2: "error" "Error during upstream refresh. Upstream refresh failed."`) + t.Log("successfully confirmed that breaking the refresh session data caused the refresh to fail") } }