Revoke upstream OIDC refresh tokens during GC

internal/controller/supervisorstorage/garbage_collector.go

@@ -4,15 +4,19 @@
 package supervisorstorage
 
 import (
+	"context"
 	"errors"
+	"fmt"
 	"time"
 
+	coreosoidc "github.com/coreos/go-oidc/v3/oidc"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/util/clock"
 	corev1informers "k8s.io/client-go/informers/core/v1"
 	"k8s.io/client-go/kubernetes"
+	"k8s.io/utils/strings/slices"
 
 	pinnipedcontroller "go.pinniped.dev/internal/controller"
 	"go.pinniped.dev/internal/controllerlib"
@@ -24,6 +28,7 @@ import (
 	"go.pinniped.dev/internal/fositestorage/refreshtoken"
 	"go.pinniped.dev/internal/oidc/provider"
 	"go.pinniped.dev/internal/plog"
+	"go.pinniped.dev/internal/psession"
 )
 
 const minimumRepeatInterval = 30 * time.Second
@@ -119,10 +124,10 @@ func (c *garbageCollectorController) Sync(ctx controllerlib.Context) error {
 		if garbageCollectAfterTime.Before(frozenClock.Now()) {
 			storageType, isSessionStorage := secret.Labels[crud.SecretLabelKey]
 			if isSessionStorage {
-				err := c.maybeRevokeUpstreamOIDCRefreshToken(storageType, secret)
+				err := c.maybeRevokeUpstreamOIDCRefreshToken(ctx.Context, storageType, secret)
 				if err != nil {
 					// Log the error for debugging purposes, but still carry on to delete the Secret despite the error.
-					plog.DebugErr("garbage collector could not revoke upstream refresh token", err, logKV(secret))
+					plog.WarningErr("garbage collector could not revoke upstream refresh token", err, logKV(secret))
 				}
 			}
 
@@ -143,70 +148,96 @@ func (c *garbageCollectorController) Sync(ctx controllerlib.Context) error {
 	return nil
 }
 
-//nolint:godot // do not complain about the following to-do comment
+func (c *garbageCollectorController) maybeRevokeUpstreamOIDCRefreshToken(ctx context.Context, storageType string, secret *v1.Secret) error {
-// TODO write unit tests for all of the following cases. Note that there is already a test
-//  double implemented for the RevokeRefreshToken() method on the objects in the idpCache
-//  to help with the test mocking. See RevokeRefreshTokenCallCount() and RevokeRefreshTokenArgs(int)
-//  in TestUpstreamOIDCIdentityProvider (in file oidctestutil.go). This will allowing following
-//  the pattern used in other unit tests that fill the idpCache with mock providers using the builders
-//  from oidctestutil.go.
-func (c *garbageCollectorController) maybeRevokeUpstreamOIDCRefreshToken(storageType string, secret *v1.Secret) error {
 	// All session storage types hold upstream refresh tokens when the upstream IDP is an OIDC provider.
 	// However, some of them will be outdated because they are not updated by fosite after creation.
 	// Our goal below is to always revoke the latest upstream refresh token that we are holding for the
 	// session, and only the latest.
 	switch storageType {
 	case authorizationcode.TypeLabelValue:
-		// For authcode storage, check if the authcode was used. If the authcode was never used, then its
+		authorizeCodeSession, err := authorizationcode.ReadFromSecret(secret)
-		// storage must contain the latest upstream refresh token, so revoke it.
+		if err != nil {
-		// TODO Use ReadFromSecret from the authorizationcode package under fositestorage to validate/parse the Secret, return any errors
+			return err
-		// TODO return nil if the upstream type is not OIDC
+		}
-		// TODO return nil if the authcode is *NOT* active (meaning that it was already used)
+		// Check if this downstream authcode was already used. If it was already used (i.e. not active anymore), then
-		// TODO lookup the idp by name in c.idpCache to get the cached provider interface, return error if not found
+		// the latest upstream refresh token can be found in one of the other storage types handled below instead.
-		// TODO use the cached interface to revoke the refresh token, return any error
+		if !authorizeCodeSession.Active {
-		plog.Trace("garbage collector successfully revoked upstream refresh token", logKV(secret))
 			return nil
+		}
+		// When the downstream authcode was never used, then its storage must contain the latest upstream refresh token.
+		return c.revokeUpstreamOIDCRefreshToken(ctx, authorizeCodeSession.Request.Session.(*psession.PinnipedSession).Custom, secret)
+
 	case accesstoken.TypeLabelValue:
-		// For access token storage, check if the "offline_access" scope was granted on the downstream
+		// For access token storage, check if the "offline_access" scope was granted on the downstream session.
-		// session. If it was not, then the user could not possibly have performed a downstream refresh.
+		// If it was granted, then the latest upstream refresh token should be found in the refresh token storage instead.
-		// In this case, the access token storage has the latest version of the upstream refresh token,
+		// If it was not granted, then the user could not possibly have performed a downstream refresh, so the
-		// so call the upstream issuer to revoke it.
+		// access token storage has the latest version of the upstream refresh token.
-		// TODO Implement ReadFromSecret in the accesstoken package, similar to how it was done in the authorizationcode package
+		accessTokenSession, err := accesstoken.ReadFromSecret(secret)
-		// TODO Use that the new ReadFromSecret func to validate/parse the Secret, return any errors
+		if err != nil {
-		// TODO return nil if the upstream type is not OIDC
+			return err
-		// TODO return nil if the "offline_access" scope was *NOT* granted on the downstream session
+		}
-		// TODO lookup the idp by name in c.idpCache to get the cached provider interface, return error if not found
+		pinnipedSession := accessTokenSession.Request.Session.(*psession.PinnipedSession)
-		// TODO use the cached interface to revoke the refresh token, return any error
+		if slices.Contains(accessTokenSession.Request.GetGrantedScopes(), coreosoidc.ScopeOfflineAccess) {
-		plog.Trace("garbage collector successfully revoked upstream refresh token", logKV(secret))
 			return nil
+		}
+		return c.revokeUpstreamOIDCRefreshToken(ctx, pinnipedSession.Custom, secret)
+
 	case refreshtoken.TypeLabelValue:
-		// For refresh token storage, revoke its upstream refresh token. This refresh token storage could
+		// For refresh token storage, always revoke its upstream refresh token. This refresh token storage could
-		// be the result of the authcode token exchange, or it could be the result of a downstream refresh.
+		// be the result of the initial downstream authcode exchange, or it could be the result of a downstream
-		// Either way, it always contains the latest upstream refresh token when it exists.
+		// refresh. Either way, it always contains the latest upstream refresh token when it exists.
-		// TODO Implement ReadFromSecret in the refreshtoken package, similar to how it was done in the authorizationcode package
+		refreshTokenSession, err := refreshtoken.ReadFromSecret(secret)
-		// TODO Use that new ReadFromSecret func to validate/parse the Secret, return any errors
+		if err != nil {
-		// TODO return nil if the upstream type is not OIDC
+			return err
-		// TODO lookup the idp by name in c.idpCache to get the cached provider interface, return error if not found
+		}
-		// TODO use the cached interface to always revoke the refresh token, return any error
+		return c.revokeUpstreamOIDCRefreshToken(ctx, refreshTokenSession.Request.Session.(*psession.PinnipedSession).Custom, secret)
-		plog.Trace("garbage collector successfully revoked upstream refresh token", logKV(secret))
+
-		return nil
 	case pkce.TypeLabelValue:
 		// For PKCE storage, its very existence means that the authcode was never exchanged, because these
 		// are deleted during authcode exchange. No need to do anything, since the upstream refresh token
 		// revocation is handled by authcode storage case above.
 		return nil
+
 	case openidconnect.TypeLabelValue:
 		// For OIDC storage, there is no need to do anything for reasons similar to the PKCE storage.
 		// These are not deleted during authcode exchange, probably due to a bug in fosite, even though it
 		// will never be read or updated again. However, the refresh token contained inside will be revoked
 		// by one of the other cases above.
 		return nil
+
 	default:
 		// There are no other storage types, so this should never happen in practice.
 		return errors.New("garbage collector saw invalid label on Secret when trying to determine if upstream revocation was needed")
 	}
 }
 
+func (c *garbageCollectorController) revokeUpstreamOIDCRefreshToken(ctx context.Context, customSessionData *psession.CustomSessionData, secret *v1.Secret) error {
+	// When session was for another upstream IDP type, e.g. LDAP, there is no upstream OIDC refresh token involved.
+	if customSessionData.ProviderType != psession.ProviderTypeOIDC {
+		return nil
+	}
+
+	// Try to find the provider that was originally used to create the stored session.
+	var foundOIDCIdentityProviderI provider.UpstreamOIDCIdentityProviderI
+	for _, p := range c.idpCache.GetOIDCIdentityProviders() {
+		if p.GetName() == customSessionData.ProviderName && p.GetResourceUID() == customSessionData.ProviderUID {
+			foundOIDCIdentityProviderI = p
+		}
+	}
+	if foundOIDCIdentityProviderI == nil {
+		return fmt.Errorf("could not find upstream OIDC provider named %q with resource UID %q", customSessionData.ProviderName, customSessionData.ProviderUID)
+	}
+
+	// Revoke the upstream refresh token. This is a noop if the upstream provider does not offer a revocation endpoint.
+	err := foundOIDCIdentityProviderI.RevokeRefreshToken(ctx, customSessionData.OIDC.UpstreamRefreshToken)
+	if err != nil {
+		return err
+	}
+
+	plog.Trace("garbage collector successfully revoked upstream OIDC refresh token (or provider has no revocation endpoint)", logKV(secret))
+	return nil
+}
+
 func logKV(secret *v1.Secret) []interface{} {
 	return []interface{}{
 		"secretName", secret.Name,

internal/controller/supervisorstorage/garbage_collector_test.go

@@ -5,10 +5,12 @@ package supervisorstorage
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"testing"
 	"time"
 
+	"github.com/ory/fosite"
 	"github.com/sclevine/spec"
 	"github.com/sclevine/spec/report"
 	"github.com/stretchr/testify/require"
@@ -23,7 +25,14 @@ import (
 	kubetesting "k8s.io/client-go/testing"
 
 	"go.pinniped.dev/internal/controllerlib"
+	"go.pinniped.dev/internal/fositestorage/accesstoken"
+	"go.pinniped.dev/internal/fositestorage/authorizationcode"
+	"go.pinniped.dev/internal/fositestorage/refreshtoken"
+	"go.pinniped.dev/internal/oidc/clientregistry"
+	"go.pinniped.dev/internal/oidc/provider"
+	"go.pinniped.dev/internal/psession"
 	"go.pinniped.dev/internal/testutil"
+	"go.pinniped.dev/internal/testutil/oidctestutil"
 )
 
 func TestGarbageCollectorControllerInformerFilters(t *testing.T) {
@@ -130,10 +139,10 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 
 		// Defer starting the informers until the last possible moment so that the
 		// nested Before's can keep adding things to the informer caches.
-		var startInformersAndController = func() {
+		var startInformersAndController = func(idpCache provider.DynamicUpstreamIDPProvider) {
 			// Set this at the last second to allow for injection of server override.
 			subject = GarbageCollectorController(
-				nil, // TODO put an IDP cache here for these tests (use the builder like other controller tests)
+				idpCache,
 				fakeClock,
 				deleteOptionsRecorder,
 				kubeInformers.Core().V1().Secrets(),
@@ -185,7 +194,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 
 		when("there are secrets without the garbage-collect-after annotation", func() {
 			it("does not delete those secrets", func() {
-				startInformersAndController()
+				startInformersAndController(nil)
 				r.NoError(controllerlib.TestSync(t, subject, *syncContext))
 
 				require.Empty(t, kubeClient.Actions())
@@ -196,7 +205,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 			})
 		})
 
-		when("there are secrets with the garbage-collect-after annotation", func() {
+		when("there are any secrets with the garbage-collect-after annotation", func() {
 			it.Before(func() {
 				firstExpiredSecret := &corev1.Secret{
 					ObjectMeta: metav1.ObjectMeta{
@@ -238,7 +247,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 			})
 
 			it("should delete any that are past their expiration", func() {
-				startInformersAndController()
+				startInformersAndController(nil)
 				r.NoError(controllerlib.TestSync(t, subject, *syncContext))
 
 				r.ElementsMatch(
@@ -262,6 +271,651 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 			})
 		})
 
+		when("there are valid, expired authcode secrets", func() {
+			it.Before(func() {
+				activeOIDCAuthcodeSession := &authorizationcode.Session{
+					Version: "2",
+					Active:  true,
+					Request: &fosite.Request{
+						ID:     "request-id-1",
+						Client: &clientregistry.Client{},
+						Session: &psession.PinnipedSession{
+							Custom: &psession.CustomSessionData{
+								ProviderUID:  "upstream-oidc-provider-uid",
+								ProviderName: "upstream-oidc-provider-name",
+								ProviderType: psession.ProviderTypeOIDC,
+								OIDC: &psession.OIDCSessionData{
+									UpstreamRefreshToken: "fake-upstream-refresh-token",
+								},
+							},
+						},
+					},
+				}
+				activeOIDCAuthcodeSessionJSON, err := json.Marshal(activeOIDCAuthcodeSession)
+				r.NoError(err)
+				activeOIDCAuthcodeSessionSecret := &corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:            "activeOIDCAuthcodeSession",
+						Namespace:       installedInNamespace,
+						UID:             "uid-123",
+						ResourceVersion: "rv-123",
+						Annotations: map[string]string{
+							"storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339),
+						},
+						Labels: map[string]string{
+							"storage.pinniped.dev/type": authorizationcode.TypeLabelValue,
+						},
+					},
+					Data: map[string][]byte{
+						"pinniped-storage-data":    activeOIDCAuthcodeSessionJSON,
+						"pinniped-storage-version": []byte("1"),
+					},
+					Type: "storage.pinniped.dev/" + authorizationcode.TypeLabelValue,
+				}
+				_, err = authorizationcode.ReadFromSecret(activeOIDCAuthcodeSessionSecret)
+				r.NoError(err, "the test author accidentally formed an invalid authcode secret")
+				r.NoError(kubeInformerClient.Tracker().Add(activeOIDCAuthcodeSessionSecret))
+				r.NoError(kubeClient.Tracker().Add(activeOIDCAuthcodeSessionSecret))
+
+				inactiveOIDCAuthcodeSession := &authorizationcode.Session{
+					Version: "2",
+					Active:  false,
+					Request: &fosite.Request{
+						ID:     "request-id-2",
+						Client: &clientregistry.Client{},
+						Session: &psession.PinnipedSession{
+							Custom: &psession.CustomSessionData{
+								ProviderUID:  "upstream-oidc-provider-uid",
+								ProviderName: "upstream-oidc-provider-name",
+								ProviderType: psession.ProviderTypeOIDC,
+								OIDC: &psession.OIDCSessionData{
+									UpstreamRefreshToken: "other-fake-upstream-refresh-token",
+								},
+							},
+						},
+					},
+				}
+				inactiveOIDCAuthcodeSessionJSON, err := json.Marshal(inactiveOIDCAuthcodeSession)
+				r.NoError(err)
+				inactiveOIDCAuthcodeSessionSecret := &corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:            "inactiveOIDCAuthcodeSession",
+						Namespace:       installedInNamespace,
+						UID:             "uid-456",
+						ResourceVersion: "rv-456",
+						Annotations: map[string]string{
+							"storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339),
+						},
+						Labels: map[string]string{
+							"storage.pinniped.dev/type": authorizationcode.TypeLabelValue,
+						},
+					},
+					Data: map[string][]byte{
+						"pinniped-storage-data":    inactiveOIDCAuthcodeSessionJSON,
+						"pinniped-storage-version": []byte("1"),
+					},
+					Type: "storage.pinniped.dev/" + authorizationcode.TypeLabelValue,
+				}
+				_, err = authorizationcode.ReadFromSecret(inactiveOIDCAuthcodeSessionSecret)
+				r.NoError(err, "the test author accidentally formed an invalid authcode secret")
+				r.NoError(kubeInformerClient.Tracker().Add(inactiveOIDCAuthcodeSessionSecret))
+				r.NoError(kubeClient.Tracker().Add(inactiveOIDCAuthcodeSessionSecret))
+			})
+
+			it("should revoke upstream tokens only from the active authcode secrets and delete them all", func() {
+				happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().
+					WithName("upstream-oidc-provider-name").
+					WithResourceUID("upstream-oidc-provider-uid").
+					WithRevokeRefreshTokenError(nil)
+				idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build())
+
+				startInformersAndController(idpListerBuilder.Build())
+				r.NoError(controllerlib.TestSync(t, subject, *syncContext))
+
+				// The upstream refresh token is only revoked for the active authcode session.
+				idpListerBuilder.RequireExactlyOneCallToRevokeRefreshToken(t,
+					"upstream-oidc-provider-name",
+					&oidctestutil.RevokeRefreshTokenArgs{
+						Ctx:          syncContext.Context,
+						RefreshToken: "fake-upstream-refresh-token",
+					},
+				)
+
+				// Both authcode session secrets are deleted.
+				r.ElementsMatch(
+					[]kubetesting.Action{
+						kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "activeOIDCAuthcodeSession"),
+						kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "inactiveOIDCAuthcodeSession"),
+					},
+					kubeClient.Actions(),
+				)
+				r.ElementsMatch(
+					[]metav1.DeleteOptions{
+						testutil.NewPreconditions("uid-123", "rv-123"),
+						testutil.NewPreconditions("uid-456", "rv-456"),
+					},
+					*deleteOptions,
+				)
+			})
+		})
+
+		when("there is an invalid, expired authcode secret", func() {
+			it.Before(func() {
+				invalidOIDCAuthcodeSession := &authorizationcode.Session{
+					Version: "2",
+					Active:  true,
+					Request: &fosite.Request{
+						ID:     "", // it is invalid for there to be a missing request ID
+						Client: &clientregistry.Client{},
+						Session: &psession.PinnipedSession{
+							Custom: &psession.CustomSessionData{
+								ProviderUID:  "upstream-oidc-provider-uid",
+								ProviderName: "upstream-oidc-provider-name",
+								ProviderType: psession.ProviderTypeOIDC,
+								OIDC: &psession.OIDCSessionData{
+									UpstreamRefreshToken: "fake-upstream-refresh-token",
+								},
+							},
+						},
+					},
+				}
+				invalidOIDCAuthcodeSessionJSON, err := json.Marshal(invalidOIDCAuthcodeSession)
+				r.NoError(err)
+				invalidOIDCAuthcodeSessionSecret := &corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:            "invalidOIDCAuthcodeSession",
+						Namespace:       installedInNamespace,
+						UID:             "uid-123",
+						ResourceVersion: "rv-123",
+						Annotations: map[string]string{
+							"storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339),
+						},
+						Labels: map[string]string{
+							"storage.pinniped.dev/type": authorizationcode.TypeLabelValue,
+						},
+					},
+					Data: map[string][]byte{
+						"pinniped-storage-data":    invalidOIDCAuthcodeSessionJSON,
+						"pinniped-storage-version": []byte("1"),
+					},
+					Type: "storage.pinniped.dev/" + authorizationcode.TypeLabelValue,
+				}
+				r.NoError(kubeInformerClient.Tracker().Add(invalidOIDCAuthcodeSessionSecret))
+				r.NoError(kubeClient.Tracker().Add(invalidOIDCAuthcodeSessionSecret))
+			})
+
+			it("should remove the secret without revoking any upstream tokens", func() {
+				happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().
+					WithName("upstream-oidc-provider-name").
+					WithResourceUID("upstream-oidc-provider-uid").
+					WithRevokeRefreshTokenError(nil)
+				idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build())
+
+				startInformersAndController(idpListerBuilder.Build())
+				r.NoError(controllerlib.TestSync(t, subject, *syncContext))
+
+				// Nothing to revoke since we couldn't read the invalid secret.
+				idpListerBuilder.RequireExactlyZeroCallsToRevokeRefreshToken(t)
+
+				// The invalid authcode session secrets is still deleted because it is expired.
+				r.ElementsMatch(
+					[]kubetesting.Action{
+						kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "invalidOIDCAuthcodeSession"),
+					},
+					kubeClient.Actions(),
+				)
+				r.ElementsMatch(
+					[]metav1.DeleteOptions{
+						testutil.NewPreconditions("uid-123", "rv-123"),
+					},
+					*deleteOptions,
+				)
+			})
+		})
+
+		when("there is a valid, expired authcode secret but its upstream name does not match any existing upstream", func() {
+			it.Before(func() {
+				wrongProviderNameOIDCAuthcodeSession := &authorizationcode.Session{
+					Version: "2",
+					Active:  true,
+					Request: &fosite.Request{
+						ID:     "request-id-1",
+						Client: &clientregistry.Client{},
+						Session: &psession.PinnipedSession{
+							Custom: &psession.CustomSessionData{
+								ProviderUID:  "upstream-oidc-provider-uid",
+								ProviderName: "upstream-oidc-provider-name-will-not-match",
+								ProviderType: psession.ProviderTypeOIDC,
+								OIDC: &psession.OIDCSessionData{
+									UpstreamRefreshToken: "fake-upstream-refresh-token",
+								},
+							},
+						},
+					},
+				}
+				wrongProviderNameOIDCAuthcodeSessionJSON, err := json.Marshal(wrongProviderNameOIDCAuthcodeSession)
+				r.NoError(err)
+				wrongProviderNameOIDCAuthcodeSessionSecret := &corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:            "wrongProviderNameOIDCAuthcodeSession",
+						Namespace:       installedInNamespace,
+						UID:             "uid-123",
+						ResourceVersion: "rv-123",
+						Annotations: map[string]string{
+							"storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339),
+						},
+						Labels: map[string]string{
+							"storage.pinniped.dev/type": authorizationcode.TypeLabelValue,
+						},
+					},
+					Data: map[string][]byte{
+						"pinniped-storage-data":    wrongProviderNameOIDCAuthcodeSessionJSON,
+						"pinniped-storage-version": []byte("1"),
+					},
+					Type: "storage.pinniped.dev/" + authorizationcode.TypeLabelValue,
+				}
+				_, err = authorizationcode.ReadFromSecret(wrongProviderNameOIDCAuthcodeSessionSecret)
+				r.NoError(err, "the test author accidentally formed an invalid authcode secret")
+				r.NoError(kubeInformerClient.Tracker().Add(wrongProviderNameOIDCAuthcodeSessionSecret))
+				r.NoError(kubeClient.Tracker().Add(wrongProviderNameOIDCAuthcodeSessionSecret))
+			})
+
+			it("should remove the secret without revoking any upstream tokens", func() {
+				happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().
+					WithName("upstream-oidc-provider-name").
+					WithResourceUID("upstream-oidc-provider-uid").
+					WithRevokeRefreshTokenError(nil)
+				idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build())
+
+				startInformersAndController(idpListerBuilder.Build())
+				r.NoError(controllerlib.TestSync(t, subject, *syncContext))
+
+				// Nothing to revoke since we couldn't find the upstream in the cache.
+				idpListerBuilder.RequireExactlyZeroCallsToRevokeRefreshToken(t)
+
+				// The authcode session secrets is still deleted because it is expired.
+				r.ElementsMatch(
+					[]kubetesting.Action{
+						kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "wrongProviderNameOIDCAuthcodeSession"),
+					},
+					kubeClient.Actions(),
+				)
+				r.ElementsMatch(
+					[]metav1.DeleteOptions{
+						testutil.NewPreconditions("uid-123", "rv-123"),
+					},
+					*deleteOptions,
+				)
+			})
+		})
+
+		when("there is a valid, expired authcode secret but its upstream UID does not match any existing upstream", func() {
+			it.Before(func() {
+				wrongProviderNameOIDCAuthcodeSession := &authorizationcode.Session{
+					Version: "2",
+					Active:  true,
+					Request: &fosite.Request{
+						ID:     "request-id-1",
+						Client: &clientregistry.Client{},
+						Session: &psession.PinnipedSession{
+							Custom: &psession.CustomSessionData{
+								ProviderUID:  "upstream-oidc-provider-uid-will-not-match",
+								ProviderName: "upstream-oidc-provider-name",
+								ProviderType: psession.ProviderTypeOIDC,
+								OIDC: &psession.OIDCSessionData{
+									UpstreamRefreshToken: "fake-upstream-refresh-token",
+								},
+							},
+						},
+					},
+				}
+				wrongProviderNameOIDCAuthcodeSessionJSON, err := json.Marshal(wrongProviderNameOIDCAuthcodeSession)
+				r.NoError(err)
+				wrongProviderNameOIDCAuthcodeSessionSecret := &corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:            "wrongProviderNameOIDCAuthcodeSession",
+						Namespace:       installedInNamespace,
+						UID:             "uid-123",
+						ResourceVersion: "rv-123",
+						Annotations: map[string]string{
+							"storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339),
+						},
+						Labels: map[string]string{
+							"storage.pinniped.dev/type": authorizationcode.TypeLabelValue,
+						},
+					},
+					Data: map[string][]byte{
+						"pinniped-storage-data":    wrongProviderNameOIDCAuthcodeSessionJSON,
+						"pinniped-storage-version": []byte("1"),
+					},
+					Type: "storage.pinniped.dev/" + authorizationcode.TypeLabelValue,
+				}
+				_, err = authorizationcode.ReadFromSecret(wrongProviderNameOIDCAuthcodeSessionSecret)
+				r.NoError(err, "the test author accidentally formed an invalid authcode secret")
+				r.NoError(kubeInformerClient.Tracker().Add(wrongProviderNameOIDCAuthcodeSessionSecret))
+				r.NoError(kubeClient.Tracker().Add(wrongProviderNameOIDCAuthcodeSessionSecret))
+			})
+
+			it("should remove the secret without revoking any upstream tokens", func() {
+				happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().
+					WithName("upstream-oidc-provider-name").
+					WithResourceUID("upstream-oidc-provider-uid").
+					WithRevokeRefreshTokenError(nil)
+				idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build())
+
+				startInformersAndController(idpListerBuilder.Build())
+				r.NoError(controllerlib.TestSync(t, subject, *syncContext))
+
+				// Nothing to revoke since we couldn't find the upstream in the cache.
+				idpListerBuilder.RequireExactlyZeroCallsToRevokeRefreshToken(t)
+
+				// The authcode session secrets is still deleted because it is expired.
+				r.ElementsMatch(
+					[]kubetesting.Action{
+						kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "wrongProviderNameOIDCAuthcodeSession"),
+					},
+					kubeClient.Actions(),
+				)
+				r.ElementsMatch(
+					[]metav1.DeleteOptions{
+						testutil.NewPreconditions("uid-123", "rv-123"),
+					},
+					*deleteOptions,
+				)
+			})
+		})
+
+		when("there is a valid, expired authcode secret but the upstream revocation fails", func() {
+			it.Before(func() {
+				activeOIDCAuthcodeSession := &authorizationcode.Session{
+					Version: "2",
+					Active:  true,
+					Request: &fosite.Request{
+						ID:     "request-id-1",
+						Client: &clientregistry.Client{},
+						Session: &psession.PinnipedSession{
+							Custom: &psession.CustomSessionData{
+								ProviderUID:  "upstream-oidc-provider-uid",
+								ProviderName: "upstream-oidc-provider-name",
+								ProviderType: psession.ProviderTypeOIDC,
+								OIDC: &psession.OIDCSessionData{
+									UpstreamRefreshToken: "fake-upstream-refresh-token",
+								},
+							},
+						},
+					},
+				}
+				activeOIDCAuthcodeSessionJSON, err := json.Marshal(activeOIDCAuthcodeSession)
+				r.NoError(err)
+				activeOIDCAuthcodeSessionSecret := &corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:            "activeOIDCAuthcodeSession",
+						Namespace:       installedInNamespace,
+						UID:             "uid-123",
+						ResourceVersion: "rv-123",
+						Annotations: map[string]string{
+							"storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339),
+						},
+						Labels: map[string]string{
+							"storage.pinniped.dev/type": authorizationcode.TypeLabelValue,
+						},
+					},
+					Data: map[string][]byte{
+						"pinniped-storage-data":    activeOIDCAuthcodeSessionJSON,
+						"pinniped-storage-version": []byte("1"),
+					},
+					Type: "storage.pinniped.dev/" + authorizationcode.TypeLabelValue,
+				}
+				_, err = authorizationcode.ReadFromSecret(activeOIDCAuthcodeSessionSecret)
+				r.NoError(err, "the test author accidentally formed an invalid authcode secret")
+				r.NoError(kubeInformerClient.Tracker().Add(activeOIDCAuthcodeSessionSecret))
+				r.NoError(kubeClient.Tracker().Add(activeOIDCAuthcodeSessionSecret))
+			})
+
+			it("should remove the secret anyway because it has expired", func() {
+				happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().
+					WithName("upstream-oidc-provider-name").
+					WithResourceUID("upstream-oidc-provider-uid").
+					WithRevokeRefreshTokenError(errors.New("some upstream revocation error")) // the upstream revocation will fail
+				idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build())
+
+				startInformersAndController(idpListerBuilder.Build())
+				r.NoError(controllerlib.TestSync(t, subject, *syncContext))
+
+				// Tried to revoke it, although this revocation will fail.
+				idpListerBuilder.RequireExactlyOneCallToRevokeRefreshToken(t,
+					"upstream-oidc-provider-name",
+					&oidctestutil.RevokeRefreshTokenArgs{
+						Ctx:          syncContext.Context,
+						RefreshToken: "fake-upstream-refresh-token",
+					},
+				)
+
+				// The authcode session secrets is still deleted because it is expired.
+				r.ElementsMatch(
+					[]kubetesting.Action{
+						kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "activeOIDCAuthcodeSession"),
+					},
+					kubeClient.Actions(),
+				)
+				r.ElementsMatch(
+					[]metav1.DeleteOptions{
+						testutil.NewPreconditions("uid-123", "rv-123"),
+					},
+					*deleteOptions,
+				)
+			})
+		})
+
+		when("there are valid, expired access token secrets", func() {
+			it.Before(func() {
+				offlineAccessGrantedOIDCAccessTokenSession := &accesstoken.Session{
+					Version: "2",
+					Request: &fosite.Request{
+						GrantedScope: fosite.Arguments{"scope1", "scope2", "offline_access"},
+						ID:           "request-id-1",
+						Client:       &clientregistry.Client{},
+						Session: &psession.PinnipedSession{
+							Custom: &psession.CustomSessionData{
+								ProviderUID:  "upstream-oidc-provider-uid",
+								ProviderName: "upstream-oidc-provider-name",
+								ProviderType: psession.ProviderTypeOIDC,
+								OIDC: &psession.OIDCSessionData{
+									UpstreamRefreshToken: "offline-access-granted-fake-upstream-refresh-token",
+								},
+							},
+						},
+					},
+				}
+				offlineAccessGrantedOIDCAccessTokenSessionJSON, err := json.Marshal(offlineAccessGrantedOIDCAccessTokenSession)
+				r.NoError(err)
+				offlineAccessGrantedOIDCAccessTokenSessionSecret := &corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:            "offlineAccessGrantedOIDCAccessTokenSession",
+						Namespace:       installedInNamespace,
+						UID:             "uid-123",
+						ResourceVersion: "rv-123",
+						Annotations: map[string]string{
+							"storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339),
+						},
+						Labels: map[string]string{
+							"storage.pinniped.dev/type": accesstoken.TypeLabelValue,
+						},
+					},
+					Data: map[string][]byte{
+						"pinniped-storage-data":    offlineAccessGrantedOIDCAccessTokenSessionJSON,
+						"pinniped-storage-version": []byte("1"),
+					},
+					Type: "storage.pinniped.dev/" + accesstoken.TypeLabelValue,
+				}
+				_, err = accesstoken.ReadFromSecret(offlineAccessGrantedOIDCAccessTokenSessionSecret)
+				r.NoError(err, "the test author accidentally formed an invalid accesstoken secret")
+				r.NoError(kubeInformerClient.Tracker().Add(offlineAccessGrantedOIDCAccessTokenSessionSecret))
+				r.NoError(kubeClient.Tracker().Add(offlineAccessGrantedOIDCAccessTokenSessionSecret))
+
+				offlineAccessNotGrantedOIDCAccessTokenSession := &accesstoken.Session{
+					Version: "2",
+					Request: &fosite.Request{
+						GrantedScope: fosite.Arguments{"scope1", "scope2"},
+						ID:           "request-id-2",
+						Client:       &clientregistry.Client{},
+						Session: &psession.PinnipedSession{
+							Custom: &psession.CustomSessionData{
+								ProviderUID:  "upstream-oidc-provider-uid",
+								ProviderName: "upstream-oidc-provider-name",
+								ProviderType: psession.ProviderTypeOIDC,
+								OIDC: &psession.OIDCSessionData{
+									UpstreamRefreshToken: "fake-upstream-refresh-token",
+								},
+							},
+						},
+					},
+				}
+				offlineAccessNotGrantedOIDCAccessTokenSessionJSON, err := json.Marshal(offlineAccessNotGrantedOIDCAccessTokenSession)
+				r.NoError(err)
+				offlineAccessNotGrantedOIDCAccessTokenSessionSecret := &corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:            "offlineAccessNotGrantedOIDCAccessTokenSession",
+						Namespace:       installedInNamespace,
+						UID:             "uid-456",
+						ResourceVersion: "rv-456",
+						Annotations: map[string]string{
+							"storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339),
+						},
+						Labels: map[string]string{
+							"storage.pinniped.dev/type": accesstoken.TypeLabelValue,
+						},
+					},
+					Data: map[string][]byte{
+						"pinniped-storage-data":    offlineAccessNotGrantedOIDCAccessTokenSessionJSON,
+						"pinniped-storage-version": []byte("1"),
+					},
+					Type: "storage.pinniped.dev/" + accesstoken.TypeLabelValue,
+				}
+				_, err = accesstoken.ReadFromSecret(offlineAccessNotGrantedOIDCAccessTokenSessionSecret)
+				r.NoError(err, "the test author accidentally formed an invalid accesstoken secret")
+				r.NoError(kubeInformerClient.Tracker().Add(offlineAccessNotGrantedOIDCAccessTokenSessionSecret))
+				r.NoError(kubeClient.Tracker().Add(offlineAccessNotGrantedOIDCAccessTokenSessionSecret))
+			})
+
+			it("should revoke upstream tokens only from the active authcode secrets and delete them all", func() {
+				happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().
+					WithName("upstream-oidc-provider-name").
+					WithResourceUID("upstream-oidc-provider-uid").
+					WithRevokeRefreshTokenError(nil)
+				idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build())
+
+				startInformersAndController(idpListerBuilder.Build())
+				r.NoError(controllerlib.TestSync(t, subject, *syncContext))
+
+				// The upstream refresh token is only revoked for the downstream session which had offline_access granted.
+				idpListerBuilder.RequireExactlyOneCallToRevokeRefreshToken(t,
+					"upstream-oidc-provider-name",
+					&oidctestutil.RevokeRefreshTokenArgs{
+						Ctx:          syncContext.Context,
+						RefreshToken: "fake-upstream-refresh-token",
+					},
+				)
+
+				// Both session secrets are deleted.
+				r.ElementsMatch(
+					[]kubetesting.Action{
+						kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "offlineAccessGrantedOIDCAccessTokenSession"),
+						kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "offlineAccessNotGrantedOIDCAccessTokenSession"),
+					},
+					kubeClient.Actions(),
+				)
+				r.ElementsMatch(
+					[]metav1.DeleteOptions{
+						testutil.NewPreconditions("uid-123", "rv-123"),
+						testutil.NewPreconditions("uid-456", "rv-456"),
+					},
+					*deleteOptions,
+				)
+			})
+		})
+
+		when("there are valid, expired refresh secrets", func() {
+			it.Before(func() {
+				oidcRefreshSession := &refreshtoken.Session{
+					Version: "2",
+					Request: &fosite.Request{
+						ID:     "request-id-1",
+						Client: &clientregistry.Client{},
+						Session: &psession.PinnipedSession{
+							Custom: &psession.CustomSessionData{
+								ProviderUID:  "upstream-oidc-provider-uid",
+								ProviderName: "upstream-oidc-provider-name",
+								ProviderType: psession.ProviderTypeOIDC,
+								OIDC: &psession.OIDCSessionData{
+									UpstreamRefreshToken: "fake-upstream-refresh-token",
+								},
+							},
+						},
+					},
+				}
+				oidcRefreshSessionJSON, err := json.Marshal(oidcRefreshSession)
+				r.NoError(err)
+				oidcRefreshSessionSecret := &corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:            "oidcRefreshSession",
+						Namespace:       installedInNamespace,
+						UID:             "uid-123",
+						ResourceVersion: "rv-123",
+						Annotations: map[string]string{
+							"storage.pinniped.dev/garbage-collect-after": frozenNow.Add(-time.Second).Format(time.RFC3339),
+						},
+						Labels: map[string]string{
+							"storage.pinniped.dev/type": refreshtoken.TypeLabelValue,
+						},
+					},
+					Data: map[string][]byte{
+						"pinniped-storage-data":    oidcRefreshSessionJSON,
+						"pinniped-storage-version": []byte("1"),
+					},
+					Type: "storage.pinniped.dev/" + refreshtoken.TypeLabelValue,
+				}
+				_, err = refreshtoken.ReadFromSecret(oidcRefreshSessionSecret)
+				r.NoError(err, "the test author accidentally formed an invalid refresh token secret")
+				r.NoError(kubeInformerClient.Tracker().Add(oidcRefreshSessionSecret))
+				r.NoError(kubeClient.Tracker().Add(oidcRefreshSessionSecret))
+			})
+
+			it("should revoke upstream tokens from the secrets and delete them all", func() {
+				happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().
+					WithName("upstream-oidc-provider-name").
+					WithResourceUID("upstream-oidc-provider-uid").
+					WithRevokeRefreshTokenError(nil)
+				idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build())
+
+				startInformersAndController(idpListerBuilder.Build())
+				r.NoError(controllerlib.TestSync(t, subject, *syncContext))
+
+				// The upstream refresh token is revoked.
+				idpListerBuilder.RequireExactlyOneCallToRevokeRefreshToken(t,
+					"upstream-oidc-provider-name",
+					&oidctestutil.RevokeRefreshTokenArgs{
+						Ctx:          syncContext.Context,
+						RefreshToken: "fake-upstream-refresh-token",
+					},
+				)
+
+				// The secret is deleted.
+				r.ElementsMatch(
+					[]kubetesting.Action{
+						kubetesting.NewDeleteAction(secretsGVR, installedInNamespace, "oidcRefreshSession"),
+					},
+					kubeClient.Actions(),
+				)
+				r.ElementsMatch(
+					[]metav1.DeleteOptions{
+						testutil.NewPreconditions("uid-123", "rv-123"),
+					},
+					*deleteOptions,
+				)
+			})
+		})
+
 		when("very little time has passed since the previous sync call", func() {
 			it.Before(func() {
 				// Add a secret that will expire in 20 seconds.
@@ -279,7 +933,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 			})
 
 			it("should do nothing to avoid being super chatty since it is called for every change to any Secret, until more time has passed", func() {
-				startInformersAndController()
+				startInformersAndController(nil)
 				require.Empty(t, kubeClient.Actions())
 
 				// Run sync once with the current time set to frozenTime.
@@ -344,7 +998,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 			})
 
 			it("does not delete that secret", func() {
-				startInformersAndController()
+				startInformersAndController(nil)
 				r.NoError(controllerlib.TestSync(t, subject, *syncContext))
 
 				r.ElementsMatch(
@@ -393,7 +1047,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 			})
 
 			it("ignores the error and continues on to delete the next expired Secret", func() {
-				startInformersAndController()
+				startInformersAndController(nil)
 				r.NoError(controllerlib.TestSync(t, subject, *syncContext))
 
 				r.ElementsMatch(

internal/fositestorage/accesstoken/accesstoken.go

@@ -10,6 +10,7 @@ import (
 
 	"github.com/ory/fosite"
 	"github.com/ory/fosite/handler/oauth2"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
 
@@ -42,7 +43,7 @@ type accessTokenStorage struct {
 	storage crud.Storage
 }
 
-type session struct {
+type Session struct {
 	Request *fosite.Request `json:"request"`
 	Version string          `json:"version"`
 }
@@ -51,6 +52,23 @@ func New(secrets corev1client.SecretInterface, clock func() time.Time, sessionSt
 	return &accessTokenStorage{storage: crud.New(TypeLabelValue, secrets, clock, sessionStorageLifetime)}
 }
 
+// ReadFromSecret reads the contents of a Secret as a Session.
+func ReadFromSecret(secret *v1.Secret) (*Session, error) {
+	session := newValidEmptyAccessTokenSession()
+	err := crud.FromSecret(TypeLabelValue, secret, session)
+	if err != nil {
+		return nil, err
+	}
+	if session.Version != accessTokenStorageVersion {
+		return nil, fmt.Errorf("%w: access token session has version %s instead of %s",
+			ErrInvalidAccessTokenRequestVersion, session.Version, accessTokenStorageVersion)
+	}
+	if session.Request.ID == "" {
+		return nil, fmt.Errorf("malformed access token session: %w", ErrInvalidAccessTokenRequestData)
+	}
+	return session, nil
+}
+
 func (a *accessTokenStorage) RevokeAccessToken(ctx context.Context, requestID string) error {
 	return a.storage.DeleteByLabel(ctx, fositestorage.StorageRequestIDLabelName, requestID)
 }
@@ -64,7 +82,7 @@ func (a *accessTokenStorage) CreateAccessTokenSession(ctx context.Context, signa
 	_, err = a.storage.Create(
 		ctx,
 		signature,
-		&session{Request: request, Version: accessTokenStorageVersion},
+		&Session{Request: request, Version: accessTokenStorageVersion},
 		map[string]string{fositestorage.StorageRequestIDLabelName: requester.GetID()},
 	)
 	return err
@@ -84,7 +102,7 @@ func (a *accessTokenStorage) DeleteAccessTokenSession(ctx context.Context, signa
 	return a.storage.Delete(ctx, signature)
 }
 
-func (a *accessTokenStorage) getSession(ctx context.Context, signature string) (*session, string, error) {
+func (a *accessTokenStorage) getSession(ctx context.Context, signature string) (*Session, string, error) {
 	session := newValidEmptyAccessTokenSession()
 	rv, err := a.storage.Get(ctx, signature, session)
 
@@ -108,8 +126,8 @@ func (a *accessTokenStorage) getSession(ctx context.Context, signature string) (
 	return session, rv, nil
 }
 
-func newValidEmptyAccessTokenSession() *session {
+func newValidEmptyAccessTokenSession() *Session {
-	return &session{
+	return &Session{
 		Request: &fosite.Request{
 			Client:  &clientregistry.Client{},
 			Session: &psession.PinnipedSession{},

internal/fositestorage/accesstoken/accesstoken_test.go

@@ -10,6 +10,7 @@ import (
 	"time"
 
 	"github.com/ory/fosite"
+	"github.com/ory/fosite/handler/openid"
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
@@ -277,3 +278,119 @@ func makeTestSubject() (context.Context, *fake.Clientset, corev1client.SecretInt
 	secrets := client.CoreV1().Secrets(namespace)
 	return context.Background(), client, secrets, New(secrets, clock.NewFakeClock(fakeNow).Now, lifetime)
 }
+
+func TestReadFromSecret(t *testing.T) {
+	tests := []struct {
+		name        string
+		secret      *corev1.Secret
+		wantSession *Session
+		wantErr     string
+	}{
+		{
+			name: "happy path",
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            "pinniped-storage-access-token-pwu5zs7lekbhnln2w4",
+					ResourceVersion: "",
+					Labels: map[string]string{
+						"storage.pinniped.dev/type": "access-token",
+					},
+				},
+				Data: map[string][]byte{
+					"pinniped-storage-data":    []byte(`{"request":{"id":"abcd-1","session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}}},"version":"2","active": true}`),
+					"pinniped-storage-version": []byte("1"),
+				},
+				Type: "storage.pinniped.dev/access-token",
+			},
+			wantSession: &Session{
+				Version: "2",
+				Request: &fosite.Request{
+					ID:     "abcd-1",
+					Client: &clientregistry.Client{},
+					Session: &psession.PinnipedSession{
+						Fosite: &openid.DefaultSession{
+							Username: "snorlax",
+							Subject:  "panda",
+						},
+						Custom: &psession.CustomSessionData{
+							ProviderUID:  "fake-provider-uid",
+							ProviderName: "fake-provider-name",
+							ProviderType: "fake-provider-type",
+							OIDC: &psession.OIDCSessionData{
+								UpstreamRefreshToken: "fake-upstream-refresh-token",
+							},
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "wrong secret type",
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            "pinniped-storage-access-token-pwu5zs7lekbhnln2w4",
+					ResourceVersion: "",
+					Labels: map[string]string{
+						"storage.pinniped.dev/type": "access-token",
+					},
+				},
+				Data: map[string][]byte{
+					"pinniped-storage-data":    []byte(`{"request":{"id":"abcd-1"},"version":"2","active": true}`),
+					"pinniped-storage-version": []byte("1"),
+				},
+				Type: "storage.pinniped.dev/not-access-token",
+			},
+			wantErr: "secret storage data has incorrect type: storage.pinniped.dev/not-access-token must equal storage.pinniped.dev/access-token",
+		},
+		{
+			name: "wrong session version",
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            "pinniped-storage-access-token-pwu5zs7lekbhnln2w4",
+					ResourceVersion: "",
+					Labels: map[string]string{
+						"storage.pinniped.dev/type": "access-token",
+					},
+				},
+				Data: map[string][]byte{
+					"pinniped-storage-data":    []byte(`{"request":{"id":"abcd-1"},"version":"wrong-version-here","active": true}`),
+					"pinniped-storage-version": []byte("1"),
+				},
+				Type: "storage.pinniped.dev/access-token",
+			},
+			wantErr: "access token request data has wrong version: access token session has version wrong-version-here instead of 2",
+		},
+		{
+			name: "missing request",
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            "pinniped-storage-access-token-pwu5zs7lekbhnln2w4",
+					ResourceVersion: "",
+					Labels: map[string]string{
+						"storage.pinniped.dev/type": "access-token",
+					},
+				},
+				Data: map[string][]byte{
+					"pinniped-storage-data":    []byte(`{"version":"2","active": true}`),
+					"pinniped-storage-version": []byte("1"),
+				},
+				Type: "storage.pinniped.dev/access-token",
+			},
+			wantErr: "malformed access token session: access token request data must be present",
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			session, err := ReadFromSecret(tt.secret)
+			if tt.wantErr == "" {
+				require.NoError(t, err)
+				require.Equal(t, tt.wantSession, session)
+			} else {
+				require.EqualError(t, err, tt.wantErr)
+				require.Nil(t, session)
+			}
+		})
+	}
+}

internal/fositestorage/authorizationcode/authorizationcode.go

@@ -39,7 +39,7 @@ type authorizeCodeStorage struct {
 	storage crud.Storage
 }
 
-type AuthorizeCodeSession struct {
+type Session struct {
 	Active  bool            `json:"active"`
 	Request *fosite.Request `json:"request"`
 	Version string          `json:"version"`
@@ -49,8 +49,8 @@ func New(secrets corev1client.SecretInterface, clock func() time.Time, sessionSt
 	return &authorizeCodeStorage{storage: crud.New(TypeLabelValue, secrets, clock, sessionStorageLifetime)}
 }
 
-// ReadFromSecret reads the contents of a Secret as an AuthorizeCodeSession.
+// ReadFromSecret reads the contents of a Secret as a Session.
-func ReadFromSecret(secret *v1.Secret) (*AuthorizeCodeSession, error) {
+func ReadFromSecret(secret *v1.Secret) (*Session, error) {
 	session := NewValidEmptyAuthorizeCodeSession()
 	err := crud.FromSecret(TypeLabelValue, secret, session)
 	if err != nil {
@@ -88,7 +88,7 @@ func (a *authorizeCodeStorage) CreateAuthorizeCodeSession(ctx context.Context, s
 	//      of the consent authorization request. It is used to identify the session.
 	//  signature for lookup in the DB
 
-	_, err = a.storage.Create(ctx, signature, &AuthorizeCodeSession{Active: true, Request: request, Version: authorizeCodeStorageVersion}, nil)
+	_, err = a.storage.Create(ctx, signature, &Session{Active: true, Request: request, Version: authorizeCodeStorageVersion}, nil)
 	return err
 }
 
@@ -126,7 +126,7 @@ func (a *authorizeCodeStorage) InvalidateAuthorizeCodeSession(ctx context.Contex
 	return nil
 }
 
-func (a *authorizeCodeStorage) getSession(ctx context.Context, signature string) (*AuthorizeCodeSession, string, error) {
+func (a *authorizeCodeStorage) getSession(ctx context.Context, signature string) (*Session, string, error) {
 	session := NewValidEmptyAuthorizeCodeSession()
 	rv, err := a.storage.Get(ctx, signature, session)
 
@@ -155,8 +155,8 @@ func (a *authorizeCodeStorage) getSession(ctx context.Context, signature string)
 	return session, rv, nil
 }
 
-func NewValidEmptyAuthorizeCodeSession() *AuthorizeCodeSession {
+func NewValidEmptyAuthorizeCodeSession() *Session {
-	return &AuthorizeCodeSession{
+	return &Session{
 		Request: &fosite.Request{
 			Client:  &clientregistry.Client{},
 			Session: &psession.PinnipedSession{},

internal/fositestorage/authorizationcode/authorizationcode_test.go

@@ -403,7 +403,7 @@ func TestReadFromSecret(t *testing.T) {
 	tests := []struct {
 		name        string
 		secret      *corev1.Secret
-		wantSession *AuthorizeCodeSession
+		wantSession *Session
 		wantErr     string
 	}{
 		{
@@ -422,7 +422,7 @@ func TestReadFromSecret(t *testing.T) {
 				},
 				Type: "storage.pinniped.dev/authcode",
 			},
-			wantSession: &AuthorizeCodeSession{
+			wantSession: &Session{
 				Version: "2",
 				Active:  true,
 				Request: &fosite.Request{

internal/fositestorage/refreshtoken/refreshtoken.go

@@ -10,6 +10,7 @@ import (
 
 	"github.com/ory/fosite"
 	"github.com/ory/fosite/handler/oauth2"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
 
@@ -42,7 +43,7 @@ type refreshTokenStorage struct {
 	storage crud.Storage
 }
 
-type session struct {
+type Session struct {
 	Request *fosite.Request `json:"request"`
 	Version string          `json:"version"`
 }
@@ -51,6 +52,23 @@ func New(secrets corev1client.SecretInterface, clock func() time.Time, sessionSt
 	return &refreshTokenStorage{storage: crud.New(TypeLabelValue, secrets, clock, sessionStorageLifetime)}
 }
 
+// ReadFromSecret reads the contents of a Secret as a Session.
+func ReadFromSecret(secret *v1.Secret) (*Session, error) {
+	session := newValidEmptyRefreshTokenSession()
+	err := crud.FromSecret(TypeLabelValue, secret, session)
+	if err != nil {
+		return nil, err
+	}
+	if session.Version != refreshTokenStorageVersion {
+		return nil, fmt.Errorf("%w: refresh token session has version %s instead of %s",
+			ErrInvalidRefreshTokenRequestVersion, session.Version, refreshTokenStorageVersion)
+	}
+	if session.Request.ID == "" {
+		return nil, fmt.Errorf("malformed refresh token session: %w", ErrInvalidRefreshTokenRequestData)
+	}
+	return session, nil
+}
+
 func (a *refreshTokenStorage) RevokeRefreshToken(ctx context.Context, requestID string) error {
 	return a.storage.DeleteByLabel(ctx, fositestorage.StorageRequestIDLabelName, requestID)
 }
@@ -64,7 +82,7 @@ func (a *refreshTokenStorage) CreateRefreshTokenSession(ctx context.Context, sig
 	_, err = a.storage.Create(
 		ctx,
 		signature,
-		&session{Request: request, Version: refreshTokenStorageVersion},
+		&Session{Request: request, Version: refreshTokenStorageVersion},
 		map[string]string{fositestorage.StorageRequestIDLabelName: requester.GetID()},
 	)
 	return err
@@ -84,7 +102,7 @@ func (a *refreshTokenStorage) DeleteRefreshTokenSession(ctx context.Context, sig
 	return a.storage.Delete(ctx, signature)
 }
 
-func (a *refreshTokenStorage) getSession(ctx context.Context, signature string) (*session, string, error) {
+func (a *refreshTokenStorage) getSession(ctx context.Context, signature string) (*Session, string, error) {
 	session := newValidEmptyRefreshTokenSession()
 	rv, err := a.storage.Get(ctx, signature, session)
 
@@ -108,8 +126,8 @@ func (a *refreshTokenStorage) getSession(ctx context.Context, signature string)
 	return session, rv, nil
 }
 
-func newValidEmptyRefreshTokenSession() *session {
+func newValidEmptyRefreshTokenSession() *Session {
-	return &session{
+	return &Session{
 		Request: &fosite.Request{
 			Client:  &clientregistry.Client{},
 			Session: &psession.PinnipedSession{},

internal/fositestorage/refreshtoken/refreshtoken_test.go

@@ -10,6 +10,7 @@ import (
 	"time"
 
 	"github.com/ory/fosite"
+	"github.com/ory/fosite/handler/openid"
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/require"
 	corev1 "k8s.io/api/core/v1"
@@ -277,3 +278,119 @@ func makeTestSubject() (context.Context, *fake.Clientset, corev1client.SecretInt
 	secrets := client.CoreV1().Secrets(namespace)
 	return context.Background(), client, secrets, New(secrets, clock.NewFakeClock(fakeNow).Now, lifetime)
 }
+
+func TestReadFromSecret(t *testing.T) {
+	tests := []struct {
+		name        string
+		secret      *corev1.Secret
+		wantSession *Session
+		wantErr     string
+	}{
+		{
+			name: "happy path",
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4",
+					ResourceVersion: "",
+					Labels: map[string]string{
+						"storage.pinniped.dev/type": "refresh-token",
+					},
+				},
+				Data: map[string][]byte{
+					"pinniped-storage-data":    []byte(`{"request":{"id":"abcd-1","session":{"fosite":{"Claims":null,"Headers":null,"ExpiresAt":null,"Username":"snorlax","Subject":"panda"},"custom":{"providerUID":"fake-provider-uid","providerName":"fake-provider-name","providerType":"fake-provider-type","oidc":{"upstreamRefreshToken":"fake-upstream-refresh-token"}}}},"version":"2","active": true}`),
+					"pinniped-storage-version": []byte("1"),
+				},
+				Type: "storage.pinniped.dev/refresh-token",
+			},
+			wantSession: &Session{
+				Version: "2",
+				Request: &fosite.Request{
+					ID:     "abcd-1",
+					Client: &clientregistry.Client{},
+					Session: &psession.PinnipedSession{
+						Fosite: &openid.DefaultSession{
+							Username: "snorlax",
+							Subject:  "panda",
+						},
+						Custom: &psession.CustomSessionData{
+							ProviderUID:  "fake-provider-uid",
+							ProviderName: "fake-provider-name",
+							ProviderType: "fake-provider-type",
+							OIDC: &psession.OIDCSessionData{
+								UpstreamRefreshToken: "fake-upstream-refresh-token",
+							},
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "wrong secret type",
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4",
+					ResourceVersion: "",
+					Labels: map[string]string{
+						"storage.pinniped.dev/type": "refresh-token",
+					},
+				},
+				Data: map[string][]byte{
+					"pinniped-storage-data":    []byte(`{"request":{"id":"abcd-1"},"version":"2","active": true}`),
+					"pinniped-storage-version": []byte("1"),
+				},
+				Type: "storage.pinniped.dev/not-refresh-token",
+			},
+			wantErr: "secret storage data has incorrect type: storage.pinniped.dev/not-refresh-token must equal storage.pinniped.dev/refresh-token",
+		},
+		{
+			name: "wrong session version",
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4",
+					ResourceVersion: "",
+					Labels: map[string]string{
+						"storage.pinniped.dev/type": "refresh-token",
+					},
+				},
+				Data: map[string][]byte{
+					"pinniped-storage-data":    []byte(`{"request":{"id":"abcd-1"},"version":"wrong-version-here","active": true}`),
+					"pinniped-storage-version": []byte("1"),
+				},
+				Type: "storage.pinniped.dev/refresh-token",
+			},
+			wantErr: "refresh token request data has wrong version: refresh token session has version wrong-version-here instead of 2",
+		},
+		{
+			name: "missing request",
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:            "pinniped-storage-refresh-token-pwu5zs7lekbhnln2w4",
+					ResourceVersion: "",
+					Labels: map[string]string{
+						"storage.pinniped.dev/type": "refresh-token",
+					},
+				},
+				Data: map[string][]byte{
+					"pinniped-storage-data":    []byte(`{"version":"2","active": true}`),
+					"pinniped-storage-version": []byte("1"),
+				},
+				Type: "storage.pinniped.dev/refresh-token",
+			},
+			wantErr: "malformed refresh token session: refresh token request data must be present",
+		},
+	}
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			session, err := ReadFromSecret(tt.secret)
+			if tt.wantErr == "" {
+				require.NoError(t, err)
+				require.Equal(t, tt.wantSession, session)
+			} else {
+				require.EqualError(t, err, tt.wantErr)
+				require.Nil(t, session)
+			}
+		})
+	}
+}

internal/testutil/oidctestutil/oidctestutil.go

@@ -495,6 +495,43 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToValidateToken(t *tes
 	)
 }
 
+func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToRevokeRefreshToken(
+	t *testing.T,
+	expectedPerformedByUpstreamName string,
+	expectedArgs *RevokeRefreshTokenArgs,
+) {
+	t.Helper()
+	var actualArgs *RevokeRefreshTokenArgs
+	var actualNameOfUpstreamWhichMadeCall string
+	actualCallCountAcrossAllOIDCUpstreams := 0
+	for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders {
+		callCountOnThisUpstream := upstreamOIDC.revokeRefreshTokenCallCount
+		actualCallCountAcrossAllOIDCUpstreams += callCountOnThisUpstream
+		if callCountOnThisUpstream == 1 {
+			actualNameOfUpstreamWhichMadeCall = upstreamOIDC.Name
+			actualArgs = upstreamOIDC.revokeRefreshTokenArgs[0]
+		}
+	}
+	require.Equal(t, 1, actualCallCountAcrossAllOIDCUpstreams,
+		"should have been exactly one call to RevokeRefreshToken() by all OIDC upstreams",
+	)
+	require.Equal(t, expectedPerformedByUpstreamName, actualNameOfUpstreamWhichMadeCall,
+		"RevokeRefreshToken() was called on the wrong OIDC upstream",
+	)
+	require.Equal(t, expectedArgs, actualArgs)
+}
+
+func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToRevokeRefreshToken(t *testing.T) {
+	t.Helper()
+	actualCallCountAcrossAllOIDCUpstreams := 0
+	for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders {
+		actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.revokeRefreshTokenCallCount
+	}
+	require.Equal(t, 0, actualCallCountAcrossAllOIDCUpstreams,
+		"expected exactly zero calls to RevokeRefreshToken()",
+	)
+}
+
 func NewUpstreamIDPListerBuilder() *UpstreamIDPListerBuilder {
 	return &UpstreamIDPListerBuilder{}
 }
@@ -633,6 +670,11 @@ func (u *TestUpstreamOIDCIdentityProviderBuilder) WithValidateTokenError(err err
 	return u
 }
 
+func (u *TestUpstreamOIDCIdentityProviderBuilder) WithRevokeRefreshTokenError(err error) *TestUpstreamOIDCIdentityProviderBuilder {
+	u.revokeRefreshTokenErr = err
+	return u
+}
+
 func (u *TestUpstreamOIDCIdentityProviderBuilder) Build() *TestUpstreamOIDCIdentityProvider {
 	return &TestUpstreamOIDCIdentityProvider{
 		Name:                     u.name,