From 26537970d5f24bc6f09d5cba38f86ee3a6f1248d Mon Sep 17 00:00:00 2001 From: vramik Date: Mon, 12 Oct 2026 23:08:14 +0200 Subject: [PATCH] Treat the admin-permissions client as an internal client Closes #51630 Co-authored-by: waterWang Signed-off-by: vramik --- .../topics/changes/changes-27_0_0.adoc | 11 +++++++ .../v2/ClientApiV2AuthorizationTest.java | 5 ++-- .../admin/fgap/ClientPermissions.java | 4 +++ .../ClientResourceTypeEvaluationTest.java | 30 +++++++++++++++++++ .../admin/authz/fgap/PermissionRESTTest.java | 23 ++++++++++---- 5 files changed, 64 insertions(+), 9 deletions(-) diff --git a/docs/documentation/upgrading/topics/changes/changes-27_0_0.adoc b/docs/documentation/upgrading/topics/changes/changes-27_0_0.adoc index 8bca57b6ce0d..bff6a6c87baa 100644 --- a/docs/documentation/upgrading/topics/changes/changes-27_0_0.adoc +++ b/docs/documentation/upgrading/topics/changes/changes-27_0_0.adoc @@ -94,6 +94,17 @@ Previously, the Redirect URL was used without validation, which could allow an o The `invite-existing-user` endpoint now also accepts an optional `client_id` query parameter, the same way `invite-user` already does. If omitted, both endpoints default to the `account` client. +=== The `admin-permissions` client is treated as an internal client +// https://github.com/keycloak/keycloak/issues/51630 + +When fine-grained admin permissions V2 are enabled on a realm, the `admin-permissions` client holds the whole authorization model of that realm. +This client is now treated as an internal client, the same way the `realm-management` client already is, so it can no longer be managed through the Admin REST API. +Previously, an administrator with a *manage* permission on clients could update it, including changing its `clientId`, even though the `access.manage` field returned for the client was already `false`. + +After upgrading, requests that update the `admin-permissions` client are rejected with a 403 status. +Requests that delete it are also rejected with a 403 status, where previously they were rejected with a 400 status. +Configuring the permissions and the policies of the realm is not affected, as it requires the `manage-realm` role rather than a permission on the client. + // ------------------------ Deprecated features ------------------------ // == Deprecated features diff --git a/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/ClientApiV2AuthorizationTest.java b/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/ClientApiV2AuthorizationTest.java index 0ed17aaff54c..9481bf016da8 100644 --- a/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/ClientApiV2AuthorizationTest.java +++ b/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/ClientApiV2AuthorizationTest.java @@ -415,10 +415,9 @@ public void cannotDeleteAdminPermissionsClient() { .map(f -> f.get(0)) .orElseThrow(() -> new AssertionError("Cannot find admin permissions client")); + // the client is internal, so the request is rejected by the permission check the same way it is for realm-management try (var response = getClientApi(realmAdminAdminClient, getRealmName(), adminPermissionsClientRep.getClientId()).deleteClient()) { - assertThat(response.getStatus(), is(400)); - var body = response.readEntity(String.class); - assertThat(body, containsString("Could not delete client")); + assertThat(response.getStatus(), is(403)); } // Verify the client still exists (was not deleted) diff --git a/services/src/main/java/org/keycloak/services/resources/admin/fgap/ClientPermissions.java b/services/src/main/java/org/keycloak/services/resources/admin/fgap/ClientPermissions.java index b71e6ed8caf6..c97362fa1b02 100644 --- a/services/src/main/java/org/keycloak/services/resources/admin/fgap/ClientPermissions.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/fgap/ClientPermissions.java @@ -721,6 +721,10 @@ protected boolean isInternal(ClientModel client) { return false; } + if (AdminPermissionsSchema.SCHEMA.isAdminPermissionClient(realm, client.getId())) { + return true; + } + if (realm.getName().equals(Config.getAdminRealm())) { return client.getClientId().endsWith(AdminRoles.APP_SUFFIX); } diff --git a/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/ClientResourceTypeEvaluationTest.java b/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/ClientResourceTypeEvaluationTest.java index 08e8a256f7d2..aebe8c4ed5af 100644 --- a/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/ClientResourceTypeEvaluationTest.java +++ b/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/ClientResourceTypeEvaluationTest.java @@ -326,6 +326,36 @@ public void testManageAllClients() { assertThat(found, not(empty())); } + @Test + public void testManageAllClientsDoesNotGrantManageOnAdminPermissionsClient() { + UserRepresentation myadmin = realm.admin().users().search("myadmin").get(0); + String adminPermissionsClientId = adminPermissionsClient.toRepresentation().getId(); + + UserPolicyRepresentation onlyMyAdminUserPolicy = createUserPolicy(realm, adminPermissionsClient, "Only My Admin User Policy", myadmin.getId()); + createAllPermission(adminPermissionsClient, clientsType, onlyMyAdminUserPolicy, Set.of(VIEW, MANAGE)); + + ClientResource clientResource = realmAdminClient.realm(realm.getName()).clients().get(adminPermissionsClientId); + + // the admin-permissions client holds the whole FGAP model, so a Clients:manage permission must not grant manage on it + ClientRepresentation rep = clientResource.toRepresentation(); + assertThat(rep.getAccess().get("manage"), is(false)); + assertThat(rep.getAccess().get("configure"), is(false)); + + // enforcement has to agree with the access report above + rep.setName("somethingNew"); + Assertions.assertThrows(ForbiddenException.class, () -> clientResource.update(rep)); + + // renaming it must not be possible either, otherwise it could be deleted as a regular client afterwards + ClientRepresentation renamed = adminPermissionsClient.toRepresentation(); + renamed.setClientId("renamed-admin-permissions"); + Assertions.assertThrows(ForbiddenException.class, () -> clientResource.update(renamed)); + + // nothing was persisted + ClientRepresentation current = adminPermissionsClient.toRepresentation(); + assertThat(current.getClientId(), is(Constants.ADMIN_PERMISSIONS_CLIENT_ID)); + assertThat(current.getName(), nullValue()); + } + @Test public void testClientScopeEvaluation() { UserRepresentation myadmin = realm.admin().users().search("myadmin").get(0); diff --git a/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionRESTTest.java b/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionRESTTest.java index f5b6cd83dbba..909c54255c0b 100644 --- a/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionRESTTest.java +++ b/tests/base/src/test/java/org/keycloak/tests/admin/authz/fgap/PermissionRESTTest.java @@ -21,6 +21,7 @@ import java.util.Set; import jakarta.ws.rs.BadRequestException; +import jakarta.ws.rs.ForbiddenException; import jakarta.ws.rs.core.Response; import org.keycloak.admin.client.Keycloak; @@ -29,7 +30,6 @@ import org.keycloak.models.Constants; import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.GroupRepresentation; -import org.keycloak.representations.idm.OAuth2ErrorRepresentation; import org.keycloak.representations.idm.RoleRepresentation; import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.representations.idm.authorization.DecisionStrategy; @@ -51,8 +51,8 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; -import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.nullValue; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.fail; @@ -68,15 +68,15 @@ public class PermissionRESTTest extends AbstractPermissionTest { @Test public void testPreventDeletingAdminPermissionsClient() { + // the client is internal, so the request is rejected by the permission check the same way it is for realm-management try { adminPermissionsClient.remove(); fail("Expected Exception wasn't thrown."); } catch (Exception ex) { - assertThat(ex, instanceOf(BadRequestException.class)); - OAuth2ErrorRepresentation error = ((BadRequestException) ex).getResponse().readEntity(OAuth2ErrorRepresentation.class); - assertThat(error.getError(), is("invalid_request")); - assertThat(error.getErrorDescription(), is("Could not delete client")); + assertThat(ex, instanceOf(ForbiddenException.class)); } + + assertThat(adminPermissionsClient.toRepresentation(), notNullValue()); } @Test @@ -84,6 +84,17 @@ public void testManageNotAllowedForAdminPermissionsClient() { ClientRepresentation representation = adminPermissionsClient.toRepresentation(); assertFalse(representation.getAccess().get("manage")); assertFalse(representation.getAccess().get("configure")); + + // enforcement has to agree with the access report above, even for a full admin + representation.setName("somethingNew"); + try { + adminPermissionsClient.update(representation); + fail("Expected Exception wasn't thrown."); + } catch (Exception ex) { + assertThat(ex, instanceOf(ForbiddenException.class)); + } + + assertThat(adminPermissionsClient.toRepresentation().getName(), nullValue()); } @Test