Merge branch 'main' into ldap_and_activedirectory_status_conditions_bug

Dockerfile

@@ -1,6 +1,6 @@
 # syntax = docker/dockerfile:1.0-experimental
 
-# Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+# Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 # SPDX-License-Identifier: Apache-2.0
 
 FROM golang:1.17.6 as build-env

apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go.tmpl

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package v1alpha1

cmd/pinniped/cmd/kubeconfig_test.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package cmd

cmd/pinniped/cmd/login_oidc_test.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package cmd

cmd/pinniped/cmd/login_static_test.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package cmd

cmd/pinniped/cmd/root.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package cmd

generated/1.17/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package v1alpha1

generated/1.18/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package v1alpha1

generated/1.19/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package v1alpha1

generated/1.20/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package v1alpha1

generated/latest/apis/supervisor/idp/v1alpha1/types_oidcidentityprovider.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package v1alpha1

hack/header.txt

@@ -1,2 +1,2 @@
-Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 SPDX-License-Identifier: Apache-2.0

internal/authenticators/authenticators.go

@@ -1,4 +1,4 @@
-// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package authenticators contains authenticator interfaces.

internal/concierge/impersonator/impersonator.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package impersonator

internal/concierge/impersonator/impersonator_test.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package impersonator

internal/concierge/server/server.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package server is the command line entry point for pinniped-concierge.

internal/controller/apicerts/certs_expirer_test.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package apicerts

internal/controller/authenticator/cachecleaner/cachecleaner_test.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package cachecleaner

internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package jwtcachefiller

internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package webhookcachefiller

internal/controller/impersonatorconfig/impersonator_config.go

@@ -1,4 +1,4 @@
-// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package impersonatorconfig

internal/controller/impersonatorconfig/impersonator_config_test.go

@@ -1,4 +1,4 @@
-// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package impersonatorconfig

internal/controller/kubecertagent/kubecertagent.go

@@ -1,4 +1,4 @@
-// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package kubecertagent provides controllers that ensure a pod (the kube-cert-agent), is

internal/controller/kubecertagent/kubecertagent_test.go

@@ -1,4 +1,4 @@
-// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package kubecertagent

internal/controller/kubecertagent/legacypodcleaner_test.go

@@ -1,4 +1,4 @@
-// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package kubecertagent

internal/controller/kubecertagent/mocks/mockdynamiccert.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 //
 

internal/controller/kubecertagent/mocks/mockpodcommandexecutor.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 //
 

internal/controller/supervisorconfig/federation_domain_watcher.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package supervisorconfig

internal/controller/supervisorconfig/federation_domain_watcher_test.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package supervisorconfig

internal/controller/supervisorstorage/garbage_collector.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package supervisorstorage
@@ -113,25 +113,30 @@ func (c *garbageCollectorController) Sync(ctx controllerlib.Context) error {
 
 		timeString, ok := secret.Annotations[crud.SecretLifetimeAnnotationKey]
 		if !ok {
+			// Secret did not request garbage collection via annotations, so skip deletion.
 			continue
 		}
 
 		garbageCollectAfterTime, err := time.Parse(crud.SecretLifetimeAnnotationDateFormat, timeString)
 		if err != nil {
 			plog.WarningErr("could not parse resource timestamp for garbage collection", err, logKV(secret)...)
+			// Can't tell if the Secret has expired or not, so skip deletion.
 			continue
 		}
 
 		if !garbageCollectAfterTime.Before(frozenClock.Now()) {
-			// not old enough yet
+			// Secret is not old enough yet, so skip deletion.
 			continue
 		}
 
+		// The Secret has expired. Check if it is a downstream session storage Secret, which may require extra processing.
 		storageType, isSessionStorage := secret.Labels[crud.SecretLabelKey]
 		if isSessionStorage {
-			err := c.maybeRevokeUpstreamOIDCRefreshToken(ctx.Context, storageType, secret)
+			revokeErr := c.maybeRevokeUpstreamOIDCToken(ctx.Context, storageType, secret)
-			if err != nil {
+			if revokeErr != nil {
-				plog.WarningErr("garbage collector could not revoke upstream refresh token", err, logKV(secret)...)
+				plog.WarningErr("garbage collector could not revoke upstream OIDC token", revokeErr, logKV(secret)...)
+				// Note that RevokeToken (called by the private helper) might have returned an error of type
+				// provider.RetryableRevocationError, in which case we would like to retry the revocation later.
 				// If the error is of a type that is worth retrying, then do not delete the Secret right away.
 				// A future call to Sync will try revocation again for that secret. However, if the Secret is
 				// getting too old, then just delete it anyway. We don't want to extend the lifetime of these
@@ -139,13 +144,15 @@ func (c *garbageCollectorController) Sync(ctx controllerlib.Context) error {
 				// cleaning them out of etcd storage.
 				fourHoursAgo := frozenClock.Now().Add(-4 * time.Hour)
 				nowIsLessThanFourHoursBeyondSecretGCTime := garbageCollectAfterTime.After(fourHoursAgo)
-				if errors.As(err, &retryableRevocationError{}) && nowIsLessThanFourHoursBeyondSecretGCTime {
+				if errors.As(revokeErr, &provider.RetryableRevocationError{}) && nowIsLessThanFourHoursBeyondSecretGCTime {
 					// Hasn't been very long since secret expired, so skip deletion to try revocation again later.
+					plog.Trace("garbage collector keeping Secret to retry upstream OIDC token revocation later", logKV(secret)...)
 					continue
 				}
 			}
 		}
 
+		// Garbage collect the Secret.
 		err = c.kubeClient.CoreV1().Secrets(secret.Namespace).Delete(ctx.Context, secret.Name, metav1.DeleteOptions{
 			Preconditions: &metav1.Preconditions{
 				UID:             &secret.UID,
@@ -162,30 +169,36 @@ func (c *garbageCollectorController) Sync(ctx controllerlib.Context) error {
 	return nil
 }
 
-func (c *garbageCollectorController) maybeRevokeUpstreamOIDCRefreshToken(ctx context.Context, storageType string, secret *v1.Secret) error {
+func (c *garbageCollectorController) maybeRevokeUpstreamOIDCToken(ctx context.Context, storageType string, secret *v1.Secret) error {
-	// All session storage types hold upstream refresh tokens when the upstream IDP is an OIDC provider.
+	// All downstream session storage types hold upstream 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.
+	// session, and only the latest, or to revoke the original upstream access token. Note that we don't
+	// bother to store new upstream access tokens seen during upstream refresh because we only need to store
+	// the upstream access token when we intend to use it *instead* of an upstream refresh token.
+	// This implies that all the storage types will contain a copy of the original upstream access token,
+	// since it is never updated in the session. Thus, we can use the same logic to decide which upstream
+	// access token to revoke as we use for upstream refresh tokens, which allows us to avoid revoking an
+	// upstream access token more than once.
 	switch storageType {
 	case authorizationcode.TypeLabelValue:
 		authorizeCodeSession, err := authorizationcode.ReadFromSecret(secret)
 		if err != nil {
 			return err
 		}
-		// Check if this downstream authcode was already used. If it was already used (i.e. not active anymore), then
+		// Check if this downstream authcode was already used. If it was already used (i.e. not active anymore),
-		// the latest upstream refresh token can be found in one of the other storage types handled below instead.
+		// then the latest upstream token can be found in one of the other storage types handled below instead.
 		if !authorizeCodeSession.Active {
 			return nil
 		}
-		// When the downstream authcode was never used, then its storage must contain the latest upstream refresh token.
+		// When the downstream authcode was never used, then its storage must contain the latest upstream token.
-		return c.revokeUpstreamOIDCRefreshToken(ctx, authorizeCodeSession.Request.Session.(*psession.PinnipedSession).Custom, secret)
+		return c.tryRevokeUpstreamOIDCToken(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 session.
-		// If it was granted, then the latest upstream refresh token should be found in the refresh token storage instead.
+		// If it was granted, then the latest upstream token should be found in the refresh token storage instead.
 		// If it was not granted, then the user could not possibly have performed a downstream refresh, so the
-		// access token storage has the latest version of the upstream refresh token.
+		// access token storage has the latest version of the upstream token.
 		accessTokenSession, err := accesstoken.ReadFromSecret(secret)
 		if err != nil {
 			return err
@@ -194,29 +207,29 @@ func (c *garbageCollectorController) maybeRevokeUpstreamOIDCRefreshToken(ctx con
 		if slices.Contains(accessTokenSession.Request.GetGrantedScopes(), coreosoidc.ScopeOfflineAccess) {
 			return nil
 		}
-		return c.revokeUpstreamOIDCRefreshToken(ctx, pinnipedSession.Custom, secret)
+		return c.tryRevokeUpstreamOIDCToken(ctx, pinnipedSession.Custom, secret)
 
 	case refreshtoken.TypeLabelValue:
-		// For refresh token storage, always revoke its upstream refresh token. This refresh token storage could
+		// For refresh token storage, always revoke its upstream token. This refresh token storage could be
-		// be the result of the initial downstream authcode exchange, or it could be the result of a downstream
+		// the result of the initial downstream authcode exchange, or it could be the result of a downstream
-		// refresh. Either way, it always contains the latest upstream refresh token when it exists.
+		// refresh. Either way, it always contains the latest upstream token when it exists.
 		refreshTokenSession, err := refreshtoken.ReadFromSecret(secret)
 		if err != nil {
 			return err
 		}
-		return c.revokeUpstreamOIDCRefreshToken(ctx, refreshTokenSession.Request.Session.(*psession.PinnipedSession).Custom, secret)
+		return c.tryRevokeUpstreamOIDCToken(ctx, refreshTokenSession.Request.Session.(*psession.PinnipedSession).Custom, secret)
 
 	case pkce.TypeLabelValue:
-		// For PKCE storage, its very existence means that the authcode was never exchanged, because these
+		// For PKCE storage, its very existence means that the downstream authcode was never exchanged, because
-		// are deleted during authcode exchange. No need to do anything, since the upstream refresh token
+		// these are deleted during downstream authcode exchange. No need to do anything, since the upstream
-		// revocation is handled by authcode storage case above.
+		// 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
+		// These are not deleted during downstream authcode exchange, probably due to a bug in fosite, even
-		// will never be read or updated again. However, the refresh token contained inside will be revoked
+		// though it will never be read or updated again. However, the upstream token contained inside will
-		// by one of the other cases above.
+		// be revoked by one of the other cases above.
 		return nil
 
 	default:
@@ -225,8 +238,8 @@ func (c *garbageCollectorController) maybeRevokeUpstreamOIDCRefreshToken(ctx con
 	}
 }
 
-func (c *garbageCollectorController) revokeUpstreamOIDCRefreshToken(ctx context.Context, customSessionData *psession.CustomSessionData, secret *v1.Secret) error {
+func (c *garbageCollectorController) tryRevokeUpstreamOIDCToken(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.
+	// When session was for another upstream IDP type, e.g. LDAP, there is no upstream OIDC token involved.
 	if customSessionData.ProviderType != psession.ProviderTypeOIDC {
 		return nil
 	}
@@ -243,32 +256,29 @@ func (c *garbageCollectorController) revokeUpstreamOIDCRefreshToken(ctx context.
 		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.
+	// In practice, there should only be one of these tokens saved in the session.
-	err := foundOIDCIdentityProviderI.RevokeRefreshToken(ctx, customSessionData.OIDC.UpstreamRefreshToken)
+	upstreamRefreshToken := customSessionData.OIDC.UpstreamRefreshToken
+	upstreamAccessToken := customSessionData.OIDC.UpstreamAccessToken
+
+	if upstreamRefreshToken != "" {
+		err := foundOIDCIdentityProviderI.RevokeToken(ctx, upstreamRefreshToken, provider.RefreshTokenType)
 		if err != nil {
-		// This could be a network failure, a 503 result which we should retry
+			return err
-		// (see https://datatracker.ietf.org/doc/html/rfc7009#section-2.2.1),
+		}
-		// or any other non-200 response from the revocation endpoint.
+		plog.Trace("garbage collector successfully revoked upstream OIDC refresh token (or provider has no revocation endpoint)", logKV(secret)...)
-		// Regardless of which, it is probably worth retrying.
+	}
-		return retryableRevocationError{wrapped: err}
+
+	if upstreamAccessToken != "" {
+		err := foundOIDCIdentityProviderI.RevokeToken(ctx, upstreamAccessToken, provider.AccessTokenType)
+		if err != nil {
+			return err
+		}
+		plog.Trace("garbage collector successfully revoked upstream OIDC access token (or provider has no revocation endpoint)", logKV(secret)...)
 	}
 
-	plog.Trace("garbage collector successfully revoked upstream OIDC refresh token (or provider has no revocation endpoint)", logKV(secret)...)
 	return nil
 }
 
-type retryableRevocationError struct {
-	wrapped error
-}
-
-func (e retryableRevocationError) Error() string {
-	return fmt.Sprintf("retryable revocation error: %v", e.wrapped)
-}
-
-func (e retryableRevocationError) Unwrap() error {
-	return e.wrapped
-}
-
 func logKV(secret *v1.Secret) []interface{} {
 	return []interface{}{
 		"secretName", secret.Name,

internal/controller/supervisorstorage/garbage_collector_test.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package supervisorstorage
@@ -260,7 +260,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 			})
 		})
 
-		when("there are valid, expired authcode secrets", func() {
+		when("there are valid, expired authcode secrets which contain upstream refresh tokens", func() {
 			it.Before(func() {
 				activeOIDCAuthcodeSession := &authorizationcode.Session{
 					Version: "2",
@@ -355,18 +355,141 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 				happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().
 					WithName("upstream-oidc-provider-name").
 					WithResourceUID("upstream-oidc-provider-uid").
-					WithRevokeRefreshTokenError(nil)
+					WithRevokeTokenError(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,
+				idpListerBuilder.RequireExactlyOneCallToRevokeToken(t,
 					"upstream-oidc-provider-name",
-					&oidctestutil.RevokeRefreshTokenArgs{
+					&oidctestutil.RevokeTokenArgs{
 						Ctx:       syncContext.Context,
-						RefreshToken: "fake-upstream-refresh-token",
+						Token:     "fake-upstream-refresh-token",
+						TokenType: provider.RefreshTokenType,
+					},
+				)
+
+				// Both authcode session secrets are deleted.
+				r.ElementsMatch(
+					[]kubetesting.Action{
+						kubetesting.NewDeleteActionWithOptions(secretsGVR, installedInNamespace, "activeOIDCAuthcodeSession", testutil.NewPreconditions("uid-123", "rv-123")),
+						kubetesting.NewDeleteActionWithOptions(secretsGVR, installedInNamespace, "inactiveOIDCAuthcodeSession", testutil.NewPreconditions("uid-456", "rv-456")),
+					},
+					kubeClient.Actions(),
+				)
+			})
+		})
+
+		when("there are valid, expired authcode secrets which contain upstream access tokens", 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{
+									UpstreamAccessToken: "fake-upstream-access-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{
+									UpstreamAccessToken: "other-fake-upstream-access-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").
+					WithRevokeTokenError(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.RequireExactlyOneCallToRevokeToken(t,
+					"upstream-oidc-provider-name",
+					&oidctestutil.RevokeTokenArgs{
+						Ctx:       syncContext.Context,
+						Token:     "fake-upstream-access-token",
+						TokenType: provider.AccessTokenType,
 					},
 				)
 
@@ -430,14 +553,14 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 				happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().
 					WithName("upstream-oidc-provider-name").
 					WithResourceUID("upstream-oidc-provider-uid").
-					WithRevokeRefreshTokenError(nil)
+					WithRevokeTokenError(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)
+				idpListerBuilder.RequireExactlyZeroCallsToRevokeToken(t)
 
 				// The invalid authcode session secrets is still deleted because it is expired.
 				r.ElementsMatch(
@@ -500,14 +623,14 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 				happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().
 					WithName("upstream-oidc-provider-name").
 					WithResourceUID("upstream-oidc-provider-uid").
-					WithRevokeRefreshTokenError(nil)
+					WithRevokeTokenError(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)
+				idpListerBuilder.RequireExactlyZeroCallsToRevokeToken(t)
 
 				// The authcode session secrets is still deleted because it is expired.
 				r.ElementsMatch(
@@ -570,14 +693,14 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 				happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().
 					WithName("upstream-oidc-provider-name").
 					WithResourceUID("upstream-oidc-provider-uid").
-					WithRevokeRefreshTokenError(nil)
+					WithRevokeTokenError(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)
+				idpListerBuilder.RequireExactlyZeroCallsToRevokeToken(t)
 
 				// The authcode session secrets is still deleted because it is expired.
 				r.ElementsMatch(
@@ -637,28 +760,60 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 				r.NoError(kubeClient.Tracker().Add(activeOIDCAuthcodeSessionSecret))
 			})
 
-			it("keeps the secret for a while longer so the revocation can be retried on a future sync", func() {
+			it("keeps the secret for a while longer so the revocation can be retried on a future sync for retryable errors", 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
+					// make the upstream revocation fail in a retryable way
+					WithRevokeTokenError(provider.NewRetryableRevocationError(errors.New("some retryable upstream revocation error")))
 				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,
+				idpListerBuilder.RequireExactlyOneCallToRevokeToken(t,
 					"upstream-oidc-provider-name",
-					&oidctestutil.RevokeRefreshTokenArgs{
+					&oidctestutil.RevokeTokenArgs{
 						Ctx:       syncContext.Context,
-						RefreshToken: "fake-upstream-refresh-token",
+						Token:     "fake-upstream-refresh-token",
+						TokenType: provider.RefreshTokenType,
 					},
 				)
 
 				// The authcode session secrets is not deleted.
 				r.Empty(kubeClient.Actions())
 			})
+
+			it("deletes the secret for non-retryable errors", func() {
+				happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().
+					WithName("upstream-oidc-provider-name").
+					WithResourceUID("upstream-oidc-provider-uid").
+					// make the upstream revocation fail in a non-retryable way
+					WithRevokeTokenError(errors.New("some upstream revocation error not worth retrying"))
+				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.RequireExactlyOneCallToRevokeToken(t,
+					"upstream-oidc-provider-name",
+					&oidctestutil.RevokeTokenArgs{
+						Ctx:       syncContext.Context,
+						Token:     "fake-upstream-refresh-token",
+						TokenType: provider.RefreshTokenType,
+					},
+				)
+
+				// The authcode session secrets is still deleted because it is expired and the revocation error is not retryable.
+				r.ElementsMatch(
+					[]kubetesting.Action{
+						kubetesting.NewDeleteActionWithOptions(secretsGVR, installedInNamespace, "activeOIDCAuthcodeSession", testutil.NewPreconditions("uid-123", "rv-123")),
+					},
+					kubeClient.Actions(),
+				)
+			})
 		})
 
 		when("there is a valid, long-since expired authcode secret but the upstream revocation fails", func() {
@@ -713,18 +868,19 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 				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
+					WithRevokeTokenError(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,
+				idpListerBuilder.RequireExactlyOneCallToRevokeToken(t,
 					"upstream-oidc-provider-name",
-					&oidctestutil.RevokeRefreshTokenArgs{
+					&oidctestutil.RevokeTokenArgs{
 						Ctx:       syncContext.Context,
-						RefreshToken: "fake-upstream-refresh-token",
+						Token:     "fake-upstream-refresh-token",
+						TokenType: provider.RefreshTokenType,
 					},
 				)
 
@@ -738,7 +894,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 			})
 		})
 
-		when("there are valid, expired access token secrets", func() {
+		when("there are valid, expired access token secrets which contain upstream refresh tokens", func() {
 			it.Before(func() {
 				offlineAccessGrantedOIDCAccessTokenSession := &accesstoken.Session{
 					Version: "2",
@@ -833,18 +989,19 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 				happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().
 					WithName("upstream-oidc-provider-name").
 					WithResourceUID("upstream-oidc-provider-uid").
-					WithRevokeRefreshTokenError(nil)
+					WithRevokeTokenError(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,
+				idpListerBuilder.RequireExactlyOneCallToRevokeToken(t,
 					"upstream-oidc-provider-name",
-					&oidctestutil.RevokeRefreshTokenArgs{
+					&oidctestutil.RevokeTokenArgs{
 						Ctx:       syncContext.Context,
-						RefreshToken: "fake-upstream-refresh-token",
+						Token:     "fake-upstream-refresh-token",
+						TokenType: provider.RefreshTokenType,
 					},
 				)
 
@@ -859,7 +1016,129 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 			})
 		})
 
-		when("there are valid, expired refresh secrets", func() {
+		when("there are valid, expired access token secrets which contain upstream access tokens", 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{
+									UpstreamAccessToken: "offline-access-granted-fake-upstream-access-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{
+									UpstreamAccessToken: "fake-upstream-access-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").
+					WithRevokeTokenError(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.RequireExactlyOneCallToRevokeToken(t,
+					"upstream-oidc-provider-name",
+					&oidctestutil.RevokeTokenArgs{
+						Ctx:       syncContext.Context,
+						Token:     "fake-upstream-access-token",
+						TokenType: provider.AccessTokenType,
+					},
+				)
+
+				// Both session secrets are deleted.
+				r.ElementsMatch(
+					[]kubetesting.Action{
+						kubetesting.NewDeleteActionWithOptions(secretsGVR, installedInNamespace, "offlineAccessGrantedOIDCAccessTokenSession", testutil.NewPreconditions("uid-123", "rv-123")),
+						kubetesting.NewDeleteActionWithOptions(secretsGVR, installedInNamespace, "offlineAccessNotGrantedOIDCAccessTokenSession", testutil.NewPreconditions("uid-456", "rv-456")),
+					},
+					kubeClient.Actions(),
+				)
+			})
+		})
+
+		when("there are valid, expired refresh secrets which contain upstream refresh tokens", func() {
 			it.Before(func() {
 				oidcRefreshSession := &refreshtoken.Session{
 					Version: "2",
@@ -909,18 +1188,95 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 				happyOIDCUpstream := oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().
 					WithName("upstream-oidc-provider-name").
 					WithResourceUID("upstream-oidc-provider-uid").
-					WithRevokeRefreshTokenError(nil)
+					WithRevokeTokenError(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,
+				idpListerBuilder.RequireExactlyOneCallToRevokeToken(t,
 					"upstream-oidc-provider-name",
-					&oidctestutil.RevokeRefreshTokenArgs{
+					&oidctestutil.RevokeTokenArgs{
 						Ctx:       syncContext.Context,
-						RefreshToken: "fake-upstream-refresh-token",
+						Token:     "fake-upstream-refresh-token",
+						TokenType: provider.RefreshTokenType,
+					},
+				)
+
+				// The secret is deleted.
+				r.ElementsMatch(
+					[]kubetesting.Action{
+						kubetesting.NewDeleteActionWithOptions(secretsGVR, installedInNamespace, "oidcRefreshSession", testutil.NewPreconditions("uid-123", "rv-123")),
+					},
+					kubeClient.Actions(),
+				)
+			})
+		})
+
+		when("there are valid, expired refresh secrets which contain upstream access tokens", 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{
+									UpstreamAccessToken: "fake-upstream-access-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").
+					WithRevokeTokenError(nil)
+				idpListerBuilder := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream.Build())
+
+				startInformersAndController(idpListerBuilder.Build())
+				r.NoError(controllerlib.TestSync(t, subject, *syncContext))
+
+				// The upstream refresh token is revoked.
+				idpListerBuilder.RequireExactlyOneCallToRevokeToken(t,
+					"upstream-oidc-provider-name",
+					&oidctestutil.RevokeTokenArgs{
+						Ctx:       syncContext.Context,
+						Token:     "fake-upstream-access-token",
+						TokenType: provider.AccessTokenType,
 					},
 				)
 
@@ -962,7 +1318,7 @@ func TestGarbageCollectorControllerSync(t *testing.T) {
 				r.False(syncContext.Queue.(*testQueue).called)
 
 				// Run sync again when not enough time has passed since the most recent run, so no delete
-				// operations should happen even though there is a expired secret now.
+				// operations should happen even though there is an expired secret now.
 				fakeClock.Step(29 * time.Second)
 				r.NoError(controllerlib.TestSync(t, subject, *syncContext))
 				require.Empty(t, kubeClient.Actions())

internal/controllermanager/prepare_controllers.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package controllermanager provides an entrypoint into running all of the controllers that run as

internal/crud/crud_test.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package crud

internal/fositestorage/accesstoken/accesstoken_test.go

@@ -53,7 +53,7 @@ func TestAccessTokenStorage(t *testing.T) {
 				},
 			},
 			Data: map[string][]byte{
-				"pinniped-storage-data":    []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"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","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`),
+				"pinniped-storage-data":    []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"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","upstreamAccessToken":"","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`),
 				"pinniped-storage-version": []byte("1"),
 			},
 			Type: "storage.pinniped.dev/access-token",
@@ -122,7 +122,7 @@ func TestAccessTokenStorageRevocation(t *testing.T) {
 				},
 			},
 			Data: map[string][]byte{
-				"pinniped-storage-data":    []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"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","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`),
+				"pinniped-storage-data":    []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"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","upstreamAccessToken":"","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`),
 				"pinniped-storage-version": []byte("1"),
 			},
 			Type: "storage.pinniped.dev/access-token",

internal/fositestorage/authorizationcode/authorizationcode.go

@@ -371,32 +371,34 @@ const ExpectedAuthorizeCodeSessionJSONFromFuzzing = `{
 				"providerType": "ɥ闣ʬ橳(ý綃ʃʚƟ覣k眐4",
 				"oidc": {
 					"upstreamRefreshToken": "tC嵽痊w",
-					"upstreamSubject": "a紽ǒ|鰽ŋ猊I",
+					"upstreamAccessToken": "a紽ǒ|鰽ŋ猊I",
-					"upstreamIssuer": "妬\u003e6鉢緋uƴŤȱʀ"
+					"upstreamSubject": "妬\u003e6鉢緋uƴŤȱʀ",
+					"upstreamIssuer": ":設虝27就伒犘c"
 				},
 				"ldap": {
-					"userDN": "Â?墖\u003cƬb獭潜Ʃ饾k|鬌R蜚蠣",
+					"userDN": "ɏȫ齁š%Op",
 					"extraRefreshAttributes": {
-						"ȱ藚ɏ¬Ê蒭堜]ȗ韚ʫ": "鷞aŚB碠k9帴ʘ赱"
+						"T妼É4İ\u003e×1": "ʥ笿0D",
+						"÷驣7Ʀ澉1æɽ誮": "ʫ繕ȫ",
+						"ŚB碠k9": "i磊ůď逳鞪?3)藵睋邔\u0026Ű"
 					}
 				},
 				"activedirectory": {
-					"userDN": "瑹xȢ~1Įx欼笝?úT妼",
+					"userDN": "s",
 					"extraRefreshAttributes": {
-						"iYn": "麹Œ颛",
+						"ƉǢIȽ齤士bEǎ儯惝IozŁ5rƖ螼": "偶宾儮猷V麹Œ颛Ė應,Ɣ鬅X¤"
-						"İ\u003e×1飞O+î艔垎0OƉǢIȽ齤士": "ȐĨf跞@)¿,ɭS隑ip偶"
 					}
 				}
 			}
 		},
 		"requestedAudience": [
-			"應,Ɣ鬅X¤",
+			"tO灞浛a齙\\蹼偦歛ơ",
-			"¤.岵骘胲ƤkǦ"
+			"皦pSǬŝ社Vƅȭǝ*"
 		],
 		"grantedAudience": [
-			"鸖I¶媁y衑拁Ȃ",
+			"ĝ\"zvưã置bņ抰蛖a³2ʫ",
-			"社Vƅȭǝ*擦28Dž 甍 ć",
+			"Ŷɽ蔒PR}Ųʓl{鼐jÃ轘屔挝",
-			"bņ抰蛖a³2ʫ承dʬ)ġ,TÀqy_"
+			"Œų崓ļ憽-蹐È_¸]fś"
 		]
 	},
 	"version": "2"

internal/fositestorage/authorizationcode/authorizationcode_test.go

@@ -65,7 +65,7 @@ func TestAuthorizationCodeStorage(t *testing.T) {
 				},
 			},
 			Data: map[string][]byte{
-				"pinniped-storage-data":    []byte(`{"active":true,"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"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","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`),
+				"pinniped-storage-data":    []byte(`{"active":true,"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"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","upstreamAccessToken":"","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`),
 				"pinniped-storage-version": []byte("1"),
 			},
 			Type: "storage.pinniped.dev/authcode",
@@ -84,7 +84,7 @@ func TestAuthorizationCodeStorage(t *testing.T) {
 				},
 			},
 			Data: map[string][]byte{
-				"pinniped-storage-data":    []byte(`{"active":false,"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"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","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`),
+				"pinniped-storage-data":    []byte(`{"active":false,"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"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","upstreamAccessToken":"","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`),
 				"pinniped-storage-version": []byte("1"),
 			},
 			Type: "storage.pinniped.dev/authcode",
@@ -389,11 +389,12 @@ func TestFuzzAndJSONNewValidEmptyAuthorizeCodeSession(t *testing.T) {
 	validSessionJSONBytes, err := json.MarshalIndent(validSession, "", "\t")
 	require.NoError(t, err)
 	authorizeCodeSessionJSONFromFuzzing := string(validSessionJSONBytes)
-	t.Log(authorizeCodeSessionJSONFromFuzzing)
 
 	// the fuzzed session and storage session should have identical JSON
 	require.JSONEq(t, authorizeCodeSessionJSONFromFuzzing, authorizeCodeSessionJSONFromStorage)
 
+	// t.Log("actual value from fuzzing", authorizeCodeSessionJSONFromFuzzing) // can be useful when updating expected value
+
 	// while the fuzzer will panic if AuthorizeRequest changes in a way that cannot be fuzzed,
 	// if it adds a new field that can be fuzzed, this check will fail
 	// thus if AuthorizeRequest changes, we will detect it here (though we could possibly miss an omitempty field)

internal/fositestorage/openidconnect/openidconnect_test.go

@@ -52,7 +52,7 @@ func TestOpenIdConnectStorage(t *testing.T) {
 				},
 			},
 			Data: map[string][]byte{
-				"pinniped-storage-data":    []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"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","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`),
+				"pinniped-storage-data":    []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"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","upstreamAccessToken":"","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`),
 				"pinniped-storage-version": []byte("1"),
 			},
 			Type: "storage.pinniped.dev/oidc",

internal/fositestorage/pkce/pkce_test.go

@@ -52,7 +52,7 @@ func TestPKCEStorage(t *testing.T) {
 				},
 			},
 			Data: map[string][]byte{
-				"pinniped-storage-data":    []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"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","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`),
+				"pinniped-storage-data":    []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"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","upstreamAccessToken":"","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`),
 				"pinniped-storage-version": []byte("1"),
 			},
 			Type: "storage.pinniped.dev/pkce",

internal/fositestorage/refreshtoken/refreshtoken_test.go

@@ -52,7 +52,7 @@ func TestRefreshTokenStorage(t *testing.T) {
 				},
 			},
 			Data: map[string][]byte{
-				"pinniped-storage-data":    []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"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","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`),
+				"pinniped-storage-data":    []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"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","upstreamAccessToken":"","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`),
 				"pinniped-storage-version": []byte("1"),
 			},
 			Type: "storage.pinniped.dev/refresh-token",
@@ -122,7 +122,7 @@ func TestRefreshTokenStorageRevocation(t *testing.T) {
 				},
 			},
 			Data: map[string][]byte{
-				"pinniped-storage-data":    []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"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","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`),
+				"pinniped-storage-data":    []byte(`{"request":{"id":"abcd-1","requestedAt":"0001-01-01T00:00:00Z","client":{"id":"pinny","redirect_uris":null,"grant_types":null,"response_types":null,"scopes":null,"audience":null,"public":true,"jwks_uri":"where","jwks":null,"token_endpoint_auth_method":"something","request_uris":null,"request_object_signing_alg":"","token_endpoint_auth_signing_alg":""},"scopes":null,"grantedScopes":null,"form":{"key":["val"]},"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","upstreamAccessToken":"","upstreamSubject":"some-subject","upstreamIssuer":"some-issuer"}}},"requestedAudience":null,"grantedAudience":null},"version":"2"}`),
 				"pinniped-storage-version": []byte("1"),
 			},
 			Type: "storage.pinniped.dev/refresh-token",

internal/kubeclient/kubeclient.go

@@ -1,4 +1,4 @@
-// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package kubeclient

internal/kubeclient/kubeclient_test.go

@@ -1,4 +1,4 @@
-// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package kubeclient

internal/mocks/credentialrequestmocks/credentialrequestmocks.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 //
 

internal/mocks/issuermocks/issuermocks.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 //
 

internal/mocks/mockkeyset/mockkeyset.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 //
 

internal/mocks/mockldapconn/mockldapconn.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 //
 

internal/mocks/mocksecrethelper/mocksecrethelper.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 //
 

internal/mocks/mocktokenauthenticator/mocktokenauthenticator.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 //
 

internal/mocks/mocktokenauthenticatorcloser/mocktokenauthenticatorcloser.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 //
 

internal/mocks/mockupstreamoidcidentityprovider/mockupstreamoidcidentityprovider.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 //
 
@@ -14,12 +14,12 @@ import (
 	reflect "reflect"
 
 	gomock "github.com/golang/mock/gomock"
-	oauth2 "golang.org/x/oauth2"
+	provider "go.pinniped.dev/internal/oidc/provider"
-	types "k8s.io/apimachinery/pkg/types"
-
 	nonce "go.pinniped.dev/pkg/oidcclient/nonce"
 	oidctypes "go.pinniped.dev/pkg/oidcclient/oidctypes"
 	pkce "go.pinniped.dev/pkg/oidcclient/pkce"
+	oauth2 "golang.org/x/oauth2"
+	types "k8s.io/apimachinery/pkg/types"
 )
 
 // MockUpstreamOIDCIdentityProviderI is a mock of UpstreamOIDCIdentityProviderI interface.
@@ -186,6 +186,20 @@ func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) GetUsernameClaim() *gom
 	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetUsernameClaim", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).GetUsernameClaim))
 }
 
+// HasUserInfoURL mocks base method.
+func (m *MockUpstreamOIDCIdentityProviderI) HasUserInfoURL() bool {
+	m.ctrl.T.Helper()
+	ret := m.ctrl.Call(m, "HasUserInfoURL")
+	ret0, _ := ret[0].(bool)
+	return ret0
+}
+
+// HasUserInfoURL indicates an expected call of HasUserInfoURL.
+func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) HasUserInfoURL() *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasUserInfoURL", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).HasUserInfoURL))
+}
+
 // PasswordCredentialsGrantAndValidateTokens mocks base method.
 func (m *MockUpstreamOIDCIdentityProviderI) PasswordCredentialsGrantAndValidateTokens(arg0 context.Context, arg1, arg2 string) (*oidctypes.Token, error) {
 	m.ctrl.T.Helper()
@@ -216,31 +230,31 @@ func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) PerformRefresh(arg0, ar
 	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "PerformRefresh", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).PerformRefresh), arg0, arg1)
 }
 
-// RevokeRefreshToken mocks base method.
+// RevokeToken mocks base method.
-func (m *MockUpstreamOIDCIdentityProviderI) RevokeRefreshToken(arg0 context.Context, arg1 string) error {
+func (m *MockUpstreamOIDCIdentityProviderI) RevokeToken(arg0 context.Context, arg1 string, arg2 provider.RevocableTokenType) error {
 	m.ctrl.T.Helper()
-	ret := m.ctrl.Call(m, "RevokeRefreshToken", arg0, arg1)
+	ret := m.ctrl.Call(m, "RevokeToken", arg0, arg1, arg2)
 	ret0, _ := ret[0].(error)
 	return ret0
 }
 
-// RevokeRefreshToken indicates an expected call of RevokeRefreshToken.
+// RevokeToken indicates an expected call of RevokeToken.
-func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) RevokeRefreshToken(arg0, arg1 interface{}) *gomock.Call {
+func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) RevokeToken(arg0, arg1, arg2 interface{}) *gomock.Call {
 	mr.mock.ctrl.T.Helper()
-	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RevokeRefreshToken", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).RevokeRefreshToken), arg0, arg1)
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RevokeToken", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).RevokeToken), arg0, arg1, arg2)
 }
 
-// ValidateToken mocks base method.
+// ValidateTokenAndMergeWithUserInfo mocks base method.
-func (m *MockUpstreamOIDCIdentityProviderI) ValidateTokenAndMergeWithUserInfo(arg0 context.Context, arg1 *oauth2.Token, arg2 nonce.Nonce, arg3 bool) (*oidctypes.Token, error) {
+func (m *MockUpstreamOIDCIdentityProviderI) ValidateTokenAndMergeWithUserInfo(arg0 context.Context, arg1 *oauth2.Token, arg2 nonce.Nonce, arg3, arg4 bool) (*oidctypes.Token, error) {
 	m.ctrl.T.Helper()
-	ret := m.ctrl.Call(m, "ValidateTokenAndMergeWithUserInfo", arg0, arg1, arg2, arg3)
+	ret := m.ctrl.Call(m, "ValidateTokenAndMergeWithUserInfo", arg0, arg1, arg2, arg3, arg4)
 	ret0, _ := ret[0].(*oidctypes.Token)
 	ret1, _ := ret[1].(error)
 	return ret0, ret1
 }
 
-// ValidateToken indicates an expected call of ValidateToken.
+// ValidateTokenAndMergeWithUserInfo indicates an expected call of ValidateTokenAndMergeWithUserInfo.
-func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) ValidateToken(arg0, arg1, arg2, arg3 interface{}) *gomock.Call {
+func (mr *MockUpstreamOIDCIdentityProviderIMockRecorder) ValidateTokenAndMergeWithUserInfo(arg0, arg1, arg2, arg3, arg4 interface{}) *gomock.Call {
 	mr.mock.ctrl.T.Helper()
-	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ValidateTokenAndMergeWithUserInfo", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).ValidateTokenAndMergeWithUserInfo), arg0, arg1, arg2, arg3)
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ValidateTokenAndMergeWithUserInfo", reflect.TypeOf((*MockUpstreamOIDCIdentityProviderI)(nil).ValidateTokenAndMergeWithUserInfo), arg0, arg1, arg2, arg3, arg4)
 }

internal/oidc/auth/auth_handler.go

@@ -174,16 +174,6 @@ func handleAuthRequestForOIDCUpstreamPasswordGrant(
 			fosite.ErrAccessDenied.WithDebug(err.Error()), true) // WithDebug hides the error from the client
 	}
 
-	if token.RefreshToken == nil || token.RefreshToken.Token == "" {
-		plog.Warning("refresh token not returned by upstream provider during password grant, "+
-			"please check configuration of OIDCIdentityProvider and the client in the upstream provider's API/UI",
-			"upstreamName", oidcUpstream.GetName(),
-			"scopes", oidcUpstream.GetScopes())
-		return writeAuthorizeError(w, oauthHelper, authorizeRequester,
-			fosite.ErrAccessDenied.WithHint(
-				"Refresh token not returned by upstream provider during password grant."), true)
-	}
-
 	subject, username, groups, err := downstreamsession.GetDownstreamIdentityFromUpstreamIDToken(oidcUpstream, token.IDToken.Claims)
 	if err != nil {
 		// Return a user-friendly error for this case which is entirely within our control.
@@ -191,31 +181,14 @@ func handleAuthRequestForOIDCUpstreamPasswordGrant(
 			fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error()), true,
 		)
 	}
-	upstreamSubject, err := downstreamsession.ExtractStringClaimValue(oidc.IDTokenSubjectClaim, oidcUpstream.GetName(), token.IDToken.Claims)
+
+	customSessionData, err := downstreamsession.MakeDownstreamOIDCCustomSessionData(oidcUpstream, token)
 	if err != nil {
-		// Return a user-friendly error for this case which is entirely within our control.
-		return writeAuthorizeError(w, oauthHelper, authorizeRequester,
-			fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error()), true,
-		)
-	}
-	upstreamIssuer, err := downstreamsession.ExtractStringClaimValue(oidc.IDTokenIssuerClaim, oidcUpstream.GetName(), token.IDToken.Claims)
-	if err != nil {
-		// Return a user-friendly error for this case which is entirely within our control.
 		return writeAuthorizeError(w, oauthHelper, authorizeRequester,
 			fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error()), true,
 		)
 	}
 
-	customSessionData := &psession.CustomSessionData{
-		ProviderUID:  oidcUpstream.GetResourceUID(),
-		ProviderName: oidcUpstream.GetName(),
-		ProviderType: psession.ProviderTypeOIDC,
-		OIDC: &psession.OIDCSessionData{
-			UpstreamRefreshToken: token.RefreshToken.Token,
-			UpstreamIssuer:       upstreamIssuer,
-			UpstreamSubject:      upstreamSubject,
-		},
-	}
 	return makeDownstreamSessionAndReturnAuthcodeRedirect(r, w, oauthHelper, authorizeRequester, subject, username, groups, customSessionData)
 }
 

internal/oidc/auth/auth_handler_test.go

@@ -56,6 +56,7 @@ func TestAuthorizationEndpoint(t *testing.T) {
 		oidcUpstreamUsernameClaim             = "the-user-claim"
 		oidcUpstreamGroupsClaim               = "the-groups-claim"
 		oidcPasswordGrantUpstreamRefreshToken = "some-opaque-token" //nolint: gosec
+		oidcUpstreamAccessToken               = "some-access-token"
 
 		downstreamIssuer                       = "https://my-downstream-issuer.com/some-path"
 		downstreamRedirectURI                  = "http://127.0.0.1/callback"
@@ -154,9 +155,15 @@ func TestAuthorizationEndpoint(t *testing.T) {
 			"state":             happyState,
 		}
 
-		fositeAccessDeniedWithMissingRefreshTokenErrorQuery = map[string]string{
+		fositeAccessDeniedWithMissingAccessTokenErrorQuery = map[string]string{
 			"error":             "access_denied",
-			"error_description": "The resource owner or authorization server denied the request. Refresh token not returned by upstream provider during password grant.",
+			"error_description": "The resource owner or authorization server denied the request. Reason: neither access token nor refresh token returned by upstream provider.",
+			"state":             happyState,
+		}
+
+		fositeAccessDeniedWithMissingUserInfoEndpointErrorQuery = map[string]string{
+			"error":             "access_denied",
+			"error_description": "The resource owner or authorization server denied the request. Reason: access token was returned by upstream provider but there was no userinfo endpoint.",
 			"state":             happyState,
 		}
 
@@ -475,6 +482,17 @@ func TestAuthorizationEndpoint(t *testing.T) {
 		},
 	}
 
+	expectedHappyOIDCPasswordGrantCustomSessionWithAccessToken := &psession.CustomSessionData{
+		ProviderUID:  oidcPasswordGrantUpstreamResourceUID,
+		ProviderName: oidcPasswordGrantUpstreamName,
+		ProviderType: psession.ProviderTypeOIDC,
+		OIDC: &psession.OIDCSessionData{
+			UpstreamAccessToken: oidcUpstreamAccessToken,
+			UpstreamSubject:     oidcUpstreamSubject,
+			UpstreamIssuer:      oidcUpstreamIssuer,
+		},
+	}
+
 	// Note that fosite puts the granted scopes as a param in the redirect URI even though the spec doesn't seem to require it
 	happyAuthcodeDownstreamRedirectLocationRegexp := downstreamRedirectURI + `\?code=([^&]+)&scope=openid&state=` + happyState
 
@@ -873,6 +891,50 @@ func TestAuthorizationEndpoint(t *testing.T) {
 			wantUpstreamStateParamInLocationHeader: true,
 			wantBodyStringWithLocationInHref:       true,
 		},
+		{
+			name:                              "OIDC password grant happy path when upstream IDP returned empty refresh token but it did return an access token and has a userinfo endpoint",
+			idps:                              oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithEmptyRefreshToken().WithAccessToken(oidcUpstreamAccessToken).WithUserInfoURL().Build()),
+			method:                            http.MethodGet,
+			path:                              happyGetRequestPath,
+			customUsernameHeader:              pointer.StringPtr(oidcUpstreamUsername),
+			customPasswordHeader:              pointer.StringPtr(oidcUpstreamPassword),
+			wantPasswordGrantCall:             happyUpstreamPasswordGrantMockExpectation,
+			wantStatus:                        http.StatusFound,
+			wantContentType:                   htmlContentType,
+			wantRedirectLocationRegexp:        happyAuthcodeDownstreamRedirectLocationRegexp,
+			wantDownstreamIDTokenSubject:      oidcUpstreamIssuer + "?sub=" + oidcUpstreamSubjectQueryEscaped,
+			wantDownstreamIDTokenUsername:     oidcUpstreamUsername,
+			wantDownstreamIDTokenGroups:       oidcUpstreamGroupMembership,
+			wantDownstreamRequestedScopes:     happyDownstreamScopesRequested,
+			wantDownstreamRedirectURI:         downstreamRedirectURI,
+			wantDownstreamGrantedScopes:       happyDownstreamScopesGranted,
+			wantDownstreamNonce:               downstreamNonce,
+			wantDownstreamPKCEChallenge:       downstreamPKCEChallenge,
+			wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod,
+			wantDownstreamCustomSessionData:   expectedHappyOIDCPasswordGrantCustomSessionWithAccessToken,
+		},
+		{
+			name:                              "OIDC password grant happy path when upstream IDP did not return a refresh token but it did return an access token and has a userinfo endpoint",
+			idps:                              oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithoutRefreshToken().WithAccessToken(oidcUpstreamAccessToken).WithUserInfoURL().Build()),
+			method:                            http.MethodGet,
+			path:                              happyGetRequestPath,
+			customUsernameHeader:              pointer.StringPtr(oidcUpstreamUsername),
+			customPasswordHeader:              pointer.StringPtr(oidcUpstreamPassword),
+			wantPasswordGrantCall:             happyUpstreamPasswordGrantMockExpectation,
+			wantStatus:                        http.StatusFound,
+			wantContentType:                   htmlContentType,
+			wantRedirectLocationRegexp:        happyAuthcodeDownstreamRedirectLocationRegexp,
+			wantDownstreamIDTokenSubject:      oidcUpstreamIssuer + "?sub=" + oidcUpstreamSubjectQueryEscaped,
+			wantDownstreamIDTokenUsername:     oidcUpstreamUsername,
+			wantDownstreamIDTokenGroups:       oidcUpstreamGroupMembership,
+			wantDownstreamRequestedScopes:     happyDownstreamScopesRequested,
+			wantDownstreamRedirectURI:         downstreamRedirectURI,
+			wantDownstreamGrantedScopes:       happyDownstreamScopesGranted,
+			wantDownstreamNonce:               downstreamNonce,
+			wantDownstreamPKCEChallenge:       downstreamPKCEChallenge,
+			wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod,
+			wantDownstreamCustomSessionData:   expectedHappyOIDCPasswordGrantCustomSessionWithAccessToken,
+		},
 		{
 			name:                 "error during upstream LDAP authentication",
 			idps:                 oidctestutil.NewUpstreamIDPListerBuilder().WithLDAP(&erroringUpstreamLDAPIdentityProvider),
@@ -1015,8 +1077,8 @@ func TestAuthorizationEndpoint(t *testing.T) {
 			wantBodyString:       "",
 		},
 		{
-			name:                  "return an error when upstream IDP did not return a refresh token",
+			name:                  "password grant returns an error when upstream IDP returns no refresh token with an access token but has no userinfo endpoint",
-			idps:                  oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithoutRefreshToken().Build()),
+			idps:                  oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithoutRefreshToken().WithAccessToken(oidcUpstreamAccessToken).WithoutUserInfoURL().Build()),
 			method:                http.MethodGet,
 			path:                  happyGetRequestPath,
 			customUsernameHeader:  pointer.StringPtr(oidcUpstreamUsername),
@@ -1024,12 +1086,12 @@ func TestAuthorizationEndpoint(t *testing.T) {
 			wantPasswordGrantCall: happyUpstreamPasswordGrantMockExpectation,
 			wantStatus:            http.StatusFound,
 			wantContentType:       "application/json; charset=utf-8",
-			wantLocationHeader:    urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithMissingRefreshTokenErrorQuery),
+			wantLocationHeader:    urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithMissingUserInfoEndpointErrorQuery),
 			wantBodyString:        "",
 		},
 		{
-			name:                  "return an error when upstream IDP did not return a refresh token",
+			name:                  "password grant returns an error when upstream IDP returns empty refresh token with an access token but has no userinfo endpoint",
-			idps:                  oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithEmptyRefreshToken().Build()),
+			idps:                  oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithEmptyRefreshToken().WithAccessToken(oidcUpstreamAccessToken).WithoutUserInfoURL().Build()),
 			method:                http.MethodGet,
 			path:                  happyGetRequestPath,
 			customUsernameHeader:  pointer.StringPtr(oidcUpstreamUsername),
@@ -1037,7 +1099,59 @@ func TestAuthorizationEndpoint(t *testing.T) {
 			wantPasswordGrantCall: happyUpstreamPasswordGrantMockExpectation,
 			wantStatus:            http.StatusFound,
 			wantContentType:       "application/json; charset=utf-8",
-			wantLocationHeader:    urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithMissingRefreshTokenErrorQuery),
+			wantLocationHeader:    urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithMissingUserInfoEndpointErrorQuery),
+			wantBodyString:        "",
+		},
+		{
+			name:                  "password grant returns an error when upstream IDP returns empty refresh token and empty access token",
+			idps:                  oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithEmptyRefreshToken().WithEmptyAccessToken().Build()),
+			method:                http.MethodGet,
+			path:                  happyGetRequestPath,
+			customUsernameHeader:  pointer.StringPtr(oidcUpstreamUsername),
+			customPasswordHeader:  pointer.StringPtr(oidcUpstreamPassword),
+			wantPasswordGrantCall: happyUpstreamPasswordGrantMockExpectation,
+			wantStatus:            http.StatusFound,
+			wantContentType:       "application/json; charset=utf-8",
+			wantLocationHeader:    urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithMissingAccessTokenErrorQuery),
+			wantBodyString:        "",
+		},
+		{
+			name:                  "password grant returns an error when upstream IDP returns no refresh and no access token",
+			idps:                  oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithoutRefreshToken().WithoutAccessToken().Build()),
+			method:                http.MethodGet,
+			path:                  happyGetRequestPath,
+			customUsernameHeader:  pointer.StringPtr(oidcUpstreamUsername),
+			customPasswordHeader:  pointer.StringPtr(oidcUpstreamPassword),
+			wantPasswordGrantCall: happyUpstreamPasswordGrantMockExpectation,
+			wantStatus:            http.StatusFound,
+			wantContentType:       "application/json; charset=utf-8",
+			wantLocationHeader:    urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithMissingAccessTokenErrorQuery),
+			wantBodyString:        "",
+		},
+		{
+			name:                  "password grant returns an error when upstream IDP returns no refresh token and empty access token",
+			idps:                  oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithoutRefreshToken().WithEmptyAccessToken().Build()),
+			method:                http.MethodGet,
+			path:                  happyGetRequestPath,
+			customUsernameHeader:  pointer.StringPtr(oidcUpstreamUsername),
+			customPasswordHeader:  pointer.StringPtr(oidcUpstreamPassword),
+			wantPasswordGrantCall: happyUpstreamPasswordGrantMockExpectation,
+			wantStatus:            http.StatusFound,
+			wantContentType:       "application/json; charset=utf-8",
+			wantLocationHeader:    urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithMissingAccessTokenErrorQuery),
+			wantBodyString:        "",
+		},
+		{
+			name:                  "password grant returns an error when upstream IDP returns empty refresh token and no access token",
+			idps:                  oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(passwordGrantUpstreamOIDCIdentityProviderBuilder().WithEmptyRefreshToken().WithoutAccessToken().Build()),
+			method:                http.MethodGet,
+			path:                  happyGetRequestPath,
+			customUsernameHeader:  pointer.StringPtr(oidcUpstreamUsername),
+			customPasswordHeader:  pointer.StringPtr(oidcUpstreamPassword),
+			wantPasswordGrantCall: happyUpstreamPasswordGrantMockExpectation,
+			wantStatus:            http.StatusFound,
+			wantContentType:       "application/json; charset=utf-8",
+			wantLocationHeader:    urlWithQuery(downstreamRedirectURI, fositeAccessDeniedWithMissingAccessTokenErrorQuery),
 			wantBodyString:        "",
 		},
 		{

internal/oidc/callback/callback_handler.go

@@ -19,7 +19,6 @@ import (
 	"go.pinniped.dev/internal/oidc/provider"
 	"go.pinniped.dev/internal/oidc/provider/formposthtml"
 	"go.pinniped.dev/internal/plog"
-	"go.pinniped.dev/internal/psession"
 )
 
 func NewHandler(
@@ -69,39 +68,17 @@ func NewHandler(
 			return httperr.New(http.StatusBadGateway, "error exchanging and validating upstream tokens")
 		}
 
-		if token.RefreshToken == nil || token.RefreshToken.Token == "" {
-			plog.Warning("refresh token not returned by upstream provider during authcode exchange, "+
-				"please check configuration of OIDCIdentityProvider and the client in the upstream provider's API/UI",
-				"upstreamName", upstreamIDPConfig.GetName(),
-				"scopes", upstreamIDPConfig.GetScopes(),
-				"additionalParams", upstreamIDPConfig.GetAdditionalAuthcodeParams())
-			return httperr.New(http.StatusUnprocessableEntity, "refresh token not returned by upstream provider during authcode exchange")
-		}
-
 		subject, username, groups, err := downstreamsession.GetDownstreamIdentityFromUpstreamIDToken(upstreamIDPConfig, token.IDToken.Claims)
 		if err != nil {
 			return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err)
 		}
 
-		upstreamSubject, err := downstreamsession.ExtractStringClaimValue(oidc.IDTokenSubjectClaim, upstreamIDPConfig.GetName(), token.IDToken.Claims)
+		customSessionData, err := downstreamsession.MakeDownstreamOIDCCustomSessionData(upstreamIDPConfig, token)
-		if err != nil {
-			return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err)
-		}
-		upstreamIssuer, err := downstreamsession.ExtractStringClaimValue(oidc.IDTokenIssuerClaim, upstreamIDPConfig.GetName(), token.IDToken.Claims)
 		if err != nil {
 			return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err)
 		}
 
-		openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, &psession.CustomSessionData{
+		openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, customSessionData)
-			ProviderUID:  upstreamIDPConfig.GetResourceUID(),
-			ProviderName: upstreamIDPConfig.GetName(),
-			ProviderType: psession.ProviderTypeOIDC,
-			OIDC: &psession.OIDCSessionData{
-				UpstreamRefreshToken: token.RefreshToken.Token,
-				UpstreamSubject:      upstreamSubject,
-				UpstreamIssuer:       upstreamIssuer,
-			},
-		})
 
 		authorizeResponder, err := oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, openIDSession)
 		if err != nil {

internal/oidc/callback/callback_handler_test.go

@@ -31,6 +31,7 @@ const (
 
 	oidcUpstreamIssuer              = "https://my-upstream-issuer.com"
 	oidcUpstreamRefreshToken        = "test-refresh-token"
+	oidcUpstreamAccessToken         = "test-access-token"
 	oidcUpstreamSubject             = "abc123-some guid" // has a space character which should get escaped in URL
 	oidcUpstreamSubjectQueryEscaped = "abc123-some+guid"
 	oidcUpstreamUsername            = "test-pinniped-username"
@@ -83,6 +84,16 @@ var (
 			UpstreamSubject:      oidcUpstreamSubject,
 		},
 	}
+	happyDownstreamAccessTokenCustomSessionData = &psession.CustomSessionData{
+		ProviderUID:  happyUpstreamIDPResourceUID,
+		ProviderName: happyUpstreamIDPName,
+		ProviderType: psession.ProviderTypeOIDC,
+		OIDC: &psession.OIDCSessionData{
+			UpstreamAccessToken: oidcUpstreamAccessToken,
+			UpstreamIssuer:      oidcUpstreamIssuer,
+			UpstreamSubject:     oidcUpstreamSubject,
+		},
+	}
 )
 
 func TestCallbackEndpoint(t *testing.T) {
@@ -200,6 +211,29 @@ func TestCallbackEndpoint(t *testing.T) {
 				args:                    happyExchangeAndValidateTokensArgs,
 			},
 		},
+		{
+			name:                              "GET with authcode exchange that returns an access token but no refresh token when there is a userinfo endpoint returns 303 to downstream client callback with its state and code",
+			idps:                              oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithEmptyRefreshToken().WithAccessToken(oidcUpstreamAccessToken).WithUserInfoURL().Build()),
+			method:                            http.MethodGet,
+			path:                              newRequestPath().WithState(happyState).String(),
+			csrfCookie:                        happyCSRFCookie,
+			wantStatus:                        http.StatusSeeOther,
+			wantRedirectLocationRegexp:        happyDownstreamRedirectLocationRegexp,
+			wantBody:                          "",
+			wantDownstreamIDTokenSubject:      oidcUpstreamIssuer + "?sub=" + oidcUpstreamSubjectQueryEscaped,
+			wantDownstreamIDTokenUsername:     oidcUpstreamUsername,
+			wantDownstreamIDTokenGroups:       oidcUpstreamGroupMembership,
+			wantDownstreamRequestedScopes:     happyDownstreamScopesRequested,
+			wantDownstreamGrantedScopes:       happyDownstreamScopesGranted,
+			wantDownstreamNonce:               downstreamNonce,
+			wantDownstreamPKCEChallenge:       downstreamPKCEChallenge,
+			wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod,
+			wantDownstreamCustomSessionData:   happyDownstreamAccessTokenCustomSessionData,
+			wantAuthcodeExchangeCall: &expectedAuthcodeExchange{
+				performedByUpstreamName: happyUpstreamIDPName,
+				args:                    happyExchangeAndValidateTokensArgs,
+			},
+		},
 		{
 			name: "upstream IDP provides no username or group claim configuration, so we use default username claim and skip groups",
 			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(
@@ -323,28 +357,70 @@ func TestCallbackEndpoint(t *testing.T) {
 			},
 		},
 		{
-			name:            "return an error when upstream IDP did not return a refresh token",
+			name:            "return an error when upstream IDP returned no refresh token with an access token when there is no userinfo endpoint",
-			idps:            oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithoutRefreshToken().Build()),
+			idps:            oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithoutRefreshToken().WithAccessToken(oidcUpstreamAccessToken).WithoutUserInfoURL().Build()),
 			method:          http.MethodGet,
 			path:            newRequestPath().WithState(happyState).String(),
 			csrfCookie:      happyCSRFCookie,
 			wantStatus:      http.StatusUnprocessableEntity,
 			wantContentType: htmlContentType,
-			wantBody:        "Unprocessable Entity: refresh token not returned by upstream provider during authcode exchange\n",
+			wantBody:        "Unprocessable Entity: access token was returned by upstream provider but there was no userinfo endpoint\n",
 			wantAuthcodeExchangeCall: &expectedAuthcodeExchange{
 				performedByUpstreamName: happyUpstreamIDPName,
 				args:                    happyExchangeAndValidateTokensArgs,
 			},
 		},
 		{
-			name:            "return an error when upstream IDP returned an empty refresh token",
+			name:            "return an error when upstream IDP returned no refresh token and no access token",
-			idps:            oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithEmptyRefreshToken().Build()),
+			idps:            oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithoutRefreshToken().WithoutAccessToken().Build()),
 			method:          http.MethodGet,
 			path:            newRequestPath().WithState(happyState).String(),
 			csrfCookie:      happyCSRFCookie,
 			wantStatus:      http.StatusUnprocessableEntity,
 			wantContentType: htmlContentType,
-			wantBody:        "Unprocessable Entity: refresh token not returned by upstream provider during authcode exchange\n",
+			wantBody:        "Unprocessable Entity: neither access token nor refresh token returned by upstream provider\n",
+			wantAuthcodeExchangeCall: &expectedAuthcodeExchange{
+				performedByUpstreamName: happyUpstreamIDPName,
+				args:                    happyExchangeAndValidateTokensArgs,
+			},
+		},
+		{
+			name:            "return an error when upstream IDP returned an empty refresh token and empty access token",
+			idps:            oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithEmptyRefreshToken().WithEmptyAccessToken().Build()),
+			method:          http.MethodGet,
+			path:            newRequestPath().WithState(happyState).String(),
+			csrfCookie:      happyCSRFCookie,
+			wantStatus:      http.StatusUnprocessableEntity,
+			wantContentType: htmlContentType,
+			wantBody:        "Unprocessable Entity: neither access token nor refresh token returned by upstream provider\n",
+			wantAuthcodeExchangeCall: &expectedAuthcodeExchange{
+				performedByUpstreamName: happyUpstreamIDPName,
+				args:                    happyExchangeAndValidateTokensArgs,
+			},
+		},
+		{
+			name:            "return an error when upstream IDP returned no refresh token and empty access token",
+			idps:            oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithoutRefreshToken().WithEmptyAccessToken().Build()),
+			method:          http.MethodGet,
+			path:            newRequestPath().WithState(happyState).String(),
+			csrfCookie:      happyCSRFCookie,
+			wantStatus:      http.StatusUnprocessableEntity,
+			wantContentType: htmlContentType,
+			wantBody:        "Unprocessable Entity: neither access token nor refresh token returned by upstream provider\n",
+			wantAuthcodeExchangeCall: &expectedAuthcodeExchange{
+				performedByUpstreamName: happyUpstreamIDPName,
+				args:                    happyExchangeAndValidateTokensArgs,
+			},
+		},
+		{
+			name:            "return an error when upstream IDP returned an empty refresh token and no access token",
+			idps:            oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(happyUpstream().WithEmptyRefreshToken().WithoutAccessToken().Build()),
+			method:          http.MethodGet,
+			path:            newRequestPath().WithState(happyState).String(),
+			csrfCookie:      happyCSRFCookie,
+			wantStatus:      http.StatusUnprocessableEntity,
+			wantContentType: htmlContentType,
+			wantBody:        "Unprocessable Entity: neither access token nor refresh token returned by upstream provider\n",
 			wantAuthcodeExchangeCall: &expectedAuthcodeExchange{
 				performedByUpstreamName: happyUpstreamIDPName,
 				args:                    happyExchangeAndValidateTokensArgs,

internal/oidc/downstreamsession/downstream_session.go

@@ -5,6 +5,7 @@
 package downstreamsession
 
 import (
+	"errors"
 	"fmt"
 	"net/url"
 	"time"
@@ -19,6 +20,7 @@ import (
 	"go.pinniped.dev/internal/oidc/provider"
 	"go.pinniped.dev/internal/plog"
 	"go.pinniped.dev/internal/psession"
+	"go.pinniped.dev/pkg/oidcclient/oidctypes"
 )
 
 const (
@@ -58,6 +60,55 @@ func MakeDownstreamSession(subject string, username string, groups []string, cus
 	return openIDSession
 }
 
+func MakeDownstreamOIDCCustomSessionData(oidcUpstream provider.UpstreamOIDCIdentityProviderI, token *oidctypes.Token) (*psession.CustomSessionData, error) {
+	upstreamSubject, err := ExtractStringClaimValue(oidc.IDTokenSubjectClaim, oidcUpstream.GetName(), token.IDToken.Claims)
+	if err != nil {
+		return nil, err
+	}
+	upstreamIssuer, err := ExtractStringClaimValue(oidc.IDTokenIssuerClaim, oidcUpstream.GetName(), token.IDToken.Claims)
+	if err != nil {
+		return nil, err
+	}
+
+	customSessionData := &psession.CustomSessionData{
+		ProviderUID:  oidcUpstream.GetResourceUID(),
+		ProviderName: oidcUpstream.GetName(),
+		ProviderType: psession.ProviderTypeOIDC,
+		OIDC: &psession.OIDCSessionData{
+			UpstreamIssuer:  upstreamIssuer,
+			UpstreamSubject: upstreamSubject,
+		},
+	}
+
+	const pleaseCheck = "please check configuration of OIDCIdentityProvider and the client in the " +
+		"upstream provider's API/UI and try to get a refresh token if possible"
+	logKV := []interface{}{
+		"upstreamName", oidcUpstream.GetName(),
+		"scopes", oidcUpstream.GetScopes(),
+		"additionalParams", oidcUpstream.GetAdditionalAuthcodeParams(),
+	}
+
+	hasRefreshToken := token.RefreshToken != nil && token.RefreshToken.Token != ""
+	hasAccessToken := token.AccessToken != nil && token.AccessToken.Token != ""
+	switch {
+	case hasRefreshToken: // we prefer refresh tokens, so check for this first
+		customSessionData.OIDC.UpstreamRefreshToken = token.RefreshToken.Token
+	case hasAccessToken: // as a fallback, we can use the access token as long as there is a userinfo endpoint
+		if !oidcUpstream.HasUserInfoURL() {
+			plog.Warning("access token was returned by upstream provider during login without a refresh token "+
+				"and there was no userinfo endpoint available on the provider. "+pleaseCheck, logKV...)
+			return nil, errors.New("access token was returned by upstream provider but there was no userinfo endpoint")
+		}
+		plog.Info("refresh token not returned by upstream provider during login, using access token instead. "+pleaseCheck, logKV...)
+		customSessionData.OIDC.UpstreamAccessToken = token.AccessToken.Token
+	default:
+		plog.Warning("refresh token and access token not returned by upstream provider during login. "+pleaseCheck, logKV...)
+		return nil, errors.New("neither access token nor refresh token returned by upstream provider")
+	}
+
+	return customSessionData, nil
+}
+
 // GrantScopesIfRequested auto-grants the scopes for which we do not require end-user approval, if they were requested.
 func GrantScopesIfRequested(authorizeRequester fosite.AuthorizeRequester) {
 	oidc.GrantScopeIfRequested(authorizeRequester, coreosoidc.ScopeOpenID)

internal/oidc/kube_storage.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package oidc

internal/oidc/provider/dynamic_upstream_idp_provider.go

@@ -5,6 +5,7 @@ package provider
 
 import (
 	"context"
+	"fmt"
 	"net/url"
 	"sync"
 
@@ -17,6 +18,14 @@ import (
 	"go.pinniped.dev/pkg/oidcclient/pkce"
 )
 
+type RevocableTokenType string
+
+// These strings correspond to the token types defined by https://datatracker.ietf.org/doc/html/rfc7009#section-2.1
+const (
+	RefreshTokenType RevocableTokenType = "refresh_token"
+	AccessTokenType  RevocableTokenType = "access_token"
+)
+
 type UpstreamOIDCIdentityProviderI interface {
 	// GetName returns a name for this upstream provider, which will be used as a component of the path for the
 	// callback endpoint hosted by the Supervisor.
@@ -31,6 +40,9 @@ type UpstreamOIDCIdentityProviderI interface {
 	// GetAuthorizationURL returns the Authorization Endpoint fetched from discovery.
 	GetAuthorizationURL() *url.URL
 
+	// HasUserInfoURL returns whether there is a non-empty value for userinfo_endpoint fetched from discovery.
+	HasUserInfoURL() bool
+
 	// GetScopes returns the scopes to request in authorization (authcode or password grant) flow.
 	GetScopes() []string
 
@@ -68,13 +80,16 @@ type UpstreamOIDCIdentityProviderI interface {
 	// validate the ID token.
 	PerformRefresh(ctx context.Context, refreshToken string) (*oauth2.Token, error)
 
-	// RevokeRefreshToken will attempt to revoke the given token, if the provider has a revocation endpoint.
+	// RevokeToken will attempt to revoke the given token, if the provider has a revocation endpoint.
-	RevokeRefreshToken(ctx context.Context, refreshToken string) error
+	// It may return an error wrapped by a RetryableRevocationError, which is an error indicating that it may
+	// be worth trying to revoke the same token again later. Any other error returned should be assumed to
+	// represent an error such that it is not worth retrying revocation later, even though revocation failed.
+	RevokeToken(ctx context.Context, token string, tokenType RevocableTokenType) error
 
 	// ValidateTokenAndMergeWithUserInfo will validate the ID token. It will also merge the claims from the userinfo endpoint response
 	// into the ID token's claims, if the provider offers the userinfo endpoint. It returns the validated/updated
 	// tokens, or an error.
-	ValidateTokenAndMergeWithUserInfo(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error)
+	ValidateTokenAndMergeWithUserInfo(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool, requireUserInfo bool) (*oidctypes.Token, error)
 }
 
 type UpstreamLDAPIdentityProviderI interface {
@@ -162,3 +177,19 @@ func (p *dynamicUpstreamIDPProvider) GetActiveDirectoryIdentityProviders() []Ups
 	defer p.mutex.RUnlock()
 	return p.activeDirectoryUpstreams
 }
+
+type RetryableRevocationError struct {
+	wrapped error
+}
+
+func NewRetryableRevocationError(wrapped error) RetryableRevocationError {
+	return RetryableRevocationError{wrapped: wrapped}
+}
+
+func (e RetryableRevocationError) Error() string {
+	return fmt.Sprintf("retryable revocation error: %v", e.wrapped)
+}
+
+func (e RetryableRevocationError) Unwrap() error {
+	return e.wrapped
+}

internal/oidc/provider/manager/manager_test.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package manager

internal/oidc/token/token_handler.go

@@ -11,12 +11,14 @@ import (
 
 	"github.com/ory/fosite"
 	"github.com/ory/x/errorsx"
+	"golang.org/x/oauth2"
 
 	"go.pinniped.dev/internal/httputil/httperr"
 	"go.pinniped.dev/internal/oidc"
 	"go.pinniped.dev/internal/oidc/provider"
 	"go.pinniped.dev/internal/plog"
 	"go.pinniped.dev/internal/psession"
+	"go.pinniped.dev/pkg/oidcclient/oidctypes"
 )
 
 var (
@@ -101,7 +103,15 @@ func upstreamRefresh(ctx context.Context, accessRequest fosite.AccessRequester,
 
 func upstreamOIDCRefresh(ctx context.Context, session *psession.PinnipedSession, providerCache oidc.UpstreamIdentityProvidersLister) error {
 	s := session.Custom
-	if s.OIDC == nil || s.OIDC.UpstreamRefreshToken == "" {
+	if s.OIDC == nil {
+		return errorsx.WithStack(errMissingUpstreamSessionInternalError)
+	}
+
+	accessTokenStored := s.OIDC.UpstreamAccessToken != ""
+	refreshTokenStored := s.OIDC.UpstreamRefreshToken != ""
+
+	exactlyOneTokenStored := (accessTokenStored || refreshTokenStored) && !(accessTokenStored && refreshTokenStored)
+	if !exactlyOneTokenStored {
 		return errorsx.WithStack(errMissingUpstreamSessionInternalError)
 	}
 
@@ -113,63 +123,88 @@ func upstreamOIDCRefresh(ctx context.Context, session *psession.PinnipedSession,
 	plog.Debug("attempting upstream refresh request",
 		"providerName", s.ProviderName, "providerType", s.ProviderType, "providerUID", s.ProviderUID)
 
-	refreshedTokens, err := p.PerformRefresh(ctx, s.OIDC.UpstreamRefreshToken)
+	var tokens *oauth2.Token
+	if refreshTokenStored {
+		tokens, err = p.PerformRefresh(ctx, s.OIDC.UpstreamRefreshToken)
 		if err != nil {
 			return errorsx.WithStack(errUpstreamRefreshError.WithHint(
 				"Upstream refresh failed.",
 			).WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
 		}
+	} else {
+		tokens = &oauth2.Token{AccessToken: s.OIDC.UpstreamAccessToken}
+	}
 
 	// Upstream refresh may or may not return a new ID token. From the spec:
 	// "the response body is the Token Response of Section 3.1.3.3 except that it might not contain an id_token."
 	// https://openid.net/specs/openid-connect-core-1_0.html#RefreshTokenResponse
-	_, hasIDTok := refreshedTokens.Extra("id_token").(string)
+	_, hasIDTok := tokens.Extra("id_token").(string)
 
 	// The spec is not 100% clear about whether an ID token from the refresh flow should include a nonce, and at
 	// least some providers do not include one, so we skip the nonce validation here (but not other validations).
-	validatedTokens, err := p.ValidateTokenAndMergeWithUserInfo(ctx, refreshedTokens, "", hasIDTok)
+	validatedTokens, err := p.ValidateTokenAndMergeWithUserInfo(ctx, tokens, "", hasIDTok, accessTokenStored)
 	if err != nil {
 		return errorsx.WithStack(errUpstreamRefreshError.WithHintf(
 			"Upstream refresh returned an invalid ID token or UserInfo response.").WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
 	}
 
-	claims := validatedTokens.IDToken.Claims
+	err = validateIdentityUnchangedSinceInitialLogin(validatedTokens, session, p.GetUsernameClaim())
-	// if we have any claims at all, we better have a subject, and it better match the previous value.
+	if err != nil {
-	// but it's possible that we don't because both returning a new refresh token on refresh and having a userinfo
+		return err
-	// endpoint are optional.
-	if len(validatedTokens.IDToken.Claims) != 0 { //nolint:nestif
-		newSub, hasSub := getString(claims, oidc.IDTokenSubjectClaim)
-		if !hasSub {
-			return errorsx.WithStack(errUpstreamRefreshError.WithHintf(
-				"Upstream refresh failed.").WithWrap(errors.New("subject in upstream refresh not found")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
-		}
-		if s.OIDC.UpstreamSubject != newSub {
-			return errorsx.WithStack(errUpstreamRefreshError.WithHintf(
-				"Upstream refresh failed.").WithWrap(errors.New("subject in upstream refresh does not match previous value")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
-		}
-		usernameClaim := p.GetUsernameClaim()
-		newUsername, hasUsername := getString(claims, usernameClaim)
-		oldUsername := session.Fosite.Claims.Extra[oidc.DownstreamUsernameClaim]
-		// its possible this won't be returned.
-		// but if it is, verify that it hasn't changed.
-		if hasUsername && oldUsername != newUsername {
-			return errorsx.WithStack(errUpstreamRefreshError.WithHintf(
-				"Upstream refresh failed.").WithWrap(errors.New("username in upstream refresh does not match previous value")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
-		}
-		newIssuer, hasIssuer := getString(claims, oidc.IDTokenIssuerClaim)
-		if hasIssuer && s.OIDC.UpstreamIssuer != newIssuer {
-			return errorsx.WithStack(errUpstreamRefreshError.WithHintf(
-				"Upstream refresh failed.").WithWrap(errors.New("issuer in upstream refresh does not match previous value")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
-		}
 	}
 
 	// Upstream refresh may or may not return a new refresh token. If we got a new refresh token, then update it in
 	// the user's session. If we did not get a new refresh token, then keep the old one in the session by avoiding
 	// overwriting the old one.
-	if refreshedTokens.RefreshToken != "" {
+	if tokens.RefreshToken != "" {
-		plog.Debug("upstream refresh request did not return a new refresh token",
+		plog.Debug("upstream refresh request returned a new refresh token",
 			"providerName", s.ProviderName, "providerType", s.ProviderType, "providerUID", s.ProviderUID)
-		s.OIDC.UpstreamRefreshToken = refreshedTokens.RefreshToken
+		s.OIDC.UpstreamRefreshToken = tokens.RefreshToken
+	}
+
+	return nil
+}
+
+func validateIdentityUnchangedSinceInitialLogin(validatedTokens *oidctypes.Token, session *psession.PinnipedSession, usernameClaimName string) error {
+	s := session.Custom
+	mergedClaims := validatedTokens.IDToken.Claims
+
+	// If we have any claims at all, we better have a subject, and it better match the previous value.
+	// but it's possible that we don't because both returning a new id token on refresh and having a userinfo
+	// endpoint are optional.
+	if len(mergedClaims) == 0 {
+		return nil
+	}
+
+	newSub, hasSub := getString(mergedClaims, oidc.IDTokenSubjectClaim)
+	if !hasSub {
+		return errorsx.WithStack(errUpstreamRefreshError.WithHintf(
+			"Upstream refresh failed.").WithWrap(errors.New("subject in upstream refresh not found")).
+			WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
+	}
+	if s.OIDC.UpstreamSubject != newSub {
+		return errorsx.WithStack(errUpstreamRefreshError.WithHintf(
+			"Upstream refresh failed.").WithWrap(errors.New("subject in upstream refresh does not match previous value")).
+			WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
+	}
+
+	newUsername, hasUsername := getString(mergedClaims, usernameClaimName)
+	oldUsername := session.Fosite.Claims.Extra[oidc.DownstreamUsernameClaim]
+	// It's possible that a username wasn't returned by the upstream provider during refresh,
+	// but if it is, verify that it hasn't changed.
+	if hasUsername && oldUsername != newUsername {
+		return errorsx.WithStack(errUpstreamRefreshError.WithHintf(
+			"Upstream refresh failed.").WithWrap(errors.New("username in upstream refresh does not match previous value")).
+			WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
+	}
+
+	newIssuer, hasIssuer := getString(mergedClaims, oidc.IDTokenIssuerClaim)
+	// It's possible that an issuer wasn't returned by the upstream provider during refresh,
+	// but if it is, verify that it hasn't changed.
+	if hasIssuer && s.OIDC.UpstreamIssuer != newIssuer {
+		return errorsx.WithStack(errUpstreamRefreshError.WithHintf(
+			"Upstream refresh failed.").WithWrap(errors.New("issuer in upstream refresh does not match previous value")).
+			WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
 	}
 
 	return nil

internal/oidc/token/token_handler_test.go

@@ -225,7 +225,7 @@ type expectedUpstreamRefresh struct {
 
 type expectedUpstreamValidateTokens struct {
 	performedByUpstreamName string
-	args                    *oidctestutil.ValidateTokenArgs
+	args                    *oidctestutil.ValidateTokenAndMergeWithUserInfoArgs
 }
 
 type tokenEndpointResponseExpectedValues struct {
@@ -881,6 +881,7 @@ func TestRefreshGrant(t *testing.T) {
 		oidcUpstreamInitialRefreshToken   = "initial-upstream-refresh-token"
 		oidcUpstreamRefreshedIDToken      = "fake-refreshed-id-token"
 		oidcUpstreamRefreshedRefreshToken = "fake-refreshed-refresh-token"
+		oidcUpstreamAccessToken           = "fake-upstream-access-token" //nolint:gosec
 
 		ldapUpstreamName        = "some-ldap-idp"
 		ldapUpstreamResourceUID = "ldap-resource-uid"
@@ -904,7 +905,7 @@ func TestRefreshGrant(t *testing.T) {
 			WithResourceUID(oidcUpstreamResourceUID)
 	}
 
-	initialUpstreamOIDCCustomSessionData := func() *psession.CustomSessionData {
+	initialUpstreamOIDCRefreshTokenCustomSessionData := func() *psession.CustomSessionData {
 		return &psession.CustomSessionData{
 			ProviderName: oidcUpstreamName,
 			ProviderUID:  oidcUpstreamResourceUID,
@@ -917,8 +918,21 @@ func TestRefreshGrant(t *testing.T) {
 		}
 	}
 
+	initialUpstreamOIDCAccessTokenCustomSessionData := func() *psession.CustomSessionData {
+		return &psession.CustomSessionData{
+			ProviderName: oidcUpstreamName,
+			ProviderUID:  oidcUpstreamResourceUID,
+			ProviderType: oidcUpstreamType,
+			OIDC: &psession.OIDCSessionData{
+				UpstreamAccessToken: oidcUpstreamAccessToken,
+				UpstreamSubject:     goodUpstreamSubject,
+				UpstreamIssuer:      goodIssuer,
+			},
+		}
+	}
+
 	upstreamOIDCCustomSessionDataWithNewRefreshToken := func(newRefreshToken string) *psession.CustomSessionData {
-		sessionData := initialUpstreamOIDCCustomSessionData()
+		sessionData := initialUpstreamOIDCRefreshTokenCustomSessionData()
 		sessionData.OIDC.UpstreamRefreshToken = newRefreshToken
 		return sessionData
 	}
@@ -957,13 +971,15 @@ func TestRefreshGrant(t *testing.T) {
 		}
 	}
 
-	happyUpstreamValidateTokenCall := func(expectedTokens *oauth2.Token) *expectedUpstreamValidateTokens {
+	happyUpstreamValidateTokenCall := func(expectedTokens *oauth2.Token, requireIDToken bool) *expectedUpstreamValidateTokens {
 		return &expectedUpstreamValidateTokens{
 			performedByUpstreamName: oidcUpstreamName,
-			args: &oidctestutil.ValidateTokenArgs{
+			args: &oidctestutil.ValidateTokenAndMergeWithUserInfoArgs{
 				Ctx:                  nil, // this will be filled in with the actual request context by the test below
 				Tok:                  expectedTokens,
 				ExpectedIDTokenNonce: "", // always expect empty string
+				RequireUserInfo:      false,
+				RequireIDToken:       requireIDToken,
 			},
 		}
 	}
@@ -986,7 +1002,7 @@ func TestRefreshGrant(t *testing.T) {
 		// Should always try to perform an upstream refresh.
 		want.wantUpstreamRefreshCall = happyOIDCUpstreamRefreshCall()
 		if expectToValidateToken != nil {
-			want.wantUpstreamOIDCValidateTokenCall = happyUpstreamValidateTokenCall(expectToValidateToken)
+			want.wantUpstreamOIDCValidateTokenCall = happyUpstreamValidateTokenCall(expectToValidateToken, true)
 		}
 		return want
 	}
@@ -1049,7 +1065,7 @@ func TestRefreshGrant(t *testing.T) {
 		{
 			name: "happy path refresh grant with openid scope granted (id token returned)",
 			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(
-				upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{
+				upstreamOIDCIdentityProviderBuilder().WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{
 					IDToken: &oidctypes.IDToken{
 						Claims: map[string]interface{}{
 							"sub": goodUpstreamSubject,
@@ -1057,9 +1073,9 @@ func TestRefreshGrant(t *testing.T) {
 					},
 				}).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()),
 			authcodeExchange: authcodeExchangeInputs{
-				customSessionData: initialUpstreamOIDCCustomSessionData(),
+				customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(),
 				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") },
-				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()),
+				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()),
 			},
 			refreshRequest: refreshRequestInputs{
 				want: happyRefreshTokenResponseForOpenIDAndOfflineAccess(
@@ -1071,7 +1087,7 @@ func TestRefreshGrant(t *testing.T) {
 		{
 			name: "refresh grant with unchanged username claim",
 			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(
-				upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim").WithValidatedTokens(&oidctypes.Token{
+				upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim").WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{
 					IDToken: &oidctypes.IDToken{
 						Claims: map[string]interface{}{
 							"some-claim":     "some-value",
@@ -1081,9 +1097,9 @@ func TestRefreshGrant(t *testing.T) {
 					},
 				}).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()),
 			authcodeExchange: authcodeExchangeInputs{
-				customSessionData: initialUpstreamOIDCCustomSessionData(),
+				customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(),
 				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") },
-				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()),
+				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()),
 			},
 			refreshRequest: refreshRequestInputs{
 				want: happyRefreshTokenResponseForOpenIDAndOfflineAccess(
@@ -1093,22 +1109,63 @@ func TestRefreshGrant(t *testing.T) {
 			},
 		},
 		{
-			name: "happy path refresh grant without openid scope granted (no id token returned)",
+			name: "refresh grant when the customsessiondata has a stored access token and no stored refresh token",
 			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(
-				upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{
+				upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim").
+					WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{
 						IDToken: &oidctypes.IDToken{
-						Claims: map[string]interface{}{},
+							Claims: map[string]interface{}{
+								"some-claim":     "some-value",
+								"sub":            goodUpstreamSubject,
+								"username-claim": goodUsername,
+							},
+						},
+						AccessToken: &oidctypes.AccessToken{
+							Token: oidcUpstreamAccessToken,
 						},
 					}).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()),
 			authcodeExchange: authcodeExchangeInputs{
-				customSessionData: initialUpstreamOIDCCustomSessionData(),
+				customSessionData: initialUpstreamOIDCAccessTokenCustomSessionData(),
+				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") },
+				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCAccessTokenCustomSessionData()),
+			}, // do not want upstreamRefreshRequest???
+			refreshRequest: refreshRequestInputs{
+				want: tokenEndpointResponseExpectedValues{
+					wantStatus:            http.StatusOK,
+					wantSuccessBodyFields: []string{"refresh_token", "id_token", "access_token", "token_type", "expires_in", "scope"},
+					wantRequestedScopes:   []string{"openid", "offline_access"},
+					wantGrantedScopes:     []string{"openid", "offline_access"},
+					wantUpstreamOIDCValidateTokenCall: &expectedUpstreamValidateTokens{
+						oidcUpstreamName,
+						&oidctestutil.ValidateTokenAndMergeWithUserInfoArgs{
+							Ctx:                  nil,                                                 // this will be filled in with the actual request context by the test below
+							Tok:                  &oauth2.Token{AccessToken: oidcUpstreamAccessToken}, // only the old access token
+							ExpectedIDTokenNonce: "",                                                  // always expect empty string
+							RequireIDToken:       false,
+							RequireUserInfo:      true,
+						},
+					},
+					wantCustomSessionDataStored: initialUpstreamOIDCAccessTokenCustomSessionData(), // doesn't change when we refresh
+				},
+			},
+		},
+		{
+			name: "happy path refresh grant without openid scope granted (no id token returned)",
+			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(
+				upstreamOIDCIdentityProviderBuilder().WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{
+					IDToken: &oidctypes.IDToken{
+						Claims: map[string]interface{}{},
+					},
+				}).WithRefreshedTokens(refreshedUpstreamTokensWithRefreshTokenWithoutIDToken()).Build()),
+			authcodeExchange: authcodeExchangeInputs{
+				customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(),
 				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "offline_access") },
 				want: tokenEndpointResponseExpectedValues{
 					wantStatus:                  http.StatusOK,
 					wantSuccessBodyFields:       []string{"refresh_token", "access_token", "token_type", "expires_in", "scope"},
 					wantRequestedScopes:         []string{"offline_access"},
 					wantGrantedScopes:           []string{"offline_access"},
-					wantCustomSessionDataStored: initialUpstreamOIDCCustomSessionData(),
+					wantCustomSessionDataStored: initialUpstreamOIDCRefreshTokenCustomSessionData(),
 				},
 			},
 			refreshRequest: refreshRequestInputs{
@@ -1118,7 +1175,7 @@ func TestRefreshGrant(t *testing.T) {
 					wantRequestedScopes:               []string{"offline_access"},
 					wantGrantedScopes:                 []string{"offline_access"},
 					wantUpstreamRefreshCall:           happyOIDCUpstreamRefreshCall(),
-					wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()),
+					wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithRefreshTokenWithoutIDToken(), false),
 					wantCustomSessionDataStored:       upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken),
 				},
 			},
@@ -1126,27 +1183,32 @@ func TestRefreshGrant(t *testing.T) {
 		{
 			name: "happy path refresh grant when the upstream refresh does not return a new ID token",
 			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(
-				upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{
+				upstreamOIDCIdentityProviderBuilder().WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{
 					IDToken: &oidctypes.IDToken{
 						Claims: map[string]interface{}{},
 					},
 				}).WithRefreshedTokens(refreshedUpstreamTokensWithRefreshTokenWithoutIDToken()).Build()),
 			authcodeExchange: authcodeExchangeInputs{
-				customSessionData: initialUpstreamOIDCCustomSessionData(),
+				customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(),
 				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") },
-				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()),
+				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()),
 			},
 			refreshRequest: refreshRequestInputs{
-				want: happyRefreshTokenResponseForOpenIDAndOfflineAccess(
+				want: tokenEndpointResponseExpectedValues{
-					upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken),
+					wantStatus:                        http.StatusOK,
-					refreshedUpstreamTokensWithRefreshTokenWithoutIDToken(), // expect ValidateTokenAndMergeWithUserInfo is called
+					wantSuccessBodyFields:             []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"},
-				),
+					wantRequestedScopes:               []string{"openid", "offline_access"},
+					wantGrantedScopes:                 []string{"openid", "offline_access"},
+					wantUpstreamRefreshCall:           happyOIDCUpstreamRefreshCall(),
+					wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithRefreshTokenWithoutIDToken(), false),
+					wantCustomSessionDataStored:       upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken),
+				},
 			},
 		},
 		{
 			name: "happy path refresh grant when the upstream refresh does not return a new refresh token",
 			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(
-				upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{
+				upstreamOIDCIdentityProviderBuilder().WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{
 					IDToken: &oidctypes.IDToken{
 						Claims: map[string]interface{}{
 							"sub": goodUpstreamSubject,
@@ -1154,13 +1216,13 @@ func TestRefreshGrant(t *testing.T) {
 					},
 				}).WithRefreshedTokens(refreshedUpstreamTokensWithIDTokenWithoutRefreshToken()).Build()),
 			authcodeExchange: authcodeExchangeInputs{
-				customSessionData: initialUpstreamOIDCCustomSessionData(),
+				customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(),
 				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") },
-				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()),
+				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()),
 			},
 			refreshRequest: refreshRequestInputs{
 				want: happyRefreshTokenResponseForOpenIDAndOfflineAccess(
-					initialUpstreamOIDCCustomSessionData(), // still has the initial refresh token stored
+					initialUpstreamOIDCRefreshTokenCustomSessionData(), // still has the initial refresh token stored
 					refreshedUpstreamTokensWithIDTokenWithoutRefreshToken(),
 				),
 			},
@@ -1168,7 +1230,7 @@ func TestRefreshGrant(t *testing.T) {
 		{
 			name: "when the refresh request adds a new scope to the list of requested scopes then it is ignored",
 			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(
-				upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{
+				upstreamOIDCIdentityProviderBuilder().WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{
 					IDToken: &oidctypes.IDToken{
 						Claims: map[string]interface{}{
 							"sub": goodUpstreamSubject,
@@ -1176,9 +1238,9 @@ func TestRefreshGrant(t *testing.T) {
 					},
 				}).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()),
 			authcodeExchange: authcodeExchangeInputs{
-				customSessionData: initialUpstreamOIDCCustomSessionData(),
+				customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(),
 				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") },
-				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()),
+				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()),
 			},
 			refreshRequest: refreshRequestInputs{
 				modifyTokenRequest: func(r *http.Request, refreshToken string, accessToken string) {
@@ -1193,7 +1255,7 @@ func TestRefreshGrant(t *testing.T) {
 		{
 			name: "when the refresh request removes a scope which was originally granted from the list of requested scopes then it is granted anyway",
 			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(
-				upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{
+				upstreamOIDCIdentityProviderBuilder().WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{
 					IDToken: &oidctypes.IDToken{
 						Claims: map[string]interface{}{
 							"sub": goodUpstreamSubject,
@@ -1201,14 +1263,14 @@ func TestRefreshGrant(t *testing.T) {
 					},
 				}).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()),
 			authcodeExchange: authcodeExchangeInputs{
-				customSessionData: initialUpstreamOIDCCustomSessionData(),
+				customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(),
 				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access pinniped:request-audience") },
 				want: tokenEndpointResponseExpectedValues{
 					wantStatus:                  http.StatusOK,
 					wantSuccessBodyFields:       []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"},
 					wantRequestedScopes:         []string{"openid", "offline_access", "pinniped:request-audience"},
 					wantGrantedScopes:           []string{"openid", "offline_access", "pinniped:request-audience"},
-					wantCustomSessionDataStored: initialUpstreamOIDCCustomSessionData(),
+					wantCustomSessionDataStored: initialUpstreamOIDCRefreshTokenCustomSessionData(),
 				},
 			},
 			refreshRequest: refreshRequestInputs{
@@ -1221,7 +1283,7 @@ func TestRefreshGrant(t *testing.T) {
 					wantRequestedScopes:               []string{"openid", "offline_access", "pinniped:request-audience"},
 					wantGrantedScopes:                 []string{"openid", "offline_access", "pinniped:request-audience"},
 					wantUpstreamRefreshCall:           happyOIDCUpstreamRefreshCall(),
-					wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()),
+					wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true),
 					wantCustomSessionDataStored:       upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken),
 				},
 			},
@@ -1229,7 +1291,7 @@ func TestRefreshGrant(t *testing.T) {
 		{
 			name: "when the refresh request does not include a scope param then it gets all the same scopes as the original authorization request",
 			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(
-				upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{
+				upstreamOIDCIdentityProviderBuilder().WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{
 					IDToken: &oidctypes.IDToken{
 						Claims: map[string]interface{}{
 							"sub": goodUpstreamSubject,
@@ -1237,9 +1299,9 @@ func TestRefreshGrant(t *testing.T) {
 					},
 				}).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()),
 			authcodeExchange: authcodeExchangeInputs{
-				customSessionData: initialUpstreamOIDCCustomSessionData(),
+				customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(),
 				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") },
-				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()),
+				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()),
 			},
 			refreshRequest: refreshRequestInputs{
 				modifyTokenRequest: func(r *http.Request, refreshToken string, accessToken string) {
@@ -1255,14 +1317,14 @@ func TestRefreshGrant(t *testing.T) {
 			name: "when a bad refresh token is sent in the refresh request",
 			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()),
 			authcodeExchange: authcodeExchangeInputs{
-				customSessionData: initialUpstreamOIDCCustomSessionData(),
+				customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(),
 				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "offline_access") },
 				want: tokenEndpointResponseExpectedValues{
 					wantStatus:                  http.StatusOK,
 					wantSuccessBodyFields:       []string{"refresh_token", "access_token", "token_type", "expires_in", "scope"},
 					wantRequestedScopes:         []string{"offline_access"},
 					wantGrantedScopes:           []string{"offline_access"},
-					wantCustomSessionDataStored: initialUpstreamOIDCCustomSessionData(),
+					wantCustomSessionDataStored: initialUpstreamOIDCRefreshTokenCustomSessionData(),
 				},
 			},
 			refreshRequest: refreshRequestInputs{
@@ -1279,14 +1341,14 @@ func TestRefreshGrant(t *testing.T) {
 			name: "when the access token is sent as if it were a refresh token",
 			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()),
 			authcodeExchange: authcodeExchangeInputs{
-				customSessionData: initialUpstreamOIDCCustomSessionData(),
+				customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(),
 				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "offline_access") },
 				want: tokenEndpointResponseExpectedValues{
 					wantStatus:                  http.StatusOK,
 					wantSuccessBodyFields:       []string{"refresh_token", "access_token", "token_type", "expires_in", "scope"},
 					wantRequestedScopes:         []string{"offline_access"},
 					wantGrantedScopes:           []string{"offline_access"},
-					wantCustomSessionDataStored: initialUpstreamOIDCCustomSessionData(),
+					wantCustomSessionDataStored: initialUpstreamOIDCRefreshTokenCustomSessionData(),
 				},
 			},
 			refreshRequest: refreshRequestInputs{
@@ -1303,14 +1365,14 @@ func TestRefreshGrant(t *testing.T) {
 			name: "when the wrong client ID is included in the refresh request",
 			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()),
 			authcodeExchange: authcodeExchangeInputs{
-				customSessionData: initialUpstreamOIDCCustomSessionData(),
+				customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(),
 				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "offline_access") },
 				want: tokenEndpointResponseExpectedValues{
 					wantStatus:                  http.StatusOK,
 					wantSuccessBodyFields:       []string{"refresh_token", "access_token", "token_type", "expires_in", "scope"},
 					wantRequestedScopes:         []string{"offline_access"},
 					wantGrantedScopes:           []string{"offline_access"},
-					wantCustomSessionDataStored: initialUpstreamOIDCCustomSessionData(),
+					wantCustomSessionDataStored: initialUpstreamOIDCRefreshTokenCustomSessionData(),
 				},
 			},
 			refreshRequest: refreshRequestInputs{
@@ -1474,7 +1536,7 @@ func TestRefreshGrant(t *testing.T) {
 			},
 		},
 		{
-			name: "when there is no OIDC refresh token in custom session data found in the session storage during the refresh request",
+			name: "when there is no OIDC refresh token nor access token in custom session data found in the session storage during the refresh request",
 			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()),
 			authcodeExchange: authcodeExchangeInputs{
 				customSessionData: &psession.CustomSessionData{
@@ -1482,7 +1544,8 @@ func TestRefreshGrant(t *testing.T) {
 					ProviderUID:  oidcUpstreamResourceUID,
 					ProviderType: oidcUpstreamType,
 					OIDC: &psession.OIDCSessionData{
-						UpstreamRefreshToken: "", // this should not happen in practice
+						UpstreamRefreshToken: "", // this should not happen in practice. we should always have exactly one of these.
+						UpstreamAccessToken:  "",
 					},
 				},
 				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") },
@@ -1493,6 +1556,7 @@ func TestRefreshGrant(t *testing.T) {
 						ProviderType: oidcUpstreamType,
 						OIDC: &psession.OIDCSessionData{
 							UpstreamRefreshToken: "", // this should not happen in practice
+							UpstreamAccessToken:  "",
 						},
 					},
 				),
@@ -1573,9 +1637,9 @@ func TestRefreshGrant(t *testing.T) {
 			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().
 				WithPerformRefreshError(errors.New("some upstream refresh error")).Build()),
 			authcodeExchange: authcodeExchangeInputs{
-				customSessionData: initialUpstreamOIDCCustomSessionData(),
+				customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(),
 				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") },
-				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()),
+				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()),
 			},
 			refreshRequest: refreshRequestInputs{
 				want: tokenEndpointResponseExpectedValues{
@@ -1595,17 +1659,17 @@ func TestRefreshGrant(t *testing.T) {
 			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().
 				WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).
 				// This is the current format of the errors returned by the production code version of ValidateTokenAndMergeWithUserInfo, see ValidateTokenAndMergeWithUserInfo in upstreamoidc.go
-				WithValidateTokenError(httperr.Wrap(http.StatusBadRequest, "some validate error", errors.New("some validate cause"))).
+				WithValidateTokenAndMergeWithUserInfoError(httperr.Wrap(http.StatusBadRequest, "some validate error", errors.New("some validate cause"))).
 				Build()),
 			authcodeExchange: authcodeExchangeInputs{
-				customSessionData: initialUpstreamOIDCCustomSessionData(),
+				customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(),
 				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") },
-				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()),
+				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()),
 			},
 			refreshRequest: refreshRequestInputs{
 				want: tokenEndpointResponseExpectedValues{
 					wantUpstreamRefreshCall:           happyOIDCUpstreamRefreshCall(),
-					wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()),
+					wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true),
 					wantStatus:                        http.StatusUnauthorized,
 					wantErrorResponseBody: here.Doc(`
 						{
@@ -1621,7 +1685,7 @@ func TestRefreshGrant(t *testing.T) {
 			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().
 				WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).
 				// This is the current format of the errors returned by the production code version of ValidateTokenAndMergeWithUserInfo, see ValidateTokenAndMergeWithUserInfo in upstreamoidc.go
-				WithValidatedTokens(&oidctypes.Token{
+				WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{
 					IDToken: &oidctypes.IDToken{
 						Claims: map[string]interface{}{
 							"sub": "something-different",
@@ -1630,14 +1694,14 @@ func TestRefreshGrant(t *testing.T) {
 				}).
 				Build()),
 			authcodeExchange: authcodeExchangeInputs{
-				customSessionData: initialUpstreamOIDCCustomSessionData(),
+				customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(),
 				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") },
-				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()),
+				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()),
 			},
 			refreshRequest: refreshRequestInputs{
 				want: tokenEndpointResponseExpectedValues{
 					wantUpstreamRefreshCall:           happyOIDCUpstreamRefreshCall(),
-					wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()),
+					wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true),
 					wantStatus:                        http.StatusUnauthorized,
 					wantErrorResponseBody: here.Doc(`
 						{
@@ -1651,7 +1715,7 @@ func TestRefreshGrant(t *testing.T) {
 		{
 			name: "refresh grant with claims but not the subject claim",
 			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(
-				upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{
+				upstreamOIDCIdentityProviderBuilder().WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{
 					IDToken: &oidctypes.IDToken{
 						Claims: map[string]interface{}{
 							"some-claim": "some-value",
@@ -1659,14 +1723,14 @@ func TestRefreshGrant(t *testing.T) {
 					},
 				}).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()),
 			authcodeExchange: authcodeExchangeInputs{
-				customSessionData: initialUpstreamOIDCCustomSessionData(),
+				customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(),
 				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") },
-				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()),
+				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()),
 			},
 			refreshRequest: refreshRequestInputs{
 				want: tokenEndpointResponseExpectedValues{
 					wantUpstreamRefreshCall:           happyOIDCUpstreamRefreshCall(),
-					wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()),
+					wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true),
 					wantStatus:                        http.StatusUnauthorized,
 					wantErrorResponseBody: here.Doc(`
 						{
@@ -1680,7 +1744,7 @@ func TestRefreshGrant(t *testing.T) {
 		{
 			name: "refresh grant with changed username claim",
 			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(
-				upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim").WithValidatedTokens(&oidctypes.Token{
+				upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim").WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{
 					IDToken: &oidctypes.IDToken{
 						Claims: map[string]interface{}{
 							"some-claim":     "some-value",
@@ -1690,14 +1754,14 @@ func TestRefreshGrant(t *testing.T) {
 					},
 				}).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()),
 			authcodeExchange: authcodeExchangeInputs{
-				customSessionData: initialUpstreamOIDCCustomSessionData(),
+				customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(),
 				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") },
-				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()),
+				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()),
 			},
 			refreshRequest: refreshRequestInputs{
 				want: tokenEndpointResponseExpectedValues{
 					wantUpstreamRefreshCall:           happyOIDCUpstreamRefreshCall(),
-					wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()),
+					wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true),
 					wantStatus:                        http.StatusUnauthorized,
 					wantErrorResponseBody: here.Doc(`
 						{
@@ -1711,7 +1775,7 @@ func TestRefreshGrant(t *testing.T) {
 		{
 			name: "refresh grant with changed issuer claim",
 			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(
-				upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim").WithValidatedTokens(&oidctypes.Token{
+				upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim").WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{
 					IDToken: &oidctypes.IDToken{
 						Claims: map[string]interface{}{
 							"some-claim": "some-value",
@@ -1721,14 +1785,14 @@ func TestRefreshGrant(t *testing.T) {
 					},
 				}).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()),
 			authcodeExchange: authcodeExchangeInputs{
-				customSessionData: initialUpstreamOIDCCustomSessionData(),
+				customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(),
 				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") },
-				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()),
+				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()),
 			},
 			refreshRequest: refreshRequestInputs{
 				want: tokenEndpointResponseExpectedValues{
 					wantUpstreamRefreshCall:           happyOIDCUpstreamRefreshCall(),
-					wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()),
+					wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true),
 					wantStatus:                        http.StatusUnauthorized,
 					wantErrorResponseBody: here.Doc(`
 						{

internal/plog/klog.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package plog

internal/psession/pinniped_session.go

@@ -61,8 +61,26 @@ const (
 
 // OIDCSessionData is the additional data needed by Pinniped when the upstream IDP is an OIDC provider.
 type OIDCSessionData struct {
+	// UpstreamRefreshToken will contain the refresh token from the upstream OIDC provider, if the upstream provider
+	// returned a refresh token during initial authorization. Otherwise, this field should be empty
+	// and the UpstreamAccessToken should be non-empty. We may not get a refresh token from the upstream provider,
+	// but we should always get an access token. However, when we do get a refresh token there is no need to
+	// also store the access token, since storing unnecessary tokens makes it possible for them to be leaked and
+	// creates more upstream revocation HTTP requests when it comes time to revoke the stored tokens.
 	UpstreamRefreshToken string `json:"upstreamRefreshToken"`
+
+	// UpstreamAccessToken will contain the access token returned by the upstream OIDC provider during initial
+	// authorization, but only when the provider did not also return a refresh token. When UpstreamRefreshToken is
+	// non-empty, then this field should be empty, indicating that we should use the upstream refresh token during
+	// downstream refresh.
+	UpstreamAccessToken string `json:"upstreamAccessToken"`
+
+	// UpstreamSubject is the "sub" claim from the upstream identity provider from the user's initial login. We store this
+	// so that we can validate that it does not change upon refresh.
 	UpstreamSubject string `json:"upstreamSubject"`
+
+	// UpstreamIssuer is the "iss" claim from the upstream identity provider from the user's initial login. We store this
+	// so that we can validate that it does not change upon refresh.
 	UpstreamIssuer string `json:"upstreamIssuer"`
 }
 

internal/registry/credentialrequest/rest_test.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package credentialrequest

internal/supervisor/server/server.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package server defines the entrypoint for the Pinniped Supervisor server.

internal/testutil/delete.go

@@ -1,4 +1,4 @@
-// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package testutil

internal/testutil/oidctestutil/oidctestutil.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package oidctestutil
@@ -68,19 +68,22 @@ type PerformRefreshArgs struct {
 	ExpectedSubject  string
 }
 
-// RevokeRefreshTokenArgs is used to spy on calls to
+// RevokeTokenArgs is used to spy on calls to
-// TestUpstreamOIDCIdentityProvider.RevokeRefreshTokenArgsFunc().
+// TestUpstreamOIDCIdentityProvider.RevokeTokenArgsFunc().
-type RevokeRefreshTokenArgs struct {
+type RevokeTokenArgs struct {
 	Ctx       context.Context
-	RefreshToken string
+	Token     string
+	TokenType provider.RevocableTokenType
 }
 
-// ValidateTokenArgs is used to spy on calls to
+// ValidateTokenAndMergeWithUserInfoArgs is used to spy on calls to
-// TestUpstreamOIDCIdentityProvider.ValidateTokenFunc().
+// TestUpstreamOIDCIdentityProvider.ValidateTokenAndMergeWithUserInfoFunc().
-type ValidateTokenArgs struct {
+type ValidateTokenAndMergeWithUserInfoArgs struct {
 	Ctx                  context.Context
 	Tok                  *oauth2.Token
 	ExpectedIDTokenNonce nonce.Nonce
+	RequireIDToken       bool
+	RequireUserInfo      bool
 }
 
 type ValidateRefreshArgs struct {
@@ -150,6 +153,7 @@ type TestUpstreamOIDCIdentityProvider struct {
 	ClientID                 string
 	ResourceUID              types.UID
 	AuthorizationURL         url.URL
+	UserInfoURL              bool
 	RevocationURL            *url.URL
 	UsernameClaim            string
 	GroupsClaim              string
@@ -172,9 +176,9 @@ type TestUpstreamOIDCIdentityProvider struct {
 
 	PerformRefreshFunc func(ctx context.Context, refreshToken string) (*oauth2.Token, error)
 
-	RevokeRefreshTokenFunc func(ctx context.Context, refreshToken string) error
+	RevokeTokenFunc func(ctx context.Context, refreshToken string, tokenType provider.RevocableTokenType) error
 
-	ValidateTokenFunc func(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error)
+	ValidateTokenAndMergeWithUserInfoFunc func(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error)
 
 	exchangeAuthcodeAndValidateTokensCallCount         int
 	exchangeAuthcodeAndValidateTokensArgs              []*ExchangeAuthcodeAndValidateTokenArgs
@@ -182,10 +186,10 @@ type TestUpstreamOIDCIdentityProvider struct {
 	passwordCredentialsGrantAndValidateTokensArgs      []*PasswordCredentialsGrantAndValidateTokensArgs
 	performRefreshCallCount                            int
 	performRefreshArgs                                 []*PerformRefreshArgs
-	revokeRefreshTokenCallCount                        int
+	revokeTokenCallCount                               int
-	revokeRefreshTokenArgs                             []*RevokeRefreshTokenArgs
+	revokeTokenArgs                                    []*RevokeTokenArgs
-	validateTokenCallCount                             int
+	validateTokenAndMergeWithUserInfoCallCount         int
-	validateTokenArgs                                  []*ValidateTokenArgs
+	validateTokenAndMergeWithUserInfoArgs              []*ValidateTokenAndMergeWithUserInfoArgs
 }
 
 var _ provider.UpstreamOIDCIdentityProviderI = &TestUpstreamOIDCIdentityProvider{}
@@ -210,6 +214,10 @@ func (u *TestUpstreamOIDCIdentityProvider) GetAuthorizationURL() *url.URL {
 	return &u.AuthorizationURL
 }
 
+func (u *TestUpstreamOIDCIdentityProvider) HasUserInfoURL() bool {
+	return u.UserInfoURL
+}
+
 func (u *TestUpstreamOIDCIdentityProvider) GetRevocationURL() *url.URL {
 	return u.RevocationURL
 }
@@ -284,16 +292,17 @@ func (u *TestUpstreamOIDCIdentityProvider) PerformRefresh(ctx context.Context, r
 	return u.PerformRefreshFunc(ctx, refreshToken)
 }
 
-func (u *TestUpstreamOIDCIdentityProvider) RevokeRefreshToken(ctx context.Context, refreshToken string) error {
+func (u *TestUpstreamOIDCIdentityProvider) RevokeToken(ctx context.Context, token string, tokenType provider.RevocableTokenType) error {
-	if u.revokeRefreshTokenArgs == nil {
+	if u.revokeTokenArgs == nil {
-		u.revokeRefreshTokenArgs = make([]*RevokeRefreshTokenArgs, 0)
+		u.revokeTokenArgs = make([]*RevokeTokenArgs, 0)
 	}
-	u.revokeRefreshTokenCallCount++
+	u.revokeTokenCallCount++
-	u.revokeRefreshTokenArgs = append(u.revokeRefreshTokenArgs, &RevokeRefreshTokenArgs{
+	u.revokeTokenArgs = append(u.revokeTokenArgs, &RevokeTokenArgs{
 		Ctx:       ctx,
-		RefreshToken: refreshToken,
+		Token:     token,
+		TokenType: tokenType,
 	})
-	return u.RevokeRefreshTokenFunc(ctx, refreshToken)
+	return u.RevokeTokenFunc(ctx, token, tokenType)
 }
 
 func (u *TestUpstreamOIDCIdentityProvider) PerformRefreshCallCount() int {
@@ -307,39 +316,41 @@ func (u *TestUpstreamOIDCIdentityProvider) PerformRefreshArgs(call int) *Perform
 	return u.performRefreshArgs[call]
 }
 
-func (u *TestUpstreamOIDCIdentityProvider) RevokeRefreshTokenCallCount() int {
+func (u *TestUpstreamOIDCIdentityProvider) RevokeTokenCallCount() int {
 	return u.performRefreshCallCount
 }
 
-func (u *TestUpstreamOIDCIdentityProvider) RevokeRefreshTokenArgs(call int) *RevokeRefreshTokenArgs {
+func (u *TestUpstreamOIDCIdentityProvider) RevokeTokenArgs(call int) *RevokeTokenArgs {
-	if u.revokeRefreshTokenArgs == nil {
+	if u.revokeTokenArgs == nil {
-		u.revokeRefreshTokenArgs = make([]*RevokeRefreshTokenArgs, 0)
+		u.revokeTokenArgs = make([]*RevokeTokenArgs, 0)
 	}
-	return u.revokeRefreshTokenArgs[call]
+	return u.revokeTokenArgs[call]
 }
 
-func (u *TestUpstreamOIDCIdentityProvider) ValidateTokenAndMergeWithUserInfo(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error) {
+func (u *TestUpstreamOIDCIdentityProvider) ValidateTokenAndMergeWithUserInfo(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool, requireUserInfo bool) (*oidctypes.Token, error) {
-	if u.validateTokenArgs == nil {
+	if u.validateTokenAndMergeWithUserInfoArgs == nil {
-		u.validateTokenArgs = make([]*ValidateTokenArgs, 0)
+		u.validateTokenAndMergeWithUserInfoArgs = make([]*ValidateTokenAndMergeWithUserInfoArgs, 0)
 	}
-	u.validateTokenCallCount++
+	u.validateTokenAndMergeWithUserInfoCallCount++
-	u.validateTokenArgs = append(u.validateTokenArgs, &ValidateTokenArgs{
+	u.validateTokenAndMergeWithUserInfoArgs = append(u.validateTokenAndMergeWithUserInfoArgs, &ValidateTokenAndMergeWithUserInfoArgs{
 		Ctx:                  ctx,
 		Tok:                  tok,
 		ExpectedIDTokenNonce: expectedIDTokenNonce,
+		RequireIDToken:       requireIDToken,
+		RequireUserInfo:      requireUserInfo,
 	})
-	return u.ValidateTokenFunc(ctx, tok, expectedIDTokenNonce)
+	return u.ValidateTokenAndMergeWithUserInfoFunc(ctx, tok, expectedIDTokenNonce)
 }
 
-func (u *TestUpstreamOIDCIdentityProvider) ValidateTokenCallCount() int {
+func (u *TestUpstreamOIDCIdentityProvider) ValidateTokenAndMergeWithUserInfoCallCount() int {
-	return u.validateTokenCallCount
+	return u.validateTokenAndMergeWithUserInfoCallCount
 }
 
-func (u *TestUpstreamOIDCIdentityProvider) ValidateTokenArgs(call int) *ValidateTokenArgs {
+func (u *TestUpstreamOIDCIdentityProvider) ValidateTokenAndMergeWithUserInfoArgs(call int) *ValidateTokenAndMergeWithUserInfoArgs {
-	if u.validateTokenArgs == nil {
+	if u.validateTokenAndMergeWithUserInfoArgs == nil {
-		u.validateTokenArgs = make([]*ValidateTokenArgs, 0)
+		u.validateTokenAndMergeWithUserInfoArgs = make([]*ValidateTokenAndMergeWithUserInfoArgs, 0)
 	}
-	return u.validateTokenArgs[call]
+	return u.validateTokenAndMergeWithUserInfoArgs[call]
 }
 
 type UpstreamIDPListerBuilder struct {
@@ -524,18 +535,18 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToPerformRefresh(t *te
 func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToValidateToken(
 	t *testing.T,
 	expectedPerformedByUpstreamName string,
-	expectedArgs *ValidateTokenArgs,
+	expectedArgs *ValidateTokenAndMergeWithUserInfoArgs,
 ) {
 	t.Helper()
-	var actualArgs *ValidateTokenArgs
+	var actualArgs *ValidateTokenAndMergeWithUserInfoArgs
 	var actualNameOfUpstreamWhichMadeCall string
 	actualCallCountAcrossAllOIDCUpstreams := 0
 	for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders {
-		callCountOnThisUpstream := upstreamOIDC.validateTokenCallCount
+		callCountOnThisUpstream := upstreamOIDC.validateTokenAndMergeWithUserInfoCallCount
 		actualCallCountAcrossAllOIDCUpstreams += callCountOnThisUpstream
 		if callCountOnThisUpstream == 1 {
 			actualNameOfUpstreamWhichMadeCall = upstreamOIDC.Name
-			actualArgs = upstreamOIDC.validateTokenArgs[0]
+			actualArgs = upstreamOIDC.validateTokenAndMergeWithUserInfoArgs[0]
 		}
 	}
 	require.Equal(t, 1, actualCallCountAcrossAllOIDCUpstreams,
@@ -551,47 +562,47 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToValidateToken(t *tes
 	t.Helper()
 	actualCallCountAcrossAllOIDCUpstreams := 0
 	for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders {
-		actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.validateTokenCallCount
+		actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.validateTokenAndMergeWithUserInfoCallCount
 	}
 	require.Equal(t, 0, actualCallCountAcrossAllOIDCUpstreams,
 		"expected exactly zero calls to ValidateTokenAndMergeWithUserInfo()",
 	)
 }
 
-func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToRevokeRefreshToken(
+func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToRevokeToken(
 	t *testing.T,
 	expectedPerformedByUpstreamName string,
-	expectedArgs *RevokeRefreshTokenArgs,
+	expectedArgs *RevokeTokenArgs,
 ) {
 	t.Helper()
-	var actualArgs *RevokeRefreshTokenArgs
+	var actualArgs *RevokeTokenArgs
 	var actualNameOfUpstreamWhichMadeCall string
 	actualCallCountAcrossAllOIDCUpstreams := 0
 	for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders {
-		callCountOnThisUpstream := upstreamOIDC.revokeRefreshTokenCallCount
+		callCountOnThisUpstream := upstreamOIDC.revokeTokenCallCount
 		actualCallCountAcrossAllOIDCUpstreams += callCountOnThisUpstream
 		if callCountOnThisUpstream == 1 {
 			actualNameOfUpstreamWhichMadeCall = upstreamOIDC.Name
-			actualArgs = upstreamOIDC.revokeRefreshTokenArgs[0]
+			actualArgs = upstreamOIDC.revokeTokenArgs[0]
 		}
 	}
 	require.Equal(t, 1, actualCallCountAcrossAllOIDCUpstreams,
-		"should have been exactly one call to RevokeRefreshToken() by all OIDC upstreams",
+		"should have been exactly one call to RevokeToken() by all OIDC upstreams",
 	)
 	require.Equal(t, expectedPerformedByUpstreamName, actualNameOfUpstreamWhichMadeCall,
-		"RevokeRefreshToken() was called on the wrong OIDC upstream",
+		"RevokeToken() was called on the wrong OIDC upstream",
 	)
 	require.Equal(t, expectedArgs, actualArgs)
 }
 
-func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToRevokeRefreshToken(t *testing.T) {
+func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToRevokeToken(t *testing.T) {
 	t.Helper()
 	actualCallCountAcrossAllOIDCUpstreams := 0
 	for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders {
-		actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.revokeRefreshTokenCallCount
+		actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.revokeTokenCallCount
 	}
 	require.Equal(t, 0, actualCallCountAcrossAllOIDCUpstreams,
-		"expected exactly zero calls to RevokeRefreshToken()",
+		"expected exactly zero calls to RevokeToken()",
 	)
 }
 
@@ -606,18 +617,20 @@ type TestUpstreamOIDCIdentityProviderBuilder struct {
 	scopes                               []string
 	idToken                              map[string]interface{}
 	refreshToken                         *oidctypes.RefreshToken
+	accessToken                          *oidctypes.AccessToken
 	usernameClaim                        string
 	groupsClaim                          string
 	refreshedTokens                      *oauth2.Token
-	validatedTokens          *oidctypes.Token
+	validatedAndMergedWithUserInfoTokens *oidctypes.Token
 	authorizationURL                     url.URL
+	hasUserInfoURL                       bool
 	additionalAuthcodeParams             map[string]string
 	allowPasswordGrant                   bool
 	authcodeExchangeErr                  error
 	passwordGrantErr                     error
 	performRefreshErr                    error
-	revokeRefreshTokenErr    error
+	revokeTokenErr                       error
-	validateTokenErr         error
+	validateTokenAndMergeWithUserInfoErr error
 }
 
 func (u *TestUpstreamOIDCIdentityProviderBuilder) WithName(value string) *TestUpstreamOIDCIdentityProviderBuilder {
@@ -640,6 +653,16 @@ func (u *TestUpstreamOIDCIdentityProviderBuilder) WithAuthorizationURL(value url
 	return u
 }
 
+func (u *TestUpstreamOIDCIdentityProviderBuilder) WithUserInfoURL() *TestUpstreamOIDCIdentityProviderBuilder {
+	u.hasUserInfoURL = true
+	return u
+}
+
+func (u *TestUpstreamOIDCIdentityProviderBuilder) WithoutUserInfoURL() *TestUpstreamOIDCIdentityProviderBuilder {
+	u.hasUserInfoURL = false
+	return u
+}
+
 func (u *TestUpstreamOIDCIdentityProviderBuilder) WithAllowPasswordGrant(value bool) *TestUpstreamOIDCIdentityProviderBuilder {
 	u.allowPasswordGrant = value
 	return u
@@ -703,6 +726,20 @@ func (u *TestUpstreamOIDCIdentityProviderBuilder) WithoutRefreshToken() *TestUps
 	return u
 }
 
+func (u *TestUpstreamOIDCIdentityProviderBuilder) WithAccessToken(token string) *TestUpstreamOIDCIdentityProviderBuilder {
+	u.accessToken = &oidctypes.AccessToken{Token: token}
+	return u
+}
+
+func (u *TestUpstreamOIDCIdentityProviderBuilder) WithEmptyAccessToken() *TestUpstreamOIDCIdentityProviderBuilder {
+	u.accessToken = &oidctypes.AccessToken{Token: ""}
+	return u
+}
+func (u *TestUpstreamOIDCIdentityProviderBuilder) WithoutAccessToken() *TestUpstreamOIDCIdentityProviderBuilder {
+	u.accessToken = nil
+	return u
+}
+
 func (u *TestUpstreamOIDCIdentityProviderBuilder) WithUpstreamAuthcodeExchangeError(err error) *TestUpstreamOIDCIdentityProviderBuilder {
 	u.authcodeExchangeErr = err
 	return u
@@ -723,18 +760,18 @@ func (u *TestUpstreamOIDCIdentityProviderBuilder) WithPerformRefreshError(err er
 	return u
 }
 
-func (u *TestUpstreamOIDCIdentityProviderBuilder) WithValidatedTokens(tokens *oidctypes.Token) *TestUpstreamOIDCIdentityProviderBuilder {
+func (u *TestUpstreamOIDCIdentityProviderBuilder) WithValidatedAndMergedWithUserInfoTokens(tokens *oidctypes.Token) *TestUpstreamOIDCIdentityProviderBuilder {
-	u.validatedTokens = tokens
+	u.validatedAndMergedWithUserInfoTokens = tokens
 	return u
 }
 
-func (u *TestUpstreamOIDCIdentityProviderBuilder) WithValidateTokenError(err error) *TestUpstreamOIDCIdentityProviderBuilder {
+func (u *TestUpstreamOIDCIdentityProviderBuilder) WithValidateTokenAndMergeWithUserInfoError(err error) *TestUpstreamOIDCIdentityProviderBuilder {
-	u.validateTokenErr = err
+	u.validateTokenAndMergeWithUserInfoErr = err
 	return u
 }
 
-func (u *TestUpstreamOIDCIdentityProviderBuilder) WithRevokeRefreshTokenError(err error) *TestUpstreamOIDCIdentityProviderBuilder {
+func (u *TestUpstreamOIDCIdentityProviderBuilder) WithRevokeTokenError(err error) *TestUpstreamOIDCIdentityProviderBuilder {
-	u.revokeRefreshTokenErr = err
+	u.revokeTokenErr = err
 	return u
 }
 
@@ -748,18 +785,19 @@ func (u *TestUpstreamOIDCIdentityProviderBuilder) Build() *TestUpstreamOIDCIdent
 		Scopes:                   u.scopes,
 		AllowPasswordGrant:       u.allowPasswordGrant,
 		AuthorizationURL:         u.authorizationURL,
+		UserInfoURL:              u.hasUserInfoURL,
 		AdditionalAuthcodeParams: u.additionalAuthcodeParams,
 		ExchangeAuthcodeAndValidateTokensFunc: func(ctx context.Context, authcode string, pkceCodeVerifier pkce.Code, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) {
 			if u.authcodeExchangeErr != nil {
 				return nil, u.authcodeExchangeErr
 			}
-			return &oidctypes.Token{IDToken: &oidctypes.IDToken{Claims: u.idToken}, RefreshToken: u.refreshToken}, nil
+			return &oidctypes.Token{IDToken: &oidctypes.IDToken{Claims: u.idToken}, RefreshToken: u.refreshToken, AccessToken: u.accessToken}, nil
 		},
 		PasswordCredentialsGrantAndValidateTokensFunc: func(ctx context.Context, username, password string) (*oidctypes.Token, error) {
 			if u.passwordGrantErr != nil {
 				return nil, u.passwordGrantErr
 			}
-			return &oidctypes.Token{IDToken: &oidctypes.IDToken{Claims: u.idToken}, RefreshToken: u.refreshToken}, nil
+			return &oidctypes.Token{IDToken: &oidctypes.IDToken{Claims: u.idToken}, RefreshToken: u.refreshToken, AccessToken: u.accessToken}, nil
 		},
 		PerformRefreshFunc: func(ctx context.Context, refreshToken string) (*oauth2.Token, error) {
 			if u.performRefreshErr != nil {
@@ -767,14 +805,14 @@ func (u *TestUpstreamOIDCIdentityProviderBuilder) Build() *TestUpstreamOIDCIdent
 			}
 			return u.refreshedTokens, nil
 		},
-		RevokeRefreshTokenFunc: func(ctx context.Context, refreshToken string) error {
+		RevokeTokenFunc: func(ctx context.Context, refreshToken string, tokenType provider.RevocableTokenType) error {
-			return u.revokeRefreshTokenErr
+			return u.revokeTokenErr
 		},
-		ValidateTokenFunc: func(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) {
+		ValidateTokenAndMergeWithUserInfoFunc: func(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) {
-			if u.validateTokenErr != nil {
+			if u.validateTokenAndMergeWithUserInfoErr != nil {
-				return nil, u.validateTokenErr
+				return nil, u.validateTokenAndMergeWithUserInfoErr
 			}
-			return u.validatedTokens, nil
+			return u.validatedAndMergedWithUserInfoTokens, nil
 		},
 	}
 }

internal/testutil/testlogger/stdr_copied.go

@@ -1,4 +1,4 @@
-// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package testlogger

internal/testutil/testlogger/testlogger.go

@@ -1,4 +1,4 @@
-// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package testlogger wraps logr.Logger to allow for writing test assertions.

internal/testutil/transcript_logger.go

@@ -1,4 +1,4 @@
-// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package testutil

internal/upstreamldap/upstreamldap.go

@@ -1,4 +1,4 @@
-// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package upstreamldap implements an abstraction of upstream LDAP IDP interactions.

internal/upstreamldap/upstreamldap_test.go

@@ -1,4 +1,4 @@
-// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package upstreamldap

internal/upstreamoidc/upstreamoidc.go

@@ -61,6 +61,19 @@ func (p *ProviderConfig) GetRevocationURL() *url.URL {
 	return p.RevocationURL
 }
 
+func (p *ProviderConfig) HasUserInfoURL() bool {
+	providerJSON := &struct {
+		UserInfoURL string `json:"userinfo_endpoint"`
+	}{}
+	if err := p.Provider.Claims(providerJSON); err != nil {
+		// This should never happen in practice because we should have already successfully
+		// parsed these claims when p.Provider was created.
+		return false
+	}
+
+	return len(providerJSON.UserInfoURL) > 0
+}
+
 func (p *ProviderConfig) GetAdditionalAuthcodeParams() map[string]string {
 	return p.AdditionalAuthcodeParams
 }
@@ -113,7 +126,7 @@ func (p *ProviderConfig) PasswordCredentialsGrantAndValidateTokens(ctx context.C
 	// There is no nonce to validate for a resource owner password credentials grant because it skips using
 	// the authorize endpoint and goes straight to the token endpoint.
 	const skipNonceValidation nonce.Nonce = ""
-	return p.ValidateTokenAndMergeWithUserInfo(ctx, tok, skipNonceValidation, true)
+	return p.ValidateTokenAndMergeWithUserInfo(ctx, tok, skipNonceValidation, true, false)
 }
 
 func (p *ProviderConfig) ExchangeAuthcodeAndValidateTokens(ctx context.Context, authcode string, pkceCodeVerifier pkce.Code, expectedIDTokenNonce nonce.Nonce, redirectURI string) (*oidctypes.Token, error) {
@@ -127,7 +140,7 @@ func (p *ProviderConfig) ExchangeAuthcodeAndValidateTokens(ctx context.Context,
 		return nil, err
 	}
 
-	return p.ValidateTokenAndMergeWithUserInfo(ctx, tok, expectedIDTokenNonce, true)
+	return p.ValidateTokenAndMergeWithUserInfo(ctx, tok, expectedIDTokenNonce, true, false)
 }
 
 func (p *ProviderConfig) PerformRefresh(ctx context.Context, refreshToken string) (*oauth2.Token, error) {
@@ -138,32 +151,39 @@ func (p *ProviderConfig) PerformRefresh(ctx context.Context, refreshToken string
 	return p.Config.TokenSource(httpClientContext, &oauth2.Token{RefreshToken: refreshToken}).Token()
 }
 
-// RevokeRefreshToken will attempt to revoke the given token, if the provider has a revocation endpoint.
+// RevokeToken will attempt to revoke the given token, if the provider has a revocation endpoint.
-func (p *ProviderConfig) RevokeRefreshToken(ctx context.Context, refreshToken string) error {
+// It may return an error wrapped by a RetryableRevocationError, which is an error indicating that it may
+// be worth trying to revoke the same token again later. Any other error returned should be assumed to
+// represent an error such that it is not worth retrying revocation later, even though revocation failed.
+func (p *ProviderConfig) RevokeToken(ctx context.Context, token string, tokenType provider.RevocableTokenType) error {
 	if p.RevocationURL == nil {
-		plog.Trace("RevokeRefreshToken() was called but upstream provider has no available revocation endpoint", "providerName", p.Name)
+		plog.Trace("RevokeToken() was called but upstream provider has no available revocation endpoint",
+			"providerName", p.Name,
+			"tokenType", tokenType,
+		)
 		return nil
 	}
 	// First try using client auth in the request params.
-	tryAnotherClientAuthMethod, err := p.tryRevokeRefreshToken(ctx, refreshToken, false)
+	tryAnotherClientAuthMethod, err := p.tryRevokeToken(ctx, token, tokenType, false)
 	if tryAnotherClientAuthMethod {
 		// Try again using basic auth this time. Overwrite the first client auth error,
 		// which isn't useful anymore when retrying.
-		_, err = p.tryRevokeRefreshToken(ctx, refreshToken, true)
+		_, err = p.tryRevokeToken(ctx, token, tokenType, true)
 	}
 	return err
 }
 
-// tryRevokeRefreshToken will call the revocation endpoint using either basic auth or by including
+// tryRevokeToken will call the revocation endpoint using either basic auth or by including
 // client auth in the request params. It will return an error when the request failed. If the
 // request failed for a reason that might be due to bad client auth, then it will return true
 // for the tryAnotherClientAuthMethod return value, indicating that it might be worth trying
 // again using the other client auth method.
 // RFC 7009 defines how to make a revocation request and how to interpret the response.
 // See https://datatracker.ietf.org/doc/html/rfc7009#section-2.1 for details.
-func (p *ProviderConfig) tryRevokeRefreshToken(
+func (p *ProviderConfig) tryRevokeToken(
 	ctx context.Context,
-	refreshToken string,
+	token string,
+	tokenType provider.RevocableTokenType,
 	useBasicAuth bool,
 ) (tryAnotherClientAuthMethod bool, err error) {
 	clientID := p.Config.ClientID
@@ -172,8 +192,8 @@ func (p *ProviderConfig) tryRevokeRefreshToken(
 	httpClient := p.Client
 
 	params := url.Values{
-		"token":           []string{refreshToken},
+		"token":           []string{token},
-		"token_type_hint": []string{"refresh_token"},
+		"token_type_hint": []string{string(tokenType)},
 	}
 	if !useBasicAuth {
 		params["client_id"] = []string{clientID}
@@ -194,22 +214,25 @@ func (p *ProviderConfig) tryRevokeRefreshToken(
 	resp, err := httpClient.Do(req)
 	if err != nil {
 		// Couldn't connect to the server or some similar error.
-		return false, err
+		// Could be a temporary network problem, so it might be worth retrying.
+		return false, provider.NewRetryableRevocationError(err)
 	}
 	defer resp.Body.Close()
 
-	switch resp.StatusCode {
+	status := resp.StatusCode
-	case http.StatusOK:
+
+	switch {
+	case status == http.StatusOK:
 		// Success!
-		plog.Trace("RevokeRefreshToken() got 200 OK response from provider's revocation endpoint", "providerName", p.Name, "usedBasicAuth", useBasicAuth)
+		plog.Trace("RevokeToken() got 200 OK response from provider's revocation endpoint", "providerName", p.Name, "usedBasicAuth", useBasicAuth)
 		return false, nil
-	case http.StatusBadRequest:
+	case status == http.StatusBadRequest:
 		// Bad request might be due to bad client auth method. Try to detect that.
-		plog.Trace("RevokeRefreshToken() got 400 Bad Request response from provider's revocation endpoint", "providerName", p.Name, "usedBasicAuth", useBasicAuth)
+		plog.Trace("RevokeToken() got 400 Bad Request response from provider's revocation endpoint", "providerName", p.Name, "usedBasicAuth", useBasicAuth)
 		body, err := io.ReadAll(resp.Body)
 		if err != nil {
 			return false,
-				fmt.Errorf("error reading response body on response with status code %d: %w", resp.StatusCode, err)
+				fmt.Errorf("error reading response body on response with status code %d: %w", status, err)
 		}
 		var parsedResp struct {
 			ErrorType        string `json:"error"`
@@ -219,27 +242,40 @@ func (p *ProviderConfig) tryRevokeRefreshToken(
 		err = json.Unmarshal(body, &parsedResp)
 		if err != nil {
 			return false,
-				fmt.Errorf("error parsing response body %q on response with status code %d: %w", bodyStr, resp.StatusCode, err)
+				fmt.Errorf("error parsing response body %q on response with status code %d: %w", bodyStr, status, err)
 		}
-		err = fmt.Errorf("server responded with status %d with body: %s", resp.StatusCode, bodyStr)
+		err = fmt.Errorf("server responded with status %d with body: %s", status, bodyStr)
 		if parsedResp.ErrorType != "invalid_client" {
-			// Got an error unrelated to client auth, so not worth trying again.
+			// Got an error unrelated to client auth, so not worth trying client auth again. Also, these are errors
+			// of the type where the server is pretty conclusively rejecting our request, so they are generally
+			// not worth trying again later either.
+			// These errors could be any of the other errors from https://datatracker.ietf.org/doc/html/rfc6749#section-5.2
+			// or "unsupported_token_type" from https://datatracker.ietf.org/doc/html/rfc7009#section-2.2.1
+			// or could be some unspecified custom error added by the OIDC provider.
 			return false, err
 		}
 		// Got an "invalid_client" response, which might mean client auth failed, so it may be worth trying again
 		// using another client auth method. See https://datatracker.ietf.org/doc/html/rfc6749#section-5.2
-		plog.Trace("RevokeRefreshToken()'s 400 Bad Request response from provider's revocation endpoint was type 'invalid_client'", "providerName", p.Name, "usedBasicAuth", useBasicAuth)
+		plog.Trace("RevokeToken()'s 400 Bad Request response from provider's revocation endpoint was type 'invalid_client'", "providerName", p.Name, "usedBasicAuth", useBasicAuth)
 		return true, err
+	case status >= 500 && status <= 599:
+		// The spec says 503 Service Unavailable should be retried by the client later.
+		// See https://datatracker.ietf.org/doc/html/rfc7009#section-2.2.1.
+		// Other forms of 5xx server errors are not particularly conclusive failures. For example, gateway errors could
+		// be caused by an underlying problem which could potentially become resolved in the near future. We'll be
+		// optimistic and call all 5xx errors retryable.
+		plog.Trace("RevokeToken() got unexpected error response from provider's revocation endpoint", "providerName", p.Name, "usedBasicAuth", useBasicAuth, "statusCode", status)
+		return false, provider.NewRetryableRevocationError(fmt.Errorf("server responded with status %d", status))
 	default:
-		// Any other error is probably not due to failed client auth.
+		// Any other error is probably not due to failed client auth, and is probably not worth retrying later.
-		plog.Trace("RevokeRefreshToken() got unexpected error response from provider's revocation endpoint", "providerName", p.Name, "usedBasicAuth", useBasicAuth, "statusCode", resp.StatusCode)
+		plog.Trace("RevokeToken() got unexpected error response from provider's revocation endpoint", "providerName", p.Name, "usedBasicAuth", useBasicAuth, "statusCode", status)
-		return false, fmt.Errorf("server responded with status %d", resp.StatusCode)
+		return false, fmt.Errorf("server responded with status %d", status)
 	}
 }
 
 // ValidateTokenAndMergeWithUserInfo will validate the ID token. It will also merge the claims from the userinfo endpoint response,
 // if the provider offers the userinfo endpoint.
-func (p *ProviderConfig) ValidateTokenAndMergeWithUserInfo(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool) (*oidctypes.Token, error) {
+func (p *ProviderConfig) ValidateTokenAndMergeWithUserInfo(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce, requireIDToken bool, requireUserInfo bool) (*oidctypes.Token, error) {
 	var validatedClaims = make(map[string]interface{})
 
 	var idTokenExpiry time.Time
@@ -255,7 +291,7 @@ func (p *ProviderConfig) ValidateTokenAndMergeWithUserInfo(ctx context.Context,
 	if len(idTokenSubject) > 0 || !requireIDToken {
 		// only fetch userinfo if the ID token has a subject or if we are ignoring the id token completely.
 		// otherwise, defer to existing ID token validation
-		if err := p.maybeFetchUserInfoAndMergeClaims(ctx, tok, validatedClaims, requireIDToken); err != nil {
+		if err := p.maybeFetchUserInfoAndMergeClaims(ctx, tok, validatedClaims, requireIDToken, requireUserInfo); err != nil {
 			return nil, httperr.Wrap(http.StatusInternalServerError, "could not fetch user info claims", err)
 		}
 	}
@@ -309,10 +345,10 @@ func (p *ProviderConfig) validateIDToken(ctx context.Context, tok *oauth2.Token,
 	return idTokenExpiry, idTok, nil
 }
 
-func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, tok *oauth2.Token, claims map[string]interface{}, requireIDToken bool) error {
+func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, tok *oauth2.Token, claims map[string]interface{}, requireIDToken bool, requireUserInfo bool) error {
 	idTokenSubject, _ := claims[oidc.IDTokenSubjectClaim].(string)
 
-	userInfo, err := p.maybeFetchUserInfo(ctx, tok)
+	userInfo, err := p.maybeFetchUserInfo(ctx, tok, requireUserInfo)
 	if err != nil {
 		return err
 	}
@@ -355,17 +391,13 @@ func (p *ProviderConfig) maybeFetchUserInfoAndMergeClaims(ctx context.Context, t
 	return nil
 }
 
-func (p *ProviderConfig) maybeFetchUserInfo(ctx context.Context, tok *oauth2.Token) (*coreosoidc.UserInfo, error) {
+func (p *ProviderConfig) maybeFetchUserInfo(ctx context.Context, tok *oauth2.Token, requireUserInfo bool) (*coreosoidc.UserInfo, error) {
-	providerJSON := &struct {
+	// implementing the user info endpoint is not required by the OIDC spec, but we may require it in certain situations.
-		UserInfoURL string `json:"userinfo_endpoint"`
+	if !p.HasUserInfoURL() {
-	}{}
+		if requireUserInfo {
-	if err := p.Provider.Claims(providerJSON); err != nil {
+			// TODO should these all be http errors?
-		// this should never happen because we should have already parsed these claims at an earlier stage
+			return nil, httperr.New(http.StatusInternalServerError, "userinfo endpoint not found, but is required")
-		return nil, httperr.Wrap(http.StatusInternalServerError, "could not unmarshal discovery JSON", err)
 		}
-
-	// implementing the user info endpoint is not required, skip this logic when it is absent
-	if len(providerJSON.UserInfoURL) == 0 {
 		return nil, nil
 	}
 

internal/upstreamoidc/upstreamoidc_test.go

@@ -24,6 +24,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"go.pinniped.dev/internal/mocks/mockkeyset"
+	"go.pinniped.dev/internal/oidc/provider"
 	"go.pinniped.dev/internal/testutil"
 	"go.pinniped.dev/pkg/oidcclient/nonce"
 	"go.pinniped.dev/pkg/oidcclient/oidctypes"
@@ -40,6 +41,9 @@ func TestProviderConfig(t *testing.T) {
 				Endpoint: oauth2.Endpoint{AuthURL: "https://example.com"},
 				Scopes:   []string{"scope1", "scope2"},
 			},
+			Provider: &mockProvider{
+				rawClaims: []byte(`{"userinfo_endpoint": "https://example.com/userinfo"}`),
+			},
 		}
 		require.Equal(t, "test-name", p.GetName())
 		require.Equal(t, "test-client-id", p.GetClientID())
@@ -54,6 +58,16 @@ func TestProviderConfig(t *testing.T) {
 		require.True(t, p.AllowsPasswordGrant())
 		p.AllowPasswordGrant = false
 		require.False(t, p.AllowsPasswordGrant())
+
+		require.True(t, p.HasUserInfoURL())
+		p.Provider = &mockProvider{
+			rawClaims: []byte(`{"some_other_endpoint": "https://example.com/blah"}`),
+		}
+		require.False(t, p.HasUserInfoURL())
+		p.Provider = &mockProvider{
+			rawClaims: []byte(`{`),
+		}
+		require.False(t, p.HasUserInfoURL())
 	})
 
 	const (
@@ -455,73 +469,171 @@ func TestProviderConfig(t *testing.T) {
 		}
 	})
 
-	t.Run("RevokeRefreshToken", func(t *testing.T) {
+	t.Run("RevokeToken", func(t *testing.T) {
 		tests := []struct {
 			name                 string
+			tokenType            provider.RevocableTokenType
 			nilRevocationURL     bool
-			statusCodes      []int
+			unreachableServer    bool
+			returnStatusCodes    []int
 			returnErrBodies      []string
 			wantErr              string
+			wantErrRegexp        string // use either wantErr or wantErrRegexp
+			wantRetryableErrType bool   // additionally assert error type when wantErr is non-empty
 			wantNumRequests      int
+			wantTokenTypeHint    string
 		}{
 			{
-				name:             "success without calling the server when there is no revocation URL set",
+				name:             "success without calling the server when there is no revocation URL set for refresh token",
+				tokenType:        provider.RefreshTokenType,
 				nilRevocationURL: true,
 				wantNumRequests:  0,
 			},
 			{
-				name:            "success when the server returns 200 OK on the first call",
+				name:             "success without calling the server when there is no revocation URL set for access token",
-				statusCodes:     []int{http.StatusOK},
+				tokenType:        provider.AccessTokenType,
-				wantNumRequests: 1,
+				nilRevocationURL: true,
+				wantNumRequests:  0,
 			},
 			{
-				name:        "success when the server returns 400 Bad Request on the first call due to client auth, then 200 OK on second call",
+				name:              "success when the server returns 200 OK on the first call for refresh token",
-				statusCodes: []int{http.StatusBadRequest, http.StatusOK},
+				tokenType:         provider.RefreshTokenType,
+				returnStatusCodes: []int{http.StatusOK},
+				wantNumRequests:   1,
+				wantTokenTypeHint: "refresh_token",
+			},
+			{
+				name:              "success when the server returns 200 OK on the first call for access token",
+				tokenType:         provider.AccessTokenType,
+				returnStatusCodes: []int{http.StatusOK},
+				wantNumRequests:   1,
+				wantTokenTypeHint: "access_token",
+			},
+			{
+				name:              "success when the server returns 400 Bad Request on the first call due to client auth, then 200 OK on second call for refresh token",
+				tokenType:         provider.RefreshTokenType,
+				returnStatusCodes: []int{http.StatusBadRequest, http.StatusOK},
 				// https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 defines this as the error for client auth failure
 				returnErrBodies:   []string{`{ "error":"invalid_client", "error_description":"unhappy" }`},
 				wantNumRequests:   2,
+				wantTokenTypeHint: "refresh_token",
+			},
+			{
+				name:              "success when the server returns 400 Bad Request on the first call due to client auth, then 200 OK on second call for access token",
+				tokenType:         provider.AccessTokenType,
+				returnStatusCodes: []int{http.StatusBadRequest, http.StatusOK},
+				// https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 defines this as the error for client auth failure
+				returnErrBodies:   []string{`{ "error":"invalid_client", "error_description":"unhappy" }`},
+				wantNumRequests:   2,
+				wantTokenTypeHint: "access_token",
 			},
 			{
 				name:                 "error when the server returns 400 Bad Request on the first call due to client auth, then any 400 error on second call",
-				statusCodes:     []int{http.StatusBadRequest, http.StatusBadRequest},
+				tokenType:            provider.RefreshTokenType,
+				returnStatusCodes:    []int{http.StatusBadRequest, http.StatusBadRequest},
 				returnErrBodies:      []string{`{ "error":"invalid_client", "error_description":"unhappy" }`, `{ "error":"anything", "error_description":"unhappy" }`},
 				wantErr:              `server responded with status 400 with body: { "error":"anything", "error_description":"unhappy" }`,
+				wantRetryableErrType: false,
 				wantNumRequests:      2,
+				wantTokenTypeHint:    "refresh_token",
 			},
 			{
 				name:                 "error when the server returns 400 Bad Request with bad JSON body on the first call",
-				statusCodes:     []int{http.StatusBadRequest},
+				tokenType:            provider.RefreshTokenType,
+				returnStatusCodes:    []int{http.StatusBadRequest},
 				returnErrBodies:      []string{`invalid JSON body`},
 				wantErr:              `error parsing response body "invalid JSON body" on response with status code 400: invalid character 'i' looking for beginning of value`,
+				wantRetryableErrType: false,
 				wantNumRequests:      1,
+				wantTokenTypeHint:    "refresh_token",
 			},
 			{
 				name:                 "error when the server returns 400 Bad Request with empty body",
-				statusCodes:     []int{http.StatusBadRequest},
+				tokenType:            provider.RefreshTokenType,
+				returnStatusCodes:    []int{http.StatusBadRequest},
 				returnErrBodies:      []string{``},
 				wantErr:              `error parsing response body "" on response with status code 400: unexpected end of JSON input`,
+				wantRetryableErrType: false,
 				wantNumRequests:      1,
+				wantTokenTypeHint:    "refresh_token",
 			},
 			{
 				name:                 "error when the server returns 400 Bad Request on the first call due to client auth, then any other error on second call",
-				statusCodes:     []int{http.StatusBadRequest, http.StatusForbidden},
+				tokenType:            provider.RefreshTokenType,
+				returnStatusCodes:    []int{http.StatusBadRequest, http.StatusForbidden},
 				returnErrBodies:      []string{`{ "error":"invalid_client", "error_description":"unhappy" }`, ""},
 				wantErr:              "server responded with status 403",
+				wantRetryableErrType: false,
 				wantNumRequests:      2,
+				wantTokenTypeHint:    "refresh_token",
 			},
 			{
 				name:                 "error when server returns any other 400 error on first call",
-				statusCodes:     []int{http.StatusBadRequest},
+				tokenType:            provider.RefreshTokenType,
+				returnStatusCodes:    []int{http.StatusBadRequest},
 				returnErrBodies:      []string{`{ "error":"anything_else", "error_description":"unhappy" }`},
 				wantErr:              `server responded with status 400 with body: { "error":"anything_else", "error_description":"unhappy" }`,
+				wantRetryableErrType: false,
 				wantNumRequests:      1,
+				wantTokenTypeHint:    "refresh_token",
 			},
 			{
 				name:                 "error when server returns any other error aside from 400 on first call",
-				statusCodes:     []int{http.StatusForbidden},
+				tokenType:            provider.RefreshTokenType,
+				returnStatusCodes:    []int{http.StatusForbidden},
 				returnErrBodies:      []string{""},
 				wantErr:              "server responded with status 403",
+				wantRetryableErrType: false,
 				wantNumRequests:      1,
+				wantTokenTypeHint:    "refresh_token",
+			},
+			{
+				name:                 "retryable error when server returns 503 on first call",
+				tokenType:            provider.RefreshTokenType,
+				returnStatusCodes:    []int{http.StatusServiceUnavailable}, // 503
+				returnErrBodies:      []string{""},
+				wantErr:              "retryable revocation error: server responded with status 503",
+				wantRetryableErrType: true,
+				wantNumRequests:      1,
+				wantTokenTypeHint:    "refresh_token",
+			},
+			{
+				name:                 "retryable error when the server returns 400 Bad Request on the first call due to client auth, then 503 on second call",
+				tokenType:            provider.AccessTokenType,
+				returnStatusCodes:    []int{http.StatusBadRequest, http.StatusServiceUnavailable}, // 400, 503
+				returnErrBodies:      []string{`{ "error":"invalid_client", "error_description":"unhappy" }`, ""},
+				wantErr:              "retryable revocation error: server responded with status 503",
+				wantRetryableErrType: true,
+				wantNumRequests:      2,
+				wantTokenTypeHint:    "access_token",
+			},
+			{
+				name:                 "retryable error when server returns any 5xx status on first call, testing lower bound of 5xx range",
+				tokenType:            provider.RefreshTokenType,
+				returnStatusCodes:    []int{http.StatusInternalServerError}, // 500
+				returnErrBodies:      []string{""},
+				wantErr:              "retryable revocation error: server responded with status 500",
+				wantRetryableErrType: true,
+				wantNumRequests:      1,
+				wantTokenTypeHint:    "refresh_token",
+			},
+			{
+				name:                 "retryable error when server returns any 5xx status on first call, testing upper bound of 5xx range",
+				tokenType:            provider.RefreshTokenType,
+				returnStatusCodes:    []int{599}, // not defined by an RFC, but sometimes considered Network Connect Timeout Error
+				returnErrBodies:      []string{""},
+				wantErr:              "retryable revocation error: server responded with status 599",
+				wantRetryableErrType: true,
+				wantNumRequests:      1,
+				wantTokenTypeHint:    "refresh_token",
+			},
+			{
+				name:                 "retryable error when the server cannot be reached",
+				tokenType:            provider.AccessTokenType,
+				unreachableServer:    true,
+				wantErrRegexp:        "^retryable revocation error: Post .*: dial tcp .*: connect: connection refused$",
+				wantRetryableErrType: true,
+				wantNumRequests:      0,
 			},
 		}
 		for _, tt := range tests {
@@ -536,23 +648,23 @@ func TestProviderConfig(t *testing.T) {
 					if numRequests == 1 {
 						// First request should use client_id/client_secret params.
 						require.Equal(t, 4, len(r.Form))
+						require.Equal(t, "test-upstream-token", r.Form.Get("token"))
+						require.Equal(t, tt.wantTokenTypeHint, r.Form.Get("token_type_hint"))
 						require.Equal(t, "test-client-id", r.Form.Get("client_id"))
 						require.Equal(t, "test-client-secret", r.Form.Get("client_secret"))
-						require.Equal(t, "refresh_token", r.Form.Get("token_type_hint"))
-						require.Equal(t, "test-initial-refresh-token", r.Form.Get("token"))
 					} else {
 						// Second request, if there is one, should use basic auth.
 						require.Equal(t, 2, len(r.Form))
-						require.Equal(t, "refresh_token", r.Form.Get("token_type_hint"))
+						require.Equal(t, "test-upstream-token", r.Form.Get("token"))
-						require.Equal(t, "test-initial-refresh-token", r.Form.Get("token"))
+						require.Equal(t, tt.wantTokenTypeHint, r.Form.Get("token_type_hint"))
 						username, password, hasBasicAuth := r.BasicAuth()
 						require.True(t, hasBasicAuth, "request should have had basic auth but did not")
 						require.Equal(t, "test-client-id", username)
 						require.Equal(t, "test-client-secret", password)
 					}
-					if tt.statusCodes[numRequests-1] != http.StatusOK {
+					if tt.returnStatusCodes[numRequests-1] != http.StatusOK {
 						w.Header().Set("content-type", "application/json")
-						http.Error(w, tt.returnErrBodies[numRequests-1], tt.statusCodes[numRequests-1])
+						http.Error(w, tt.returnErrBodies[numRequests-1], tt.returnStatusCodes[numRequests-1])
 					}
 					// Otherwise, responds with 200 OK and empty body by default.
 				}))
@@ -574,16 +686,35 @@ func TestProviderConfig(t *testing.T) {
 					p.RevocationURL = nil
 				}
 
-				err = p.RevokeRefreshToken(
+				if tt.unreachableServer {
+					tokenServer.Close() // make the sever unreachable by closing it before making any requests
+				}
+
+				err = p.RevokeToken(
 					context.Background(),
-					"test-initial-refresh-token",
+					"test-upstream-token",
+					tt.tokenType,
 				)
 
 				require.Equal(t, tt.wantNumRequests, numRequests,
 					"did not make expected number of requests to revocation endpoint")
 
+				if tt.wantErr != "" || tt.wantErrRegexp != "" { // nolint:nestif
 					if tt.wantErr != "" {
 						require.EqualError(t, err, tt.wantErr)
+					} else {
+						require.Error(t, err)
+						require.Regexp(t, tt.wantErrRegexp, err.Error())
+					}
+
+					if tt.wantRetryableErrType {
+						require.ErrorAs(t, err, &provider.RetryableRevocationError{})
+					} else if errors.As(err, &provider.RetryableRevocationError{}) {
+						// There is no NotErrorAs() assertion available in the current version of testify, so do the equivalent.
+						require.Fail(t, "error should not be As RetryableRevocationError")
+					}
+				} else {
+					require.NoError(t, err)
 				}
 			})
 		}
@@ -609,6 +740,7 @@ func TestProviderConfig(t *testing.T) {
 			tok              *oauth2.Token
 			nonce            nonce.Nonce
 			requireIDToken   bool
+			requireUserInfo  bool
 			userInfo         *oidc.UserInfo
 			rawClaims        []byte
 			userInfoErr      error
@@ -694,6 +826,34 @@ func TestProviderConfig(t *testing.T) {
 					},
 				},
 			},
+			{
+				name:            "userinfo is required, token with id, access and refresh tokens, valid nonce, and userinfo with a value that doesn't exist in the id token",
+				tok:             testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}),
+				nonce:           "some-nonce",
+				requireIDToken:  true,
+				requireUserInfo: true,
+				rawClaims:       []byte(`{"userinfo_endpoint": "not-empty"}`),
+				userInfo:        forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`),
+				wantMergedTokens: &oidctypes.Token{
+					AccessToken: &oidctypes.AccessToken{
+						Token:  "test-access-token",
+						Type:   "test-token-type",
+						Expiry: metav1.NewTime(expiryTime),
+					},
+					RefreshToken: &oidctypes.RefreshToken{
+						Token: "test-initial-refresh-token",
+					},
+					IDToken: &oidctypes.IDToken{
+						Token: goodIDToken,
+						Claims: map[string]interface{}{
+							"iss":   "some-issuer",
+							"nonce": "some-nonce",
+							"sub":   "some-subject",
+							"name":  "Pinny TheSeal",
+						},
+					},
+				},
+			},
 			{
 				name:           "claims from userinfo override id token claims",
 				tok:            testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJzb21lLXN1YmplY3QiLCJuYW1lIjoiSm9obiBEb2UiLCJpc3MiOiJzb21lLWlzc3VlciIsIm5vbmNlIjoic29tZS1ub25jZSJ9.sBWi3_4cfGwrmMFZWkCghw4uvCnHN35h9xNX1gkwOtj6Oz_yKqpj7wfO4AqeWsRyrDGnkmIZbVuhAAJqPSi4GlNzN4NU8zh53PGDUpFlpDI1dvqDjIRb9iIEJpRIj34--Sz41H0ooxviIzvUdZFvQlaSzLOqgjR3ddHe2urhbtUuz_DsabP84AWo2DSg0y3ull6DRvk_DvzC6HNN8JwVi08fFvvV9BVq8kjdVeob7gajJkuGSTjsxNZGs5rbBuxBx0MZTQ8boR1fDNdG70GoIb4SsCoBSs7pZxtmGZPHInteY1SilHDDDmpQuE-LvSmvvPN_Cyk1d3eS-IR7hBbCAA"}),
@@ -748,6 +908,32 @@ func TestProviderConfig(t *testing.T) {
 					},
 				},
 			},
+			{
+				name:            "token with id, access and refresh tokens and valid nonce, but no userinfo endpoint from discovery and it's not required",
+				tok:             testTokenWithoutIDToken.WithExtra(map[string]interface{}{"id_token": goodIDToken}),
+				nonce:           "some-nonce",
+				requireIDToken:  true,
+				requireUserInfo: false,
+				rawClaims:       []byte(`{"not_the_userinfo_endpoint": "some-other-endpoint"}`),
+				wantMergedTokens: &oidctypes.Token{
+					AccessToken: &oidctypes.AccessToken{
+						Token:  "test-access-token",
+						Type:   "test-token-type",
+						Expiry: metav1.NewTime(expiryTime),
+					},
+					RefreshToken: &oidctypes.RefreshToken{
+						Token: "test-initial-refresh-token",
+					},
+					IDToken: &oidctypes.IDToken{
+						Token: goodIDToken,
+						Claims: map[string]interface{}{
+							"iss":   "some-issuer",
+							"nonce": "some-nonce",
+							"sub":   "some-subject",
+						},
+					},
+				},
+			},
 			{
 				name:           "token with no id token but valid userinfo",
 				tok:            testTokenWithoutIDToken,
@@ -838,6 +1024,23 @@ func TestProviderConfig(t *testing.T) {
 				userInfo:       forceUserInfoWithClaims("some-subject", `{"name": "Pinny TheSeal", "sub": "some-subject"}`),
 				wantErr:        "received response missing ID token",
 			},
+			{
+				name:            "expected to have userinfo, but doesn't",
+				tok:             testTokenWithoutIDToken,
+				nonce:           "some-other-nonce",
+				requireUserInfo: true,
+				rawClaims:       []byte(`{}`),
+				wantErr:         "could not fetch user info claims: userinfo endpoint not found, but is required",
+			},
+			{
+				name:            "expected to have id token and userinfo, but doesn't have either",
+				tok:             testTokenWithoutIDToken,
+				nonce:           "some-other-nonce",
+				requireUserInfo: true,
+				requireIDToken:  true,
+				rawClaims:       []byte(`{}`),
+				wantErr:         "received response missing ID token",
+			},
 			{
 				name:           "mismatched access token hash",
 				tok:            testTokenWithoutIDToken,
@@ -898,7 +1101,7 @@ func TestProviderConfig(t *testing.T) {
 						userInfoErr: tt.userInfoErr,
 					},
 				}
-				gotTok, err := p.ValidateTokenAndMergeWithUserInfo(context.Background(), tt.tok, tt.nonce, tt.requireIDToken)
+				gotTok, err := p.ValidateTokenAndMergeWithUserInfo(context.Background(), tt.tok, tt.nonce, tt.requireIDToken, tt.requireUserInfo)
 				if tt.wantErr != "" {
 					require.Error(t, err)
 					require.Equal(t, tt.wantErr, err.Error())
@@ -982,6 +1185,36 @@ func TestProviderConfig(t *testing.T) {
 				rawClaims:          []byte(`{}`), // user info not supported
 				wantUserInfoCalled: false,
 			},
+			{
+				name:        "valid but userinfo endpoint could not be found due to parse error",
+				authCode:    "valid",
+				returnIDTok: validIDToken,
+				wantToken: oidctypes.Token{
+					AccessToken: &oidctypes.AccessToken{
+						Token:  "test-access-token",
+						Expiry: metav1.Time{},
+					},
+					RefreshToken: &oidctypes.RefreshToken{
+						Token: "test-refresh-token",
+					},
+					IDToken: &oidctypes.IDToken{
+						Token:  validIDToken,
+						Expiry: metav1.Time{},
+						Claims: map[string]interface{}{
+							"foo": "bar",
+							"bat": "baz",
+							"aud": "test-client-id",
+							"iat": 1.606768593e+09,
+							"jti": "test-jti",
+							"nbf": 1.606768593e+09,
+							"sub": "test-user",
+						},
+					},
+				},
+				// cannot be parsed as json, but note that in this case constructing a real provider would have failed
+				rawClaims:          []byte(`{`),
+				wantUserInfoCalled: false,
+			},
 			{
 				name:        "valid",
 				authCode:    "valid",
@@ -1011,13 +1244,6 @@ func TestProviderConfig(t *testing.T) {
 				rawClaims:          []byte(`{}`), // user info not supported
 				wantUserInfoCalled: false,
 			},
-			{
-				name:        "user info discovery parse error",
-				authCode:    "valid",
-				returnIDTok: validIDToken,
-				rawClaims:   []byte(`junk`), // user info discovery fails
-				wantErr:     "could not fetch user info claims: could not unmarshal discovery JSON: invalid character 'j' looking for beginning of value",
-			},
 			{
 				name:        "user info fetch error",
 				authCode:    "valid",

pkg/oidcclient/login.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package oidcclient implements a CLI OIDC login flow.
@@ -822,7 +822,7 @@ func (h *handlerState) handleRefresh(ctx context.Context, refreshToken *oidctype
 
 	// The spec is not 100% clear about whether an ID token from the refresh flow should include a nonce, and at least
 	// some providers do not include one, so we skip the nonce validation here (but not other validations).
-	return upstreamOIDCIdentityProvider.ValidateTokenAndMergeWithUserInfo(ctx, refreshed, "", true)
+	return upstreamOIDCIdentityProvider.ValidateTokenAndMergeWithUserInfo(ctx, refreshed, "", true, false)
 }
 
 func (h *handlerState) handleAuthCodeCallback(w http.ResponseWriter, r *http.Request) (err error) {

pkg/oidcclient/login_test.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package oidcclient
@@ -406,7 +406,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 					h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) provider.UpstreamOIDCIdentityProviderI {
 						mock := mockUpstream(t)
 						mock.EXPECT().
-							ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true).
+							ValidateTokenAndMergeWithUserInfo(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true, false).
 							Return(&testToken, nil)
 						mock.EXPECT().
 							PerformRefresh(gomock.Any(), testToken.RefreshToken.Token).
@@ -453,7 +453,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 					h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) provider.UpstreamOIDCIdentityProviderI {
 						mock := mockUpstream(t)
 						mock.EXPECT().
-							ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true).
+							ValidateTokenAndMergeWithUserInfo(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true, false).
 							Return(nil, fmt.Errorf("some validation error"))
 						mock.EXPECT().
 							PerformRefresh(gomock.Any(), "test-refresh-token-returning-invalid-id-token").
@@ -1648,7 +1648,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 					h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) provider.UpstreamOIDCIdentityProviderI {
 						mock := mockUpstream(t)
 						mock.EXPECT().
-							ValidateToken(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true).
+							ValidateTokenAndMergeWithUserInfo(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true, false).
 							Return(&testToken, nil)
 						mock.EXPECT().
 							PerformRefresh(gomock.Any(), testToken.RefreshToken.Token).

test/integration/ldap_client_test.go

@@ -1,4 +1,4 @@
-// Copyright 2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package integration

test/integration/supervisor_login_test.go

@@ -129,6 +129,40 @@ func TestSupervisorLogin(t *testing.T) {
 			wantDownstreamIDTokenUsernameToMatch: func(_ string) string { return "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Username) + "$" },
 			wantDownstreamIDTokenGroups:          env.SupervisorUpstreamOIDC.ExpectedGroups,
 		},
+		{
+			name: "oidc without refresh token",
+			maybeSkip: func(t *testing.T) {
+				// never need to skip this test
+			},
+			createIDP: func(t *testing.T) string {
+				t.Helper()
+				oidcIDP := testlib.CreateTestOIDCIdentityProvider(t, idpv1alpha1.OIDCIdentityProviderSpec{
+					Issuer: env.SupervisorUpstreamOIDC.Issuer,
+					TLS: &idpv1alpha1.TLSSpec{
+						CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte(env.SupervisorUpstreamOIDC.CABundle)),
+					},
+					Client: idpv1alpha1.OIDCClient{
+						SecretName: testlib.CreateClientCredsSecret(t, env.SupervisorUpstreamOIDC.ClientID, env.SupervisorUpstreamOIDC.ClientSecret).Name,
+					},
+					Claims: idpv1alpha1.OIDCClaims{
+						Username: env.SupervisorUpstreamOIDC.UsernameClaim,
+						Groups:   env.SupervisorUpstreamOIDC.GroupsClaim,
+					},
+					AuthorizationConfig: idpv1alpha1.OIDCAuthorizationConfig{
+						AdditionalScopes: []string{"email"}, // does not ask for offline_access.
+					},
+				}, idpv1alpha1.PhaseReady)
+				return oidcIDP.Name
+			},
+			requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlow,
+			breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, _, _ string) {
+				fositeSessionData := pinnipedSession.Fosite
+				fositeSessionData.Claims.Extra["username"] = "some-incorrect-username"
+			},
+			wantDownstreamIDTokenSubjectToMatch:  "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Issuer+"?sub=") + ".+",
+			wantDownstreamIDTokenUsernameToMatch: func(_ string) string { return "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Username) + "$" },
+			wantDownstreamIDTokenGroups:          env.SupervisorUpstreamOIDC.ExpectedGroups,
+		},
 		{
 			name: "oidc with CLI password flow",
 			maybeSkip: func(t *testing.T) {

test/testlib/env.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2022 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package testlib