Skip to content

Commit e1e8ef4

Browse files
authored
fix(aci): prevent deletion of system-created monitors in API (#103843)
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
1 parent 3e73c85 commit e1e8ef4

File tree

5 files changed

+96
-6
lines changed

5 files changed

+96
-6
lines changed

src/sentry/workflow_engine/endpoints/organization_detector_details.py

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,6 @@
2121
)
2222
from sentry.apidocs.parameters import DetectorParams, GlobalParams
2323
from sentry.db.postgres.transactions import in_test_hide_transaction_boundary
24-
from sentry.grouping.grouptype import ErrorGroupType
2524
from sentry.incidents.grouptype import MetricIssue
2625
from sentry.incidents.metric_issue_detector import schedule_update_project_config
2726
from sentry.issues import grouptype
@@ -31,6 +30,7 @@
3130
from sentry.workflow_engine.endpoints.serializers.detector_serializer import DetectorSerializer
3231
from sentry.workflow_engine.endpoints.validators.detector_workflow import (
3332
BulkDetectorWorkflowsValidator,
33+
can_delete_detector,
3434
can_edit_detector,
3535
)
3636
from sentry.workflow_engine.endpoints.validators.utils import get_unknown_detector_type_error
@@ -193,12 +193,9 @@ def delete(self, request: Request, organization: Organization, detector: Detecto
193193
"""
194194
Delete a detector
195195
"""
196-
if not can_edit_detector(detector, request):
196+
if not can_delete_detector(detector, request):
197197
raise PermissionDenied
198198

199-
if detector.type == ErrorGroupType.slug:
200-
return Response(status=403)
201-
202199
validator = get_detector_validator(
203200
request, detector.project, detector.type, instance=detector
204201
)

src/sentry/workflow_engine/endpoints/organization_detector_index.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@
5050
from sentry.workflow_engine.endpoints.validators.base import BaseDetectorTypeValidator
5151
from sentry.workflow_engine.endpoints.validators.detector_workflow import (
5252
BulkDetectorWorkflowsValidator,
53+
can_delete_detectors,
5354
can_edit_detectors,
5455
)
5556
from sentry.workflow_engine.endpoints.validators.detector_workflow_mutation import (
@@ -506,7 +507,7 @@ def delete(self, request: Request, organization: Organization) -> Response:
506507
)
507508

508509
# Check if the user has edit permissions for all detectors
509-
if not can_edit_detectors(queryset, request):
510+
if not can_delete_detectors(queryset, request):
510511
raise PermissionDenied
511512

512513
for detector in queryset:

src/sentry/workflow_engine/endpoints/validators/detector_workflow.py

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,31 @@ def can_edit_detector(detector: Detector, request: Request) -> bool:
6969
return can_edit_user_created_detectors(request, detector.project)
7070

7171

72+
def can_delete_detectors(detectors: QuerySet[Detector], request: Request) -> bool:
73+
"""
74+
Determine if the requesting user has access to delete the given detectors.
75+
Only user-created detectors can be deleted, and require "alerts:write" permission.
76+
"""
77+
if any(is_system_created_detector(detector) for detector in detectors):
78+
return False
79+
80+
projects = Project.objects.filter(
81+
id__in=detectors.values_list("project_id", flat=True).distinct()
82+
)
83+
return all(can_edit_user_created_detectors(request, project) for project in projects)
84+
85+
86+
def can_delete_detector(detector: Detector, request: Request) -> bool:
87+
"""
88+
Determine if the requesting user has access to delete the given detector.
89+
Only user-created detectors can be deleted, and require "alerts:write" permission.
90+
"""
91+
if is_system_created_detector(detector):
92+
return False
93+
94+
return can_edit_user_created_detectors(request, detector.project)
95+
96+
7297
def can_edit_detector_workflow_connections(detector: Detector, request: Request) -> bool:
7398
"""
7499
Anyone with alert write access to the project can connect/disconnect detectors of any type,

tests/sentry/workflow_engine/endpoints/test_organization_detector_details.py

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -869,3 +869,19 @@ def test_anomaly_detection(
869869
mock_seer_request.assert_called_once_with(
870870
source_id=int(self.data_source.source_id), organization=self.organization
871871
)
872+
873+
def test_cannot_delete_system_created_detector(self) -> None:
874+
error_detector = self.create_detector(
875+
project=self.project,
876+
name="Error Detector",
877+
type=ErrorGroupType.slug,
878+
)
879+
880+
self.get_error_response(self.organization.slug, error_detector.id, status_code=403)
881+
882+
# Verify detector was not deleted
883+
error_detector.refresh_from_db()
884+
assert error_detector.status != ObjectStatus.PENDING_DELETION
885+
assert not RegionScheduledDeletion.objects.filter(
886+
model_name="Detector", object_id=error_detector.id
887+
).exists()

tests/sentry/workflow_engine/endpoints/test_organization_detector_index.py

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1795,3 +1795,54 @@ def test_delete_detectors_permission_denied(self) -> None:
17951795

17961796
# Verify detector was not affected
17971797
self.assert_unaffected_detectors([self.detector, other_detector])
1798+
1799+
def test_delete_system_created_detector_by_id_prevented(self) -> None:
1800+
# Test that system-created detectors cannot be deleted via bulk delete by ID
1801+
error_detector = self.create_detector(
1802+
project=self.project,
1803+
name="Error Detector",
1804+
type=ErrorGroupType.slug,
1805+
)
1806+
1807+
self.get_error_response(
1808+
self.organization.slug,
1809+
qs_params={"id": str(error_detector.id)},
1810+
status_code=403,
1811+
)
1812+
1813+
self.assert_unaffected_detectors([error_detector])
1814+
1815+
def test_delete_system_and_user_created(self) -> None:
1816+
# Test that permission is denied when request includes system-created detectors
1817+
error_detector = self.create_detector(
1818+
project=self.project,
1819+
name="Error Detector",
1820+
type=ErrorGroupType.slug,
1821+
)
1822+
1823+
self.get_error_response(
1824+
self.organization.slug,
1825+
qs_params=[
1826+
("id", str(self.detector.id)),
1827+
("id", str(error_detector.id)),
1828+
],
1829+
status_code=403,
1830+
)
1831+
1832+
self.assert_unaffected_detectors([self.detector, error_detector])
1833+
1834+
def test_delete_system_and_user_created_with_query_filters(self) -> None:
1835+
# Test that permission is denied when query filter request includes system-created detectors
1836+
error_detector = self.create_detector(
1837+
project=self.project,
1838+
name="Test Error Detector",
1839+
type=ErrorGroupType.slug,
1840+
)
1841+
1842+
self.get_error_response(
1843+
self.organization.slug,
1844+
qs_params={"query": "Test", "project": self.project.id},
1845+
status_code=403,
1846+
)
1847+
1848+
self.assert_unaffected_detectors([self.detector, error_detector])

0 commit comments

Comments
 (0)