implement upstream refresh for github

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())
+}

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

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