Update group memberships during refresh for upstream OIDC providers

Update the user's group memberships when possible. Note that we won't
always have enough information to be able to update it (see code
comments).

internal/oidc/downstreamsession/downstream_session.go

@@ -126,7 +126,7 @@ func GetDownstreamIdentityFromUpstreamIDToken(
 		return "", "", nil, err
 	}
 
-	groups, err := getGroupsFromUpstreamIDToken(upstreamIDPConfig, idTokenClaims)
+	groups, err := GetGroupsFromUpstreamIDToken(upstreamIDPConfig, idTokenClaims)
 	if err != nil {
 		return "", "", nil, err
 	}
@@ -231,7 +231,10 @@ func downstreamSubjectFromUpstreamOIDC(upstreamIssuerAsString string, upstreamSu
 	return fmt.Sprintf("%s?%s=%s", upstreamIssuerAsString, oidc.IDTokenSubjectClaim, url.QueryEscape(upstreamSubject))
 }
 
-func getGroupsFromUpstreamIDToken(
+// GetGroupsFromUpstreamIDToken returns mapped group names coerced into a slice of strings.
+// It returns nil when there is no configured groups claim name, or then when the configured claim name is not found
+// in the provided map of claims. It returns an error when the claim exists but its value cannot be parsed.
+func GetGroupsFromUpstreamIDToken(
 	upstreamIDPConfig provider.UpstreamOIDCIdentityProviderI,
 	idTokenClaims map[string]interface{},
 ) ([]string, error) {

internal/oidc/token/token_handler.go

@@ -15,10 +15,10 @@ import (
 
 	"go.pinniped.dev/internal/httputil/httperr"
 	"go.pinniped.dev/internal/oidc"
+	"go.pinniped.dev/internal/oidc/downstreamsession"
 	"go.pinniped.dev/internal/oidc/provider"
 	"go.pinniped.dev/internal/plog"
 	"go.pinniped.dev/internal/psession"
-	"go.pinniped.dev/pkg/oidcclient/oidctypes"
 )
 
 var (
@@ -140,19 +140,41 @@ func upstreamOIDCRefresh(ctx context.Context, session *psession.PinnipedSession,
 	// https://openid.net/specs/openid-connect-core-1_0.html#RefreshTokenResponse
 	_, hasIDTok := tokens.Extra("id_token").(string)
 
+	// We may or may not have an ID token, and we may or may not have a userinfo endpoint to call for more claims.
+	// Use what we can (one, both, or neither) and return the union of their claims. If we stored an access token,
+	// then require that the userinfo endpoint exists and returns a successful response, or else we would have no
+	// way to check that the user's session was not revoked on the server.
 	// 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, 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))
+			"Upstream refresh returned an invalid ID token or UserInfo response.").WithWrap(err).
+			WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
 	}
+	mergedClaims := validatedTokens.IDToken.Claims
 
-	err = validateIdentityUnchangedSinceInitialLogin(validatedTokens, session, p.GetUsernameClaim())
+	// To the extent possible, check that the user's basic identity hasn't changed.
+	err = validateIdentityUnchangedSinceInitialLogin(mergedClaims, session, p.GetUsernameClaim())
 	if err != nil {
 		return err
 	}
 
+	// If possible, update the user's group memberships. The configured groups claim name (if there is one) may or
+	// may not be included in the newly fetched and merged claims. It could be missing due to a misconfiguration of the
+	// claim name. It could also be missing because the claim was originally found in the ID token during login, but
+	// now we might not have a refreshed ID token.
+	// If the claim is found, then use it to update the user's group membership in the session.
+	// If the claim is not found, then we have no new information about groups, so skip updating the group membership
+	// and let any old groups memberships in the session remain.
+	refreshedGroups, err := downstreamsession.GetGroupsFromUpstreamIDToken(p, mergedClaims)
+	if err != nil {
+		return err
+	}
+	if refreshedGroups != nil {
+		session.Fosite.Claims.Extra[oidc.DownstreamGroupsClaim] = refreshedGroups
+	}
+
 	// 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.
@@ -165,9 +187,8 @@ func upstreamOIDCRefresh(ctx context.Context, session *psession.PinnipedSession,
 	return nil
 }
 
-func validateIdentityUnchangedSinceInitialLogin(validatedTokens *oidctypes.Token, session *psession.PinnipedSession, usernameClaimName string) error {
+func validateIdentityUnchangedSinceInitialLogin(mergedClaims map[string]interface{}, 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
@@ -229,7 +250,8 @@ func findOIDCProviderByNameAndValidateUID(
 		}
 	}
 	return nil, errorsx.WithStack(errUpstreamRefreshError.
-		WithHint("Provider from upstream session data was not found.").WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
+		WithHint("Provider from upstream session data was not found.").
+		WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
 }
 
 func upstreamLDAPRefresh(ctx context.Context, providerCache oidc.UpstreamIdentityProvidersLister, session *psession.PinnipedSession) error {
@@ -272,7 +294,8 @@ func upstreamLDAPRefresh(ctx context.Context, providerCache oidc.UpstreamIdentit
 	})
 	if err != nil {
 		return errorsx.WithStack(errUpstreamRefreshError.WithHint(
-			"Upstream refresh failed.").WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
+			"Upstream refresh failed.").WithWrap(err).
+			WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
 	}
 
 	return nil
@@ -296,14 +319,16 @@ func findLDAPProviderByNameAndValidateUID(
 		if p.GetName() == s.ProviderName {
 			if p.GetResourceUID() != s.ProviderUID {
 				return nil, "", errorsx.WithStack(errUpstreamRefreshError.WithHint(
-					"Provider from upstream session data has changed its resource UID since authentication.").WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
+					"Provider from upstream session data has changed its resource UID since authentication.").
+					WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
 			}
 			return p, dn, nil
 		}
 	}
 
 	return nil, "", errorsx.WithStack(errUpstreamRefreshError.
-		WithHint("Provider from upstream session data was not found.").WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
+		WithHint("Provider from upstream session data was not found.").
+		WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType))
 }
 
 func getDownstreamUsernameFromPinnipedSession(session *psession.PinnipedSession) (string, error) {

internal/oidc/token/token_handler_test.go

@@ -77,8 +77,7 @@ const (
 var (
 	goodAuthTime        = time.Date(1, 2, 3, 4, 5, 6, 7, time.UTC)
 	goodRequestedAtTime = time.Date(7, 6, 5, 4, 3, 2, 1, time.UTC)
-	goodGroups          = []string{"group1", "groups2"}
+	goodGroups          = []string{"group1", "groups2"} // the default groups set by the authorize endpoint for these tests
-	expectedGoodGroups  = []interface{}{"group1", "groups2"}
 
 	hmacSecretFunc = func() []byte {
 		return []byte(hmacSecret)
@@ -234,6 +233,7 @@ type tokenEndpointResponseExpectedValues struct {
 	wantErrorResponseBody             string
 	wantRequestedScopes               []string
 	wantGrantedScopes                 []string
+	wantGroups                        []string
 	wantUpstreamRefreshCall           *expectedUpstreamRefresh
 	wantUpstreamOIDCValidateTokenCall *expectedUpstreamValidateTokens
 	wantCustomSessionDataStored       *psession.CustomSessionData
@@ -252,7 +252,7 @@ type authcodeExchangeInputs struct {
 	want              tokenEndpointResponseExpectedValues
 }
 
-func TestTokenEndpoint(t *testing.T) {
+func TestTokenEndpointAuthcodeExchange(t *testing.T) {
 	tests := []struct {
 		name             string
 		authcodeExchange authcodeExchangeInputs
@@ -266,6 +266,7 @@ func TestTokenEndpoint(t *testing.T) {
 					wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "scope", "expires_in"}, // no refresh token
 					wantRequestedScopes:   []string{"openid", "profile", "email"},
 					wantGrantedScopes:     []string{"openid"},
+					wantGroups:            goodGroups,
 				},
 			},
 		},
@@ -278,6 +279,7 @@ func TestTokenEndpoint(t *testing.T) {
 					wantSuccessBodyFields: []string{"access_token", "token_type", "scope", "expires_in"}, // no id or refresh tokens
 					wantRequestedScopes:   []string{"profile", "email"},
 					wantGrantedScopes:     []string{},
+					wantGroups:            goodGroups,
 				},
 			},
 		},
@@ -290,6 +292,7 @@ func TestTokenEndpoint(t *testing.T) {
 					wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "scope", "expires_in", "refresh_token"}, // all possible tokens
 					wantRequestedScopes:   []string{"openid", "offline_access"},
 					wantGrantedScopes:     []string{"openid", "offline_access"},
+					wantGroups:            goodGroups,
 				},
 			},
 		},
@@ -302,6 +305,7 @@ func TestTokenEndpoint(t *testing.T) {
 					wantSuccessBodyFields: []string{"access_token", "token_type", "scope", "expires_in", "refresh_token"}, // no id token
 					wantRequestedScopes:   []string{"offline_access"},
 					wantGrantedScopes:     []string{"offline_access"},
+					wantGroups:            goodGroups,
 				},
 			},
 		},
@@ -561,6 +565,7 @@ func TestTokenEndpointWhenAuthcodeIsUsedTwice(t *testing.T) {
 					wantSuccessBodyFields: []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"},
 					wantRequestedScopes:   []string{"openid", "offline_access", "profile", "email"},
 					wantGrantedScopes:     []string{"openid", "offline_access"},
+					wantGroups:            goodGroups,
 				},
 			},
 		},
@@ -601,7 +606,7 @@ func TestTokenEndpointWhenAuthcodeIsUsedTwice(t *testing.T) {
 			// 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.wantRequestedScopes, test.authcodeExchange.want.wantGrantedScopes, nil)
+				test.authcodeExchange.want.wantRequestedScopes, test.authcodeExchange.want.wantGrantedScopes, test.authcodeExchange.want.wantGroups, nil)
 
 			// 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)
@@ -614,12 +619,13 @@ func TestTokenEndpointWhenAuthcodeIsUsedTwice(t *testing.T) {
 	}
 }
 
-func TestTokenExchange(t *testing.T) {
+func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn:ietf:params:oauth:grant-type:token-exchange"
 	successfulAuthCodeExchange := tokenEndpointResponseExpectedValues{
 		wantStatus:            http.StatusOK,
 		wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "expires_in", "scope"},
 		wantRequestedScopes:   []string{"openid", "pinniped:request-audience"},
 		wantGrantedScopes:     []string{"openid", "pinniped:request-audience"},
+		wantGroups:            goodGroups,
 	}
 
 	doValidAuthCodeExchange := authcodeExchangeInputs{
@@ -726,6 +732,7 @@ func TestTokenExchange(t *testing.T) {
 					wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "expires_in", "scope"},
 					wantRequestedScopes:   []string{"openid"},
 					wantGrantedScopes:     []string{"openid"},
+					wantGroups:            goodGroups,
 				},
 			},
 			requestedAudience:        "some-workload-cluster",
@@ -743,6 +750,7 @@ func TestTokenExchange(t *testing.T) {
 					wantSuccessBodyFields: []string{"access_token", "token_type", "expires_in", "scope"},
 					wantRequestedScopes:   []string{"pinniped:request-audience"},
 					wantGrantedScopes:     []string{"pinniped:request-audience"},
+					wantGroups:            goodGroups,
 				},
 			},
 			requestedAudience:        "some-workload-cluster",
@@ -805,7 +813,7 @@ func TestTokenExchange(t *testing.T) {
 				require.Contains(t, rsp.Body.String(), test.wantResponseBodyContains)
 			}
 
-			// The remaining assertions apply only the the happy path.
+			// The remaining assertions apply only to the happy path.
 			if rsp.Code != http.StatusOK {
 				return
 			}
@@ -844,7 +852,7 @@ func TestTokenExchange(t *testing.T) {
 			require.Equal(t, goodSubject, tokenClaims["sub"])
 			require.Equal(t, goodIssuer, tokenClaims["iss"])
 			require.Equal(t, goodUsername, tokenClaims["username"])
-			require.Equal(t, expectedGoodGroups, tokenClaims["groups"])
+			require.Equal(t, toSliceOfInterface(test.authcodeExchange.want.wantGroups), tokenClaims["groups"])
 
 			// Also assert that some are the same as the original downstream ID token.
 			requireClaimsAreEqual(t, "iss", claimsOfFirstIDToken, tokenClaims)       // issuer
@@ -991,6 +999,7 @@ func TestRefreshGrant(t *testing.T) {
 			wantRequestedScopes:         []string{"openid", "offline_access"},
 			wantGrantedScopes:           []string{"openid", "offline_access"},
 			wantCustomSessionDataStored: wantCustomSessionDataStored,
+			wantGroups:                  goodGroups,
 		}
 		return want
 	}
@@ -1128,13 +1137,14 @@ func TestRefreshGrant(t *testing.T) {
 				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"},
+					wantGroups:            goodGroups,
 					wantUpstreamOIDCValidateTokenCall: &expectedUpstreamValidateTokens{
 						oidcUpstreamName,
 						&oidctestutil.ValidateTokenAndMergeWithUserInfoArgs{
@@ -1199,12 +1209,100 @@ func TestRefreshGrant(t *testing.T) {
 					wantSuccessBodyFields:             []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"},
 					wantRequestedScopes:               []string{"openid", "offline_access"},
 					wantGrantedScopes:                 []string{"openid", "offline_access"},
+					wantGroups:                        goodGroups,
 					wantUpstreamRefreshCall:           happyOIDCUpstreamRefreshCall(),
 					wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithRefreshTokenWithoutIDToken(), false),
 					wantCustomSessionDataStored:       upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken),
 				},
 			},
 		},
+		{
+			name: "happy path refresh grant when the upstream refresh returns new group memberships (as strings) from the merged ID token and userinfo results, it updates groups",
+			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(
+				upstreamOIDCIdentityProviderBuilder().WithGroupsClaim("my-groups-claim").WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{
+					IDToken: &oidctypes.IDToken{
+						Claims: map[string]interface{}{
+							"sub":             goodUpstreamSubject,
+							"my-groups-claim": []string{"new-group1", "new-group2", "new-group3"}, // refreshed claims includes updated groups
+						},
+					},
+				}).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()),
+			authcodeExchange: authcodeExchangeInputs{
+				customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(),
+				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") },
+				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()),
+			},
+			refreshRequest: refreshRequestInputs{
+				want: tokenEndpointResponseExpectedValues{
+					wantStatus:                        http.StatusOK,
+					wantSuccessBodyFields:             []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"},
+					wantRequestedScopes:               []string{"openid", "offline_access"},
+					wantGrantedScopes:                 []string{"openid", "offline_access"},
+					wantGroups:                        []string{"new-group1", "new-group2", "new-group3"},
+					wantUpstreamRefreshCall:           happyOIDCUpstreamRefreshCall(),
+					wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true),
+					wantCustomSessionDataStored:       upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken),
+				},
+			},
+		},
+		{
+			name: "happy path refresh grant when the upstream refresh returns new group memberships (as interface{} types) from the merged ID token and userinfo results, it updates groups",
+			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(
+				upstreamOIDCIdentityProviderBuilder().WithGroupsClaim("my-groups-claim").WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{
+					IDToken: &oidctypes.IDToken{
+						Claims: map[string]interface{}{
+							"sub":             goodUpstreamSubject,
+							"my-groups-claim": []interface{}{"new-group1", "new-group2", "new-group3"}, // refreshed claims includes updated groups
+						},
+					},
+				}).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()),
+			authcodeExchange: authcodeExchangeInputs{
+				customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(),
+				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") },
+				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()),
+			},
+			refreshRequest: refreshRequestInputs{
+				want: tokenEndpointResponseExpectedValues{
+					wantStatus:                        http.StatusOK,
+					wantSuccessBodyFields:             []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"},
+					wantRequestedScopes:               []string{"openid", "offline_access"},
+					wantGrantedScopes:                 []string{"openid", "offline_access"},
+					wantGroups:                        []string{"new-group1", "new-group2", "new-group3"},
+					wantUpstreamRefreshCall:           happyOIDCUpstreamRefreshCall(),
+					wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true),
+					wantCustomSessionDataStored:       upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken),
+				},
+			},
+		},
+		{
+			name: "happy path refresh grant when the upstream refresh does not return new group memberships from the merged ID token and userinfo results, it keeps groups from initial login",
+			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(
+				upstreamOIDCIdentityProviderBuilder().WithGroupsClaim("my-groups-claim").WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{
+					IDToken: &oidctypes.IDToken{
+						Claims: map[string]interface{}{
+							"sub": goodUpstreamSubject,
+							// "my-groups-claim" is omitted from the refreshed claims
+						},
+					},
+				}).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()),
+			authcodeExchange: authcodeExchangeInputs{
+				customSessionData: initialUpstreamOIDCRefreshTokenCustomSessionData(),
+				modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") },
+				want:              happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCRefreshTokenCustomSessionData()),
+			},
+			refreshRequest: refreshRequestInputs{
+				want: tokenEndpointResponseExpectedValues{
+					wantStatus:                        http.StatusOK,
+					wantSuccessBodyFields:             []string{"refresh_token", "access_token", "id_token", "token_type", "expires_in", "scope"},
+					wantRequestedScopes:               []string{"openid", "offline_access"},
+					wantGrantedScopes:                 []string{"openid", "offline_access"},
+					wantGroups:                        goodGroups, // the same groups as from the initial login
+					wantUpstreamRefreshCall:           happyOIDCUpstreamRefreshCall(),
+					wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true),
+					wantCustomSessionDataStored:       upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken),
+				},
+			},
+		},
 		{
 			name: "happy path refresh grant when the upstream refresh does not return a new refresh token",
 			idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(
@@ -1270,6 +1368,7 @@ func TestRefreshGrant(t *testing.T) {
 					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"},
+					wantGroups:                  goodGroups,
 					wantCustomSessionDataStored: initialUpstreamOIDCRefreshTokenCustomSessionData(),
 				},
 			},
@@ -1282,6 +1381,7 @@ func TestRefreshGrant(t *testing.T) {
 					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"},
+					wantGroups:                        goodGroups,
 					wantUpstreamRefreshCall:           happyOIDCUpstreamRefreshCall(),
 					wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true),
 					wantCustomSessionDataStored:       upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken),
@@ -2597,7 +2697,8 @@ func TestRefreshGrant(t *testing.T) {
 
 			requireTokenEndpointBehavior(t,
 				test.refreshRequest.want,
-				test.authcodeExchange.customSessionData,
+				test.authcodeExchange.want.wantGroups,   // the old groups from the initial login
+				test.authcodeExchange.customSessionData, // the old custom session data from the initial login
 				wantAtHashClaimInIDToken,
 				wantNonceValueInIDToken,
 				refreshResponse,
@@ -2724,7 +2825,8 @@ func exchangeAuthcodeForTokens(t *testing.T, test authcodeExchangeInputs, idps p
 
 	requireTokenEndpointBehavior(t,
 		test.want,
-		test.customSessionData,
+		goodGroups,             // the old groups from the initial login
+		test.customSessionData, // the old custom session data from the initial login
 		wantAtHashClaimInIDToken,
 		wantNonceValueInIDToken,
 		rsp,
@@ -2740,6 +2842,7 @@ func exchangeAuthcodeForTokens(t *testing.T, test authcodeExchangeInputs, idps p
 func requireTokenEndpointBehavior(
 	t *testing.T,
 	test tokenEndpointResponseExpectedValues,
+	oldGroups []string,
 	oldCustomSessionData *psession.CustomSessionData,
 	wantAtHashClaimInIDToken bool,
 	wantNonceValueInIDToken bool,
@@ -2763,10 +2866,10 @@ func requireTokenEndpointBehavior(
 		wantRefreshToken := contains(test.wantSuccessBodyFields, "refresh_token")
 
 		requireInvalidAuthCodeStorage(t, authCode, oauthStore, secrets)
-		requireValidAccessTokenStorage(t, parsedResponseBody, oauthStore, test.wantRequestedScopes, test.wantGrantedScopes, test.wantCustomSessionDataStored, secrets)
+		requireValidAccessTokenStorage(t, parsedResponseBody, oauthStore, test.wantRequestedScopes, test.wantGrantedScopes, test.wantGroups, test.wantCustomSessionDataStored, secrets)
 		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 from the initial login.
+		// Performing a refresh does not update the OIDC storage, so after a refresh it should still have the old custom session data and old groups from the initial login.
-		requireValidOIDCStorage(t, parsedResponseBody, authCode, oauthStore, test.wantRequestedScopes, test.wantGrantedScopes, oldCustomSessionData)
+		requireValidOIDCStorage(t, parsedResponseBody, authCode, oauthStore, test.wantRequestedScopes, test.wantGrantedScopes, oldGroups, oldCustomSessionData)
 
 		expectedNumberOfRefreshTokenSessionsStored := 0
 		if wantRefreshToken {
@@ -2775,10 +2878,10 @@ func requireTokenEndpointBehavior(
 		expectedNumberOfIDSessionsStored := 0
 		if wantIDToken {
 			expectedNumberOfIDSessionsStored = 1
-			requireValidIDToken(t, parsedResponseBody, jwtSigningKey, wantAtHashClaimInIDToken, wantNonceValueInIDToken, parsedResponseBody["access_token"].(string))
+			requireValidIDToken(t, parsedResponseBody, jwtSigningKey, wantAtHashClaimInIDToken, wantNonceValueInIDToken, test.wantGroups, parsedResponseBody["access_token"].(string))
 		}
 		if wantRefreshToken {
-			requireValidRefreshTokenStorage(t, parsedResponseBody, oauthStore, test.wantRequestedScopes, test.wantGrantedScopes, test.wantCustomSessionDataStored, secrets)
+			requireValidRefreshTokenStorage(t, parsedResponseBody, oauthStore, test.wantRequestedScopes, test.wantGrantedScopes, test.wantGroups, test.wantCustomSessionDataStored, secrets)
 		}
 
 		testutil.RequireNumberOfSecretsMatchingLabelSelector(t, secrets, labels.Set{crud.SecretLabelKey: authorizationcode.TypeLabelValue}, 1)
@@ -3016,6 +3119,7 @@ func requireValidRefreshTokenStorage(
 	storage fositeoauth2.CoreStorage,
 	wantRequestedScopes []string,
 	wantGrantedScopes []string,
+	wantGroups []string,
 	wantCustomSessionData *psession.CustomSessionData,
 	secrets v1.SecretInterface,
 ) {
@@ -3038,6 +3142,7 @@ func requireValidRefreshTokenStorage(
 		wantRequestedScopes,
 		wantGrantedScopes,
 		true,
+		wantGroups,
 		wantCustomSessionData,
 	)
 
@@ -3050,6 +3155,7 @@ func requireValidAccessTokenStorage(
 	storage fositeoauth2.CoreStorage,
 	wantRequestedScopes []string,
 	wantGrantedScopes []string,
+	wantGroups []string,
 	wantCustomSessionData *psession.CustomSessionData,
 	secrets v1.SecretInterface,
 ) {
@@ -3091,6 +3197,7 @@ func requireValidAccessTokenStorage(
 		wantRequestedScopes,
 		wantGrantedScopes,
 		true,
+		wantGroups,
 		wantCustomSessionData,
 	)
 
@@ -3133,6 +3240,7 @@ func requireValidOIDCStorage(
 	storage openid.OpenIDConnectRequestStorage,
 	wantRequestedScopes []string,
 	wantGrantedScopes []string,
+	wantGroups []string,
 	wantCustomSessionData *psession.CustomSessionData,
 ) {
 	t.Helper()
@@ -3156,6 +3264,7 @@ func requireValidOIDCStorage(
 			wantRequestedScopes,
 			wantGrantedScopes,
 			false,
+			wantGroups,
 			wantCustomSessionData,
 		)
 	} else {
@@ -3171,6 +3280,7 @@ func requireValidStoredRequest(
 	wantRequestedScopes []string,
 	wantGrantedScopes []string,
 	wantAccessTokenExpiresAt bool,
+	wantGroups []string,
 	wantCustomSessionData *psession.CustomSessionData,
 ) {
 	t.Helper()
@@ -3198,7 +3308,7 @@ func requireValidStoredRequest(
 		// Our custom claims from the authorize endpoint should still be set.
 		require.Equal(t, map[string]interface{}{
 			"username": goodUsername,
-			"groups":   expectedGoodGroups,
+			"groups":   toSliceOfInterface(wantGroups),
 		}, claims.Extra)
 
 		// We are in charge of setting these fields. For the purpose of testing, we ensure that the
@@ -3287,6 +3397,7 @@ func requireValidIDToken(
 	jwtSigningKey *ecdsa.PrivateKey,
 	wantAtHashClaimInIDToken bool,
 	wantNonceValueInIDToken bool,
+	wantGroupsInIDToken []string,
 	actualAccessToken string,
 ) {
 	t.Helper()
@@ -3335,7 +3446,7 @@ func requireValidIDToken(
 	require.NoError(t, err)
 	require.Equal(t, goodSubject, claims.Subject)
 	require.Equal(t, goodUsername, claims.Username)
-	require.Equal(t, goodGroups, claims.Groups)
+	require.Equal(t, wantGroupsInIDToken, claims.Groups)
 	require.Len(t, claims.Audience, 1)
 	require.Equal(t, goodClient, claims.Audience[0])
 	require.Equal(t, goodIssuer, claims.Issuer)
@@ -3388,3 +3499,11 @@ func contains(haystack []string, needle string) bool {
 	}
 	return false
 }
+
+func toSliceOfInterface(s []string) []interface{} {
+	r := make([]interface{}, len(s))
+	for i := range s {
+		r[i] = s[i]
+	}
+	return r
+}