From 44b4235b5056b61a5b8593dfb519ba9e7e058528 Mon Sep 17 00:00:00 2001 From: Vlasta Ramik Date: Thu, 18 Sep 2025 14:51:04 +0200 Subject: [PATCH] Validation for immediate workflows Closes #42382 Signed-off-by: vramik --- .../keycloak/models/workflow/Workflow.java | 11 ++- ...licyEvent.java => AdhocWorkflowEvent.java} | 0 .../models/workflow/WorkflowsManager.java | 90 +++++++++++++----- .../admin/resource/WorkflowResource.java | 12 +++ .../model/workflow/AdhocWorkflowTest.java | 51 ++++++---- .../model/workflow/AggregatedStepTest.java | 57 ++++++++++- .../workflow/WorkflowManagementTest.java | 95 ++++++++++++++++++- 7 files changed, 264 insertions(+), 52 deletions(-) rename services/src/main/java/org/keycloak/models/workflow/{AdhocResourcePolicyEvent.java => AdhocWorkflowEvent.java} (100%) diff --git a/server-spi-private/src/main/java/org/keycloak/models/workflow/Workflow.java b/server-spi-private/src/main/java/org/keycloak/models/workflow/Workflow.java index a63bb0361c9..351e37bc130 100644 --- a/server-spi-private/src/main/java/org/keycloak/models/workflow/Workflow.java +++ b/server-spi-private/src/main/java/org/keycloak/models/workflow/Workflow.java @@ -25,6 +25,9 @@ import org.keycloak.component.ComponentModel; public class Workflow { + public static final String SCHEDULED_KEY = "scheduled"; + public static final String RECURRING_KEY = "recurring"; + private MultivaluedHashMap config; private String providerId; private String id; @@ -70,18 +73,18 @@ public class Workflow { } public boolean isRecurring() { - return config != null && Boolean.parseBoolean(config.getFirst("recurring")); + return config != null && Boolean.parseBoolean(config.getFirst(RECURRING_KEY)); } public boolean isScheduled() { - return config != null && Boolean.parseBoolean(config.getFirstOrDefault("scheduled", "true")); + return config != null && Boolean.parseBoolean(config.getFirstOrDefault(SCHEDULED_KEY, "true")); } public Long getNotBefore() { return notBefore; } - public void setNotBefore(Long milliseconds) { - this.notBefore = milliseconds; + public void setNotBefore(Long notBefore) { + this.notBefore = notBefore; } } diff --git a/services/src/main/java/org/keycloak/models/workflow/AdhocResourcePolicyEvent.java b/services/src/main/java/org/keycloak/models/workflow/AdhocWorkflowEvent.java similarity index 100% rename from services/src/main/java/org/keycloak/models/workflow/AdhocResourcePolicyEvent.java rename to services/src/main/java/org/keycloak/models/workflow/AdhocWorkflowEvent.java 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 15174c88d8f..ba4a27ccd62 100644 --- a/services/src/main/java/org/keycloak/models/workflow/WorkflowsManager.java +++ b/services/src/main/java/org/keycloak/models/workflow/WorkflowsManager.java @@ -17,16 +17,6 @@ package org.keycloak.models.workflow; -import static java.util.Optional.ofNullable; - -import java.util.ArrayList; -import java.util.List; -import java.util.Map; -import java.util.Map.Entry; -import java.util.Objects; -import java.util.Optional; -import java.util.stream.Stream; - import jakarta.ws.rs.BadRequestException; import org.jboss.logging.Logger; import org.keycloak.common.Profile; @@ -37,11 +27,24 @@ import org.keycloak.component.ComponentModel; import org.keycloak.models.KeycloakSession; import org.keycloak.models.ModelValidationException; import org.keycloak.models.RealmModel; -import org.keycloak.models.workflow.WorkflowStateProvider.ScheduledStep; import org.keycloak.models.utils.KeycloakModelUtils; -import org.keycloak.representations.workflows.WorkflowStepRepresentation; +import org.keycloak.models.workflow.WorkflowStateProvider.ScheduledStep; import org.keycloak.representations.workflows.WorkflowConditionRepresentation; import org.keycloak.representations.workflows.WorkflowRepresentation; +import org.keycloak.representations.workflows.WorkflowStepRepresentation; + +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Map.Entry; +import java.util.Objects; +import java.util.Optional; +import java.util.stream.Stream; + +import static java.util.Optional.ofNullable; +import static org.keycloak.models.workflow.Workflow.RECURRING_KEY; +import static org.keycloak.models.workflow.Workflow.SCHEDULED_KEY; +import static org.keycloak.models.workflow.WorkflowStep.AFTER_KEY; public class WorkflowsManager { @@ -85,6 +88,8 @@ public class WorkflowsManager { for (int i = 0; i < steps.size(); i++) { WorkflowStep step = steps.get(i); + validateStep(workflow, step); + // assign priority based on index. step.setPriority(i + 1); @@ -122,8 +127,6 @@ public class WorkflowsManager { persisted.setSteps(step.getSteps()); - validateStep(persisted); - return persisted; } @@ -334,6 +337,8 @@ public class WorkflowsManager { MultivaluedHashMap config = ofNullable(rep.getConfig()).orElse(new MultivaluedHashMap<>()); List conditions = ofNullable(rep.getConditions()).orElse(List.of()); + validateWorkflow(rep, config); + for (WorkflowConditionRepresentation condition : conditions) { String conditionProviderId = condition.getProviderId(); config.computeIfAbsent("conditions", key -> new ArrayList<>()).add(conditionProviderId); @@ -355,6 +360,27 @@ public class WorkflowsManager { return workflow; } + private void validateWorkflow(WorkflowRepresentation rep, MultivaluedHashMap config) { + // Validations: + // workflow cannot be both immediate and recurring + // immediate workflow cannot have time conditions + // all steps of scheduled workflow must have time condition + + boolean isImmediate = config.containsKey(SCHEDULED_KEY) && !Boolean.parseBoolean(config.getFirst(SCHEDULED_KEY)); + boolean isRecurring = config.containsKey(RECURRING_KEY) && Boolean.parseBoolean(config.getFirst(RECURRING_KEY)); + boolean hasTimeCondition = rep.getSteps().stream().allMatch(step -> step.getConfig() != null + && step.getConfig().containsKey(AFTER_KEY)); + if (isImmediate && isRecurring) { + throw new WorkflowInvalidStateException("Workflow cannot be both immediate and recurring."); + } + if (isImmediate && hasTimeCondition) { + throw new WorkflowInvalidStateException("Immediate workflow cannot have steps with time conditions."); + } + if (!isImmediate && !hasTimeCondition) { + throw new WorkflowInvalidStateException("Scheduled workflow cannot have steps without time conditions."); + } + } + private WorkflowStep toModel(WorkflowStepRepresentation rep) { List subSteps = new ArrayList<>(); @@ -375,17 +401,35 @@ public class WorkflowsManager { return type.resolveResource(session, resourceId); } - private void validateStep(WorkflowStep persisted) throws ModelValidationException { - List aggregated = persisted.getSteps(); + private void validateStep(Workflow workflow, WorkflowStep step) throws ModelValidationException { + boolean isAggregatedStep = !step.getSteps().isEmpty(); + boolean isScheduledWorkflow = workflow.isScheduled(); - if (!aggregated.isEmpty()) { - WorkflowStepProvider provider = getStepProvider(persisted); - - if (!(provider instanceof AggregatedStepProvider)) { - // for now, only AggregatedActionProvider supports having sub-actions but we might want to support - // in the future more actions from having sub-actions by querying the capability from the provider or via + if (isAggregatedStep) { + if (!step.getProviderId().equals(AggregatedStepProviderFactory.ID)) { + // for now, only AggregatedStepProvider supports having sub-steps, but we might want to support + // in the future more steps from having sub-steps by querying the capability from the provider or via // a marker interface - throw new ModelValidationException("Action provider " + persisted.getProviderId() + " does not support aggregated actions"); + throw new ModelValidationException("Step provider " + step.getProviderId() + " does not support aggregated steps"); + } + + List subSteps = step.getSteps(); + // for each sub-step (in case it's not aggregated step on its own) check all it's sub-steps do not have + // time conditions, all its sub-steps are meant to be run at once + if (subSteps.stream().anyMatch(subStep -> + subStep.getConfig().getFirst(AFTER_KEY) != null && + !subStep.getProviderId().equals(AggregatedStepProviderFactory.ID))) { + throw new ModelValidationException("Sub-steps of aggregated step cannot have time conditions."); + } + } else { + if (isScheduledWorkflow) { + if (step.getConfig().getFirst(AFTER_KEY) == null || step.getAfter() < 0) { + throw new ModelValidationException("All steps of scheduled workflow must have a valid 'after' time condition."); + } + } else { // immediate workflow + if (step.getConfig().getFirst(AFTER_KEY) != null) { + throw new ModelValidationException("Immediate workflow step cannot have a time condition."); + } } } } 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 a21a82eb9eb..25d4f4693ea 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 @@ -54,6 +54,15 @@ public class WorkflowResource { return manager.toRepresentation(workflow); } + /** + * Bind the workflow to the resource. + * + * @param type the resource type + * @param resourceId the resource id + * @param notBefore optional notBefore time in milliseconds to schedule the first workflow step, + * it overrides the first workflow step time configuration (after). + * if set and the workflow is not scheduled (immediate) a Bad Request response will be returned + */ @POST @Consumes(MediaType.APPLICATION_JSON) @Path("bind/{type}/{resourceId}") @@ -65,6 +74,9 @@ public class WorkflowResource { } if (notBefore != null) { + if (!workflow.isScheduled()) { + throw ErrorResponse.error("Immediate workflows does not support binding with provided time.", Response.Status.BAD_REQUEST); + } workflow.setNotBefore(notBefore); } diff --git a/tests/base/src/test/java/org/keycloak/tests/admin/model/workflow/AdhocWorkflowTest.java b/tests/base/src/test/java/org/keycloak/tests/admin/model/workflow/AdhocWorkflowTest.java index 6b9a77a6b1d..d7e31ef8650 100644 --- a/tests/base/src/test/java/org/keycloak/tests/admin/model/workflow/AdhocWorkflowTest.java +++ b/tests/base/src/test/java/org/keycloak/tests/admin/model/workflow/AdhocWorkflowTest.java @@ -1,7 +1,9 @@ package org.keycloak.tests.admin.model.workflow; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.hasSize; +import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; @@ -9,6 +11,7 @@ import static org.junit.jupiter.api.Assertions.assertNull; import java.time.Duration; import java.util.List; +import jakarta.ws.rs.BadRequestException; import jakarta.ws.rs.core.Response; import org.junit.jupiter.api.Test; import org.keycloak.common.util.Time; @@ -49,6 +52,7 @@ public class AdhocWorkflowTest { .withSteps(WorkflowStepRepresentation.create() .of(SetUserAttributeStepProviderFactory.ID) .withConfig("message", "message") + .after(Duration.ofDays(1)) .build()) .build()).close(); @@ -60,6 +64,31 @@ public class AdhocWorkflowTest { assertThat(aggregatedStep.getProviderId(), is(SetUserAttributeStepProviderFactory.ID)); } + @Test + public void testBindAdHocScheduledWithImmediateWorkflow() { + managedRealm.admin().workflows().create(WorkflowRepresentation.create() + .of(EventBasedWorkflowProviderFactory.ID) + .immediate() + .withSteps(WorkflowStepRepresentation.create() + .of(SetUserAttributeStepProviderFactory.ID) + .withConfig("message", "message") + .build()) + .build()).close(); + + List workflows = managedRealm.admin().workflows().list(); + assertThat(workflows, hasSize(1)); + WorkflowRepresentation workflow = workflows.get(0); + + try (Response response = managedRealm.admin().users().create(getUserRepresentation("alice", "Alice", "Wonderland", "alice@wornderland.org"))) { + String id = ApiUtil.getCreatedId(response); + try { + managedRealm.admin().workflows().workflow(workflow.getId()).bind(ResourceType.USERS.name(), id, Duration.ofDays(5).toMillis()); + } catch (Exception e) { + assertThat(e, instanceOf(BadRequestException.class)); + } + } + } + @Test public void testRunAdHocScheduledWorkflow() { managedRealm.admin().workflows().create(WorkflowRepresentation.create() @@ -79,30 +108,13 @@ public class AdhocWorkflowTest { String id = ApiUtil.getCreatedId(response); managedRealm.admin().workflows().workflow(workflow.getId()).bind(ResourceType.USERS.name(), id); } - - runOnServer.run((session -> { - RealmModel realm = configureSessionContext(session); - WorkflowsManager manager = new WorkflowsManager(session); - UserModel user = session.users().getUserByUsername(realm, "alice"); - - manager.runScheduledSteps(); - assertNull(user.getAttributes().get("message")); - - try { - Time.setOffset(Math.toIntExact(Duration.ofDays(6).toSeconds())); - manager.runScheduledSteps(); - user = session.users().getUserByUsername(realm, "alice"); - assertNotNull(user.getAttributes().get("message")); - } finally { - Time.setOffset(0); - } - })); } @Test - public void testRunAdHocNonScheduledWorkflow() { + public void testRunAdHocImmediateWorkflow() { managedRealm.admin().workflows().create(WorkflowRepresentation.create() .of(EventBasedWorkflowProviderFactory.ID) + .immediate() .withSteps(WorkflowStepRepresentation.create() .of(SetUserAttributeStepProviderFactory.ID) .withConfig("message", "message") @@ -134,6 +146,7 @@ public class AdhocWorkflowTest { .of(EventBasedWorkflowProviderFactory.ID) .withSteps(WorkflowStepRepresentation.create() .of(SetUserAttributeStepProviderFactory.ID) + .after(Duration.ofDays(5)) .withConfig("message", "message") .build()) .build()).close(); 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 01a1429b4f8..73e23299aa5 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 @@ -87,7 +87,37 @@ public class AggregatedStepTest { } @Test - public void testFailCreateIfSettingActionsToRegularActions() { + public void testCreateAggregatedStepAsSubStep() { + managedRealm.admin().workflows().create(WorkflowRepresentation.create() + .of(UserCreationTimeWorkflowProviderFactory.ID) + .withSteps( + WorkflowStepRepresentation.create().of(AggregatedStepProviderFactory.ID) + .after(Duration.ofDays(5)) + .withSteps(WorkflowStepRepresentation.create() + .of(AggregatedStepProviderFactory.ID) + .withConfig("message", "message") + .after(Duration.ofDays(5)) + .withSteps(WorkflowStepRepresentation.create() + .of(SetUserAttributeStepProviderFactory.ID) + .withConfig("message", "message") + .build(), + WorkflowStepRepresentation.create() + .of(DisableUserStepProviderFactory.ID) + .build() + ) + .build(), + WorkflowStepRepresentation.create() + .of(DisableUserStepProviderFactory.ID) + .build() + ).build()) + .build()).close(); + + List workflows = managedRealm.admin().workflows().list(); + assertThat(workflows, hasSize(1)); + } + + @Test + public void testFailCreateIfSettingStepsToRegularSteps() { try (Response response = managedRealm.admin().workflows().create(WorkflowRepresentation.create() .of(UserCreationTimeWorkflowProviderFactory.ID) .withSteps( @@ -104,7 +134,30 @@ public class AggregatedStepTest { ).build()) .build())) { assertThat(response.getStatus(), is(Status.BAD_REQUEST.getStatusCode())); - assertThat(response.readEntity(ErrorRepresentation.class).getErrorMessage(), equalTo("Action provider " + SetUserAttributeStepProviderFactory.ID + " does not support aggregated actions")); + assertThat(response.readEntity(ErrorRepresentation.class).getErrorMessage(), equalTo("Step provider " + SetUserAttributeStepProviderFactory.ID + " does not support aggregated steps")); + } + } + + @Test + public void testFailCreateIfSubStepHasTimeCondition() { + try (Response response = managedRealm.admin().workflows().create(WorkflowRepresentation.create() + .of(UserCreationTimeWorkflowProviderFactory.ID) + .withSteps( + WorkflowStepRepresentation.create().of(SetUserAttributeStepProviderFactory.ID) + .after(Duration.ofDays(5)) + .withConfig("key", "value") + .withSteps(WorkflowStepRepresentation.create() + .of(SetUserAttributeStepProviderFactory.ID) + .withConfig("message", "message") + .after(Duration.ofDays(1)) + .build(), + WorkflowStepRepresentation.create() + .of(DisableUserStepProviderFactory.ID) + .build() + ).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")); } } 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 7c31a681ed9..70b1a956cce 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 @@ -18,6 +18,8 @@ package org.keycloak.tests.admin.model.workflow; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -58,6 +60,7 @@ import org.keycloak.models.workflow.WorkflowStateProvider.ScheduledStep; import org.keycloak.models.workflow.SetUserAttributeStepProviderFactory; import org.keycloak.models.workflow.UserCreationTimeWorkflowProviderFactory; import org.keycloak.models.workflow.conditions.IdentityProviderWorkflowConditionFactory; +import org.keycloak.representations.idm.ErrorRepresentation; import org.keycloak.representations.idm.IdentityProviderRepresentation; import org.keycloak.representations.workflows.WorkflowStepRepresentation; import org.keycloak.representations.workflows.WorkflowConditionRepresentation; @@ -79,7 +82,7 @@ public class WorkflowManagementTest { private static final String REALM_NAME = "default"; - @InjectRunOnServer(permittedPackages = "org.keycloak.tests") + @InjectRunOnServer(permittedPackages = {"org.keycloak.tests", "org.hamcrest"}) RunOnServerClient runOnServer; @InjectRealm(lifecycle = LifeCycle.METHOD) @@ -557,11 +560,9 @@ public class WorkflowManagementTest { .immediate() .withSteps( WorkflowStepRepresentation.create().of(SetUserAttributeStepProviderFactory.ID) - .after(Duration.ofDays(1)) .withConfig("message", "message") .build(), WorkflowStepRepresentation.create().of(DisableUserStepProviderFactory.ID) - .after(Duration.ofDays(2)) .build() ).build()).close(); @@ -572,11 +573,97 @@ public class WorkflowManagementTest { runOnServer.run(session -> { configureSessionContext(session); UserModel user = session.users().getUserByUsername(session.getContext().getRealm(), "testuser"); - assertEquals("message", user.getAttributes().get("message").get(0)); + assertThat(user, notNullValue()); + assertThat(user.getAttributes(), notNullValue()); + assertThat(user.getAttributes().get("message"), notNullValue()); + assertThat(user.getAttributes().get("message").get(0), is("message")); assertFalse(user.isEnabled()); }); } + @Test + public void testCreateImmediateWorkflowWithTimeConditions() { + List workflows = WorkflowRepresentation.create() + .of(UserCreationTimeWorkflowProviderFactory.ID) + .immediate() + .withSteps( + WorkflowStepRepresentation.create().of(NotifyUserStepProviderFactory.ID) + .after(Duration.ofDays(3)) + .build(), + WorkflowStepRepresentation.create().of(DisableUserStepProviderFactory.ID) + .after(Duration.ofDays(7)) + .build() + ).build(); + + try (Response response = managedRealm.admin().workflows().create(workflows)) { + 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() { + List workflows = WorkflowRepresentation.create() + .of(UserCreationTimeWorkflowProviderFactory.ID) + .withSteps( + WorkflowStepRepresentation.create().of(NotifyUserStepProviderFactory.ID) + .build(), + WorkflowStepRepresentation.create().of(DisableUserStepProviderFactory.ID) + .build() + ).build(); + + try (Response response = managedRealm.admin().workflows().create(workflows)) { + 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 testCreateWorkflowMarkedAsBothImmediateAndRecurring() { + List workflows = WorkflowRepresentation.create() + .of(UserCreationTimeWorkflowProviderFactory.ID) + .immediate() + .recurring() + .withSteps( + WorkflowStepRepresentation.create().of(NotifyUserStepProviderFactory.ID) + .after(Duration.ofDays(3)) + .build(), + WorkflowStepRepresentation.create().of(DisableUserStepProviderFactory.ID) + .after(Duration.ofDays(7)) + .build() + ).build(); + + try (Response response = managedRealm.admin().workflows().create(workflows)) { + assertThat(response.getStatus(), is(Response.Status.BAD_REQUEST.getStatusCode())); + String error = response.readEntity(String.class); + assertThat(error, containsString("Workflow cannot be both immediate and recurring.")); + } + } + + @Test + public void testFailCreateNegativeTime() { + try (Response response = managedRealm.admin().workflows().create(WorkflowRepresentation.create() + .of(UserCreationTimeWorkflowProviderFactory.ID) + .withSteps( + WorkflowStepRepresentation.create().of(SetUserAttributeStepProviderFactory.ID) + .after(Duration.ofDays(-5)) + .withConfig("key", "value") + .withSteps(WorkflowStepRepresentation.create() + .of(SetUserAttributeStepProviderFactory.ID) + .withConfig("message", "message") + .build(), + WorkflowStepRepresentation.create() + .of(DisableUserStepProviderFactory.ID) + .build() + ).build()) + .build())) { + 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")); + } + } + @Test public void testNotifyUserStepSendsEmailWithDefaultDisableMessage() { // Create workflow: disable at 10 days, notify 3 days before (at day 7)