Skip to content

Add name uniqueness validation to workflows #44999

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sguilhen wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add name uniqueness validation to workflows #44999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions model/jpa/src/main/java/org/keycloak/models/workflow/DefaultWorkflowProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public ResourceTypeSelector getResourceTypeSelector(ResourceType type) {
@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())) {
Expand Down Expand Up @@ -197,7 +197,7 @@ public WorkflowRepresentation toRepresentation(Workflow workflow) {

@Override
public Workflow toModel(WorkflowRepresentation rep) {
WorkflowValidator.validateWorkflow(session, rep);
WorkflowValidator.validateWorkflow(session, this, rep);

MultivaluedHashMap<String, String> config = ofNullable(rep.getConfig()).orElse(new MultivaluedHashMap<>());
if (rep.getCancelInProgress() != null) {
Expand Down
18 changes: 13 additions & 5 deletions model/jpa/src/main/java/org/keycloak/models/workflow/WorkflowValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@

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())) {
Expand Down Expand Up @@ -119,9 +121,15 @@ private static void validateConditionExpression(KeycloakSession session, String
}
}

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.");
}
}
}
2 changes: 1 addition & 1 deletion services/src/main/java/org/keycloak/models/workflow/AddRequiredActionStepProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public void run(WorkflowExecutionContext context) {
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));
}
}
}
Expand Down
16 changes: 2 additions & 14 deletions services/src/main/java/org/keycloak/models/workflow/GroupBasedStepProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@

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;

Expand Down Expand Up @@ -49,22 +49,10 @@ private Stream<GroupModel> getGroups() {
}

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;
}

Expand Down
52 changes: 40 additions & 12 deletions tests/base/src/test/java/org/keycloak/tests/workflow/WorkflowManagementTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
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;
Expand Down Expand Up @@ -240,6 +241,33 @@ public void testFailCreateWorkflowWithNegativeTime() {
}
}

@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();
Expand Down Expand Up @@ -497,18 +525,18 @@ public void testGetActiveWorkflowsForResource() {

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);
}

Expand Down
Loading