From 985ec6d3061ff253cf0d4218f69f1a0455dcf20f Mon Sep 17 00:00:00 2001 From: Stefan Guilhen Date: Thu, 18 Dec 2025 10:30:20 -0300 Subject: [PATCH] Add name uniqueness validation to workflows Closes #43914 Signed-off-by: Stefan Guilhen # Conflicts: # tests/base/src/test/java/org/keycloak/tests/workflow/WorkflowManagementTest.java --- .../workflow/DefaultWorkflowProvider.java | 4 +- .../models/workflow/WorkflowValidator.java | 18 +++++-- .../AddRequiredActionStepProvider.java | 2 +- .../workflow/GroupBasedStepProvider.java | 16 +----- .../workflow/WorkflowManagementTest.java | 52 ++++++++++++++----- 5 files changed, 58 insertions(+), 34 deletions(-) diff --git a/model/jpa/src/main/java/org/keycloak/models/workflow/DefaultWorkflowProvider.java b/model/jpa/src/main/java/org/keycloak/models/workflow/DefaultWorkflowProvider.java index ab16a329f3c..25a2f1c340e 100644 --- a/model/jpa/src/main/java/org/keycloak/models/workflow/DefaultWorkflowProvider.java +++ b/model/jpa/src/main/java/org/keycloak/models/workflow/DefaultWorkflowProvider.java @@ -59,7 +59,7 @@ public class DefaultWorkflowProvider implements WorkflowProvider { @Override public void updateWorkflow(Workflow workflow, WorkflowRepresentation representation) { // first step - ensure the updated workflow is valid - WorkflowValidator.validateWorkflow(session, representation); + WorkflowValidator.validateWorkflow(session, this, representation); // check if there are scheduled steps for this workflow - if there aren't, we can update freely if (!stateProvider.hasScheduledSteps(workflow.getId())) { @@ -197,7 +197,7 @@ public class DefaultWorkflowProvider implements WorkflowProvider { @Override public Workflow toModel(WorkflowRepresentation rep) { - WorkflowValidator.validateWorkflow(session, rep); + WorkflowValidator.validateWorkflow(session, this, rep); MultivaluedHashMap config = ofNullable(rep.getConfig()).orElse(new MultivaluedHashMap<>()); if (rep.getCancelInProgress() != null) { diff --git a/model/jpa/src/main/java/org/keycloak/models/workflow/WorkflowValidator.java b/model/jpa/src/main/java/org/keycloak/models/workflow/WorkflowValidator.java index f04ee6a44c5..bb959509e6f 100644 --- a/model/jpa/src/main/java/org/keycloak/models/workflow/WorkflowValidator.java +++ b/model/jpa/src/main/java/org/keycloak/models/workflow/WorkflowValidator.java @@ -18,8 +18,10 @@ import static java.util.Optional.ofNullable; public class WorkflowValidator { - public static void validateWorkflow(KeycloakSession session, WorkflowRepresentation rep) throws WorkflowInvalidStateException { - validateField(rep, "name", rep.getName()); + public static void validateWorkflow(KeycloakSession session, WorkflowProvider provider, WorkflowRepresentation rep) throws WorkflowInvalidStateException { + + validateWorkflowName(provider, rep); + //TODO: validate event and resource conditions (`on` and `if` properties) using the providers with a custom evaluator that calls validate on // each condition provider used in the expression once we have the event condition providers implemented if (StringUtil.isNotBlank(rep.getOn())) { @@ -119,9 +121,15 @@ public class WorkflowValidator { } } - private static void validateField(Object obj, String fieldName, String value) throws WorkflowInvalidStateException { - if (StringUtil.isBlank(value)) { - throw new WorkflowInvalidStateException("%s field '%s' cannot be null or empty.".formatted(obj.getClass().getCanonicalName(), fieldName)); + private static void validateWorkflowName(WorkflowProvider provider, WorkflowRepresentation representation) throws WorkflowInvalidStateException { + String name = representation.getName(); + if (StringUtil.isBlank(name)) { + throw new WorkflowInvalidStateException("Workflow name cannot be null or empty."); + } + + // validate name uniqueness + if (provider.getWorkflows().anyMatch(wf -> wf.getName().equals(name) && !wf.getId().equals(representation.getId()))) { + throw new WorkflowInvalidStateException("Workflow name must be unique. A workflow with name '" + name + "' already exists."); } } } diff --git a/services/src/main/java/org/keycloak/models/workflow/AddRequiredActionStepProvider.java b/services/src/main/java/org/keycloak/models/workflow/AddRequiredActionStepProvider.java index cba6e5ccb6f..37b83bfe9c5 100644 --- a/services/src/main/java/org/keycloak/models/workflow/AddRequiredActionStepProvider.java +++ b/services/src/main/java/org/keycloak/models/workflow/AddRequiredActionStepProvider.java @@ -32,7 +32,7 @@ public class AddRequiredActionStepProvider implements WorkflowStepProvider { log.debugv("Adding required action {0} to user {1})", action, user.getId()); user.addRequiredAction(action); } catch (IllegalArgumentException e) { - log.warnv("Invalid required action {0} configured in AddRequiredActionProvider", stepModel.getConfig().getFirst(REQUIRED_ACTION_KEY)); + log.warnv("Invalid required action {0} configured in AddRequiredActionStepProvider", stepModel.getConfig().getFirst(REQUIRED_ACTION_KEY)); } } } diff --git a/services/src/main/java/org/keycloak/models/workflow/GroupBasedStepProvider.java b/services/src/main/java/org/keycloak/models/workflow/GroupBasedStepProvider.java index 153e05eca3f..8063bfc8121 100644 --- a/services/src/main/java/org/keycloak/models/workflow/GroupBasedStepProvider.java +++ b/services/src/main/java/org/keycloak/models/workflow/GroupBasedStepProvider.java @@ -5,10 +5,10 @@ import java.util.stream.Stream; import org.keycloak.component.ComponentModel; import org.keycloak.models.GroupModel; -import org.keycloak.models.GroupProvider; import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; import org.keycloak.models.UserModel; +import org.keycloak.models.utils.KeycloakModelUtils; import org.jboss.logging.Logger; @@ -49,22 +49,10 @@ public abstract class GroupBasedStepProvider implements WorkflowStepProvider { } private GroupModel getGroup(String name) { - GroupProvider groups = session.groups(); - String[] paths = name.split("/"); - RealmModel realm = getRealm(); - GroupModel group = null; - - for (String part : paths) { - if (part.isEmpty()) { - continue; - } - group = groups.getGroupByName(realm, group, part); - } - + GroupModel group = KeycloakModelUtils.findGroupByPath(session, getRealm(), name); if (group == null) { throw new IllegalStateException("Could not find group for name or path: " + name); } - return group; } diff --git a/tests/base/src/test/java/org/keycloak/tests/workflow/WorkflowManagementTest.java b/tests/base/src/test/java/org/keycloak/tests/workflow/WorkflowManagementTest.java index 1b5d997698d..838883fa953 100644 --- a/tests/base/src/test/java/org/keycloak/tests/workflow/WorkflowManagementTest.java +++ b/tests/base/src/test/java/org/keycloak/tests/workflow/WorkflowManagementTest.java @@ -66,6 +66,7 @@ import com.fasterxml.jackson.jakarta.rs.yaml.JacksonYAMLProvider; import com.fasterxml.jackson.jakarta.rs.yaml.YAMLMediaTypes; import org.junit.jupiter.api.Test; +import static org.keycloak.models.workflow.ResourceOperationType.USER_AUTHENTICATED; import static org.keycloak.models.workflow.ResourceOperationType.USER_CREATED; import static org.hamcrest.MatcherAssert.assertThat; @@ -240,6 +241,33 @@ public class WorkflowManagementTest extends AbstractWorkflowTest { } } + @Test + public void testFailCreateWorkflowWithDuplicateName() { + // create first workflow + managedRealm.admin().workflows().create(WorkflowRepresentation.withName("myworkflow") + .onEvent(USER_CREATED.name()) + .withSteps( + WorkflowStepRepresentation.create().of(SetUserAttributeStepProviderFactory.ID) + .after(Duration.ofDays(5)) + .withConfig("key", "value") + .build()) + .build()).close(); + + // try to create second workflow with same name + try (Response response = managedRealm.admin().workflows().create(WorkflowRepresentation.withName("myworkflow") + .onEvent(USER_AUTHENTICATED.name()) + .withSteps( + WorkflowStepRepresentation.create().of(DisableUserStepProviderFactory.ID) + .after(Duration.ofDays(10)) + .build()) + .build())) { + assertThat(response.getStatus(), is(Response.Status.BAD_REQUEST.getStatusCode())); + assertThat(response.readEntity(ErrorRepresentation.class).getErrorMessage(), + equalTo("Workflow name must be unique. A workflow with name 'myworkflow' already exists.")); + } + + } + @Test public void testDelete() { WorkflowsResource workflows = managedRealm.admin().workflows(); @@ -497,18 +525,18 @@ public class WorkflowManagementTest extends AbstractWorkflowTest { String workflowId; try (Response response = - managedRealm.admin().workflows().create(WorkflowRepresentation.withName(name) - .withSteps( - WorkflowStepRepresentation.create().of(NotifyUserStepProviderFactory.ID) - .after(Duration.ofDays(5)) - .build(), - WorkflowStepRepresentation.create().of(SetUserAttributeStepProviderFactory.ID) - .withConfig("key", "value") - .build(), - WorkflowStepRepresentation.create().of(DisableUserStepProviderFactory.ID) - .after(Duration.ofDays(15)) - .build() - ).build())) { + managedRealm.admin().workflows().create(WorkflowRepresentation.withName(name) + .withSteps( + WorkflowStepRepresentation.create().of(NotifyUserStepProviderFactory.ID) + .after(Duration.ofDays(5)) + .build(), + WorkflowStepRepresentation.create().of(SetUserAttributeStepProviderFactory.ID) + .withConfig("key", "value") + .build(), + WorkflowStepRepresentation.create().of(DisableUserStepProviderFactory.ID) + .after(Duration.ofDays(15)) + .build() + ).build())) { workflowId = ApiUtil.getCreatedId(response); }