Consolidation of notification creation #8824

kiblik · 2023-10-13T08:34:25Z

Purpose of this PR: If some action triggers the creation of a notification during clicking in UI. The same notification should be triggered also if the same happens via API. Until now many actions have been avoiding these notifications. Implementation is done by Django Signals.

Some notes:

General rule: if the object is removed in cascade (e.g. removal product triggers removal of all engagements, tests, ...) notification is created only for the highest removed element (to avoid spamming).
The only exception to this ^ general rule is the removal of the Product. If Product Type is removed, notification is triggered for all Products as well
PR hasn't announced any new notifications. All of them have been created based on existing implementation.
There is a set of notifications which hasn't been migrated and they are performed only for UI actions because they would generate useless noise. E.g. "creation of findings" vs "creation of findings during scan imports". But "TODO" comments have been added.
If AuditLog is enabled, the message might contain also the actor's name. If the user disabled Auditlog, the user will be notified without knowing who did it. This is a small step back compared to the previous implementation.
Most of the actions have been with the label event="other". Now notification engine accepts any event name as meaningful and for Alerts event is translated to source name. If event type does not exist as the field in Notification, the engine works with the event as with other

github-actions · 2023-11-07T16:32:42Z

python: can't open file '/home/runner/work/django-DefectDojo/django-DefectDojo/.github/scripts/git_protect.py': [Errno 2] No such file or directory

github-actions · 2023-11-07T16:33:56Z

The following files are protected and cannot be modified:
dojo/product_type/views.py
dojo/engagement/views.py
dojo/product/views.py
dojo/test/views.py
dojo/notifications/helper.py
dojo/finding/views.py
unittests/test_notifications.py
dojo/utils.py
dojo/endpoint/views.py

github-actions · 2023-11-07T17:10:06Z

The following files are protected and cannot be modified:
dojo/endpoint/views.py
dojo/engagement/views.py
dojo/product_type/views.py
dojo/utils.py
unittests/test_notifications.py
dojo/product/views.py
dojo/notifications/helper.py
dojo/test/views.py
dojo/finding/views.py

github-actions · 2023-11-08T13:35:19Z

The following files are protected and cannot be modified:
dojo/finding/views.py
dojo/utils.py
unittests/test_rest_framework.py
dojo/test/views.py
dojo/engagement/views.py
dojo/product_type/views.py
dojo/notifications/helper.py
dojo/endpoint/views.py
unittests/test_notifications.py
dojo/product/views.py

github-actions · 2023-11-08T14:49:37Z

The following files are protected and cannot be modified:
dojo/finding/views.py
unittests/test_rest_framework.py
dojo/utils.py
dojo/product_type/views.py
dojo/engagement/views.py
dojo/endpoint/views.py
dojo/test/views.py
dojo/product/views.py
dojo/notifications/helper.py
.github/workflows/check-protected-files.yml
requirements.txt
unittests/test_notifications.py

github-actions · 2023-11-08T15:00:55Z

The following files are protected and cannot be modified:
dojo/engagement/views.py
dojo/notifications/helper.py
dojo/product/views.py
dojo/utils.py
dojo/product_type/views.py
requirements.txt
dojo/endpoint/views.py
dojo/finding/views.py
.github/workflows/check-protected-files.yml
dojo/test/views.py
unittests/test_notifications.py
unittests/test_rest_framework.py

github-actions · 2023-11-08T17:02:02Z

The following files are protected and cannot be modified:
dojo/utils.py
dojo/endpoint/views.py
unittests/test_notifications.py
dojo/product/views.py
unittests/test_rest_framework.py
dojo/product_type/views.py
dojo/notifications/helper.py
requirements.txt
dojo/engagement/views.py
.github/workflows/check-protected-files.yml
dojo/finding/views.py
dojo/test/views.py

github-actions · 2023-11-08T17:03:01Z

The following files are protected and cannot be modified:
unittests/test_notifications.py
dojo/product_type/views.py
dojo/engagement/views.py
unittests/test_rest_framework.py
dojo/product/views.py
dojo/notifications/helper.py
dojo/utils.py
dojo/test/views.py
dojo/endpoint/views.py
dojo/finding/views.py

github-actions · 2023-11-08T17:33:50Z

The following files are protected and cannot be modified:
dojo/endpoint/views.py
unittests/test_notifications.py
dojo/product/views.py
dojo/test/views.py
dojo/engagement/views.py
dojo/notifications/helper.py
dojo/finding/views.py
dojo/product_type/views.py
dojo/utils.py
dojo/endpoint/receivers.py
unittests/test_rest_framework.py

Signed-off-by: kiblik <[email protected]>

github-actions · 2024-05-03T06:43:32Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

cneill

Most of the issues I found last time have been fixed! Thanks for digging into this. I think I found the source of the issue with endpoint deletions not triggering alerts: bulk delete (see video). I'm not sure whether that deserves its own PR or if it would be in scope for this one, I'll leave that up to you.

cneill · 2024-05-16T16:48:27Z

dojo/endpoint/signals.py

+            description = _('The endpoint "%(name)s" was deleted by %(user)s') % {
+                                'name': str(instance), 'user': le.actor}


I just checked this again and it looks like the actor is being populated successfully now when the alert is created (API and UI)!

However, I think I may have found the source of the missing notifications that I mentioned last time - using bulk delete on the Endpoints page does not trigger an alert.

Screen.Recording.2024-05-16.at.11.42.58.mov

cneill · 2024-05-16T16:51:01Z

dojo/test/signals.py


-logger = logging.getLogger(__name__)
+@receiver(post_delete, sender=Test)
+def test_post_delete(sender, instance, using, origin, **kwargs):


This remains true - no Test deletion alerts when deleting from either the UI or API

dojo/engagement/signals.py

cneill · 2024-05-16T16:58:58Z

dojo/product/signals.py

+    if settings.ENABLE_AUDITLOG:
+        le = LogEntry.objects.get(
+                action=LogEntry.Action.DELETE,
+                content_type=ContentType.objects.get(app_label='dojo', model='product'),
+                object_id=instance.id


This is working with both API and UI now 👍

kiblik · 2024-05-17T08:41:54Z

Most of the issues I found last time have been fixed! Thanks for digging into this. I think I found the source of the issue with endpoint deletions not triggering alerts: bulk delete (see video). I'm not sure whether that deserves its own PR or if it would be in scope for this one, I'll leave that up to you.

Thank you for your feedback and for testing again.

Bulk removal of endpoint: It hasn't been implemented before this PR. Only single removal was implemented. So we would not be speaking only about fixing a bug but implementing the new feature. I would prefer to implement it in the new PR because smaller changes are easier to review and this PR is focused only on the migration of the notification processor.
- If you are interested in the reason why it is not working now: post_delete signals are sending origin to inform you who triggered deletion. In case of removal of one endpoint (ep = Endpoint.objects.get(id=pk).delete()) origin is the endpoint itself. In case of bulk removal (eps = Endpoint.objects.filter(id__in=[pk1, pk2, pk3]).delete()), origin is QuerySet. This will need some special handling. By the way, Findings support bulk removal so they will need this handling as well.
Removal of test: This has been implemented before however I found a bug in the implementation. So this kind of notification was broken before this PR.
- dojo/test/views.py:delete_test sends in create_notification list of recipients as users (as Django models)
  
  django-DefectDojo/dojo/test/views.py
  
  Line 342 in 2234848
  
  recipients=[test.engagement.lead],
  
  but dojo/notifications/helper.py:create_notification expects usernames (as strings)
  
  django-DefectDojo/dojo/notifications/helper.py
  
  Line 33 in 2234848
  
  for recipient_notifications in Notifications.objects.filter(user__username__in=kwargs['recipients'], user__is_active=True, product=None):
  
  So same as Endpoints, I would prefer fix this bug in separated PR. Sidenote: I noticed the same bug in some other places as well. I will fix them there as well. Interesting that nobody complained :).

If everybody agrees, I hope this PR is ready for merge.

cneill

Thanks for the thorough explanation! I agree that it would be better to address the work required for those additional fixes in one or more follow-on PRs 👍

github-actions bot added the unittests label Oct 13, 2023

kiblik force-pushed the notification_api_avoiders branch from da741c1 to 3c0ad81 Compare October 13, 2023 08:36

kiblik marked this pull request as draft October 13, 2023 09:08

kiblik force-pushed the notification_api_avoiders branch 3 times, most recently from 2782354 to b08896c Compare October 14, 2023 19:18

kiblik force-pushed the notification_api_avoiders branch from 3efebbc to a210f12 Compare October 23, 2023 19:12

kiblik force-pushed the notification_api_avoiders branch from a210f12 to 10ef9e8 Compare October 31, 2023 09:46

kiblik force-pushed the notification_api_avoiders branch from 31156ef to 931cee7 Compare November 7, 2023 16:33

kiblik force-pushed the notification_api_avoiders branch from 931cee7 to 877a5a7 Compare November 7, 2023 17:09

kiblik force-pushed the notification_api_avoiders branch from 73cdc7f to f77d4ab Compare November 8, 2023 14:49

kiblik force-pushed the notification_api_avoiders branch from f77d4ab to d732f5e Compare November 8, 2023 14:57

kiblik force-pushed the notification_api_avoiders branch from 334ae53 to 3e2ddc0 Compare November 8, 2023 17:02

kiblik force-pushed the notification_api_avoiders branch 4 times, most recently from 556c299 to 3ec4cb3 Compare November 8, 2023 20:24

github-actions bot added the parser label Nov 8, 2023

kiblik marked this pull request as ready for review November 8, 2023 21:50

kiblik force-pushed the notification_api_avoiders branch from e5f5171 to a5e8119 Compare November 9, 2023 21:44

kiblik mentioned this pull request Nov 15, 2023

Fix incorrect implementation of auditlog managment #9002

Merged

kiblik and others added 18 commits May 3, 2024 08:42

process finding_group

e78b95d

Fixes

89dc696

Add test_auditlog_on_off

e675c14

unittests: move auditloging

0219f66

event name consolidation

51aab78

Fix renamed engagement

233dc54

Fix testing auditlog

cb709f3

Try exclude test_auditlog_on_off

79adb89

Change get_system_setting('enable_auditlog') to settings.ENABLE_AUDITLOG

d24ebd4

Change TODOs

7f40fbd

JIRA: more specific event names

139820a

Fix Ruff

437dc46

Fix tests

253b5e4

Unittest: Set username rendering

3526028

Signed-off-by: kiblik <[email protected]>

Eng: Do not process "Not Started" -> "In Progress"

9f61aa6

Signed-off-by: kiblik <[email protected]>

Fix Eng ruff

f6b63f9

Fix TokenAuthentication

26a06d2

localization

8524d92

kiblik force-pushed the notification_api_avoiders branch from 2b4a518 to 8524d92 Compare May 3, 2024 06:43

github-actions bot removed the conflicts-detected label May 3, 2024

kiblik closed this May 3, 2024

kiblik reopened this May 3, 2024

cneill reviewed May 16, 2024

View reviewed changes

cneill approved these changes May 17, 2024

View reviewed changes

mtesauro merged commit 3273c68 into DefectDojo:dev May 18, 2024
237 of 238 checks passed

kiblik deleted the notification_api_avoiders branch May 18, 2024 07:03

This was referenced May 21, 2024

Ruff: Add FIX001 and FIX003 #10240

Merged

fix(integrity check): update hash + handle collectstatic #10241

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidation of notification creation #8824

Consolidation of notification creation #8824

kiblik commented Oct 13, 2023 •

edited by Maffooch

Loading

github-actions bot commented Nov 7, 2023

github-actions bot commented Nov 7, 2023

github-actions bot commented Nov 7, 2023

github-actions bot commented Nov 8, 2023

github-actions bot commented Nov 8, 2023

github-actions bot commented Nov 8, 2023

github-actions bot commented Nov 8, 2023

github-actions bot commented Nov 8, 2023

github-actions bot commented Nov 8, 2023

github-actions bot commented May 3, 2024

cneill left a comment

cneill May 16, 2024

cneill May 16, 2024

cneill May 16, 2024

kiblik commented May 17, 2024

cneill left a comment

		description = _('The endpoint "%(name)s" was deleted by %(user)s') % {
		'name': str(instance), 'user': le.actor}

Consolidation of notification creation #8824

Consolidation of notification creation #8824

Conversation

kiblik commented Oct 13, 2023 • edited by Maffooch Loading

github-actions bot commented Nov 7, 2023

github-actions bot commented Nov 7, 2023

github-actions bot commented Nov 7, 2023

github-actions bot commented Nov 8, 2023

github-actions bot commented Nov 8, 2023

github-actions bot commented Nov 8, 2023

github-actions bot commented Nov 8, 2023

github-actions bot commented Nov 8, 2023

github-actions bot commented Nov 8, 2023

github-actions bot commented May 3, 2024

cneill left a comment

Choose a reason for hiding this comment

cneill May 16, 2024

Choose a reason for hiding this comment

cneill May 16, 2024

Choose a reason for hiding this comment

cneill May 16, 2024

Choose a reason for hiding this comment

kiblik commented May 17, 2024

cneill left a comment

Choose a reason for hiding this comment

kiblik commented Oct 13, 2023 •

edited by Maffooch

Loading