Introduce a version of @OidcClientFilter for the reactive client

[geoand]

Closes: #16522

[sberyozkin]

Oh wow, just stepped outside for a late lunch and the issue is already resolved :-), thanks @geoand, that should be useful for Resteasy Reactive users for sure.

I'd only like to wait for @rsvoboda for us to discuss how to handle this @OidcClientFilter, I think just having a package name changed is OK, very minor migration effort, I was considering pushing it down to quarkus-oidc-client where it would be misplaced or creating oidc-client-filter-common which seems unnecessary, and it would not solve a similar problem for oidc-token-propagation-reactive, so it look fine to me, @rsvoboda are you OK with only a package name having to be changed for @OidcClientFilter ? As discussed separately, it is likely one can expect some minor migration effort anyway for some moderate complexity REST applications, so the annotation package name change for this annotation is not a problem I believe.

[sberyozkin]

I'll follow up next week with another PR to support a similar mechanism for oidc-token-propagation-reactive

[geoand]

@sberyozkin I agree with your concern, so let's wait for the discussion before merging

[sberyozkin]

@geoand Can you please update https://quarkus.io/guides/security-openid-connect-client-reference#oidc-client-reactive-filter a bit, when you'll have a few mins, it currently says , and it currently can only be registered with org.eclipse.microprofile.rest.client.annotation.RegisterProvider annotation: followed by one example, but you can just copy the block from the section below which has You can selectively register OidcClientRequestFilter by using either io.quarkus.oidc.client.filter.OidcClientFilter or org.eclipse.microprofile.rest.client.annotation.RegisterProvider annotations: followed by two examples and change the package and then we can tweak it a bit as needed.

[geoand]

@sberyozkin good point. Done

[sberyozkin]

@geoand Good stuff, thanks

[quarkus-bot]

Failing Jobs - Building 6a9327a

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

[rsvoboda]

Personally I would prefer creating oidc-client-filter-common to have the same FQCN for OidcClientFilter.
This way migration would be transparent for the end users, they would just update the dependencies (or cli update command one day). The is also oidc-token-propagation extension with AccessToken annotation.

Maybe there could be oidc-common or oidc-annotations which would include both io.quarkus.oidc.token.propagation.AccessToken and io.quarkus.oidc.client.filter.OidcClientFilter.

[sberyozkin]

@rsvoboda We already have oidc-common but I'm not sure yet we need to move these annotations down to it, I mean, if a user is switching to the reactive client filter, they may as well change their REST service methods to return Uni<String> instead of String, see what I mean, the migration is very likely to happen anyway...
Moving them to oidc-common is an option but I'm hesitating a bit...

[sberyozkin]

oidc-token-propagation does not depend on any of oidc dependencies as it just propagates the tokens - they may come from smallrye-jwt, so if only @OidcClientFilter were to be moved then I guess it would be better if it landed in oidc-client, that would still leave the option of introducing some other common module at some later stage but for now it is not obvious it is necessary.
Lets see early next week :-)

[geoand]

I think what @rsvoboda is proposing is to move the annotation to a new module, that will then be shared by both old and new (reactive) implementations.

I personally think that makes sense.

[sberyozkin]

Hi @geoand Sure, I think for now I can move it to quarkus-oidc-client as both quarkus-oidc-client-filter and quarkus-oidc-client-reactive-filter depend on it. We also have quarkus-oidc-common which quarkus-oidc-client and quarkus-oidc depend upon, but I'm not sure about moving it to quarkus-oidc-common as another pair of extensions where such a transition will currently require the package name update is quarkus-oidc-token-propagation and quarkus-oidc-token-propagation-reactive - but these extensions don't depend on quarkus-oidc-common.

So moving @OidcClientFilter to quarkus-oidc-client will address the specific concerns from Rostislav, while will keep the option open for quarkus-oidc-client-common if we decide to do something similar for quarkus-oidc-token-propagation - or may be for the quarkus-oidc-token-propagation to quarkus-oidc-token-propagation-reactve transition we can push the related annotations to the already existing quarkus-security.

[geoand]

@sberyozkin +1.

I propose merging this for now and then you can handle the modules as you see fit. Does that sound good?

[sberyozkin]

@geoand Sure, sounds good, I was just about to check out your branch but yeah, would be simpler to follow up, I'll open a new issue/PR straight afterwards, thanks

[geoand]

🎉