Refactor to move invocation of identity transforms out of IDP interfaces

Each endpoint handler is now responsible for applying the identity
transformations and creating most of the session data, rather than each
implementation of the upstream IDP interface. This shares code better,
and reduces the responsibilities of the implementations of the IDP
interface by letting them focus more on the upstream stuff.

Also refactor the parameters and return types of the IDP interfaces to
make them more clear, and because they can be more focused on upstream
identities (pre-identity transformation). This clarifies the
responsibilities of the implementations of the IDP interface.

internal/federationdomain/downstreamsession/downstream_session.go

@@ -25,38 +25,75 @@ import (
 
 const idTransformUnexpectedErr = constable.Error("configured identity transformation or policy resulted in unexpected error")
 
-// MakeDownstreamSession creates a downstream OIDC session.
-func MakeDownstreamSession(identity *resolvedprovider.Identity, grantedScopes []string, clientID string) *psession.PinnipedSession {
+// SessionConfig is everything that is needed to start a new downstream Pinniped session, including the upstream and
+// downstream identities of the user. All fields are required.
+type SessionConfig struct {
+	UpstreamIdentity    *resolvedprovider.Identity
+	UpstreamLoginExtras *resolvedprovider.IdentityLoginExtras
+
+	// The downstream username.
+	Username string
+	// The downstream groups.
+	Groups []string
+
+	// The ID of the client who started the new downstream session.
+	ClientID string
+	// The scopes that were granted for the new downstream session.
+	GrantedScopes []string
+}
+
+// NewPinnipedSession creates a downstream Pinniped session.
+func NewPinnipedSession(
+	idp resolvedprovider.FederationDomainResolvedIdentityProvider,
+	c *SessionConfig,
+) *psession.PinnipedSession {
 	now := time.Now().UTC()
-	openIDSession := &psession.PinnipedSession{
+
+	customSessionData := &psession.CustomSessionData{
+		Username:         c.Username,
+		UpstreamUsername: c.UpstreamIdentity.UpstreamUsername,
+		UpstreamGroups:   c.UpstreamIdentity.UpstreamGroups,
+		ProviderUID:      idp.GetProvider().GetResourceUID(),
+		ProviderName:     idp.GetProvider().GetName(),
+		ProviderType:     idp.GetSessionProviderType(),
+		Warnings:         c.UpstreamLoginExtras.Warnings,
+	}
+	idp.ApplyIDPSpecificSessionDataToSession(customSessionData, c.UpstreamIdentity.IDPSpecificSessionData)
+
+	pinnipedSession := &psession.PinnipedSession{
 		Fosite: &openid.DefaultSession{
 			Claims: &jwt.IDTokenClaims{
-				Subject:     identity.Subject,
+				Subject:     c.UpstreamIdentity.DownstreamSubject,
 				RequestedAt: now,
 				AuthTime:    now,
 			},
 		},
-		Custom: identity.SessionData,
+		Custom: customSessionData,
 	}
 
 	extras := map[string]interface{}{}
-	extras[oidcapi.IDTokenClaimAuthorizedParty] = clientID
-	if slices.Contains(grantedScopes, oidcapi.ScopeUsername) {
-		extras[oidcapi.IDTokenClaimUsername] = identity.SessionData.Username
+
+	extras[oidcapi.IDTokenClaimAuthorizedParty] = c.ClientID
+
+	if slices.Contains(c.GrantedScopes, oidcapi.ScopeUsername) {
+		extras[oidcapi.IDTokenClaimUsername] = c.Username
 	}
-	if slices.Contains(grantedScopes, oidcapi.ScopeGroups) {
-		groups := identity.Groups
+
+	if slices.Contains(c.GrantedScopes, oidcapi.ScopeGroups) {
+		groups := c.Groups
 		if groups == nil {
 			groups = []string{}
 		}
 		extras[oidcapi.IDTokenClaimGroups] = groups
 	}
-	if len(identity.AdditionalClaims) > 0 {
-		extras[oidcapi.IDTokenClaimAdditionalClaims] = identity.AdditionalClaims
-	}
-	openIDSession.IDTokenClaims().Extra = extras
 
-	return openIDSession
+	if len(c.UpstreamLoginExtras.DownstreamAdditionalClaims) > 0 {
+		extras[oidcapi.IDTokenClaimAdditionalClaims] = c.UpstreamLoginExtras.DownstreamAdditionalClaims
+	}
+
+	pinnipedSession.IDTokenClaims().Extra = extras
+
+	return pinnipedSession
 }
 
 // AutoApproveScopes auto-grants the scopes which we support and for which we do not require end-user approval,
@@ -89,11 +126,11 @@ func AutoApproveScopes(authorizeRequester fosite.AuthorizeRequester) {
 // or potentially reject the identity.
 func ApplyIdentityTransformations(
 	ctx context.Context,
-	identityTransforms *idtransform.TransformationPipeline,
+	transforms *idtransform.TransformationPipeline,
 	username string,
 	groups []string,
 ) (string, []string, error) {
-	transformationResult, err := identityTransforms.Evaluate(ctx, username, groups)
+	transformationResult, err := transforms.Evaluate(ctx, username, groups)
 	if err != nil {
 		plog.Error("unexpected identity transformation error during authentication", err, "inputUsername", username)
 		return "", nil, idTransformUnexpectedErr

internal/federationdomain/endpoints/auth/auth_handler.go

@@ -130,7 +130,7 @@ func (h *authorizeHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	upstreamProvider, err := chooseUpstreamIDP(idpNameQueryParamValue, h.idpFinder)
+	idp, err := chooseUpstreamIDP(idpNameQueryParamValue, h.idpFinder)
 	if err != nil {
 		oidc.WriteAuthorizeError(r, w,
 			h.oauthHelperWithoutStorage,
@@ -142,7 +142,7 @@ func (h *authorizeHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	h.authorize(w, r, requestedBrowserlessFlow, idpNameQueryParamValue, upstreamProvider)
+	h.authorize(w, r, requestedBrowserlessFlow, idpNameQueryParamValue, idp)
 }
 
 func (h *authorizeHandler) authorize(
@@ -150,7 +150,7 @@ func (h *authorizeHandler) authorize(
 	r *http.Request,
 	requestedBrowserlessFlow bool,
 	idpNameQueryParamValue string,
-	upstreamProvider resolvedprovider.FederationDomainResolvedIdentityProvider,
+	idp resolvedprovider.FederationDomainResolvedIdentityProvider,
 ) {
 	// Browser flows do not need session storage at this step. For browser flows, the request parameters
 	// should be forwarded to the next step as upstream state parameters to avoid storing session state
@@ -177,9 +177,9 @@ func (h *authorizeHandler) authorize(
 	downstreamsession.AutoApproveScopes(authorizeRequester)
 
 	if requestedBrowserlessFlow {
-		err = h.authorizeWithoutBrowser(r, w, oauthHelper, authorizeRequester, upstreamProvider)
+		err = h.authorizeWithoutBrowser(r, w, oauthHelper, authorizeRequester, idp)
 	} else {
-		err = h.authorizeWithBrowser(r, w, oauthHelper, authorizeRequester, upstreamProvider)
+		err = h.authorizeWithBrowser(r, w, oauthHelper, authorizeRequester, idp)
 	}
 	if err != nil {
 		oidc.WriteAuthorizeError(r, w, oauthHelper, authorizeRequester, err, requestedBrowserlessFlow)
@@ -191,7 +191,7 @@ func (h *authorizeHandler) authorizeWithoutBrowser(
 	w http.ResponseWriter,
 	oauthHelper fosite.OAuth2Provider,
 	authorizeRequester fosite.AuthorizeRequester,
-	upstreamProvider resolvedprovider.FederationDomainResolvedIdentityProvider,
+	idp resolvedprovider.FederationDomainResolvedIdentityProvider,
 ) error {
 	if err := requireStaticClientForUsernameAndPasswordHeaders(authorizeRequester); err != nil {
 		return err
@@ -204,14 +204,25 @@ func (h *authorizeHandler) authorizeWithoutBrowser(
 
 	groupsWillBeIgnored := !slices.Contains(authorizeRequester.GetGrantedScopes(), oidcapi.ScopeGroups)
 
-	identity, err := upstreamProvider.Login(r.Context(), submittedUsername, submittedPassword, groupsWillBeIgnored)
+	identity, loginExtras, err := idp.Login(r.Context(), submittedUsername, submittedPassword, groupsWillBeIgnored)
 	if err != nil {
 		return err
 	}
 
-	session := downstreamsession.MakeDownstreamSession(
-		identity, authorizeRequester.GetGrantedScopes(), authorizeRequester.GetClient().GetID(),
-	)
+	username, groups, err := downstreamsession.ApplyIdentityTransformations(r.Context(),
+		idp.GetTransforms(), identity.UpstreamUsername, identity.UpstreamGroups)
+	if err != nil {
+		return fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error())
+	}
+
+	session := downstreamsession.NewPinnipedSession(idp, &downstreamsession.SessionConfig{
+		UpstreamIdentity:    identity,
+		UpstreamLoginExtras: loginExtras,
+		Username:            username,
+		Groups:              groups,
+		ClientID:            authorizeRequester.GetClient().GetID(),
+		GrantedScopes:       authorizeRequester.GetGrantedScopes(),
+	})
 
 	oidc.PerformAuthcodeRedirect(r, w, oauthHelper, authorizeRequester, session, true)
 
@@ -223,7 +234,7 @@ func (h *authorizeHandler) authorizeWithBrowser(
 	w http.ResponseWriter,
 	oauthHelper fosite.OAuth2Provider,
 	authorizeRequester fosite.AuthorizeRequester,
-	upstreamProvider resolvedprovider.FederationDomainResolvedIdentityProvider,
+	idp resolvedprovider.FederationDomainResolvedIdentityProvider,
 ) error {
 	authRequestState, err := generateUpstreamAuthorizeRequestState(r, w,
 		authorizeRequester,
@@ -231,8 +242,8 @@ func (h *authorizeHandler) authorizeWithBrowser(
 		h.generateCSRF,
 		h.generateNonce,
 		h.generatePKCE,
-		upstreamProvider.GetDisplayName(),
-		upstreamProvider.GetSessionProviderType(),
+		idp.GetDisplayName(),
+		idp.GetSessionProviderType(),
 		h.cookieCodec,
 		h.upstreamStateEncoder,
 	)
@@ -240,7 +251,7 @@ func (h *authorizeHandler) authorizeWithBrowser(
 		return err
 	}
 
-	redirectURL, err := upstreamProvider.UpstreamAuthorizeRedirectURL(authRequestState, h.downstreamIssuerURL)
+	redirectURL, err := idp.UpstreamAuthorizeRedirectURL(authRequestState, h.downstreamIssuerURL)
 	if err != nil {
 		return err
 	}

internal/federationdomain/endpoints/callback/callback_handler.go

@@ -31,8 +31,8 @@ func NewHandler(
 			return err
 		}
 
-		oidcIdentityProvider, err := upstreamIDPs.FindUpstreamIDPByDisplayName(state.UpstreamName)
-		if err != nil || oidcIdentityProvider == nil {
+		idp, err := upstreamIDPs.FindUpstreamIDPByDisplayName(state.UpstreamName)
+		if err != nil || idp == nil {
 			plog.Warning("upstream provider not found")
 			return httperr.New(http.StatusUnprocessableEntity, "upstream provider not found")
 		}
@@ -57,21 +57,31 @@ func NewHandler(
 		// an error if the client requested a scope that they are not allowed to request, so we don't need to worry about that here.
 		downstreamsession.AutoApproveScopes(authorizeRequester)
 
-		identity, err := oidcIdentityProvider.HandleCallback(r.Context(), authcode(r), state.PKCECode, state.Nonce, redirectURI)
+		identity, loginExtras, err := idp.HandleCallback(r.Context(), authcode(r), state.PKCECode, state.Nonce, redirectURI)
 		if err != nil {
 			return err
 		}
 
-		session := downstreamsession.MakeDownstreamSession(
-			identity,
-			authorizeRequester.GetGrantedScopes(),
-			authorizeRequester.GetClient().GetID(),
+		username, groups, err := downstreamsession.ApplyIdentityTransformations(
+			r.Context(), idp.GetTransforms(), identity.UpstreamUsername, identity.UpstreamGroups,
 		)
+		if err != nil {
+			return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err)
+		}
+
+		session := downstreamsession.NewPinnipedSession(idp, &downstreamsession.SessionConfig{
+			UpstreamIdentity:    identity,
+			UpstreamLoginExtras: loginExtras,
+			Username:            username,
+			Groups:              groups,
+			ClientID:            authorizeRequester.GetClient().GetID(),
+			GrantedScopes:       authorizeRequester.GetGrantedScopes(),
+		})
 
 		authorizeResponder, err := oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, session)
 		if err != nil {
 			plog.WarningErr("error while generating and saving authcode", err,
-				"identityProviderDisplayName", oidcIdentityProvider.GetDisplayName(), "fositeErr", oidc.FositeErrorForLog(err))
+				"identityProviderDisplayName", idp.GetDisplayName(), "fositeErr", oidc.FositeErrorForLog(err))
 			return httperr.Wrap(http.StatusInternalServerError, "error while generating and saving authcode", err)
 		}
 

internal/federationdomain/endpoints/login/post_login_handler.go

@@ -24,7 +24,7 @@ import (
 func NewPostHandler(issuerURL string, upstreamIDPs federationdomainproviders.FederationDomainIdentityProvidersFinderI, oauthHelper fosite.OAuth2Provider) HandlerFunc {
 	return func(w http.ResponseWriter, r *http.Request, encodedState string, decodedState *oidc.UpstreamStateParamData) error {
 		// Note that the login handler prevents this handler from being called with OIDC upstreams.
-		ldapUpstream, err := upstreamIDPs.FindUpstreamIDPByDisplayName(decodedState.UpstreamName)
+		idp, err := upstreamIDPs.FindUpstreamIDPByDisplayName(decodedState.UpstreamName)
 		if err != nil {
 			// This shouldn't normally happen because the authorization endpoint ensured that this provider existed
 			// at that time. It would be possible in the unlikely event that the provider was deleted during the login.
@@ -70,7 +70,7 @@ func NewPostHandler(issuerURL string, upstreamIDPs federationdomainproviders.Fed
 		skipGroups := !slices.Contains(authorizeRequester.GetGrantedScopes(), oidcapi.ScopeGroups)
 
 		// Attempt to authenticate the user with the upstream IDP.
-		identity, err := ldapUpstream.Login(r.Context(), submittedUsername, submittedPassword, skipGroups)
+		identity, loginExtras, err := idp.Login(r.Context(), submittedUsername, submittedPassword, skipGroups)
 		if err != nil {
 			switch {
 			case errors.Is(err, resolvedldap.ErrUnexpectedUpstreamLDAPError):
@@ -82,17 +82,28 @@ func NewPostHandler(issuerURL string, upstreamIDPs federationdomainproviders.Fed
 				// The user may try to log in again if they'd like, so redirect back to the login page with an error.
 				return redirectToLoginPage(r, w, issuerURL, encodedState, loginurl.ShowBadUserPassErr)
 			default:
-				// Some other error happened, e.g. a configured identity transformation failed.
+				// Some other error happened.
 				oidc.WriteAuthorizeError(r, w, oauthHelper, authorizeRequester, err, false)
 				return nil
 			}
 		}
 
-		session := downstreamsession.MakeDownstreamSession(
-			identity,
-			authorizeRequester.GetGrantedScopes(),
-			authorizeRequester.GetClient().GetID(),
-		)
+		username, groups, err := downstreamsession.ApplyIdentityTransformations(r.Context(),
+			idp.GetTransforms(), identity.UpstreamUsername, identity.UpstreamGroups)
+		if err != nil {
+			err = fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error())
+			oidc.WriteAuthorizeError(r, w, oauthHelper, authorizeRequester, err, false)
+			return nil
+		}
+
+		session := downstreamsession.NewPinnipedSession(idp, &downstreamsession.SessionConfig{
+			UpstreamIdentity:    identity,
+			UpstreamLoginExtras: loginExtras,
+			Username:            username,
+			Groups:              groups,
+			ClientID:            authorizeRequester.GetClient().GetID(),
+			GrantedScopes:       authorizeRequester.GetGrantedScopes(),
+		})
 
 		oidc.PerformAuthcodeRedirect(r, w, oauthHelper, authorizeRequester, session, false)
 

internal/federationdomain/endpoints/token/token_handler.go

@@ -6,6 +6,7 @@ package token
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 
@@ -21,6 +22,7 @@ import (
 	"go.pinniped.dev/internal/federationdomain/oidc"
 	"go.pinniped.dev/internal/federationdomain/resolvedprovider"
 	"go.pinniped.dev/internal/httputil/httperr"
+	"go.pinniped.dev/internal/idtransform"
 	"go.pinniped.dev/internal/plog"
 	"go.pinniped.dev/internal/psession"
 )
@@ -113,36 +115,82 @@ func upstreamRefresh(
 
 	skipGroups := !slices.Contains(accessRequest.GetGrantedScopes(), oidcapi.ScopeGroups)
 
-	clientID := accessRequest.GetClient().GetID()
+	if session.IDTokenClaims().AuthTime.IsZero() {
+		return errorsx.WithStack(resolvedprovider.ErrMissingUpstreamSessionInternalError())
+	}
 
 	err := validateSessionHasUsername(session)
 	if err != nil {
 		return err
 	}
 
+	var oldTransformedGroups []string
+	if !skipGroups {
+		// Only validate the groups in the session if the groups scope was granted.
+		oldTransformedGroups, err = validateAndGetDownstreamGroupsFromSession(session)
+		if err != nil {
+			return err
+		}
+	}
+
 	idp, err := findProviderByNameAndType(providerName, customSessionData.ProviderType, providerUID, idpLister)
 	if err != nil {
 		return err
 	}
 
-	oldTransformedUsername := session.Custom.Username
-	var oldTransformedGroups []string
-	if !skipGroups {
-		oldTransformedGroups, err = getDownstreamGroupsFromSession(session)
-		if err != nil {
-			return err
-		}
+	cloneOfIDPSpecificSessionData := idp.CloneIDPSpecificSessionDataFromSession(session.Custom)
+	if cloneOfIDPSpecificSessionData == nil {
+		return errorsx.WithStack(resolvedprovider.ErrMissingUpstreamSessionInternalError())
 	}
 
-	refreshedGroups, err := idp.UpstreamRefresh(ctx, session, skipGroups)
+	oldUntransformedUsername := session.Custom.UpstreamUsername
+	oldUntransformedGroups := session.Custom.UpstreamGroups
+	oldTransformedUsername := session.Custom.Username
+
+	previousIdentity := resolvedprovider.Identity{
+		UpstreamUsername:       oldUntransformedUsername,
+		UpstreamGroups:         oldUntransformedGroups,
+		DownstreamSubject:      session.Fosite.Claims.Subject,
+		IDPSpecificSessionData: cloneOfIDPSpecificSessionData,
+	}
+
+	// Perform the upstream refresh.
+	refreshedIdentity, err := idp.UpstreamRefresh(ctx, &previousIdentity, skipGroups)
+	if err != nil {
+		return err
+	}
+
+	// If the idp wants to update the session with new information from the refresh, then update it.
+	if refreshedIdentity.IDPSpecificSessionData != nil {
+		idp.ApplyIDPSpecificSessionDataToSession(session.Custom, refreshedIdentity.IDPSpecificSessionData)
+	}
+
+	if refreshedIdentity.UpstreamGroups == nil {
+		// If we could not get a new list of groups, then we still need the untransformed groups list to be able to
+		// run the transformations again, so fetch the original untransformed groups list from the session.
+		// We should also run the transformations on the original groups even when the groups scope was not granted,
+		// because a transformation policy may want to reject the authentication based on the group memberships, even
+		// though the group memberships will not be shared with the client (in the code below) due to the groups scope
+		// not being granted.
+		refreshedIdentity.UpstreamGroups = oldUntransformedGroups
+	}
+
+	refreshedTransformedGroups, err := applyIdentityTransformationsDuringRefresh(ctx,
+		idp.GetTransforms(),
+		oldTransformedUsername, // this function validates that the old and new transformed usernames match
+		refreshedIdentity.UpstreamUsername,
+		refreshedIdentity.UpstreamGroups,
+		session.Custom.ProviderName,
+		session.Custom.ProviderType,
+	)
 	if err != nil {
 		return err
 	}
 
 	if !skipGroups {
-		warnIfGroupsChanged(ctx, oldTransformedGroups, refreshedGroups, oldTransformedUsername, clientID)
+		warnIfGroupsChanged(ctx, oldTransformedGroups, refreshedTransformedGroups, oldTransformedUsername, accessRequest.GetClient().GetID())
 		// Replace the old value for the downstream groups in the user's session with the new value.
-		session.Fosite.Claims.Extra[oidcapi.IDTokenClaimGroups] = refreshedGroups
+		session.Fosite.Claims.Extra[oidcapi.IDTokenClaimGroups] = refreshedTransformedGroups
 	}
 
 	return nil
@@ -178,7 +226,42 @@ func validateSessionHasUsername(session *psession.PinnipedSession) error {
 	return nil
 }
 
-func getDownstreamGroupsFromSession(session *psession.PinnipedSession) ([]string, error) {
+// applyIdentityTransformationsDuringRefresh is similar to downstreamsession.ApplyIdentityTransformations
+// but with validation that the username has not changed, and with slightly different error messaging.
+func applyIdentityTransformationsDuringRefresh(
+	ctx context.Context,
+	transforms *idtransform.TransformationPipeline,
+	oldTransformedUsername string,
+	upstreamUsername string,
+	upstreamGroups []string,
+	providerName string,
+	providerType psession.ProviderType,
+) ([]string, error) {
+	transformationResult, err := transforms.Evaluate(ctx, upstreamUsername, upstreamGroups)
+	if err != nil {
+		return nil, errUpstreamRefreshError().WithHintf(
+			"Upstream refresh error while applying configured identity transformations.").
+			WithTrace(err).
+			WithDebugf("provider name: %q, provider type: %q", providerName, providerType)
+	}
+
+	if !transformationResult.AuthenticationAllowed {
+		return nil, errUpstreamRefreshError().WithHintf(
+			"Upstream refresh rejected by configured identity policy: %s.", transformationResult.RejectedAuthenticationMessage).
+			WithDebugf("provider name: %q, provider type: %q", providerName, providerType)
+	}
+
+	if oldTransformedUsername != transformationResult.Username {
+		return nil, errUpstreamRefreshError().WithHintf(
+			"Upstream refresh failed.").
+			WithTrace(errors.New("username in upstream refresh does not match previous value")).
+			WithDebugf("provider name: %q, provider type: %q", providerName, providerType)
+	}
+
+	return transformationResult.Groups, nil
+}
+
+func validateAndGetDownstreamGroupsFromSession(session *psession.PinnipedSession) ([]string, error) {
 	extra := session.Fosite.Claims.Extra
 	if extra == nil {
 		return nil, errorsx.WithStack(errMissingUpstreamSessionInternalError())

internal/federationdomain/resolvedprovider/resolved_provider.go

@@ -5,7 +5,6 @@ package resolvedprovider
 
 import (
 	"context"
-	"errors"
 	"net/http"
 
 	"github.com/ory/fosite"
@@ -18,14 +17,74 @@ import (
 	"go.pinniped.dev/pkg/oidcclient/pkce"
 )
 
+// Identity is the information that an identity provider must determine from the upstream IDP during login.
+// This information will also be passed back to the identity provider interface during a refresh flow to
+// represent the user's previous identity from their original login or most recent refresh, to aid in
+// validating the refresh.
 type Identity struct {
-	// Note that the username is stored in SessionData.Username.
-	SessionData      *psession.CustomSessionData
-	Groups           []string
-	Subject          string
-	AdditionalClaims map[string]interface{}
+	// The username extracted from the upstream identity provider, before identity
+	// transformations are applied to determine the final downstream username.
+	// Must not be empty.
+	UpstreamUsername string
+
+	// The group names extracted from the upstream identity provider, before identity transformations
+	// are applied to determine the final downstream group names. nil or an empty list means that the
+	// user belongs to no upstream groups (before applying identity transformations to determine their
+	// downstream group memberships).
+	UpstreamGroups []string
+
+	// The downstream subject determined for this user in an identity provider-specific way.
+	// Must not be empty.
+	DownstreamSubject string
+
+	// The portion of the user's session data which is specific to the upstream identity provider type.
+	// Refer to the fields of psession.CustomSessionData whose types are specific to an identity provider type.
+	// Must not be nil.
+	IDPSpecificSessionData interface{}
 }
 
+// IdentityLoginExtras are additional information that an identity provider may choose to determine
+// during login. This information will not be passed into the
+// FederationDomainResolvedIdentityProvider.UpstreamRefresh function, so it does not impact upstream
+// refreshes. Its fields are optional and may be nil.
+type IdentityLoginExtras struct {
+	// The downstream additional claims determined for this user in an identity provider-specific way, if any.
+	DownstreamAdditionalClaims map[string]interface{}
+
+	// Login warnings to show the user after they exchange their downstream authcode, if any.
+	Warnings []string
+}
+
+// RefreshedIdentity represents the parts of an identity that an identity provider may update
+// from the upstream IDP during a refresh. It will be returned when performing an upstream refresh.
+type RefreshedIdentity struct {
+	// The username extracted from the upstream identity provider, before identity
+	// transformations are applied to determine the final downstream username.
+	// Must not be empty.
+	UpstreamUsername string
+
+	// The group names extracted from the upstream identity provider, before identity
+	// transformations are applied to determine the final downstream group names.
+	// If refreshing the groups was not possible, then set this to nil, and the user's old groups
+	// from their session will be used again. Returning an empty list of groups will mean
+	// that the user's upstream group membership will be updated to make them belong to no
+	// upstream groups (before applying identity transformations to determine their downstream
+	// group memberships).
+	UpstreamGroups []string
+
+	// The portion of the user's session data which is specific to the upstream identity provider type.
+	// Refer to the fields of psession.CustomSessionData whose types are specific to an identity provider type.
+	// Set this to be the potentially updated IDP-specific session data. If no updates were required, then
+	// set this to nil.
+	IDPSpecificSessionData interface{}
+}
+
+// UpstreamAuthorizeRequestState is the state capturing the downstream authorization request, used as a parameter to
+// FederationDomainResolvedIdentityProvider.UpstreamAuthorizeRedirectURL to help formulate the upstream authorization
+// request. It includes the state param that should be sent in the upstream authorization request. It also includes
+// the information needed to create the PKCE and nonce parameters for the upstream authorization request. If the
+// upstream authorization request does not need PKCE and/or nonce params, then implementations of
+// FederationDomainResolvedIdentityProvider.UpstreamAuthorizeRedirectURL may choose to ignore those struct fields.
 type UpstreamAuthorizeRequestState struct {
 	EncodedStateParam string
 	PKCE              pkce.Code
@@ -33,18 +92,41 @@ type UpstreamAuthorizeRequestState struct {
 }
 
 type FederationDomainResolvedIdentityProvider interface {
+	// GetDisplayName returns the display name of this identity provider, as configured in the FederationDomain.
 	GetDisplayName() string
 
+	// GetProvider returns a representation of the upstream identity provider custom resource related to this
+	// identity provider for the FederationDomain, e.g. the OIDCIdentityProvider.
 	GetProvider() upstreamprovider.UpstreamIdentityProviderI
 
+	// GetSessionProviderType returns the type of session created by this identity provider.
 	GetSessionProviderType() psession.ProviderType
 
+	// GetIDPDiscoveryType returns the type of this identity provider, as shown by the IDP discovery endpoint.
 	GetIDPDiscoveryType() v1alpha1.IDPType
 
+	// GetIDPDiscoveryFlows returns the supported flows of this identity provider,
+	// as shown by the IDP discovery endpoint.
 	GetIDPDiscoveryFlows() []v1alpha1.IDPFlow
 
+	// GetTransforms returns the compiled version of the identity transformations and policies configured on the
+	// FederationDomain for this identity provider.
 	GetTransforms() *idtransform.TransformationPipeline
 
+	// CloneIDPSpecificSessionDataFromSession should reach into the provided session and return a clone
+	// of the field which is specific to the upstream identity provider type. If the session's field is
+	// nil, then return nil.
+	// Refer to the fields of psession.CustomSessionData whose types are specific to an identity provider type.
+	CloneIDPSpecificSessionDataFromSession(session *psession.CustomSessionData) interface{}
+
+	// ApplyIDPSpecificSessionDataToSession assigns the IDP-specific portion of the session data into a session.
+	// The IDP-specific session data provided to this function will be from an Identity that was returned by
+	// one of the other functions of this interface, so an implementation of this function can make safe
+	// assumptions about the type of idpSpecificSessionData for casting, based upon how it chooses to return
+	// IDPSpecificSessionData in Identity structs. If the given session already has any IDP-specific session
+	// data, it should be overwritten by this function.
+	ApplyIDPSpecificSessionDataToSession(session *psession.CustomSessionData, idpSpecificSessionData interface{})
+
 	// UpstreamAuthorizeRedirectURL returns the URL to which the user's browser can be redirected to continue
 	// the downstream browser-based authorization flow. Returned errors should be of type fosite.RFC6749Error.
 	UpstreamAuthorizeRedirectURL(state *UpstreamAuthorizeRequestState, downstreamIssuerURL string) (string, error)
@@ -55,25 +137,26 @@ type FederationDomainResolvedIdentityProvider interface {
 	// The groupsWillBeIgnored parameter will be true when the returned groups are going to be ignored by the caller,
 	// in which case this function may be able to save some effort by avoiding getting the user's upstream groups.
 	// Returned errors should be of type fosite.RFC6749Error.
-	Login(ctx context.Context, submittedUsername string, submittedPassword string, groupsWillBeIgnored bool) (*Identity, error)
+	Login(ctx context.Context, submittedUsername string, submittedPassword string, groupsWillBeIgnored bool) (*Identity, *IdentityLoginExtras, error)
 
 	// HandleCallback handles an OAuth-style callback in a browser-based flow. This function should complete
 	// the authorization with the upstream identity provider using the authCode, extract their upstream
 	// identity, and transform it into their downstream identity.
 	// Returned errors should be from the httperr package.
-	HandleCallback(ctx context.Context, authCode string, pkce pkce.Code, nonce nonce.Nonce, redirectURI string) (*Identity, error)
+	HandleCallback(ctx context.Context, authCode string, pkce pkce.Code, nonce nonce.Nonce, redirectURI string) (*Identity, *IdentityLoginExtras, error)
 
 	// UpstreamRefresh performs a refresh with the upstream provider.
-	// The user's session information is passed in, and implementations should be careful mutating anything about the
-	// session because changes will be saved into the session.
-	// If possible, implementations should update the user's group memberships by fetching them from the
+	// The user's previous identity information is provided as a parameter.
+	// Implementations may use this information to assist in refreshes, but mutations to this argument will be ignored.
+	// If possible, implementations should update the user's upstream group memberships by fetching them from the
 	// upstream provider during the refresh, and returning them.
 	// The groupsWillBeIgnored parameter will be true when the returned groups are going to be ignored by the caller,
 	// in which case this function may be able to save some effort by avoiding getting the user's upstream groups.
 	// Returned errors should be of type fosite.RFC6749Error.
-	UpstreamRefresh(ctx context.Context, session *psession.PinnipedSession, groupsWillBeIgnored bool) (refreshedGroups []string, err error)
+	UpstreamRefresh(ctx context.Context, identity *Identity, groupsWillBeIgnored bool) (refreshedIdentity *RefreshedIdentity, err error)
 }
 
+// ErrMissingUpstreamSessionInternalError returns a common type of error that can happen during a login or refresh.
 func ErrMissingUpstreamSessionInternalError() *fosite.RFC6749Error {
 	return &fosite.RFC6749Error{
 		ErrorField:       "error",
@@ -83,6 +166,7 @@ func ErrMissingUpstreamSessionInternalError() *fosite.RFC6749Error {
 	}
 }
 
+// ErrUpstreamRefreshError returns a common type of error that can happen during a refresh.
 func ErrUpstreamRefreshError() *fosite.RFC6749Error {
 	return &fosite.RFC6749Error{
 		ErrorField:       "error",
@@ -90,36 +174,3 @@ func ErrUpstreamRefreshError() *fosite.RFC6749Error {
 		CodeField:        http.StatusUnauthorized,
 	}
 }
-
-func TransformRefreshedIdentity(
-	ctx context.Context,
-	transforms *idtransform.TransformationPipeline,
-	oldTransformedUsername string,
-	upstreamUsername string,
-	upstreamGroups []string,
-	providerName string,
-	providerType psession.ProviderType,
-) (*idtransform.TransformationResult, error) {
-	transformationResult, err := transforms.Evaluate(ctx, upstreamUsername, upstreamGroups)
-	if err != nil {
-		return nil, ErrUpstreamRefreshError().WithHintf(
-			"Upstream refresh error while applying configured identity transformations.").
-			WithTrace(err).
-			WithDebugf("provider name: %q, provider type: %q", providerName, providerType)
-	}
-
-	if !transformationResult.AuthenticationAllowed {
-		return nil, ErrUpstreamRefreshError().WithHintf(
-			"Upstream refresh rejected by configured identity policy: %s.", transformationResult.RejectedAuthenticationMessage).
-			WithDebugf("provider name: %q, provider type: %q", providerName, providerType)
-	}
-
-	if oldTransformedUsername != transformationResult.Username {
-		return nil, ErrUpstreamRefreshError().WithHintf(
-			"Upstream refresh failed.").
-			WithTrace(errors.New("username in upstream refresh does not match previous value")).
-			WithDebugf("provider name: %q, provider type: %q", providerName, providerType)
-	}
-
-	return transformationResult, nil
-}

internal/federationdomain/resolvedprovider/resolvedldap/resolved_ldap_provider.go

@@ -5,6 +5,7 @@ package resolvedldap
 
 import (
 	"context"
+	"fmt"
 	"net/http"
 
 	"github.com/ory/fosite"
@@ -12,7 +13,6 @@ import (
 
 	"go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1"
 	"go.pinniped.dev/internal/authenticators"
-	"go.pinniped.dev/internal/federationdomain/downstreamsession"
 	"go.pinniped.dev/internal/federationdomain/downstreamsubject"
 	"go.pinniped.dev/internal/federationdomain/endpoints/loginurl"
 	"go.pinniped.dev/internal/federationdomain/resolvedprovider"
@@ -64,6 +64,33 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) GetTransforms() *idtransf
 	return p.Transforms
 }
 
+func (p *FederationDomainResolvedLDAPIdentityProvider) CloneIDPSpecificSessionDataFromSession(session *psession.CustomSessionData) interface{} {
+	switch p.GetSessionProviderType() {
+	case psession.ProviderTypeLDAP:
+		if session.LDAP == nil {
+			return nil
+		}
+		return session.LDAP.Clone()
+	case psession.ProviderTypeActiveDirectory:
+		if session.ActiveDirectory == nil {
+			return nil
+		}
+		return session.ActiveDirectory.Clone()
+	case psession.ProviderTypeOIDC: // this is just here to avoid a lint error about not handling all cases
+		fallthrough
+	default:
+		return nil
+	}
+}
+
+func (p *FederationDomainResolvedLDAPIdentityProvider) ApplyIDPSpecificSessionDataToSession(session *psession.CustomSessionData, idpSpecificSessionData interface{}) {
+	if p.GetSessionProviderType() == psession.ProviderTypeActiveDirectory {
+		session.ActiveDirectory = idpSpecificSessionData.(*psession.ActiveDirectorySessionData)
+		return
+	}
+	session.LDAP = idpSpecificSessionData.(*psession.LDAPSessionData)
+}
+
 func (p *FederationDomainResolvedLDAPIdentityProvider) UpstreamAuthorizeRedirectURL(state *resolvedprovider.UpstreamAuthorizeRequestState, downstreamIssuerURL string) (string, error) {
 	loginURL, err := loginurl.URL(downstreamIssuerURL, state.EncodedStateParam, loginurl.ShowNoError)
 	if err != nil {
@@ -99,33 +126,49 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) Login(
 	submittedUsername string,
 	submittedPassword string,
 	groupsWillBeIgnored bool,
-) (*resolvedprovider.Identity, error) {
+) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) {
 	authenticateResponse, authenticated, err := p.Provider.AuthenticateUser(ctx, submittedUsername, submittedPassword, groupsWillBeIgnored)
 	if err != nil {
 		plog.WarningErr("unexpected error during upstream LDAP authentication", err, "upstreamName", p.Provider.GetName())
-		return nil, ErrUnexpectedUpstreamLDAPError.WithWrap(err)
+		return nil, nil, ErrUnexpectedUpstreamLDAPError.WithWrap(err)
 	}
 	if !authenticated {
-		return nil, ErrAccessDeniedDueToUsernamePasswordNotAccepted
+		return nil, nil, ErrAccessDeniedDueToUsernamePasswordNotAccepted
 	}
 
-	subject := downstreamSubjectFromUpstreamLDAP(p.Provider, authenticateResponse, p.DisplayName)
+	subject := downstreamSubjectFromUpstreamLDAP(p.Provider, authenticateResponse, p.GetDisplayName())
 	upstreamUsername := authenticateResponse.User.GetName()
 	upstreamGroups := authenticateResponse.User.GetGroups()
 
-	username, groups, err := downstreamsession.ApplyIdentityTransformations(ctx, p.Transforms, upstreamUsername, upstreamGroups)
-	if err != nil {
-		return nil, fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error())
+	var sessionData interface{}
+	switch p.GetSessionProviderType() {
+	case psession.ProviderTypeLDAP:
+		sessionData = &psession.LDAPSessionData{
+			UserDN:                 authenticateResponse.DN,
+			ExtraRefreshAttributes: authenticateResponse.ExtraRefreshAttributes,
+		}
+	case psession.ProviderTypeActiveDirectory:
+		sessionData = &psession.ActiveDirectorySessionData{
+			UserDN:                 authenticateResponse.DN,
+			ExtraRefreshAttributes: authenticateResponse.ExtraRefreshAttributes,
+		}
+	case psession.ProviderTypeOIDC: // this is just here to avoid a lint error about not handling all cases
+		fallthrough
+	default:
+		return nil, nil, ErrUnexpectedUpstreamLDAPError.WithWrap(fmt.Errorf("unexpected provider type %q", p.GetSessionProviderType()))
 	}
 
-	customSessionData := makeDownstreamLDAPOrADCustomSessionData(
-		p.Provider, p.SessionProviderType, authenticateResponse, username, upstreamUsername, upstreamGroups)
-
 	return &resolvedprovider.Identity{
-		SessionData: customSessionData,
-		Groups:      groups,
-		Subject:     subject,
-	}, nil
+			UpstreamUsername:       upstreamUsername,
+			UpstreamGroups:         upstreamGroups,
+			DownstreamSubject:      subject,
+			IDPSpecificSessionData: sessionData,
+		},
+		&resolvedprovider.IdentityLoginExtras{
+			DownstreamAdditionalClaims: nil,
+			Warnings:                   nil,
+		},
+		nil
 }
 
 func (p *FederationDomainResolvedLDAPIdentityProvider) HandleCallback(
@@ -134,109 +177,73 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) HandleCallback(
 	_pkce pkce.Code,
 	_nonce nonce.Nonce,
 	_redirectURI string,
-) (*resolvedprovider.Identity, error) {
-	return nil, httperr.New(http.StatusInternalServerError, "not supported for this type of identity provider")
+) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) {
+	return nil, nil, httperr.New(http.StatusInternalServerError,
+		"HandleCallback() is not supported for LDAP and ActiveDirectory types of identity provider")
 }
 
 func (p *FederationDomainResolvedLDAPIdentityProvider) UpstreamRefresh(
 	ctx context.Context,
-	session *psession.PinnipedSession,
+	identity *resolvedprovider.Identity,
 	groupsWillBeIgnored bool,
-) (refreshedGroups []string, err error) {
-	s := session.Custom
-
+) (refreshedIdentity *resolvedprovider.RefreshedIdentity, err error) {
 	var dn string
-	if s.ProviderType == psession.ProviderTypeLDAP {
-		dn = s.LDAP.UserDN
-	} else if s.ProviderType == psession.ProviderTypeActiveDirectory {
-		dn = s.ActiveDirectory.UserDN
-	}
-
-	validLDAP := s.ProviderType == psession.ProviderTypeLDAP && s.LDAP != nil && s.LDAP.UserDN != ""
-	validAD := s.ProviderType == psession.ProviderTypeActiveDirectory && s.ActiveDirectory != nil && s.ActiveDirectory.UserDN != ""
-	if !(validLDAP || validAD) {
-		return nil, errorsx.WithStack(resolvedprovider.ErrMissingUpstreamSessionInternalError())
-	}
-
 	var additionalAttributes map[string]string
-	if s.ProviderType == psession.ProviderTypeLDAP {
-		additionalAttributes = s.LDAP.ExtraRefreshAttributes
-	} else {
-		additionalAttributes = s.ActiveDirectory.ExtraRefreshAttributes
+
+	switch p.GetSessionProviderType() {
+	case psession.ProviderTypeLDAP:
+		sessionData, ok := identity.IDPSpecificSessionData.(*psession.LDAPSessionData)
+		if !ok {
+			// This shouldn't really happen.
+			return nil, errorsx.WithStack(resolvedprovider.ErrMissingUpstreamSessionInternalError())
+		}
+		dn = sessionData.UserDN
+		additionalAttributes = sessionData.ExtraRefreshAttributes
+	case psession.ProviderTypeActiveDirectory:
+		sessionData, ok := identity.IDPSpecificSessionData.(*psession.ActiveDirectorySessionData)
+		if !ok {
+			// This shouldn't really happen.
+			return nil, errorsx.WithStack(resolvedprovider.ErrMissingUpstreamSessionInternalError())
+		}
+		dn = sessionData.UserDN
+		additionalAttributes = sessionData.ExtraRefreshAttributes
+	case psession.ProviderTypeOIDC: // this is just here to avoid a lint error about not handling all cases
+		fallthrough
+	default:
+		// This shouldn't really happen.
+		return nil, resolvedprovider.ErrUpstreamRefreshError().WithHintf(
+			"Unexpected provider type during refresh %q", p.GetSessionProviderType()).WithTrace(err).
+			WithDebugf("provider name: %q, provider type: %q", p.Provider.GetName(), p.GetSessionProviderType())
 	}
 
-	if session.IDTokenClaims().AuthTime.IsZero() {
+	if dn == "" {
 		return nil, errorsx.WithStack(resolvedprovider.ErrMissingUpstreamSessionInternalError())
 	}
 
-	oldTransformedUsername := session.Custom.Username
-	oldUntransformedUsername := session.Custom.UpstreamUsername
-	oldUntransformedGroups := session.Custom.UpstreamGroups
-
 	plog.Debug("attempting upstream refresh request",
-		"providerName", s.ProviderName, "providerType", s.ProviderType, "providerUID", s.ProviderUID)
+		"providerName", p.Provider.GetName(), "providerType", p.GetSessionProviderType(), "providerUID", p.Provider.GetResourceUID())
 
 	refreshedUntransformedGroups, err := p.Provider.PerformRefresh(ctx, upstreamprovider.RefreshAttributes{
-		Username:             oldUntransformedUsername,
-		Subject:              session.Fosite.Claims.Subject,
+		Username:             identity.UpstreamUsername,
+		Subject:              identity.DownstreamSubject,
 		DN:                   dn,
-		Groups:               oldUntransformedGroups,
+		Groups:               identity.UpstreamGroups,
 		AdditionalAttributes: additionalAttributes,
 		SkipGroups:           groupsWillBeIgnored,
-	}, p.DisplayName)
+	}, p.GetDisplayName())
 	if err != nil {
 		return nil, resolvedprovider.ErrUpstreamRefreshError().WithHint(
 			"Upstream refresh failed.").WithTrace(err).
-			WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)
+			WithDebugf("provider name: %q, provider type: %q", p.Provider.GetName(), p.GetSessionProviderType())
 	}
 
-	transformationResult, err := resolvedprovider.TransformRefreshedIdentity(ctx,
-		p.Transforms,
-		oldTransformedUsername,
-		oldUntransformedUsername, // LDAP PerformRefresh validates that the username did not change, so this is also the refreshed upstream username
-		refreshedUntransformedGroups,
-		s.ProviderName,
-		s.ProviderType,
-	)
-	if err != nil {
-		return nil, err
-	}
-
-	return transformationResult.Groups, nil
-}
-
-func makeDownstreamLDAPOrADCustomSessionData(
-	ldapUpstream upstreamprovider.UpstreamLDAPIdentityProviderI,
-	idpType psession.ProviderType,
-	authenticateResponse *authenticators.Response,
-	username string,
-	untransformedUpstreamUsername string,
-	untransformedUpstreamGroups []string,
-) *psession.CustomSessionData {
-	customSessionData := &psession.CustomSessionData{
-		Username:         username,
-		UpstreamUsername: untransformedUpstreamUsername,
-		UpstreamGroups:   untransformedUpstreamGroups,
-		ProviderUID:      ldapUpstream.GetResourceUID(),
-		ProviderName:     ldapUpstream.GetName(),
-		ProviderType:     idpType,
-	}
-
-	if idpType == psession.ProviderTypeLDAP {
-		customSessionData.LDAP = &psession.LDAPSessionData{
-			UserDN:                 authenticateResponse.DN,
-			ExtraRefreshAttributes: authenticateResponse.ExtraRefreshAttributes,
-		}
-	}
-
-	if idpType == psession.ProviderTypeActiveDirectory {
-		customSessionData.ActiveDirectory = &psession.ActiveDirectorySessionData{
-			UserDN:                 authenticateResponse.DN,
-			ExtraRefreshAttributes: authenticateResponse.ExtraRefreshAttributes,
-		}
-	}
-
-	return customSessionData
+	return &resolvedprovider.RefreshedIdentity{
+		// LDAP PerformRefresh validates that the username did not change during refresh,
+		// so the original upstream username is also the refreshed upstream username.
+		UpstreamUsername:       identity.UpstreamUsername,
+		UpstreamGroups:         refreshedUntransformedGroups,
+		IDPSpecificSessionData: nil,
+	}, nil
 }
 
 func downstreamSubjectFromUpstreamLDAP(

internal/federationdomain/resolvedprovider/resolvedoidc/resolved_oidc_provider.go

@@ -20,7 +20,6 @@ import (
 	"go.pinniped.dev/generated/latest/apis/supervisor/oidc"
 	oidcapi "go.pinniped.dev/generated/latest/apis/supervisor/oidc"
 	"go.pinniped.dev/internal/constable"
-	"go.pinniped.dev/internal/federationdomain/downstreamsession"
 	"go.pinniped.dev/internal/federationdomain/downstreamsubject"
 	"go.pinniped.dev/internal/federationdomain/resolvedprovider"
 	"go.pinniped.dev/internal/federationdomain/upstreamprovider"
@@ -87,6 +86,17 @@ func (p *FederationDomainResolvedOIDCIdentityProvider) GetTransforms() *idtransf
 	return p.Transforms
 }
 
+func (p *FederationDomainResolvedOIDCIdentityProvider) CloneIDPSpecificSessionDataFromSession(session *psession.CustomSessionData) interface{} {
+	if session.OIDC == nil {
+		return nil
+	}
+	return session.OIDC.Clone()
+}
+
+func (p *FederationDomainResolvedOIDCIdentityProvider) ApplyIDPSpecificSessionDataToSession(session *psession.CustomSessionData, idpSpecificSessionData interface{}) {
+	session.OIDC = idpSpecificSessionData.(*psession.OIDCSessionData)
+}
+
 func (p *FederationDomainResolvedOIDCIdentityProvider) UpstreamAuthorizeRedirectURL(state *resolvedprovider.UpstreamAuthorizeRequestState, downstreamIssuerURL string) (string, error) {
 	upstreamOAuthConfig := oauth2.Config{
 		ClientID: p.Provider.GetClientID(),
@@ -120,10 +130,10 @@ func (p *FederationDomainResolvedOIDCIdentityProvider) Login(
 	submittedUsername string,
 	submittedPassword string,
 	_groupsWillBeIgnored bool, // ignored because we always compute the user's group memberships for OIDC, if possible
-) (*resolvedprovider.Identity, error) {
+) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) {
 	if !p.Provider.AllowsPasswordGrant() {
 		// Return a user-friendly error for this case which is entirely within our control.
-		return nil, fosite.ErrAccessDenied.WithHint(
+		return nil, nil, fosite.ErrAccessDenied.WithHint(
 			"Resource owner password credentials grant is not allowed for this upstream provider according to its configuration.")
 	}
 
@@ -136,35 +146,35 @@ func (p *FederationDomainResolvedOIDCIdentityProvider) Login(
 		// However, the exact response is undefined in the sense that there is no such thing as a password grant in
 		// the OIDC spec, so we don't try too hard to read the upstream errors in this case. (E.g. Dex departs from the
 		// spec and returns something other than an "invalid_grant" error for bad resource owner credentials.)
-		return nil, fosite.ErrAccessDenied.WithDebug(err.Error()) // WithDebug hides the error from the client
+		return nil, nil, fosite.ErrAccessDenied.WithDebug(err.Error()) // WithDebug hides the error from the client
 	}
 
-	subject, upstreamUsername, upstreamGroups, err := getDownstreamIdentityFromUpstreamIDToken(
-		p.Provider, token.IDToken.Claims, p.DisplayName,
+	subject, upstreamUsername, upstreamGroups, err := getIdentityFromUpstreamIDToken(
+		p.Provider, token.IDToken.Claims, p.GetDisplayName(),
 	)
 	if err != nil {
 		// Return a user-friendly error for this case which is entirely within our control.
-		return nil, fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error())
-	}
-
-	username, groups, err := downstreamsession.ApplyIdentityTransformations(ctx, p.Transforms, upstreamUsername, upstreamGroups)
-	if err != nil {
-		return nil, fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error())
+		return nil, nil, fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error())
 	}
 
 	additionalClaims := mapAdditionalClaimsFromUpstreamIDToken(p.Provider, token.IDToken.Claims)
 
-	customSessionData, err := makeDownstreamOIDCCustomSessionData(p.Provider, token, username, upstreamUsername, upstreamGroups)
+	oidcSessionData, warnings, err := makeDownstreamOIDCSessionData(p.Provider, token)
 	if err != nil {
-		return nil, fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error())
+		return nil, nil, fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error())
 	}
 
 	return &resolvedprovider.Identity{
-		SessionData:      customSessionData,
-		Groups:           groups,
-		Subject:          subject,
-		AdditionalClaims: additionalClaims,
-	}, nil
+			UpstreamUsername:       upstreamUsername,
+			UpstreamGroups:         upstreamGroups,
+			DownstreamSubject:      subject,
+			IDPSpecificSessionData: oidcSessionData,
+		},
+		&resolvedprovider.IdentityLoginExtras{
+			DownstreamAdditionalClaims: additionalClaims,
+			Warnings:                   warnings,
+		},
+		nil
 }
 
 func (p *FederationDomainResolvedOIDCIdentityProvider) HandleCallback(
@@ -173,7 +183,7 @@ func (p *FederationDomainResolvedOIDCIdentityProvider) HandleCallback(
 	pkce pkce.Code,
 	nonce nonce.Nonce,
 	redirectURI string,
-) (*resolvedprovider.Identity, error) {
+) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) {
 	token, err := p.Provider.ExchangeAuthcodeAndValidateTokens(
 		ctx,
 		authCode,
@@ -183,76 +193,68 @@ func (p *FederationDomainResolvedOIDCIdentityProvider) HandleCallback(
 	)
 	if err != nil {
 		plog.WarningErr("error exchanging and validating upstream tokens", err, "upstreamName", p.Provider.GetName())
-		return nil, httperr.New(http.StatusBadGateway, "error exchanging and validating upstream tokens")
+		return nil, nil, httperr.New(http.StatusBadGateway, "error exchanging and validating upstream tokens")
 	}
 
-	subject, upstreamUsername, upstreamGroups, err := getDownstreamIdentityFromUpstreamIDToken(
-		p.Provider, token.IDToken.Claims, p.DisplayName,
+	subject, upstreamUsername, upstreamGroups, err := getIdentityFromUpstreamIDToken(
+		p.Provider, token.IDToken.Claims, p.GetDisplayName(),
 	)
 	if err != nil {
-		return nil, httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err)
-	}
-
-	username, groups, err := downstreamsession.ApplyIdentityTransformations(
-		ctx, p.Transforms, upstreamUsername, upstreamGroups,
-	)
-	if err != nil {
-		return nil, httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err)
+		return nil, nil, httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err)
 	}
 
 	additionalClaims := mapAdditionalClaimsFromUpstreamIDToken(p.Provider, token.IDToken.Claims)
 
-	customSessionData, err := makeDownstreamOIDCCustomSessionData(
-		p.Provider, token, username, upstreamUsername, upstreamGroups,
-	)
+	oidcSessionData, warnings, err := makeDownstreamOIDCSessionData(p.Provider, token)
 	if err != nil {
-		return nil, httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err)
+		return nil, nil, httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err)
 	}
 
 	return &resolvedprovider.Identity{
-		SessionData:      customSessionData,
-		Groups:           groups,
-		Subject:          subject,
-		AdditionalClaims: additionalClaims,
-	}, nil
+			UpstreamUsername:       upstreamUsername,
+			UpstreamGroups:         upstreamGroups,
+			DownstreamSubject:      subject,
+			IDPSpecificSessionData: oidcSessionData,
+		},
+		&resolvedprovider.IdentityLoginExtras{
+			DownstreamAdditionalClaims: additionalClaims,
+			Warnings:                   warnings,
+		},
+		nil
 }
 
 func (p *FederationDomainResolvedOIDCIdentityProvider) UpstreamRefresh(
 	ctx context.Context,
-	session *psession.PinnipedSession,
+	identity *resolvedprovider.Identity,
 	groupsWillBeIgnored bool,
-) (refreshedGroups []string, err error) {
-	s := session.Custom
-
-	if s.OIDC == nil {
+) (refreshedIdentity *resolvedprovider.RefreshedIdentity, err error) {
+	sessionData, ok := identity.IDPSpecificSessionData.(*psession.OIDCSessionData)
+	if !ok {
+		// This shouldn't really happen.
 		return nil, errorsx.WithStack(resolvedprovider.ErrMissingUpstreamSessionInternalError())
 	}
 
-	accessTokenStored := s.OIDC.UpstreamAccessToken != ""
-	refreshTokenStored := s.OIDC.UpstreamRefreshToken != ""
+	accessTokenStored := sessionData.UpstreamAccessToken != ""
+	refreshTokenStored := sessionData.UpstreamRefreshToken != ""
 
 	exactlyOneTokenStored := (accessTokenStored || refreshTokenStored) && !(accessTokenStored && refreshTokenStored)
 	if !exactlyOneTokenStored {
 		return nil, errorsx.WithStack(resolvedprovider.ErrMissingUpstreamSessionInternalError())
 	}
 
-	oldTransformedUsername := session.Custom.Username
-	oldUntransformedUsername := session.Custom.UpstreamUsername
-	oldUntransformedGroups := session.Custom.UpstreamGroups
-
 	plog.Debug("attempting upstream refresh request",
-		"providerName", s.ProviderName, "providerType", s.ProviderType, "providerUID", s.ProviderUID)
+		"providerName", p.Provider.GetName(), "providerType", p.GetSessionProviderType(), "providerUID", p.Provider.GetResourceUID())
 
 	var tokens *oauth2.Token
 	if refreshTokenStored {
-		tokens, err = p.Provider.PerformRefresh(ctx, s.OIDC.UpstreamRefreshToken)
+		tokens, err = p.Provider.PerformRefresh(ctx, sessionData.UpstreamRefreshToken)
 		if err != nil {
 			return nil, resolvedprovider.ErrUpstreamRefreshError().WithHint(
 				"Upstream refresh failed.",
-			).WithTrace(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)
+			).WithTrace(err).WithDebugf("provider name: %q, provider type: %q", p.Provider.GetName(), p.GetSessionProviderType())
 		}
 	} else {
-		tokens = &oauth2.Token{AccessToken: s.OIDC.UpstreamAccessToken}
+		tokens = &oauth2.Token{AccessToken: sessionData.UpstreamAccessToken}
 	}
 
 	// Upstream refresh may or may not return a new ID token. From the spec:
@@ -270,13 +272,13 @@ func (p *FederationDomainResolvedOIDCIdentityProvider) UpstreamRefresh(
 	if err != nil {
 		return nil, resolvedprovider.ErrUpstreamRefreshError().WithHintf(
 			"Upstream refresh returned an invalid ID token or UserInfo response.").WithTrace(err).
-			WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)
+			WithDebugf("provider name: %q, provider type: %q", p.Provider.GetName(), p.GetSessionProviderType())
 	}
 	mergedClaims := validatedTokens.IDToken.Claims
 
 	// To the extent possible, check that the user's basic identity hasn't changed. We check that their downstream
 	// username has not changed separately below, as part of reapplying the transformations.
-	err = validateSubjectAndIssuerUnchangedSinceInitialLogin(mergedClaims, session)
+	err = validateUpstreamSubjectAndIssuerUnchangedSinceInitialLogin(mergedClaims, sessionData, p.Provider.GetName(), p.GetSessionProviderType())
 	if err != nil {
 		return nil, err
 	}
@@ -294,7 +296,7 @@ func (p *FederationDomainResolvedOIDCIdentityProvider) UpstreamRefresh(
 		if err != nil {
 			return nil, resolvedprovider.ErrUpstreamRefreshError().WithHintf(
 				"Upstream refresh error while extracting groups claim.").WithTrace(err).
-				WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)
+				WithDebugf("provider name: %q, provider type: %q", p.Provider.GetName(), p.GetSessionProviderType())
 		}
 	}
 
@@ -304,46 +306,35 @@ func (p *FederationDomainResolvedOIDCIdentityProvider) UpstreamRefresh(
 
 	if !hasRefreshedUntransformedUsername {
 		// If we could not get a new username, then we still need the untransformed username to be able to
-		// run the transformations again, so fetch the original untransformed username from the session.
-		refreshedUntransformedUsername = oldUntransformedUsername
-	}
-	if refreshedUntransformedGroups == nil {
-		// If we could not get a new list of groups, then we still need the untransformed groups list to be able to
-		// run the transformations again, so fetch the original untransformed groups list from the session.
-		// We should also run the transformations on the original groups even when the groups scope was not granted,
-		// because a transformation policy may want to reject the authentication based on the group memberships, even
-		// though the group memberships will not be shared with the client (in the code below) due to the groups scope
-		// not being granted.
-		refreshedUntransformedGroups = oldUntransformedGroups
+		// run the transformations again, so use the original untransformed username from the session.
+		refreshedUntransformedUsername = identity.UpstreamUsername
 	}
 
-	transformationResult, err := resolvedprovider.TransformRefreshedIdentity(ctx,
-		p.Transforms,
-		oldTransformedUsername,
-		refreshedUntransformedUsername,
-		refreshedUntransformedGroups,
-		s.ProviderName,
-		s.ProviderType,
-	)
-	if err != nil {
-		return nil, err
-	}
+	updatedSessionData := sessionData.Clone()
 
 	// 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 tokens.RefreshToken != "" {
 		plog.Debug("upstream refresh request returned a new refresh token",
-			"providerName", s.ProviderName, "providerType", s.ProviderType, "providerUID", s.ProviderUID)
-		s.OIDC.UpstreamRefreshToken = tokens.RefreshToken
+			"providerName", p.Provider.GetName(), "providerType", p.GetSessionProviderType(), "providerUID", p.Provider.GetResourceUID())
+
+		updatedSessionData.UpstreamRefreshToken = tokens.RefreshToken
 	}
 
-	return transformationResult.Groups, nil
+	return &resolvedprovider.RefreshedIdentity{
+		UpstreamUsername:       refreshedUntransformedUsername,
+		UpstreamGroups:         refreshedUntransformedGroups,
+		IDPSpecificSessionData: updatedSessionData,
+	}, nil
 }
 
-func validateSubjectAndIssuerUnchangedSinceInitialLogin(mergedClaims map[string]interface{}, session *psession.PinnipedSession) error {
-	s := session.Custom
-
+func validateUpstreamSubjectAndIssuerUnchangedSinceInitialLogin(
+	mergedClaims map[string]interface{},
+	s *psession.OIDCSessionData,
+	providerName string,
+	providerType psession.ProviderType,
+) error {
 	// 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.
@@ -355,21 +346,21 @@ func validateSubjectAndIssuerUnchangedSinceInitialLogin(mergedClaims map[string]
 	if !hasSub {
 		return resolvedprovider.ErrUpstreamRefreshError().WithHintf(
 			"Upstream refresh failed.").WithTrace(errors.New("subject in upstream refresh not found")).
-			WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)
+			WithDebugf("provider name: %q, provider type: %q", providerName, providerType)
 	}
-	if s.OIDC.UpstreamSubject != newSub {
+	if s.UpstreamSubject != newSub {
 		return resolvedprovider.ErrUpstreamRefreshError().WithHintf(
 			"Upstream refresh failed.").WithTrace(errors.New("subject in upstream refresh does not match previous value")).
-			WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)
+			WithDebugf("provider name: %q, provider type: %q", providerName, providerType)
 	}
 
 	newIssuer, hasIssuer := getString(mergedClaims, oidcapi.IDTokenClaimIssuer)
 	// 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 {
+	if hasIssuer && s.UpstreamIssuer != newIssuer {
 		return resolvedprovider.ErrUpstreamRefreshError().WithHintf(
 			"Upstream refresh failed.").WithTrace(errors.New("issuer in upstream refresh does not match previous value")).
-			WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)
+			WithDebugf("provider name: %q, provider type: %q", providerName, providerType)
 	}
 
 	return nil
@@ -380,33 +371,22 @@ func getString(m map[string]interface{}, key string) (string, bool) {
 	return val, ok
 }
 
-func makeDownstreamOIDCCustomSessionData(
+func makeDownstreamOIDCSessionData(
 	oidcUpstream upstreamprovider.UpstreamOIDCIdentityProviderI,
 	token *oidctypes.Token,
-	username string,
-	untransformedUpstreamUsername string,
-	untransformedUpstreamGroups []string,
-) (*psession.CustomSessionData, error) {
+) (*psession.OIDCSessionData, []string, error) {
 	upstreamSubject, err := extractStringClaimValue(oidc.IDTokenClaimSubject, oidcUpstream.GetName(), token.IDToken.Claims)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	upstreamIssuer, err := extractStringClaimValue(oidc.IDTokenClaimIssuer, oidcUpstream.GetName(), token.IDToken.Claims)
 	if err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 
-	customSessionData := &psession.CustomSessionData{
-		Username:         username,
-		UpstreamUsername: untransformedUpstreamUsername,
-		UpstreamGroups:   untransformedUpstreamGroups,
-		ProviderUID:      oidcUpstream.GetResourceUID(),
-		ProviderName:     oidcUpstream.GetName(),
-		ProviderType:     psession.ProviderTypeOIDC,
-		OIDC: &psession.OIDCSessionData{
-			UpstreamIssuer:  upstreamIssuer,
-			UpstreamSubject: upstreamSubject,
-		},
+	sessionData := &psession.OIDCSessionData{
+		UpstreamIssuer:  upstreamIssuer,
+		UpstreamSubject: upstreamSubject,
 	}
 
 	const pleaseCheck = "please check configuration of OIDCIdentityProvider and the client in the " +
@@ -417,41 +397,43 @@ func makeDownstreamOIDCCustomSessionData(
 		"additionalParams", oidcUpstream.GetAdditionalAuthcodeParams(),
 	}
 
+	var warnings []string
+
 	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
+		sessionData.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")
+			return nil, 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
+		sessionData.UpstreamAccessToken = token.AccessToken.Token
 		// When we are in a flow where we will be performing access token based refresh, issue a warning to the client if the access
 		// token lifetime is very short, since that would mean that the user's session is very short.
 		// The warnings are stored here and will be processed by the token handler.
 		threeHoursFromNow := metav1.NewTime(time.Now().Add(3 * time.Hour))
 		if !token.AccessToken.Expiry.IsZero() && token.AccessToken.Expiry.Before(&threeHoursFromNow) {
-			customSessionData.Warnings = append(customSessionData.Warnings, "Access token from identity provider has lifetime of less than 3 hours. Expect frequent prompts to log in.")
+			warnings = []string{"Access token from identity provider has lifetime of less than 3 hours. Expect frequent prompts to log in."}
 		}
 	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 nil, nil, errors.New("neither access token nor refresh token returned by upstream provider")
 	}
 
-	return customSessionData, nil
+	return sessionData, warnings, nil
 }
 
-// getDownstreamIdentityFromUpstreamIDToken returns the mapped subject, username, and group names, in that order.
-func getDownstreamIdentityFromUpstreamIDToken(
+// getIdentityFromUpstreamIDToken returns the mapped subject, username, and group names, in that order.
+func getIdentityFromUpstreamIDToken(
 	upstreamIDPConfig upstreamprovider.UpstreamOIDCIdentityProviderI,
 	idTokenClaims map[string]interface{},
 	idpDisplayName string,
 ) (string, string, []string, error) {
-	subject, username, err := getSubjectAndUsernameFromUpstreamIDToken(upstreamIDPConfig, idTokenClaims, idpDisplayName)
+	subject, username, err := getDownstreamSubjectAndUpstreamUsernameFromUpstreamIDToken(upstreamIDPConfig, idTokenClaims, idpDisplayName)
 	if err != nil {
 		return "", "", nil, err
 	}
@@ -485,7 +467,7 @@ func mapAdditionalClaimsFromUpstreamIDToken(
 	return mapped
 }
 
-func getSubjectAndUsernameFromUpstreamIDToken(
+func getDownstreamSubjectAndUpstreamUsernameFromUpstreamIDToken(
 	upstreamIDPConfig upstreamprovider.UpstreamOIDCIdentityProviderI,
 	idTokenClaims map[string]interface{},
 	idpDisplayName string,
@@ -504,7 +486,7 @@ func getSubjectAndUsernameFromUpstreamIDToken(
 
 	usernameClaimName := upstreamIDPConfig.GetUsernameClaim()
 	if usernameClaimName == "" {
-		return subject, downstreamUsernameFromUpstreamOIDCSubject(upstreamIssuer, upstreamSubject), nil
+		return subject, mappedUsernameFromUpstreamOIDCSubject(upstreamIssuer, upstreamSubject), nil
 	}
 
 	// If the upstream username claim is configured to be the special "email" claim and the upstream "email_verified"
@@ -572,7 +554,7 @@ func extractStringClaimValue(claimName string, upstreamIDPName string, idTokenCl
 	return valueAsString, nil
 }
 
-func downstreamUsernameFromUpstreamOIDCSubject(upstreamIssuerAsString string, upstreamSubject string) string {
+func mappedUsernameFromUpstreamOIDCSubject(upstreamIssuerAsString string, upstreamSubject string) string {
 	return fmt.Sprintf("%s?%s=%s", upstreamIssuerAsString,
 		oidc.IDTokenClaimSubject, url.QueryEscape(upstreamSubject),
 	)

internal/psession/pinniped_session.go

@@ -1,9 +1,10 @@
-// Copyright 2021-2023 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package psession
 
 import (
+	"maps"
 	"time"
 
 	"github.com/mohae/deepcopy"
@@ -108,18 +109,37 @@ type OIDCSessionData struct {
 	UpstreamIssuer string `json:"upstreamIssuer"`
 }
 
+func (s *OIDCSessionData) Clone() *OIDCSessionData {
+	dataCopy := *s // this shortcut works because all fields in this type are currently strings (no pointers)
+	return &dataCopy
+}
+
 // LDAPSessionData is the additional data needed by Pinniped when the upstream IDP is an LDAP provider.
 type LDAPSessionData struct {
 	UserDN                 string            `json:"userDN"`
 	ExtraRefreshAttributes map[string]string `json:"extraRefreshAttributes,omitempty"`
 }
 
+func (s *LDAPSessionData) Clone() *LDAPSessionData {
+	return &LDAPSessionData{
+		UserDN:                 s.UserDN,
+		ExtraRefreshAttributes: maps.Clone(s.ExtraRefreshAttributes), // shallow copy works because all keys and values are strings
+	}
+}
+
 // ActiveDirectorySessionData is the additional data needed by Pinniped when the upstream IDP is an Active Directory provider.
 type ActiveDirectorySessionData struct {
 	UserDN                 string            `json:"userDN"`
 	ExtraRefreshAttributes map[string]string `json:"extraRefreshAttributes,omitempty"`
 }
 
+func (s *ActiveDirectorySessionData) Clone() *ActiveDirectorySessionData {
+	return &ActiveDirectorySessionData{
+		UserDN:                 s.UserDN,
+		ExtraRefreshAttributes: maps.Clone(s.ExtraRefreshAttributes), // shallow copy works because all keys and values are strings
+	}
+}
+
 // NewPinnipedSession returns a new empty session.
 func NewPinnipedSession() *PinnipedSession {
 	return &PinnipedSession{