From 354710acbc7466b1471c7d3aacc9b19ccda22719 Mon Sep 17 00:00:00 2001 From: vramik Date: Fri, 17 Jul 2026 11:33:13 +0200 Subject: [PATCH] Aggregate policy partial evaluation diverges from runtime semantics under FGAP v2 Closes #51143 Signed-off-by: vramik --- .../aggregated/AggregatePolicyProvider.java | 16 ++- .../fgap/UserResourceTypeFilteringTest.java | 100 ++++++++++++++++++ 2 files changed, 114 insertions(+), 2 deletions(-) diff --git a/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/aggregated/AggregatePolicyProvider.java b/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/aggregated/AggregatePolicyProvider.java index a153b49f8e3d..2c5e688e2b83 100644 --- a/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/aggregated/AggregatePolicyProvider.java +++ b/authz/policy/common/src/main/java/org/keycloak/authorization/policy/provider/aggregated/AggregatePolicyProvider.java @@ -40,6 +40,7 @@ import org.keycloak.models.RealmModel; import org.keycloak.models.UserModel; import org.keycloak.representations.idm.authorization.DecisionStrategy; +import org.keycloak.representations.idm.authorization.Logic; import org.keycloak.representations.idm.authorization.ResourceType; import org.jboss.logging.Logger; @@ -120,11 +121,22 @@ public boolean evaluate(KeycloakSession session, Policy policy, UserModel subjec PolicyProvider policyProvider = session.getProvider(AuthorizationProvider.class).getProvider(associatedPolicy.getType()); if (policyProvider instanceof PartialEvaluationPolicyProvider partialPolicyProvider) { - if (partialPolicyProvider.evaluate(session, associatedPolicy, subject)) { + boolean childResult = partialPolicyProvider.evaluate(session, associatedPolicy, subject); + + if (Logic.NEGATIVE.equals(associatedPolicy.getLogic())) { + childResult = !childResult; + } + + if (childResult) { grants++; } } else { - return false; + // we cannot partially evaluate this child, so the aggregate + // must resolve to "deny" regardless of its own Logic. The caller (PartialEvaluator) + // will invert our return value when policy.getLogic() == NEGATIVE, so we + // pre-compensate: return true for NEGATIVE (inverted to false by caller), + // false for POSITIVE (used as-is). Net result is always deny. + return Logic.NEGATIVE.equals(policy.getLogic()); } } diff --git a/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypeFilteringTest.java b/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypeFilteringTest.java index 060a2323882a..d49100caa87e 100644 --- a/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypeFilteringTest.java +++ b/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypeFilteringTest.java @@ -23,6 +23,7 @@ import java.util.stream.Collectors; import java.util.stream.Stream; +import jakarta.ws.rs.ForbiddenException; import jakarta.ws.rs.client.Client; import jakarta.ws.rs.client.WebTarget; import jakarta.ws.rs.core.GenericType; @@ -47,6 +48,7 @@ import org.keycloak.representations.idm.authorization.GroupPolicyRepresentation; import org.keycloak.representations.idm.authorization.Logic; import org.keycloak.representations.idm.authorization.RolePolicyRepresentation; +import org.keycloak.representations.idm.authorization.TimePolicyRepresentation; import org.keycloak.representations.idm.authorization.UserPolicyRepresentation; import org.keycloak.testframework.annotations.InjectAdminClient; import org.keycloak.testframework.annotations.InjectClient; @@ -78,6 +80,7 @@ import static org.hamcrest.Matchers.not; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @KeycloakIntegrationTest @@ -203,6 +206,103 @@ public void testViewUserUsingRolePolicy() { assertEquals(1, search.size()); } + @Test + public void testAggregatePolicyWithNegativeChildExcludesDeniedUsers() { + UserRepresentation myadmin = realm.admin().users().search("myadmin").get(0); + UserPolicyRepresentation denyMyAdmin = createUserPolicy( + Logic.NEGATIVE, realm, adminPermissionsClient, "Not My Admin User Policy", myadmin.getId()); + AggregatePolicyRepresentation aggregatePolicy = createAggregatedPolicy( + adminPermissionsClient, "Positive Aggregate With Negative Child Policy", + Logic.POSITIVE, DecisionStrategy.AFFIRMATIVE, denyMyAdmin.getName()); + Set deniedUsers = Set.of("user-0", "user-15", "user-30"); + + createPermission(adminPermissionsClient, deniedUsers, usersType, Set.of(VIEW), aggregatePolicy); + + UserRepresentation deniedUser = realm.admin().users().search("user-15").get(0); + assertThrows(ForbiddenException.class, () -> realmAdminClient.realm(realm.getName()) + .users().get(deniedUser.getId()).toRepresentation()); + + List search = realmAdminClient.realm(realm.getName()) + .users().search(null, -1, -1); + assertThat(search, is(empty())); + assertThat(realmAdminClient.realm(realm.getName()).users().count("user-"), is(0)); + } + + @Test + public void testNegativeAggregateWithUnsupportedChildExcludesDeniedUser() { + TimePolicyRepresentation timePolicy = new TimePolicyRepresentation(); + timePolicy.setName("Always Matching Time Policy"); + try (Response response = adminPermissionsClient.authorization().policies().time() + .create(timePolicy)) { + assertThat(response.getStatus(), is(Response.Status.CREATED.getStatusCode())); + } + + AggregatePolicyRepresentation aggregatePolicy = createAggregatedPolicy( + adminPermissionsClient, "Negative Aggregate With Time Child Policy", + Logic.NEGATIVE, DecisionStrategy.AFFIRMATIVE, timePolicy.getName()); + + UserRepresentation deniedUser = realm.admin().users().search("user-15").get(0); + createPermission(adminPermissionsClient, deniedUser.getId(), USERS_RESOURCE_TYPE, Set.of(VIEW), aggregatePolicy); + + assertThrows(ForbiddenException.class, () -> realmAdminClient.realm(realm.getName()) + .users().get(deniedUser.getId()).toRepresentation()); + + List search = realmAdminClient.realm(realm.getName()) + .users().search(deniedUser.getUsername(), 0, 10); + assertThat(search, is(empty())); + assertThat(realmAdminClient.realm(realm.getName()).users() + .count(null, null, null, deniedUser.getUsername()), is(0)); + } + + @Test + public void testAggregatePolicyUnanimousWithMixedChildren() { + UserRepresentation myadmin = realm.admin().users().search("myadmin").get(0); + UserPolicyRepresentation allowMyAdmin = createUserPolicy( + Logic.POSITIVE, realm, adminPermissionsClient, "Allow My Admin User Policy", myadmin.getId()); + UserPolicyRepresentation denyMyAdmin = createUserPolicy( + Logic.NEGATIVE, realm, adminPermissionsClient, "Deny My Admin User Policy", myadmin.getId()); + AggregatePolicyRepresentation aggregatePolicy = createAggregatedPolicy( + adminPermissionsClient, "Unanimous Aggregate With Mixed Children", + Logic.POSITIVE, DecisionStrategy.UNANIMOUS, allowMyAdmin.getName(), denyMyAdmin.getName()); + + UserRepresentation deniedUser = realm.admin().users().search("user-15").get(0); + createPermission(adminPermissionsClient, deniedUser.getId(), USERS_RESOURCE_TYPE, Set.of(VIEW), aggregatePolicy); + + assertThrows(ForbiddenException.class, () -> realmAdminClient.realm(realm.getName()) + .users().get(deniedUser.getId()).toRepresentation()); + + List search = realmAdminClient.realm(realm.getName()) + .users().search(deniedUser.getUsername(), 0, 10); + assertThat(search, is(empty())); + assertThat(realmAdminClient.realm(realm.getName()).users() + .count(null, null, null, deniedUser.getUsername()), is(0)); + } + + @Test + public void testNestedAggregateWithNegativeChild() { + UserRepresentation myadmin = realm.admin().users().search("myadmin").get(0); + UserPolicyRepresentation denyMyAdmin = createUserPolicy( + Logic.NEGATIVE, realm, adminPermissionsClient, "Not My Admin User Policy", myadmin.getId()); + AggregatePolicyRepresentation innerAggregate = createAggregatedPolicy( + adminPermissionsClient, "Inner Aggregate With Negative Child", + Logic.POSITIVE, DecisionStrategy.AFFIRMATIVE, denyMyAdmin.getName()); + AggregatePolicyRepresentation outerAggregate = createAggregatedPolicy( + adminPermissionsClient, "Outer Aggregate Wrapping Inner", + Logic.POSITIVE, DecisionStrategy.AFFIRMATIVE, innerAggregate.getName()); + + UserRepresentation deniedUser = realm.admin().users().search("user-15").get(0); + createPermission(adminPermissionsClient, deniedUser.getId(), USERS_RESOURCE_TYPE, Set.of(VIEW), outerAggregate); + + assertThrows(ForbiddenException.class, () -> realmAdminClient.realm(realm.getName()) + .users().get(deniedUser.getId()).toRepresentation()); + + List search = realmAdminClient.realm(realm.getName()) + .users().search(deniedUser.getUsername(), 0, 10); + assertThat(search, is(empty())); + assertThat(realmAdminClient.realm(realm.getName()).users() + .count(null, null, null, deniedUser.getUsername()), is(0)); + } + @Test public void testViewUserUsingMultiplePolicies() { List search = realmAdminClient.realm(realm.getName()).users().search(null, 0, 10);