Use keycloak-client libraries instead of keycloak-common, keycloak-core and keycloak-adapter-spi #43260
Conversation
quarkus-bot
bot
added
area/dependencies
|
Thanks @pskopek for the PR, I think I understand the idea, but I'm not sure I appreciate the consequences if any for Quarkus, I'd like to ask @pedroigor and @mposolda to review, thanks |
|
@pskopek I can try the devservice myself a little bit later to save you some time |
This comment has been minimized.
This comment has been minimized.
|
😭 Deploy PR Preview failed. |
|
@pskopek afaics, only the dependency on |
This comment has been minimized.
This comment has been minimized.
|
The failures are unrelated. |
|
@pskopek Keycloak devservice is OK with this update :-), thanks |
pskopek
force-pushed
the
kc-client-libs
branch
from
71f2bbc to
8bd84fc
Compare
|
@sberyozkin @aloubyansky I fixed the issues with versions you requested. There is a separation between Keycloak Server (25.0.6) for testing purpose and Keycloak Client Libs (26.0.0) used in Quarkus. |
|
Dependency-wise this looks better @pskopek, thanks! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
pskopek
force-pushed
the
kc-client-libs
branch
from
8bd84fc to
948263f
Compare
pskopek
force-pushed
the
kc-client-libs
branch
from
0f25936 to
3ed1996
Compare
|
CI failures look related. |
pskopek
force-pushed
the
kc-client-libs
branch
from
fbf5c2a to
c426b2c
Compare
|
@aloubyansky can you check you're all good now and approve if so? Thanks! |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@pskopek so we're making progress but there's still a test consistently failing. Could you have a look? Thanks. |
|
@pskopek do you need help finalizing this? It's going to be in the way of Keycloak updates very soon. |
|
Here are a few more details about the failures (apparently a certificate issue): |
|
@sberyozkin @michalvavrik could one of you have a look at this test ^? I think this PR is something we want for next LTS/RHBQ. |
I checked code / deps changes and can't see anything obvious. Will check this PR out later this week and debug it unless @sberyozkin comes first. |
|
@gsmet @michalvavrik Sorry, I'll have a look |
|
@gsmet @michalvavrik It is not certificate issue. It is a On With this PR: In the latter case, and it is the only difference I see, is an extra attribute The content-length difference is 26 characters, which I matches . It might not be necessarily a cause of the failure, but it very well might be. My bet at this stage is that Jackson Object Mapping has been affected, but I'd appreciate some help with tracing what may have been impacted as I'm not very uptodate on the mapping mechanics |
|
@sberyozkin I'll have a look at that and serialization discussed in a comment thread above and get back to you today. |
|
Hello @sberyozkin , please cherry-pick and push this commit 0d282a3 into this PR (provided you agree with changes of course), because I don't have required rights. It both fixed the test failures and resolves this comment thread: #43260 (comment). Ideally, also rebase this PR on current main. Thanks Then we will need to get @geoand approval and this PR can be merged. Cheers |
|
Thanks @michalvavrik, that is brilliant, let me cherry-pick it |
…re and keycloak-adapter-spi Closes quarkusio#43259 Signed-off-by: Peter Skopek <[email protected]>
sberyozkin
force-pushed
the
kc-client-libs
branch
from
c426b2c to
27d6bc2
Compare
Status for workflow
|
| this.objectMapper = new ObjectMapper(); | ||
| // Same like JSONSerialization class. Makes it possible to use admin-client against older | ||
| // versions of Keycloak server where the properties on representations might be different | ||
| this.objectMapper.setSerializationInclusion(JsonInclude.Include.NON_NULL); | ||
| // The client must work with the newer versions of Keycloak server, which might contain the JSON fields | ||
| // not yet known by the client. So unknown fields will be ignored. | ||
| this.objectMapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false); |
There was a problem hiding this comment.
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.
@geoand I'll need bit more information. I'll try to answer your comment and please provide more context:
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.
- They are 2 different extensions, considering they are just 3 lines, I repeated it instead of putting this method to some util in Keycloak Admin client common
- These 2 extensions are not going to be used together, so they can't share same ObjectMapper instance
- AFAICT this object mapper is stored on the provider level (at least in the RESTEasy Classic where I debugged it), so the method you are commenting on is called once per provider instance. If you think I should create it on startup and keep it in static variable, np, I'll do it
- we need different object mapper than is managed by Quarkus (instance in CDI) because these to options set here (include non-null, fail on unknown false) shouldn't be set for other REST clients or wherever is the ObjectMapper used
- I checked
jacksonproviderthat 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 properties
There was a problem hiding this comment.
These 2 extensions are not going to be used together, so they can't share same ObjectMapper instance
Okay, that settles it, thanks!
There was a problem hiding this comment.
If we are not reusing an ObjectMapper we already have, shouldn't we let the Keycloak libraries instantiate and configure the ObjectMapper as they see fit?
Or do we absolutely need to pass one?
There was a problem hiding this comment.
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.
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.
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.
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?
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)).
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.
- Absolutely, but we can't know and I asked repeatedly if it is fine. I agree with you. We have Keycloak folk integrating Keycloak admin client here, it can't get any better. Only thing we could do is to switch to the KC
JacksonProviderin 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 Clientis both tested in native and preferred. - Even if we do that, we still have manually managed
ObjectMapperin 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.
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.
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.
sberyozkin
added
the
triage/waiting-for-ci
Status for workflow
|
quarkus-bot
bot
removed
the
triage/waiting-for-ci
This PR uses keycloak-client 26.0.0-alpha2. If it passes all checks and reviews I can release final and update the PR.
Closes #43259