Improve Client Type Integration Tests (#29944)

closes #30017

Signed-off-by: Patrick Jennings <pajennin@redhat.com>

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) {
+    private ClientTypeException(String message, Object... parameters) {
-        super(message);
-    }
-
-    public 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;
+        }
+    }
 }

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(
+            Object[] paramsWithFailures = propertyUpdateExceptions.stream().map(ClientTypeException::getParameters).flatMap(Stream::of).toArray();
-                    "Cannot change property of client as it is not allowed by the specified client type.",
+            throw ClientTypeException.Message.CLIENT_UPDATE_FAILED_CLIENT_TYPE_VALIDATION.exception(paramsWithFailures);
-                    propertyUpdateExceptions.stream().map(ClientTypeException::getParameters).flatMap(Stream::of).toArray());
         }
     }
 
@@ -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);
     }

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,7 +110,8 @@ 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);
@@ -118,7 +121,6 @@ public class DefaultClientTypeManager implements ClientTypeManager {
             throw cte;
         }
     }
-    }
 
     static List<ClientTypeRepresentation> validateAndCastConfiguration(KeycloakSession session, List<ClientTypeRepresentation> clientTypes, List<ClientTypeRepresentation> globalTypes) {
         Set<String> usedNames = globalTypes.stream()
@@ -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);

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);
                     }
                 }
             }

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(
+            throw ClientTypeException.Message.CLIENT_UPDATE_FAILED_CLIENT_TYPE_VALIDATION.exception(propertyName);
-                    "Property " + propertyName + " is read-only due to client type " + clientType.getName(),
-                    propertyName);
         }
 
         // Delegate to clientSetter

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();
             }
         }
 

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<ClientTypeRepresentation> globalClientTypeNames = clientTypes.getGlobalClientTypes().stream()
+        List<ClientTypeRepresentation> globalClientTypeNames = new ArrayList<>(clientTypes.getGlobalClientTypes());
-                .collect(Collectors.toList());
         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<String, ClientTypeRepresentation.PropertyConfig>());
+            clientType.setConfig(new HashMap<>());
-            clientTypes.setRealmClientTypes(Arrays.asList(clientType));
+            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<ClientTypeRepresentation> clientTypes, String... expectedNames) {
         List<String> names = clientTypes.stream()
                 .map(ClientTypeRepresentation::getName)