From a11d5de3c651c6cea3cc9b05a41244b6e42521f8 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Mon, 6 Jan 2025 13:59:31 -0800 Subject: [PATCH] adjust tests for new error handler & update base endpoint classes to use new error types --- .../sentry_apps/api/bases/sentryapps.py | 63 ++++++++++++++----- .../sentry_apps/api/bases/test_sentryapps.py | 14 ++--- ...t_organization_sentry_app_installations.py | 2 +- .../api/endpoints/test_sentry_app_details.py | 8 +-- .../api/endpoints/test_sentry_apps.py | 19 +++--- 5 files changed, 70 insertions(+), 36 deletions(-) diff --git a/src/sentry/sentry_apps/api/bases/sentryapps.py b/src/sentry/sentry_apps/api/bases/sentryapps.py index 749be5c6e2dad9..86ff91d3f12b93 100644 --- a/src/sentry/sentry_apps/api/bases/sentryapps.py +++ b/src/sentry/sentry_apps/api/bases/sentryapps.py @@ -5,8 +5,7 @@ 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 @@ -14,10 +13,11 @@ 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) diff --git a/tests/sentry/sentry_apps/api/bases/test_sentryapps.py b/tests/sentry/sentry_apps/api/bases/test_sentryapps.py index 5d32a6657e8200..54b72a471143d4 100644 --- a/tests/sentry/sentry_apps/api/bases/test_sentryapps.py +++ b/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") diff --git a/tests/sentry/sentry_apps/api/endpoints/test_organization_sentry_app_installations.py b/tests/sentry/sentry_apps/api/endpoints/test_organization_sentry_app_installations.py index f90b3aa4830a17..c0595df64a97cb 100644 --- a/tests/sentry/sentry_apps/api/endpoints/test_organization_sentry_app_installations.py +++ b/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}) 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 bef1fcd53d2639..0726f06595fd4e 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,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." 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 ba18f4735755fe..c54dae943e481e 100644 --- a/tests/sentry/sentry_apps/api/endpoints/test_sentry_apps.py +++ b/tests/sentry/sentry_apps/api/endpoints/test_sentry_apps.py @@ -402,8 +402,11 @@ 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") @@ -411,7 +414,7 @@ def test_superuser_cannot_create_app_in_nonexistent_organization(self): 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(