From 03db2e8b56a04dd5e8d8e23a307b16ee23036f4a Mon Sep 17 00:00:00 2001 From: Patrick Jennings Date: Tue, 2 Apr 2024 14:48:51 -0400 Subject: [PATCH] Integration tests around client type parameter validation. Throw common ClientTypeException with invalid params requested during client creation/update requests. This gets translated into ErrorResponseException in the Resource handlers. Signed-off-by: Patrick Jennings --- .../clienttype/impl/DefaultClientType.java | 33 ++++++++----------- .../resources/admin/ClientResource.java | 2 +- .../resources/admin/ClientsResource.java | 2 +- .../testsuite/client/ClientTypesTest.java | 19 ++++++++++- 4 files changed, 33 insertions(+), 23 deletions(-) diff --git a/services/src/main/java/org/keycloak/services/clienttype/impl/DefaultClientType.java b/services/src/main/java/org/keycloak/services/clienttype/impl/DefaultClientType.java index c6e25a9456..a63f803f04 100644 --- a/services/src/main/java/org/keycloak/services/clienttype/impl/DefaultClientType.java +++ b/services/src/main/java/org/keycloak/services/clienttype/impl/DefaultClientType.java @@ -18,29 +18,23 @@ package org.keycloak.services.clienttype.impl; -import java.beans.PropertyDescriptor; -import java.lang.reflect.InvocationTargetException; -import java.lang.reflect.Method; -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import java.util.Objects; -import java.util.Optional; -import java.util.stream.Collectors; -import java.util.stream.Stream; - import jakarta.ws.rs.core.Response; import org.jboss.logging.Logger; -import org.keycloak.common.util.ObjectUtil; +import org.keycloak.client.clienttype.ClientType; +import org.keycloak.client.clienttype.ClientTypeException; import org.keycloak.models.ClientModel; import org.keycloak.models.KeycloakSession; import org.keycloak.models.utils.ModelToRepresentation; import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.ClientTypeRepresentation; -import org.keycloak.client.clienttype.ClientType; -import org.keycloak.client.clienttype.ClientTypeException; -import org.keycloak.representations.idm.ErrorRepresentation; -import org.keycloak.services.ErrorResponse; + +import java.beans.PropertyDescriptor; +import java.lang.reflect.Method; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.stream.Collectors; /** * @author Marek Posolda @@ -104,17 +98,16 @@ public class DefaultClientType implements ClientType { validateClientRequest(newClient, currentRep); } - protected void validateClientRequest(ClientRepresentation newClient, ClientRepresentation currentClient) { + protected void validateClientRequest(ClientRepresentation newClient, ClientRepresentation currentClient) throws ClientTypeException { List validationErrors = clientType.getConfig().entrySet().stream() .filter(property -> clientPropertyHasInvalidChangeRequested(currentClient, newClient, property.getKey(), property.getValue())) .map(Map.Entry::getKey) .collect(Collectors.toList()); if (validationErrors.size() > 0) { - throw ErrorResponse.error( + throw new ClientTypeException( "Cannot change property of client as it is not allowed by the specified client type.", - validationErrors.toArray(), - Response.Status.BAD_REQUEST); + validationErrors.toArray()); } } 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 3662e9246e..ff70e8db47 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 @@ -186,7 +186,7 @@ public class ClientResource { } catch (ModelDuplicateException e) { throw ErrorResponse.exists("Client already exists"); } catch (ClientTypeException cte) { - throw ErrorResponse.error(cte.getMessage(), Response.Status.BAD_REQUEST); + throw ErrorResponse.error(cte.getMessage(), cte.getParameters(), Response.Status.BAD_REQUEST); } catch (ClientPolicyException cpe) { throw new ErrorResponseException(cpe.getError(), cpe.getErrorDetail(), Response.Status.BAD_REQUEST); } diff --git a/services/src/main/java/org/keycloak/services/resources/admin/ClientsResource.java b/services/src/main/java/org/keycloak/services/resources/admin/ClientsResource.java index 5ea265e52c..aa665bb2ff 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/ClientsResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/ClientsResource.java @@ -229,7 +229,7 @@ public class ClientsResource { throw new ErrorResponseException(cpe.getError(), cpe.getErrorDetail(), Response.Status.BAD_REQUEST); } catch (ClientTypeException cte) { - throw ErrorResponse.error(cte.getMessage(), Response.Status.BAD_REQUEST); + throw ErrorResponse.error(cte.getMessage(), cte.getParameters(), Response.Status.BAD_REQUEST); } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientTypesTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientTypesTest.java index f2f56a90fb..cd506c55ae 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientTypesTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientTypesTest.java @@ -28,6 +28,7 @@ import org.keycloak.protocol.oidc.OIDCLoginProtocol; import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.ClientTypeRepresentation; import org.keycloak.representations.idm.ClientTypesRepresentation; +import org.keycloak.representations.idm.ErrorRepresentation; import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.services.clienttype.impl.DefaultClientTypeProviderFactory; import org.keycloak.testsuite.AbstractTestRealmKeycloakTest; @@ -44,6 +45,7 @@ import java.util.List; import java.util.stream.Collectors; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsInAnyOrder; import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; import static org.keycloak.common.Profile.Feature.CLIENT_TYPES; @@ -122,7 +124,6 @@ public class ClientTypesTest extends AbstractTestRealmKeycloakTest { // Expected } - clientRep.setServiceAccountsEnabled(true); // Adding non-applicable attribute should not fail @@ -134,6 +135,22 @@ public class ClientTypesTest extends AbstractTestRealmKeycloakTest { testRealm().clients().get(clientRep.getId()).update(clientRep); } + @Test + public void testCreateClientFailsWithMultipleInvalidClientTypeOverrides() { + ClientRepresentation clientRep = ClientBuilder.create() + .clientId("invalid-client-type-fields-set") + .type(ClientTypeManager.SERVICE_ACCOUNT) + .serviceAccountsEnabled(false) + .publicClient() + .build(); + + Response response = testRealm().clients().create(clientRep); + assertEquals(Response.Status.BAD_REQUEST, response.getStatusInfo()); + ErrorRepresentation errorRepresentation = response.readEntity(ErrorRepresentation.class); + assertThat( + List.of(errorRepresentation.getParams()), + containsInAnyOrder("publicClient", "serviceAccountsEnabled")); + } @Test public void testClientTypesAdminRestAPI_globalTypes() {