Skip to content

Commit 9244989

Browse files
committed
Fix allOf/anyOf Abstain Logic
Closes gh-13069
1 parent 744b74f commit 9244989

File tree

3 files changed

+24
-5
lines changed

3 files changed

+24
-5
lines changed

core/src/main/java/org/springframework/security/authorization/AuthorizationManagers.java

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,11 +41,17 @@ public static <T> AuthorizationManager<T> anyOf(AuthorizationManager<T>... manag
4141
List<AuthorizationDecision> decisions = new ArrayList<>();
4242
for (AuthorizationManager<T> manager : managers) {
4343
AuthorizationDecision decision = manager.check(authentication, object);
44-
if (decision == null || decision.isGranted()) {
44+
if (decision == null) {
45+
continue;
46+
}
47+
if (decision.isGranted()) {
4548
return decision;
4649
}
4750
decisions.add(decision);
4851
}
52+
if (decisions.isEmpty()) {
53+
return new AuthorizationDecision(false);
54+
}
4955
return new CompositeAuthorizationDecision(false, decisions);
5056
};
5157
}
@@ -64,11 +70,17 @@ public static <T> AuthorizationManager<T> allOf(AuthorizationManager<T>... manag
6470
List<AuthorizationDecision> decisions = new ArrayList<>();
6571
for (AuthorizationManager<T> manager : managers) {
6672
AuthorizationDecision decision = manager.check(authentication, object);
67-
if (decision != null && !decision.isGranted()) {
73+
if (decision == null) {
74+
continue;
75+
}
76+
if (!decision.isGranted()) {
6877
return decision;
6978
}
7079
decisions.add(decision);
7180
}
81+
if (decisions.isEmpty()) {
82+
return new AuthorizationDecision(true);
83+
}
7284
return new CompositeAuthorizationDecision(true, decisions);
7385
};
7486
}

core/src/test/java/org/springframework/security/authorization/AuthorizationManagersTests.java

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,12 +36,14 @@ void checkAnyOfWhenOneGrantedThenGrantedDecision() {
3636
assertThat(decision.isGranted()).isTrue();
3737
}
3838

39+
// gh-13069
3940
@Test
40-
void checkAnyOfWhenOneAbstainedThenAbstainedDecision() {
41+
void checkAnyOfWhenAllNonAbstainingDeniesThenDeniedDecision() {
4142
AuthorizationManager<?> composed = AuthorizationManagers.anyOf((a, o) -> new AuthorizationDecision(false),
4243
(a, o) -> null);
4344
AuthorizationDecision decision = composed.check(null, null);
44-
assertThat(decision).isNull();
45+
assertThat(decision).isNotNull();
46+
assertThat(decision.isGranted()).isFalse();
4547
}
4648

4749
@Test
@@ -61,8 +63,9 @@ void checkAllOfWhenAllGrantedThenGrantedDecision() {
6163
assertThat(decision.isGranted()).isTrue();
6264
}
6365

66+
// gh-13069
6467
@Test
65-
void checkAllOfWhenOneAbstainedThenGrantedDecision() {
68+
void checkAllOfWhenAllNonAbstainingGrantsThenGrantedDecision() {
6669
AuthorizationManager<?> composed = AuthorizationManagers.allOf((a, o) -> new AuthorizationDecision(true),
6770
(a, o) -> null);
6871
AuthorizationDecision decision = composed.check(null, null);

docs/modules/ROOT/pages/migration/servlet/authorization.adoc

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -278,6 +278,10 @@ Read on to find the best match for your situation.
278278
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`].
279279

280280
However, if you do discover that you cannot accept the default authorization managers, you can use `AuthorizationManagers.allOf` to compose your own arrangement.
281+
282+
Note that there is a difference with `allOf`, which is that if all delegates abstain then it grants authorization.
283+
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.
284+
281285
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`].
282286

283287
==== I use `AffirmativeBased`

0 commit comments

Comments
 (0)