ref(sentry apps): Use new error design for sentry app errors (#83355)

Author: Christinarlong

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,
             )
 

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

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(

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

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")
 
 

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")