Rename a func and collapse applying id transforms into creating session

internal/federationdomain/downstreamsession/downstream_session.go

@@ -30,27 +30,29 @@ const idTransformUnexpectedErr = constable.Error("configured identity transforma
 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.
+// NewPinnipedSession applies the configured FederationDomain identity transformations
+// and creates a downstream Pinniped session.
 func NewPinnipedSession(
+	ctx context.Context,
 	idp resolvedprovider.FederationDomainResolvedIdentityProvider,
 	c *SessionConfig,
-) *psession.PinnipedSession {
+) (*psession.PinnipedSession, error) {
 	now := time.Now().UTC()
 
+	downstreamUsername, downstreamGroups, err := applyIdentityTransformations(ctx,
+		idp.GetTransforms(), c.UpstreamIdentity.UpstreamUsername, c.UpstreamIdentity.UpstreamGroups)
+	if err != nil {
+		return nil, err
+	}
+
 	customSessionData := &psession.CustomSessionData{
-		Username:         c.Username,
+		Username:         downstreamUsername,
 		UpstreamUsername: c.UpstreamIdentity.UpstreamUsername,
 		UpstreamGroups:   c.UpstreamIdentity.UpstreamGroups,
 		ProviderUID:      idp.GetProvider().GetResourceUID(),
@@ -76,15 +78,14 @@ func NewPinnipedSession(
 	extras[oidcapi.IDTokenClaimAuthorizedParty] = c.ClientID
 
 	if slices.Contains(c.GrantedScopes, oidcapi.ScopeUsername) {
-		extras[oidcapi.IDTokenClaimUsername] = c.Username
+		extras[oidcapi.IDTokenClaimUsername] = downstreamUsername
 	}
 
 	if slices.Contains(c.GrantedScopes, oidcapi.ScopeGroups) {
-		groups := c.Groups
-		if groups == nil {
-			groups = []string{}
+		if downstreamGroups == nil {
+			downstreamGroups = []string{}
 		}
-		extras[oidcapi.IDTokenClaimGroups] = groups
+		extras[oidcapi.IDTokenClaimGroups] = downstreamGroups
 	}
 
 	if len(c.UpstreamLoginExtras.DownstreamAdditionalClaims) > 0 {
@@ -93,7 +94,7 @@ func NewPinnipedSession(
 
 	pinnipedSession.IDTokenClaims().Extra = extras
 
-	return pinnipedSession
+	return pinnipedSession, nil
 }
 
 // AutoApproveScopes auto-grants the scopes which we support and for which we do not require end-user approval,
@@ -122,9 +123,9 @@ func AutoApproveScopes(authorizeRequester fosite.AuthorizeRequester) {
 	}
 }
 
-// ApplyIdentityTransformations applies an identity transformation pipeline to an upstream identity to transform
+// applyIdentityTransformations applies an identity transformation pipeline to an upstream identity to transform
 // or potentially reject the identity.
-func ApplyIdentityTransformations(
+func applyIdentityTransformations(
 	ctx context.Context,
 	transforms *idtransform.TransformationPipeline,
 	username string,

internal/federationdomain/downstreamsession/downstream_session_test.go

@@ -82,7 +82,7 @@ func TestApplyIdentityTransformations(t *testing.T) {
 				pipeline.AppendTransformation(compiledTransform)
 			}
 
-			gotUsername, gotGroups, err := ApplyIdentityTransformations(context.Background(), pipeline, tt.username, tt.groups)
+			gotUsername, gotGroups, err := applyIdentityTransformations(context.Background(), pipeline, tt.username, tt.groups)
 			if tt.wantErr != "" {
 				require.EqualError(t, err, tt.wantErr)
 				require.Empty(t, gotUsername)

internal/federationdomain/endpoints/auth/auth_handler.go

@@ -209,20 +209,15 @@ func (h *authorizeHandler) authorizeWithoutBrowser(
 		return err
 	}
 
-	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{
+	session, err := downstreamsession.NewPinnipedSession(r.Context(), idp, &downstreamsession.SessionConfig{
 		UpstreamIdentity:    identity,
 		UpstreamLoginExtras: loginExtras,
-		Username:            username,
-		Groups:              groups,
 		ClientID:            authorizeRequester.GetClient().GetID(),
 		GrantedScopes:       authorizeRequester.GetGrantedScopes(),
 	})
+	if err != nil {
+		return fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error())
+	}
 
 	oidc.PerformAuthcodeRedirect(r, w, oauthHelper, authorizeRequester, session, true)
 

internal/federationdomain/endpoints/callback/callback_handler.go

@@ -57,26 +57,20 @@ 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, loginExtras, err := idp.HandleCallback(r.Context(), authcode(r), state.PKCECode, state.Nonce, redirectURI)
+		identity, loginExtras, err := idp.LoginFromCallback(r.Context(), authcode(r), state.PKCECode, state.Nonce, redirectURI)
 		if err != nil {
 			return err
 		}
 
-		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{
+		session, err := downstreamsession.NewPinnipedSession(r.Context(), idp, &downstreamsession.SessionConfig{
 			UpstreamIdentity:    identity,
 			UpstreamLoginExtras: loginExtras,
-			Username:            username,
-			Groups:              groups,
 			ClientID:            authorizeRequester.GetClient().GetID(),
 			GrantedScopes:       authorizeRequester.GetGrantedScopes(),
 		})
+		if err != nil {
+			return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err)
+		}
 
 		authorizeResponder, err := oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, session)
 		if err != nil {

internal/federationdomain/endpoints/login/post_login_handler.go

@@ -88,23 +88,18 @@ func NewPostHandler(issuerURL string, upstreamIDPs federationdomainproviders.Fed
 			}
 		}
 
-		username, groups, err := downstreamsession.ApplyIdentityTransformations(r.Context(),
-			idp.GetTransforms(), identity.UpstreamUsername, identity.UpstreamGroups)
+		session, err := downstreamsession.NewPinnipedSession(r.Context(), idp, &downstreamsession.SessionConfig{
+			UpstreamIdentity:    identity,
+			UpstreamLoginExtras: loginExtras,
+			ClientID:            authorizeRequester.GetClient().GetID(),
+			GrantedScopes:       authorizeRequester.GetGrantedScopes(),
+		})
 		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)
 
 		return nil

internal/federationdomain/endpoints/token/token_handler.go

@@ -226,7 +226,7 @@ func validateSessionHasUsername(session *psession.PinnipedSession) error {
 	return nil
 }
 
-// applyIdentityTransformationsDuringRefresh is similar to downstreamsession.ApplyIdentityTransformations
+// 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,

internal/federationdomain/resolvedprovider/resolved_provider.go

@@ -83,8 +83,8 @@ type RefreshedIdentity struct {
 // 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.
+// upstream authorization request does not allow PKCE, then implementations of
+// FederationDomainResolvedIdentityProvider.UpstreamAuthorizeRedirectURL may choose to ignore that struct field.
 type UpstreamAuthorizeRequestState struct {
 	EncodedStateParam string
 	PKCE              pkce.Code
@@ -131,6 +131,13 @@ type FederationDomainResolvedIdentityProvider interface {
 	// the downstream browser-based authorization flow. Returned errors should be of type fosite.RFC6749Error.
 	UpstreamAuthorizeRedirectURL(state *UpstreamAuthorizeRequestState, downstreamIssuerURL string) (string, error)
 
+	// LoginFromCallback 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. If the upstream does not allow PKCE, then
+	// the pkce parameter can be ignored.
+	// Returned errors should be from the httperr package.
+	LoginFromCallback(ctx context.Context, authCode string, pkce pkce.Code, nonce nonce.Nonce, redirectURI string) (*Identity, *IdentityLoginExtras, error)
+
 	// Login performs auth using a username and password that was submitted by the client, without a web browser.
 	// This function should authenticate the user with the upstream identity provider, extract their upstream
 	// identity, and transform it into their downstream identity.
@@ -139,12 +146,6 @@ type FederationDomainResolvedIdentityProvider interface {
 	// Returned errors should be of type fosite.RFC6749Error.
 	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, *IdentityLoginExtras, error)
-
 	// UpstreamRefresh performs a refresh with the upstream provider.
 	// 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.

internal/federationdomain/resolvedprovider/resolvedldap/resolved_ldap_provider.go

@@ -171,7 +171,7 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) Login(
 		nil
 }
 
-func (p *FederationDomainResolvedLDAPIdentityProvider) HandleCallback(
+func (p *FederationDomainResolvedLDAPIdentityProvider) LoginFromCallback(
 	_ctx context.Context,
 	_authCode string,
 	_pkce pkce.Code,
@@ -179,7 +179,7 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) HandleCallback(
 	_redirectURI string,
 ) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) {
 	return nil, nil, httperr.New(http.StatusInternalServerError,
-		"HandleCallback() is not supported for LDAP and ActiveDirectory types of identity provider")
+		"LoginFromCallback() is not supported for LDAP and ActiveDirectory types of identity provider")
 }
 
 func (p *FederationDomainResolvedLDAPIdentityProvider) UpstreamRefresh(

internal/federationdomain/resolvedprovider/resolvedoidc/resolved_oidc_provider.go

@@ -177,7 +177,7 @@ func (p *FederationDomainResolvedOIDCIdentityProvider) Login(
 		nil
 }
 
-func (p *FederationDomainResolvedOIDCIdentityProvider) HandleCallback(
+func (p *FederationDomainResolvedOIDCIdentityProvider) LoginFromCallback(
 	ctx context.Context,
 	authCode string,
 	pkce pkce.Code,