From 262e13d4bd84b04e11a42068b087b86b3292669c Mon Sep 17 00:00:00 2001 From: Kamil Krzywannski Date: Sat, 11 Jul 2026 16:12:14 +0200 Subject: [PATCH] Adapt plain values in reactive method security Kotlin suspend methods with @Cacheable can return a plain cached value on a cache hit. Reactive method security assumed proceed() always returned a Mono or Flux, which caused a ClassCastException. Normalize proceed() results so plain values are adapted to Mono or Flux. Closes gh-19400 Signed-off-by: Kamil Krzywannski --- ...rePostAdviceReactiveMethodInterceptor.java | 35 ++++-- ...linReactiveMethodSecurityCacheableTests.kt | 110 ++++++++++++++++++ ...ManagerAfterReactiveMethodInterceptor.java | 14 +-- ...anagerBeforeReactiveMethodInterceptor.java | 6 +- .../method/ReactiveMethodInvocationUtils.java | 49 ++++++++ ...erAfterReactiveMethodInterceptorTests.java | 28 +++++ ...rBeforeReactiveMethodInterceptorTests.java | 46 ++++++++ 7 files changed, 263 insertions(+), 25 deletions(-) create mode 100644 config/src/test/kotlin/org/springframework/security/config/annotation/method/configuration/KotlinReactiveMethodSecurityCacheableTests.kt diff --git a/access/src/main/java/org/springframework/security/access/prepost/PrePostAdviceReactiveMethodInterceptor.java b/access/src/main/java/org/springframework/security/access/prepost/PrePostAdviceReactiveMethodInterceptor.java index 9befbb01433..aa8232658a5 100644 --- a/access/src/main/java/org/springframework/security/access/prepost/PrePostAdviceReactiveMethodInterceptor.java +++ b/access/src/main/java/org/springframework/security/access/prepost/PrePostAdviceReactiveMethodInterceptor.java @@ -118,18 +118,17 @@ public Object invoke(final MethodInvocation invocation) { // @formatter:on PostInvocationAttribute attr = findPostInvocationAttribute(attributes); if (Mono.class.isAssignableFrom(returnType)) { - return toInvoke.flatMap((auth) -> PrePostAdviceReactiveMethodInterceptor.>proceed(invocation) + return toInvoke.flatMap((auth) -> proceedAsMono(invocation) .map((r) -> (attr != null) ? this.postAdvice.after(auth, invocation, attr, r) : r)); } if (Flux.class.isAssignableFrom(returnType)) { - return toInvoke.flatMapMany((auth) -> PrePostAdviceReactiveMethodInterceptor.>proceed(invocation) + return toInvoke.flatMapMany((auth) -> proceedAsFlux(invocation) .map((r) -> (attr != null) ? this.postAdvice.after(auth, invocation, attr, r) : r)); } if (hasFlowReturnType) { if (isSuspendingFunction) { - return toInvoke - .flatMapMany((auth) -> Flux.from(PrePostAdviceReactiveMethodInterceptor.proceed(invocation)) - .map((r) -> (attr != null) ? this.postAdvice.after(auth, invocation, attr, r) : r)); + return toInvoke.flatMapMany((auth) -> proceedAsFlux(invocation) + .map((r) -> (attr != null) ? this.postAdvice.after(auth, invocation, attr, r) : r)); } else { ReactiveAdapter adapter = ReactiveAdapterRegistry.getSharedInstance().getAdapter(returnType); @@ -141,18 +140,32 @@ public Object invoke(final MethodInvocation invocation) { return KotlinDelegate.asFlow(response); } } - return toInvoke.flatMap((auth) -> Mono.from(PrePostAdviceReactiveMethodInterceptor.proceed(invocation)) + return toInvoke.flatMap((auth) -> proceedAsMono(invocation) .map((r) -> (attr != null) ? this.postAdvice.after(auth, invocation, attr, r) : r)); } @SuppressWarnings("unchecked") - private static > @Nullable T proceed(final MethodInvocation invocation) { - try { - return (T) invocation.proceed(); + private static Mono proceedAsMono(MethodInvocation invocation) { + Object result = flowProceed(invocation); + if (result instanceof Mono mono) { + return (Mono) mono; } - catch (Throwable throwable) { - throw Exceptions.propagate(throwable); + if (result instanceof Publisher publisher) { + return Mono.from(publisher); + } + return Mono.justOrEmpty(result); + } + + @SuppressWarnings("unchecked") + private static Flux proceedAsFlux(MethodInvocation invocation) { + Object result = flowProceed(invocation); + if (result instanceof Flux flux) { + return (Flux) flux; + } + if (result instanceof Publisher publisher) { + return Flux.from(publisher); } + return (result != null) ? Flux.just(result) : Flux.empty(); } private static @Nullable Object flowProceed(final MethodInvocation invocation) { diff --git a/config/src/test/kotlin/org/springframework/security/config/annotation/method/configuration/KotlinReactiveMethodSecurityCacheableTests.kt b/config/src/test/kotlin/org/springframework/security/config/annotation/method/configuration/KotlinReactiveMethodSecurityCacheableTests.kt new file mode 100644 index 00000000000..6835c189f81 --- /dev/null +++ b/config/src/test/kotlin/org/springframework/security/config/annotation/method/configuration/KotlinReactiveMethodSecurityCacheableTests.kt @@ -0,0 +1,110 @@ +/* + * Copyright 2004-present the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.security.config.annotation.method.configuration + +import kotlinx.coroutines.runBlocking +import org.assertj.core.api.Assertions.assertThat +import org.junit.jupiter.api.BeforeEach +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.extension.ExtendWith +import org.springframework.beans.factory.annotation.Autowired +import org.springframework.cache.CacheManager +import org.springframework.cache.annotation.Cacheable +import org.springframework.cache.annotation.EnableCaching +import org.springframework.cache.concurrent.ConcurrentMapCacheManager +import org.springframework.context.annotation.Bean +import org.springframework.context.annotation.Configuration +import org.springframework.security.access.prepost.PreAuthorize +import org.springframework.security.test.context.support.WithMockUser +import org.springframework.test.context.ContextConfiguration +import org.springframework.test.context.junit.jupiter.SpringExtension +import java.util.concurrent.atomic.AtomicInteger + +// gh-19400 +@ExtendWith(SpringExtension::class) +@ContextConfiguration +class KotlinReactiveMethodSecurityCacheableTests { + + @Autowired + lateinit var cachedService: CachedSuspendService + + @Autowired + lateinit var cacheManager: CacheManager + + @BeforeEach + fun setUp() { + cacheManager.getCache("demo")?.clear() + cachedService.resetCounter() + } + + @Test + @WithMockUser(authorities = ["demo.read"]) + fun `suspending PreAuthorize and Cacheable when cache hit then returns cached value`() { + runBlocking { + val first = cachedService.demo() + val second = cachedService.demo() + assertThat(first).isEqualTo(second) + assertThat(first.invocation).isEqualTo(1) + assertThat(cachedService.invocationCount()).isEqualTo(1) + } + } + + @Test + @WithMockUser(authorities = ["demo.read"]) + fun `suspending PreAuthorize and Cacheable when cache miss then invokes target`() { + runBlocking { + val result = cachedService.demo() + assertThat(result.invocation).isEqualTo(1) + assertThat(result.message).isEqualTo("cached response") + assertThat(cachedService.invocationCount()).isEqualTo(1) + } + } + + @Configuration + @EnableReactiveMethodSecurity + @EnableCaching + open class Config { + @Bean + open fun cacheManager(): CacheManager = ConcurrentMapCacheManager("demo") + + @Bean + open fun cachedService(): CachedSuspendService = CachedSuspendService() + } +} + +open class CachedSuspendService { + private val invocationCounter = AtomicInteger() + + @Cacheable("demo", key = "'fixed'") + @PreAuthorize("hasAuthority('demo.read')") + open suspend fun demo(): DemoResponse = + DemoResponse( + message = "cached response", + invocation = invocationCounter.incrementAndGet(), + ) + + open fun invocationCount(): Int = invocationCounter.get() + + open fun resetCounter() { + invocationCounter.set(0) + } +} + +data class DemoResponse( + val message: String, + val invocation: Int, +) diff --git a/core/src/main/java/org/springframework/security/authorization/method/AuthorizationManagerAfterReactiveMethodInterceptor.java b/core/src/main/java/org/springframework/security/authorization/method/AuthorizationManagerAfterReactiveMethodInterceptor.java index fe5e2613b8a..35ef1815e79 100644 --- a/core/src/main/java/org/springframework/security/authorization/method/AuthorizationManagerAfterReactiveMethodInterceptor.java +++ b/core/src/main/java/org/springframework/security/authorization/method/AuthorizationManagerAfterReactiveMethodInterceptor.java @@ -137,11 +137,7 @@ public Object invoke(MethodInvocation mi) throws Throwable { ReactiveAdapter adapter = ReactiveAdapterRegistry.getSharedInstance().getAdapter(type); if (hasFlowReturnType) { if (isSuspendingFunction) { - Publisher publisher = ReactiveMethodInvocationUtils.proceed(mi); - if (publisher == null) { - return Flux.empty(); - } - return Flux.from(publisher).materialize().flatMap(postAuthorize); + return ReactiveMethodInvocationUtils.proceedAsFlux(mi).materialize().flatMap(postAuthorize); } else { Assert.state(adapter != null, () -> "The returnType " + type + " on " + method @@ -152,15 +148,11 @@ public Object invoke(MethodInvocation mi) throws Throwable { return KotlinDelegate.asFlow(response); } } - Publisher publisher = ReactiveMethodInvocationUtils.proceed(mi); - if (publisher == null) { - return Flux.empty(); - } if (isMultiValue(type, adapter)) { - Flux flux = Flux.from(publisher).materialize().flatMap(postAuthorize); + Flux flux = ReactiveMethodInvocationUtils.proceedAsFlux(mi).materialize().flatMap(postAuthorize); return (adapter != null) ? adapter.fromPublisher(flux) : flux; } - Mono mono = Mono.from(publisher).materialize().flatMap(postAuthorize); + Mono mono = ReactiveMethodInvocationUtils.proceedAsMono(mi).materialize().flatMap(postAuthorize); return (adapter != null) ? adapter.fromPublisher(mono) : mono; } diff --git a/core/src/main/java/org/springframework/security/authorization/method/AuthorizationManagerBeforeReactiveMethodInterceptor.java b/core/src/main/java/org/springframework/security/authorization/method/AuthorizationManagerBeforeReactiveMethodInterceptor.java index 4c5801d8785..0a01fe3e999 100644 --- a/core/src/main/java/org/springframework/security/authorization/method/AuthorizationManagerBeforeReactiveMethodInterceptor.java +++ b/core/src/main/java/org/springframework/security/authorization/method/AuthorizationManagerBeforeReactiveMethodInterceptor.java @@ -121,7 +121,7 @@ public Object invoke(MethodInvocation mi) throws Throwable { ReactiveAdapter adapter = ReactiveAdapterRegistry.getSharedInstance().getAdapter(type); if (hasFlowReturnType) { if (isSuspendingFunction) { - return preAuthorized(mi, Flux.defer(() -> ReactiveMethodInvocationUtils.proceed(mi))); + return preAuthorized(mi, Flux.defer(() -> ReactiveMethodInvocationUtils.proceedAsFlux(mi))); } else { Assert.state(adapter != null, () -> "The returnType " + type + " on " + method @@ -132,10 +132,10 @@ public Object invoke(MethodInvocation mi) throws Throwable { } } if (isMultiValue(type, adapter)) { - Flux result = preAuthorized(mi, Flux.defer(() -> ReactiveMethodInvocationUtils.proceed(mi))); + Flux result = preAuthorized(mi, Flux.defer(() -> ReactiveMethodInvocationUtils.proceedAsFlux(mi))); return (adapter != null) ? adapter.fromPublisher(result) : result; } - Mono result = preAuthorized(mi, Mono.defer(() -> ReactiveMethodInvocationUtils.proceed(mi))); + Mono result = preAuthorized(mi, Mono.defer(() -> ReactiveMethodInvocationUtils.proceedAsMono(mi))); return (adapter != null) ? adapter.fromPublisher(result) : result; } diff --git a/core/src/main/java/org/springframework/security/authorization/method/ReactiveMethodInvocationUtils.java b/core/src/main/java/org/springframework/security/authorization/method/ReactiveMethodInvocationUtils.java index 1bbf8a199dc..2d47444d579 100644 --- a/core/src/main/java/org/springframework/security/authorization/method/ReactiveMethodInvocationUtils.java +++ b/core/src/main/java/org/springframework/security/authorization/method/ReactiveMethodInvocationUtils.java @@ -18,7 +18,10 @@ import org.aopalliance.intercept.MethodInvocation; import org.jspecify.annotations.Nullable; +import org.reactivestreams.Publisher; import reactor.core.Exceptions; +import reactor.core.publisher.Flux; +import reactor.core.publisher.Mono; /** * For internal use only, as this contract is likely to change. @@ -37,6 +40,52 @@ final class ReactiveMethodInvocationUtils { } } + /** + * Proceeds with the {@link MethodInvocation} and adapts the result to a {@link Mono}. + *

+ * Kotlin suspending functions are invoked by Spring AOP as {@link Mono}, but other + * advisors in the chain (for example, {@code @Cacheable} on a cache hit) may return a + * plain value. This method normalizes both cases so reactive method security can + * compose over the invocation result. + * @param mi the method invocation + * @return a {@link Mono} that emits the invocation result + */ + static Mono proceedAsMono(MethodInvocation mi) { + return toMono(proceed(mi)); + } + + /** + * Proceeds with the {@link MethodInvocation} and adapts the result to a {@link Flux}. + * @param mi the method invocation + * @return a {@link Flux} that emits the invocation result(s) + * @see #proceedAsMono(MethodInvocation) + */ + static Flux proceedAsFlux(MethodInvocation mi) { + return toFlux(proceed(mi)); + } + + @SuppressWarnings("unchecked") + private static Mono toMono(@Nullable Object result) { + if (result instanceof Mono mono) { + return (Mono) mono; + } + if (result instanceof Publisher publisher) { + return Mono.from(publisher); + } + return Mono.justOrEmpty(result); + } + + @SuppressWarnings("unchecked") + private static Flux toFlux(@Nullable Object result) { + if (result instanceof Flux flux) { + return (Flux) flux; + } + if (result instanceof Publisher publisher) { + return Flux.from(publisher); + } + return (result != null) ? Flux.just(result) : Flux.empty(); + } + private ReactiveMethodInvocationUtils() { } diff --git a/core/src/test/java/org/springframework/security/authorization/method/AuthorizationManagerAfterReactiveMethodInterceptorTests.java b/core/src/test/java/org/springframework/security/authorization/method/AuthorizationManagerAfterReactiveMethodInterceptorTests.java index 3c2358964de..1cfa8fb1039 100644 --- a/core/src/test/java/org/springframework/security/authorization/method/AuthorizationManagerAfterReactiveMethodInterceptorTests.java +++ b/core/src/test/java/org/springframework/security/authorization/method/AuthorizationManagerAfterReactiveMethodInterceptorTests.java @@ -254,6 +254,26 @@ public void invokeWhenCustomAuthorizationDeniedExceptionThenThrows() throws Thro .isThrownBy(() -> ((Mono) advice.invoke(mockMethodInvocation)).block()); } + // gh-19400 + @Test + public void invokeSuspendingWhenProceedReturnsPlainValueThenWrapsAsMono() throws Throwable { + MethodInvocation mockMethodInvocation = spy(new MockMethodInvocation(new Sample(), + Sample.class.getDeclaredMethod("suspending", kotlin.coroutines.Continuation.class))); + // Simulates @Cacheable cache hit returning a plain value instead of a Mono + given(mockMethodInvocation.proceed()).willReturn("cached"); + ReactiveAuthorizationManager mockReactiveAuthorizationManager = mock( + ReactiveAuthorizationManager.class); + given(mockReactiveAuthorizationManager.authorize(any(), any())) + .willReturn(Mono.just(new AuthorizationDecision(true))); + AuthorizationManagerAfterReactiveMethodInterceptor interceptor = new AuthorizationManagerAfterReactiveMethodInterceptor( + Pointcut.TRUE, mockReactiveAuthorizationManager); + Object result = interceptor.invoke(mockMethodInvocation); + assertThat(result).asInstanceOf(InstanceOfAssertFactories.type(Mono.class)) + .extracting(Mono::block) + .isEqualTo("cached"); + verify(mockReactiveAuthorizationManager).authorize(any(), any()); + } + private Object masking(InvocationOnMock invocation) { MethodInvocationResult result = invocation.getArgument(0); return result.getResult() + "-masked"; @@ -279,6 +299,14 @@ Flux flux() { return Flux.just("john", "bob"); } + /** + * JVM shape of a Kotlin {@code suspend} function (last parameter is + * {@link kotlin.coroutines.Continuation}). + */ + Object suspending(kotlin.coroutines.Continuation continuation) { + return null; + } + } static class MyAuthzDeniedException extends AuthorizationDeniedException { diff --git a/core/src/test/java/org/springframework/security/authorization/method/AuthorizationManagerBeforeReactiveMethodInterceptorTests.java b/core/src/test/java/org/springframework/security/authorization/method/AuthorizationManagerBeforeReactiveMethodInterceptorTests.java index 09b069302a6..dff2a217550 100644 --- a/core/src/test/java/org/springframework/security/authorization/method/AuthorizationManagerBeforeReactiveMethodInterceptorTests.java +++ b/core/src/test/java/org/springframework/security/authorization/method/AuthorizationManagerBeforeReactiveMethodInterceptorTests.java @@ -225,6 +225,44 @@ public void invokeWhenCustomAuthorizationDeniedExceptionThenThrows() throws Thro .isThrownBy(() -> ((Mono) advice.invoke(mockMethodInvocation)).block()); } + // gh-19400 + @Test + public void invokeSuspendingWhenProceedReturnsPlainValueThenWrapsAsMono() throws Throwable { + MethodInvocation mockMethodInvocation = spy(new MockMethodInvocation(new Sample(), + Sample.class.getDeclaredMethod("suspending", kotlin.coroutines.Continuation.class))); + // Simulates @Cacheable cache hit returning a plain value instead of a Mono + given(mockMethodInvocation.proceed()).willReturn("cached"); + ReactiveAuthorizationManager mockReactiveAuthorizationManager = mock( + ReactiveAuthorizationManager.class); + given(mockReactiveAuthorizationManager.authorize(any(), eq(mockMethodInvocation))) + .willReturn(Mono.just(new AuthorizationDecision(true))); + AuthorizationManagerBeforeReactiveMethodInterceptor interceptor = new AuthorizationManagerBeforeReactiveMethodInterceptor( + Pointcut.TRUE, mockReactiveAuthorizationManager); + Object result = interceptor.invoke(mockMethodInvocation); + assertThat(result).asInstanceOf(InstanceOfAssertFactories.type(Mono.class)) + .extracting(Mono::block) + .isEqualTo("cached"); + verify(mockReactiveAuthorizationManager).authorize(any(), eq(mockMethodInvocation)); + } + + // gh-19400 + @Test + public void invokeSuspendingWhenProceedReturnsMonoThenUsesMono() throws Throwable { + MethodInvocation mockMethodInvocation = spy(new MockMethodInvocation(new Sample(), + Sample.class.getDeclaredMethod("suspending", kotlin.coroutines.Continuation.class))); + given(mockMethodInvocation.proceed()).willReturn(Mono.just("from-mono")); + ReactiveAuthorizationManager mockReactiveAuthorizationManager = mock( + ReactiveAuthorizationManager.class); + given(mockReactiveAuthorizationManager.authorize(any(), eq(mockMethodInvocation))) + .willReturn(Mono.just(new AuthorizationDecision(true))); + AuthorizationManagerBeforeReactiveMethodInterceptor interceptor = new AuthorizationManagerBeforeReactiveMethodInterceptor( + Pointcut.TRUE, mockReactiveAuthorizationManager); + Object result = interceptor.invoke(mockMethodInvocation); + assertThat(result).asInstanceOf(InstanceOfAssertFactories.type(Mono.class)) + .extracting(Mono::block) + .isEqualTo("from-mono"); + } + interface HandlingReactiveAuthorizationManager extends ReactiveAuthorizationManager, MethodAuthorizationDeniedHandler { @@ -240,6 +278,14 @@ Flux flux() { return Flux.just("john", "bob"); } + /** + * JVM shape of a Kotlin {@code suspend} function (last parameter is + * {@link kotlin.coroutines.Continuation}). + */ + Object suspending(kotlin.coroutines.Continuation continuation) { + return null; + } + } static class MyAuthzDeniedException extends AuthorizationDeniedException {