From 02cca6f7379dadac8088e5f71c5cefbc87da0ecb Mon Sep 17 00:00:00 2001 From: Josh Cummings Date: Wed, 7 Aug 2024 10:08:11 -0600 Subject: [PATCH] Polish AuthorizationAdvisorProxyFactory advisor configuration Closes gh-15497 --- .../AuthorizationProxyConfiguration.java | 10 ++--- .../AuthorizationAdvisorProxyFactory.java | 43 ++++++++++++++++--- ...AuthorizationAdvisorProxyFactoryTests.java | 23 ++++++++++ 3 files changed, 62 insertions(+), 14 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 a4e29505a7..3e49ff786e 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,22 +41,18 @@ final class AuthorizationProxyConfiguration implements AopInfrastructureBean { ObjectProvider> customizers) { List advisors = new ArrayList<>(); provider.forEach(advisors::add); - AuthorizationAdvisorProxyFactory factory = AuthorizationAdvisorProxyFactory.withDefaults(); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(advisors); customizers.forEach((c) -> c.customize(factory)); - factory.setAdvisors(advisors); return factory; } @Bean @Role(BeanDefinition.ROLE_INFRASTRUCTURE) - static MethodInterceptor authorizeReturnObjectMethodInterceptor(ObjectProvider provider, + static MethodInterceptor authorizeReturnObjectMethodInterceptor( AuthorizationAdvisorProxyFactory authorizationProxyFactory) { AuthorizeReturnObjectMethodInterceptor interceptor = new AuthorizeReturnObjectMethodInterceptor( authorizationProxyFactory); - List advisors = new ArrayList<>(); - provider.forEach(advisors::add); - advisors.add(interceptor); - authorizationProxyFactory.setAdvisors(advisors); + authorizationProxyFactory.addAdvisors(interceptor); 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 42248b3da2..400f612b30 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,14 +86,28 @@ public final class AuthorizationAdvisorProxyFactory private static final TargetVisitor DEFAULT_VISITOR_SKIP_VALUE_TYPES = TargetVisitor.of(new ClassVisitor(), new IgnoreValueTypeVisitor(), DEFAULT_VISITOR); - private List advisors; + private List advisors = new ArrayList<>(); private TargetVisitor visitor = DEFAULT_VISITOR; - private AuthorizationAdvisorProxyFactory(List advisors) { - this.advisors = new ArrayList<>(advisors); - this.advisors.add(new AuthorizeReturnObjectMethodInterceptor(this)); - setAdvisors(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(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); } /** @@ -108,7 +122,9 @@ public final class AuthorizationAdvisorProxyFactory advisors.add(AuthorizationManagerAfterMethodInterceptor.postAuthorize()); advisors.add(new PreFilterAuthorizationMethodInterceptor()); advisors.add(new PostFilterAuthorizationMethodInterceptor()); - return new AuthorizationAdvisorProxyFactory(advisors); + AuthorizationAdvisorProxyFactory proxyFactory = new AuthorizationAdvisorProxyFactory(advisors); + proxyFactory.addAdvisors(new AuthorizeReturnObjectMethodInterceptor(proxyFactory)); + return proxyFactory; } /** @@ -123,7 +139,9 @@ public final class AuthorizationAdvisorProxyFactory advisors.add(AuthorizationManagerAfterReactiveMethodInterceptor.postAuthorize()); advisors.add(new PreFilterAuthorizationReactiveMethodInterceptor()); advisors.add(new PostFilterAuthorizationReactiveMethodInterceptor()); - return new AuthorizationAdvisorProxyFactory(advisors); + AuthorizationAdvisorProxyFactory proxyFactory = new AuthorizationAdvisorProxyFactory(advisors); + proxyFactory.addAdvisors(new AuthorizeReturnObjectMethodInterceptor(proxyFactory)); + return proxyFactory; } /** @@ -161,13 +179,21 @@ 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); @@ -179,7 +205,10 @@ 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 15389c2ec2..b6e816d59d 100644 --- a/core/src/test/java/org/springframework/security/authorization/AuthorizationAdvisorProxyFactoryTests.java +++ b/core/src/test/java/org/springframework/security/authorization/AuthorizationAdvisorProxyFactoryTests.java @@ -319,6 +319,29 @@ 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);