diff --git a/src/sentry/workflow_engine/endpoints/organization_alertrule_detector_index.py b/src/sentry/workflow_engine/endpoints/organization_alertrule_detector_index.py index 3bc38baf6adb..ed0670c95ab7 100644 --- a/src/sentry/workflow_engine/endpoints/organization_alertrule_detector_index.py +++ b/src/sentry/workflow_engine/endpoints/organization_alertrule_detector_index.py @@ -59,7 +59,8 @@ def get(self, request: Request, organization: Organization) -> Response: alert_rule_id = validator.validated_data.get("alert_rule_id") detector_id = validator.validated_data.get("detector_id") - queryset = AlertRuleDetector.objects.filter(detector__project__organization=organization) + projects = self.get_projects(request, organization, include_all_accessible=True) + queryset = AlertRuleDetector.objects.filter(detector__project__in=projects) if detector_id: queryset = queryset.filter(detector_id=detector_id) @@ -77,11 +78,13 @@ def get(self, request: Request, organization: Organization) -> Response: # Fallback: if alert_rule_id was provided but no AlertRuleDetector was found, # try looking up Detector directly using calculated detector_id - if alert_rule_id: + if alert_rule_id and not rule_id: try: calculated_detector_id = get_object_id_from_fake_id(int(alert_rule_id)) + if detector_id and detector_id != calculated_detector_id: + raise ResourceDoesNotExist detector = Detector.objects.with_type_filters().get( - id=calculated_detector_id, project__organization=organization + id=calculated_detector_id, project__in=projects ) if detector: diff --git a/src/sentry/workflow_engine/endpoints/organization_alertrule_workflow_index.py b/src/sentry/workflow_engine/endpoints/organization_alertrule_workflow_index.py index 2762dcc17902..e250af572d4b 100644 --- a/src/sentry/workflow_engine/endpoints/organization_alertrule_workflow_index.py +++ b/src/sentry/workflow_engine/endpoints/organization_alertrule_workflow_index.py @@ -1,3 +1,4 @@ +from django.db.models import Q from drf_spectacular.utils import extend_schema from rest_framework.request import Request from rest_framework.response import Response @@ -17,12 +18,14 @@ ) from sentry.apidocs.parameters import GlobalParams from sentry.models.organization import Organization +from sentry.models.rule import Rule from sentry.workflow_engine.endpoints.serializers.alertrule_workflow_serializer import ( AlertRuleWorkflowSerializer, ) from sentry.workflow_engine.endpoints.validators.alertrule_workflow import ( AlertRuleWorkflowValidator, ) +from sentry.workflow_engine.models.alertrule_detector import AlertRuleDetector from sentry.workflow_engine.models.alertrule_workflow import AlertRuleWorkflow @@ -68,6 +71,32 @@ def get(self, request: Request, organization: Organization) -> Response: if rule_id: queryset = queryset.filter(rule_id=rule_id) + projects = self.get_projects(request, organization, include_all_accessible=True) + accessible_detectors = AlertRuleDetector.objects.filter(detector__project__in=projects) + # Limit source lookups to these candidates, rather than every accessible alert. + candidate_ids = list(queryset.values_list("rule_id", "alert_rule_id")) + candidate_rule_ids = [rule_id for rule_id, _ in candidate_ids if rule_id is not None] + candidate_alert_rule_ids = [ + alert_rule_id for _, alert_rule_id in candidate_ids if alert_rule_id is not None + ] + accessible_rules = Rule.objects.filter(id__in=candidate_rule_ids, project__in=projects) + # Authorize the rule being mapped, not any project connected to its workflow: + # deduplicated workflows can contain mappings from several projects. + # Legacy rules also cover migrations without an AlertRuleDetector (e.g. cron). + queryset = queryset.filter( + Q(rule_id__in=accessible_rules.values("id")) + | Q( + rule_id__in=accessible_detectors.filter(rule_id__in=candidate_rule_ids).values( + "rule_id" + ) + ) + | Q( + alert_rule_id__in=accessible_detectors.filter( + alert_rule_id__in=candidate_alert_rule_ids + ).values("alert_rule_id") + ) + ) + alert_rule_workflow = queryset.first() if not alert_rule_workflow: raise ResourceDoesNotExist diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_alertrule_detector.py b/tests/sentry/workflow_engine/endpoints/test_organization_alertrule_detector.py index 0f111621f729..f013a5348611 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_alertrule_detector.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_alertrule_detector.py @@ -1,5 +1,6 @@ from sentry.api.serializers import serialize from sentry.incidents.endpoints.serializers.utils import get_fake_id_from_object_id +from sentry.incidents.grouptype import MetricIssue from sentry.testutils.cases import APITestCase from sentry.testutils.silo import cell_silo_test @@ -140,3 +141,64 @@ def test_fallback_with_nonexistent_detector(self) -> None: self.get_error_response( self.organization.slug, alert_rule_id=str(nonexistent_fake_id), status_code=404 ) + + +@cell_silo_test +class OrganizationAlertRuleDetectorProjectAccessTest(OrganizationAlertRuleDetectorAPITestCase): + def setUp(self) -> None: + super().setUp() + self.organization.flags.allow_joinleave = False + self.organization.save() + self.member = self.create_user(is_superuser=False) + self.create_member( + user=self.member, organization=self.organization, role="member", teams=[] + ) + self.login_as(self.member) + + def test_mapping_requires_project_access(self) -> None: + self.get_error_response( + self.organization.slug, detector_id=self.detector_1.id, status_code=404 + ) + self.get_error_response(self.organization.slug, rule_id=67890, status_code=404) + self.get_error_response(self.organization.slug, alert_rule_id=12345, status_code=404) + + self.create_team_membership(team=self.team, user=self.member) + response = self.get_success_response(self.organization.slug, rule_id=67890) + assert response.data == serialize(self.alert_rule_detector_2, self.member) + response = self.get_success_response(self.organization.slug, alert_rule_id=12345) + assert response.data == serialize(self.alert_rule_detector_1, self.member) + + def test_fallback_respects_project_access_and_filters(self) -> None: + detector = self.create_detector(project=self.project, type=MetricIssue.slug) + fake_alert_rule_id = get_fake_id_from_object_id(detector.id) + self.get_error_response( + self.organization.slug, alert_rule_id=fake_alert_rule_id, status_code=404 + ) + + self.create_team_membership(team=self.team, user=self.member) + response = self.get_success_response( + self.organization.slug, alert_rule_id=fake_alert_rule_id, detector_id=detector.id + ) + assert response.data == { + "detectorId": str(detector.id), + "alertRuleId": str(fake_alert_rule_id), + "ruleId": None, + } + + self.get_error_response( + self.organization.slug, + alert_rule_id=fake_alert_rule_id, + detector_id=self.detector_1.id, + status_code=404, + ) + self.get_error_response( + self.organization.slug, + alert_rule_id=fake_alert_rule_id, + rule_id=67890, + status_code=404, + ) + self.get_error_response( + self.organization.slug, + alert_rule_id=get_fake_id_from_object_id(self.other_detector.id), + status_code=404, + ) diff --git a/tests/sentry/workflow_engine/endpoints/test_organization_alertrule_workflow.py b/tests/sentry/workflow_engine/endpoints/test_organization_alertrule_workflow.py index e2dd70960cf3..41e3289c8d76 100644 --- a/tests/sentry/workflow_engine/endpoints/test_organization_alertrule_workflow.py +++ b/tests/sentry/workflow_engine/endpoints/test_organization_alertrule_workflow.py @@ -1,6 +1,7 @@ from sentry.api.serializers import serialize from sentry.testutils.cases import APITestCase from sentry.testutils.silo import cell_silo_test +from sentry.workflow_engine.models import AlertRuleDetector, AlertRuleWorkflow class OrganizationAlertRuleWorkflowAPITestCase(APITestCase): @@ -23,6 +24,12 @@ def setUp(self) -> None: self.alert_rule_workflow_3 = self.create_alert_rule_workflow( alert_rule_id=11111, workflow=self.workflow_3 ) + self.detector = self.create_detector(project=self.project) + self.create_alert_rule_detector(alert_rule_id=12345, detector=self.detector) + self.create_alert_rule_detector(rule_id=67890, detector=self.detector) + self.create_alert_rule_detector(alert_rule_id=11111, detector=self.detector) + self.create_detector_workflow(detector=self.detector, workflow=self.workflow_1) + self.create_detector_workflow(detector=self.detector, workflow=self.workflow_2) # Create workflow in different organization to test filtering self.other_org = self.create_organization() @@ -97,3 +104,76 @@ def test_organization_isolation(self) -> None: self.get_error_response( self.organization.slug, workflow_id=str(self.other_workflow.id), status_code=404 ) + + +@cell_silo_test +class OrganizationAlertRuleWorkflowProjectAccessTest(OrganizationAlertRuleWorkflowAPITestCase): + def setUp(self) -> None: + super().setUp() + self.organization.flags.allow_joinleave = False + self.organization.save() + self.member = self.create_user(is_superuser=False) + self.create_member( + user=self.member, organization=self.organization, role="member", teams=[] + ) + self.login_as(self.member) + + def test_mapping_requires_source_project_access(self) -> None: + self.get_error_response( + self.organization.slug, workflow_id=self.workflow_1.id, status_code=404 + ) + self.get_error_response(self.organization.slug, rule_id=67890, status_code=404) + self.get_error_response(self.organization.slug, alert_rule_id=12345, status_code=404) + # A detached workflow must not make its legacy mapping organization-visible. + self.get_error_response( + self.organization.slug, workflow_id=self.workflow_3.id, status_code=404 + ) + + self.create_team_membership(team=self.team, user=self.member) + response = self.get_success_response(self.organization.slug, rule_id=67890) + assert response.data == serialize(self.alert_rule_workflow_2, self.member) + response = self.get_success_response(self.organization.slug, alert_rule_id=12345) + assert response.data == serialize(self.alert_rule_workflow_1, self.member) + response = self.get_success_response(self.organization.slug, workflow_id=self.workflow_3.id) + assert response.data == serialize(self.alert_rule_workflow_3, self.member) + + def test_legacy_source_resolution(self) -> None: + rule = self.create_project_rule(project=self.project) + mapping = AlertRuleWorkflow.objects.get(rule_id=rule.id) + AlertRuleDetector.objects.filter(rule_id=rule.id).delete() + self.get_error_response(self.organization.slug, rule_id=rule.id, status_code=404) + + self.create_team_membership(team=self.team, user=self.member) + response = self.get_success_response(self.organization.slug, rule_id=rule.id) + assert response.data == serialize(mapping, self.member) + + # Even an accessible workflow cannot authorize a mapping with no source. + orphan = self.create_alert_rule_workflow(rule_id=987654321, workflow=mapping.workflow) + self.get_error_response(self.organization.slug, rule_id=orphan.rule_id, status_code=404) + + def test_shared_workflow_only_returns_accessible_mapping(self) -> None: + rule = self.create_project_rule(project=self.project) + AlertRuleWorkflow.objects.filter(rule_id=rule.id).update(workflow=self.workflow_1) + accessible_team = self.create_team(organization=self.organization) + accessible_project = self.create_project( + organization=self.organization, teams=[accessible_team] + ) + self.create_team_membership(team=accessible_team, user=self.member) + detector = self.create_detector(project=accessible_project) + self.create_detector_workflow(detector=detector, workflow=self.workflow_1) + self.create_alert_rule_detector(alert_rule_id=22222, detector=detector) + accessible_mapping = self.create_alert_rule_workflow( + alert_rule_id=22222, workflow=self.workflow_1 + ) + + # The older, inaccessible mapping must not be selected by first(). + response = self.get_success_response(self.organization.slug, workflow_id=self.workflow_1.id) + assert response.data == serialize(accessible_mapping, self.member) + self.get_error_response(self.organization.slug, alert_rule_id=12345, status_code=404) + self.get_error_response(self.organization.slug, rule_id=rule.id, status_code=404) + self.get_error_response( + self.organization.slug, + workflow_id=self.workflow_1.id, + alert_rule_id=12345, + status_code=404, + )