From 59ec1f6480339c6e13e6a2b9b418a21d23419012 Mon Sep 17 00:00:00 2001 From: Josh Cummings Date: Sat, 10 Aug 2024 16:36:59 -0600 Subject: [PATCH] Revert "Polish AuthorizationAdvisorProxyFactory advisor configuration" This commit had some unintended consequences when the advisor interceptor was published in a Spring Boot application. As such, 15497 will be reopened to investigate. In the meantime, this commit reverts the previous change so as to allow the build to pass. Issue gh-15497 --- .../AuthorizationProxyConfiguration.java | 10 +++-- .../AuthorizationAdvisorProxyFactory.java | 43 +++---------------- ...AuthorizationAdvisorProxyFactoryTests.java | 23 ---------- 3 files changed, 14 insertions(+), 62 deletions(-) diff --git a/config/src/main/java/org/springframework/security/config/annotation/method/configuration/AuthorizationProxyConfiguration.java b/config/src/main/java/org/springframework/security/config/annotation/method/configuration/AuthorizationProxyConfiguration.java index 3e49ff786e..a4e29505a7 100644 --- a/config/src/main/java/org/springframework/security/config/annotation/method/configuration/AuthorizationProxyConfiguration.java +++ b/config/src/main/java/org/springframework/security/config/annotation/method/configuration/AuthorizationProxyConfiguration.java @@ -41,18 +41,22 @@ final class AuthorizationProxyConfiguration implements AopInfrastructureBean { ObjectProvider> customizers) { List advisors = new ArrayList<>(); provider.forEach(advisors::add); - AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(advisors); + AuthorizationAdvisorProxyFactory factory = AuthorizationAdvisorProxyFactory.withDefaults(); customizers.forEach((c) -> c.customize(factory)); + factory.setAdvisors(advisors); return factory; } @Bean @Role(BeanDefinition.ROLE_INFRASTRUCTURE) - static MethodInterceptor authorizeReturnObjectMethodInterceptor( + static MethodInterceptor authorizeReturnObjectMethodInterceptor(ObjectProvider provider, AuthorizationAdvisorProxyFactory authorizationProxyFactory) { AuthorizeReturnObjectMethodInterceptor interceptor = new AuthorizeReturnObjectMethodInterceptor( authorizationProxyFactory); - authorizationProxyFactory.addAdvisors(interceptor); + List advisors = new ArrayList<>(); + provider.forEach(advisors::add); + advisors.add(interceptor); + authorizationProxyFactory.setAdvisors(advisors); return interceptor; } 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 400f612b30..42248b3da2 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 @@ -86,28 +86,14 @@ public final class AuthorizationAdvisorProxyFactory private static final TargetVisitor DEFAULT_VISITOR_SKIP_VALUE_TYPES = TargetVisitor.of(new ClassVisitor(), new IgnoreValueTypeVisitor(), DEFAULT_VISITOR); - private List advisors = new ArrayList<>(); + private List advisors; private TargetVisitor visitor = DEFAULT_VISITOR; - /** - * Construct an {@link AuthorizationAdvisorProxyFactory} with the following advisors - * @param advisors the list of advisors to wrap around proxied objects - * @since 6.4 - */ - public AuthorizationAdvisorProxyFactory(List advisors) { - this.advisors.addAll(advisors); - AnnotationAwareOrderComparator.sort(this.advisors); - } - - /** - * Construct an {@link AuthorizationAdvisorProxyFactory} with the following advisors - * @param advisors the list of advisors to wrap around proxied objects - * @since 6.4 - */ - public AuthorizationAdvisorProxyFactory(AuthorizationAdvisor... advisors) { - this.advisors.addAll(List.of(advisors)); - AnnotationAwareOrderComparator.sort(this.advisors); + private AuthorizationAdvisorProxyFactory(List advisors) { + this.advisors = new ArrayList<>(advisors); + this.advisors.add(new AuthorizeReturnObjectMethodInterceptor(this)); + setAdvisors(this.advisors); } /** @@ -122,9 +108,7 @@ public final class AuthorizationAdvisorProxyFactory advisors.add(AuthorizationManagerAfterMethodInterceptor.postAuthorize()); advisors.add(new PreFilterAuthorizationMethodInterceptor()); advisors.add(new PostFilterAuthorizationMethodInterceptor()); - AuthorizationAdvisorProxyFactory proxyFactory = new AuthorizationAdvisorProxyFactory(advisors); - proxyFactory.addAdvisors(new AuthorizeReturnObjectMethodInterceptor(proxyFactory)); - return proxyFactory; + return new AuthorizationAdvisorProxyFactory(advisors); } /** @@ -139,9 +123,7 @@ public final class AuthorizationAdvisorProxyFactory advisors.add(AuthorizationManagerAfterReactiveMethodInterceptor.postAuthorize()); advisors.add(new PreFilterAuthorizationReactiveMethodInterceptor()); advisors.add(new PostFilterAuthorizationReactiveMethodInterceptor()); - AuthorizationAdvisorProxyFactory proxyFactory = new AuthorizationAdvisorProxyFactory(advisors); - proxyFactory.addAdvisors(new AuthorizeReturnObjectMethodInterceptor(proxyFactory)); - return proxyFactory; + return new AuthorizationAdvisorProxyFactory(advisors); } /** @@ -179,21 +161,13 @@ public final class AuthorizationAdvisorProxyFactory return factory.getProxy(); } - public void addAdvisors(AuthorizationAdvisor... advisors) { - this.advisors.addAll(List.of(advisors)); - AnnotationAwareOrderComparator.sort(this.advisors); - } - /** * Add advisors that should be included to each proxy created. * *

* All advisors are re-sorted by their advisor order. * @param advisors the advisors to add - * @deprecated Either use the constructor to provide a complete set of advisors or use - * {@link #addAdvisors(AuthorizationAdvisor...)} to add to the existing list */ - @Deprecated public void setAdvisors(AuthorizationAdvisor... advisors) { this.advisors = new ArrayList<>(List.of(advisors)); AnnotationAwareOrderComparator.sort(this.advisors); @@ -205,10 +179,7 @@ public final class AuthorizationAdvisorProxyFactory *

* All advisors are re-sorted by their advisor order. * @param advisors the advisors to add - * @deprecated Either use the constructor to provide a complete set of advisors or use - * {@link #addAdvisors(AuthorizationAdvisor...)} to add to the existing list */ - @Deprecated public void setAdvisors(Collection advisors) { this.advisors = new ArrayList<>(advisors); AnnotationAwareOrderComparator.sort(this.advisors); 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 b6e816d59d..15389c2ec2 100644 --- a/core/src/test/java/org/springframework/security/authorization/AuthorizationAdvisorProxyFactoryTests.java +++ b/core/src/test/java/org/springframework/security/authorization/AuthorizationAdvisorProxyFactoryTests.java @@ -319,29 +319,6 @@ public class AuthorizationAdvisorProxyFactoryTests { verify(advisor, atLeastOnce()).getPointcut(); } - @Test - public void addAdvisorsWhenProxyThenVisits() { - AuthorizationAdvisor advisor = mock(AuthorizationAdvisor.class); - given(advisor.getAdvice()).willReturn(advisor); - given(advisor.getPointcut()).willReturn(Pointcut.TRUE); - AuthorizationAdvisorProxyFactory factory = AuthorizationAdvisorProxyFactory.withDefaults(); - factory.addAdvisors(advisor); - Flight flight = proxy(factory, this.flight); - flight.getAltitude(); - verify(advisor, atLeastOnce()).getPointcut(); - } - - @Test - public void constructWhenProxyThenVisitsAdvisors() { - AuthorizationAdvisor advisor = mock(AuthorizationAdvisor.class); - given(advisor.getAdvice()).willReturn(advisor); - given(advisor.getPointcut()).willReturn(Pointcut.TRUE); - AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(advisor); - Flight flight = proxy(factory, this.flight); - flight.getAltitude(); - verify(advisor, atLeastOnce()).getPointcut(); - } - @Test public void setTargetVisitorThenUses() { TargetVisitor visitor = mock(TargetVisitor.class);