From 59f6c3dd0e25eab1f3075d8eddbf9f4db613f9e1 Mon Sep 17 00:00:00 2001 From: yybmion Date: Tue, 5 Aug 2025 18:02:01 +0900 Subject: [PATCH] Warning when EnableTransactionManagement has lower precedence than EnableMethodSecurity Warns when transaction management has same or lower precedence to prevent issues with @PostAuthorize rollback on methods with side effects. Fixes: #17544 Signed-off-by: yybmion --- .../PrePostMethodSecurityConfiguration.java | 66 +++++++- ...ePostMethodSecurityConfigurationTests.java | 157 ++++++++++++++++++ 2 files changed, 222 insertions(+), 1 deletion(-) diff --git a/config/src/main/java/org/springframework/security/config/annotation/method/configuration/PrePostMethodSecurityConfiguration.java b/config/src/main/java/org/springframework/security/config/annotation/method/configuration/PrePostMethodSecurityConfiguration.java index 88e3eae23b..c788fe0939 100644 --- a/config/src/main/java/org/springframework/security/config/annotation/method/configuration/PrePostMethodSecurityConfiguration.java +++ b/config/src/main/java/org/springframework/security/config/annotation/method/configuration/PrePostMethodSecurityConfiguration.java @@ -16,13 +16,18 @@ package org.springframework.security.config.annotation.method.configuration; +import java.util.Map; + import org.aopalliance.intercept.MethodInterceptor; import org.aopalliance.intercept.MethodInvocation; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; import org.springframework.aop.Pointcut; import org.springframework.aop.framework.AopInfrastructureBean; import org.springframework.beans.BeansException; import org.springframework.beans.factory.ObjectProvider; +import org.springframework.beans.factory.SmartInitializingSingleton; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.config.BeanDefinition; import org.springframework.context.ApplicationContext; @@ -31,6 +36,7 @@ import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.ImportAware; import org.springframework.context.annotation.Role; +import org.springframework.core.annotation.AnnotationUtils; import org.springframework.core.type.AnnotationMetadata; import org.springframework.security.access.expression.method.DefaultMethodSecurityExpressionHandler; import org.springframework.security.access.expression.method.MethodSecurityExpressionHandler; @@ -50,18 +56,24 @@ import org.springframework.security.config.core.GrantedAuthorityDefaults; import org.springframework.security.core.annotation.AnnotationTemplateExpressionDefaults; import org.springframework.security.core.context.SecurityContextHolderStrategy; +import org.springframework.transaction.annotation.EnableTransactionManagement; +import org.springframework.util.ClassUtils; /** * Base {@link Configuration} for enabling Spring Security Method Security. * * @author Evgeniy Cheban * @author Josh Cummings + * @author Yoobin Yoon * @since 5.6 * @see EnableMethodSecurity */ @Configuration(value = "_prePostMethodSecurityConfiguration", proxyBeanMethods = false) @Role(BeanDefinition.ROLE_INFRASTRUCTURE) -final class PrePostMethodSecurityConfiguration implements ImportAware, ApplicationContextAware, AopInfrastructureBean { +final class PrePostMethodSecurityConfiguration + implements ImportAware, ApplicationContextAware, AopInfrastructureBean, SmartInitializingSingleton { + + private static final Log logger = LogFactory.getLog(PrePostMethodSecurityConfiguration.class); private static final Pointcut preFilterPointcut = new PreFilterAuthorizationMethodInterceptor().getPointcut(); @@ -87,6 +99,10 @@ final class PrePostMethodSecurityConfiguration implements ImportAware, Applicati private final DefaultMethodSecurityExpressionHandler expressionHandler = new DefaultMethodSecurityExpressionHandler(); + private ApplicationContext applicationContext; + + private boolean prePostEnabled = true; + PrePostMethodSecurityConfiguration( ObjectProvider>> preAuthorizeProcessor, ObjectProvider>> postAuthorizeProcessor) { @@ -106,11 +122,20 @@ final class PrePostMethodSecurityConfiguration implements ImportAware, Applicati @Override public void setApplicationContext(ApplicationContext context) throws BeansException { + this.applicationContext = context; this.expressionHandler.setApplicationContext(context); this.preAuthorizeAuthorizationManager.setApplicationContext(context); this.postAuthorizeAuthorizationManager.setApplicationContext(context); } + @Override + public void afterSingletonsInstantiated() { + if (!this.prePostEnabled) { + return; + } + validateTransactionManagementPrecedence(); + } + @Autowired(required = false) void setGrantedAuthorityDefaults(GrantedAuthorityDefaults grantedAuthorityDefaults) { this.expressionHandler.setDefaultRolePrefix(grantedAuthorityDefaults.getRolePrefix()); @@ -192,6 +217,7 @@ static SecurityHintsRegistrar prePostAuthorizeExpressionHintsRegistrar() { @Override public void setImportMetadata(AnnotationMetadata importMetadata) { EnableMethodSecurity annotation = importMetadata.getAnnotations().get(EnableMethodSecurity.class).synthesize(); + this.prePostEnabled = annotation.prePostEnabled(); this.preFilterMethodInterceptor.setOrder(this.preFilterMethodInterceptor.getOrder() + annotation.offset()); this.preAuthorizeMethodInterceptor .setOrder(this.preAuthorizeMethodInterceptor.getOrder() + annotation.offset()); @@ -200,4 +226,42 @@ public void setImportMetadata(AnnotationMetadata importMetadata) { this.postFilterMethodInterceptor.setOrder(this.postFilterMethodInterceptor.getOrder() + annotation.offset()); } + /** + * Validates that @EnableTransactionManagement has higher precedence + * than @EnableMethodSecurity. This is important to ensure that @PostAuthorize checks + * happen before transaction commit, allowing rollback on authorization failures. + */ + private void validateTransactionManagementPrecedence() { + try { + int currentMethodSecurityOrder = this.preAuthorizeMethodInterceptor.getOrder(); + + Map txMgmtBeans = this.applicationContext + .getBeansWithAnnotation(EnableTransactionManagement.class); + + for (Map.Entry entry : txMgmtBeans.entrySet()) { + Class configClass = ClassUtils.getUserClass(entry.getValue().getClass()); + EnableTransactionManagement txMgmt = AnnotationUtils.findAnnotation(configClass, + EnableTransactionManagement.class); + + if (txMgmt != null) { + int txOrder = txMgmt.order(); + if (txOrder >= currentMethodSecurityOrder) { + logger.warn("@EnableTransactionManagement has same or lower precedence (order=" + txOrder + + ") than @EnableMethodSecurity (effective order=" + currentMethodSecurityOrder + + "). This may cause issues with @PostAuthorize on methods with side effects. " + + "Consider setting @EnableTransactionManagement(order = 0) or adjusting the order values. " + + "See Spring Security migration guide for more details."); + break; + } + } + } + } + catch (BeansException ex) { + logger.warn("Could not validate transaction management precedence due to bean access issues", ex); + } + catch (Exception ex) { + logger.debug("Could not validate transaction management precedence", ex); + } + } + } diff --git a/config/src/test/java/org/springframework/security/config/annotation/method/configuration/PrePostMethodSecurityConfigurationTests.java b/config/src/test/java/org/springframework/security/config/annotation/method/configuration/PrePostMethodSecurityConfigurationTests.java index baad6eabea..c04e0e7442 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/method/configuration/PrePostMethodSecurityConfigurationTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/method/configuration/PrePostMethodSecurityConfigurationTests.java @@ -28,6 +28,10 @@ import java.util.function.Consumer; import java.util.function.Supplier; +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; import io.micrometer.observation.Observation; import io.micrometer.observation.ObservationHandler; import io.micrometer.observation.ObservationRegistry; @@ -40,6 +44,7 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; +import org.slf4j.LoggerFactory; import org.springframework.aop.Advisor; import org.springframework.aop.Pointcut; @@ -125,6 +130,7 @@ import org.springframework.test.web.servlet.MockMvc; import org.springframework.test.web.servlet.MvcResult; import org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder; +import org.springframework.transaction.annotation.EnableTransactionManagement; import org.springframework.web.bind.annotation.ControllerAdvice; import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.GetMapping; @@ -156,6 +162,7 @@ * * @author Evgeniy Cheban * @author Josh Cummings + * @author Yoobin Yoon */ @ExtendWith({ SpringExtension.class, SpringTestContextExtension.class }) @ContextConfiguration(classes = SecurityContextChangedListenerConfig.class) @@ -1348,6 +1355,85 @@ void getWhenCustomAdvisorAuthenticationNameNotMatchThenRespondsWithForbidden() t this.mvc.perform(requestWithUser).andExpect(status().isForbidden()); } + @Test + public void configureWhenTransactionManagementThenWarningCondition() { + this.spring.register(TransactionManagementConfig.class).autowire(); + assertThat(this.spring.getContext().getBean(MethodSecurityService.class)).isNotNull(); + EnableTransactionManagement txMgmt = TransactionManagementConfig.class + .getAnnotation(EnableTransactionManagement.class); + assertThat(txMgmt.order()).isGreaterThan(100); + } + + @Test + public void configureWhenTransactionManagementLowerPrecedenceThenWarningCondition() { + this.spring.register(TransactionManagementLowerPrecedenceConfig.class).autowire(); + EnableTransactionManagement txMgmt = TransactionManagementLowerPrecedenceConfig.class + .getAnnotation(EnableTransactionManagement.class); + assertThat(txMgmt.order()).isGreaterThan(100); + } + + @Test + public void configureWhenTransactionManagementHigherPrecedenceThenNoWarningCondition() { + this.spring.register(TransactionManagementHigherPrecedenceConfig.class).autowire(); + assertThat(this.spring.getContext().getBean(MethodSecurityService.class)).isNotNull(); + EnableTransactionManagement txMgmt = TransactionManagementHigherPrecedenceConfig.class + .getAnnotation(EnableTransactionManagement.class); + assertThat(txMgmt.order()).isLessThanOrEqualTo(100); + } + + @Test + public void configureWhenTransactionManagementSameOrderThenWarningCondition() { + this.spring.register(TransactionManagementSameOrderConfig.class).autowire(); + EnableTransactionManagement txMgmt = TransactionManagementSameOrderConfig.class + .getAnnotation(EnableTransactionManagement.class); + assertThat(txMgmt.order()).isEqualTo(100); + } + + @Test + public void configureWhenMethodSecurityOffsetThenWarningCondition() { + this.spring.register(MethodSecurityOffsetWithTransactionConfig.class).autowire(); + EnableMethodSecurity methodSecurity = MethodSecurityOffsetWithTransactionConfig.class + .getAnnotation(EnableMethodSecurity.class); + EnableTransactionManagement txMgmt = MethodSecurityOffsetWithTransactionConfig.class + .getAnnotation(EnableTransactionManagement.class); + int effectiveMethodSecurityOrder = 100 + methodSecurity.offset(); + assertThat(txMgmt.order()).isGreaterThan(effectiveMethodSecurityOrder); + } + + @Test + public void configureWhenTransactionManagementSameOrderThenNoWarningCondition() { + this.spring.register(TransactionManagementSameOrderConfig.class).autowire(); + EnableTransactionManagement txMgmt = TransactionManagementSameOrderConfig.class + .getAnnotation(EnableTransactionManagement.class); + assertThat(txMgmt.order()).isEqualTo(100); + } + + @Test + public void configureWhenTransactionManagementLowerPrecedenceThenValidationRuns() { + assertThatNoException() + .isThrownBy(() -> this.spring.register(TransactionManagementLowerPrecedenceConfig.class).autowire()); + assertThat(this.spring.getContext().getBean(MethodSecurityService.class)).isNotNull(); + } + + @Test + public void validateTransactionManagementPrecedenceWhenLowerPrecedenceThenLogsWarning() { + Logger logger = (Logger) LoggerFactory.getLogger(PrePostMethodSecurityConfiguration.class); + ListAppender appender = new ListAppender<>(); + appender.start(); + logger.addAppender(appender); + try { + this.spring.register(TransactionManagementLowerPrecedenceConfig.class).autowire(); + assertThat(appender.list).hasSize(1); + assertThat(appender.list.get(0).getLevel()).isEqualTo(Level.WARN); + assertThat(appender.list.get(0).getMessage()) + .contains("@EnableTransactionManagement has same or lower precedence"); + + } + finally { + logger.detachAppender(appender); + } + } + private static Consumer disallowBeanOverriding() { return (context) -> ((AnnotationConfigWebApplicationContext) context).setAllowBeanDefinitionOverriding(false); } @@ -2201,4 +2287,75 @@ public String getName() { } + @Configuration + @EnableMethodSecurity(prePostEnabled = true) + @EnableTransactionManagement + static class TransactionManagementConfig { + + @Bean + MethodSecurityService methodSecurityService() { + return new MethodSecurityServiceImpl(); + } + + } + + @Configuration + @EnableMethodSecurity(prePostEnabled = true) + @EnableTransactionManagement(order = 300) + static class TransactionManagementLowerPrecedenceConfig { + + @Bean + MethodSecurityService methodSecurityService() { + return new MethodSecurityServiceImpl(); + } + + } + + @Configuration + @EnableMethodSecurity(prePostEnabled = true) + @EnableTransactionManagement(order = 0) + static class TransactionManagementHigherPrecedenceConfig { + + @Bean + MethodSecurityService methodSecurityService() { + return new MethodSecurityServiceImpl(); + } + + } + + @Configuration + @EnableMethodSecurity(prePostEnabled = true) + @EnableTransactionManagement(order = 100) + static class TransactionManagementSameOrderConfig { + + @Bean + MethodSecurityService methodSecurityService() { + return new MethodSecurityServiceImpl(); + } + + } + + @Configuration + @EnableMethodSecurity(prePostEnabled = true) + static class NoTransactionManagementConfig { + + @Bean + MethodSecurityService methodSecurityService() { + return new MethodSecurityServiceImpl(); + } + + } + + @Configuration + @EnableMethodSecurity(prePostEnabled = true, offset = 50) + @EnableTransactionManagement(order = 200) + static class MethodSecurityOffsetWithTransactionConfig { + + @Bean + MethodSecurityService methodSecurityService() { + return new MethodSecurityServiceImpl(); + } + + } + }