From 55556fc63c5bb553571be3ecdd53e8ecd7f0409d Mon Sep 17 00:00:00 2001 From: Stan Silvert Date: Wed, 16 Nov 2016 12:43:11 -0500 Subject: [PATCH 1/2] KEYCLOAK-3681: Delete top flow doesn't remove all subflows --- .../AuthenticationManagementResource.java | 17 +++++++------ .../admin/authentication/FlowTest.java | 24 +++++++++++++++++++ 2 files changed, 34 insertions(+), 7 deletions(-) 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..f43812e733 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,30 @@ 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"); + 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() { From a0ae8c126ef05b9cb86be67177deba4e54fe3931 Mon Sep 17 00:00:00 2001 From: Stan Silvert Date: Wed, 16 Nov 2016 12:45:34 -0500 Subject: [PATCH 2/2] Add comment to test --- .../org/keycloak/testsuite/admin/authentication/FlowTest.java | 3 +++ 1 file changed, 3 insertions(+) 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 f43812e733..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 @@ -50,6 +50,9 @@ public class FlowTest extends AbstractAuthenticationTest { 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"); }