From 9244989b2e6423b150d4b6906b050ea46352181f Mon Sep 17 00:00:00 2001
From: Josh Cummings <josh.cummings@gmail.com>
Date: Mon, 24 Apr 2023 14:43:12 -0600
Subject: [PATCH] Fix allOf/anyOf Abstain Logic

Closes gh-13069
---
 .../authorization/AuthorizationManagers.java     | 16 ++++++++++++++--
 .../AuthorizationManagersTests.java              |  9 ++++++---
 .../pages/migration/servlet/authorization.adoc   |  4 ++++
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/core/src/main/java/org/springframework/security/authorization/AuthorizationManagers.java b/core/src/main/java/org/springframework/security/authorization/AuthorizationManagers.java
index a2809155ed..6fa0ba8049 100644
--- a/core/src/main/java/org/springframework/security/authorization/AuthorizationManagers.java
+++ b/core/src/main/java/org/springframework/security/authorization/AuthorizationManagers.java
@@ -41,11 +41,17 @@ public final class AuthorizationManagers {
 			List<AuthorizationDecision> decisions = new ArrayList<>();
 			for (AuthorizationManager<T> manager : managers) {
 				AuthorizationDecision decision = manager.check(authentication, object);
-				if (decision == null || decision.isGranted()) {
+				if (decision == null) {
+					continue;
+				}
+				if (decision.isGranted()) {
 					return decision;
 				}
 				decisions.add(decision);
 			}
+			if (decisions.isEmpty()) {
+				return new AuthorizationDecision(false);
+			}
 			return new CompositeAuthorizationDecision(false, decisions);
 		};
 	}
@@ -64,11 +70,17 @@ public final class AuthorizationManagers {
 			List<AuthorizationDecision> decisions = new ArrayList<>();
 			for (AuthorizationManager<T> manager : managers) {
 				AuthorizationDecision decision = manager.check(authentication, object);
-				if (decision != null && !decision.isGranted()) {
+				if (decision == null) {
+					continue;
+				}
+				if (!decision.isGranted()) {
 					return decision;
 				}
 				decisions.add(decision);
 			}
+			if (decisions.isEmpty()) {
+				return new AuthorizationDecision(true);
+			}
 			return new CompositeAuthorizationDecision(true, decisions);
 		};
 	}
diff --git a/core/src/test/java/org/springframework/security/authorization/AuthorizationManagersTests.java b/core/src/test/java/org/springframework/security/authorization/AuthorizationManagersTests.java
index 48bdf646e1..b05b2f4106 100644
--- a/core/src/test/java/org/springframework/security/authorization/AuthorizationManagersTests.java
+++ b/core/src/test/java/org/springframework/security/authorization/AuthorizationManagersTests.java
@@ -36,12 +36,14 @@ class AuthorizationManagersTests {
 		assertThat(decision.isGranted()).isTrue();
 	}
 
+	// gh-13069
 	@Test
-	void checkAnyOfWhenOneAbstainedThenAbstainedDecision() {
+	void checkAnyOfWhenAllNonAbstainingDeniesThenDeniedDecision() {
 		AuthorizationManager<?> composed = AuthorizationManagers.anyOf((a, o) -> new AuthorizationDecision(false),
 				(a, o) -> null);
 		AuthorizationDecision decision = composed.check(null, null);
-		assertThat(decision).isNull();
+		assertThat(decision).isNotNull();
+		assertThat(decision.isGranted()).isFalse();
 	}
 
 	@Test
@@ -61,8 +63,9 @@ class AuthorizationManagersTests {
 		assertThat(decision.isGranted()).isTrue();
 	}
 
+	// gh-13069
 	@Test
-	void checkAllOfWhenOneAbstainedThenGrantedDecision() {
+	void checkAllOfWhenAllNonAbstainingGrantsThenGrantedDecision() {
 		AuthorizationManager<?> composed = AuthorizationManagers.allOf((a, o) -> new AuthorizationDecision(true),
 				(a, o) -> null);
 		AuthorizationDecision decision = composed.check(null, null);
diff --git a/docs/modules/ROOT/pages/migration/servlet/authorization.adoc b/docs/modules/ROOT/pages/migration/servlet/authorization.adoc
index c94ebc60ab..524552f2f4 100644
--- a/docs/modules/ROOT/pages/migration/servlet/authorization.adoc
+++ b/docs/modules/ROOT/pages/migration/servlet/authorization.adoc
@@ -278,6 +278,10 @@ Read on to find the best match for your situation.
 If your application uses {security-api-url}org/springframework/security/access/vote/UnanimousBased.html[`UnanimousBased`] with the default voters, you likely need do nothing since unanimous-based is the default behavior with {security-api-url}org/springframework/security/config/annotation/method/configuration/EnableMethodSecurity.html[`@EnableMethodSecurity`].
 
 However, if you do discover that you cannot accept the default authorization managers, you can use `AuthorizationManagers.allOf` to compose your own arrangement.
+
+Note that there is a difference with `allOf`, which is that if all delegates abstain then it grants authorization.
+If you must deny authorization when all delegates abstain, please implement a composite {security-api-url}org/springframework/security/authorization/AuthorizationManager.html[`AuthorizationManager`] that takes the set of delegate ``AuthorizationManager``s into account.
+
 Having done that, please follow the details in the reference manual for xref:servlet/authorization/method-security.adoc#jc-method-security-custom-authorization-manager[adding a custom `AuthorizationManager`].
 
 ==== I use `AffirmativeBased`