-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Conversation
Hi there 👋, @DryRunSecurity here, below is a summary of our analysis and 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:
Powered by DryRun Security |
Closing and re-opening to try to make Flake8 test happy |
@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 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved
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 atemplate
flag to the checking query. However, I also needed to reorder checkers because "existing template" would fail withNotification for user and product already exists
so a reorder was needed.