fix(api): refuse a new contact assignment to an inactive user - #15879
Open
svader0 wants to merge 5 commits into
Open
fix(api): refuse a new contact assignment to an inactive user#15879svader0 wants to merge 5 commits into
svader0 wants to merge 5 commits into
Conversation
The products endpoint accepted an inactive user on product_manager, technical_contact and team_manager, while the assets endpoint filtered only asset_managers and left the other two open. The web UI has always refused all three, so the API was the inconsistent side. The guard lives on run_validation so both Pro subclasses inherit it, and it no-ops when the submitted user already holds that field. A full PUT and the Vue form's PATCH both echo every current value back, so rejecting the echo would delete the historical reference the rule exists to protect. The asset_managers queryset restriction is removed for the same reason: it raised Invalid pk on a replay, before any guard could allow it.
The dropdowns exclude inactive users, but nothing widened the queryset for the instance's own value, so opening and saving an untouched asset wrote None over a contact who had since been deactivated. prod_type on the line above already handles this the same way.
Also point the notification use case at the Rules Engine 2 email node, which sends to any address and needs no DefectDojo account.
…e guard tests The coverage test needs no request or fixture, so it extends DojoTestCase. The form test's comment no longer names a downstream product.
|
This pull request contains a critical finding where a sensitive authorization file was modified by an unauthorized author.
🔴 Configured Sensitive Codepath Modified by Non-Allowed Author in
|
| Vulnerability | Configured Sensitive Codepath Modified by Non-Allowed Author |
|---|---|
| Description | File 'dojo/authorization/serializer_guards.py' matches configured sensitive codepath pattern 'dojo/authorization/*.py' and was modified by 'svader0' (commit 4d8d859) who is not in the allowed authors list. |
We've notified @mtesauro.
Comment to provide feedback on these findings.
Report false positive: @dryrunsecurity fp [FINDING ID] [FEEDBACK]
Report low-impact: @dryrunsecurity nit [FINDING ID] [FEEDBACK]
Example: @dryrunsecurity fp drs_90eda195 This code is not user-facing
All finding details can be found in the DryRun Security Dashboard.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The products API accepted an inactive user as
product_manager,technical_contactorteam_manager. The assets API filtered onlyasset_managers. The web UI refused all three. This PR addsActiveUserContactGuardMixinto the product and asset serializers, so a new assignment to an inactive user gets a validation error on that field.A value already stored on the row is still accepted. A PUT or PATCH that echoes the current contacts keeps working, and an existing assignment stays in place when the user is deactivated later. Clearing a contact is still allowed. The
asset_managersqueryset restriction is removed so a replayed value no longer fails as an invalid pk.The legacy edit form now keeps the row's own inactive contact selectable, so saving an untouched form no longer writes
Noneover it.Test results
unittests/test_inactive_user_contact_assignment.pycovers active and inactive assignment on both endpoints for create and patch, replay of an existing inactive contact, clearing a contact, deactivation after assignment, an unrelated edit, the legacy form, and a sweep that fails when a serializer writes a contact field without the guard.Documentation
The asset modelling pages now state that personnel fields take active users only and that existing assignments stay readable. They point the notification use case at a Rules Engine 2 Email node, which sends to any address.
Checklist
bugfix.