Polish AuthorizationAdvisorProxyFactory

- Ensure Reasonable Defaults
- Simplify Construction

Issue gh-14596

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<AuthorizationAdvisor> provider) {
 		List<AuthorizationAdvisor> advisors = new ArrayList<>();
 		provider.forEach(advisors::add);
-		AnnotationAwareOrderComparator.sort(advisors);
+		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
-		return new AuthorizationAdvisorProxyFactory(advisors);
+		factory.setAdvisors(advisors);
+		return factory;
 	}
 
 }

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<AuthorizationAdvisor> advisors;
+	private List<AuthorizationAdvisor> advisors = new ArrayList<>();
 
-	public AuthorizationAdvisorProxyFactory(AuthorizationAdvisor... advisors) {
+	public AuthorizationAdvisorProxyFactory() {
-		this.advisors = List.of(advisors);
+		List<AuthorizationAdvisor> advisors = new ArrayList<>();
-	}
+		advisors.add(AuthorizationManagerBeforeMethodInterceptor.preAuthorize());
-
+		advisors.add(AuthorizationManagerAfterMethodInterceptor.postAuthorize());
-	public AuthorizationAdvisorProxyFactory(Collection<AuthorizationAdvisor> advisors) {
+		advisors.add(new PreFilterAuthorizationMethodInterceptor());
-		this.advisors = List.copyOf(advisors);
+		advisors.add(new PostFilterAuthorizationMethodInterceptor());
-	}
+		setAdvisors(advisors);
-
-	/**
-	 * Create a new {@link AuthorizationAdvisorProxyFactory} that includes the given
-	 * advisors in addition to any advisors {@code this} instance already has.
-	 *
-	 * <p>
-	 * 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<AuthorizationAdvisor> merged = new ArrayList<>(this.advisors.size() + advisors.length);
-		merged.addAll(this.advisors);
-		merged.addAll(List.of(advisors));
-		AnnotationAwareOrderComparator.sort(merged);
-		return new AuthorizationAdvisorProxyFactory(merged);
 	}
 
 	/**
@@ -165,6 +153,30 @@ public final class AuthorizationAdvisorProxyFactory implements AuthorizationProx
 		return factory.getProxy();
 	}
 
+	/**
+	 * Add advisors that should be included to each proxy created.
+	 *
+	 * <p>
+	 * 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.
+	 *
+	 * <p>
+	 * All advisors are re-sorted by their advisor order.
+	 * @param advisors the advisors to add
+	 */
+	public void setAdvisors(Collection<AuthorizationAdvisor> advisors) {
+		this.advisors = new ArrayList<>(advisors);
+		AnnotationAwareOrderComparator.sort(this.advisors);
+	}
+
 	@SuppressWarnings("unchecked")
 	private <T> T proxyCast(T target) {
 		return (T) proxy(target);

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
+		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
-			.preAuthorize();
-		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
 		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
+		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
-			.preAuthorize();
-		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
 		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
+		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
-			.preAuthorize();
-		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
 		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
+		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
-			.preAuthorize();
-		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
 		List<Flight> flights = List.of(this.flight);
 		List<Flight> 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
+		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
-			.preAuthorize();
-		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
 		Set<Flight> flights = Set.of(this.flight);
 		Set<Flight> 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
+		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
-			.preAuthorize();
-		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
 		Queue<Flight> flights = new LinkedList<>(List.of(this.flight));
 		Queue<Flight> 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
+		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
-			.preAuthorize();
-		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
 		SortedSet<User> users = Collections.unmodifiableSortedSet(new TreeSet<>(Set.of(this.alan)));
 		SortedSet<User> secured = proxy(factory, users);
 		secured
@@ -165,9 +150,7 @@ public class AuthorizationAdvisorProxyFactoryTests {
 	@Test
 	public void proxyWhenImmutableSortedMapThenReturnsSecuredImmutableSortedMap() {
 		SecurityContextHolder.getContext().setAuthentication(this.user);
-		AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor
+		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
-			.preAuthorize();
-		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
 		SortedMap<String, User> users = Collections
 			.unmodifiableSortedMap(new TreeMap<>(Map.of(this.alan.getId(), this.alan)));
 		SortedMap<String, User> secured = proxy(factory, users);
@@ -180,9 +163,7 @@ public class AuthorizationAdvisorProxyFactoryTests {
 	@Test
 	public void proxyWhenImmutableMapThenReturnsSecuredImmutableMap() {
 		SecurityContextHolder.getContext().setAuthentication(this.user);
-		AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor
+		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
-			.preAuthorize();
-		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
 		Map<String, User> users = Map.of(this.alan.getId(), this.alan);
 		Map<String, User> 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
+		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
-			.preAuthorize();
-		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
 		List<Flight> flights = new ArrayList<>(List.of(this.flight));
 		List<Flight> 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
+		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
-			.preAuthorize();
-		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
 		Set<Flight> flights = new HashSet<>(Set.of(this.flight));
 		Set<Flight> 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
+		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
-			.preAuthorize();
-		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
 		SortedSet<User> users = new TreeSet<>(Set.of(this.alan));
 		SortedSet<User> 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
+		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
-			.preAuthorize();
-		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
 		SortedMap<String, User> users = new TreeMap<>(Map.of(this.alan.getId(), this.alan));
 		SortedMap<String, User> 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
+		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
-			.preAuthorize();
-		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
 		Map<String, User> users = new HashMap<>(Map.of(this.alan.getId(), this.alan));
 		Map<String, User> 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
+		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
-			.preAuthorize();
-		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
 		Optional<Flight> flights = Optional.of(this.flight);
 		assertThat(flights.get().getAltitude()).isEqualTo(35000d);
 		Optional<Flight> secured = proxy(factory, flights);
@@ -274,9 +243,7 @@ public class AuthorizationAdvisorProxyFactoryTests {
 	@Test
 	public void proxyWhenPreAuthorizeForStreamThenHonors() {
 		SecurityContextHolder.getContext().setAuthentication(this.user);
-		AuthorizationManagerBeforeMethodInterceptor preAuthorize = AuthorizationManagerBeforeMethodInterceptor
+		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
-			.preAuthorize();
-		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
 		Stream<Flight> flights = Stream.of(this.flight);
 		Stream<Flight> 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
+		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
-			.preAuthorize();
-		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
 		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
+		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
-			.preAuthorize();
-		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
 		Iterator<Flight> flights = List.of(this.flight).iterator();
 		Iterator<Flight> 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
+		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
-			.preAuthorize();
-		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
 		Iterable<User> users = new UserRepository();
 		Iterable<User> 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
+		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory();
-			.preAuthorize();
-		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(preAuthorize);
 		Class<Flight> 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();