ESB-1218 Modified Keycloak plugin XML configuration

keycloak-plugin/README.md

@@ -27,8 +27,8 @@ This plugin doesn't come with Role and Group management, because Entando Core ro
 >- `keycloak.authenticated.user.default.authorizations`: **[OPTIONAL]** Use if you want to automatically assign `group:role` to any user that logs in, comma separated. Example: `administrators:admin,readers`
 
 ## Environment variables
->- `KC_CONFIG_REFRESH`: specifies the refresh period -in cron style!- of the dynamic configuration used to assign authorizations to the loggin-in users. The default is `0 * * * * *`
->- `KC_SYNC_CLEAN`: Specify the periodicity of the internal synchronization table cleanup. The default is `0 0 0/4 * * *`
+>- `KC_CONFIG_REFRESH`: specifies the refresh period -in cron style!- of the dynamic configuration used to assign authorizations to the loggin-in users. The default is `0 0/15 * * * *` (every 15 minutes)
+>- `KC_SYNC_CLEAN`: specifies the periodicity -in cron style!- of the internal synchronization table cleanup. **Disabled by default** (`0 0 0 31 2 *`, a date that never occurs); set a real cron expression to enable it.
 >- `KC_SYNC_BATCH_SIZE`: Specify the batch size for the internal synchronization table cleanup. The default is `100`
 
 

keycloak-plugin/src/main/java/org/entando/entando/keycloak/services/KeycloakAuthorizationManager.java

@@ -87,7 +87,7 @@ public KeycloakAuthorizationManager(final KeycloakConfiguration configuration,
     /**
      * Immutable list of mapping elements that are currently active. The configuration is constantly updated
      * either by reloading the global configuration or after a certain amount of time (by default,
-     * one minute)
+     * 15 minutes — see {@code KC_CONFIG_REFRESH})
      */
     private final transient Map<String, KeycloakImportConfig> config = new ConcurrentHashMap<>();
 
@@ -102,7 +102,6 @@ public void initTenantAware() throws Exception {
 
         List<DynamicMappingElement> profileMappings = new ArrayList<>();
         List<DynamicMappingElement> jwtMappings = new ArrayList<>();
-        List<String> ignore = new ArrayList<>();
         List<String> roles = new ArrayList<>();
         List<String> groups = new ArrayList<>();
         List<String> excludeUsers = new ArrayList<>();
@@ -115,6 +114,7 @@ public void initTenantAware() throws Exception {
                 DynamicMapping dynConf = xmlMapper.readValue(xml, DynamicMapping.class);
                 if (dynConf != null) {
                     if (dynConf.mapping != null) {
+                        dynConf.mapping.forEach(this::normalizeMappingElement);
 
                         final Map<Boolean, List<DynamicMappingElement>> partitioned =
                                 dynConf.mapping.stream()
@@ -128,14 +128,9 @@ public void initTenantAware() throws Exception {
                         log.debug("{} dynamic auth mapping found, {} getConfig().profileMappings",
                                 jwtMappings.size(), profileMappings.size());
                     }
-                    ignore = ofNullable(dynConf.exclusions)
-                            .orElse(List.of());
-                    roles = ofNullable(dynConf.roles)
-                            .orElseGet(List::of);
-                    groups = ofNullable(dynConf.groups)
-                            .orElse(List.of());
-                    excludeUsers = ofNullable(dynConf.excludeUsers)
-                            .orElse(List.of());
+                    roles = normalizeNames(dynConf.roles);
+                    groups = normalizeNames(dynConf.groups);
+                    excludeUsers = normalizeNames(dynConf.excludeUsers);
                     enabled = ofNullable(dynConf.enabled)
                             .orElse(false);
                     persist = ofNullable(dynConf.persist)
@@ -149,12 +144,12 @@ public void initTenantAware() throws Exception {
                 jwtMappings.forEach(m -> log.debug("jwt mapping active: {}", m.toString()));
             }
             // finally
-            KeycloakImportConfig cfg = new KeycloakImportConfig(profileMappings, jwtMappings, ignore, roles, groups, enabled, persist, excludeUsers);
+            KeycloakImportConfig cfg = new KeycloakImportConfig(profileMappings, jwtMappings, roles, groups, enabled, persist, excludeUsers);
 
             setImportConfiguration(cfg);
         } catch (Exception e) {
             log.error("Error initializing KeycloakAuthorizationManager", e);
-            KeycloakImportConfig cfg = new KeycloakImportConfig(List.of(), List.of(), List.of(), List.of(), List.of(), false, PersistKind.NONE, List.of());
+            KeycloakImportConfig cfg = new KeycloakImportConfig(List.of(), List.of(), List.of(), List.of(), false, PersistKind.NONE, List.of());
 
             setImportConfiguration(cfg);
             throw e;
@@ -190,6 +185,43 @@ public void cleanSyncData() {
         }
     }
 
+    /**
+     * Normalize a configured list of role, group or user names: surrounding whitespace is stripped,
+     * blank entries are dropped and duplicates are collapsed. Names are compared against the values
+     * extracted from the JWT or the user profile, which are normalized the same way, so that a
+     * stray space in the configuration cannot silently disable an entry.
+     *
+     * @param names the raw configured names, possibly null
+     * @return an immutable, normalized list; never null
+     */
+    private List<String> normalizeNames(List<String> names) {
+        return ofNullable(names)
+                .orElseGet(List::of)
+                .stream()
+                .filter(Objects::nonNull)
+                .map(String::trim)
+                .filter(StringUtils::isNotBlank)
+                .distinct()
+                .toList();
+    }
+
+    /**
+     * Trim the free-text fields of a mapping element in place. A stray leading/trailing space or
+     * newline pasted into {@code <attribute>}, {@code <path>} or {@code <separator>} would otherwise
+     * never match the real Keycloak attribute key, JWT claim path or role-group token, silently
+     * turning the whole mapping into a no-op instead of a validation failure.
+     *
+     * @param elem the mapping element to normalize, possibly null
+     */
+    private void normalizeMappingElement(DynamicMappingElement elem) {
+        if (elem == null) {
+            return;
+        }
+        elem.attribute = StringUtils.trim(elem.attribute);
+        elem.path = StringUtils.trim(elem.path);
+        elem.separator = StringUtils.trim(elem.separator);
+    }
+
     /**
      * Check whether the dynamic configuration element provided is valid
      * @param elem the single dynamic mapping element to validate
@@ -236,7 +268,8 @@ public void processNewUser(final UserDetails user, final String token, final boo
         readLock.lock();
         try {
             final List<String> excludedUsers = getImportConfiguration().getExcludeUsers();
-            if (excludedUsers != null && user.getUsername() != null && excludedUsers.contains(user.getUsername())) {
+            if (excludedUsers != null && user.getUsername() != null
+                    && excludedUsers.contains(user.getUsername().trim())) {
                 log.debug("user {} is in the excludeUsers list, skipping dynamic sync", user.getUsername());
                 return;
             }
@@ -576,30 +609,27 @@ private Authorization finalizeAssociation(KeycloakUser user, DynamicMappingEleme
         return finalizeAssociation(user, roleName, groupName, true);
     }
 
-    private boolean isIgnored(String name) {
-        if (getImportConfiguration().getIgnore() == null || StringUtils.isBlank(name)) {
-            return false;
-        }
-        return getImportConfiguration().getIgnore().contains(name.trim());
-    }
-
     private Authorization finalizeAssociation(KeycloakUser user, String roleName, String groupName,
             boolean createRoleIfMissing) {
-        // is it excluded?
-        if (isIgnored(roleName) || isIgnored(groupName)) {
-            log.info("Role {} or Group {} is in the exclusions list. Skipping assignment for user {}", roleName, groupName, user.getUsername());
+        // names are normalized here so that every source (JWT claim, profile attribute, role-group
+        // token) is compared against the allowlists on the same terms
+        final String role = StringUtils.trimToNull(roleName);
+        final String group = StringUtils.trimToNull(groupName);
+
+        if (role == null && group == null) {
+            log.warn("Blank role and group extracted for user {}, discarding", user.getUsername());
             return null;
         }
         // are they managed?
-        if (StringUtils.isNotBlank(roleName) && !getImportConfiguration().getRoles().contains(roleName)) {
-            log.warn("Role {} is not in the roles allowlist. Skipping assignment for user {}", roleName, user.getUsername());
+        if (StringUtils.isNotBlank(role) && !getImportConfiguration().getRoles().contains(role)) {
+            log.warn("Role {} is not in the roles allowlist. Skipping assignment for user {}", role, user.getUsername());
             return null;
         }
-        if (StringUtils.isNotBlank(groupName) && !getImportConfiguration().getGroups().contains(groupName)) {
-            log.warn("Group {} is not in the groups allowlist. Skipping assignment for user {}", groupName, user.getUsername());
+        if (StringUtils.isNotBlank(group) && !getImportConfiguration().getGroups().contains(group)) {
+            log.warn("Group {} is not in the groups allowlist. Skipping assignment for user {}", group, user.getUsername());
             return null;
         }
-        return createAuthorization(roleName, groupName, createRoleIfMissing);
+        return createAuthorization(role, group, createRoleIfMissing);
     }
 
     private Authorization createAuthorization(String roleName, String groupName, boolean createRoleIfMissing) {

keycloak-plugin/src/main/java/org/entando/entando/keycloak/services/KeycloakImportConfig.java

@@ -15,7 +15,6 @@ public class KeycloakImportConfig {
 
     private transient List<DynamicMappingElement> profileMappings = new ArrayList<>();
     private transient List<DynamicMappingElement> jwtMappings =  new ArrayList<>();
-    private transient List<String> ignore;
     private transient List<String> roles;
     private transient List<String> groups;
     private transient Boolean enabled;

keycloak-plugin/src/main/java/org/entando/entando/keycloak/services/mapping/DynamicMapping.java

@@ -2,6 +2,7 @@
 
 import com.fasterxml.jackson.annotation.JsonInclude;
 import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlElementWrapper;
+import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlProperty;
 import com.fasterxml.jackson.dataformat.xml.annotation.JacksonXmlRootElement;
 import java.util.List;
 
@@ -10,21 +11,27 @@
 public class DynamicMapping {
 
     @JacksonXmlElementWrapper(localName = "mappings")
+    @JacksonXmlProperty(localName = "mapping")
     public List<DynamicMappingElement> mapping;
 
-    @JacksonXmlElementWrapper(localName = "exclusions")
-    public List<String> exclusions;
-
     @JacksonXmlElementWrapper(localName = "roles")
+    @JacksonXmlProperty(localName = "role")
     public List<String> roles;
 
     @JacksonXmlElementWrapper(localName = "groups")
+    @JacksonXmlProperty(localName = "group")
     public List<String> groups;
 
     @JacksonXmlElementWrapper(localName = "excludeUsers")
+    @JacksonXmlProperty(localName = "excludeUser")
     public List<String> excludeUsers;
 
     public Boolean enabled;
     public PersistKind persist;
 
+    @Deprecated(forRemoval = true)
+    @JacksonXmlElementWrapper(localName = "exclusions")
+    @JacksonXmlProperty(localName = "exclusion")
+    public List<String> exclusions;
+
 }

keycloak-plugin/src/main/java/org/entando/entando/keycloak/services/mapping/DynamicMappingKind.java

@@ -4,6 +4,7 @@
 import com.fasterxml.jackson.annotation.JsonValue;
 import java.util.Arrays;
 import lombok.Getter;
+import org.apache.commons.lang3.StringUtils;
 
 public enum DynamicMappingKind {
 
@@ -30,8 +31,9 @@ public String getXmlValue() {
 
     @JsonCreator
     public static DynamicMappingKind fromValue(String value) {
+        final String trimmed = StringUtils.trim(value);
         return Arrays.stream(values())
-                .filter(k -> k.kind.equalsIgnoreCase(value))
+                .filter(k -> k.kind.equalsIgnoreCase(trimmed))
                 .findFirst()
                 .orElseThrow(() ->
                         new IllegalArgumentException("Unknown DynamicMappingKind: " + value));

keycloak-plugin/src/main/java/org/entando/entando/keycloak/services/mapping/PersistKind.java

@@ -3,6 +3,7 @@
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonValue;
 import java.util.Arrays;
+import org.apache.commons.lang3.StringUtils;
 
 public enum PersistKind {
     NONE("none"),
@@ -24,8 +25,9 @@ public String getXmlValue() {
 
     @JsonCreator
     public static PersistKind fromValue(String value) {
+        final String trimmed = StringUtils.trim(value);
         return Arrays.stream(values())
-                .filter(k -> k.kind.equalsIgnoreCase(value))
+                .filter(k -> k.kind.equalsIgnoreCase(trimmed))
                 .findFirst()
                 .orElseThrow(() ->
                         new IllegalArgumentException("Unknown PersistKind: " + value));

keycloak-plugin/src/main/java/org/entando/entando/keycloak/services/oidc/OidcMappingHelper.java

@@ -8,7 +8,6 @@
 import java.util.Base64;
 import java.util.Collections;
 import java.util.List;
-import java.util.stream.Collectors;
 import org.entando.entando.ent.util.EntLogging.EntLogFactory;
 import org.entando.entando.ent.util.EntLogging.EntLogger;
 import org.entando.entando.keycloak.services.mapping.DynamicMappingElement;
@@ -100,21 +99,26 @@ private static List<String> extractAuthorizationsFromNode(JsonNode authNode, Dyn
         if (authNode.isArray()) {
             return extractFromArrayNode(authNode);
         }
-        if (authNode.isTextual()) {
-            return List.of(authNode.asText());
-        }
-        if (authNode.isNumber()) {
-            return List.of(authNode.asText());
+        if (authNode.isTextual() || authNode.isNumber()) {
+            final String value = authNode.asText().trim();
+            return value.isEmpty() ? Collections.emptyList() : List.of(value);
         }
         log.warn("Unsupported node type for path '{}' in JWT: {}", claimMapper.path, authNode.getNodeType());
         return Collections.emptyList();
     }
 
+    /**
+     * Collect the textual elements of a claim array, discarding blank ones. A blank name cannot be
+     * matched against the allowlists, so keeping it would only produce an empty authorization.
+     *
+     * @param arrayNode the claim array node
+     * @return the trimmed, non-blank values
+     */
     private static List<String> extractFromArrayNode(JsonNode arrayNode) {
         List<String> authorizations = new ArrayList<>();
         for (JsonNode node : arrayNode) {
-            if (node.isTextual()) {
-                authorizations.add(node.asText());
+            if (node.isTextual() && !node.asText().isBlank()) {
+                authorizations.add(node.asText().trim());
             }
         }
         return authorizations;

keycloak-plugin/src/main/resources/liquibase/entando-keycloak-auth/changeSetPort.xml

@@ -6,5 +6,6 @@
            http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-4.4.xsd">
 
     <include file="port/00000000000003_dataPort_production.xml" relativeToChangelogFile="true" />
+    <include file="port/00000000000004_dataPort_production_removeExclusions.xml" relativeToChangelogFile="true" />
 
 </databaseChangeLog>

keycloak-plugin/src/main/resources/liquibase/entando-keycloak-auth/port/00000000000004_dataPort_production_removeExclusions.xml

@@ -0,0 +1,99 @@
+<databaseChangeLog
+        xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
+        xmlns:ext="http://www.liquibase.org/xml/ns/dbchangelog-ext"
+        xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+        xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog-ext http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-ext.xsd
+           http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-4.4.xsd">
+
+    <!--
+        The <exclusions> element of the dynamicAuthMapping seed value is retired : it is parsed
+        and ignored, never enforced.
+        Editing port/clob/production/sysconfig_kc.xml in place would NOT update databases that
+        already ran the 00000000000001_dataPort_production_v3 insert - Liquibase does not detect a
+        checksum change when only the referenced valueClobFile content changes - so the row would
+        silently keep the old value forever. This changeSet updates it explicitly instead.
+
+        The precondition matches only if the stored config is BYTE-FOR-BYTE identical to the exact,
+        unmodified value that changeSet 00000000000001_dataPort_production_v3 originally inserted.
+        This is deliberately conservative:
+          - fresh install: the insert changeSet writes the original value, this one then rewrites it
+            to the no-exclusions value in the same run;
+          - already-migrated, untouched environment: same rewrite happens on upgrade;
+          - ANY environment where the row differs in any way from the pristine shipped default
+            (an operator customized roles/groups/mappings, or even just re-saved it through the
+            admin UI) is left completely untouched - onFail="MARK_RAN" skips the update rather than
+            risk overwriting a customization. <exclusions> lingering there is harmless dead weight,
+            not a correctness issue (see DynamicMapping.java).
+        Uses Liquibase's own <update>/valueClobFile mechanism (not raw SQL string functions) so the
+        SET side is generated per-dialect by Liquibase itself - no REPLACE()/regex portability
+        concerns across Derby/PostgreSQL/MySQL/H2.
+        Re-running this changeSet is a no-op the second time: after the first successful run the
+        stored value no longer matches the precondition, so it cannot match again.
+    -->
+    <changeSet id="00000000000004_dataPort_production_removeExclusions" author="entando" context="production">
+        <preConditions onFail="MARK_RAN">
+            <sqlCheck expectedResult="1"><![CDATA[
+SELECT COUNT(*) FROM sysconfig
+WHERE version = 'production' AND item = 'dynamicAuthMapping'
+AND CAST(config AS VARCHAR(4000)) = '<?xml version="1.0" encoding="UTF-8"?>
+<DynamicMapping>
+    <enabled>false</enabled>
+    <persist>FULL</persist>
+    <mappings>
+        <mapping>
+            <enabled>true</enabled>
+            <path>groups</path>
+            <kind>GROUPCLAIM</kind>
+        </mapping>
+        <mapping>
+            <enabled>true</enabled>
+            <path>realm_access.roles</path>
+            <kind>ROLECLAIM</kind>
+        </mapping>
+        <mapping>
+            <enabled>false</enabled>
+            <path>realm_access.roles</path>
+            <kind>ROLEGROUPCLAIM</kind>
+            <separator>_SEP_</separator>
+        </mapping>
+        <mapping>
+            <enabled>false</enabled>
+            <attribute>AD_ROLE</attribute>
+            <kind>ROLE</kind>
+        </mapping>
+        <mapping>
+            <enabled>false</enabled>
+            <attribute>AD_GROUP</attribute>
+            <kind>GROUP</kind>
+        </mapping>
+        <mapping>
+            <enabled>false</enabled>
+            <attribute>AD_GROUPROLE</attribute>
+            <kind>ROLEGROUP</kind>
+            <separator>_r_</separator>
+        </mapping>
+    </mappings>
+    <exclusions>
+        <exclusion>default-roles-entando-development</exclusion>
+        <exclusion>offline_access</exclusion>
+        <exclusion>uma_authorization</exclusion>
+    </exclusions>
+    <roles>
+        <role>imported_role</role>
+        <role>imported_role2</role>
+    </roles>
+    <groups>
+        <group>imported_group</group>
+        <group>imported_group2</group>
+    </groups>
+</DynamicMapping>
+'
+            ]]></sqlCheck>
+        </preConditions>
+        <update tableName="sysconfig">
+            <column name="config" valueClobFile="clob/production/sysconfig_kc_no_exclusions.xml" />
+            <where>version = 'production' AND item = 'dynamicAuthMapping'</where>
+        </update>
+    </changeSet>
+
+</databaseChangeLog>