From 2417de3df34cb538666edfe32bfb3ba061e18b2c Mon Sep 17 00:00:00 2001 From: Tran Ngoc Nhan Date: Fri, 28 Mar 2025 01:48:27 +0700 Subject: [PATCH 1/3] Sort Advisors AfterSingletonsInstantiated In order to make so that authorization advisors are sorted only one time and also as part of the configuration lifecycle, AuthorizationAdvisorProxyFactory now implements SmartInitializingBean. Closes gh-16819 Signed-off-by: Tran Ngoc Nhan --- .../method/AuthorizationAdvisorProxyFactory.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/org/springframework/security/authorization/method/AuthorizationAdvisorProxyFactory.java b/core/src/main/java/org/springframework/security/authorization/method/AuthorizationAdvisorProxyFactory.java index 3bc69949c81..16566997c0f 100644 --- a/core/src/main/java/org/springframework/security/authorization/method/AuthorizationAdvisorProxyFactory.java +++ b/core/src/main/java/org/springframework/security/authorization/method/AuthorizationAdvisorProxyFactory.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2024 the original author or authors. + * Copyright 2002-2025 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. @@ -42,6 +42,7 @@ import org.springframework.aop.Advisor; import org.springframework.aop.framework.ProxyFactory; +import org.springframework.beans.factory.SmartInitializingSingleton; import org.springframework.core.annotation.AnnotationAwareOrderComparator; import org.springframework.lang.NonNull; import org.springframework.security.authorization.AuthorizationProxyFactory; @@ -75,7 +76,7 @@ * @since 6.3 */ public final class AuthorizationAdvisorProxyFactory - implements AuthorizationProxyFactory, Iterable { + implements AuthorizationProxyFactory, Iterable, SmartInitializingSingleton { private static final boolean isReactivePresent = ClassUtils.isPresent("reactor.core.publisher.Mono", null); @@ -126,6 +127,11 @@ public static AuthorizationAdvisorProxyFactory withReactiveDefaults() { return new AuthorizationAdvisorProxyFactory(advisors); } + @Override + public void afterSingletonsInstantiated() { + AnnotationAwareOrderComparator.sort(this.advisors); + } + /** * Proxy an object to enforce authorization advice. * @@ -146,7 +152,6 @@ public static AuthorizationAdvisorProxyFactory withReactiveDefaults() { */ @Override public Object proxy(Object target) { - AnnotationAwareOrderComparator.sort(this.advisors); if (target == null) { return null; } From 5b7baee3c3cca983366c607bb66830429e1d4ddb Mon Sep 17 00:00:00 2001 From: Josh Cummings <3627351+jzheaux@users.noreply.github.com> Date: Thu, 27 Mar 2025 14:09:29 -0600 Subject: [PATCH 2/3] Add Test Issue gh-16819 --- ...rePostMethodSecurityConfigurationTests.java | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) 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 223fc3f8fc2..b0fb127e88c 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 @@ -1,5 +1,5 @@ /* - * Copyright 2002-2024 the original author or authors. + * Copyright 2002-2025 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. @@ -47,6 +47,7 @@ import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Import; import org.springframework.context.annotation.Role; +import org.springframework.core.annotation.AnnotationAwareOrderComparator; import org.springframework.core.annotation.AnnotationConfigurationException; import org.springframework.security.access.AccessDeniedException; import org.springframework.security.access.PermissionEvaluator; @@ -1007,6 +1008,21 @@ public void methodWhenMetaAnnotationPropertiesHasClassProperties() { assertThat(service.getIdPath("uid")).isEqualTo("uid"); } + // gh-16819 + @Test + void autowireWhenDefaultsThenAdvisorAnnotationsAreSorted() { + this.spring.register(MethodSecurityServiceConfig.class).autowire(); + AuthorizationAdvisorProxyFactory proxyFactory = this.spring.getContext() + .getBean(AuthorizationAdvisorProxyFactory.class); + AnnotationAwareOrderComparator comparator = AnnotationAwareOrderComparator.INSTANCE; + AuthorizationAdvisor previous = null; + for (AuthorizationAdvisor advisor : proxyFactory) { + boolean ordered = previous == null || comparator.compare(previous, advisor) < 0; + assertThat(ordered).isTrue(); + previous = advisor; + } + } + private static Consumer disallowBeanOverriding() { return (context) -> ((AnnotationConfigWebApplicationContext) context).setAllowBeanDefinitionOverriding(false); } From fc6e1af64601392b2c648ea59a1a459fcce64cad Mon Sep 17 00:00:00 2001 From: Josh Cummings <3627351+jzheaux@users.noreply.github.com> Date: Thu, 27 Mar 2025 15:29:44 -0600 Subject: [PATCH 3/3] Sort Default Advisors and Added Advisors This commit ensures that the default advisors and added advisors are sorted in the event that this component is not being published as a Spring bean. Issue gh-16819 --- .../AuthorizationAdvisorProxyFactory.java | 14 +++++---- ...AuthorizationAdvisorProxyFactoryTests.java | 29 ++++++++++++++++++- 2 files changed, 37 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/org/springframework/security/authorization/method/AuthorizationAdvisorProxyFactory.java b/core/src/main/java/org/springframework/security/authorization/method/AuthorizationAdvisorProxyFactory.java index 16566997c0f..57731a35982 100644 --- a/core/src/main/java/org/springframework/security/authorization/method/AuthorizationAdvisorProxyFactory.java +++ b/core/src/main/java/org/springframework/security/authorization/method/AuthorizationAdvisorProxyFactory.java @@ -109,7 +109,9 @@ public static AuthorizationAdvisorProxyFactory withDefaults() { advisors.add(AuthorizationManagerAfterMethodInterceptor.postAuthorize()); advisors.add(new PreFilterAuthorizationMethodInterceptor()); advisors.add(new PostFilterAuthorizationMethodInterceptor()); - return new AuthorizationAdvisorProxyFactory(advisors); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(advisors); + AnnotationAwareOrderComparator.sort(factory.advisors); + return factory; } /** @@ -124,7 +126,9 @@ public static AuthorizationAdvisorProxyFactory withReactiveDefaults() { advisors.add(AuthorizationManagerAfterReactiveMethodInterceptor.postAuthorize()); advisors.add(new PreFilterAuthorizationReactiveMethodInterceptor()); advisors.add(new PostFilterAuthorizationReactiveMethodInterceptor()); - return new AuthorizationAdvisorProxyFactory(advisors); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(advisors); + AnnotationAwareOrderComparator.sort(factory.advisors); + return factory; } @Override @@ -160,9 +164,9 @@ public Object proxy(Object target) { return proxied; } ProxyFactory factory = new ProxyFactory(target); - for (Advisor advisor : this.advisors) { - factory.addAdvisors(advisor); - } + List advisors = new ArrayList<>(this.advisors); + AnnotationAwareOrderComparator.sort(advisors); + factory.addAdvisors(advisors); factory.setProxyTargetClass(!Modifier.isFinal(target.getClass().getModifiers())); return factory.getProxy(); } diff --git a/core/src/test/java/org/springframework/security/authorization/AuthorizationAdvisorProxyFactoryTests.java b/core/src/test/java/org/springframework/security/authorization/AuthorizationAdvisorProxyFactoryTests.java index 15389c2ec2c..0b325cbd0cb 100644 --- a/core/src/test/java/org/springframework/security/authorization/AuthorizationAdvisorProxyFactoryTests.java +++ b/core/src/test/java/org/springframework/security/authorization/AuthorizationAdvisorProxyFactoryTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2024 the original author or authors. + * Copyright 2002-2025 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. @@ -38,6 +38,7 @@ import org.junit.jupiter.api.Test; import org.springframework.aop.Pointcut; +import org.springframework.core.annotation.AnnotationAwareOrderComparator; import org.springframework.security.access.AccessDeniedException; import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.security.authentication.TestAuthentication; @@ -336,6 +337,32 @@ public void setTargetVisitorIgnoreValueTypesThenIgnores() { assertThat(factory.proxy(35)).isEqualTo(35); } + // gh-16819 + @Test + void advisorsWhenWithDefaultsThenAreSorted() { + AuthorizationAdvisorProxyFactory proxyFactory = AuthorizationAdvisorProxyFactory.withDefaults(); + AnnotationAwareOrderComparator comparator = AnnotationAwareOrderComparator.INSTANCE; + AuthorizationAdvisor previous = null; + for (AuthorizationAdvisor advisor : proxyFactory) { + boolean ordered = previous == null || comparator.compare(previous, advisor) < 0; + assertThat(ordered).isTrue(); + previous = advisor; + } + } + + // gh-16819 + @Test + void advisorsWhenWithReactiveDefaultsThenAreSorted() { + AuthorizationAdvisorProxyFactory proxyFactory = AuthorizationAdvisorProxyFactory.withReactiveDefaults(); + AnnotationAwareOrderComparator comparator = AnnotationAwareOrderComparator.INSTANCE; + AuthorizationAdvisor previous = null; + for (AuthorizationAdvisor advisor : proxyFactory) { + boolean ordered = previous == null || comparator.compare(previous, advisor) < 0; + assertThat(ordered).isTrue(); + previous = advisor; + } + } + private Authentication authenticated(String user, String... authorities) { return TestAuthentication.authenticated(TestAuthentication.withUsername(user).authorities(authorities).build()); }