From 7d6975eedf930ea51cced07d3b66aadb8010865a Mon Sep 17 00:00:00 2001 From: Mia Hsu Date: Fri, 21 Nov 2025 11:03:02 -0800 Subject: [PATCH 1/3] fix(aci): prevent deletion of system-created monitors in API --- .../endpoints/organization_detector_index.py | 16 +++- .../test_organization_detector_index.py | 79 +++++++++++++++++++ 2 files changed, 94 insertions(+), 1 deletion(-) diff --git a/src/sentry/workflow_engine/endpoints/organization_detector_index.py b/src/sentry/workflow_engine/endpoints/organization_detector_index.py index 4fef0177f7cc8b..550fa4f4ca8a46 100644 --- a/src/sentry/workflow_engine/endpoints/organization_detector_index.py +++ b/src/sentry/workflow_engine/endpoints/organization_detector_index.py @@ -51,6 +51,7 @@ from sentry.workflow_engine.endpoints.validators.detector_workflow import ( BulkDetectorWorkflowsValidator, can_edit_detectors, + is_system_created_detector, ) from sentry.workflow_engine.endpoints.validators.detector_workflow_mutation import ( DetectorWorkflowMutationValidator, @@ -509,7 +510,20 @@ def delete(self, request: Request, organization: Organization) -> Response: if not can_edit_detectors(queryset, request): raise PermissionDenied - for detector in queryset: + # Filter out system-created detectors from deletion + deletable_detectors = [ + detector for detector in queryset if not is_system_created_detector(detector) + ] + + if not deletable_detectors: + return Response( + { + "detail": "No detectors can be deleted. System-created detectors cannot be deleted." + }, + status=status.HTTP_400_BAD_REQUEST, + ) + + for detector in deletable_detectors: with transaction.atomic(router.db_for_write(Detector)): RegionScheduledDeletion.schedule(detector, days=0, actor=request.user) create_audit_entry( diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_detector_index.py b/tests/sentry/workflow_engine/endpoints/test_organization_detector_index.py index 730f210855b7af..af17136bdbdce3 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_detector_index.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_detector_index.py @@ -1795,3 +1795,82 @@ def test_delete_detectors_permission_denied(self) -> None: # Verify detector was not affected self.assert_unaffected_detectors([self.detector, other_detector]) + + def test_delete_system_created_detector_by_id_prevented(self) -> None: + # Test that system-created detectors cannot be deleted via bulk delete by ID + error_detector = self.create_detector( + project=self.project, + name="Error Detector", + type=ErrorGroupType.slug, + ) + + response = self.get_error_response( + self.organization.slug, + qs_params={"id": str(error_detector.id)}, + status_code=400, + ) + + assert "System-created detectors cannot be deleted" in str(response.data["detail"]) + + # Verify the error detector was not affected + self.assert_unaffected_detectors([error_detector]) + + def test_delete_mixed_detectors_filters_system_created(self) -> None: + # Test that system-created detectors are excluded when deleting by ID + error_detector = self.create_detector( + project=self.project, + name="Error Detector", + type=ErrorGroupType.slug, + ) + + with outbox_runner(): + self.get_success_response( + self.organization.slug, + qs_params=[ + ("id", str(self.detector.id)), + ("id", str(error_detector.id)), + ], + status_code=204, + ) + + # User-created detector should be scheduled for deletion + self.detector.refresh_from_db() + assert self.detector.status == ObjectStatus.PENDING_DELETION + assert RegionScheduledDeletion.objects.filter( + model_name="Detector", + object_id=self.detector.id, + ).exists() + + # System-created detector should NOT be scheduled for deletion + self.assert_unaffected_detectors([error_detector]) + + def test_delete_by_query_filters_system_created(self) -> None: + # Test that system-created detectors are excluded when deleting by query + error_detector = self.create_detector( + project=self.project, + name="Test Error Detector", + type=ErrorGroupType.slug, + ) + + with outbox_runner(): + self.get_success_response( + self.organization.slug, + qs_params={"query": "test", "project": self.project.id}, + status_code=204, + ) + + # User-created detector should be scheduled for deletion + self.detector.refresh_from_db() + assert self.detector.status == ObjectStatus.PENDING_DELETION + assert RegionScheduledDeletion.objects.filter( + model_name="Detector", + object_id=self.detector.id, + ).exists() + + # System-created detector should NOT be deleted + error_detector.refresh_from_db() + assert error_detector.status != ObjectStatus.PENDING_DELETION + assert not RegionScheduledDeletion.objects.filter( + model_name="Detector", + object_id=error_detector.id, + ).exists() From 702f902be6e3f6d9e2a3b83d8dde4598b330f8b9 Mon Sep 17 00:00:00 2001 From: Mia Hsu Date: Fri, 21 Nov 2025 16:17:31 -0800 Subject: [PATCH 2/3] can_delete helper and fix details endpoint --- .../organization_detector_details.py | 3 +- .../endpoints/organization_detector_index.py | 19 +---- .../endpoints/validators/detector_workflow.py | 25 +++++++ .../test_organization_detector_details.py | 16 +++++ .../test_organization_detector_index.py | 70 ++++++------------- 5 files changed, 67 insertions(+), 66 deletions(-) diff --git a/src/sentry/workflow_engine/endpoints/organization_detector_details.py b/src/sentry/workflow_engine/endpoints/organization_detector_details.py index 63f4a5e02b1162..c65390a57a9945 100644 --- a/src/sentry/workflow_engine/endpoints/organization_detector_details.py +++ b/src/sentry/workflow_engine/endpoints/organization_detector_details.py @@ -31,6 +31,7 @@ from sentry.workflow_engine.endpoints.serializers.detector_serializer import DetectorSerializer from sentry.workflow_engine.endpoints.validators.detector_workflow import ( BulkDetectorWorkflowsValidator, + can_delete_detector, can_edit_detector, ) from sentry.workflow_engine.endpoints.validators.utils import get_unknown_detector_type_error @@ -193,7 +194,7 @@ def delete(self, request: Request, organization: Organization, detector: Detecto """ Delete a detector """ - if not can_edit_detector(detector, request): + if not can_delete_detector(detector, request): raise PermissionDenied if detector.type == ErrorGroupType.slug: diff --git a/src/sentry/workflow_engine/endpoints/organization_detector_index.py b/src/sentry/workflow_engine/endpoints/organization_detector_index.py index 550fa4f4ca8a46..19eb04bd910c2a 100644 --- a/src/sentry/workflow_engine/endpoints/organization_detector_index.py +++ b/src/sentry/workflow_engine/endpoints/organization_detector_index.py @@ -50,8 +50,8 @@ from sentry.workflow_engine.endpoints.validators.base import BaseDetectorTypeValidator from sentry.workflow_engine.endpoints.validators.detector_workflow import ( BulkDetectorWorkflowsValidator, + can_delete_detectors, can_edit_detectors, - is_system_created_detector, ) from sentry.workflow_engine.endpoints.validators.detector_workflow_mutation import ( DetectorWorkflowMutationValidator, @@ -507,23 +507,10 @@ def delete(self, request: Request, organization: Organization) -> Response: ) # Check if the user has edit permissions for all detectors - if not can_edit_detectors(queryset, request): + if not can_delete_detectors(queryset, request): raise PermissionDenied - # Filter out system-created detectors from deletion - deletable_detectors = [ - detector for detector in queryset if not is_system_created_detector(detector) - ] - - if not deletable_detectors: - return Response( - { - "detail": "No detectors can be deleted. System-created detectors cannot be deleted." - }, - status=status.HTTP_400_BAD_REQUEST, - ) - - for detector in deletable_detectors: + for detector in queryset: with transaction.atomic(router.db_for_write(Detector)): RegionScheduledDeletion.schedule(detector, days=0, actor=request.user) create_audit_entry( diff --git a/src/sentry/workflow_engine/endpoints/validators/detector_workflow.py b/src/sentry/workflow_engine/endpoints/validators/detector_workflow.py index 7104a44768005a..6fd40062950608 100644 --- a/src/sentry/workflow_engine/endpoints/validators/detector_workflow.py +++ b/src/sentry/workflow_engine/endpoints/validators/detector_workflow.py @@ -69,6 +69,31 @@ def can_edit_detector(detector: Detector, request: Request) -> bool: return can_edit_user_created_detectors(request, detector.project) +def can_delete_detectors(detectors: QuerySet[Detector], request: Request) -> bool: + """ + Determine if the requesting user has access to delete the given detectors. + Only user-created detectors can be deleted, and require "alerts:write" permission. + """ + if any(is_system_created_detector(detector) for detector in detectors): + return False + + projects = Project.objects.filter( + id__in=detectors.values_list("project_id", flat=True).distinct() + ) + return all(can_edit_user_created_detectors(request, project) for project in projects) + + +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) + + def can_edit_detector_workflow_connections(detector: Detector, request: Request) -> bool: """ Anyone with alert write access to the project can connect/disconnect detectors of any type, diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_detector_details.py b/tests/sentry/workflow_engine/endpoints/test_organization_detector_details.py index a66669ac26d980..10eec9256752e2 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_detector_details.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_detector_details.py @@ -843,3 +843,19 @@ def test_anomaly_detection( mock_seer_request.assert_called_once_with( source_id=int(self.data_source.source_id), organization=self.organization ) + + def test_cannot_delete_system_created_detector(self) -> None: + error_detector = self.create_detector( + project=self.project, + name="Error Detector", + type=ErrorGroupType.slug, + ) + + self.get_error_response(self.organization.slug, error_detector.id, status_code=403) + + # Verify detector was not deleted + self.detector.refresh_from_db() + assert self.detector.status != ObjectStatus.PENDING_DELETION + assert not RegionScheduledDeletion.objects.filter( + model_name="Detector", object_id=self.detector.id + ).exists() diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_detector_index.py b/tests/sentry/workflow_engine/endpoints/test_organization_detector_index.py index af17136bdbdce3..2c1d2f9c8fa58b 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_detector_index.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_detector_index.py @@ -1804,73 +1804,45 @@ def test_delete_system_created_detector_by_id_prevented(self) -> None: type=ErrorGroupType.slug, ) - response = self.get_error_response( + self.get_error_response( self.organization.slug, qs_params={"id": str(error_detector.id)}, - status_code=400, + status_code=403, ) - assert "System-created detectors cannot be deleted" in str(response.data["detail"]) - - # Verify the error detector was not affected self.assert_unaffected_detectors([error_detector]) - def test_delete_mixed_detectors_filters_system_created(self) -> None: - # Test that system-created detectors are excluded when deleting by ID + def test_delete_system_and_user_created(self) -> None: + # Test that permission is denied when request includes system-created detectors error_detector = self.create_detector( project=self.project, name="Error Detector", type=ErrorGroupType.slug, ) - with outbox_runner(): - self.get_success_response( - self.organization.slug, - qs_params=[ - ("id", str(self.detector.id)), - ("id", str(error_detector.id)), - ], - status_code=204, - ) - - # User-created detector should be scheduled for deletion - self.detector.refresh_from_db() - assert self.detector.status == ObjectStatus.PENDING_DELETION - assert RegionScheduledDeletion.objects.filter( - model_name="Detector", - object_id=self.detector.id, - ).exists() + self.get_error_response( + self.organization.slug, + qs_params=[ + ("id", str(self.detector.id)), + ("id", str(error_detector.id)), + ], + status_code=403, + ) - # System-created detector should NOT be scheduled for deletion - self.assert_unaffected_detectors([error_detector]) + self.assert_unaffected_detectors([self.detector, error_detector]) - def test_delete_by_query_filters_system_created(self) -> None: - # Test that system-created detectors are excluded when deleting by query + def test_delete_system_and_user_created_with_query_filters(self) -> None: + # Test that permission is denied when query filter request includes system-created detectors error_detector = self.create_detector( project=self.project, name="Test Error Detector", type=ErrorGroupType.slug, ) - with outbox_runner(): - self.get_success_response( - self.organization.slug, - qs_params={"query": "test", "project": self.project.id}, - status_code=204, - ) - - # User-created detector should be scheduled for deletion - self.detector.refresh_from_db() - assert self.detector.status == ObjectStatus.PENDING_DELETION - assert RegionScheduledDeletion.objects.filter( - model_name="Detector", - object_id=self.detector.id, - ).exists() + self.get_error_response( + self.organization.slug, + qs_params={"query": "Test", "project": self.project.id}, + status_code=403, + ) - # System-created detector should NOT be deleted - error_detector.refresh_from_db() - assert error_detector.status != ObjectStatus.PENDING_DELETION - assert not RegionScheduledDeletion.objects.filter( - model_name="Detector", - object_id=error_detector.id, - ).exists() + self.assert_unaffected_detectors([self.detector, error_detector]) From 519ca73350ff9831f244c8f42ec286597c204405 Mon Sep 17 00:00:00 2001 From: Mia Hsu Date: Mon, 24 Nov 2025 09:52:09 -0800 Subject: [PATCH 3/3] fixes --- .../endpoints/organization_detector_details.py | 4 ---- .../endpoints/test_organization_detector_details.py | 6 +++--- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/sentry/workflow_engine/endpoints/organization_detector_details.py b/src/sentry/workflow_engine/endpoints/organization_detector_details.py index c65390a57a9945..999e4146725e03 100644 --- a/src/sentry/workflow_engine/endpoints/organization_detector_details.py +++ b/src/sentry/workflow_engine/endpoints/organization_detector_details.py @@ -21,7 +21,6 @@ ) from sentry.apidocs.parameters import DetectorParams, GlobalParams from sentry.db.postgres.transactions import in_test_hide_transaction_boundary -from sentry.grouping.grouptype import ErrorGroupType from sentry.incidents.grouptype import MetricIssue from sentry.incidents.metric_issue_detector import schedule_update_project_config from sentry.issues import grouptype @@ -197,9 +196,6 @@ def delete(self, request: Request, organization: Organization, detector: Detecto if not can_delete_detector(detector, request): raise PermissionDenied - if detector.type == ErrorGroupType.slug: - return Response(status=403) - validator = get_detector_validator( request, detector.project, detector.type, instance=detector ) diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_detector_details.py b/tests/sentry/workflow_engine/endpoints/test_organization_detector_details.py index 10eec9256752e2..5ccd5b7f7733eb 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_detector_details.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_detector_details.py @@ -854,8 +854,8 @@ def test_cannot_delete_system_created_detector(self) -> None: self.get_error_response(self.organization.slug, error_detector.id, status_code=403) # Verify detector was not deleted - self.detector.refresh_from_db() - assert self.detector.status != ObjectStatus.PENDING_DELETION + error_detector.refresh_from_db() + assert error_detector.status != ObjectStatus.PENDING_DELETION assert not RegionScheduledDeletion.objects.filter( - model_name="Detector", object_id=self.detector.id + model_name="Detector", object_id=error_detector.id ).exists()