Skip to content

Partial evaluation misses ancestor group policies with extendChildren #51186

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
vramik wants to merge 1 commit into
base: main
Choose a base branch
from
+100 −1
Open

Partial evaluation misses ancestor group policies with extendChildren #51186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion ...n/src/main/java/org/keycloak/authorization/policy/provider/group/GroupPolicyProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/
package org.keycloak.authorization.policy.provider.group;

import java.util.ArrayList;
import java.util.List;
import java.util.function.BiFunction;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -111,7 +112,19 @@ public Stream<Policy> getPermissions(KeycloakSession session, ResourceType resou
StoreFactory storeFactory = provider.getStoreFactory();
ResourceServer resourceServer = storeFactory.getResourceServerStore().findByClient(adminPermissionsClient);
PolicyStore policyStore = storeFactory.getPolicyStore();
List<String> groupIds = user.getGroupsStream().map(GroupModel::getId).toList();
// getParent() issues a query per ancestor; may need optimization for deep hierarchies
List<String> groupIds = user.getGroupsStream()
.flatMap(group -> {
List<String> ids = new ArrayList<>();
GroupModel current = group;
while (current != null) {
Comment on lines +116 to +120
ids.add(current.getId());
current = current.getParent();
}
Comment on lines +115 to +123
return ids.stream();
})
.distinct()
.toList();

return policyStore.findDependentPolicies(resourceServer, resourceType.getType(), groupResourceType == null ? null : groupResourceType.getType(), GroupPolicyProviderFactory.ID, "groups", groupIds);
}
Expand Down
86 changes: 86 additions & 0 deletions ...base/src/test/java/org/keycloak/tests/admin/authz/fgap/UserResourceTypeFilteringTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -78,6 +79,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
Expand Down Expand Up @@ -466,6 +468,90 @@ public void testViewUserUsingRoleInheritedFromCompositeRole() {
assertEquals(1, search.size());
}

@Test
public void testExtendChildrenGroupDenyExcludesDeniedUserFromSearch() {
UserRepresentation myadmin = realm.admin().users().search("myadmin").get(0);
UserPolicyRepresentation allowMyAdmin = createUserPolicy(
realm, adminPermissionsClient, "Only My Admin User Policy", myadmin.getId());
createAllPermission(adminPermissionsClient, usersType, allowMyAdmin, Set.of(VIEW));

GroupRepresentation parentGroup = createGroup("fgap-parent-" + KeycloakModelUtils.generateId());
GroupRepresentation childGroup = new GroupRepresentation();
childGroup.setName("fgap-child-" + KeycloakModelUtils.generateId());
try (Response response = realm.admin().groups().group(parentGroup.getId()).subGroup(childGroup)) {
assertThat(response.getStatus(), is(Response.Status.CREATED.getStatusCode()));
childGroup.setId(ApiUtil.getCreatedId(response));
}
realm.admin().users().get(myadmin.getId()).joinGroup(childGroup.getId());

GroupPolicyRepresentation denyParentSubtree = new GroupPolicyRepresentation();
denyParentSubtree.setName("Deny Parent Subtree Policy");
denyParentSubtree.setLogic(Logic.NEGATIVE);
denyParentSubtree.addGroup(parentGroup.getId(), true);
try (Response response = adminPermissionsClient.authorization().policies().group()
.create(denyParentSubtree)) {
assertThat(response.getStatus(), is(Response.Status.CREATED.getStatusCode()));
}

UserRepresentation deniedUser = realm.admin().users().search("user-15").get(0);
createPermission(adminPermissionsClient, deniedUser.getId(), USERS_RESOURCE_TYPE, Set.of(VIEW), denyParentSubtree);

assertThrows(ForbiddenException.class, () -> realmAdminClient.realm(realm.getName())
.users().get(deniedUser.getId()).toRepresentation());

List<UserRepresentation> search = realmAdminClient.realm(realm.getName())
.users().search(deniedUser.getUsername(), 0, 10);
assertThat(search.stream().map(UserRepresentation::getId).toList(),
not(hasItems(deniedUser.getId())));
assertThat(realmAdminClient.realm(realm.getName()).users()
.count(null, null, null, deniedUser.getUsername()), is(0));
}

@Test
public void testExtendChildrenGroupDenyThreeLevelHierarchy() {
UserRepresentation myadmin = realm.admin().users().search("myadmin").get(0);
UserPolicyRepresentation allowMyAdmin = createUserPolicy(
realm, adminPermissionsClient, "Only My Admin User Policy", myadmin.getId());
createAllPermission(adminPermissionsClient, usersType, allowMyAdmin, Set.of(VIEW));

GroupRepresentation grandparentGroup = createGroup("fgap-grandparent-" + KeycloakModelUtils.generateId());
GroupRepresentation parentGroup = new GroupRepresentation();
parentGroup.setName("fgap-parent-" + KeycloakModelUtils.generateId());
try (Response response = realm.admin().groups().group(grandparentGroup.getId()).subGroup(parentGroup)) {
assertThat(response.getStatus(), is(Response.Status.CREATED.getStatusCode()));
parentGroup.setId(ApiUtil.getCreatedId(response));
}
GroupRepresentation childGroup = new GroupRepresentation();
childGroup.setName("fgap-child-" + KeycloakModelUtils.generateId());
try (Response response = realm.admin().groups().group(parentGroup.getId()).subGroup(childGroup)) {
assertThat(response.getStatus(), is(Response.Status.CREATED.getStatusCode()));
childGroup.setId(ApiUtil.getCreatedId(response));
}
realm.admin().users().get(myadmin.getId()).joinGroup(childGroup.getId());

GroupPolicyRepresentation denyGrandparentSubtree = new GroupPolicyRepresentation();
denyGrandparentSubtree.setName("Deny Grandparent Subtree Policy");
denyGrandparentSubtree.setLogic(Logic.NEGATIVE);
denyGrandparentSubtree.addGroup(grandparentGroup.getId(), true);
try (Response response = adminPermissionsClient.authorization().policies().group()
.create(denyGrandparentSubtree)) {
assertThat(response.getStatus(), is(Response.Status.CREATED.getStatusCode()));
}

UserRepresentation deniedUser = realm.admin().users().search("user-15").get(0);
createPermission(adminPermissionsClient, deniedUser.getId(), USERS_RESOURCE_TYPE, Set.of(VIEW), denyGrandparentSubtree);

assertThrows(ForbiddenException.class, () -> realmAdminClient.realm(realm.getName())
.users().get(deniedUser.getId()).toRepresentation());

List<UserRepresentation> search = realmAdminClient.realm(realm.getName())
.users().search(deniedUser.getUsername(), 0, 10);
assertThat(search.stream().map(UserRepresentation::getId).toList(),
not(hasItems(deniedUser.getId())));
assertThat(realmAdminClient.realm(realm.getName()).users()
.count(null, null, null, deniedUser.getUsername()), is(0));
}

@Test
public void testSessionEndpointRespectsUserViewPermission() {
UserRepresentation myadmin = realm.admin().users().search("myadmin").get(0);
Expand Down