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

Fix Webhook notification test #11629

Merged
merged 1 commit into from
Jan 24, 2025
Merged

Conversation

cneill
Copy link
Contributor

@cneill cneill commented Jan 23, 2025

Description

Adding a Webhook notification was producing this error:

image

It appears that the get_webhook_manager_instance method was intended to return a WebhookNotificationManger but maybe there was a typo. With this fix, the test "ping" runs successfully again.

Screenshot 2025-01-23 at 13 01 04
Screenshot 2025-01-23 at 13 00 07

Ruff also auto-fixed some code styling in this file...

Copy link

dryrunsecurity bot commented Jan 23, 2025

DryRun Security Summary

The pull request enhances the Notification Webhooks functionality by updating import statements, improving error handling, adding webhook deactivation, and refining the webhook testing method to ensure better reliability and user communication.

Expand for full summary

Summary:

The code changes in this pull request are focused on improving the Notification Webhooks functionality in the application. The key changes include updating the import statements, modifying the get_webhook_manager_instance() method to return the correct class instance, renaming and calling the _test_webhooks_notification() method, and adding new functionality to deactivate a webhook. Additionally, the error handling in the process_form() method has been improved to better communicate issues to the user when the test_webhooks_notification() method raises an exception.

From a security perspective, the changes seem to be primarily focused on enhancing the functionality and reliability of the Notification Webhooks feature. The improved error handling and the addition of the _test_webhooks_notification() method help to ensure that webhook notifications are properly validated and that any issues are communicated to the user. However, the presence of TODOs related to user permissions and access control suggests that there might be room for improvement in this area, and it's important to ensure that the application's access control mechanisms are robust and secure.

Files Changed:

  • dojo/notifications/views.py: This file contains the changes related to the Notification Webhooks functionality. The key changes include:
    1. Updating the import statements to use the WebhookNotificationManger class from the dojo.notifications.helper module.
    2. Modifying the get_webhook_manager_instance() method to return an instance of WebhookNotificationManger instead of Notification_Webhooks.
    3. Renaming the test_webhooks_notification() method to _test_webhooks_notification() and calling it from the process_form() method in the AddNotificationWebhooksView and EditNotificationWebhooksView classes.
    4. Adding new functionality to the EditNotificationWebhooksView class to deactivate a webhook, including updating the webhook's status, first_error, last_error, and note fields.
    5. Improving the error handling in the process_form() method of the AddNotificationWebhooksView and EditNotificationWebhooksView classes to display an error message to the user if the test_webhooks_notification() method raises a requests.exceptions.RequestException.

Code Analysis

We ran 9 analyzers against 1 file and 0 analyzers had findings. 9 analyzers had no findings.

View PR in the DryRun Dashboard.

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

@Maffooch Maffooch merged commit 0e5cbe3 into DefectDojo:bugfix Jan 24, 2025
72 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants