fix(aci): prevent deletion of system-created monitors in API #103843
Conversation
github-actions
bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| # Filter out system-created detectors from deletion | ||
| deletable_detectors = [ | ||
| detector for detector in queryset if not is_system_created_detector(detector) | ||
| ] |
There was a problem hiding this comment.
Bug: Permission check occurs before filtering system-created detectors
The permission check via can_edit_detectors happens on the full queryset before filtering out system-created detectors. This means if a user's query matches both user-created and system-created detectors, they need org:write permission even though system-created detectors won't actually be deleted. Users with only alerts:write permission are incorrectly denied from deleting their own user-created detectors when system-created detectors are in the queryset. The permission check needs to happen after filtering to deletable_detectors.
| ) | ||
|
|
||
| # Check if the user has edit permissions for all detectors | ||
| if not can_edit_detectors(queryset, request): |
There was a problem hiding this comment.
I wonder if we should have a can_delete_detectors function for this? We should also lock down the DELETE /detectors/<id>/ endpoint.
I think for this API it would be better if we rejected if any system-created detectors are included in the queryset. That way the results are more consistent (since we already reject if you don't have edit permissions for some of the detectors)
tests/sentry/workflow_engine/endpoints/test_organization_detector_details.py
Show resolved
Hide resolved
| def can_delete_detector(detector: Detector, request: Request) -> bool: | ||
| """ | ||
| Determine if the requesting user has access to delete the given detector. | ||
| Only user-created detectors can be deleted, and require "alerts:write" permission. | ||
| """ | ||
| if is_system_created_detector(detector): | ||
| return False | ||
|
|
||
| return can_edit_user_created_detectors(request, detector.project) |
There was a problem hiding this comment.
Instead of having 2 functions that do the same thing, you could pass a single detector to can_delete_detectors as a list.
There was a problem hiding this comment.
the single detector function is nice because it saves us a lookup from the Project table
ameliahsu
deleted the
mia/aci/be-prevent-deleting-system-created-monitors
branch
system-created monitors should not be deleted by anyone, regardless of permissions
backend fix to go with #103838
closes https://linear.app/getsentry/issue/NEW-646/prevent-api-from-deleting-error-monitor-detectors-via-bulk-edit