From 6e6f96a0f4e87ee4c4488c49ee0bbf63920a4a57 Mon Sep 17 00:00:00 2001 From: Avgustin Marinov Date: Fri, 21 Jun 2024 08:27:24 +0300 Subject: [PATCH] Fix lastModifiedBy on modification perfomed by the JpaRolloutExecutor (#1748) 1. The auditor is got on transaction commit - so haven't used the tenant & user context until now - write system 2. The start/stop/delete are called by the user (saved in lastModifiedBy) but then executed in JpaRolloutExecutor So the change is: 1. Fix auditor for actions taken by JpaRolloutExecutor to be the createdBy 2. for start/stop/delete the auditor is set to the lastModifiedBy for the transaction (hence all action taken) Signed-off-by: Marinov Avgustin --- .gitignore | 3 ++ .../repository/jpa/JpaRolloutExecutor.java | 33 +++++++++++++++---- .../repository/jpa/JpaRolloutHandler.java | 2 ++ .../security/SpringSecurityAuditorAware.java | 24 +++++++++++++- 4 files changed, 55 insertions(+), 7 deletions(-) diff --git a/.gitignore b/.gitignore index a9f10d29c..ca03ce79c 100644 --- a/.gitignore +++ b/.gitignore @@ -55,6 +55,9 @@ classpath-data.json # Intellij *.iml +# local Spring configs +application-local.properties + # Maven maven.properties .flattened-pom.xml diff --git a/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaRolloutExecutor.java b/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaRolloutExecutor.java index 56f949e36..bd7b05db9 100644 --- a/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaRolloutExecutor.java +++ b/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaRolloutExecutor.java @@ -23,7 +23,6 @@ import jakarta.persistence.EntityManager; import lombok.extern.slf4j.Slf4j; import org.eclipse.hawkbit.repository.DeploymentManagement; import org.eclipse.hawkbit.repository.QuotaManagement; -import org.eclipse.hawkbit.repository.RepositoryProperties; import org.eclipse.hawkbit.repository.RolloutApprovalStrategy; import org.eclipse.hawkbit.repository.RolloutExecutor; import org.eclipse.hawkbit.repository.RolloutGroupManagement; @@ -60,6 +59,7 @@ import org.eclipse.hawkbit.repository.model.RolloutGroup.RolloutGroupStatus; import org.eclipse.hawkbit.repository.model.RolloutGroup.RolloutGroupSuccessCondition; import org.eclipse.hawkbit.repository.model.Target; import org.eclipse.hawkbit.repository.model.helper.EventPublisherHolder; +import org.eclipse.hawkbit.security.SpringSecurityAuditorAware; import org.eclipse.hawkbit.tenancy.TenantAware; import org.springframework.data.domain.PageRequest; import org.springframework.data.domain.Slice; @@ -152,20 +152,41 @@ public class JpaRolloutExecutor implements RolloutExecutor { case CREATING: handleCreateRollout((JpaRollout) rollout); break; - case DELETING: - handleDeleteRollout((JpaRollout) rollout); - break; case READY: handleReadyRollout(rollout); break; case STARTING: - handleStartingRollout(rollout); + // the lastModifiedBy user is probably the user that has actually called the rollout start (unless overridden) - not the creator + SpringSecurityAuditorAware.setAuditorOverride(rollout.getLastModifiedBy()); + try { + handleStartingRollout(rollout); + } finally { + // clear, ALWAYS, the set auditor override + SpringSecurityAuditorAware.clearAuditorOverride(); + } break; case RUNNING: handleRunningRollout((JpaRollout) rollout); break; case STOPPING: - handleStopRollout((JpaRollout) rollout); + // the lastModifiedBy user is probably the user that has actually called the rollout stop (unless overridden) - not the creator + SpringSecurityAuditorAware.setAuditorOverride(rollout.getLastModifiedBy()); + try { + handleStopRollout((JpaRollout) rollout); + } finally { + // clear, ALWAYS, the set auditor override + SpringSecurityAuditorAware.clearAuditorOverride(); + } + break; + case DELETING: + // the lastModifiedBy user is probably the user that has actually called the rollout delete (unless overridden) - not the creator + SpringSecurityAuditorAware.setAuditorOverride(rollout.getLastModifiedBy()); + try { + handleDeleteRollout((JpaRollout) rollout); + } finally { + // clear, ALWAYS, the set auditor override + SpringSecurityAuditorAware.clearAuditorOverride(); + } break; default: log.error("Rollout in status {} not supposed to be handled!", rollout.getStatus()); diff --git a/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaRolloutHandler.java b/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaRolloutHandler.java index a6595c35a..b968c88ce 100644 --- a/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaRolloutHandler.java +++ b/hawkbit-repository/hawkbit-repository-jpa/src/main/java/org/eclipse/hawkbit/repository/jpa/JpaRolloutHandler.java @@ -104,6 +104,8 @@ public class JpaRolloutHandler implements RolloutHandler { DeploymentHelper.runInNewTransaction(txManager, handlerId + "-" + rolloutId, status -> { rolloutManagement.get(rolloutId).ifPresentOrElse( rollout -> { + // auditor is retrieved and set on transaction commit + // if not overridden, the system user will be the auditor rollout.getAccessControlContext().ifPresentOrElse( context -> // has stored context - executes it with it contextAware.runInContext( diff --git a/hawkbit-security-core/src/main/java/org/eclipse/hawkbit/security/SpringSecurityAuditorAware.java b/hawkbit-security-core/src/main/java/org/eclipse/hawkbit/security/SpringSecurityAuditorAware.java index b12e9719e..6580e9be8 100644 --- a/hawkbit-security-core/src/main/java/org/eclipse/hawkbit/security/SpringSecurityAuditorAware.java +++ b/hawkbit-security-core/src/main/java/org/eclipse/hawkbit/security/SpringSecurityAuditorAware.java @@ -24,8 +24,17 @@ import org.springframework.security.oauth2.core.oidc.user.OidcUser; */ public class SpringSecurityAuditorAware implements AuditorAware { + // Sometimes 'system' need to override the auditor when do create/modify actions in context of a tenant and user. + // Though this could be made using runAsTenantAsUser sometimes (as in transaction) this override is needed + // after runAsTenantAsUser (because it seems that auditor is got in commit time). + // So this thread local variable provides option to override explicitly the auditor. + private static final ThreadLocal AUDITOR_OVERRIDE = new ThreadLocal<>(); + @Override public Optional getCurrentAuditor() { + if (AUDITOR_OVERRIDE.get() != null) { + return Optional.of(AUDITOR_OVERRIDE.get()); + } final Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); @@ -36,7 +45,20 @@ public class SpringSecurityAuditorAware implements AuditorAware { return Optional.ofNullable(getCurrentAuditor(authentication)); } - private static String getCurrentAuditor(final Authentication authentication) { + // Always shall be followed by {@link #clearAuditorOverride} + public static void setAuditorOverride(final String auditor) { + if (auditor == null) { + AUDITOR_OVERRIDE.remove(); + } else { + AUDITOR_OVERRIDE.set(auditor); + } + } + + public static void clearAuditorOverride() { + AUDITOR_OVERRIDE.remove(); + } + + protected String getCurrentAuditor(final Authentication authentication) { if (authentication.getPrincipal() instanceof UserDetails) { return ((UserDetails) authentication.getPrincipal()).getUsername(); }