diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ce76b11c58b..74ebeb2908d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1240,10 +1240,6 @@ jobs: - build - conditional timeout-minutes: 20 - strategy: - matrix: - service-type: [ new-service, v1-service ] - fail-fast: false steps: - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 @@ -1252,12 +1248,7 @@ jobs: uses: ./.github/actions/integration-test-setup - name: Run tests - run: | - declare -A PARAMS - PARAMS["new-service"]="-Dkc.admin-v2.client-service.legacy.enabled=false" - PARAMS["v1-service"]="" - - ./mvnw verify -pl rest/admin-v2/tests/pom.xml ${PARAMS["${{ matrix.service-type }}"]} + run: ./mvnw verify -pl rest/admin-v2/tests/pom.xml mixed-cluster-compatibility-tests: name: Cluster Compatibility Tests diff --git a/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakClientTest.java b/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakClientTest.java index bb41046edc1..ba8af1d965d 100644 --- a/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakClientTest.java +++ b/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakClientTest.java @@ -50,8 +50,10 @@ import io.fabric8.kubernetes.api.model.Secret; import io.fabric8.kubernetes.api.model.SecretBuilder; import io.fabric8.kubernetes.api.model.SecretKeySelector; import io.fabric8.kubernetes.api.model.ServiceBuilder; +import io.quarkus.test.junit.QuarkusTest; import org.awaitility.Awaitility; import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; import static org.keycloak.operator.testsuite.utils.K8sUtils.deployKeycloak; @@ -60,7 +62,9 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertNull; -public abstract class KeycloakClientTest extends BaseOperatorTest { +@Tag(BaseOperatorTest.SLOW) +@QuarkusTest +public class KeycloakClientTest extends BaseOperatorTest { private static final String CLIENT_SECRET = "client-secret"; private static final String CLIENT_TRUSTSTORE_SECRET = "example-mtls-truststore-secret"; diff --git a/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakLegacyServiceClientTest.java b/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakLegacyServiceClientTest.java deleted file mode 100644 index e24d49328f0..00000000000 --- a/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakLegacyServiceClientTest.java +++ /dev/null @@ -1,12 +0,0 @@ -package org.keycloak.operator.testsuite.integration; - -import io.quarkus.test.junit.QuarkusTest; -import org.junit.jupiter.api.Tag; - -/** - * Just temporary - once we use the new client service, we can remove this and the {@link KeycloakNewServiceClientTest} - */ -@Tag(BaseOperatorTest.SLOW) -@QuarkusTest -public class KeycloakLegacyServiceClientTest extends KeycloakClientTest { -} diff --git a/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakNewServiceClientTest.java b/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakNewServiceClientTest.java deleted file mode 100644 index 5f4fbfe495e..00000000000 --- a/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakNewServiceClientTest.java +++ /dev/null @@ -1,39 +0,0 @@ -package org.keycloak.operator.testsuite.integration; - -import org.keycloak.operator.crds.v2beta1.deployment.Keycloak; -import org.keycloak.operator.crds.v2beta1.deployment.ValueOrSecret; - -import io.quarkus.test.junit.QuarkusTest; -import org.junit.jupiter.api.Tag; - -/** - * Just temporary - once we use the new client service by default, we can remove this and the {@link KeycloakLegacyServiceClientTest} - */ -@Tag(BaseOperatorTest.SLOW) -@QuarkusTest -public class KeycloakNewServiceClientTest extends KeycloakClientTest { - - @Override - protected Keycloak getTestDeployment(boolean disableProbes) { - Keycloak kc = super.getTestDeployment(disableProbes); - - String newClientServiceProperty = "-Dkc.admin-v2.client-service.legacy.enabled=false"; - var existingJavaOptsAppend = kc.getSpec().getEnv().stream() - .filter(env -> "JAVA_OPTS_APPEND".equals(env.getName())) - .findFirst() - .orElse(null); - - if (existingJavaOptsAppend != null) { - String existingValue = existingJavaOptsAppend.getValue(); - if (existingValue != null && !existingValue.isBlank()) { - existingJavaOptsAppend.setValue(existingValue + " " + newClientServiceProperty); - } else { - existingJavaOptsAppend.setValue(newClientServiceProperty); - } - } else { - kc.getSpec().getEnv().add(new ValueOrSecret("JAVA_OPTS_APPEND", newClientServiceProperty)); - } - - return kc; - } -} 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 bf6c4b07a73..92d39db8935 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 @@ -19,7 +19,7 @@ import org.keycloak.representations.admin.v2.BaseClientRepresentation; import org.keycloak.services.PatchType; import org.keycloak.services.ServiceException; import org.keycloak.services.client.ClientService; -import org.keycloak.services.client.ClientServiceHelper; +import org.keycloak.services.client.DefaultClientService; import org.keycloak.services.resources.admin.RealmAdminResource; import org.keycloak.services.resources.admin.fgap.AdminPermissionEvaluator; @@ -39,7 +39,7 @@ public class DefaultClientApi implements ClientApi { this.session = session; this.clientId = clientId; this.realm = realm; - this.clientService = ClientServiceHelper.getClientService(session, realm, permissions, realmAdminResource); + this.clientService = new DefaultClientService(session, realm, permissions, realmAdminResource); } @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 d72bbc5e71d..1bf83488365 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 @@ -16,7 +16,7 @@ import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; import org.keycloak.representations.admin.v2.BaseClientRepresentation; import org.keycloak.services.client.ClientService; -import org.keycloak.services.client.ClientServiceHelper; +import org.keycloak.services.client.DefaultClientService; import org.keycloak.services.resources.admin.RealmAdminResource; import org.keycloak.services.resources.admin.fgap.AdminPermissionEvaluator; @@ -38,7 +38,7 @@ public class DefaultClientsApi implements ClientsApi { this.realm = realm; this.permissions = permissions; this.realmAdminResource = realmAdminResource; - this.clientService = ClientServiceHelper.getClientService(session, realm, permissions, realmAdminResource); + this.clientService = new DefaultClientService(session, realm, permissions, realmAdminResource); } @GET diff --git a/rest/admin-v2/services/src/main/java/org/keycloak/services/client/ClientServiceHelper.java b/rest/admin-v2/services/src/main/java/org/keycloak/services/client/ClientServiceHelper.java deleted file mode 100644 index a24fd99795a..00000000000 --- a/rest/admin-v2/services/src/main/java/org/keycloak/services/client/ClientServiceHelper.java +++ /dev/null @@ -1,36 +0,0 @@ -package org.keycloak.services.client; - -import jakarta.annotation.Nonnull; - -import org.keycloak.models.KeycloakSession; -import org.keycloak.models.RealmModel; -import org.keycloak.services.resources.admin.RealmAdminResource; -import org.keycloak.services.resources.admin.fgap.AdminPermissionEvaluator; - -import org.jboss.logging.Logger; - -/** - * Helper method to obtain client service implementation for Admin Client API v2 - *

- * TODO this is only temporary solution to distinguish between legacy and default client service - */ -public class ClientServiceHelper { - private static final Logger log = Logger.getLogger(ClientServiceHelper.class); - - public static boolean isLegacyClientServiceEnabled() { - return Boolean.parseBoolean(System.getProperty("kc.admin-v2.client-service.legacy.enabled", "true")); - } - - public static ClientService getClientService(@Nonnull KeycloakSession session, - @Nonnull RealmModel realm, - @Nonnull AdminPermissionEvaluator permissions, - @Nonnull RealmAdminResource realmAdminResource) { - if (isLegacyClientServiceEnabled()) { - return new DefaultClientService(session, realm, permissions, realmAdminResource); - } else { - log.debug("New ClientService is used"); - return new NewClientService(session, realm, permissions, realmAdminResource); - } - } - -} 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 e14a3454677..497318ff02f 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 @@ -14,17 +14,20 @@ import java.util.stream.Stream; import jakarta.annotation.Nonnull; import jakarta.validation.groups.Default; import jakarta.ws.rs.NotFoundException; -import jakarta.ws.rs.WebApplicationException; import jakarta.ws.rs.core.Response; +import org.keycloak.authorization.fgap.AdminPermissionsSchema; import org.keycloak.events.admin.OperationType; import org.keycloak.events.admin.ResourceType; import org.keycloak.events.admin.v2.AdminEventV2Builder; import org.keycloak.models.ClientModel; import org.keycloak.models.KeycloakSession; +import org.keycloak.models.ModelException; +import org.keycloak.models.ModelValidationException; import org.keycloak.models.RealmModel; import org.keycloak.models.RoleModel; import org.keycloak.models.mapper.ClientModelMapper; +import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.models.utils.ModelToRepresentation; import org.keycloak.representations.admin.v2.BaseClientRepresentation; import org.keycloak.representations.admin.v2.OIDCClientRepresentation; @@ -34,17 +37,22 @@ import org.keycloak.representations.admin.v2.validation.PutClient; import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.RoleRepresentation; import org.keycloak.services.PatchType; +import org.keycloak.services.RolesService; import org.keycloak.services.ServiceException; +import org.keycloak.services.clientpolicy.ClientPolicyException; +import org.keycloak.services.clientpolicy.context.AdminClientRegisterContext; +import org.keycloak.services.clientpolicy.context.AdminClientRegisteredContext; +import org.keycloak.services.clientpolicy.context.AdminClientUnregisterContext; +import org.keycloak.services.clientpolicy.context.AdminClientUpdateContext; +import org.keycloak.services.clientpolicy.context.AdminClientUpdatedContext; +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.ClientResource; -import org.keycloak.services.resources.admin.ClientsResource; 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; -import org.keycloak.utils.StringUtil; import org.keycloak.validation.ValidationUtil; import org.keycloak.validation.jakarta.HibernateValidatorProvider; import org.keycloak.validation.jakarta.JakartaValidatorProvider; @@ -60,21 +68,20 @@ import org.apache.http.HttpEntity; import org.apache.http.util.EntityUtils; import static org.keycloak.representations.admin.v2.validators.ClientSecretNotBlankValidator.isClientSecret; +import static org.keycloak.utils.StringUtil.isBlank; /** - * Legacy implementation of ClientService for Admin API v2 that uses Admin API v1 under hood. + * Default client service for Admin Client API v2 */ public class DefaultClientService implements ClientService { private static final ObjectMapper MAPPER = new ObjectMapperResolver().getContext(null); private final KeycloakSession session; - private final JakartaValidatorProvider validator; private final AdminPermissionEvaluator permissions; - private final AdminEventBuilder adminEventBuilder; - - // v1 resources private final RealmAdminResource realmResource; - private final ClientsResource clientsResource; + private final AdminEventBuilder adminEventBuilder; + private final JakartaValidatorProvider validator; + private final RolesService rolesService; public DefaultClientService(@Nonnull KeycloakSession session, @Nonnull RealmModel realm, @@ -82,31 +89,54 @@ public class DefaultClientService implements ClientService { @Nonnull RealmAdminResource realmResource) { this.session = session; this.permissions = permissions; - this.validator = new HibernateValidatorProvider(new ValidationContext(session, realm)); - this.realmResource = realmResource; - this.clientsResource = realmResource.getClients(); - this.adminEventBuilder = new AdminEventV2Builder(realm, permissions.adminAuth(), session, session.getContext().getConnection()) - .resource(ResourceType.CLIENT); + 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); + } + + protected Optional avoidClientIdPhishing(ClientModel client) throws ServiceException { + if (client == null && !permissions.clients().canList()) { + // we do this to make sure somebody can't phish client IDs + throw new ServiceException(Response.Status.FORBIDDEN); + } + return Optional.ofNullable(client); } @Override - public Optional getClient(RealmModel realm, String clientId, ClientProjectionOptions projectionOptions) throws ServiceException { - // TODO: is the access map on the representation needed - var clientResource = assertAndGetClientResource(realm, clientId); - return Optional.of(clientResource) - .map(ClientResource::viewClientModel) - .map(model -> getMapper(model.getProtocol()).fromModel(model)); + public Optional getClient(@Nonnull RealmModel realm, + @Nonnull String clientId, + ClientProjectionOptions projectionOptions) throws ServiceException { + ClientModel client = avoidClientIdPhishing(realm.getClientByClientId(clientId)).orElseThrow(() -> new ServiceException("Could not find client", Response.Status.NOT_FOUND)); + permissions.clients().requireView(client); + + try { + session.clientPolicy().triggerOnEvent(new AdminClientViewContext(client, permissions.adminAuth())); + return Optional.ofNullable(getMapper(client.getProtocol()).fromModel(client)); + } catch (ClientPolicyException e) { + throw new ServiceException(e.getErrorDetail(), Response.Status.BAD_REQUEST); + } } @Override - 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) - .filter(model -> model.getProtocol() != null) // Skip clients with null protocol - .map(model -> getMapper(model.getProtocol()).fromModel(model)) - .filter(java.util.Objects::nonNull); + public Stream getClients(@Nonnull RealmModel realm, + ClientProjectionOptions projectionOptions, + ClientSearchOptions searchOptions, + ClientSortAndSliceOptions sortAndSliceOptions) { + permissions.clients().requireList(); + + // When FGAP is enabled, authorization filtering is applied at the JPA layer (via PartialEvaluator predicates), so we trust the DB results. + // When disabled, we fall back to in-memory filtering by VIEW_CLIENTS role. + boolean canView = AdminPermissionsSchema.SCHEMA.isAdminPermissionsEnabled(realm) || permissions.clients().canView(); + try { + return realm.getClientsStream() + .filter(client -> canView || permissions.clients().canView(client)) + .filter(client -> client.getProtocol() != null) + .map(client -> getMapper(client.getProtocol()).fromModel(client)) + .filter(java.util.Objects::nonNull); + } catch (ModelException e) { + throw new ServiceException(e.getMessage(), Response.Status.BAD_REQUEST); + } } @Override @@ -119,109 +149,28 @@ public class DefaultClientService implements ClientService { return createOrUpdate(realm, clientId, client, CreateOrUpdateStrategy.PUT); } - protected enum CreateOrUpdateStrategy { - ONLY_CREATE(CreateClient.class), - PUT(PutClient.class), - PATCH(PatchClient.class); + @Override + public void deleteClient(RealmModel realm, String clientId) throws ServiceException { + ClientModel client = avoidClientIdPhishing(realm.getClientByClientId(clientId)) + .orElseThrow(() -> new ServiceException("Could not find client", Response.Status.NOT_FOUND)); - private final Class validationGroup; - - CreateOrUpdateStrategy(Class validationGroup) { - this.validationGroup = validationGroup; + permissions.clients().requireManage(client); + try { + AdminPermissionsSchema.SCHEMA.throwExceptionIfAdminPermissionClient(session, client.getId()); + session.clientPolicy().triggerOnEvent(new AdminClientUnregisterContext(client, permissions.adminAuth())); + } catch (ModelValidationException e) { + throw new ServiceException(e.getMessage(), Response.Status.BAD_REQUEST); + } catch (ClientPolicyException e) { + throw new ServiceException(e.getErrorDetail(), Response.Status.BAD_REQUEST); } - public Class getValidationGroup() { - return validationGroup; - } - } + var clientRepresentation = Optional.ofNullable(getMapper(client.getProtocol()).fromModel(client)) + .orElseThrow(() -> new ServiceException("Cannot map client model", Response.Status.BAD_REQUEST)); - private CreateOrUpdateResult createOrUpdate(RealmModel realm, String clientId, BaseClientRepresentation client, CreateOrUpdateStrategy strategy) throws ServiceException { - validateUnknownFields(client); - if (!strategy.equals(CreateOrUpdateStrategy.ONLY_CREATE)) { - assertSameClientIds(clientId, client.getClientId()); - } - - boolean created = false; - ClientModel model; - ClientModelMapper mapper = getMapper(client.getProtocol()); - - var clientResource = getClientResource(realm, clientId).orElse(null); - if (clientResource != null) { - if (strategy.equals(CreateOrUpdateStrategy.ONLY_CREATE)) { - throw new ServiceException("Client already exists", Response.Status.CONFLICT); - } - validator.validate(client, strategy.getValidationGroup(), Default.class); - - model = mapper.toModel(client, clientResource.viewClientModel()); - var rep = ModelToRepresentation.toRepresentation(model, session); - - try (var response = clientResource.update(rep)) { - // close response and consume payload due to performance reasons - EntityUtils.consumeQuietly((HttpEntity) response.getEntity()); - } + if (new ClientManager(new RealmManager(session)).removeClient(realm, client)) { + fireAdminEvent(OperationType.DELETE, clientRepresentation); } else { - created = true; - validator.validate(client, strategy.getValidationGroup(), Default.class); - - // First, create a basic v1 representation to persist the client in the database. - // We can't use mapper.toModel(client) directly for creation because the "detached model" - var basicRep = new ClientRepresentation(); - basicRep.setClientId(client.getClientId()); - basicRep.setProtocol(client.getProtocol()); - - // TODO: we should avoid 'instanceOf' once we stop using the v1 representation - if (client instanceof OIDCClientRepresentation oidcClient) { - var auth = oidcClient.getAuth(); - if (auth != null && isClientSecret(auth.getMethod())) { - // this makes sure that client secret is generated for "create" methods if necessary - basicRep.setPublicClient(false); - basicRep.setClientAuthenticatorType(auth.getMethod()); - basicRep.setSecret(auth.getSecret()); - } - } - - // Create the client in the database - model = clientsResource.createClientModel(basicRep); - clientResource = clientsResource.getClient(model.getId()); - - // TODO: we should avoid 'instanceOf' once we stop using the v1 representation - if (model.getSecret() != null && client instanceof OIDCClientRepresentation oidcClient) { - // set generated secret - oidcClient.getAuth().setSecret(model.getSecret()); - } - - mapper.toModel(client, model); - - // Validate the fully populated model (createClientModel only validates the basic model) - ValidationUtil.validateClient(session, model, true, r -> { - session.getTransactionManager().setRollbackOnly(); - throw new ServiceException(r.getAllErrorsAsString(), Response.Status.BAD_REQUEST); - }); - } - - handleRoles(clientResource.getRoleContainerResource(), client.getRoles()); - if (client instanceof OIDCClientRepresentation oidcClient) { - handleServiceAccount(clientResource.getRoleContainerResource(), realmResource.getRoleContainerResource(), model, oidcClient); - } - - fireAdminEvent(created ? OperationType.CREATE : OperationType.UPDATE, mapper.fromModel(model)); - - return new CreateOrUpdateResult(mapper.fromModel(model), created); - } - - /** - * Fires a v2 admin event for client operations (only enabled for testing now to avoid duplicated admin events) - * - * @param operationType the type of operation (CREATE, UPDATE, DELETE) - * @param representation the v2 representation of the client - */ - protected void fireAdminEvent(OperationType operationType, BaseClientRepresentation representation) { - if (Boolean.parseBoolean(System.getProperty("kc.admin-v2.client-service.events.enabled","false"))) { - adminEventBuilder - .operation(operationType) - .resourcePath(session.getContext().getUri()) - .representation(representation) - .success(); + throw new ServiceException("Could not delete client", Response.Status.BAD_REQUEST); } } @@ -264,15 +213,140 @@ public class DefaultClientService implements ClientService { return null; } - @Override - public void deleteClient(RealmModel realm, String clientId) throws ServiceException { - var clientResource = assertAndGetClientResource(realm, clientId); - var client = Optional.of(clientResource.viewClientModel()) - .map(c -> getMapper(c.getProtocol()).fromModel(c)) - .orElseThrow(() -> new ServiceException("Cannot map client model", Response.Status.BAD_REQUEST)); + protected enum CreateOrUpdateStrategy { + ONLY_CREATE(CreateClient.class), + PUT(PutClient.class), + PATCH(PatchClient.class); - clientResource.deleteClient(); - fireAdminEvent(OperationType.DELETE, client); + private final Class validationGroup; + + CreateOrUpdateStrategy(Class validationGroup) { + this.validationGroup = validationGroup; + } + + public Class getValidationGroup() { + return validationGroup; + } + } + + private CreateOrUpdateResult createOrUpdate(RealmModel realm, String clientId, BaseClientRepresentation client, CreateOrUpdateStrategy strategy) throws ServiceException { + validateUnknownFields(client); + ClientModel model = null; + if (!strategy.equals(CreateOrUpdateStrategy.ONLY_CREATE)) { + assertSameClientIds(clientId, client.getClientId()); + model = avoidClientIdPhishing(realm.getClientByClientId(clientId)).orElse(null); + } + boolean alreadyExists = model != null; + ClientModelMapper mapper = getMapper(client.getProtocol()); + + try { + if (alreadyExists) { + switch (strategy) { + case ONLY_CREATE -> throw new ServiceException("Client already exists", Response.Status.CONFLICT); + case PUT, PATCH -> { + // Check permissions, execute validations and trigger client policies + permissions.clients().requireConfigure(model); + 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 + model = mapper.toModel(client, model); + + // Validate the fully populated model + ValidationUtil.validateClient(session, model, false, r -> { + session.getTransactionManager().setRollbackOnly(); + throw new ServiceException(r.getAllErrorsAsString(), Response.Status.BAD_REQUEST); + }); + + session.clientPolicy().triggerOnEvent(new AdminClientUpdatedContext(proposedRepresentation, model, permissions.adminAuth())); + } + } + } else { + // Check permissions, execute validations and trigger client policies + permissions.clients().requireManage(); + validator.validate(client, strategy.getValidationGroup(), Default.class); + var proposedRepresentation = getProposedOldRepresentation(realm, client, mapper); + session.clientPolicy().triggerOnEvent(new AdminClientRegisterContext(proposedRepresentation, permissions.adminAuth())); + + // Add basic attributes + model = realm.addClient(clientId); + model.setProtocol(client.getProtocol()); + + // Generate random secret if applicable + generateClientSecretIfNeeded(client, model); + mapper.toModel(client, model); + + // Validate the fully populated model + ValidationUtil.validateClient(session, model, true, r -> { + session.getTransactionManager().setRollbackOnly(); + throw new ServiceException(r.getAllErrorsAsString(), Response.Status.BAD_REQUEST); + }); + session.clientPolicy().triggerOnEvent(new AdminClientRegisteredContext(model, permissions.adminAuth())); + } + } catch (ClientPolicyException e) { + throw new ServiceException(e.getErrorDetail(), Response.Status.BAD_REQUEST); + } + + // Setup roles + var clientRoles = rolesService.resource(model); + handleRoles(clientRoles, client.getRoles()); + + // OIDC specific + if (client instanceof OIDCClientRepresentation oidcClient) { + handleServiceAccount(clientRoles, rolesService.resource(realm), model, oidcClient); + } + + fireAdminEvent(alreadyExists ? OperationType.UPDATE : OperationType.CREATE, mapper.fromModel(model)); + return new CreateOrUpdateResult(mapper.fromModel(model), !alreadyExists); + } + + /** + * Fires a v2 admin event for client operations (only enabled for testing now to avoid duplicated admin events) + * + * @param operationType the type of operation (CREATE, UPDATE, DELETE) + * @param representation the v2 representation of the client + */ + protected void fireAdminEvent(OperationType operationType, BaseClientRepresentation representation) { + if (Boolean.parseBoolean(System.getProperty("kc.admin-v2.client-service.events.enabled", "false"))) { + adminEventBuilder + .operation(operationType) + .resourcePath(session.getContext().getUri()) + .representation(representation) + .success(); + } + } + + /** + * Creates a temporary client to convert BaseClientRepresentation to ClientRepresentation. + * Required because client policy contexts expect ClientRepresentation (v1), but there's no + * direct converter from BaseClientRepresentation (v2 API). The temp client is immediately removed. + *

+ * For more details, see the keycloak#47576. + */ + private ClientRepresentation getProposedOldRepresentation(RealmModel realm, BaseClientRepresentation client, ClientModelMapper mapper) { + String tempId = "__temp__" + client.getClientId() + "__" + System.nanoTime(); + ClientModel tempModel = mapper.toModel(client, realm.addClient(tempId)); + try { + var proposedRepresentation = ModelToRepresentation.toRepresentation(tempModel, session); + proposedRepresentation.setClientId(client.getClientId()); + return proposedRepresentation; + } finally { + realm.removeClient(tempModel.getId()); + } + } + + // TODO we should find a way on how to evoke it on the mapper level? + private void generateClientSecretIfNeeded(BaseClientRepresentation client, ClientModel model) { + 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)); + } + } } protected void assertSameClientIds(String pathId, String payloadId) { @@ -379,25 +453,8 @@ public class DefaultClientService implements ClientService { } } - private ClientResource assertAndGetClientResource(RealmModel realm, String clientId) { - var client = Optional.ofNullable(session.clients().getClientByClientId(realm, clientId)); - // handles the phishing check - return clientsResource.getClient(client.map(ClientModel::getId).orElse("")); - } - protected ClientModelMapper getMapper(String protocol) { return Optional.ofNullable(session.getProvider(ClientModelMapper.class, protocol)) .orElseThrow(() -> new ServiceException("Mapper not found, unsupported client protocol: " + protocol, Response.Status.BAD_REQUEST)); } - - private Optional getClientResource(RealmModel realm, String clientId) { - if (StringUtil.isBlank(clientId)) { - return Optional.empty(); - } - try { - return Optional.of(assertAndGetClientResource(realm, clientId)); - } catch (WebApplicationException e) { - return Optional.empty(); - } - } } diff --git a/rest/admin-v2/tests/pom.xml b/rest/admin-v2/tests/pom.xml index e7f427f12a6..9968e731f05 100644 --- a/rest/admin-v2/tests/pom.xml +++ b/rest/admin-v2/tests/pom.xml @@ -25,6 +25,10 @@ + + org.keycloak + keycloak-admin-v2-services + org.keycloak.testframework keycloak-test-framework-core diff --git a/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/AbstractClientApiV2Test.java b/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/AbstractClientApiV2Test.java index 0bac3f3b985..b774cd3586b 100644 --- a/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/AbstractClientApiV2Test.java +++ b/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/AbstractClientApiV2Test.java @@ -3,16 +3,11 @@ package org.keycloak.tests.admin.client.v2; import jakarta.ws.rs.core.HttpHeaders; import org.keycloak.admin.client.Keycloak; -import org.keycloak.services.client.ClientServiceHelper; import org.keycloak.testframework.annotations.InjectAdminClient; -import org.keycloak.testframework.remote.runonserver.InjectRunOnServer; -import org.keycloak.testframework.remote.runonserver.RunOnServerClient; import com.fasterxml.jackson.databind.ObjectMapper; import org.apache.http.HttpMessage; -import org.jboss.logging.Logger; import org.junit.jupiter.api.BeforeAll; -import org.junit.jupiter.api.BeforeEach; public abstract class AbstractClientApiV2Test { protected static ObjectMapper mapper; @@ -34,24 +29,8 @@ public abstract class AbstractClientApiV2Test { @BeforeAll public static void setupMapper() { mapper = new ObjectMapper(); - if (!ClientServiceHelper.isLegacyClientServiceEnabled()) { - Logger.getLogger(AbstractClientApiV2Test.class).infof("New Client service is used"); - } } - // BEGIN remove once we drop support for legacy ClientService that uses Client API v1 under hood - @InjectRunOnServer - RunOnServerClient runOnServer; - - @BeforeEach - public void setupNewClientService() { - var isLegacyEnabled = ClientServiceHelper.isLegacyClientServiceEnabled(); - if (!isLegacyEnabled) { - runOnServer.run(session -> System.setProperty("kc.admin-v2.client-service.legacy.enabled", "false")); - } - } - // END - protected void setAuthHeader(HttpMessage request) { String token = adminClient.tokenManager().getAccessTokenString(); request.setHeader(HttpHeaders.AUTHORIZATION, "Bearer " + token); 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 683506b8c87..5118b208042 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 @@ -22,7 +22,6 @@ import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.authorization.Logic; import org.keycloak.representations.idm.authorization.ScopePermissionRepresentation; import org.keycloak.representations.idm.authorization.UserPolicyRepresentation; -import org.keycloak.services.client.ClientServiceHelper; import org.keycloak.testframework.admin.AdminClientFactory; import org.keycloak.testframework.annotations.InjectAdminClientFactory; import org.keycloak.testframework.annotations.InjectClient; @@ -447,11 +446,7 @@ public class ClientApiV2AuthorizationTest extends AbstractClientApiV2Test { try (var response = client.execute(request)) { assertThat(response.getStatusLine().getStatusCode(), is(400)); var body = EntityUtils.toString(response.getEntity()); - if (ClientServiceHelper.isLegacyClientServiceEnabled()) { - assertThat(body, containsString("unknown_error")); - } else { - assertThat(body, containsString("Not supported for this client")); - } + assertThat(body, containsString("Not supported for this client")); } // Verify the client still exists (was not deleted) diff --git a/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/ClientPoliciesV2Test.java b/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/ClientPoliciesV2Test.java index 68e2711118f..895eb5365a8 100644 --- a/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/ClientPoliciesV2Test.java +++ b/rest/admin-v2/tests/src/test/java/org/keycloak/tests/admin/client/v2/ClientPoliciesV2Test.java @@ -41,7 +41,6 @@ import org.keycloak.representations.idm.ClientPolicyExecutorRepresentation; import org.keycloak.representations.idm.ClientPolicyRepresentation; import org.keycloak.representations.idm.ClientProfileRepresentation; import org.keycloak.representations.idm.ClientProfilesRepresentation; -import org.keycloak.services.client.ClientServiceHelper; import org.keycloak.services.clientpolicy.ClientPolicyEvent; import org.keycloak.services.clientpolicy.condition.AnyClientConditionFactory; import org.keycloak.services.clientpolicy.condition.ClientUpdaterContextConditionFactory; @@ -152,12 +151,7 @@ public class ClientPoliciesV2Test extends AbstractClientApiV2Test { // Should fail with 400 Bad Request due to policy violation assertEquals(400, response.getStatusLine().getStatusCode()); String body = EntityUtils.toString(response.getEntity()); - if (ClientServiceHelper.isLegacyClientServiceEnabled()) { - assertThat(body, containsString("invalid_client_metadata")); - } else { - // TODO might be more consistent in error messages - assertThat(body, containsString("Invalid client metadata: token_endpoint_auth_method")); - } + assertThat(body, containsString("Invalid client metadata: token_endpoint_auth_method")); } } @@ -280,12 +274,7 @@ public class ClientPoliciesV2Test extends AbstractClientApiV2Test { // Should fail with 400 Bad Request due to policy violation assertEquals(400, response.getStatusLine().getStatusCode()); String body = EntityUtils.toString(response.getEntity()); - if (ClientServiceHelper.isLegacyClientServiceEnabled()) { - assertThat(body, containsString("invalid_client_metadata")); - } else { - // TODO might be more consistent in error messages - assertThat(body, containsString("Invalid client metadata: token_endpoint_auth_method")); - } + assertThat(body, containsString("Invalid client metadata: token_endpoint_auth_method")); } // Verify the client was NOT updated (transaction rollback worked) @@ -388,7 +377,7 @@ public class ClientPoliciesV2Test extends AbstractClientApiV2Test { // Should fail with 400 Bad Request due to policy violation assertEquals(400, response.getStatusLine().getStatusCode()); String body = EntityUtils.toString(response.getEntity()); - assertThat(body, containsString("invalid_client_metadata")); + assertThat(body, containsString("Invalid client metadata: token_endpoint_auth_method")); } // Verify the client was NOT updated (transaction rollback worked) @@ -528,12 +517,7 @@ public class ClientPoliciesV2Test extends AbstractClientApiV2Test { EntityUtils.consumeQuietly(response.getEntity()); } - // for now, the VIEW is also present, but it is not required for update - if (ClientServiceHelper.isLegacyClientServiceEnabled()) { - assertClientPolicyEventIsEmitted(ClientPolicyEvent.VIEW, ClientPolicyEvent.UPDATE, ClientPolicyEvent.UPDATED); - } else { - assertClientPolicyEventIsEmitted(ClientPolicyEvent.UPDATE, ClientPolicyEvent.UPDATED); - } + assertClientPolicyEventIsEmitted(ClientPolicyEvent.UPDATE, ClientPolicyEvent.UPDATED); } /** @@ -558,12 +542,7 @@ public class ClientPoliciesV2Test extends AbstractClientApiV2Test { setupAlwaysAppliedTestPolicy(); cleanupClient(rep.getClientId()); - if (ClientServiceHelper.isLegacyClientServiceEnabled()) { - // for now, the VIEW is also present, but it is not required for delete - assertClientPolicyEventIsEmitted(ClientPolicyEvent.VIEW, ClientPolicyEvent.UNREGISTER); - } else { - assertClientPolicyEventIsEmitted(ClientPolicyEvent.UNREGISTER); - } + assertClientPolicyEventIsEmitted(ClientPolicyEvent.UNREGISTER); } private void assertClientPolicyEventIsEmitted(ClientPolicyEvent... events) {