Combine search query param with other filters in users endpoint - #50003
Combine search query param with other filters in users endpoint#50003hakdogan wants to merge 1 commit into
Conversation
|
@hakdogan thank you; that will greatly improve my user search, where I have to filter by IDP in a multi-tenant application (each "tenant" is identified as an IDP, and users authenticated by a provider should not "see" users from other providers). |
|
Thanks @ch4mpy that per-IDP, multi-tenant scenario is exactly the kind of use case this addresses. With the change, search now stacks with idpAlias (and the other filters) instead of overriding them, so e.g.
returns only users linked to that identity provider and matching the search term. The PR is open and waiting on a maintainer review. Worth flagging for the reviewers: the earlier "other params are ignored" point comes from the UserQueryMethodsProvider / getUsersCount Javadoc, while the public REST/OpenAPI docs describe only the search semantics so if this change lands, that SPI Javadoc should be updated to match. I'm happy to adjust the scope based on what the maintainers prefer. |
The 'search' query param on GET /admin/realms/{realm}/users overrode the
other attribute filters (email, username, idpAlias, idpUserId, firstName,
lastName, exact, q) instead of stacking with them, so a request like
?email=foo&username=baz&search=bam ignored email and username and only
applied the full-text search.
The branching is reworked so that, unless the search uses a special prefix
(id:/username:/email:) which keeps its dedicated lookup path, all provided
filters are collected into a single attribute map and applied together.
Closes keycloak#49995
Signed-off-by: Hüseyin Akdoğan <huseyin@keymate.io>
0469cce to
6cc3881
Compare
There was a problem hiding this comment.
Pull request overview
Combines full-text user search with explicit filters in the Admin REST API.
Changes:
- Consolidates search and attribute filters.
- Preserves prefix lookup and service-account behavior.
- Adds database-backed regression coverage.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
UsersResource.java |
Combines user-search filters. |
UserSearchTest.java |
Tests username and email combinations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (search != null) { | ||
| attributes.put(UserModel.SEARCH, search.trim()); |
There was a problem hiding this comment.
Accurate observation, but this is pre-existing behaviour in the LDAP provider rather than something introduced here, and it matches the current SPI contract documented in UserQueryMethodsProvider#searchForUserStream ("when SEARCH is set all other params are ignored"). Changing it means changing that contract and reworking the LDAP query building, so I have raised the scoping question with the maintainers in the conversation below rather than expanding this PR unilaterally.
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.webauthn.passwordless.WebAuthnPwdLessPropertyTest#timeout |
|
The automated review flagged that this PR does not fix the same problem for LDAP backed users, and that is correct. Before expanding the scope I would like maintainer input on the contract question behind it.
So the real question is the contract itself, not just the LDAP provider. This PR changes the admin REST endpoint so that My suggestion: keep this PR scoped to the REST endpoint and the JPA path, and track the LDAP provider and the SPI javadoc as a follow up issue that I am happy to pick up. Technically the LDAP side is feasible: If you would rather see it all land as one change, say the word and I will extend this PR accordingly. For the record, the branch was rebased on current This has been open since June without a maintainer review, so I would appreciate maintainer guidance on the scoping question above. |
Closes #49995
Problem
On
GET /admin/realms/{realm}/users, thesearchquery param overrode the other attribute filters instead of stacking with them. A request like:GET /admin/realms/my-realm/users?email=foo@bar&username=baz&search=bam
ignored
emailandusernameand only applied the full-textsearch, so it returned every user matchingbamin username, first/last name or email. The same happened withidpAlias,idpUserId,firstName,lastName,exactandq.Cause
In
UsersResource#getUsers, the logic was anif (search != null) … else if (other filters) … else …chain. Whensearchwas present, the branch that handles all the other filters was never reached, and thesearchbranch builtan attribute map containing only
SEARCH(plusenabled/emailVerified/ created timestamps) before returning early.Fix
The branching is reworked so that all provided filters are collected into a single attribute map and applied together, letting
searchstack with the other filters. The special prefix lookups (id:/username:/email:) keep their dedicated path and short-circuit as before. TheincludeServiceAccountsbehaviour is preserved: service accounts are excluded whensearchis used and included for plain attribute filtering.The underlying store layer (
JpaUserProvider#predicates) already supports combiningSEARCHwith the individual field predicates, so no model changes were needed.Testing
Added
UserSearchTest#searchStacksWithOtherFilters, which creates two users sharing a common last name and verifies that combiningsearchwithusername/emailnarrows the result to the matching user instead of returning both.