Skip to content
Jump to bottom

Automated Test: feature-groups-authz-implementation #325

Closed
wants to merge 1 commit into from
+831 −126
Closed

Automated Test: feature-groups-authz-implementation #325

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 4 additions & 8 deletions rest/admin-ui-ext/src/main/java/org/keycloak/admin/ui/rest/BruteForceUsersResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,16 +144,12 @@ public final Stream<BruteUser> searchUser(@QueryParam("search") String search,
private Stream<BruteUser> searchForUser(Map<String, String> attributes, RealmModel realm, UserPermissionEvaluator usersEvaluator, Boolean briefRepresentation, Integer firstResult, Integer maxResults, Boolean includeServiceAccounts) {
attributes.put(UserModel.INCLUDE_SERVICE_ACCOUNT, includeServiceAccounts.toString());

if (!auth.users().canView()) {
Set<String> groupModels = auth.groups().getGroupsWithViewPermission();

if (!groupModels.isEmpty()) {
session.setAttribute(UserModel.GROUPS, groupModels);
}
Set<String> groupIds = auth.groups().getGroupIdsWithViewPermission();
if (!groupIds.isEmpty()) {
session.setAttribute(UserModel.GROUPS, groupIds);
}

Stream<UserModel> userModels = session.users().searchForUserStream(realm, attributes, firstResult, maxResults);
return toRepresentation(realm, usersEvaluator, briefRepresentation, userModels);
return toRepresentation(realm, usersEvaluator, briefRepresentation, session.users().searchForUserStream(realm, attributes, firstResult, maxResults));
}

private Stream<BruteUser> toRepresentation(RealmModel realm, UserPermissionEvaluator usersEvaluator,
Expand Down
50 changes: 37 additions & 13 deletions server-spi-private/src/main/java/org/keycloak/authorization/AdminPermissionsSchema.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.keycloak.models.ClientModel;
import org.keycloak.models.ClientProvider;
import org.keycloak.models.Constants;
import org.keycloak.models.GroupModel;
import org.keycloak.models.KeycloakSession;
import org.keycloak.models.ModelException;
import org.keycloak.models.ModelIllegalStateException;
Expand All @@ -50,27 +51,42 @@
import org.keycloak.representations.idm.authorization.ScopeRepresentation;

public class AdminPermissionsSchema extends AuthorizationSchema {

public static final String USERS_RESOURCE_TYPE = "Users";

public static final String CLIENTS_RESOURCE_TYPE = "Clients";
public static final String GROUPS_RESOURCE_TYPE = "Groups";
public static final String USERS_RESOURCE_TYPE = "Users";

//scopes
// common scopes
public static final String MANAGE = "manage";
public static final String VIEW = "view";
public static final String IMPERSONATE = "impersonate";
public static final String MAP_ROLES = "map-roles";
public static final String MANAGE_GROUP_MEMBERSHIP = "manage-group-membership";

// client specific scopes
public static final String CONFIGURE = "configure";
public static final String MAP_ROLES_CLIENT_SCOPE = "map-roles-client-scope";
public static final String MAP_ROLES_COMPOSITE = "map-roles-composite";

public static final ResourceType USERS = new ResourceType(USERS_RESOURCE_TYPE, Set.of(MANAGE, VIEW, IMPERSONATE, MAP_ROLES, MANAGE_GROUP_MEMBERSHIP));
// group specific scopes
public static final String MANAGE_MEMBERSHIP = "manage-membership";
public static final String MANAGE_MEMBERS = "manage-members";
public static final String VIEW_MEMBERS = "view-members";

// user specific scopes
public static final String IMPERSONATE = "impersonate";
public static final String MAP_ROLES = "map-roles";
public static final String MANAGE_GROUP_MEMBERSHIP = "manage-group-membership";

public static final ResourceType CLIENTS = new ResourceType(CLIENTS_RESOURCE_TYPE, Set.of(CONFIGURE, MANAGE, MAP_ROLES, MAP_ROLES_CLIENT_SCOPE, MAP_ROLES_COMPOSITE, VIEW));
public static final ResourceType GROUPS = new ResourceType(GROUPS_RESOURCE_TYPE, Set.of(MANAGE, VIEW, MANAGE_MEMBERSHIP, MANAGE_MEMBERS, VIEW_MEMBERS));
public static final ResourceType USERS = new ResourceType(USERS_RESOURCE_TYPE, Set.of(MANAGE, VIEW, IMPERSONATE, MAP_ROLES, MANAGE_GROUP_MEMBERSHIP));

public static final AdminPermissionsSchema SCHEMA = new AdminPermissionsSchema();

private AdminPermissionsSchema() {
super(Map.of(USERS_RESOURCE_TYPE, USERS, CLIENTS_RESOURCE_TYPE, CLIENTS));
super(Map.of(
CLIENTS_RESOURCE_TYPE, CLIENTS,
GROUPS_RESOURCE_TYPE, GROUPS,
USERS_RESOURCE_TYPE, USERS
));
}

public Resource getOrCreateResource(KeycloakSession session, ResourceServer resourceServer, String policyType, String resourceType, String id) {
Expand All @@ -86,12 +102,13 @@ public Resource getOrCreateResource(KeycloakSession session, ResourceServer reso
return resource;
}

String name = null;
String name;

if (USERS.getType().equals(resourceType)) {
name = resolveUser(session, id);
} else if (CLIENTS.getType().equals(resourceType)) {
name = resolveClient(session, id);
switch (resourceType) {
case CLIENTS_RESOURCE_TYPE -> name = resolveClient(session, id);
case GROUPS_RESOURCE_TYPE -> name = resolveGroup(session, id);
case USERS_RESOURCE_TYPE -> name = resolveUser(session, id);
default -> throw new IllegalStateException("Resource type [" + resourceType + "] not found.");
}

if (name == null) {
Expand Down Expand Up @@ -161,6 +178,13 @@ public void throwExceptionIfAdminPermissionClient(KeycloakSession session, Strin
}
}

private String resolveGroup(KeycloakSession session, String id) {
RealmModel realm = session.getContext().getRealm();
GroupModel group = session.groups().getGroupById(realm, id);

return group == null ? null : group.getId();
}

private String resolveUser(KeycloakSession session, String id) {
RealmModel realm = session.getContext().getRealm();
UserModel user = session.users().getUserById(realm, id);
Expand Down
4 changes: 0 additions & 4 deletions server-spi-private/src/main/java/org/keycloak/models/utils/ModelToRepresentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,10 +173,6 @@ public static GroupRepresentation toRepresentation(GroupModel group, boolean ful
return rep;
}

public static Stream<GroupModel> searchGroupModelsByAttributes(KeycloakSession session, RealmModel realm, Map<String,String> attributes, Integer first, Integer max) {
return session.groups().searchGroupsByAttributes(realm, attributes, first, max);
}

@Deprecated
public static Stream<GroupRepresentation> toGroupHierarchy(KeycloakSession session, RealmModel realm, boolean full) {
return session.groups().getTopLevelGroupsStream(realm, null, null)
Expand Down
5 changes: 2 additions & 3 deletions services/src/main/java/org/keycloak/services/resources/admin/GroupResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,10 +174,9 @@ public Stream<GroupRepresentation> getSubGroups(
@Parameter(description = "The maximum number of results that are to be returned. Defaults to 10") @QueryParam("max") @DefaultValue("10") Integer max,
@Parameter(description = "Boolean which defines whether brief groups representations are returned or not (default: false)") @QueryParam("briefRepresentation") @DefaultValue("false") Boolean briefRepresentation) {
this.auth.groups().requireView(group);
boolean canViewGlobal = auth.groups().canView();
return paginatedStream(
group.getSubGroupsStream(search, exact, -1, -1)
.filter(g -> canViewGlobal || auth.groups().canView(g)), first, max)
.filter(auth.groups()::canView), first, max)
.map(g -> GroupUtils.populateSubGroupCount(g, GroupUtils.toRepresentation(auth.groups(), g, !briefRepresentation)));
}

Expand All @@ -204,7 +203,7 @@ public Response addChild(GroupRepresentation rep) {

try {
Response.ResponseBuilder builder = Response.status(204);
GroupModel child = null;
GroupModel child;
if (rep.getId() != null) {
child = realm.getGroupById(rep.getId());
if (child == null) {
Expand Down
8 changes: 3 additions & 5 deletions services/src/main/java/org/keycloak/services/resources/admin/GroupsResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@
import org.keycloak.models.KeycloakSession;
import org.keycloak.models.ModelDuplicateException;
import org.keycloak.models.RealmModel;
import org.keycloak.models.utils.ModelToRepresentation;
import org.keycloak.organization.utils.Organizations;
import org.keycloak.representations.idm.GroupRepresentation;
import org.keycloak.services.ErrorResponse;
Expand Down Expand Up @@ -100,7 +99,7 @@ public Stream<GroupRepresentation> getGroups(@QueryParam("search") String search
Stream<GroupModel> stream;
if (Objects.nonNull(searchQuery)) {
Map<String, String> attributes = SearchQueryUtils.getFields(searchQuery);
stream = ModelToRepresentation.searchGroupModelsByAttributes(session, realm, attributes, firstResult, maxResults);
stream = session.groups().searchGroupsByAttributes(realm, attributes, firstResult, maxResults);
} else if (Objects.nonNull(search)) {
stream = session.groups().searchForGroupByNameStream(realm, search.trim(), exact, firstResult, maxResults);
} else {
Expand All @@ -110,9 +109,8 @@ public Stream<GroupRepresentation> getGroups(@QueryParam("search") String search
if (populateHierarchy) {
return GroupUtils.populateGroupHierarchyFromSubGroups(session, realm, stream, !briefRepresentation, groupsEvaluator);
}
boolean canViewGlobal = groupsEvaluator.canView();
return stream
.filter(g -> canViewGlobal || groupsEvaluator.canView(g))

return stream.filter(groupsEvaluator::canView)
.map(g -> GroupUtils.populateSubGroupCount(g, GroupUtils.toRepresentation(groupsEvaluator, g, !briefRepresentation)));
Comment on lines +112 to 114
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Pagination before permission filtering may return fewer results than max.

The canView filter is applied after searchGroupsByAttributes has already applied pagination (firstResult, maxResults). If some groups are filtered out, the page will be smaller than maxResults. This is pre-existing behavior across all three search paths, but worth noting since the V2 per-group permission model makes it more likely that individual groups are filtered out (compared to a global admin role check that either allows all or none).

🤖 Prompt for AI Agents 
In
`@services/src/main/java/org/keycloak/services/resources/admin/GroupsResource.java`
around lines 112 - 114, The current pipeline applies pagination inside
searchGroupsByAttributes then filters with groupsEvaluator::canView, which can
produce fewer than max results; update the logic in GroupsResource so permission
filtering occurs before pagination: retrieve the candidate stream/list (from
searchGroupsByAttributes or equivalent), apply groupsEvaluator::canView first,
then apply pagination (skip firstResult and limit to maxResults) and finally map
to GroupUtils.populateSubGroupCount/GroupUtils.toRepresentation; alternatively,
fetch a larger/unpaged candidate set and continue filtering until you produce
maxResults visible items, ensuring the methods GroupUtils.populateSubGroupCount
and GroupUtils.toRepresentation are applied only after permission filtering.

}

Expand Down
19 changes: 7 additions & 12 deletions services/src/main/java/org/keycloak/services/resources/admin/UsersResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,6 @@
import java.util.Optional;
import java.util.Properties;
import java.util.Set;
import java.util.function.Predicate;
import java.util.stream.Collectors;
import java.util.stream.Stream;

Expand Down Expand Up @@ -396,7 +395,7 @@ public Integer getUsersCount(
} else if (userPermissionEvaluator.canView()) {
return session.users().getUsersCount(realm, search.trim());
} else {
return session.users().getUsersCount(realm, search.trim(), auth.groups().getGroupsWithViewPermission());
return session.users().getUsersCount(realm, search.trim(), auth.groups().getGroupIdsWithViewPermission());
}
} else if (last != null || first != null || email != null || username != null || emailVerified != null || enabled != null || !searchAttributes.isEmpty()) {
Map<String, String> parameters = new HashMap<>();
Expand All @@ -423,12 +422,12 @@ public Integer getUsersCount(
if (userPermissionEvaluator.canView()) {
return session.users().getUsersCount(realm, parameters);
} else {
return session.users().getUsersCount(realm, parameters, auth.groups().getGroupsWithViewPermission());
return session.users().getUsersCount(realm, parameters, auth.groups().getGroupIdsWithViewPermission());
}
} else if (userPermissionEvaluator.canView()) {
return session.users().getUsersCount(realm);
} else {
return session.users().getUsersCount(realm, auth.groups().getGroupsWithViewPermission());
return session.users().getUsersCount(realm, auth.groups().getGroupIdsWithViewPermission());
}
}

Expand All @@ -446,16 +445,12 @@ public UserProfileResource userProfile() {
private Stream<UserRepresentation> searchForUser(Map<String, String> attributes, RealmModel realm, UserPermissionEvaluator usersEvaluator, Boolean briefRepresentation, Integer firstResult, Integer maxResults, Boolean includeServiceAccounts) {
attributes.put(UserModel.INCLUDE_SERVICE_ACCOUNT, includeServiceAccounts.toString());

if (!auth.users().canView()) {
Set<String> groupModels = auth.groups().getGroupsWithViewPermission();

if (!groupModels.isEmpty()) {
session.setAttribute(UserModel.GROUPS, groupModels);
}
Set<String> groupIds = auth.groups().getGroupIdsWithViewPermission();
if (!groupIds.isEmpty()) {
session.setAttribute(UserModel.GROUPS, groupIds);
}

Stream<UserModel> userModels = session.users().searchForUserStream(realm, attributes, firstResult, maxResults).filter(usersEvaluator::canView);
return toRepresentation(realm, usersEvaluator, briefRepresentation, userModels);
return toRepresentation(realm, usersEvaluator, briefRepresentation, session.users().searchForUserStream(realm, attributes, firstResult, maxResults));
}

private Stream<UserRepresentation> toRepresentation(RealmModel realm, UserPermissionEvaluator usersEvaluator, Boolean briefRepresentation, Stream<UserModel> userModels) {
Expand Down
12 changes: 6 additions & 6 deletions ...ces/src/main/java/org/keycloak/services/resources/admin/permissions/AdminPermissions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,10 @@ public static AdminPermissionManagement management(KeycloakSession session, Real
}

public static void registerListener(ProviderEventManager manager) {
manager.register(new ProviderEventListener() {
@Override
public void onEvent(ProviderEvent event) {
if (Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ)) {
if (Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ)) {
manager.register(new ProviderEventListener() {
@Override
public void onEvent(ProviderEvent event) {
if (event instanceof RoleContainerModel.RoleRemovedEvent) {
RoleContainerModel.RoleRemovedEvent cast = (RoleContainerModel.RoleRemovedEvent) event;
RoleModel role = cast.getRole();
Expand All @@ -94,8 +94,8 @@ public void onEvent(ProviderEvent event) {
management(cast.getKeycloakSession(), cast.getRealm()).groups().setPermissionsEnabled(cast.getGroup(), false);
}
}
}
});
});
}
}


Expand Down
92 changes: 87 additions & 5 deletions ...main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionEvaluator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
*/
package org.keycloak.services.resources.admin.permissions;

import org.keycloak.authorization.AdminPermissionsSchema;
import org.keycloak.models.AdminRoles;
import org.keycloak.models.GroupModel;

import java.util.Map;
Expand All @@ -26,41 +28,121 @@
* @version $Revision: 1 $
*/
public interface GroupPermissionEvaluator {

/**
* Returns {@code true} if the caller has at least one of {@link AdminRoles#QUERY_GROUPS},
* {@link AdminRoles#MANAGE_USERS} or {@link AdminRoles#VIEW_USERS} roles.
* <p/>
* For V2 only: Also if it has a permission to {@link AdminPermissionsSchema#VIEW} or
* {@link AdminPermissionsSchema#MANAGE} groups.
*/
boolean canList();

/**
* Throws ForbiddenException if {@link #canList()} returns {@code false}.
*/
void requireList();

/**
* Returns {@code true} if the caller has {@link AdminRoles#MANAGE_USERS} role.
* <p/>
* Or if it has a permission to {@link AdminPermissionsSchema#MANAGE} the group.
*/
boolean canManage(GroupModel group);

/**
* Throws ForbiddenException if {@link #canManage(GroupModel)} returns {@code false}.
*/
void requireManage(GroupModel group);

/**
* Returns {@code true} if the caller has one of {@link AdminRoles#MANAGE_USERS} or
* {@link AdminRoles#VIEW_USERS} roles.
* <p/>
* Or if it has a permission to {@link AdminPermissionsSchema#VIEW} or
* {@link AdminPermissionsSchema#MANAGE} the group.
*/
boolean canView(GroupModel group);

/**
* Throws ForbiddenException if {@link #canView(GroupModel)} returns {@code false}.
*/
void requireView(GroupModel group);

/**
* Returns {@code true} if the caller has {@link AdminRoles#MANAGE_USERS} role.
* <p/>
* For V2 only: Also if it has permission to {@link AdminPermissionsSchema#VIEW} or
* {@link AdminPermissionsSchema#MANAGE} groups.
*/
boolean canManage();
Comment on lines +72 to 78
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Javadoc for canManage() seems incorrect — includes VIEW scope.

The Javadoc states that canManage() returns true if the caller has a VIEW permission on groups. This would make canManage() and canView() equivalent in V2 mode. Either the Javadoc is wrong (should reference only MANAGE), or the semantics of the global canManage() are intentionally weaker than canManage(GroupModel). Please clarify.

🤖 Prompt for AI Agents 
In
`@services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionEvaluator.java`
around lines 72 - 78, The Javadoc for canManage() is misleading because it
mentions AdminPermissionsSchema#VIEW (making it equivalent to canView()); update
the documentation on GroupPermissionEvaluator.canManage() to accurately reflect
its semantics—either remove the reference to AdminPermissionsSchema#VIEW and
state it returns true only for AdminRoles#MANAGE_USERS or
AdminPermissionsSchema#MANAGE (for V2), or explicitly document that it
intentionally differs from canView(); ensure you reference canView(),
canManage(), AdminRoles#MANAGE_USERS, and AdminPermissionsSchema#MANAGE in the
updated comment so readers can locate the related behavior.


/**
* Throws ForbiddenException if {@link #canManage()} returns {@code false}.
*/
void requireManage();

/**
* Returns {@code true} if the caller has one of {@link AdminRoles#MANAGE_USERS} or
* {@link AdminRoles#VIEW_USERS} roles.
* <p/>
* Or if it has a permission to {@link AdminPermissionsSchema#VIEW} or
* {@link AdminPermissionsSchema#MANAGE} groups.
*/
boolean canView();

/**
* Throws ForbiddenException if {@link #canView()} returns {@code false}.
*/
void requireView();

boolean getGroupsWithViewPermission(GroupModel group);

/**
* Throws ForbiddenException if {@link #canViewMembers(GroupModel)} returns {@code false}.
*/
void requireViewMembers(GroupModel group);

/**
* Returns {@code true} if {@link UserPermissionEvaluator#canManage()} evaluates to {@code true}.
* <p/>
* Or if it has a permission to {@link AdminPermissionsSchema#MANAGE_MEMBERS} of the group.
*/
boolean canManageMembers(GroupModel group);

/**
* Returns {@code true} if the caller has one of {@link AdminRoles#MANAGE_USERS} role.
* <p/>
* Or if it has a permission to {@link AdminPermissionsSchema#MANAGE} the group or
* {@link AdminPermissionsSchema#MANAGE_MEMBERSHIP} of the group.
*/
boolean canManageMembership(GroupModel group);


/**
* Returns {@code true} if {@link UserPermissionEvaluator#canView()} evaluates to {@code true}.
* <p/>
* Or if it has a permission to {@link AdminPermissionsSchema#VIEW_MEMBERS} or
* {@link AdminPermissionsSchema#MANAGE_MEMBERS} of the group.
*/
boolean canViewMembers(GroupModel group);


/**
* Throws ForbiddenException if {@link #canManageMembership(GroupModel)} returns {@code false}.
*/
void requireManageMembership(GroupModel group);

/**
* Throws ForbiddenException if {@link #canManageMembership(GroupModel)} returns {@code false}.
*/
void requireManageMembers(GroupModel group);
Comment on lines +132 to 135
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Javadoc references wrong method: canManageMembership instead of canManageMembers.

requireManageMembers(GroupModel) should throw if canManageMembers(GroupModel) returns false, not canManageMembership(GroupModel) — these are two distinct methods with different semantics.

📝 Proposed fix 
     /**
-     * Throws ForbiddenException if {`@link` `#canManageMembership`(GroupModel)} returns {`@code` false}.
+     * Throws ForbiddenException if {`@link` `#canManageMembers`(GroupModel)} returns {`@code` false}.
      */
     void requireManageMembers(GroupModel group);
🤖 Prompt for AI Agents 
In
`@services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionEvaluator.java`
around lines 132 - 135, Update the Javadoc on requireManageMembers(GroupModel)
to reference the correct method canManageMembers(GroupModel) (instead of
canManageMembership), and ensure the description reads that it throws
ForbiddenException if canManageMembers(GroupModel) returns false; keep
references to ForbiddenException and GroupModel intact.


/**
* Returns Map with information what access the caller for the provided group has.
*/
Map<String, Boolean> getAccess(GroupModel group);

Set<String> getGroupsWithViewPermission();
/**
* If {@link UserPermissionEvaluator#canView()} evaluates to {@code true}, returns empty set.
*
* @return Stream of IDs of groups with view permission.
*/
Set<String> getGroupIdsWithViewPermission();
}