Fix client-attributes condition configuration

closes #33390

Signed-off-by: mposolda <mposolda@gmail.com>

server-spi/src/main/java/org/keycloak/models/IdentityProviderMapperModel.java

@@ -17,17 +17,11 @@
 
 package org.keycloak.models;
 
-import com.fasterxml.jackson.core.type.TypeReference;
+import org.keycloak.models.utils.MapperTypeSerializer;
-import org.keycloak.util.JsonSerialization;
 
-import java.io.IOException;
 import java.io.Serializable;
-import java.util.Collections;
 import java.util.List;
 import java.util.Map;
-import java.util.stream.Collectors;
-
-import static java.util.Collections.emptyMap;
 
 /**
  * Specifies a mapping from broker login to user data.
@@ -38,9 +32,6 @@ import static java.util.Collections.emptyMap;
 public class IdentityProviderMapperModel implements Serializable {
     public static final String SYNC_MODE = "syncMode";
 
-    private static final TypeReference<List<StringPair>> MAP_TYPE_REPRESENTATION = new TypeReference<List<StringPair>>() {
-    };
-
     protected String id;
     protected String name;
     protected String identityProviderAlias;
@@ -98,20 +89,7 @@ public class IdentityProviderMapperModel implements Serializable {
 
     public Map<String, List<String>> getConfigMap(String configKey) {
         String configMap = config.get(configKey);
-        if (configMap == null) {
+        return MapperTypeSerializer.deserialize(configMap);
-            return emptyMap();
-        }
-
-        try {
-            List<StringPair> map = JsonSerialization.readValue(configMap, MAP_TYPE_REPRESENTATION);
-            return map.stream().collect(
-                    Collectors.collectingAndThen(
-                            Collectors.groupingBy(StringPair::getKey,
-                                    Collectors.mapping(StringPair::getValue, Collectors.toUnmodifiableList())),
-                            Collections::unmodifiableMap));
-        } catch (IOException e) {
-            throw new RuntimeException("Could not deserialize json: " + configMap, e);
-        }
     }
 
     @Override
@@ -130,25 +108,4 @@ public class IdentityProviderMapperModel implements Serializable {
     public int hashCode() {
         return id.hashCode();
     }
-
-    static class StringPair {
-        private String key;
-        private String value;
-
-        public String getKey() {
-            return key;
-        }
-
-        public void setKey(String key) {
-            this.key = key;
-        }
-
-        public String getValue() {
-            return value;
-        }
-
-        public void setValue(String value) {
-            this.value = value;
-        }
-    }
 }

server-spi/src/main/java/org/keycloak/models/utils/MapperTypeSerializer.java

@@ -0,0 +1,106 @@
+/*
+ * Copyright 2024 Red Hat, Inc. and/or its affiliates
+ *  and other contributors as indicated by the @author tags.
+ *
+ *  Licensed under the Apache License, Version 2.0 (the "License");
+ *  you may not use this file except in compliance with the License.
+ *  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ *
+ */
+
+package org.keycloak.models.utils;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;
+
+import com.fasterxml.jackson.core.type.TypeReference;
+import org.keycloak.util.JsonSerialization;
+
+import static java.util.Collections.emptyMap;
+
+/**
+ * Serializer and deserializer for {@link org.keycloak.provider.ProviderConfigProperty#MAP_TYPE}
+ *
+ * @author <a href="mailto:mposolda@redhat.com">Marek Posolda</a>
+ */
+public class MapperTypeSerializer {
+
+    private static final TypeReference<List<StringPair>> MAP_TYPE_REPRESENTATION = new TypeReference<>() {
+    };
+
+    public static Map<String, List<String>> deserialize(String configString) {
+        if (configString == null) {
+            return emptyMap();
+        }
+
+        try {
+            List<StringPair> map = JsonSerialization.readValue(configString, MAP_TYPE_REPRESENTATION);
+            return map.stream().collect(
+                    Collectors.collectingAndThen(
+                            Collectors.groupingBy(StringPair::getKey,
+                                    Collectors.mapping(StringPair::getValue, Collectors.toUnmodifiableList())),
+                            Collections::unmodifiableMap));
+        } catch (IOException e) {
+            throw new RuntimeException("Could not deserialize json: " + configString, e);
+        }
+    }
+
+    public static String serialize(Map<String, List<String>> config) {
+        List<StringPair> pairs = config.entrySet()
+                .stream()
+                .flatMap(entry -> {
+                    String key = entry.getKey();
+                    List<String> values = entry.getValue();
+                    return values
+                            .stream()
+                            .map(value -> new StringPair(key, value));
+                })
+                .toList();
+        try {
+            return JsonSerialization.writeValueAsString(pairs);
+        } catch (IOException e) {
+            throw new RuntimeException("Could not serialize json: " + config, e);
+        }
+    }
+
+    static class StringPair {
+        private String key;
+        private String value;
+
+        public StringPair() {
+        }
+
+        private StringPair(String key, String value) {
+            this.key = key;
+            this.value = value;
+        }
+
+        public String getKey() {
+            return key;
+        }
+
+        public void setKey(String key) {
+            this.key = key;
+        }
+
+        public String getValue() {
+            return value;
+        }
+
+        public void setValue(String value) {
+            this.value = value;
+        }
+    }
+}

server-spi/src/test/java/org/keycloak/utils/MapperTypeSerializerTest.java

@@ -0,0 +1,75 @@
+/*
+ * Copyright 2024 Red Hat, Inc. and/or its affiliates
+ *  and other contributors as indicated by the @author tags.
+ *
+ *  Licensed under the Apache License, Version 2.0 (the "License");
+ *  you may not use this file except in compliance with the License.
+ *  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ *
+ */
+
+package org.keycloak.utils;
+
+import java.util.List;
+import java.util.Map;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.keycloak.common.util.MultivaluedHashMap;
+import org.keycloak.common.util.MultivaluedMap;
+import org.keycloak.models.utils.MapperTypeSerializer;
+
+/**
+ * @author <a href="mailto:mposolda@redhat.com">Marek Posolda</a>
+ */
+public class MapperTypeSerializerTest {
+
+    @Test
+    public void testBasicSerializeAndDeserialize() {
+        // Serialize
+        MultivaluedMap<String, String> simpleMap = new MultivaluedHashMap<>() {
+            {
+                putSingle("attr1", "Apple");
+                putSingle("attr2", "Orange");
+            }
+        };
+        String s = MapperTypeSerializer.serialize(simpleMap);
+
+        // Check after deserialize, it is equal to serialized
+        Map<String, List<String>> deserialized = MapperTypeSerializer.deserialize(s);
+        Assert.assertEquals(simpleMap, deserialized);
+
+        // Deserialize from String
+        deserialized = MapperTypeSerializer.deserialize("[{\"key\":\"attr2\",\"value\":\"Orange\"},{\"key\":\"attr1\",\"value\":\"Apple\"}]");
+        Assert.assertEquals(simpleMap, deserialized);
+    }
+
+    @Test
+    public void testMultivaluedSerializeAndDeserialize() {
+        // Deserialize with some multivalued value
+        Map<String, List<String>> deserialized = MapperTypeSerializer.deserialize("[{\"key\":\"attr2\",\"value\":\"Orange\"},{\"key\":\"attr1\",\"value\":\"Apple\"},{\"key\":\"attr2\",\"value\":\"Peach\"}]");
+        Assert.assertEquals(deserialized.get("attr1").size(), 1);
+        Assert.assertEquals(deserialized.get("attr1").get(0), "Apple");
+        Assert.assertEquals(deserialized.get("attr2").size(), 2);
+        Assert.assertTrue(deserialized.get("attr2").contains("Orange"));
+        Assert.assertTrue(deserialized.get("attr2").contains("Peach"));
+        Assert.assertFalse(deserialized.get("attr2").contains("Apple"));
+
+        // Serialize and deserialize again from String and check it is same value
+        String s = MapperTypeSerializer.serialize(deserialized);
+        Assert.assertEquals(MapperTypeSerializer.deserialize(s), deserialized);
+    }
+
+
+
+
+}

services/src/main/java/org/keycloak/services/clientpolicy/condition/ClientAttributesCondition.java

@@ -20,12 +20,14 @@ package org.keycloak.services.clientpolicy.condition;
 import org.jboss.logging.Logger;
 import org.keycloak.models.ClientModel;
 import org.keycloak.models.KeycloakSession;
+import org.keycloak.models.utils.MapperTypeSerializer;
 import org.keycloak.representations.idm.ClientPolicyConditionConfigurationRepresentation;
 import org.keycloak.services.clientpolicy.ClientPolicyContext;
 import org.keycloak.services.clientpolicy.ClientPolicyException;
 import org.keycloak.services.clientpolicy.ClientPolicyVote;
 import org.keycloak.services.clientpolicy.context.PreAuthorizationRequestContext;
 
+import java.util.List;
 import java.util.Map;
 
 /**
@@ -46,13 +48,13 @@ public class ClientAttributesCondition extends AbstractClientPolicyConditionProv
 
     public static class Configuration extends ClientPolicyConditionConfigurationRepresentation {
 
-        protected Map<String, String> attributes;
+        private String attributes;
 
-        public Map<String, String> getAttributes() {
+        public String getAttributes() {
             return attributes;
         }
 
-        public void setAttributes(Map<String, String> attributes) {
+        public void setAttributes(String attributes) {
             this.attributes = attributes;
         }
     }
@@ -100,7 +102,7 @@ public class ClientAttributesCondition extends AbstractClientPolicyConditionProv
     private boolean isAttributesMatched(ClientModel client) {
         if (client == null) return false;
 
-        Map<String, String> attributesForMatching = getAttributesForMatching();
+        Map<String, List<String>> attributesForMatching = getAttributesForMatching();
         if (attributesForMatching == null) return false;
 
         Map<String, String> clientAttributes = client.getAttributes();
@@ -111,12 +113,28 @@ public class ClientAttributesCondition extends AbstractClientPolicyConditionProv
         }
 
         return attributesForMatching.entrySet().stream()
-                .allMatch(entry -> clientAttributes.containsKey(entry.getKey()) && clientAttributes.get(entry.getKey()).equals(entry.getValue()));
+                .allMatch(entry -> {
+                    String key = entry.getKey();
+                    if (key == null) {
+                        logger.warnf("Empty key in configuration of client-attributes condition");
+                        return false;
+                    }
+                    if (entry.getValue() == null || entry.getValue().isEmpty()) {
+                        logger.warnf("Empty value in the configuration of client-attributes condition for the attribute %s. This cannot match any client", key);
+                        return false;
+                    }
+                    if (entry.getValue().size() > 1) {
+                        logger.warnf("More values in the configuration of client-attributes condition for the attribute %s. This cannot match any client", key);
+                        return false;
+                    }
+                    String value = entry.getValue().get(0);
+                    return clientAttributes.containsKey(key) && clientAttributes.get(key).equals(value);
+                });
     }
 
-    private Map<String, String> getAttributesForMatching() {
+    private Map<String, List<String>> getAttributesForMatching() {
         if (configuration.getAttributes() == null) return null;
-        return configuration.getAttributes();
+        return MapperTypeSerializer.deserialize(configuration.getAttributes());
     }
 
 }

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

@@ -46,6 +46,7 @@ import org.keycloak.authentication.authenticators.client.JWTClientSecretAuthenti
 import org.keycloak.authentication.authenticators.client.X509ClientAuthenticator;
 import org.keycloak.client.registration.ClientRegistrationException;
 import org.keycloak.common.Profile;
+import org.keycloak.common.util.MultivaluedHashMap;
 import org.keycloak.models.AdminRoles;
 import org.keycloak.models.Constants;
 import org.keycloak.models.OAuth2DeviceConfig;
@@ -399,10 +400,10 @@ public class ClientPoliciesConditionTest extends AbstractClientPoliciesTest {
         json = (new ClientPoliciesBuilder()).addPolicy(
                 (new ClientPolicyBuilder()).createPolicy(POLICY_NAME, "Het Eerste Beleid", Boolean.TRUE)
                         .addCondition(ClientAttributesConditionFactory.PROVIDER_ID,
-                                createClientAttributesConditionConfig(new HashMap<String, String>() {
+                                createClientAttributesConditionConfig(new MultivaluedHashMap<String, String>() {
                                     {
-                                        put("attr1", "Apple");
+                                        putSingle("attr1", "Apple");
-                                        put("attr2", "Orange");
+                                        putSingle("attr2", "Orange");
                                     }
                                 }))
                         .addProfile(PROFILE_NAME)

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/ClientPoliciesUtil.java

@@ -22,6 +22,7 @@ import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
 
 import org.keycloak.common.util.Base64Url;
+import org.keycloak.common.util.MultivaluedMap;
 import org.keycloak.crypto.AsymmetricSignatureSignerContext;
 import org.keycloak.crypto.ECDSASignatureSignerContext;
 import org.keycloak.crypto.KeyType;
@@ -33,6 +34,7 @@ import org.keycloak.jose.jwk.ECPublicJWK;
 import org.keycloak.jose.jwk.JWK;
 import org.keycloak.jose.jwk.RSAPublicJWK;
 import org.keycloak.jose.jws.JWSHeader;
+import org.keycloak.models.utils.MapperTypeSerializer;
 import org.keycloak.protocol.oidc.grants.ciba.clientpolicy.executor.SecureCibaAuthenticationRequestSigningAlgorithmExecutor;
 import org.keycloak.representations.dpop.DPoP;
 import org.keycloak.representations.idm.ClientPoliciesRepresentation;
@@ -406,9 +408,10 @@ public final class ClientPoliciesUtil {
         return config;
     }
 
-    public static ClientAttributesCondition.Configuration createClientAttributesConditionConfig(Map<String, String> attributes) {
+    public static ClientAttributesCondition.Configuration createClientAttributesConditionConfig(MultivaluedMap<String, String> attributes) {
         ClientAttributesCondition.Configuration config = new ClientAttributesCondition.Configuration();
-        config.setAttributes(attributes);
+        String attrsAsString = MapperTypeSerializer.serialize(attributes);
+        config.setAttributes(attrsAsString);
         return config;
     }