Skip to content
Jump to bottom

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

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

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

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)));
}

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();

/**
* 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 points to the wrong predicate for requireManageMembers.
Line 132 should reference canManageMembers, not canManageMembership.

📝 Javadoc fix 
-     * Throws ForbiddenException if {`@link` `#canManageMembership`(GroupModel)} returns {`@code` false}.
+     * Throws ForbiddenException if {`@link` `#canManageMembers`(GroupModel)} returns {`@code` false}.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/**
* Throws ForbiddenException if {@link #canManageMembership(GroupModel)} returns {@code false}.
*/
void requireManageMembers(GroupModel group);
/**
* 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 for the requireManageMembers method
in GroupPermissionEvaluator to reference the correct predicate name
canManageMembers (not canManageMembership); change the `@link` target in the
comment above requireManageMembers so it points to canManageMembers to match the
actual predicate/method name.


/**
* 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();
Comment on lines +142 to +147
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

Return type in Javadoc should match Set.
Line 145 mentions “Stream” but the method returns a Set.

📝 Javadoc fix 
-     * `@return` Stream of IDs of groups with view permission.
+     * `@return` Set of IDs of groups with view permission.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/**
* If {@link UserPermissionEvaluator#canView()} evaluates to {@code true}, returns empty set.
*
* @return Stream of IDs of groups with view permission.
*/
Set<String> getGroupIdsWithViewPermission();
/**
* If {`@link` UserPermissionEvaluator#canView()} evaluates to {`@code` true}, returns empty set.
*
* `@return` Set of IDs of groups with view permission.
*/
Set<String> getGroupIdsWithViewPermission();
🤖 Prompt for AI Agents 
In
`@services/src/main/java/org/keycloak/services/resources/admin/permissions/GroupPermissionEvaluator.java`
around lines 142 - 147, The Javadoc for
GroupPermissionEvaluator#getGroupIdsWithViewPermission incorrectly refers to a
"Stream" while the method returns a Set; update the Javadoc to say it returns a
Set (e.g., "Set of IDs of groups with view permission") and ensure the
description and `@return` tag match the actual return type, keeping the reference
to UserPermissionEvaluator#canView() intact.

}