From e91dd011c5fbcdce36499c00214ce089b842d5a7 Mon Sep 17 00:00:00 2001 From: mposolda Date: Tue, 20 Jun 2017 14:50:00 +0200 Subject: [PATCH] KEYCLOAK-4438 Disable kerberos flow when provider removed --- .../util/ConcurrentMultivaluedHashMap.java | 19 +++-- .../KerberosFederationProviderFactory.java | 5 ++ .../ldap/LDAPStorageProviderFactory.java | 10 ++- .../org/keycloak/models/jpa/RealmAdapter.java | 6 +- .../keycloak/models/utils/ComponentUtil.java | 12 ++++ .../keycloak/component/ComponentFactory.java | 12 ++++ .../testsuite/arquillian/TestContext.java | 9 ++- .../keycloak/testsuite/util/TestCleanup.java | 69 ++++++++----------- .../testsuite/admin/UserStorageRestTest.java | 59 ++++++++++++++++ 9 files changed, 151 insertions(+), 50 deletions(-) diff --git a/common/src/main/java/org/keycloak/common/util/ConcurrentMultivaluedHashMap.java b/common/src/main/java/org/keycloak/common/util/ConcurrentMultivaluedHashMap.java index c092c6f9d1..258f076546 100755 --- a/common/src/main/java/org/keycloak/common/util/ConcurrentMultivaluedHashMap.java +++ b/common/src/main/java/org/keycloak/common/util/ConcurrentMultivaluedHashMap.java @@ -31,9 +31,9 @@ public class ConcurrentMultivaluedHashMap extends ConcurrentHashMap list = new CopyOnWriteArrayList<>(); + List list = createListInstance(); list.add(value); - put(key, list); + put(key, list); // Just override with new List instance } public void addAll(K key, V... newValues) @@ -84,8 +84,15 @@ public class ConcurrentMultivaluedHashMap extends ConcurrentHashMap getList(K key) { List list = get(key); - if (list == null) - put(key, list = new CopyOnWriteArrayList()); + + if (list == null) { + list = createListInstance(); + List existing = putIfAbsent(key, list); + if (existing != null) { + list = existing; + } + } + return list; } @@ -97,4 +104,8 @@ public class ConcurrentMultivaluedHashMap extends ConcurrentHashMap createListInstance() { + return new CopyOnWriteArrayList<>(); + } + } diff --git a/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/KerberosFederationProviderFactory.java b/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/KerberosFederationProviderFactory.java index e061f5eeb3..75e9d181bc 100755 --- a/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/KerberosFederationProviderFactory.java +++ b/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/KerberosFederationProviderFactory.java @@ -156,4 +156,9 @@ public class KerberosFederationProviderFactory implements UserStorageProviderFac AuthenticationExecutionModel.Requirement.ALTERNATIVE, AuthenticationExecutionModel.Requirement.DISABLED); } + @Override + public void preRemove(KeycloakSession session, RealmModel realm, ComponentModel model) { + CredentialHelper.setOrReplaceAuthenticationRequirement(session, realm, CredentialRepresentation.KERBEROS, + AuthenticationExecutionModel.Requirement.DISABLED, null); + } } diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProviderFactory.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProviderFactory.java index c962af886d..0d4c07bdd5 100755 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProviderFactory.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProviderFactory.java @@ -384,8 +384,14 @@ public class LDAPStorageProviderFactory implements UserStorageProviderFactory { ComponentEntity c = em.find(ComponentEntity.class, component.getId()); if (c == null) return; session.users().preRemove(this, component); + ComponentUtil.notifyPreRemove(session, this, component); removeComponents(component.getId()); getEntity().getComponents().remove(c); } @@ -1896,7 +1897,10 @@ public class RealmAdapter implements RealmModel, JpaModel { getEntity().getComponents().stream() .filter(sameParent) .map(this::entityToModel) - .forEach(c -> session.users().preRemove(this, c)); + .forEach((ComponentModel c) -> { + session.users().preRemove(this, c); + ComponentUtil.notifyPreRemove(session, this, c); + }); getEntity().getComponents().removeIf(sameParent); } diff --git a/server-spi-private/src/main/java/org/keycloak/models/utils/ComponentUtil.java b/server-spi-private/src/main/java/org/keycloak/models/utils/ComponentUtil.java index 1d41f0dd4b..0c603c44c6 100644 --- a/server-spi-private/src/main/java/org/keycloak/models/utils/ComponentUtil.java +++ b/server-spi-private/src/main/java/org/keycloak/models/utils/ComponentUtil.java @@ -17,6 +17,7 @@ package org.keycloak.models.utils; +import org.jboss.logging.Logger; import org.keycloak.component.ComponentFactory; import org.keycloak.component.ComponentModel; import org.keycloak.models.KeycloakSession; @@ -38,6 +39,8 @@ import java.util.Map; */ public class ComponentUtil { + private static final Logger logger = Logger.getLogger(ComponentUtil.class); + public static Map getComponentConfigProperties(KeycloakSession session, ComponentRepresentation component) { return getComponentConfigProperties(session, component.getProviderType(), component.getProviderId()); } @@ -102,5 +105,14 @@ public class ComponentUtil { ((OnUpdateComponent)session.userStorageManager()).onUpdate(session, realm, oldModel, newModel); } } + public static void notifyPreRemove(KeycloakSession session, RealmModel realm, ComponentModel model) { + try { + ComponentFactory factory = getComponentFactory(session, model); + factory.preRemove(session, realm, model); + } catch (IllegalArgumentException iae) { + // We allow to remove broken providers without throwing an exception + logger.warn(iae.getMessage()); + } + } } diff --git a/server-spi/src/main/java/org/keycloak/component/ComponentFactory.java b/server-spi/src/main/java/org/keycloak/component/ComponentFactory.java index 695637fc0a..b4c6f44610 100644 --- a/server-spi/src/main/java/org/keycloak/component/ComponentFactory.java +++ b/server-spi/src/main/java/org/keycloak/component/ComponentFactory.java @@ -79,6 +79,18 @@ public interface ComponentFactory ex } + /** + * Called before the component is removed. + * + * @param session + * @param realm + * @param model model of the component, which is going to be removed + */ + default + void preRemove(KeycloakSession session, RealmModel realm, ComponentModel model) { + + } + /** * These are config properties that are common across all implementation of this component type * diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/arquillian/TestContext.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/arquillian/TestContext.java index 30b0405c51..cfe6d84565 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/arquillian/TestContext.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/arquillian/TestContext.java @@ -20,6 +20,7 @@ import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; import javax.ws.rs.NotFoundException; @@ -54,7 +55,7 @@ public final class TestContext { private boolean initialized; // Key is realmName, value are objects to clean after the test method - private Map cleanups = new HashMap<>(); + private Map cleanups = new ConcurrentHashMap<>(); public TestContext(SuiteContext suiteContext, Class testClass) { this.suiteContext = suiteContext; @@ -146,7 +147,11 @@ public final class TestContext { TestCleanup cleanup = cleanups.get(realmName); if (cleanup == null) { cleanup = new TestCleanup(adminClient, realmName); - cleanups.put(realmName, cleanup); + TestCleanup existing = cleanups.putIfAbsent(realmName, cleanup); + + if (existing != null) { + cleanup = existing; + } } return cleanup; } diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/TestCleanup.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/TestCleanup.java index 17ff44a9c6..192712e180 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/TestCleanup.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/TestCleanup.java @@ -17,13 +17,13 @@ package org.keycloak.testsuite.util; -import java.util.LinkedList; import java.util.List; import javax.ws.rs.NotFoundException; import org.keycloak.admin.client.Keycloak; import org.keycloak.admin.client.resource.RealmResource; +import org.keycloak.common.util.ConcurrentMultivaluedHashMap; /** * Enlist resources to be cleaned after test method @@ -32,18 +32,21 @@ import org.keycloak.admin.client.resource.RealmResource; */ public class TestCleanup { + private static final String IDENTITY_PROVIDER_ALIASES = "IDENTITY_PROVIDER_ALIASES"; + private static final String USER_IDS = "USER_IDS"; + private static final String COMPONENT_IDS = "COMPONENT_IDS"; + private static final String CLIENT_UUIDS = "CLIENT_UUIDS"; + private static final String ROLE_IDS = "ROLE_IDS"; + private static final String GROUP_IDS = "GROUP_IDS"; + private static final String AUTH_FLOW_IDS = "AUTH_FLOW_IDS"; + private static final String AUTH_CONFIG_IDS = "AUTH_CONFIG_IDS"; + private final Keycloak adminClient; private final String realmName; + // Key is kind of entity (eg. "client", "role", "user" etc), Values are all kind of entities of given type to cleanup + private ConcurrentMultivaluedHashMap entities = new ConcurrentMultivaluedHashMap<>(); - private List identityProviderAliases; - private List userIds; - private List componentIds; - private List clientUuids; - private List roleIds; - private List groupIds; - private List authFlowIds; - private List authConfigIds; public TestCleanup(Keycloak adminClient, String realmName) { this.adminClient = adminClient; @@ -52,72 +55,49 @@ public class TestCleanup { public void addUserId(String userId) { - if (userIds == null) { - userIds = new LinkedList<>(); - } - userIds.add(userId); + entities.add(USER_IDS, userId); } public void addIdentityProviderAlias(String identityProviderAlias) { - if (identityProviderAliases == null) { - identityProviderAliases = new LinkedList<>(); - } - identityProviderAliases.add(identityProviderAlias); + entities.add(IDENTITY_PROVIDER_ALIASES, identityProviderAlias); } public void addComponentId(String componentId) { - if (componentIds == null) { - componentIds = new LinkedList<>(); - } - componentIds.add(componentId); + entities.add(COMPONENT_IDS, componentId); } public void addClientUuid(String clientUuid) { - if (clientUuids == null) { - clientUuids = new LinkedList<>(); - } - clientUuids.add(clientUuid); + entities.add(CLIENT_UUIDS, clientUuid); } public void addRoleId(String roleId) { - if (roleIds == null) { - roleIds = new LinkedList<>(); - } - roleIds.add(roleId); + entities.add(ROLE_IDS, roleId); } public void addGroupId(String groupId) { - if (groupIds == null) { - groupIds = new LinkedList<>(); - } - groupIds.add(groupId); + entities.add(GROUP_IDS, groupId); } public void addAuthenticationFlowId(String flowId) { - if (authFlowIds == null) { - authFlowIds = new LinkedList<>(); - } - authFlowIds.add(flowId); + entities.add(AUTH_FLOW_IDS, flowId); } public void addAuthenticationConfigId(String executionConfigId) { - if (authConfigIds == null) { - authConfigIds = new LinkedList<>(); - } - authConfigIds.add(executionConfigId); + entities.add(AUTH_CONFIG_IDS, executionConfigId); } public void executeCleanup() { RealmResource realm = adminClient.realm(realmName); + List userIds = entities.get(USER_IDS); if (userIds != null) { for (String userId : userIds) { try { @@ -128,6 +108,7 @@ public class TestCleanup { } } + List identityProviderAliases = entities.get(IDENTITY_PROVIDER_ALIASES); if (identityProviderAliases != null) { for (String idpAlias : identityProviderAliases) { try { @@ -138,6 +119,7 @@ public class TestCleanup { } } + List componentIds = entities.get(COMPONENT_IDS); if (componentIds != null) { for (String componentId : componentIds) { try { @@ -148,6 +130,7 @@ public class TestCleanup { } } + List clientUuids = entities.get(CLIENT_UUIDS); if (clientUuids != null) { for (String clientUuId : clientUuids) { try { @@ -158,6 +141,7 @@ public class TestCleanup { } } + List roleIds = entities.get(ROLE_IDS); if (roleIds != null) { for (String roleId : roleIds) { try { @@ -168,6 +152,7 @@ public class TestCleanup { } } + List groupIds = entities.get(GROUP_IDS); if (groupIds != null) { for (String groupId : groupIds) { try { @@ -178,6 +163,7 @@ public class TestCleanup { } } + List authFlowIds = entities.get(AUTH_FLOW_IDS); if (authFlowIds != null) { for (String flowId : authFlowIds) { try { @@ -188,6 +174,7 @@ public class TestCleanup { } } + List authConfigIds = entities.get(AUTH_CONFIG_IDS); if (authConfigIds != null) { for (String configId : authConfigIds) { try { diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserStorageRestTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserStorageRestTest.java index 15f0564c44..d7f494f6c1 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserStorageRestTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/UserStorageRestTest.java @@ -156,6 +156,65 @@ public class UserStorageRestTest extends AbstractAdminTest { } + + // KEYCLOAK-4438 + @Test + public void testKerberosAuthenticatorDisabledWhenProviderRemoved() { + // Assert kerberos authenticator DISABLED + AuthenticationExecutionInfoRepresentation kerberosExecution = findKerberosExecution(); + Assert.assertEquals(kerberosExecution.getRequirement(), AuthenticationExecutionModel.Requirement.DISABLED.toString()); + + // create LDAP provider with kerberos + ComponentRepresentation ldapRep = new ComponentRepresentation(); + ldapRep.setName("ldap2"); + ldapRep.setProviderId("ldap"); + ldapRep.setProviderType(UserStorageProvider.class.getName()); + ldapRep.setConfig(new MultivaluedHashMap<>()); + ldapRep.getConfig().putSingle("priority", Integer.toString(2)); + ldapRep.getConfig().putSingle(KerberosConstants.ALLOW_KERBEROS_AUTHENTICATION, "true"); + + + String id = createComponent(ldapRep); + + // Assert kerberos authenticator ALTERNATIVE + kerberosExecution = findKerberosExecution(); + Assert.assertEquals(kerberosExecution.getRequirement(), AuthenticationExecutionModel.Requirement.ALTERNATIVE.toString()); + + // Remove LDAP provider + realm.components().component(id).remove(); + + // Assert kerberos authenticator DISABLED + kerberosExecution = findKerberosExecution(); + Assert.assertEquals(kerberosExecution.getRequirement(), AuthenticationExecutionModel.Requirement.DISABLED.toString()); + + // Add kerberos provider + ComponentRepresentation kerberosRep = new ComponentRepresentation(); + kerberosRep.setName("kerberos"); + kerberosRep.setProviderId("kerberos"); + kerberosRep.setProviderType(UserStorageProvider.class.getName()); + kerberosRep.setConfig(new MultivaluedHashMap<>()); + kerberosRep.getConfig().putSingle("priority", Integer.toString(2)); + + id = createComponent(kerberosRep); + + + // Assert kerberos authenticator ALTERNATIVE + kerberosExecution = findKerberosExecution(); + Assert.assertEquals(kerberosExecution.getRequirement(), AuthenticationExecutionModel.Requirement.ALTERNATIVE.toString()); + + // Switch kerberos authenticator to REQUIRED + kerberosExecution.setRequirement(AuthenticationExecutionModel.Requirement.REQUIRED.toString()); + realm.flows().updateExecutions("browser", kerberosExecution); + + // Remove Kerberos provider + realm.components().component(id).remove(); + + // Assert kerberos authenticator DISABLED + kerberosExecution = findKerberosExecution(); + Assert.assertEquals(kerberosExecution.getRequirement(), AuthenticationExecutionModel.Requirement.DISABLED.toString()); + } + + @Test public void testValidateAndCreateLdapProvider() { // Invalid filter