From a6698d78dbbeac2dd02f5c0211baf356d04a511e Mon Sep 17 00:00:00 2001 From: Greg Pstrucha <875316+gricha@users.noreply.github.com> Date: Sat, 31 Oct 2026 21:23:39 -0700 Subject: [PATCH] ref(issues): Narrow request.user in issue endpoints Issue endpoints (hashes, similar issues, notes, issue views, supergroups, release commits) read the caller through `sentry.auth.principal`; personal issue-view writes use `require_user()`. - A service account can unmerge (was a 500: `ServiceAccountId` isn't serializable) and gets 404 editing notes it doesn't own; posting a note and saving issue views stay 403 `user-required` by design. Seer no longer receives a service-account id as the user. New tests cover unmerge and notes. - Legacy API keys get 401 instead of a 500 on starred issue views. - Users and agents see no change. --- src/sentry/issues/endpoints/group_hashes.py | 8 +++-- .../endpoints/group_integration_details.py | 5 ++- src/sentry/issues/endpoints/group_notes.py | 6 +++- .../issues/endpoints/group_notes_details.py | 15 ++++++--- .../group_similar_issues_embeddings.py | 5 +-- ...ation_group_search_view_details_starred.py | 14 ++++----- ...ization_group_search_view_starred_order.py | 3 +- .../organization_group_search_view_visit.py | 3 +- .../organization_group_search_views.py | 7 +++-- ...organization_group_search_views_starred.py | 5 +-- .../organization_issues_with_supergroups.py | 5 ++- tests/sentry/auth/test_principal.py | 2 +- .../issues/endpoints/test_group_hashes.py | 31 ++++++++++++++++++- .../endpoints/test_group_notes_details.py | 27 ++++++++++++++++ 14 files changed, 108 insertions(+), 28 deletions(-) diff --git a/src/sentry/issues/endpoints/group_hashes.py b/src/sentry/issues/endpoints/group_hashes.py index 6491739e3a4a..c33a52e8eaf6 100644 --- a/src/sentry/issues/endpoints/group_hashes.py +++ b/src/sentry/issues/endpoints/group_hashes.py @@ -27,10 +27,12 @@ from sentry.apidocs.examples.event_examples import EventExamples from sentry.apidocs.parameters import CursorQueryParam, GlobalParams, IssueParams from sentry.apidocs.utils import inline_sentry_response_serializer +from sentry.auth.principal import acting_user_id from sentry.constants import CELL_API_DEPRECATION_DATE from sentry.issues.endpoints.bases.group import GroupEndpoint from sentry.models.group import Group from sentry.models.grouphash import GroupHash +from sentry.service_accounts.principal import ServiceAccountPrincipal from sentry.services import eventstore from sentry.tasks.unmerge import start_unmerge from sentry.users.models.user import User @@ -160,7 +162,7 @@ def put(self, request: Request, group: Group) -> Response: ) start_unmerge.delay( - group.project_id, group.id, None, grouphashes, request.user.id if request.user else None + group.project_id, group.id, None, grouphashes, acting_user_id(request.user) ) return Response(status=202) @@ -169,7 +171,7 @@ def __handle_results( self, project_id: int, group_id: int, - user: User | RpcUser | AnonymousUser | None, + user: User | RpcUser | AnonymousUser | ServiceAccountPrincipal | None, full: str | bool, results: Sequence[dict[str, str]], ) -> list[GroupHashesResult]: @@ -190,7 +192,7 @@ def __handle_results( def __handle_result( self, - user: User | RpcUser | AnonymousUser | None, + user: User | RpcUser | AnonymousUser | ServiceAccountPrincipal | None, project_id: int, group_id: int, full: str | bool, diff --git a/src/sentry/issues/endpoints/group_integration_details.py b/src/sentry/issues/endpoints/group_integration_details.py index 799aca4acec5..be7d463a28d6 100644 --- a/src/sentry/issues/endpoints/group_integration_details.py +++ b/src/sentry/issues/endpoints/group_integration_details.py @@ -60,6 +60,7 @@ from sentry.models.group import Group from sentry.models.grouplink import GroupLink from sentry.models.organization import Organization +from sentry.service_accounts.principal import ServiceAccountPrincipal from sentry.shared_integrations.exceptions import ( IntegrationConfigurationError, IntegrationError, @@ -716,7 +717,9 @@ def delete( return Response(status=204) def _has_issue_feature( - self, organization: Organization, user: User | RpcUser | AnonymousUser + self, + organization: Organization, + user: User | RpcUser | AnonymousUser | ServiceAccountPrincipal, ) -> bool: has_issue_basic = features.has( "organizations:integrations-issue-basic", organization, actor=user diff --git a/src/sentry/issues/endpoints/group_notes.py b/src/sentry/issues/endpoints/group_notes.py index faa455d8e5b6..9083791a92eb 100644 --- a/src/sentry/issues/endpoints/group_notes.py +++ b/src/sentry/issues/endpoints/group_notes.py @@ -14,6 +14,7 @@ ) from sentry.api.serializers.rest_framework.group_notes import NoteSerializer from sentry.apidocs.utils import inline_sentry_response_serializer +from sentry.auth.principal import require_user from sentry.constants import CELL_API_DEPRECATION_DATE from sentry.issues.action_log.read_metrics import ( ActivityReadResult, @@ -92,7 +93,10 @@ def post(self, request: Request, group: Group) -> Response: data["mentions"] = [m.dict() for m in data["mentions"]] activity = create_comment( - group=group, user=request.user, data=data, source=resolve_action_source(request) + group=group, + user=require_user(request), + data=data, + source=resolve_action_source(request), ) if activity is None: return Response( diff --git a/src/sentry/issues/endpoints/group_notes_details.py b/src/sentry/issues/endpoints/group_notes_details.py index 3468b494cf41..cdb6f72074b7 100644 --- a/src/sentry/issues/endpoints/group_notes_details.py +++ b/src/sentry/issues/endpoints/group_notes_details.py @@ -16,6 +16,7 @@ from sentry.api.utils import to_valid_int_id from sentry.apidocs.constants import RESPONSE_NO_CONTENT from sentry.apidocs.utils import inline_sentry_response_serializer +from sentry.auth.principal import owned_by_acting_user, require_user from sentry.constants import CELL_API_DEPRECATION_DATE from sentry.issues.action_log.read_metrics import ( ActivityReadResult, @@ -55,7 +56,10 @@ def delete(self, request: Request, group: Group, note_id: str) -> Response: try: note = Activity.objects.get( - group=group, type=ActivityType.NOTE.value, user_id=request.user.id, id=note_id_int + owned_by_acting_user(request.user), + group=group, + type=ActivityType.NOTE.value, + id=note_id_int, ) except Activity.DoesNotExist: raise ResourceDoesNotExist @@ -63,7 +67,7 @@ def delete(self, request: Request, group: Group, note_id: str) -> Response: try: delete_comment( note, - user=request.user, + user=require_user(request), source=resolve_action_source(request), ) except Activity.DoesNotExist: @@ -90,7 +94,10 @@ def put(self, request: Request, group: Group, note_id: str) -> Response: try: note = Activity.objects.get( - group=group, type=ActivityType.NOTE.value, user_id=request.user.id, id=note_id_int + owned_by_acting_user(request.user), + group=group, + type=ActivityType.NOTE.value, + id=note_id_int, ) except Activity.DoesNotExist: raise ResourceDoesNotExist @@ -106,7 +113,7 @@ def put(self, request: Request, group: Group, note_id: str) -> Response: try: note = update_comment( note, - user=request.user, + user=require_user(request), data=dict(payload), source=resolve_action_source(request), ) diff --git a/src/sentry/issues/endpoints/group_similar_issues_embeddings.py b/src/sentry/issues/endpoints/group_similar_issues_embeddings.py index d345f68bd551..3dc6abc928b7 100644 --- a/src/sentry/issues/endpoints/group_similar_issues_embeddings.py +++ b/src/sentry/issues/endpoints/group_similar_issues_embeddings.py @@ -30,6 +30,7 @@ killswitch_enabled, stacktrace_exceeds_limits, ) +from sentry.service_accounts.principal import ServiceAccountPrincipal from sentry.users.models.user import User from sentry.utils.safe import get_path @@ -51,7 +52,7 @@ class GroupSimilarIssuesEmbeddingsEndpoint(GroupEndpoint): def get_formatted_results( self, similar_issues_data: Sequence[SeerSimilarIssueData], - user: User | AnonymousUser, + user: User | AnonymousUser | ServiceAccountPrincipal, group: Group, ) -> Sequence[tuple[Mapping[str, Any], Mapping[str, Any]] | None]: """ @@ -138,7 +139,7 @@ def get(self, request: Request, group: Group) -> Response: logger.info("Similar issues embeddings parameters", extra=similar_issues_params) viewer_context = SeerViewerContext( - organization_id=group.project.organization.id, user_id=request.user.id + organization_id=group.project.organization.id, user_id=acting_user_id(request.user) ) results, _model_used = get_similarity_data_from_seer( similar_issues_params, viewer_context=viewer_context diff --git a/src/sentry/issues/endpoints/organization_group_search_view_details_starred.py b/src/sentry/issues/endpoints/organization_group_search_view_details_starred.py index c141a9e6ca10..54f27a2b453f 100644 --- a/src/sentry/issues/endpoints/organization_group_search_view_details_starred.py +++ b/src/sentry/issues/endpoints/organization_group_search_view_details_starred.py @@ -7,6 +7,7 @@ from sentry.api.base import cell_silo_endpoint from sentry.api.bases.organization import OrganizationEndpoint, OrganizationPermission from sentry.api.utils import to_valid_int_id +from sentry.auth.principal import require_user from sentry.models.groupsearchview import GroupSearchView, GroupSearchViewVisibility from sentry.models.groupsearchviewstarred import GroupSearchViewStarred from sentry.models.organization import Organization @@ -43,6 +44,8 @@ def post(self, request: Request, organization: Organization, view_id: str) -> Re if not request.user.is_authenticated: return Response(status=status.HTTP_400_BAD_REQUEST) + user = require_user(request) + view_id_int = to_valid_int_id("view_id", view_id, raise_404=True) serializer = StarViewSerializer(data=request.data) @@ -57,8 +60,7 @@ def post(self, request: Request, organization: Organization, view_id: str) -> Re return Response(status=status.HTTP_404_NOT_FOUND) if not ( - view.user_id == request.user.id - or view.visibility == GroupSearchViewVisibility.ORGANIZATION + view.user_id == user.id or view.visibility == GroupSearchViewVisibility.ORGANIZATION ): return Response(status=status.HTTP_404_NOT_FOUND) @@ -66,17 +68,15 @@ def post(self, request: Request, organization: Organization, view_id: str) -> Re insert_position = ( serializer.validated_data["position"] if "position" in serializer.validated_data - else GroupSearchViewStarred.objects.num_starred_views(organization, request.user.id) + else GroupSearchViewStarred.objects.num_starred_views(organization, user.id) ) if GroupSearchViewStarred.objects.insert_starred_view( - organization, request.user.id, view, insert_position + organization, user.id, view, insert_position ): return Response(status=status.HTTP_200_OK) else: - if GroupSearchViewStarred.objects.delete_starred_view( - organization, request.user.id, view - ): + if GroupSearchViewStarred.objects.delete_starred_view(organization, user.id, view): return Response(status=status.HTTP_200_OK) return Response(status=status.HTTP_204_NO_CONTENT) diff --git a/src/sentry/issues/endpoints/organization_group_search_view_starred_order.py b/src/sentry/issues/endpoints/organization_group_search_view_starred_order.py index 03d4ce4d19bb..d7ac06837e6b 100644 --- a/src/sentry/issues/endpoints/organization_group_search_view_starred_order.py +++ b/src/sentry/issues/endpoints/organization_group_search_view_starred_order.py @@ -7,6 +7,7 @@ from sentry.api.api_publish_status import ApiPublishStatus from sentry.api.base import cell_silo_endpoint from sentry.api.bases.organization import OrganizationEndpoint, OrganizationPermission +from sentry.auth.principal import require_user from sentry.models.groupsearchviewstarred import GroupSearchViewStarred from sentry.models.organization import Organization @@ -50,7 +51,7 @@ def put(self, request: Request, organization: Organization) -> Response: with transaction.atomic(using=router.db_for_write(GroupSearchViewStarred)): GroupSearchViewStarred.objects.reorder_starred_views( organization=organization, - user_id=request.user.id, + user_id=require_user(request).id, new_view_positions=view_ids, ) except IntegrityError as e: diff --git a/src/sentry/issues/endpoints/organization_group_search_view_visit.py b/src/sentry/issues/endpoints/organization_group_search_view_visit.py index 53428443006e..cd2448014827 100644 --- a/src/sentry/issues/endpoints/organization_group_search_view_visit.py +++ b/src/sentry/issues/endpoints/organization_group_search_view_visit.py @@ -8,6 +8,7 @@ from sentry.api.base import cell_silo_endpoint from sentry.api.bases.organization import OrganizationEndpoint, OrganizationPermission from sentry.api.utils import to_valid_int_id +from sentry.auth.principal import require_user from sentry.models.groupsearchview import GroupSearchView from sentry.models.groupsearchviewlastvisited import GroupSearchViewLastVisited from sentry.models.organization import Organization @@ -43,7 +44,7 @@ def post(self, request: Request, organization: Organization, view_id: str) -> Re # Create or update the last_visited timestamp GroupSearchViewLastVisited.objects.update_or_create( organization=organization, - user_id=request.user.id, + user_id=require_user(request).id, group_search_view=view, defaults={"last_visited": timezone.now()}, ) diff --git a/src/sentry/issues/endpoints/organization_group_search_views.py b/src/sentry/issues/endpoints/organization_group_search_views.py index 689d3184fd47..c305a175f1a3 100644 --- a/src/sentry/issues/endpoints/organization_group_search_views.py +++ b/src/sentry/issues/endpoints/organization_group_search_views.py @@ -29,7 +29,7 @@ from sentry.apidocs.parameters import CursorQueryParam, GlobalParams from sentry.apidocs.response_types import ValidationErrorResponse, as_validation_errors from sentry.apidocs.utils import inline_sentry_response_serializer -from sentry.auth.principal import owned_by_acting_user +from sentry.auth.principal import owned_by_acting_user, require_user from sentry.models.groupsearchview import GroupSearchView, GroupSearchViewVisibility from sentry.models.groupsearchviewlastvisited import GroupSearchViewLastVisited from sentry.models.groupsearchviewstarred import GroupSearchViewStarred @@ -266,10 +266,11 @@ def post( for project in projects: self.check_object_permissions(request, project) + user = require_user(request) # Create the new view view = GroupSearchView.objects.create( organization=organization, - user_id=request.user.id, + user_id=user.id, name=validated_data["name"], query=validated_data["query"], query_sort=validated_data["querySort"], @@ -283,7 +284,7 @@ def post( if validated_data.get("starred"): GroupSearchViewStarred.objects.insert_starred_view( organization=organization, - user_id=request.user.id, + user_id=user.id, view=view, ) diff --git a/src/sentry/issues/endpoints/organization_group_search_views_starred.py b/src/sentry/issues/endpoints/organization_group_search_views_starred.py index f74e3f63dae0..aac526e69cfa 100644 --- a/src/sentry/issues/endpoints/organization_group_search_views_starred.py +++ b/src/sentry/issues/endpoints/organization_group_search_views_starred.py @@ -7,6 +7,7 @@ from sentry.api.bases.organization import OrganizationEndpoint, OrganizationPermission from sentry.api.serializers import serialize from sentry.api.serializers.models.groupsearchviewstarred import GroupSearchViewStarredSerializer +from sentry.auth.principal import require_user from sentry.models.groupsearchviewstarred import GroupSearchViewStarred from sentry.models.organization import Organization @@ -30,9 +31,9 @@ def get(self, request: Request, organization: Organization) -> Response: Retrieve a list of starred views for the current organization member. """ - assert request.user.id is not None + user = require_user(request) starred_views = GroupSearchViewStarred.objects.filter( - organization=organization, user_id=request.user.id + organization=organization, user_id=user.id ).select_related("group_search_view") return self.paginate( diff --git a/src/sentry/issues/endpoints/organization_issues_with_supergroups.py b/src/sentry/issues/endpoints/organization_issues_with_supergroups.py index b598bcc36fee..9589a985e012 100644 --- a/src/sentry/issues/endpoints/organization_issues_with_supergroups.py +++ b/src/sentry/issues/endpoints/organization_issues_with_supergroups.py @@ -16,6 +16,7 @@ from sentry.api.helpers.group_index import calculate_stats_period from sentry.api.helpers.group_index.validators import ValidationError from sentry.api.utils import get_date_range_from_stats_period, handle_query_errors +from sentry.auth.principal import acting_user_id from sentry.issues.endpoints.organization_group_index import ( ERR_INVALID_STATS_PERIOD, search_and_serialize_issues, @@ -94,7 +95,9 @@ def get(self, request: Request, organization: Organization) -> Response: # breaking the stream. try: supergroup_data = get_supergroups_by_group_ids( - organization, [int(g["id"]) for g in groups], user_id=request.user.id + organization, + [int(g["id"]) for g in groups], + user_id=acting_user_id(request.user), ) except SeerApiError: logger.exception("issues_with_supergroups.seer_fetch_failed") diff --git a/tests/sentry/auth/test_principal.py b/tests/sentry/auth/test_principal.py index 0e6cdee21cd0..f2a5a228e022 100644 --- a/tests/sentry/auth/test_principal.py +++ b/tests/sentry/auth/test_principal.py @@ -274,7 +274,7 @@ def _is_request_user_type_check(node: ast.AST) -> bool: # Lower these as callers move off. Exact, so the baseline can't drift back up. -REQUEST_USER_ID_BASELINE = 232 +REQUEST_USER_ID_BASELINE = 217 REQUEST_USER_TYPE_CHECK_BASELINE = 31 diff --git a/tests/sentry/issues/endpoints/test_group_hashes.py b/tests/sentry/issues/endpoints/test_group_hashes.py index b81bcc450b0a..1e91758e27bf 100644 --- a/tests/sentry/issues/endpoints/test_group_hashes.py +++ b/tests/sentry/issues/endpoints/test_group_hashes.py @@ -1,7 +1,7 @@ from unittest.mock import patch from urllib.parse import urlencode -from django.test import RequestFactory +from django.test import RequestFactory, override_settings from rest_framework.request import Request from sentry.api.serializers.models.event import FULL_PAYLOAD_MAX_PER_PAGE @@ -10,6 +10,8 @@ from sentry.models.grouphash import GroupHash from sentry.testutils.cases import APITestCase, SnubaTestCase from sentry.testutils.helpers.datetime import before_now +from sentry.testutils.helpers.features import with_feature +from sentry.viewer_context import Actor, ActorType, ViewerContext, encode_viewer_context class GroupHashesTest(APITestCase, SnubaTestCase): @@ -256,6 +258,33 @@ def test_unmerge_put_member(self) -> None: tags={"platform": "javascript", "sdk": "sentry.javascript.nextjs"}, ) + @override_settings(SEER_API_SHARED_SECRET="service-account-secret") + @with_feature("organizations:service-accounts") + def test_unmerge_service_account(self) -> None: + group = self.create_group(platform="javascript") + GroupHash.objects.create(project=group.project, group=group, hash="a" * 32) + service_account = self.create_service_account( + organization=self.organization, + scope_list=["event:write"], + project_ids=[group.project.id], + ) + viewer_context = encode_viewer_context( + ViewerContext( + actor=Actor(type=ActorType.SERVICE_ACCOUNT, actor_id=service_account.id), + organization_id=self.organization.id, + ), + key="service-account-secret", + ) + url = "?".join( + [ + f"/api/0/organizations/{self.organization.slug}/issues/{group.id}/hashes/", + urlencode({"id": ["a" * 32]}, True), + ] + ) + + response = self.client.put(url, format="json", HTTP_X_VIEWER_CONTEXT=viewer_context) + assert response.status_code == 202, response.content + def test_unmerge_conflict(self) -> None: self.login_as(user=self.user) diff --git a/tests/sentry/issues/endpoints/test_group_notes_details.py b/tests/sentry/issues/endpoints/test_group_notes_details.py index 3e0a77934e6a..73b155151b97 100644 --- a/tests/sentry/issues/endpoints/test_group_notes_details.py +++ b/tests/sentry/issues/endpoints/test_group_notes_details.py @@ -3,6 +3,7 @@ from unittest.mock import MagicMock, patch import responses +from django.test import override_settings from sentry.api.serializers.models.groupactionlogentry import get_serialized_activity_items from sentry.hybridcloud.models.outbox import outbox_context @@ -22,6 +23,7 @@ from sentry.testutils.helpers.features import with_feature from sentry.testutils.silo import assume_test_silo_mode from sentry.types.activity import ActivityType +from sentry.viewer_context import Actor, ActorType, ViewerContext, encode_viewer_context # The default Activity fixture represents a comment created before GALE dual writes. @@ -152,6 +154,31 @@ def test_delete_another_users_comment_with_write_token(self) -> None: assert response.status_code == 404, response.content assert Activity.objects.filter(id=self.activity.id).exists() + @override_settings(SEER_API_SHARED_SECRET="service-account-secret") + @with_feature("organizations:service-accounts") + def test_service_account_has_no_comments_to_edit_or_delete(self) -> None: + service_account = self.create_service_account( + organization=self.organization, + scope_list=["event:write", "event:admin"], + project_ids=[self.project.id], + ) + viewer_context = encode_viewer_context( + ViewerContext( + actor=Actor(type=ActorType.SERVICE_ACCOUNT, actor_id=service_account.id), + organization_id=self.organization.id, + ), + key="service-account-secret", + ) + + response = self.client.put( + self.url, {"text": "edited"}, format="json", HTTP_X_VIEWER_CONTEXT=viewer_context + ) + assert response.status_code == 404, response.content + + response = self.client.delete(self.url, format="json", HTTP_X_VIEWER_CONTEXT=viewer_context) + assert response.status_code == 404, response.content + assert Activity.objects.filter(id=self.activity.id).exists() + def test_delete_comment_and_subscription(self) -> None: """Test that if a user deletes their comment on an issue, we delete the subscription too""" self.login_as(user=self.user)