From ea4155bbcd55c53eb154b5567a2f41ef989dbcf0 Mon Sep 17 00:00:00 2001 From: rmartinc Date: Wed, 6 Mar 2024 19:44:14 +0100 Subject: [PATCH] Remove recursively when deleting an authentication executor Closes #24795 Signed-off-by: rmartinc --- .../migration/migrators/MigrateTo22_0_0.java | 8 +-- .../models/utils/KeycloakModelUtils.java | 51 ++++++++++++++++--- .../AuthenticationManagementResource.java | 31 ++++++----- .../admin/authentication/FlowTest.java | 27 +++++++++- 4 files changed, 89 insertions(+), 28 deletions(-) diff --git a/model/storage-private/src/main/java/org/keycloak/migration/migrators/MigrateTo22_0_0.java b/model/storage-private/src/main/java/org/keycloak/migration/migrators/MigrateTo22_0_0.java index 1a96a87a725..a6496124a75 100644 --- a/model/storage-private/src/main/java/org/keycloak/migration/migrators/MigrateTo22_0_0.java +++ b/model/storage-private/src/main/java/org/keycloak/migration/migrators/MigrateTo22_0_0.java @@ -41,25 +41,25 @@ public class MigrateTo22_0_0 implements Migration { @Override public void migrate(KeycloakSession session) { - session.realms().getRealmsStream().forEach(this::removeHttpChallengeFlow); + session.realms().getRealmsStream().forEach(realm -> removeHttpChallengeFlow(session, realm)); //login, account, email themes are handled by JpaUpdate22_0_0_RemoveRhssoThemes } @Override public void migrateImport(KeycloakSession session, RealmModel realm, RealmRepresentation rep, boolean skipUserDependent) { - removeHttpChallengeFlow(realm); + removeHttpChallengeFlow(session, realm); updateLoginTheme(realm); updateAccountTheme(realm); updateEmailTheme(realm); updateClientAttributes(realm); } - private void removeHttpChallengeFlow(RealmModel realm) { + private void removeHttpChallengeFlow(KeycloakSession session, RealmModel realm) { AuthenticationFlowModel httpChallenge = realm.getFlowByAlias(HTTP_CHALLENGE_FLOW); if (httpChallenge == null) return; try { - KeycloakModelUtils.deepDeleteAuthenticationFlow(realm, httpChallenge, () -> {}, () -> {}); + KeycloakModelUtils.deepDeleteAuthenticationFlow(session, realm, httpChallenge, () -> {}, () -> {}); LOG.debugf("Removed '%s' authentication flow in realm '%s'", HTTP_CHALLENGE_FLOW, realm.getName()); } catch (ModelException me) { LOG.errorf("Authentication flow '%s' is in use in realm '%s' and cannot be removed. Please update your deployment to avoid using this flow before migration to latest Keycloak", diff --git a/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java b/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java index 795e0a7abd4..894d72e35b4 100755 --- a/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java +++ b/server-spi-private/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java @@ -29,7 +29,9 @@ import org.keycloak.common.util.SecretGenerator; import org.keycloak.common.util.Time; import org.keycloak.component.ComponentModel; import org.keycloak.crypto.Algorithm; +import org.keycloak.deployment.DeployedConfigurationsManager; import org.keycloak.models.AuthenticationExecutionModel; +import org.keycloak.models.AuthenticatorConfigModel; import org.keycloak.models.AuthenticationFlowModel; import org.keycloak.models.ClientModel; import org.keycloak.models.ClientScopeModel; @@ -44,7 +46,6 @@ import org.keycloak.models.KeycloakSessionFactory; import org.keycloak.models.KeycloakSessionTask; import org.keycloak.models.KeycloakSessionTaskWithResult; import org.keycloak.models.RealmModel; -import org.keycloak.models.RealmProvider; import org.keycloak.models.RoleModel; import org.keycloak.models.ScopeContainerModel; import org.keycloak.models.UserModel; @@ -861,12 +862,13 @@ public final class KeycloakModelUtils { /** * Recursively remove authentication flow (including all subflows and executions) from the model storage * - * @param realm + * @param session The keycloak session + * @param realm The realm * @param authFlow flow to delete - * @param flowUnavailableHandler Will be executed when flow or some of it's subflow is null + * @param flowUnavailableHandler Will be executed when flow, sub-flow or executor is null * @param builtinFlowHandler will be executed when flow is built-in flow */ - public static void deepDeleteAuthenticationFlow(RealmModel realm, AuthenticationFlowModel authFlow, Runnable flowUnavailableHandler, Runnable builtinFlowHandler) { + public static void deepDeleteAuthenticationFlow(KeycloakSession session, RealmModel realm, AuthenticationFlowModel authFlow, Runnable flowUnavailableHandler, Runnable builtinFlowHandler) { if (authFlow == null) { flowUnavailableHandler.run(); return; @@ -876,14 +878,47 @@ public final class KeycloakModelUtils { } realm.getAuthenticationExecutionsStream(authFlow.getId()) - .map(AuthenticationExecutionModel::getFlowId) - .filter(Objects::nonNull) - .map(realm::getAuthenticationFlowById) - .forEachOrdered(subflow -> deepDeleteAuthenticationFlow(realm, subflow, flowUnavailableHandler, builtinFlowHandler)); + .forEachOrdered(authExecutor -> deepDeleteAuthenticationExecutor(session, realm, authExecutor, flowUnavailableHandler, builtinFlowHandler)); realm.removeAuthenticationFlow(authFlow); } + /** + * Recursively remove authentication executor (including sub-flows and configs) from the model storage + * + * @param session The keycloak session + * @param realm The realm + * @param authExecutor The authentication executor to remove + * @param flowUnavailableHandler Handler that will be executed when flow, sub-flow or executor is null + * @param builtinFlowHandler Handler that will be executed when flow is built-in flow + */ + public static void deepDeleteAuthenticationExecutor(KeycloakSession session, RealmModel realm, AuthenticationExecutionModel authExecutor, Runnable flowUnavailableHandler, Runnable builtinFlowHandler) { + if (authExecutor == null) { + flowUnavailableHandler.run(); + return; + } + + // recursively remove sub flows + if (authExecutor.getFlowId() != null) { + AuthenticationFlowModel authFlow = realm.getAuthenticationFlowById(authExecutor.getFlowId()); + deepDeleteAuthenticationFlow(session, realm, authFlow, flowUnavailableHandler, builtinFlowHandler); + } + + // remove the config if not shared + if (authExecutor.getAuthenticatorConfig() != null) { + DeployedConfigurationsManager configManager = new DeployedConfigurationsManager(session); + if (configManager.getDeployedAuthenticatorConfig(authExecutor.getAuthenticatorConfig()) == null) { + AuthenticatorConfigModel config = configManager.getAuthenticatorConfig(realm, authExecutor.getAuthenticatorConfig()); + if (config != null) { + realm.removeAuthenticatorConfig(config); + } + } + } + + // remove the executor at the end + realm.removeAuthenticatorExecution(authExecutor); + } + public static ClientScopeModel getClientScopeByName(RealmModel realm, String clientScopeName) { return realm.getClientScopesStream() .filter(clientScope -> Objects.equals(clientScopeName, clientScope.getName())) diff --git a/services/src/main/java/org/keycloak/services/resources/admin/AuthenticationManagementResource.java b/services/src/main/java/org/keycloak/services/resources/admin/AuthenticationManagementResource.java index 5438a018450..cff525353de 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/AuthenticationManagementResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/AuthenticationManagementResource.java @@ -350,10 +350,13 @@ public class AuthenticationManagementResource { public void deleteFlow(@Parameter(description = "Flow id") @PathParam("id") String id) { auth.realm().requireManageRealm(); - KeycloakModelUtils.deepDeleteAuthenticationFlow(realm, realm.getAuthenticationFlowById(id), - () -> { - throw new NotFoundException("Could not find flow with id"); - }, + AuthenticationFlowModel flow = realm.getAuthenticationFlowById(id); + if (flow == null) { + throw new NotFoundException("Flow not found"); + } + + KeycloakModelUtils.deepDeleteAuthenticationFlow(session, realm, flow, + () -> {}, // allow deleting even with missing references () -> { throw new BadRequestException("Can't delete built in flow"); } @@ -388,8 +391,9 @@ public class AuthenticationManagementResource { AuthenticationFlowModel flow = realm.getFlowByAlias(flowAlias); if (flow == null) { logger.debug("flow not found: " + flowAlias); - return Response.status(NOT_FOUND).build(); + throw new NotFoundException("Flow not found"); } + AuthenticationFlowModel copy = copyFlow(session, realm, flow, newName); data.put("id", copy.getId()); @@ -662,8 +666,7 @@ public class AuthenticationManagementResource { rep.setRequirement(execution.getRequirement().name()); rep.setFlowId(execution.getFlowId()); result.add(rep); - AuthenticationFlowModel subFlow = realm.getAuthenticationFlowById(execution.getFlowId()); - recurseExecutions(subFlow, result, level + 1); + recurseExecutions(flowRef, result, level + 1); } else { String providerId = execution.getAuthenticator(); ConfigurableAuthenticatorFactory factory = CredentialHelper.getConfigurableAuthenticatorFactory(session, providerId); @@ -944,19 +947,19 @@ public class AuthenticationManagementResource { if (model == null) { session.getTransactionManager().setRollbackOnly(); throw new NotFoundException("Illegal execution"); - } + AuthenticationFlowModel parentFlow = getParentFlow(model); if (parentFlow.isBuiltIn()) { throw new BadRequestException("It is illegal to remove execution from a built in flow"); } - if(model.getFlowId() != null) { - AuthenticationFlowModel nonTopLevelFlow = realm.getAuthenticationFlowById(model.getFlowId()); - realm.removeAuthenticationFlow(nonTopLevelFlow); - } - - realm.removeAuthenticatorExecution(model); + KeycloakModelUtils.deepDeleteAuthenticationExecutor(session, realm, model, + () -> {}, // allow deleting even with missing references + () -> { + throw new BadRequestException("It is illegal to remove execution from a built in flow"); + } + ); adminEvent.operation(OperationType.DELETE).resource(ResourceType.AUTH_EXECUTION).resourcePath(session.getContext().getUri()).success(); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/authentication/FlowTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/authentication/FlowTest.java index 4fe469305d2..f8a13cc8d4a 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/authentication/FlowTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/authentication/FlowTest.java @@ -20,7 +20,6 @@ package org.keycloak.testsuite.admin.authentication; import org.junit.Assert; import org.junit.Test; import org.keycloak.authentication.authenticators.browser.IdentityProviderAuthenticatorFactory; -import org.keycloak.common.Profile; import org.keycloak.common.util.StreamUtil; import org.keycloak.events.admin.OperationType; import org.keycloak.events.admin.ResourceType; @@ -30,7 +29,6 @@ import org.keycloak.representations.idm.AuthenticationExecutionInfoRepresentatio import org.keycloak.representations.idm.AuthenticationFlowRepresentation; import org.keycloak.representations.idm.AuthenticatorConfigRepresentation; import org.keycloak.representations.idm.OAuth2ErrorRepresentation; -import org.keycloak.testsuite.ProfileAssume; import org.keycloak.testsuite.admin.ApiUtil; import org.keycloak.testsuite.util.AdminEventPaths; import org.keycloak.testsuite.util.ContainerAssume; @@ -84,6 +82,29 @@ public class FlowTest extends AbstractAuthenticationTest { // Under the old code, this would throw an error because "grandchild" // was left in the database addFlowToParent("child", "grandchild"); + + authMgmtResource.deleteFlow(findFlowByAlias("Foo", authMgmtResource.getFlows()).getId()); + } + + @Test + public void testRemoveExecutionSubflow() { + createFlow(newFlow("Foo", "Foo flow", "generic", true, false)); + addFlowToParent("Foo", "child"); + addFlowToParent("child", "grandchild"); + + // remove the foo child but using the execution + List fooExecutions = authMgmtResource.getExecutions("Foo"); + AuthenticationExecutionInfoRepresentation childExececution = fooExecutions.stream() + .filter(r -> "child".equals(r.getDisplayName()) && r.getLevel() == 0).findAny().orElse(null); + assertNotNull(childExececution); + authMgmtResource.removeExecution(childExececution.getId()); + assertAdminEvents.clear(); + + // check subflows were removed and can be re-created + addFlowToParent("Foo", "child"); + addFlowToParent("child", "grandchild"); + + authMgmtResource.deleteFlow(findFlowByAlias("Foo", authMgmtResource.getFlows()).getId()); } private void addFlowToParent(String parentAlias, String childAlias) { @@ -315,6 +336,8 @@ public class FlowTest extends AbstractAuthenticationTest { authMgmtResource.addExecutionFlow("parent", params); assertAdminEvents.assertEvent(testRealmId, OperationType.CREATE, AdminEventPaths.authAddExecutionFlowPath("parent"), params, ResourceType.AUTH_EXECUTION_FLOW); + + authMgmtResource.deleteFlow(findFlowByAlias("parent", authMgmtResource.getFlows()).getId()); } @Test