From 5828fab2583d1663b58236994090ad6b8a810293 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Barto=C5=A1?= Date: Wed, 3 Dec 2025 09:41:18 +0100 Subject: [PATCH] [admin-api-v2] Incorrect DTO/DAO mapping (#44587) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [admin-api-v2] Incorrect DTO/DAO mapping Closes #44586 Signed-off-by: Martin Bartoš * Handle roles and service account operations, cleanup service contract Signed-off-by: Martin Bartoš --------- Signed-off-by: Martin Bartoš --- .../models/mapper/ClientModelMapper.java | 8 +- .../mapper/MapStructClientModelMapper.java | 79 ++++++- .../admin/v2/ClientRepresentation.java | 55 +++++ .../services/client/ClientService.java | 8 +- .../services/client/DefaultClientService.java | 141 ++++++++++-- .../admin/api/client/DefaultClientApi.java | 20 +- .../admin/api/client/DefaultClientsApi.java | 16 +- .../admin/api/realm/DefaultRealmApi.java | 2 +- .../admin/client/v2/ClientApiV2Test.java | 203 ++++++++++++++++++ .../resources/admin/ClientResource.java | 26 ++- 10 files changed, 491 insertions(+), 67 deletions(-) diff --git a/rest/admin-v2/api/src/main/java/org/keycloak/models/mapper/ClientModelMapper.java b/rest/admin-v2/api/src/main/java/org/keycloak/models/mapper/ClientModelMapper.java index 7ebb422079f..63cc0bce8bd 100644 --- a/rest/admin-v2/api/src/main/java/org/keycloak/models/mapper/ClientModelMapper.java +++ b/rest/admin-v2/api/src/main/java/org/keycloak/models/mapper/ClientModelMapper.java @@ -1,12 +1,16 @@ package org.keycloak.models.mapper; import org.keycloak.models.ClientModel; +import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; import org.keycloak.representations.admin.v2.ClientRepresentation; +import org.keycloak.services.ServiceException; public interface ClientModelMapper { - ClientRepresentation fromModel(ClientModel model); + ClientRepresentation fromModel(KeycloakSession session, ClientModel model); - void toModel(ClientModel model, ClientRepresentation rep, RealmModel realm); + ClientModel toModel(KeycloakSession session, RealmModel realm, ClientModel existingModel, ClientRepresentation rep) throws ServiceException; + + ClientModel toModel(KeycloakSession session, RealmModel realm, ClientRepresentation rep) throws ServiceException; } diff --git a/rest/admin-v2/api/src/main/java/org/keycloak/models/mapper/MapStructClientModelMapper.java b/rest/admin-v2/api/src/main/java/org/keycloak/models/mapper/MapStructClientModelMapper.java index 04b4d5c0a07..70535568bb0 100644 --- a/rest/admin-v2/api/src/main/java/org/keycloak/models/mapper/MapStructClientModelMapper.java +++ b/rest/admin-v2/api/src/main/java/org/keycloak/models/mapper/MapStructClientModelMapper.java @@ -1,36 +1,89 @@ package org.keycloak.models.mapper; +import java.util.Collections; import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; import org.keycloak.models.ClientModel; +import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; import org.keycloak.models.RoleModel; import org.keycloak.representations.admin.v2.ClientRepresentation; +import org.keycloak.services.ServiceException; import org.mapstruct.Context; import org.mapstruct.Mapper; import org.mapstruct.Mapping; import org.mapstruct.MappingTarget; import org.mapstruct.Named; +import org.mapstruct.ObjectFactory; @Mapper public interface MapStructClientModelMapper extends ClientModelMapper { + + @Override + @ModelToRep + ClientRepresentation fromModel(@Context KeycloakSession session, ClientModel model); + + // we don't want to ignore nulls so that we completely overwrite the state + @Override + @RepToModel + ClientModel toModel(@Context KeycloakSession session, @Context RealmModel realm, @MappingTarget ClientModel existingModel, ClientRepresentation rep) throws ServiceException; + + @Override + @RepToModel + ClientModel toModel(@Context KeycloakSession session, @Context RealmModel realm, ClientRepresentation rep) throws ServiceException; + + /*-------------------------------------* + * MAPPERS * + *-------------------------------------*/ + @Mapping(target = "name", source = "displayName") + @Mapping(target = "baseUrl", source = "appUrl") + @Mapping(target = "redirectUris", source = "appRedirectUrls") + @Mapping(target = "authenticationFlowBindingOverrides", source = "loginFlows", ignore = true) // TODO + @Mapping(target = "publicClient", source = "auth.enabled", qualifiedByName = "isPublicClientPrimitive") + @Mapping(target = "clientAuthenticatorType", source = "auth.method") + @Mapping(target = "secret", source = "auth.secret") + @Mapping(target = "serviceAccountsEnabled", source = "serviceAccount.enabled") + @interface RepToModel { + } + @Mapping(target = "displayName", source = "name") @Mapping(target = "appUrl", source = "baseUrl") @Mapping(target = "appRedirectUrls", source = "redirectUris") @Mapping(target = "loginFlows", source = "authenticationFlowBindingOverrides", ignore = true) - @Mapping(target = "auth", ignore = true) // TODO + @Mapping(target = "auth.enabled", source = "publicClient", qualifiedByName = "isPublicClient") + @Mapping(target = "auth.method", source = "clientAuthenticatorType") + @Mapping(target = "auth.secret", source = "secret") + @Mapping(target = "auth.certificate", ignore = true) // no cert in the representation @Mapping(target = "roles", source = "rolesStream", qualifiedByName = "getRoleStrings") @Mapping(target = "serviceAccount.enabled", source = "serviceAccountsEnabled") - @Mapping(target = "serviceAccount.roles", source = "rolesStream", qualifiedByName = "getServiceAccountRoles") - @Override - ClientRepresentation fromModel(ClientModel model); + @Mapping(target = "serviceAccount.roles", source = ".", qualifiedByName = "getServiceAccountRoles") + @interface ModelToRep { + } - // we don't want to ignore nulls so that we completely overwrite the state - @Override - void toModel(@MappingTarget ClientModel model, ClientRepresentation rep, @Context RealmModel realm); + /*-------------------------------------* + * HELPER METHODS * + *-------------------------------------*/ + @ObjectFactory + default ClientModel createClientModel(@Context RealmModel realm, ClientRepresentation rep) { + // dummy add/remove to obtain a detached model + var model = realm.addClient(rep.getClientId()); + realm.removeClient(model.getId()); + return model; + } + + @Named("isPublicClientPrimitive") + default boolean isPublicClientPrimitive(Boolean authEnabled) { + var result = isPublicClient(authEnabled); + return result != null ? result : false; + } + + @Named("isPublicClient") + default Boolean isPublicClient(Boolean authEnabled) { + return authEnabled != null ? !authEnabled : null; + } @Named("getRoleStrings") default Set getRoleStrings(Stream stream) { @@ -38,9 +91,13 @@ public interface MapStructClientModelMapper extends ClientModelMapper { } @Named("getServiceAccountRoles") - default Set getServiceAccountRoles(Stream stream) { - return stream.filter(f -> true) //TODO check roles for SA - .map(RoleModel::getName) - .collect(Collectors.toSet()); + default Set getServiceAccountRoles(@Context KeycloakSession session, ClientModel client) { + if (client.isServiceAccountsEnabled()) { + return session.users().getServiceAccount(client) + .getRoleMappingsStream() + .map(RoleModel::getName) + .collect(Collectors.toSet()); + } + return Collections.emptySet(); } } diff --git a/rest/admin-v2/api/src/main/java/org/keycloak/representations/admin/v2/ClientRepresentation.java b/rest/admin-v2/api/src/main/java/org/keycloak/representations/admin/v2/ClientRepresentation.java index c6e8498e6d9..b992d2de29d 100644 --- a/rest/admin-v2/api/src/main/java/org/keycloak/representations/admin/v2/ClientRepresentation.java +++ b/rest/admin-v2/api/src/main/java/org/keycloak/representations/admin/v2/ClientRepresentation.java @@ -1,6 +1,7 @@ package org.keycloak.representations.admin.v2; import java.util.LinkedHashSet; +import java.util.Objects; import java.util.Set; import jakarta.validation.Valid; @@ -211,6 +212,21 @@ public class ClientRepresentation extends BaseRepresentation { public void setCertificate(String certificate) { this.certificate = certificate; } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof Auth auth)) return false; + return Objects.equals(enabled, auth.enabled) + && Objects.equals(method, auth.method) + && Objects.equals(secret, auth.secret) + && Objects.equals(certificate, auth.certificate); + } + + @Override + public int hashCode() { + return Objects.hash(enabled, method, secret, certificate); + } } @JsonInclude(JsonInclude.Include.NON_ABSENT) @@ -238,5 +254,44 @@ public class ClientRepresentation extends BaseRepresentation { public void setRoles(Set roles) { this.roles = roles; } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (!(o instanceof ServiceAccount that)) return false; + return Objects.equals(enabled, that.enabled) + && Objects.equals(roles, that.roles); + } + + @Override + public int hashCode() { + return Objects.hash(enabled, roles); + } + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + ClientRepresentation that = (ClientRepresentation) o; + return Objects.equals(clientId, that.clientId) + && Objects.equals(displayName, that.displayName) + && Objects.equals(description, that.description) + && Objects.equals(protocol, that.protocol) + && Objects.equals(enabled, that.enabled) + && Objects.equals(appUrl, that.appUrl) + && Objects.equals(appRedirectUrls, that.appRedirectUrls) + && Objects.equals(loginFlows, that.loginFlows) + && Objects.equals(auth, that.auth) + && Objects.equals(webOrigins, that.webOrigins) + && Objects.equals(roles, that.roles) + && Objects.equals(serviceAccount, that.serviceAccount); + } + + @Override + public int hashCode() { + return Objects.hash(clientId, displayName, description, protocol, enabled, appUrl, appRedirectUrls, + loginFlows, auth, webOrigins, roles, serviceAccount + ); } } diff --git a/rest/admin-v2/api/src/main/java/org/keycloak/services/client/ClientService.java b/rest/admin-v2/api/src/main/java/org/keycloak/services/client/ClientService.java index 17c5df0f3ea..ad4816fe5f8 100644 --- a/rest/admin-v2/api/src/main/java/org/keycloak/services/client/ClientService.java +++ b/rest/admin-v2/api/src/main/java/org/keycloak/services/client/ClientService.java @@ -7,8 +7,6 @@ import org.keycloak.models.RealmModel; import org.keycloak.representations.admin.v2.ClientRepresentation; import org.keycloak.services.Service; import org.keycloak.services.ServiceException; -import org.keycloak.services.resources.admin.ClientResource; -import org.keycloak.services.resources.admin.ClientsResource; public interface ClientService extends Service { @@ -29,12 +27,12 @@ public interface ClientService extends Service { record CreateOrUpdateResult(ClientRepresentation representation, boolean created) {} - Optional getClient(ClientResource clientResource, RealmModel realm, String clientId, ClientProjectionOptions projectionOptions); + Optional getClient(RealmModel realm, String clientId, ClientProjectionOptions projectionOptions); - Stream getClients(ClientsResource clientsResource, RealmModel realm, ClientProjectionOptions projectionOptions, ClientSearchOptions searchOptions, ClientSortAndSliceOptions sortAndSliceOptions); + Stream getClients(RealmModel realm, ClientProjectionOptions projectionOptions, ClientSearchOptions searchOptions, ClientSortAndSliceOptions sortAndSliceOptions); Stream deleteClients(RealmModel realm, ClientSearchOptions searchOptions); - CreateOrUpdateResult createOrUpdate(ClientsResource clientsResource, ClientResource clientResource, RealmModel realm, ClientRepresentation client, boolean allowUpdate) throws ServiceException; + CreateOrUpdateResult createOrUpdate(RealmModel realm, ClientRepresentation client, boolean allowUpdate) throws ServiceException; } diff --git a/rest/admin-v2/api/src/main/java/org/keycloak/services/client/DefaultClientService.java b/rest/admin-v2/api/src/main/java/org/keycloak/services/client/DefaultClientService.java index 41591d59dbd..2018f1e8be6 100644 --- a/rest/admin-v2/api/src/main/java/org/keycloak/services/client/DefaultClientService.java +++ b/rest/admin-v2/api/src/main/java/org/keycloak/services/client/DefaultClientService.java @@ -1,21 +1,29 @@ package org.keycloak.services.client; +import java.util.Collections; +import java.util.List; import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; import java.util.stream.Stream; +import jakarta.ws.rs.NotFoundException; import jakarta.ws.rs.core.Response; import org.keycloak.models.ClientModel; import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; +import org.keycloak.models.RoleModel; import org.keycloak.models.mapper.ClientModelMapper; import org.keycloak.models.mapper.MapStructModelMapper; import org.keycloak.models.utils.ModelToRepresentation; import org.keycloak.representations.admin.v2.ClientRepresentation; import org.keycloak.representations.admin.v2.validation.CreateClientDefault; +import org.keycloak.representations.idm.RoleRepresentation; import org.keycloak.services.ServiceException; import org.keycloak.services.resources.admin.ClientResource; import org.keycloak.services.resources.admin.ClientsResource; +import org.keycloak.services.resources.admin.RealmAdminResource; import org.keycloak.validation.jakarta.HibernateValidatorProvider; import org.keycloak.validation.jakarta.JakartaValidatorProvider; @@ -24,55 +32,61 @@ public class DefaultClientService implements ClientService { private final KeycloakSession session; private final ClientModelMapper mapper; private final JakartaValidatorProvider validator; + private final RealmAdminResource realmAdminResource; + private final ClientsResource clientsResource; + private ClientResource clientResource; - public DefaultClientService(KeycloakSession session) { + public DefaultClientService(KeycloakSession session, RealmAdminResource realmAdminResource, ClientResource clientResource) { this.session = session; + this.realmAdminResource = realmAdminResource; + this.clientResource = clientResource; + + this.clientsResource = realmAdminResource.getClients(); this.mapper = new MapStructModelMapper().clients(); this.validator = new HibernateValidatorProvider(); } - @Override - public Optional getClient(ClientResource clientResource, RealmModel realm, String clientId, - ClientProjectionOptions projectionOptions) { - // TODO: is the access map on the representation needed - return Optional.ofNullable(clientResource).map(ClientResource::viewClientModel).map(mapper::fromModel); + public DefaultClientService(KeycloakSession session, RealmAdminResource realmAdminResource) { + this(session, realmAdminResource, null); } @Override - public Stream getClients(ClientsResource clientsResource, RealmModel realm, - ClientProjectionOptions projectionOptions, ClientSearchOptions searchOptions, - ClientSortAndSliceOptions sortAndSliceOptions) { + public Optional getClient(RealmModel realm, String clientId, ClientProjectionOptions projectionOptions) { // TODO: is the access map on the representation needed - return clientsResource.getClientModels(null, true, false, null, null, null).map(mapper::fromModel); + return Optional.ofNullable(clientResource).map(ClientResource::viewClientModel).map(model -> mapper.fromModel(session, model)); } @Override - public CreateOrUpdateResult createOrUpdate(ClientsResource clientsResource, ClientResource clientResource, - RealmModel realm, ClientRepresentation client, boolean allowUpdate) throws ServiceException { + public Stream getClients(RealmModel realm, ClientProjectionOptions projectionOptions, + ClientSearchOptions searchOptions, ClientSortAndSliceOptions sortAndSliceOptions) { + // TODO: is the access map on the representation needed + return clientsResource.getClientModels(null, true, false, null, null, null).map(model -> mapper.fromModel(session, model)); + } + + @Override + public CreateOrUpdateResult createOrUpdate(RealmModel realm, ClientRepresentation client, boolean allowUpdate) throws ServiceException { boolean created = false; - ClientModel model = null; + ClientModel model; if (clientResource != null) { if (!allowUpdate) { throw new ServiceException("Client already exists", Response.Status.CONFLICT); } - model = clientResource.viewClientModel(); - mapper.toModel(model, client, realm); + model = mapper.toModel(session, realm, clientResource.viewClientModel(), client); var rep = ModelToRepresentation.toRepresentation(model, session); clientResource.update(rep); } else { created = true; validator.validate(client, CreateClientDefault.class); // TODO improve it to avoid second validation when we know it is create and not update - // dummy add/remove to obtain a detached model - model = realm.addClient(client.getClientId()); - realm.removeClient(model.getId()); - - mapper.toModel(model, client, realm); + model = mapper.toModel(session, realm, client); var rep = ModelToRepresentation.toRepresentation(model, session); model = clientsResource.createClientModel(rep); + clientResource = clientsResource.getClient(model.getId()); } - var updated = mapper.fromModel(model); + handleRoles(client.getRoles()); + handleServiceAccount(model, client.getServiceAccount()); + var updated = mapper.fromModel(session, model); return new CreateOrUpdateResult(updated, created); } @@ -83,4 +97,89 @@ public class DefaultClientService implements ClientService { return null; } + /** + * Declaratively manage client roles - ensures the client has exactly the roles specified in 'rolesFromRep' + *

+ * Reuses API v1 logic + */ + protected void handleRoles(Set rolesFromRep) { + var roleResource = clientResource.getRoleContainerResource(); + + Set desiredRoleNames = Optional.ofNullable(rolesFromRep) + .orElse(Collections.emptySet()); + + Set currentRoleNames = roleResource.getRoles(null, null, null, false) + .map(RoleRepresentation::getName) + .collect(Collectors.toSet()); + + // Add missing roles (in desiredRoleNames but not in currentRoleNames) + desiredRoleNames.stream() + .filter(roleName -> !currentRoleNames.contains(roleName)) + .forEach(roleName -> roleResource.createRole(new RoleRepresentation(roleName, "", false))); + + // Remove extra roles (in currentRoleNames but not in desiredRoleNames) + currentRoleNames.stream() + .filter(role -> !desiredRoleNames.contains(role)) + .forEach(roleResource::deleteRole); + } + + /** + * Declaratively manage service account - enables/disables it and ensures it has exactly the roles specified (realm and client roles) + *

+ * Reuses API v1 logic + */ + protected void handleServiceAccount(ClientModel model, ClientRepresentation.ServiceAccount serviceAccount) { + if (serviceAccount != null && serviceAccount.getEnabled() != null) { + ClientResource.updateClientServiceAccount(session, model, serviceAccount.getEnabled()); + + if (serviceAccount.getEnabled()) { + var clientRoleResource = clientResource.getRoleContainerResource(); + var realmRoleResource = realmAdminResource.getRoleContainerResource(); + + var serviceAccountUser = session.users().getServiceAccount(model); + var serviceAccountRoleResource = realmAdminResource.users().user(clientResource.getServiceAccountUser().getId()).getRoleMappings(); + + Set desiredRoleNames = Optional.ofNullable(serviceAccount.getRoles()).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); + } + + // 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); + } + } + } + } + } + } diff --git a/rest/admin-v2/rest/src/main/java/org/keycloak/admin/api/client/DefaultClientApi.java b/rest/admin-v2/rest/src/main/java/org/keycloak/admin/api/client/DefaultClientApi.java index 1df39b06135..4e2b49a5679 100644 --- a/rest/admin-v2/rest/src/main/java/org/keycloak/admin/api/client/DefaultClientApi.java +++ b/rest/admin-v2/rest/src/main/java/org/keycloak/admin/api/client/DefaultClientApi.java @@ -18,7 +18,7 @@ import org.keycloak.services.ServiceException; import org.keycloak.services.client.ClientService; import org.keycloak.services.client.DefaultClientService; import org.keycloak.services.resources.admin.ClientResource; -import org.keycloak.services.resources.admin.ClientsResource; +import org.keycloak.services.resources.admin.RealmAdminResource; import org.keycloak.services.util.ObjectMapperResolver; import com.fasterxml.jackson.core.JsonProcessingException; @@ -34,26 +34,26 @@ public class DefaultClientApi implements ClientApi { private final ClientService clientService; private final ClientResource clientResource; - private final ClientsResource clientsResource; private final String clientId; private final ObjectMapper objectMapper; private static final ObjectMapper MAPPER = new ObjectMapperResolver().getContext(null); - public DefaultClientApi(KeycloakSession session, ClientsResource clientsResource, ClientResource clientResource, String clientId) { + public DefaultClientApi(KeycloakSession session, RealmAdminResource realmAdminResource, ClientResource clientResource, String clientId) { this.session = session; - this.realm = Objects.requireNonNull(session.getContext().getRealm()); - this.client = Objects.requireNonNull(session.getContext().getClient()); - this.clientService = new DefaultClientService(session); - this.clientsResource = clientsResource; this.clientResource = clientResource; this.clientId = clientId; + + this.realm = Objects.requireNonNull(session.getContext().getRealm()); + this.client = Objects.requireNonNull(session.getContext().getClient()); + this.clientService = new DefaultClientService(session, realmAdminResource, clientResource); + this.objectMapper = MAPPER; } @Override public ClientRepresentation getClient() { - return clientService.getClient(clientResource, realm, client.getClientId(), null) + return clientService.getClient(realm, client.getClientId(), null) .orElseThrow(() -> new NotFoundException("Cannot find the specified client")); } @@ -64,7 +64,7 @@ public class DefaultClientApi implements ClientApi { throw new WebApplicationException("cliendId in payload does not match the clientId in the path", Response.Status.BAD_REQUEST); } validateUnknownFields(client); - var result = clientService.createOrUpdate(clientsResource, clientResource, realm, client, true); + var result = clientService.createOrUpdate(realm, client, true); return Response.status(result.created() ? Response.Status.CREATED : Response.Status.OK).entity(result.representation()).build(); } catch (ServiceException e) { throw new WebApplicationException(e.getMessage(), e.getSuggestedResponseStatus().orElse(Response.Status.BAD_REQUEST)); @@ -86,7 +86,7 @@ public class DefaultClientApi implements ClientApi { ClientRepresentation updated = objectReader.readValue(patch); validateUnknownFields(updated); - return clientService.createOrUpdate(clientsResource, clientResource, realm, updated, true).representation(); + return clientService.createOrUpdate(realm, updated, true).representation(); } catch (IllegalArgumentException e) { throw new WebApplicationException("Unsupported media type", Response.Status.UNSUPPORTED_MEDIA_TYPE); } catch (JsonProcessingException e) { diff --git a/rest/admin-v2/rest/src/main/java/org/keycloak/admin/api/client/DefaultClientsApi.java b/rest/admin-v2/rest/src/main/java/org/keycloak/admin/api/client/DefaultClientsApi.java index 5a8e9f8bb07..d1769101fcc 100644 --- a/rest/admin-v2/rest/src/main/java/org/keycloak/admin/api/client/DefaultClientsApi.java +++ b/rest/admin-v2/rest/src/main/java/org/keycloak/admin/api/client/DefaultClientsApi.java @@ -17,6 +17,7 @@ import org.keycloak.services.ServiceException; import org.keycloak.services.client.ClientService; import org.keycloak.services.client.DefaultClientService; import org.keycloak.services.resources.admin.ClientsResource; +import org.keycloak.services.resources.admin.RealmAdminResource; import org.keycloak.validation.jakarta.HibernateValidatorProvider; import org.keycloak.validation.jakarta.JakartaValidatorProvider; @@ -25,19 +26,22 @@ public class DefaultClientsApi implements ClientsApi { private final RealmModel realm; private final ClientService clientService; private final JakartaValidatorProvider validator; + private final RealmAdminResource realmAdminResource; private final ClientsResource clientsResource; - public DefaultClientsApi(KeycloakSession session, ClientsResource clientsResource) { + public DefaultClientsApi(KeycloakSession session, RealmAdminResource realmAdminResource) { this.session = session; + this.realmAdminResource = realmAdminResource; + this.realm = Objects.requireNonNull(session.getContext().getRealm()); - this.clientService = new DefaultClientService(session); + this.clientService = new DefaultClientService(session, realmAdminResource); this.validator = new HibernateValidatorProvider(); - this.clientsResource = clientsResource; + this.clientsResource = realmAdminResource.getClients(); } @Override public Stream getClients() { - return clientService.getClients(clientsResource, realm, null, null, null); + return clientService.getClients(realm, null, null, null); } @Override @@ -46,7 +50,7 @@ public class DefaultClientsApi implements ClientsApi { DefaultClientApi.validateUnknownFields(client); validator.validate(client, CreateClientDefault.class); return Response.status(Response.Status.CREATED) - .entity(clientService.createOrUpdate(clientsResource, null, realm, client, false).representation()) + .entity(clientService.createOrUpdate(realm, client, false).representation()) .build(); } catch (ServiceException e) { throw new WebApplicationException(e.getMessage(), e.getSuggestedResponseStatus().orElse(Response.Status.BAD_REQUEST)); @@ -56,7 +60,7 @@ public class DefaultClientsApi implements ClientsApi { @Override public ClientApi client(@PathParam("id") String clientId) { var client = Optional.ofNullable(session.clients().getClientByClientId(realm, clientId)); - return new DefaultClientApi(session, clientsResource, client.map(c -> clientsResource.getClient(c.getId())).orElse(null), clientId); + return new DefaultClientApi(session, realmAdminResource, client.map(c -> clientsResource.getClient(c.getId())).orElse(null), clientId); } } diff --git a/rest/admin-v2/rest/src/main/java/org/keycloak/admin/api/realm/DefaultRealmApi.java b/rest/admin-v2/rest/src/main/java/org/keycloak/admin/api/realm/DefaultRealmApi.java index e84ad8d9d2d..9384de9fdf8 100644 --- a/rest/admin-v2/rest/src/main/java/org/keycloak/admin/api/realm/DefaultRealmApi.java +++ b/rest/admin-v2/rest/src/main/java/org/keycloak/admin/api/realm/DefaultRealmApi.java @@ -19,7 +19,7 @@ public class DefaultRealmApi implements RealmApi { @Path("clients") @Override public ClientsApi clients() { - return new DefaultClientsApi(session, realmAdminResource.getClients()); + return new DefaultClientsApi(session, realmAdminResource); } } 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 3e1c574c4d3..ed1ba9c3850 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 @@ -17,6 +17,8 @@ package org.keycloak.tests.admin.client.v2; +import java.util.Set; + import jakarta.ws.rs.core.HttpHeaders; import jakarta.ws.rs.core.MediaType; @@ -48,6 +50,7 @@ import org.apache.http.util.EntityUtils; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; +import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.notNullValue; @@ -276,6 +279,206 @@ public class ClientApiV2Test { } } + @Test + public void createFullClient() throws Exception { + HttpPost request = new HttpPost(HOSTNAME_LOCAL_ADMIN + "/realms/master/clients"); + setAuthHeader(request); + request.setHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON); + + ClientRepresentation rep = getTestingFullClientRep(); + request.setEntity(new StringEntity(mapper.writeValueAsString(rep))); + + try (var response = client.execute(request)) { + assertEquals(201, response.getStatusLine().getStatusCode()); + ClientRepresentation client = mapper.createParser(response.getEntity().getContent()).readValueAs(ClientRepresentation.class); + assertThat(client, is(rep)); + } + } + + @Test + public void createFullClientWrongServiceAccountRoles() throws Exception { + HttpPost request = new HttpPost(HOSTNAME_LOCAL_ADMIN + "/realms/master/clients"); + setAuthHeader(request); + request.setHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON); + + ClientRepresentation rep = getTestingFullClientRep(); + rep.getServiceAccount().setRoles(Set.of("non-existing", "bad-role")); + request.setEntity(new StringEntity(mapper.writeValueAsString(rep))); + + try (var response = client.execute(request)) { + assertEquals(400, response.getStatusLine().getStatusCode()); + assertThat(EntityUtils.toString(response.getEntity()), containsString("Cannot assign role to the service account (field 'serviceAccount.roles') as it does not exist")); + } + } + + @Test + public void declarativeRoleManagement() throws Exception { + // 1. Create a client with initial roles + HttpPut createRequest = new HttpPut(HOSTNAME_LOCAL_ADMIN + "/realms/master/clients/declarative-role-test"); + setAuthHeader(createRequest); + createRequest.setHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON); + + ClientRepresentation rep = new ClientRepresentation(); + rep.setClientId("declarative-role-test"); + rep.setEnabled(true); + rep.setRoles(Set.of("role1", "role2", "role3")); + + createRequest.setEntity(new StringEntity(mapper.writeValueAsString(rep))); + + try (var response = client.execute(createRequest)) { + assertEquals(201, response.getStatusLine().getStatusCode()); + ClientRepresentation created = mapper.createParser(response.getEntity().getContent()).readValueAs(ClientRepresentation.class); + assertThat(created.getRoles(), is(Set.of("role1", "role2", "role3"))); + } + + // 2. Update with completely new roles - should remove old ones and add new ones + HttpPut updateRequest = new HttpPut(HOSTNAME_LOCAL_ADMIN + "/realms/master/clients/declarative-role-test"); + setAuthHeader(updateRequest); + updateRequest.setHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON); + + rep.setRoles(Set.of("new-role1", "new-role2")); + updateRequest.setEntity(new StringEntity(mapper.writeValueAsString(rep))); + + try (var response = client.execute(updateRequest)) { + assertEquals(200, response.getStatusLine().getStatusCode()); + ClientRepresentation updated = mapper.createParser(response.getEntity().getContent()).readValueAs(ClientRepresentation.class); + assertThat(updated.getRoles(), is(Set.of("new-role1", "new-role2"))); + } + + // 3. Update with partial overlap - keep some, add some, remove some + rep.setRoles(Set.of("new-role1", "add-role3", "add-role4")); + updateRequest.setEntity(new StringEntity(mapper.writeValueAsString(rep))); + + try (var response = client.execute(updateRequest)) { + assertEquals(200, response.getStatusLine().getStatusCode()); + ClientRepresentation updated = mapper.createParser(response.getEntity().getContent()).readValueAs(ClientRepresentation.class); + assertThat(updated.getRoles(), is(Set.of("new-role1", "add-role3", "add-role4"))); + } + + // 4. Update with same roles - should be idempotent + rep.setRoles(Set.of("new-role1", "add-role3", "add-role4")); + updateRequest.setEntity(new StringEntity(mapper.writeValueAsString(rep))); + + try (var response = client.execute(updateRequest)) { + assertEquals(200, response.getStatusLine().getStatusCode()); + ClientRepresentation updated = mapper.createParser(response.getEntity().getContent()).readValueAs(ClientRepresentation.class); + assertThat(updated.getRoles(), is(Set.of("new-role1", "add-role3", "add-role4"))); + } + + // 5. Update with empty set - should remove all roles + rep.setRoles(Set.of()); + updateRequest.setEntity(new StringEntity(mapper.writeValueAsString(rep))); + + try (var response = client.execute(updateRequest)) { + assertEquals(200, response.getStatusLine().getStatusCode()); + ClientRepresentation updated = mapper.createParser(response.getEntity().getContent()).readValueAs(ClientRepresentation.class); + assertThat(updated.getRoles(), is(Set.of())); + } + } + + @Test + public void declarativeServiceAccountRoleManagement() throws Exception { + // 1. Create a client with service account and initial realm roles + HttpPut createRequest = new HttpPut(HOSTNAME_LOCAL_ADMIN + "/realms/master/clients/sa-declarative-test"); + setAuthHeader(createRequest); + createRequest.setHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON); + + ClientRepresentation rep = new ClientRepresentation(); + rep.setClientId("sa-declarative-test"); + rep.setEnabled(true); + + var serviceAccount = new ClientRepresentation.ServiceAccount(); + serviceAccount.setEnabled(true); + serviceAccount.setRoles(Set.of("default-roles-master", "offline_access")); + rep.setServiceAccount(serviceAccount); + + createRequest.setEntity(new StringEntity(mapper.writeValueAsString(rep))); + + try (var response = client.execute(createRequest)) { + assertEquals(201, response.getStatusLine().getStatusCode()); + ClientRepresentation created = mapper.createParser(response.getEntity().getContent()).readValueAs(ClientRepresentation.class); + assertThat(created.getServiceAccount().getRoles(), is(Set.of("default-roles-master", "offline_access"))); + } + + // 2. Update with completely new roles - should remove old ones and add new ones + HttpPut updateRequest = new HttpPut(HOSTNAME_LOCAL_ADMIN + "/realms/master/clients/sa-declarative-test"); + setAuthHeader(updateRequest); + updateRequest.setHeader(HttpHeaders.CONTENT_TYPE, MediaType.APPLICATION_JSON); + + serviceAccount.setRoles(Set.of("uma_authorization", "offline_access")); + rep.setServiceAccount(serviceAccount); + updateRequest.setEntity(new StringEntity(mapper.writeValueAsString(rep))); + + try (var response = client.execute(updateRequest)) { + assertEquals(200, response.getStatusLine().getStatusCode()); + ClientRepresentation updated = mapper.createParser(response.getEntity().getContent()).readValueAs(ClientRepresentation.class); + assertThat(updated.getServiceAccount().getRoles(), is(Set.of("uma_authorization", "offline_access"))); + } + + // 3. Update with partial overlap - keep some, add some, remove some + serviceAccount.setRoles(Set.of("offline_access", "default-roles-master")); + rep.setServiceAccount(serviceAccount); + updateRequest.setEntity(new StringEntity(mapper.writeValueAsString(rep))); + + try (var response = client.execute(updateRequest)) { + assertEquals(200, response.getStatusLine().getStatusCode()); + ClientRepresentation updated = mapper.createParser(response.getEntity().getContent()).readValueAs(ClientRepresentation.class); + assertThat(updated.getServiceAccount().getRoles(), is(Set.of("offline_access", "default-roles-master"))); + } + + // 4. Update with same roles - should be idempotent + serviceAccount.setRoles(Set.of("offline_access", "default-roles-master")); + rep.setServiceAccount(serviceAccount); + updateRequest.setEntity(new StringEntity(mapper.writeValueAsString(rep))); + + try (var response = client.execute(updateRequest)) { + assertEquals(200, response.getStatusLine().getStatusCode()); + ClientRepresentation updated = mapper.createParser(response.getEntity().getContent()).readValueAs(ClientRepresentation.class); + assertThat(updated.getServiceAccount().getRoles(), is(Set.of("offline_access", "default-roles-master"))); + } + + // 5. Update with empty set - should remove all roles + serviceAccount.setRoles(Set.of()); + rep.setServiceAccount(serviceAccount); + updateRequest.setEntity(new StringEntity(mapper.writeValueAsString(rep))); + + try (var response = client.execute(updateRequest)) { + assertEquals(200, response.getStatusLine().getStatusCode()); + ClientRepresentation updated = mapper.createParser(response.getEntity().getContent()).readValueAs(ClientRepresentation.class); + assertThat(updated.getServiceAccount().getRoles(), is(Set.of())); + } + } + + private ClientRepresentation getTestingFullClientRep() { + var rep = new ClientRepresentation(); + rep.setClientId("my-client"); + rep.setDisplayName("My Client"); + rep.setDescription("This is My Client"); + rep.setProtocol(ClientRepresentation.OIDC); + rep.setEnabled(true); + rep.setAppUrl("http://localhost:3000"); + rep.setAppRedirectUrls(Set.of("http://localhost:3000", "http://localhost:3001")); + // no login flows -> only flow overrides map + // rep.setLoginFlows(Set.of("browser")); + var auth = new ClientRepresentation.Auth(); + auth.setEnabled(true); + auth.setMethod("client-jwt"); + auth.setSecret("secret-1234"); + // no certificate inside the old rep + // auth.setCertificate("certificate-5678"); + rep.setAuth(auth); + rep.setWebOrigins(Set.of("http://localhost:4000", "http://localhost:4001")); + rep.setRoles(Set.of("view-consent", "manage-account")); + var serviceAccount = new ClientRepresentation.ServiceAccount(); + serviceAccount.setEnabled(true); + // TODO when roles are not set and SA is enabled, the default role 'default-roles-master' for the SA is used for the master realm + serviceAccount.setRoles(Set.of("default-roles-master")); + rep.setServiceAccount(serviceAccount); + // not implemented yet + // rep.setAdditionalFields(Map.of("key1", "val1", "key2", "val2")); + return rep; + } + // TODO Rewrite the tests to not need explicit auth. They should use the admin client directly. private void setAuthHeader(HttpMessage request, Keycloak adminClient) { String token = adminClient.tokenManager().getAccessTokenString(); diff --git a/services/src/main/java/org/keycloak/services/resources/admin/ClientResource.java b/services/src/main/java/org/keycloak/services/resources/admin/ClientResource.java index d6cd0c82bbd..dc471befcbd 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/ClientResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/ClientResource.java @@ -822,17 +822,7 @@ public class ClientResource { } private void updateClientFromRep(ClientRepresentation rep, ClientModel client, KeycloakSession session) throws ModelDuplicateException { - UserModel serviceAccount = this.session.users().getServiceAccount(client); - boolean serviceAccountScopeAssigned = client.getClientScopes(true).containsKey(ServiceAccountConstants.SERVICE_ACCOUNT_SCOPE); - if (Boolean.TRUE.equals(rep.isServiceAccountsEnabled())) { - if (serviceAccount == null || !serviceAccountScopeAssigned) { - new ClientManager(new RealmManager(session)).enableServiceAccount(client); - } - } else if (Boolean.FALSE.equals(rep.isServiceAccountsEnabled()) || !client.isServiceAccountsEnabled()) { - if (serviceAccount != null || serviceAccountScopeAssigned) { - new ClientManager(new RealmManager(session)).disableServiceAccount(client); - } - } + updateClientServiceAccount(session, client, rep.isServiceAccountsEnabled()); if (rep.getClientId() != null && !rep.getClientId().equals(client.getClientId())) { new ClientManager(new RealmManager(session)).clientIdChanged(client, rep); @@ -851,6 +841,20 @@ public class ClientResource { updateAuthorizationSettings(rep); } + public static void updateClientServiceAccount(KeycloakSession session, ClientModel client, Boolean isServiceAccountEnabled) { + UserModel serviceAccount = session.users().getServiceAccount(client); + boolean serviceAccountScopeAssigned = client.getClientScopes(true).containsKey(ServiceAccountConstants.SERVICE_ACCOUNT_SCOPE); + if (Boolean.TRUE.equals(isServiceAccountEnabled)) { + if (serviceAccount == null || !serviceAccountScopeAssigned) { + new ClientManager(new RealmManager(session)).enableServiceAccount(client); + } + } else if (Boolean.FALSE.equals(isServiceAccountEnabled) || !client.isServiceAccountsEnabled()) { + if (serviceAccount != null || serviceAccountScopeAssigned) { + new ClientManager(new RealmManager(session)).disableServiceAccount(client); + } + } + } + private void updateAuthorizationSettings(ClientRepresentation rep) { if (Profile.isFeatureEnabled(Profile.Feature.AUTHORIZATION)) { if (Boolean.TRUE.equals(rep.getAuthorizationServicesEnabled())) {