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 087022fca3..0128425953 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 @@ -251,7 +251,11 @@ public class AuthenticationManagementResource { @NoCache public void deleteFlow(@PathParam("id") String id) { auth.requireManage(); - + + deleteFlow(id, true); + } + + private void deleteFlow(String id, boolean isTopMostLevel) { AuthenticationFlowModel flow = realm.getAuthenticationFlowById(id); if (flow == null) { throw new NotFoundException("Could not find flow with id"); @@ -259,18 +263,17 @@ public class AuthenticationManagementResource { if (flow.isBuiltIn()) { throw new BadRequestException("Can't delete built in flow"); } + List executions = realm.getAuthenticationExecutions(id); for (AuthenticationExecutionModel execution : executions) { - if(execution.getFlowId() != null) { - AuthenticationFlowModel nonTopLevelFlow = realm.getAuthenticationFlowById(execution.getFlowId()); - realm.removeAuthenticationFlow(nonTopLevelFlow); - } - realm.removeAuthenticatorExecution(execution); + if(execution.getFlowId() != null) { + deleteFlow(execution.getFlowId(), false); + } } realm.removeAuthenticationFlow(flow); // Use just one event for top-level flow. Using separate events won't work properly for flows of depth 2 or bigger - adminEvent.operation(OperationType.DELETE).resourcePath(uriInfo).success(); + if (isTopMostLevel) adminEvent.operation(OperationType.DELETE).resourcePath(uriInfo).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 90f88745a9..bce911703f 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 @@ -37,6 +37,33 @@ import java.util.Map; */ public class FlowTest extends AbstractAuthenticationTest { + // KEYCLOAK-3681: Delete top flow doesn't delete all subflows + @Test + public void testRemoveSubflows() { + authMgmtResource.createFlow(newFlow("Foo", "Foo flow", "generic", true, false)); + addFlowToParent("Foo", "child"); + addFlowToParent("child", "grandchild"); + + List flows = authMgmtResource.getFlows(); + AuthenticationFlowRepresentation found = findFlowByAlias("Foo", flows); + authMgmtResource.deleteFlow(found.getId()); + + authMgmtResource.createFlow(newFlow("Foo", "Foo flow", "generic", true, false)); + addFlowToParent("Foo", "child"); + + // Under the old code, this would throw an error because "grandchild" + // was left in the database + addFlowToParent("child", "grandchild"); + } + + private void addFlowToParent(String parentAlias, String childAlias) { + Map data = new HashMap<>(); + data.put("alias", childAlias); + data.put("type", "generic"); + data.put("description", childAlias + " flow"); + authMgmtResource.addExecutionFlow(parentAlias, data); + } + @Test public void testAddRemoveFlow() {