From f541bce49278c082abfa3991444ce29dbafedb45 Mon Sep 17 00:00:00 2001 From: Josh Cummings Date: Fri, 15 Mar 2024 11:27:36 -0600 Subject: [PATCH] Polish AuthorizationAdvisorProxyFactory - Ensure Reasonable Defaults - Simplify Construction Issue gh-14596 --- .../AuthorizationProxyConfiguration.java | 6 +- .../AuthorizationAdvisorProxyFactory.java | 60 +++++++------ ...AuthorizationAdvisorProxyFactoryTests.java | 85 +++++-------------- 3 files changed, 61 insertions(+), 90 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 6aa247c667..9b561eb221 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 @@ -25,7 +25,6 @@ import org.springframework.beans.factory.config.BeanDefinition; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.context.annotation.Role; -import org.springframework.core.annotation.AnnotationAwareOrderComparator; import org.springframework.security.authorization.AuthorizationAdvisorProxyFactory; import org.springframework.security.authorization.method.AuthorizationAdvisor; @@ -37,8 +36,9 @@ final class AuthorizationProxyConfiguration implements AopInfrastructureBean { static AuthorizationAdvisorProxyFactory authorizationProxyFactory(ObjectProvider provider) { List advisors = new ArrayList<>(); provider.forEach(advisors::add); - AnnotationAwareOrderComparator.sort(advisors); - return new AuthorizationAdvisorProxyFactory(advisors); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(); + factory.setAdvisors(advisors); + return factory; } } diff --git a/core/src/main/java/org/springframework/security/authorization/AuthorizationAdvisorProxyFactory.java b/core/src/main/java/org/springframework/security/authorization/AuthorizationAdvisorProxyFactory.java index 3fbb8df980..ceffb7ded1 100644 --- a/core/src/main/java/org/springframework/security/authorization/AuthorizationAdvisorProxyFactory.java +++ b/core/src/main/java/org/springframework/security/authorization/AuthorizationAdvisorProxyFactory.java @@ -40,6 +40,10 @@ import org.springframework.aop.Advisor; import org.springframework.aop.framework.ProxyFactory; import org.springframework.core.annotation.AnnotationAwareOrderComparator; import org.springframework.security.authorization.method.AuthorizationAdvisor; +import org.springframework.security.authorization.method.AuthorizationManagerAfterMethodInterceptor; +import org.springframework.security.authorization.method.AuthorizationManagerBeforeMethodInterceptor; +import org.springframework.security.authorization.method.PostFilterAuthorizationMethodInterceptor; +import org.springframework.security.authorization.method.PreFilterAuthorizationMethodInterceptor; import org.springframework.util.ClassUtils; /** @@ -71,31 +75,15 @@ import org.springframework.util.ClassUtils; */ public final class AuthorizationAdvisorProxyFactory implements AuthorizationProxyFactory { - private final Collection advisors; + private List advisors = new ArrayList<>(); - public AuthorizationAdvisorProxyFactory(AuthorizationAdvisor... advisors) { - this.advisors = List.of(advisors); - } - - public AuthorizationAdvisorProxyFactory(Collection advisors) { - this.advisors = List.copyOf(advisors); - } - - /** - * Create a new {@link AuthorizationAdvisorProxyFactory} that includes the given - * advisors in addition to any advisors {@code this} instance already has. - * - *

- * All advisors are re-sorted by their advisor order. - * @param advisors the advisors to add - * @return a new {@link AuthorizationAdvisorProxyFactory} instance - */ - public AuthorizationAdvisorProxyFactory withAdvisors(AuthorizationAdvisor... advisors) { - List merged = new ArrayList<>(this.advisors.size() + advisors.length); - merged.addAll(this.advisors); - merged.addAll(List.of(advisors)); - AnnotationAwareOrderComparator.sort(merged); - return new AuthorizationAdvisorProxyFactory(merged); + public AuthorizationAdvisorProxyFactory() { + List advisors = new ArrayList<>(); + advisors.add(AuthorizationManagerBeforeMethodInterceptor.preAuthorize()); + advisors.add(AuthorizationManagerAfterMethodInterceptor.postAuthorize()); + advisors.add(new PreFilterAuthorizationMethodInterceptor()); + advisors.add(new PostFilterAuthorizationMethodInterceptor()); + setAdvisors(advisors); } /** @@ -165,6 +153,30 @@ public final class AuthorizationAdvisorProxyFactory implements AuthorizationProx return factory.getProxy(); } + /** + * 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 + */ + public void setAdvisors(AuthorizationAdvisor... advisors) { + this.advisors = new ArrayList<>(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 + */ + public void setAdvisors(Collection advisors) { + this.advisors = new ArrayList<>(advisors); + AnnotationAwareOrderComparator.sort(this.advisors); + } + @SuppressWarnings("unchecked") private T proxyCast(T target) { return (T) proxy(target); 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 2a7852b28c..9ca1ce9b4d 100644 --- a/core/src/test/java/org/springframework/security/authorization/AuthorizationAdvisorProxyFactoryTests.java +++ b/core/src/test/java/org/springframework/security/authorization/AuthorizationAdvisorProxyFactoryTests.java @@ -41,7 +41,6 @@ import org.springframework.security.access.AccessDeniedException; import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.security.authentication.TestAuthentication; import org.springframework.security.authorization.method.AuthorizationAdvisor; -import org.springframework.security.authorization.method.AuthorizationManagerBeforeMethodInterceptor; import org.springframework.security.core.Authentication; import org.springframework.security.core.context.SecurityContextHolder; @@ -65,9 +64,7 @@ public class AuthorizationAdvisorProxyFactoryTests { @Test public void proxyWhenPreAuthorizeThenHonors() { SecurityContextHolder.getContext().setAuthentication(this.user); - AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor - .preAuthorize(); - AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(); Flight flight = new Flight(); assertThat(flight.getAltitude()).isEqualTo(35000d); Flight secured = proxy(factory, flight); @@ -78,9 +75,7 @@ public class AuthorizationAdvisorProxyFactoryTests { @Test public void proxyWhenPreAuthorizeOnInterfaceThenHonors() { SecurityContextHolder.getContext().setAuthentication(this.user); - AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor - .preAuthorize(); - AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(); assertThat(this.alan.getFirstName()).isEqualTo("alan"); User secured = proxy(factory, this.alan); assertThatExceptionOfType(AccessDeniedException.class).isThrownBy(secured::getFirstName); @@ -94,9 +89,7 @@ public class AuthorizationAdvisorProxyFactoryTests { @Test public void proxyWhenPreAuthorizeOnRecordThenHonors() { SecurityContextHolder.getContext().setAuthentication(this.user); - AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor - .preAuthorize(); - AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(); HasSecret repo = new Repository("secret"); assertThat(repo.secret()).isEqualTo("secret"); HasSecret secured = proxy(factory, repo); @@ -109,9 +102,7 @@ public class AuthorizationAdvisorProxyFactoryTests { @Test public void proxyWhenImmutableListThenReturnsSecuredImmutableList() { SecurityContextHolder.getContext().setAuthentication(this.user); - AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor - .preAuthorize(); - AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(); List flights = List.of(this.flight); List secured = proxy(factory, flights); secured.forEach( @@ -123,9 +114,7 @@ public class AuthorizationAdvisorProxyFactoryTests { @Test public void proxyWhenImmutableSetThenReturnsSecuredImmutableSet() { SecurityContextHolder.getContext().setAuthentication(this.user); - AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor - .preAuthorize(); - AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(); Set flights = Set.of(this.flight); Set secured = proxy(factory, flights); secured.forEach( @@ -137,9 +126,7 @@ public class AuthorizationAdvisorProxyFactoryTests { @Test public void proxyWhenQueueThenReturnsSecuredQueue() { SecurityContextHolder.getContext().setAuthentication(this.user); - AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor - .preAuthorize(); - AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(); Queue flights = new LinkedList<>(List.of(this.flight)); Queue secured = proxy(factory, flights); assertThat(flights.size()).isEqualTo(secured.size()); @@ -151,9 +138,7 @@ public class AuthorizationAdvisorProxyFactoryTests { @Test public void proxyWhenImmutableSortedSetThenReturnsSecuredImmutableSortedSet() { SecurityContextHolder.getContext().setAuthentication(this.user); - AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor - .preAuthorize(); - AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(); SortedSet users = Collections.unmodifiableSortedSet(new TreeSet<>(Set.of(this.alan))); SortedSet secured = proxy(factory, users); secured @@ -165,9 +150,7 @@ public class AuthorizationAdvisorProxyFactoryTests { @Test public void proxyWhenImmutableSortedMapThenReturnsSecuredImmutableSortedMap() { SecurityContextHolder.getContext().setAuthentication(this.user); - AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor - .preAuthorize(); - AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(); SortedMap users = Collections .unmodifiableSortedMap(new TreeMap<>(Map.of(this.alan.getId(), this.alan))); SortedMap secured = proxy(factory, users); @@ -180,9 +163,7 @@ public class AuthorizationAdvisorProxyFactoryTests { @Test public void proxyWhenImmutableMapThenReturnsSecuredImmutableMap() { SecurityContextHolder.getContext().setAuthentication(this.user); - AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor - .preAuthorize(); - AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(); Map users = Map.of(this.alan.getId(), this.alan); Map secured = proxy(factory, users); secured.forEach( @@ -194,9 +175,7 @@ public class AuthorizationAdvisorProxyFactoryTests { @Test public void proxyWhenMutableListThenReturnsSecuredMutableList() { SecurityContextHolder.getContext().setAuthentication(this.user); - AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor - .preAuthorize(); - AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(); List flights = new ArrayList<>(List.of(this.flight)); List secured = proxy(factory, flights); secured.forEach( @@ -208,9 +187,7 @@ public class AuthorizationAdvisorProxyFactoryTests { @Test public void proxyWhenMutableSetThenReturnsSecuredMutableSet() { SecurityContextHolder.getContext().setAuthentication(this.user); - AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor - .preAuthorize(); - AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(); Set flights = new HashSet<>(Set.of(this.flight)); Set secured = proxy(factory, flights); secured.forEach( @@ -222,9 +199,7 @@ public class AuthorizationAdvisorProxyFactoryTests { @Test public void proxyWhenMutableSortedSetThenReturnsSecuredMutableSortedSet() { SecurityContextHolder.getContext().setAuthentication(this.user); - AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor - .preAuthorize(); - AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(); SortedSet users = new TreeSet<>(Set.of(this.alan)); SortedSet secured = proxy(factory, users); secured.forEach((u) -> assertThatExceptionOfType(AccessDeniedException.class).isThrownBy(u::getFirstName)); @@ -235,9 +210,7 @@ public class AuthorizationAdvisorProxyFactoryTests { @Test public void proxyWhenMutableSortedMapThenReturnsSecuredMutableSortedMap() { SecurityContextHolder.getContext().setAuthentication(this.user); - AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor - .preAuthorize(); - AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(); SortedMap users = new TreeMap<>(Map.of(this.alan.getId(), this.alan)); SortedMap secured = proxy(factory, users); secured.forEach((id, u) -> assertThatExceptionOfType(AccessDeniedException.class).isThrownBy(u::getFirstName)); @@ -248,9 +221,7 @@ public class AuthorizationAdvisorProxyFactoryTests { @Test public void proxyWhenMutableMapThenReturnsSecuredMutableMap() { SecurityContextHolder.getContext().setAuthentication(this.user); - AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor - .preAuthorize(); - AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(); Map users = new HashMap<>(Map.of(this.alan.getId(), this.alan)); Map secured = proxy(factory, users); secured.forEach((id, u) -> assertThatExceptionOfType(AccessDeniedException.class).isThrownBy(u::getFirstName)); @@ -261,9 +232,7 @@ public class AuthorizationAdvisorProxyFactoryTests { @Test public void proxyWhenPreAuthorizeForOptionalThenHonors() { SecurityContextHolder.getContext().setAuthentication(this.user); - AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor - .preAuthorize(); - AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(); Optional flights = Optional.of(this.flight); assertThat(flights.get().getAltitude()).isEqualTo(35000d); Optional secured = proxy(factory, flights); @@ -274,9 +243,7 @@ public class AuthorizationAdvisorProxyFactoryTests { @Test public void proxyWhenPreAuthorizeForStreamThenHonors() { SecurityContextHolder.getContext().setAuthentication(this.user); - AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor - .preAuthorize(); - AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(); Stream flights = Stream.of(this.flight); Stream secured = proxy(factory, flights); assertThatExceptionOfType(AccessDeniedException.class).isThrownBy(() -> secured.forEach(Flight::getAltitude)); @@ -286,9 +253,7 @@ public class AuthorizationAdvisorProxyFactoryTests { @Test public void proxyWhenPreAuthorizeForArrayThenHonors() { SecurityContextHolder.getContext().setAuthentication(this.user); - AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor - .preAuthorize(); - AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(); Flight[] flights = { this.flight }; Flight[] secured = proxy(factory, flights); assertThatExceptionOfType(AccessDeniedException.class).isThrownBy(secured[0]::getAltitude); @@ -298,9 +263,7 @@ public class AuthorizationAdvisorProxyFactoryTests { @Test public void proxyWhenPreAuthorizeForIteratorThenHonors() { SecurityContextHolder.getContext().setAuthentication(this.user); - AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor - .preAuthorize(); - AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(); Iterator flights = List.of(this.flight).iterator(); Iterator secured = proxy(factory, flights); assertThatExceptionOfType(AccessDeniedException.class).isThrownBy(() -> secured.next().getAltitude()); @@ -310,9 +273,7 @@ public class AuthorizationAdvisorProxyFactoryTests { @Test public void proxyWhenPreAuthorizeForIterableThenHonors() { SecurityContextHolder.getContext().setAuthentication(this.user); - AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor - .preAuthorize(); - AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(); Iterable users = new UserRepository(); Iterable secured = proxy(factory, users); assertThatExceptionOfType(AccessDeniedException.class).isThrownBy(() -> secured.forEach(User::getFirstName)); @@ -321,9 +282,7 @@ public class AuthorizationAdvisorProxyFactoryTests { @Test public void proxyWhenPreAuthorizeForClassThenHonors() { - AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor - .preAuthorize(); - AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize); + AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(); Class clazz = proxy(factory, Flight.class); assertThat(clazz.getSimpleName()).contains("SpringCGLIB$$0"); Flight secured = proxy(factory, this.flight); @@ -334,12 +293,12 @@ public class AuthorizationAdvisorProxyFactoryTests { } @Test - public void withAdvisorsWhenProxyThenVisits() { + public void setAdvisorsWhenProxyThenVisits() { AuthorizationAdvisor advisor = mock(AuthorizationAdvisor.class); given(advisor.getAdvice()).willReturn(advisor); given(advisor.getPointcut()).willReturn(Pointcut.TRUE); AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(); - factory = factory.withAdvisors(advisor); + factory.setAdvisors(advisor); Flight flight = proxy(factory, this.flight); flight.getAltitude(); verify(advisor, atLeastOnce()).getPointcut();