DefaultClientType service will now validate all client type default values and respond with bad request message with the affending parameters that attempt to override readonly in the client type config.

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

services/src/main/java/org/keycloak/services/clienttype/impl/DefaultClientType.java

@@ -19,10 +19,17 @@
 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.models.ClientModel;
@@ -32,6 +39,8 @@ 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;
 
 /**
  * @author <a href="mailto:mposolda@redhat.com">Marek Posolda</a>
@@ -81,47 +90,75 @@ public class DefaultClientType implements ClientType {
 
     @Override
     public void onCreate(ClientRepresentation createdClient) throws ClientTypeException {
-        for (Map.Entry<String, ClientTypeRepresentation.PropertyConfig> property : clientType.getConfig().entrySet()) {
+        // Create empty client augmented with the applicable default client type values.
-            ClientTypeRepresentation.PropertyConfig propertyConfig = property.getValue();
+        ClientRepresentation defaultClientRep = augmentClient(new ClientRepresentation());
-            if (!propertyConfig.getApplicable()) continue;
+
-            if (propertyConfig.getDefaultValue() != null) {
+        validateClientRequest(createdClient, defaultClientRep);
-                if (clientRepresentationProperties.containsKey(property.getKey())) {
+
-                    // Java property on client representation
+        augmentClient(createdClient);
-                    Method setter = clientRepresentationProperties.get(property.getKey()).getWriteMethod();
-                    try {
-                        setter.invoke(createdClient, propertyConfig.getDefaultValue());
-                    } catch (Exception e) {
-                        logger.warnf("Cannot set property '%s' on client with value '%s'. Check configuration of the client type '%s'", property.getKey(), propertyConfig.getDefaultValue(), clientType.getName());
-                        throw new ClientTypeException("Cannot set property on client", e);
-                    }
-                } else {
-                    // Client attribute
-                    if (createdClient.getAttributes() == null) {
-                        createdClient.setAttributes(new HashMap<>());
-                    }
-                    createdClient.getAttributes().put(property.getKey(), propertyConfig.getDefaultValue().toString());
-                }
-            }
-        }
     }
 
     @Override
     public void onUpdate(ClientModel currentClient, ClientRepresentation newClient) throws ClientTypeException {
-        ClientRepresentation oldClient = ModelToRepresentation.toRepresentation(currentClient, session);
+        ClientRepresentation currentRep = ModelToRepresentation.toRepresentation(currentClient, session);
-        for (Map.Entry<String, ClientTypeRepresentation.PropertyConfig> property : clientType.getConfig().entrySet()) {
+        validateClientRequest(newClient, currentRep);
-            String propertyName = property.getKey();
+    }
-            ClientTypeRepresentation.PropertyConfig propertyConfig = property.getValue();
 
-            Object oldVal = getClientProperty(oldClient, propertyName);
+    protected void validateClientRequest(ClientRepresentation newClient, ClientRepresentation currentClient) {
-            Object newVal = getClientProperty(newClient, propertyName);
+        List<String> validationErrors = clientType.getConfig().entrySet().stream()
+                .filter(property -> clientPropertyHasInvalidChangeRequested(currentClient, newClient, property.getKey(), property.getValue()))
+                .map(Map.Entry::getKey)
+                .collect(Collectors.toList());
 
-            // Validate that read-only client properties were not changed. Also validate that non-applicable properties were not changed.
+        if (validationErrors.size() > 0) {
-            if (!propertyConfig.getApplicable() || propertyConfig.getReadOnly()) {
+            throw ErrorResponse.error(
-                if (!ObjectUtil.isEqualOrBothNull(oldVal, newVal)) {
+                    "Cannot change property of client as it is not allowed by the specified client type.",
-                    logger.warnf("Cannot change property '%s' of client '%s' . Old value '%s', New value '%s'", propertyName, currentClient.getClientId(), oldVal, newVal);
+                    validationErrors.toArray(),
-                    throw new ClientTypeException("Cannot change property of client as it is not allowed");
+                    Response.Status.BAD_REQUEST);
         }
     }
+
+    protected ClientRepresentation augmentClient(ClientRepresentation client) {
+        clientType.getConfig().entrySet()
+                .forEach(property -> setClientProperty(client, property.getKey(), property.getValue()));
+        return client;
+    }
+
+    private boolean clientPropertyHasInvalidChangeRequested(
+            ClientRepresentation oldClient,
+            ClientRepresentation newClient,
+            String propertyName,
+            ClientTypeRepresentation.PropertyConfig propertyConfig) {
+        // Validate that read-only client properties were not changed.
+        return propertyConfig.getApplicable() &&
+                propertyConfig.getReadOnly() &&
+                !Objects.isNull(getClientProperty(newClient, propertyName)) &&
+                !Objects.equals(getClientProperty(oldClient, propertyName), getClientProperty(newClient, propertyName));
+    }
+
+    private void setClientProperty(ClientRepresentation client,
+                               String propertyName,
+                               ClientTypeRepresentation.PropertyConfig propertyConfig) {
+
+        if (!propertyConfig.getApplicable() || propertyConfig.getDefaultValue() == null) {
+            return;
+        }
+
+        if (clientRepresentationProperties.containsKey(propertyName)) {
+            // Java property on client representation
+            Method setter = clientRepresentationProperties.get(propertyName).getWriteMethod();
+            try {
+                setter.invoke(client, propertyConfig.getDefaultValue());
+            } catch (Exception e) {
+                logger.warnf("Cannot set property '%s' on client with value '%s'. Check configuration of the client type '%s'", propertyName, propertyConfig.getDefaultValue(), clientType.getName());
+                throw new ClientTypeException("Cannot set property on client", e);
+            }
+        } else {
+            // Client attribute
+            if (client.getAttributes() == null) {
+                client.setAttributes(new HashMap<>());
+            }
+            client.getAttributes().put(propertyName, propertyConfig.getDefaultValue().toString());
         }
     }
 

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientTypesTest.java

@@ -122,15 +122,11 @@ public class ClientTypesTest extends AbstractTestRealmKeycloakTest {
             // Expected
         }
 
-        // Adding non-applicable attribute should fail
+
         clientRep.setServiceAccountsEnabled(true);
+
+        // Adding non-applicable attribute should not fail
         clientRep.getAttributes().put(ClientModel.LOGO_URI, "https://foo");
-        try {
-            testRealm().clients().get(clientRep.getId()).update(clientRep);
-            Assert.fail("Not expected to update client");
-        } catch (BadRequestException bre) {
-            // Expected
-        }
 
         // Update of supported attribute should be successful
         clientRep.getAttributes().remove(ClientModel.LOGO_URI);