Polish AuthorizeReturnObject SUpport

- Remove dependency on concrete implementation - Simplify AuthorizationAdvisorProxyFactory construction - Add ability to configure alternative proxying behaviors
jzheaux · Mar 14, 2024 · 769ed43 · 769ed43
1 parent 63b9d60
commit 769ed43
Show file tree

Hide file tree

Showing 12 changed files with 683 additions and 161 deletions.
diff --git a/...work/security/config/annotation/method/configuration/AuthorizationProxyConfiguration.java b/...work/security/config/annotation/method/configuration/AuthorizationProxyConfiguration.java
@@ -27,32 +27,59 @@
 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.AuthorizationProxyFactory;
+import org.springframework.security.authorization.AuthorizationProxyFactoryPredicate;
 import org.springframework.security.authorization.method.AuthorizationAdvisor;
 import org.springframework.security.authorization.method.AuthorizationProxyMethodInterceptor;
-import org.springframework.security.config.Customizer;
 
 @Configuration(proxyBeanMethods = false)
 final class AuthorizationProxyConfiguration implements AopInfrastructureBean {
 
 	@Bean
 	@Role(BeanDefinition.ROLE_INFRASTRUCTURE)
-	static AuthorizationAdvisorProxyFactory authorizationProxyFactory(ObjectProvider<AuthorizationAdvisor> provider,
-			ObjectProvider<Customizer<AuthorizationAdvisorProxyFactory>> customizers) {
+	static DelegatingAuthorizationProxyFactory authorizationProxyFactory(ObjectProvider<AuthorizationAdvisor> provider,
+			List<AuthorizationProxyFactoryPredicate> predicates) {
+		DelegatingAuthorizationProxyFactory delegating = new DelegatingAuthorizationProxyFactory(predicates);
 		List<AuthorizationAdvisor> advisors = new ArrayList<>();
 		provider.forEach(advisors::add);
-		AnnotationAwareOrderComparator.sort(advisors);
-		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(advisors);
-		customizers.forEach((customizer) -> customizer.customize(factory));
-		return factory;
+		delegating.defaults.setAdvisors(advisors);
+		return delegating;
 	}
 
 	@Bean
 	@Role(BeanDefinition.ROLE_INFRASTRUCTURE)
-	static MethodInterceptor authorizationProxyMethodInterceptor(
-			AuthorizationAdvisorProxyFactory authorizationProxyFactory) {
-		return new AuthorizationProxyMethodInterceptor(authorizationProxyFactory);
+	static MethodInterceptor authorizationProxyMethodInterceptor(ObjectProvider<AuthorizationAdvisor> provider,
+			DelegatingAuthorizationProxyFactory authorizationProxyFactory) {
+		AuthorizationProxyMethodInterceptor interceptor = new AuthorizationProxyMethodInterceptor(
+				authorizationProxyFactory);
+		List<AuthorizationAdvisor> advisors = new ArrayList<>();
+		provider.forEach(advisors::add);
+		advisors.add(interceptor);
+		authorizationProxyFactory.defaults.setAdvisors(advisors);
+		return interceptor;
+	}
+
+	private static class DelegatingAuthorizationProxyFactory implements AuthorizationProxyFactory {
+
+		private final List<AuthorizationProxyFactoryPredicate> predicates;
+
+		private final AuthorizationAdvisorProxyFactory defaults = new AuthorizationAdvisorProxyFactory();
+
+		DelegatingAuthorizationProxyFactory(List<AuthorizationProxyFactoryPredicate> predicates) {
+			this.predicates = new ArrayList<>(predicates);
+		}
+
+		@Override
+		public Object proxy(Object object) {
+			for (AuthorizationProxyFactoryPredicate factory : this.predicates) {
+				if (factory.proxies(object)) {
+					return factory.proxy(object);
+				}
+			}
+			return this.defaults.proxy(object);
+		}
+
 	}
 
 }
diff --git a/...urity/config/annotation/method/configuration/ReactiveAuthorizationProxyConfiguration.java b/...urity/config/annotation/method/configuration/ReactiveAuthorizationProxyConfiguration.java
@@ -0,0 +1,85 @@
+/*
+ * Copyright 2002-2024 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.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.security.config.annotation.method.configuration;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.aopalliance.intercept.MethodInterceptor;
+
+import org.springframework.aop.framework.AopInfrastructureBean;
+import org.springframework.beans.factory.ObjectProvider;
+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.security.authorization.AuthorizationProxyFactory;
+import org.springframework.security.authorization.AuthorizationProxyFactoryPredicate;
+import org.springframework.security.authorization.ReactiveAuthorizationAdvisorProxyFactory;
+import org.springframework.security.authorization.method.AuthorizationAdvisor;
+import org.springframework.security.authorization.method.AuthorizationProxyMethodInterceptor;
+
+@Configuration(proxyBeanMethods = false)
+final class ReactiveAuthorizationProxyConfiguration implements AopInfrastructureBean {
+
+	@Bean
+	@Role(BeanDefinition.ROLE_INFRASTRUCTURE)
+	static DelegatingAuthorizationProxyFactory authorizationProxyFactory(ObjectProvider<AuthorizationAdvisor> provider,
+			List<AuthorizationProxyFactoryPredicate> predicates) {
+		DelegatingAuthorizationProxyFactory delegating = new DelegatingAuthorizationProxyFactory(predicates);
+		List<AuthorizationAdvisor> advisors = new ArrayList<>();
+		provider.forEach(advisors::add);
+		delegating.defaults.setAdvisors(advisors);
+		return delegating;
+	}
+
+	@Bean
+	@Role(BeanDefinition.ROLE_INFRASTRUCTURE)
+	static MethodInterceptor authorizationProxyMethodInterceptor(ObjectProvider<AuthorizationAdvisor> provider,
+			DelegatingAuthorizationProxyFactory authorizationProxyFactory) {
+		AuthorizationProxyMethodInterceptor interceptor = new AuthorizationProxyMethodInterceptor(
+				authorizationProxyFactory);
+		List<AuthorizationAdvisor> advisors = new ArrayList<>();
+		provider.forEach(advisors::add);
+		advisors.add(interceptor);
+		authorizationProxyFactory.defaults.setAdvisors(advisors);
+		return interceptor;
+	}
+
+	private static class DelegatingAuthorizationProxyFactory implements AuthorizationProxyFactory {
+
+		private final List<AuthorizationProxyFactoryPredicate> predicates;
+
+		private final ReactiveAuthorizationAdvisorProxyFactory defaults = new ReactiveAuthorizationAdvisorProxyFactory();
+
+		DelegatingAuthorizationProxyFactory(List<AuthorizationProxyFactoryPredicate> predicates) {
+			this.predicates = new ArrayList<>(predicates);
+		}
+
+		@Override
+		public Object proxy(Object object) {
+			for (AuthorizationProxyFactoryPredicate factory : this.predicates) {
+				if (factory.proxies(object)) {
+					return factory.proxy(object);
+				}
+			}
+			return this.defaults.proxy(object);
+		}
+
+	}
+
+}
diff --git a/...ework/security/config/annotation/method/configuration/ReactiveMethodSecuritySelector.java b/...ework/security/config/annotation/method/configuration/ReactiveMethodSecuritySelector.java
@@ -51,7 +51,7 @@ public String[] selectImports(AnnotationMetadata importMetadata) {
 		else {
 			imports.add(ReactiveMethodSecurityConfiguration.class.getName());
 		}
-		imports.add(AuthorizationProxyConfiguration.class.getName());
+		imports.add(ReactiveAuthorizationProxyConfiguration.class.getName());
 		return imports.toArray(new String[0]);
 	}
 

diff --git a/...urity/config/annotation/method/configuration/PrePostMethodSecurityConfigurationTests.java b/...urity/config/annotation/method/configuration/PrePostMethodSecurityConfigurationTests.java
@@ -58,16 +58,16 @@
 import org.springframework.security.access.prepost.PostFilter;
 import org.springframework.security.access.prepost.PreAuthorize;
 import org.springframework.security.access.prepost.PreFilter;
-import org.springframework.security.authorization.AuthorizationAdvisorProxyFactory;
 import org.springframework.security.authorization.AuthorizationDecision;
 import org.springframework.security.authorization.AuthorizationEventPublisher;
 import org.springframework.security.authorization.AuthorizationManager;
+import org.springframework.security.authorization.AuthorizationProxyFactoryPredicate;
+import org.springframework.security.authorization.SkipAuthorizationProxyFactoryPredicate;
 import org.springframework.security.authorization.method.AuthorizationInterceptorsOrder;
 import org.springframework.security.authorization.method.AuthorizationManagerBeforeMethodInterceptor;
 import org.springframework.security.authorization.method.AuthorizeReturnObject;
 import org.springframework.security.authorization.method.MethodInvocationResult;
 import org.springframework.security.authorization.method.PrePostTemplateDefaults;
-import org.springframework.security.config.Customizer;
 import org.springframework.security.config.annotation.SecurityContextChangedListenerConfig;
 import org.springframework.security.config.core.GrantedAuthorityDefaults;
 import org.springframework.security.config.test.SpringTestContext;
@@ -1146,8 +1146,9 @@ List<String> resultsContainDave(List<String> list) {
 	static class AuthorizeResultConfig {
 
 		@Bean
-		static Customizer<AuthorizationAdvisorProxyFactory> returnObject() {
-			return (proxyFactory) -> proxyFactory.setSkipProxy(AuthorizationAdvisorProxyFactory.SKIP_VALUE_TYPES);
+		@Role(BeanDefinition.ROLE_INFRASTRUCTURE)
+		static AuthorizationProxyFactoryPredicate skipValueTypes() {
+			return SkipAuthorizationProxyFactoryPredicate.skipValueTypes();
 		}
 
 		@Bean

diff --git a/...rity/config/annotation/method/configuration/ReactiveMethodSecurityConfigurationTests.java b/...rity/config/annotation/method/configuration/ReactiveMethodSecurityConfigurationTests.java
@@ -30,8 +30,10 @@
 import reactor.test.StepVerifier;
 
 import org.springframework.beans.factory.annotation.Autowired;
+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.expression.EvaluationContext;
 import org.springframework.security.access.AccessDeniedException;
 import org.springframework.security.access.expression.SecurityExpressionRoot;
@@ -42,9 +44,9 @@
 import org.springframework.security.access.prepost.PreAuthorize;
 import org.springframework.security.access.prepost.PreFilter;
 import org.springframework.security.authentication.TestAuthentication;
-import org.springframework.security.authorization.AuthorizationAdvisorProxyFactory;
+import org.springframework.security.authorization.AuthorizationProxyFactoryPredicate;
+import org.springframework.security.authorization.SkipAuthorizationProxyFactoryPredicate;
 import org.springframework.security.authorization.method.AuthorizeReturnObject;
-import org.springframework.security.config.Customizer;
 import org.springframework.security.config.core.GrantedAuthorityDefaults;
 import org.springframework.security.config.test.SpringTestContext;
 import org.springframework.security.config.test.SpringTestContextExtension;
@@ -241,8 +243,9 @@ public void bar(String param) {
 	static class AuthorizeResultConfig {
 
 		@Bean
-		static Customizer<AuthorizationAdvisorProxyFactory> returnObject() {
-			return (proxyFactory) -> proxyFactory.setSkipProxy(AuthorizationAdvisorProxyFactory.SKIP_VALUE_TYPES);
+		@Role(BeanDefinition.ROLE_INFRASTRUCTURE)
+		static AuthorizationProxyFactoryPredicate skipValueTypes() {
+			return SkipAuthorizationProxyFactoryPredicate.skipValueTypes();
 		}
 
 		@Bean

diff --git a/...ain/java/org/springframework/security/authorization/AuthorizationAdvisorProxyFactory.java b/...ain/java/org/springframework/security/authorization/AuthorizationAdvisorProxyFactory.java
@@ -34,17 +34,16 @@
 import java.util.SortedSet;
 import java.util.TreeMap;
 import java.util.TreeSet;
-import java.util.function.Predicate;
 import java.util.stream.Stream;
 
-import reactor.core.publisher.Flux;
-import reactor.core.publisher.Mono;
-
 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.util.Assert;
+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;
 
 /**
@@ -76,39 +75,15 @@
  */
 public final class AuthorizationAdvisorProxyFactory implements AuthorizationProxyFactory {
 
-	private static final boolean isReactivePresent = ClassUtils.isPresent("reactor.core.publisher.Mono", null);
-
-	public static final Predicate<Class<?>> SKIP_VALUE_TYPES = ClassUtils::isSimpleValueType;
-
-	private final Collection<AuthorizationAdvisor> advisors;
-
-	private Predicate<Class<?>> skipProxy = (returnType) -> false;
-
-	public AuthorizationAdvisorProxyFactory(AuthorizationAdvisor... advisors) {
-		this.advisors = List.of(advisors);
-	}
+	private List<AuthorizationAdvisor> advisors = new ArrayList<>();
 
-	public AuthorizationAdvisorProxyFactory(Collection<AuthorizationAdvisor> 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.
-	 *
-	 * <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);
-		AuthorizationAdvisorProxyFactory factory = new AuthorizationAdvisorProxyFactory(merged);
-		factory.setSkipProxy(this.skipProxy);
-		return factory;
+	public AuthorizationAdvisorProxyFactory() {
+		List<AuthorizationAdvisor> advisors = new ArrayList<>();
+		advisors.add(AuthorizationManagerBeforeMethodInterceptor.preAuthorize());
+		advisors.add(AuthorizationManagerAfterMethodInterceptor.postAuthorize());
+		advisors.add(new PreFilterAuthorizationMethodInterceptor());
+		advisors.add(new PostFilterAuthorizationMethodInterceptor());
+		setAdvisors(advisors);
 	}
 
 	/**
@@ -134,9 +109,6 @@ public Object proxy(Object target) {
 		if (target == null) {
 			return null;
 		}
-		if (this.skipProxy.test(target.getClass())) {
-			return target;
-		}
 		if (target instanceof Class<?> targetClass) {
 			return proxyClass(targetClass);
 		}
@@ -173,14 +145,6 @@ public Object proxy(Object target) {
 		if (target instanceof Optional<?> optional) {
 			return proxyOptional(optional);
 		}
-		if (isReactivePresent) {
-			if (MonoProxySupport.isMono(target)) {
-				return MonoProxySupport.proxy(target, this);
-			}
-			if (FluxProxySupport.isFlux(target)) {
-				return FluxProxySupport.proxy(target, this);
-			}
-		}
 		ProxyFactory factory = new ProxyFactory(target);
 		for (Advisor advisor : this.advisors) {
 			factory.addAdvisors(advisor);
@@ -189,9 +153,28 @@ public Object proxy(Object target) {
 		return factory.getProxy();
 	}
 
-	public void setSkipProxy(Predicate<Class<?>> skipProxy) {
-		Assert.notNull(skipProxy, "skipProxy must not be null");
-		this.skipProxy = skipProxy;
+	/**
+	 * 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")
@@ -334,28 +317,4 @@ private Optional<?> proxyOptional(Optional<?> optional) {
 		return optional.map(this::proxy);
 	}
 
-	private static final class MonoProxySupport {
-
-		static boolean isMono(Object target) {
-			return target instanceof Mono;
-		}
-
-		static Mono<?> proxy(Object target, AuthorizationProxyFactory factory) {
-			return ((Mono<?>) target).map(factory::proxy);
-		}
-
-	}
-
-	private static final class FluxProxySupport {
-
-		static boolean isFlux(Object target) {
-			return target instanceof Flux;
-		}
-
-		static Flux<?> proxy(Object target, AuthorizationProxyFactory factory) {
-			return ((Flux<?>) target).map(factory::proxy);
-		}
-
-	}
-
 }