8000 Allow searching for multiple users by their ids by maxhov · Pull Request #37901 · keycloak/keycloak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 1 commit into from
Apr 23, 2025

Conversation

maxhov
Copy link
Contributor
@maxhov maxhov commented Mar 7, 2025

An implementation to batch search for users by their ids.

Closes #12025.

@maxhov maxhov requested review from a team as code owners March 7, 2025 09:13
@maxhov maxhov changed the title Allow searching for multiple users by their ids. Closes #12025. Allow searching for multiple users by their ids Mar 7, 2025
@maxhov maxhov force-pushed the 12025-multiple-user-ids branch 4 times, most recently from 2734b74 to c8ca8a1 Compare March 7, 2025 13:51
@maxhov maxhov force-pushed the 12025-multiple-user-ids branch from c8ca8a1 to 1e584ff Compare March 14, 2025 12:37
jonkoops
jonkoops previously approved these changes Mar 17, 2025
Copy link
Contributor
@jonkoops jonkoops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jonkoops
Copy link
Contributor

@maxhov could you rebase the PR and fix the conflicts?

@maxhov maxhov force-pushed the 12025-multiple-user-ids branch 2 times, most recently from 09218ef to ec45ce9 Compare March 17, 2025 13:29
jonkoops
jonkoops previously approved these changes Mar 17, 2025
keycloak-github-bot[bot]

This comment was marked as outdated.

@keycloak-github-bot

This comment was marked as outdated.

jonkoops
jonkoops previously approved these changes Mar 25, 2025
Copy link
Contributor
@jonkoops jonkoops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor
@mposolda mposolda left a 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
Copy link
Contributor

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 3735fe3

@maxhov maxhov force-pushed the 12025-multiple-user-ids branch 2 times, most recently from 8afbf7f to 3735fe3 Compare March 26, 2025 08:50
@jonkoops jonkoops force-pushed the 12025-multiple-user-ids branch from 3735fe3 to 3d9b94e Compare March 26, 2025 10:14
@sguilhen
Copy link
Contributor

I've discussed this with @pedroigor , and we think we should probably be leveraging the search=id:UUID option we currently have to support multiple ids.

So if you send a GET to /users/search=id:UUID you are doing a search by id. Can't we expand that to accept multiple ids instead of adding a new param? So you would send a GET /users/search=id:UUID1 UUID2 UUID3 in a similar way we do to query by attributes where you do GET users/q=aatr1:value1 attr2:value2?

@maxhov
Copy link
Contributor Author
maxhov commented Mar 26, 2025

I've discussed this with @pedroigor , and we think we should probably be leveraging the search=id:UUID option we currently have to support multiple ids.

So if you send a GET to /users/search=id:UUID you are doing a search by id. Can't we expand that to accept multiple ids instead of adding a new param? So you would send a GET /users/search=id:UUID1 UUID2 UUID3 in a similar way we do to query by attributes where you do GET users/q=aatr1:value1 attr2:value2?

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.

@sguilhen
Copy link
Contributor

I've discussed this with @pedroigor , and we think we should probably be leveraging the search=id:UUID option we currently have to support multiple ids.
So if you send a GET to /users/search=id:UUID you are doing a search by id. Can't we expand that to accept multiple ids instead of adding a new param? So you would send a GET /users/search=id:UUID1 UUID2 UUID3 in a similar way we do to query by attributes where you do GET users/q=aatr1:value1 attr2:value2?

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

@jonkoops
Copy link
Contributor

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.

@maxhov maxhov force-pushed the 12025-multiple-user-ids branch 3 times, most recently from 59f2d0e to 36008a9 Compare April 2, 2025 19:21
@maxhov
Copy link
Contributor Author
maxhov commented Apr 2, 2025

I've discussed this with @pedroigor , and we think we should probably be leveraging the search=id:UUID option we currently have to support multiple ids.
So if you send a GET to /users/search=id:UUID you are doing a search by id. Can't we expand that to accept multiple ids instead of adding a new param? So you would send a GET /users/search=id:UUID1 UUID2 UUID3 in a similar way we do to query by attributes where you do GET users/q=aatr1:value1 attr2:value2?

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

@jonkoops @sguilhen I modified the implementation to accept multiple values when searching for users by id.

jonkoops
jonkoops previously approved these changes Apr 3, 2025
@jonkoops jonkoops requested a review from mposolda April 3, 2025 08:54
@sguilhen
Copy link
Contributor

@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.

@maxhov
Copy link
Contributor Author
maxhov commented Apr 16, 2025

@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.

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>
@maxhov maxhov force-pushed the 12025-multiple-user-ids branch from 93ce31e to 85f36d9 Compare April 16, 2025 18:56
@sguilhen sguilhen requested a review from pedroigor April 16, 2025 19:55
Copy link
@keycloak-github-bot keycloak-github-bot bot left a 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

@keycloak-github-bot
Copy link

Unreported flaky test detected

If 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#loginWithExistingUserWithBruteForceEnabled

Keycloak CI - Java Distribution IT (windows-latest - temurin - 24)

org.openqa.selenium.NoSuchElementException: 
Unable to locate element
For documentation on this error, please visit: https://www.selenium.dev/documentation/webdriver/troubleshooting/errors#no-such-element-exception
Build info: version: '4.28.1', revision: '73f5ad48a2'
System info: os.name: 'Windows Server 2022', os.arch: 'amd64', os.version: '10.0', java.version: '24'
...

Report flaky test

Copy link
Contributor
@pedroigor pedroigor left a 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.

@pedroigor
Copy link
Contributor

Thanks for your time @maxhov !

@pedroigor pedroigor merged commit 9654210 into keycloak:main Apr 23, 2025
77 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Get multiple users by Ids
7 participants
0