From 27262be5694a2414baf3bc6ecbba6382de5e24f6 Mon Sep 17 00:00:00 2001 From: Erik Jan de Wit Date: Thu, 21 May 2026 08:30:16 +0200 Subject: [PATCH] fix for service account role management in admin v2 (#48166) * fix for service account role management in admin v2 fixes: #47966 Signed-off-by: Erik Jan de Wit * fixed merge error Signed-off-by: Erik Jan de Wit * fix test Signed-off-by: Erik Jan de Wit * fixed build error Signed-off-by: Erik Jan de Wit * fixed tests Signed-off-by: Erik Jan de Wit * Remove unused RealmAdminResource from DefaultClientsApi and DefaultAdminApi Signed-off-by: Peter Zaoral * Update rest/admin-v2/services/src/main/java/org/keycloak/services/client/DefaultClientService.java Co-authored-by: Peter Zaoral Signed-off-by: Erik Jan de Wit * also update the context Signed-off-by: Erik Jan de Wit * fix merge error Signed-off-by: Erik Jan de Wit * revert change Signed-off-by: Erik Jan de Wit * Update rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/DefaultAdminApi.java Co-authored-by: Peter Zaoral Signed-off-by: Erik Jan de Wit * Update rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/DefaultAdminApi.java Co-authored-by: Peter Zaoral Signed-off-by: Erik Jan de Wit --------- Signed-off-by: Erik Jan de Wit Signed-off-by: Peter Zaoral Signed-off-by: Erik Jan de Wit Co-authored-by: Peter Zaoral Co-authored-by: Peter Zaoral --- quarkus/deployment/pom.xml | 4 + .../keycloak/admin/api/client/ClientApi.java | 3 +- .../rest/admin/api/DefaultAdminApi.java | 19 ++- .../admin/api/client/DefaultClientApi.java | 7 +- .../admin/api/client/DefaultClientsApi.java | 13 +- .../services/client/DefaultClientService.java | 129 +++++++++++------- .../admin/client/v2/ClientApiV2Test.java | 72 ++++++++-- .../mapper/OIDCClientModelMapperTest.java | 27 ++++ 8 files changed, 185 insertions(+), 89 deletions(-) diff --git a/quarkus/deployment/pom.xml b/quarkus/deployment/pom.xml index 1c8061d8952..5754c57415c 100644 --- a/quarkus/deployment/pom.xml +++ b/quarkus/deployment/pom.xml @@ -20,6 +20,10 @@ org.keycloak keycloak-quarkus-server + + org.keycloak + keycloak-core + io.quarkus quarkus-core-deployment diff --git a/rest/admin-v2/api/src/main/java/org/keycloak/admin/api/client/ClientApi.java b/rest/admin-v2/api/src/main/java/org/keycloak/admin/api/client/ClientApi.java index 4fb7680a562..600f9d38b3d 100644 --- a/rest/admin-v2/api/src/main/java/org/keycloak/admin/api/client/ClientApi.java +++ b/rest/admin-v2/api/src/main/java/org/keycloak/admin/api/client/ClientApi.java @@ -2,7 +2,6 @@ package org.keycloak.admin.api.client; import java.io.InputStream; -import jakarta.validation.Valid; import jakarta.ws.rs.Consumes; import jakarta.ws.rs.DELETE; import jakarta.ws.rs.GET; @@ -42,7 +41,7 @@ public interface ClientApi { @APIResponse(responseCode = "200", content = @Content(schema = @Schema(implementation = BaseClientRepresentation.class))), @APIResponse(responseCode = "201", description = "Created", content = @Content(schema = @Schema(implementation = BaseClientRepresentation.class))) }) - Response createOrUpdateClient(@Valid BaseClientRepresentation client); + Response createOrUpdateClient(BaseClientRepresentation client); @PATCH @Consumes(PatchTypeNames.JSON_MERGE) diff --git a/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/DefaultAdminApi.java b/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/DefaultAdminApi.java index 16bf345d971..a22bcb7f869 100644 --- a/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/DefaultAdminApi.java +++ b/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/DefaultAdminApi.java @@ -1,14 +1,13 @@ package org.keycloak.rest.admin.api; +import jakarta.ws.rs.NotFoundException; + import org.keycloak.admin.api.AdminApi; import org.keycloak.admin.api.client.ClientsApi; import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; -import org.keycloak.protocol.oidc.TokenManager; import org.keycloak.rest.admin.api.client.DefaultClientsApi; import org.keycloak.services.resources.admin.AdminRoot; -import org.keycloak.services.resources.admin.RealmAdminResource; -import org.keycloak.services.resources.admin.RealmsAdminResource; import org.keycloak.services.resources.admin.fgap.AdminPermissionEvaluator; import org.keycloak.services.resources.admin.fgap.AdminPermissions; @@ -17,20 +16,18 @@ public class DefaultAdminApi implements AdminApi { private final RealmModel realm; private final AdminPermissionEvaluator permissions; - // v1 resources - private final RealmAdminResource realmAdminResource; - public DefaultAdminApi(KeycloakSession session, String realmName) { this.session = session; var authInfo = AdminRoot.authenticateRealmAdminRequest(session); - this.permissions = AdminPermissions.evaluator(session, authInfo.getRealm(), authInfo); - this.realm = session.realms().getRealmByName(realmName); - // remove v1 resource once we are not attached to API v1 - this.realmAdminResource = new RealmsAdminResource(session, authInfo, new TokenManager()).getRealmAdmin(realmName); + RealmModel realm = session.realms().getRealmByName(realmName); + if (realm == null) throw new NotFoundException("Realm not found."); + session.getContext().setRealm(realm); + this.realm = realm; + this.permissions = AdminPermissions.evaluator(session, realm, authInfo); } @Override public ClientsApi clientsV2() { - return new DefaultClientsApi(session, realm, permissions, realmAdminResource); + return new DefaultClientsApi(session, realm, permissions); } } diff --git a/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/client/DefaultClientApi.java b/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/client/DefaultClientApi.java index 90c526d7693..0440d6c65e5 100644 --- a/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/client/DefaultClientApi.java +++ b/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/client/DefaultClientApi.java @@ -20,7 +20,6 @@ import org.keycloak.services.PatchType; import org.keycloak.services.ServiceException; import org.keycloak.services.client.ClientService; import org.keycloak.services.client.DefaultClientService; -import org.keycloak.services.resources.admin.RealmAdminResource; import org.keycloak.services.resources.admin.fgap.AdminPermissionEvaluator; @@ -34,14 +33,12 @@ public class DefaultClientApi implements ClientApi { public DefaultClientApi(@Nonnull KeycloakSession session, @Nonnull RealmModel realm, @Nonnull String clientId, - @Nonnull AdminPermissionEvaluator permissions, - // remove v1 resource once we are not attached to API v1 - @Nonnull RealmAdminResource realmAdminResource) { + @Nonnull AdminPermissionEvaluator permissions) { this.session = session; this.clientId = clientId; this.realm = realm; this.permissions = permissions; - this.clientService = new DefaultClientService(session, realm, permissions, realmAdminResource); + this.clientService = new DefaultClientService(session, realm, permissions); } @GET diff --git a/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/client/DefaultClientsApi.java b/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/client/DefaultClientsApi.java index 1cda721e4c8..af993176941 100644 --- a/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/client/DefaultClientsApi.java +++ b/rest/admin-v2/services/src/main/java/org/keycloak/rest/admin/api/client/DefaultClientsApi.java @@ -19,7 +19,6 @@ import org.keycloak.representations.admin.v2.BaseClientRepresentation; import org.keycloak.services.client.ClientService; import org.keycloak.services.client.ClientService.ClientProjectionOptions; import org.keycloak.services.client.DefaultClientService; -import org.keycloak.services.resources.admin.RealmAdminResource; import org.keycloak.services.resources.admin.fgap.AdminPermissionEvaluator; public class DefaultClientsApi implements ClientsApi { @@ -29,19 +28,13 @@ public class DefaultClientsApi implements ClientsApi { private final RealmModel realm; private final ClientService clientService; - // v1 resources - private final RealmAdminResource realmAdminResource; - public DefaultClientsApi(@Nonnull KeycloakSession session, @Nonnull RealmModel realm, - @Nonnull AdminPermissionEvaluator permissions, - // remove v1 resource once we are not attached to API v1 - @Nonnull RealmAdminResource realmAdminResource) { + @Nonnull AdminPermissionEvaluator permissions) { this.session = session; this.realm = realm; this.permissions = permissions; - this.realmAdminResource = realmAdminResource; - this.clientService = new DefaultClientService(session, realm, permissions, realmAdminResource); + this.clientService = new DefaultClientService(session, realm, permissions); } @Override @@ -71,7 +64,7 @@ public class DefaultClientsApi implements ClientsApi { @Override public ClientApi client(@PathParam("id") String clientId) { enforceAntiPhishingIfClientMissing(clientId); - return new DefaultClientApi(session, realm, clientId, permissions, realmAdminResource); + return new DefaultClientApi(session, realm, clientId, permissions); } } diff --git a/rest/admin-v2/services/src/main/java/org/keycloak/services/client/DefaultClientService.java b/rest/admin-v2/services/src/main/java/org/keycloak/services/client/DefaultClientService.java index 693e4073fdd..de09ec7742f 100644 --- a/rest/admin-v2/services/src/main/java/org/keycloak/services/client/DefaultClientService.java +++ b/rest/admin-v2/services/src/main/java/org/keycloak/services/client/DefaultClientService.java @@ -2,6 +2,7 @@ package org.keycloak.services.client; import java.io.IOException; import java.io.InputStream; +import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Objects; @@ -13,7 +14,6 @@ import java.util.stream.Stream; import jakarta.annotation.Nonnull; import jakarta.validation.groups.Default; -import jakarta.ws.rs.NotFoundException; import jakarta.ws.rs.core.Response; import org.keycloak.authorization.fgap.AdminPermissionsSchema; @@ -26,6 +26,7 @@ import org.keycloak.models.ModelException; import org.keycloak.models.ModelValidationException; import org.keycloak.models.RealmModel; import org.keycloak.models.RoleModel; +import org.keycloak.models.UserModel; import org.keycloak.models.mapper.ClientModelMapper; import org.keycloak.models.mapper.ClientModelMappers; import org.keycloak.models.utils.KeycloakModelUtils; @@ -50,7 +51,6 @@ import org.keycloak.services.clientpolicy.context.AdminClientViewContext; import org.keycloak.services.managers.ClientManager; import org.keycloak.services.managers.RealmManager; import org.keycloak.services.resources.admin.AdminEventBuilder; -import org.keycloak.services.resources.admin.RealmAdminResource; import org.keycloak.services.resources.admin.RoleContainerResource; import org.keycloak.services.resources.admin.fgap.AdminPermissionEvaluator; import org.keycloak.services.util.ObjectMapperResolver; @@ -63,6 +63,7 @@ import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.core.JsonToken; import com.fasterxml.jackson.databind.JsonMappingException; +import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.databind.ObjectReader; import org.apache.http.HttpEntity; @@ -80,18 +81,15 @@ public class DefaultClientService implements ClientService { private final KeycloakSession session; private final AdminPermissionEvaluator permissions; - private final RealmAdminResource realmResource; private final AdminEventBuilder adminEventBuilder; private final JakartaValidatorProvider validator; private final RolesService rolesService; public DefaultClientService(@Nonnull KeycloakSession session, @Nonnull RealmModel realm, - @Nonnull AdminPermissionEvaluator permissions, - @Nonnull RealmAdminResource realmResource) { + @Nonnull AdminPermissionEvaluator permissions) { this.session = session; this.permissions = permissions; - this.realmResource = realmResource; this.adminEventBuilder = new AdminEventV2Builder(realm, permissions.adminAuth(), session, session.getContext().getConnection()).resource(ResourceType.CLIENT); this.validator = new HibernateValidatorProvider(new ValidationContext(session, realm)); this.rolesService = new RolesService(session, realm, permissions, adminEventBuilder); @@ -148,12 +146,12 @@ public class DefaultClientService implements ClientService { @Override public BaseClientRepresentation createClient(RealmModel realm, BaseClientRepresentation client) throws ServiceException { - return createOrUpdate(realm, null, client, CreateOrUpdateStrategy.ONLY_CREATE).representation(); + return createOrUpdate(realm, null, client, CreateOrUpdateStrategy.ONLY_CREATE, false).representation(); } @Override public CreateOrUpdateResult createOrUpdateClient(RealmModel realm, String clientId, BaseClientRepresentation client) throws ServiceException { - return createOrUpdate(realm, clientId, client, CreateOrUpdateStrategy.PUT); + return createOrUpdate(realm, clientId, client, CreateOrUpdateStrategy.PUT, false); } @Override @@ -189,9 +187,25 @@ public class DefaultClientService implements ClientService { .orElseThrow(() -> new ServiceException("Cannot find the specified client", Response.Status.NOT_FOUND)); BaseClientRepresentation updated; + boolean patchExplicitNullSecret = false; switch (patchType) { case JSON_MERGE -> { - try (JsonParser parser = MAPPER.getFactory().createParser(patch)) { + final byte[] patchData; + try { + patchData = patch.readAllBytes(); + } catch (IOException e) { + throw new ServiceException("Unknown Error Occurred", Response.Status.INTERNAL_SERVER_ERROR); + } + try { + JsonNode root = MAPPER.readTree(patchData); + JsonNode authNode = root.get("auth"); + if (authNode != null && authNode.has("secret") && authNode.get("secret").isNull()) { + patchExplicitNullSecret = true; + } + } catch (IOException e) { + throw new ServiceException(e.getMessage(), Response.Status.BAD_REQUEST); + } + try (JsonParser parser = MAPPER.getFactory().createParser(patchData)) { final ObjectReader objectReader = MAPPER.readerForUpdating(getOriginalClient.get()); JsonToken nextToken = parser.nextToken(); if (nextToken != JsonToken.START_OBJECT) { @@ -213,7 +227,7 @@ public class DefaultClientService implements ClientService { default -> throw new ServiceException("Invalid patch type", Response.Status.UNSUPPORTED_MEDIA_TYPE); } - return createOrUpdate(realm, clientId, updated, CreateOrUpdateStrategy.PATCH).representation(); + return createOrUpdate(realm, clientId, updated, CreateOrUpdateStrategy.PATCH, patchExplicitNullSecret).representation(); } @Override @@ -238,7 +252,7 @@ public class DefaultClientService implements ClientService { } } - private CreateOrUpdateResult createOrUpdate(RealmModel realm, String clientId, BaseClientRepresentation client, CreateOrUpdateStrategy strategy) throws ServiceException { + private CreateOrUpdateResult createOrUpdate(RealmModel realm, String clientId, BaseClientRepresentation client, CreateOrUpdateStrategy strategy, boolean patchExplicitNullSecret) throws ServiceException { validateUnknownFields(client); ClientModel model = null; if (!strategy.equals(CreateOrUpdateStrategy.ONLY_CREATE)) { @@ -255,13 +269,12 @@ public class DefaultClientService implements ClientService { case PUT, PATCH -> { // Check permissions, execute validations and trigger client policies permissions.clients().requireConfigure(model); + // Must run before bean validation: PutClient requires a non-blank secret for client-secret methods + generateClientSecretIfNeeded(client, model, strategy, patchExplicitNullSecret); validator.validate(client, strategy.getValidationGroup(), Default.class); var proposedRepresentation = getProposedOldRepresentation(realm, client, mapper); session.clientPolicy().triggerOnEvent(new AdminClientUpdateContext(proposedRepresentation, model, permissions.adminAuth())); - // Generate random secret if applicable - generateClientSecretIfNeeded(client, model); - // Update model mapper.toModel(client, model); @@ -286,7 +299,7 @@ public class DefaultClientService implements ClientService { model.setProtocol(client.getProtocol()); // Generate random secret if applicable - generateClientSecretIfNeeded(client, model); + generateClientSecretIfNeeded(client, model, strategy, patchExplicitNullSecret); mapper.toModel(client, model); // Validate the fully populated model @@ -306,7 +319,7 @@ public class DefaultClientService implements ClientService { // OIDC specific if (client instanceof OIDCClientRepresentation oidcClient) { - handleServiceAccount(clientRoles, rolesService.resource(realm), model, oidcClient); + handleServiceAccount(model, oidcClient); } fireAdminEvent(alreadyExists ? OperationType.UPDATE : OperationType.CREATE, mapper.fromModel(model)); @@ -351,11 +364,21 @@ public class DefaultClientService implements ClientService { } // TODO we should find a way on how to evoke it on the mapper level? - private void generateClientSecretIfNeeded(BaseClientRepresentation client, ClientModel model) { + private void generateClientSecretIfNeeded(BaseClientRepresentation client, ClientModel model, CreateOrUpdateStrategy strategy, boolean patchExplicitNullSecret) { if (client.getProtocol().equals(OIDCClientRepresentation.PROTOCOL)) { var auth = ((OIDCClientRepresentation) client).getAuth(); if (auth != null && isClientSecret(auth.getMethod()) && isBlank(auth.getSecret())) { - auth.setSecret(KeycloakModelUtils.generateSecret(model)); + if (strategy == CreateOrUpdateStrategy.PATCH && patchExplicitNullSecret) { + auth.setSecret(KeycloakModelUtils.generateSecret(model)); + } else { + // On PUT the client often omits the secret; reuse the persisted secret before bean validation (PutClient). + // On PATCH without explicit JSON null for secret, keep the same semantics (do not rotate). + if (!isBlank(model.getSecret())) { + auth.setSecret(model.getSecret()); + } else { + auth.setSecret(KeycloakModelUtils.generateSecret(model)); + } + } } } } @@ -402,9 +425,10 @@ public class DefaultClientService implements ClientService { /** * Declaratively manage service account - enables/disables it and ensures it has exactly the roles specified (realm and client roles) *

- * Reuses API v1 logic + * Applies mappings on the {@link UserModel} with the same permission checks as the Admin REST role-mapping resources, but without + * routing through nested JAX-RS resources (which are not suited for in-process service calls). */ - protected void handleServiceAccount(RoleContainerResource clientRoleResource, RoleContainerResource realmRoleResource, ClientModel model, OIDCClientRepresentation rep) { + protected void handleServiceAccount(ClientModel model, OIDCClientRepresentation rep) { boolean serviceAccountEnabled = rep.getLoginFlows().contains(OIDCClientRepresentation.Flow.SERVICE_ACCOUNT); ClientManager.updateClientServiceAccount(session, model, serviceAccountEnabled); @@ -413,49 +437,48 @@ public class DefaultClientService implements ClientService { return; } - var serviceAccountUser = new ClientManager(new RealmManager(session)).getServiceAccountUser(model) + UserModel serviceAccountUser = new ClientManager(new RealmManager(session)).getServiceAccountUser(model) .orElseThrow(() -> new ServiceException("Cannot find service account user", Response.Status.BAD_REQUEST)); - var serviceAccountRoleResource = realmResource.users().user(serviceAccountUser.getId()).getRoleMappings(); + RealmModel realm = model.getRealm(); Set desiredRoleNames = Optional.ofNullable(rep.getServiceAccountRoles()).orElse(Collections.emptySet()); Set currentRoles = serviceAccountUser.getRoleMappingsStream().collect(Collectors.toSet()); Set currentRoleNames = currentRoles.stream().map(RoleModel::getName).collect(Collectors.toSet()); - // Get missing roles (in desired but not in current) - List missingRoles = desiredRoleNames.stream() - .filter(roleName -> !currentRoleNames.contains(roleName)) - .map(roleName -> { - try { - return clientRoleResource.getRole(roleName); // client role - } catch (NotFoundException e) { - try { - return realmRoleResource.getRole(roleName); // realm role - } catch (NotFoundException e2) { - throw new ServiceException("Cannot assign role to the service account (field 'serviceAccount.roles') as it does not exist", Response.Status.BAD_REQUEST); - } - } - }) - .toList(); - - // Add missing roles (in desired but not in current) - if (!missingRoles.isEmpty()) { - serviceAccountRoleResource.addRealmRoleMappings(missingRoles); + // serviceAccountRoles are plain names; client roles on this client are resolved before realm roles (name collisions favor the client). + List rolesToAdd = new ArrayList<>(); + for (String roleName : desiredRoleNames) { + if (currentRoleNames.contains(roleName)) { + continue; + } + RoleModel clientRole = model.getRole(roleName); + RoleModel resolved = clientRole != null ? clientRole : realm.getRole(roleName); + if (resolved == null) { + throw new ServiceException("Cannot assign role to the service account (field 'serviceAccount.roles') as it does not exist", Response.Status.BAD_REQUEST); + } + rolesToAdd.add(resolved); } - // Get extra roles (in current but not in desired) - List extraRoles = currentRoles.stream() - .filter(role -> !desiredRoleNames.contains(role.getName())) - .map(ModelToRepresentation::toRepresentation) - .toList(); - - // Remove extra roles (in current but not in desired) - if (!extraRoles.isEmpty()) { - try { - serviceAccountRoleResource.deleteRealmRoleMappings(extraRoles); - } catch (NotFoundException e) { - throw new ServiceException("Cannot unassign role from the service account (field 'serviceAccount.roles') as it does not exist", Response.Status.BAD_REQUEST); + List rolesToRemove = new ArrayList<>(); + for (RoleModel role : currentRoles) { + if (!desiredRoleNames.contains(role.getName())) { + rolesToRemove.add(role); } } + + if (rolesToAdd.isEmpty() && rolesToRemove.isEmpty()) { + return; + } + + permissions.users().requireMapRoles(serviceAccountUser); + for (RoleModel role : rolesToAdd) { + permissions.roles().requireMapRole(role); + serviceAccountUser.grantRole(role); + } + for (RoleModel role : rolesToRemove) { + permissions.roles().requireMapRole(role); + serviceAccountUser.deleteRoleMapping(role); + } } protected void validateUnknownFields(BaseClientRepresentation rep) { diff --git a/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/ClientApiV2Test.java b/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/ClientApiV2Test.java index 1f491ff19ea..7a7a7e6f7ed 100644 --- a/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/ClientApiV2Test.java +++ b/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/ClientApiV2Test.java @@ -19,7 +19,9 @@ package org.keycloak.tests.admin.client.v2; import java.io.ByteArrayInputStream; import java.io.IOException; +import java.nio.charset.StandardCharsets; import java.util.List; +import java.util.Locale; import java.util.Set; import java.util.stream.Stream; @@ -531,6 +533,43 @@ public class ClientApiV2Test extends AbstractClientApiV2Test{ } } + @Test + public void declarativeServiceAccountClientRoleManagement() { + String defaultRealmRoles = "default-roles-%s".formatted(testRealm.getName().toLowerCase()); + OIDCClientRepresentation rep = new OIDCClientRepresentation(); + rep.setClientId("sa-client-role-test"); + rep.setEnabled(true); + rep.setRoles(Set.of("my-client-role")); + rep.setLoginFlows(Set.of(OIDCClientRepresentation.Flow.SERVICE_ACCOUNT)); + rep.setServiceAccountRoles(Set.of(defaultRealmRoles, "offline_access")); + + OIDCClientRepresentation.Auth auth = new OIDCClientRepresentation.Auth(); + auth.setMethod(ClientIdAndSecretAuthenticator.PROVIDER_ID); + rep.setAuth(auth); + + try (var response = getClientsApi().createClient(rep)) { + assertEquals(201, response.getStatus()); + OIDCClientRepresentation created = response.readEntity(OIDCClientRepresentation.class); + assertThat(created.getRoles(), is(Set.of("my-client-role"))); + assertThat(created.getServiceAccountRoles(), is(Set.of(defaultRealmRoles, "offline_access"))); + } + + rep.setServiceAccountRoles(Set.of(defaultRealmRoles, "offline_access", "my-client-role")); + try (var response = getClientsApi().client("sa-client-role-test").createOrUpdateClient(rep)) { + assertEquals(200, response.getStatus()); + OIDCClientRepresentation updated = response.readEntity(OIDCClientRepresentation.class); + assertThat(updated.getServiceAccountRoles(), is(Set.of(defaultRealmRoles, "offline_access", "my-client-role"))); + } + + rep.setServiceAccountRoles(Set.of(defaultRealmRoles, "offline_access")); + try (var response = getClientsApi().client("sa-client-role-test").createOrUpdateClient(rep)) { + assertEquals(200, response.getStatus()); + OIDCClientRepresentation updated = response.readEntity(OIDCClientRepresentation.class); + assertThat(updated.getServiceAccountRoles(), is(Set.of(defaultRealmRoles, "offline_access"))); + assertThat(updated.getRoles(), is(Set.of("my-client-role"))); + } + } + @Test public void versionedClientsApi() throws Exception { final var ADMIN_API_URL = "http://localhost:8080/admin/api/master"; @@ -763,8 +802,9 @@ public class ClientApiV2Test extends AbstractClientApiV2Test{ OIDCClientRepresentation.Auth authWithoutSecret = new OIDCClientRepresentation.Auth(); authWithoutSecret.setMethod(authenticationMethod); - authWithoutSecret.setAdditionalField("secret", null); - OIDCClientRepresentation.Auth patchedAuth = getResultingAuthConfigPatch(authWithoutSecret, clientId); + authWithoutSecret.setSecret(null); + OIDCClientRepresentation.Auth patchedAuth = getResultingAuthConfigPatchRawAuth(clientId, + "{\"method\":\"" + authenticationMethod + "\",\"secret\":null}"); assertThat(patchedAuth, notNullValue()); String newlyGeneratedSecret = patchedAuth.getSecret(); assertThat(newlyGeneratedSecret, not(is(createdAuth.getSecret()))); @@ -786,9 +826,7 @@ public class ClientApiV2Test extends AbstractClientApiV2Test{ assertThat(createdAuth, notNullValue()); assertThat(createdAuth.getSecret(), is(auth.getSecret())); - OIDCClientRepresentation.Auth authWithoutSecret = new OIDCClientRepresentation.Auth(); - authWithoutSecret.setAdditionalField("secret", null); - OIDCClientRepresentation.Auth patchedAuth = getResultingAuthConfigPatch(authWithoutSecret, clientId); + OIDCClientRepresentation.Auth patchedAuth = getResultingAuthConfigPatchRawAuth(clientId, "{\"secret\":null}"); assertThat(patchedAuth, notNullValue()); String newlyGeneratedSecret = patchedAuth.getSecret(); assertThat(newlyGeneratedSecret, not(is(createdAuth.getSecret()))); @@ -858,7 +896,7 @@ public class ClientApiV2Test extends AbstractClientApiV2Test{ @ParameterizedTest @ValueSource(strings = { ClientIdAndSecretAuthenticator.PROVIDER_ID, JWTClientSecretAuthenticator.PROVIDER_ID }) - void expectValidationFailureForUpdatePutWithoutSecret(String authenticationMethod) throws IOException { + void putUpdateWithNullSecretReusesPersistedSecret(String authenticationMethod) throws IOException { String clientId = authenticationMethod + "-validation-update-put"; OIDCClientRepresentation.Auth auth = new OIDCClientRepresentation.Auth(); auth.setMethod(authenticationMethod); @@ -869,8 +907,9 @@ public class ClientApiV2Test extends AbstractClientApiV2Test{ assertThat(createdAuth.getSecret(), is(auth.getSecret())); auth.setSecret(null); - var assertionError = assertThrows(AssertionError.class, () -> getResultingAuthConfigPut(auth, clientId)); - assertThat(assertionError.getMessage(), Matchers.containsString("was <400>")); + OIDCClientRepresentation.Auth putAuth = getResultingAuthConfigPut(auth, clientId); + assertThat(putAuth, notNullValue()); + assertThat(putAuth.getSecret(), is(createdAuth.getSecret())); } @ParameterizedTest @@ -926,6 +965,23 @@ public class ClientApiV2Test extends AbstractClientApiV2Test{ return assertClientEnabledIdDescriptionAndAuth(rep, createdClient); } + /** + * Applies a JSON merge patch with a raw {@code auth} object fragment (must include {@code "secret":null} when rotating secrets). + */ + private OIDCClientRepresentation.Auth getResultingAuthConfigPatchRawAuth(String clientId, String authObjectJson) throws JsonProcessingException { + OIDCClientRepresentation rep = new OIDCClientRepresentation(); + rep.setEnabled(true); + rep.setClientId(clientId); + rep.setDescription("I'm OIDC Client"); + rep.setAuth(mapper.readValue(authObjectJson, OIDCClientRepresentation.Auth.class)); + String body = String.format(Locale.ROOT, + "{\"enabled\":true,\"clientId\":\"%s\",\"description\":\"I'm OIDC Client\",\"auth\":%s}", + clientId, authObjectJson); + OIDCClientRepresentation createdClient = (OIDCClientRepresentation) getClientsApi().client(clientId).patchClient( + new ByteArrayInputStream(body.getBytes(StandardCharsets.UTF_8))); + return assertClientEnabledIdDescriptionAndAuth(rep, createdClient); + } + private OIDCClientRepresentation getResultingClientRep(OIDCClientRepresentation.Auth auth, String clientId, String... additionalFields) throws IllegalArgumentException { OIDCClientRepresentation rep = new OIDCClientRepresentation(); rep.setEnabled(true); diff --git a/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/mapper/OIDCClientModelMapperTest.java b/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/mapper/OIDCClientModelMapperTest.java index e0b966827ab..ff764868f4c 100644 --- a/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/mapper/OIDCClientModelMapperTest.java +++ b/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/mapper/OIDCClientModelMapperTest.java @@ -228,6 +228,33 @@ public class OIDCClientModelMapperTest { } } + @TestOnServer + public void fromModel_mapsServiceAccountClientRoles(KeycloakSession session) { + RealmModel realm = session.realms().getRealmByName("master"); + session.getContext().setRealm(realm); + + ClientModel clientModel = realm.addClient("test-sa-client-role-mapper"); + try { + setupBasicClientModel(clientModel); + clientModel.setServiceAccountsEnabled(true); + RoleModel clientRole = clientModel.addRole("mapper-client-sa-role"); + + String username = "service-account-" + clientModel.getClientId(); + UserModel serviceAccount = session.users().addUser(realm, username); + serviceAccount.setEnabled(true); + serviceAccount.setServiceAccountClientLink(clientModel.getId()); + + serviceAccount.grantRole(clientRole); + + OIDCClientModelMapper mapper = getModelMapper(session); + OIDCClientRepresentation rep = (OIDCClientRepresentation) mapper.fromModel(clientModel); + + assertThat(rep.getServiceAccountRoles(), hasItems("mapper-client-sa-role")); + } finally { + realm.removeClient(clientModel.getId()); + } + } + @TestOnServer public void fromModel_emptyServiceAccountRolesWhenServiceAccountDisabled(KeycloakSession session) { RealmModel realm = session.realms().getRealmByName("master");