From 9814733dd3c3e9c49d96c7576d43983ba9480504 Mon Sep 17 00:00:00 2001 From: Patrick Jennings Date: Mon, 1 Apr 2024 15:18:26 -0400 Subject: [PATCH] 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 --- .../clienttype/impl/DefaultClientType.java | 107 ++++++++++++------ .../testsuite/client/ClientTypesTest.java | 10 +- 2 files changed, 75 insertions(+), 42 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 85a7c8765c..c6e25a9456 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 @@ -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 Marek Posolda @@ -81,47 +90,75 @@ public class DefaultClientType implements ClientType { @Override public void onCreate(ClientRepresentation createdClient) throws ClientTypeException { - for (Map.Entry property : clientType.getConfig().entrySet()) { - ClientTypeRepresentation.PropertyConfig propertyConfig = property.getValue(); - if (!propertyConfig.getApplicable()) continue; - if (propertyConfig.getDefaultValue() != null) { - if (clientRepresentationProperties.containsKey(property.getKey())) { - // Java property on client representation - 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()); - } - } - } + // Create empty client augmented with the applicable default client type values. + ClientRepresentation defaultClientRep = augmentClient(new ClientRepresentation()); + + validateClientRequest(createdClient, defaultClientRep); + + augmentClient(createdClient); } @Override - public void onUpdate(ClientModel currentClient, ClientRepresentation newClient) throws ClientTypeException{ - ClientRepresentation oldClient = ModelToRepresentation.toRepresentation(currentClient, session); - for (Map.Entry property : clientType.getConfig().entrySet()) { - String propertyName = property.getKey(); - ClientTypeRepresentation.PropertyConfig propertyConfig = property.getValue(); + public void onUpdate(ClientModel currentClient, ClientRepresentation newClient) throws ClientTypeException { + ClientRepresentation currentRep = ModelToRepresentation.toRepresentation(currentClient, session); + validateClientRequest(newClient, currentRep); + } - Object oldVal = getClientProperty(oldClient, propertyName); - Object newVal = getClientProperty(newClient, propertyName); + protected void validateClientRequest(ClientRepresentation newClient, ClientRepresentation currentClient) { + List 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 (!propertyConfig.getApplicable() || propertyConfig.getReadOnly()) { - if (!ObjectUtil.isEqualOrBothNull(oldVal, newVal)) { - logger.warnf("Cannot change property '%s' of client '%s' . Old value '%s', New value '%s'", propertyName, currentClient.getClientId(), oldVal, newVal); - throw new ClientTypeException("Cannot change property of client as it is not allowed"); - } + if (validationErrors.size() > 0) { + throw ErrorResponse.error( + "Cannot change property of client as it is not allowed by the specified client type.", + validationErrors.toArray(), + 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()); } } 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 9a2ac13381..f2f56a90fb 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 @@ -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);