More refactoring of auth handler and related refactor of upstreamldap

- continued refactoring the auth handler to share more code between
  the two supported browserless flows: OIDC and LDAP/AD
- the upstreamldap package should not know about the concept of
  OIDC granted scopes, so refactored it to be a skipGroups bool

internal/authenticators/authenticators.go

@@ -1,4 +1,4 @@
-// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved.
+// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package authenticators contains authenticator interfaces.
@@ -31,7 +31,7 @@ import (
 // See k8s.io/apiserver/pkg/authentication/authenticator/interfaces.go for the token authenticator
 // interface, as well as the Response type.
 type UserAuthenticator interface {
-	AuthenticateUser(ctx context.Context, username, password string, grantedScopes []string) (*Response, bool, error)
+	AuthenticateUser(ctx context.Context, username, password string, skipGroups bool) (*Response, bool, error)
 }
 
 type Response struct {

internal/federationdomain/downstreamsession/downstream_session.go

@@ -1,4 +1,4 @@
-// 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 downstreamsession provides some shared helpers for creating downstream OIDC sessions.
@@ -45,41 +45,46 @@ const (
 	idpNameSubjectQueryParam = "idpName"
 )
 
+type Identity struct {
+	// Note that the username is stored in SessionData.Username.
+	SessionData      *psession.CustomSessionData
+	Groups           []string
+	Subject          string
+	AdditionalClaims map[string]interface{}
+}
+
 // MakeDownstreamSession creates a downstream OIDC session.
 func MakeDownstreamSession(
-	subject string,
-	username string,
-	groups []string,
+	identity *Identity,
 	grantedScopes []string,
 	clientID string,
-	custom *psession.CustomSessionData,
-	additionalClaims map[string]interface{},
 ) *psession.PinnipedSession {
 	now := time.Now().UTC()
 	openIDSession := &psession.PinnipedSession{
 		Fosite: &openid.DefaultSession{
 			Claims: &jwt.IDTokenClaims{
-				Subject:     subject,
+				Subject:     identity.Subject,
 				RequestedAt: now,
 				AuthTime:    now,
 			},
 		},
-		Custom: custom,
-	}
-	if groups == nil {
-		groups = []string{}
+		Custom: identity.SessionData,
 	}
 
 	extras := map[string]interface{}{}
 	extras[oidcapi.IDTokenClaimAuthorizedParty] = clientID
 	if slices.Contains(grantedScopes, oidcapi.ScopeUsername) {
-		extras[oidcapi.IDTokenClaimUsername] = username
+		extras[oidcapi.IDTokenClaimUsername] = identity.SessionData.Username
 	}
 	if slices.Contains(grantedScopes, oidcapi.ScopeGroups) {
+		groups := identity.Groups
+		if groups == nil {
+			groups = []string{}
+		}
 		extras[oidcapi.IDTokenClaimGroups] = groups
 	}
-	if len(additionalClaims) > 0 {
-		extras[oidcapi.IDTokenClaimAdditionalClaims] = additionalClaims
+	if len(identity.AdditionalClaims) > 0 {
+		extras[oidcapi.IDTokenClaimAdditionalClaims] = identity.AdditionalClaims
 	}
 	openIDSession.IDTokenClaims().Extra = extras
 

internal/federationdomain/endpoints/auth/auth_handler.go

@@ -5,6 +5,7 @@
 package auth
 
 import (
+	"context"
 	"fmt"
 	"net/http"
 	"net/url"
@@ -14,6 +15,7 @@ import (
 	"github.com/ory/fosite/handler/openid"
 	"github.com/ory/fosite/token/jwt"
 	"golang.org/x/oauth2"
+	"k8s.io/utils/strings/slices"
 
 	oidcapi "go.pinniped.dev/generated/latest/apis/supervisor/oidc"
 	"go.pinniped.dev/internal/federationdomain/csrftoken"
@@ -189,9 +191,9 @@ func (h *authorizeHandler) authorize(
 	downstreamsession.AutoApproveScopes(authorizeRequester)
 
 	if requestedBrowserlessFlow {
-		err = h.authorizeWithoutBrowser(w, r, oauthHelper, authorizeRequester, oidcUpstream, ldapUpstream)
+		err = h.authorizeWithoutBrowser(r, w, oauthHelper, authorizeRequester, oidcUpstream, ldapUpstream)
 	} else {
-		err = h.authorizeWithBrowser(w, r, oauthHelper, authorizeRequester, oidcUpstream, ldapUpstream)
+		err = h.authorizeWithBrowser(r, w, oauthHelper, authorizeRequester, oidcUpstream, ldapUpstream)
 	}
 	if err != nil {
 		oidc.WriteAuthorizeError(r, w, oauthHelper, authorizeRequester, err, requestedBrowserlessFlow)
@@ -199,8 +201,8 @@ func (h *authorizeHandler) authorize(
 }
 
 func (h *authorizeHandler) authorizeWithoutBrowser(
-	w http.ResponseWriter,
 	r *http.Request,
+	w http.ResponseWriter,
 	oauthHelper fosite.OAuth2Provider,
 	authorizeRequester fosite.AuthorizeRequester,
 	oidcUpstream *resolvedprovider.FederationDomainResolvedOIDCIdentityProvider,
@@ -215,11 +217,10 @@ func (h *authorizeHandler) authorizeWithoutBrowser(
 		return err
 	}
 
+	var identity *downstreamsession.Identity
 	switch {
 	case oidcUpstream != nil:
-		return handleAuthRequestForOIDCUpstreamPasswordGrant(r, w,
-			oauthHelper,
-			authorizeRequester,
+		identity, err = handleAuthRequestForOIDCUpstreamPasswordGrant(r.Context(),
 			oidcUpstream.Provider,
 			oidcUpstream.Transforms,
 			oidcUpstream.DisplayName,
@@ -227,25 +228,36 @@ func (h *authorizeHandler) authorizeWithoutBrowser(
 			submittedPassword,
 		)
 	case ldapUpstream != nil:
-		return handleAuthRequestForLDAPUpstreamCLIFlow(r, w,
-			oauthHelper,
-			authorizeRequester,
+		identity, err = handleAuthRequestForLDAPUpstreamCLIFlow(r.Context(),
 			ldapUpstream.Provider,
-			ldapUpstream.SessionProviderType,
 			ldapUpstream.Transforms,
 			ldapUpstream.DisplayName,
 			submittedUsername,
 			submittedPassword,
+			ldapUpstream.SessionProviderType,
+			!slices.Contains(authorizeRequester.GetGrantedScopes(), oidcapi.ScopeGroups),
 		)
 	default:
 		// It should not actually be possible to reach this case.
 		return fosite.ErrServerError.WithHint("Huh? Unknown upstream IDP type.")
 	}
+
+	if err != nil {
+		return err
+	}
+
+	session := downstreamsession.MakeDownstreamSession(
+		identity, authorizeRequester.GetGrantedScopes(), authorizeRequester.GetClient().GetID(),
+	)
+
+	oidc.PerformAuthcodeRedirect(r, w, oauthHelper, authorizeRequester, session, true)
+
+	return nil
 }
 
 func (h *authorizeHandler) authorizeWithBrowser(
-	w http.ResponseWriter,
 	r *http.Request,
+	w http.ResponseWriter,
 	oauthHelper fosite.OAuth2Provider,
 	authorizeRequester fosite.AuthorizeRequester,
 	oidcUpstream *resolvedprovider.FederationDomainResolvedOIDCIdentityProvider,
@@ -295,41 +307,40 @@ func shouldShowIDPChooser(
 }
 
 func handleAuthRequestForLDAPUpstreamCLIFlow(
-	r *http.Request,
-	w http.ResponseWriter,
-	oauthHelper fosite.OAuth2Provider,
-	authorizeRequester fosite.AuthorizeRequester,
+	ctx context.Context,
 	ldapUpstream upstreamprovider.UpstreamLDAPIdentityProviderI,
-	idpType psession.ProviderType,
 	identityTransforms *idtransform.TransformationPipeline,
 	idpDisplayName string,
 	submittedUsername string,
 	submittedPassword string,
-) error {
-	authenticateResponse, authenticated, err := ldapUpstream.AuthenticateUser(r.Context(), submittedUsername, submittedPassword, authorizeRequester.GetGrantedScopes())
+	idpType psession.ProviderType,
+	skipGroups bool,
+) (*downstreamsession.Identity, error) {
+	authenticateResponse, authenticated, err := ldapUpstream.AuthenticateUser(ctx, submittedUsername, submittedPassword, skipGroups)
 	if err != nil {
 		plog.WarningErr("unexpected error during upstream LDAP authentication", err, "upstreamName", ldapUpstream.GetName())
-		return errUnexpectedUpstreamError.WithWrap(err)
+		return nil, errUnexpectedUpstreamError.WithWrap(err)
 	}
 	if !authenticated {
-		return fosite.ErrAccessDenied.WithHint("Username/password not accepted by LDAP provider.")
+		return nil, fosite.ErrAccessDenied.WithHint("Username/password not accepted by LDAP provider.")
 	}
 
 	subject := downstreamsession.DownstreamSubjectFromUpstreamLDAP(ldapUpstream, authenticateResponse, idpDisplayName)
 	upstreamUsername := authenticateResponse.User.GetName()
 	upstreamGroups := authenticateResponse.User.GetGroups()
 
-	username, groups, err := downstreamsession.ApplyIdentityTransformations(r.Context(), identityTransforms, upstreamUsername, upstreamGroups)
+	username, groups, err := downstreamsession.ApplyIdentityTransformations(ctx, identityTransforms, upstreamUsername, upstreamGroups)
 	if err != nil {
-		return fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error())
+		return nil, fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error())
 	}
 
 	customSessionData := downstreamsession.MakeDownstreamLDAPOrADCustomSessionData(ldapUpstream, idpType, authenticateResponse, username, upstreamUsername, upstreamGroups)
-	openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups,
-		authorizeRequester.GetGrantedScopes(), authorizeRequester.GetClient().GetID(), customSessionData, map[string]interface{}{})
-	oidc.PerformAuthcodeRedirect(r, w, oauthHelper, authorizeRequester, openIDSession, true)
 
-	return nil
+	return &downstreamsession.Identity{
+		SessionData: customSessionData,
+		Groups:      groups,
+		Subject:     subject,
+	}, nil
 }
 
 func handleAuthRequestForLDAPUpstreamBrowserFlow(
@@ -370,23 +381,20 @@ func handleAuthRequestForLDAPUpstreamBrowserFlow(
 }
 
 func handleAuthRequestForOIDCUpstreamPasswordGrant(
-	r *http.Request,
-	w http.ResponseWriter,
-	oauthHelper fosite.OAuth2Provider,
-	authorizeRequester fosite.AuthorizeRequester,
+	ctx context.Context,
 	oidcUpstream upstreamprovider.UpstreamOIDCIdentityProviderI,
 	identityTransforms *idtransform.TransformationPipeline,
 	idpDisplayName string,
 	submittedUsername string,
 	submittedPassword string,
-) error {
+) (*downstreamsession.Identity, error) {
 	if !oidcUpstream.AllowsPasswordGrant() {
 		// Return a user-friendly error for this case which is entirely within our control.
-		return fosite.ErrAccessDenied.WithHint(
+		return nil, fosite.ErrAccessDenied.WithHint(
 			"Resource owner password credentials grant is not allowed for this upstream provider according to its configuration.")
 	}
 
-	token, err := oidcUpstream.PasswordCredentialsGrantAndValidateTokens(r.Context(), submittedUsername, submittedPassword)
+	token, err := oidcUpstream.PasswordCredentialsGrantAndValidateTokens(ctx, submittedUsername, submittedPassword)
 	if err != nil {
 		// Upstream password grant errors can be generic errors (e.g. a network failure) or can be oauth2.RetrieveError errors
 		// which represent the http response from the upstream server. These could be a 5XX or some other unexpected error,
@@ -395,7 +403,7 @@ func handleAuthRequestForOIDCUpstreamPasswordGrant(
 		// 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 fosite.ErrAccessDenied.WithDebug(err.Error()) // WithDebug hides the error from the client
+		return nil, fosite.ErrAccessDenied.WithDebug(err.Error()) // WithDebug hides the error from the client
 	}
 
 	subject, upstreamUsername, upstreamGroups, err := downstreamsession.GetDownstreamIdentityFromUpstreamIDToken(
@@ -403,27 +411,27 @@ func handleAuthRequestForOIDCUpstreamPasswordGrant(
 	)
 	if err != nil {
 		// Return a user-friendly error for this case which is entirely within our control.
-		return fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error())
+		return nil, fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error())
 	}
 
-	username, groups, err := downstreamsession.ApplyIdentityTransformations(r.Context(), identityTransforms, upstreamUsername, upstreamGroups)
+	username, groups, err := downstreamsession.ApplyIdentityTransformations(ctx, identityTransforms, upstreamUsername, upstreamGroups)
 	if err != nil {
-		return fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error())
+		return nil, fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error())
 	}
 
 	additionalClaims := downstreamsession.MapAdditionalClaimsFromUpstreamIDToken(oidcUpstream, token.IDToken.Claims)
 
 	customSessionData, err := downstreamsession.MakeDownstreamOIDCCustomSessionData(oidcUpstream, token, username, upstreamUsername, upstreamGroups)
 	if err != nil {
-		return fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error())
+		return nil, fosite.ErrAccessDenied.WithHintf("Reason: %s.", err.Error())
 	}
 
-	openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups,
-		authorizeRequester.GetGrantedScopes(), authorizeRequester.GetClient().GetID(), customSessionData, additionalClaims)
-
-	oidc.PerformAuthcodeRedirect(r, w, oauthHelper, authorizeRequester, openIDSession, true)
-
-	return nil
+	return &downstreamsession.Identity{
+		SessionData:      customSessionData,
+		Groups:           groups,
+		Subject:          subject,
+		AdditionalClaims: additionalClaims,
+	}, nil
 }
 
 func handleAuthRequestForOIDCUpstreamBrowserFlow(
@@ -615,7 +623,7 @@ func handleBrowserFlowAuthRequest(
 		return nil, fosite.ErrServerError.WithHint("Error encoding upstream state param.").WithWrap(err)
 	}
 
-	promptParam := r.Form.Get(promptParamName)
+	promptParam := authorizeRequester.GetRequestForm().Get(promptParamName)
 	if promptParam == promptParamNone && oidc.ScopeWasRequested(authorizeRequester, oidcapi.ScopeOpenID) {
 		return nil, fosite.ErrLoginRequired
 	}

internal/federationdomain/endpoints/callback/callback_handler.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package callback provides a handler for the OIDC callback endpoint.
@@ -93,10 +93,18 @@ func NewHandler(
 			return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err)
 		}
 
-		openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups,
-			authorizeRequester.GetGrantedScopes(), authorizeRequester.GetClient().GetID(), customSessionData, additionalClaims)
+		session := downstreamsession.MakeDownstreamSession(
+			&downstreamsession.Identity{
+				SessionData:      customSessionData,
+				Groups:           groups,
+				Subject:          subject,
+				AdditionalClaims: additionalClaims,
+			},
+			authorizeRequester.GetGrantedScopes(),
+			authorizeRequester.GetClient().GetID(),
+		)
 
-		authorizeResponder, err := oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, openIDSession)
+		authorizeResponder, err := oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, session)
 		if err != nil {
 			plog.WarningErr("error while generating and saving authcode", err,
 				"upstreamName", upstreamIDPConfig.GetName(), "fositeErr", oidc.FositeErrorForLog(err))

internal/federationdomain/endpoints/login/post_login_handler.go

@@ -1,4 +1,4 @@
-// Copyright 2022-2023 the Pinniped contributors. All Rights Reserved.
+// Copyright 2022-2024 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package login
@@ -8,7 +8,9 @@ import (
 	"net/url"
 
 	"github.com/ory/fosite"
+	"k8s.io/utils/strings/slices"
 
+	oidcapi "go.pinniped.dev/generated/latest/apis/supervisor/oidc"
 	"go.pinniped.dev/internal/federationdomain/downstreamsession"
 	"go.pinniped.dev/internal/federationdomain/federationdomainproviders"
 	"go.pinniped.dev/internal/federationdomain/oidc"
@@ -63,8 +65,10 @@ func NewPostHandler(issuerURL string, upstreamIDPs federationdomainproviders.Fed
 		}
 
 		// Attempt to authenticate the user with the upstream IDP.
-		authenticateResponse, authenticated, err := ldapUpstream.Provider.AuthenticateUser(
-			r.Context(), submittedUsername, submittedPassword, authorizeRequester.GetGrantedScopes(),
+		authenticateResponse, authenticated, err := ldapUpstream.Provider.AuthenticateUser(r.Context(),
+			submittedUsername,
+			submittedPassword,
+			!slices.Contains(authorizeRequester.GetGrantedScopes(), oidcapi.ScopeGroups),
 		)
 		if err != nil {
 			plog.WarningErr("unexpected error during upstream LDAP authentication", err, "upstreamName", ldapUpstream.Provider.GetName())
@@ -100,9 +104,18 @@ func NewPostHandler(issuerURL string, upstreamIDPs federationdomainproviders.Fed
 
 		customSessionData := downstreamsession.MakeDownstreamLDAPOrADCustomSessionData(
 			ldapUpstream.Provider, ldapUpstream.SessionProviderType, authenticateResponse, username, upstreamUsername, upstreamGroups)
-		openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups,
-			authorizeRequester.GetGrantedScopes(), authorizeRequester.GetClient().GetID(), customSessionData, map[string]interface{}{})
-		oidc.PerformAuthcodeRedirect(r, w, oauthHelper, authorizeRequester, openIDSession, false)
+
+		session := downstreamsession.MakeDownstreamSession(
+			&downstreamsession.Identity{
+				SessionData: customSessionData,
+				Groups:      groups,
+				Subject:     subject,
+			},
+			authorizeRequester.GetGrantedScopes(),
+			authorizeRequester.GetClient().GetID(),
+		)
+
+		oidc.PerformAuthcodeRedirect(r, w, oauthHelper, authorizeRequester, session, false)
 
 		return nil
 	}

internal/federationdomain/endpoints/token/token_handler.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package token provides a handler for the OIDC token endpoint.
@@ -111,14 +111,15 @@ func upstreamRefresh(ctx context.Context, accessRequest fosite.AccessRequester,
 		return errorsx.WithStack(errMissingUpstreamSessionInternalError())
 	}
 
-	grantedScopes := accessRequest.GetGrantedScopes()
+	groupsScopeNotGranted := !slices.Contains(accessRequest.GetGrantedScopes(), oidcapi.ScopeGroups)
+
 	clientID := accessRequest.GetClient().GetID()
 
 	switch customSessionData.ProviderType {
 	case psession.ProviderTypeOIDC:
-		return upstreamOIDCRefresh(ctx, idpLister, session, grantedScopes, clientID)
+		return upstreamOIDCRefresh(ctx, idpLister, session, groupsScopeNotGranted, clientID)
 	case psession.ProviderTypeLDAP, psession.ProviderTypeActiveDirectory:
-		return upstreamLDAPRefresh(ctx, idpLister, session, grantedScopes, clientID)
+		return upstreamLDAPRefresh(ctx, idpLister, session, groupsScopeNotGranted, clientID)
 	default:
 		return errorsx.WithStack(errMissingUpstreamSessionInternalError())
 	}
@@ -129,11 +130,10 @@ func upstreamOIDCRefresh(
 	ctx context.Context,
 	idpLister federationdomainproviders.FederationDomainIdentityProvidersListerI,
 	session *psession.PinnipedSession,
-	grantedScopes []string,
+	skipGroups bool,
 	clientID string,
 ) error {
 	s := session.Custom
-	groupsScopeGranted := slices.Contains(grantedScopes, oidcapi.ScopeGroups)
 
 	if s.OIDC == nil {
 		return errorsx.WithStack(errMissingUpstreamSessionInternalError())
@@ -194,7 +194,7 @@ func upstreamOIDCRefresh(
 	}
 
 	var refreshedUntransformedGroups []string
-	if groupsScopeGranted {
+	if !skipGroups {
 		// 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
@@ -236,7 +236,7 @@ func upstreamOIDCRefresh(
 		return err
 	}
 	var oldTransformedGroups []string
-	if groupsScopeGranted {
+	if !skipGroups {
 		oldTransformedGroups, err = getDownstreamGroupsFromPinnipedSession(session)
 		if err != nil {
 			return err
@@ -255,7 +255,7 @@ func upstreamOIDCRefresh(
 		return err
 	}
 
-	if groupsScopeGranted {
+	if !skipGroups {
 		warnIfGroupsChanged(ctx, oldTransformedGroups, transformationResult.Groups, transformationResult.Username, clientID)
 		// Replace the old value for the downstream groups in the user's session with the new value.
 		session.Fosite.Claims.Extra[oidcapi.IDTokenClaimGroups] = transformationResult.Groups
@@ -343,18 +343,17 @@ func upstreamLDAPRefresh(
 	ctx context.Context,
 	idpLister federationdomainproviders.FederationDomainIdentityProvidersListerI,
 	session *psession.PinnipedSession,
-	grantedScopes []string,
+	skipGroups bool,
 	clientID string,
 ) error {
 	s := session.Custom
-	groupsScopeGranted := slices.Contains(grantedScopes, oidcapi.ScopeGroups)
 
 	oldTransformedUsername, err := getDownstreamUsernameFromPinnipedSession(session)
 	if err != nil {
 		return err
 	}
 	var oldTransformedGroups []string
-	if groupsScopeGranted {
+	if !skipGroups {
 		oldTransformedGroups, err = getDownstreamGroupsFromPinnipedSession(session)
 		if err != nil {
 			return err
@@ -393,7 +392,7 @@ func upstreamLDAPRefresh(
 		DN:                   dn,
 		Groups:               oldUntransformedGroups,
 		AdditionalAttributes: additionalAttributes,
-		GrantedScopes:        grantedScopes,
+		SkipGroups:           skipGroups,
 	}, p.DisplayName)
 	if err != nil {
 		return errUpstreamRefreshError().WithHint(
@@ -413,7 +412,7 @@ func upstreamLDAPRefresh(
 		return err
 	}
 
-	if groupsScopeGranted {
+	if !skipGroups {
 		warnIfGroupsChanged(ctx, oldTransformedGroups, transformationResult.Groups, transformationResult.Username, clientID)
 		// Replace the old value for the downstream groups in the user's session with the new value.
 		session.Fosite.Claims.Extra[oidcapi.IDTokenClaimGroups] = transformationResult.Groups

internal/federationdomain/upstreamprovider/upsteam_provider.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package upstreamprovider
@@ -25,14 +25,17 @@ const (
 )
 
 // RefreshAttributes contains information about the user from the original login request
-// and previous refreshes.
+// and previous refreshes to be used during an LDAP session refresh.
 type RefreshAttributes struct {
 	Username             string
 	Subject              string
 	DN                   string
 	Groups               []string
 	AdditionalAttributes map[string]string
-	GrantedScopes        []string
+	// Skip group search for this particular session refresh.
+	// E.g. This could be set to true when the user was not granted the downstream groups scope.
+	// There is no reason to spend the cost of an LDAP group search unless we are going to use the results.
+	SkipGroups bool
 }
 
 type UpstreamOIDCIdentityProviderI interface {

internal/testutil/oidctestutil/oidctestutil.go

@@ -212,7 +212,7 @@ func (u *TestUpstreamLDAPIdentityProvider) GetName() string {
 	return u.Name
 }
 
-func (u *TestUpstreamLDAPIdentityProvider) AuthenticateUser(ctx context.Context, username, password string, _grantedScopes []string) (*authenticators.Response, bool, error) {
+func (u *TestUpstreamLDAPIdentityProvider) AuthenticateUser(ctx context.Context, username, password string, _skipGroups bool) (*authenticators.Response, bool, error) {
 	return u.AuthenticateFunc(ctx, username, password)
 }
 

internal/upstreamldap/upstreamldap.go

@@ -1,4 +1,4 @@
-// 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 upstreamldap implements an abstraction of upstream LDAP IDP interactions.
@@ -20,10 +20,8 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apiserver/pkg/authentication/user"
-	"k8s.io/utils/strings/slices"
 	"k8s.io/utils/trace"
 
-	oidcapi "go.pinniped.dev/generated/latest/apis/supervisor/oidc"
 	"go.pinniped.dev/internal/authenticators"
 	"go.pinniped.dev/internal/crypto/ptls"
 	"go.pinniped.dev/internal/endpointaddr"
@@ -248,11 +246,12 @@ func (p *Provider) PerformRefresh(ctx context.Context, storedRefreshAttributes u
 		}
 	}
 
+	// If we were configured to always skip group refresh for all users and all sessions, then skip it.
 	if p.c.GroupSearch.SkipGroupRefresh {
 		return storedRefreshAttributes.Groups, nil
 	}
-	// if we were not granted the groups scope, we should not search for groups or return any.
-	if !slices.Contains(storedRefreshAttributes.GrantedScopes, oidcapi.ScopeGroups) {
+	// If we were asked to skip group search for this particular session, then do not search for groups or return any.
+	if storedRefreshAttributes.SkipGroups {
 		return nil, nil
 	}
 
@@ -423,23 +422,23 @@ func (p *Provider) TestConnection(ctx context.Context) error {
 // authentication for a given end user's username. It runs the same logic as AuthenticateUser except it does
 // not bind as that user, so it does not test their password. It returns the same values that a real call to
 // AuthenticateUser with the correct password would return.
-func (p *Provider) DryRunAuthenticateUser(ctx context.Context, username string, grantedScopes []string) (*authenticators.Response, bool, error) {
+func (p *Provider) DryRunAuthenticateUser(ctx context.Context, username string, skipGroups bool) (*authenticators.Response, bool, error) {
 	endUserBindFunc := func(conn Conn, foundUserDN string) error {
 		// Act as if the end user bind always succeeds.
 		return nil
 	}
-	return p.authenticateUserImpl(ctx, username, grantedScopes, endUserBindFunc)
+	return p.authenticateUserImpl(ctx, username, skipGroups, endUserBindFunc)
 }
 
 // AuthenticateUser authenticates an end user and returns their mapped username, groups, and UID. Implements authenticators.UserAuthenticator.
-func (p *Provider) AuthenticateUser(ctx context.Context, username, password string, grantedScopes []string) (*authenticators.Response, bool, error) {
+func (p *Provider) AuthenticateUser(ctx context.Context, username, password string, skipGroups bool) (*authenticators.Response, bool, error) {
 	endUserBindFunc := func(conn Conn, foundUserDN string) error {
 		return conn.Bind(foundUserDN, password)
 	}
-	return p.authenticateUserImpl(ctx, username, grantedScopes, endUserBindFunc)
+	return p.authenticateUserImpl(ctx, username, skipGroups, endUserBindFunc)
 }
 
-func (p *Provider) authenticateUserImpl(ctx context.Context, username string, grantedScopes []string, bindFunc func(conn Conn, foundUserDN string) error) (*authenticators.Response, bool, error) {
+func (p *Provider) authenticateUserImpl(ctx context.Context, username string, skipGroups bool, bindFunc func(conn Conn, foundUserDN string) error) (*authenticators.Response, bool, error) {
 	t := trace.FromContext(ctx).Nest("slow ldap authenticate user attempt", trace.Field{Key: "providerName", Value: p.GetName()})
 	defer t.LogIfLong(500 * time.Millisecond) // to help users debug slow LDAP searches
 
@@ -468,7 +467,7 @@ func (p *Provider) authenticateUserImpl(ctx context.Context, username string, gr
 		return nil, false, fmt.Errorf(`error binding as %q before user search: %w`, p.c.BindUsername, err)
 	}
 
-	response, err := p.searchAndBindUser(conn, username, grantedScopes, bindFunc)
+	response, err := p.searchAndBindUser(conn, username, skipGroups, bindFunc)
 	if err != nil {
 		p.traceAuthFailure(t, err)
 		return nil, false, err
@@ -565,7 +564,7 @@ func (p *Provider) SearchForDefaultNamingContext(ctx context.Context) (string, e
 	return searchBase, nil
 }
 
-func (p *Provider) searchAndBindUser(conn Conn, username string, grantedScopes []string, bindFunc func(conn Conn, foundUserDN string) error) (*authenticators.Response, error) {
+func (p *Provider) searchAndBindUser(conn Conn, username string, skipGroups bool, bindFunc func(conn Conn, foundUserDN string) error) (*authenticators.Response, error) {
 	searchResult, err := conn.Search(p.userSearchRequest(username))
 	if err != nil {
 		plog.All(`error searching for user`,
@@ -612,7 +611,7 @@ func (p *Provider) searchAndBindUser(conn Conn, username string, grantedScopes [
 	}
 
 	var mappedGroupNames []string
-	if slices.Contains(grantedScopes, oidcapi.ScopeGroups) {
+	if !skipGroups {
 		var groupSearchUserAttributeForFilterValue string
 		if p.useGroupSearchUserAttributeForFilter() {
 			groupSearchUserAttributeForFilterValue, err = p.getSearchResultAttributeValue(p.c.GroupSearch.UserAttributeForFilter, userEntry, username)

internal/upstreamldap/upstreamldap_test.go

@@ -1,4 +1,4 @@
-// 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 upstreamldap
@@ -178,7 +178,7 @@ func TestEndUserAuthentication(t *testing.T) {
 		name                       string
 		username                   string
 		password                   string
-		grantedScopes              []string
+		skipGroups                 bool
 		providerConfig             *ProviderConfig
 		searchMocks                func(conn *mockldapconn.MockConn)
 		bindEndUserMocks           func(conn *mockldapconn.MockConn)
@@ -292,10 +292,10 @@ func TestEndUserAuthentication(t *testing.T) {
 			}),
 		},
 		{
-			name:           "when groups scope isn't granted, don't do group search",
+			name:           "when groups are requested to be skipped, don't do group search",
 			username:       testUpstreamUsername,
 			password:       testUpstreamPassword,
-			grantedScopes:  []string{},
+			skipGroups:     true,
 			providerConfig: providerConfig(nil),
 			searchMocks: func(conn *mockldapconn.MockConn) {
 				conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1)
@@ -755,10 +755,10 @@ func TestEndUserAuthentication(t *testing.T) {
 			wantAuthResponse: expectedAuthResponse(nil),
 		},
 		{
-			name:          "when the UserAttributeForFilter is set to something other than dn but groups scope is not granted so skips validating UserAttributeForFilter attribute value",
-			username:      testUpstreamUsername,
-			password:      testUpstreamPassword,
-			grantedScopes: []string{}, // no groups scope
+			name:       "when the UserAttributeForFilter is set to something other than dn but groups are to be skipped, so skips validating UserAttributeForFilter attribute value",
+			username:   testUpstreamUsername,
+			password:   testUpstreamPassword,
+			skipGroups: true,
 			providerConfig: providerConfig(func(p *ProviderConfig) {
 				p.GroupSearch.UserAttributeForFilter = "someUserAttrName"
 			}),
@@ -1421,11 +1421,7 @@ func TestEndUserAuthentication(t *testing.T) {
 
 			ldapProvider := New(*tt.providerConfig)
 
-			if tt.grantedScopes == nil {
-				tt.grantedScopes = []string{"groups"}
-			}
-
-			authResponse, authenticated, err := ldapProvider.AuthenticateUser(context.Background(), tt.username, tt.password, tt.grantedScopes)
+			authResponse, authenticated, err := ldapProvider.AuthenticateUser(context.Background(), tt.username, tt.password, tt.skipGroups)
 			require.Equal(t, !tt.wantToSkipDial, dialWasAttempted)
 			switch {
 			case tt.wantError != nil:
@@ -1457,7 +1453,7 @@ func TestEndUserAuthentication(t *testing.T) {
 			}
 			// Skip tt.bindEndUserMocks since DryRunAuthenticateUser() never binds as the end user.
 
-			authResponse, authenticated, err = ldapProvider.DryRunAuthenticateUser(context.Background(), tt.username, tt.grantedScopes)
+			authResponse, authenticated, err = ldapProvider.DryRunAuthenticateUser(context.Background(), tt.username, tt.skipGroups)
 			require.Equal(t, !tt.wantToSkipDial, dialWasAttempted)
 			switch {
 			case tt.wantError != nil:
@@ -1589,7 +1585,7 @@ func TestUpstreamRefresh(t *testing.T) {
 	tests := []struct {
 		name           string
 		providerConfig *ProviderConfig
-		grantedScopes  []string
+		skipGroups     bool
 		setupMocks     func(conn *mockldapconn.MockConn)
 		refreshUserDN  string
 		dialError      error
@@ -1726,7 +1722,7 @@ func TestUpstreamRefresh(t *testing.T) {
 			wantGroups: nil, // do not update groups
 		},
 		{
-			name:           "happy path where group search is configured but groups scope isn't included",
+			name:           "happy path where group search is configured but groups search is requested to be skipped",
 			providerConfig: providerConfig(nil),
 			setupMocks: func(conn *mockldapconn.MockConn) {
 				conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1)
@@ -1734,8 +1730,8 @@ func TestUpstreamRefresh(t *testing.T) {
 				// note that group search is not expected
 				conn.EXPECT().Close().Times(1)
 			},
-			grantedScopes: []string{},
-			wantGroups:    nil,
+			skipGroups: true,
+			wantGroups: nil,
 		},
 		{
 			name: "happy path where group search is configured but skipGroupRefresh is set, when the UserAttributeForFilter is set to something other than dn, still skips group refresh, and skips validating UserAttributeForFilter attribute value",
@@ -1755,7 +1751,7 @@ func TestUpstreamRefresh(t *testing.T) {
 			wantGroups: nil, // do not update groups
 		},
 		{
-			name: "happy path where group search is configured but groups scope isn't included, when the UserAttributeForFilter is set to something other than dn, still skips group refresh, and skips validating UserAttributeForFilter attribute value",
+			name: "happy path where group search is configured but groups search is requested to be skipped, when the UserAttributeForFilter is set to something other than dn, still skips group refresh, and skips validating UserAttributeForFilter attribute value",
 			providerConfig: providerConfig(func(p *ProviderConfig) {
 				p.GroupSearch.UserAttributeForFilter = "someUserAttrName"
 			}),
@@ -1768,8 +1764,8 @@ func TestUpstreamRefresh(t *testing.T) {
 				// note that group search is not expected
 				conn.EXPECT().Close().Times(1)
 			},
-			grantedScopes: []string{},
-			wantGroups:    nil,
+			skipGroups: true,
+			wantGroups: nil,
 		},
 		{
 			name: "happy path when the UserAttributeForFilter is set to something other than dn",
@@ -2275,9 +2271,6 @@ func TestUpstreamRefresh(t *testing.T) {
 				tt.refreshUserDN = testUserSearchResultDNValue // default for all tests
 			}
 
-			if tt.grantedScopes == nil {
-				tt.grantedScopes = []string{"groups"}
-			}
 			initialPwdLastSetEncoded := base64.RawURLEncoding.EncodeToString([]byte("132801740800000000"))
 			ldapProvider := New(*tt.providerConfig)
 			subject := fmt.Sprintf(
@@ -2289,7 +2282,7 @@ func TestUpstreamRefresh(t *testing.T) {
 				Subject:              subject,
 				DN:                   tt.refreshUserDN,
 				AdditionalAttributes: map[string]string{pwdLastSetAttribute: initialPwdLastSetEncoded},
-				GrantedScopes:        tt.grantedScopes,
+				SkipGroups:           tt.skipGroups,
 			}, testUpstreamName)
 			if tt.wantErr != "" {
 				require.Error(t, err)

test/integration/ldap_client_test.go

@@ -1,4 +1,4 @@
-// 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 integration
@@ -73,7 +73,7 @@ func TestLDAPSearch_Parallel(t *testing.T) {
 		name                string
 		username            string
 		password            string
-		grantedScopes       []string
+		skipGroups          bool
 		provider            *upstreamldap.Provider
 		wantError           testutil.RequireErrorStringFunc
 		wantAuthResponse    *authenticators.Response
@@ -116,11 +116,11 @@ func TestLDAPSearch_Parallel(t *testing.T) {
 			},
 		},
 		{
-			name:          "groups scope not in granted scopes",
-			username:      "pinny",
-			password:      pinnyPassword,
-			grantedScopes: []string{},
-			provider:      upstreamldap.New(*providerConfig(nil)),
+			name:       "skip groups",
+			username:   "pinny",
+			password:   pinnyPassword,
+			skipGroups: true,
+			provider:   upstreamldap.New(*providerConfig(nil)),
 			wantAuthResponse: &authenticators.Response{
 				User:                   &user.DefaultInfo{Name: "pinny", UID: b64("1000"), Groups: nil},
 				DN:                     "cn=pinny,ou=users,dc=pinniped,dc=dev",
@@ -741,10 +741,7 @@ func TestLDAPSearch_Parallel(t *testing.T) {
 	for _, test := range tests {
 		tt := test
 		t.Run(tt.name, func(t *testing.T) {
-			if tt.grantedScopes == nil {
-				tt.grantedScopes = []string{"groups"}
-			}
-			authResponse, authenticated, err := tt.provider.AuthenticateUser(ctx, tt.username, tt.password, tt.grantedScopes)
+			authResponse, authenticated, err := tt.provider.AuthenticateUser(ctx, tt.username, tt.password, tt.skipGroups)
 
 			switch {
 			case tt.wantError != nil:
@@ -802,7 +799,7 @@ func TestSimultaneousLDAPRequestsOnSingleProvider(t *testing.T) {
 			authUserCtx, authUserCtxCancelFunc := context.WithTimeout(context.Background(), 2*time.Minute)
 			defer authUserCtxCancelFunc()
 
-			authResponse, authenticated, err := provider.AuthenticateUser(authUserCtx, env.SupervisorUpstreamLDAP.TestUserCN, env.SupervisorUpstreamLDAP.TestUserPassword, []string{"groups"})
+			authResponse, authenticated, err := provider.AuthenticateUser(authUserCtx, env.SupervisorUpstreamLDAP.TestUserCN, env.SupervisorUpstreamLDAP.TestUserPassword, false)
 			resultCh <- authUserResult{
 				response:      authResponse,
 				authenticated: authenticated,