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(api-notif): Fix order of validators #10533

Merged
merged 1 commit into from
Jul 12, 2024

Conversation

kiblik
Copy link
Contributor

@kiblik kiblik commented Jul 8, 2024

During the implementation of #7311, I noticed specific edge cases that the implemented validate function did not handle correctly.

If there is an existing System Notification (user=None, product=None, template=false), it is not possible to add a Notification Template. It failed with Notification for user and product already exists even though it wasn't true. So I add a template flag to the checking query. However, I also needed to reorder checkers because "existing template" would fail with Notification for user and product already exists so a reorder was needed.

Copy link

dryrunsecurity bot commented Jul 8, 2024

Hi there 👋, @DryRunSecurity here, below is a summary of our analysis and findings.

DryRun Security Status Findings
Server-Side Request Forgery Analyzer 0 findings
Configured Codepaths Analyzer 0 findings
IDOR Analyzer 0 findings
Sensitive Files Analyzer 0 findings
SQL Injection Analyzer 0 findings
Authn/Authz Analyzer 1 finding
Secrets Analyzer 0 findings

Note

🟢 Risk threshold not exceeded.

Change Summary (click to expand)

The following is a summary of changes in this pull request made by me, your security buddy 🤖. Note that this summary is auto-generated and not meant to be a definitive list of security issues but rather a helpful summary from a security perspective.

Summary:

The code changes in this pull request are focused on improving the validation process and data management for the Notifications model in the Defect Dojo application. The key changes include ensuring that only one notification template can exist at a time, and that a notification for a user and product combination cannot be created if one already exists. These changes help to maintain data integrity and prevent the creation of duplicate notifications, which could lead to potential security issues if not properly managed.

Additionally, the ability to create notification templates is introduced, which can be a useful feature for centrally managing notification settings. However, it is important to ensure that the template creation process is properly secured and that only authorized users can create or modify templates.

The changes also include updates to the unit tests for the API v2 notifications functionality, which are focused on improving the testing process and providing more detailed information in case of test failures. While these changes do not directly introduce any security concerns, it is important to consider the overall security aspects of the application, such as authentication, authorization, input validation, error handling, and data protection.

Files Changed:

  1. dojo/api_v2/serializers.py:

    • The validate method in the NotificationsSerializer has been updated to ensure that only one notification template can exist at a time and that a notification for a user and product combination cannot be created if one already exists.
    • The template field has been added to the NotificationsSerializer, allowing the creation of notification templates.
  2. unittests/test_apiv2_notifications.py:

    • The setUp method is modified to include the r.data in the assertion for the HTTP status code, providing more detailed information in case the assertion fails.
    • No other significant changes are made in the provided patch.

Powered by DryRun Security

@mtesauro
Copy link
Contributor

mtesauro commented Jul 8, 2024

Closing and re-opening to try to make Flake8 test happy

@mtesauro mtesauro closed this Jul 8, 2024
@mtesauro mtesauro reopened this Jul 8, 2024
@mtesauro
Copy link
Contributor

mtesauro commented Jul 8, 2024

@kiblik Not sure what's up with flake8-your-pr since we have a PR that Maffooch did in bugfix to remove the need for that test now that we have ruff running.

Anyway, don't worry about making it happy for this PR. We'll sort out why in other PRs 👍

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

@mtesauro mtesauro merged commit 60ddc18 into DefectDojo:bugfix Jul 12, 2024
236 checks passed
@kiblik kiblik deleted the api_notif_validators branch July 12, 2024 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants