diff --git a/services/src/main/java/org/keycloak/models/workflow/WorkflowsManager.java b/services/src/main/java/org/keycloak/models/workflow/WorkflowsManager.java index 0c48b78ba5c..4b617348f31 100644 --- a/services/src/main/java/org/keycloak/models/workflow/WorkflowsManager.java +++ b/services/src/main/java/org/keycloak/models/workflow/WorkflowsManager.java @@ -91,11 +91,6 @@ public class WorkflowsManager { for (int i = 0; i < steps.size(); i++) { WorkflowStep step = steps.get(i); - if (workflow.getId().equals(parentId)) { - // only validate top-level steps, sub-steps are validated as part of the parent step validation - validateStep(workflow, step); - } - // assign priority based on index. step.setPriority(i + 1); @@ -311,6 +306,7 @@ public class WorkflowsManager { } public void updateWorkflow(Workflow workflow, MultivaluedHashMap config) { + validateWorkflow(toRepresentation(workflow), config); ComponentModel component = getWorkflowComponent(workflow.getId()); component.setConfig(config); getRealm().updateComponent(component); @@ -398,7 +394,7 @@ public class WorkflowsManager { List steps = new ArrayList<>(); for (WorkflowStepRepresentation stepRep : rep.getSteps()) { - steps.add(toModel(stepRep)); + steps.add(toModel(workflow, stepRep)); } addSteps(workflow, workflow.getId(), steps); @@ -441,16 +437,6 @@ public class WorkflowsManager { } } - private WorkflowStep toModel(WorkflowStepRepresentation rep) { - List subSteps = new ArrayList<>(); - - for (WorkflowStepRepresentation subStep : ofNullable(rep.getSteps()).orElse(List.of())) { - subSteps.add(toModel(subStep)); - } - - return new WorkflowStep(rep.getUses(), rep.getConfig(), subSteps); - } - public void bind(Workflow workflow, ResourceType type, String resourceId) { processEvent(List.of(workflow), new AdhocWorkflowEvent(type, resourceId)); } @@ -461,7 +447,11 @@ public class WorkflowsManager { return type.resolveResource(session, resourceId); } - private void validateStep(Workflow workflow, WorkflowStep step) throws ModelValidationException { + private void validateStep(Workflow workflow, WorkflowStep step, boolean topLevel) throws ModelValidationException { + if (step.getAfter() < 0) { + throw new ModelValidationException("Step 'after' time condition cannot be negative."); + } + boolean isAggregatedStep = !step.getSteps().isEmpty(); boolean isScheduledWorkflow = workflow.isScheduled(); @@ -484,13 +474,15 @@ public class WorkflowsManager { throw new ModelValidationException("Sub-steps of aggregated step cannot have time conditions."); } } else { - if (isScheduledWorkflow) { - if (step.getConfig().getFirst(CONFIG_AFTER) == null || step.getAfter() < 0) { + if (isScheduledWorkflow && topLevel) { + if (step.getConfig().getFirst(CONFIG_AFTER) == null) { throw new ModelValidationException("All steps of scheduled workflow must have a valid 'after' time condition."); } - } else { // immediate workflow + } else { // immediate workflow | sub-step of aggregated step if (step.getConfig().getFirst(CONFIG_AFTER) != null) { - throw new ModelValidationException("Immediate workflow step cannot have a time condition."); + throw new ModelValidationException(topLevel ? + "Immediate workflow step cannot have a time condition." : + "Sub-step of aggregated step cannot have a time conditions."); } } } @@ -586,14 +578,21 @@ public class WorkflowsManager { } } - public WorkflowStep toStepModel(WorkflowStepRepresentation rep) { + public WorkflowStep toModel(Workflow workflow, WorkflowStepRepresentation rep) { + return toModel(workflow, rep, true); + } + + private WorkflowStep toModel(Workflow workflow, WorkflowStepRepresentation rep, boolean topLevel) { List subSteps = new ArrayList<>(); for (WorkflowStepRepresentation subStep : ofNullable(rep.getSteps()).orElse(List.of())) { - subSteps.add(toStepModel(subStep)); + subSteps.add(toModel(workflow, subStep, false)); } - return new WorkflowStep(rep.getUses(), rep.getConfig(), subSteps); + WorkflowStep step = new WorkflowStep(rep.getUses(), rep.getConfig(), subSteps); + validateStep(workflow, step, topLevel); + + return step; } public WorkflowConditionProvider getConditionProvider(String providerId, MultivaluedHashMap modelConfig) { diff --git a/services/src/main/java/org/keycloak/workflow/admin/resource/WorkflowResource.java b/services/src/main/java/org/keycloak/workflow/admin/resource/WorkflowResource.java index 104f22f2de8..5aadd33b03a 100644 --- a/services/src/main/java/org/keycloak/workflow/admin/resource/WorkflowResource.java +++ b/services/src/main/java/org/keycloak/workflow/admin/resource/WorkflowResource.java @@ -39,6 +39,9 @@ public class WorkflowResource { } } + /** + * Update the workflow configuration. The method does not update the workflow steps. + */ @PUT public void update(WorkflowRepresentation rep) { try { diff --git a/services/src/main/java/org/keycloak/workflow/admin/resource/WorkflowStepsResource.java b/services/src/main/java/org/keycloak/workflow/admin/resource/WorkflowStepsResource.java index b3dcf93bea5..8417b958952 100644 --- a/services/src/main/java/org/keycloak/workflow/admin/resource/WorkflowStepsResource.java +++ b/services/src/main/java/org/keycloak/workflow/admin/resource/WorkflowStepsResource.java @@ -41,10 +41,12 @@ import org.eclipse.microprofile.openapi.annotations.responses.APIResponse; import org.eclipse.microprofile.openapi.annotations.responses.APIResponses; import org.eclipse.microprofile.openapi.annotations.tags.Tag; +import org.keycloak.models.ModelException; import org.keycloak.models.workflow.WorkflowStep; import org.keycloak.models.workflow.Workflow; import org.keycloak.models.workflow.WorkflowsManager; import org.keycloak.representations.workflows.WorkflowStepRepresentation; +import org.keycloak.services.ErrorResponse; /** * Resource for managing steps within a workflow. @@ -105,13 +107,16 @@ public class WorkflowStepsResource { @Parameter(description = "Position to insert the step at (0-based index). If not specified, step is added at the end.") @QueryParam("position") Integer position) { if (stepRep == null) { - throw new BadRequestException("Step representation cannot be null"); + throw ErrorResponse.error("Step representation cannot be null", Response.Status.BAD_REQUEST); } + try { + WorkflowStep step = workflowsManager.toModel(workflow, stepRep); + WorkflowStep addedStep = workflowsManager.addStepToWorkflow(workflow.getId(), step, position); - WorkflowStep step = workflowsManager.toStepModel(stepRep); - WorkflowStep addedStep = workflowsManager.addStepToWorkflow(workflow.getId(), step, position); - - return Response.ok(workflowsManager.toRepresentation(addedStep)).build(); + return Response.ok(workflowsManager.toRepresentation(addedStep)).build(); + } catch (ModelException e) { + throw ErrorResponse.error(e.getMessage(), Response.Status.BAD_REQUEST); + } } /** diff --git a/tests/base/src/test/java/org/keycloak/tests/admin/model/workflow/AggregatedStepTest.java b/tests/base/src/test/java/org/keycloak/tests/admin/model/workflow/AggregatedStepTest.java index c4d70ba5ae2..0f0fae7de5e 100644 --- a/tests/base/src/test/java/org/keycloak/tests/admin/model/workflow/AggregatedStepTest.java +++ b/tests/base/src/test/java/org/keycloak/tests/admin/model/workflow/AggregatedStepTest.java @@ -50,7 +50,7 @@ public class AggregatedStepTest { @Test public void testCreate() { - managedRealm.admin().workflows().create(WorkflowRepresentation.create() + try (Response response = managedRealm.admin().workflows().create(WorkflowRepresentation.create() .of(UserCreationTimeWorkflowProviderFactory.ID) .withSteps( WorkflowStepRepresentation.create().of(AggregatedStepProviderFactory.ID) @@ -63,32 +63,33 @@ public class AggregatedStepTest { .of(DisableUserStepProviderFactory.ID) .build() ).build()) - .build()).close(); - - List workflows = managedRealm.admin().workflows().list(); - assertThat(workflows, hasSize(1)); - WorkflowRepresentation workflow = workflows.get(0); - assertThat(workflow.getSteps(), hasSize(1)); - WorkflowStepRepresentation aggregatedStep = workflow.getSteps().get(0); - assertThat(aggregatedStep.getUses(), is(AggregatedStepProviderFactory.ID)); - List steps = aggregatedStep.getSteps(); - assertThat(steps, hasSize(2)); - assertStep(steps, SetUserAttributeStepProviderFactory.ID, a -> { - assertNotNull(a.getConfig()); - assertThat(a.getConfig().isEmpty(), is(false)); - assertThat(a.getConfig(), hasEntry("priority", List.of("1"))); - assertThat(a.getConfig(), hasEntry("message", List.of("message"))); - }); - assertStep(steps, DisableUserStepProviderFactory.ID, a -> { - assertNotNull(a.getConfig()); - assertThat(a.getConfig().isEmpty(), is(false)); - assertThat(a.getConfig(), hasEntry("priority", List.of("2"))); - }); + .build())) { + assertThat(response.getStatus(), is(Status.CREATED.getStatusCode())); + List workflows = managedRealm.admin().workflows().list(); + assertThat(workflows, hasSize(1)); + WorkflowRepresentation workflow = workflows.get(0); + assertThat(workflow.getSteps(), hasSize(1)); + WorkflowStepRepresentation aggregatedStep = workflow.getSteps().get(0); + assertThat(aggregatedStep.getUses(), is(AggregatedStepProviderFactory.ID)); + List steps = aggregatedStep.getSteps(); + assertThat(steps, hasSize(2)); + assertStep(steps, SetUserAttributeStepProviderFactory.ID, a -> { + assertNotNull(a.getConfig()); + assertThat(a.getConfig().isEmpty(), is(false)); + assertThat(a.getConfig(), hasEntry("priority", List.of("1"))); + assertThat(a.getConfig(), hasEntry("message", List.of("message"))); + }); + assertStep(steps, DisableUserStepProviderFactory.ID, a -> { + assertNotNull(a.getConfig()); + assertThat(a.getConfig().isEmpty(), is(false)); + assertThat(a.getConfig(), hasEntry("priority", List.of("2"))); + }); + } } @Test public void testCreateAggregatedStepAsSubStep() { - managedRealm.admin().workflows().create(WorkflowRepresentation.create() + try (Response response = managedRealm.admin().workflows().create(WorkflowRepresentation.create() .of(UserCreationTimeWorkflowProviderFactory.ID) .withSteps( WorkflowStepRepresentation.create().of(AggregatedStepProviderFactory.ID) @@ -110,10 +111,11 @@ public class AggregatedStepTest { .of(DisableUserStepProviderFactory.ID) .build() ).build()) - .build()).close(); - - List workflows = managedRealm.admin().workflows().list(); - assertThat(workflows, hasSize(1)); + .build())) { + assertThat(response.getStatus(), is(Status.CREATED.getStatusCode())); + List workflows = managedRealm.admin().workflows().list(); + assertThat(workflows, hasSize(1)); + } } @Test @@ -157,7 +159,7 @@ public class AggregatedStepTest { ).build()) .build())) { assertThat(response.getStatus(), is(Status.BAD_REQUEST.getStatusCode())); - assertThat(response.readEntity(ErrorRepresentation.class).getErrorMessage(), equalTo("Step provider " + SetUserAttributeStepProviderFactory.ID + " does not support aggregated steps")); + assertThat(response.readEntity(ErrorRepresentation.class).getErrorMessage(), equalTo("Sub-step of aggregated step cannot have a time conditions.")); } } diff --git a/tests/base/src/test/java/org/keycloak/tests/admin/model/workflow/WorkflowManagementTest.java b/tests/base/src/test/java/org/keycloak/tests/admin/model/workflow/WorkflowManagementTest.java index 08b6d8f2074..d87cefc6797 100644 --- a/tests/base/src/test/java/org/keycloak/tests/admin/model/workflow/WorkflowManagementTest.java +++ b/tests/base/src/test/java/org/keycloak/tests/admin/model/workflow/WorkflowManagementTest.java @@ -28,6 +28,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.keycloak.representations.workflows.WorkflowConstants.CONFIG_SCHEDULED; import java.time.Duration; import java.util.Arrays; @@ -629,6 +630,65 @@ public class WorkflowManagementTest { } } + @Test + public void testUpdateWorkflowFromImmediateToScheduled() { + // Create an immediate workflow + WorkflowSetRepresentation workflows = WorkflowRepresentation.create() + .of(UserCreationTimeWorkflowProviderFactory.ID) + .immediate() + .withSteps( + WorkflowStepRepresentation.create().of(NotifyUserStepProviderFactory.ID) + .build(), + WorkflowStepRepresentation.create().of(DisableUserStepProviderFactory.ID) + .build() + ).build(); + + WorkflowsResource workflowsResource = managedRealm.admin().workflows(); + try (Response response = workflowsResource.create(workflows)) { + assertThat(response.getStatus(), is(Response.Status.CREATED.getStatusCode())); + } + + WorkflowRepresentation workflow = workflowsResource.list().get(0); + // Attempt to update the workflow to scheduled + workflow.getConfig().putSingle(CONFIG_SCHEDULED, "true"); + + try (Response response = workflowsResource.workflow(workflow.getId()).update(workflow)) { + assertThat(response.getStatus(), is(Response.Status.BAD_REQUEST.getStatusCode())); + String error = response.readEntity(String.class); + assertThat(error, containsString("Scheduled workflow cannot have steps without time conditions.")); + } + } + + @Test + public void testUpdateWorkflowFromScheduledToImmediate() { + // Create a scheduled workflow + WorkflowSetRepresentation workflows = WorkflowRepresentation.create() + .of(UserCreationTimeWorkflowProviderFactory.ID) + .withSteps( + WorkflowStepRepresentation.create().of(NotifyUserStepProviderFactory.ID) + .after(Duration.ofDays(1)) + .build(), + WorkflowStepRepresentation.create().of(DisableUserStepProviderFactory.ID) + .after(Duration.ofDays(1)) + .build() + ).build(); + + WorkflowsResource workflowsResource = managedRealm.admin().workflows(); + try (Response response = workflowsResource.create(workflows)) { + assertThat(response.getStatus(), is(Response.Status.CREATED.getStatusCode())); + } + + WorkflowRepresentation workflow = workflowsResource.list().get(0); + // Attempt to update the workflow to immediate + workflow.setScheduled(false); + + try (Response response = workflowsResource.workflow(workflow.getId()).update(workflow)) { + assertThat(response.getStatus(), is(Response.Status.BAD_REQUEST.getStatusCode())); + String error = response.readEntity(String.class); + assertThat(error, containsString("Immediate workflow cannot have steps with time conditions.")); + } + } + @Test public void testCreateScheduledWorkflowWithoutTimeConditions() { WorkflowSetRepresentation workflows = WorkflowRepresentation.create() @@ -670,8 +730,8 @@ public class WorkflowManagementTest { } @Test - public void testFailCreateNegativeTime() { - try (Response response = managedRealm.admin().workflows().create(WorkflowRepresentation.create() + public void testFailCreateWorkflowWithNegativeTime() { + WorkflowSetRepresentation workflows = WorkflowRepresentation.create() .of(UserCreationTimeWorkflowProviderFactory.ID) .withSteps( WorkflowStepRepresentation.create().of(SetUserAttributeStepProviderFactory.ID) @@ -685,9 +745,10 @@ public class WorkflowManagementTest { .of(DisableUserStepProviderFactory.ID) .build() ).build()) - .build())) { + .build(); + try (Response response = managedRealm.admin().workflows().create(workflows)) { assertThat(response.getStatus(), is(Response.Status.BAD_REQUEST.getStatusCode())); - assertThat(response.readEntity(ErrorRepresentation.class).getErrorMessage(), equalTo("Step provider " + SetUserAttributeStepProviderFactory.ID + " does not support aggregated steps")); + assertThat(response.readEntity(ErrorRepresentation.class).getErrorMessage(), equalTo("Step 'after' time condition cannot be negative.")); } } diff --git a/tests/base/src/test/java/org/keycloak/tests/admin/model/workflow/WorkflowStepManagementTest.java b/tests/base/src/test/java/org/keycloak/tests/admin/model/workflow/WorkflowStepManagementTest.java index 60f956863c3..0e0b3120e63 100644 --- a/tests/base/src/test/java/org/keycloak/tests/admin/model/workflow/WorkflowStepManagementTest.java +++ b/tests/base/src/test/java/org/keycloak/tests/admin/model/workflow/WorkflowStepManagementTest.java @@ -44,13 +44,23 @@ import org.keycloak.testframework.remote.runonserver.InjectRunOnServer; import org.keycloak.testframework.remote.runonserver.RunOnServerClient; import jakarta.ws.rs.core.Response; +import org.keycloak.testframework.util.ApiUtil; + import java.time.Duration; import java.util.List; import java.util.Map; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.nullValue; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.keycloak.representations.workflows.WorkflowConstants.CONFIG_SCHEDULED; @KeycloakIntegrationTest(config = WorkflowsServerConfig.class) public class WorkflowStepManagementTest { @@ -126,6 +136,64 @@ public class WorkflowStepManagementTest { assertTrue(foundOurStep, "Our added step should be present in the workflow"); } + @Test + public void testAddStepWithTimeToImmediateWorkflow() { + workflowsResource.create(WorkflowRepresentation.create() + .of(UserCreationTimeWorkflowProviderFactory.ID) + .onEvent(ResourceOperationType.USER_ADD.toString()) + .name("immediate-workflow") + .immediate() + .withSteps( + WorkflowStepRepresentation.create().of(NotifyUserStepProviderFactory.ID).build() + ).build()).close(); + + String immediateWorkflowId = workflowsResource.list().stream() + .filter(wf -> "immediate-workflow".equals(wf.getName())) + .findFirst() + .map(WorkflowRepresentation::getId) + .orElse(null); + assertThat(immediateWorkflowId, notNullValue()); + + WorkflowRepresentation representation = workflowsResource.workflow(immediateWorkflowId).toRepresentation(); + assertThat(representation.getConfig().getFirst(CONFIG_SCHEDULED), equalTo("false")); + + // Attempt to add a step with 'after' time (should be rejected) + WorkflowStepRepresentation stepRep = new WorkflowStepRepresentation(); + stepRep.setUses(DisableUserStepProviderFactory.ID); + stepRep.setConfig("name", "Invalid Step"); + stepRep.setConfig("after", String.valueOf(Duration.ofDays(30).toMillis())); + + try (Response response = workflowsResource.workflow(immediateWorkflowId).steps().create(stepRep)) { + assertThat(response.getStatus(), is(Response.Status.BAD_REQUEST.getStatusCode())); + String errorMessage = response.readEntity(String.class); + assertThat(errorMessage, containsString("Immediate workflow step cannot have a time condition.")); + } + // Verify step was not added (should still be only the original setup step) + List allSteps = workflowsResource.workflow(immediateWorkflowId).steps().list(); + assertThat(allSteps, hasSize(1)); + } + + @Test + public void testAddStepWithoutTimeToScheduledWorkflow() { + WorkflowRepresentation representation = workflowsResource.workflow(workflowId).toRepresentation(); + assertThat(representation.getConfig().getFirst(CONFIG_SCHEDULED), nullValue());//immediate + + // Attempt to add a step without 'after' time (should be rejected) + WorkflowStepRepresentation stepRep = WorkflowStepRepresentation.create() + .of(NotifyUserStepProviderFactory.ID) + .build(); + + try (Response response = workflowsResource.workflow(workflowId).steps().create(stepRep)) { + assertThat(response.getStatus(), is(Response.Status.BAD_REQUEST.getStatusCode())); + String errorMessage = response.readEntity(String.class); + assertThat(errorMessage, containsString("All steps of scheduled workflow must have a valid 'after' time condition.")); + } + + // Verify step was not added (should still be only the original setup step) + List allSteps = workflowsResource.workflow(workflowId).steps().list(); + assertThat(allSteps, hasSize(1)); + } + @Test public void testRemoveStepFromWorkflow() { WorkflowResource workflow = workflowsResource.workflow(workflowId);