diff --git a/common/src/main/java/org/keycloak/common/util/DurationConverter.java b/common/src/main/java/org/keycloak/common/util/DurationConverter.java new file mode 100644 index 00000000000..f86bfc48517 --- /dev/null +++ b/common/src/main/java/org/keycloak/common/util/DurationConverter.java @@ -0,0 +1,66 @@ +package org.keycloak.common.util; + +import java.time.Duration; +import java.time.format.DateTimeParseException; +import java.util.regex.Pattern; + +public class DurationConverter { + + private static final String PERIOD = "P"; + private static final String PERIOD_OF_TIME = "PT"; + public static final Pattern DIGITS = Pattern.compile("^[-+]?\\d+$"); + private static final Pattern DIGITS_AND_UNIT = Pattern.compile("^(?:[-+]?\\d+(?:\\.\\d+)?(?i)[hms])+$"); + private static final Pattern DAYS = Pattern.compile("^[-+]?\\d+(?i)d$"); + private static final Pattern MILLIS = Pattern.compile("^[-+]?\\d+(?i)ms$"); + + /** + * If the {@code value} starts with a number, then: + * + * + * Otherwise, {@link Duration#parse(CharSequence)} is called. + * + * @param value a string duration + * @return the parsed {@link Duration} + * @throws IllegalArgumentException in case of parse failure + */ + public static Duration parseDuration(String value) { + if (value == null || value.trim().isEmpty()) { + return null; + } + if (DIGITS.asPredicate().test(value)) { + return Duration.ofSeconds(Long.parseLong(value)); + } else if (MILLIS.asPredicate().test(value)) { + return Duration.ofMillis(Long.parseLong(value.substring(0, value.length() - 2))); + } + + try { + if (DIGITS_AND_UNIT.asPredicate().test(value)) { + return Duration.parse(PERIOD_OF_TIME + value); + } else if (DAYS.asPredicate().test(value)) { + return Duration.parse(PERIOD + value); + } + + return Duration.parse(value); + } catch (DateTimeParseException e) { + throw new IllegalArgumentException(e); + } + } + + /** + * Checks whether the given value represents a positive duration. + * + * @param value a string duration following the same format as in {@link #parseDuration(String)} + * @return true if the value represents a positive duration, false otherwise + */ + public static boolean isPositiveDuration(String value) { + Duration duration = parseDuration(value); + return duration != null && !duration.isNegative() && !duration.isZero(); + } +} diff --git a/core/src/main/java/org/keycloak/representations/workflows/WorkflowStepRepresentation.java b/core/src/main/java/org/keycloak/representations/workflows/WorkflowStepRepresentation.java index 711f413f435..43d29487bef 100644 --- a/core/src/main/java/org/keycloak/representations/workflows/WorkflowStepRepresentation.java +++ b/core/src/main/java/org/keycloak/representations/workflows/WorkflowStepRepresentation.java @@ -52,8 +52,8 @@ public final class WorkflowStepRepresentation extends AbstractWorkflowComponentR return getConfigValue(CONFIG_AFTER, String.class); } - public void setAfter(long ms) { - setConfig(CONFIG_AFTER, String.valueOf(ms)); + public void setAfter(String after) { + setConfig(CONFIG_AFTER, after); } public String getPriority() { @@ -86,7 +86,11 @@ public final class WorkflowStepRepresentation extends AbstractWorkflowComponentR } public Builder after(Duration duration) { - step.setAfter(duration.toMillis()); + return after(String.valueOf(duration.getSeconds())); + } + + public Builder after(String after) { + step.setAfter(after); return this; } @@ -95,15 +99,6 @@ public final class WorkflowStepRepresentation extends AbstractWorkflowComponentR return this; } - public Builder before(WorkflowStepRepresentation targetStep, Duration timeBeforeTarget) { - // Calculate absolute time: targetStep.after - timeBeforeTarget - String targetAfter = targetStep.getConfig().get(CONFIG_AFTER).get(0); - long targetTime = Long.parseLong(targetAfter); - long thisTime = targetTime - timeBeforeTarget.toMillis(); - step.setAfter(thisTime); - return this; - } - public Builder withConfig(String key, String value) { step.setConfig(key, value); return this; diff --git a/integration/admin-client/src/main/java/org/keycloak/admin/client/resource/WorkflowResource.java b/integration/admin-client/src/main/java/org/keycloak/admin/client/resource/WorkflowResource.java index 543d73579e1..0db1d0ae1f4 100644 --- a/integration/admin-client/src/main/java/org/keycloak/admin/client/resource/WorkflowResource.java +++ b/integration/admin-client/src/main/java/org/keycloak/admin/client/resource/WorkflowResource.java @@ -35,7 +35,7 @@ public interface WorkflowResource { @Path("bind/{type}/{resourceId}") @POST @Consumes(MediaType.APPLICATION_JSON) - void bind(@PathParam("type") String type, @PathParam("resourceId") String resourceId, Long milliseconds); + void bind(@PathParam("type") String type, @PathParam("resourceId") String resourceId, String notBefore); @Path("deactivate/{type}/{resourceId}") @POST 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 a09d760cc9b..b9198c67277 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 @@ -16,6 +16,7 @@ import java.util.stream.Stream; import jakarta.ws.rs.BadRequestException; import org.jboss.logging.Logger; +import org.keycloak.common.util.DurationConverter; import org.keycloak.common.util.MultivaluedHashMap; import org.keycloak.component.ComponentFactory; import org.keycloak.component.ComponentModel; @@ -233,8 +234,8 @@ public class DefaultWorkflowProvider implements WorkflowProvider { if (isAlreadyScheduledInSession(event, workflow)) { return; } - // If the workflow has a notBefore set, schedule the first step with it - if (workflow.getNotBefore() != null && workflow.getNotBefore() > 0) { + // If the workflow has a positive notBefore set, schedule the first step with it + if (DurationConverter.isPositiveDuration(workflow.getNotBefore())) { scheduleWorkflow(event, workflow); } else { DefaultWorkflowExecutionContext context = new DefaultWorkflowExecutionContext(session, workflow, event); @@ -321,7 +322,7 @@ public class DefaultWorkflowProvider implements WorkflowProvider { throw new WorkflowInvalidStateException("Workflow restart step must be the last step."); } boolean hasScheduledStep = steps.stream() - .anyMatch(step -> Integer.parseInt(ofNullable(step.getAfter()).orElse("0")) > 0); + .anyMatch(step -> DurationConverter.isPositiveDuration(step.getAfter())); if (!hasScheduledStep) { throw new WorkflowInvalidStateException("A workflow with a restart step must have at least one step with a time delay."); } diff --git a/model/jpa/src/main/java/org/keycloak/models/workflow/JpaWorkflowStateProvider.java b/model/jpa/src/main/java/org/keycloak/models/workflow/JpaWorkflowStateProvider.java index 9b5f67bd217..af726d9caef 100644 --- a/model/jpa/src/main/java/org/keycloak/models/workflow/JpaWorkflowStateProvider.java +++ b/model/jpa/src/main/java/org/keycloak/models/workflow/JpaWorkflowStateProvider.java @@ -24,12 +24,15 @@ import jakarta.persistence.criteria.CriteriaQuery; import jakarta.persistence.criteria.Predicate; import jakarta.persistence.criteria.Root; import org.jboss.logging.Logger; +import org.keycloak.common.util.DurationConverter; import org.keycloak.common.util.Time; import org.keycloak.connections.jpa.JpaConnectionProvider; import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; import org.keycloak.utils.StringUtil; +import java.time.Duration; +import java.time.Instant; import java.util.List; public class JpaWorkflowStateProvider implements WorkflowStateProvider { @@ -57,17 +60,24 @@ public class JpaWorkflowStateProvider implements WorkflowStateProvider { @Override public void scheduleStep(Workflow workflow, WorkflowStep step, String resourceId, String executionId) { WorkflowStateEntity entity = em.find(WorkflowStateEntity.class, executionId); + Duration duration = DurationConverter.parseDuration(step.getAfter()); + if (duration == null) { + // shouldn't happen as the step duration should have been validated before + throw new IllegalArgumentException("Invalid duration (%s) found when scheduling step %s in workflow %s" + .formatted(step.getAfter(), step.getProviderId(), workflow.getName())); + } + if (entity == null) { entity = new WorkflowStateEntity(); entity.setResourceId(resourceId); entity.setWorkflowId(workflow.getId()); entity.setExecutionId(executionId); entity.setScheduledStepId(step.getId()); - entity.setScheduledStepTimestamp(Time.currentTimeMillis() + step.getAfter()); + entity.setScheduledStepTimestamp(Instant.now().plus(duration).toEpochMilli()); em.persist(entity); } else { entity.setScheduledStepId(step.getId()); - entity.setScheduledStepTimestamp(Time.currentTimeMillis() + step.getAfter()); + entity.setScheduledStepTimestamp(Instant.now().plus(duration).toEpochMilli()); } } diff --git a/model/jpa/src/main/java/org/keycloak/models/workflow/RunWorkflowTask.java b/model/jpa/src/main/java/org/keycloak/models/workflow/RunWorkflowTask.java index b4f9d59cccd..f7365911d78 100644 --- a/model/jpa/src/main/java/org/keycloak/models/workflow/RunWorkflowTask.java +++ b/model/jpa/src/main/java/org/keycloak/models/workflow/RunWorkflowTask.java @@ -3,6 +3,7 @@ package org.keycloak.models.workflow; import java.util.List; import org.jboss.logging.Logger; +import org.keycloak.common.util.DurationConverter; import org.keycloak.models.KeycloakSession; class RunWorkflowTask extends WorkflowTransactionalTask { @@ -34,7 +35,7 @@ class RunWorkflowTask extends WorkflowTransactionalTask { WorkflowStateProvider stateProvider = session.getProvider(WorkflowStateProvider.class); for (WorkflowStep step : stepsToRun) { - if (step.getAfter() > 0) { + if (DurationConverter.isPositiveDuration(step.getAfter())) { // If a step has a time defined, schedule it and stop processing the other steps of workflow log.debugf("Scheduling step %s to run in %d ms for resource %s (execution id: %s)", step.getProviderId(), step.getAfter(), resourceId, executionId); diff --git a/model/jpa/src/main/java/org/keycloak/models/workflow/ScheduleWorkflowTask.java b/model/jpa/src/main/java/org/keycloak/models/workflow/ScheduleWorkflowTask.java index 9036c1030d5..693c852fb24 100644 --- a/model/jpa/src/main/java/org/keycloak/models/workflow/ScheduleWorkflowTask.java +++ b/model/jpa/src/main/java/org/keycloak/models/workflow/ScheduleWorkflowTask.java @@ -32,7 +32,7 @@ final class ScheduleWorkflowTask extends WorkflowTransactionalTask { WorkflowStep firstStep = workflow.getSteps().findFirst().orElseThrow(() -> new WorkflowInvalidStateException("No steps found for workflow " + workflow.getName())); log.debugf("Scheduling first step '%s' of workflow '%s' for resource %s based on on event %s with notBefore %d", firstStep.getProviderId(), workflow.getName(), event.getResourceId(), event.getOperation(), workflow.getNotBefore()); - Long originalAfter = firstStep.getAfter(); + String originalAfter = firstStep.getAfter(); try { firstStep.setAfter(workflow.getNotBefore()); WorkflowStateProvider stateProvider = session.getProvider(WorkflowStateProvider.class); 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 8160aebc090..bab2c13b537 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 @@ -22,16 +22,19 @@ import static org.keycloak.representations.workflows.WorkflowConstants.CONFIG_EN import static org.keycloak.representations.workflows.WorkflowConstants.CONFIG_ERROR; import static org.keycloak.representations.workflows.WorkflowConstants.CONFIG_NAME; +import java.time.Duration; import java.util.List; import java.util.Map; import java.util.stream.Stream; +import org.keycloak.common.util.DurationConverter; import org.keycloak.common.util.MultivaluedHashMap; import org.keycloak.component.ComponentModel; import org.keycloak.models.KeycloakSession; import org.keycloak.models.ModelValidationException; import org.keycloak.models.RealmModel; import org.keycloak.representations.workflows.WorkflowStepRepresentation; +import org.keycloak.utils.StringUtil; public class Workflow { @@ -39,7 +42,7 @@ public class Workflow { private final KeycloakSession session; private MultivaluedHashMap config; private String id; - private Long notBefore; + private String notBefore; public Workflow(KeycloakSession session, ComponentModel c) { this.session = session; @@ -72,11 +75,11 @@ public class Workflow { return config != null && Boolean.parseBoolean(config.getFirstOrDefault(CONFIG_ENABLED, "true")); } - public Long getNotBefore() { + public String getNotBefore() { return notBefore; } - public void setNotBefore(Long notBefore) { + public void setNotBefore(String notBefore) { this.notBefore = notBefore; } @@ -133,22 +136,33 @@ public class Workflow { } private WorkflowStep toModel(WorkflowStepRepresentation rep) { - WorkflowStep step = new WorkflowStep(rep.getUses(), rep.getConfig()); - validateStep(step); - return step; + validateStep(rep); + return new WorkflowStep(rep.getUses(), rep.getConfig()); } - private void validateStep(WorkflowStep step) throws ModelValidationException { - if (step.getAfter() < 0) { - throw new ModelValidationException("Step 'after' time condition cannot be negative."); + private void validateStep(WorkflowStepRepresentation step) throws ModelValidationException { + + // validate the step rep has 'uses' defined + if (StringUtil.isBlank(step.getUses())) { + throw new ModelValidationException("Step 'uses' cannot be null or empty."); + } + + // validate the after time, if present + try { + Duration duration = DurationConverter.parseDuration(step.getAfter()); + if (duration != null && duration.isNegative()) { // duration can only be null if the config is not set + throw new ModelValidationException("Step 'after' configuration cannot be negative."); + } + } catch (IllegalArgumentException e) { + throw new ModelValidationException("Step 'after' configuration is not valid: " + step.getAfter()); } // verify the step does have valid provider WorkflowStepProviderFactory factory = (WorkflowStepProviderFactory) session - .getKeycloakSessionFactory().getProviderFactory(WorkflowStepProvider.class, step.getProviderId()); + .getKeycloakSessionFactory().getProviderFactory(WorkflowStepProvider.class, step.getUses()); if (factory == null) { - throw new WorkflowInvalidStateException("Step not found: " + step.getProviderId()); + throw new WorkflowInvalidStateException("Step not found: " + step.getUses()); } } } diff --git a/server-spi-private/src/main/java/org/keycloak/models/workflow/WorkflowStep.java b/server-spi-private/src/main/java/org/keycloak/models/workflow/WorkflowStep.java index ce15575e9c8..b449e872490 100644 --- a/server-spi-private/src/main/java/org/keycloak/models/workflow/WorkflowStep.java +++ b/server-spi-private/src/main/java/org/keycloak/models/workflow/WorkflowStep.java @@ -81,12 +81,12 @@ public class WorkflowStep implements Comparable { } } - public void setAfter(Long ms) { - setConfig(CONFIG_AFTER, String.valueOf(ms)); + public void setAfter(String after) { + setConfig(CONFIG_AFTER, after); } - public Long getAfter() { - return Long.valueOf(getConfig().getFirstOrDefault(CONFIG_AFTER, "0")); + public String getAfter() { + return getConfig().getFirst(CONFIG_AFTER); } @Override diff --git a/services/src/main/java/org/keycloak/models/workflow/NotifyUserStepProvider.java b/services/src/main/java/org/keycloak/models/workflow/NotifyUserStepProvider.java index 8c85537ef5e..d80b927100b 100644 --- a/services/src/main/java/org/keycloak/models/workflow/NotifyUserStepProvider.java +++ b/services/src/main/java/org/keycloak/models/workflow/NotifyUserStepProvider.java @@ -20,6 +20,7 @@ package org.keycloak.models.workflow; import static org.keycloak.representations.workflows.WorkflowConstants.CONFIG_AFTER; import org.jboss.logging.Logger; +import org.keycloak.common.util.DurationConverter; import org.keycloak.component.ComponentModel; import org.keycloak.email.EmailException; import org.keycloak.email.EmailTemplateProvider; @@ -119,21 +120,21 @@ public class NotifyUserStepProvider implements WorkflowStepProvider { } private String getNextStepType() { - Map nextStepMap = getNextNonNotificationStep(); + Map nextStepMap = getNextNonNotificationStep(); return nextStepMap.isEmpty() ? "unknown-step" : nextStepMap.keySet().iterator().next().getProviderId(); } private int calculateDaysUntilNextStep() { - Map nextStepMap = getNextNonNotificationStep(); + Map nextStepMap = getNextNonNotificationStep(); if (nextStepMap.isEmpty()) { return 0; } - Long timeToNextStep = nextStepMap.values().iterator().next(); - return Math.toIntExact(Duration.ofMillis(timeToNextStep).toDays()); + Duration timeToNextStep = nextStepMap.values().iterator().next(); + return Math.toIntExact(timeToNextStep.toDays()); } - private Map getNextNonNotificationStep() { - long timeToNextNonNotificationStep = 0L; + private Map getNextNonNotificationStep() { + Duration timeToNextNonNotificationStep = Duration.ZERO; RealmModel realm = session.getContext().getRealm(); ComponentModel workflowModel = realm.getComponent(stepModel.getParentId()); @@ -150,7 +151,8 @@ public class NotifyUserStepProvider implements WorkflowStepProvider { boolean foundCurrent = false; for (ComponentModel step : steps) { if (foundCurrent) { - timeToNextNonNotificationStep += step.get(CONFIG_AFTER, 0L); + Duration duration = DurationConverter.parseDuration(step.get(CONFIG_AFTER, "0")); + timeToNextNonNotificationStep = timeToNextNonNotificationStep.plus(duration != null ? duration : Duration.ZERO); if (!step.getProviderId().equals("notify-user")) { // we found the next non-notification action, accumulate its time and break return Map.of(step, timeToNextNonNotificationStep); 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 86a0c6063d8..5231477d21f 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 @@ -61,13 +61,14 @@ public class WorkflowResource { * * @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). + * @param notBefore optional value representing the time to schedule the first workflow step, overriding the first + * step time configuration (after). The value is either an integer representing the seconds from now, + * an integer followed by 'ms' representing milliseconds from now, or an ISO-8601 date string. */ @POST @Consumes(MediaType.APPLICATION_JSON) @Path("bind/{type}/{resourceId}") - public void bind(@PathParam("type") ResourceType type, @PathParam("resourceId") String resourceId, Long notBefore) { + public void bind(@PathParam("type") ResourceType type, @PathParam("resourceId") String resourceId, String notBefore) { Object resource = provider.getResourceTypeSelector(type).resolveResource(resourceId); if (resource == null) { 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 162af588baa..f7d104bc177 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 @@ -65,7 +65,7 @@ public class AdhocWorkflowTest extends AbstractWorkflowTest { 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()); + managedRealm.admin().workflows().workflow(workflow.getId()).bind(ResourceType.USERS.name(), id, "5D"); } catch (Exception e) { assertThat(e, instanceOf(BadRequestException.class)); } @@ -135,7 +135,7 @@ public class AdhocWorkflowTest extends AbstractWorkflowTest { try (Response response = managedRealm.admin().users().create(getUserRepresentation("alice", "Alice", "Wonderland", "alice@wornderland.org"))) { id = ApiUtil.getCreatedId(response); - managedRealm.admin().workflows().workflow(workflow.getId()).bind(ResourceType.USERS.name(), id, Duration.ofDays(5).toMillis()); + managedRealm.admin().workflows().workflow(workflow.getId()).bind(ResourceType.USERS.name(), id, "5D"); } runScheduledSteps(Duration.ZERO); @@ -158,7 +158,8 @@ public class AdhocWorkflowTest extends AbstractWorkflowTest { } })); - managedRealm.admin().workflows().workflow(workflow.getId()).bind(ResourceType.USERS.name(), id, Duration.ofDays(10).toMillis()); + // using seconds as the notBefore parameter just to check if this format is also working properly + managedRealm.admin().workflows().workflow(workflow.getId()).bind(ResourceType.USERS.name(), id, String.valueOf(Duration.ofDays(10).toSeconds())); runScheduledSteps(Duration.ZERO); 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 894f26e29bc..1356fe3e961 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 @@ -257,7 +257,7 @@ public class WorkflowManagementTest extends AbstractWorkflowTest { // revert conditions, but change one of the steps workflow.setConditions(null); - workflow.getSteps().get(0).setAfter(Duration.ofDays(8).toMillis()); + workflow.getSteps().get(0).setAfter("8D"); // 8 days try (Response response = workflows.workflow(workflow.getId()).update(workflow)) { assertThat(response.getStatus(), is(Response.Status.BAD_REQUEST.getStatusCode())); } @@ -679,7 +679,7 @@ public class WorkflowManagementTest extends AbstractWorkflowTest { .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 'after' time condition cannot be negative.")); + assertThat(response.readEntity(ErrorRepresentation.class).getErrorMessage(), equalTo("Step 'after' configuration cannot be negative.")); } }