resteasy-reactive Interceptors don't get executed after Authorization failure

[javanegmond]

Describe the bug

When using quarkus-resteasy-reactive, Interceptors on the resource method are not executed when authorization has not passed (for instance due to use of @RolesAllowed). This is different from quarkus-resteasy, where such Interceptors do get executed.

Our use case for this is to log some additional information of the resource method. Example are annotations on the resource method.

Expected behavior

Interceptors get executed.

Actual behavior

Interceptors do not get executed.

How to Reproduce?

Please see attached reproducer.
I copied the @Logged interceptor example and slightly adjusted it. You can run the unit test to see it does not log anything from LoggedInterceptor on the endpoint with @RolesAllowed. Change the dependency from quarkus-resteasy-reactive to quarkus-resteasy and you will see the logging as expected.

reactive-interceptor.zip

Output of uname -a or ver

Linux 5.4.0-144-generic

Output of java -version

openjdk version "17.0.6" 2023-01-17

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.16.4.Final

Build tool (ie. output of mvnw --version or gradlew --version)

Apache Maven 3.6.3

Additional information

I'm not sure if this is intended behavior. However, it is most definitely a notable difference that is not mentioned on the migration page[1].
Similarly, I've found that also @ServerRequestFilter annotated methods and ContainerRequestFilter implementations do not get called. However, ContainerResponseFilter do get called. I have not checked @ServerResponseFilter.

[1]https://quarkus.io/guides/resteasy-reactive-migration

[quarkus-bot]

/cc @FroMage (resteasy-reactive), @Sgitario (resteasy-reactive), @geoand (resteasy-reactive), @stuartwdouglas (resteasy-reactive)

[geoand]

@michalvavrik is this something you are aware of? Is it expected?

[michalvavrik]

@michalvavrik is this something you are aware of? Is it expected?

Yes @geoand , it's expected, it (peforming security checks by ServerRestHandler rather than interceptors) fixed a lot of issues, please see here #19598. We had users with similar use cases and suggested Vert.X failure handler https://quarkus.io/guides/security-built-in-authentication-support-concept#how-to-customize-authentication-exception-responses. You just do your logging and call event.next(), no response customization.

[michalvavrik]

Not sure where to document it, maybe migration page?

[geoand]

Why not add it to RESTEasy Reactive reference doc?

[michalvavrik]

Sure. I'm terribly busy this week. Unless you want to do it, I'll do it next week.

@javanegmond is doc reference I provided and hint event.next() sufficient for you to make it work? You might need to play with handler priority depending on proactive authentication (enabled/disabled).

[geoand]

Sure. I'm terribly busy this week. Unless you want to do it, I'll do it next week.

No problem at all

[javanegmond]

We had users with similar use cases and suggested Vert.X failure handler https://quarkus.io/guides/security-built-in-authentication-support-concept#how-to-customize-authentication-exception-responses. You just do your logging and call event.next(), no response customization.

While I think that will work (will have to try later), I don't think it's a good solution.
An Interceptor in quarkus-resteasy is run regardless of whether or not authentication/authorization has passed. Using a Failure handler like that as a workaround for quarkus-resteasy-reactive leads to code paths that read unexpected, and therefore score low on maintainability. It would lead to two disjoint code paths being responsible for the same logic.

[michalvavrik]

While I think that will work (will have to try later), I don't think it's a good solution. An Interceptor in quarkus-resteasy is run regardless of whether or not authentication/authorization has passed. Using a Failure handler like that as a workaround for quarkus-resteasy-reactive leads to code paths that read unexpected, and therefore score low on maintainability. It would lead to two disjoint code paths being responsible for the same logic.

I hear you, but authentication checks should always run first and running CDI interceptors defined by users after failed checks seems wrong to me. There should be better way to provide logging so that user logging don't have to differ between auth failure/successful request.

[javanegmond]

I hear you, but authentication checks should always run first and running CDI interceptors defined by users after failed checks seems wrong to me. There should be better way to provide logging so that user logging don't have to differ between auth failure/successful request.

For auditibility, we are required to log some details (other than the endpoint, which I assume we can just enable some logging for) about the resource being called, regardless of failed auth, how else would you propose to do this than an interceptor?
FWIW: in our use case we do have a custom IdentityProvider and AuthenticationMechanism. I would be perfectly fine to do such actions in there, but I haven't found a way to access annotations on the resource method from there.

I do hear you as well; especially auth checks should always run first. Deserialization of input should probably also not happen if auth checks fail. This creates a bit of a conundrum I guess.

[michalvavrik]

I hear you, but authentication checks should always run first and running CDI interceptors defined by users after failed checks seems wrong to me. There should be better way to provide logging so that user logging don't have to differ between auth failure/successful request.

For auditibility, we are required to log some details (other than the endpoint, which I assume we can just enable some logging for) about the resource being called, regardless of failed auth, how else would you propose to do this than an interceptor?

I'd expect to have one way to log things, but I'm pretty sure this is not related to the security. There can be exception during serialization or thrown by any other filter/handler and you won't have these logged by your interceptor either. It's much more general problem. I suggest you discuss it on Zulip with developers or open issue as I won't be very helpful here.

FWIW: in our use case we do have a custom IdentityProvider and AuthenticationMechanism. I would be perfectly fine to do such actions in there, but I haven't found a way to access annotations on the resource method from there.

You can't access invoked method from there, you can work with request info as uri, query params and security identity. I think when there is sent challenge into your custom auth mechanism, you can get user from RoutingContext.

I do hear you as well; especially auth checks should always run first. Deserialization of input should probably also not happen if auth checks fail. This creates a bit of a conundrum I guess.

Yep.

[michalvavrik]

https://quarkus.io/support/

[javanegmond]

We had users with similar use cases and suggested Vert.X failure handler https://quarkus.io/guides/security-built-in-authentication-support-concept#how-to-customize-authentication-exception-responses. You just do your logging and call event.next(), no response customization.

While I think that will work (will have to try later), I don't think it's a good solution. An Interceptor in quarkus-resteasy is run regardless of whether or not authentication/authorization has passed. Using a Failure handler like that as a workaround for quarkus-resteasy-reactive leads to code paths that read unexpected, and therefore score low on maintainability. It would lead to two disjoint code paths being responsible for the same logic.

I just had some time to toy around with a vertx failure handler. Added a failure handler like in the example given in the documentation that was linked. I used the previously provided reproducer as a base. While I observe the init being called, the handler implementation ever gets called in this example. Besides that, even if it does get called, I didn't immediately see a way to obtain the resource method from the RouteContext.

I would like to clarify that this all regards an authorization failure, not an authentication failure. Since the authorization is 'configured' (for lack of a better term) via the @RolesAllowed annotation, the resource method is most definitely determinable. I feel like there should be a way to act on it, even if not calling the actual resource method (and not deserializing the request etc).

I have found a workaround that will likely be acceptable for our usecase, using a ContainerResponseFilter. It's not ideal: In the case of successful execution I would prefer the intended logging to happen before any logging in the business method. But alas, we take what we can get and using trace/spanIds we can correlate the logging regardless.

https://quarkus.io/support/

I fail to see your intent with the reply of just a support URL.

[michalvavrik]

We had users with similar use cases and suggested Vert.X failure handler https://quarkus.io/guides/security-built-in-authentication-support-concept#how-to-customize-authentication-exception-responses. You just do your logging and call event.next(), no response customization.

While I think that will work (will have to try later), I don't think it's a good solution. An Interceptor in quarkus-resteasy is run regardless of whether or not authentication/authorization has passed. Using a Failure handler like that as a workaround for quarkus-resteasy-reactive leads to code paths that read unexpected, and therefore score low on maintainability. It would lead to two disjoint code paths being responsible for the same logic.

I just had some time to toy around with a vertx failure handler. Added a failure handler like in the example given in the documentation that was linked. I used the previously provided reproducer as a base. While I observe the init being called, the handler implementation ever gets called in this example. Besides that, even if it does get called, I didn't immediately see a way to obtain the resource method from the RouteContext.

You can't access resource method from RoutingContext as it happens before that method is even matched, you need to rely on path.

I would like to clarify that this all regards an authorization failure, not an authentication failure. Since the authorization is 'configured' (for lack of a better term) via the @RolesAllowed annotation, the resource method is most definitely determinable. I feel like there should be a way to act on it, even if not calling the actual resource method (and not deserializing the request etc).

Yes and no. You will need to write extra code that Quarkus currently don't contain to match annotation instance over resource method with request match as we don't create separate security check for each instance (I mean, you can use @RolesAllowed("admin") over numerous methods and we want to create just one check, therefore the binding is not available at the time of failure).

I feel like ^^ is not helpful for you, but you should be able to uniquely identify resource method based on path, so that's all currently Quarkus offer.

https://quarkus.io/support/

I fail to see your intent with the reply of just a support URL.

You opened this issue for authorization issue, but logging failures before CDI interceptors run concerns more than authorization security check failure as I tried to establish above - if incorrect JSON object can't be deserialized, your interceptor won't catch it, but for audibility you will want to know it, won't you? I said in the comment above that I'm not right person to offer opinion and proposed you discuss it with developers. Then realized you might not know what I meant and provided the URL.

[michalvavrik]

I just had some time to toy around with a vertx failure handler. Added a failure handler like in the example given in the documentation that was linked. I used the previously provided reproducer as a base. While I observe the init being called, the handler implementation ever gets called in this example.

Let's push it somewhere and I'll check, thanks.

[javanegmond]

Let's push it somewhere and I'll check, thanks.

reactive-interceptor-with-failure-handler.zip
Here you go.

You can't access resource method from RoutingContext as it happens before that method is even matched, you need to rely on path.
I suspect this is related to why the failure handler in above example is never reached? In any case, this goes beyond the initial issue I guess.

https://quarkus.io/support/

I fail to see your intent with the reply of just a support URL.

You opened this issue for authorization issue, but logging failures before CDI interceptors run concerns more than authorization security check failure as I tried to establish above - if incorrect JSON object can't be deserialized, your interceptor won't catch it, but for audibility you will want to know it, won't you? I said in the comment above that I'm not right person to offer opinion and proposed you discuss it with developers. Then realized you might not know what I meant and provided the URL.

Copy. I hadn't really thought about what would happen with deserialization failures, but that does sound about right. This suggests to me that my workaround using a ResponseFilter is the most feasible solution for the time being.

I would like to clarify that this all regards an authorization failure, not an authentication failure. Since the authorization is 'configured' (for lack of a better term) via the @RolesAllowed annotation, the resource method is most definitely determinable. I feel like there should be a way to act on it, even if not calling the actual resource method (and not deserializing the request etc).

Yes and no. You will need to write extra code that Quarkus currently don't contain to match annotation instance over resource method with request match as we don't create separate security check for each instance (I mean, you can use @RolesAllowed("admin") over numerous methods and we want to create just one check, therefore the binding is not available at the time of failure).

I feel like ^^ is not helpful for you, but you should be able to uniquely identify resource method based on path, so that's all currently Quarkus offer.

I feel like this goes a bit beyond the context of this issue maybe, but how do you want to do just one check if @RolesAllowed happens over numerous methods? You can't possibly evaluate the whole call tree beforehand right? Or maybe I'm just misunderstanding you on this.

In any case, I feel like it would be great to document this difference in behavior of quarkus-resteasy and quarkus-resteasy-reactive. After this whole interaction I do think that the resteasy-reactive behavior is the "more correct" implementation.

[michalvavrik]

reactive-interceptor-with-failure-handler.zip Here you go.

I see, the docs URL I provided document authentication failures with proactive authentication, when authorization check you force by @RolesAllowed is performed, RR ends response, so your failure handle won't get called. Only way would be to create custom exception mapper (also documented there) that catches UnauthorizedException and ForbiddenException (as documented, we try to discourage it, please see reasons for this in docs). Sorry, my bad.

I feel like this goes a bit beyond the context of this issue maybe, but how do you want to do just one check if @RolesAllowed happens over numerous methods? You can't possibly evaluate the whole call tree beforehand right? Or maybe I'm just misunderstanding you on this.

I just meant that we simply don't keep the information about resource methods for which the roles allowed security check is performed as the same class instance is used to secure multiple resource methods (if they use same annotation value).

In any case, I feel like it would be great to document this difference in behavior of quarkus-resteasy and quarkus-resteasy-reactive. After this whole interaction I do think that the resteasy-reactive behavior is the "more correct" implementation.

Sure, the docs need to be improved. Will do it next week (or week after that, busy :-)).

[michalvavrik]

This suggests to me that my workaround using a ResponseFilter is the most feasible solution for the time being.

glad it works for you