Refactors for custom ID token lifetime based on PR feedback

internal/federationdomain/clientregistry/clientregistry.go

@@ -35,6 +35,10 @@ type Client struct {
 	IDTokenLifetimeConfiguration time.Duration
 }
 
+func (c *Client) GetIDTokenLifetimeConfiguration() time.Duration {
+	return c.IDTokenLifetimeConfiguration
+}
+
 // Client implements the base, OIDC, and response_mode client interfaces of Fosite.
 var (
 	_ fosite.Client              = (*Client)(nil)
@@ -181,7 +185,7 @@ func oidcClientCRToFositeClient(oidcClient *configv1alpha1.OIDCClient, clientSec
 	var idTokenLifetime time.Duration
 	if idTokenLifetimeOverrideInSeconds != nil {
 		// It should be safe to cast this int32 to time.Duration, because time.Duration is an int64.
-		idTokenLifetime = time.Duration(*(oidcClient.Spec.TokenLifetimes.IDTokenSeconds)) * time.Second
+		idTokenLifetime = time.Duration(*(idTokenLifetimeOverrideInSeconds)) * time.Second
 	}
 
 	return &Client{

internal/federationdomain/clientregistry/clientregistry_test.go

@@ -224,7 +224,7 @@ func TestClientManager(t *testing.T) {
 						AllowedGrantTypes:   []configv1alpha1.GrantType{"authorization_code", "refresh_token"},
 						AllowedScopes:       []configv1alpha1.Scope{"openid", "offline_access", "username", "groups"},
 						AllowedRedirectURIs: []configv1alpha1.RedirectURI{"http://localhost:8080"},
-						TokenLifetimes:      configv1alpha1.OIDCClientTokenLifetimes{IDTokenSeconds: ptr.To[int32]((4242))},
+						TokenLifetimes:      configv1alpha1.OIDCClientTokenLifetimes{IDTokenSeconds: ptr.To[int32](4242)},
 					},
 				},
 				{
@@ -306,7 +306,7 @@ func requireEqualsPinnipedCLI(t *testing.T, c *Client) {
 	require.Equal(t, "none", c.GetTokenEndpointAuthMethod())
 	require.Equal(t, "RS256", c.GetTokenEndpointAuthSigningAlgorithm())
 	require.Equal(t, []fosite.ResponseModeType{"", "query", "form_post"}, c.GetResponseModes())
-	require.Equal(t, 0*time.Second, c.IDTokenLifetimeConfiguration)
+	require.Equal(t, 0*time.Second, c.GetIDTokenLifetimeConfiguration())
 
 	marshaled, err := json.Marshal(c)
 	require.NoError(t, err)
@@ -341,7 +341,7 @@ func requireEqualsPinnipedCLI(t *testing.T, c *Client) {
 		  "request_uris": null,
 		  "request_object_signing_alg": "",
 		  "token_endpoint_auth_signing_alg": "RS256",
-          "IDTokenLifetimeConfiguration": 0
+		  "IDTokenLifetimeConfiguration": 0
 		}`, string(marshaled))
 }
 
@@ -365,7 +365,7 @@ func requireDynamicOIDCClient(
 	require.Equal(t, wantRedirectURIs, c.GetRedirectURIs())
 	require.Equal(t, wantGrantTypes, c.GetGrantTypes())
 	require.Equal(t, wantScopes, c.GetScopes())
-	require.Equal(t, wantIDTokenLifetimeConfiguration, c.IDTokenLifetimeConfiguration)
+	require.Equal(t, wantIDTokenLifetimeConfiguration, c.GetIDTokenLifetimeConfiguration())
 
 	// The following are always the same for all OIDCClients.
 	require.Nil(t, c.GetHashedSecret())

internal/federationdomain/endpoints/token/token_handler.go

@@ -95,13 +95,13 @@ func NewHandler(
 }
 
 func maybeOverrideDefaultAccessTokenLifetime(overrideAccessTokenLifespan timeouts.OverrideLifespan, accessRequest fosite.AccessRequester) {
-	if doOverride, newLifespan := overrideAccessTokenLifespan(accessRequest); doOverride {
+	if newLifespan, doOverride := overrideAccessTokenLifespan(accessRequest); doOverride {
 		accessRequest.GetSession().SetExpiresAt(fosite.AccessToken, time.Now().UTC().Add(newLifespan).Round(time.Second))
 	}
 }
 
 func maybeOverrideDefaultIDTokenLifetime(baseCtx context.Context, overrideIDTokenLifespan timeouts.OverrideLifespan, accessRequest fosite.AccessRequester) context.Context {
-	if doOverride, newLifespan := overrideIDTokenLifespan(accessRequest); doOverride {
+	if newLifespan, doOverride := overrideIDTokenLifespan(accessRequest); doOverride {
 		return idtokenlifespan.OverrideIDTokenLifespanInContext(baseCtx, newLifespan)
 	}
 	return baseCtx

internal/federationdomain/oidc/oidc.go

@@ -173,31 +173,31 @@ func DefaultOIDCTimeoutsConfiguration() timeouts.Configuration {
 		AuthorizeCodeLifespan: authorizationCodeLifespan,
 
 		AccessTokenLifespan: accessTokenLifespan,
-		OverrideDefaultAccessTokenLifespan: func(accessRequest fosite.AccessRequester) (bool, time.Duration) {
+		OverrideDefaultAccessTokenLifespan: func(accessRequest fosite.AccessRequester) (time.Duration, bool) {
 			// Not currently overriding the defaults.
-			return false, 0
+			return 0, false
 		},
 
 		IDTokenLifespan: idTokenLifespan,
-		OverrideDefaultIDTokenLifespan: func(accessRequest fosite.AccessRequester) (bool, time.Duration) {
+		OverrideDefaultIDTokenLifespan: func(accessRequest fosite.AccessRequester) (time.Duration, bool) {
 			client := accessRequest.GetClient()
 			// Don't allow OIDCClients to override the default lifetime for ID tokens returned
 			// by RFC8693 token exchange. This is not user configurable for now.
-			if !accessRequest.GetGrantTypes().ExactOne(oidcapi.GrantTypeTokenExchange) {
+			if !accessRequest.GetGrantTypes().Has(oidcapi.GrantTypeTokenExchange) {
 				if castClient, ok := client.(*clientregistry.Client); !ok {
 					// All clients returned by our client registry implement clientregistry.Client,
 					// so this should be a safe cast in practice.
 					plog.Error("could not check if client overrides token lifetimes",
 						errors.New("could not cast client to *clientregistry.Client"),
 						"clientID", client.GetID(), "clientType", reflect.TypeOf(client))
-				} else if castClient.IDTokenLifetimeConfiguration > 0 {
+				} else if castClient.GetIDTokenLifetimeConfiguration() > 0 {
 					// An OIDCClient resource has provided an override, so use it.
 					// Note that the pinniped-cli client never overrides this value.
-					return true, castClient.IDTokenLifetimeConfiguration
+					return castClient.GetIDTokenLifetimeConfiguration(), true
 				}
 			}
 			// Otherwise, do not override the defaults.
-			return false, 0
+			return 0, false
 		},
 
 		RefreshTokenLifespan: refreshTokenLifespan,

internal/federationdomain/oidc/oidc_test.go

@@ -38,7 +38,7 @@ func TestOverrideDefaultAccessTokenLifespan(t *testing.T) {
 	c := DefaultOIDCTimeoutsConfiguration()
 
 	// We are not yet overriding access token lifetimes.
-	doOverride, newLifespan := c.OverrideDefaultAccessTokenLifespan(nil)
+	newLifespan, doOverride := c.OverrideDefaultAccessTokenLifespan(nil)
 	require.Equal(t, false, doOverride)
 	require.Equal(t, time.Duration(0), newLifespan)
 }
@@ -109,7 +109,7 @@ func TestOverrideIDTokenLifespan(t *testing.T) {
 
 			c := DefaultOIDCTimeoutsConfiguration()
 
-			doOverride, newLifespan := c.OverrideDefaultIDTokenLifespan(tt.accessRequest)
+			newLifespan, doOverride := c.OverrideDefaultIDTokenLifespan(tt.accessRequest)
 			require.Equal(t, tt.wantOverride, doOverride)
 			require.Equal(t, tt.wantLifespan, newLifespan)
 		})

internal/federationdomain/timeouts/timeouts_configuration.go

@@ -14,7 +14,7 @@ type StorageLifetime func(requester fosite.Requester) time.Duration
 
 // OverrideLifespan is a function that, given a request, can suggest to override the default lifespan
 // by returning true along with a new lifespan. When false is returned, the returned duration should be ignored.
-type OverrideLifespan func(accessRequest fosite.AccessRequester) (bool, time.Duration)
+type OverrideLifespan func(accessRequest fosite.AccessRequester) (time.Duration, bool)
 
 type Configuration struct {
 	// The length of time that our state param that we encrypt and pass to the upstream OIDC IDP should be considered

test/integration/supervisor_oidc_client_test.go

@@ -6,7 +6,6 @@ package integration
 import (
 	"context"
 	"fmt"
-	"k8s.io/utils/ptr"
 	"sort"
 	"strings"
 	"testing"
@@ -16,6 +15,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/utils/ptr"
 
 	supervisorconfigv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/config/v1alpha1"
 	"go.pinniped.dev/internal/oidcclientsecretstorage"