Keycloak admin client classic - use default Quarkus Jackson serializers also in native mode

[michalvavrik]

closes: #29035

Currently behavior between quarkus-keycloak-admin-client and quarkus-keycloak-admin-client-reactive differs in native mode where the classic version does fail on uknown properties during JSON deserialization. The reason is that in native com.fasterxml.jackson.jaxrs.json.JacksonJsonProvider is missing (so producing ReflectiveClassBuildItem would fix the issue) and thus, Quarkus ObjectMapper is not loaded as QuarkusObjectMapperContextResolver is never called (only in native). This PR prevents Keycloak from using it's own version of ResteasyJackson2Provider (JacksonProvider), therefore default Quarkus provider is used and Quarkus ObjectMapper is used. This is a native issue and we don't have suitable module so no automated tests.

[quarkus-bot]

Failing Jobs - Building 6851757

Status	Name	Step	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
✔️	JVM Tests - JDK 17
✖	JVM Tests - JDK 17 MacOS M1	Set up runner	⚠️ Check →	Logs	Raw logs
✔️	JVM Tests - JDK 18

[sberyozkin]

Thanks @michalvavrik for your time

[gsmet]

@michalvavrik when you say Quarkus ObjectMapper, are you referring to the one users can customize? If so, I'm wondering if it's a good idea: there's a good chance the Keycloak Admin Client actually requires a specific configuration that a user could break by configuring the Quarkus ObjectMapper.

[michalvavrik]

@gsmet Yes, what is the situation after the PR is that QuarkusJacksonSerializer (at least I checked it is registered, which provider is used depends on your set up) is used instead of the JacksonProvider from Keycloak and user can customze ObjectMapper in standard way. Same is done by the reactive Keycloak for some time already, you can customize it, let see io.quarkus.keycloak.admin.client.reactive.runtime.ResteasyReactiveClientProvider#registerJacksonProviders. IMHO behavior should be same between classic/reactive.

[gsmet]

I agree the behavior should be the same. But I think we should make sure we have a specific ObjectMapper that is configured with the correct behavior required by the Keycloak Admin Client. But maybe I'm misunderstanding something. We can talk about it on Monday, there's no hurry.

[michalvavrik]

I don't say you are wrong, just for your information - user already could customize ObjectMapper before this PR in JVM mode - as in JVM mode, what I call Quarkus ObjectMapper was used (the one from Arc). Behavior only differed in the native mode where com.fasterxml.jackson.jaxrs.json.JacksonJsonProvider was missing and that somehow lead to different behavior (quite hard to debug this in native mode, but I checked that in JVM mode that provider is used and its registration for reflection "fixed issue", I choosed the fix in this PR as it looked cleaner to me).

[gsmet]

OK I had a closer look at this. I really think it's not a good idea to use the default Quarkus mapper as it might be configured by the user in a way that wouldn't be compatible with Keycloak.
I think we need Keycloak to use its own mapper that is configured to be compatible with Keycloak requirements. I'm not entirely sure we should enable the option to ignore unknown properties TBH. Because it might work but it might also cause some things to get ignores without triggering an error. And in a security-related context, I'm a bit worried it might cause some serious issues.

In any case, I think the discussion about the options to enable is a bit orthogonal, I would:

• first make sure we have a specific ObjectMapper for both clients (and an identical one for both).
• second, we should probably discuss with the Keycloak team if being more lenient in these ObjectMappers is a good idea.

Let me know if I missed something but I think I have a better grasp at it now.

[michalvavrik]

OK I had a closer look at this. I really think it's not a good idea to use the default Quarkus mapper as it might be configured by the user in a way that wouldn't be compatible with Keycloak. I think we need Keycloak to use its own mapper that is configured to be compatible with Keycloak requirements. I'm not entirely sure we should enable the option to ignore unknown properties TBH. Because it might work but it might also cause some things to get ignores without triggering an error. And in a security-related context, I'm a bit worried it might cause some serious issues.

I absolutely agree, that's why I suggested here #29035 (comment) that reactive shouldn't be used as fix. For example in #29035 user use KC 20.0.0 with quarkus-keycloak-admin-client (that has 19.0.3), so it can't be guaranteed to work properly in all cases.

In any case, I think the discussion about the options to enable is a bit orthogonal, I would:

• first make sure we have a specific ObjectMapper for both clients (and an identical one for both).

I could do that if no-one objects (next weekend, or maybe depending on result of the second point later?).

• second, we should probably discuss with the Keycloak team if being more lenient in these ObjectMappers is a good idea.

This change would be breaking one as till now user could customize KC ObjectMapper (reactive JVM | native & classic JVM). I am not familiar with KC team, could maybe @sberyozkin do it and afterwards the first point should be addressed?

I can imagine user want to be lenient to communication with an external interface that can change, but KC admin client failing over missing property is a good thing (valuable information, they still can work around it as I explained in #29035).