From 71dcbec642b6419ae6a7c7fb865816bbbd04adea Mon Sep 17 00:00:00 2001 From: mposolda Date: Fri, 14 May 2021 14:11:36 +0200 Subject: [PATCH] KEYCLOAK-18108 Refactoring retrieve of condition/executor providers. Make sure correct configuration of executor/condition is used for particular provider --- .../ClientPolicyConditionRepresentation.java | 16 +- .../ClientPolicyExecutorRepresentation.java | 16 +- .../java/org/keycloak/JsonParserTest.java | 28 +++- .../test/resources/sample-client-policy.json | 20 +++ .../component/JsonConfigComponentModel.java | 113 ++++++++++++++ .../org/keycloak/models/KeycloakSession.java | 13 ++ .../services/DefaultKeycloakSession.java | 16 +- .../clientpolicy/ClientPoliciesUtil.java | 146 ++++++++---------- ...ientPolicyModel.java => ClientPolicy.java} | 3 +- ...ntProfileModel.java => ClientProfile.java} | 2 +- .../DefaultClientPolicyManager.java | 21 +-- .../client/AbstractClientPoliciesTest.java | 40 +++-- .../ClientPoliciesImportExportTest.java | 2 +- .../testsuite/client/ClientPoliciesTest.java | 29 +++- 14 files changed, 329 insertions(+), 136 deletions(-) create mode 100644 core/src/test/resources/sample-client-policy.json create mode 100644 server-spi/src/main/java/org/keycloak/component/JsonConfigComponentModel.java rename services/src/main/java/org/keycloak/services/clientpolicy/{ClientPolicyModel.java => ClientPolicy.java} (97%) rename services/src/main/java/org/keycloak/services/clientpolicy/{ClientProfileModel.java => ClientProfile.java} (96%) diff --git a/core/src/main/java/org/keycloak/representations/idm/ClientPolicyConditionRepresentation.java b/core/src/main/java/org/keycloak/representations/idm/ClientPolicyConditionRepresentation.java index b6a9c33c20..a06178592c 100644 --- a/core/src/main/java/org/keycloak/representations/idm/ClientPolicyConditionRepresentation.java +++ b/core/src/main/java/org/keycloak/representations/idm/ClientPolicyConditionRepresentation.java @@ -19,6 +19,7 @@ package org.keycloak.representations.idm; import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.databind.JsonNode; /** * @author Marek Posolda @@ -28,15 +29,8 @@ public class ClientPolicyConditionRepresentation { @JsonProperty("condition") private String conditionProviderId; - private ClientPolicyConditionConfigurationRepresentation configuration; - - public ClientPolicyConditionRepresentation() { - } - - public ClientPolicyConditionRepresentation(String conditionProviderId, ClientPolicyConditionConfigurationRepresentation configuration) { - this.conditionProviderId = conditionProviderId; - this.configuration = configuration; - } + @JsonProperty("configuration") + private JsonNode configuration; public String getConditionProviderId() { return conditionProviderId; @@ -46,11 +40,11 @@ public class ClientPolicyConditionRepresentation { this.conditionProviderId = conditionProviderId; } - public ClientPolicyConditionConfigurationRepresentation getConfiguration() { + public JsonNode getConfiguration() { return configuration; } - public void setConfiguration(ClientPolicyConditionConfigurationRepresentation configuration) { + public void setConfiguration(JsonNode configuration) { this.configuration = configuration; } } diff --git a/core/src/main/java/org/keycloak/representations/idm/ClientPolicyExecutorRepresentation.java b/core/src/main/java/org/keycloak/representations/idm/ClientPolicyExecutorRepresentation.java index ab0f96d084..c0215bf498 100644 --- a/core/src/main/java/org/keycloak/representations/idm/ClientPolicyExecutorRepresentation.java +++ b/core/src/main/java/org/keycloak/representations/idm/ClientPolicyExecutorRepresentation.java @@ -19,6 +19,7 @@ package org.keycloak.representations.idm; import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.databind.JsonNode; /** * @author Marek Posolda @@ -28,15 +29,8 @@ public class ClientPolicyExecutorRepresentation { @JsonProperty("executor") private String executorProviderId; - private ClientPolicyExecutorConfigurationRepresentation configuration; - - public ClientPolicyExecutorRepresentation() { - } - - public ClientPolicyExecutorRepresentation(String executorProviderId, ClientPolicyExecutorConfigurationRepresentation configuration) { - this.executorProviderId = executorProviderId; - this.configuration = configuration; - } + @JsonProperty("configuration") + private JsonNode configuration; public String getExecutorProviderId() { return executorProviderId; @@ -46,11 +40,11 @@ public class ClientPolicyExecutorRepresentation { this.executorProviderId = providerId; } - public ClientPolicyExecutorConfigurationRepresentation getConfiguration() { + public JsonNode getConfiguration() { return configuration; } - public void setConfiguration(ClientPolicyExecutorConfigurationRepresentation configuration) { + public void setConfiguration(JsonNode configuration) { this.configuration = configuration; } } diff --git a/core/src/test/java/org/keycloak/JsonParserTest.java b/core/src/test/java/org/keycloak/JsonParserTest.java index 819ea5d399..d61fe9f29f 100755 --- a/core/src/test/java/org/keycloak/JsonParserTest.java +++ b/core/src/test/java/org/keycloak/JsonParserTest.java @@ -22,6 +22,10 @@ import org.junit.Test; import org.keycloak.representations.IDToken; import org.keycloak.representations.JsonWebToken; import org.keycloak.representations.adapters.config.AdapterConfig; +import org.keycloak.representations.idm.ClientPoliciesRepresentation; +import org.keycloak.representations.idm.ClientPolicyConditionConfigurationRepresentation; +import org.keycloak.representations.idm.ClientPolicyConditionRepresentation; +import org.keycloak.representations.idm.ClientPolicyRepresentation; import org.keycloak.representations.idm.authorization.ResourceRepresentation; import org.keycloak.representations.oidc.OIDCClientRepresentation; import org.keycloak.util.JsonSerialization; @@ -30,6 +34,7 @@ import java.io.IOException; import java.io.InputStream; import java.util.Collection; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -185,5 +190,26 @@ public class JsonParserTest { return JsonSerialization.readValue(repp, Map.class); } + @Test + public void testReadClientPolicy() throws Exception { + InputStream is = getClass().getClassLoader().getResourceAsStream("sample-client-policy.json"); + ClientPoliciesRepresentation clientPolicies = JsonSerialization.readValue(is, ClientPoliciesRepresentation.class); -} + Assert.assertEquals(clientPolicies.getPolicies().size(), 1); + ClientPolicyRepresentation clientPolicy = clientPolicies.getPolicies().get(0); + Assert.assertEquals("some-policy", clientPolicy.getName()); + List conditions = clientPolicy.getConditions(); + Assert.assertEquals(conditions.size(), 1); + ClientPolicyConditionRepresentation condition = conditions.get(0); + Assert.assertEquals("some-condition", condition.getConditionProviderId()); + + ClientPolicyConditionConfigurationRepresentation configRep = JsonSerialization.mapper.convertValue(condition.getConfiguration(), ClientPolicyConditionConfigurationRepresentation.class); + Assert.assertEquals(true, configRep.isNegativeLogic()); + Assert.assertEquals("val1", configRep.getConfigAsMap().get("string-option")); + Assert.assertEquals(14, configRep.getConfigAsMap().get("int-option")); + Assert.assertEquals(true, configRep.getConfigAsMap().get("bool-option")); + Assert.assertNull(configRep.getConfigAsMap().get("not-existing-option")); + } + + +} \ No newline at end of file diff --git a/core/src/test/resources/sample-client-policy.json b/core/src/test/resources/sample-client-policy.json new file mode 100644 index 0000000000..f374a99184 --- /dev/null +++ b/core/src/test/resources/sample-client-policy.json @@ -0,0 +1,20 @@ +{ + "policies": [ + { + "name": "some-policy", + "description": "This is some client policy.", + "enabled": true, + "conditions": [ + { + "condition": "some-condition", + "configuration": { + "is-negative-logic": true, + "string-option": "val1", + "int-option": 14, + "bool-option": true + } + } + ] + } + ] +} \ No newline at end of file diff --git a/server-spi/src/main/java/org/keycloak/component/JsonConfigComponentModel.java b/server-spi/src/main/java/org/keycloak/component/JsonConfigComponentModel.java new file mode 100644 index 0000000000..3ecc5ffe61 --- /dev/null +++ b/server-spi/src/main/java/org/keycloak/component/JsonConfigComponentModel.java @@ -0,0 +1,113 @@ +/* + * Copyright 2021 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.component; + +import com.fasterxml.jackson.databind.JsonNode; +import org.keycloak.provider.Provider; + +/** + * Component model backed by JSON configuration. Useful for providers, which rely on JSON configuration rather than on ComponentModel, which is directly + * persisted as entity in the DB (store). + * + * @author Marek Posolda + */ +public class JsonConfigComponentModel extends ComponentModel { + + private final String providerType; + private final String providerId; + private final String componentId; + private final JsonNode configNode; + + /** + * @param providerType + * @param realmId + * @param providerId + * @param configNode JSON configuration of this provider. For example if node corresponds to JSON like "{\"foo\":\"bar\"}", then + * component configuration is supposed to have one configuration option "foo" with value "bar" + */ + public JsonConfigComponentModel(Class providerType, String realmId, String providerId, JsonNode configNode) { + checkNotNull(providerType, "providerType must be not null"); + checkNotNull(realmId, "realmId must be not null"); + checkNotNull(providerId, "providerId must be not null"); + checkNotNull(configNode, "configNode must be not null for provider " + providerId); + this.providerType = providerType.getName(); + this.providerId = providerId; + this.configNode = configNode; + + // We don't have realm model ID of the component, so componentId based on the realmId, providerType, providerId and hashCode of configurations. + this.componentId = realmId + "::" + providerType + "::" + this.providerId + "::" + configNode.hashCode(); + } + + private void checkNotNull(Object value, String message) { + if (value == null) { + throw new IllegalArgumentException(message); + } + } + + + @Override + public String getProviderType() { + return providerType; + } + + @Override + public String getProviderId() { + return providerId; + } + + @Override + public String getName() { + return componentId + "-config"; + } + + @Override + public String getId() { + return componentId; + } + + @Override + public boolean get(String key, boolean defaultValue) { + JsonNode sub = configNode.get(key); + return sub == null ? defaultValue : sub.asBoolean(); + } + + @Override + public long get(String key, long defaultValue) { + JsonNode sub = configNode.get(key); + return sub == null ? defaultValue : sub.asLong(); + } + + @Override + public int get(String key, int defaultValue) { + JsonNode sub = configNode.get(key); + return sub == null ? defaultValue : sub.asInt(); + } + + @Override + public String get(String key, String defaultValue) { + JsonNode sub = configNode.get(key); + return sub == null ? defaultValue : sub.asText(); + } + + @Override + public String get(String key) { + return get(key, null); + } + +} diff --git a/server-spi/src/main/java/org/keycloak/models/KeycloakSession.java b/server-spi/src/main/java/org/keycloak/models/KeycloakSession.java index a1f85419bc..6442a2a8d4 100755 --- a/server-spi/src/main/java/org/keycloak/models/KeycloakSession.java +++ b/server-spi/src/main/java/org/keycloak/models/KeycloakSession.java @@ -27,6 +27,7 @@ import org.keycloak.storage.federated.UserFederatedStorageProvider; import org.keycloak.vault.VaultTranscriber; import java.util.Set; +import java.util.function.Function; /** * @author Bill Burke @@ -74,6 +75,18 @@ public interface KeycloakSession extends InvalidationHandler { */ T getComponentProvider(Class clazz, String componentId); + /** + * Returns a component provider for a component from the realm that is relevant to this session. + * The relevant realm must be set prior to calling this method in the context, see {@link KeycloakContext#getRealm()}. + * @param + * @param clazz + * @param componentId Component configuration + * @param modelGetter Getter to retrieve componentModel + * @throws IllegalArgumentException If the realm is not set in the context. + * @return Provider configured according to the {@link componentId}, {@code null} if it cannot be instantiated. + */ + T getComponentProvider(Class clazz, String componentId, Function modelGetter); + /** * * @param diff --git a/services/src/main/java/org/keycloak/services/DefaultKeycloakSession.java b/services/src/main/java/org/keycloak/services/DefaultKeycloakSession.java index da01849d9d..f95f75f572 100644 --- a/services/src/main/java/org/keycloak/services/DefaultKeycloakSession.java +++ b/services/src/main/java/org/keycloak/services/DefaultKeycloakSession.java @@ -65,6 +65,7 @@ import java.util.Map; import java.util.Map.Entry; import java.util.Set; import java.util.function.Consumer; +import java.util.function.Function; import java.util.stream.Collectors; /** @@ -337,8 +338,19 @@ public class DefaultKeycloakSession implements KeycloakSession { } @Override - @SuppressWarnings("unchecked") public T getComponentProvider(Class clazz, String componentId) { + final RealmModel realm = getContext().getRealm(); + if (realm == null) { + throw new IllegalArgumentException("Realm not set in the context."); + } + + // Loads componentModel from the realm + return this.getComponentProvider(clazz, componentId, KeycloakModelUtils.componentModelGetter(realm.getId(), componentId)); + } + + @Override + @SuppressWarnings("unchecked") + public T getComponentProvider(Class clazz, String componentId, Function modelGetter) { Integer hash = clazz.hashCode() + componentId.hashCode(); T provider = (T) providers.get(hash); final RealmModel realm = getContext().getRealm(); @@ -351,7 +363,7 @@ public class DefaultKeycloakSession implements KeycloakSession { // allowed on JDK 1.8, attempt of such a modification throws ConcurrentModificationException with JDK 9+ if (provider == null) { final String realmId = realm.getId(); - ProviderFactory providerFactory = factory.getProviderFactory(clazz, realmId, componentId, KeycloakModelUtils.componentModelGetter(realmId, componentId)); + ProviderFactory providerFactory = factory.getProviderFactory(clazz, realmId, componentId, modelGetter); if (providerFactory != null) { provider = providerFactory.create(this); providers.put(hash, provider); diff --git a/services/src/main/java/org/keycloak/services/clientpolicy/ClientPoliciesUtil.java b/services/src/main/java/org/keycloak/services/clientpolicy/ClientPoliciesUtil.java index 7035c08760..f348085009 100644 --- a/services/src/main/java/org/keycloak/services/clientpolicy/ClientPoliciesUtil.java +++ b/services/src/main/java/org/keycloak/services/clientpolicy/ClientPoliciesUtil.java @@ -22,16 +22,17 @@ import java.io.IOException; import java.io.InputStream; import java.util.ArrayList; import java.util.Collections; -import java.util.HashMap; import java.util.LinkedList; import java.util.List; -import java.util.Map; import java.util.Set; import java.util.stream.Collectors; +import com.fasterxml.jackson.databind.JsonNode; import org.jboss.logging.Logger; import org.keycloak.common.Profile; +import org.keycloak.component.ComponentModel; +import org.keycloak.component.JsonConfigComponentModel; import org.keycloak.models.Constants; import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; @@ -71,75 +72,57 @@ public class ClientPoliciesUtil { } /** - * gets existing client profiles in a realm as model. - * not return null. + * Gets existing client profile of given name with resolved executor providers. It can be profile from realm or from global client profiles. */ - static Map getClientProfilesModel(KeycloakSession session, RealmModel realm, List globalClientProfiles) { - // get existing profiles as json - String profilesJson = getClientProfilesJsonString(realm); - if (profilesJson == null) { - return Collections.emptyMap(); - } - - // deserialize existing profiles (json -> representation) - ClientProfilesRepresentation profilesRep = null; - try { - profilesRep = convertClientProfilesJsonToRepresentation(profilesJson); - } catch (ClientPolicyException e) { - logger.warnv("Failed to serialize client profiles json string. err={0}, errDetail={1}", e.getError(), e.getErrorDetail()); - return Collections.emptyMap(); - } - if (profilesRep == null || profilesRep.getProfiles() == null) { - return Collections.emptyMap(); - } + static ClientProfile getClientProfileModel(KeycloakSession session, RealmModel realm, ClientProfilesRepresentation profilesRep, List globalClientProfiles, String profileName) throws ClientPolicyException { + // Obtain profiles from realm List profiles = profilesRep.getProfiles(); + if (profiles == null) { + profiles = new ArrayList<>(); + } // Add global profiles as well profiles.addAll(globalClientProfiles); - // constructing existing profiles (representation -> model) - Map profileMap = new HashMap<>(); - for (ClientProfileRepresentation profileRep : profilesRep.getProfiles()) { - // ignore profile without name - if (profileRep.getName() == null) { - continue; - } - - ClientProfileModel profileModel = new ClientProfileModel(); - profileModel.setName(profileRep.getName()); - profileModel.setDescription(profileRep.getDescription()); - - if (profileRep.getExecutors() == null) { - profileModel.setExecutors(new ArrayList<>()); - profileMap.put(profileRep.getName(), profileModel); - continue; - } - - List executors = new ArrayList<>(); - if (profileRep.getExecutors() != null) { - for (ClientPolicyExecutorRepresentation executorRep : profileRep.getExecutors()) { - ClientPolicyExecutorProvider provider = session.getProvider(ClientPolicyExecutorProvider.class, executorRep.getExecutorProviderId()); - if (provider == null) { - // executor's provider not found. just skip it. - logger.warnf("Executor with provider ID %s not found", executorRep.getExecutorProviderId()); - continue; - } - - try { - ClientPolicyExecutorConfigurationRepresentation configuration = (ClientPolicyExecutorConfigurationRepresentation) JsonSerialization.mapper.convertValue(executorRep.getConfiguration(), provider.getExecutorConfigurationClass()); - provider.setupConfiguration(configuration); - executors.add(provider); - } catch (IllegalArgumentException iae) { - logger.warnv("failed for Configuration Setup during setup provider {0} :: error = {1}", executorRep.getExecutorProviderId(), iae.getMessage()); - } - } - } - profileModel.setExecutors(executors); - - profileMap.put(profileRep.getName(), profileModel); + ClientProfileRepresentation profileRep = profiles.stream() + .filter(clientProfile -> profileName.equals(clientProfile.getName())) + .findFirst().orElse(null); + if (profileRep == null) { + return null; } - return profileMap; + ClientProfile profileModel = new ClientProfile(); + profileModel.setName(profileRep.getName()); + profileModel.setDescription(profileRep.getDescription()); + + if (profileRep.getExecutors() == null) { + profileModel.setExecutors(new ArrayList<>()); + return profileModel; + } + + List executors = new ArrayList<>(); + if (profileRep.getExecutors() != null) { + for (ClientPolicyExecutorRepresentation executorRep : profileRep.getExecutors()) { + ClientPolicyExecutorProvider provider = getExecutorProvider(session, realm, executorRep.getExecutorProviderId(), executorRep.getConfiguration()); + executors.add(provider); + } + } + profileModel.setExecutors(executors); + + return profileModel; + } + + private static ClientPolicyExecutorProvider getExecutorProvider(KeycloakSession session, RealmModel realm, String providerId, JsonNode config) { + ComponentModel componentModel = new JsonConfigComponentModel(ClientPolicyExecutorProvider.class, realm.getId(), providerId, config); + ClientPolicyExecutorProvider executorProvider = session.getComponentProvider(ClientPolicyExecutorProvider.class, componentModel.getId(), sessionFactory -> componentModel); + if (executorProvider == null) { + // condition's provider not found. just skip it. + throw new IllegalStateException("Executor with provider ID " + providerId + " not found"); + } + + ClientPolicyExecutorConfigurationRepresentation configuration = (ClientPolicyExecutorConfigurationRepresentation) JsonSerialization.mapper.convertValue(config, executorProvider.getExecutorConfigurationClass()); + executorProvider.setupConfiguration(configuration); + return executorProvider; } /** @@ -306,10 +289,10 @@ public class ClientPoliciesUtil { } /** - * get existing enabled client policies in a realm as model. + * Gets existing enabled client policies in a realm. * not return null. */ - static List getEnabledClientPoliciesModel(KeycloakSession session, RealmModel realm) { + static List getEnabledClientPolicies(KeycloakSession session, RealmModel realm) { // get existing profiles as json String policiesJson = getClientPoliciesJsonString(realm); if (policiesJson == null) { @@ -329,7 +312,7 @@ public class ClientPoliciesUtil { } // constructing existing policies (representation -> model) - List policyList = new ArrayList<>(); + List policyList = new ArrayList<>(); for (ClientPolicyRepresentation policyRep: policiesRep.getPolicies()) { // ignore policy without name if (policyRep.getName() == null) { @@ -341,7 +324,7 @@ public class ClientPoliciesUtil { continue; } - ClientPolicyModel policyModel = new ClientPolicyModel(); + ClientPolicy policyModel = new ClientPolicy(); policyModel.setName(policyRep.getName()); policyModel.setDescription(policyRep.getDescription()); policyModel.setEnable(true); @@ -349,20 +332,8 @@ public class ClientPoliciesUtil { List conditions = new ArrayList<>(); if (policyRep.getConditions() != null) { for (ClientPolicyConditionRepresentation conditionRep : policyRep.getConditions()) { - ClientPolicyConditionProvider provider = session.getProvider(ClientPolicyConditionProvider.class, conditionRep.getConditionProviderId()); - if (provider == null) { - // condition's provider not found. just skip it. - logger.warnf("Condition with provider ID %s not found", conditionRep.getConditionProviderId()); - continue; - } - - try { - ClientPolicyConditionConfigurationRepresentation configuration = (ClientPolicyConditionConfigurationRepresentation) JsonSerialization.mapper.convertValue(conditionRep.getConfiguration(), provider.getConditionConfigurationClass()); - provider.setupConfiguration(configuration); - conditions.add(provider); - } catch (IllegalArgumentException iae) { - logger.warnv("failed for Configuration Setup :: error = {0}", iae.getMessage()); - } + ClientPolicyConditionProvider provider = getConditionProvider(session, realm, conditionRep.getConditionProviderId(), conditionRep.getConfiguration()); + conditions.add(provider); } } policyModel.setConditions(conditions); @@ -377,6 +348,19 @@ public class ClientPoliciesUtil { return policyList; } + private static ClientPolicyConditionProvider getConditionProvider(KeycloakSession session, RealmModel realm, String providerId, JsonNode config) { + ComponentModel componentModel = new JsonConfigComponentModel(ClientPolicyConditionProvider.class, realm.getId(), providerId, config); + ClientPolicyConditionProvider conditionProvider = session.getComponentProvider(ClientPolicyConditionProvider.class, componentModel.getId(), sessionFactory -> componentModel); + if (conditionProvider == null) { + // condition's provider not found. just skip it. + throw new IllegalStateException("Condition with provider ID " + providerId + " not found"); + } + + ClientPolicyConditionConfigurationRepresentation configuration = (ClientPolicyConditionConfigurationRepresentation) JsonSerialization.mapper.convertValue(config, conditionProvider.getConditionConfigurationClass()); + conditionProvider.setupConfiguration(configuration); + return conditionProvider; + } + /** * convert client policies as representation to json. * can return null. diff --git a/services/src/main/java/org/keycloak/services/clientpolicy/ClientPolicyModel.java b/services/src/main/java/org/keycloak/services/clientpolicy/ClientPolicy.java similarity index 97% rename from services/src/main/java/org/keycloak/services/clientpolicy/ClientPolicyModel.java rename to services/src/main/java/org/keycloak/services/clientpolicy/ClientPolicy.java index 6859b1c81c..0d1916055b 100644 --- a/services/src/main/java/org/keycloak/services/clientpolicy/ClientPolicyModel.java +++ b/services/src/main/java/org/keycloak/services/clientpolicy/ClientPolicy.java @@ -13,6 +13,7 @@ * 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.services.clientpolicy; @@ -25,7 +26,7 @@ import org.keycloak.services.clientpolicy.condition.ClientPolicyConditionProvide /** * @author Takashi Norimatsu */ -public class ClientPolicyModel implements Serializable { +class ClientPolicy implements Serializable { protected String name; protected String description; diff --git a/services/src/main/java/org/keycloak/services/clientpolicy/ClientProfileModel.java b/services/src/main/java/org/keycloak/services/clientpolicy/ClientProfile.java similarity index 96% rename from services/src/main/java/org/keycloak/services/clientpolicy/ClientProfileModel.java rename to services/src/main/java/org/keycloak/services/clientpolicy/ClientProfile.java index 5379b128b6..07f249e29d 100644 --- a/services/src/main/java/org/keycloak/services/clientpolicy/ClientProfileModel.java +++ b/services/src/main/java/org/keycloak/services/clientpolicy/ClientProfile.java @@ -26,7 +26,7 @@ import org.keycloak.services.clientpolicy.executor.ClientPolicyExecutorProvider; /** * @author Takashi Norimatsu */ -public class ClientProfileModel implements Serializable { +class ClientProfile implements Serializable { protected String name; protected String description; diff --git a/services/src/main/java/org/keycloak/services/clientpolicy/DefaultClientPolicyManager.java b/services/src/main/java/org/keycloak/services/clientpolicy/DefaultClientPolicyManager.java index 33201f4b1c..95653cfb64 100644 --- a/services/src/main/java/org/keycloak/services/clientpolicy/DefaultClientPolicyManager.java +++ b/services/src/main/java/org/keycloak/services/clientpolicy/DefaultClientPolicyManager.java @@ -20,9 +20,7 @@ package org.keycloak.services.clientpolicy; import java.io.IOException; import java.util.LinkedList; import java.util.List; -import java.util.Map; import java.util.function.Supplier; -import java.util.stream.Collectors; import org.jboss.logging.Logger; import org.keycloak.common.Profile; @@ -68,15 +66,14 @@ public class DefaultClientPolicyManager implements ClientPolicyManager { } private void doPolicyOperation(ClientConditionOperation condition, ClientExecutorOperation executor, RealmModel realm) throws ClientPolicyException { - Map map = ClientPoliciesUtil.getClientProfilesModel(session, realm, globalClientProfilesSupplier.get()); - List list = ClientPoliciesUtil.getEnabledClientPoliciesModel(session, realm).stream().collect(Collectors.toList()); + List list = ClientPoliciesUtil.getEnabledClientPolicies(session, realm); if (list == null || list.isEmpty()) { logger.trace("POLICY OPERATION :: No enabled policy."); return; } - for (ClientPolicyModel policy: list) { + for (ClientPolicy policy: list) { logger.tracev("POLICY OPERATION :: policy name = {0}", policy.getName()); if (!isSatisfied(policy, condition)) { logger.tracev("POLICY UNSATISFIED :: policy name = {0}", policy.getName()); @@ -84,12 +81,12 @@ public class DefaultClientPolicyManager implements ClientPolicyManager { } logger.tracev("POLICY APPLIED :: policy name = {0}", policy.getName()); - execute(policy, executor, map); + execute(policy, executor, realm); } } private boolean isSatisfied( - ClientPolicyModel policy, + ClientPolicy policy, ClientConditionOperation op) throws ClientPolicyException { if (policy.getConditions() == null || policy.getConditions().isEmpty()) { @@ -133,16 +130,20 @@ public class DefaultClientPolicyManager implements ClientPolicyManager { } private void execute( - ClientPolicyModel policy, + ClientPolicy policy, ClientExecutorOperation op, - Map map) throws ClientPolicyException { + RealmModel realm) throws ClientPolicyException { if (policy.getProfiles() == null || policy.getProfiles().isEmpty()) { logger.tracev("NO PROFILE :: policy name = {0}", policy.getName()); + return; } + // Get profiles from realm + ClientProfilesRepresentation clientProfiles = ClientPoliciesUtil.getClientProfilesRepresentation(session, realm); + for (String profileName : policy.getProfiles()) { - ClientProfileModel profile = map.get(profileName); + ClientProfile profile = ClientPoliciesUtil.getClientProfileModel(session, realm, clientProfiles, globalClientProfilesSupplier.get(), profileName); if (profile == null) { logger.tracev("PROFILE NOT FOUND :: policy name = {0}, profile name = {1}", policy.getName(), profileName); continue; diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/AbstractClientPoliciesTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/AbstractClientPoliciesTest.java index be4abefe49..8ff114edaf 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/AbstractClientPoliciesTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/AbstractClientPoliciesTest.java @@ -279,7 +279,7 @@ public abstract class AbstractClientPoliciesTest extends AbstractKeycloakTest { } - protected void assertExpectedLoadedProfiles(Consumer modifiedAssertion) { + protected void assertExpectedLoadedProfiles(Consumer modifiedAssertion) throws Exception { // retrieve loaded builtin profiles ClientProfilesRepresentation actualProfilesRep = getProfilesWithGlobals(); @@ -768,6 +768,11 @@ public abstract class AbstractClientPoliciesTest extends AbstractKeycloakTest { profilesRep.setProfiles(new ArrayList<>()); } + // Create client profile from existing representation + public ClientProfilesBuilder(ClientProfilesRepresentation existingRep) { + this.profilesRep = existingRep; + } + public ClientProfilesBuilder addProfile(ClientProfileRepresentation rep) { profilesRep.getProfiles().add(rep); return this; @@ -809,11 +814,14 @@ public abstract class AbstractClientPoliciesTest extends AbstractKeycloakTest { return this; } - public ClientProfileBuilder addExecutor(String providerId, ClientPolicyExecutorConfigurationRepresentation config) { + public ClientProfileBuilder addExecutor(String providerId, ClientPolicyExecutorConfigurationRepresentation config) throws Exception { if (config == null) { config = new ClientPolicyExecutorConfigurationRepresentation(); } - profileRep.getExecutors().add(new ClientPolicyExecutorRepresentation(providerId, config)); + ClientPolicyExecutorRepresentation executor = new ClientPolicyExecutorRepresentation(); + executor.setExecutorProviderId(providerId); + executor.setConfiguration(JsonSerialization.mapper.readValue(JsonSerialization.mapper.writeValueAsBytes(config), JsonNode.class)); + profileRep.getExecutors().add(executor); return this; } @@ -930,8 +938,11 @@ public abstract class AbstractClientPoliciesTest extends AbstractKeycloakTest { return this; } - public ClientPolicyBuilder addCondition(String providerId, ClientPolicyConditionConfigurationRepresentation config) { - policyRep.getConditions().add(new ClientPolicyConditionRepresentation(providerId, config)); + public ClientPolicyBuilder addCondition(String providerId, ClientPolicyConditionConfigurationRepresentation config) throws Exception { + ClientPolicyConditionRepresentation condition = new ClientPolicyConditionRepresentation(); + condition.setConditionProviderId(providerId); + condition.setConfiguration(JsonSerialization.mapper.readValue(JsonSerialization.mapper.writeValueAsBytes(config), JsonNode.class)); + policyRep.getConditions().add(condition); return this; } @@ -1278,16 +1289,15 @@ public abstract class AbstractClientPoliciesTest extends AbstractKeycloakTest { assertExpectedAugmenedExecutor(isAugment, PKCEEnforcerExecutorFactory.PROVIDER_ID, profileRep); } - protected void assertExpectedSecureClientAuthEnforceExecutor(List clientAuthns, boolean isAugment, String clientAuthnsAugment, ClientProfileRepresentation profileRep) { + protected void assertExpectedSecureClientAuthEnforceExecutor(List clientAuthns, boolean isAugment, String clientAuthnsAugment, ClientProfileRepresentation profileRep) throws Exception { assertExpectedAugmenedExecutor(isAugment, SecureClientAuthenticatorExecutorFactory.PROVIDER_ID, profileRep); assertNotNull(profileRep); - Map actualExecutorConfig = getConfigOfExecutor(SecureClientAuthenticatorExecutorFactory.PROVIDER_ID, profileRep); + JsonNode actualExecutorConfig = getConfigOfExecutor(SecureClientAuthenticatorExecutorFactory.PROVIDER_ID, profileRep); assertNotNull(actualExecutorConfig); - - Set actualClientAuthns = new HashSet<>((Collection) actualExecutorConfig.get("client-authns")); + Set actualClientAuthns = new HashSet<>((Collection) JsonSerialization.readValue(actualExecutorConfig.get("client-authns").toString(), List.class)); assertEquals(new HashSet<>(clientAuthns), actualClientAuthns); - String actualClientAuthnAugment = actualExecutorConfig.get("client-authns-augment").toString(); + String actualClientAuthnAugment = actualExecutorConfig.get("client-authns-augment").textValue(); assertEquals(clientAuthnsAugment, actualClientAuthnAugment); } @@ -1317,17 +1327,17 @@ public abstract class AbstractClientPoliciesTest extends AbstractKeycloakTest { protected void assertExpectedAugmenedExecutor(boolean isAugment, String providerId, ClientProfileRepresentation profileRep) { assertNotNull(profileRep); - Map actualExecutorConfig = getConfigOfExecutor(providerId, profileRep); + JsonNode actualExecutorConfig = getConfigOfExecutor(providerId, profileRep); assertNotNull(actualExecutorConfig); - boolean actualIsAugment = actualExecutorConfig.get("is-augment") == null ? false : (Boolean) actualExecutorConfig.get("is-augment"); + boolean actualIsAugment = actualExecutorConfig.get("is-augment") == null ? false : actualExecutorConfig.get("is-augment").asBoolean(); assertEquals(isAugment, actualIsAugment); } - private Map getConfigOfExecutor(String providerId, ClientProfileRepresentation profileRep) { + private JsonNode getConfigOfExecutor(String providerId, ClientProfileRepresentation profileRep) { ClientPolicyExecutorRepresentation executorRep = profileRep.getExecutors().stream() .filter(profileRepp -> providerId.equals(profileRepp.getExecutorProviderId())) .findFirst().orElse(null); - return executorRep == null ? null : executorRep.getConfiguration().getConfigAsMap(); + return executorRep == null ? null : executorRep.getConfiguration(); } // Assertions about policies @@ -1455,7 +1465,7 @@ public abstract class AbstractClientPoliciesTest extends AbstractKeycloakTest { } private void assertExpectedEmptyConfig(String executorProviderId, ClientProfileRepresentation profileRep) { - Map config = getConfigOfExecutor(executorProviderId, profileRep); + JsonNode config = getConfigOfExecutor(executorProviderId, profileRep); Assert.assertTrue("Expected empty configuration for provider " + executorProviderId, config.isEmpty()); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesImportExportTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesImportExportTest.java index 3f22a80f83..bb4b4819ad 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesImportExportTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesImportExportTest.java @@ -72,7 +72,7 @@ public class ClientPoliciesImportExportTest extends AbstractClientPoliciesTest { testRealmExportImport(); } - private void testRealmExportImport() throws LifecycleException { + private void testRealmExportImport() throws Exception { testingClient.testing().exportImport().setAction(ExportImportConfig.ACTION_EXPORT); testingClient.testing().exportImport().setRealmName("test"); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesTest.java index e16b69e1ff..ad3398eda6 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/client/ClientPoliciesTest.java @@ -207,6 +207,31 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { assertEquals(JWTClientSecretAuthenticator.PROVIDER_ID, getClientByAdmin(cId).getClientAuthenticatorType()); } + // KEYCLOAK-18108 + @Test + public void testTwoProfilesWithDifferentConfigurationOfSameExecutorType() throws Exception { + setupPolicyClientIdAndSecretNotAcceptableAuthType(POLICY_NAME); + + // register another profile with "SecureClientAuthEnforceExecutorFactory", but use different configuration of client authenticator. + // This profile won't allow JWTClientSecretAuthenticator.PROVIDER_ID + String profileName = "UnusedProfile"; + String json = (new ClientProfilesBuilder(getProfilesWithoutGlobals())).addProfile( + (new ClientProfileBuilder()).createProfile(profileName, "Profile with SecureClientAuthEnforceExecutorFactory") + .addExecutor(SecureClientAuthenticatorExecutorFactory.PROVIDER_ID, + createSecureClientAuthEnforceExecutorConfig(Boolean.FALSE, + Arrays.asList(JWTClientAuthenticator.PROVIDER_ID, X509ClientAuthenticator.PROVIDER_ID), + null)) + .toRepresentation() + ).toString(); + updateProfiles(json); + + // Make sure it is still possible to create client with JWTClientSecretAuthenticator. The "UnusedProfile" should not be used as it is not referenced from any client policy + String cId = createClientByAdmin(generateSuffixedName(CLIENT_NAME), (ClientRepresentation clientRep) -> { + clientRep.setClientAuthenticatorType(JWTClientSecretAuthenticator.PROVIDER_ID); + }); + assertEquals(JWTClientSecretAuthenticator.PROVIDER_ID, getClientByAdmin(cId).getClientAuthenticatorType()); + } + @Test public void testAdminClientUpdateAcceptableAuthType() throws Exception { setupPolicyClientIdAndSecretNotAcceptableAuthType(POLICY_NAME); @@ -2051,7 +2076,7 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { } } - private void setupPolicyClientIdAndSecretNotAcceptableAuthType(String policyName) throws ClientPolicyException { + private void setupPolicyClientIdAndSecretNotAcceptableAuthType(String policyName) throws Exception { // register profiles String profileName = "MyProfile"; String json = (new ClientProfilesBuilder()).addProfile( @@ -2075,7 +2100,7 @@ public class ClientPoliciesTest extends AbstractClientPoliciesTest { updatePolicies(json); } - private void setupPolicyAuthzCodeFlowUnderMultiPhasePolicy(String policyName) throws ClientPolicyException { + private void setupPolicyAuthzCodeFlowUnderMultiPhasePolicy(String policyName) throws Exception { // register profiles String profileName = "MyProfile"; String json = (new ClientProfilesBuilder()).addProfile(