Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Backport: Enhance policy violation de-duplication logic #4235

Merged
merged 1 commit into from
Oct 9, 2024
Merged

Backport: Enhance policy violation de-duplication logic #4235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 48 additions & 6 deletions src/main/java/org/dependencytrack/persistence/PolicyQueryManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -187,13 +188,54 @@ public synchronized void reconcilePolicyViolations(

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) {
// 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<ViolationIdentity, PolicyViolation>();
Expand Down
139 changes: 138 additions & 1 deletion src/test/java/org/dependencytrack/policy/PolicyEngineTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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()));
}

}