Adjust tests and comments for upgrade to latest version of fosite

internal/controller/supervisorstorage/garbage_collector.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package supervisorstorage
@@ -227,8 +227,7 @@ func (c *garbageCollectorController) maybeRevokeUpstreamOIDCToken(ctx context.Co
 
 	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 downstream authcode exchange, probably due to a bug in fosite, even
-		// though it will never be read or updated again. However, the upstream token contained inside will
+		// These are deleted during downstream authcode exchange. The upstream token contained inside will
 		// be revoked by one of the other cases above.
 		return nil
 

internal/federationdomain/endpoints/token/token_handler_test.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package token
@@ -944,23 +944,17 @@ func TestTokenEndpointWhenAuthcodeIsUsedTwice(t *testing.T) {
 			requireInvalidAccessTokenStorage(t, parsedResponseBody, oauthStore)
 			// This was previously invalidated by the first request, so it remains invalidated
 			requireInvalidPKCEStorage(t, authCode, oauthStore)
-			// Fosite never cleans up OpenID Connect session storage, so it is still there.
-			// Note that customSessionData is only relevant to refresh grant, so we leave it as nil for this
-			// authcode exchange test, even though in practice it would actually be in the session.
-			requireValidOIDCStorage(t, parsedResponseBody, authCode, oauthStore,
-				test.authcodeExchange.want.wantClientID,
-				test.authcodeExchange.want.wantRequestedScopes, test.authcodeExchange.want.wantGrantedScopes,
-				test.authcodeExchange.want.wantUsername, test.authcodeExchange.want.wantGroups,
-				nil, test.authcodeExchange.want.wantAdditionalClaims, approxRequestTime)
+			// OpenID Connect session storage is deleted during a successful authcode exchange.
+			requireDeletedOIDCStorage(t, authCode, oauthStore)
 
 			// Check that the access token and refresh token storage were both deleted, and the number of other storage objects did not change.
 			testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: authorizationcode.TypeLabelValue}, 1)
-			testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 1)
+			testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 0)
 			testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: accesstoken.TypeLabelValue}, 0)
 			testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: refreshtoken.TypeLabelValue}, 0)
 			testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: pkce.TypeLabelValue}, 0)
 			// Assert the number of all secrets, excluding any OIDCClient's storage secret, since those are not related to session storage.
-			testutil.RequireNumberOfSecretsExcludingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: oidcclientsecretstorage.TypeLabelValue}, 2)
+			testutil.RequireNumberOfSecretsExcludingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: oidcclientsecretstorage.TypeLabelValue}, 1)
 		})
 	}
 }
@@ -4425,11 +4419,9 @@ func TestRefreshGrant(t *testing.T) {
 			// Refreshed ID tokens do not include the nonce from the original auth request
 			wantNonceValueInIDToken := false
 
-			requireTokenEndpointBehavior(t,
+			requireTokenEndpointBehavior(
+				t,
 				test.refreshRequest.want,
-				test.authcodeExchange.want.wantUsername, // the old username from the initial login
-				test.authcodeExchange.want.wantGroups,   // the old groups from the initial login
-				test.authcodeExchange.customSessionData, // the old custom session data from the initial login
 				wantNonceValueInIDToken,
 				refreshResponse,
 				authCode,
@@ -4564,11 +4556,9 @@ func exchangeAuthcodeForTokens(
 
 	wantNonceValueInIDToken := true // ID tokens returned by the authcode exchange must include the nonce from the auth request (unlike refreshed ID tokens)
 
-	requireTokenEndpointBehavior(t,
+	requireTokenEndpointBehavior(
+		t,
 		test.want,
-		test.want.wantUsername, // the old username from the initial login
-		test.want.wantGroups,   // the old groups from the initial login
-		test.customSessionData, // the old custom session data from the initial login
 		wantNonceValueInIDToken,
 		rsp,
 		authCode,
@@ -4584,9 +4574,6 @@ func exchangeAuthcodeForTokens(
 func requireTokenEndpointBehavior(
 	t *testing.T,
 	test tokenEndpointResponseExpectedValues,
-	oldUsername string,
-	oldGroups []string,
-	oldCustomSessionData *psession.CustomSessionData,
 	wantNonceValueInIDToken bool,
 	tokenEndpointResponse *httptest.ResponseRecorder,
 	authCode string,
@@ -4611,16 +4598,13 @@ func requireTokenEndpointBehavior(
 		requireInvalidAuthCodeStorage(t, authCode, oauthStore, secrets, requestTime)
 		requireValidAccessTokenStorage(t, parsedResponseBody, oauthStore, test.wantClientID, test.wantRequestedScopes, test.wantGrantedScopes, test.wantUsername, test.wantGroups, test.wantCustomSessionDataStored, test.wantAdditionalClaims, secrets, requestTime)
 		requireInvalidPKCEStorage(t, authCode, oauthStore)
-		// Performing a refresh does not update the OIDC storage, so after a refresh it should still have the old custom session data and old username and groups from the initial login.
-		requireValidOIDCStorage(t, parsedResponseBody, authCode, oauthStore, test.wantClientID, test.wantRequestedScopes, test.wantGrantedScopes, oldUsername, oldGroups, oldCustomSessionData, test.wantAdditionalClaims, requestTime)
+		requireDeletedOIDCStorage(t, authCode, oauthStore) // The OIDC storage was deleted during the authcode exchange.
 
 		expectedNumberOfRefreshTokenSessionsStored := 0
 		if wantRefreshToken {
 			expectedNumberOfRefreshTokenSessionsStored = 1
 		}
-		expectedNumberOfIDSessionsStored := 0
 		if wantIDToken {
-			expectedNumberOfIDSessionsStored = 1
 			requireValidIDToken(t, parsedResponseBody, jwtSigningKey, test.wantClientID, wantNonceValueInIDToken, test.wantUsername, test.wantGroups, test.wantAdditionalClaims, parsedResponseBody["access_token"].(string), requestTime)
 		}
 		if wantRefreshToken {
@@ -4631,9 +4615,9 @@ func requireTokenEndpointBehavior(
 		testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: accesstoken.TypeLabelValue}, 1)
 		testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: pkce.TypeLabelValue}, 0)
 		testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: refreshtoken.TypeLabelValue}, expectedNumberOfRefreshTokenSessionsStored)
-		testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, expectedNumberOfIDSessionsStored)
+		testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: openidconnect.TypeLabelValue}, 0)
 		// Assert the number of all secrets, excluding any OIDCClient's storage secret, since those are not related to session storage.
-		testutil.RequireNumberOfSecretsExcludingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: oidcclientsecretstorage.TypeLabelValue}, 2+expectedNumberOfRefreshTokenSessionsStored+expectedNumberOfIDSessionsStored)
+		testutil.RequireNumberOfSecretsExcludingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: oidcclientsecretstorage.TypeLabelValue}, 2+expectedNumberOfRefreshTokenSessionsStored)
 	} else {
 		require.NotNil(t, test.wantErrorResponseBody, "problem with test table setup: wanted failure but did not specify failure response body")
 
@@ -5001,52 +4985,11 @@ func requireInvalidPKCEStorage(
 	require.True(t, errors.Is(err, fosite.ErrNotFound))
 }
 
-func requireValidOIDCStorage(
-	t *testing.T,
-	body map[string]interface{},
-	code string,
-	storage openid.OpenIDConnectRequestStorage,
-	wantClientID string,
-	wantRequestedScopes []string,
-	wantGrantedScopes []string,
-	wantUsername string,
-	wantGroups []string,
-	wantCustomSessionData *psession.CustomSessionData,
-	wantAdditionalClaims map[string]interface{},
-	requestTime time.Time,
-) {
+func requireDeletedOIDCStorage(t *testing.T, code string, storage openid.OpenIDConnectRequestStorage) {
 	t.Helper()
 
-	if slices.Contains(wantGrantedScopes, "openid") {
-		// Make sure the OIDC session is still there. Note that Fosite stores OIDC sessions using the full auth code as a key.
-		storedRequest, err := storage.GetOpenIDConnectSession(context.Background(), code, nil)
-		require.NoError(t, err)
-
-		// Fosite stores OIDC sessions with only the nonce in the original request form.
-		accessToken, ok := body["access_token"]
-		require.True(t, ok)
-		accessTokenString, ok := accessToken.(string)
-		require.Truef(t, ok, "wanted access_token to be a string, but got %T", accessToken)
-		require.NotEmpty(t, accessTokenString)
-
-		requireValidStoredRequest(
-			t,
-			storedRequest,
-			storedRequest.Sanitize([]string{"nonce"}).GetRequestForm(),
-			wantClientID,
-			wantRequestedScopes,
-			wantGrantedScopes,
-			false,
-			wantUsername,
-			wantGroups,
-			wantCustomSessionData,
-			wantAdditionalClaims,
-			requestTime,
-		)
-	} else {
-		_, err := storage.GetOpenIDConnectSession(context.Background(), code, nil)
-		require.True(t, errors.Is(err, fosite.ErrNotFound))
-	}
+	_, err := storage.GetOpenIDConnectSession(context.Background(), code, nil)
+	require.True(t, errors.Is(err, fosite.ErrNotFound))
 }
 
 func requireValidStoredRequest(

internal/federationdomain/endpointsmanager/manager_test.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package endpointsmanager
@@ -211,8 +211,8 @@ func TestManager(t *testing.T) {
 			r.Equal("some-state", actualLocationQueryParams.Get("state"))
 
 			// Make sure that we wired up the callback endpoint to use kube storage for fosite sessions.
-			r.Equal(len(kubeClient.Actions()), numberOfKubeActionsBeforeThisRequest+3,
-				"did not perform any kube actions during the callback request, but should have")
+			r.Equal(numberOfKubeActionsBeforeThisRequest+3, len(kubeClient.Actions()),
+				"did not perform expected number of kube actions during the callback request")
 
 			// Return the important parts of the response so we can use them in our next request to the token endpoint.
 			return actualLocationQueryParams.Get("code")
@@ -253,8 +253,8 @@ func TestManager(t *testing.T) {
 			oidctestutil.VerifyECDSAIDToken(t, jwkIssuer, downstreamClientID, privateKey, idToken)
 
 			// Make sure that we wired up the callback endpoint to use kube storage for fosite sessions.
-			r.Equal(len(kubeClient.Actions()), numberOfKubeActionsBeforeThisRequest+9,
-				"did not perform any kube actions during the callback request, but should have")
+			r.Equal(numberOfKubeActionsBeforeThisRequest+10, len(kubeClient.Actions()),
+				"did not perform expected number of kube actions during the callback request")
 		}
 
 		requireJWKSRequestToBeHandled := func(requestIssuer, requestURLSuffix, expectedJWKKeyID string) *jose.JSONWebKeySet {

internal/federationdomain/oidc/oidc.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package oidc contains common OIDC functionality needed by FederationDomains to implement
@@ -6,11 +6,9 @@
 package oidc
 
 import (
-	"context"
 	"crypto/subtle"
 	"fmt"
 	"net/http"
-	"net/url"
 	"time"
 
 	"github.com/felixge/httpsnoop"
@@ -131,10 +129,6 @@ func FositeOauth2Helper(
 	jwksProvider jwks.DynamicJWKSProvider,
 	timeoutsConfiguration timeouts.Configuration,
 ) fosite.OAuth2Provider {
-	isRedirectURISecureStrict := func(_ context.Context, uri *url.URL) bool {
-		return fosite.IsRedirectURISecureStrict(uri)
-	}
-
 	oauthConfig := &fosite.Config{
 		IDTokenIssuer: issuer,
 
@@ -157,7 +151,7 @@ func FositeOauth2Helper(
 		MinParameterEntropy: fosite.MinParameterEntropy,
 
 		// do not allow custom scheme redirects, only https and http (on loopback)
-		RedirectSecureChecker: isRedirectURISecureStrict,
+		RedirectSecureChecker: fosite.IsRedirectURISecureStrict,
 
 		// html template for rendering the authorization response when the request has response_mode=form_post
 		FormPostHTMLTemplate: formposthtml.Template(),

internal/federationdomain/storage/kube_storage.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package storage
@@ -107,10 +107,7 @@ func (k KubeStorage) DeletePKCERequestSession(ctx context.Context, signatureOfAu
 // Fosite will create these in the authorize endpoint when it creates an authcode, but only if the user
 // requested the openid scope.
 //
-// Fosite will never delete these, which is likely a bug in fosite. Although there is a delete method below, fosite
-// never calls it. Used during authcode redemption, they will never be accessed again after a successful authcode
-// redemption. Although that implies that they should probably follow a lifecycle similar the the PKCE storage, they
-// are, in fact, not deleted.
+// Used during authcode redemption, and will be deleted during a successful authcode redemption.
 //
 
 func (k KubeStorage) CreateOpenIDConnectSession(ctx context.Context, fullAuthcode string, requester fosite.Requester) error {
@@ -122,7 +119,7 @@ func (k KubeStorage) GetOpenIDConnectSession(ctx context.Context, fullAuthcode s
 }
 
 func (k KubeStorage) DeleteOpenIDConnectSession(ctx context.Context, fullAuthcode string) error {
-	return k.oidcStorage.DeleteOpenIDConnectSession(ctx, fullAuthcode) //nolint:staticcheck  // we know this is deprecated and never called.  our GC controller cleans these up.
+	return k.oidcStorage.DeleteOpenIDConnectSession(ctx, fullAuthcode)
 }
 
 //

internal/federationdomain/timeouts/timeouts_configuration.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package timeouts
@@ -51,8 +51,8 @@ type Configuration struct {
 	PKCESessionStorageLifetime time.Duration
 
 	// OIDCSessionStorageLifetime is the length of time after which the OIDC session data related to an authcode
-	// is allowed to be garbage collected from storage. Due to a bug in an underlying library, these are not explicitly
-	// deleted. Similar to the PKCE session, they are not needed anymore after the corresponding authcode has expired.
+	// is allowed to be garbage collected from storage. After the authcode is successfully redeemed, the OIDC session is
+	// explicitly deleted. Similar to the PKCE session, they are not needed anymore after the corresponding authcode has expired.
 	// Therefore, this can be just slightly longer than the AuthorizeCodeLifespan. We'll avoid making it exactly the same
 	// as AuthorizeCodeLifespan to avoid any chance of the garbage collector deleting it while it is being used.
 	OIDCSessionStorageLifetime time.Duration

internal/fositestorage/openidconnect/openidconnect_test.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package openidconnect
@@ -100,7 +100,7 @@ func TestOpenIdConnectStorage(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, request, newRequest)
 
-	err = storage.DeleteOpenIDConnectSession(ctx, "fancy-code.fancy-signature") //nolint:staticcheck  // we know this is deprecated and never called.  our GC controller cleans these up.
+	err = storage.DeleteOpenIDConnectSession(ctx, "fancy-code.fancy-signature")
 	require.NoError(t, err)
 
 	testutil.LogActualJSONFromCreateAction(t, client, 0) // makes it easier to update expected values when needed

internal/testutil/oidctestutil/oidctestutil.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package oidctestutil
@@ -1302,13 +1302,28 @@ func validateAuthcodeStorage(
 	// Check which scopes were granted.
 	require.ElementsMatch(t, wantDownstreamGrantedScopes, storedRequestFromAuthcode.GetGrantedScopes())
 
+	// Don't care about the order of requested scopes, as long as they match the expected list.
+	storedRequestedScopes := storedRequestFromAuthcode.Form["scope"]
+	require.Len(t, storedRequestedScopes, 1)
+	require.NotEmpty(t, storedRequestedScopes[0])
+	storedRequestedScopesSlice := strings.Split(storedRequestedScopes[0], " ")
+	require.ElementsMatch(t, storedRequestedScopesSlice, wantDownstreamRequestedScopes)
+
 	// Check all the other fields of the stored request.
 	require.NotEmpty(t, storedRequestFromAuthcode.ID)
 	require.Equal(t, wantDownstreamClientID, storedRequestFromAuthcode.Client.GetID())
 	require.ElementsMatch(t, wantDownstreamRequestedScopes, storedRequestFromAuthcode.RequestedScope)
 	require.Nil(t, storedRequestFromAuthcode.RequestedAudience)
 	require.Empty(t, storedRequestFromAuthcode.GrantedAudience)
-	require.Equal(t, url.Values{"redirect_uri": []string{wantDownstreamRedirectURI}}, storedRequestFromAuthcode.Form)
+	require.Equal(t,
+		url.Values{
+			"client_id":     []string{wantDownstreamClientID},
+			"redirect_uri":  []string{wantDownstreamRedirectURI},
+			"response_type": []string{"code"},
+			"scope":         storedRequestedScopes, // already asserted about this actual value above
+		},
+		storedRequestFromAuthcode.Form,
+	)
 	testutil.RequireTimeInDelta(t, time.Now(), storedRequestFromAuthcode.RequestedAt, timeComparisonFudgeFactor)
 
 	// We're not using these fields yet, so confirm that we did not set them (for now).