From 78d7aa24a1822083a6d0509b7b8b87399851a3a0 Mon Sep 17 00:00:00 2001 From: Patrick Jennings Date: Mon, 8 Apr 2024 11:43:40 -0400 Subject: [PATCH 01/15] Adding additional non-applicable client fields to the default service-account client type configuration. Signed-off-by: Patrick Jennings --- .../keycloak-default-client-types.json | 74 +++++++++++-------- 1 file changed, 42 insertions(+), 32 deletions(-) diff --git a/services/src/main/resources/keycloak-default-client-types.json b/services/src/main/resources/keycloak-default-client-types.json index 0cbd2991825..8db24450499 100644 --- a/services/src/main/resources/keycloak-default-client-types.json +++ b/services/src/main/resources/keycloak-default-client-types.json @@ -6,8 +6,8 @@ "config": { "standardFlowEnabled": { "applicable": true, - "read-only": true, - "default-value": true + "default-value": true, + "read-only": true } } }, @@ -18,59 +18,69 @@ "alwaysDisplayInConsole": { "applicable": false }, + "authorizationServicesEnabled": { + "applicable": false + }, + "bearerOnly": { + "applicable": false + }, "consentRequired": { "applicable": true, - "read-only": true, - "default-value": false + "default-value": false, + "read-only": true + }, + "directAccessGrantsEnabled": { + "applicable": false + }, + "frontchannelLogout": { + "applicable": false + }, + "implicitFlowEnabled": { + "applicable": false }, "login_theme": { "applicable": false }, - "protocol": { - "applicable": true, - "read-only": true, - "default-value": "openid-connect" + "logoUri": { + "applicable": false }, - "publicClient": { - "applicable": true, - "read-only": true, - "default-value": false + "oauth2.device.authorization.grant.enabled": { + "applicable": false }, - "bearerOnly": { - "applicable": true, - "read-only": true, - "default-value": false + "oidc.ciba.grant.enabled": { + "applicable": false }, - "standardFlowEnabled": { - "applicable": true, - "read-only": true, - "default-value": false + "policyUri": { + "applicable": false }, - "implicitFlowEnabled": { + "protocol": { "applicable": true, - "read-only": true, - "default-value": false + "default-value": "openid-connect", + "read-only": true }, - "directAccessGrantsEnabled": { + "publicClient": { "applicable": true, - "read-only": true, - "default-value": false + "default-value": false, + "read-only": true + }, + "redirectUris": { + "applicable": false }, "serviceAccountsEnabled": { "applicable": true, - "read-only": true, - "default-value": true + "default-value": true, + "read-only": true }, - "logoUri": { + "standardFlowEnabled": { "applicable": false }, - "policyUri": { + "tosUri": { "applicable": false }, - "tosUri": { + "webOrigins": { "applicable": false } } } ] -} \ No newline at end of file +} From 80bd73f82873f211deaa03bc01b0d6d2679b91fb Mon Sep 17 00:00:00 2001 From: Patrick Jennings Date: Fri, 19 Apr 2024 09:27:32 -0400 Subject: [PATCH 02/15] Creating TypedClientAttribute which maps clientmodel fields to standard client type configurations. Adding overrides for fields in TypeAwareClientModelDelegate required for service-account client type. Signed-off-by: Patrick Jennings --- .../client/TypeAwareClientModelDelegate.java | 154 ++++++++++++++---- .../client/TypedClientAttribute.java | 90 ++++++++++ 2 files changed, 216 insertions(+), 28 deletions(-) create mode 100644 services/src/main/java/org/keycloak/services/clienttype/client/TypedClientAttribute.java diff --git a/services/src/main/java/org/keycloak/services/clienttype/client/TypeAwareClientModelDelegate.java b/services/src/main/java/org/keycloak/services/clienttype/client/TypeAwareClientModelDelegate.java index aa76430d43d..33a27acf87a 100644 --- a/services/src/main/java/org/keycloak/services/clienttype/client/TypeAwareClientModelDelegate.java +++ b/services/src/main/java/org/keycloak/services/clienttype/client/TypeAwareClientModelDelegate.java @@ -18,14 +18,12 @@ package org.keycloak.services.clienttype.client; -import java.util.function.Consumer; +import java.util.Map; import java.util.function.Supplier; -import org.keycloak.common.util.ObjectUtil; import org.keycloak.models.ClientModel; import org.keycloak.models.delegate.ClientModelLazyDelegate; import org.keycloak.client.clienttype.ClientType; -import org.keycloak.client.clienttype.ClientTypeException; /** * Delegates to client-type and underlying delegate @@ -47,45 +45,145 @@ public TypeAwareClientModelDelegate(ClientType clientType, Supplier @Override public boolean isStandardFlowEnabled() { - return getBooleanProperty("standardFlowEnabled", super::isStandardFlowEnabled); + return TypedClientAttribute.STANDARD_FLOW_ENABLED + .getClientAttribute(clientType, super::isStandardFlowEnabled, Boolean.class); } @Override public void setStandardFlowEnabled(boolean standardFlowEnabled) { - setBooleanProperty("standardFlowEnabled", standardFlowEnabled, super::setStandardFlowEnabled); + TypedClientAttribute.STANDARD_FLOW_ENABLED + .setClientAttribute(clientType, standardFlowEnabled, super::setStandardFlowEnabled, Boolean.class); } + @Override + public boolean isBearerOnly() { + return TypedClientAttribute.BEARER_ONLY + .getClientAttribute(clientType, super::isBearerOnly, Boolean.class); + } - protected boolean getBooleanProperty(String propertyName, Supplier clientGetter) { - // Check if clientType supports the feature. If not, simply return false - if (!clientType.isApplicable(propertyName)) { - return false; - } + @Override + public void setBearerOnly(boolean bearerOnly) { + TypedClientAttribute.BEARER_ONLY + .setClientAttribute(clientType, bearerOnly, super::setBearerOnly, Boolean.class); + } - // Check if this is read-only. If yes, then we just directly delegate to return stuff from the clientType rather than from client - if (clientType.isReadOnly(propertyName)) { - return clientType.getDefaultValue(propertyName, Boolean.class); - } + @Override + public boolean isConsentRequired() { + return TypedClientAttribute.CONSENT_REQUIRED + .getClientAttribute(clientType, super::isConsentRequired, Boolean.class); + } + + @Override + public void setConsentRequired(boolean consentRequired) { + TypedClientAttribute.CONSENT_REQUIRED + .setClientAttribute(clientType, consentRequired, super::setConsentRequired, Boolean.class); + } + + @Override + public boolean isDirectAccessGrantsEnabled() { + return TypedClientAttribute.DIRECT_ACCESS_GRANTS_ENABLED + .getClientAttribute(clientType, super::isDirectAccessGrantsEnabled, Boolean.class); + } + + @Override + public void setDirectAccessGrantsEnabled(boolean directAccessGrantsEnabled) { + TypedClientAttribute.DIRECT_ACCESS_GRANTS_ENABLED + .setClientAttribute(clientType, directAccessGrantsEnabled, super::setDirectAccessGrantsEnabled, Boolean.class); + } + + @Override + public boolean isAlwaysDisplayInConsole() { + return TypedClientAttribute.ALWAYS_DISPLAY_IN_CONSOLE + .getClientAttribute(clientType, super::isAlwaysDisplayInConsole, Boolean.class); + } + + @Override + public void setAlwaysDisplayInConsole(boolean alwaysDisplayInConsole) { + TypedClientAttribute.ALWAYS_DISPLAY_IN_CONSOLE + .setClientAttribute(clientType, alwaysDisplayInConsole, super::setAlwaysDisplayInConsole, Boolean.class); + } + + @Override + public boolean isFrontchannelLogout() { + return TypedClientAttribute.FRONTCHANNEL_LOGOUT + .getClientAttribute(clientType, super::isFrontchannelLogout, Boolean.class); + } + + @Override + public void setFrontchannelLogout(boolean frontchannelLogout) { + TypedClientAttribute.FRONTCHANNEL_LOGOUT + .setClientAttribute(clientType, frontchannelLogout, super::setFrontchannelLogout, Boolean.class); + } + + @Override + public boolean isImplicitFlowEnabled() { + return TypedClientAttribute.IMPLICIT_FLOW_ENABLED + .getClientAttribute(clientType, super::isImplicitFlowEnabled, Boolean.class); + } + + @Override + public void setImplicitFlowEnabled(boolean implicitFlowEnabled) { + TypedClientAttribute.IMPLICIT_FLOW_ENABLED + .setClientAttribute(clientType, implicitFlowEnabled, super::setImplicitFlowEnabled, Boolean.class); + } - // Delegate to clientGetter - return clientGetter.get(); + @Override + public String getProtocol() { + return TypedClientAttribute.PROTOCOL + .getClientAttribute(clientType, super::getProtocol, String.class); } - protected void setBooleanProperty(String propertyName, Boolean newValue, Consumer clientSetter) { - // Check if clientType supports the feature. If not, return directly - if (!clientType.isApplicable(propertyName)) { - return; + @Override + public void setProtocol(String protocol) { + TypedClientAttribute.PROTOCOL + .setClientAttribute(clientType, protocol, super::setProtocol, String.class); + } + + @Override + public boolean isPublicClient() { + return TypedClientAttribute.PUBLIC_CLIENT + .getClientAttribute(clientType, super::isPublicClient, Boolean.class); + } + + @Override + public void setPublicClient(boolean flag) { + TypedClientAttribute.PUBLIC_CLIENT + .setClientAttribute(clientType, flag, super::setPublicClient, Boolean.class); + } + + @Override + public void setAttribute(String name, String value) { + TypedClientAttribute attribute = TypedClientAttribute.getAttributeByName(name); + if (attribute != null) { + attribute.setClientAttribute(clientType, value, (newValue) -> super.setAttribute(name, newValue), String.class); + } else { + super.setAttribute(name, value); } + } - // Check if this is read-only. If yes and there is an attempt to change some stuff, then throw an exception - if (clientType.isReadOnly(propertyName)) { - Boolean oldVal = clientType.getDefaultValue(propertyName, Boolean.class); - if (!ObjectUtil.isEqualOrBothNull(oldVal, newValue)) { - throw new ClientTypeException("Property " + propertyName + " of client " + getClientId() + " is read-only due to client type " + clientType.getName()); - } + @Override + public void removeAttribute(String name) { + TypedClientAttribute attribute = TypedClientAttribute.getAttributeByName(name); + if (attribute != null) { + attribute.setClientAttribute(clientType, null, (val) -> super.removeAttribute(name), String.class); + } else { + super.removeAttribute(name); } + } + + @Override + public String getAttribute(String name) { + TypedClientAttribute attribute = TypedClientAttribute.getAttributeByName(name); + if (attribute != null) { + return attribute.getClientAttribute(clientType, () -> super.getAttribute(name), String.class); + } else { + return super.getAttribute(name); + } + } - // Call clientSetter - clientSetter.accept(newValue); + @Override + public Map getAttributes() { + // TODO: Determine how to augment defined client type config attributes here. + return super.getAttributes(); } } \ No newline at end of file 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 new file mode 100644 index 00000000000..8d2a56a3824 --- /dev/null +++ b/services/src/main/java/org/keycloak/services/clienttype/client/TypedClientAttribute.java @@ -0,0 +1,90 @@ +package org.keycloak.services.clienttype.client; + +import org.keycloak.client.clienttype.ClientType; +import org.keycloak.client.clienttype.ClientTypeException; +import org.keycloak.common.util.ObjectUtil; + +import java.util.Arrays; +import java.util.HashMap; +import java.util.Map; +import java.util.Objects; +import java.util.function.Consumer; +import java.util.function.Supplier; + +enum TypedClientAttribute { + STANDARD_FLOW_ENABLED("standardFlowEnabled", false), + BEARER_ONLY("bearerOnly", false), + CONSENT_REQUIRED("consentRequired", false), + DIRECT_ACCESS_GRANTS_ENABLED("directAccessGrantsEnabled", false), + ALWAYS_DISPLAY_IN_CONSOLE("alwaysDisplayInConsole", false), + AUTHORIZATION_SERVICES_ENABLED("authorizationServicesEnabled", false), + FRONTCHANNEL_LOGOUT("frontchannelLogout", false), + IMPLICIT_FLOW_ENABLED("implicitFlowEnabled", false), + LOGIN_THEME("login_theme", ""), + LOGO_URI("logoUri", null), + DEVICE_AUTHORIZATION_GRANT_ENABLED("oauth2.device.authorization.grant.enabled", "false"), + CIBA_GRANT_ENABLED("oidc.ciba.grant.enabled", "false"), + POLICY_URI("policyUri", null), + PROTOCOL("protocol", null), + PUBLIC_CLIENT("publicClient", false), + REDIRECT_URIS("redirectUris", false); + + public String getPropertyName() { + return propertyName; + } + + private final String propertyName; + private final Object nonApplicableValue; + private static final Map attributeByName = new HashMap<>(); + + static { + Arrays.stream(TypedClientAttribute.values()) + .forEach(attribute -> attributeByName.put(attribute.getPropertyName(), attribute)); + } + + TypedClientAttribute(String propertyName, Object nonApplicableValue) { + this.propertyName = propertyName; + this.nonApplicableValue = nonApplicableValue; + } + + public T getClientAttribute(ClientType clientType, Supplier clientGetter, Class tClass) { + // Check if clientType supports the feature. + if (!clientType.isApplicable(propertyName)) { + return tClass.cast(nonApplicableValue); + } + + // Check if this is read-only. If yes, then we just directly delegate to return stuff from the clientType rather than from client + if (clientType.isReadOnly(propertyName)) { + return clientType.getDefaultValue(propertyName, tClass); + } + + // Delegate to clientGetter + return clientGetter.get(); + } + + public void setClientAttribute(ClientType clientType, T newValue, Consumer clientSetter, Class tClass) { + // Check if clientType supports the feature. If not, return directly + if (!clientType.isApplicable(propertyName) && !Objects.equals(nonApplicableValue, newValue)) { + throw new ClientTypeException( + "Property is not-applicable to client type " + clientType.getName() + + " and can not be modified.", propertyName); + } + + // Check if this is read-only. If yes and there is an attempt to change some stuff, then throw an exception + if (clientType.isReadOnly(propertyName)) { + T oldVal = clientType.getDefaultValue(propertyName, tClass); + if (!ObjectUtil.isEqualOrBothNull(oldVal, newValue)) { + throw new ClientTypeException( + "Property " + propertyName + " is read-only due to client type " + clientType.getName(), + propertyName); + } + } + + // Delegate to clientSetter + clientSetter.accept(newValue); + } + + public static TypedClientAttribute getAttributeByName(String name) { + return attributeByName.get(name); + } +} From a493b67a2b65b6f299f8cfebfced7401bd720667 Mon Sep 17 00:00:00 2001 From: Patrick Jennings Date: Fri, 19 Apr 2024 14:54:43 -0400 Subject: [PATCH 03/15] Splitting client type attribute enum into 3 separate enums, representing the top level ClientModel fields, the extended attributes through the client_attributes table, and the composable fields on ClientRepresentation. Signed-off-by: Patrick Jennings --- .../client/clienttype/ClientType.java | 5 + .../client/TypeAwareClientModelDelegate.java | 12 +- .../client/TypedClientAttribute.java | 115 +++++++++++++++--- .../clienttype/impl/DefaultClientType.java | 14 ++- 4 files changed, 115 insertions(+), 31 deletions(-) diff --git a/server-spi-private/src/main/java/org/keycloak/client/clienttype/ClientType.java b/server-spi-private/src/main/java/org/keycloak/client/clienttype/ClientType.java index afd9f021868..fec5e4e7d29 100644 --- a/server-spi-private/src/main/java/org/keycloak/client/clienttype/ClientType.java +++ b/server-spi-private/src/main/java/org/keycloak/client/clienttype/ClientType.java @@ -20,6 +20,9 @@ import org.keycloak.models.ClientModel; import org.keycloak.representations.idm.ClientRepresentation; +import org.keycloak.representations.idm.ClientTypeRepresentation; + +import java.util.Map; /** * TODO:client-types javadocs @@ -41,6 +44,8 @@ public interface ClientType { T getDefaultValue(String optionName, Class optionType); + Map getConfiguration(); + // Augment at the client type // Augment particular client on creation of client (TODO:client-types Should it be clientModel or clientRepresentation? Or something else?) void onCreate(ClientRepresentation newClient) throws ClientTypeException; diff --git a/services/src/main/java/org/keycloak/services/clienttype/client/TypeAwareClientModelDelegate.java b/services/src/main/java/org/keycloak/services/clienttype/client/TypeAwareClientModelDelegate.java index 33a27acf87a..15c3b79d205 100644 --- a/services/src/main/java/org/keycloak/services/clienttype/client/TypeAwareClientModelDelegate.java +++ b/services/src/main/java/org/keycloak/services/clienttype/client/TypeAwareClientModelDelegate.java @@ -20,6 +20,7 @@ import java.util.Map; import java.util.function.Supplier; +import java.util.stream.Collectors; import org.keycloak.models.ClientModel; import org.keycloak.models.delegate.ClientModelLazyDelegate; @@ -153,7 +154,7 @@ public void setPublicClient(boolean flag) { @Override public void setAttribute(String name, String value) { - TypedClientAttribute attribute = TypedClientAttribute.getAttributeByName(name); + ExtendedTypedClientAttribute attribute = ExtendedTypedClientAttribute.getAttributesByName().get(name); if (attribute != null) { attribute.setClientAttribute(clientType, value, (newValue) -> super.setAttribute(name, newValue), String.class); } else { @@ -163,7 +164,7 @@ public void setAttribute(String name, String value) { @Override public void removeAttribute(String name) { - TypedClientAttribute attribute = TypedClientAttribute.getAttributeByName(name); + ExtendedTypedClientAttribute attribute = ExtendedTypedClientAttribute.getAttributesByName().get(name); if (attribute != null) { attribute.setClientAttribute(clientType, null, (val) -> super.removeAttribute(name), String.class); } else { @@ -173,7 +174,7 @@ public void removeAttribute(String name) { @Override public String getAttribute(String name) { - TypedClientAttribute attribute = TypedClientAttribute.getAttributeByName(name); + ExtendedTypedClientAttribute attribute = ExtendedTypedClientAttribute.getAttributesByName().get(name); if (attribute != null) { return attribute.getClientAttribute(clientType, () -> super.getAttribute(name), String.class); } else { @@ -183,7 +184,8 @@ public String getAttribute(String name) { @Override public Map getAttributes() { - // TODO: Determine how to augment defined client type config attributes here. - return super.getAttributes(); + return clientType.getConfiguration().entrySet().stream() + .filter(entry -> TypedClientAttribute.getAttributesByName().containsKey(entry.getKey())) + .collect(Collectors.toMap(Map.Entry::getKey, entry -> getAttribute(entry.getKey()))); } } \ No newline at end of file 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 8d2a56a3824..e337c430dfe 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 @@ -11,35 +11,89 @@ import java.util.function.Consumer; import java.util.function.Supplier; -enum TypedClientAttribute { + +enum ExtendedTypedClientAttribute implements TypedClientAttributeInterface { + // Extended Client Type attributes defined as client attribute entities. + DEVICE_AUTHORIZATION_GRANT_ENABLED("oauth2.device.authorization.grant.enabled", "false"), + CIBA_GRANT_ENABLED("oidc.ciba.grant.enabled", "false"); + + private static final Map attributesByName = new HashMap<>(); + + static { + Arrays.stream(ExtendedTypedClientAttribute.values()) + .forEach(attribute -> attributesByName.put(attribute.getPropertyName(), attribute)); + } + + private final String propertyName; + private final Object nonApplicableValue; + + ExtendedTypedClientAttribute(String propertyName, Object nonApplicableValue) { + this.propertyName = propertyName; + this.nonApplicableValue = nonApplicableValue; + } + + @Override + public String getPropertyName() { + return propertyName; + } + + @Override + public Object getNonApplicableValue() { + return nonApplicableValue; + } + + public static Map getAttributesByName() { + return attributesByName; + } +} + +enum TypedClientRepresentationAttribute implements TypedClientAttributeInterface { + // Client type attributes specific to client representations. + AUTHORIZATION_SERVICES_ENABLED("authorizationServicesEnabled", false), + LOGIN_THEME("login_theme", ""), + LOGO_URI("logoUri", null), + POLICY_URI("policyUri", null); + + private final String propertyName; + private final Object nonApplicableValue; + + TypedClientRepresentationAttribute(String propertyName, Object nonApplicableValue) { + this.propertyName = propertyName; + this.nonApplicableValue = nonApplicableValue; + } + + @Override + public String getPropertyName() { + return propertyName; + } + + @Override + public Object getNonApplicableValue() { + return nonApplicableValue; + } +} + +enum TypedClientAttribute implements TypedClientAttributeInterface { + // Client Type attributes shared by client model and representation. STANDARD_FLOW_ENABLED("standardFlowEnabled", false), BEARER_ONLY("bearerOnly", false), CONSENT_REQUIRED("consentRequired", false), DIRECT_ACCESS_GRANTS_ENABLED("directAccessGrantsEnabled", false), ALWAYS_DISPLAY_IN_CONSOLE("alwaysDisplayInConsole", false), - AUTHORIZATION_SERVICES_ENABLED("authorizationServicesEnabled", false), FRONTCHANNEL_LOGOUT("frontchannelLogout", false), IMPLICIT_FLOW_ENABLED("implicitFlowEnabled", false), - LOGIN_THEME("login_theme", ""), - LOGO_URI("logoUri", null), - DEVICE_AUTHORIZATION_GRANT_ENABLED("oauth2.device.authorization.grant.enabled", "false"), - CIBA_GRANT_ENABLED("oidc.ciba.grant.enabled", "false"), - POLICY_URI("policyUri", null), PROTOCOL("protocol", null), PUBLIC_CLIENT("publicClient", false), REDIRECT_URIS("redirectUris", false); - public String getPropertyName() { - return propertyName; - } - private final String propertyName; private final Object nonApplicableValue; - private static final Map attributeByName = new HashMap<>(); + + private static final Map attributesByName = new HashMap<>(); static { Arrays.stream(TypedClientAttribute.values()) - .forEach(attribute -> attributeByName.put(attribute.getPropertyName(), attribute)); + .forEach(attribute -> attributesByName.put(attribute.getPropertyName(), attribute)); } TypedClientAttribute(String propertyName, Object nonApplicableValue) { @@ -47,7 +101,27 @@ public String getPropertyName() { this.nonApplicableValue = nonApplicableValue; } - public T getClientAttribute(ClientType clientType, Supplier clientGetter, Class tClass) { + @Override + public String getPropertyName() { + return propertyName; + } + + @Override + public Object getNonApplicableValue() { + return nonApplicableValue; + } + + public static Map getAttributesByName() { + return attributesByName; + } +} + +interface TypedClientAttributeInterface { + + default T getClientAttribute(ClientType clientType, Supplier clientGetter, Class tClass) { + String propertyName = getPropertyName(); + Object nonApplicableValue = getNonApplicableValue(); + // Check if clientType supports the feature. if (!clientType.isApplicable(propertyName)) { return tClass.cast(nonApplicableValue); @@ -62,12 +136,14 @@ public T getClientAttribute(ClientType clientType, Supplier clientGetter, return clientGetter.get(); } - public void setClientAttribute(ClientType clientType, T newValue, Consumer clientSetter, Class tClass) { + default void setClientAttribute(ClientType clientType, T newValue, Consumer clientSetter, Class tClass) { + String propertyName = getPropertyName(); + Object nonApplicableValue = getNonApplicableValue(); // Check if clientType supports the feature. If not, return directly if (!clientType.isApplicable(propertyName) && !Objects.equals(nonApplicableValue, newValue)) { throw new ClientTypeException( "Property is not-applicable to client type " + clientType.getName() - + " and can not be modified.", propertyName); + + " and can not be modified.", propertyName); } // Check if this is read-only. If yes and there is an attempt to change some stuff, then throw an exception @@ -84,7 +160,6 @@ public void setClientAttribute(ClientType clientType, T newValue, ConsumerMarek Posolda @@ -82,6 +80,11 @@ public T getDefaultValue(String optionName, Class optionType) { return (cfg != null && cfg.getDefaultValue() != null) ? optionType.cast(cfg.getDefaultValue()) : null; } + @Override + public Map getConfiguration() { + return clientType.getConfig(); + } + @Override public void onCreate(ClientRepresentation createdClient) throws ClientTypeException { // Create empty client augmented with the applicable default client type values. @@ -102,9 +105,9 @@ protected void validateClientRequest(ClientRepresentation newClient, ClientRepre List validationErrors = clientType.getConfig().entrySet().stream() .filter(property -> clientPropertyHasInvalidChangeRequested(currentClient, newClient, property.getKey(), property.getValue())) .map(Map.Entry::getKey) - .collect(Collectors.toList()); + .toList(); - if (validationErrors.size() > 0) { + if (!validationErrors.isEmpty()) { throw new ClientTypeException( "Cannot change property of client as it is not allowed by the specified client type.", validationErrors.toArray()); @@ -112,8 +115,7 @@ protected void validateClientRequest(ClientRepresentation newClient, ClientRepre } protected ClientRepresentation augmentClient(ClientRepresentation client) { - clientType.getConfig().entrySet() - .forEach(property -> setClientProperty(client, property.getKey(), property.getValue())); + clientType.getConfig().forEach((key, value) -> setClientProperty(client, key, value)); return client; } From cced214e38c81c8414a6d03785f416fad6dae9a0 Mon Sep 17 00:00:00 2001 From: Patrick Jennings Date: Mon, 22 Apr 2024 16:03:24 -0400 Subject: [PATCH 04/15] Removing reflection use for client types. Validation will be done in the RepresentationToModel methods that are responsible for the ClientRepresentation -> ClientModel create and update static methods. Signed-off-by: Patrick Jennings More updates Signed-off-by: Patrick Jennings --- .../client/clienttype/ClientType.java | 7 +- .../delegate/ClientModelLazyDelegate.java | 2 +- .../models/utils/RepresentationToModel.java | 212 ++++++++++++------ .../client/TypeAwareClientModelDelegate.java | 52 ++++- .../client/TypedClientAttribute.java | 92 +++----- .../clienttype/impl/DefaultClientType.java | 119 +--------- .../impl/DefaultClientTypeProvider.java | 12 +- .../DefaultClientTypeProviderFactory.java | 44 +--- .../services/managers/ClientManager.java | 7 - .../resources/admin/ClientResource.java | 11 - 10 files changed, 233 insertions(+), 325 deletions(-) diff --git a/server-spi-private/src/main/java/org/keycloak/client/clienttype/ClientType.java b/server-spi-private/src/main/java/org/keycloak/client/clienttype/ClientType.java index fec5e4e7d29..9f5a2e903f0 100644 --- a/server-spi-private/src/main/java/org/keycloak/client/clienttype/ClientType.java +++ b/server-spi-private/src/main/java/org/keycloak/client/clienttype/ClientType.java @@ -19,7 +19,6 @@ package org.keycloak.client.clienttype; import org.keycloak.models.ClientModel; -import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.ClientTypeRepresentation; import java.util.Map; @@ -47,9 +46,5 @@ public interface ClientType { Map getConfiguration(); // Augment at the client type - // Augment particular client on creation of client (TODO:client-types Should it be clientModel or clientRepresentation? Or something else?) - void onCreate(ClientRepresentation newClient) throws ClientTypeException; - - // Augment particular client on update of client (TODO:client-types Should it be clientModel or clientRepresentation? Or something else?) - void onUpdate(ClientModel currentClient, ClientRepresentation clientToUpdate) throws ClientTypeException; + ClientModel augment(ClientModel client); } \ No newline at end of file diff --git a/server-spi-private/src/main/java/org/keycloak/models/delegate/ClientModelLazyDelegate.java b/server-spi-private/src/main/java/org/keycloak/models/delegate/ClientModelLazyDelegate.java index 2ffa10b097f..50fa25a299a 100644 --- a/server-spi-private/src/main/java/org/keycloak/models/delegate/ClientModelLazyDelegate.java +++ b/server-spi-private/src/main/java/org/keycloak/models/delegate/ClientModelLazyDelegate.java @@ -23,7 +23,7 @@ import org.keycloak.models.ProtocolMapperModel; import org.keycloak.models.RealmModel; import org.keycloak.models.RoleModel; -import java.util.List; + import java.util.Map; import java.util.Set; import java.util.concurrent.atomic.AtomicMarkableReference; 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 2f2b7f07237..cc5c9df769d 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 @@ -18,6 +18,7 @@ package org.keycloak.models.utils; import java.io.IOException; +import java.util.ArrayList; import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; @@ -26,9 +27,13 @@ import java.util.Map; import java.util.Map.Entry; import java.util.Objects; +import java.util.Optional; import java.util.Set; +import java.util.function.Consumer; import java.util.function.Function; +import java.util.function.Supplier; import java.util.stream.Collectors; +import java.util.stream.Stream; import org.jboss.logging.Logger; import org.keycloak.Config; @@ -50,8 +55,12 @@ import org.keycloak.broker.provider.IdentityProvider; import org.keycloak.broker.provider.IdentityProviderFactory; import org.keycloak.broker.social.SocialIdentityProvider; +import org.keycloak.client.clienttype.ClientType; +import org.keycloak.client.clienttype.ClientTypeException; +import org.keycloak.client.clienttype.ClientTypeManager; import org.keycloak.common.Profile; import org.keycloak.common.util.MultivaluedHashMap; +import org.keycloak.common.util.ObjectUtil; import org.keycloak.common.util.UriUtils; import org.keycloak.component.ComponentModel; import org.keycloak.credential.CredentialModel; @@ -62,6 +71,7 @@ import org.keycloak.models.AuthenticatorConfigModel; import org.keycloak.models.ClientModel; import org.keycloak.models.ClientScopeModel; +import org.keycloak.models.ClientSecretConstants; import org.keycloak.models.Constants; import org.keycloak.models.FederatedIdentityModel; import org.keycloak.models.GroupModel; @@ -317,76 +327,95 @@ public static ClientModel createClient(KeycloakSession session, RealmModel realm logger.debugv("Create client: {0}", resourceRep.getClientId()); ClientModel client = resourceRep.getId() != null ? realm.addClient(resourceRep.getId(), resourceRep.getClientId()) : realm.addClient(resourceRep.getClientId()); - if (resourceRep.getName() != null) client.setName(resourceRep.getName()); - if (resourceRep.getDescription() != null) client.setDescription(resourceRep.getDescription()); - if (resourceRep.getType() != null) client.setType(resourceRep.getType()); - if (resourceRep.isEnabled() != null) client.setEnabled(resourceRep.isEnabled()); - if (resourceRep.isAlwaysDisplayInConsole() != null) client.setAlwaysDisplayInConsole(resourceRep.isAlwaysDisplayInConsole()); - client.setManagementUrl(resourceRep.getAdminUrl()); - if (resourceRep.isSurrogateAuthRequired() != null) - client.setSurrogateAuthRequired(resourceRep.isSurrogateAuthRequired()); - if (resourceRep.getRootUrl() != null) client.setRootUrl(resourceRep.getRootUrl()); - if (resourceRep.getBaseUrl() != null) client.setBaseUrl(resourceRep.getBaseUrl()); - if (resourceRep.isBearerOnly() != null) client.setBearerOnly(resourceRep.isBearerOnly()); - if (resourceRep.isConsentRequired() != null) client.setConsentRequired(resourceRep.isConsentRequired()); - // Backwards compatibility only - if (resourceRep.isDirectGrantsOnly() != null) { - logger.warn("Using deprecated 'directGrantsOnly' configuration in JSON representation. It will be removed in future versions"); - client.setStandardFlowEnabled(!resourceRep.isDirectGrantsOnly()); - client.setDirectAccessGrantsEnabled(resourceRep.isDirectGrantsOnly()); + if (Profile.isFeatureEnabled(Profile.Feature.CLIENT_TYPES) && resourceRep.getType() != null) { + ClientTypeManager mgr = session.getProvider(ClientTypeManager.class); + ClientType clientType = mgr.getClientType(realm, resourceRep.getType()); + client = clientType.augment(client); } - if (resourceRep.isStandardFlowEnabled() != null) - client.setStandardFlowEnabled(resourceRep.isStandardFlowEnabled()); - if (resourceRep.isImplicitFlowEnabled() != null) - client.setImplicitFlowEnabled(resourceRep.isImplicitFlowEnabled()); - if (resourceRep.isDirectAccessGrantsEnabled() != null) - client.setDirectAccessGrantsEnabled(resourceRep.isDirectAccessGrantsEnabled()); - if (resourceRep.isServiceAccountsEnabled() != null) - client.setServiceAccountsEnabled(resourceRep.isServiceAccountsEnabled()); - - if (resourceRep.isPublicClient() != null) client.setPublicClient(resourceRep.isPublicClient()); - if (resourceRep.isFrontchannelLogout() != null) - client.setFrontchannelLogout(resourceRep.isFrontchannelLogout()); + return updateClientProperties(realm, client, resourceRep, mappedFlows); + } - // set defaults to openid-connect if no protocol specified - if (resourceRep.getProtocol() != null) { - client.setProtocol(resourceRep.getProtocol()); - } else { - client.setProtocol(OIDC); - } - if (resourceRep.getNodeReRegistrationTimeout() != null) { - client.setNodeReRegistrationTimeout(resourceRep.getNodeReRegistrationTimeout()); - } else { - client.setNodeReRegistrationTimeout(-1); - } + public static ClientModel updateClientProperties(RealmModel realm, ClientModel client, ClientRepresentation resourceRep, Map mappedFlows) { + List> clientAttrMapper = new ArrayList>() {{ + add(() -> updateProperty(client::setName, resourceRep::getName)); + add(() -> updateProperty(client::setDescription, resourceRep::getDescription)); + add(() -> updateProperty(client::setType, resourceRep::getType)); + add(() -> updateProperty(client::setEnabled, resourceRep::isEnabled)); + add(() -> updateProperty(client::setAlwaysDisplayInConsole, resourceRep::isAlwaysDisplayInConsole)); + add(() -> updateProperty(client::setManagementUrl, resourceRep::getAdminUrl)); + add(() -> updateProperty(client::setSurrogateAuthRequired, resourceRep::isSurrogateAuthRequired)); + add(() -> updateProperty(client::setRootUrl, resourceRep::getRootUrl)); + add(() -> updateProperty(client::setBaseUrl, resourceRep::getBaseUrl)); + add(() -> updateProperty(client::setBearerOnly, resourceRep::isBearerOnly)); + add(() -> updateProperty(client::setConsentRequired, resourceRep::isConsentRequired)); + add(() -> updateProperty(client::setStandardFlowEnabled, resourceRep::isStandardFlowEnabled)); + add(() -> updateProperty(client::setImplicitFlowEnabled, resourceRep::isImplicitFlowEnabled)); + add(() -> updateProperty(client::setDirectAccessGrantsEnabled, resourceRep::isDirectAccessGrantsEnabled)); + add(() -> updateProperty(client::setServiceAccountsEnabled, resourceRep::isServiceAccountsEnabled)); + add(() -> updateProperty(client::setPublicClient, resourceRep::isPublicClient)); + add(() -> updateProperty(client::setFrontchannelLogout, resourceRep::isFrontchannelLogout)); + add(() -> updateProperty(client::setNotBefore, resourceRep::getNotBefore)); + // Fields with defaults if not initially provided + add(() -> updatePropertyWithDefault(client::setProtocol, resourceRep::getProtocol, client::getProtocol, () -> OIDC)); + add(() -> updatePropertyWithDefault(client::setNodeReRegistrationTimeout, resourceRep::getNodeReRegistrationTimeout, client::getNodeReRegistrationTimeout, () -> -1)); + add(() -> updatePropertyWithDefault(client::setClientAuthenticatorType, resourceRep::getClientAuthenticatorType, client::getClientAuthenticatorType, KeycloakModelUtils::getDefaultClientAuthenticatorType)); + // Client Secret + add(() -> updatePropertyWithDefault(client::setSecret, resourceRep::getSecret, client::getSecret, () -> { + if (client.isPublicClient() || client.isBearerOnly()) { + return null; + } + // adding secret if the client isn't public nor bearer only + String currentSecret = client.getSecret(); + String newSecret = resourceRep.getSecret(); + + if (newSecret == null && currentSecret == null) { + return KeycloakModelUtils.generateSecret(client); + } else if (newSecret != null) { + resourceRep.setSecret(newSecret); + return newSecret; + } + return currentSecret; + })); + // Redirect uris / Web origins + add(() -> updateProperty(client::setRedirectUris, () -> resourceRep.getRedirectUris().stream().collect(Collectors.toSet()))); + add(() -> updatePropertyWithDefault(uris -> uris.forEach(client::addWebOrigin), resourceRep::getWebOrigins, client::getWebOrigins, () -> { + if (!client.getWebOrigins().isEmpty()) { + return client.getWebOrigins(); + } + return Optional.ofNullable(resourceRep.getWebOrigins()).orElseGet(() -> resourceRep.getWebOrigins() + .stream() + .filter(uri -> uri.startsWith("http")) + .map(UriUtils::getOrigin) + .distinct() + .collect(Collectors.toList())); + })); + }}; - if (resourceRep.getNotBefore() != null) { - client.setNotBefore(resourceRep.getNotBefore()); + if (resourceRep.getAttributes() != null) { + for (Map.Entry entry : removeEmptyString(resourceRep.getAttributes()).entrySet()) { + clientAttrMapper.add(() -> updateProperty(val -> client.setAttribute(entry.getKey(), val), entry::getValue)); + } } - if (resourceRep.getClientAuthenticatorType() != null) { - client.setClientAuthenticatorType(resourceRep.getClientAuthenticatorType()); - } else { - client.setClientAuthenticatorType(KeycloakModelUtils.getDefaultClientAuthenticatorType()); - } + List validationExceptions = clientAttrMapper + .stream() + .map(Supplier::get) + .filter(Objects::nonNull) + .collect(Collectors.toList()); - // adding secret if the client isn't public nor bearer only - if (Objects.nonNull(resourceRep.getSecret())) { - client.setSecret(resourceRep.getSecret()); - } else { - if (client.isPublicClient() || client.isBearerOnly()) { - client.setSecret(null); - } else { - KeycloakModelUtils.generateSecret(client); - } + if (validationExceptions.size() > 0) { + throw new ClientTypeException( + "Cannot change property of client as it is not allowed by the specified client type.", + validationExceptions.stream().map(ClientTypeException::getParameters).flatMap(Stream::of).toArray()); } - if (resourceRep.getAttributes() != null) { - for (Map.Entry entry : resourceRep.getAttributes().entrySet()) { - client.setAttribute(entry.getKey(), entry.getValue()); - } + // Backwards compatibility only + if (resourceRep.isDirectGrantsOnly() != null) { + logger.warn("Using deprecated 'directGrantsOnly' configuration in JSON representation. It will be removed in future versions"); + client.setStandardFlowEnabled(!resourceRep.isDirectGrantsOnly()); + client.setDirectAccessGrantsEnabled(resourceRep.isDirectGrantsOnly()); } if ("saml".equals(resourceRep.getProtocol()) @@ -413,12 +442,6 @@ public static ClientModel createClient(KeycloakSession session, RealmModel realm } } - - if (resourceRep.getRedirectUris() != null) { - for (String redirectUri : resourceRep.getRedirectUris()) { - client.addRedirectUri(redirectUri); - } - } if (resourceRep.getWebOrigins() != null) { for (String webOrigin : resourceRep.getWebOrigins()) { logger.debugv("Client: {0} webOrigin: {1}", resourceRep.getClientId(), webOrigin); @@ -489,6 +512,19 @@ private static void addClientScopeToClient(RealmModel realm, ClientModel client, } public static void updateClient(ClientRepresentation rep, ClientModel resource, KeycloakSession session) { + + if (Profile.isFeatureEnabled(Profile.Feature.CLIENT_TYPES)) { + if (!ObjectUtil.isEqualOrBothNull(rep.getType(), rep.getType())) { + throw new ClientTypeException("Not supported to change client type"); + } + if (rep.getType() != null) { + RealmModel realm = session.getContext().getRealm(); + ClientTypeManager mgr = session.getProvider(ClientTypeManager.class); + ClientType clientType = mgr.getClientType(realm, rep.getType()); + resource = clientType.augment(resource); + } + } + String newClientId = rep.getClientId(); String previousClientId = resource.getClientId(); if (newClientId != null) resource.setClientId(newClientId); @@ -584,10 +620,11 @@ public static void updateClient(ClientRepresentation rep, ClientModel resource, resource.updateClient(); if (!Objects.equals(newClientId, previousClientId)) { + ClientModel finalResource = resource; ClientModel.ClientIdChangeEvent event = new ClientModel.ClientIdChangeEvent() { @Override public ClientModel getUpdatedClient() { - return resource; + return finalResource; } @Override @@ -663,6 +700,47 @@ public static void updateClientScopes(ClientRepresentation resourceRep, ClientMo } } + /** + * Update property, if not null. Captures {@link ClientTypeException} if thrown by the setter. + * @param modelSetter setter to call. + * @param representationGetter getter supplying the property update. + * @return Whether a {@link ClientTypeException} was thrown. + * @param Type of property. + */ + private static ClientTypeException updateProperty(Consumer modelSetter, Supplier representationGetter) { + T value = representationGetter.get(); + if (value != null) { + try { + modelSetter.accept(value); + } + catch (ClientTypeException cte) { + return cte; + } + } + return null; + } + + /** + * Update property with values supplied by the methods provided. The first non-null value will be used to set the + * property. + * @param modelSetter The setter to call with the property updates. + * @param representationGetter First getter of the representation to query for a non-null value. + * @param modelGetter Second getter corresponding to the value current assigned for the property. + * @param defaultSupplier If no other non-null value is found, this value shall be used. + * @return Whether a {@link ClientTypeException} was thrown. + * @param Type of property. + */ + private static ClientTypeException updatePropertyWithDefault(Consumer modelSetter, Supplier representationGetter, Supplier modelGetter, Supplier defaultSupplier) { + return updateProperty(modelSetter, () -> Stream.of(representationGetter, modelGetter, defaultSupplier) + .map(Supplier::get) + .map(Optional::ofNullable) + .filter(Optional::isPresent) + .map(Optional::get) + .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/client/TypeAwareClientModelDelegate.java b/services/src/main/java/org/keycloak/services/clienttype/client/TypeAwareClientModelDelegate.java index 15c3b79d205..1309da027d2 100644 --- a/services/src/main/java/org/keycloak/services/clienttype/client/TypeAwareClientModelDelegate.java +++ b/services/src/main/java/org/keycloak/services/clienttype/client/TypeAwareClientModelDelegate.java @@ -18,13 +18,16 @@ package org.keycloak.services.clienttype.client; +import java.util.HashMap; import java.util.Map; +import java.util.Set; import java.util.function.Supplier; import java.util.stream.Collectors; import org.keycloak.models.ClientModel; import org.keycloak.models.delegate.ClientModelLazyDelegate; import org.keycloak.client.clienttype.ClientType; +import org.keycloak.representations.idm.ClientTypeRepresentation; /** * Delegates to client-type and underlying delegate @@ -152,9 +155,33 @@ public void setPublicClient(boolean flag) { .setClientAttribute(clientType, flag, super::setPublicClient, Boolean.class); } + @Override + public Set getRedirectUris() { + return TypedClientAttribute.REDIRECT_URIS + .getClientAttribute(clientType, super::getRedirectUris, Set.class); + } + + @Override + public void setRedirectUris(Set redirectUris) { + TypedClientAttribute.REDIRECT_URIS + .setClientAttribute(clientType, redirectUris, super::setRedirectUris, Set.class); + } + + @Override + public void addRedirectUri(String redirectUri) { + TypedClientAttribute.REDIRECT_URIS + .setClientAttribute(clientType, redirectUri, super::addRedirectUri, String.class); + } + + @Override + public void removeRedirectUri(String redirectUri) { + TypedClientAttribute.REDIRECT_URIS + .setClientAttribute(clientType, null, (val) -> super.removeAttribute(redirectUri), String.class); + } + @Override public void setAttribute(String name, String value) { - ExtendedTypedClientAttribute attribute = ExtendedTypedClientAttribute.getAttributesByName().get(name); + TypedClientExtendedAttribute attribute = TypedClientExtendedAttribute.getAttributesByName().get(name); if (attribute != null) { attribute.setClientAttribute(clientType, value, (newValue) -> super.setAttribute(name, newValue), String.class); } else { @@ -164,7 +191,7 @@ public void setAttribute(String name, String value) { @Override public void removeAttribute(String name) { - ExtendedTypedClientAttribute attribute = ExtendedTypedClientAttribute.getAttributesByName().get(name); + TypedClientExtendedAttribute attribute = TypedClientExtendedAttribute.getAttributesByName().get(name); if (attribute != null) { attribute.setClientAttribute(clientType, null, (val) -> super.removeAttribute(name), String.class); } else { @@ -174,7 +201,7 @@ public void removeAttribute(String name) { @Override public String getAttribute(String name) { - ExtendedTypedClientAttribute attribute = ExtendedTypedClientAttribute.getAttributesByName().get(name); + TypedClientExtendedAttribute attribute = TypedClientExtendedAttribute.getAttributesByName().get(name); if (attribute != null) { return attribute.getClientAttribute(clientType, () -> super.getAttribute(name), String.class); } else { @@ -184,8 +211,21 @@ public String getAttribute(String name) { @Override public Map getAttributes() { - return clientType.getConfiguration().entrySet().stream() - .filter(entry -> TypedClientAttribute.getAttributesByName().containsKey(entry.getKey())) - .collect(Collectors.toMap(Map.Entry::getKey, entry -> getAttribute(entry.getKey()))); + // Start with attributes set on the delegate. + Map attributes = new HashMap<>(super.getAttributes()); + + // Get extended client type attributes and values from the client type configuration. + Set extendedClientTypeAttributes = + clientType.getConfiguration().entrySet().stream() + .map(Map.Entry::getKey) + .filter(entry -> TypedClientExtendedAttribute.getAttributesByName().containsKey(entry)) + .collect(Collectors.toSet()); + + // Augment client type attributes on top of attributes on the delegate. + for (String entry : extendedClientTypeAttributes) { + attributes.put(entry, getAttribute(entry)); + } + + return attributes; } } \ No newline at end of file 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 e337c430dfe..0f7d74482b9 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 @@ -1,33 +1,36 @@ package org.keycloak.services.clienttype.client; +import org.jboss.logging.Logger; import org.keycloak.client.clienttype.ClientType; import org.keycloak.client.clienttype.ClientTypeException; import org.keycloak.common.util.ObjectUtil; import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.Map; import java.util.Objects; +import java.util.Set; import java.util.function.Consumer; import java.util.function.Supplier; - -enum ExtendedTypedClientAttribute implements TypedClientAttributeInterface { - // Extended Client Type attributes defined as client attribute entities. - DEVICE_AUTHORIZATION_GRANT_ENABLED("oauth2.device.authorization.grant.enabled", "false"), - CIBA_GRANT_ENABLED("oidc.ciba.grant.enabled", "false"); - - private static final Map attributesByName = new HashMap<>(); - - static { - Arrays.stream(ExtendedTypedClientAttribute.values()) - .forEach(attribute -> attributesByName.put(attribute.getPropertyName(), attribute)); - } +enum TypedClientAttribute implements TypedClientAttributeInterface { + // Client Type attributes shared by client model and representation. + STANDARD_FLOW_ENABLED("standardFlowEnabled", false), + BEARER_ONLY("bearerOnly", false), + CONSENT_REQUIRED("consentRequired", false), + DIRECT_ACCESS_GRANTS_ENABLED("directAccessGrantsEnabled", false), + ALWAYS_DISPLAY_IN_CONSOLE("alwaysDisplayInConsole", false), + FRONTCHANNEL_LOGOUT("frontchannelLogout", false), + IMPLICIT_FLOW_ENABLED("implicitFlowEnabled", false), + PROTOCOL("protocol", null), + PUBLIC_CLIENT("publicClient", false), + REDIRECT_URIS("redirectUris", Set.of()); private final String propertyName; private final Object nonApplicableValue; - ExtendedTypedClientAttribute(String propertyName, Object nonApplicableValue) { + TypedClientAttribute(String propertyName, Object nonApplicableValue) { this.propertyName = propertyName; this.nonApplicableValue = nonApplicableValue; } @@ -41,62 +44,27 @@ public String getPropertyName() { public Object getNonApplicableValue() { return nonApplicableValue; } - - public static Map getAttributesByName() { - return attributesByName; - } } -enum TypedClientRepresentationAttribute implements TypedClientAttributeInterface { - // Client type attributes specific to client representations. - AUTHORIZATION_SERVICES_ENABLED("authorizationServicesEnabled", false), +enum TypedClientExtendedAttribute implements TypedClientAttributeInterface { + // Extended Client Type attributes defined as client attribute entities. + DEVICE_AUTHORIZATION_GRANT_ENABLED("oauth2.device.authorization.grant.enabled", "false"), + CIBA_GRANT_ENABLED("oidc.ciba.grant.enabled", "false"), LOGIN_THEME("login_theme", ""), LOGO_URI("logoUri", null), POLICY_URI("policyUri", null); - private final String propertyName; - private final Object nonApplicableValue; - - TypedClientRepresentationAttribute(String propertyName, Object nonApplicableValue) { - this.propertyName = propertyName; - this.nonApplicableValue = nonApplicableValue; - } + private static final Map attributesByName = new HashMap<>(); - @Override - public String getPropertyName() { - return propertyName; - } - - @Override - public Object getNonApplicableValue() { - return nonApplicableValue; + static { + Arrays.stream(TypedClientExtendedAttribute.values()) + .forEach(attribute -> attributesByName.put(attribute.getPropertyName(), attribute)); } -} - -enum TypedClientAttribute implements TypedClientAttributeInterface { - // Client Type attributes shared by client model and representation. - STANDARD_FLOW_ENABLED("standardFlowEnabled", false), - BEARER_ONLY("bearerOnly", false), - CONSENT_REQUIRED("consentRequired", false), - DIRECT_ACCESS_GRANTS_ENABLED("directAccessGrantsEnabled", false), - ALWAYS_DISPLAY_IN_CONSOLE("alwaysDisplayInConsole", false), - FRONTCHANNEL_LOGOUT("frontchannelLogout", false), - IMPLICIT_FLOW_ENABLED("implicitFlowEnabled", false), - PROTOCOL("protocol", null), - PUBLIC_CLIENT("publicClient", false), - REDIRECT_URIS("redirectUris", false); private final String propertyName; private final Object nonApplicableValue; - private static final Map attributesByName = new HashMap<>(); - - static { - Arrays.stream(TypedClientAttribute.values()) - .forEach(attribute -> attributesByName.put(attribute.getPropertyName(), attribute)); - } - - TypedClientAttribute(String propertyName, Object nonApplicableValue) { + TypedClientExtendedAttribute(String propertyName, Object nonApplicableValue) { this.propertyName = propertyName; this.nonApplicableValue = nonApplicableValue; } @@ -111,12 +79,13 @@ public Object getNonApplicableValue() { return nonApplicableValue; } - public static Map getAttributesByName() { + public static Map getAttributesByName() { return attributesByName; } } interface TypedClientAttributeInterface { + Logger logger = Logger.getLogger(TypedClientAttributeInterface.class); default T getClientAttribute(ClientType clientType, Supplier clientGetter, Class tClass) { String propertyName = getPropertyName(); @@ -124,7 +93,12 @@ default T getClientAttribute(ClientType clientType, Supplier clientGetter // Check if clientType supports the feature. if (!clientType.isApplicable(propertyName)) { - return tClass.cast(nonApplicableValue); + try { + return tClass.cast(nonApplicableValue); + } catch (ClassCastException e) { + logger.error("Could not apply client type property %s: %s", propertyName, e); + throw e; + } } // Check if this is read-only. If yes, then we just directly delegate to return stuff from the clientType rather than from client 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 5dcc3692c9b..4deeea9c45a 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,38 +18,22 @@ package org.keycloak.services.clienttype.impl; -import org.jboss.logging.Logger; 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.services.clienttype.client.TypeAwareClientModelDelegate; -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; /** * @author Marek Posolda */ public class DefaultClientType implements ClientType { - private static final Logger logger = Logger.getLogger(DefaultClientType.class); - - private final KeycloakSession session; private final ClientTypeRepresentation clientType; - private final Map clientRepresentationProperties; - - public DefaultClientType(KeycloakSession session, ClientTypeRepresentation clientType, Map clientRepresentationProperties) { - this.session = session; + public DefaultClientType(ClientTypeRepresentation clientType) { this.clientType = clientType; - this.clientRepresentationProperties = clientRepresentationProperties; } @Override @@ -86,102 +70,7 @@ public Map getConfiguration() { } @Override - public void onCreate(ClientRepresentation createdClient) throws ClientTypeException { - // 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 currentRep = ModelToRepresentation.toRepresentation(currentClient, session); - validateClientRequest(newClient, currentRep); - } - - 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) - .toList(); - - if (!validationErrors.isEmpty()) { - throw new ClientTypeException( - "Cannot change property of client as it is not allowed by the specified client type.", - validationErrors.toArray()); - } - } - - protected ClientRepresentation augmentClient(ClientRepresentation client) { - clientType.getConfig().forEach((key, value) -> setClientProperty(client, key, value)); - return client; - } - - private boolean clientPropertyHasInvalidChangeRequested( - ClientRepresentation oldClient, - ClientRepresentation newClient, - String propertyName, - ClientTypeRepresentation.PropertyConfig propertyConfig) { - Object newClientProperty = getClientProperty(newClient, propertyName); - Object oldClientProperty = getClientProperty(oldClient, propertyName); - - return ( - // Validate that non-applicable client properties were not changed. - !propertyConfig.getApplicable() && - !Objects.isNull(newClientProperty) && - !Objects.equals(oldClientProperty, newClientProperty) - ) || ( - // Validate that applicable read-only client properties were not changed. - propertyConfig.getApplicable() && - propertyConfig.getReadOnly() && - !Objects.isNull(newClientProperty) && - !Objects.equals(oldClientProperty, newClientProperty) - ); - } - - 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()); - } - } - - private Object getClientProperty(ClientRepresentation client, String propertyName) { - PropertyDescriptor propertyDescriptor = clientRepresentationProperties.get(propertyName); - - if (propertyDescriptor != null) { - // Java property - Method getter = propertyDescriptor.getReadMethod(); - try { - return getter.invoke(client); - } catch (Exception e) { - logger.warnf("Cannot read property '%s' on client '%s'. Client type is '%s'", propertyName, client.getClientId(), clientType.getName()); - throw new ClientTypeException("Cannot read property of client", e); - } - } else { - // Attribute - return client.getAttributes() == null ? null : client.getAttributes().get(propertyName); - } + public ClientModel augment(ClientModel client) { + return new TypeAwareClientModelDelegate(this, () -> client); } } \ 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 218b06e7ca4..10c12f83f85 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 @@ -18,11 +18,9 @@ package org.keycloak.services.clienttype.impl; -import java.beans.PropertyDescriptor; import java.util.Map; import org.jboss.logging.Logger; -import org.keycloak.models.KeycloakSession; import org.keycloak.representations.idm.ClientTypeRepresentation; import org.keycloak.client.clienttype.ClientType; import org.keycloak.client.clienttype.ClientTypeException; @@ -35,17 +33,9 @@ public class DefaultClientTypeProvider implements ClientTypeProvider { private static final Logger logger = Logger.getLogger(DefaultClientTypeProvider.class); - private final KeycloakSession session; - private final Map clientRepresentationProperties; - - public DefaultClientTypeProvider(KeycloakSession session, Map clientRepresentationProperties) { - this.session = session; - this.clientRepresentationProperties = clientRepresentationProperties; - } - @Override public ClientType getClientType(ClientTypeRepresentation clientTypeRep) { - return new DefaultClientType(session, clientTypeRep, clientRepresentationProperties); + return new DefaultClientType(clientTypeRep); } @Override diff --git a/services/src/main/java/org/keycloak/services/clienttype/impl/DefaultClientTypeProviderFactory.java b/services/src/main/java/org/keycloak/services/clienttype/impl/DefaultClientTypeProviderFactory.java index 474aad10730..04c7af37f31 100644 --- a/services/src/main/java/org/keycloak/services/clienttype/impl/DefaultClientTypeProviderFactory.java +++ b/services/src/main/java/org/keycloak/services/clienttype/impl/DefaultClientTypeProviderFactory.java @@ -18,23 +18,11 @@ package org.keycloak.services.clienttype.impl; -import java.beans.BeanInfo; -import java.beans.IntrospectionException; -import java.beans.Introspector; -import java.beans.PropertyDescriptor; -import java.lang.reflect.Method; -import java.util.Arrays; -import java.util.Map; -import java.util.Set; -import java.util.function.Function; -import java.util.stream.Collectors; - import org.keycloak.Config; import org.keycloak.common.Profile; import org.keycloak.models.KeycloakSession; import org.keycloak.models.KeycloakSessionFactory; import org.keycloak.provider.EnvironmentDependentProviderFactory; -import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.client.clienttype.ClientTypeProvider; import org.keycloak.client.clienttype.ClientTypeProviderFactory; @@ -45,41 +33,13 @@ public class DefaultClientTypeProviderFactory implements ClientTypeProviderFacto public static final String PROVIDER_ID = "default"; - private Map clientRepresentationProperties; - @Override public ClientTypeProvider create(KeycloakSession session) { - return new DefaultClientTypeProvider(session, clientRepresentationProperties); + return new DefaultClientTypeProvider(); } @Override - public void init(Config.Scope config) { - Set filtered = Arrays.stream(new String[] {"attributes", "type"}).collect(Collectors.toSet()); - - try { - BeanInfo bi = Introspector.getBeanInfo(ClientRepresentation.class); - PropertyDescriptor[] pd = bi.getPropertyDescriptors(); - clientRepresentationProperties = Arrays.stream(pd) - .filter(desc -> !filtered.contains(desc.getName())) - .filter(desc -> desc.getWriteMethod() != null) - .map(desc -> { - // Take "is" methods into consideration - if (desc.getReadMethod() == null && Boolean.class.equals(desc.getPropertyType())) { - String methodName = "is" + desc.getName().substring(0, 1).toUpperCase() + desc.getName().substring(1); - try { - Method getter = ClientRepresentation.class.getDeclaredMethod(methodName); - desc.setReadMethod(getter); - } catch (Exception e) { - throw new IllegalStateException("Getter method for property " + desc.getName() + " cannot be found"); - } - } - return desc; - }) - .collect(Collectors.toMap(PropertyDescriptor::getName, Function.identity())); - } catch (IntrospectionException ie) { - throw new IllegalStateException("Introspection of Client representation failed", ie); - } - } + public void init(Config.Scope config) {} @Override public void postInit(KeycloakSessionFactory factory) { diff --git a/services/src/main/java/org/keycloak/services/managers/ClientManager.java b/services/src/main/java/org/keycloak/services/managers/ClientManager.java index 2c499e4c622..cc623ce5332 100644 --- a/services/src/main/java/org/keycloak/services/managers/ClientManager.java +++ b/services/src/main/java/org/keycloak/services/managers/ClientManager.java @@ -42,8 +42,6 @@ import org.keycloak.representations.adapters.config.BaseRealmConfig; import org.keycloak.representations.adapters.config.PolicyEnforcerConfig; import org.keycloak.representations.idm.ClientRepresentation; -import org.keycloak.client.clienttype.ClientType; -import org.keycloak.client.clienttype.ClientTypeManager; import org.keycloak.sessions.AuthenticationSessionProvider; import java.net.URI; @@ -82,11 +80,6 @@ public ClientManager() { * @return */ public static ClientModel createClient(KeycloakSession session, RealmModel realm, ClientRepresentation rep) { - if (Profile.isFeatureEnabled(Profile.Feature.CLIENT_TYPES) && rep.getType() != null) { - ClientTypeManager mgr = session.getProvider(ClientTypeManager.class); - ClientType clientType = mgr.getClientType(realm, rep.getType()); - clientType.onCreate(rep); - } ClientModel client = RepresentationToModel.createClient(session, realm, rep); 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 ff70e8db472..099e3be25ac 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 @@ -152,17 +152,6 @@ public Response update(final ClientRepresentation rep) { session.setAttribute(ClientSecretConstants.CLIENT_SECRET_ROTATION_ENABLED,Boolean.FALSE); session.clientPolicy().triggerOnEvent(new AdminClientUpdateContext(rep, client, auth.adminAuth())); - if (Profile.isFeatureEnabled(Profile.Feature.CLIENT_TYPES)) { - if (!ObjectUtil.isEqualOrBothNull(rep.getType(), client.getType())) { - throw new ClientTypeException("Not supported to change client type"); - } - if (rep.getType() != null) { - ClientTypeManager mgr = session.getProvider(ClientTypeManager.class); - ClientType clientType = mgr.getClientType(realm, rep.getType()); - clientType.onUpdate(client, rep); - } - } - updateClientFromRep(rep, client, session); ValidationUtil.validateClient(session, client, false, r -> { From 71ed13149802d32589ea8457398692256787545a Mon Sep 17 00:00:00 2001 From: Patrick Jennings Date: Tue, 23 Apr 2024 14:51:13 -0400 Subject: [PATCH 05/15] Update client utilzes type aware client property update method. Signed-off-by: Patrick Jennings --- .../models/utils/RepresentationToModel.java | 284 +++++++----------- .../client/TypeAwareClientModelDelegate.java | 13 +- .../client/TypedClientAttribute.java | 7 +- 3 files changed, 118 insertions(+), 186 deletions(-) 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 cc5c9df769d..66b962eb468 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 @@ -71,7 +71,6 @@ import org.keycloak.models.AuthenticatorConfigModel; import org.keycloak.models.ClientModel; import org.keycloak.models.ClientScopeModel; -import org.keycloak.models.ClientSecretConstants; import org.keycloak.models.Constants; import org.keycloak.models.FederatedIdentityModel; import org.keycloak.models.GroupModel; @@ -334,82 +333,7 @@ public static ClientModel createClient(KeycloakSession session, RealmModel realm client = clientType.augment(client); } - return updateClientProperties(realm, client, resourceRep, mappedFlows); - } - - public static ClientModel updateClientProperties(RealmModel realm, ClientModel client, ClientRepresentation resourceRep, Map mappedFlows) { - List> clientAttrMapper = new ArrayList>() {{ - add(() -> updateProperty(client::setName, resourceRep::getName)); - add(() -> updateProperty(client::setDescription, resourceRep::getDescription)); - add(() -> updateProperty(client::setType, resourceRep::getType)); - add(() -> updateProperty(client::setEnabled, resourceRep::isEnabled)); - add(() -> updateProperty(client::setAlwaysDisplayInConsole, resourceRep::isAlwaysDisplayInConsole)); - add(() -> updateProperty(client::setManagementUrl, resourceRep::getAdminUrl)); - add(() -> updateProperty(client::setSurrogateAuthRequired, resourceRep::isSurrogateAuthRequired)); - add(() -> updateProperty(client::setRootUrl, resourceRep::getRootUrl)); - add(() -> updateProperty(client::setBaseUrl, resourceRep::getBaseUrl)); - add(() -> updateProperty(client::setBearerOnly, resourceRep::isBearerOnly)); - add(() -> updateProperty(client::setConsentRequired, resourceRep::isConsentRequired)); - add(() -> updateProperty(client::setStandardFlowEnabled, resourceRep::isStandardFlowEnabled)); - add(() -> updateProperty(client::setImplicitFlowEnabled, resourceRep::isImplicitFlowEnabled)); - add(() -> updateProperty(client::setDirectAccessGrantsEnabled, resourceRep::isDirectAccessGrantsEnabled)); - add(() -> updateProperty(client::setServiceAccountsEnabled, resourceRep::isServiceAccountsEnabled)); - add(() -> updateProperty(client::setPublicClient, resourceRep::isPublicClient)); - add(() -> updateProperty(client::setFrontchannelLogout, resourceRep::isFrontchannelLogout)); - add(() -> updateProperty(client::setNotBefore, resourceRep::getNotBefore)); - // Fields with defaults if not initially provided - add(() -> updatePropertyWithDefault(client::setProtocol, resourceRep::getProtocol, client::getProtocol, () -> OIDC)); - add(() -> updatePropertyWithDefault(client::setNodeReRegistrationTimeout, resourceRep::getNodeReRegistrationTimeout, client::getNodeReRegistrationTimeout, () -> -1)); - add(() -> updatePropertyWithDefault(client::setClientAuthenticatorType, resourceRep::getClientAuthenticatorType, client::getClientAuthenticatorType, KeycloakModelUtils::getDefaultClientAuthenticatorType)); - // Client Secret - add(() -> updatePropertyWithDefault(client::setSecret, resourceRep::getSecret, client::getSecret, () -> { - if (client.isPublicClient() || client.isBearerOnly()) { - return null; - } - // adding secret if the client isn't public nor bearer only - String currentSecret = client.getSecret(); - String newSecret = resourceRep.getSecret(); - - if (newSecret == null && currentSecret == null) { - return KeycloakModelUtils.generateSecret(client); - } else if (newSecret != null) { - resourceRep.setSecret(newSecret); - return newSecret; - } - return currentSecret; - })); - // Redirect uris / Web origins - add(() -> updateProperty(client::setRedirectUris, () -> resourceRep.getRedirectUris().stream().collect(Collectors.toSet()))); - add(() -> updatePropertyWithDefault(uris -> uris.forEach(client::addWebOrigin), resourceRep::getWebOrigins, client::getWebOrigins, () -> { - if (!client.getWebOrigins().isEmpty()) { - return client.getWebOrigins(); - } - return Optional.ofNullable(resourceRep.getWebOrigins()).orElseGet(() -> resourceRep.getWebOrigins() - .stream() - .filter(uri -> uri.startsWith("http")) - .map(UriUtils::getOrigin) - .distinct() - .collect(Collectors.toList())); - })); - }}; - - if (resourceRep.getAttributes() != null) { - for (Map.Entry entry : removeEmptyString(resourceRep.getAttributes()).entrySet()) { - clientAttrMapper.add(() -> updateProperty(val -> client.setAttribute(entry.getKey(), val), entry::getValue)); - } - } - - List validationExceptions = clientAttrMapper - .stream() - .map(Supplier::get) - .filter(Objects::nonNull) - .collect(Collectors.toList()); - - if (validationExceptions.size() > 0) { - throw new ClientTypeException( - "Cannot change property of client as it is not allowed by the specified client type.", - validationExceptions.stream().map(ClientTypeException::getParameters).flatMap(Stream::of).toArray()); - } + updateClientProperties(client, resourceRep); // Backwards compatibility only if (resourceRep.isDirectGrantsOnly() != null) { @@ -420,7 +344,7 @@ public static ClientModel updateClientProperties(RealmModel realm, ClientModel c if ("saml".equals(resourceRep.getProtocol()) && (resourceRep.getAttributes() == null - || !resourceRep.getAttributes().containsKey("saml.artifact.binding.identifier"))) { + || !resourceRep.getAttributes().containsKey("saml.artifact.binding.identifier"))) { client.setAttribute("saml.artifact.binding.identifier", computeArtifactBindingIdentifierString(resourceRep.getClientId())); } @@ -442,29 +366,6 @@ public static ClientModel updateClientProperties(RealmModel realm, ClientModel c } } - if (resourceRep.getWebOrigins() != null) { - for (String webOrigin : resourceRep.getWebOrigins()) { - logger.debugv("Client: {0} webOrigin: {1}", resourceRep.getClientId(), webOrigin); - client.addWebOrigin(webOrigin); - } - } else { - // add origins from redirect uris - if (resourceRep.getRedirectUris() != null) { - Set origins = new HashSet(); - for (String redirectUri : resourceRep.getRedirectUris()) { - logger.debugv("add redirect-uri to origin: {0}", redirectUri); - if (redirectUri.startsWith("http")) { - String origin = UriUtils.getOrigin(redirectUri); - logger.debugv("adding default client origin: {0}", origin); - origins.add(origin); - } - } - if (origins.size() > 0) { - client.setWebOrigins(origins); - } - } - } - if (resourceRep.getRegisteredNodes() != null) { for (Map.Entry entry : resourceRep.getRegisteredNodes().entrySet()) { client.registerNode(entry.getKey(), entry.getValue()); @@ -490,12 +391,6 @@ public static ClientModel updateClientProperties(RealmModel realm, ClientModel c updateClientScopes(resourceRep, client); - if (resourceRep.isFullScopeAllowed() != null) { - client.setFullScopeAllowed(resourceRep.isFullScopeAllowed()); - } else { - client.setFullScopeAllowed(!client.isConsentRequired()); - } - client.updateClient(); resourceRep.setId(client.getId()); @@ -527,42 +422,10 @@ public static void updateClient(ClientRepresentation rep, ClientModel resource, String newClientId = rep.getClientId(); String previousClientId = resource.getClientId(); + if (newClientId != null) resource.setClientId(newClientId); - if (rep.getName() != null) resource.setName(rep.getName()); - if (rep.getDescription() != null) resource.setDescription(rep.getDescription()); - if (rep.getType() != null) resource.setType(rep.getType()); - if (rep.isEnabled() != null) resource.setEnabled(rep.isEnabled()); - if (rep.isAlwaysDisplayInConsole() != null) resource.setAlwaysDisplayInConsole(rep.isAlwaysDisplayInConsole()); - if (rep.isBearerOnly() != null) resource.setBearerOnly(rep.isBearerOnly()); - if (rep.isConsentRequired() != null) resource.setConsentRequired(rep.isConsentRequired()); - if (rep.isStandardFlowEnabled() != null) resource.setStandardFlowEnabled(rep.isStandardFlowEnabled()); - if (rep.isImplicitFlowEnabled() != null) resource.setImplicitFlowEnabled(rep.isImplicitFlowEnabled()); - if (rep.isDirectAccessGrantsEnabled() != null) - resource.setDirectAccessGrantsEnabled(rep.isDirectAccessGrantsEnabled()); - if (rep.isServiceAccountsEnabled() != null) resource.setServiceAccountsEnabled(rep.isServiceAccountsEnabled()); - if (rep.isPublicClient() != null) resource.setPublicClient(rep.isPublicClient()); - if (rep.isFullScopeAllowed() != null) resource.setFullScopeAllowed(rep.isFullScopeAllowed()); - if (rep.isFrontchannelLogout() != null) resource.setFrontchannelLogout(rep.isFrontchannelLogout()); - if (rep.getRootUrl() != null) resource.setRootUrl(rep.getRootUrl()); - if (rep.getAdminUrl() != null) resource.setManagementUrl(rep.getAdminUrl()); - if (rep.getBaseUrl() != null) resource.setBaseUrl(rep.getBaseUrl()); - if (rep.isSurrogateAuthRequired() != null) resource.setSurrogateAuthRequired(rep.isSurrogateAuthRequired()); - if (rep.getNodeReRegistrationTimeout() != null) - resource.setNodeReRegistrationTimeout(rep.getNodeReRegistrationTimeout()); - if (rep.getClientAuthenticatorType() != null) - resource.setClientAuthenticatorType(rep.getClientAuthenticatorType()); - if (rep.getProtocol() != null) resource.setProtocol(rep.getProtocol()); - if (rep.getAttributes() != null) { - for (Map.Entry entry : rep.getAttributes().entrySet()) { - resource.setAttribute(entry.getKey(), entry.getValue()); - } - } - if (rep.getAttributes() != null) { - for (Map.Entry entry : removeEmptyString(rep.getAttributes()).entrySet()) { - resource.setAttribute(entry.getKey(), entry.getValue()); - } - } + updateClientProperties(resource, rep); if ("saml".equals(rep.getProtocol()) && (rep.getAttributes() == null @@ -584,39 +447,12 @@ public static void updateClient(ClientRepresentation rep, ClientModel resource, } } - if (rep.getNotBefore() != null) { - resource.setNotBefore(rep.getNotBefore()); - } - - List redirectUris = rep.getRedirectUris(); - if (redirectUris != null) { - resource.setRedirectUris(new HashSet<>(redirectUris)); - } - - List webOrigins = rep.getWebOrigins(); - if (webOrigins != null) { - resource.setWebOrigins(new HashSet<>(webOrigins)); - } - if (rep.getRegisteredNodes() != null) { for (Map.Entry entry : rep.getRegisteredNodes().entrySet()) { resource.registerNode(entry.getKey(), entry.getValue()); } } - if (resource.isPublicClient() || resource.isBearerOnly()) { - resource.setSecret(null); - } else { - String currentSecret = resource.getSecret(); - String newSecret = rep.getSecret(); - - if (newSecret == null && currentSecret == null) { - KeycloakModelUtils.generateSecret(resource); - } else if (newSecret != null) { - resource.setSecret(newSecret); - } - } - resource.updateClient(); if (!Objects.equals(newClientId, previousClientId)) { @@ -646,6 +482,86 @@ public KeycloakSession getKeycloakSession() { } } + private static ClientModel updateClientProperties(ClientModel client, ClientRepresentation rep) { + List> clientAttrUpdates = new ArrayList>() {{ + add(updateProperty(client::setName, rep::getName)); + add(updateProperty(client::setDescription, rep::getDescription)); + add(updateProperty(client::setType, rep::getType)); + add(updateProperty(client::setEnabled, rep::isEnabled)); + add(updateProperty(client::setAlwaysDisplayInConsole, rep::isAlwaysDisplayInConsole)); + add(updateProperty(client::setManagementUrl, rep::getAdminUrl)); + add(updateProperty(client::setSurrogateAuthRequired, rep::isSurrogateAuthRequired)); + add(updateProperty(client::setRootUrl, rep::getRootUrl)); + add(updateProperty(client::setBaseUrl, rep::getBaseUrl)); + add(updateProperty(client::setBearerOnly, rep::isBearerOnly)); + add(updateProperty(client::setConsentRequired, rep::isConsentRequired)); + add(updateProperty(client::setStandardFlowEnabled, rep::isStandardFlowEnabled)); + add(updateProperty(client::setImplicitFlowEnabled, rep::isImplicitFlowEnabled)); + add(updateProperty(client::setDirectAccessGrantsEnabled, rep::isDirectAccessGrantsEnabled)); + add(updateProperty(client::setServiceAccountsEnabled, rep::isServiceAccountsEnabled)); + add(updateProperty(client::setPublicClient, rep::isPublicClient)); + add(updateProperty(client::setFrontchannelLogout, rep::isFrontchannelLogout)); + add(updateProperty(client::setNotBefore, rep::getNotBefore)); + // Fields with defaults if not initially provided + add(updatePropertyWithDefault(client::setProtocol, rep::getProtocol, client::getProtocol, () -> OIDC)); + add(updatePropertyWithDefault(client::setNodeReRegistrationTimeout, rep::getNodeReRegistrationTimeout, client::getNodeReRegistrationTimeout, () -> -1)); + add(updatePropertyWithDefault(client::setClientAuthenticatorType, rep::getClientAuthenticatorType, client::getClientAuthenticatorType, KeycloakModelUtils::getDefaultClientAuthenticatorType)); + add(updatePropertyWithDefault(client::setFullScopeAllowed, rep::isFullScopeAllowed, client::isFullScopeAllowed, () -> !client.isConsentRequired())); + // Client Secret + add(updatePropertyWithDefault(client::setSecret, rep::getSecret, client::getSecret, () -> { + if (client.isPublicClient() || client.isBearerOnly()) { + return null; + } + // adding secret if the client isn't public nor bearer only + String currentSecret = client.getSecret(); + String newSecret = rep.getSecret(); + + if (newSecret == null && currentSecret == null) { + return KeycloakModelUtils.generateSecret(client); + } else if (newSecret != null) { + rep.setSecret(newSecret); + return newSecret; + } + return null; + })); + // Redirect uris / Web origins + add(updateProperty(client::setRedirectUris, () -> Optional.ofNullable(rep.getRedirectUris()).map(HashSet::new).orElse(null))); + add(updatePropertyWithDefault(uris -> uris.forEach(client::addWebOrigin), rep::getWebOrigins, client::getWebOrigins, () -> { + // add origins from redirect uris + if (rep.getRedirectUris() != null) { + return rep.getRedirectUris() + .stream() + .filter(uri -> uri.startsWith("http")) + .map(UriUtils::getOrigin) + .distinct() + .collect(Collectors.toList()); + } + return null; + })); + }}; + + // Extended client attributes + if (rep.getAttributes() != null) { + for (Map.Entry entry : removeEmptyString(rep.getAttributes()).entrySet()) { + clientAttrUpdates.add(updateProperty(val -> client.setAttribute(entry.getKey(), val), entry::getValue)); + } + } + + List validationExceptions = clientAttrUpdates + .stream() + .map(Supplier::get) + .filter(Objects::nonNull) + .collect(Collectors.toList()); + + if (validationExceptions.size() > 0) { + throw new ClientTypeException( + "Cannot change property of client as it is not allowed by the specified client type.", + validationExceptions.stream().map(ClientTypeException::getParameters).flatMap(Stream::of).toArray()); + } + + return client; + } + public static void updateClientProtocolMappers(ClientRepresentation rep, ClientModel resource) { if (rep.getProtocolMappers() != null) { @@ -701,28 +617,32 @@ public static void updateClientScopes(ClientRepresentation resourceRep, ClientMo } /** - * Update property, if not null. Captures {@link ClientTypeException} if thrown by the setter. + * Create Supplier to update property, if not null. + * Captures {@link ClientTypeException} if thrown by the setter. + * * @param modelSetter setter to call. * @param representationGetter getter supplying the property update. * @return Whether a {@link ClientTypeException} was thrown. * @param Type of property. */ - private static ClientTypeException updateProperty(Consumer modelSetter, Supplier representationGetter) { - T value = representationGetter.get(); - if (value != null) { - try { - modelSetter.accept(value); - } - catch (ClientTypeException cte) { - return cte; + private static Supplier updateProperty(Consumer modelSetter, Supplier representationGetter) { + return () -> { + T value = representationGetter.get(); + if (value != null) { + try { + modelSetter.accept(value); + } catch (ClientTypeException cte) { + return cte; + } } - } - return null; + return null; + }; } /** - * Update property with values supplied by the methods provided. The first non-null value will be used to set the - * property. + * Supplier to update property with values supplied by the methods provided. + * The first non-null value will be used to set the property. + * * @param modelSetter The setter to call with the property updates. * @param representationGetter First getter of the representation to query for a non-null value. * @param modelGetter Second getter corresponding to the value current assigned for the property. @@ -730,7 +650,7 @@ private static ClientTypeException updateProperty(Consumer modelSetter, S * @return Whether a {@link ClientTypeException} was thrown. * @param Type of property. */ - private static ClientTypeException updatePropertyWithDefault(Consumer modelSetter, Supplier representationGetter, Supplier modelGetter, Supplier defaultSupplier) { + private static Supplier updatePropertyWithDefault(Consumer modelSetter, Supplier representationGetter, Supplier modelGetter, Supplier defaultSupplier) { return updateProperty(modelSetter, () -> Stream.of(representationGetter, modelGetter, defaultSupplier) .map(Supplier::get) .map(Optional::ofNullable) diff --git a/services/src/main/java/org/keycloak/services/clienttype/client/TypeAwareClientModelDelegate.java b/services/src/main/java/org/keycloak/services/clienttype/client/TypeAwareClientModelDelegate.java index 1309da027d2..c4af87013cd 100644 --- a/services/src/main/java/org/keycloak/services/clienttype/client/TypeAwareClientModelDelegate.java +++ b/services/src/main/java/org/keycloak/services/clienttype/client/TypeAwareClientModelDelegate.java @@ -27,7 +27,6 @@ import org.keycloak.models.ClientModel; import org.keycloak.models.delegate.ClientModelLazyDelegate; import org.keycloak.client.clienttype.ClientType; -import org.keycloak.representations.idm.ClientTypeRepresentation; /** * Delegates to client-type and underlying delegate @@ -131,6 +130,18 @@ public void setImplicitFlowEnabled(boolean implicitFlowEnabled) { .setClientAttribute(clientType, implicitFlowEnabled, super::setImplicitFlowEnabled, Boolean.class); } + @Override + public boolean isServiceAccountsEnabled() { + return TypedClientAttribute.SERVICE_ACCOUNTS_ENABLED + .getClientAttribute(clientType, super::isServiceAccountsEnabled, Boolean.class); + } + + @Override + public void setServiceAccountsEnabled(boolean flag) { + TypedClientAttribute.SERVICE_ACCOUNTS_ENABLED + .setClientAttribute(clientType, flag, super::setServiceAccountsEnabled, Boolean.class); + } + @Override public String getProtocol() { return TypedClientAttribute.PROTOCOL 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 0f7d74482b9..7b98d072cf2 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 @@ -6,7 +6,6 @@ import org.keycloak.common.util.ObjectUtil; import java.util.Arrays; -import java.util.Collections; import java.util.HashMap; import java.util.Map; import java.util.Objects; @@ -15,7 +14,7 @@ import java.util.function.Supplier; enum TypedClientAttribute implements TypedClientAttributeInterface { - // Client Type attributes shared by client model and representation. + // Top Level client attributes STANDARD_FLOW_ENABLED("standardFlowEnabled", false), BEARER_ONLY("bearerOnly", false), CONSENT_REQUIRED("consentRequired", false), @@ -25,7 +24,9 @@ enum TypedClientAttribute implements TypedClientAttributeInterface { IMPLICIT_FLOW_ENABLED("implicitFlowEnabled", false), PROTOCOL("protocol", null), PUBLIC_CLIENT("publicClient", false), - REDIRECT_URIS("redirectUris", Set.of()); + REDIRECT_URIS("redirectUris", Set.of()), + SERVICE_ACCOUNTS_ENABLED("serviceAccountsEnabled", false), + ; private final String propertyName; private final Object nonApplicableValue; From 491f5bab9ba6f7572a8acd11bd93e91be8563954 Mon Sep 17 00:00:00 2001 From: Patrick Jennings Date: Tue, 23 Apr 2024 15:19:38 -0400 Subject: [PATCH 06/15] If user inputted representation object does not contain non-null value, try to get property value from the client. Type aware client model will return non-applicable or default value to keep fields consistent. Signed-off-by: Patrick Jennings --- .../models/utils/RepresentationToModel.java | 62 ++++++++++--------- .../client/TypeAwareClientModelDelegate.java | 26 +++++++- .../client/TypedClientAttribute.java | 1 + 3 files changed, 58 insertions(+), 31 deletions(-) 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 66b962eb468..41c57a006b0 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 @@ -482,26 +482,31 @@ public KeycloakSession getKeycloakSession() { } } - private static ClientModel updateClientProperties(ClientModel client, ClientRepresentation rep) { + /** + * Update client properties and process client type validation exceptions. + * @param client + * @param rep + */ + private static void updateClientProperties(ClientModel client, ClientRepresentation rep) { List> clientAttrUpdates = new ArrayList>() {{ - add(updateProperty(client::setName, rep::getName)); - add(updateProperty(client::setDescription, rep::getDescription)); - add(updateProperty(client::setType, rep::getType)); - add(updateProperty(client::setEnabled, rep::isEnabled)); - add(updateProperty(client::setAlwaysDisplayInConsole, rep::isAlwaysDisplayInConsole)); - add(updateProperty(client::setManagementUrl, rep::getAdminUrl)); - add(updateProperty(client::setSurrogateAuthRequired, rep::isSurrogateAuthRequired)); - add(updateProperty(client::setRootUrl, rep::getRootUrl)); - add(updateProperty(client::setBaseUrl, rep::getBaseUrl)); - add(updateProperty(client::setBearerOnly, rep::isBearerOnly)); - add(updateProperty(client::setConsentRequired, rep::isConsentRequired)); - add(updateProperty(client::setStandardFlowEnabled, rep::isStandardFlowEnabled)); - add(updateProperty(client::setImplicitFlowEnabled, rep::isImplicitFlowEnabled)); - add(updateProperty(client::setDirectAccessGrantsEnabled, rep::isDirectAccessGrantsEnabled)); - add(updateProperty(client::setServiceAccountsEnabled, rep::isServiceAccountsEnabled)); - add(updateProperty(client::setPublicClient, rep::isPublicClient)); - add(updateProperty(client::setFrontchannelLogout, rep::isFrontchannelLogout)); - add(updateProperty(client::setNotBefore, rep::getNotBefore)); + add(updatePropertyWithDefault(client::setName, rep::getName, client::getName)); + add(updatePropertyWithDefault(client::setDescription, rep::getDescription, client::getDescription)); + add(updatePropertyWithDefault(client::setType, rep::getType, client::getType)); + add(updatePropertyWithDefault(client::setEnabled, rep::isEnabled, client::isEnabled)); + add(updatePropertyWithDefault(client::setAlwaysDisplayInConsole, rep::isAlwaysDisplayInConsole, client::isAlwaysDisplayInConsole)); + add(updatePropertyWithDefault(client::setManagementUrl, rep::getAdminUrl, client::getManagementUrl)); + add(updatePropertyWithDefault(client::setSurrogateAuthRequired, rep::isSurrogateAuthRequired, client::isSurrogateAuthRequired)); + add(updatePropertyWithDefault(client::setRootUrl, rep::getRootUrl, client::getRootUrl)); + add(updatePropertyWithDefault(client::setBaseUrl, rep::getBaseUrl, client::getBaseUrl)); + add(updatePropertyWithDefault(client::setBearerOnly, rep::isBearerOnly, client::isBearerOnly)); + add(updatePropertyWithDefault(client::setConsentRequired, rep::isConsentRequired, client::isConsentRequired)); + add(updatePropertyWithDefault(client::setStandardFlowEnabled, rep::isStandardFlowEnabled, client::isStandardFlowEnabled)); + add(updatePropertyWithDefault(client::setImplicitFlowEnabled, rep::isImplicitFlowEnabled, client::isImplicitFlowEnabled)); + add(updatePropertyWithDefault(client::setDirectAccessGrantsEnabled, rep::isDirectAccessGrantsEnabled, client::isDirectAccessGrantsEnabled)); + add(updatePropertyWithDefault(client::setServiceAccountsEnabled, rep::isServiceAccountsEnabled, client::isServiceAccountsEnabled)); + add(updatePropertyWithDefault(client::setPublicClient, rep::isPublicClient, client::isPublicClient)); + add(updatePropertyWithDefault(client::setFrontchannelLogout, rep::isFrontchannelLogout, client::isFrontchannelLogout)); + add(updatePropertyWithDefault(client::setNotBefore, rep::getNotBefore, client::getNotBefore)); // Fields with defaults if not initially provided add(updatePropertyWithDefault(client::setProtocol, rep::getProtocol, client::getProtocol, () -> OIDC)); add(updatePropertyWithDefault(client::setNodeReRegistrationTimeout, rep::getNodeReRegistrationTimeout, client::getNodeReRegistrationTimeout, () -> -1)); @@ -525,7 +530,7 @@ private static ClientModel updateClientProperties(ClientModel client, ClientRepr return null; })); // Redirect uris / Web origins - add(updateProperty(client::setRedirectUris, () -> Optional.ofNullable(rep.getRedirectUris()).map(HashSet::new).orElse(null))); + add(updatePropertyWithDefault(client::setRedirectUris, () -> Optional.ofNullable(rep.getRedirectUris()).map(HashSet::new).orElse(null))); add(updatePropertyWithDefault(uris -> uris.forEach(client::addWebOrigin), rep::getWebOrigins, client::getWebOrigins, () -> { // add origins from redirect uris if (rep.getRedirectUris() != null) { @@ -543,7 +548,8 @@ private static ClientModel updateClientProperties(ClientModel client, ClientRepr // Extended client attributes if (rep.getAttributes() != null) { for (Map.Entry entry : removeEmptyString(rep.getAttributes()).entrySet()) { - clientAttrUpdates.add(updateProperty(val -> client.setAttribute(entry.getKey(), val), entry::getValue)); + clientAttrUpdates.add( + updatePropertyWithDefault(val -> client.setAttribute(entry.getKey(), val), entry::getValue, () -> client.getAttribute(entry.getKey()))); } } @@ -558,8 +564,6 @@ private static ClientModel updateClientProperties(ClientModel client, ClientRepr "Cannot change property of client as it is not allowed by the specified client type.", validationExceptions.stream().map(ClientTypeException::getParameters).flatMap(Stream::of).toArray()); } - - return client; } public static void updateClientProtocolMappers(ClientRepresentation rep, ClientModel resource) { @@ -640,18 +644,16 @@ private static Supplier updateProperty(Consumer mode } /** - * Supplier to update property with values supplied by the methods provided. - * The first non-null value will be used to set the property. + * Creates supplier to update property with values supplied by the methods provided in order. + * The first non-null value supplied will be used to the property setter. * * @param modelSetter The setter to call with the property updates. - * @param representationGetter First getter of the representation to query for a non-null value. - * @param modelGetter Second getter corresponding to the value current assigned for the property. - * @param defaultSupplier If no other non-null value is found, this value shall be used. + * @param getters Getter to query for a non-null value. * @return Whether a {@link ClientTypeException} was thrown. * @param Type of property. */ - private static Supplier updatePropertyWithDefault(Consumer modelSetter, Supplier representationGetter, Supplier modelGetter, Supplier defaultSupplier) { - return updateProperty(modelSetter, () -> Stream.of(representationGetter, modelGetter, defaultSupplier) + private static Supplier updatePropertyWithDefault(Consumer modelSetter, Supplier... getters) { + return updateProperty(modelSetter, () -> Stream.of(getters) .map(Supplier::get) .map(Optional::ofNullable) .filter(Optional::isPresent) diff --git a/services/src/main/java/org/keycloak/services/clienttype/client/TypeAwareClientModelDelegate.java b/services/src/main/java/org/keycloak/services/clienttype/client/TypeAwareClientModelDelegate.java index c4af87013cd..0ae60f3976d 100644 --- a/services/src/main/java/org/keycloak/services/clienttype/client/TypeAwareClientModelDelegate.java +++ b/services/src/main/java/org/keycloak/services/clienttype/client/TypeAwareClientModelDelegate.java @@ -166,6 +166,30 @@ public void setPublicClient(boolean flag) { .setClientAttribute(clientType, flag, super::setPublicClient, Boolean.class); } + @Override + public Set getWebOrigins() { + return TypedClientAttribute.WEB_ORIGINS + .getClientAttribute(clientType, super::getWebOrigins, Set.class); + } + + @Override + public void setWebOrigins(Set webOrigins) { + TypedClientAttribute.WEB_ORIGINS + .setClientAttribute(clientType, webOrigins, super::setWebOrigins, Set.class); + } + + @Override + public void addWebOrigin(String webOrigin) { + TypedClientAttribute.WEB_ORIGINS + .setClientAttribute(clientType, webOrigin, super::addWebOrigin, String.class); + } + + @Override + public void removeWebOrigin(String webOrigin) { + TypedClientAttribute.WEB_ORIGINS + .setClientAttribute(clientType, null, (val) -> super.removeWebOrigin(webOrigin), String.class); + } + @Override public Set getRedirectUris() { return TypedClientAttribute.REDIRECT_URIS @@ -187,7 +211,7 @@ public void addRedirectUri(String redirectUri) { @Override public void removeRedirectUri(String redirectUri) { TypedClientAttribute.REDIRECT_URIS - .setClientAttribute(clientType, null, (val) -> super.removeAttribute(redirectUri), String.class); + .setClientAttribute(clientType, null, (val) -> super.removeRedirectUri(redirectUri), String.class); } @Override 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 7b98d072cf2..a7932b05ef7 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 @@ -26,6 +26,7 @@ enum TypedClientAttribute implements TypedClientAttributeInterface { PUBLIC_CLIENT("publicClient", false), REDIRECT_URIS("redirectUris", Set.of()), SERVICE_ACCOUNTS_ENABLED("serviceAccountsEnabled", false), + WEB_ORIGINS("webOrigins", Set.of()), ; private final String propertyName; From b46e3bbe627aa7cdd15e4cee7b74d5b1b0942bba Mon Sep 17 00:00:00 2001 From: Patrick Jennings Date: Wed, 24 Apr 2024 10:17:22 -0400 Subject: [PATCH 07/15] Cleaning up RepresentationToModel Signed-off-by: Patrick Jennings --- .../models/utils/RepresentationToModel.java | 93 +++++++++++-------- .../client/TypedClientAttribute.java | 7 +- 2 files changed, 55 insertions(+), 45 deletions(-) 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 41c57a006b0..3438a5fbe26 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 @@ -19,6 +19,7 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collection; import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; @@ -381,7 +382,6 @@ public static ClientModel createClient(KeycloakSession session, RealmModel realm } MigrationUtils.updateProtocolMappers(client); - } if (resourceRep.getClientTemplate() != null) { @@ -483,9 +483,9 @@ public KeycloakSession getKeycloakSession() { } /** - * Update client properties and process client type validation exceptions. - * @param client - * @param rep + * Update client properties and process any {@link ClientTypeException} validation errors combining into one to be thrown. + * @param client New or existing client to update + * @param rep Incoming */ private static void updateClientProperties(ClientModel client, ClientRepresentation rep) { List> clientAttrUpdates = new ArrayList>() {{ @@ -513,36 +513,10 @@ private static void updateClientProperties(ClientModel client, ClientRepresentat add(updatePropertyWithDefault(client::setClientAuthenticatorType, rep::getClientAuthenticatorType, client::getClientAuthenticatorType, KeycloakModelUtils::getDefaultClientAuthenticatorType)); add(updatePropertyWithDefault(client::setFullScopeAllowed, rep::isFullScopeAllowed, client::isFullScopeAllowed, () -> !client.isConsentRequired())); // Client Secret - add(updatePropertyWithDefault(client::setSecret, rep::getSecret, client::getSecret, () -> { - if (client.isPublicClient() || client.isBearerOnly()) { - return null; - } - // adding secret if the client isn't public nor bearer only - String currentSecret = client.getSecret(); - String newSecret = rep.getSecret(); - - if (newSecret == null && currentSecret == null) { - return KeycloakModelUtils.generateSecret(client); - } else if (newSecret != null) { - rep.setSecret(newSecret); - return newSecret; - } - return null; - })); + add(updatePropertyWithDefault(client::setSecret, rep::getSecret, client::getSecret, () -> determineNewSecret(client, rep))); // Redirect uris / Web origins - add(updatePropertyWithDefault(client::setRedirectUris, () -> Optional.ofNullable(rep.getRedirectUris()).map(HashSet::new).orElse(null))); - add(updatePropertyWithDefault(uris -> uris.forEach(client::addWebOrigin), rep::getWebOrigins, client::getWebOrigins, () -> { - // add origins from redirect uris - if (rep.getRedirectUris() != null) { - return rep.getRedirectUris() - .stream() - .filter(uri -> uri.startsWith("http")) - .map(UriUtils::getOrigin) - .distinct() - .collect(Collectors.toList()); - } - return null; - })); + add(updatePropertyWithDefault(client::setRedirectUris, () -> collectionToSet(rep.getRedirectUris()))); + add(updatePropertyWithDefault(client::setWebOrigins, () -> collectionToSet(rep.getWebOrigins()), client::getWebOrigins, () -> webOriginsFromRedirectUris(rep))); }}; // Extended client attributes @@ -566,6 +540,38 @@ private static void updateClientProperties(ClientModel client, ClientRepresentat } } + private static String determineNewSecret(ClientModel client, ClientRepresentation rep) { + if (client.isPublicClient() || client.isBearerOnly()) { + // Clear out the secret with null + client.setSecret(null); + return null; + } + + // adding secret if the client isn't public nor bearer only + String currentSecret = client.getSecret(); + String newSecret = rep.getSecret(); + + if (newSecret == null && currentSecret == null) { + return KeycloakModelUtils.generateSecret(client); + } else if (newSecret != null) { + rep.setSecret(newSecret); + return newSecret; + } + // Do not change current secret. + return null; + } + + private static Set webOriginsFromRedirectUris(ClientRepresentation rep) { + if (rep.getRedirectUris() == null) { + return null; + } + return rep.getRedirectUris() + .stream() + .filter(uri -> uri.startsWith("http")) + .map(UriUtils::getOrigin) + .collect(Collectors.toSet()); + } + public static void updateClientProtocolMappers(ClientRepresentation rep, ClientModel resource) { if (rep.getProtocolMappers() != null) { @@ -626,7 +632,7 @@ public static void updateClientScopes(ClientRepresentation resourceRep, ClientMo * * @param modelSetter setter to call. * @param representationGetter getter supplying the property update. - * @return Whether a {@link ClientTypeException} was thrown. + * @return {@link Supplier} resulting whether a {@link ClientTypeException} was thrown. * @param Type of property. */ private static Supplier updateProperty(Consumer modelSetter, Supplier representationGetter) { @@ -644,22 +650,23 @@ private static Supplier updateProperty(Consumer mode } /** - * Creates supplier to update property with values supplied by the methods provided in order. - * The first non-null value supplied will be used to the property setter. + * Update property with the first non-null value supplied in order. + * The first non-null value supplied or else null will be sent to the setter. * - * @param modelSetter The setter to call with the property updates. - * @param getters Getter to query for a non-null value. - * @return Whether a {@link ClientTypeException} was thrown. + * @param modelSetter {@link Consumer} The setter to call. + * @param getters {@link Supplier} to query for the first non-null value. + * @return {@link Supplier} with results of operation. * @param Type of property. */ private static Supplier updatePropertyWithDefault(Consumer modelSetter, Supplier... getters) { - return updateProperty(modelSetter, () -> Stream.of(getters) + T firstNonNullSupplied = Stream.of(getters) .map(Supplier::get) .map(Optional::ofNullable) .filter(Optional::isPresent) .map(Optional::get) .findFirst() - .orElse(null)); + .orElse(null); + return updateProperty(modelSetter, () -> firstNonNullSupplied); } @@ -1607,4 +1614,8 @@ public static ResourceServer createResourceServer(ClientModel client, KeycloakSe return toModel(representation, authorization, client); } + + private static Set collectionToSet(Collection collection) { + return Optional.ofNullable(collection).map(HashSet::new).orElse(null); + } } 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 a7932b05ef7..6479b03c010 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 @@ -9,7 +9,6 @@ import java.util.HashMap; import java.util.Map; import java.util.Objects; -import java.util.Set; import java.util.function.Consumer; import java.util.function.Supplier; @@ -24,9 +23,9 @@ enum TypedClientAttribute implements TypedClientAttributeInterface { IMPLICIT_FLOW_ENABLED("implicitFlowEnabled", false), PROTOCOL("protocol", null), PUBLIC_CLIENT("publicClient", false), - REDIRECT_URIS("redirectUris", Set.of()), + REDIRECT_URIS("redirectUris", null), SERVICE_ACCOUNTS_ENABLED("serviceAccountsEnabled", false), - WEB_ORIGINS("webOrigins", Set.of()), + WEB_ORIGINS("webOrigins", null), ; private final String propertyName; @@ -52,7 +51,7 @@ enum TypedClientExtendedAttribute implements TypedClientAttributeInterface { // Extended Client Type attributes defined as client attribute entities. DEVICE_AUTHORIZATION_GRANT_ENABLED("oauth2.device.authorization.grant.enabled", "false"), CIBA_GRANT_ENABLED("oidc.ciba.grant.enabled", "false"), - LOGIN_THEME("login_theme", ""), + LOGIN_THEME("login_theme", null), LOGO_URI("logoUri", null), POLICY_URI("policyUri", null); From 1100c1a6b7266b7e5cc9ddd1ae3ebcfa33c6625b Mon Sep 17 00:00:00 2001 From: Patrick Jennings Date: Thu, 25 Apr 2024 14:00:37 -0400 Subject: [PATCH 08/15] Fixing issue when updating client secret. Signed-off-by: Patrick Jennings --- .../models/utils/RepresentationToModel.java | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) 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 3438a5fbe26..d93b5e2a329 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 @@ -513,7 +513,7 @@ private static void updateClientProperties(ClientModel client, ClientRepresentat add(updatePropertyWithDefault(client::setClientAuthenticatorType, rep::getClientAuthenticatorType, client::getClientAuthenticatorType, KeycloakModelUtils::getDefaultClientAuthenticatorType)); add(updatePropertyWithDefault(client::setFullScopeAllowed, rep::isFullScopeAllowed, client::isFullScopeAllowed, () -> !client.isConsentRequired())); // Client Secret - add(updatePropertyWithDefault(client::setSecret, rep::getSecret, client::getSecret, () -> determineNewSecret(client, rep))); + add(updatePropertyWithDefault(client::setSecret, () -> determineNewSecret(client, rep))); // Redirect uris / Web origins add(updatePropertyWithDefault(client::setRedirectUris, () -> collectionToSet(rep.getRedirectUris()))); add(updatePropertyWithDefault(client::setWebOrigins, () -> collectionToSet(rep.getWebOrigins()), client::getWebOrigins, () -> webOriginsFromRedirectUris(rep))); @@ -541,7 +541,7 @@ private static void updateClientProperties(ClientModel client, ClientRepresentat } private static String determineNewSecret(ClientModel client, ClientRepresentation rep) { - if (client.isPublicClient() || client.isBearerOnly()) { + if (Boolean.TRUE.equals(rep.isPublicClient()) || Boolean.TRUE.equals(rep.isBearerOnly())) { // Clear out the secret with null client.setSecret(null); return null; @@ -554,7 +554,6 @@ private static String determineNewSecret(ClientModel client, ClientRepresentatio if (newSecret == null && currentSecret == null) { return KeycloakModelUtils.generateSecret(client); } else if (newSecret != null) { - rep.setSecret(newSecret); return newSecret; } // Do not change current secret. @@ -637,13 +636,13 @@ public static void updateClientScopes(ClientRepresentation resourceRep, ClientMo */ private static Supplier updateProperty(Consumer modelSetter, Supplier representationGetter) { return () -> { - T value = representationGetter.get(); - if (value != null) { - try { - modelSetter.accept(value); - } catch (ClientTypeException cte) { - return cte; + try { + T value = representationGetter.get(); + if (value != null) { + modelSetter.accept(value); } + } catch (ClientTypeException cte) { + return cte; } return null; }; @@ -659,14 +658,12 @@ private static Supplier updateProperty(Consumer mode * @param Type of property. */ private static Supplier updatePropertyWithDefault(Consumer modelSetter, Supplier... getters) { - T firstNonNullSupplied = Stream.of(getters) + Stream firstNonNullSupplied = Stream.of(getters) .map(Supplier::get) .map(Optional::ofNullable) .filter(Optional::isPresent) - .map(Optional::get) - .findFirst() - .orElse(null); - return updateProperty(modelSetter, () -> firstNonNullSupplied); + .map(Optional::get); + return updateProperty(modelSetter, () -> firstNonNullSupplied.findFirst().orElse(null)); } From 2ccb37edc6ce499dbe8047f8836cdcdbc66577a2 Mon Sep 17 00:00:00 2001 From: Patrick Jennings Date: Fri, 26 Apr 2024 16:28:22 -0400 Subject: [PATCH 09/15] Fixing issue where created clients would not have fullscope allowed, because getter is a boolean and so cannot be null. Signed-off-by: Patrick Jennings --- .../models/utils/RepresentationToModel.java | 84 ++++++++++--------- 1 file changed, 45 insertions(+), 39 deletions(-) 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 d93b5e2a329..eae5536b230 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 @@ -334,7 +334,7 @@ public static ClientModel createClient(KeycloakSession session, RealmModel realm client = clientType.augment(client); } - updateClientProperties(client, resourceRep); + updateClientProperties(client, resourceRep, true); // Backwards compatibility only if (resourceRep.isDirectGrantsOnly() != null) { @@ -425,7 +425,7 @@ public static void updateClient(ClientRepresentation rep, ClientModel resource, if (newClientId != null) resource.setClientId(newClientId); - updateClientProperties(resource, rep); + updateClientProperties(resource, rep, false); if ("saml".equals(rep.getProtocol()) && (rep.getAttributes() == null @@ -484,59 +484,66 @@ public KeycloakSession getKeycloakSession() { /** * Update client properties and process any {@link ClientTypeException} validation errors combining into one to be thrown. - * @param client New or existing client to update - * @param rep Incoming + * + * @param client {@link ClientModel} to update + * @param rep {@link ClientRepresentation} to apply updates. + * @param isNew Whether the client is new or not (needed because some getters cannot return null). */ - private static void updateClientProperties(ClientModel client, ClientRepresentation rep) { - List> clientAttrUpdates = new ArrayList>() {{ - add(updatePropertyWithDefault(client::setName, rep::getName, client::getName)); - add(updatePropertyWithDefault(client::setDescription, rep::getDescription, client::getDescription)); - add(updatePropertyWithDefault(client::setType, rep::getType, client::getType)); - add(updatePropertyWithDefault(client::setEnabled, rep::isEnabled, client::isEnabled)); - add(updatePropertyWithDefault(client::setAlwaysDisplayInConsole, rep::isAlwaysDisplayInConsole, client::isAlwaysDisplayInConsole)); - add(updatePropertyWithDefault(client::setManagementUrl, rep::getAdminUrl, client::getManagementUrl)); - add(updatePropertyWithDefault(client::setSurrogateAuthRequired, rep::isSurrogateAuthRequired, client::isSurrogateAuthRequired)); - add(updatePropertyWithDefault(client::setRootUrl, rep::getRootUrl, client::getRootUrl)); - add(updatePropertyWithDefault(client::setBaseUrl, rep::getBaseUrl, client::getBaseUrl)); - add(updatePropertyWithDefault(client::setBearerOnly, rep::isBearerOnly, client::isBearerOnly)); - add(updatePropertyWithDefault(client::setConsentRequired, rep::isConsentRequired, client::isConsentRequired)); - add(updatePropertyWithDefault(client::setStandardFlowEnabled, rep::isStandardFlowEnabled, client::isStandardFlowEnabled)); - add(updatePropertyWithDefault(client::setImplicitFlowEnabled, rep::isImplicitFlowEnabled, client::isImplicitFlowEnabled)); - add(updatePropertyWithDefault(client::setDirectAccessGrantsEnabled, rep::isDirectAccessGrantsEnabled, client::isDirectAccessGrantsEnabled)); - add(updatePropertyWithDefault(client::setServiceAccountsEnabled, rep::isServiceAccountsEnabled, client::isServiceAccountsEnabled)); - add(updatePropertyWithDefault(client::setPublicClient, rep::isPublicClient, client::isPublicClient)); - add(updatePropertyWithDefault(client::setFrontchannelLogout, rep::isFrontchannelLogout, client::isFrontchannelLogout)); - add(updatePropertyWithDefault(client::setNotBefore, rep::getNotBefore, client::getNotBefore)); + private static void updateClientProperties(ClientModel client, ClientRepresentation rep, boolean isNew) { + List> clientPropertyUpdates = new ArrayList>() {{ + /** + * Values from the ClientRepresentation take precedence. + * Then values from the ClientModel (these may be augmented from the client type configuration). + * Otherwise, we can choose some sane default. + */ + add(updatePropertyAction(client::setName, rep::getName, client::getName)); + add(updatePropertyAction(client::setDescription, rep::getDescription, client::getDescription)); + add(updatePropertyAction(client::setType, rep::getType, client::getType)); + add(updatePropertyAction(client::setEnabled, rep::isEnabled, client::isEnabled)); + add(updatePropertyAction(client::setAlwaysDisplayInConsole, rep::isAlwaysDisplayInConsole, client::isAlwaysDisplayInConsole)); + add(updatePropertyAction(client::setManagementUrl, rep::getAdminUrl, client::getManagementUrl)); + add(updatePropertyAction(client::setSurrogateAuthRequired, rep::isSurrogateAuthRequired, client::isSurrogateAuthRequired)); + add(updatePropertyAction(client::setRootUrl, rep::getRootUrl, client::getRootUrl)); + add(updatePropertyAction(client::setBaseUrl, rep::getBaseUrl, client::getBaseUrl)); + add(updatePropertyAction(client::setBearerOnly, rep::isBearerOnly, client::isBearerOnly)); + add(updatePropertyAction(client::setConsentRequired, rep::isConsentRequired, client::isConsentRequired)); + add(updatePropertyAction(client::setStandardFlowEnabled, rep::isStandardFlowEnabled, client::isStandardFlowEnabled)); + add(updatePropertyAction(client::setImplicitFlowEnabled, rep::isImplicitFlowEnabled, client::isImplicitFlowEnabled)); + add(updatePropertyAction(client::setDirectAccessGrantsEnabled, rep::isDirectAccessGrantsEnabled, client::isDirectAccessGrantsEnabled)); + add(updatePropertyAction(client::setServiceAccountsEnabled, rep::isServiceAccountsEnabled, client::isServiceAccountsEnabled)); + add(updatePropertyAction(client::setPublicClient, rep::isPublicClient, client::isPublicClient)); + add(updatePropertyAction(client::setFrontchannelLogout, rep::isFrontchannelLogout, client::isFrontchannelLogout)); + add(updatePropertyAction(client::setNotBefore, rep::getNotBefore, client::getNotBefore)); // Fields with defaults if not initially provided - add(updatePropertyWithDefault(client::setProtocol, rep::getProtocol, client::getProtocol, () -> OIDC)); - add(updatePropertyWithDefault(client::setNodeReRegistrationTimeout, rep::getNodeReRegistrationTimeout, client::getNodeReRegistrationTimeout, () -> -1)); - add(updatePropertyWithDefault(client::setClientAuthenticatorType, rep::getClientAuthenticatorType, client::getClientAuthenticatorType, KeycloakModelUtils::getDefaultClientAuthenticatorType)); - add(updatePropertyWithDefault(client::setFullScopeAllowed, rep::isFullScopeAllowed, client::isFullScopeAllowed, () -> !client.isConsentRequired())); + add(updatePropertyAction(client::setProtocol, rep::getProtocol, client::getProtocol, () -> OIDC)); + add(updatePropertyAction(client::setNodeReRegistrationTimeout, rep::getNodeReRegistrationTimeout, client::getNodeReRegistrationTimeout, () -> -1)); + add(updatePropertyAction(client::setClientAuthenticatorType, rep::getClientAuthenticatorType, client::getClientAuthenticatorType, KeycloakModelUtils::getDefaultClientAuthenticatorType)); + add(updatePropertyAction(client::setFullScopeAllowed, rep::isFullScopeAllowed, () -> isNew ? !client.isConsentRequired() : client.isFullScopeAllowed())); // Client Secret - add(updatePropertyWithDefault(client::setSecret, () -> determineNewSecret(client, rep))); + add(updatePropertyAction(client::setSecret, () -> determineNewSecret(client, rep))); // Redirect uris / Web origins - add(updatePropertyWithDefault(client::setRedirectUris, () -> collectionToSet(rep.getRedirectUris()))); - add(updatePropertyWithDefault(client::setWebOrigins, () -> collectionToSet(rep.getWebOrigins()), client::getWebOrigins, () -> webOriginsFromRedirectUris(rep))); + add(updatePropertyAction(client::setRedirectUris, () -> collectionToSet(rep.getRedirectUris()))); + add(updatePropertyAction(client::setWebOrigins, () -> collectionToSet(rep.getWebOrigins()), client::getWebOrigins, () -> webOriginsFromRedirectUris(rep))); }}; // Extended client attributes if (rep.getAttributes() != null) { for (Map.Entry entry : removeEmptyString(rep.getAttributes()).entrySet()) { - clientAttrUpdates.add( - updatePropertyWithDefault(val -> client.setAttribute(entry.getKey(), val), entry::getValue, () -> client.getAttribute(entry.getKey()))); + clientPropertyUpdates.add( + updatePropertyAction(val -> client.setAttribute(entry.getKey(), val), entry::getValue, () -> client.getAttribute(entry.getKey()))); } } - List validationExceptions = clientAttrUpdates + List propertyUpdateExceptions = clientPropertyUpdates .stream() .map(Supplier::get) .filter(Objects::nonNull) .collect(Collectors.toList()); - if (validationExceptions.size() > 0) { + if (propertyUpdateExceptions.size() > 0) { throw new ClientTypeException( "Cannot change property of client as it is not allowed by the specified client type.", - validationExceptions.stream().map(ClientTypeException::getParameters).flatMap(Stream::of).toArray()); + propertyUpdateExceptions.stream().map(ClientTypeException::getParameters).flatMap(Stream::of).toArray()); } } @@ -649,15 +656,14 @@ private static Supplier updateProperty(Consumer mode } /** - * Update property with the first non-null value supplied in order. - * The first non-null value supplied or else null will be sent to the setter. + * Create an update property action, passing the first non-null value supplied to the setter, in argument order. * * @param modelSetter {@link Consumer} The setter to call. * @param getters {@link Supplier} to query for the first non-null value. * @return {@link Supplier} with results of operation. * @param Type of property. */ - private static Supplier updatePropertyWithDefault(Consumer modelSetter, Supplier... getters) { + private static Supplier updatePropertyAction(Consumer modelSetter, Supplier... getters) { Stream firstNonNullSupplied = Stream.of(getters) .map(Supplier::get) .map(Optional::ofNullable) From 43679e1b228c43b2f2424ea6ec969ee32080cb67 Mon Sep 17 00:00:00 2001 From: Patrick Jennings Date: Mon, 29 Apr 2024 16:51:31 -0400 Subject: [PATCH 10/15] Need to be able to clear out client attributes on update as was allowed before and causing failures in integration tests. Signed-off-by: Patrick Jennings --- .../models/utils/RepresentationToModel.java | 28 +++++++++++++------ 1 file changed, 20 insertions(+), 8 deletions(-) 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 eae5536b230..d77e481a725 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 @@ -516,19 +516,19 @@ private static void updateClientProperties(ClientModel client, ClientRepresentat add(updatePropertyAction(client::setNotBefore, rep::getNotBefore, client::getNotBefore)); // Fields with defaults if not initially provided add(updatePropertyAction(client::setProtocol, rep::getProtocol, client::getProtocol, () -> OIDC)); - add(updatePropertyAction(client::setNodeReRegistrationTimeout, rep::getNodeReRegistrationTimeout, client::getNodeReRegistrationTimeout, () -> -1)); + add(updatePropertyAction(client::setNodeReRegistrationTimeout, rep::getNodeReRegistrationTimeout, () -> defaultNodeReRegistraionTimeout(client, isNew))); add(updatePropertyAction(client::setClientAuthenticatorType, rep::getClientAuthenticatorType, client::getClientAuthenticatorType, KeycloakModelUtils::getDefaultClientAuthenticatorType)); - add(updatePropertyAction(client::setFullScopeAllowed, rep::isFullScopeAllowed, () -> isNew ? !client.isConsentRequired() : client.isFullScopeAllowed())); + add(updatePropertyAction(client::setFullScopeAllowed, rep::isFullScopeAllowed, () -> defaultFullScopeAllowed(client, isNew))); // Client Secret add(updatePropertyAction(client::setSecret, () -> determineNewSecret(client, rep))); // Redirect uris / Web origins add(updatePropertyAction(client::setRedirectUris, () -> collectionToSet(rep.getRedirectUris()))); - add(updatePropertyAction(client::setWebOrigins, () -> collectionToSet(rep.getWebOrigins()), client::getWebOrigins, () -> webOriginsFromRedirectUris(rep))); + add(updatePropertyAction(client::setWebOrigins, () -> collectionToSet(rep.getWebOrigins()), () -> defaultWebOrigins(client))); }}; // Extended client attributes if (rep.getAttributes() != null) { - for (Map.Entry entry : removeEmptyString(rep.getAttributes()).entrySet()) { + for (Map.Entry entry : rep.getAttributes().entrySet()) { clientPropertyUpdates.add( updatePropertyAction(val -> client.setAttribute(entry.getKey(), val), entry::getValue, () -> client.getAttribute(entry.getKey()))); } @@ -547,6 +547,17 @@ private static void updateClientProperties(ClientModel client, ClientRepresentat } } + private static Boolean defaultFullScopeAllowed(ClientModel client, boolean isNew) { + return isNew ? !client.isConsentRequired() : client.isFullScopeAllowed(); + } + + private static Integer defaultNodeReRegistraionTimeout(ClientModel client, boolean isNew) { + if (!ObjectUtil.isEqualOrBothNull(client.getNodeReRegistrationTimeout(), 0) || !isNew) { + return client.getNodeReRegistrationTimeout(); + } + return -1; + } + private static String determineNewSecret(ClientModel client, ClientRepresentation rep) { if (Boolean.TRUE.equals(rep.isPublicClient()) || Boolean.TRUE.equals(rep.isBearerOnly())) { // Clear out the secret with null @@ -567,11 +578,12 @@ private static String determineNewSecret(ClientModel client, ClientRepresentatio return null; } - private static Set webOriginsFromRedirectUris(ClientRepresentation rep) { - if (rep.getRedirectUris() == null) { - return null; + private static Set defaultWebOrigins(ClientModel client) { + Optional> webOrigins = Optional.of(client.getWebOrigins()).filter(origins -> !origins.isEmpty()); + if (webOrigins.isPresent()) { + return webOrigins.get(); } - return rep.getRedirectUris() + return client.getRedirectUris() .stream() .filter(uri -> uri.startsWith("http")) .map(UriUtils::getOrigin) From e6c04971b5a7372744fb2ce187727204eb98b3ef Mon Sep 17 00:00:00 2001 From: Patrick Jennings Date: Tue, 30 Apr 2024 09:45:24 -0400 Subject: [PATCH 11/15] Fixing issues with redirectUri and weborigins defaults in type aware clients. Signed-off-by: Patrick Jennings --- .../models/utils/RepresentationToModel.java | 19 ++++++++++++++----- .../client/TypedClientAttribute.java | 5 +++-- 2 files changed, 17 insertions(+), 7 deletions(-) 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 d77e481a725..2aeabb07f2e 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 @@ -522,7 +522,7 @@ private static void updateClientProperties(ClientModel client, ClientRepresentat // Client Secret add(updatePropertyAction(client::setSecret, () -> determineNewSecret(client, rep))); // Redirect uris / Web origins - add(updatePropertyAction(client::setRedirectUris, () -> collectionToSet(rep.getRedirectUris()))); + add(updatePropertyAction(client::setRedirectUris, () -> collectionToSet(rep.getRedirectUris()), client::getRedirectUris)); add(updatePropertyAction(client::setWebOrigins, () -> collectionToSet(rep.getWebOrigins()), () -> defaultWebOrigins(client))); }}; @@ -579,10 +579,16 @@ private static String determineNewSecret(ClientModel client, ClientRepresentatio } private static Set defaultWebOrigins(ClientModel client) { - Optional> webOrigins = Optional.of(client.getWebOrigins()).filter(origins -> !origins.isEmpty()); - if (webOrigins.isPresent()) { - return webOrigins.get(); + Set webOrigins = client.getWebOrigins(); + if (webOrigins != null && !webOrigins.isEmpty()) { + return webOrigins; } + + Set redirectUris = client.getRedirectUris(); + if (redirectUris == null || redirectUris.isEmpty()) { + return null; + } + return client.getRedirectUris() .stream() .filter(uri -> uri.startsWith("http")) @@ -1631,6 +1637,9 @@ public static ResourceServer createResourceServer(ClientModel client, KeycloakSe } private static Set collectionToSet(Collection collection) { - return Optional.ofNullable(collection).map(HashSet::new).orElse(null); + return Optional.ofNullable(collection) + .filter(col -> !col.isEmpty()) + .map(HashSet::new) + .orElse(null); } } 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 6479b03c010..acf680840f9 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 @@ -9,6 +9,7 @@ import java.util.HashMap; import java.util.Map; import java.util.Objects; +import java.util.Set; import java.util.function.Consumer; import java.util.function.Supplier; @@ -23,9 +24,9 @@ enum TypedClientAttribute implements TypedClientAttributeInterface { IMPLICIT_FLOW_ENABLED("implicitFlowEnabled", false), PROTOCOL("protocol", null), PUBLIC_CLIENT("publicClient", false), - REDIRECT_URIS("redirectUris", null), + REDIRECT_URIS("redirectUris", Set.of()), SERVICE_ACCOUNTS_ENABLED("serviceAccountsEnabled", false), - WEB_ORIGINS("webOrigins", null), + WEB_ORIGINS("webOrigins", Set.of()), ; private final String propertyName; From 4087e41e579ae4fad2a350c60e3dd8b0d1d880d4 Mon Sep 17 00:00:00 2001 From: Patrick Jennings Date: Tue, 30 Apr 2024 10:59:11 -0400 Subject: [PATCH 12/15] Need to allow client attributes the ability to clear out values during update. Signed-off-by: Patrick Jennings --- .../keycloak/models/utils/RepresentationToModel.java | 8 +++----- .../clienttype/client/TypedClientAttribute.java | 5 ++--- .../keycloak/testsuite/client/ClientTypesTest.java | 12 ++++-------- 3 files changed, 9 insertions(+), 16 deletions(-) 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 2aeabb07f2e..6ed399a8ec7 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 @@ -530,7 +530,7 @@ private static void updateClientProperties(ClientModel client, ClientRepresentat if (rep.getAttributes() != null) { for (Map.Entry entry : rep.getAttributes().entrySet()) { clientPropertyUpdates.add( - updatePropertyAction(val -> client.setAttribute(entry.getKey(), val), entry::getValue, () -> client.getAttribute(entry.getKey()))); + updatePropertyAction(val -> client.setAttribute(entry.getKey(), val), entry::getValue)); } } @@ -575,7 +575,7 @@ private static String determineNewSecret(ClientModel client, ClientRepresentatio return newSecret; } // Do not change current secret. - return null; + return currentSecret; } private static Set defaultWebOrigins(ClientModel client) { @@ -663,9 +663,7 @@ private static Supplier updateProperty(Consumer mode return () -> { try { T value = representationGetter.get(); - if (value != null) { - modelSetter.accept(value); - } + modelSetter.accept(value); } catch (ClientTypeException cte) { return cte; } 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 acf680840f9..bf026c426b8 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 @@ -117,9 +117,8 @@ default void setClientAttribute(ClientType clientType, T newValue, Consumer< Object nonApplicableValue = getNonApplicableValue(); // Check if clientType supports the feature. If not, return directly if (!clientType.isApplicable(propertyName) && !Objects.equals(nonApplicableValue, newValue)) { - throw new ClientTypeException( - "Property is not-applicable to client type " + clientType.getName() - + " and can not be modified.", propertyName); + logger.warnf("Property %s is not-applicable to client type %s and can not be modified.", propertyName, clientType.getName()); + return; } // Check if this is read-only. If yes and there is an attempt to change some stuff, then throw an 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 259dd888bdf..592a4ca4a4f 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 @@ -126,14 +126,10 @@ public void testUpdateClientWithClientType() { clientRep.setServiceAccountsEnabled(true); - // Adding non-applicable attribute should not fail + // Adding non-applicable attribute should not fail but not update client attribute 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) { - assertErrorResponseContainsParams(bre.getResponse(), "logoUri"); - } + testRealm().clients().get(clientRep.getId()).update(clientRep); + assertEquals(testRealm().clients().get(clientRep.getId()).toRepresentation().getAttributes().get(ClientModel.LOGO_URI), null); // Update of supported attribute should be successful clientRep.getAttributes().remove(ClientModel.LOGO_URI); @@ -180,7 +176,7 @@ public void testClientTypesAdminRestAPI_globalTypes() { assertEquals("default", serviceAccountType.getProvider()); ClientTypeRepresentation.PropertyConfig cfg = serviceAccountType.getConfig().get("standardFlowEnabled"); - assertPropertyConfig("standardFlowEnabled", cfg, true, true, false); + assertPropertyConfig("standardFlowEnabled", cfg, false, null, null); cfg = serviceAccountType.getConfig().get("serviceAccountsEnabled"); assertPropertyConfig("serviceAccountsEnabled", cfg, true, true, true); From 9c8107781402235885a15c68358d2efa3aa33342 Mon Sep 17 00:00:00 2001 From: Patrick Jennings Date: Tue, 30 Apr 2024 13:55:56 -0400 Subject: [PATCH 13/15] Renaming interface based on PR feedback. Signed-off-by: Patrick Jennings --- .../models/utils/RepresentationToModel.java | 3 +- .../client/TypeAwareClientModelDelegate.java | 56 +++++++++---------- ...e.java => TypedClientSimpleAttribute.java} | 10 ++-- 3 files changed, 34 insertions(+), 35 deletions(-) rename services/src/main/java/org/keycloak/services/clienttype/client/{TypedClientAttribute.java => TypedClientSimpleAttribute.java} (94%) 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 6ed399a8ec7..4c33909abd3 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 @@ -561,7 +561,6 @@ private static Integer defaultNodeReRegistraionTimeout(ClientModel client, boole private static String determineNewSecret(ClientModel client, ClientRepresentation rep) { if (Boolean.TRUE.equals(rep.isPublicClient()) || Boolean.TRUE.equals(rep.isBearerOnly())) { // Clear out the secret with null - client.setSecret(null); return null; } @@ -586,7 +585,7 @@ private static Set defaultWebOrigins(ClientModel client) { Set redirectUris = client.getRedirectUris(); if (redirectUris == null || redirectUris.isEmpty()) { - return null; + return new HashSet<>(); } return client.getRedirectUris() diff --git a/services/src/main/java/org/keycloak/services/clienttype/client/TypeAwareClientModelDelegate.java b/services/src/main/java/org/keycloak/services/clienttype/client/TypeAwareClientModelDelegate.java index 0ae60f3976d..f514858c8b7 100644 --- a/services/src/main/java/org/keycloak/services/clienttype/client/TypeAwareClientModelDelegate.java +++ b/services/src/main/java/org/keycloak/services/clienttype/client/TypeAwareClientModelDelegate.java @@ -48,169 +48,169 @@ public TypeAwareClientModelDelegate(ClientType clientType, Supplier @Override public boolean isStandardFlowEnabled() { - return TypedClientAttribute.STANDARD_FLOW_ENABLED + return TypedClientSimpleAttribute.STANDARD_FLOW_ENABLED .getClientAttribute(clientType, super::isStandardFlowEnabled, Boolean.class); } @Override public void setStandardFlowEnabled(boolean standardFlowEnabled) { - TypedClientAttribute.STANDARD_FLOW_ENABLED + TypedClientSimpleAttribute.STANDARD_FLOW_ENABLED .setClientAttribute(clientType, standardFlowEnabled, super::setStandardFlowEnabled, Boolean.class); } @Override public boolean isBearerOnly() { - return TypedClientAttribute.BEARER_ONLY + return TypedClientSimpleAttribute.BEARER_ONLY .getClientAttribute(clientType, super::isBearerOnly, Boolean.class); } @Override public void setBearerOnly(boolean bearerOnly) { - TypedClientAttribute.BEARER_ONLY + TypedClientSimpleAttribute.BEARER_ONLY .setClientAttribute(clientType, bearerOnly, super::setBearerOnly, Boolean.class); } @Override public boolean isConsentRequired() { - return TypedClientAttribute.CONSENT_REQUIRED + return TypedClientSimpleAttribute.CONSENT_REQUIRED .getClientAttribute(clientType, super::isConsentRequired, Boolean.class); } @Override public void setConsentRequired(boolean consentRequired) { - TypedClientAttribute.CONSENT_REQUIRED + TypedClientSimpleAttribute.CONSENT_REQUIRED .setClientAttribute(clientType, consentRequired, super::setConsentRequired, Boolean.class); } @Override public boolean isDirectAccessGrantsEnabled() { - return TypedClientAttribute.DIRECT_ACCESS_GRANTS_ENABLED + return TypedClientSimpleAttribute.DIRECT_ACCESS_GRANTS_ENABLED .getClientAttribute(clientType, super::isDirectAccessGrantsEnabled, Boolean.class); } @Override public void setDirectAccessGrantsEnabled(boolean directAccessGrantsEnabled) { - TypedClientAttribute.DIRECT_ACCESS_GRANTS_ENABLED + TypedClientSimpleAttribute.DIRECT_ACCESS_GRANTS_ENABLED .setClientAttribute(clientType, directAccessGrantsEnabled, super::setDirectAccessGrantsEnabled, Boolean.class); } @Override public boolean isAlwaysDisplayInConsole() { - return TypedClientAttribute.ALWAYS_DISPLAY_IN_CONSOLE + return TypedClientSimpleAttribute.ALWAYS_DISPLAY_IN_CONSOLE .getClientAttribute(clientType, super::isAlwaysDisplayInConsole, Boolean.class); } @Override public void setAlwaysDisplayInConsole(boolean alwaysDisplayInConsole) { - TypedClientAttribute.ALWAYS_DISPLAY_IN_CONSOLE + TypedClientSimpleAttribute.ALWAYS_DISPLAY_IN_CONSOLE .setClientAttribute(clientType, alwaysDisplayInConsole, super::setAlwaysDisplayInConsole, Boolean.class); } @Override public boolean isFrontchannelLogout() { - return TypedClientAttribute.FRONTCHANNEL_LOGOUT + return TypedClientSimpleAttribute.FRONTCHANNEL_LOGOUT .getClientAttribute(clientType, super::isFrontchannelLogout, Boolean.class); } @Override public void setFrontchannelLogout(boolean frontchannelLogout) { - TypedClientAttribute.FRONTCHANNEL_LOGOUT + TypedClientSimpleAttribute.FRONTCHANNEL_LOGOUT .setClientAttribute(clientType, frontchannelLogout, super::setFrontchannelLogout, Boolean.class); } @Override public boolean isImplicitFlowEnabled() { - return TypedClientAttribute.IMPLICIT_FLOW_ENABLED + return TypedClientSimpleAttribute.IMPLICIT_FLOW_ENABLED .getClientAttribute(clientType, super::isImplicitFlowEnabled, Boolean.class); } @Override public void setImplicitFlowEnabled(boolean implicitFlowEnabled) { - TypedClientAttribute.IMPLICIT_FLOW_ENABLED + TypedClientSimpleAttribute.IMPLICIT_FLOW_ENABLED .setClientAttribute(clientType, implicitFlowEnabled, super::setImplicitFlowEnabled, Boolean.class); } @Override public boolean isServiceAccountsEnabled() { - return TypedClientAttribute.SERVICE_ACCOUNTS_ENABLED + return TypedClientSimpleAttribute.SERVICE_ACCOUNTS_ENABLED .getClientAttribute(clientType, super::isServiceAccountsEnabled, Boolean.class); } @Override public void setServiceAccountsEnabled(boolean flag) { - TypedClientAttribute.SERVICE_ACCOUNTS_ENABLED + TypedClientSimpleAttribute.SERVICE_ACCOUNTS_ENABLED .setClientAttribute(clientType, flag, super::setServiceAccountsEnabled, Boolean.class); } @Override public String getProtocol() { - return TypedClientAttribute.PROTOCOL + return TypedClientSimpleAttribute.PROTOCOL .getClientAttribute(clientType, super::getProtocol, String.class); } @Override public void setProtocol(String protocol) { - TypedClientAttribute.PROTOCOL + TypedClientSimpleAttribute.PROTOCOL .setClientAttribute(clientType, protocol, super::setProtocol, String.class); } @Override public boolean isPublicClient() { - return TypedClientAttribute.PUBLIC_CLIENT + return TypedClientSimpleAttribute.PUBLIC_CLIENT .getClientAttribute(clientType, super::isPublicClient, Boolean.class); } @Override public void setPublicClient(boolean flag) { - TypedClientAttribute.PUBLIC_CLIENT + TypedClientSimpleAttribute.PUBLIC_CLIENT .setClientAttribute(clientType, flag, super::setPublicClient, Boolean.class); } @Override public Set getWebOrigins() { - return TypedClientAttribute.WEB_ORIGINS + return TypedClientSimpleAttribute.WEB_ORIGINS .getClientAttribute(clientType, super::getWebOrigins, Set.class); } @Override public void setWebOrigins(Set webOrigins) { - TypedClientAttribute.WEB_ORIGINS + TypedClientSimpleAttribute.WEB_ORIGINS .setClientAttribute(clientType, webOrigins, super::setWebOrigins, Set.class); } @Override public void addWebOrigin(String webOrigin) { - TypedClientAttribute.WEB_ORIGINS + TypedClientSimpleAttribute.WEB_ORIGINS .setClientAttribute(clientType, webOrigin, super::addWebOrigin, String.class); } @Override public void removeWebOrigin(String webOrigin) { - TypedClientAttribute.WEB_ORIGINS + TypedClientSimpleAttribute.WEB_ORIGINS .setClientAttribute(clientType, null, (val) -> super.removeWebOrigin(webOrigin), String.class); } @Override public Set getRedirectUris() { - return TypedClientAttribute.REDIRECT_URIS + return TypedClientSimpleAttribute.REDIRECT_URIS .getClientAttribute(clientType, super::getRedirectUris, Set.class); } @Override public void setRedirectUris(Set redirectUris) { - TypedClientAttribute.REDIRECT_URIS + TypedClientSimpleAttribute.REDIRECT_URIS .setClientAttribute(clientType, redirectUris, super::setRedirectUris, Set.class); } @Override public void addRedirectUri(String redirectUri) { - TypedClientAttribute.REDIRECT_URIS + TypedClientSimpleAttribute.REDIRECT_URIS .setClientAttribute(clientType, redirectUri, super::addRedirectUri, String.class); } @Override public void removeRedirectUri(String redirectUri) { - TypedClientAttribute.REDIRECT_URIS + TypedClientSimpleAttribute.REDIRECT_URIS .setClientAttribute(clientType, null, (val) -> super.removeRedirectUri(redirectUri), String.class); } diff --git a/services/src/main/java/org/keycloak/services/clienttype/client/TypedClientAttribute.java b/services/src/main/java/org/keycloak/services/clienttype/client/TypedClientSimpleAttribute.java similarity index 94% rename from services/src/main/java/org/keycloak/services/clienttype/client/TypedClientAttribute.java rename to services/src/main/java/org/keycloak/services/clienttype/client/TypedClientSimpleAttribute.java index bf026c426b8..14d615c23b2 100644 --- a/services/src/main/java/org/keycloak/services/clienttype/client/TypedClientAttribute.java +++ b/services/src/main/java/org/keycloak/services/clienttype/client/TypedClientSimpleAttribute.java @@ -13,7 +13,7 @@ import java.util.function.Consumer; import java.util.function.Supplier; -enum TypedClientAttribute implements TypedClientAttributeInterface { +enum TypedClientSimpleAttribute implements TypedClientAttribute { // Top Level client attributes STANDARD_FLOW_ENABLED("standardFlowEnabled", false), BEARER_ONLY("bearerOnly", false), @@ -32,7 +32,7 @@ enum TypedClientAttribute implements TypedClientAttributeInterface { private final String propertyName; private final Object nonApplicableValue; - TypedClientAttribute(String propertyName, Object nonApplicableValue) { + TypedClientSimpleAttribute(String propertyName, Object nonApplicableValue) { this.propertyName = propertyName; this.nonApplicableValue = nonApplicableValue; } @@ -48,7 +48,7 @@ public Object getNonApplicableValue() { } } -enum TypedClientExtendedAttribute implements TypedClientAttributeInterface { +enum TypedClientExtendedAttribute implements TypedClientAttribute { // Extended Client Type attributes defined as client attribute entities. DEVICE_AUTHORIZATION_GRANT_ENABLED("oauth2.device.authorization.grant.enabled", "false"), CIBA_GRANT_ENABLED("oidc.ciba.grant.enabled", "false"), @@ -86,8 +86,8 @@ public static Map getAttributesByName() { } } -interface TypedClientAttributeInterface { - Logger logger = Logger.getLogger(TypedClientAttributeInterface.class); +interface TypedClientAttribute { + Logger logger = Logger.getLogger(TypedClientAttribute.class); default T getClientAttribute(ClientType clientType, Supplier clientGetter, Class tClass) { String propertyName = getPropertyName(); From 8f917d935fb2786292d38f4c527f0922c0ab86e9 Mon Sep 17 00:00:00 2001 From: Patrick Jennings Date: Tue, 30 Apr 2024 16:47:52 -0400 Subject: [PATCH 14/15] Shall be able to override URI sets with an empty set. Signed-off-by: Patrick Jennings --- .../java/org/keycloak/models/utils/RepresentationToModel.java | 1 - 1 file changed, 1 deletion(-) 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 4c33909abd3..466aff4f0fe 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 @@ -1635,7 +1635,6 @@ public static ResourceServer createResourceServer(ClientModel client, KeycloakSe private static Set collectionToSet(Collection collection) { return Optional.ofNullable(collection) - .filter(col -> !col.isEmpty()) .map(HashSet::new) .orElse(null); } From 188e36db2ea11cbd3f4e45a0a7431cef136e8fe5 Mon Sep 17 00:00:00 2001 From: Patrick Jennings Date: Wed, 1 May 2024 14:19:48 -0400 Subject: [PATCH 15/15] Comments around fields that are primitive and may cause problems determining whether to set sane default on create. Signed-off-by: Patrick Jennings --- .../models/utils/RepresentationToModel.java | 23 ++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) 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 466aff4f0fe..254e91e63a1 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 @@ -18,11 +18,11 @@ package org.keycloak.models.utils; import java.io.IOException; -import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; +import java.util.LinkedList; import java.util.List; import java.util.ListIterator; import java.util.Map; @@ -490,7 +490,7 @@ public KeycloakSession getKeycloakSession() { * @param isNew Whether the client is new or not (needed because some getters cannot return null). */ private static void updateClientProperties(ClientModel client, ClientRepresentation rep, boolean isNew) { - List> clientPropertyUpdates = new ArrayList>() {{ + List> clientPropertyUpdates = new LinkedList>() {{ /** * Values from the ClientRepresentation take precedence. * Then values from the ClientModel (these may be augmented from the client type configuration). @@ -516,7 +516,7 @@ private static void updateClientProperties(ClientModel client, ClientRepresentat add(updatePropertyAction(client::setNotBefore, rep::getNotBefore, client::getNotBefore)); // Fields with defaults if not initially provided add(updatePropertyAction(client::setProtocol, rep::getProtocol, client::getProtocol, () -> OIDC)); - add(updatePropertyAction(client::setNodeReRegistrationTimeout, rep::getNodeReRegistrationTimeout, () -> defaultNodeReRegistraionTimeout(client, isNew))); + add(updatePropertyAction(client::setNodeReRegistrationTimeout, rep::getNodeReRegistrationTimeout, () -> defaultNodeReRegistrationTimeout(client, isNew))); add(updatePropertyAction(client::setClientAuthenticatorType, rep::getClientAuthenticatorType, client::getClientAuthenticatorType, KeycloakModelUtils::getDefaultClientAuthenticatorType)); add(updatePropertyAction(client::setFullScopeAllowed, rep::isFullScopeAllowed, () -> defaultFullScopeAllowed(client, isNew))); // Client Secret @@ -548,14 +548,21 @@ private static void updateClientProperties(ClientModel client, ClientRepresentat } private static Boolean defaultFullScopeAllowed(ClientModel client, boolean isNew) { - return isNew ? !client.isConsentRequired() : client.isFullScopeAllowed(); + // If the client is newly created and the value is set to true, the value must be augmented through a + // client type configuration, so do not update. Yet if new and false, the field MAY be controlled through a + // client type. We can only attempt to set the sane default based on consent required. + return isNew && !client.isFullScopeAllowed() ? !client.isConsentRequired() : client.isFullScopeAllowed(); } - private static Integer defaultNodeReRegistraionTimeout(ClientModel client, boolean isNew) { - if (!ObjectUtil.isEqualOrBothNull(client.getNodeReRegistrationTimeout(), 0) || !isNew) { - return client.getNodeReRegistrationTimeout(); + private static Integer defaultNodeReRegistrationTimeout(ClientModel client, boolean isNew) { + // If the client is newly created and the value is 0, the client MAY not be augmented with a client type. + if (isNew && Objects.equals(client.getNodeReRegistrationTimeout(), 0)) { + // Return the sane default. + return -1; } - return -1; + // Update client request without an overriding value or is new and augmented with a client type, + // do not attempt to update. + return client.getNodeReRegistrationTimeout(); } private static String determineNewSecret(ClientModel client, ClientRepresentation rep) {