Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Use keycloak-client libraries instead of keycloak-common, keycloak-core and keycloak-adapter-spi #43260
Use keycloak-client libraries instead of keycloak-common, keycloak-core and keycloak-adapter-spi #43260
Changes from all commits
0304e70
27d6bc2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Same here actually. We now have a dependency with the Apache HTTP Client and I don't think we want (except if I'm mistaken) that.
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.
@gsmet This is a mistake at
keycloak-client-common-synced
side. I will fix it there and update this PR with new version ofkeycloak-client
.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.
Thanks I will have another look next week.
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.
@pskopek I think I misunderstood you. The issue is not fixed, right?
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 released new version of keycloak-client (26.0.3) and updated the PR. It is fixed now.
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'm surprised to this both here and in ResteasyReactiveClientProvider.java.
ObjectMapper
is a really heavy object and reusing them should be done if possible.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.
@geoand I'll need bit more information. I'll try to answer your comment and please provide more context:
jacksonprovider
that is part of the Keycloak here https://github.com/keycloak/keycloak/blob/main/integration/admin-client/src/main/java/org/keycloak/admin/client/JacksonProvider.java and it's basically c&p so I trust Keycloak folk know what they are doing with setting these 2 configuration propertiesThere 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.
Okay, that settles it, thanks!
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.
If we are not reusing an
ObjectMapper
we already have, shouldn't we let the Keycloak libraries instantiate and configure theObjectMapper
as they see fit?Or do we absolutely need to pass one?
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.
Experience is that Keycloaks provider wasn't native compatible in the past. What I did now was necessary in past (you can lookup issues, JSONB was one of them), but I don't know if it is still true, I think we have limited native test coverage for RESTEasy one. Does it matter?
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.
Well, it somehow matters given we had to tweak things to have a compatible client. And we're not entirely sure we enabled all the right options. It seems to work for now but what if they change the serialization options in the future?
Now maybe I'm being too cautious but that's something we could miss in the future. And if for instance, we don't serialize something correctly, it might end up having security consequences.
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.
Note that if you're on PTO, this can wait early Janiuary :). I'm not on PTO so that's why you see me popping :).
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.
note that this code came from @pskopek in the REST client version and I repeatedly asked him if it is fine to do that (not that I got satisfactory answer, this was only one - #43260 (comment)).
JacksonProvider
in the RESTEasy one, but I am pretty sure we don't have test coverage for all the past native issues because there wasn't a good place to put them. I believe thatKeycloak Admin REST Client
is both tested in native and preferred.ObjectMapper
in the REST admin client, so if we need to get it right there, why can't we use same code here?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 agree, but the situation in the reactive client is same, so I don't believe it would make a difference here. What would make a difference is Keycloak providing ObjectMapper factory we could re-use in both extensions. @pskopek WDYT? This way, Keycloak would be owner of the logic.
The idea that Keycloak client is compatible with both previous and future versions of Keycloak makes me nervous as I can't forsee if any incompatibility could result in a vulnerability, but I have already mentioned it before.