Skip to content

Commit 59f6c3d

Browse files
yybmionyybmion
committed
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 <[email protected]>
1 parent f61a8de commit 59f6c3d

File tree

2 files changed

+222
-1
lines changed

2 files changed

+222
-1
lines changed

‎config/src/main/java/org/springframework/security/config/annotation/method/configuration/PrePostMethodSecurityConfiguration.java

Lines changed: 65 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,18 @@
1616

1717
package org.springframework.security.config.annotation.method.configuration;
1818

19+
import java.util.Map;
20+
1921
import org.aopalliance.intercept.MethodInterceptor;
2022
import org.aopalliance.intercept.MethodInvocation;
23+
import org.apache.commons.logging.Log;
24+
import org.apache.commons.logging.LogFactory;
2125

2226
import org.springframework.aop.Pointcut;
2327
import org.springframework.aop.framework.AopInfrastructureBean;
2428
import org.springframework.beans.BeansException;
2529
import org.springframework.beans.factory.ObjectProvider;
30+
import org.springframework.beans.factory.SmartInitializingSingleton;
2631
import org.springframework.beans.factory.annotation.Autowired;
2732
import org.springframework.beans.factory.config.BeanDefinition;
2833
import org.springframework.context.ApplicationContext;
@@ -31,6 +36,7 @@
3136
import org.springframework.context.annotation.Configuration;
3237
import org.springframework.context.annotation.ImportAware;
3338
import org.springframework.context.annotation.Role;
39+
import org.springframework.core.annotation.AnnotationUtils;
3440
import org.springframework.core.type.AnnotationMetadata;
3541
import org.springframework.security.access.expression.method.DefaultMethodSecurityExpressionHandler;
3642
import org.springframework.security.access.expression.method.MethodSecurityExpressionHandler;
@@ -50,18 +56,24 @@
5056
import org.springframework.security.config.core.GrantedAuthorityDefaults;
5157
import org.springframework.security.core.annotation.AnnotationTemplateExpressionDefaults;
5258
import org.springframework.security.core.context.SecurityContextHolderStrategy;
59+
import org.springframework.transaction.annotation.EnableTransactionManagement;
60+
import org.springframework.util.ClassUtils;
5361

5462
/**
5563
* Base {@link Configuration} for enabling Spring Security Method Security.
5664
*
5765
* @author Evgeniy Cheban
5866
* @author Josh Cummings
67+
* @author Yoobin Yoon
5968
* @since 5.6
6069
* @see EnableMethodSecurity
6170
*/
6271
@Configuration(value = "_prePostMethodSecurityConfiguration", proxyBeanMethods = false)
6372
@Role(BeanDefinition.ROLE_INFRASTRUCTURE)
64-
final class PrePostMethodSecurityConfiguration implements ImportAware, ApplicationContextAware, AopInfrastructureBean {
73+
final class PrePostMethodSecurityConfiguration
74+
implements ImportAware, ApplicationContextAware, AopInfrastructureBean, SmartInitializingSingleton {
75+
76+
private static final Log logger = LogFactory.getLog(PrePostMethodSecurityConfiguration.class);
6577

6678
private static final Pointcut preFilterPointcut = new PreFilterAuthorizationMethodInterceptor().getPointcut();
6779

@@ -87,6 +99,10 @@ final class PrePostMethodSecurityConfiguration implements ImportAware, Applicati
8799

88100
private final DefaultMethodSecurityExpressionHandler expressionHandler = new DefaultMethodSecurityExpressionHandler();
89101

102+
private ApplicationContext applicationContext;
103+
104+
private boolean prePostEnabled = true;
105+
90106
PrePostMethodSecurityConfiguration(
91107
ObjectProvider<ObjectPostProcessor<AuthorizationManager<MethodInvocation>>> preAuthorizeProcessor,
92108
ObjectProvider<ObjectPostProcessor<AuthorizationManager<MethodInvocationResult>>> postAuthorizeProcessor) {
@@ -106,11 +122,20 @@ final class PrePostMethodSecurityConfiguration implements ImportAware, Applicati
106122

107123
@Override
108124
public void setApplicationContext(ApplicationContext context) throws BeansException {
125+
this.applicationContext = context;
109126
this.expressionHandler.setApplicationContext(context);
110127
this.preAuthorizeAuthorizationManager.setApplicationContext(context);
111128
this.postAuthorizeAuthorizationManager.setApplicationContext(context);
112129
}
113130

131+
@Override
132+
public void afterSingletonsInstantiated() {
133+
if (!this.prePostEnabled) {
134+
return;
135+
}
136+
validateTransactionManagementPrecedence();
137+
}
138+
114139
@Autowired(required = false)
115140
void setGrantedAuthorityDefaults(GrantedAuthorityDefaults grantedAuthorityDefaults) {
116141
this.expressionHandler.setDefaultRolePrefix(grantedAuthorityDefaults.getRolePrefix());
@@ -192,6 +217,7 @@ static SecurityHintsRegistrar prePostAuthorizeExpressionHintsRegistrar() {
192217
@Override
193218
public void setImportMetadata(AnnotationMetadata importMetadata) {
194219
EnableMethodSecurity annotation = importMetadata.getAnnotations().get(EnableMethodSecurity.class).synthesize();
220+
this.prePostEnabled = annotation.prePostEnabled();
195221
this.preFilterMethodInterceptor.setOrder(this.preFilterMethodInterceptor.getOrder() + annotation.offset());
196222
this.preAuthorizeMethodInterceptor
197223
.setOrder(this.preAuthorizeMethodInterceptor.getOrder() + annotation.offset());
@@ -200,4 +226,42 @@ public void setImportMetadata(AnnotationMetadata importMetadata) {
200226
this.postFilterMethodInterceptor.setOrder(this.postFilterMethodInterceptor.getOrder() + annotation.offset());
201227
}
202228

229+
/**
230+
* Validates that @EnableTransactionManagement has higher precedence
231+
* than @EnableMethodSecurity. This is important to ensure that @PostAuthorize checks
232+
* happen before transaction commit, allowing rollback on authorization failures.
233+
*/
234+
private void validateTransactionManagementPrecedence() {
235+
try {
236+
int currentMethodSecurityOrder = this.preAuthorizeMethodInterceptor.getOrder();
237+
238+
Map<String, Object> txMgmtBeans = this.applicationContext
239+
.getBeansWithAnnotation(EnableTransactionManagement.class);
240+
241+
for (Map.Entry<String, Object> entry : txMgmtBeans.entrySet()) {
242+
Class<?> configClass = ClassUtils.getUserClass(entry.getValue().getClass());
243+
EnableTransactionManagement txMgmt = AnnotationUtils.findAnnotation(configClass,
244+
EnableTransactionManagement.class);
245+
246+
if (txMgmt != null) {
247+
int txOrder = txMgmt.order();
248+
if (txOrder >= currentMethodSecurityOrder) {
249+
logger.warn("@EnableTransactionManagement has same or lower precedence (order=" + txOrder
250+
+ ") than @EnableMethodSecurity (effective order=" + currentMethodSecurityOrder
251+
+ "). This may cause issues with @PostAuthorize on methods with side effects. "
252+
+ "Consider setting @EnableTransactionManagement(order = 0) or adjusting the order values. "
253+
+ "See Spring Security migration guide for more details.");
254+
break;
255+
}
256+
}
257+
}
258+
}
259+
catch (BeansException ex) {
260+
logger.warn("Could not validate transaction management precedence due to bean access issues", ex);
261+
}
262+
catch (Exception ex) {
263+
logger.debug("Could not validate transaction management precedence", ex);
264+
}
265+
}
266+
203267
}

‎config/src/test/java/org/springframework/security/config/annotation/method/configuration/PrePostMethodSecurityConfigurationTests.java

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@
2828
import java.util.function.Consumer;
2929
import java.util.function.Supplier;
3030

31+
import ch.qos.logback.classic.Level;
32+
import ch.qos.logback.classic.Logger;
33+
import ch.qos.logback.classic.spi.ILoggingEvent;
34+
import ch.qos.logback.core.read.ListAppender;
3135
import io.micrometer.observation.Observation;
3236
import io.micrometer.observation.ObservationHandler;
3337
import io.micrometer.observation.ObservationRegistry;
@@ -40,6 +44,7 @@
4044
import org.junit.jupiter.api.extension.ExtendWith;
4145
import org.junit.jupiter.params.ParameterizedTest;
4246
import org.junit.jupiter.params.provider.ValueSource;
47+
import org.slf4j.LoggerFactory;
4348

4449
import org.springframework.aop.Advisor;
4550
import org.springframework.aop.Pointcut;
@@ -125,6 +130,7 @@
125130
import org.springframework.test.web.servlet.MockMvc;
126131
import org.springframework.test.web.servlet.MvcResult;
127132
import org.springframework.test.web.servlet.request.MockHttpServletRequestBuilder;
133+
import org.springframework.transaction.annotation.EnableTransactionManagement;
128134
import org.springframework.web.bind.annotation.ControllerAdvice;
129135
import org.springframework.web.bind.annotation.ExceptionHandler;
130136
import org.springframework.web.bind.annotation.GetMapping;
@@ -156,6 +162,7 @@
156162
*
157163
* @author Evgeniy Cheban
158164
* @author Josh Cummings
165+
* @author Yoobin Yoon
159166
*/
160167
@ExtendWith({ SpringExtension.class, SpringTestContextExtension.class })
161168
@ContextConfiguration(classes = SecurityContextChangedListenerConfig.class)
@@ -1348,6 +1355,85 @@ void getWhenCustomAdvisorAuthenticationNameNotMatchThenRespondsWithForbidden() t
13481355
this.mvc.perform(requestWithUser).andExpect(status().isForbidden());
13491356
}
13501357

1358+
@Test
1359+
public void configureWhenTransactionManagementThenWarningCondition() {
1360+
this.spring.register(TransactionManagementConfig.class).autowire();
1361+
assertThat(this.spring.getContext().getBean(MethodSecurityService.class)).isNotNull();
1362+
EnableTransactionManagement txMgmt = TransactionManagementConfig.class
1363+
.getAnnotation(EnableTransactionManagement.class);
1364+
assertThat(txMgmt.order()).isGreaterThan(100);
1365+
}
1366+
1367+
@Test
1368+
public void configureWhenTransactionManagementLowerPrecedenceThenWarningCondition() {
1369+
this.spring.register(TransactionManagementLowerPrecedenceConfig.class).autowire();
1370+
EnableTransactionManagement txMgmt = TransactionManagementLowerPrecedenceConfig.class
1371+
.getAnnotation(EnableTransactionManagement.class);
1372+
assertThat(txMgmt.order()).isGreaterThan(100);
1373+
}
1374+
1375+
@Test
1376+
public void configureWhenTransactionManagementHigherPrecedenceThenNoWarningCondition() {
1377+
this.spring.register(TransactionManagementHigherPrecedenceConfig.class).autowire();
1378+
assertThat(this.spring.getContext().getBean(MethodSecurityService.class)).isNotNull();
1379+
EnableTransactionManagement txMgmt = TransactionManagementHigherPrecedenceConfig.class
1380+
.getAnnotation(EnableTransactionManagement.class);
1381+
assertThat(txMgmt.order()).isLessThanOrEqualTo(100);
1382+
}
1383+
1384+
@Test
1385+
public void configureWhenTransactionManagementSameOrderThenWarningCondition() {
1386+
this.spring.register(TransactionManagementSameOrderConfig.class).autowire();
1387+
EnableTransactionManagement txMgmt = TransactionManagementSameOrderConfig.class
1388+
.getAnnotation(EnableTransactionManagement.class);
1389+
assertThat(txMgmt.order()).isEqualTo(100);
1390+
}
1391+
1392+
@Test
1393+
public void configureWhenMethodSecurityOffsetThenWarningCondition() {
1394+
this.spring.register(MethodSecurityOffsetWithTransactionConfig.class).autowire();
1395+
EnableMethodSecurity methodSecurity = MethodSecurityOffsetWithTransactionConfig.class
1396+
.getAnnotation(EnableMethodSecurity.class);
1397+
EnableTransactionManagement txMgmt = MethodSecurityOffsetWithTransactionConfig.class
1398+
.getAnnotation(EnableTransactionManagement.class);
1399+
int effectiveMethodSecurityOrder = 100 + methodSecurity.offset();
1400+
assertThat(txMgmt.order()).isGreaterThan(effectiveMethodSecurityOrder);
1401+
}
1402+
1403+
@Test
1404+
public void configureWhenTransactionManagementSameOrderThenNoWarningCondition() {
1405+
this.spring.register(TransactionManagementSameOrderConfig.class).autowire();
1406+
EnableTransactionManagement txMgmt = TransactionManagementSameOrderConfig.class
1407+
.getAnnotation(EnableTransactionManagement.class);
1408+
assertThat(txMgmt.order()).isEqualTo(100);
1409+
}
1410+
1411+
@Test
1412+
public void configureWhenTransactionManagementLowerPrecedenceThenValidationRuns() {
1413+
assertThatNoException()
1414+
.isThrownBy(() -> this.spring.register(TransactionManagementLowerPrecedenceConfig.class).autowire());
1415+
assertThat(this.spring.getContext().getBean(MethodSecurityService.class)).isNotNull();
1416+
}
1417+
1418+
@Test
1419+
public void validateTransactionManagementPrecedenceWhenLowerPrecedenceThenLogsWarning() {
1420+
Logger logger = (Logger) LoggerFactory.getLogger(PrePostMethodSecurityConfiguration.class);
1421+
ListAppender<ILoggingEvent> appender = new ListAppender<>();
1422+
appender.start();
1423+
logger.addAppender(appender);
1424+
try {
1425+
this.spring.register(TransactionManagementLowerPrecedenceConfig.class).autowire();
1426+
assertThat(appender.list).hasSize(1);
1427+
assertThat(appender.list.get(0).getLevel()).isEqualTo(Level.WARN);
1428+
assertThat(appender.list.get(0).getMessage())
1429+
.contains("@EnableTransactionManagement has same or lower precedence");
1430+
1431+
}
1432+
finally {
1433+
logger.detachAppender(appender);
1434+
}
1435+
}
1436+
13511437
private static Consumer<ConfigurableWebApplicationContext> disallowBeanOverriding() {
13521438
return (context) -> ((AnnotationConfigWebApplicationContext) context).setAllowBeanDefinitionOverriding(false);
13531439
}
@@ -2201,4 +2287,75 @@ public String getName() {
22012287

22022288
}
22032289

2290+
@Configuration
2291+
@EnableMethodSecurity(prePostEnabled = true)
2292+
@EnableTransactionManagement
2293+
static class TransactionManagementConfig {
2294+
2295+
@Bean
2296+
MethodSecurityService methodSecurityService() {
2297+
return new MethodSecurityServiceImpl();
2298+
}
2299+
2300+
}
2301+
2302+
@Configuration
2303+
@EnableMethodSecurity(prePostEnabled = true)
2304+
@EnableTransactionManagement(order = 300)
2305+
static class TransactionManagementLowerPrecedenceConfig {
2306+
2307+
@Bean
2308+
MethodSecurityService methodSecurityService() {
2309+
return new MethodSecurityServiceImpl();
2310+
}
2311+
2312+
}
2313+
2314+
@Configuration
2315+
@EnableMethodSecurity(prePostEnabled = true)
2316+
@EnableTransactionManagement(order = 0)
2317+
static class TransactionManagementHigherPrecedenceConfig {
2318+
2319+
@Bean
2320+
MethodSecurityService methodSecurityService() {
2321+
return new MethodSecurityServiceImpl();
2322+
}
2323+
2324+
}
2325+
2326+
@Configuration
2327+
@EnableMethodSecurity(prePostEnabled = true)
2328+
@EnableTransactionManagement(order = 100)
2329+
static class TransactionManagementSameOrderConfig {
2330+
2331+
@Bean
2332+
MethodSecurityService methodSecurityService() {
2333+
return new MethodSecurityServiceImpl();
2334+
}
2335+
2336+
}
2337+
2338+
@Configuration
2339+
@EnableMethodSecurity(prePostEnabled = true)
2340+
static class NoTransactionManagementConfig {
2341+
2342+
@Bean
2343+
MethodSecurityService methodSecurityService() {
2344+
return new MethodSecurityServiceImpl();
2345+
}
2346+
2347+
}
2348+
2349+
@Configuration
2350+
@EnableMethodSecurity(prePostEnabled = true, offset = 50)
2351+
@EnableTransactionManagement(order = 200)
2352+
static class MethodSecurityOffsetWithTransactionConfig {
2353+
2354+
@Bean
2355+
MethodSecurityService methodSecurityService() {
2356+
return new MethodSecurityServiceImpl();
2357+
}
2358+
2359+
}
2360+
22042361
}

0 commit comments

Comments
 (0)