Backport: Prevent duplicate policy violations

src/main/java/org/dependencytrack/persistence/PolicyQueryManager.java

@@ -43,10 +43,13 @@
 import java.util.Collection;
 import java.util.Date;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.UUID;
 
+import static org.dependencytrack.util.PersistenceUtil.assertNonPersistentAll;
 import static org.dependencytrack.util.PersistenceUtil.assertPersistent;
 import static org.dependencytrack.util.PersistenceUtil.assertPersistentAll;
 
@@ -153,38 +156,79 @@ public PolicyCondition updatePolicyCondition(final PolicyCondition policyConditi
         return persist(pc);
     }
 
+    private record ViolationIdentity(
+            long componentId,
+            long conditionId,
+            PolicyViolation.Type type) {
+
+        private ViolationIdentity(final PolicyViolation violation) {
+            this(violation.getComponent().getId(), violation.getPolicyCondition().getId(), violation.getType());
+        }
+
+    }
+
     /**
      * Intelligently adds dependencies for components that are not already a dependency
      * of the specified project and removes the dependency relationship for components
      * that are not in the list of specified components.
      * @param component the project to bind components to
-     * @param policyViolations the complete list of existing dependent components
-     */
-    public synchronized void reconcilePolicyViolations(final Component component, final List<PolicyViolation> policyViolations) {
-        // Removes violations as dependencies to the project for all
-        // components not included in the list provided
-        List<PolicyViolation> markedForDeletion = new ArrayList<>();
-        for (final PolicyViolation existingViolation: getAllPolicyViolations(component)) {
-            boolean keep = false;
-            for (final PolicyViolation violation: policyViolations) {
-                if (violation.getType() == existingViolation.getType()
-                        && violation.getPolicyCondition().getId() == existingViolation.getPolicyCondition().getId()
-                        && violation.getComponent().getId() == existingViolation.getComponent().getId())
-                {
-                    keep = true;
-                    break;
+     * @param reportedViolations the complete list of existing dependent components
+     */
+    public synchronized void reconcilePolicyViolations(
+            final Component component,
+            final List<PolicyViolation> reportedViolations) {
+        assertPersistent(component, "component must be persistent");
+        assertNonPersistentAll(reportedViolations, "reportedViolations must not be persistent");
+
+        runInTransaction(() -> {
+            final List<PolicyViolation> existingViolations = getAllPolicyViolations(component);
+            final var violationsToCreate = new ArrayList<PolicyViolation>();
+            final var violationsToDelete = new ArrayList<PolicyViolation>();
+
+            final var existingViolationByIdentity = new HashMap<ViolationIdentity, PolicyViolation>();
+            for (final PolicyViolation violation : existingViolations) {
+                // Previous reconciliation logic allowed for duplicate violations
+                // to exist. Take that into consideration and ensure their deletion.
+                final boolean isDuplicate = existingViolationByIdentity.putIfAbsent(
+                        new ViolationIdentity(violation), violation) != null;
+                if (isDuplicate) {
+                    violationsToDelete.add(violation);
                 }
             }
-            if (!keep) {
-                markedForDeletion.add(existingViolation);
+
+            final var reportedViolationsByIdentity = new HashMap<ViolationIdentity, PolicyViolation>();
+            for (final PolicyViolation violation : reportedViolations) {
+                reportedViolationsByIdentity.put(new ViolationIdentity(violation), violation);
             }
-        }
-        if (!markedForDeletion.isEmpty()) {
-            for (final PolicyViolation violation : markedForDeletion) {
-                deleteViolationAnalysisTrail(violation);
+
+            final Set<ViolationIdentity> violationIdentities = new HashSet<>(
+                    existingViolationByIdentity.size() + reportedViolationsByIdentity.size());
+            violationIdentities.addAll(existingViolationByIdentity.keySet());
+            violationIdentities.addAll(reportedViolationsByIdentity.keySet());
+
+            for (final ViolationIdentity identity : violationIdentities) {
+                final PolicyViolation existingViolation = existingViolationByIdentity.get(identity);
+                final PolicyViolation reportedViolation = reportedViolationsByIdentity.get(identity);
+
+                if (existingViolation == null) {
+                    violationsToCreate.add(reportedViolation);
+                } else if (reportedViolation == null) {
+                    violationsToDelete.add(existingViolation);
+                }
             }
-            delete(markedForDeletion);
-        }
+
+            if (!violationsToCreate.isEmpty()) {
+                persist(violationsToCreate);
+            }
+
+            if (!violationsToDelete.isEmpty()) {
+                for (final PolicyViolation violation : violationsToDelete) {
+                    deleteViolationAnalysisTrail(violation);
+                }
+
+                delete(violationsToDelete);
+            }
+        });
     }
 
     /**
@@ -222,7 +266,7 @@ public PolicyViolation clonePolicyViolation(PolicyViolation sourcePolicyViolatio
             if(comments != null){
                 violationAnalysis.setAnalysisComments(comments);
             }
-            policyViolation.setAnalysis(violationAnalysis); 
+            policyViolation.setAnalysis(violationAnalysis);
             policyViolation.getAnalysis().setPolicyViolation(policyViolation);
             policyViolation.setUuid(sourcePolicyViolation.getUuid());
             return policyViolation;
@@ -465,7 +509,7 @@ public List<ViolationAnalysisComment> cloneViolationAnalysisComments(PolicyViola
                 comments.add(comment);
             }
         }
-        
+
         return comments;
     }
 

src/main/java/org/dependencytrack/policy/PolicyEngine.java

@@ -27,6 +27,7 @@
 import org.dependencytrack.model.Tag;
 import org.dependencytrack.persistence.QueryManager;
 import org.dependencytrack.util.NotificationUtil;
+
 import java.util.ArrayList;
 import java.util.Date;
 import java.util.List;
@@ -102,10 +103,10 @@ private List<PolicyViolation> evaluate(final QueryManager qm, final List<Policy>
                 }
                 if (Policy.Operator.ANY == policy.getOperator()) {
                     if (policyConditionsViolated > 0) {
-                        policyViolations.addAll(createPolicyViolations(qm, policyConditionViolations));
+                        policyViolations.addAll(createPolicyViolations(policyConditionViolations));
                     }
                 } else if (Policy.Operator.ALL == policy.getOperator() && policyConditionsViolated == policy.getPolicyConditions().size()) {
-                    policyViolations.addAll(createPolicyViolations(qm, policyConditionViolations));
+                    policyViolations.addAll(createPolicyViolations(policyConditionViolations));
                 }
             }
         }
@@ -125,15 +126,15 @@ private boolean isPolicyAssignedToProject(Policy policy, Project project) {
         return (policy.getProjects().stream().anyMatch(p -> p.getId() == project.getId()) || (Boolean.TRUE.equals(policy.isIncludeChildren()) && isPolicyAssignedToParentProject(policy, project)));
     }
 
-    private List<PolicyViolation> createPolicyViolations(final QueryManager qm, final List<PolicyConditionViolation> pcvList) {
+    private List<PolicyViolation> createPolicyViolations(final List<PolicyConditionViolation> pcvList) {
         final List<PolicyViolation> policyViolations = new ArrayList<>();
         for (PolicyConditionViolation pcv : pcvList) {
             final PolicyViolation pv = new PolicyViolation();
             pv.setComponent(pcv.getComponent());
             pv.setPolicyCondition(pcv.getPolicyCondition());
             pv.setType(determineViolationType(pcv.getPolicyCondition().getSubject()));
             pv.setTimestamp(new Date());
-            policyViolations.add(qm.addPolicyViolationIfNotExist(pv));
+            policyViolations.add(pv);
         }
         return policyViolations;
     }

src/main/java/org/dependencytrack/tasks/BomUploadProcessingTask.java

@@ -77,6 +77,7 @@
 import java.util.Set;
 import java.util.UUID;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.ReentrantLock;
 import java.util.function.Predicate;
 
 import static org.apache.commons.lang3.StringUtils.isNotBlank;
@@ -99,6 +100,7 @@
 import static org.dependencytrack.parser.cyclonedx.util.ModelConverter.convertToProject;
 import static org.dependencytrack.parser.cyclonedx.util.ModelConverter.convertToProjectMetadata;
 import static org.dependencytrack.parser.cyclonedx.util.ModelConverter.flatten;
+import static org.dependencytrack.util.LockUtil.getLockForProjectAndNamespace;
 import static org.dependencytrack.util.PersistenceUtil.applyIfChanged;
 import static org.dependencytrack.util.PersistenceUtil.assertPersistent;
 
@@ -169,17 +171,21 @@ private void processEvent(final Context ctx, final BomUploadEvent event) {
             ctx.bomSerialNumber = cdxBom.getSerialNumber().replaceFirst("^urn:uuid:", "");
         }
 
+        final ReentrantLock lock = getLockForProjectAndNamespace(ctx.project, getClass().getSimpleName());
         try (var ignoredMdcBomFormat = MDC.putCloseable(MDC_BOM_FORMAT, ctx.bomFormat.getFormatShortName());
              var ignoredMdcBomSpecVersion = MDC.putCloseable(MDC_BOM_SPEC_VERSION, ctx.bomSpecVersion);
              var ignoredMdcBomSerialNumber = MDC.putCloseable(MDC_BOM_SERIAL_NUMBER, ctx.bomSerialNumber);
              var ignoredMdcBomVersion = MDC.putCloseable(MDC_BOM_VERSION, String.valueOf(ctx.bomVersion))) {
+            lock.lock();
             processBom(ctx, cdxBom);
 
             LOGGER.debug("Dispatching %d events".formatted(eventsToDispatch.size()));
             eventsToDispatch.forEach(Event::dispatch);
         } catch (RuntimeException e) {
             LOGGER.error("Failed to process BOM", e);
             dispatchBomProcessingFailedNotification(ctx, e);
+        } finally {
+            lock.unlock();
         }
     }
 
@@ -272,8 +278,6 @@ private void processBom(final Context ctx, final org.cyclonedx.model.Bom cdxBom)
             // by project; This is not the case for vulnerabilities. We don't want the entire TRX to fail,
             // just because another TRX created or modified the same vulnerability record.
 
-            // TODO: Introduce locking by project ID / UUID to avoid processing BOMs for the same project concurrently.
-
             qm.runInTransaction(() -> {
                 final Project persistentProject = processProject(ctx, qm, project, projectMetadata);
 

src/main/java/org/dependencytrack/tasks/PolicyEvaluationTask.java

@@ -18,34 +18,52 @@
  */
 package org.dependencytrack.tasks;
 
-import alpine.common.logging.Logger;
 import alpine.event.framework.Event;
 import alpine.event.framework.Subscriber;
 import org.dependencytrack.event.PolicyEvaluationEvent;
 import org.dependencytrack.event.ProjectMetricsUpdateEvent;
 import org.dependencytrack.model.Component;
 import org.dependencytrack.model.Project;
 import org.dependencytrack.policy.PolicyEngine;
+import org.slf4j.MDC;
+
 import java.util.ArrayList;
 import java.util.List;
+import java.util.concurrent.locks.ReentrantLock;
 
-public class PolicyEvaluationTask implements Subscriber {
+import static org.dependencytrack.common.MdcKeys.MDC_EVENT_TOKEN;
+import static org.dependencytrack.common.MdcKeys.MDC_PROJECT_NAME;
+import static org.dependencytrack.common.MdcKeys.MDC_PROJECT_UUID;
+import static org.dependencytrack.common.MdcKeys.MDC_PROJECT_VERSION;
+import static org.dependencytrack.util.LockUtil.getLockForProjectAndNamespace;
 
-    private static final Logger LOGGER = Logger.getLogger(PolicyEvaluationTask.class);
+public class PolicyEvaluationTask implements Subscriber {
 
     /**
      * {@inheritDoc}
      */
     @Override
     public void inform(final Event e) {
-        if (e instanceof PolicyEvaluationEvent event) {
-            if (event.getProject() != null) {
-                if (event.getComponents() != null && !event.getComponents().isEmpty()) {
-                    performPolicyEvaluation(event.getProject(), event.getComponents());
-                } else {
-                    performPolicyEvaluation(event.getProject(), new ArrayList<>());
-                }
+        if (!(e instanceof final PolicyEvaluationEvent event)) {
+            return;
+        }
+        if (event.getProject() == null) {
+            return;
+        }
+
+        final ReentrantLock lock = getLockForProjectAndNamespace(event.getProject(), getClass().getSimpleName());
+        try (var ignoredMdcProjectUuid = MDC.putCloseable(MDC_PROJECT_UUID, event.getProject().getUuid().toString());
+             var ignoredMdcProjectName = MDC.putCloseable(MDC_PROJECT_NAME, event.getProject().getName());
+             var ignoredMdcProjectVersion = MDC.putCloseable(MDC_PROJECT_VERSION, event.getProject().getVersion());
+             var ignoredMdcEventToken = MDC.putCloseable(MDC_EVENT_TOKEN, event.getChainIdentifier().toString())) {
+            lock.lock();
+            if (event.getComponents() != null && !event.getComponents().isEmpty()) {
+                performPolicyEvaluation(event.getProject(), event.getComponents());
+            } else {
+                performPolicyEvaluation(event.getProject(), new ArrayList<>());
             }
+        } finally {
+            lock.unlock();
         }
     }
 

src/main/java/org/dependencytrack/tasks/VulnerabilityAnalysisTask.java

@@ -43,12 +43,15 @@
 import org.dependencytrack.tasks.scanners.SnykAnalysisTask;
 import org.dependencytrack.tasks.scanners.TrivyAnalysisTask;
 import org.dependencytrack.tasks.scanners.VulnDbAnalysisTask;
+
 import java.time.Duration;
 import java.time.Instant;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.UUID;
-import java.util.stream.Collectors;
+import java.util.concurrent.locks.ReentrantLock;
+
+import static org.dependencytrack.util.LockUtil.getLockForProjectAndNamespace;
 
 public class VulnerabilityAnalysisTask implements Subscriber {
 
@@ -60,33 +63,59 @@ public class VulnerabilityAnalysisTask implements Subscriber {
     @Override
     public void inform(final Event e) {
         if (e instanceof VulnerabilityAnalysisEvent event) {
-            if (event.getComponents() != null && event.getComponents().size() > 0) {
-                final List<Component> components = new ArrayList<>();
-                try (final QueryManager qm = new QueryManager()) {
-                    for (final Component c : event.getComponents()) {
-                        // Ensures the current component (and related objects such as Project) are attached to the
-                        // current persistence manager. This may cause duplicate projects to be created and other
-                        // unexpected behavior.
-                        components.add(qm.getObjectByUuid(Component.class, c.getUuid()));
+            final ReentrantLock projectLock;
+            if (event.getProject() != null) {
+                projectLock = getLockForProjectAndNamespace(event.getProject(), getClass().getSimpleName());
+            } else {
+                projectLock = null;
+            }
+
+            try {
+                if (projectLock != null) {
+                    projectLock.lock();
+                }
+
+                if (event.getComponents() != null && !event.getComponents().isEmpty()) {
+                    final List<Component> components = new ArrayList<>();
+                    try (final QueryManager qm = new QueryManager()) {
+                        for (final Component c : event.getComponents()) {
+                            // Ensures the current component (and related objects such as Project) are attached to the
+                            // current persistence manager. This may cause duplicate projects to be created and other
+                            // unexpected behavior.
+                            components.add(qm.getObjectByUuid(Component.class, c.getUuid()));
+                        }
+                        analyzeComponents(qm, components, e);
                     }
-                    analyzeComponents(qm, components, e);
+                }
+            } finally {
+                if (projectLock != null) {
+                    projectLock.unlock();
                 }
             }
-        } else if (e instanceof PortfolioVulnerabilityAnalysisEvent event) {
+        } else if (e instanceof PortfolioVulnerabilityAnalysisEvent) {
             LOGGER.info("Analyzing portfolio");
             try (final QueryManager qm = new QueryManager()) {
                 final List<UUID> projectUuids = qm.getAllProjects(true)
                         .stream()
                         .map(Project::getUuid)
-                        .collect(Collectors.toList());
+                        .toList();
                 for (final UUID projectUuid : projectUuids) {
                     final Project project = qm.getObjectByUuid(Project.class, projectUuid);
-                    if (project == null) continue;
-                    final List<Component> components = qm.getAllComponents(project);
-                    LOGGER.info("Analyzing " + components.size() + " components in project: " + project.getUuid());
-                    analyzeComponents(qm, components, e);
-                    performPolicyEvaluation(project, components);
-                    LOGGER.info("Completed scheduled analysis of " + components.size() + " components in project: " + project.getUuid());
+                    if (project == null) {
+                        continue;
+                    }
+
+                    final ReentrantLock projectLock = getLockForProjectAndNamespace(project, getClass().getSimpleName());
+                    try {
+                        projectLock.lock();
+                        final List<Component> components = qm.getAllComponents(project);
+                        LOGGER.info("Analyzing " + components.size() + " components in project: " + project.getUuid());
+                        analyzeComponents(qm, components, e);
+                        performPolicyEvaluation(project, components);
+                        LOGGER.info("Completed scheduled analysis of " + components.size() + " components in project: " + project.getUuid());
+                    } finally {
+                        projectLock.unlock();
+                    }
                 }
             }
             LOGGER.info("Portfolio analysis complete");