From d40d3af2adf16bbc337c560b9d15fcafc81bc9e6 Mon Sep 17 00:00:00 2001 From: nscuro Date: Wed, 9 Oct 2024 14:20:49 +0200 Subject: [PATCH] Enhance policy violation de-duplication logic Instead of just discarding *any* duplicate, compare them first, and only keep the most "useful" one. https://github.com/DependencyTrack/dependency-track/issues/4215#issuecomment-2400099379 Signed-off-by: nscuro --- .../persistence/PolicyQueryManager.java | 54 ++++++- .../policy/PolicyEngineTest.java | 139 +++++++++++++++++- 2 files changed, 186 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/dependencytrack/persistence/PolicyQueryManager.java b/src/main/java/org/dependencytrack/persistence/PolicyQueryManager.java index 2f2637aa2..3aa3b0719 100644 --- a/src/main/java/org/dependencytrack/persistence/PolicyQueryManager.java +++ b/src/main/java/org/dependencytrack/persistence/PolicyQueryManager.java @@ -46,6 +46,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.UUID; @@ -187,13 +188,54 @@ public synchronized void reconcilePolicyViolations( final var existingViolationByIdentity = new HashMap(); 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) { + // Previous (<= 4.12.0) reconciliation logic allowed for duplicate violations to exist. + // Take that into consideration and ensure their deletion. + existingViolationByIdentity.compute(new ViolationIdentity(violation), (ignored, duplicateViolation) -> { + if (duplicateViolation == null) { + return violation; + } + + // Prefer to keep violations with existing analysis. + if (violation.getAnalysis() != null && duplicateViolation.getAnalysis() == null) { + violationsToDelete.add(duplicateViolation); + return violation; + } else if (violation.getAnalysis() == null && duplicateViolation.getAnalysis() != null) { + violationsToDelete.add(violation); + return duplicateViolation; + } + + // If none of the violations have an analysis, prefer to keep the oldest. + if (violation.getAnalysis() == null && duplicateViolation.getAnalysis() == null) { + final int timestampComparisonResult = Objects.compare( + violation.getTimestamp(), duplicateViolation.getTimestamp(), Date::compareTo); + if (timestampComparisonResult < 0) { + // Duplicate violation is newer. + violationsToDelete.add(duplicateViolation); + return violation; + } else if (timestampComparisonResult > 0) { + // Duplicate violation is older. + violationsToDelete.add(violation); + return duplicateViolation; + } + + // Everything else being equal, keep the duplicate violation. + violationsToDelete.add(violation); + return duplicateViolation; + } + + // If both violations have an analysis, prefer to keep the suppressed one. + if (violation.getAnalysis().isSuppressed() && !duplicateViolation.getAnalysis().isSuppressed()) { + violationsToDelete.add(duplicateViolation); + return violation; + } else if (!violation.getAnalysis().isSuppressed() && duplicateViolation.getAnalysis().isSuppressed()) { + violationsToDelete.add(violation); + return duplicateViolation; + } + + // Everything else being equal, keep the duplicate violation. violationsToDelete.add(violation); - } + return duplicateViolation; + }); } final var reportedViolationsByIdentity = new HashMap(); diff --git a/src/test/java/org/dependencytrack/policy/PolicyEngineTest.java b/src/test/java/org/dependencytrack/policy/PolicyEngineTest.java index 71f30b06c..e5683a1c9 100644 --- a/src/test/java/org/dependencytrack/policy/PolicyEngineTest.java +++ b/src/test/java/org/dependencytrack/policy/PolicyEngineTest.java @@ -36,6 +36,7 @@ import org.dependencytrack.model.Project; import org.dependencytrack.model.Severity; import org.dependencytrack.model.Tag; +import org.dependencytrack.model.ViolationAnalysis; import org.dependencytrack.model.ViolationAnalysisState; import org.dependencytrack.model.Vulnerability; import org.dependencytrack.notification.NotificationGroup; @@ -50,11 +51,12 @@ import org.junit.Test; import org.junit.jupiter.api.Assertions; -import java.sql.Date; import java.time.Duration; import java.time.Instant; +import java.time.temporal.ChronoUnit; import java.util.ArrayList; import java.util.Collections; +import java.util.Date; import java.util.List; import java.util.UUID; import java.util.concurrent.ConcurrentLinkedQueue; @@ -468,4 +470,139 @@ public void violationReconciliationTest() { assertThat(violation.getPolicyCondition().getPolicy().getName()).isEqualTo("Policy A")); } + @Test + public void violationReconciliationWithDuplicatesTest() { + final var project = new Project(); + project.setName("acme-app"); + qm.persist(project); + + final var componentA = new Component(); + componentA.setProject(project); + componentA.setName("acme-lib-a"); + qm.persist(componentA); + final var componentB = new Component(); + componentB.setProject(project); + componentB.setName("acme-lib-b"); + qm.persist(componentB); + final var componentC = new Component(); + componentC.setProject(project); + componentC.setName("acme-lib-c"); + qm.persist(componentC); + final var componentD = new Component(); + componentD.setProject(project); + componentD.setName("acme-lib-d"); + qm.persist(componentD); + + final var policy = new Policy(); + policy.setName("policy"); + policy.setOperator(Operator.ALL); + policy.setViolationState(ViolationState.INFO); + qm.persist(policy); + + final var policyCondition = new PolicyCondition(); + policyCondition.setPolicy(policy); + policyCondition.setSubject(Subject.COORDINATES); + policyCondition.setOperator(PolicyCondition.Operator.MATCHES); + policyCondition.setValue(""" + {name: "*"} + """); + qm.persist(policyCondition); + + final var violationTimestamp = new Date(); + + // Create two identical violations for component A. + final var policyViolationComponentA = new PolicyViolation(); + policyViolationComponentA.setPolicyCondition(policyCondition); + policyViolationComponentA.setComponent(componentA); + policyViolationComponentA.setTimestamp(violationTimestamp); + policyViolationComponentA.setType(PolicyViolation.Type.OPERATIONAL); + qm.persist(policyViolationComponentA); + final var policyViolationDuplicateComponentA = new PolicyViolation(); + policyViolationDuplicateComponentA.setPolicyCondition(policyCondition); + policyViolationDuplicateComponentA.setComponent(componentA); + policyViolationDuplicateComponentA.setTimestamp(violationTimestamp); + policyViolationDuplicateComponentA.setType(PolicyViolation.Type.OPERATIONAL); + qm.persist(policyViolationDuplicateComponentA); + + // Create two almost identical violations for component B, + // where one of them is older than the other. + final var policyViolationComponentB = new PolicyViolation(); + policyViolationComponentB.setPolicyCondition(policyCondition); + policyViolationComponentB.setComponent(componentB); + policyViolationComponentB.setTimestamp(violationTimestamp); + policyViolationComponentB.setType(PolicyViolation.Type.OPERATIONAL); + qm.persist(policyViolationComponentB); + final var policyViolationDuplicateComponentB = new PolicyViolation(); + policyViolationDuplicateComponentB.setPolicyCondition(policyCondition); + policyViolationDuplicateComponentB.setComponent(componentB); + policyViolationDuplicateComponentB.setTimestamp(Date.from(Instant.now().minus(5, ChronoUnit.MINUTES))); + policyViolationDuplicateComponentB.setType(PolicyViolation.Type.OPERATIONAL); + qm.persist(policyViolationDuplicateComponentB); + + // Create two identical violations for component C. + // Only one of them has an analysis. + final var policyViolationComponentC = new PolicyViolation(); + policyViolationComponentC.setPolicyCondition(policyCondition); + policyViolationComponentC.setComponent(componentC); + policyViolationComponentC.setTimestamp(violationTimestamp); + policyViolationComponentC.setType(PolicyViolation.Type.OPERATIONAL); + qm.persist(policyViolationComponentC); + final var policyViolationDuplicateComponentC = new PolicyViolation(); + policyViolationDuplicateComponentC.setPolicyCondition(policyCondition); + policyViolationDuplicateComponentC.setComponent(componentC); + policyViolationDuplicateComponentC.setTimestamp(violationTimestamp); + policyViolationDuplicateComponentC.setType(PolicyViolation.Type.OPERATIONAL); + qm.persist(policyViolationDuplicateComponentC); + final var violationAnalysisDuplicateComponentC = new ViolationAnalysis(); + violationAnalysisDuplicateComponentC.setPolicyViolation(policyViolationDuplicateComponentC); + violationAnalysisDuplicateComponentC.setComponent(componentC); + violationAnalysisDuplicateComponentC.setViolationAnalysisState(ViolationAnalysisState.APPROVED); + qm.persist(violationAnalysisDuplicateComponentC); + + // Create two identical violations for component D. + // Both have an analysis, but only one of them is suppressed. + final var policyViolationComponentD = new PolicyViolation(); + policyViolationComponentD.setPolicyCondition(policyCondition); + policyViolationComponentD.setComponent(componentD); + policyViolationComponentD.setTimestamp(violationTimestamp); + policyViolationComponentD.setType(PolicyViolation.Type.OPERATIONAL); + qm.persist(policyViolationComponentD); + final var violationAnalysisComponentD = new ViolationAnalysis(); + violationAnalysisComponentD.setPolicyViolation(policyViolationComponentD); + violationAnalysisComponentD.setComponent(componentD); + violationAnalysisComponentD.setViolationAnalysisState(ViolationAnalysisState.REJECTED); + qm.persist(violationAnalysisComponentD); + final var policyViolationDuplicateComponentD = new PolicyViolation(); + policyViolationDuplicateComponentD.setPolicyCondition(policyCondition); + policyViolationDuplicateComponentD.setComponent(componentD); + policyViolationDuplicateComponentD.setTimestamp(violationTimestamp); + policyViolationDuplicateComponentD.setType(PolicyViolation.Type.OPERATIONAL); + qm.persist(policyViolationDuplicateComponentD); + final var violationAnalysisDuplicateComponentD = new ViolationAnalysis(); + violationAnalysisDuplicateComponentD.setPolicyViolation(policyViolationDuplicateComponentD); + violationAnalysisDuplicateComponentD.setComponent(componentD); + violationAnalysisDuplicateComponentD.setViolationAnalysisState(ViolationAnalysisState.REJECTED); + violationAnalysisDuplicateComponentD.setSuppressed(true); + qm.persist(violationAnalysisDuplicateComponentD); + + final var policyEngine = new PolicyEngine(); + policyEngine.evaluate(List.of(componentA, componentB, componentC, componentD)); + + // For component A, the first violation (i.e. lower ID) must be kept. + assertThat(qm.getAllPolicyViolations(componentA, /* includeSuppressed */ true)).satisfiesExactlyInAnyOrder( + violation -> assertThat(violation.getId()).isEqualTo(policyViolationComponentA.getId())); + + // For component B, the older violation must be kept. + assertThat(qm.getAllPolicyViolations(componentB, /* includeSuppressed */ true)).satisfiesExactlyInAnyOrder( + violation -> assertThat(violation.getId()).isEqualTo(policyViolationDuplicateComponentB.getId())); + + // For component C, the violation with analysis must be kept. + assertThat(qm.getAllPolicyViolations(componentC, /* includeSuppressed */ true)).satisfiesExactlyInAnyOrder( + violation -> assertThat(violation.getId()).isEqualTo(policyViolationDuplicateComponentC.getId())); + + // For component D, the suppressed violation must be kept. + assertThat(qm.getAllPolicyViolations(componentD, /* includeSuppressed */ true)).satisfiesExactlyInAnyOrder( + violation -> assertThat(violation.getId()).isEqualTo(policyViolationDuplicateComponentD.getId())); + } + } \ No newline at end of file