diff --git a/internal/api/grpc/user/v2/integration_test/query_test.go b/internal/api/grpc/user/v2/integration_test/query_test.go index 70c23ad0dee..e0958f4f138 100644 --- a/internal/api/grpc/user/v2/integration_test/query_test.go +++ b/internal/api/grpc/user/v2/integration_test/query_test.go @@ -6,6 +6,7 @@ import ( "context" "encoding/base64" "errors" + "fmt" "slices" "testing" "time" @@ -716,6 +717,86 @@ func TestServer_ListUsers(t *testing.T) { }, }, }, + { + // https://github.com/zitadel/zitadel/issues/10825 + name: "list user with metadata by IDs with offset and limit, ok", + args: args{ + IamCTX, + &user.ListUsersRequest{}, + func(ctx context.Context, request *user.ListUsersRequest) userAttrs { + infos := createUsers(ctx, orgResp.OrganizationId, 3, false) + request.Queries = []*user.SearchQuery{} + request.Queries = append(request.Queries, OrganizationIdQuery(orgResp.OrganizationId)) + request.Queries = append(request.Queries, InUserIDsQuery(infos.userIDs())) + + // With the original bug, this would multiply the "TotalResult" by 2. + for _, user := range infos { + for i := 0; i < 2; i++ { + Instance.SetUserMetadata(ctx, user.UserID, fmt.Sprintf("key-%d", i), fmt.Sprintf("value-%d", i)) + } + } + infos = infos[1:] // prevent panic in user ID setting below + + request.SortingColumn = user.UserFieldName_USER_FIELD_NAME_CREATION_DATE + request.Query = &object.ListQuery{ + Offset: 1, + Limit: 2, + } + return infos + }, + }, + want: &user.ListUsersResponse{ + Details: &object.ListDetails{ + TotalResult: 3, + Timestamp: timestamppb.Now(), + }, + SortingColumn: user.UserFieldName_USER_FIELD_NAME_CREATION_DATE, + Result: []*user.User{ + { + State: user.UserState_USER_STATE_ACTIVE, + Type: &user.User_Human{ + Human: &user.HumanUser{ + Profile: &user.HumanProfile{ + GivenName: "Mickey", + FamilyName: "Mouse", + NickName: gu.Ptr("Mickey"), + DisplayName: gu.Ptr("Mickey Mouse"), + PreferredLanguage: gu.Ptr("nl"), + Gender: user.Gender_GENDER_MALE.Enum(), + }, + Email: &user.HumanEmail{ + IsVerified: true, + }, + Phone: &user.HumanPhone{ + IsVerified: true, + }, + }, + }, + }, + { + State: user.UserState_USER_STATE_ACTIVE, + Type: &user.User_Human{ + Human: &user.HumanUser{ + Profile: &user.HumanProfile{ + GivenName: "Mickey", + FamilyName: "Mouse", + NickName: gu.Ptr("Mickey"), + DisplayName: gu.Ptr("Mickey Mouse"), + PreferredLanguage: gu.Ptr("nl"), + Gender: user.Gender_GENDER_MALE.Enum(), + }, + Email: &user.HumanEmail{ + IsVerified: true, + }, + Phone: &user.HumanPhone{ + IsVerified: true, + }, + }, + }, + }, + }, + }, + }, { name: "list user by username, ok", args: args{ @@ -900,6 +981,85 @@ func TestServer_ListUsers(t *testing.T) { }, }, }, + { + // https://github.com/zitadel/zitadel/issues/10825 + name: "list user with metadata by emails with offset and limit, ok", + args: args{ + IamCTX, + &user.ListUsersRequest{}, + func(ctx context.Context, request *user.ListUsersRequest) userAttrs { + infos := createUsers(ctx, orgResp.OrganizationId, 3, false) + request.Queries = []*user.SearchQuery{} + request.Queries = append(request.Queries, OrganizationIdQuery(orgResp.OrganizationId)) + request.Queries = append(request.Queries, InUserEmailsQuery(infos.emails())) + + // With the original bug, this would multiply the "TotalResult" by 2. + for _, user := range infos { + for i := 0; i < 2; i++ { + Instance.SetUserMetadata(ctx, user.UserID, fmt.Sprintf("key-%d", i), fmt.Sprintf("value-%d", i)) + } + } + infos = infos[1:] // prevent panic in user ID setting below + + request.SortingColumn = user.UserFieldName_USER_FIELD_NAME_CREATION_DATE + request.Query = &object.ListQuery{ + Offset: 1, + Limit: 2, + } + return infos + }, + }, + want: &user.ListUsersResponse{ + Details: &object.ListDetails{ + TotalResult: 3, + Timestamp: timestamppb.Now(), + }, + SortingColumn: user.UserFieldName_USER_FIELD_NAME_CREATION_DATE, + Result: []*user.User{ + { + State: user.UserState_USER_STATE_ACTIVE, + Type: &user.User_Human{ + Human: &user.HumanUser{ + Profile: &user.HumanProfile{ + GivenName: "Mickey", + FamilyName: "Mouse", + NickName: gu.Ptr("Mickey"), + DisplayName: gu.Ptr("Mickey Mouse"), + PreferredLanguage: gu.Ptr("nl"), + Gender: user.Gender_GENDER_MALE.Enum(), + }, + Email: &user.HumanEmail{ + IsVerified: true, + }, + Phone: &user.HumanPhone{ + IsVerified: true, + }, + }, + }, + }, { + State: user.UserState_USER_STATE_ACTIVE, + Type: &user.User_Human{ + Human: &user.HumanUser{ + Profile: &user.HumanProfile{ + GivenName: "Mickey", + FamilyName: "Mouse", + NickName: gu.Ptr("Mickey"), + DisplayName: gu.Ptr("Mickey Mouse"), + PreferredLanguage: gu.Ptr("nl"), + Gender: user.Gender_GENDER_MALE.Enum(), + }, + Email: &user.HumanEmail{ + IsVerified: true, + }, + Phone: &user.HumanPhone{ + IsVerified: true, + }, + }, + }, + }, + }, + }, + }, { name: "list user in emails no found, ok", args: args{ diff --git a/internal/query/search_query.go b/internal/query/search_query.go index b3d974d703d..e59decf1b0d 100644 --- a/internal/query/search_query.go +++ b/internal/query/search_query.go @@ -23,6 +23,8 @@ type SearchRequest struct { Limit uint64 SortingColumn Column Asc bool + + sortingConsumed bool } func (req *SearchRequest) toQuery(query sq.SelectBuilder) sq.SelectBuilder { @@ -32,15 +34,20 @@ func (req *SearchRequest) toQuery(query sq.SelectBuilder) sq.SelectBuilder { if req.Limit > 0 { query = query.Limit(req.Limit) } + return req.consumeSorting(query) +} - if !req.SortingColumn.isZero() { +// consumeSorting sets the sorting column to the query once. +// subsequent calls will not set the sorting column again. +func (req *SearchRequest) consumeSorting(query sq.SelectBuilder) sq.SelectBuilder { + if !req.sortingConsumed && !req.SortingColumn.isZero() { clause := req.SortingColumn.orderBy() if !req.Asc { clause += " DESC" } query = query.OrderByClause(clause) + req.sortingConsumed = true } - return query } diff --git a/internal/query/user.go b/internal/query/user.go index a7ffff7d8b0..742f82cc111 100644 --- a/internal/query/user.go +++ b/internal/query/user.go @@ -634,11 +634,8 @@ func (q *Queries) searchUsers(ctx context.Context, queries *UserSearchQueries, p ctx, span := tracing.NewSpan(ctx) defer func() { span.EndWithError(err) }() - query, scan := prepareUsersQuery(queries.SortingColumn) - query = userPermissionCheckV2(ctx, query, permissionCheckV2, queries.Queries) - stmt, args, err := queries.toQuery(query).Where(sq.Eq{ - UserInstanceIDCol.identifier(): authz.GetInstance(ctx).InstanceID(), - }).ToSql() + query, scan := queries.prepareUsersQuery(ctx, permissionCheckV2) + stmt, args, err := queries.toQuery(query).ToSql() if err != nil { return nil, zerrors.ThrowInternal(err, "QUERY-Dgbg2", "Errors.Query.SQLStatement") } @@ -726,14 +723,6 @@ func (q *Queries) SearchClaimedUserIDsOfOrgDomain(ctx context.Context, domain, o return userIDs, err } -func (q *UserSearchQueries) toQuery(query sq.SelectBuilder) sq.SelectBuilder { - query = q.SearchRequest.toQuery(query) - for _, q := range q.Queries { - query = q.toQuery(query) - } - return query -} - func (r *UserSearchQueries) AppendMyResourceOwnerQuery(orgID string) error { query, err := NewUserResourceOwnerSearchQuery(orgID, TextEquals) if err != nil { @@ -1270,147 +1259,171 @@ func prepareUserUniqueQuery() (sq.SelectBuilder, func(*sql.Row) (bool, error)) { } } -func prepareUsersQuery(orderBy Column) (sq.SelectBuilder, func(*sql.Rows) (*Users, error)) { - if orderBy.isZero() { - orderBy = UserIDCol +// prepareUsersQuery creates the select query for searching users and returns a matching scan function. +// Permissions, filters and sorting are applied in a `SELECT FROM` distinct sub-select. +// The count over window function and limit are applied in the outer query. +// It is not possible to pass more filters to the returned query, as they need to be applied in the sub-select. +func (q *UserSearchQueries) prepareUsersQuery(ctx context.Context, permissionCheckV2 bool) (sq.SelectBuilder, func(*sql.Rows) (*Users, error)) { + if q.SortingColumn.isZero() { + q.SortingColumn = UserIDCol } - return sq.Select( - UserIDCol.identifier(), - UserCreationDateCol.identifier(), - UserChangeDateCol.identifier(), - UserResourceOwnerCol.identifier(), - UserSequenceCol.identifier(), - UserStateCol.identifier(), - UserTypeCol.identifier(), - UserUsernameCol.identifier(), - userLoginNamesListCol.identifier(), - userPreferredLoginNameCol.identifier(), - HumanUserIDCol.identifier(), - HumanFirstNameCol.identifier(), - HumanLastNameCol.identifier(), - HumanNickNameCol.identifier(), - HumanDisplayNameCol.identifier(), - HumanPreferredLanguageCol.identifier(), - HumanGenderCol.identifier(), - HumanAvatarURLCol.identifier(), - HumanEmailCol.identifier(), - HumanIsEmailVerifiedCol.identifier(), - HumanPhoneCol.identifier(), - HumanIsPhoneVerifiedCol.identifier(), - HumanPasswordChangeRequiredCol.identifier(), - HumanPasswordChangedCol.identifier(), - HumanMFAInitSkippedCol.identifier(), - MachineUserIDCol.identifier(), - MachineNameCol.identifier(), - MachineDescriptionCol.identifier(), - MachineSecretCol.identifier(), - MachineAccessTokenTypeCol.identifier(), - orderBy.orderBy(), - countColumn.identifier()). - Distinct(). - From(userTable.identifier()). - LeftJoin(join(HumanUserIDCol, UserIDCol)). - LeftJoin(join(MachineUserIDCol, UserIDCol)). - LeftJoin(join(UserMetadataUserIDCol, UserIDCol)). - JoinClause(joinLoginNames). - PlaceholderFormat(sq.Dollar), - func(rows *sql.Rows) (*Users, error) { - users := make([]*User, 0) - var count uint64 - for rows.Next() { - u := new(User) - loginNames := database.TextArray[string]{} - preferredLoginName := sql.NullString{} - human, machine := sqlHuman{}, sqlMachine{} - var orderByValue any + // start building the sub-select + query := sq.Select( + UserIDCol.identifier(), + UserCreationDateCol.identifier(), + UserChangeDateCol.identifier(), + UserResourceOwnerCol.identifier(), + UserSequenceCol.identifier(), + UserStateCol.identifier(), + UserTypeCol.identifier(), + UserUsernameCol.identifier(), + userLoginNamesListCol.identifier(), + userPreferredLoginNameCol.identifier(), + HumanUserIDCol.identifier(), + HumanFirstNameCol.identifier(), + HumanLastNameCol.identifier(), + HumanNickNameCol.identifier(), + HumanDisplayNameCol.identifier(), + HumanPreferredLanguageCol.identifier(), + HumanGenderCol.identifier(), + HumanAvatarURLCol.identifier(), + HumanEmailCol.identifier(), + HumanIsEmailVerifiedCol.identifier(), + HumanPhoneCol.identifier(), + HumanIsPhoneVerifiedCol.identifier(), + HumanPasswordChangeRequiredCol.identifier(), + HumanPasswordChangedCol.identifier(), + HumanMFAInitSkippedCol.identifier(), + MachineUserIDCol.identifier(), + MachineNameCol.identifier(), + MachineDescriptionCol.identifier(), + MachineSecretCol.identifier(), + MachineAccessTokenTypeCol.identifier(), + q.SortingColumn.orderBy()). + Distinct(). + From(userTable.identifier()). + LeftJoin(join(HumanUserIDCol, UserIDCol)). + LeftJoin(join(MachineUserIDCol, UserIDCol)). + LeftJoin(join(UserMetadataUserIDCol, UserIDCol)). + JoinClause(joinLoginNames). + Where(sq.Eq{UserInstanceIDCol.identifier(): authz.GetInstance(ctx).InstanceID()}) - err := rows.Scan( - &u.ID, - &u.CreationDate, - &u.ChangeDate, - &u.ResourceOwner, - &u.Sequence, - &u.State, - &u.Type, - &u.Username, - &loginNames, - &preferredLoginName, + query = userPermissionCheckV2(ctx, query, permissionCheckV2, q.Queries) + // apply requested filters + for _, q := range q.Queries { + query = q.toQuery(query) + } + // apply sorting in the sub-select,because the identifier is fully qualified. + query = q.consumeSorting(query) - &human.humanID, - &human.firstName, - &human.lastName, - &human.nickName, - &human.displayName, - &human.preferredLanguage, - &human.gender, - &human.avatarKey, - &human.email, - &human.isEmailVerified, - &human.phone, - &human.isPhoneVerified, - &human.passwordChangeRequired, - &human.passwordChanged, - &human.mfaInitSkipped, + // set the sub-select as source for the outer query + query = sq.Select( + "*", + countColumn.identifier(), + ).FromSelect(query, "results") - &machine.machineID, - &machine.name, - &machine.description, - &machine.encodedSecret, - &machine.accessTokenType, + // apply limit and offset in the outer query + query = q.toQuery(query) + query = query.PlaceholderFormat(sq.Dollar) - &orderByValue, - &count, - ) - if err != nil { - return nil, err - } + return query, func(rows *sql.Rows) (*Users, error) { + users := make([]*User, 0) + var count uint64 + for rows.Next() { + u := new(User) + loginNames := database.TextArray[string]{} + preferredLoginName := sql.NullString{} - u.LoginNames = loginNames - if preferredLoginName.Valid { - u.PreferredLoginName = preferredLoginName.String - } + human, machine := sqlHuman{}, sqlMachine{} + var orderByValue any - if human.humanID.Valid { - u.Human = &Human{ - FirstName: human.firstName.String, - LastName: human.lastName.String, - NickName: human.nickName.String, - DisplayName: human.displayName.String, - AvatarKey: human.avatarKey.String, - PreferredLanguage: language.Make(human.preferredLanguage.String), - Gender: domain.Gender(human.gender.Int32), - Email: domain.EmailAddress(human.email.String), - IsEmailVerified: human.isEmailVerified.Bool, - Phone: domain.PhoneNumber(human.phone.String), - IsPhoneVerified: human.isPhoneVerified.Bool, - PasswordChangeRequired: human.passwordChangeRequired.Bool, - PasswordChanged: human.passwordChanged.Time, - MFAInitSkipped: human.mfaInitSkipped.Time, - } - } else if machine.machineID.Valid { - u.Machine = &Machine{ - Name: machine.name.String, - Description: machine.description.String, - EncodedSecret: machine.encodedSecret.String, - AccessTokenType: domain.OIDCTokenType(machine.accessTokenType.Int32), - } - } + err := rows.Scan( + &u.ID, + &u.CreationDate, + &u.ChangeDate, + &u.ResourceOwner, + &u.Sequence, + &u.State, + &u.Type, + &u.Username, + &loginNames, + &preferredLoginName, - users = append(users, u) + &human.humanID, + &human.firstName, + &human.lastName, + &human.nickName, + &human.displayName, + &human.preferredLanguage, + &human.gender, + &human.avatarKey, + &human.email, + &human.isEmailVerified, + &human.phone, + &human.isPhoneVerified, + &human.passwordChangeRequired, + &human.passwordChanged, + &human.mfaInitSkipped, + + &machine.machineID, + &machine.name, + &machine.description, + &machine.encodedSecret, + &machine.accessTokenType, + + &orderByValue, + &count, + ) + if err != nil { + return nil, err } - if err := rows.Close(); err != nil { - return nil, zerrors.ThrowInternal(err, "QUERY-frhbd", "Errors.Query.CloseRows") + u.LoginNames = loginNames + if preferredLoginName.Valid { + u.PreferredLoginName = preferredLoginName.String } - return &Users{ - Users: users, - SearchResponse: SearchResponse{ - Count: count, - }, - }, nil + if human.humanID.Valid { + u.Human = &Human{ + FirstName: human.firstName.String, + LastName: human.lastName.String, + NickName: human.nickName.String, + DisplayName: human.displayName.String, + AvatarKey: human.avatarKey.String, + PreferredLanguage: language.Make(human.preferredLanguage.String), + Gender: domain.Gender(human.gender.Int32), + Email: domain.EmailAddress(human.email.String), + IsEmailVerified: human.isEmailVerified.Bool, + Phone: domain.PhoneNumber(human.phone.String), + IsPhoneVerified: human.isPhoneVerified.Bool, + PasswordChangeRequired: human.passwordChangeRequired.Bool, + PasswordChanged: human.passwordChanged.Time, + MFAInitSkipped: human.mfaInitSkipped.Time, + } + } else if machine.machineID.Valid { + u.Machine = &Machine{ + Name: machine.name.String, + Description: machine.description.String, + EncodedSecret: machine.encodedSecret.String, + AccessTokenType: domain.OIDCTokenType(machine.accessTokenType.Int32), + } + } + + users = append(users, u) } + + if err := rows.Close(); err != nil { + return nil, zerrors.ThrowInternal(err, "QUERY-frhbd", "Errors.Query.CloseRows") + } + + return &Users{ + Users: users, + SearchResponse: SearchResponse{ + Count: count, + }, + }, nil + } } type sqlHuman struct { diff --git a/internal/query/user_test.go b/internal/query/user_test.go index c67eee286f4..e70bd98185c 100644 --- a/internal/query/user_test.go +++ b/internal/query/user_test.go @@ -367,7 +367,8 @@ var ( "password_set", "count", } - usersQuery = `SELECT DISTINCT projections.users14.id,` + + usersQuery = `SELECT *, COUNT(*) OVER () FROM (` + + `SELECT DISTINCT projections.users14.id,` + ` projections.users14.creation_date,` + ` projections.users14.change_date,` + ` projections.users14.resource_owner,` + @@ -397,13 +398,14 @@ var ( ` projections.users14_machines.description,` + ` projections.users14_machines.secret,` + ` projections.users14_machines.access_token_type,` + - ` projections.users14.id,` + - ` COUNT(*) OVER ()` + + ` projections.users14.id` + ` FROM projections.users14` + ` LEFT JOIN projections.users14_humans ON projections.users14.id = projections.users14_humans.user_id AND projections.users14.instance_id = projections.users14_humans.instance_id` + ` LEFT JOIN projections.users14_machines ON projections.users14.id = projections.users14_machines.user_id AND projections.users14.instance_id = projections.users14_machines.instance_id` + ` LEFT JOIN projections.user_metadata5 ON projections.users14.id = projections.user_metadata5.user_id AND projections.users14.instance_id = projections.user_metadata5.instance_id` + - ` LEFT JOIN LATERAL (SELECT ARRAY_AGG(ln.login_name ORDER BY ln.login_name) AS login_names, MAX(CASE WHEN ln.is_primary THEN ln.login_name ELSE NULL END) AS preferred_login_name FROM projections.login_names3 AS ln WHERE ln.user_id = projections.users14.id AND ln.instance_id = projections.users14.instance_id) AS login_names ON TRUE` + ` LEFT JOIN LATERAL (SELECT ARRAY_AGG(ln.login_name ORDER BY ln.login_name) AS login_names, MAX(CASE WHEN ln.is_primary THEN ln.login_name ELSE NULL END) AS preferred_login_name FROM projections.login_names3 AS ln WHERE ln.user_id = projections.users14.id AND ln.instance_id = projections.users14.instance_id) AS login_names ON TRUE` + + ` WHERE projections.users14.instance_id = $1 ORDER BY projections.users14.id DESC` + + `) AS results` usersCols = []string{ "id", "creation_date", @@ -949,7 +951,8 @@ func Test_UserPrepares(t *testing.T) { { name: "prepareUsersQuery no result", prepare: func() (sq.SelectBuilder, func(*sql.Rows) (*Users, error)) { - return prepareUsersQuery(UserIDCol) + q := &UserSearchQueries{} + return q.prepareUsersQuery(t.Context(), false) }, want: want{ sqlExpectations: mockQuery( @@ -969,7 +972,8 @@ func Test_UserPrepares(t *testing.T) { { name: "prepareUsersQuery one result", prepare: func() (sq.SelectBuilder, func(*sql.Rows) (*Users, error)) { - return prepareUsersQuery(UserIDCol) + q := &UserSearchQueries{} + return q.prepareUsersQuery(t.Context(), false) }, want: want{ sqlExpectations: mockQueries( @@ -1053,7 +1057,8 @@ func Test_UserPrepares(t *testing.T) { { name: "prepareUsersQuery one result, no sorting", prepare: func() (sq.SelectBuilder, func(*sql.Rows) (*Users, error)) { - return prepareUsersQuery(Column{}) + q := &UserSearchQueries{} + return q.prepareUsersQuery(t.Context(), false) }, want: want{ sqlExpectations: mockQueries( @@ -1135,13 +1140,19 @@ func Test_UserPrepares(t *testing.T) { }, }, { - name: "prepareUsersQuery multiple results", + name: "prepareUsersQuery multiple results, offset and limit", prepare: func() (sq.SelectBuilder, func(*sql.Rows) (*Users, error)) { - return prepareUsersQuery(UserIDCol) + q := &UserSearchQueries{ + SearchRequest: SearchRequest{ + Offset: 1, + Limit: 2, + }, + } + return q.prepareUsersQuery(t.Context(), false) }, want: want{ sqlExpectations: mockQueries( - regexp.QuoteMeta(usersQuery), + regexp.QuoteMeta(usersQuery+` LIMIT 2 OFFSET 1`), usersCols, [][]driver.Value{ { @@ -1274,7 +1285,8 @@ func Test_UserPrepares(t *testing.T) { { name: "prepareUsersQuery sql err", prepare: func() (sq.SelectBuilder, func(*sql.Rows) (*Users, error)) { - return prepareUsersQuery(UserIDCol) + q := &UserSearchQueries{} + return q.prepareUsersQuery(t.Context(), false) }, want: want{ sqlExpectations: mockQueryErr(