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.

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

Notifications: Convert to classes #11296

Merged
merged 13 commits into from
Dec 18, 2024
Merged

Notifications: Convert to classes #11296

merged 13 commits into from
Dec 18, 2024

Conversation

Maffooch
Copy link
Contributor

@Maffooch Maffooch commented Nov 19, 2024

This has been a long time coming 😄

[sc-8894]

Copy link

dryrunsecurity bot commented Nov 19, 2024

DryRun Security Summary

The pull request enhances the DefectDojo application's notification system by introducing a centralized NotificationManager class with improved security, performance, and customization features across multiple files.

Expand for full summary

Summary:

The code changes in this pull request are focused on improving the notification functionality in the DefectDojo application, which is an open-source application security management tool. The changes span across several files, including the default_importer.py, default_reimporter.py, engagement/views.py, base_importer.py, notifications/views.py, test_notifications.py, and notifications/helper.py.

The key security-related highlights from the changes are:

  1. Improved Notification System: The code introduces a new NotificationManager class that centralizes the notification logic, handling various types of notifications (e.g., Slack, Microsoft Teams, email, webhooks, alerts) and providing customization options for the notification content.

  2. Webhook Management: The code includes robust functionality for managing the status of webhook endpoints, including automatic reactivation of temporarily inactive webhooks. This helps ensure the reliability and delivery of notifications through webhooks.

  3. User Authorization: The notification system considers user permissions when determining which users should receive notifications, reducing the risk of information disclosure.

  4. Error Handling and Logging: The code includes comprehensive error handling and logging mechanisms, which can aid in troubleshooting and monitoring potential security issues related to the notification system.

  5. Asynchronous Notifications: The use of Celery tasks to send notifications asynchronously helps improve the overall performance of the application, which is an important consideration for security tools.

  6. Content Rendering: The code uses Django's template engine to render the notification messages, ensuring that the content is properly formatted and sanitized, reducing the risk of potential security vulnerabilities, such as cross-site scripting (XSS) attacks.

Overall, the changes in this pull request demonstrate a focus on enhancing the security monitoring and alerting capabilities of the DefectDojo application, which is a crucial aspect of an effective application security management tool.

Files Changed:

  1. dojo/importers/default_importer.py: Improvements to the notification functionality, including more detailed information in the create_notification calls.
  2. dojo/importers/default_reimporter.py: Refactoring of the notification system, removing the direct import of the dojo.notifications.helper module.
  3. dojo/engagement/views.py: Replacement of the notify_test_created() function call with a more detailed create_notification() call, providing more context about the new test.
  4. dojo/importers/base_importer.py: Introduction of new functions related to the notification system, asynchronous processing, endpoint management, vulnerability ID management, file management, and finding mitigation.
  5. dojo/notifications/views.py: Refactoring of the NotificationWebhooksView class and improvements to the process_form() methods in the AddNotificationWebhooksView and EditNotificationWebhooksView classes.
  6. unittests/test_notifications.py: Introduction of the NotificationManager class and updates to the notification logic, including handling of user-level and system-level notification settings, and improvements to the webhook functionality.
  7. dojo/notifications/helper.py: Implementation of the NotificationManager class and its associated manager classes for different notification types, as well as the create_notification function and other supporting functionality.

Code Analysis

We ran 9 analyzers against 7 files and 1 analyzer had findings. 8 analyzers had no findings.

Analyzer Findings
Authn/Authz Analyzer 1 finding

View PR in the DryRun Dashboard.

@Maffooch Maffooch marked this pull request as ready for review November 28, 2024 07:47
Copy link
Contributor

@mtesauro mtesauro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

Copy link
Contributor

github-actions bot commented Dec 2, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

@cneill cneill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple small things

dojo/notifications/helper.py Show resolved Hide resolved
dojo/notifications/helper.py Outdated Show resolved Hide resolved
dojo/notifications/helper.py Outdated Show resolved Hide resolved
dojo/notifications/helper.py Outdated Show resolved Hide resolved
dojo/notifications/helper.py Outdated Show resolved Hide resolved
logger.debug("creating personal notifications for event: %s", event)
# There are notification like deleting a product type that shall not be sent to users.
# These notifications will have the parameter no_users=True
if kwargs.get("no_users") is False:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing no_users=None ~= no_users=False?

Suggested change
if kwargs.get("no_users") is False:
if not kwargs.get("no_users"):

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will need to do more testing on this one. There is unit test coverage here for the way that it is

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remembered that in most cases, this value is set to False in the call here - this where all notifications are created from within the app. The only exception is calling some of these classes directly within unit tests

To address your comment, I went with this approach: if kwargs.get("no_users", False) is False

Copy link
Contributor

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

@mtesauro mtesauro merged commit 2f79dc6 into DefectDojo:bugfix Dec 18, 2024
73 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants