dependency-track icon indicating copy to clipboard operation
dependency-track copied to clipboard

Support for summarized and scheduled notifications

Open MM-msr opened this issue 1 year ago • 16 comments

Description

Currently, Dependency-Track Alerts send a single notification for every single event happening that an alert is subscribed to (e.g. NEW_VULNERABILITY). This can lead to a lot of emails for users and creates the risk of overlooking important notifications.

To improve the user experience of the Alerts, scheduled notifications are implemented in Dependency-Track to send summarized notifications of new events between the last and current scheduled notification. The schedule is defined as cron expression.

Scheduled notification management is available via API and Frontend under Administration -> Notifications -> Scheduled Alerts, which is introduced in the Frontend Pull Request [reference will be added after creation].

The PR includes default templates for console and email publishing. The needed UI-Changes are available in the Frontend-PR at DependencyTrack/frontend#903.

Addressed Issue

#322

Additional Details

  • DEFAULT_SCHEDULED_CRON_EXPRESSION as environment variable for the default cron expression of new rules
  • Error: "The type javax.validation.Payload cannot be resolved. It is indirectly referenced from required type com.github.packageurl.validator.PackageURL" was introduced in my local development environment after syncing the main project and the transition of some javax packages to jakarta
    • was fixed by manually adding javax.validation-api to pom.xml as depencency
  • While evaluating and debugging the consideration of child projects in the notification process, it has turned out that projects delivered by QueryManager.getAllProjects() and some other methods didn't consistently include the correct child references. To workaround this behavior, the notification task "SendScheduledNotificationTask" uses a project-by-project-retrieval approach in "evaluateAffectedProjects(qm, rule)". This worked out empirically to deliver consistent child references.
  • DefaultPublisher-Retrieval was changed from by-class to a combination of name + class (or by-enum on a higher layer) to support multiple default publishers with same publisher class. This change is needed to use the existing publisher classes without defining new ones for scheduled default publishers.

  • There might be an open bug currently in the toJson-Process for PolicyViolation, where the policy is not fully included in the PolicyCondition and thus the Policy.getUUID().toString() call fails with NullRefException. Often it worked for me just fine, sometimes not. Sometimes fixed with creating a new policy, sometimes with reboot of the application. With some additional random exceptions with the H2 database on ORM-Queries, i don't know for sure, if my code is the reason for this behavior or the instability of H2 with the ORM-mapper in my development environment.

Getting violations for scheduled mail:

PolicyQueryManager, L. 340, getPolicyViolations(Project, boolean, ZonedDateTime)

JSONify the data:

NotificationUtil, L. 550ff., toJson(PolicyViolation) -> toJson(pv.getPolicyCondition) -> toJson(pc.getPolicy) -> policy.getUUID().toString() -> [EXCEPTION]

Please test the expected behavior in a more robust environment (docker, other infrastructures, etc.) to validate

Screenshots

Administration_CreateScheduledAlert_Popup Administration_CreateScheduledPublisher_Popup Administration_ScheduledAlerts ScheduledMail_Vulnerabilities

Checklist

  • [x] I have read and understand the contributing guidelines
  • ~This PR fixes a defect, and I have provided tests to verify that the fix is effective~
  • [x] This PR implements an enhancement, and I have provided tests to verify that it works as intended
  • ~This PR introduces changes to the database model, and I have added corresponding [update logic]~(https://github.com/DependencyTrack/dependency-track/tree/master/src/main/java/org/dependencytrack/upgrade)
  • [x] This PR introduces new or alters existing behavior, and I have updated the documentation accordingly

MM-msr avatar Jul 07 '24 15:07 MM-msr

@nscuro After i finally found some time this week to restart the PR process, i cherry-picked all necessary commits from my previous branch, corrected all sign-offs and so on. From the coding side it seems to be fine for now, although additional testing from different perspectives should be done.

Please note in particular the missing javax.validation reference in "Additional details", that seems to be ignored in the current master and seems to be a personal problem of my workspace or an unseen error until now from the transition to jakarta. There may be some more topics to discuss, but the main implementation should be completed by now.

The branch for the PR is based on the same commit my other branches started, but merged with a master newer than this feature branch was created on. Because of this and the recent cherry-picks, the commit history is a bit mixed up with other commits, but that may be gone when the feature branch gets updated with the missing commits from the current master branch.

Please provide me with additional information for the errors of the Static Code Analysis as i cannot see any useful information in the details link. If there is anything else i need to do or you need/want to know, please get in touch with me. I will assist to the best of my abilities.

MM-msr avatar Jul 07 '24 15:07 MM-msr

This looks very interesting. But so does the commit history ;-) Could you rebase it onto current master to make the code changes in the PR "reviewable"?

valentijnscholten avatar Jul 07 '24 21:07 valentijnscholten

@valentijnscholten Yeah, i know :D In my git graph, all looked fine, after the PR i had this 😅 I'll try to rebase it and see, if it's possible without making it worse. After the first PR-attempt went terribly wrong, i'm a bit scared tbh 😆 But i will give it a try and see where i land. Maybe it helps already in the mean time to update the feature branch with the commits it is behind to resolve some "false-included" commits in this PR?

MM-msr avatar Jul 08 '24 08:07 MM-msr

Because of this and the recent cherry-picks, the commit history is a bit mixed up with other commits, but that may be gone when the feature branch gets updated with the missing commits from the current master branch.

Let me push the latest changes from master to that branch.

Please provide me with additional information for the errors of the Static Code Analysis as i cannot see any useful information in the details link.

Unfortunately Codacy requires us to manually enable each "base" branch for analysis. Unless that is done, analysis will fail immediately. I have now enabled the feature branch, so the check should work for the next push.

nscuro avatar Jul 08 '24 09:07 nscuro

Updated the branches for dependency-track and frontend with the latest master. You may need to close and reopen the PR since GitHub doesn't update PRs automatically when the base branch gets updated.

nscuro avatar Jul 08 '24 09:07 nscuro

@valentijnscholten Should be "fixed" by now, thanks to @nscuro :) The update did thankfully what i hoped for.

The current CI test exceptions in BomUploadProcessingTaskTest.java seems to be caused by other stuff, that this PR does not affect, but if i can provide any support, please let me know.

MM-msr avatar Jul 08 '24 13:07 MM-msr

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
:x: -1.44% (target: -1.00%) :x: 37.63% (target: 70.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (a4d5a7b328bd93a7dc32f56efb00d0f60b70b480) 22074 16811 76.16%
Head commit (7ea910c97a36338f0a6eec38b0510971a17fb437) 22993 (+919) 17180 (+369) 74.72% (-1.44%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#3925) 885 333 37.63%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences


:rocket: Don’t miss a bit, follow what’s new on Codacy.

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

codacy-production[bot] avatar Jul 08 '24 14:07 codacy-production[bot]

The current CI test exceptions in BomUploadProcessingTaskTest.java seems to be caused by other stuff, that this PR does not affect, but if i can provide any support, please let me know.

You got lucky and ran into a flaky test 😅 Re-ran the workflow and it completed successfully this time.

nscuro avatar Jul 08 '24 17:07 nscuro

Thanks a lot @nscuro for the initial feedback! I'm currently in the process of writing additional test cases to meet the Code Coverage Quota. I may consider to add some of your suggested changes after that as well or reply to them later this week to discuss them. I think by the end of the week, we know if it's a good time to merge it :) I'll try to get the most things done until then.

MM-msr avatar Jul 22 '24 06:07 MM-msr

Short update from my side: i added some tests and applied your suggested changes for records. Therefore, i created a factory to create these notification subjects with the logic which was previously located in the specific classes. I want to test it a bit more and implement some additional suggested changes before i push it, but i will try to finish my local changes in the next days.

MM-msr avatar Jul 28 '24 20:07 MM-msr

@nscuro Hey there :) Could you trigger a test execution? As i can't really test most of the net-related test cases i want to know if i have to fix sth prior to releasing the PRs from draft state. Thanks in advance!

MM-msr avatar Aug 05 '24 15:08 MM-msr

Is this by any chance another flaky test? :D

MM-msr avatar Aug 07 '24 07:08 MM-msr

Is this by any chance another flaky test? :D

Looks like a hiccup in fetching dependencies. I re-started the workflow.

nscuro avatar Aug 07 '24 09:08 nscuro

I will release the PR for now from the draft state to allow a broader review. I fixed some minor test errors. Sadly, i can't really get a grip on the other major errors:

  • the 500 error codes seem like a server error and may not be related to the code itself...but that's only a guessing
  • the publisher tests were created like the others, but i can't really interpret the test error message

I can look into it with assistance, but if a developer with more experience in java testing and especially this project might take a look, i would really appreciate it. Maybe it's a hiccup from the test env, as e.g. the invalid cron test (ScheduledNotificationRuleResourceTest.java, L. 182ff.) returns the expected string in my browser, but as it seems not in the github tests.

MM-msr avatar Aug 12 '24 16:08 MM-msr

@nscuro can you give feedback if you need further changes and/or by when you will have time to look at this?

rkg-mm avatar Aug 31 '24 17:08 rkg-mm

@rkg-mm I'll try to complete a review this week.

nscuro avatar Sep 02 '24 09:09 nscuro

@nscuro could you please update the feature branch to the latest master? our branch is updated to the master branch which results in a lot of changes now :D Same for the frontend please

rkg-mm avatar Oct 25 '24 19:10 rkg-mm

@rkg-mm Done. The GitHub diff of both PRs won't update though until you push a new commit.

nscuro avatar Oct 25 '24 19:10 nscuro

The previously failing unit tests for this PR should now all be fixed. @nscuro could you please re-approve the pending GitHub workflows for the latest commit?

rbt-mm avatar Nov 12 '24 14:11 rbt-mm

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
:white_check_mark: -0.65% (target: -1.00%) :x: 62.93% (target: 70.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (dd9bb4da37949a7a56eccc300d0b2a40c83b4152) 22519 17829 79.17%
Head commit (27b85df000deb28c41f4e119ed174a26fb348a08) 23404 (+885) 18377 (+548) 78.52% (-0.65%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#3925) 920 579 62.93%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

codacy-production[bot] avatar Nov 19 '24 16:11 codacy-production[bot]

Oops, totally forgot that the workflows still require a maintainer approval for this PR 😅.

I think the remaining Codacy code coverage issues should be resolved by the new commits. @nscuro could you please approve the workflows for us again?

rbt-mm avatar Jan 27 '25 13:01 rbt-mm

@rkg-mm @rbt-mm @MM-msr First let me say sorry for dragging this for so long. All your work is truly appreciated and I hate that this has been open for so long.

I have updated the target branch with the latest master, and also raised a PR to bringt your branch up to date: https://github.com/MM-msr/dependency-track/pull/2

There are a few changes I'd like to see here and with respect to everyone's time and resources I believe it's best if I simply propose the changes in a PR into your branch, which you can then review whether it still meets your needs.

Things I'd like to change:

  1. Reduce the duplication in code and database tables that is introduced by treating ScheduledNotificationRules and NotificationRules as separate types (see also my previous comment here).
  2. Generation of notification subjects must work for large(r) portfolios. At the moment we're loading all findings since the last submission at once, we do repetitive queries to load projects for every single finding, and so on. I just spent a lot of time to nuke such patterns from legacy code and really don't want to introduce them again in another place.
  3. I'm trying to see if we can avoid introducing a separate scheduling mechanism that bypasses the existing task/eventing mechanism. Of course while still allowing users to change Cron expressions at runtime.

Separately, I was wondering whether we should even allow users to configure scheduled notifications without limiting them to projects. At the moment, limiting to projects is optional. If users were to create such notifications for their entire portfolio, I'm seriously struggling to see how all involved systems could possibly handle the size of the notification content. It feels like a footgun to me. WDYT?

nscuro avatar Mar 02 '25 16:03 nscuro

Thank you very much, we will check the PR tomorrow asap.

Separately, I was wondering whether we should even allow users to configure scheduled notifications without limiting them to projects. At the moment, limiting to projects is optional. If users were to create such notifications for their entire portfolio, I'm seriously struggling to see how all involved systems could possibly handle the size of the notification content. It feels like a footgun to me. WDYT?

That's true, this might cause some trouble,especially as you first configure everything else and THEN limit to projects, could cause some timeframe in which you accidentally trigger this without scope. Enforce limiting probably makes sense.

rkg-mm avatar Mar 03 '25 18:03 rkg-mm

we will check the PR tomorrow asap.

Thanks. To be clear, this initial PR is solely to bring the code up-to-date. I will raise follow-up PRs after that so the changes are easier to see.

Enforce limiting probably makes sense.

Cool. Will look into adding that.

nscuro avatar Mar 03 '25 18:03 nscuro

Thanks. To be clear, this initial PR is solely to bring the code up-to-date. I will raise follow-up PRs after that so the changes are easier to see.

sorry, misunderstood - I merged this one directly.

rkg-mm avatar Mar 03 '25 18:03 rkg-mm

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
:white_check_mark: -0.64% (target: -1.00%) :x: 62.84% (target: 70.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (370fb14b5b86352286af0f5b087c0883e6ab41e9) 23239 18662 80.30%
Head commit (e51d2f16125f76c49c9a594506aa73056b4c47c7) 24124 (+885) 19218 (+556) 79.66% (-0.64%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#3925) 923 580 62.84%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

codacy-production[bot] avatar Mar 03 '25 18:03 codacy-production[bot]

Working on my changes here: https://github.com/MM-msr/dependency-track/pull/3

nscuro avatar Mar 08 '25 19:03 nscuro

Admittedly I struggled with finding the time to complete this but it's finally done, you can find the final result, including a high-level description of how this PR evolved here: #4783.

nscuro avatar Apr 01 '25 20:04 nscuro

Superseded by #4783

nscuro avatar Apr 03 '25 19:04 nscuro