diff --git a/src/sentry/workflow_engine/endpoints/organization_detector_details.py b/src/sentry/workflow_engine/endpoints/organization_detector_details.py index 63f4a5e02b1162..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 @@ -31,6 +30,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,12 +193,9 @@ 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: - return Response(status=403) - validator = get_detector_validator( request, detector.project, detector.type, instance=detector ) diff --git a/src/sentry/workflow_engine/endpoints/organization_detector_index.py b/src/sentry/workflow_engine/endpoints/organization_detector_index.py index 4fef0177f7cc8b..19eb04bd910c2a 100644 --- a/src/sentry/workflow_engine/endpoints/organization_detector_index.py +++ b/src/sentry/workflow_engine/endpoints/organization_detector_index.py @@ -50,6 +50,7 @@ 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, ) from sentry.workflow_engine.endpoints.validators.detector_workflow_mutation import ( @@ -506,7 +507,7 @@ 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 for detector in queryset: 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..5ccd5b7f7733eb 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 + 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() 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..2c1d2f9c8fa58b 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,54 @@ 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, + ) + + self.get_error_response( + self.organization.slug, + qs_params={"id": str(error_detector.id)}, + status_code=403, + ) + + self.assert_unaffected_detectors([error_detector]) + + 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, + ) + + self.get_error_response( + self.organization.slug, + qs_params=[ + ("id", str(self.detector.id)), + ("id", str(error_detector.id)), + ], + status_code=403, + ) + + self.assert_unaffected_detectors([self.detector, error_detector]) + + 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, + ) + + self.get_error_response( + self.organization.slug, + qs_params={"query": "Test", "project": self.project.id}, + status_code=403, + ) + + self.assert_unaffected_detectors([self.detector, error_detector])