From c8f75f60b920c2770145d263293f3ab734b34f81 Mon Sep 17 00:00:00 2001 From: Charlie Luo Date: Tue, 21 Apr 2026 16:27:10 -0700 Subject: [PATCH] ref(assign): Switch issue assignment off OwnerActorField MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue assignment has a project-access layer that other OwnerActorField consumers lack: GroupValidator.validate_assignedTo already verifies the assignee has access to the project. That project-access check is the real permission gate for issues. Switch GroupValidator.assignedTo to ActorField so assignment no longer requires the assigner to be a member of the target team, and add an open-membership bypass so all assignment is allowed when allow_joinleave is set. OwnerActorField is unchanged — the 10 other consumers (alert rules, monitors, workflows, detectors, etc.) keep their existing team membership check. Co-authored-by: Claude --- .../references/serializer-patterns.md | 21 +++----- .../helpers/group_index/validators/group.py | 22 ++++---- .../slack/webhooks/actions/test_status.py | 4 ++ .../issues/endpoints/test_group_details.py | 2 + .../api/endpoints/test_project_group_index.py | 53 +++++++++++-------- 5 files changed, 57 insertions(+), 45 deletions(-) diff --git a/.agents/skills/sentry-security/references/serializer-patterns.md b/.agents/skills/sentry-security/references/serializer-patterns.md index 7c414592695b2a..5be8eba416c0ef 100644 --- a/.agents/skills/sentry-security/references/serializer-patterns.md +++ b/.agents/skills/sentry-security/references/serializer-patterns.md @@ -8,16 +8,15 @@ ## ActorField vs OwnerActorField -`ActorField` accepts any user or team ID without validating the requesting user's relationship to that actor. `OwnerActorField` validates team membership before allowing assignment. +`ActorField` accepts any user or team ID without validating the requesting user's relationship to that actor. `OwnerActorField` additionally checks that the requesting user is a member of the target team. **Location:** `src/sentry/api/fields/actor.py` ### When to use which -| Field | Validates Membership | Use For | -| ----------------- | -------------------- | --------------------------------- | -| `ActorField` | No | Read-only display, filtering | -| `OwnerActorField` | Yes | Owner assignment, assignee fields | +Default to `OwnerActorField` for any write-op field accepting a team or user reference (assignment, ownership, delegation). Originally PR #106074. + +One known exception: `GroupValidator.assignedTo` uses `ActorField`. Issue assignment is a label — it doesn't grant access, and project-access is validated separately in `validate_assignedTo`. Don't expand this exception without explicit review. ### Real vulnerability: ActorField for ownership (PR #106074) @@ -25,7 +24,7 @@ ```python class IssueAlertRuleSerializer(serializers.Serializer): - owner = ActorField(required=False) # BUG: Any actor, no membership check + owner = ActorField(required=False) # BUG: Any team, no membership check ``` **Fixed code:** @@ -42,18 +41,12 @@ class IssueAlertRuleSerializer(serializers.Serializer): 3. If user is a member of the target team → allowed 4. Otherwise → `ValidationError("You can only assign teams you are a member of")` -### What to look for - -Any serializer field that accepts a team or user reference for **write operations** (assignment, ownership, delegation) should use `OwnerActorField`, not `ActorField`. - -Search pattern: +### Finding existing uses ``` grep -rn "ActorField" --include="*.py" src/sentry/api/ ``` -Flag any `ActorField()` used in a serializer for PUT/POST operations where the field sets an owner or assignee. - ## Foreign Key IDs in Request Bodies When a serializer accepts an ID that references another model, the serializer or view must validate that the referenced object belongs to the same organization. @@ -91,7 +84,7 @@ A serializer accepted `condition_group_id` from user input, allowing a user to i ## Checklist ``` -□ Owner/assignee fields use OwnerActorField, not ActorField +□ Owner/assignee fields use OwnerActorField (exception: issue assignment uses ActorField) □ FK ID fields in request body are validated against the org □ IDs that should be server-set are not exposed in the serializer □ Serializer context includes organization for validation diff --git a/src/sentry/api/helpers/group_index/validators/group.py b/src/sentry/api/helpers/group_index/validators/group.py index 336b8d8f656ab7..3402c4b4cafa8f 100644 --- a/src/sentry/api/helpers/group_index/validators/group.py +++ b/src/sentry/api/helpers/group_index/validators/group.py @@ -4,7 +4,7 @@ from drf_spectacular.utils import extend_schema_serializer from rest_framework import serializers -from sentry.api.fields.actor import OwnerActorField +from sentry.api.fields.actor import ActorField from sentry.api.helpers.group_index.validators.inbox_details import InboxDetailsValidator from sentry.api.helpers.group_index.validators.status_details import StatusDetailsValidator from sentry.models.group import STATUS_UPDATE_CHOICES, Group @@ -53,7 +53,7 @@ class GroupValidator(serializers.Serializer[Group]): discard = serializers.BooleanField( help_text="If true, discards the issues instead of updating them." ) - assignedTo = OwnerActorField( + assignedTo = ActorField( help_text="The user or team that should be assigned to the issues. Values take the form of ``, `user:`, ``, ``, or `team:`." ) priority = serializers.ChoiceField( @@ -82,19 +82,21 @@ class GroupValidator(serializers.Serializer[Group]): # for the moment, the CLI sends this for any issue update, so allow nulls snoozeDuration = serializers.IntegerField(allow_null=True) - def validate_assignedTo(self, value: Actor) -> Actor: + def validate_assignedTo(self, value: Actor | None) -> Actor | None: + if not value: + return value + + organization = self.context.get("organization") + if organization and organization.flags.allow_joinleave: + return value + if ( - value - and value.is_user + value.is_user and not self.context["project"].member_set.filter(user_id=value.id).exists() ): raise serializers.ValidationError("Cannot assign to non-team member") - if ( - value - and value.is_team - and not self.context["project"].teams.filter(id=value.id).exists() - ): + if value.is_team and not self.context["project"].teams.filter(id=value.id).exists(): raise serializers.ValidationError( "Cannot assign to a team without access to the project" ) diff --git a/tests/sentry/integrations/slack/webhooks/actions/test_status.py b/tests/sentry/integrations/slack/webhooks/actions/test_status.py index df677bb12bf10e..c10e712a00da05 100644 --- a/tests/sentry/integrations/slack/webhooks/actions/test_status.py +++ b/tests/sentry/integrations/slack/webhooks/actions/test_status.py @@ -596,6 +596,8 @@ def test_assign_issue_through_unfurl(self) -> None: } def test_assign_issue_where_team_not_in_project(self) -> None: + self.organization.flags.allow_joinleave = False + self.organization.save() user2 = self.create_user(is_superuser=False) team2 = self.create_team( organization=self.organization, members=[self.user], name="Ecosystem" @@ -609,6 +611,8 @@ def test_assign_issue_where_team_not_in_project(self) -> None: assert not GroupAssignee.objects.filter(group=self.group).exists() def test_assign_issue_where_team_not_in_project_through_unfurl(self) -> None: + self.organization.flags.allow_joinleave = False + self.organization.save() user2 = self.create_user(is_superuser=False) team2 = self.create_team( organization=self.organization, members=[self.user], name="Ecosystem" diff --git a/tests/sentry/issues/endpoints/test_group_details.py b/tests/sentry/issues/endpoints/test_group_details.py index af1d0b7685928e..f1282092824a42 100644 --- a/tests/sentry/issues/endpoints/test_group_details.py +++ b/tests/sentry/issues/endpoints/test_group_details.py @@ -611,6 +611,8 @@ def test_assign_unavailable_team(self) -> None: self.login_as(user=self.user) group = self.create_group() + group.project.organization.flags.allow_joinleave = False + group.project.organization.save() team = self.create_team(organization=group.project.organization, members=[self.user]) url = f"/api/0/organizations/{group.organization.slug}/issues/{group.id}/" diff --git a/tests/snuba/api/endpoints/test_project_group_index.py b/tests/snuba/api/endpoints/test_project_group_index.py index 0f2a2c895fbfdd..69192674e8cdcf 100644 --- a/tests/snuba/api/endpoints/test_project_group_index.py +++ b/tests/snuba/api/endpoints/test_project_group_index.py @@ -1400,13 +1400,11 @@ def test_assign_team(self) -> None: assert response.status_code == 200, response.content assert response.data["assignedTo"] is None - def test_assign_team_not_member_of_when_open_membership_disabled(self) -> None: + def test_assign_team_when_user_not_on_target_team_in_closed_membership(self) -> None: """ - Test that a user cannot assign an issue to a team they are not a member of - when Open Membership is disabled. This is a regression test for an authorization - bypass vulnerability. + A user with project access can assign any team that also has project access, + even one they are not a member of. """ - # Disable Open Membership self.organization.flags.allow_joinleave = False self.organization.save() @@ -1427,34 +1425,33 @@ def test_assign_team_not_member_of_when_open_membership_disabled(self) -> None: url = f"{self.path}?id={group.id}" response = self.client.put(url, data={"assignedTo": f"team:{other_team.id}"}) - assert response.status_code == 400, response.content - assert "only assign teams you are a member of" in str(response.data) - assert not GroupAssignee.objects.filter(group=group, team=other_team).exists() + assert response.status_code == 200, response.content + assert response.data["assignedTo"]["id"] == str(other_team.id) + assert GroupAssignee.objects.filter(group=group, team=other_team).exists() - def test_assign_team_not_member_of_with_team_admin_scope(self) -> None: - """ - Test that a user with team:admin scope CAN assign an issue to a team they are - not a member of, even when Open Membership is disabled. - """ + def test_assign_team_without_project_access_when_open_membership_disabled(self) -> None: self.organization.flags.allow_joinleave = False self.organization.save() group = self.create_group() + member_user = self.create_user("member@example.com") + member_team = self.create_team(organization=group.project.organization, name="member-team") + self.create_member( + user=member_user, organization=self.organization, role="member", teams=[member_team] + ) + group.project.add_team(member_team) + other_team = self.create_team(organization=group.project.organization, name="other-team") - group.project.add_team(other_team) - admin_user = self.create_user("admin@example.com") - self.create_member(user=admin_user, organization=self.organization, role="owner") - self.login_as(user=admin_user) + self.login_as(user=member_user) url = f"{self.path}?id={group.id}" response = self.client.put(url, data={"assignedTo": f"team:{other_team.id}"}) - assert response.status_code == 200, response.content - assert response.data["assignedTo"]["id"] == str(other_team.id) - assert response.data["assignedTo"]["type"] == "team" - assert GroupAssignee.objects.filter(group=group, team=other_team).exists() + assert response.status_code == 400, response.content + assert "without access to the project" in str(response.data) + assert not GroupAssignee.objects.filter(group=group, team=other_team).exists() def test_assign_team_when_open_membership_enabled(self) -> None: """ @@ -1486,6 +1483,20 @@ def test_assign_team_when_open_membership_enabled(self) -> None: assert response.data["assignedTo"]["type"] == "team" assert GroupAssignee.objects.filter(group=group, team=other_team).exists() + def test_unassign_in_closed_membership(self) -> None: + self.organization.flags.allow_joinleave = False + self.organization.save() + + group = self.create_group() + GroupAssignee.objects.assign(group, self.user) + self.login_as(user=self.user) + + url = f"{self.path}?id={group.id}" + response = self.client.put(url, data={"assignedTo": ""}) + + assert response.status_code == 200, response.content + assert not GroupAssignee.objects.filter(group=group).exists() + def test_discard(self) -> None: group1 = self.create_group(is_public=True) group2 = self.create_group(is_public=True)