From e582a17a7cd16b42bd67783a3ea317139dab23f3 Mon Sep 17 00:00:00 2001 From: mposolda Date: Mon, 30 Sep 2024 21:47:37 +0200 Subject: [PATCH] Fix client-attributes condition configuration closes #33390 Signed-off-by: mposolda --- .../models/IdentityProviderMapperModel.java | 47 +------- .../models/utils/MapperTypeSerializer.java | 106 ++++++++++++++++++ .../utils/MapperTypeSerializerTest.java | 75 +++++++++++++ .../condition/ClientAttributesCondition.java | 32 ++++-- .../policies/ClientPoliciesConditionTest.java | 7 +- .../testsuite/util/ClientPoliciesUtil.java | 7 +- 6 files changed, 217 insertions(+), 57 deletions(-) create mode 100644 server-spi/src/main/java/org/keycloak/models/utils/MapperTypeSerializer.java create mode 100644 server-spi/src/test/java/org/keycloak/utils/MapperTypeSerializerTest.java diff --git a/server-spi/src/main/java/org/keycloak/models/IdentityProviderMapperModel.java b/server-spi/src/main/java/org/keycloak/models/IdentityProviderMapperModel.java index c28986c85a..ad73952162 100755 --- a/server-spi/src/main/java/org/keycloak/models/IdentityProviderMapperModel.java +++ b/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.util.JsonSerialization; +import org.keycloak.models.utils.MapperTypeSerializer; -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> MAP_TYPE_REPRESENTATION = new TypeReference>() { - }; - protected String id; protected String name; protected String identityProviderAlias; @@ -98,20 +89,7 @@ public class IdentityProviderMapperModel implements Serializable { public Map> getConfigMap(String configKey) { String configMap = config.get(configKey); - if (configMap == null) { - return emptyMap(); - } - - try { - List 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); - } + return MapperTypeSerializer.deserialize(configMap); } @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; - } - } } diff --git a/server-spi/src/main/java/org/keycloak/models/utils/MapperTypeSerializer.java b/server-spi/src/main/java/org/keycloak/models/utils/MapperTypeSerializer.java new file mode 100644 index 0000000000..6b0d18359b --- /dev/null +++ b/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 Marek Posolda + */ +public class MapperTypeSerializer { + + private static final TypeReference> MAP_TYPE_REPRESENTATION = new TypeReference<>() { + }; + + public static Map> deserialize(String configString) { + if (configString == null) { + return emptyMap(); + } + + try { + List 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> config) { + List pairs = config.entrySet() + .stream() + .flatMap(entry -> { + String key = entry.getKey(); + List 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; + } + } +} diff --git a/server-spi/src/test/java/org/keycloak/utils/MapperTypeSerializerTest.java b/server-spi/src/test/java/org/keycloak/utils/MapperTypeSerializerTest.java new file mode 100644 index 0000000000..e216e4b960 --- /dev/null +++ b/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 Marek Posolda + */ +public class MapperTypeSerializerTest { + + @Test + public void testBasicSerializeAndDeserialize() { + // Serialize + MultivaluedMap simpleMap = new MultivaluedHashMap<>() { + { + putSingle("attr1", "Apple"); + putSingle("attr2", "Orange"); + } + }; + String s = MapperTypeSerializer.serialize(simpleMap); + + // Check after deserialize, it is equal to serialized + Map> 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> 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); + } + + + + +} diff --git a/services/src/main/java/org/keycloak/services/clientpolicy/condition/ClientAttributesCondition.java b/services/src/main/java/org/keycloak/services/clientpolicy/condition/ClientAttributesCondition.java index 7ed8229348..143c6aefa2 100644 --- a/services/src/main/java/org/keycloak/services/clientpolicy/condition/ClientAttributesCondition.java +++ b/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 attributes; + private String attributes; - public Map getAttributes() { + public String getAttributes() { return attributes; } - public void setAttributes(Map 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 attributesForMatching = getAttributesForMatching(); + Map> attributesForMatching = getAttributesForMatching(); if (attributesForMatching == null) return false; Map 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 getAttributesForMatching() { + private Map> getAttributesForMatching() { if (configuration.getAttributes() == null) return null; - return configuration.getAttributes(); + return MapperTypeSerializer.deserialize(configuration.getAttributes()); } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/policies/ClientPoliciesConditionTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/policies/ClientPoliciesConditionTest.java index fcc6bb5597..9921bbde45 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/policies/ClientPoliciesConditionTest.java +++ b/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() { + createClientAttributesConditionConfig(new MultivaluedHashMap() { { - put("attr1", "Apple"); - put("attr2", "Orange"); + putSingle("attr1", "Apple"); + putSingle("attr2", "Orange"); } })) .addProfile(PROFILE_NAME) diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/ClientPoliciesUtil.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/ClientPoliciesUtil.java index 2b3948800a..c30c8f2493 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/util/ClientPoliciesUtil.java +++ b/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 attributes) { + public static ClientAttributesCondition.Configuration createClientAttributesConditionConfig(MultivaluedMap attributes) { ClientAttributesCondition.Configuration config = new ClientAttributesCondition.Configuration(); - config.setAttributes(attributes); + String attrsAsString = MapperTypeSerializer.serialize(attributes); + config.setAttributes(attrsAsString); return config; }