From 20f5a152781ba22492394fabf7cb1ffbf280153c Mon Sep 17 00:00:00 2001 From: Stefan Guilhen Date: Fri, 12 Sep 2025 12:01:00 -0300 Subject: [PATCH] Adjust scheduled action time so that it is always based on the previous action Closes #42385 Signed-off-by: Stefan Guilhen --- .../JpaResourcePolicyStateProvider.java | 6 +-- .../policy/ResourcePolicyStateProvider.java | 6 +-- .../policy/NotifyUserActionProvider.java | 44 +++++++------------ .../models/policy/ResourcePolicyManager.java | 2 +- .../policy/ActionRunnerScheduledTaskTest.java | 2 +- .../policy/ResourcePolicyManagementTest.java | 24 +++++----- .../policy/UserCreationTimePolicyTest.java | 2 +- .../UserSessionRefreshTimePolicyTest.java | 2 +- 8 files changed, 37 insertions(+), 51 deletions(-) diff --git a/model/jpa/src/main/java/org/keycloak/models/policy/JpaResourcePolicyStateProvider.java b/model/jpa/src/main/java/org/keycloak/models/policy/JpaResourcePolicyStateProvider.java index b553f1cf10c..72584b61cda 100644 --- a/model/jpa/src/main/java/org/keycloak/models/policy/JpaResourcePolicyStateProvider.java +++ b/model/jpa/src/main/java/org/keycloak/models/policy/JpaResourcePolicyStateProvider.java @@ -54,7 +54,7 @@ public class JpaResourcePolicyStateProvider implements ResourcePolicyStateProvid } @Override - public void scheduleAction(ResourcePolicy policy, ResourceAction action, long scheduledTimeOffset, String resourceId) { + public void scheduleAction(ResourcePolicy policy, ResourceAction action, String resourceId) { ResourcePolicyStateEntity.PrimaryKey pk = new ResourcePolicyStateEntity.PrimaryKey(resourceId, policy.getId()); ResourcePolicyStateEntity entity = em.find(ResourcePolicyStateEntity.class, pk); if (entity == null) { @@ -63,12 +63,12 @@ public class JpaResourcePolicyStateProvider implements ResourcePolicyStateProvid entity.setPolicyId(policy.getId()); entity.setPolicyProviderId(policy.getProviderId()); entity.setScheduledActionId(action.getId()); - entity.setScheduledActionTimestamp(Time.currentTimeMillis() + scheduledTimeOffset); + entity.setScheduledActionTimestamp(Time.currentTimeMillis() + action.getAfter()); em.persist(entity); } else { entity.setScheduledActionId(action.getId()); - entity.setScheduledActionTimestamp(Time.currentTimeMillis() + scheduledTimeOffset); + entity.setScheduledActionTimestamp(Time.currentTimeMillis() + action.getAfter()); } } diff --git a/model/storage-private/src/main/java/org/keycloak/models/policy/ResourcePolicyStateProvider.java b/model/storage-private/src/main/java/org/keycloak/models/policy/ResourcePolicyStateProvider.java index 5793297cffa..7629392716c 100644 --- a/model/storage-private/src/main/java/org/keycloak/models/policy/ResourcePolicyStateProvider.java +++ b/model/storage-private/src/main/java/org/keycloak/models/policy/ResourcePolicyStateProvider.java @@ -51,11 +51,7 @@ public interface ResourcePolicyStateProvider extends Provider { */ void removeAll(); - default void scheduleAction(ResourcePolicy policy, ResourceAction action, String resourceId) { - this.scheduleAction(policy, action, action.getAfter(), resourceId); - } - - void scheduleAction(ResourcePolicy policy, ResourceAction action, long scheduledTimeOffset, String resourceId); + void scheduleAction(ResourcePolicy policy, ResourceAction action, String resourceId); ScheduledAction getScheduledAction(String policyId, String resourceId); diff --git a/services/src/main/java/org/keycloak/models/policy/NotifyUserActionProvider.java b/services/src/main/java/org/keycloak/models/policy/NotifyUserActionProvider.java index f650a77fde7..7f2c074b75f 100644 --- a/services/src/main/java/org/keycloak/models/policy/NotifyUserActionProvider.java +++ b/services/src/main/java/org/keycloak/models/policy/NotifyUserActionProvider.java @@ -87,8 +87,7 @@ public class NotifyUserActionProvider implements ResourceActionProvider { } // Return default subject key based on next action type - String defaultSubjectKey = getDefaultSubjectKey(nextActionType); - return defaultSubjectKey; + return getDefaultSubjectKey(nextActionType); } private String getBodyTemplate() { @@ -124,35 +123,22 @@ public class NotifyUserActionProvider implements ResourceActionProvider { } private String getNextActionType() { - ComponentModel nextAction = getNextNonNotificationAction(); - return nextAction != null ? nextAction.getProviderId() : "unknown-action"; + Map nextActionMap = getNextNonNotificationAction(); + return nextActionMap.isEmpty() ? "unknown-action" : nextActionMap.keySet().iterator().next().getProviderId(); } private int calculateDaysUntilNextAction() { - ComponentModel nextAction = getNextNonNotificationAction(); - if (nextAction == null) { - return 0; - } - - String currentAfter = actionModel.get(AFTER_KEY); - String nextAfter = nextAction.get(AFTER_KEY); - - if (currentAfter == null || nextAfter == null) { - return 0; - } - - try { - long currentMillis = Long.parseLong(currentAfter); - long nextMillis = Long.parseLong(nextAfter); - Duration difference = Duration.ofMillis(nextMillis - currentMillis); - return Math.toIntExact(difference.toDays()); - } catch (NumberFormatException e) { - log.warnv("Invalid days format: current={0}, next={1}", currentAfter, nextAfter); + Map nextActionMap = getNextNonNotificationAction(); + if (nextActionMap.isEmpty()) { return 0; } + Long timeToNextAction = nextActionMap.values().iterator().next(); + return Math.toIntExact(Duration.ofMillis(timeToNextAction).toDays()); } - private ComponentModel getNextNonNotificationAction() { + private Map getNextNonNotificationAction() { + long timeToNextNonNotificationAction = 0L; + RealmModel realm = session.getContext().getRealm(); ComponentModel policyModel = realm.getComponent(actionModel.getParentId()); @@ -167,15 +153,19 @@ public class NotifyUserActionProvider implements ResourceActionProvider { // Find current action and return next non-notification action boolean foundCurrent = false; for (ComponentModel action : actions) { - if (foundCurrent && !action.getProviderId().equals("notify-user-action-provider")) { - return action; + if (foundCurrent) { + timeToNextNonNotificationAction += action.get(AFTER_KEY, 0L); + if (!action.getProviderId().equals("notify-user-action-provider")) { + // we found the next non-notification action, accumulate its time and break + return Map.of(action, timeToNextNonNotificationAction); + } } if (action.getId().equals(actionModel.getId())) { foundCurrent = true; } } - return null; + return Map.of(); } private String getDefaultSubjectKey(String actionType) { diff --git a/services/src/main/java/org/keycloak/models/policy/ResourcePolicyManager.java b/services/src/main/java/org/keycloak/models/policy/ResourcePolicyManager.java index 3cb8ab65e9b..f3f70ae7eaa 100644 --- a/services/src/main/java/org/keycloak/models/policy/ResourcePolicyManager.java +++ b/services/src/main/java/org/keycloak/models/policy/ResourcePolicyManager.java @@ -259,7 +259,7 @@ public class ResourcePolicyManager { if (actions.size() > i + 1) { // schedule the next action using the time offset difference between the actions. ResourceAction nextAction = actions.get(i + 1); - policyStateProvider.scheduleAction(policy, nextAction, nextAction.getAfter() - currentAction.getAfter(), scheduled.resourceId()); + policyStateProvider.scheduleAction(policy, nextAction, scheduled.resourceId()); } else { // this was the last action, check if the policy is recurring - i.e. if we need to schedule the first action again if (policy.isRecurring()) { diff --git a/tests/base/src/test/java/org/keycloak/tests/admin/model/policy/ActionRunnerScheduledTaskTest.java b/tests/base/src/test/java/org/keycloak/tests/admin/model/policy/ActionRunnerScheduledTaskTest.java index aca8d65d393..82f2c0eeec2 100644 --- a/tests/base/src/test/java/org/keycloak/tests/admin/model/policy/ActionRunnerScheduledTaskTest.java +++ b/tests/base/src/test/java/org/keycloak/tests/admin/model/policy/ActionRunnerScheduledTaskTest.java @@ -83,7 +83,7 @@ public class ActionRunnerScheduledTaskTest { .withConfig("message", "message") .build(), ResourcePolicyActionRepresentation.create().of(DisableUserActionProviderFactory.ID) - .after(Duration.ofDays(10)) + .after(Duration.ofDays(5)) .build() ).build()).close(); diff --git a/tests/base/src/test/java/org/keycloak/tests/admin/model/policy/ResourcePolicyManagementTest.java b/tests/base/src/test/java/org/keycloak/tests/admin/model/policy/ResourcePolicyManagementTest.java index c91d9d00b48..3c8a8437c8c 100644 --- a/tests/base/src/test/java/org/keycloak/tests/admin/model/policy/ResourcePolicyManagementTest.java +++ b/tests/base/src/test/java/org/keycloak/tests/admin/model/policy/ResourcePolicyManagementTest.java @@ -102,7 +102,7 @@ public class ResourcePolicyManagementTest { .after(Duration.ofDays(5)) .build(), ResourcePolicyActionRepresentation.create().of(DisableUserActionProviderFactory.ID) - .after(Duration.ofDays(10)) + .after(Duration.ofDays(5)) .build() ).build(); @@ -130,7 +130,7 @@ public class ResourcePolicyManagementTest { .after(Duration.ofDays(5)) .build(), ResourcePolicyActionRepresentation.create().of(DisableUserActionProviderFactory.ID) - .after(Duration.ofDays(10)) + .after(Duration.ofDays(5)) .build() ).build(); @@ -198,7 +198,7 @@ public class ResourcePolicyManagementTest { .after(Duration.ofDays(5)) .build(), ResourcePolicyActionRepresentation.create().of(DisableUserActionProviderFactory.ID) - .after(Duration.ofDays(10)) + .after(Duration.ofDays(5)) .build() ).build(); @@ -230,7 +230,7 @@ public class ResourcePolicyManagementTest { .after(Duration.ofDays(5)) .build(), ResourcePolicyActionRepresentation.create().of(DisableUserActionProviderFactory.ID) - .after(Duration.ofDays(10)) + .after(Duration.ofDays(5)) .build() ).build()).close(); @@ -309,7 +309,7 @@ public class ResourcePolicyManagementTest { .after(Duration.ofDays(5)) .build(), ResourcePolicyActionRepresentation.create().of(DisableUserActionProviderFactory.ID) - .after(Duration.ofDays(10)) + .after(Duration.ofDays(5)) .build() ).build()).close(); @@ -405,7 +405,7 @@ public class ResourcePolicyManagementTest { .after(Duration.ofDays(5)) .build(), ResourcePolicyActionRepresentation.create().of(DisableUserActionProviderFactory.ID) - .after(Duration.ofDays(10)) + .after(Duration.ofDays(5)) .build() ).build()).close(); @@ -567,7 +567,7 @@ public class ResourcePolicyManagementTest { .withConfig("message", "message") .build(), ResourcePolicyActionRepresentation.create().of(DisableUserActionProviderFactory.ID) - .after(Duration.ofDays(2)) + .after(Duration.ofDays(1)) .build() ).build()).close(); @@ -594,7 +594,7 @@ public class ResourcePolicyManagementTest { .withConfig("reason", "inactivity") .build(), ResourcePolicyActionRepresentation.create().of(DisableUserActionProviderFactory.ID) - .after(Duration.ofDays(10)) + .after(Duration.ofDays(3)) .build() ).build()).close(); @@ -632,7 +632,7 @@ public class ResourcePolicyManagementTest { .withConfig("reason", "inactivity") .build(), ResourcePolicyActionRepresentation.create().of(DeleteUserActionProviderFactory.ID) - .after(Duration.ofDays(30)) + .after(Duration.ofDays(15)) .build() ).build()).close(); @@ -672,7 +672,7 @@ public class ResourcePolicyManagementTest { .withConfig("custom_subject_key", "customComplianceSubject") .build(), ResourcePolicyActionRepresentation.create().of(DisableUserActionProviderFactory.ID) - .after(Duration.ofDays(7)) + .after(Duration.ofDays(2)) .build() ).build()).close(); @@ -707,7 +707,7 @@ public class ResourcePolicyManagementTest { .after(Duration.ofDays(5)) .build(), ResourcePolicyActionRepresentation.create().of(DisableUserActionProviderFactory.ID) - .after(Duration.ofDays(10)) + .after(Duration.ofDays(5)) .build() ).build()).close(); @@ -747,7 +747,7 @@ public class ResourcePolicyManagementTest { .withConfig("reason", "inactivity") .build(), ResourcePolicyActionRepresentation.create().of(DisableUserActionProviderFactory.ID) - .after(Duration.ofDays(30)) + .after(Duration.ofDays(15)) .build() ).build()).close(); diff --git a/tests/base/src/test/java/org/keycloak/tests/admin/model/policy/UserCreationTimePolicyTest.java b/tests/base/src/test/java/org/keycloak/tests/admin/model/policy/UserCreationTimePolicyTest.java index 82b83af7785..60160b0811c 100644 --- a/tests/base/src/test/java/org/keycloak/tests/admin/model/policy/UserCreationTimePolicyTest.java +++ b/tests/base/src/test/java/org/keycloak/tests/admin/model/policy/UserCreationTimePolicyTest.java @@ -69,7 +69,7 @@ public class UserCreationTimePolicyTest { .after(Duration.ofDays(5)) .build(), ResourcePolicyActionRepresentation.create().of(DisableUserActionProviderFactory.ID) - .after(Duration.ofDays(10)) + .after(Duration.ofDays(5)) .build() ).build()).close(); diff --git a/tests/base/src/test/java/org/keycloak/tests/admin/model/policy/UserSessionRefreshTimePolicyTest.java b/tests/base/src/test/java/org/keycloak/tests/admin/model/policy/UserSessionRefreshTimePolicyTest.java index af191bba68d..ad1f46b57a7 100644 --- a/tests/base/src/test/java/org/keycloak/tests/admin/model/policy/UserSessionRefreshTimePolicyTest.java +++ b/tests/base/src/test/java/org/keycloak/tests/admin/model/policy/UserSessionRefreshTimePolicyTest.java @@ -108,7 +108,7 @@ public class UserSessionRefreshTimePolicyTest { .after(Duration.ofDays(5)) .build(), ResourcePolicyActionRepresentation.create().of(DisableUserActionProviderFactory.ID) - .after(Duration.ofDays(10)) + .after(Duration.ofDays(5)) .build() ).build()).close();