Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -50,22 +50,27 @@
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";

//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";
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));
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 AdminPermissionsSchema SCHEMA = new AdminPermissionsSchema();

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

public Resource getOrCreateResource(KeycloakSession session, ResourceServer resourceServer, String policyType, String resourceType, String id) {
Expand All @@ -85,6 +90,8 @@ public Resource getOrCreateResource(KeycloakSession session, ResourceServer reso

if (USERS.getType().equals(resourceType)) {
name = resolveUser(session, id);
} else if (CLIENTS.getType().equals(resourceType)) {
name = resolveClient(session, id);
}

if (name == null) {
Expand Down Expand Up @@ -165,6 +172,17 @@ private String resolveUser(KeycloakSession session, String id) {
return user == null ? null : user.getId();
}

private String resolveClient(KeycloakSession session, String id) {
RealmModel realm = session.getContext().getRealm();
ClientModel client = session.clients().getClientById(realm, id);

if (client == null) {
client = session.clients().getClientByClientId(realm, id);
}

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

private StoreFactory getStoreFactory(KeycloakSession session) {
AuthorizationProvider authzProvider = session.getProvider(AuthorizationProvider.class);
return authzProvider.getStoreFactory();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,23 +74,25 @@ public static void registerListener(ProviderEventManager manager) {
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();
RealmModel realm;
if (role.getContainer() instanceof ClientModel) {
realm = ((ClientModel)role.getContainer()).getRealm();
if (Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ)) {
if (event instanceof RoleContainerModel.RoleRemovedEvent) {
RoleContainerModel.RoleRemovedEvent cast = (RoleContainerModel.RoleRemovedEvent) event;
RoleModel role = cast.getRole();
RealmModel realm;
if (role.getContainer() instanceof ClientModel) {
realm = ((ClientModel) role.getContainer()).getRealm();

} else {
realm = (RealmModel)role.getContainer();
} else {
realm = (RealmModel) role.getContainer();
}
management(cast.getKeycloakSession(), realm).roles().setPermissionsEnabled(role, false);
} else if (event instanceof ClientModel.ClientRemovedEvent) {
ClientModel.ClientRemovedEvent cast = (ClientModel.ClientRemovedEvent) event;
management(cast.getKeycloakSession(), cast.getClient().getRealm()).clients().setPermissionsEnabled(cast.getClient(), false);
} else if (event instanceof GroupModel.GroupRemovedEvent) {
GroupModel.GroupRemovedEvent cast = (GroupModel.GroupRemovedEvent) event;
management(cast.getKeycloakSession(), cast.getRealm()).groups().setPermissionsEnabled(cast.getGroup(), false);
}
management(cast.getKeycloakSession(), realm).roles().setPermissionsEnabled(role, false);
} else if (event instanceof ClientModel.ClientRemovedEvent) {
ClientModel.ClientRemovedEvent cast = (ClientModel.ClientRemovedEvent)event;
management(cast.getKeycloakSession(), cast.getClient().getRealm()).clients().setPermissionsEnabled(cast.getClient(), false);
} else if (event instanceof GroupModel.GroupRemovedEvent) {
GroupModel.GroupRemovedEvent cast = (GroupModel.GroupRemovedEvent)event;
management(cast.getKeycloakSession(), cast.getRealm()).groups().setPermissionsEnabled(cast.getGroup(), false);
}
Comment on lines +77 to 96

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for both feature flags usage patterns
echo "=== ADMIN_FINE_GRAINED_AUTHZ usage ==="
rg -n "ADMIN_FINE_GRAINED_AUTHZ" --type java | head -150

echo ""
echo "=== Files containing both V1 and V2 flags ==="
rg -l "ADMIN_FINE_GRAINED_AUTHZ" --type java | xargs -I {} sh -c 'if grep -q "ADMIN_FINE_GRAINED_AUTHZ_V2" "$1"; then echo "$1"; fi' _ {}

Repository: AI-Code-Review-Evals/coderabbit-keycloak

Length of output: 12484


🏁 Script executed:

#!/bin/bash
# Get the AdminPermissions.java file and check surrounding context
fd "AdminPermissions.java" --type f -x cat -n {} | head -150

Repository: AI-Code-Review-Evals/coderabbit-keycloak

Length of output: 5594


🏁 Script executed:

#!/bin/bash
# Examine MgmtPermissions.java line 85 for context
sed -n '80,90p' services/src/main/java/org/keycloak/services/resources/admin/permissions/MgmtPermissions.java

Repository: AI-Code-Review-Evals/coderabbit-keycloak

Length of output: 685


🏁 Script executed:

#!/bin/bash
# Check what MgmtPermissionsV2 does - verify if it also relies on cleanup handlers
fd "MgmtPermissionsV2.java" --type f -x wc -l {}
fd "MgmtPermissionsV2.java" --type f -x head -100 {}

Repository: AI-Code-Review-Evals/coderabbit-keycloak

Length of output: 2554


Fix event listener feature flag check to support both V1 and V2.

The cleanup handlers for RoleRemovedEvent, ClientRemovedEvent, and GroupRemovedEvent at line 77 are gated behind only ADMIN_FINE_GRAINED_AUTHZ (V1). If V2 is enabled without V1, these cleanup handlers won't execute, leaving orphaned permission resources. Change the condition to check for either flag, matching the pattern used in MgmtPermissions.java line 85:

if (Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ) || Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ_V2)) {
🤖 Prompt for AI Agents
In
@services/src/main/java/org/keycloak/services/resources/admin/permissions/AdminPermissions.java
around lines 77 - 96, The event cleanup block in AdminPermissions (handling
RoleContainerModel.RoleRemovedEvent, ClientModel.ClientRemovedEvent,
GroupModel.GroupRemovedEvent) is currently guarded only by
Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ so it won't run when only
ADMIN_FINE_GRAINED_AUTHZ_V2 is enabled; update the feature check to require
either flag (i.e., check
Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ) ||
Profile.isFeatureEnabled(Profile.Feature.ADMIN_FINE_GRAINED_AUTHZ_V2)) so the
management(...) .roles().setPermissionsEnabled(...),
.clients().setPermissionsEnabled(...), and .groups().setPermissionsEnabled(...)
cleanup handlers always run for V1 or V2.

}
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/
package org.keycloak.services.resources.admin.permissions;

import org.keycloak.models.AdminRoles;
import org.keycloak.models.ClientModel;
import org.keycloak.models.ClientScopeModel;

Expand All @@ -31,54 +32,161 @@ public interface ClientPermissionEvaluator {

void setPermissionsEnabled(ClientModel client, boolean enable);

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

/**
* Returns {@code true} if the caller has {@link org.keycloak.models.AdminRoles#MANAGE_CLIENTS} role.
* <p/>
* For V2 only: Also if it has permission to {@link org.keycloak.authorization.AdminPermissionsSchema#MANAGE}.
*/
boolean canManage();

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

/**
* Returns {@code true} if the caller has {@link org.keycloak.models.AdminRoles#MANAGE_CLIENTS} role.
* <p/>
* For V2 only: Also if it has permission to {@link org.keycloak.authorization.AdminPermissionsSchema#MANAGE}.
*/
boolean canManageClientScopes();

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

/**
* Returns {@code true} if the caller has at least one of the {@link org.keycloak.models.AdminRoles#MANAGE_CLIENTS} or {@link org.keycloak.models.AdminRoles#VIEW_CLIENTS} roles.
* <p/>
* For V2 only: Also if it has permission to {@link org.keycloak.authorization.AdminPermissionsSchema#VIEW}.
*/
boolean canView();

/**
* Returns {@code true} if {@link #canView()} returns {@code true}.
* <p/>
* Or if the caller has at least one of the {@link AdminRoles#QUERY_CLIENTS} or {@link AdminRoles#QUERY_USERS} roles.
*/
boolean canList();

/**
* Returns {@code true} if {@link #canView()} returns {@code true}.
*/
boolean canViewClientScopes();

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

/**
* Returns {@code true} if {@link #canView()} returns {@code true}.
* <p/>
* Or if the caller has {@link AdminRoles#QUERY_CLIENTS} role.
*/
boolean canListClientScopes();

/**
* Returns {@code true} if {@link #canView()} returns {@code true}.
*/
void requireView();
Comment on lines +95 to 98

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Javadoc incorrectly states "Returns" for a void method.

The Javadoc says "Returns {@code true} if {@link #canView()} returns {@code true}." but requireView() returns void and throws ForbiddenException on failure.

📝 Proposed fix
-    /**
-     * Returns {@code true} if {@link #canView()} returns {@code true}.
-     */
+    /**
+     * Throws ForbiddenException if {@link #canView()} returns {@code false}.
+     */
     void requireView();
🤖 Prompt for AI Agents
In
@services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionEvaluator.java
around lines 95 - 98, The Javadoc for requireView() is incorrect: it claims a
return value but the method is void and throws ForbiddenException on failure;
update the Javadoc for requireView() in ClientPermissionEvaluator to describe
that it enforces view permission, throws ForbiddenException when canView() is
false, and replace the incorrect "Returns ..." line with a short description and
a @throws ForbiddenException tag referencing canView().


/**
* Returns {@code true} if {@link #canViewClientScopes()} returns {@code true}.
*/
void requireViewClientScopes();
Comment on lines +100 to 103

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Javadoc incorrectly states "Returns" for a void method.

Same issue as requireView() - the Javadoc should describe throwing behavior, not return value.

📝 Proposed fix
-    /**
-     * Returns {@code true} if {@link #canViewClientScopes()} returns {@code true}.
-     */
+    /**
+     * Throws ForbiddenException if {@link #canViewClientScopes()} returns {@code false}.
+     */
     void requireViewClientScopes();
📝 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
/**
* Returns {@code true} if {@link #canViewClientScopes()} returns {@code true}.
*/
void requireViewClientScopes();
/**
* Throws ForbiddenException if {@link #canViewClientScopes()} returns {@code false}.
*/
void requireViewClientScopes();
🤖 Prompt for AI Agents
In
@services/src/main/java/org/keycloak/services/resources/admin/permissions/ClientPermissionEvaluator.java
around lines 100 - 103, The Javadoc for requireViewClientScopes() incorrectly
uses "Returns" for a void method; update the Javadoc to describe the throwing
behavior instead (e.g., state that the method throws an appropriate exception if
canViewClientScopes() is false) similar to the fix for requireView(), and ensure
the comment references the canViewClientScopes() check and the exception
type/condition thrown by requireViewClientScopes().


/**
* Returns {@code true} if the caller has {@link org.keycloak.models.AdminRoles#MANAGE_CLIENTS} role.
* <p/>
* Or if the caller has a permission to {@link AdminPermissionManagement#MANAGE_SCOPE} the client.
* <p/>
* For V2 only: Also if the caller has a permission to {@link org.keycloak.authorization.AdminPermissionsSchema#MANAGE} all clients.
*/
boolean canManage(ClientModel client);

/**
* Returns {@code true} if {@link #canManage(ClientModel)} returns {@code true}.
* <p/>
* Or if the caller has a permission to {@link ClientPermissionManagement#CONFIGURE_SCOPE} the client.
* <p/>
* For V2 only: Also if the caller has a permission to {@link org.keycloak.authorization.AdminPermissionsSchema#CONFIGURE} all clients.
*/
boolean canConfigure(ClientModel client);

/**
* Throws ForbiddenException if {@link #canConfigure(ClientModel)} returns {@code false}.
*/
void requireConfigure(ClientModel client);

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

/**
* Returns {@code true} if {@link #canView()} or {@link #canConfigure(ClientModel)} returns {@code true}.
* <p/>
* Or if the caller has a permission to {@link AdminPermissionManagement#VIEW_SCOPE} the client.
* <p/>
* For V2 only: Also if the caller has a permission to {@link org.keycloak.authorization.AdminPermissionsSchema#VIEW} all clients.
*/
boolean canView(ClientModel client);

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

/**
* Returns {@code true} if the caller has {@link org.keycloak.models.AdminRoles#MANAGE_CLIENTS} role.
* <p/>
* For V2 only: Also if it has permission to {@link org.keycloak.authorization.AdminPermissionsSchema#MANAGE}.
*/
boolean canManage(ClientScopeModel clientScope);

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

/**
* Returns {@code true} if the caller has at least one of the {@link org.keycloak.models.AdminRoles#VIEW_CLIENTS} or {@link org.keycloak.models.AdminRoles#MANAGE_CLIENTS} roles.
* <p/>
* For V2 only: Also if it has permission to {@link org.keycloak.authorization.AdminPermissionsSchema#VIEW} or {@link org.keycloak.authorization.AdminPermissionsSchema#MANAGE}.
*/
boolean canView(ClientScopeModel clientScope);

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

/**
* Returns {@code true} if the caller has a permission to {@link ClientPermissionManagement#MAP_ROLES_SCOPE} for the client.
* <p/>
* For V2 only: Also if the caller has a permission to {@link org.keycloak.authorization.AdminPermissionsSchema#MAP_ROLES} for all clients.
*/
boolean canMapRoles(ClientModel client);

/**
* Returns {@code true} if the caller has a permission to {@link ClientPermissionManagement#MAP_ROLES_COMPOSITE_SCOPE} for the client.
* <p/>
* For V2 only: Also if the caller has a permission to {@link org.keycloak.authorization.AdminPermissionsSchema#MAP_ROLES_COMPOSITE} for all clients.
*/
boolean canMapCompositeRoles(ClientModel client);

/**
* Returns {@code true} if the caller has a permission to {@link ClientPermissionManagement#MAP_ROLES_CLIENT_SCOPE} for the client.
* <p/>
* For V2 only: Also if the caller has a permission to {@link org.keycloak.authorization.AdminPermissionsSchema#MAP_ROLES_CLIENT_SCOPE} for all clients.
*/
boolean canMapClientScopeRoles(ClientModel client);

Map<String, Boolean> getAccess(ClientModel client);
Expand Down
Loading