From 6393702e70c4027fbf93aec857422add059801b0 Mon Sep 17 00:00:00 2001 From: Josh Cummings Date: Mon, 24 Apr 2023 14:43:12 -0600 Subject: [PATCH] Fix allOf/anyOf Abstain Logic Closes gh-13487 --- .../authorization/AuthorizationManagers.java | 16 ++++++++++++++-- .../AuthorizationManagersTests.java | 9 ++++++--- 2 files changed, 20 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 a2809155edc..6fa0ba80491 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 static AuthorizationManager anyOf(AuthorizationManager... manag List decisions = new ArrayList<>(); for (AuthorizationManager 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 static AuthorizationManager allOf(AuthorizationManager... manag List decisions = new ArrayList<>(); for (AuthorizationManager 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 48bdf646e1e..b05b2f41066 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 @@ void checkAnyOfWhenOneGrantedThenGrantedDecision() { 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 @@ void checkAllOfWhenAllGrantedThenGrantedDecision() { 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);