Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Comment on lines +86 to +94
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having 2 functions that do the same thing, you could pass a single detector to can_delete_detectors as a list.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the single detector function is nice because it saves us a lookup from the Project table



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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Original file line number Diff line number Diff line change
Expand Up @@ -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])
Loading