-
Notifications
You must be signed in to change notification settings - Fork 7.4k
Allow searching for multiple users by their ids #37901
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
2734b74
to
c8ca8a1
Compare
integration/admin-client/src/main/java/org/keycloak/admin/client/resource/UsersResource.java
Outdated
Show resolved
Hide resolved
c8ca8a1
to
1e584ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@maxhov could you rebase the PR and fix the conflicts? |
09218ef
to
ec45ce9
Compare
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added one minor comment inline regarding javadoc.
However I think this should be reviewed by @keycloak/core-iam team as well as users are in their area of ownership and this PR is updating admin REST API, which looks like a change, which should be approved by core-iam team.
@@ -245,6 +245,12 @@ List<UserRepresentation> search(@QueryParam("search") String search, | |||
List<UserRepresentation> list(@QueryParam("first") Integer firstResult, | |||
@QueryParam("max") Integer maxResults); | |||
|
|||
@GET |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible if you add the javadoc to this new method similar to bb4837d ? Especially it is needed to mention that method with the parameters like ids
and briefRepresentation
is supported since Keycloak 26.2 (if we manage to make this PR within Keycloak 26.2, which is going to be in next weeks).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 3735fe3
8afbf7f
to
3735fe3
Compare
3735fe3
to
3d9b94e
Compare
I've discussed this with @pedroigor , and we think we should probably be leveraging the So if you send a GET to |
This is what I suggested here: #37901 (comment). I am happy to redo this one more time (it is the third one already :)), so can we come to a concensus here first to what the approach should be @jonkoops @sguilhen @pedroigor @mposolda ? IMO the search query would be the most logical choice. |
Ah sorry, I missed that comment. As people say, third time's the charm :) But seriously, I think this is the best way to move with this. Let's see what the others think before you spend more time in this PR |
I am fine with whatever approach, if the others believe the search is the best way to do this then let's go for that. |
59f2d0e
to
36008a9
Compare
@jonkoops @sguilhen I modified the implementation to accept multiple values when searching for users by id. |
services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java
Show resolved
Hide resolved
@maxhov Would you be able to rebase? Looks like we have conflicts @pedroigor Can we can this in once it is rebased? It had all needed approvals and been open for a while now. |
36008a9
to
93ce31e
Compare
I rebased it and merged as best as I could. Commit d98ca0a adds some filtering and I am not sure if the way I merged it, the intent of the commit is still valid. @pedroigor is the author of the commit, so it would be good if he can validate how I handled it. |
Signed-off-by: maxhov <14804474+maxhov@users.noreply.github.com>
93ce31e
to
85f36d9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unreported flaky test detected, please review
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.broker.KcOidcBrokerTest#loginWithExistingUserWithBruteForceEnabledKeycloak CI - Java Distribution IT (windows-latest - temurin - 24)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also agree that it would be better to use a specific id
and multivalued parameter. But it would introduce yet another one to an endpoint that is already hard to follow.
Hopefully, we'll be able to refactor those parameters in Admin API v2.
Thanks for your time @maxhov ! |
An implementation to batch search for users by their ids.
Closes #12025.