From 5144f8d85f667b1a9dc195e7db5a4260ff928f6f Mon Sep 17 00:00:00 2001 From: Patrick Jennings Date: Fri, 31 May 2024 03:53:22 -0400 Subject: [PATCH] Improve Client Type Integration Tests (#29944) closes #30017 Signed-off-by: Patrick Jennings --- .../clienttype/ClientTypeException.java | 41 ++++++++++++--- .../models/utils/RepresentationToModel.java | 8 ++- .../clienttype/DefaultClientTypeManager.java | 30 +++++------ .../DefaultClientTypeManagerFactory.java | 4 +- .../client/TypedClientAttribute.java | 6 +-- .../impl/DefaultClientTypeProvider.java | 4 +- .../testsuite/client/ClientTypesTest.java | 50 +++++++++++-------- 7 files changed, 90 insertions(+), 53 deletions(-) diff --git a/server-spi-private/src/main/java/org/keycloak/client/clienttype/ClientTypeException.java b/server-spi-private/src/main/java/org/keycloak/client/clienttype/ClientTypeException.java index 2dd3b36045..e03a518d78 100644 --- a/server-spi-private/src/main/java/org/keycloak/client/clienttype/ClientTypeException.java +++ b/server-spi-private/src/main/java/org/keycloak/client/clienttype/ClientTypeException.java @@ -25,15 +25,44 @@ import org.keycloak.models.ModelException; */ public class ClientTypeException extends ModelException { - public ClientTypeException(String message) { - super(message); - } - - public ClientTypeException(String message, Object ... parameters) { + private ClientTypeException(String message, Object... parameters) { super(message, parameters); } - public ClientTypeException(String message, Throwable cause) { + private ClientTypeException(String message, Throwable cause) { super(message, cause); } + + public enum Message { + /** + * Register all client type exception messages through this enum to keep things consistent across the services. + */ + INVALID_CLIENT_TYPE("Invalid client type"), + CANNOT_CHANGE_CLIENT_TYPE("Not supported to change client type"), + INVALID_CLIENT_TYPE_PROVIDER("Did not find client type provider"), + CLIENT_TYPE_FIELD_NOT_APPLICABLE("Invalid configuration of 'applicable' property on client type"), + INVALID_CLIENT_TYPE_CONFIGURATION("Invalid configuration of property on client type"), + DUPLICATE_CLIENT_TYPE("Duplicated client type name"), + CLIENT_UPDATE_FAILED_CLIENT_TYPE_VALIDATION("Cannot change property of client as it is not allowed by the specified client type."), + CLIENT_TYPE_NOT_FOUND("Client type not found."), + CLIENT_TYPE_FAILED_TO_LOAD("Failed to load client type."); + + private final String message; + + Message(String message) { + this.message = message; + } + + public ClientTypeException exception(Object... parameters) { + return new ClientTypeException(message, parameters); + } + + public ClientTypeException exception(String message, Throwable cause) { + return new ClientTypeException(message, cause); + } + + public String getMessage() { + return message; + } + } } diff --git a/server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java b/server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java index 522afb3051..e036f17154 100755 --- a/server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java +++ b/server-spi-private/src/main/java/org/keycloak/models/utils/RepresentationToModel.java @@ -415,7 +415,7 @@ public class RepresentationToModel { if (Profile.isFeatureEnabled(Profile.Feature.CLIENT_TYPES)) { if (!ObjectUtil.isEqualOrBothNull(resource.getType(), rep.getType())) { - throw new ClientTypeException("Not supported to change client type"); + throw ClientTypeException.Message.CANNOT_CHANGE_CLIENT_TYPE.exception(); } if (rep.getType() != null) { RealmModel realm = session.getContext().getRealm(); @@ -546,9 +546,8 @@ public class RepresentationToModel { .collect(Collectors.toList()); if (propertyUpdateExceptions.size() > 0) { - throw new ClientTypeException( - "Cannot change property of client as it is not allowed by the specified client type.", - propertyUpdateExceptions.stream().map(ClientTypeException::getParameters).flatMap(Stream::of).toArray()); + Object[] paramsWithFailures = propertyUpdateExceptions.stream().map(ClientTypeException::getParameters).flatMap(Stream::of).toArray(); + throw ClientTypeException.Message.CLIENT_UPDATE_FAILED_CLIENT_TYPE_VALIDATION.exception(paramsWithFailures); } } @@ -700,7 +699,6 @@ public class RepresentationToModel { return updateProperty(modelSetter, () -> firstNonNullSupplied.findFirst().orElse(null)); } - private static String generateProtocolNameKey(String protocol, String name) { return String.format("%s%%%s", protocol, name); } diff --git a/services/src/main/java/org/keycloak/services/clienttype/DefaultClientTypeManager.java b/services/src/main/java/org/keycloak/services/clienttype/DefaultClientTypeManager.java index b49b906c1c..ea406545b7 100644 --- a/services/src/main/java/org/keycloak/services/clienttype/DefaultClientTypeManager.java +++ b/services/src/main/java/org/keycloak/services/clienttype/DefaultClientTypeManager.java @@ -69,7 +69,8 @@ public class DefaultClientTypeManager implements ClientTypeManager { result = JsonSerialization.readValue(asStr, ClientTypesRepresentation.class); result.setGlobalClientTypes(globalClientTypes); } catch (IOException ioe) { - throw new ClientTypeException("Failed to deserialize client types from JSON string", ioe); + logger.errorf("Failed to load client type for realm '%s'.", realm.getName()); + throw ClientTypeException.Message.CLIENT_TYPE_FAILED_TO_LOAD.exception(ioe); } } return result; @@ -86,7 +87,8 @@ public class DefaultClientTypeManager implements ClientTypeManager { String asStr = JsonSerialization.writeValueAsString(noGlobalsCopy); realm.setAttribute(CLIENT_TYPE_REALM_ATTRIBUTE, asStr); } catch (IOException ioe) { - throw new ClientTypeException("Failed to serialize client types to String", ioe); + logger.errorf("Failed to load global client type."); + throw ClientTypeException.Message.CLIENT_TYPE_FAILED_TO_LOAD.exception(ioe); } } @@ -97,7 +99,7 @@ public class DefaultClientTypeManager implements ClientTypeManager { ClientTypeRepresentation clientType = getClientTypeByName(clientTypes, typeName); if (clientType == null) { logger.errorf("Referenced client type '%s' not found", typeName); - throw new ClientTypeException("Client type not found"); + throw ClientTypeException.Message.CLIENT_TYPE_NOT_FOUND.exception(); } ClientTypeProvider provider = session.getProvider(ClientTypeProvider.class, clientType.getProvider()); @@ -108,15 +110,15 @@ public class DefaultClientTypeManager implements ClientTypeManager { public ClientModel augmentClient(ClientModel client) throws ClientTypeException { if (client.getType() == null) { return client; - } else { - try { - ClientType clientType = getClientType(client.getRealm(), client.getType()); - return new TypeAwareClientModelDelegate(clientType, () -> client); - } catch(ClientTypeException cte) { - logger.errorf("Could not augment client, %s, due to client type exception: %s", - client, cte); - throw cte; - } + } + + try { + ClientType clientType = getClientType(client.getRealm(), client.getType()); + return new TypeAwareClientModelDelegate(clientType, () -> client); + } catch(ClientTypeException cte) { + logger.errorf("Could not augment client, %s, due to client type exception: %s", + client, cte); + throw cte; } } @@ -136,13 +138,13 @@ public class DefaultClientTypeManager implements ClientTypeManager { ClientTypeProvider clientTypeProvider = session.getProvider(ClientTypeProvider.class, clientType.getProvider()); if (clientTypeProvider == null) { logger.errorf("Did not find client type provider '%s' for the client type '%s'", clientType.getProvider(), clientType.getName()); - throw new ClientTypeException("Did not find client type provider"); + throw ClientTypeException.Message.INVALID_CLIENT_TYPE_PROVIDER.exception(); } // Validate name is not duplicated if (currentNames.contains(clientType.getName())) { logger.errorf("Duplicated client type name '%s'", clientType.getName()); - throw new ClientTypeException("Duplicated client type name"); + throw ClientTypeException.Message.DUPLICATE_CLIENT_TYPE.exception(); } clientType = clientTypeProvider.checkClientTypeConfig(clientType); diff --git a/services/src/main/java/org/keycloak/services/clienttype/DefaultClientTypeManagerFactory.java b/services/src/main/java/org/keycloak/services/clienttype/DefaultClientTypeManagerFactory.java index c9e267d651..491d2cdddc 100644 --- a/services/src/main/java/org/keycloak/services/clienttype/DefaultClientTypeManagerFactory.java +++ b/services/src/main/java/org/keycloak/services/clienttype/DefaultClientTypeManagerFactory.java @@ -24,6 +24,7 @@ import java.util.List; import org.jboss.logging.Logger; import org.keycloak.Config; +import org.keycloak.client.clienttype.ClientTypeException; import org.keycloak.client.clienttype.ClientTypeManager; import org.keycloak.client.clienttype.ClientTypeManagerFactory; import org.keycloak.common.Profile; @@ -82,7 +83,8 @@ public class DefaultClientTypeManagerFactory implements ClientTypeManagerFactory ClientTypesRepresentation globalTypesRep = JsonSerialization.readValue(getClass().getResourceAsStream("/keycloak-default-client-types.json"), ClientTypesRepresentation.class); this.globalClientTypes = DefaultClientTypeManager.validateAndCastConfiguration(session, globalTypesRep.getRealmClientTypes(), Collections.emptyList()); } catch (IOException e) { - throw new IllegalStateException("Failed to deserialize global proposed client types from JSON.", e); + logger.error("Failed to deserialize global proposed client types from JSON."); + throw ClientTypeException.Message.CLIENT_TYPE_FAILED_TO_LOAD.exception(e); } } } diff --git a/services/src/main/java/org/keycloak/services/clienttype/client/TypedClientAttribute.java b/services/src/main/java/org/keycloak/services/clienttype/client/TypedClientAttribute.java index 72db504ad7..bfe1ea71b2 100644 --- a/services/src/main/java/org/keycloak/services/clienttype/client/TypedClientAttribute.java +++ b/services/src/main/java/org/keycloak/services/clienttype/client/TypedClientAttribute.java @@ -118,9 +118,7 @@ interface TypedClientAttribute { // If there is an attempt to change a value for an applicable field with a read-only value set, then throw an exception. T oldVal = clientType.getTypeValue(propertyName, tClass); if (!ObjectUtil.isEqualOrBothNull(oldVal, newValue)) { - throw new ClientTypeException( - "Property " + propertyName + " is read-only due to client type " + clientType.getName(), - propertyName); + throw ClientTypeException.Message.CLIENT_UPDATE_FAILED_CLIENT_TYPE_VALIDATION.exception(propertyName); } // Delegate to clientSetter @@ -129,4 +127,4 @@ interface TypedClientAttribute { String getPropertyName(); Object getNonApplicableValue(); -} \ No newline at end of file +} diff --git a/services/src/main/java/org/keycloak/services/clienttype/impl/DefaultClientTypeProvider.java b/services/src/main/java/org/keycloak/services/clienttype/impl/DefaultClientTypeProvider.java index adb1f83fd6..8d1f1257f0 100644 --- a/services/src/main/java/org/keycloak/services/clienttype/impl/DefaultClientTypeProvider.java +++ b/services/src/main/java/org/keycloak/services/clienttype/impl/DefaultClientTypeProvider.java @@ -47,13 +47,13 @@ public class DefaultClientTypeProvider implements ClientTypeProvider { if (propConfig.getApplicable() == null) { logger.errorf("Property '%s' does not have 'applicable' configured for client type '%s'", propertyName, clientType.getName()); - throw new ClientTypeException("Invalid configuration of 'applicable' property on client type"); + throw ClientTypeException.Message.CLIENT_TYPE_FIELD_NOT_APPLICABLE.exception(); } // Not supported to set value for properties, which are not applicable for the particular client if (!propConfig.getApplicable() && propConfig.getValue() != null) { logger.errorf("Property '%s' is not applicable and so should not have read-only or default-value set for client type '%s'", propertyName, clientType.getName()); - throw new ClientTypeException("Invalid configuration of property on client type"); + throw ClientTypeException.Message.INVALID_CLIENT_TYPE_CONFIGURATION.exception(); } } 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 9572063f55..6bc57cc048 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 @@ -21,6 +21,7 @@ package org.keycloak.testsuite.client; import jakarta.ws.rs.BadRequestException; import jakarta.ws.rs.core.Response; import org.junit.Test; +import org.keycloak.client.clienttype.ClientTypeException; import org.keycloak.client.clienttype.ClientTypeManager; import org.keycloak.models.ClientModel; import org.keycloak.protocol.oidc.OIDCLoginProtocol; @@ -38,7 +39,7 @@ import org.keycloak.testsuite.arquillian.annotation.EnableFeature; import org.keycloak.testsuite.arquillian.annotation.UncaughtServerErrorExpected; import org.keycloak.testsuite.util.ClientBuilder; -import java.util.Arrays; +import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.stream.Collectors; @@ -48,6 +49,8 @@ import static org.hamcrest.Matchers.everyItem; import static org.hamcrest.Matchers.hasItems; import static org.hamcrest.Matchers.in; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.junit.Assert.fail; import static org.keycloak.common.Profile.Feature.CLIENT_TYPES; @@ -112,7 +115,7 @@ public class ClientTypesTest extends AbstractTestRealmKeycloakTest { testRealm().clients().get(clientRep.getId()).update(clientRep); Assert.fail("Not expected to update client"); } catch (BadRequestException bre) { - // Expected + assertErrorContainsMessage(bre, ClientTypeException.Message.CANNOT_CHANGE_CLIENT_TYPE); } // Updating read-only attribute should fail @@ -130,7 +133,7 @@ public class ClientTypesTest extends AbstractTestRealmKeycloakTest { // Adding non-applicable attribute should not fail but not update client attribute clientRep.getAttributes().put(ClientModel.LOGO_URI, "https://foo"); testRealm().clients().get(clientRep.getId()).update(clientRep); - assertEquals(testRealm().clients().get(clientRep.getId()).toRepresentation().getAttributes().get(ClientModel.LOGO_URI), null); + assertNull(testRealm().clients().get(clientRep.getId()).toRepresentation().getAttributes().get(ClientModel.LOGO_URI)); // Update of supported attribute should be successful clientRep.getAttributes().remove(ClientModel.LOGO_URI); @@ -151,22 +154,13 @@ public class ClientTypesTest extends AbstractTestRealmKeycloakTest { assertErrorResponseContainsParams(response, "publicClient", "serviceAccountsEnabled"); } - private void assertErrorResponseContainsParams(Response response, String... items) { - assertEquals(Response.Status.BAD_REQUEST, response.getStatusInfo()); - ErrorRepresentation errorRepresentation = response.readEntity(ErrorRepresentation.class); - assertThat( - List.of(items), - everyItem(in(errorRepresentation.getParams()))); - } - @Test public void testClientTypesAdminRestAPI_globalTypes() { ClientTypesRepresentation clientTypes = testRealm().clientTypes().getClientTypes(); assertEquals(0, clientTypes.getRealmClientTypes().size()); - List globalClientTypeNames = clientTypes.getGlobalClientTypes().stream() - .collect(Collectors.toList()); + List globalClientTypeNames = new ArrayList<>(clientTypes.getGlobalClientTypes()); assertNames(globalClientTypeNames, "sla", "service-account"); ClientTypeRepresentation serviceAccountType = clientTypes.getGlobalClientTypes().stream() @@ -194,12 +188,12 @@ public class ClientTypesTest extends AbstractTestRealmKeycloakTest { try { clientType.setName("sla1"); clientType.setProvider("non-existent"); - clientType.setConfig(new HashMap()); - clientTypes.setRealmClientTypes(Arrays.asList(clientType)); + clientType.setConfig(new HashMap<>()); + clientTypes.setRealmClientTypes(List.of(clientType)); testRealm().clientTypes().updateClientTypes(clientTypes); Assert.fail("Not expected to update client types"); } catch (BadRequestException bre) { - // Expected + assertErrorContainsMessage(bre, ClientTypeException.Message.INVALID_CLIENT_TYPE_PROVIDER); } // Test attribute without applicable should fail @@ -210,7 +204,7 @@ public class ClientTypesTest extends AbstractTestRealmKeycloakTest { testRealm().clientTypes().updateClientTypes(clientTypes); Assert.fail("Not expected to update client types"); } catch (BadRequestException bre) { - // Expected + assertErrorContainsMessage(bre, ClientTypeException.Message.CLIENT_TYPE_FIELD_NOT_APPLICABLE); } // Test non-applicable attribute with default-value should fail @@ -221,7 +215,7 @@ public class ClientTypesTest extends AbstractTestRealmKeycloakTest { testRealm().clientTypes().updateClientTypes(clientTypes); Assert.fail("Not expected to update client types"); } catch (BadRequestException bre) { - // Expected + assertErrorContainsMessage(bre, ClientTypeException.Message.INVALID_CLIENT_TYPE_CONFIGURATION); } // Update should be successful @@ -241,7 +235,7 @@ public class ClientTypesTest extends AbstractTestRealmKeycloakTest { testRealm().clientTypes().updateClientTypes(clientTypes); Assert.fail("Not expected to update client types"); } catch (BadRequestException bre) { - // Expected + assertErrorContainsMessage(bre, ClientTypeException.Message.DUPLICATE_CLIENT_TYPE); } // Also test duplicated global name should fail @@ -250,7 +244,7 @@ public class ClientTypesTest extends AbstractTestRealmKeycloakTest { testRealm().clientTypes().updateClientTypes(clientTypes); Assert.fail("Not expected to update client types"); } catch (BadRequestException bre) { - // Expected + assertErrorContainsMessage(bre, ClientTypeException.Message.DUPLICATE_CLIENT_TYPE); } // Different name should be fine @@ -272,6 +266,20 @@ public class ClientTypesTest extends AbstractTestRealmKeycloakTest { assertNames(clientTypes.getGlobalClientTypes(), "sla", "service-account"); } + private void assertErrorResponseContainsParams(Response response, String... items) { + assertEquals(Response.Status.BAD_REQUEST, response.getStatusInfo()); + ErrorRepresentation errorRepresentation = response.readEntity(ErrorRepresentation.class); + assertThat( + List.of(items), + everyItem(in(errorRepresentation.getParams()))); + } + + private void assertErrorContainsMessage(BadRequestException bre, ClientTypeException.Message expectedException) { + ErrorRepresentation errorRepresentation = bre.getResponse().readEntity(ErrorRepresentation.class); + assertNotNull(errorRepresentation); + assertEquals(expectedException.getMessage(), errorRepresentation.getErrorMessage()); + } + private void assertNames(List clientTypes, String... expectedNames) { List names = clientTypes.stream() .map(ClientTypeRepresentation::getName) @@ -312,4 +320,4 @@ public class ClientTypesTest extends AbstractTestRealmKeycloakTest { } } } -} \ No newline at end of file +}