Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AuthorizationManager[Before/After]ReactiveMethodInterceptor doesn't support Kotlin coroutines #12080

Closed
Tracked by #11335
jarias opened this issue Oct 25, 2022 · 6 comments
Assignees
Labels
in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement
Milestone

Comments

@jarias
Copy link

jarias commented Oct 25, 2022

Expected Behavior

AuthorizationManager[Before/After]ReactiveMethodInterceptor should suport Kotlin coroutines

Current Behavior

The now deprecated PrePostAdviceReactiveMethodInterceptor did support Kotlin coroutines but the newer counterparts don't

Context

At least when using a custom annotation with the new AuthorizationManager[Before/After]ReactiveMethodInterceptor like:

    @Bean
    @Role(BeanDefinition.ROLE_INFRASTRUCTURE)
    fun customAuthorize(): Advisor {
        val pointcut = Pointcuts.union(
            AnnotationMatchingPointcut(null, Can::class.java, true),
            AnnotationMatchingPointcut(Can::class.java, true)
        )
        val interceptor = AuthorizationManagerBeforeReactiveMethodInterceptor(
            pointcut,
            CanDoAuthorizationManager()
        )
        interceptor.order = AuthorizationInterceptorsOrder.PRE_AUTHORIZE.order + 1
        return interceptor
    }

I can't annotated suspended functions in Kotlin, looking at the deprecated PrePostAdviceReactiveMethodInterceptor coroutines functionality was added a while back, but looks like the same functionality wasn't ported to the new classes.

Workaround is to use mono {} to wrap the suspended functions and return Mono instead to satisfy

@jarias jarias added status: waiting-for-triage An issue we've not yet triaged type: enhancement A general enhancement labels Oct 25, 2022
@jarias jarias changed the title AuthorizationManager[Before/After]ReactiveMethodInterceptor don't support Kotlin coroutines AuthorizationManager[Before/After]ReactiveMethodInterceptor doesn't support Kotlin coroutines Oct 25, 2022
@jzheaux
Copy link
Contributor

jzheaux commented Dec 22, 2022

This is blocked until spring-projects/spring-framework#22462 is addressed.

See #9867 (review) for additional context.

@martinformi
Copy link

martinformi commented May 26, 2023

May I ask about the status? We made some effort to upgrade to Spring Boot 3.1.0 where reactive/suspend functions are heavily used (with @EnableReactiveMethodSecurity and @PreAuthorize annotations). I am afraid that the bug spring-projects/spring-framework#22462 will not be resolved soon. What could be a workaround here?

@F43nd1r
Copy link

F43nd1r commented Nov 3, 2023

Any updates here? The underlying issue spring-projects/spring-framework#22462 is closed since august

@sjohnr
Copy link
Member

sjohnr commented Nov 8, 2023

No updates @martinformi @F43nd1r. I opened gh-13770 with a list of the classes that require enhancements to support this, but I just closed it as a duplicate of this issue. If anyone is interested in contributing the enhancements, it would be most welcome.

@F43nd1r
Copy link

F43nd1r commented Nov 8, 2023

@sjohnr took a look at the code of AuthorizationManagerBeforeReactiveMethodInterceptor as an example.

However, to me it looks like there are competing concepts in the code:
On the one hand, there's a check which enforces the return type to extend Publisher, meaning only Mono, Flux and friends are supported.
On the other hand, there's a ReactiveAdapterRegistry after that check, which could convert other return types (e.g. Flow) to Mono or Flux.

Also, the ReactiveAdapterRegistry identifies adapters by return type, which is fundamentally incompatible with suspend functions as they only return Object and instead should be identified by the last parameter being a Continuation.

Am I misunderstanding what the ReactiveAdapterRegistry is for? Could you give some pointers how the team expects this to be implemented?

@sjohnr
Copy link
Member

sjohnr commented Nov 8, 2023

@F43nd1r it's tricky indeed. I am not an expert on reactive types so I had to work it out as well.

The support in PrePostAdviceReactiveMethodInterceptor (@EnableReactiveMethodSecurity(useAuthorizationManager = false)) has already been updated with gh-13764 via 92256f0. I believe the updates we're discussing here would be similar. The main difference being that the newer classes don't have any support for suspend so it needs to be added, whereas the older class was only missing support for nested suspend calls.

@jzheaux jzheaux self-assigned this Nov 14, 2023
@jzheaux jzheaux added in: web An issue in web modules (web, webmvc) and removed status: blocked An issue that's blocked on an external project change labels Nov 14, 2023
@jzheaux jzheaux added this to the 6.2.0 milestone Nov 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement
Projects
Status: Done
Development

No branches or pull requests

5 participants