Skip to content

Commit 6393702

Browse files
committed
Fix allOf/anyOf Abstain Logic
Closes gh-13487
1 parent 83c0f42 commit 6393702

File tree

2 files changed

+20
-5
lines changed

2 files changed

+20
-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);

0 commit comments

Comments
 (0)