Skip to content
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

Add first version of Keycloak admin client based on Reactive REST Client #24300

Merged
merged 2 commits into from
Mar 28, 2022

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Mar 14, 2022

Alternative to #24294 that requires a new Keycloak release

@quarkus-bot

This comment was marked as resolved.

@geoand geoand changed the title #20505 2 Add first version of Keycloak admin client based on Reactive REST Client (alternative) Mar 14, 2022
@geoand geoand changed the title Add first version of Keycloak admin client based on Reactive REST Client (alternative) Add first version of Keycloak admin client based on Reactive REST Client Mar 28, 2022
@geoand geoand marked this pull request as ready for review March 28, 2022 06:35
@geoand geoand requested a review from sberyozkin March 28, 2022 06:35
@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/keycloak labels Mar 28, 2022
@geoand geoand requested a review from cescoffier March 28, 2022 12:11
@cescoffier
Copy link
Member

Wasn't the last release of keycloak enough?

@geoand
Copy link
Contributor Author

geoand commented Mar 28, 2022

It introduced the changes we needed. This is the new extension that uses the changes and leverages the Reactive REST Client under the hood

@cescoffier
Copy link
Member

Ah ok, I misundertood.

Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

We would need to document that somewhere.
We also do not have any tests for the non-reactive version (I think @sberyozkin was working on that), but we would need some tests for this version.

The rest looks great!

@geoand
Copy link
Contributor Author

geoand commented Mar 28, 2022

I added a couple mentions in the security docs.
Furthermore, it has already been mentioned in https://github.com/quarkusio/quarkus/blob/main/docs/src/main/asciidoc/resteasy-reactive-migration.adoc#keycloak-admin-client

@quarkus-bot

This comment has been minimized.

Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @geoand

@geoand
Copy link
Contributor Author

geoand commented Mar 28, 2022

YW!

@sberyozkin
Copy link
Member

sberyozkin commented Mar 28, 2022

Hi @geoand @cescoffier integration-tests/keycloak-authorization was the only one which was using the non-reactive admin client - a few of OIDC integration tests were doing it it was slowing down the native builds as the admin client was bringing a lot of extra dependencies so in the end it was only integration-tests/keycloak-authorization which was using it. This PR has changed it and now the reactive admin client is used - it is fine;
I'll update one of other OIDC tests to use the non-reactive admin client for completeness with one of the next Keycloak related updates

@geoand geoand requested a review from cescoffier March 28, 2022 18:52
@gsmet gsmet dismissed cescoffier’s stale review March 28, 2022 20:02

I think the doc comment was addressed. As for the tests, Sergey will deal with them later AFAICS.

@gsmet gsmet merged commit 65ac87b into quarkusio:main Mar 28, 2022
@quarkus-bot quarkus-bot bot added this to the 2.9 - main milestone Mar 28, 2022
@gsmet gsmet modified the milestones: 2.9 - main, 2.8.0.Final Mar 28, 2022
@geoand geoand deleted the #20505-2 branch March 29, 2022 05:33
@Foobartender
Copy link
Contributor

Foobartender commented Apr 17, 2022

Quick feedback: I tried it and got a 404 error. It tries to access the URL below, where the last path element ("roles") is duplicated:
http://localhost:9080/auth/admin/realms/myrealm/roles/roles?search=myrole&briefRepresentation=true

I'll look at it closer and create an issue as soon as I find the time.

Edit: has been fixed (#24991, #25028).

@rvansa
Copy link
Contributor

rvansa commented Sep 19, 2022

I wonder, can we expect a client that uses Mutiny in the API?

@geoand
Copy link
Contributor Author

geoand commented Sep 19, 2022

That's one for @sberyozkin and @pedroigor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/keycloak release/noteworthy-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants