Remove recursively when deleting an authentication executor

Closes #24795

Signed-off-by: rmartinc <rmartinc@redhat.com>

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",

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)
+                .forEachOrdered(authExecutor -> deepDeleteAuthenticationExecutor(session, realm, authExecutor, flowUnavailableHandler, builtinFlowHandler));
-                .filter(Objects::nonNull)
-                .map(realm::getAuthenticationFlowById)
-                .forEachOrdered(subflow -> deepDeleteAuthenticationFlow(realm, subflow, 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()))

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),
+        AuthenticationFlowModel flow = realm.getAuthenticationFlowById(id);
-                () -> {
+        if (flow == null) {
-                    throw new NotFoundException("Could not find flow with id");
+            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(flowRef, result, level + 1);
-                recurseExecutions(subFlow, 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) {
+        KeycloakModelUtils.deepDeleteAuthenticationExecutor(session, realm, model,
-        	AuthenticationFlowModel nonTopLevelFlow = realm.getAuthenticationFlowById(model.getFlowId());
+                () -> {}, // allow deleting even with missing references
-        	realm.removeAuthenticationFlow(nonTopLevelFlow);
+                () -> {
-        }
+                    throw new BadRequestException("It is illegal to remove execution from a built in flow");
-
+                }
-        realm.removeAuthenticatorExecution(model);
+        );
 
         adminEvent.operation(OperationType.DELETE).resource(ResourceType.AUTH_EXECUTION).resourcePath(session.getContext().getUri()).success();
     }

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<AuthenticationExecutionInfoRepresentation> 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