Skip to content

Commit d779af1

Browse files
emyhrbergemyhrberg
committed
Fix comments from nscuro to move severity filtering code
- Move severity filtering code, so it doesn't reach the publisher - Remove some unnecessary publisher tests - Add new tests in NotificationRouterTest, SendMailPublisherTest and SlackPublisherTest Signed-off-by: Erik Myhrberg <[email protected]>
1 parent 271d118 commit d779af1

File tree

11 files changed

+323
-136
lines changed

11 files changed

+323
-136
lines changed

‎src/main/java/org/dependencytrack/model/NotificationRule.java

Lines changed: 11 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -49,11 +49,13 @@
4949
import javax.jdo.annotations.PrimaryKey;
5050
import javax.jdo.annotations.Unique;
5151
import java.io.Serializable;
52+
import java.util.Arrays;
5253
import java.util.ArrayList;
5354
import java.util.Collections;
5455
import java.util.Date;
5556
import java.util.List;
5657
import java.util.Set;
58+
import java.util.stream.Collectors;
5759
import java.util.TreeSet;
5860
import java.util.UUID;
5961

@@ -329,32 +331,22 @@ public void setNotifyOn(Set<NotificationGroup> groups) {
329331
this.notifyOn = sb.toString();
330332
}
331333

332-
public List<Severity> getNotifySeverities(){
333-
List<Severity> result = new ArrayList<>();
334-
if (notifySeverities != null) {
335-
String[] severities = notifySeverities.split(",");
336-
for (String s: severities) {
337-
result.add(Severity.valueOf(s.trim()));
338-
}
339-
} else {
340-
return List.of(Severity.values());
334+
public List<Severity> getNotifySeverities() {
335+
if (notifySeverities == null) {
336+
return List.of(); // empty (user did not pick anything)
341337
}
342-
return result;
338+
return Arrays.stream(notifySeverities.split(","))
339+
.map(String::trim)
340+
.map(Severity::valueOf)
341+
.collect(Collectors.toList());
343342
}
344343

345344
public void setNotifySeverities(List<Severity> notifySeverities){
346-
if (notifySeverities.isEmpty()){
345+
if (notifySeverities == null || notifySeverities.isEmpty()){
347346
this.notifySeverities = null;
348347
return;
349348
}
350-
StringBuilder sb = new StringBuilder();
351-
for (int i=0; i<notifySeverities.size(); i++) {
352-
sb.append(notifySeverities.get(i));
353-
if (i+1 < notifySeverities.size()) {
354-
sb.append(",");
355-
}
356-
}
357-
this.notifySeverities = sb.toString();
349+
this.notifySeverities = notifySeverities.stream().map(Enum::name).collect(Collectors.joining(","));
358350
}
359351

360352
public NotificationPublisher getPublisher() {

‎src/main/java/org/dependencytrack/notification/NotificationRouter.java

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,9 @@
2626
import org.dependencytrack.model.NotificationPublisher;
2727
import org.dependencytrack.model.NotificationRule;
2828
import org.dependencytrack.model.Project;
29+
import org.dependencytrack.model.Severity;
2930
import org.dependencytrack.model.Tag;
31+
import org.dependencytrack.model.Vulnerability;
3032
import org.dependencytrack.notification.publisher.PublishContext;
3133
import org.dependencytrack.notification.publisher.Publisher;
3234
import org.dependencytrack.notification.vo.AnalysisDecisionChange;
@@ -90,7 +92,7 @@ public void inform(final Notification notification) {
9092
.add(CONFIG_TEMPLATE_KEY, notificationPublisher.getTemplate())
9193
.addAll(Json.createObjectBuilder(config))
9294
.build();
93-
publisher.inform(ruleCtx, restrictNotificationToRuleProjects(notification, rule), notificationPublisherConfig, rule.getNotifySeverities());
95+
publisher.inform(ruleCtx, restrictNotificationToRuleProjects(notification, rule), notificationPublisherConfig);
9496
} else {
9597
LOGGER.error("The defined notification publisher is not assignable from " + Publisher.class.getCanonicalName() + " (%s)".formatted(ruleCtx));
9698
}
@@ -218,12 +220,50 @@ List<NotificationRule> resolveRules(final PublishContext ctx, final Notification
218220
pm.detachCopyAll(result);
219221
LOGGER.debug("Matched %d notification rules (%s)".formatted(result.size(), ctx));
220222

223+
final List<NotificationRule> severityFiltered = new ArrayList<>();
224+
for (final NotificationRule rule : result) {
225+
List<Severity> severities = rule.getNotifySeverities();
226+
if (severities == null || severities.isEmpty()) {
227+
severities = List.of(Severity.values());
228+
}
229+
230+
// NewVulnerabilityIdentified
231+
if (notification.getSubject() instanceof NewVulnerabilityIdentified vi && vi.getVulnerability() != null) {
232+
Severity s = vi.getVulnerability().getSeverity();
233+
if (s == null || severities.contains(s)) {
234+
severityFiltered.add(rule);
235+
}
236+
// else: skip
237+
continue;
238+
}
239+
240+
// NewVulnerableDependency
241+
if (notification.getSubject() instanceof NewVulnerableDependency nd) {
242+
List<Vulnerability> vs = nd.getVulnerabilities();
243+
if (vs == null || vs.isEmpty()) {
244+
severityFiltered.add(rule);
245+
} else {
246+
List<Severity> finalSeverities = severities;
247+
boolean anyMatch = vs.stream()
248+
.map(Vulnerability::getSeverity)
249+
.anyMatch(sev -> sev != null && finalSeverities.contains(sev));
250+
if (anyMatch) {
251+
severityFiltered.add(rule);
252+
}
253+
}
254+
continue;
255+
}
256+
257+
// everything else (no severity to filter on)
258+
severityFiltered.add(rule);
259+
}
260+
221261
if (NotificationScope.PORTFOLIO.name().equals(notification.getScope())
222262
&& notification.getSubject() instanceof final NewVulnerabilityIdentified subject) {
223-
limitToProject(qm, ctx, rules, result, notification, subject.getComponent().getProject());
263+
limitToProject(qm, ctx, rules, severityFiltered, notification, subject.getComponent().getProject());
224264
} else if (NotificationScope.PORTFOLIO.name().equals(notification.getScope())
225265
&& notification.getSubject() instanceof final NewVulnerableDependency subject) {
226-
limitToProject(qm, ctx, rules, result, notification, subject.getComponent().getProject());
266+
limitToProject(qm, ctx, rules, severityFiltered, notification, subject.getComponent().getProject());
227267
} else if (NotificationScope.PORTFOLIO.name().equals(notification.getScope())
228268
&& notification.getSubject() instanceof final BomConsumedOrProcessed subject) {
229269
limitToProject(qm, ctx, rules, result, notification, subject.getProject());
@@ -405,4 +445,4 @@ private boolean isChildOfProjectMatching(final Project childProject, final Predi
405445
return false;
406446
}
407447

408-
}
448+
}

‎src/main/java/org/dependencytrack/notification/publisher/Publisher.java

Lines changed: 0 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@
2626
import io.pebbletemplates.pebble.template.PebbleTemplate;
2727
import org.dependencytrack.exception.PublisherException;
2828
import org.dependencytrack.model.ConfigPropertyConstants;
29-
import org.dependencytrack.model.Severity;
3029
import org.dependencytrack.notification.NotificationScope;
3130
import org.dependencytrack.notification.vo.AnalysisDecisionChange;
3231
import org.dependencytrack.notification.vo.BomConsumedOrProcessed;
@@ -48,7 +47,6 @@
4847
import java.io.Writer;
4948
import java.time.ZoneId;
5049
import java.util.HashMap;
51-
import java.util.List;
5250
import java.util.Map;
5351

5452
public interface Publisher {
@@ -63,30 +61,6 @@ public interface Publisher {
6361

6462
void inform(final PublishContext ctx, final Notification notification, final JsonObject config);
6563

66-
// Use severity filtering if configured
67-
default void inform(PublishContext ctx,
68-
Notification notification,
69-
JsonObject config,
70-
List<Severity> notifySeverities) {
71-
72-
if (notification == null || config == null) {
73-
return;
74-
}
75-
76-
// Filter out unwanted notifications based on the configured severities
77-
if (notifySeverities != null && !notifySeverities.isEmpty()
78-
&& notification.getSubject() instanceof NewVulnerabilityIdentified vuln) {
79-
80-
final Severity sev = vuln.getVulnerability().getSeverity();
81-
if (sev != null && !notifySeverities.contains(sev)) {
82-
return;
83-
}
84-
}
85-
86-
// Call the default inform method
87-
inform(ctx, notification, config);
88-
}
89-
9064
PebbleEngine getTemplateEngine();
9165

9266
default PebbleTemplate getTemplate(JsonObject config) {

‎src/test/java/org/dependencytrack/notification/NotificationRouterTest.java

Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import org.dependencytrack.model.NotificationRule;
2929
import org.dependencytrack.model.Project;
3030
import org.dependencytrack.model.Tag;
31+
import org.dependencytrack.model.Severity;
3132
import org.dependencytrack.model.Vex;
3233
import org.dependencytrack.model.Vulnerability;
3334
import org.dependencytrack.notification.publisher.DefaultNotificationPublishers;
@@ -346,6 +347,85 @@ void testDisabledRule() {
346347
assertThat(router.resolveRules(PublishContext.from(notification), notification)).isEmpty();
347348
}
348349

350+
@Test
351+
void testNewVulnerabilityIdentifiedShouldTriggerNotification() {
352+
final Project projectA = qm.createProject("Project A", null, "1.0", null, null, null, true, false);
353+
var componentA = new Component();
354+
componentA.setProject(projectA);
355+
componentA.setName("Component A");
356+
componentA = qm.createComponent(componentA, false);
357+
358+
final Project projectB = qm.createProject("Project B", null, "1.0", null, null, null, true, false);
359+
var componentB = new Component();
360+
componentB.setProject(projectB);
361+
componentB.setName("Component B");
362+
componentB = qm.createComponent(componentB, false);
363+
364+
final NotificationPublisher publisher = createMockPublisher();
365+
366+
final NotificationRule rule = qm.createNotificationRule("Test Rule", NotificationScope.PORTFOLIO, NotificationLevel.INFORMATIONAL, publisher);
367+
rule.setNotifyOn(Set.of(NotificationGroup.NEW_VULNERABILITY));
368+
rule.setProjects(List.of(projectA));
369+
370+
// Set which severities to trigger notification
371+
rule.setNotifySeverities(List.of(Severity.HIGH));
372+
373+
final var notification = new Notification();
374+
notification.setScope(NotificationScope.PORTFOLIO.name());
375+
notification.setGroup(NotificationGroup.NEW_VULNERABILITY.name());
376+
notification.setLevel(NotificationLevel.INFORMATIONAL);
377+
notification.setSubject(new NewVulnerabilityIdentified(null, qm.detach(componentB), Set.of(), null));
378+
379+
final var router = new NotificationRouter();
380+
assertThat(router.resolveRules(PublishContext.from(notification), notification)).isEmpty();
381+
382+
// Set a high vulnerability that should trigger a notification
383+
final Vulnerability highVuln = new Vulnerability();
384+
highVuln.setSeverity(Severity.HIGH);
385+
notification.setSubject(new NewVulnerabilityIdentified(highVuln, qm.detach(componentA), Set.of(), null));
386+
assertThat(router.resolveRules(PublishContext.from(notification), notification))
387+
.satisfiesExactly(resolvedRule -> assertThat(resolvedRule.getName()).isEqualTo("Test Rule"));
388+
}
389+
390+
@Test
391+
void testNewVulnerabilityIdentifiedShouldNotTriggerNotification() {
392+
final Project projectA = qm.createProject("Project A", null, "1.0", null, null, null, true, false);
393+
var componentA = new Component();
394+
componentA.setProject(projectA);
395+
componentA.setName("Component A");
396+
componentA = qm.createComponent(componentA, false);
397+
398+
final Project projectB = qm.createProject("Project B", null, "1.0", null, null, null, true, false);
399+
var componentB = new Component();
400+
componentB.setProject(projectB);
401+
componentB.setName("Component B");
402+
componentB = qm.createComponent(componentB, false);
403+
404+
final NotificationPublisher publisher = createMockPublisher();
405+
406+
final NotificationRule rule = qm.createNotificationRule("Test Rule", NotificationScope.PORTFOLIO, NotificationLevel.INFORMATIONAL, publisher);
407+
rule.setNotifyOn(Set.of(NotificationGroup.NEW_VULNERABILITY));
408+
rule.setProjects(List.of(projectA));
409+
410+
// Set which severities to trigger notification (CRITICAL and HIGH only)
411+
rule.setNotifySeverities(List.of(Severity.CRITICAL, Severity.HIGH));
412+
413+
// Set a low severity that should NOT trigger a notification
414+
final Vulnerability lowVuln = new Vulnerability();
415+
lowVuln.setSeverity(Severity.LOW);
416+
417+
final var notification = new Notification();
418+
notification.setScope(NotificationScope.PORTFOLIO.name());
419+
notification.setGroup(NotificationGroup.NEW_VULNERABILITY.name());
420+
notification.setLevel(NotificationLevel.INFORMATIONAL);
421+
notification.setSubject(new NewVulnerabilityIdentified(lowVuln, qm.detach(componentB), Set.of(), null));
422+
423+
final var router = new NotificationRouter();
424+
425+
// This should not trigger a notification
426+
assertThat(router.resolveRules(PublishContext.from(notification), notification)).isEmpty();
427+
}
428+
349429
@Test
350430
void testNewVulnerabilityIdentifiedLimitedToProject() {
351431
final Project projectA = qm.createProject("Project A", null, "1.0", null, null, null, true, false);
@@ -414,6 +494,95 @@ void testNewVulnerableDependencyLimitedToProject() {
414494
.satisfiesExactly(resolvedRule -> assertThat(resolvedRule.getName()).isEqualTo("Test Rule"));
415495
}
416496

497+
@Test
498+
void testNewVulnerableDependencyThatShouldTriggerNotification() {
499+
final Project projectA = qm.createProject("Project A", null, "1.0", null, null, null, true, false);
500+
var componentA = new Component();
501+
componentA.setProject(projectA);
502+
componentA.setName("Component A");
503+
componentA = qm.createComponent(componentA, false);
504+
505+
final Project projectB = qm.createProject("Project B", null, "1.0", null, null, null, true, false);
506+
var componentB = new Component();
507+
componentB.setProject(projectB);
508+
componentB.setName("Component B");
509+
componentB = qm.createComponent(componentB, false);
510+
511+
final NotificationPublisher publisher = createSlackPublisher();
512+
513+
final NotificationRule rule = qm.createNotificationRule("Test Rule", NotificationScope.PORTFOLIO, NotificationLevel.INFORMATIONAL, publisher);
514+
rule.setNotifyOn(Set.of(NotificationGroup.NEW_VULNERABLE_DEPENDENCY));
515+
rule.setProjects(List.of(projectA));
516+
517+
// Set which severities to trigger notification
518+
rule.setNotifySeverities(List.of(Severity.HIGH));
519+
520+
final var notification = new Notification();
521+
notification.setScope(NotificationScope.PORTFOLIO.name());
522+
notification.setGroup(NotificationGroup.NEW_VULNERABLE_DEPENDENCY.name());
523+
notification.setLevel(NotificationLevel.INFORMATIONAL);
524+
notification.setSubject(new NewVulnerableDependency(qm.detach(componentB), null));
525+
526+
final var router = new NotificationRouter();
527+
assertThat(router.resolveRules(PublishContext.from(notification), notification)).isEmpty();
528+
529+
// Set two high vulnerabilities
530+
final Vulnerability highVuln1 = new Vulnerability();
531+
highVuln1.setSeverity(Severity.HIGH);
532+
final Vulnerability highVuln2 = new Vulnerability();
533+
highVuln2.setSeverity(Severity.HIGH);
534+
List<Vulnerability> vulnerabilities = List.of(highVuln1, highVuln2);
535+
536+
// Set a new vulnerable dependency that should trigger a notification
537+
notification.setSubject(new NewVulnerableDependency(qm.detach(componentA), vulnerabilities));
538+
assertThat(router.resolveRules(PublishContext.from(notification), notification))
539+
.satisfiesExactly(resolvedRule -> assertThat(resolvedRule.getName()).isEqualTo("Test Rule"));
540+
}
541+
542+
@Test
543+
void testNewVulnerableDependencyThatShouldNotTriggerNotification() {
544+
final Project projectA = qm.createProject("Project A", null, "1.0", null, null, null, true, false);
545+
var componentA = new Component();
546+
componentA.setProject(projectA);
547+
componentA.setName("Component A");
548+
componentA = qm.createComponent(componentA, false);
549+
550+
final Project projectB = qm.createProject("Project B", null, "1.0", null, null, null, true, false);
551+
var componentB = new Component();
552+
componentB.setProject(projectB);
553+
componentB.setName("Component B");
554+
componentB = qm.createComponent(componentB, false);
555+
556+
final NotificationPublisher publisher = createSlackPublisher();
557+
558+
final NotificationRule rule = qm.createNotificationRule("Test Rule", NotificationScope.PORTFOLIO, NotificationLevel.INFORMATIONAL, publisher);
559+
rule.setNotifyOn(Set.of(NotificationGroup.NEW_VULNERABLE_DEPENDENCY));
560+
rule.setProjects(List.of(projectA));
561+
562+
// Set which severities to trigger notification
563+
rule.setNotifySeverities(List.of(Severity.HIGH));
564+
565+
final var notification = new Notification();
566+
notification.setScope(NotificationScope.PORTFOLIO.name());
567+
notification.setGroup(NotificationGroup.NEW_VULNERABLE_DEPENDENCY.name());
568+
notification.setLevel(NotificationLevel.INFORMATIONAL);
569+
notification.setSubject(new NewVulnerableDependency(qm.detach(componentB), null));
570+
571+
final var router = new NotificationRouter();
572+
assertThat(router.resolveRules(PublishContext.from(notification), notification)).isEmpty();
573+
574+
// Set two low vulnerabilities
575+
final Vulnerability highVuln1 = new Vulnerability();
576+
highVuln1.setSeverity(Severity.LOW);
577+
final Vulnerability highVuln2 = new Vulnerability();
578+
highVuln2.setSeverity(Severity.LOW);
579+
List<Vulnerability> vulnerabilities = List.of(highVuln1, highVuln2);
580+
581+
// Set a new vulnerable dependency that should trigger a notification
582+
notification.setSubject(new NewVulnerableDependency(qm.detach(componentA), vulnerabilities));
583+
assertThat(router.resolveRules(PublishContext.from(notification), notification)).isEmpty();
584+
}
585+
417586
@Test
418587
void testBomConsumedOrProcessedLimitedToProject() {
419588
final Project projectA = qm.createProject("Project A", null, "1.0", null, null, null, true, false);

0 commit comments

Comments
 (0)