From 89162bcba97fc8fbe2ff8b73171e04db2d39002a Mon Sep 17 00:00:00 2001 From: Christinarlong <60594860+Christinarlong@users.noreply.github.com> Date: Fri, 17 Jan 2025 13:40:09 -0800 Subject: [PATCH] ref(sentry apps): Use new error design for sentry app errors (#83355) --- .../sentry_apps/api/bases/sentryapps.py | 61 +++++++++---------- .../external_issues/external_issue_creator.py | 5 +- .../external_issues/issue_link_creator.py | 3 +- src/sentry/sentry_apps/utils/errors.py | 46 +++++++------- .../sentry_apps/api/bases/test_sentryapps.py | 14 ++--- .../api/endpoints/test_sentry_app_details.py | 39 ++++++++++-- .../api/endpoints/test_sentry_apps.py | 14 +++-- 7 files changed, 105 insertions(+), 77 deletions(-) diff --git a/src/sentry/sentry_apps/api/bases/sentryapps.py b/src/sentry/sentry_apps/api/bases/sentryapps.py index 71a069307dfc46..cb2c509ed45829 100644 --- a/src/sentry/sentry_apps/api/bases/sentryapps.py +++ b/src/sentry/sentry_apps/api/bases/sentryapps.py @@ -6,18 +6,16 @@ from typing import Any import sentry_sdk -from rest_framework.exceptions import PermissionDenied from rest_framework.permissions import BasePermission from rest_framework.request import Request from rest_framework.response import Response from sentry.api.authentication import ClientIdSecretAuthentication from sentry.api.base import Endpoint -from sentry.api.exceptions import ResourceDoesNotExist from sentry.api.permissions import SentryPermission, StaffPermissionMixin from sentry.auth.staff import is_active_staff from sentry.auth.superuser import is_active_superuser, superuser_has_permission -from sentry.coreapi import APIError, APIUnauthorized +from sentry.coreapi import APIError from sentry.integrations.api.bases.integration import PARANOID_GET from sentry.middleware.stats import add_request_metric_tags from sentry.models.organization import OrganizationStatus @@ -103,10 +101,9 @@ def has_object_permission(self, request: Request, view, context: RpcUserOrganiza # User must be a part of the Org they're trying to create the app in. if context.organization.status != OrganizationStatus.ACTIVE or not context.member: - raise SentryAppIntegratorError( - APIUnauthorized( - "User must be a part of the Org they're trying to create the app in" - ) + raise SentryAppError( + message="User must be a part of the Org they're trying to create the app in", + status_code=401, ) assert request.method, "method must be present in request to get permissions" @@ -131,7 +128,12 @@ def handle_exception_with_details(self, request, exc, handler_context=None, scop def _handle_sentry_app_exception(self, exception: Exception): if isinstance(exception, SentryAppIntegratorError) or isinstance(exception, SentryAppError): - response = Response({"detail": str(exception)}, status=exception.status_code) + response_body: dict[str, Any] = {"detail": exception.message} + + if public_context := exception.public_context: + response_body.update({"context": public_context}) + + response = Response(response_body, status=exception.status_code) response.exception = True return response @@ -154,7 +156,7 @@ def _get_organization_slug(self, request: Request): organization_slug = request.data.get("organization") if not organization_slug or not isinstance(organization_slug, str): error_message = "Please provide a valid value for the 'organization' field." - raise SentryAppError(ResourceDoesNotExist(error_message)) + raise SentryAppError(message=error_message, status_code=404) return organization_slug def _get_organization_for_superuser_or_staff( @@ -166,7 +168,7 @@ def _get_organization_for_superuser_or_staff( if context is None: error_message = f"Organization '{organization_slug}' does not exist." - raise SentryAppError(ResourceDoesNotExist(error_message)) + raise SentryAppError(message=error_message, status_code=404) return context @@ -178,7 +180,7 @@ def _get_organization_for_user( ) if context is None or context.member is None: error_message = f"User does not belong to the '{organization_slug}' organization." - raise SentryAppIntegratorError(PermissionDenied(to_single_line_str(error_message))) + raise SentryAppError(message=to_single_line_str(error_message), status_code=403) return context def _get_org_context(self, request: Request) -> RpcUserOrganizationContext: @@ -260,10 +262,13 @@ def has_object_permission(self, request: Request, view, sentry_app: RpcSentryApp # if app is unpublished, user must be in the Org who owns the app. if not sentry_app.is_published: if not any(sentry_app.owner_id == org.id for org in organizations): - raise SentryAppIntegratorError( - APIUnauthorized( - "User must be in the app owner's organization for unpublished apps" - ) + raise SentryAppError( + message="User must be in the app owner's organization for unpublished apps", + status_code=403, + public_context={ + "integration": sentry_app.slug, + "user_organizations": [org.slug for org in organizations], + }, ) # TODO(meredith): make a better way to allow for public @@ -299,9 +304,7 @@ def convert_args( try: sentry_app = SentryApp.objects.get(slug__id_or_slug=sentry_app_id_or_slug) except SentryApp.DoesNotExist: - raise SentryAppIntegratorError( - ResourceDoesNotExist("Could not find the requested sentry app"), status_code=404 - ) + raise SentryAppError(message="Could not find the requested sentry app", status_code=404) self.check_object_permissions(request, sentry_app) @@ -320,9 +323,7 @@ def convert_args( else: sentry_app = app_service.get_sentry_app_by_slug(slug=sentry_app_id_or_slug) if sentry_app is None: - raise SentryAppIntegratorError( - ResourceDoesNotExist("Could not find the requested sentry app"), status_code=404 - ) + raise SentryAppError(message="Could not find the requested sentry app", status_code=404) self.check_object_permissions(request, sentry_app) @@ -353,8 +354,10 @@ def has_object_permission(self, request: Request, view, organization): else () ) if not any(organization.id == org.id for org in organizations): - raise SentryAppIntegratorError( - APIUnauthorized("User must belong to the given organization"), status_code=403 + raise SentryAppError( + message="User must belong to the given organization", + status_code=403, + public_context={"user_organizations": [org.slug for org in organizations]}, ) assert request.method, "method must be present in request to get permissions" return ensure_scoped_permission(request, self.scope_map.get(request.method)) @@ -379,9 +382,7 @@ def convert_args(self, request: Request, organization_id_or_slug, *args, **kwarg ) if organization is None: - raise SentryAppIntegratorError( - ResourceDoesNotExist("Could not find requested organization"), status_code=404 - ) + raise SentryAppError(message="Could not find requested organization", status_code=404) self.check_object_permissions(request, organization) kwargs["organization"] = organization @@ -437,9 +438,7 @@ def has_object_permission(self, request: Request, view, installation): or not org_context.member or org_context.organization.status != OrganizationStatus.ACTIVE ): - raise SentryAppIntegratorError( - ResourceDoesNotExist("Given organization is not valid"), status_code=404 - ) + raise SentryAppError(message="Given organization is not valid", status_code=404) assert request.method, "method must be present in request to get permissions" return ensure_scoped_permission(request, self.scope_map.get(request.method)) @@ -452,8 +451,8 @@ def convert_args(self, request: Request, uuid, *args, **kwargs): installations = app_service.get_many(filter=dict(uuids=[uuid])) installation = installations[0] if installations else None if installation is None: - raise SentryAppIntegratorError( - ResourceDoesNotExist("Could not find given sentry app installation"), + raise SentryAppError( + message="Could not find given sentry app installation", status_code=404, ) diff --git a/src/sentry/sentry_apps/external_issues/external_issue_creator.py b/src/sentry/sentry_apps/external_issues/external_issue_creator.py index 31e3f42c674415..428a2fc92246c4 100644 --- a/src/sentry/sentry_apps/external_issues/external_issue_creator.py +++ b/src/sentry/sentry_apps/external_issues/external_issue_creator.py @@ -46,4 +46,7 @@ def run(self) -> PlatformExternalIssue: "sentry_app_slug": self.install.sentry_app.slug, }, ) - raise SentryAppSentryError(e) from e + raise SentryAppSentryError( + message="Failed to create external issue obj", + webhook_context={"error": str(e)}, + ) from e diff --git a/src/sentry/sentry_apps/external_issues/issue_link_creator.py b/src/sentry/sentry_apps/external_issues/issue_link_creator.py index fb0dc0b5dacc6a..94a24e5dcbb87c 100644 --- a/src/sentry/sentry_apps/external_issues/issue_link_creator.py +++ b/src/sentry/sentry_apps/external_issues/issue_link_creator.py @@ -3,7 +3,6 @@ from django.db import router, transaction -from sentry.coreapi import APIUnauthorized from sentry.models.group import Group from sentry.sentry_apps.external_issues.external_issue_creator import ExternalIssueCreator from sentry.sentry_apps.external_requests.issue_link_requester import IssueLinkRequester @@ -33,7 +32,7 @@ def run(self) -> PlatformExternalIssue: def _verify_action(self) -> None: if self.action not in VALID_ACTIONS: - raise SentryAppSentryError(APIUnauthorized(f"Invalid action '{self.action}'")) + raise SentryAppSentryError(message=f"Invalid action: {self.action}", status_code=401) def _make_external_request(self) -> dict[str, Any]: response = IssueLinkRequester( diff --git a/src/sentry/sentry_apps/utils/errors.py b/src/sentry/sentry_apps/utils/errors.py index 6c9cdc7186148c..e2dc665fc26469 100644 --- a/src/sentry/sentry_apps/utils/errors.py +++ b/src/sentry/sentry_apps/utils/errors.py @@ -1,4 +1,5 @@ from enum import Enum +from typing import Any class SentryAppErrorType(Enum): @@ -7,43 +8,38 @@ class SentryAppErrorType(Enum): SENTRY = "sentry" -# Represents a user/client error that occured during a Sentry App process -class SentryAppError(Exception): - error_type = SentryAppErrorType.CLIENT - status_code = 400 +class SentryAppBaseError(Exception): + error_type: SentryAppErrorType + status_code: int def __init__( self, - error: Exception | None = None, + message: str, status_code: int | None = None, + public_context: dict[str, Any] | None = None, + webhook_context: dict[str, Any] | None = None, ) -> None: - if status_code: - self.status_code = status_code + self.status_code = status_code or self.status_code + # Info that gets sent only to the integrator via webhook + self.public_context = public_context or {} + # Info that gets sent to the end user via endpoint Response AND sent to integrator + self.webhook_context = webhook_context or {} + self.message = message + + +# Represents a user/client error that occured during a Sentry App process +class SentryAppError(SentryAppBaseError): + error_type = SentryAppErrorType.CLIENT + status_code = 400 # Represents an error caused by a 3p integrator during a Sentry App process -class SentryAppIntegratorError(Exception): +class SentryAppIntegratorError(SentryAppBaseError): error_type = SentryAppErrorType.INTEGRATOR status_code = 400 - def __init__( - self, - error: Exception | None = None, - status_code: int | None = None, - ) -> None: - if status_code: - self.status_code = status_code - # Represents an error that's our (sentry's) fault -class SentryAppSentryError(Exception): +class SentryAppSentryError(SentryAppBaseError): error_type = SentryAppErrorType.SENTRY status_code = 500 - - def __init__( - self, - error: Exception | None = None, - status_code: int | None = None, - ) -> None: - if status_code: - self.status_code = status_code diff --git a/tests/sentry/sentry_apps/api/bases/test_sentryapps.py b/tests/sentry/sentry_apps/api/bases/test_sentryapps.py index 54b72a471143d4..aeb444252c3d72 100644 --- a/tests/sentry/sentry_apps/api/bases/test_sentryapps.py +++ b/tests/sentry/sentry_apps/api/bases/test_sentryapps.py @@ -14,7 +14,7 @@ SentryAppPermission, add_integration_platform_metric_tag, ) -from sentry.sentry_apps.utils.errors import SentryAppIntegratorError +from sentry.sentry_apps.utils.errors import SentryAppError from sentry.testutils.cases import TestCase from sentry.testutils.helpers.options import override_options from sentry.testutils.silo import control_silo_test @@ -43,7 +43,7 @@ def test_request_user_is_not_app_owner_fails(self): request=self.make_request(user=non_owner, method="GET"), endpoint=self.endpoint ) - with pytest.raises(SentryAppIntegratorError): + with pytest.raises(SentryAppError): self.permission.has_object_permission(self.request, None, self.sentry_app) def test_has_permission(self): @@ -83,7 +83,7 @@ def test_superuser_has_permission_read_only(self): request._request.method = "POST" - with pytest.raises(SentryAppIntegratorError): + with pytest.raises(SentryAppError): self.permission.has_object_permission(request, None, self.sentry_app) @override_options({"superuser.read-write.ga-rollout": True}) @@ -143,7 +143,7 @@ def test_retrieves_sentry_app(self): assert kwargs["sentry_app"].id == self.sentry_app.id def test_raises_when_sentry_app_not_found(self): - with pytest.raises(SentryAppIntegratorError): + with pytest.raises(SentryAppError): self.endpoint.convert_args(self.request, "notanapp") @@ -181,7 +181,7 @@ def test_request_user_not_in_organization(self): self.make_request(user=user, method="GET"), endpoint=self.endpoint ) - with pytest.raises(SentryAppIntegratorError): + with pytest.raises(SentryAppError): self.permission.has_object_permission(request, None, self.installation) def test_superuser_has_permission(self): @@ -206,7 +206,7 @@ def test_superuser_has_permission_read_only(self): assert self.permission.has_object_permission(request, None, self.installation) request._request.method = "POST" - with pytest.raises(SentryAppIntegratorError): + with pytest.raises(SentryAppError): self.permission.has_object_permission(request, None, self.installation) @override_options({"superuser.read-write.ga-rollout": True}) @@ -242,7 +242,7 @@ def test_retrieves_installation(self): assert kwargs["installation"].id == self.installation.id def test_raises_when_sentry_app_not_found(self): - with pytest.raises(SentryAppIntegratorError): + with pytest.raises(SentryAppError): self.endpoint.convert_args(self.request, "1234") diff --git a/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_details.py b/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_details.py index 4b30fb99652183..644a23b184d412 100644 --- a/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_details.py +++ b/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_details.py @@ -85,10 +85,26 @@ def test_retrieving_internal_integrations_as_org_member(self): def test_internal_integrations_are_not_public(self): # User not in Org who owns the Integration self.login_as(self.create_user()) - self.get_error_response(self.internal_integration.slug, status_code=400) + response = self.get_error_response(self.internal_integration.slug, status_code=403) + assert ( + response.data["detail"] + == "User must be in the app owner's organization for unpublished apps" + ) + assert response.data["context"] == { + "integration": self.internal_integration.slug, + "user_organizations": [], + } def test_users_do_not_see_unowned_unpublished_apps(self): - self.get_error_response(self.unowned_unpublished_app.slug, status_code=400) + response = self.get_error_response(self.unowned_unpublished_app.slug, status_code=403) + assert ( + response.data["detail"] + == "User must be in the app owner's organization for unpublished apps" + ) + assert response.data["context"] == { + "integration": self.unowned_unpublished_app.slug, + "user_organizations": [self.organization.slug], + } @control_silo_test @@ -440,13 +456,22 @@ def test_cannot_update_features_published_app_permissions(self): def test_cannot_update_non_owned_apps(self): app = self.create_sentry_app(name="SampleApp", organization=self.create_organization()) - self.get_error_response( + response = self.get_error_response( app.slug, name="NewName", webhookUrl="https://newurl.com", - status_code=400, + status_code=403, ) + assert ( + response.data["detail"] + == "User must be in the app owner's organization for unpublished apps" + ) + assert response.data["context"] == { + "integration": app.slug, + "user_organizations": [self.organization.slug], + } + def test_superuser_can_update_popularity(self): self.login_as(user=self.superuser, superuser=True) app = self.create_sentry_app(name="SampleApp", organization=self.organization) @@ -741,12 +766,16 @@ def test_staff_cannot_delete_unpublished_app(self): self.login_as(staff_user, staff=False) response = self.get_error_response( self.unpublished_app.slug, - status_code=400, + status_code=403, ) assert ( response.data["detail"] == "User must be in the app owner's organization for unpublished apps" ) + assert response.data["context"] == { + "integration": self.unpublished_app.slug, + "user_organizations": [], + } assert not AuditLogEntry.objects.filter( event=audit_log.get_event_id("SENTRY_APP_REMOVE") diff --git a/tests/sentry/sentry_apps/api/endpoints/test_sentry_apps.py b/tests/sentry/sentry_apps/api/endpoints/test_sentry_apps.py index 54920c1dab38b8..15fff13603fd6d 100644 --- a/tests/sentry/sentry_apps/api/endpoints/test_sentry_apps.py +++ b/tests/sentry/sentry_apps/api/endpoints/test_sentry_apps.py @@ -402,7 +402,7 @@ def setUp(self): def test_staff_cannot_create_app(self): """We do not allow staff to create Sentry Apps b/c this cannot be done in _admin.""" self.login_as(self.staff_user, staff=True) - response = self.get_error_response(**self.get_data(), status_code=400) + response = self.get_error_response(**self.get_data(), status_code=401) assert ( response.data["detail"] == "User must be a part of the Org they're trying to create the app in" @@ -412,8 +412,10 @@ def test_superuser_cannot_create_app_in_nonexistent_organization(self): sentry_app = self.create_internal_integration(name="Foo Bar") data = self.get_data(name=sentry_app.name, organization="some-non-existent-org") - response = self.get_error_response(**data, status_code=400) - assert response.data == {"detail": "Organization 'some-non-existent-org' does not exist."} + response = self.get_error_response(**data, status_code=404) + assert response.data == { + "detail": "Organization 'some-non-existent-org' does not exist.", + } def test_superuser_can_create_with_popularity(self): response = self.get_success_response( @@ -543,7 +545,7 @@ def test_cannot_create_app_without_organization(self): sentry_app = self.create_internal_integration(name="Foo Bar") data = self.get_data(name=sentry_app.name, organization=None) - response = self.get_error_response(**data, status_code=400) + response = self.get_error_response(**data, status_code=404) assert response.data == { "detail": "Please provide a valid value for the 'organization' field.", } @@ -554,7 +556,7 @@ def test_cannot_create_app_in_alien_organization(self): sentry_app = self.create_internal_integration(name="Foo Bar") data = self.get_data(name=sentry_app.name, organization=other_organization.slug) - response = self.get_error_response(**data, status_code=400) + response = self.get_error_response(**data, status_code=403) assert response.data["detail"].startswith("User does not belong to") def test_user_cannot_create_app_in_nonexistent_organization(self): @@ -562,7 +564,7 @@ def test_user_cannot_create_app_in_nonexistent_organization(self): sentry_app = self.create_internal_integration(name="Foo Bar") data = self.get_data(name=sentry_app.name, organization="some-non-existent-org") - response = self.get_error_response(**data, status_code=400) + response = self.get_error_response(**data, status_code=403) assert response.data["detail"].startswith("User does not belong to") def test_nonsuperuser_cannot_create_with_popularity(self):