[RLM] Cleanup code from initial PR

Closes #42316

Signed-off-by: vramik <vramik@redhat.com>
This commit is contained in:
vramik 2025-09-05 11:57:58 +02:00 committed by Pedro Igor
parent 40476b53d9
commit 3507773854
4 changed files with 9 additions and 174 deletions

View File

@ -17,9 +17,6 @@
package org.keycloak.models.policy;
import java.util.List;
import java.util.Set;
import jakarta.persistence.EntityManager;
import jakarta.persistence.criteria.CriteriaBuilder;
import jakarta.persistence.criteria.CriteriaDelete;
@ -31,7 +28,8 @@ 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.models.UserModel;
import java.util.List;
public class JpaResourcePolicyStateProvider implements ResourcePolicyStateProvider {
@ -117,57 +115,6 @@ public class JpaResourcePolicyStateProvider implements ResourcePolicyStateProvid
.toList();
}
@Override
public void update(String policyId, String policyProviderId, List<String> resourceIds, String newLastCompletedActionId) {
for (String resourceId : resourceIds) {
ResourcePolicyStateEntity.PrimaryKey pk = new ResourcePolicyStateEntity.PrimaryKey(resourceId, policyId);
ResourcePolicyStateEntity entity = em.find(ResourcePolicyStateEntity.class, pk);
if (entity == null) {
if (LOGGER.isTraceEnabled()) {
LOGGER.tracev("Initial record for policyId ({0}), new_last_compl_actionId ({1}), userId ({2})", policyId, newLastCompletedActionId, resourceId);
}
entity = new ResourcePolicyStateEntity();
entity.setResourceId(resourceId);
entity.setPolicyId(policyId);
entity.setPolicyProviderId(policyProviderId);
em.persist(entity);
} else {
if (LOGGER.isTraceEnabled()) {
LOGGER.tracev("Changing record for policyId ({0}), last_compl_actionId ({1}), new_last_compl_actionId ({2}), userId ({3})",
entity.getPolicyId(), entity.getScheduledActionId(), newLastCompletedActionId, resourceId);
}
}
entity.setScheduledActionId(newLastCompletedActionId);
entity.setScheduledActionTimestamp(Time.currentTimeMillis());
}
}
@Override
public void removeByCompletedActions(String policyId, Set<String> deletedActionIds) {
if (deletedActionIds == null || deletedActionIds.isEmpty()) {
return;
}
CriteriaBuilder cb = em.getCriteriaBuilder();
CriteriaDelete<ResourcePolicyStateEntity> delete = cb.createCriteriaDelete(ResourcePolicyStateEntity.class);
Root<ResourcePolicyStateEntity> stateRoot = delete.from(ResourcePolicyStateEntity.class);
Predicate policyPredicate = cb.equal(stateRoot.get("policyId"), policyId);
Predicate inClausePredicate = stateRoot.get("scheduledActionId").in(deletedActionIds);
delete.where(cb.and(policyPredicate, inClausePredicate));
int deletedCount = em.createQuery(delete).executeUpdate();
if (LOGGER.isTraceEnabled()) {
if (deletedCount > 0) {
LOGGER.tracev("Deleted {0} orphaned state records for policy {1}", deletedCount, policyId);
}
}
}
@Override
public void removeByResource(String resourceId) {
CriteriaBuilder cb = em.getCriteriaBuilder();

View File

@ -18,25 +18,14 @@
package org.keycloak.models.policy;
import org.keycloak.provider.Provider;
import java.util.List;
import java.util.Set;
/**
* Interface serves as state check for policy actions.
*/
public interface ResourcePolicyStateProvider extends Provider {
/**
* Updates the state for a list of resources that have just completed a new action.
* This will perform an update for existing states or an insert for new states.
*/
void update(String policyId, String policyProviderId, List<String> resourceIds, String newLastCompletedActionId);
/**
* Deletes the orphaned state records.
*/
void removeByCompletedActions(String policyId, Set<String> deletedActionIds);
/**
* Deletes the state records associated with the given {@code resourceId}.
*

View File

@ -19,15 +19,11 @@ package org.keycloak.models.policy;
import static java.util.Optional.ofNullable;
import java.time.Duration;
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.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import jakarta.ws.rs.BadRequestException;
@ -49,14 +45,13 @@ public class ResourcePolicyManager {
private static final Logger log = Logger.getLogger(ResourcePolicyManager.class);
private final KeycloakSession session;
private final ResourcePolicyStateProvider policyStateProvider;
public static boolean isFeatureEnabled() {
return Profile.isFeatureEnabled(Feature.RESOURCE_LIFECYCLE);
}
private final KeycloakSession session;
public ResourcePolicyManager(KeycloakSession session) {
this.session = session;
this.policyStateProvider = session.getKeycloakSessionFactory().getProviderFactory(ResourcePolicyStateProvider.class).create(session);
@ -87,48 +82,8 @@ public class ResourcePolicyManager {
return new ResourcePolicy(realm.addComponentModel(model));
}
/*
This method takes an ordered list of actions. First action in the list has the highest priority, last action has the lowest priority
It is used for both create and update actions
---------------------------------------------------------------------------------------
using delete-and-recreate approach for now as it seems more simple and robust solution
todo: consider changing it to "diff-and-update" (more complex) approach where we'd need to
* keep existing actions
* create newly added actions
* delete removed actions
* reorder existing action according to new order (we may add gaps between priority so that we won't need to update all existing actions)
* with the gap approach, it may eventually happen that there won't be any space between the two action, in that case we'd have to trigger recalculation of priorities
*/
public void updateActions(ResourcePolicy policy, List<ResourceAction> actions) {
validateActions(actions);
// get the stable IDs of the new actions
Set<String> newActionIds = actions.stream()
.map(ResourceAction::getId)
.filter(Objects::nonNull)
.collect(Collectors.toSet());
// get the stable IDs of the old actions
List<ResourceAction> oldActions = getActions(policy.getId());
Set<String> oldActionIds = oldActions.stream()
.map(ResourceAction::getId)
.collect(Collectors.toSet());
// find which action IDs were deleted
oldActionIds.removeAll(newActionIds); // The remaining IDs are the deleted ones
// delete orphaned state records - this means that we actually reset the flow for users which completed the action which is being removed
// it seems like the best way to handle this
if (!oldActionIds.isEmpty()) {
policyStateProvider.removeByCompletedActions(policy.getId(), oldActionIds);
}
RealmModel realm = getRealm();
// remove all existing actions of the policy
realm.removeComponents(policy.getId());
// add the new actions
// This method takes an ordered list of actions. First action in the list has the highest priority, last action has the lowest priority
public void createActions(ResourcePolicy policy, List<ResourceAction> actions) {
for (int i = 0; i < actions.size(); i++) {
ResourceAction action = actions.get(i);
@ -209,50 +164,15 @@ public class ResourcePolicyManager {
}
public ResourceActionProvider getActionProvider(ResourceAction action) {
RealmModel realm = session.getContext().getRealm();
ComponentFactory<?, ?> actionFactory = (ComponentFactory<?, ?>) session.getKeycloakSessionFactory()
.getProviderFactory(ResourceActionProvider.class, action.getProviderId());
return (ResourceActionProvider) actionFactory.create(session, realm.getComponent(action.getId()));
return (ResourceActionProvider) actionFactory.create(session, getRealm().getComponent(action.getId()));
}
private RealmModel getRealm() {
return session.getContext().getRealm();
}
private void validateActions(List<ResourceAction> actions) {
// the list should be in the desired priority order
for (int i = 0; i < actions.size(); i++) {
ResourceAction currentAction = actions.get(i);
// check that each action's duration is positive.
if (currentAction.getAfter() <= 0) {
throw new BadRequestException("Validation Error: 'after' duration must be positive.");
}
if (i > 0) {// skip for initial action
ResourceAction previousAction = actions.get(i - 1);
// compare current with the previous action in the list
if (currentAction.getAfter() < previousAction.getAfter()) {
throw new BadRequestException(
String.format("Validation Error: The 'after' duration for action #%d (%s) cannot be less than the duration of the preceding action #%d (%s).",
i + 1, formatDuration(currentAction.getAfter()),
i, formatDuration(previousAction.getAfter()))
);
}
}
}
}
private String formatDuration(long millis) {
long days = Duration.ofMillis(millis).toDays();
if (days > 0) {
return String.format("%d day(s)", days);
} else {
long hours = Duration.ofMillis(millis).toHours();
return String.format("%d hour(s)", hours);
}
}
public void removePolicies() {
RealmModel realm = getRealm();
realm.getComponentsStream(realm.getId(), ResourcePolicyProvider.class.getName()).forEach(policy -> {
@ -388,7 +308,6 @@ public class ResourcePolicyManager {
}
public ResourcePolicy toModel(ResourcePolicyRepresentation rep) {
ResourcePolicyManager manager = new ResourcePolicyManager(session);
MultivaluedHashMap<String, String> config = ofNullable(rep.getConfig()).orElse(new MultivaluedHashMap<>());
for (ResourcePolicyConditionRepresentation condition : rep.getConditions()) {
@ -400,14 +319,14 @@ public class ResourcePolicyManager {
}
}
ResourcePolicy policy = manager.addPolicy(rep.getProviderId(), config);
ResourcePolicy policy = addPolicy(rep.getProviderId(), config);
List<ResourceAction> actions = new ArrayList<>();
for (ResourcePolicyActionRepresentation actionRep : rep.getActions()) {
actions.add(toModel(actionRep));
}
manager.updateActions(policy, actions);
createActions(policy, actions);
return policy;
}

View File

@ -187,26 +187,6 @@ public class ResourcePolicyManagementTest {
assertThat(policy.getName(), is("changed"));
}
@Test
public void testTimeVsPriorityConflictingActions() {
List<ResourcePolicyRepresentation> expectedPolicies = ResourcePolicyRepresentation.create()
.of(UserCreationTimeResourcePolicyProviderFactory.ID)
.withActions(
ResourcePolicyActionRepresentation.create().of(NotifyUserActionProviderFactory.ID)
.after(Duration.ofDays(10))
.build(),
ResourcePolicyActionRepresentation.create().of(DisableUserActionProviderFactory.ID)
.after(Duration.ofDays(5))
.build()
).build();
RealmResourcePolicies policies = managedRealm.admin().resources().policies();
try (Response response = policies.create(expectedPolicies)) {
assertThat(response.getStatus(), is(Status.BAD_REQUEST.getStatusCode()));
}
}
@Test
public void testPolicyDoesNotFallThroughActionsInSingleRun() {
managedRealm.admin().resources().policies().create(ResourcePolicyRepresentation.create()