diff --git a/internal/federationdomain/clientregistry/clientregistry.go b/internal/federationdomain/clientregistry/clientregistry.go index 573de6fe7..d46809bb3 100644 --- a/internal/federationdomain/clientregistry/clientregistry.go +++ b/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{ diff --git a/internal/federationdomain/clientregistry/clientregistry_test.go b/internal/federationdomain/clientregistry/clientregistry_test.go index 5f800ff9f..cc1b0dc6e 100644 --- a/internal/federationdomain/clientregistry/clientregistry_test.go +++ b/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()) diff --git a/internal/federationdomain/endpoints/token/token_handler.go b/internal/federationdomain/endpoints/token/token_handler.go index e441f6ce6..f9d31a7f2 100644 --- a/internal/federationdomain/endpoints/token/token_handler.go +++ b/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 diff --git a/internal/federationdomain/oidc/oidc.go b/internal/federationdomain/oidc/oidc.go index 7715f9e89..ba839ca7c 100644 --- a/internal/federationdomain/oidc/oidc.go +++ b/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, diff --git a/internal/federationdomain/oidc/oidc_test.go b/internal/federationdomain/oidc/oidc_test.go index bf9c30890..c629dfcc0 100644 --- a/internal/federationdomain/oidc/oidc_test.go +++ b/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) }) diff --git a/internal/federationdomain/timeouts/timeouts_configuration.go b/internal/federationdomain/timeouts/timeouts_configuration.go index 639051b15..782a3c2d1 100644 --- a/internal/federationdomain/timeouts/timeouts_configuration.go +++ b/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 diff --git a/test/integration/supervisor_oidc_client_test.go b/test/integration/supervisor_oidc_client_test.go index f76d380fc..1658eceb7 100644 --- a/test/integration/supervisor_oidc_client_test.go +++ b/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"