Polish AuthorizationManager#authorize

Issue gh-14843
spring-projects · Oct 14, 2024 · 9ce5a76 · 9ce5a76
1 parent e764492
commit 9ce5a76
Show file tree

Hide file tree

Showing 20 changed files with 160 additions and 73 deletions.
diff --git a/...g/src/main/java/org/springframework/security/config/http/DefaultFilterChainValidator.java b/...g/src/main/java/org/springframework/security/config/http/DefaultFilterChainValidator.java
@@ -221,9 +221,8 @@ private boolean checkLoginPageIsPublic(List<Filter> filters, FilterInvocation lo
 			AuthorizationManager<HttpServletRequest> authorizationManager = authorizationFilter
 				.getAuthorizationManager();
 			try {
-				AuthorizationResult decision = authorizationManager.authorize(() -> TEST,
-						loginRequest.getHttpRequest());
-				return decision != null && decision.isGranted();
+				AuthorizationResult result = authorizationManager.authorize(() -> TEST, loginRequest.getHttpRequest());
+				return result != null && result.isGranted();
 			}
 			catch (Exception ex) {
 				return false;
@@ -253,9 +252,8 @@ private Supplier<Boolean> deriveAnonymousCheck(List<Filter> filters, FilterInvoc
 			return () -> {
 				AuthorizationManager<HttpServletRequest> authorizationManager = authorizationFilter
 					.getAuthorizationManager();
-				AuthorizationResult decision = authorizationManager.authorize(() -> token,
-						loginRequest.getHttpRequest());
-				return decision != null && decision.isGranted();
+				AuthorizationResult result = authorizationManager.authorize(() -> token, loginRequest.getHttpRequest());
+				return result != null && result.isGranted();
 			};
 		}
 		return () -> true;

diff --git a/...va/org/springframework/security/config/method/PointcutDelegatingAuthorizationManager.java b/...va/org/springframework/security/config/method/PointcutDelegatingAuthorizationManager.java
@@ -25,6 +25,7 @@
 import org.springframework.aop.support.AopUtils;
 import org.springframework.security.authorization.AuthorizationDecision;
 import org.springframework.security.authorization.AuthorizationManager;
+import org.springframework.security.authorization.AuthorizationResult;
 import org.springframework.security.core.Authentication;
 
 class PointcutDelegatingAuthorizationManager implements AuthorizationManager<MethodInvocation> {
@@ -37,11 +38,24 @@ class PointcutDelegatingAuthorizationManager implements AuthorizationManager<Met
 
 	@Override
 	public AuthorizationDecision check(Supplier<Authentication> authentication, MethodInvocation object) {
+		AuthorizationResult result = authorize(authentication, object);
+		if (result == null) {
+			return null;
+		}
+		if (result instanceof AuthorizationDecision decision) {
+			return decision;
+		}
+		throw new IllegalArgumentException(
+				"Please either call authorize or ensure that the returned result is of type AuthorizationDecision");
+	}
+
+	@Override
+	public AuthorizationResult authorize(Supplier<Authentication> authentication, MethodInvocation object) {
 		for (Map.Entry<Pointcut, AuthorizationManager<MethodInvocation>> entry : this.managers.entrySet()) {
 			Class<?> targetClass = (object.getThis() != null) ? AopUtils.getTargetClass(object.getThis()) : null;
 			if (entry.getKey().getClassFilter().matches(targetClass)
 					&& entry.getKey().getMethodMatcher().matches(object.getMethod(), targetClass)) {
-				return entry.getValue().check(authentication, object);
+				return entry.getValue().authorize(authentication, object);
 			}
 		}
 		return new AuthorizationDecision(false);

diff --git a/.../springframework/security/config/method/InterceptMethodsBeanDefinitionDecoratorTests.java b/.../springframework/security/config/method/InterceptMethodsBeanDefinitionDecoratorTests.java
@@ -168,6 +168,7 @@ public void transactionalAuthorizationManagerMethodsShouldBeSecured() {
 
 	@Test
 	public void targetCustomAuthorizationManagerUsed() {
+		given(this.mockAuthorizationManager.authorize(any(), any())).willCallRealMethod();
 		given(this.mockAuthorizationManager.check(any(), any())).willReturn(new AuthorizationDecision(true));
 		this.targetCustomAuthorizationManager.doSomething();
 		verify(this.mockAuthorizationManager).check(any(), any());

diff --git a/...c/main/java/org/springframework/security/authorization/AuthorityAuthorizationManager.java b/...c/main/java/org/springframework/security/authorization/AuthorityAuthorizationManager.java
@@ -139,7 +139,9 @@ private static String[] toNamedRolesArray(String rolePrefix, String[] roles) {
 	 * @param authentication the {@link Supplier} of the {@link Authentication} to check
 	 * @param object the {@link T} object to check
 	 * @return an {@link AuthorizationDecision}
+	 * @deprecated please use {@link #authorize(Supplier, Object)} instead
 	 */
+	@Deprecated
 	@Override
 	public AuthorizationDecision check(Supplier<Authentication> authentication, T object) {
 		return this.delegate.check(authentication, this.authorities);

diff --git a/core/src/main/java/org/springframework/security/authorization/AuthorizationManagers.java b/core/src/main/java/org/springframework/security/authorization/AuthorizationManagers.java
@@ -18,6 +18,9 @@
 
 import java.util.ArrayList;
 import java.util.List;
+import java.util.function.Supplier;
+
+import org.springframework.security.core.Authentication;
 
 /**
  * A factory class to create an {@link AuthorizationManager} instances.
@@ -55,22 +58,22 @@ public static <T> AuthorizationManager<T> anyOf(AuthorizationManager<T>... manag
 	@SafeVarargs
 	public static <T> AuthorizationManager<T> anyOf(AuthorizationDecision allAbstainDefaultDecision,
 			AuthorizationManager<T>... managers) {
-		return (authentication, object) -> {
-			List<AuthorizationDecision> decisions = new ArrayList<>();
+		return (AuthorizationManagerCheckAdapter<T>) (authentication, object) -> {
+			List<AuthorizationResult> results = new ArrayList<>();
 			for (AuthorizationManager<T> manager : managers) {
-				AuthorizationDecision decision = manager.check(authentication, object);
-				if (decision == null) {
+				AuthorizationResult result = manager.authorize(authentication, object);
+				if (result == null) {
 					continue;
 				}
-				if (decision.isGranted()) {
-					return decision;
+				if (result.isGranted()) {
+					return result;
 				}
-				decisions.add(decision);
+				results.add(result);
 			}
-			if (decisions.isEmpty()) {
+			if (results.isEmpty()) {
 				return allAbstainDefaultDecision;
 			}
-			return new CompositeAuthorizationDecision(false, decisions);
+			return new CompositeAuthorizationDecision(false, results);
 		};
 	}
 
@@ -101,22 +104,22 @@ public static <T> AuthorizationManager<T> allOf(AuthorizationManager<T>... manag
 	@SafeVarargs
 	public static <T> AuthorizationManager<T> allOf(AuthorizationDecision allAbstainDefaultDecision,
 			AuthorizationManager<T>... managers) {
-		return (authentication, object) -> {
-			List<AuthorizationDecision> decisions = new ArrayList<>();
+		return (AuthorizationManagerCheckAdapter<T>) (authentication, object) -> {
+			List<AuthorizationResult> results = new ArrayList<>();
 			for (AuthorizationManager<T> manager : managers) {
-				AuthorizationDecision decision = manager.check(authentication, object);
-				if (decision == null) {
+				AuthorizationResult result = manager.authorize(authentication, object);
+				if (result == null) {
 					continue;
 				}
-				if (!decision.isGranted()) {
-					return decision;
+				if (!result.isGranted()) {
+					return result;
 				}
-				decisions.add(decision);
+				results.add(result);
 			}
-			if (decisions.isEmpty()) {
+			if (results.isEmpty()) {
 				return allAbstainDefaultDecision;
 			}
-			return new CompositeAuthorizationDecision(true, decisions);
+			return new CompositeAuthorizationDecision(true, results);
 		};
 	}
 
@@ -131,11 +134,11 @@ public static <T> AuthorizationManager<T> allOf(AuthorizationDecision allAbstain
 	 */
 	public static <T> AuthorizationManager<T> not(AuthorizationManager<T> manager) {
 		return (authentication, object) -> {
-			AuthorizationDecision decision = manager.check(authentication, object);
-			if (decision == null) {
+			AuthorizationResult result = manager.authorize(authentication, object);
+			if (result == null) {
 				return null;
 			}
-			return new NotAuthorizationDecision(decision);
+			return new NotAuthorizationDecision(result);
 		};
 	}
 
@@ -144,34 +147,53 @@ private AuthorizationManagers() {
 
 	private static final class CompositeAuthorizationDecision extends AuthorizationDecision {
 
-		private final List<AuthorizationDecision> decisions;
+		private final List<AuthorizationResult> results;
 
-		private CompositeAuthorizationDecision(boolean granted, List<AuthorizationDecision> decisions) {
+		private CompositeAuthorizationDecision(boolean granted, List<AuthorizationResult> results) {
 			super(granted);
-			this.decisions = decisions;
+			this.results = results;
 		}
 
 		@Override
 		public String toString() {
-			return "CompositeAuthorizationDecision [decisions=" + this.decisions + ']';
+			return "CompositeAuthorizationDecision [results=" + this.results + ']';
 		}
 
 	}
 
 	private static final class NotAuthorizationDecision extends AuthorizationDecision {
 
-		private final AuthorizationDecision decision;
+		private final AuthorizationResult result;
 
-		private NotAuthorizationDecision(AuthorizationDecision decision) {
-			super(!decision.isGranted());
-			this.decision = decision;
+		private NotAuthorizationDecision(AuthorizationResult result) {
+			super(!result.isGranted());
+			this.result = result;
 		}
 
 		@Override
 		public String toString() {
-			return "NotAuthorizationDecision [decision=" + this.decision + ']';
+			return "NotAuthorizationDecision [result=" + this.result + ']';
 		}
 
 	}
 
+	private interface AuthorizationManagerCheckAdapter<T> extends AuthorizationManager<T> {
+
+		@Override
+		default AuthorizationDecision check(Supplier<Authentication> authentication, T object) {
+			AuthorizationResult result = authorize(authentication, object);
+			if (result == null) {
+				return null;
+			}
+			if (result instanceof AuthorizationDecision decision) {
+				return decision;
+			}
+			throw new IllegalArgumentException(
+					"please call #authorize or ensure that the result is of type AuthorizationDecision");
+		}
+
+		AuthorizationResult authorize(Supplier<Authentication> authentication, T object);
+
+	}
+
 }
diff --git a/...main/java/org/springframework/security/authorization/AuthorizationObservationContext.java b/...main/java/org/springframework/security/authorization/AuthorizationObservationContext.java
@@ -33,8 +33,6 @@ public class AuthorizationObservationContext<T> extends Observation.Context {
 
 	private final T object;
 
-	private AuthorizationDecision decision;
-
 	private AuthorizationResult authorizationResult;
 
 	public AuthorizationObservationContext(T object) {
@@ -77,9 +75,14 @@ public T getObject() {
 	 */
 	@Deprecated
 	public AuthorizationDecision getDecision() {
-		Assert.isInstanceOf(AuthorizationDecision.class, this.authorizationResult,
+		if (this.authorizationResult == null) {
+			return null;
+		}
+		if (this.authorizationResult instanceof AuthorizationDecision decision) {
+			return decision;
+		}
+		throw new IllegalArgumentException(
 				"Please call getAuthorizationResult instead. If you must call getDecision, please ensure that the result you provide is of type AuthorizationDecision");
-		return (AuthorizationDecision) this.authorizationResult;
 	}
 
 	/**
@@ -89,9 +92,7 @@ public AuthorizationDecision getDecision() {
 	 */
 	@Deprecated
 	public void setDecision(AuthorizationDecision decision) {
-		Assert.isInstanceOf(AuthorizationDecision.class, decision,
-				"Please call setAuthorizationResult instead. If you must call getDecision, please ensure that the result you provide is of type AuthorizationDecision");
-		this.decision = decision;
+		this.authorizationResult = decision;
 	}
 
 	/**

diff --git a/...main/java/org/springframework/security/authorization/ObservationAuthorizationManager.java b/...main/java/org/springframework/security/authorization/ObservationAuthorizationManager.java
@@ -61,6 +61,10 @@ public ObservationAuthorizationManager(ObservationRegistry registry, Authorizati
 		}
 	}
 
+	/**
+	 * @deprecated please use {@link #authorize(Supplier, Object)} instead
+	 */
+	@Deprecated
 	@Override
 	public AuthorizationDecision check(Supplier<Authentication> authentication, T object) {
 		AuthorizationObservationContext<T> context = new AuthorizationObservationContext<>(object);

diff --git a/...a/org/springframework/security/authorization/ObservationReactiveAuthorizationManager.java b/...a/org/springframework/security/authorization/ObservationReactiveAuthorizationManager.java
@@ -56,6 +56,10 @@ public ObservationReactiveAuthorizationManager(ObservationRegistry registry,
 		}
 	}
 
+	/**
+	 * @deprecated please use {@link #authorize(Mono, Object)} instead
+	 */
+	@Deprecated
 	@Override
 	public Mono<AuthorizationDecision> check(Mono<Authentication> authentication, T object) {
 		AuthorizationObservationContext<T> context = new AuthorizationObservationContext<>(object);

diff --git a/...ngframework/security/authorization/method/AuthorizationManagerAfterMethodInterceptor.java b/...ngframework/security/authorization/method/AuthorizationManagerAfterMethodInterceptor.java
@@ -182,22 +182,22 @@ public void setSecurityContextHolderStrategy(SecurityContextHolderStrategy strat
 	private Object attemptAuthorization(MethodInvocation mi, Object result) {
 		this.logger.debug(LogMessage.of(() -> "Authorizing method invocation " + mi));
 		MethodInvocationResult object = new MethodInvocationResult(mi, result);
-		AuthorizationResult decision = this.authorizationManager.authorize(this::getAuthentication, object);
-		this.eventPublisher.publishAuthorizationEvent(this::getAuthentication, object, decision);
-		if (decision != null && !decision.isGranted()) {
+		AuthorizationResult authorizationResult = this.authorizationManager.authorize(this::getAuthentication, object);
+		this.eventPublisher.publishAuthorizationEvent(this::getAuthentication, object, authorizationResult);
+		if (authorizationResult != null && !authorizationResult.isGranted()) {
 			this.logger.debug(LogMessage.of(() -> "Failed to authorize " + mi + " with authorization manager "
-					+ this.authorizationManager + " and decision " + decision));
-			return handlePostInvocationDenied(object, decision);
+					+ this.authorizationManager + " and authorizationResult " + authorizationResult));
+			return handlePostInvocationDenied(object, authorizationResult);
 		}
 		this.logger.debug(LogMessage.of(() -> "Authorized method invocation " + mi));
 		return result;
 	}
 
-	private Object handlePostInvocationDenied(MethodInvocationResult mi, AuthorizationResult decision) {
+	private Object handlePostInvocationDenied(MethodInvocationResult mi, AuthorizationResult result) {
 		if (this.authorizationManager instanceof MethodAuthorizationDeniedHandler deniedHandler) {
-			return deniedHandler.handleDeniedInvocationResult(mi, decision);
+			return deniedHandler.handleDeniedInvocationResult(mi, result);
 		}
-		return this.defaultHandler.handleDeniedInvocationResult(mi, decision);
+		return this.defaultHandler.handleDeniedInvocationResult(mi, result);
 	}
 
 	private Authentication getAuthentication() {

diff --git a/...gframework/security/authorization/method/AuthorizationManagerBeforeMethodInterceptor.java b/...gframework/security/authorization/method/AuthorizationManagerBeforeMethodInterceptor.java
@@ -246,18 +246,18 @@ public void setSecurityContextHolderStrategy(SecurityContextHolderStrategy secur
 
 	private Object attemptAuthorization(MethodInvocation mi) throws Throwable {
 		this.logger.debug(LogMessage.of(() -> "Authorizing method invocation " + mi));
-		AuthorizationResult decision;
+		AuthorizationResult result;
 		try {
-			decision = this.authorizationManager.authorize(this::getAuthentication, mi);
+			result = this.authorizationManager.authorize(this::getAuthentication, mi);
 		}
 		catch (AuthorizationDeniedException denied) {
 			return handle(mi, denied);
 		}
-		this.eventPublisher.publishAuthorizationEvent(this::getAuthentication, mi, decision);
-		if (decision != null && !decision.isGranted()) {
+		this.eventPublisher.publishAuthorizationEvent(this::getAuthentication, mi, result);
+		if (result != null && !result.isGranted()) {
 			this.logger.debug(LogMessage.of(() -> "Failed to authorize " + mi + " with authorization manager "
-					+ this.authorizationManager + " and decision " + decision));
-			return handle(mi, decision);
+					+ this.authorizationManager + " and result " + result));
+			return handle(mi, result);
 		}
 		this.logger.debug(LogMessage.of(() -> "Authorized method invocation " + mi));
 		return proceed(mi);
@@ -282,11 +282,11 @@ private Object handle(MethodInvocation mi, AuthorizationDeniedException denied)
 		return this.defaultHandler.handleDeniedInvocation(mi, denied);
 	}
 
-	private Object handle(MethodInvocation mi, AuthorizationResult decision) {
+	private Object handle(MethodInvocation mi, AuthorizationResult result) {
 		if (this.authorizationManager instanceof MethodAuthorizationDeniedHandler handler) {
-			return handler.handleDeniedInvocation(mi, decision);
+			return handler.handleDeniedInvocation(mi, result);
 		}
-		return this.defaultHandler.handleDeniedInvocation(mi, decision);
+		return this.defaultHandler.handleDeniedInvocation(mi, result);
 	}
 
 	private Authentication getAuthentication() {