Merge pull request #1871 from vmware-tanzu/always_search_groups

Don't skip upstream group memberships when groups scope is not granted

internal/authenticators/authenticators.go

@@ -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, skipGroups bool) (*Response, bool, error)
+	AuthenticateUser(ctx context.Context, username, password string) (*Response, bool, error)
 }
 
 type Response struct {

internal/federationdomain/endpoints/auth/auth_handler.go

@@ -13,7 +13,6 @@ import (
 	"github.com/ory/fosite"
 	"github.com/ory/fosite/handler/openid"
 	"github.com/ory/fosite/token/jwt"
-	"k8s.io/utils/strings/slices"
 
 	oidcapi "go.pinniped.dev/generated/latest/apis/supervisor/oidc"
 	"go.pinniped.dev/internal/federationdomain/csrftoken"
@@ -202,9 +201,7 @@ func (h *authorizeHandler) authorizeWithoutBrowser(
 		return err
 	}
 
-	groupsWillBeIgnored := !slices.Contains(authorizeRequester.GetGrantedScopes(), oidcapi.ScopeGroups)
-
-	identity, loginExtras, err := idp.Login(r.Context(), submittedUsername, submittedPassword, groupsWillBeIgnored)
+	identity, loginExtras, err := idp.Login(r.Context(), submittedUsername, submittedPassword)
 	if err != nil {
 		return err
 	}

internal/federationdomain/endpoints/login/post_login_handler.go

@@ -9,9 +9,7 @@ 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/endpoints/loginurl"
 	"go.pinniped.dev/internal/federationdomain/federationdomainproviders"
@@ -67,10 +65,8 @@ func NewPostHandler(issuerURL string, upstreamIDPs federationdomainproviders.Fed
 			return redirectToLoginPage(r, w, issuerURL, encodedState, loginurl.ShowBadUserPassErr)
 		}
 
-		skipGroups := !slices.Contains(authorizeRequester.GetGrantedScopes(), oidcapi.ScopeGroups)
-
 		// Attempt to authenticate the user with the upstream IDP.
-		identity, loginExtras, err := idp.Login(r.Context(), submittedUsername, submittedPassword, skipGroups)
+		identity, loginExtras, err := idp.Login(r.Context(), submittedUsername, submittedPassword)
 		if err != nil {
 			switch {
 			case errors.Is(err, resolvedldap.ErrUnexpectedUpstreamLDAPError):

internal/federationdomain/endpoints/login/post_login_handler_test.go

@@ -20,6 +20,7 @@ import (
 	configv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/config/v1alpha1"
 	supervisorfake "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/fake"
 	"go.pinniped.dev/internal/authenticators"
+	"go.pinniped.dev/internal/celtransformer"
 	"go.pinniped.dev/internal/federationdomain/endpoints/jwks"
 	"go.pinniped.dev/internal/federationdomain/oidc"
 	"go.pinniped.dev/internal/federationdomain/oidcclientvalidator"
@@ -674,6 +675,44 @@ func TestPostLoginEndpoint(t *testing.T) {
 			wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod,
 			wantDownstreamCustomSessionData:   expectedHappyLDAPUpstreamCustomSession,
 		},
+		{
+			name: "login with dynamic client which is not allowed to request groups still runs identity transformations with upstream groups " +
+				"in case transforms want to reject auth based on groups, even though groups would not be included in final ID token",
+			idps: testidplister.NewUpstreamIDPListerBuilder().
+				WithLDAP(upstreamLDAPIdentityProviderBuilder.
+					WithTransformsForFederationDomain(transformtestutil.NewPipeline(t,
+						[]celtransformer.CELTransformation{
+							&celtransformer.AllowAuthenticationPolicy{
+								Expression:                    `!groups.exists(g, g in ["` + happyLDAPGroups[0] + `"])`, // reject auth for users who belongs to an upstream group
+								RejectedAuthenticationMessage: `users who belong to certain upstream group are not allowed`,
+							},
+						}),
+					).Build()),
+			kubeResources: func(t *testing.T, supervisorClient *supervisorfake.Clientset, kubeClient *fake.Clientset) {
+				oidcClient, secret := testutil.OIDCClientAndStorageSecret(t,
+					"some-namespace", downstreamDynamicClientID, downstreamDynamicClientUID,
+					[]configv1alpha1.GrantType{"authorization_code", "refresh_token"}, // token exchange not allowed (required to exclude groups scope)
+					[]configv1alpha1.Scope{"openid", "offline_access", "username"},    // groups not allowed
+					downstreamRedirectURI, []string{testutil.HashedPassword1AtGoMinCost}, oidcclientvalidator.Validate)
+				require.NoError(t, supervisorClient.Tracker().Add(oidcClient))
+				require.NoError(t, kubeClient.Tracker().Add(secret))
+			},
+			decodedState: modifyHappyLDAPDecodedState(func(data *oidc.UpstreamStateParamData) {
+				data.AuthParams = shallowCopyAndModifyQuery(happyDownstreamRequestParamsQueryForDynamicClient,
+					map[string]string{"scope": "openid username offline_access"},
+				).Encode()
+			}),
+			formParams:      happyUsernamePasswordFormParams,
+			wantStatus:      http.StatusSeeOther,
+			wantContentType: htmlContentType,
+			wantBodyString:  "",
+			wantRedirectLocationString: urlWithQuery(downstreamRedirectURI, map[string]string{
+				"error": "access_denied",
+				// auth was rejected because of the upstream group to which the user belonged, as shown by the configured RejectedAuthenticationMessage appearing here
+				"error_description": "The resource owner or authorization server denied the request. Reason: configured identity policy rejected this authentication: users who belong to certain upstream group are not allowed.",
+				"state":             happyDownstreamState,
+			}),
+		},
 		{
 			name: "happy LDAP when downstream OIDC validations are skipped because the openid scope was not requested",
 			idps: testidplister.NewUpstreamIDPListerBuilder().WithLDAP(upstreamLDAPIdentityProvider),

internal/federationdomain/endpoints/token/token_handler.go

@@ -147,7 +147,7 @@ func upstreamRefresh(
 	oldUntransformedGroups := session.Custom.UpstreamGroups
 	oldTransformedUsername := session.Custom.Username
 
-	previousIdentity := resolvedprovider.Identity{
+	previousIdentity := &resolvedprovider.Identity{
 		UpstreamUsername:       oldUntransformedUsername,
 		UpstreamGroups:         oldUntransformedGroups,
 		DownstreamSubject:      session.Fosite.Claims.Subject,
@@ -155,7 +155,7 @@ func upstreamRefresh(
 	}
 
 	// Perform the upstream refresh.
-	refreshedIdentity, err := idp.UpstreamRefresh(ctx, &previousIdentity, skipGroups)
+	refreshedIdentity, err := idp.UpstreamRefresh(ctx, previousIdentity)
 	if err != nil {
 		return err
 	}

internal/federationdomain/endpoints/token/token_handler_test.go

@@ -42,6 +42,7 @@ import (
 
 	configv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/config/v1alpha1"
 	supervisorfake "go.pinniped.dev/generated/latest/client/supervisor/clientset/versioned/fake"
+	"go.pinniped.dev/internal/celtransformer"
 	"go.pinniped.dev/internal/crud"
 	"go.pinniped.dev/internal/federationdomain/clientregistry"
 	"go.pinniped.dev/internal/federationdomain/endpoints/jwks"
@@ -2907,6 +2908,64 @@ func TestRefreshGrant(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "refresh grant when the upstream refresh when groups scope not requested on original request, when using dynamic client, " +
+				"still runs identity transformations with upstream groups in case transforms want to reject auth based on groups, even though groups would not be included in final ID token",
+			idps: testidplister.NewUpstreamIDPListerBuilder().WithOIDC(
+				upstreamOIDCIdentityProviderBuilder().WithGroupsClaim("my-groups-claim").WithValidatedAndMergedWithUserInfoTokens(&oidctypes.Token{
+					IDToken: &oidctypes.IDToken{
+						Claims: map[string]interface{}{
+							"sub":             goodUpstreamSubject,
+							"my-groups-claim": []string{"new-group1", "new-group2", "new-group3"}, // refreshed claims includes updated groups
+						},
+					},
+				}).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).
+					WithTransformsForFederationDomain(transformtestutil.NewPipeline(t,
+						[]celtransformer.CELTransformation{
+							&celtransformer.AllowAuthenticationPolicy{
+								Expression:                    `!groups.exists(g, g in ["` + "new-group1" + `"])`, // reject auth for users who belongs to an upstream group
+								RejectedAuthenticationMessage: `users who belong to certain upstream group are not allowed`,
+							},
+						}),
+					).Build()),
+			kubeResources: addFullyCapableDynamicClientAndSecretToKubeResources,
+			authcodeExchange: authcodeExchangeInputs{
+				modifyAuthRequest: func(r *http.Request) {
+					addDynamicClientIDToFormPostBody(r)
+					r.Form.Set("scope", "openid offline_access username")
+				},
+				modifyTokenRequest: modifyAuthcodeTokenRequestWithDynamicClientAuth,
+				customSessionData:  initialUpstreamOIDCRefreshTokenCustomSessionData(),
+				want: tokenEndpointResponseExpectedValues{
+					wantStatus:                  http.StatusOK,
+					wantClientID:                dynamicClientID,
+					wantSuccessBodyFields:       []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"},
+					wantRequestedScopes:         []string{"openid", "offline_access", "username"},
+					wantGrantedScopes:           []string{"openid", "offline_access", "username"},
+					wantCustomSessionDataStored: initialUpstreamOIDCRefreshTokenCustomSessionData(),
+					wantUsername:                goodUsername,
+					wantGroups:                  nil,
+				},
+			},
+			refreshRequest: refreshRequestInputs{
+				modifyTokenRequest: func(r *http.Request, refreshToken string, accessToken string) {
+					r.Body = happyRefreshRequestBody(refreshToken).WithClientID("").WithScope("openid offline_access username").ReadCloser()
+					r.SetBasicAuth(dynamicClientID, testutil.PlaintextPassword1) // Use basic auth header instead.
+				},
+				want: tokenEndpointResponseExpectedValues{
+					wantUpstreamRefreshCall:           happyOIDCUpstreamRefreshCall(),
+					wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens(), true),
+					wantStatus:                        http.StatusUnauthorized,
+					// auth was rejected because of the upstream group to which the user belonged, as shown by the configured RejectedAuthenticationMessage appearing here
+					wantErrorResponseBody: here.Doc(`
+						{
+							"error":             "error",
+							"error_description": "Error during upstream refresh. Upstream refresh rejected by configured identity policy: users who belong to certain upstream group are not allowed."
+						}
+					`),
+				},
+			},
+		},
 		{
 			// fosite does not look at the scopes provided in refresh requests, although it is a valid parameter.
 			// even if 'groups' is not sent in the refresh request, we will send groups all the same.

internal/federationdomain/resolvedprovider/resolved_provider.go

@@ -141,20 +141,16 @@ type FederationDomainResolvedIdentityProvider interface {
 	// 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.
-	// 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, *IdentityLoginExtras, error)
+	Login(ctx context.Context, submittedUsername string, submittedPassword 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.
 	// 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, identity *Identity, groupsWillBeIgnored bool) (refreshedIdentity *RefreshedIdentity, err error)
+	UpstreamRefresh(ctx context.Context, identity *Identity) (refreshedIdentity *RefreshedIdentity, err error)
 }
 
 // ErrMissingUpstreamSessionInternalError returns a common type of error that can happen during a login or refresh.

internal/federationdomain/resolvedprovider/resolvedldap/resolved_ldap_provider.go

@@ -125,9 +125,8 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) Login(
 	ctx context.Context,
 	submittedUsername string,
 	submittedPassword string,
-	groupsWillBeIgnored bool,
 ) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) {
-	authenticateResponse, authenticated, err := p.Provider.AuthenticateUser(ctx, submittedUsername, submittedPassword, groupsWillBeIgnored)
+	authenticateResponse, authenticated, err := p.Provider.AuthenticateUser(ctx, submittedUsername, submittedPassword)
 	if err != nil {
 		plog.WarningErr("unexpected error during upstream LDAP authentication", err, "upstreamName", p.Provider.GetName())
 		return nil, nil, ErrUnexpectedUpstreamLDAPError.WithWrap(err)
@@ -185,7 +184,6 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) LoginFromCallback(
 func (p *FederationDomainResolvedLDAPIdentityProvider) UpstreamRefresh(
 	ctx context.Context,
 	identity *resolvedprovider.Identity,
-	groupsWillBeIgnored bool,
 ) (refreshedIdentity *resolvedprovider.RefreshedIdentity, err error) {
 	var dn string
 	var additionalAttributes map[string]string
@@ -229,7 +227,6 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) UpstreamRefresh(
 		DN:                   dn,
 		Groups:               identity.UpstreamGroups,
 		AdditionalAttributes: additionalAttributes,
-		SkipGroups:           groupsWillBeIgnored,
 	}, p.GetDisplayName())
 	if err != nil {
 		return nil, resolvedprovider.ErrUpstreamRefreshError().WithHint(

internal/federationdomain/resolvedprovider/resolvedoidc/resolved_oidc_provider.go

@@ -129,7 +129,6 @@ func (p *FederationDomainResolvedOIDCIdentityProvider) Login(
 	ctx context.Context,
 	submittedUsername string,
 	submittedPassword string,
-	_groupsWillBeIgnored bool, // ignored because we always compute the user's group memberships for OIDC, if possible
 ) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) {
 	if !p.Provider.AllowsPasswordGrant() {
 		// Return a user-friendly error for this case which is entirely within our control.
@@ -226,7 +225,6 @@ func (p *FederationDomainResolvedOIDCIdentityProvider) LoginFromCallback(
 func (p *FederationDomainResolvedOIDCIdentityProvider) UpstreamRefresh(
 	ctx context.Context,
 	identity *resolvedprovider.Identity,
-	groupsWillBeIgnored bool,
 ) (refreshedIdentity *resolvedprovider.RefreshedIdentity, err error) {
 	sessionData, ok := identity.IDPSpecificSessionData.(*psession.OIDCSessionData)
 	if !ok {
@@ -283,21 +281,18 @@ func (p *FederationDomainResolvedOIDCIdentityProvider) UpstreamRefresh(
 		return nil, err
 	}
 
-	var refreshedUntransformedGroups []string
-	if !groupsWillBeIgnored {
-		// If possible, update the user's group memberships. The configured groups claim name (if there is one) may or
-		// may not be included in the newly fetched and merged claims. It could be missing due to a misconfiguration of the
-		// claim name. It could also be missing because the claim was originally found in the ID token during login, but
-		// now we might not have a refreshed ID token.
-		// If the claim is found, then use it to update the user's group membership in the session.
-		// If the claim is not found, then we have no new information about groups, so skip updating the group membership
-		// and let any old groups memberships in the session remain.
-		refreshedUntransformedGroups, err = getGroupsFromUpstreamIDToken(p.Provider, mergedClaims)
-		if err != nil {
-			return nil, resolvedprovider.ErrUpstreamRefreshError().WithHintf(
-				"Upstream refresh error while extracting groups claim.").WithTrace(err).
-				WithDebugf("provider name: %q, provider type: %q", p.Provider.GetName(), p.GetSessionProviderType())
-		}
+	// If possible, update the user's group memberships. The configured groups claim name (if there is one) may or
+	// may not be included in the newly fetched and merged claims. It could be missing due to a misconfiguration of the
+	// claim name. It could also be missing because the claim was originally found in the ID token during login, but
+	// now we might not have a refreshed ID token.
+	// If the claim is found, then use it to update the user's group membership in the session.
+	// If the claim is not found, then we have no new information about groups, so skip updating the group membership
+	// and let any old groups memberships in the session remain.
+	refreshedUntransformedGroups, err := getGroupsFromUpstreamIDToken(p.Provider, mergedClaims)
+	if err != nil {
+		return nil, resolvedprovider.ErrUpstreamRefreshError().WithHintf(
+			"Upstream refresh error while extracting groups claim.").WithTrace(err).
+			WithDebugf("provider name: %q, provider type: %q", p.Provider.GetName(), p.GetSessionProviderType())
 	}
 
 	// It's possible that a username wasn't returned by the upstream provider during refresh,

internal/federationdomain/upstreamprovider/upsteam_provider.go

@@ -32,10 +32,6 @@ type RefreshAttributes struct {
 	DN                   string
 	Groups               []string
 	AdditionalAttributes map[string]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
 }
 
 // UpstreamIdentityProviderI includes the interface functions that are common to all upstream identity provider types.

internal/testutil/oidctestutil/testldapprovider.go

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

internal/testutil/transformtestutil/transformtestutil.go

@@ -1,4 +1,4 @@
-// Copyright 2023 the Pinniped contributors. All Rights Reserved.
+// Copyright 2023-2024 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package transformtestutil
@@ -56,3 +56,20 @@ func NewRejectAllAuthPipeline(t *testing.T) *idtransform.TransformationPipeline
 
 	return p
 }
+
+func NewPipeline(t *testing.T, transforms []celtransformer.CELTransformation) *idtransform.TransformationPipeline {
+	t.Helper()
+
+	transformer, err := celtransformer.NewCELTransformer(5 * time.Second)
+	require.NoError(t, err)
+
+	p := idtransform.NewTransformationPipeline()
+
+	for _, transform := range transforms {
+		compiledTransform, err := transformer.CompileTransformation(transform, nil)
+		require.NoError(t, err)
+		p.AppendTransformation(compiledTransform)
+	}
+
+	return p
+}

internal/upstreamldap/upstreamldap.go

@@ -250,10 +250,6 @@ func (p *Provider) PerformRefresh(ctx context.Context, storedRefreshAttributes u
 	if p.c.GroupSearch.SkipGroupRefresh {
 		return storedRefreshAttributes.Groups, nil
 	}
-	// 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
-	}
 
 	var groupSearchUserAttributeForFilterValue string
 	if p.useGroupSearchUserAttributeForFilter() {
@@ -422,23 +418,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, skipGroups bool) (*authenticators.Response, bool, error) {
+func (p *Provider) DryRunAuthenticateUser(ctx context.Context, username string) (*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, skipGroups, endUserBindFunc)
+	return p.authenticateUserImpl(ctx, username, 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, skipGroups bool) (*authenticators.Response, bool, error) {
+func (p *Provider) AuthenticateUser(ctx context.Context, username, password string) (*authenticators.Response, bool, error) {
 	endUserBindFunc := func(conn Conn, foundUserDN string) error {
 		return conn.Bind(foundUserDN, password)
 	}
-	return p.authenticateUserImpl(ctx, username, skipGroups, endUserBindFunc)
+	return p.authenticateUserImpl(ctx, username, endUserBindFunc)
 }
 
-func (p *Provider) authenticateUserImpl(ctx context.Context, username string, skipGroups bool, bindFunc func(conn Conn, foundUserDN string) error) (*authenticators.Response, bool, error) {
+func (p *Provider) authenticateUserImpl(ctx context.Context, username string, 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
 
@@ -467,7 +463,7 @@ func (p *Provider) authenticateUserImpl(ctx context.Context, username string, sk
 		return nil, false, fmt.Errorf(`error binding as %q before user search: %w`, p.c.BindUsername, err)
 	}
 
-	response, err := p.searchAndBindUser(conn, username, skipGroups, bindFunc)
+	response, err := p.searchAndBindUser(conn, username, bindFunc)
 	if err != nil {
 		p.traceAuthFailure(t, err)
 		return nil, false, err
@@ -564,7 +560,7 @@ func (p *Provider) SearchForDefaultNamingContext(ctx context.Context) (string, e
 	return searchBase, nil
 }
 
-func (p *Provider) searchAndBindUser(conn Conn, username string, skipGroups bool, bindFunc func(conn Conn, foundUserDN string) error) (*authenticators.Response, error) {
+func (p *Provider) searchAndBindUser(conn Conn, username string, 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`,
@@ -610,22 +606,19 @@ func (p *Provider) searchAndBindUser(conn Conn, username string, skipGroups bool
 		return nil, err
 	}
 
-	var mappedGroupNames []string
-	if !skipGroups {
-		var groupSearchUserAttributeForFilterValue string
-		if p.useGroupSearchUserAttributeForFilter() {
-			groupSearchUserAttributeForFilterValue, err = p.getSearchResultAttributeValue(p.c.GroupSearch.UserAttributeForFilter, userEntry, username)
-			if err != nil {
-				return nil, err
-			}
-		}
-
-		mappedGroupNames, err = p.searchGroupsForUserMembership(conn, userEntry.DN, groupSearchUserAttributeForFilterValue)
+	var groupSearchUserAttributeForFilterValue string
+	if p.useGroupSearchUserAttributeForFilter() {
+		groupSearchUserAttributeForFilterValue, err = p.getSearchResultAttributeValue(p.c.GroupSearch.UserAttributeForFilter, userEntry, username)
 		if err != nil {
 			return nil, err
 		}
 	}
 
+	mappedGroupNames, err := p.searchGroupsForUserMembership(conn, userEntry.DN, groupSearchUserAttributeForFilterValue)
+	if err != nil {
+		return nil, err
+	}
+
 	mappedRefreshAttributes := make(map[string]string)
 	for k := range p.c.RefreshAttributeChecks {
 		mappedVal, err := p.getSearchResultAttributeRawValueEncoded(k, userEntry, username)

internal/upstreamldap/upstreamldap_test.go

@@ -178,7 +178,6 @@ func TestEndUserAuthentication(t *testing.T) {
 		name                       string
 		username                   string
 		password                   string
-		skipGroups                 bool
 		providerConfig             *ProviderConfig
 		searchMocks                func(conn *mockldapconn.MockConn)
 		bindEndUserMocks           func(conn *mockldapconn.MockConn)
@@ -291,25 +290,6 @@ func TestEndUserAuthentication(t *testing.T) {
 				info.Groups = []string{}
 			}),
 		},
-		{
-			name:           "when groups are requested to be skipped, don't do group search",
-			username:       testUpstreamUsername,
-			password:       testUpstreamPassword,
-			skipGroups:     true,
-			providerConfig: providerConfig(nil),
-			searchMocks: func(conn *mockldapconn.MockConn) {
-				conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1)
-				conn.EXPECT().Search(expectedUserSearch(nil)).Return(exampleUserSearchResult, nil).Times(1)
-				conn.EXPECT().Close().Times(1)
-			},
-			bindEndUserMocks: func(conn *mockldapconn.MockConn) {
-				conn.EXPECT().Bind(testUserSearchResultDNValue, testUpstreamPassword).Times(1)
-			},
-			wantAuthResponse: expectedAuthResponse(func(r *authenticators.Response) {
-				info := r.User.(*user.DefaultInfo)
-				info.Groups = nil
-			}),
-		},
 		{
 			name:     "when the UsernameAttribute is dn and there is a user search filter provided",
 			username: testUpstreamUsername,
@@ -754,30 +734,6 @@ func TestEndUserAuthentication(t *testing.T) {
 			},
 			wantAuthResponse: expectedAuthResponse(nil),
 		},
-		{
-			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"
-			}),
-			searchMocks: func(conn *mockldapconn.MockConn) {
-				conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1)
-				conn.EXPECT().Search(expectedUserSearch(func(r *ldap.SearchRequest) {
-					// need to additionally ask for the attribute when performing user search
-					r.Attributes = []string{testUserSearchUsernameAttribute, testUserSearchUIDAttribute, "someUserAttrName"}
-				})).Return(exampleUserSearchResult, nil).Times(1) // result does not contain someUserAttrName, but does not matter since group search is skipped
-				conn.EXPECT().Close().Times(1)
-			},
-			bindEndUserMocks: func(conn *mockldapconn.MockConn) {
-				conn.EXPECT().Bind(testUserSearchResultDNValue, testUpstreamPassword).Times(1)
-			},
-			wantAuthResponse: expectedAuthResponse(func(r *authenticators.Response) {
-				info := r.User.(*user.DefaultInfo)
-				info.Groups = nil
-			}),
-		},
 		{
 			name:     "when the UserAttributeForFilter is set to something other than dn but that attribute is not returned by the user search",
 			username: testUpstreamUsername,
@@ -1421,7 +1377,7 @@ func TestEndUserAuthentication(t *testing.T) {
 
 			ldapProvider := New(*tt.providerConfig)
 
-			authResponse, authenticated, err := ldapProvider.AuthenticateUser(context.Background(), tt.username, tt.password, tt.skipGroups)
+			authResponse, authenticated, err := ldapProvider.AuthenticateUser(context.Background(), tt.username, tt.password)
 			require.Equal(t, !tt.wantToSkipDial, dialWasAttempted)
 			switch {
 			case tt.wantError != nil:
@@ -1453,7 +1409,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.skipGroups)
+			authResponse, authenticated, err = ldapProvider.DryRunAuthenticateUser(context.Background(), tt.username)
 			require.Equal(t, !tt.wantToSkipDial, dialWasAttempted)
 			switch {
 			case tt.wantError != nil:
@@ -1585,7 +1541,6 @@ func TestUpstreamRefresh(t *testing.T) {
 	tests := []struct {
 		name           string
 		providerConfig *ProviderConfig
-		skipGroups     bool
 		setupMocks     func(conn *mockldapconn.MockConn)
 		refreshUserDN  string
 		dialError      error
@@ -1721,18 +1676,6 @@ func TestUpstreamRefresh(t *testing.T) {
 			},
 			wantGroups: nil, // do not update groups
 		},
-		{
-			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)
-				conn.EXPECT().Search(expectedUserSearch(nil)).Return(happyPathUserSearchResult, nil).Times(1)
-				// note that group search is not expected
-				conn.EXPECT().Close().Times(1)
-			},
-			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",
 			providerConfig: providerConfig(func(p *ProviderConfig) {
@@ -1750,23 +1693,6 @@ func TestUpstreamRefresh(t *testing.T) {
 			},
 			wantGroups: nil, // do not update groups
 		},
-		{
-			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"
-			}),
-			setupMocks: func(conn *mockldapconn.MockConn) {
-				conn.EXPECT().Bind(testBindUsername, testBindPassword).Times(1)
-				conn.EXPECT().Search(expectedUserSearch(func(r *ldap.SearchRequest) {
-					// need to additionally ask for the attribute when performing user search
-					r.Attributes = []string{testUserSearchUsernameAttribute, testUserSearchUIDAttribute, "someUserAttrName", pwdLastSetAttribute}
-				})).Return(happyPathUserSearchResult, nil).Times(1)
-				// note that group search is not expected
-				conn.EXPECT().Close().Times(1)
-			},
-			skipGroups: true,
-			wantGroups: nil,
-		},
 		{
 			name: "happy path when the UserAttributeForFilter is set to something other than dn",
 			providerConfig: providerConfig(func(p *ProviderConfig) {
@@ -2282,7 +2208,6 @@ func TestUpstreamRefresh(t *testing.T) {
 				Subject:              subject,
 				DN:                   tt.refreshUserDN,
 				AdditionalAttributes: map[string]string{pwdLastSetAttribute: initialPwdLastSetEncoded},
-				SkipGroups:           tt.skipGroups,
 			}, testUpstreamName)
 			if tt.wantErr != "" {
 				require.Error(t, err)

test/integration/ldap_client_test.go

@@ -73,7 +73,6 @@ func TestLDAPSearch_Parallel(t *testing.T) {
 		name                string
 		username            string
 		password            string
-		skipGroups          bool
 		provider            *upstreamldap.Provider
 		wantError           testutil.RequireErrorStringFunc
 		wantAuthResponse    *authenticators.Response
@@ -115,18 +114,6 @@ func TestLDAPSearch_Parallel(t *testing.T) {
 				ExtraRefreshAttributes: map[string]string{},
 			},
 		},
-		{
-			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",
-				ExtraRefreshAttributes: map[string]string{},
-			},
-		},
 		{
 			name:     "when the user search filter is already wrapped by parenthesis",
 			username: "pinny",
@@ -741,7 +728,7 @@ func TestLDAPSearch_Parallel(t *testing.T) {
 	for _, test := range tests {
 		tt := test
 		t.Run(tt.name, func(t *testing.T) {
-			authResponse, authenticated, err := tt.provider.AuthenticateUser(ctx, tt.username, tt.password, tt.skipGroups)
+			authResponse, authenticated, err := tt.provider.AuthenticateUser(ctx, tt.username, tt.password)
 
 			switch {
 			case tt.wantError != nil:
@@ -799,7 +786,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, false)
+			authResponse, authenticated, err := provider.AuthenticateUser(authUserCtx, env.SupervisorUpstreamLDAP.TestUserCN, env.SupervisorUpstreamLDAP.TestUserPassword)
 			resultCh <- authUserResult{
 				response:      authResponse,
 				authenticated: authenticated,