spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.81k stars 5.9k forks source link

@EnableReactiveMethodSecurity does not support ResponseEntity<Publisher<T>> as return type #14731

Open elkhart opened 8 months ago

elkhart commented 8 months ago

According to the Webflux documentation it is allowed to use the return type ResponseEntity<Mono<T>>

Neither AuthorizationManagerBeforeReactiveMethodInterceptor nor AuthorizationManagerAfterReactiveMethodInterceptor support this.

Using a method signature with the described return type causes an java.lang.IllegalStateException like

java.lang.IllegalStateException: The returnType class org.springframework.http.ResponseEntity on public org.springframework.http.ResponseEntity example.ExampleController.getSomething() must return an instance of org.reactivestreams.Publisher (for example, a Mono or Flux) in order to support Reactor Context
    at org.springframework.util.Assert.state(Assert.java:97)

Maybe a little bit of context:
We're using a code generator which produces such method signatures causing the exception whenever the return type is a list of something.
fun getSomething(): ResponseEntity<Flow<SomeDTO>>
We also found a workaround by adjusting the generator-based template but this is rather really just a workaround hence this ticket.

To Reproduce Use simple project with spring-security 6.x and have @EnableReactiveMethodSecurity configured. Add @PreAuthorize to a controller method with the return type ResponseEntity<Mono<T>> with T being some arbitrary DTO class. Run the server, call the endpoint and you should see the mentioned exception.

Expected behavior @EnableReactiveMethodSecurity should allow all valid return types defined for Webflux

Sample Don't have one yet.

kse-music commented 8 months ago

According to the Spring Security documentation , the return type of the method must be a org.reactivestreams.Publisher (that is, a Mono or a Flux).

elkhart commented 7 months ago

According to the Spring Security documentation , the return type of the method must be a org.reactivestreams.Publisher (that is, a Mono or a Flux).

Hey @kse-music, thanks for the note and the link. I can see the

  1. Supports reactive return types.

item. Are you suggesting that I should change this from bug to enhancement? Guess I'll just do that. Question is if somebody is interested and how hard would it actually be? Any thoughts?

Just a question aside: I saw that with spring-security 6.2 suspend methods are also considered again. Wouldn't it make sense to rephrase the 2. bullet point especially

Note that we are waiting on additional coroutine support from the Spring Framework before adding coroutine support.

jzheaux commented 7 months ago

@elkhart, thanks for the idea and @kse-music for the PR.

I think it makes sense to be able to add Spring Security method annotations to return types that Spring Web supports. That said, we don't want to add references to spring-web into spring-security-core.

It may work to register a ReactiveAdapter to the ReactiveAdapterRegistry that extracts the body from the Publisher, though this will require some research, so I'm just noting it for now.

In the meantime, @elkhart, are you able to return a Mono<ResponseEntity<T>> instead?

jzheaux commented 7 months ago

The reason that the reactive method interceptors require that the return type be reactive is to ensure that the security context gets propagated to the authorization manager for evaluation. Without this, the method invocation itself may not have access to the security context either.

I imagine that until https://github.com/spring-projects/spring-security/issues/7594 is addressed, it won't be feasible to use reactive authorization components on a method that returns a non-reactive result (even though that value is wrapping a reactive result).

I will leave this ticket open for now until #7594 is resolved and we can go from there.