adjust tests for new error handler & update base endpoint classes to …

…use new error types

src/sentry/sentry_apps/api/bases/sentryapps.py

@@ -5,19 +5,19 @@
 from functools import wraps
 from typing import Any
 
-from django.http import Http404
-from rest_framework.exceptions import PermissionDenied
+from rest_framework.exceptions import APIException, PermissionDenied
 from rest_framework.permissions import BasePermission
 from rest_framework.request import Request
 from rest_framework.response import Response
 from rest_framework.serializers import ValidationError
 
 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
+from sentry.coreapi import APIError, APIUnauthorized
 from sentry.integrations.api.bases.integration import PARANOID_GET
 from sentry.middleware.stats import add_request_metric_tags
 from sentry.models.organization import OrganizationStatus
@@ -27,6 +27,7 @@
 )
 from sentry.sentry_apps.models.sentry_app import SentryApp
 from sentry.sentry_apps.services.app import RpcSentryApp, app_service
+from sentry.sentry_apps.utils.errors import SentryAppError, SentryAppIntegratorError
 from sentry.users.models.user import User
 from sentry.users.services.user import RpcUser
 from sentry.users.services.user.service import user_service
@@ -52,7 +53,17 @@ def wrapped(self, *args, **kwargs):
 def _handle_sentry_app_exception(exception: Exception):
     # If the error_type attr exists we know the error is one of SentryAppError or SentryAppIntegratorError
     if hasattr(exception, "error_type"):
-        response = Response({"error": str(exception)}, status=exception.status_code)
+
+        # hacky! Children classes of APIExceotion don't stringify nicely since we just want the message
+        if isinstance(exception.__cause__, APIException):
+            wrapped_error = exception.__cause__
+            wrapped_error_message = str(wrapped_error.detail[0]) or wrapped_error.default_detail
+            response = Response(
+                {"error": wrapped_error_message},
+                status=exception.status_code,
+            )
+        else:
+            response = Response({"error": str(exception)}, status=exception.status_code)
         response.exception = True
         return response
 
@@ -109,7 +120,11 @@ 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 Http404
+            raise SentryAppIntegratorError(
+                APIUnauthorized(
+                    "User must be a part of the Org they're trying to create the app in"
+                )
+            )
 
         assert request.method, "method must be present in request to get permissions"
         return ensure_scoped_permission(request, self.scope_map.get(request.method))
@@ -134,7 +149,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 ValidationError({"organization": to_single_line_str(error_message)})
+            raise SentryAppError from ValidationError(to_single_line_str(error_message))
         return organization_slug
 
     def _get_organization_for_superuser_or_staff(
@@ -146,7 +161,7 @@ def _get_organization_for_superuser_or_staff(
 
         if context is None:
             error_message = f"Organization '{organization_slug}' does not exist."
-            raise ValidationError({"organization": to_single_line_str(error_message)})
+            raise SentryAppError from ValidationError(to_single_line_str(error_message))
 
         return context
 
@@ -158,7 +173,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 PermissionDenied(to_single_line_str(error_message))
+            raise SentryAppIntegratorError(PermissionDenied(to_single_line_str(error_message)))
         return context
 
     def _get_org_context(self, request: Request) -> RpcUserOrganizationContext:
@@ -245,7 +260,11 @@ 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 Http404
+                raise SentryAppIntegratorError(
+                    APIUnauthorized(
+                        "User must be in the app owner's organization for unpublished apps"
+                    )
+                )
 
         # TODO(meredith): make a better way to allow for public
         # endpoints. we can't use ensure_scoped_permission now
@@ -280,7 +299,9 @@ def convert_args(
         try:
             sentry_app = SentryApp.objects.get(slug__id_or_slug=sentry_app_id_or_slug)
         except SentryApp.DoesNotExist:
-            raise Http404
+            raise SentryAppIntegratorError(
+                ResourceDoesNotExist("Could not find the requested sentry app"), status_code=404
+            )
 
         self.check_object_permissions(request, sentry_app)
 
@@ -304,7 +325,9 @@ 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 Http404
+            raise SentryAppIntegratorError(
+                ResourceDoesNotExist("Could not find the requested sentry app"), status_code=404
+            )
 
         self.check_object_permissions(request, sentry_app)
 
@@ -340,8 +363,9 @@ def has_object_permission(self, request: Request, view, organization):
             else ()
         )
         if not any(organization.id == org.id for org in organizations):
-            raise Http404
-
+            raise SentryAppIntegratorError(
+                APIUnauthorized("User must belong to the given organization"), status_code=403
+            )
         assert request.method, "method must be present in request to get permissions"
         return ensure_scoped_permission(request, self.scope_map.get(request.method))
 
@@ -365,7 +389,9 @@ def convert_args(self, request: Request, organization_id_or_slug, *args, **kwarg
             )
 
         if organization is None:
-            raise Http404
+            raise SentryAppIntegratorError(
+                ResourceDoesNotExist("Could not find requested organization"), status_code=404
+            )
         self.check_object_permissions(request, organization)
 
         kwargs["organization"] = organization
@@ -426,7 +452,9 @@ def has_object_permission(self, request: Request, view, installation):
             or not org_context.member
             or org_context.organization.status != OrganizationStatus.ACTIVE
         ):
-            raise Http404
+            raise SentryAppIntegratorError(
+                ResourceDoesNotExist("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))
@@ -439,7 +467,10 @@ 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 Http404
+            raise SentryAppIntegratorError(
+                ResourceDoesNotExist("Could not find given sentry app installation"),
+                status_code=404,
+            )
 
         self.check_object_permissions(request, installation)
 

tests/sentry/sentry_apps/api/bases/test_sentryapps.py

@@ -3,7 +3,6 @@
 
 import pytest
 from django.contrib.auth.models import AnonymousUser
-from django.http import Http404
 from django.test.utils import override_settings
 from rest_framework.request import Request
 
@@ -15,6 +14,7 @@
     SentryAppPermission,
     add_integration_platform_metric_tag,
 )
+from sentry.sentry_apps.utils.errors import SentryAppIntegratorError
 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(Http404):
+        with pytest.raises(SentryAppIntegratorError):
             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(Http404):
+        with pytest.raises(SentryAppIntegratorError):
             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(Http404):
+        with pytest.raises(SentryAppIntegratorError):
             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(Http404):
+        with pytest.raises(SentryAppIntegratorError):
             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(Http404):
+        with pytest.raises(SentryAppIntegratorError):
             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(Http404):
+        with pytest.raises(SentryAppIntegratorError):
             self.endpoint.convert_args(self.request, "1234")
 
 

tests/sentry/sentry_apps/api/endpoints/test_organization_sentry_app_installations.py

@@ -170,7 +170,7 @@ def test_install_superuser_read(self):
         self.login_as(user=self.superuser, superuser=True)
 
         app = self.create_sentry_app(name="Sample", organization=self.org)
-        self.get_error_response(self.org.slug, slug=app.slug, status_code=404)
+        self.get_error_response(self.org.slug, slug=app.slug, status_code=403)
 
     @override_settings(SENTRY_SELF_HOSTED=False)
     @override_options({"superuser.read-write.ga-rollout": True})

tests/sentry/sentry_apps/api/endpoints/test_sentry_app_details.py

@@ -85,10 +85,10 @@ 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=404)
+        self.get_error_response(self.internal_integration.slug, status_code=400)
 
     def test_users_do_not_see_unowned_unpublished_apps(self):
-        self.get_error_response(self.unowned_unpublished_app.slug, status_code=404)
+        self.get_error_response(self.unowned_unpublished_app.slug, status_code=400)
 
 
 @control_silo_test
@@ -444,7 +444,7 @@ def test_cannot_update_non_owned_apps(self):
             app.slug,
             name="NewName",
             webhookUrl="https://newurl.com",
-            status_code=404,
+            status_code=400,
         )
 
     def test_superuser_can_update_popularity(self):
@@ -741,7 +741,7 @@ 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=404,
+            status_code=400,
         )
         assert response.data["detail"] == "Not found."
 

tests/sentry/sentry_apps/api/endpoints/test_sentry_apps.py

@@ -402,16 +402,19 @@ 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=404)
-        assert response.data["detail"] == "Not found."
+        response = self.get_error_response(**self.get_data(), status_code=400)
+        assert (
+            response.data["error"]
+            == "User must be a part of the Org they're trying to create the app in"
+        )
 
     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 == {
-            "organization": "Organization 'some-non-existent-org' does not exist.",
+            "error": "Organization 'some-non-existent-org' does not exist.",
         }
 
     def test_superuser_can_create_with_popularity(self):
@@ -544,7 +547,7 @@ def test_cannot_create_app_without_organization(self):
         data = self.get_data(name=sentry_app.name, organization=None)
         response = self.get_error_response(**data, status_code=400)
         assert response.data == {
-            "organization": "Please provide a valid value for the 'organization' field.",
+            "error": "Please provide a valid value for the 'organization' field.",
         }
 
     def test_cannot_create_app_in_alien_organization(self):
@@ -553,16 +556,16 @@ 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=403)
-        assert response.data["detail"].startswith("User does not belong to")
+        response = self.get_error_response(**data, status_code=400)
+        assert response.data["error"].startswith("User does not belong to")
 
     def test_user_cannot_create_app_in_nonexistent_organization(self):
         self.create_project(organization=self.organization)
         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=403)
-        assert response.data["detail"].startswith("User does not belong to")
+        response = self.get_error_response(**data, status_code=400)
+        assert response.data["error"].startswith("User does not belong to")
 
     def test_nonsuperuser_cannot_create_with_popularity(self):
         response = self.get_success_response(