From 470ba75f38796c44bc9f2ad67f080023f434ffc5 Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Tue, 16 Apr 2024 15:36:43 -0700 Subject: [PATCH 1/2] fix(issue-platform): Check permissions for assignees sent to the issue platform We should make sure that the assignee for an occurrence actually belongs to the org. --- src/sentry/api/fields/actor.py | 34 +++--------------- src/sentry/issues/occurrence_consumer.py | 14 +++++++- src/sentry/utils/actor.py | 36 ++++++++++++++++++- .../sentry/issues/test_occurrence_consumer.py | 21 ++++++++++- 4 files changed, 73 insertions(+), 32 deletions(-) diff --git a/src/sentry/api/fields/actor.py b/src/sentry/api/fields/actor.py index 8efcdd25e32036..8c0c288b4333c5 100644 --- a/src/sentry/api/fields/actor.py +++ b/src/sentry/api/fields/actor.py @@ -4,10 +4,7 @@ from drf_spectacular.utils import extend_schema_field from rest_framework import serializers -from sentry.models.organizationmember import OrganizationMember -from sentry.models.team import Team -from sentry.models.user import User -from sentry.utils.actor import ActorTuple +from sentry.utils.actor import parse_and_validate_actor @extend_schema_field(field=OpenApiTypes.STR) @@ -20,29 +17,8 @@ def to_representation(self, value): return value.identifier def to_internal_value(self, data): - if not data: - return None + actor_tuple = parse_and_validate_actor(data, self.context["organization"].id) - try: - actor = ActorTuple.from_actor_identifier(data) - except Exception: - raise serializers.ValidationError( - "Could not parse actor. Format should be `type:id` where type is `team` or `user`." - ) - try: - obj = actor.resolve() - except (Team.DoesNotExist, User.DoesNotExist): - raise serializers.ValidationError(f"{actor.type.__name__} does not exist") - - if actor.type == Team: - if obj.organization != self.context["organization"]: - raise serializers.ValidationError("Team is not a member of this organization") - elif actor.type == User: - if not OrganizationMember.objects.filter( - organization=self.context["organization"], user_id=obj.id - ).exists(): - raise serializers.ValidationError("User is not a member of this organization") - - if self.as_actor: - return actor.resolve_to_actor() - return actor + if self.as_actor and actor_tuple: + return actor_tuple.resolve_to_actor() + return actor_tuple diff --git a/src/sentry/issues/occurrence_consumer.py b/src/sentry/issues/occurrence_consumer.py index 9a96aa0d05ec48..861d789463a410 100644 --- a/src/sentry/issues/occurrence_consumer.py +++ b/src/sentry/issues/occurrence_consumer.py @@ -22,6 +22,7 @@ from sentry.models.organization import Organization from sentry.models.project import Project from sentry.utils import metrics +from sentry.utils.actor import parse_and_validate_actor logger = logging.getLogger(__name__) @@ -142,6 +143,17 @@ def _get_kwargs(payload: Mapping[str, Any]) -> Mapping[str, Any]: try: with metrics.timer("occurrence_ingest.duration", instance="_get_kwargs"): metrics.distribution("occurrence.ingest.size.data", len(payload), unit="byte") + assignee_identifier = None + project = Project.objects.get_from_cache(id=payload["project_id"]) + try: + assignee = parse_and_validate_actor( + payload.get("assignee"), project.organization_id + ) + if assignee: + assignee_identifier = assignee.identifier + except Exception: + logger.exception("Failed to validate assignee for occurrence") + occurrence_data = { "id": UUID(payload["id"]).hex, "project_id": payload["project_id"], @@ -154,7 +166,7 @@ def _get_kwargs(payload: Mapping[str, Any]) -> Mapping[str, Any]: "type": payload["type"], "detection_time": payload["detection_time"], "level": payload.get("level", DEFAULT_LEVEL), - "assignee": payload.get("assignee"), + "assignee": assignee_identifier, } process_occurrence_data(occurrence_data) diff --git a/src/sentry/utils/actor.py b/src/sentry/utils/actor.py index 2ed20142064dfc..294b260be2546d 100644 --- a/src/sentry/utils/actor.py +++ b/src/sentry/utils/actor.py @@ -6,11 +6,12 @@ from rest_framework import serializers +from sentry.services.hybrid_cloud.user import RpcUser + if TYPE_CHECKING: from sentry.models.actor import Actor from sentry.models.team import Team from sentry.models.user import User - from sentry.services.hybrid_cloud.user import RpcUser class ActorTuple(namedtuple("Actor", "id type")): @@ -164,3 +165,36 @@ def fetch_actor_by_id(cls: type[User] | type[Team], id: int) -> Team | RpcUser: return user else: raise ValueError(f"Cls {cls} is not a valid actor type.") + + +def parse_and_validate_actor( + actor_identifier: str | None, organization_id: int +) -> ActorTuple | None: + from sentry.models.organizationmember import OrganizationMember + from sentry.models.team import Team + from sentry.models.user import User + + if not actor_identifier: + return None + + try: + actor = ActorTuple.from_actor_identifier(actor_identifier) + except Exception: + raise serializers.ValidationError( + "Could not parse actor. Format should be `type:id` where type is `team` or `user`." + ) + try: + obj = actor.resolve() + except (Team.DoesNotExist, User.DoesNotExist): + raise serializers.ValidationError(f"{actor.type.__name__} does not exist") + + if isinstance(obj, Team): + if obj.organization_id != organization_id: + raise serializers.ValidationError("Team is not a member of this organization") + elif isinstance(obj, RpcUser): + if not OrganizationMember.objects.filter( + organization_id=organization_id, user_id=obj.id + ).exists(): + raise serializers.ValidationError("User is not a member of this organization") + + return actor diff --git a/tests/sentry/issues/test_occurrence_consumer.py b/tests/sentry/issues/test_occurrence_consumer.py index 34c534df8e2520..9484e452ada6d4 100644 --- a/tests/sentry/issues/test_occurrence_consumer.py +++ b/tests/sentry/issues/test_occurrence_consumer.py @@ -465,6 +465,25 @@ def test_assignee(self) -> None: kwargs = _get_kwargs(message) assert kwargs["occurrence_data"]["assignee"] == f"user:{self.user.id}" + def test_assignee_perms(self) -> None: + message = deepcopy(get_test_message(self.project.id)) + random_user = self.create_user() + message["assignee"] = f"user:{random_user.id}" + kwargs = _get_kwargs(message) + assert kwargs["occurrence_data"]["assignee"] is None + + message = deepcopy(get_test_message(self.project.id)) + message["assignee"] = "user:99999999999" + kwargs = _get_kwargs(message) + assert kwargs["occurrence_data"]["assignee"] is None + + other_org = self.create_organization() + random_team = self.create_team(other_org) + message = deepcopy(get_test_message(self.project.id)) + message["assignee"] = f"team:{random_team.id}" + kwargs = _get_kwargs(message) + assert kwargs["occurrence_data"]["assignee"] is None + def test_assignee_none(self) -> None: kwargs = _get_kwargs(deepcopy(get_test_message(self.project.id))) assert kwargs["occurrence_data"]["assignee"] is None @@ -475,4 +494,4 @@ def test_assignee_none(self) -> None: message = deepcopy(get_test_message(self.project.id)) message["assignee"] = "" kwargs = _get_kwargs(message) - assert kwargs["occurrence_data"]["assignee"] == "" + assert kwargs["occurrence_data"]["assignee"] is None From d1d0ce62ad01063b40740b4c861ecd80ed13b63a Mon Sep 17 00:00:00 2001 From: Dan Fuller Date: Tue, 16 Apr 2024 16:27:32 -0700 Subject: [PATCH 2/2] fix test only fetch project if necessary --- src/sentry/issues/occurrence_consumer.py | 18 +++++++++--------- .../sentry/issues/test_occurrence_consumer.py | 4 ++-- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/sentry/issues/occurrence_consumer.py b/src/sentry/issues/occurrence_consumer.py index 861d789463a410..e10fcf2cbdf434 100644 --- a/src/sentry/issues/occurrence_consumer.py +++ b/src/sentry/issues/occurrence_consumer.py @@ -144,15 +144,15 @@ def _get_kwargs(payload: Mapping[str, Any]) -> Mapping[str, Any]: with metrics.timer("occurrence_ingest.duration", instance="_get_kwargs"): metrics.distribution("occurrence.ingest.size.data", len(payload), unit="byte") assignee_identifier = None - project = Project.objects.get_from_cache(id=payload["project_id"]) - try: - assignee = parse_and_validate_actor( - payload.get("assignee"), project.organization_id - ) - if assignee: - assignee_identifier = assignee.identifier - except Exception: - logger.exception("Failed to validate assignee for occurrence") + payload_assignee = payload.get("assignee") + if payload_assignee: + project = Project.objects.get_from_cache(id=payload["project_id"]) + try: + assignee = parse_and_validate_actor(payload_assignee, project.organization_id) + if assignee: + assignee_identifier = assignee.identifier + except Exception: + logger.exception("Failed to validate assignee for occurrence") occurrence_data = { "id": UUID(payload["id"]).hex, diff --git a/tests/sentry/issues/test_occurrence_consumer.py b/tests/sentry/issues/test_occurrence_consumer.py index 9484e452ada6d4..5884b1da8c1dea 100644 --- a/tests/sentry/issues/test_occurrence_consumer.py +++ b/tests/sentry/issues/test_occurrence_consumer.py @@ -387,8 +387,8 @@ def test_missing_event_id_in_event_data(self) -> None: def test_project_ids_mismatch(self) -> None: message = deepcopy(get_test_message(self.project.id)) - message["project_id"] = 1 - message["event"]["project_id"] = 2 + message["project_id"] = self.project.id + message["event"]["project_id"] = 999999999999 with pytest.raises(InvalidEventPayloadError): _get_kwargs(message)