From 4e5edcd2286796a9a104df7ab8fa17edc46e75e1 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Mon, 13 Jan 2025 14:39:23 -0800 Subject: [PATCH 01/18] revise bases errors to use new design --- .../sentry_apps/api/bases/sentryapps.py | 66 ++++++++++--------- src/sentry/sentry_apps/utils/errors.py | 49 +++++++------- 2 files changed, 59 insertions(+), 56 deletions(-) diff --git a/src/sentry/sentry_apps/api/bases/sentryapps.py b/src/sentry/sentry_apps/api/bases/sentryapps.py index 71a069307dfc46..b8df43a64d7797 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=403, ) assert request.method, "method must be present in request to get permissions" @@ -131,7 +128,13 @@ 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 = Response( + { + "detail": exception.message, + "context": exception.extras.get("public_context", {}), + }, + status=exception.status_code, + ) response.exception = True return response @@ -154,7 +157,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) return organization_slug def _get_organization_for_superuser_or_staff( @@ -166,7 +169,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) return context @@ -178,7 +181,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)) return context def _get_org_context(self, request: Request) -> RpcUserOrganizationContext: @@ -260,10 +263,15 @@ 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, + extras={ + "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 +307,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 +326,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 +357,12 @@ 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, + extras={ + "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 +387,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 +443,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 +456,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/utils/errors.py b/src/sentry/sentry_apps/utils/errors.py index 6c9cdc7186148c..7815517b81254d 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, TypedDict class SentryAppErrorType(Enum): @@ -7,43 +8,41 @@ 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 ErrorContext(TypedDict, total=False): + # Info that gets sent only to the integrator via webhook + webhook_context: dict[str, Any] + # Info that gets sent to the end user via endpoint Response AND sent to integrator + public_context: dict[str, Any] + + +class SentryAppBaseError(Exception): + error_type: SentryAppErrorType + status_code: int def __init__( self, - error: Exception | None = None, + message: str, status_code: int | None = None, + extras: ErrorContext | None = None, ) -> None: - if status_code: - self.status_code = status_code + self.status_code = status_code or self.status_code + self.extras = extras 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 From 7eb854c987c36379d9a33cc98f0a9007227905ad Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Mon, 13 Jan 2025 15:36:44 -0800 Subject: [PATCH 02/18] fix tests --- .../sentry_apps/api/bases/test_sentryapps.py | 14 +++---- .../api/endpoints/test_sentry_app_details.py | 39 ++++++++++++++++--- .../api/endpoints/test_sentry_apps.py | 8 +++- 3 files changed, 47 insertions(+), 14 deletions(-) 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..249c775edb9ff9 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=403) assert ( response.data["detail"] == "User must be a part of the Org they're trying to create the app in" @@ -413,7 +413,10 @@ 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 == {"detail": "Organization 'some-non-existent-org' does not exist."} + assert response.data == { + "detail": "Organization 'some-non-existent-org' does not exist.", + "context": {}, + } def test_superuser_can_create_with_popularity(self): response = self.get_success_response( @@ -546,6 +549,7 @@ def test_cannot_create_app_without_organization(self): response = self.get_error_response(**data, status_code=400) assert response.data == { "detail": "Please provide a valid value for the 'organization' field.", + "context": {}, } def test_cannot_create_app_in_alien_organization(self): From 6799898c83f9b68c5aeabc19b653cf120252dfd6 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Mon, 13 Jan 2025 23:37:59 -0800 Subject: [PATCH 03/18] correct sentryappsentry errors --- .../sentry_apps/external_issues/external_issue_creator.py | 5 ++++- src/sentry/sentry_apps/external_issues/issue_link_creator.py | 3 +-- 2 files changed, 5 insertions(+), 3 deletions(-) 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..7dec1e854714e8 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", + extras={"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..ccc1543f16edf6 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}") def _make_external_request(self) -> dict[str, Any]: response = IssueLinkRequester( From 260020ada4b06ab2eb3d59599346b1aa495e001f Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Tue, 14 Jan 2025 14:22:51 -0800 Subject: [PATCH 04/18] update status codes to be more accurate --- .../sentry_apps/api/bases/sentryapps.py | 20 +++++++++---------- .../external_issues/issue_link_creator.py | 2 +- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/sentry/sentry_apps/api/bases/sentryapps.py b/src/sentry/sentry_apps/api/bases/sentryapps.py index b8df43a64d7797..aff624799efec7 100644 --- a/src/sentry/sentry_apps/api/bases/sentryapps.py +++ b/src/sentry/sentry_apps/api/bases/sentryapps.py @@ -103,7 +103,7 @@ def has_object_permission(self, request: Request, view, context: RpcUserOrganiza if context.organization.status != OrganizationStatus.ACTIVE or not context.member: raise SentryAppError( message="User must be a part of the Org they're trying to create the app in", - status_code=403, + status_code=401, ) assert request.method, "method must be present in request to get permissions" @@ -128,13 +128,11 @@ 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": exception.message, - "context": exception.extras.get("public_context", {}), - }, - status=exception.status_code, - ) + response_body = {"detail": exception.message} + context = exception.extras.get("public_context", None) + response_body.update({"context": context}) if context else "" + + response = Response(response_body, status=exception.status_code) response.exception = True return response @@ -157,7 +155,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(message=error_message) + raise SentryAppError(message=error_message, status_code=404) return organization_slug def _get_organization_for_superuser_or_staff( @@ -169,7 +167,7 @@ def _get_organization_for_superuser_or_staff( if context is None: error_message = f"Organization '{organization_slug}' does not exist." - raise SentryAppError(message=error_message) + raise SentryAppError(message=error_message, status_code=404) return context @@ -181,7 +179,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 SentryAppError(message=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: 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 ccc1543f16edf6..94a24e5dcbb87c 100644 --- a/src/sentry/sentry_apps/external_issues/issue_link_creator.py +++ b/src/sentry/sentry_apps/external_issues/issue_link_creator.py @@ -32,7 +32,7 @@ def run(self) -> PlatformExternalIssue: def _verify_action(self) -> None: if self.action not in VALID_ACTIONS: - raise SentryAppSentryError(message=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( From fadefc93388c68334d84f7ad050e16408143dd2a Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Tue, 14 Jan 2025 15:55:00 -0800 Subject: [PATCH 05/18] fix tests and address PR --- .../sentry_apps/api/endpoints/test_sentry_apps.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) 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 249c775edb9ff9..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=403) + 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,10 +412,9 @@ 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) + response = self.get_error_response(**data, status_code=404) assert response.data == { "detail": "Organization 'some-non-existent-org' does not exist.", - "context": {}, } def test_superuser_can_create_with_popularity(self): @@ -546,10 +545,9 @@ 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.", - "context": {}, } def test_cannot_create_app_in_alien_organization(self): @@ -558,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): @@ -566,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): From 90751f4e936a290a9fff6854a0250a87680c3a23 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Tue, 14 Jan 2025 16:21:00 -0800 Subject: [PATCH 06/18] cmon mypy :C --- src/sentry/sentry_apps/api/bases/sentryapps.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/sentry/sentry_apps/api/bases/sentryapps.py b/src/sentry/sentry_apps/api/bases/sentryapps.py index aff624799efec7..d009161ed575a5 100644 --- a/src/sentry/sentry_apps/api/bases/sentryapps.py +++ b/src/sentry/sentry_apps/api/bases/sentryapps.py @@ -128,9 +128,10 @@ 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_body = {"detail": exception.message} + response_body: dict[str, Any] = {"detail": exception.message} context = exception.extras.get("public_context", None) - response_body.update({"context": context}) if context else "" + if context is not None: + response_body.update({"context": context}) response = Response(response_body, status=exception.status_code) response.exception = True From c1cded0f5e2965a25c5bc4a80b7f07992249b61f Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Tue, 14 Jan 2025 16:47:24 -0800 Subject: [PATCH 07/18] break out extras to public and webhook context properties --- src/sentry/sentry_apps/api/bases/sentryapps.py | 18 +++++++----------- .../external_issues/external_issue_creator.py | 2 +- src/sentry/sentry_apps/utils/errors.py | 17 +++++++---------- 3 files changed, 15 insertions(+), 22 deletions(-) diff --git a/src/sentry/sentry_apps/api/bases/sentryapps.py b/src/sentry/sentry_apps/api/bases/sentryapps.py index d009161ed575a5..a332e6d1dfdcb7 100644 --- a/src/sentry/sentry_apps/api/bases/sentryapps.py +++ b/src/sentry/sentry_apps/api/bases/sentryapps.py @@ -129,9 +129,9 @@ 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_body: dict[str, Any] = {"detail": exception.message} - context = exception.extras.get("public_context", None) - if context is not None: - response_body.update({"context": context}) + public_context = exception.public_context + if public_context: + response_body.update({"context": public_context}) response = Response(response_body, status=exception.status_code) response.exception = True @@ -265,11 +265,9 @@ def has_object_permission(self, request: Request, view, sentry_app: RpcSentryApp raise SentryAppError( message="User must be in the app owner's organization for unpublished apps", status_code=403, - extras={ - "public_context": { - "integration": sentry_app.slug, - "user_organizations": [org.slug for org in organizations], - } + public_context={ + "integration": sentry_app.slug, + "user_organizations": [org.slug for org in organizations], }, ) @@ -359,9 +357,7 @@ def has_object_permission(self, request: Request, view, organization): raise SentryAppError( message="User must belong to the given organization", status_code=403, - extras={ - "public_context": {"user_organizations": [org.slug for org in organizations]} - }, + 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)) 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 7dec1e854714e8..428a2fc92246c4 100644 --- a/src/sentry/sentry_apps/external_issues/external_issue_creator.py +++ b/src/sentry/sentry_apps/external_issues/external_issue_creator.py @@ -48,5 +48,5 @@ def run(self) -> PlatformExternalIssue: ) raise SentryAppSentryError( message="Failed to create external issue obj", - extras={"webhook_context": {"error": str(e)}}, + webhook_context={"error": str(e)}, ) from e diff --git a/src/sentry/sentry_apps/utils/errors.py b/src/sentry/sentry_apps/utils/errors.py index 7815517b81254d..e2dc665fc26469 100644 --- a/src/sentry/sentry_apps/utils/errors.py +++ b/src/sentry/sentry_apps/utils/errors.py @@ -1,5 +1,5 @@ from enum import Enum -from typing import Any, TypedDict +from typing import Any class SentryAppErrorType(Enum): @@ -8,13 +8,6 @@ class SentryAppErrorType(Enum): SENTRY = "sentry" -class ErrorContext(TypedDict, total=False): - # Info that gets sent only to the integrator via webhook - webhook_context: dict[str, Any] - # Info that gets sent to the end user via endpoint Response AND sent to integrator - public_context: dict[str, Any] - - class SentryAppBaseError(Exception): error_type: SentryAppErrorType status_code: int @@ -23,10 +16,14 @@ def __init__( self, message: str, status_code: int | None = None, - extras: ErrorContext | None = None, + public_context: dict[str, Any] | None = None, + webhook_context: dict[str, Any] | None = None, ) -> None: self.status_code = status_code or self.status_code - self.extras = extras or {} + # 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 From d4b256056d672cc07327e03cff3cb53622d55e5b Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Wed, 15 Jan 2025 14:53:45 -0800 Subject: [PATCH 08/18] select_requester changes --- .../installation_external_requests.py | 16 +------ .../external_requests/select_requester.py | 44 ++++++++++--------- 2 files changed, 24 insertions(+), 36 deletions(-) diff --git a/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py b/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py index d7b0d19592303a..3842e3c39c817e 100644 --- a/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py +++ b/src/sentry/sentry_apps/api/endpoints/installation_external_requests.py @@ -1,13 +1,11 @@ import logging -from jsonschema import ValidationError from rest_framework.request import Request from rest_framework.response import Response from sentry.api.api_owners import ApiOwner from sentry.api.api_publish_status import ApiPublishStatus from sentry.api.base import region_silo_endpoint -from sentry.coreapi import APIError from sentry.models.project import Project from sentry.sentry_apps.api.bases.sentryapps import SentryAppInstallationBaseEndpoint from sentry.sentry_apps.external_requests.select_requester import SelectRequester @@ -40,18 +38,6 @@ def get(self, request: Request, installation) -> Response: if project: kwargs.update({"project_slug": project.slug}) - try: - choices = SelectRequester(**kwargs).run() - except ValidationError as e: - return Response( - {"error": e.message}, - status=400, - ) - except APIError: - message = f'Error retrieving select field options from {request.GET.get("uri")}' - return Response( - {"error": message}, - status=400, - ) + choices = SelectRequester(**kwargs).run() return Response(choices) diff --git a/src/sentry/sentry_apps/external_requests/select_requester.py b/src/sentry/sentry_apps/external_requests/select_requester.py index 9df1240653f150..e31691c20732a6 100644 --- a/src/sentry/sentry_apps/external_requests/select_requester.py +++ b/src/sentry/sentry_apps/external_requests/select_requester.py @@ -6,13 +6,12 @@ from uuid import uuid4 from django.utils.functional import cached_property -from jsonschema import ValidationError -from sentry.coreapi import APIError from sentry.http import safe_urlread from sentry.sentry_apps.external_requests.utils import send_and_save_sentry_app_request, validate from sentry.sentry_apps.services.app import RpcSentryAppInstallation from sentry.sentry_apps.services.app.model import RpcSentryApp +from sentry.sentry_apps.utils.errors import SentryAppIntegratorError from sentry.utils import json logger = logging.getLogger("sentry.sentry_apps.external_requests") @@ -78,24 +77,28 @@ def run(self) -> SelectRequesterResult: message = "select-requester.request-failed" logger.info(message, extra=extra) - raise APIError( - f"Something went wrong while getting SelectFields from {self.sentry_app.slug}" + raise SentryAppIntegratorError( + message=f"Something went wrong while getting SelectFields from {self.sentry_app.slug}", + webhook_context={"error_type": message, **extra}, ) from e if not self._validate_response(response): - logger.info( - "select-requester.invalid-response", - extra={ - "response": response, - "sentry_app_slug": self.sentry_app.slug, - "install_uuid": self.install.uuid, - "project_slug": self.project_slug, - "url": url, + extras = { + "response": response, + "sentry_app_slug": self.sentry_app.slug, + "install_uuid": self.install.uuid, + "project_slug": self.project_slug, + "url": url, + } + logger.info("select-requester.invalid-response", extra=extras) + + raise SentryAppIntegratorError( + message=f"Invalid response format for SelectField in {self.sentry_app.slug} from uri: {self.uri}", + webhook_context={ + "error_type": "select-requester.invalid-integrator-response", + **extras, }, ) - raise ValidationError( - f"Invalid response format for SelectField in {self.sentry_app.slug} from uri: {self.uri}" - ) return self._format_response(response) def _build_url(self) -> str: @@ -130,14 +133,13 @@ def _format_response(self, resp: Sequence[dict[str, Any]]) -> SelectRequesterRes for option in resp: if not ("value" in option and "label" in option): - logger.info( - "select-requester.invalid-response", - extra={ - "resposnse": resp, - "error_msg": "Missing `value` or `label` in option data for SelectField", + raise SentryAppIntegratorError( + message="Missing `value` or `label` in option data for SelectField", + webhook_context={ + "error_type": "select-requester.missing-fields", + "response": resp, }, ) - raise ValidationError("Missing `value` or `label` in option data for SelectField") choices.append([option["value"], option["label"]]) From b9fbc6ca3c5fdf64a92c4f09c344cf48026b8b82 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Thu, 16 Jan 2025 11:18:56 -0800 Subject: [PATCH 09/18] issue link req code --- .../external_requests/issue_link_requester.py | 44 ++++++++++++------- 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/src/sentry/sentry_apps/external_requests/issue_link_requester.py b/src/sentry/sentry_apps/external_requests/issue_link_requester.py index 50279b4d79f8ff..1740ebc025df61 100644 --- a/src/sentry/sentry_apps/external_requests/issue_link_requester.py +++ b/src/sentry/sentry_apps/external_requests/issue_link_requester.py @@ -5,13 +5,12 @@ from uuid import uuid4 from django.utils.functional import cached_property -from jsonschema import ValidationError -from sentry.coreapi import APIError from sentry.http import safe_urlread from sentry.models.group import Group from sentry.sentry_apps.external_requests.utils import send_and_save_sentry_app_request, validate from sentry.sentry_apps.services.app import RpcSentryAppInstallation +from sentry.sentry_apps.utils.errors import SentryAppIntegratorError from sentry.users.models.user import User from sentry.users.services.user import RpcUser from sentry.utils import json @@ -74,26 +73,37 @@ def run(self) -> dict[str, Any]: body = safe_urlread(request) response = json.loads(body) except (json.JSONDecodeError, TypeError): - raise ValidationError(f"Unable to parse response from {self.sentry_app.slug}") - except Exception as e: - logger.info( - "issue-link-requester.error", - extra={ - "sentry_app": self.sentry_app.slug, - "install": self.install.uuid, - "project": self.group.project.slug, - "group": self.group.id, - "uri": self.uri, - "error_message": str(e), + raise SentryAppIntegratorError( + message=f"Unable to parse response from {self.sentry_app.slug}", + webhook_context={ + "error_type": "issue-link-requester.invalid-json", + "response": body, }, ) - raise APIError( - f"Issue occured while trying to contact {self.sentry_app.slug} to link issue" + except Exception as e: + error_type = "issue-link-requester.error" + extras = { + "sentry_app": self.sentry_app.slug, + "install": self.install.uuid, + "project": self.group.project.slug, + "group": self.group.id, + "uri": self.uri, + "error_message": str(e), + } + logger.info(error_type, extra=extras) + raise SentryAppIntegratorError( + message=f"Issue occured while trying to contact {self.sentry_app.slug} to link issue", + webhook_context={"error_type": error_type, **extras}, ) if not self._validate_response(response): - raise ValidationError( - f"Invalid response format from sentry app {self.sentry_app} when linking issue" + raise SentryAppIntegratorError( + message=f"Invalid response format from sentry app {self.sentry_app} when linking issue", + webhook_context={ + "error_type": "issue-link-requester.invalid-response", + "response": response, + "uri": self.uri, + }, ) return response From 086f9c0b6b3e08b0ab46142bb2b220b1806d753b Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 17 Jan 2025 09:47:18 -0800 Subject: [PATCH 10/18] code changes for installation_ext_issue_act endpt & ext_issue_creator --- .../installation_external_issue_actions.py | 46 ++++++++----------- .../external_issues/external_issue_creator.py | 7 ++- 2 files changed, 22 insertions(+), 31 deletions(-) diff --git a/src/sentry/sentry_apps/api/endpoints/installation_external_issue_actions.py b/src/sentry/sentry_apps/api/endpoints/installation_external_issue_actions.py index 13ae408d361ae1..491a76ecd4c44d 100644 --- a/src/sentry/sentry_apps/api/endpoints/installation_external_issue_actions.py +++ b/src/sentry/sentry_apps/api/endpoints/installation_external_issue_actions.py @@ -1,14 +1,11 @@ from django.utils.functional import empty -from jsonschema import ValidationError from rest_framework.request import Request from rest_framework.response import Response -from sentry_sdk import capture_exception from sentry.api.api_owners import ApiOwner from sentry.api.api_publish_status import ApiPublishStatus from sentry.api.base import region_silo_endpoint from sentry.api.serializers import serialize -from sentry.coreapi import APIError, APIUnauthorized from sentry.models.group import Group from sentry.models.project import Project from sentry.sentry_apps.api.bases.sentryapps import SentryAppInstallationBaseEndpoint @@ -16,6 +13,7 @@ PlatformExternalIssueSerializer, ) from sentry.sentry_apps.external_issues.issue_link_creator import IssueLinkCreator +from sentry.sentry_apps.utils.errors import SentryAppError from sentry.users.models.user import User from sentry.users.services.user.serial import serialize_rpc_user @@ -45,7 +43,9 @@ def post(self, request: Request, installation) -> Response: data = request.data.copy() if not {"groupId", "action", "uri"}.issubset(data.keys()): - return Response(status=400) + raise SentryAppError( + message="Missing groupId, action, uri in request body", status_code=400 + ) group_id = data.get("groupId") del data["groupId"] @@ -56,34 +56,26 @@ def post(self, request: Request, installation) -> Response: project_id__in=Project.objects.filter(organization_id=installation.organization_id), ) except Group.DoesNotExist: - return Response(status=404) + raise SentryAppError( + message="Could not find the corresponding issue for the given groupId", + status_code=400, + ) action = data.pop("action") uri = data.pop("uri") - try: - user = _extract_lazy_object(request.user) - if isinstance(user, User): - user = serialize_rpc_user(user) + user = _extract_lazy_object(request.user) + if isinstance(user, User): + user = serialize_rpc_user(user) - external_issue = IssueLinkCreator( - install=installation, - group=group, - action=action, - fields=data, - uri=uri, - user=user, - ).run() - except (APIError, ValidationError, APIUnauthorized) as e: - return Response({"error": str(e)}, status=400) - except Exception as e: - error_id = capture_exception(e) - return Response( - { - "error": f"Something went wrong while trying to link issue. Sentry error ID: {error_id}" - }, - status=500, - ) + external_issue = IssueLinkCreator( + install=installation, + group=group, + action=action, + fields=data, + uri=uri, + user=user, + ).run() return Response( serialize(objects=external_issue, serializer=PlatformExternalIssueSerializer()) 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 428a2fc92246c4..797134093b2eb2 100644 --- a/src/sentry/sentry_apps/external_issues/external_issue_creator.py +++ b/src/sentry/sentry_apps/external_issues/external_issue_creator.py @@ -38,15 +38,14 @@ def run(self) -> PlatformExternalIssue: return self.external_issue[0] except Exception as e: logger.info( - "create-failed", + "platform-external-issue.create-failed", + exc_info=e, extra={ - "error": str(e), - "installtion_id": self.install.uuid, + "installation_id": self.install.uuid, "group_id": self.group.id, "sentry_app_slug": self.install.sentry_app.slug, }, ) raise SentryAppSentryError( message="Failed to create external issue obj", - webhook_context={"error": str(e)}, ) from e From 90331037be570d73cfdcfcb1b3cc0b0d181e0f7d Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 17 Jan 2025 09:50:21 -0800 Subject: [PATCH 11/18] code changes for installation_ext_issue_details --- .../api/endpoints/installation_external_issue_details.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/sentry/sentry_apps/api/endpoints/installation_external_issue_details.py b/src/sentry/sentry_apps/api/endpoints/installation_external_issue_details.py index 3bbd636b84623d..702e709ffa9427 100644 --- a/src/sentry/sentry_apps/api/endpoints/installation_external_issue_details.py +++ b/src/sentry/sentry_apps/api/endpoints/installation_external_issue_details.py @@ -8,6 +8,7 @@ SentryAppInstallationExternalIssueBaseEndpoint as ExternalIssueBaseEndpoint, ) from sentry.sentry_apps.models.platformexternalissue import PlatformExternalIssue +from sentry.sentry_apps.utils.errors import SentryAppError @region_silo_endpoint @@ -24,7 +25,10 @@ def delete(self, request: Request, installation, external_issue_id) -> Response: service_type=installation.sentry_app.slug, ) except PlatformExternalIssue.DoesNotExist: - return Response(status=404) + raise SentryAppError( + message="Could not find the corresponding external issue from given external_issue_id", + status_code=404, + ) deletions.exec_sync(platform_external_issue) From b2efbb4faf23baa1cda307598288587b1a641ef0 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 17 Jan 2025 09:53:34 -0800 Subject: [PATCH 12/18] code changes for installation_ext_issues --- .../api/endpoints/installation_external_issues.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/sentry/sentry_apps/api/endpoints/installation_external_issues.py b/src/sentry/sentry_apps/api/endpoints/installation_external_issues.py index 8f4f6073b761b9..6aef63a5562bc1 100644 --- a/src/sentry/sentry_apps/api/endpoints/installation_external_issues.py +++ b/src/sentry/sentry_apps/api/endpoints/installation_external_issues.py @@ -16,6 +16,7 @@ PlatformExternalIssueSerializer as ResponsePlatformExternalIssueSerializer, ) from sentry.sentry_apps.external_issues.external_issue_creator import ExternalIssueCreator +from sentry.sentry_apps.utils.errors import SentryAppError class PlatformExternalIssueSerializer(serializers.Serializer): @@ -40,7 +41,10 @@ def post(self, request: Request, installation) -> Response: project_id__in=Project.objects.filter(organization_id=installation.organization_id), ) except Group.DoesNotExist: - return Response(status=404) + raise SentryAppError( + message="Could not find the corresponding issue for the given issueId", + status_code=404, + ) serializer = PlatformExternalIssueSerializer(data=request.data) if serializer.is_valid(): From 43a7551462ca1589c785504fc795630685c954f1 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 17 Jan 2025 12:51:24 -0800 Subject: [PATCH 13/18] fix tests --- .../external_requests/select_requester.py | 9 +- ...app_installation_external_issue_actions.py | 2 +- .../test_issue_link_requester.py | 7 +- .../test_select_requester.py | 97 ++++++++----------- 4 files changed, 51 insertions(+), 64 deletions(-) diff --git a/src/sentry/sentry_apps/external_requests/select_requester.py b/src/sentry/sentry_apps/external_requests/select_requester.py index e31691c20732a6..1a62fa150fab10 100644 --- a/src/sentry/sentry_apps/external_requests/select_requester.py +++ b/src/sentry/sentry_apps/external_requests/select_requester.py @@ -60,7 +60,6 @@ def run(self) -> SelectRequesterResult: "sentry_app_slug": self.sentry_app.slug, "install_uuid": self.install.uuid, "project_slug": self.project_slug, - "error_message": str(e), } if not url: @@ -76,9 +75,9 @@ def run(self) -> SelectRequesterResult: extra.update({"url": url}) message = "select-requester.request-failed" - logger.info(message, extra=extra) + logger.info(message, exc_info=e, extra=extra) raise SentryAppIntegratorError( - message=f"Something went wrong while getting SelectFields from {self.sentry_app.slug}", + message=f"Something went wrong while getting options for Select FormField from {self.sentry_app.slug}", webhook_context={"error_type": message, **extra}, ) from e @@ -93,7 +92,7 @@ def run(self) -> SelectRequesterResult: logger.info("select-requester.invalid-response", extra=extras) raise SentryAppIntegratorError( - message=f"Invalid response format for SelectField in {self.sentry_app.slug} from uri: {self.uri}", + message=f"Invalid response format for Select FormField in {self.sentry_app.slug} from uri: {self.uri}", webhook_context={ "error_type": "select-requester.invalid-integrator-response", **extras, @@ -134,7 +133,7 @@ def _format_response(self, resp: Sequence[dict[str, Any]]) -> SelectRequesterRes for option in resp: if not ("value" in option and "label" in option): raise SentryAppIntegratorError( - message="Missing `value` or `label` in option data for SelectField", + message="Missing `value` or `label` in option data for Select FormField", webhook_context={ "error_type": "select-requester.missing-fields", "response": resp, diff --git a/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_installation_external_issue_actions.py b/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_installation_external_issue_actions.py index a0e80b7f710d4c..3e0986affe4084 100644 --- a/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_installation_external_issue_actions.py +++ b/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_installation_external_issue_actions.py @@ -78,6 +78,6 @@ def test_external_issue_doesnt_get_created(self): assert response.status_code == 400 assert ( response.content - == b'{"error":"Issue occured while trying to contact testin to link issue"}' + == b'{"detail":"Issue occured while trying to contact testin to link issue"}' ) assert not PlatformExternalIssue.objects.all() diff --git a/tests/sentry/sentry_apps/external_requests/test_issue_link_requester.py b/tests/sentry/sentry_apps/external_requests/test_issue_link_requester.py index 082a40e84cdd6c..a7943c7a079b00 100644 --- a/tests/sentry/sentry_apps/external_requests/test_issue_link_requester.py +++ b/tests/sentry/sentry_apps/external_requests/test_issue_link_requester.py @@ -1,10 +1,9 @@ import pytest import responses -from jsonschema import ValidationError -from sentry.coreapi import APIError from sentry.sentry_apps.external_requests.issue_link_requester import IssueLinkRequester from sentry.sentry_apps.services.app import app_service +from sentry.sentry_apps.utils.errors import SentryAppIntegratorError from sentry.testutils.cases import TestCase from sentry.users.services.user.serial import serialize_rpc_user from sentry.utils import json @@ -95,7 +94,7 @@ def test_invalid_response_format(self): status=200, content_type="application/json", ) - with pytest.raises(ValidationError): + with pytest.raises(SentryAppIntegratorError): IssueLinkRequester( install=self.install, group=self.group, @@ -114,7 +113,7 @@ def test_500_response(self): status=500, ) - with pytest.raises(APIError): + with pytest.raises(SentryAppIntegratorError): IssueLinkRequester( install=self.install, group=self.group, diff --git a/tests/sentry/sentry_apps/external_requests/test_select_requester.py b/tests/sentry/sentry_apps/external_requests/test_select_requester.py index 45ad13df13145d..982f2b85dce162 100644 --- a/tests/sentry/sentry_apps/external_requests/test_select_requester.py +++ b/tests/sentry/sentry_apps/external_requests/test_select_requester.py @@ -1,10 +1,9 @@ import pytest import responses -from jsonschema import ValidationError -from sentry.coreapi import APIError from sentry.sentry_apps.external_requests.select_requester import SelectRequester from sentry.sentry_apps.services.app import app_service +from sentry.sentry_apps.utils.errors import SentryAppIntegratorError from sentry.testutils.cases import TestCase from sentry.utils.sentry_apps import SentryAppWebhookRequestsBuffer @@ -61,22 +60,38 @@ def test_makes_request(self): @responses.activate def test_invalid_response_missing_label(self): # missing 'label' + url = f"https://example.com/get-issues?installationId={self.install.uuid}&projectSlug={self.project.slug}" + uri = "/get-issues" + invalid_format = {"value": "12345"} responses.add( method=responses.GET, - url=f"https://example.com/get-issues?installationId={self.install.uuid}&projectSlug={self.project.slug}", + url=url, json=invalid_format, status=200, content_type="application/json", ) - with pytest.raises(ValidationError): + with pytest.raises(SentryAppIntegratorError) as exception_info: SelectRequester( install=self.install, project_slug=self.project.slug, - uri="/get-issues", + uri=uri, ).run() + assert ( + exception_info.value.message + == f"Invalid response format for Select FormField in {self.sentry_app.slug} from uri: {uri}" + ) + assert exception_info.value.webhook_context == { + "error_type": "select-requester.invalid-integrator-response", + "response": invalid_format, + "sentry_app_slug": self.sentry_app.slug, + "install_uuid": self.install.uuid, + "project_slug": self.project.slug, + "url": url, + } + @responses.activate def test_invalid_response_missing_value(self): # missing 'label' and 'value' @@ -91,13 +106,22 @@ def test_invalid_response_missing_value(self): content_type="application/json", ) - with pytest.raises(ValidationError): + with pytest.raises(SentryAppIntegratorError) as exception_info: SelectRequester( install=self.install, project_slug=self.project.slug, uri="/get-issues", ).run() + assert ( + exception_info.value.message + == "Missing `value` or `label` in option data for Select FormField" + ) + assert exception_info.value.webhook_context == { + "error_type": "select-requester.missing-fields", + "response": invalid_format, + } + @responses.activate def test_500_response(self): responses.add( @@ -107,7 +131,7 @@ def test_500_response(self): status=500, ) - with pytest.raises(APIError): + with pytest.raises(SentryAppIntegratorError): SelectRequester( install=self.install, project_slug=self.project.slug, @@ -123,63 +147,28 @@ def test_500_response(self): @responses.activate def test_api_error_message(self): + url = f"https://example.com/get-issues?installationId={self.install.uuid}&projectSlug={self.project.slug}" responses.add( method=responses.GET, - url=f"https://example.com/get-issues?installationId={self.install.uuid}&projectSlug={self.project.slug}", + url=url, body="Something failed", status=500, ) - with pytest.raises(APIError) as exception_info: + with pytest.raises(SentryAppIntegratorError) as exception_info: SelectRequester( install=self.install, project_slug=self.project.slug, uri="/get-issues", ).run() assert ( - str(exception_info.value) - == f"Something went wrong while getting SelectFields from {self.sentry_app.slug}" - ) - - @responses.activate - def test_validation_error_message_validator(self): - uri = "/get-issues" - - responses.add( - method=responses.GET, - url=f"https://example.com/get-issues?installationId={self.install.uuid}&projectSlug={self.project.slug}", - json={}, - status=200, - ) - - with pytest.raises(ValidationError) as exception_info: - SelectRequester( - install=self.install, - project_slug=self.project.slug, - uri=uri, - ).run() - - assert ( - str(exception_info.value) - == f"Invalid response format for SelectField in {self.sentry_app.slug} from uri: {uri}" - ) - - @responses.activate - def test_validation_error_message_missing_field(self): - responses.add( - method=responses.GET, - url=f"https://example.com/get-issues?installationId={self.install.uuid}&projectSlug={self.project.slug}", - json=[{"bruh": "bruhhhhh"}], - status=200, - ) - - with pytest.raises(ValidationError) as exception_info: - SelectRequester( - install=self.install, - project_slug=self.project.slug, - uri="/get-issues", - ).run() - - assert ( - str(exception_info.value) == "Missing `value` or `label` in option data for SelectField" + exception_info.value.message + == f"Something went wrong while getting options for Select FormField from {self.sentry_app.slug}" ) + assert exception_info.value.webhook_context == { + "error_type": "select-requester.request-failed", + "sentry_app_slug": self.sentry_app.slug, + "install_uuid": self.install.uuid, + "project_slug": self.project.slug, + "url": url, + } From 6b79a81c11539ba6e31619b2c3694d0001a908da Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Tue, 21 Jan 2025 01:38:43 -0800 Subject: [PATCH 14/18] wasn't catching errors in components --- .../sentry_apps/models/sentry_app_installation.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/sentry/sentry_apps/models/sentry_app_installation.py b/src/sentry/sentry_apps/models/sentry_app_installation.py index 62cfb87dac734f..989c3a6c43e348 100644 --- a/src/sentry/sentry_apps/models/sentry_app_installation.py +++ b/src/sentry/sentry_apps/models/sentry_app_installation.py @@ -19,6 +19,11 @@ from sentry.hybridcloud.outbox.category import OutboxCategory from sentry.projects.services.project import RpcProject from sentry.sentry_apps.services.app.model import RpcSentryAppComponent, RpcSentryAppInstallation +from sentry.sentry_apps.utils.errors import ( + SentryAppError, + SentryAppIntegratorError, + SentryAppSentryError, +) from sentry.types.region import find_regions_for_orgs if TYPE_CHECKING: @@ -240,6 +245,12 @@ def prepare_ui_component( component=component, install=installation, project_slug=project_slug, values=values ).run() return component - except (APIError, ValidationError): + except ( + APIError, + ValidationError, + SentryAppIntegratorError, + SentryAppError, + SentryAppSentryError, + ): # TODO(nisanthan): For now, skip showing the UI Component if the API requests fail return None From 601b8cfad05585cb1f10d5cb2c84c3dc3a977f1f Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Tue, 21 Jan 2025 02:28:35 -0800 Subject: [PATCH 15/18] use more accurate status codes and infos --- .../api/endpoints/installation_external_issue_actions.py | 2 +- .../sentry_apps/external_requests/issue_link_requester.py | 8 +++++++- .../sentry_apps/external_requests/select_requester.py | 2 ++ ...test_sentry_app_installation_external_issue_actions.py | 2 +- .../test_sentry_app_installation_external_requests.py | 2 +- 5 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/sentry/sentry_apps/api/endpoints/installation_external_issue_actions.py b/src/sentry/sentry_apps/api/endpoints/installation_external_issue_actions.py index 491a76ecd4c44d..1f0caf4277e016 100644 --- a/src/sentry/sentry_apps/api/endpoints/installation_external_issue_actions.py +++ b/src/sentry/sentry_apps/api/endpoints/installation_external_issue_actions.py @@ -58,7 +58,7 @@ def post(self, request: Request, installation) -> Response: except Group.DoesNotExist: raise SentryAppError( message="Could not find the corresponding issue for the given groupId", - status_code=400, + status_code=404, ) action = data.pop("action") diff --git a/src/sentry/sentry_apps/external_requests/issue_link_requester.py b/src/sentry/sentry_apps/external_requests/issue_link_requester.py index 1740ebc025df61..fd190906521b31 100644 --- a/src/sentry/sentry_apps/external_requests/issue_link_requester.py +++ b/src/sentry/sentry_apps/external_requests/issue_link_requester.py @@ -77,14 +77,17 @@ def run(self) -> dict[str, Any]: message=f"Unable to parse response from {self.sentry_app.slug}", webhook_context={ "error_type": "issue-link-requester.invalid-json", + "uri": self.uri, "response": body, + "installation_uuid": self.install.uuid, }, + status_code=503, ) except Exception as e: error_type = "issue-link-requester.error" extras = { "sentry_app": self.sentry_app.slug, - "install": self.install.uuid, + "installation_uuid": self.install.uuid, "project": self.group.project.slug, "group": self.group.id, "uri": self.uri, @@ -94,6 +97,7 @@ def run(self) -> dict[str, Any]: raise SentryAppIntegratorError( message=f"Issue occured while trying to contact {self.sentry_app.slug} to link issue", webhook_context={"error_type": error_type, **extras}, + status_code=503, ) if not self._validate_response(response): @@ -102,8 +106,10 @@ def run(self) -> dict[str, Any]: webhook_context={ "error_type": "issue-link-requester.invalid-response", "response": response, + "installation_uuid": self.install.uuid, "uri": self.uri, }, + status_code=503, ) return response diff --git a/src/sentry/sentry_apps/external_requests/select_requester.py b/src/sentry/sentry_apps/external_requests/select_requester.py index 1a62fa150fab10..04982b2d00589b 100644 --- a/src/sentry/sentry_apps/external_requests/select_requester.py +++ b/src/sentry/sentry_apps/external_requests/select_requester.py @@ -79,6 +79,7 @@ def run(self) -> SelectRequesterResult: raise SentryAppIntegratorError( message=f"Something went wrong while getting options for Select FormField from {self.sentry_app.slug}", webhook_context={"error_type": message, **extra}, + status_code=503, ) from e if not self._validate_response(response): @@ -138,6 +139,7 @@ def _format_response(self, resp: Sequence[dict[str, Any]]) -> SelectRequesterRes "error_type": "select-requester.missing-fields", "response": resp, }, + status_code=503, ) choices.append([option["value"], option["label"]]) diff --git a/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_installation_external_issue_actions.py b/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_installation_external_issue_actions.py index 3e0986affe4084..658c6ec089d1e2 100644 --- a/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_installation_external_issue_actions.py +++ b/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_installation_external_issue_actions.py @@ -75,7 +75,7 @@ def test_external_issue_doesnt_get_created(self): ) response = self.client.post(self.url, data=data, format="json") - assert response.status_code == 400 + assert response.status_code == 503 assert ( response.content == b'{"detail":"Issue occured while trying to contact testin to link issue"}' diff --git a/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_installation_external_requests.py b/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_installation_external_requests.py index d46f07773dff49..f73d3ec877efc1 100644 --- a/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_installation_external_requests.py +++ b/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_installation_external_requests.py @@ -90,4 +90,4 @@ def test_external_request_fails(self): ) url = self.url + f"?uri={self.project.id}" response = self.client.get(url, format="json") - assert response.status_code == 400 + assert response.status_code == 503 From 41131231463cd91d10782042e64aa2550911d0fc Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 24 Jan 2025 14:34:52 -0800 Subject: [PATCH 16/18] update to catch only RequestExceptions --- .../sentry_apps/external_requests/issue_link_requester.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sentry/sentry_apps/external_requests/issue_link_requester.py b/src/sentry/sentry_apps/external_requests/issue_link_requester.py index fd190906521b31..a2bd823526ef6c 100644 --- a/src/sentry/sentry_apps/external_requests/issue_link_requester.py +++ b/src/sentry/sentry_apps/external_requests/issue_link_requester.py @@ -5,6 +5,7 @@ from uuid import uuid4 from django.utils.functional import cached_property +from requests import RequestException from sentry.http import safe_urlread from sentry.models.group import Group @@ -83,7 +84,7 @@ def run(self) -> dict[str, Any]: }, status_code=503, ) - except Exception as e: + except RequestException as e: error_type = "issue-link-requester.error" extras = { "sentry_app": self.sentry_app.slug, From 209e41d939c830a665189af661afa5ae091d186d Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Fri, 24 Jan 2025 17:09:35 -0800 Subject: [PATCH 17/18] make a serializer to tell why the user input was wrong --- .../installation_external_issue_actions.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/sentry/sentry_apps/api/endpoints/installation_external_issue_actions.py b/src/sentry/sentry_apps/api/endpoints/installation_external_issue_actions.py index 1f0caf4277e016..7416a9e5a29a81 100644 --- a/src/sentry/sentry_apps/api/endpoints/installation_external_issue_actions.py +++ b/src/sentry/sentry_apps/api/endpoints/installation_external_issue_actions.py @@ -1,4 +1,5 @@ from django.utils.functional import empty +from rest_framework import serializers from rest_framework.request import Request from rest_framework.response import Response @@ -32,6 +33,12 @@ def _extract_lazy_object(lo): return lo._wrapped +class SentryAppInstallationExternalIssueActionsSerializer(serializers.Serizlizer): + groupId = serializers.CharField(required=True, allow_null=False) + action = serializers.CharField(required=True, allow_null=False) + uri = serializers.CharField(required=True, allow_null=False) + + @region_silo_endpoint class SentryAppInstallationExternalIssueActionsEndpoint(SentryAppInstallationBaseEndpoint): owner = ApiOwner.INTEGRATIONS @@ -42,10 +49,10 @@ class SentryAppInstallationExternalIssueActionsEndpoint(SentryAppInstallationBas def post(self, request: Request, installation) -> Response: data = request.data.copy() - if not {"groupId", "action", "uri"}.issubset(data.keys()): - raise SentryAppError( - message="Missing groupId, action, uri in request body", status_code=400 - ) + external_issue_action_serializer = SentryAppInstallationExternalIssueActionsSerializer(data) + + if not external_issue_action_serializer.is_valid(): + return Response(external_issue_action_serializer.errors, status=400) group_id = data.get("groupId") del data["groupId"] From 86cad03944cea8e638420f21fdedfc3bf1fc53f3 Mon Sep 17 00:00:00 2001 From: Christinarlong Date: Mon, 27 Jan 2025 15:35:30 -0800 Subject: [PATCH 18/18] switch to 500 error code for 3p errors --- .../api/endpoints/installation_external_issue_actions.py | 6 ++++-- .../sentry_apps/external_requests/issue_link_requester.py | 6 +++--- .../sentry_apps/external_requests/select_requester.py | 4 ++-- .../test_sentry_app_installation_external_issue_actions.py | 2 +- .../test_sentry_app_installation_external_requests.py | 2 +- 5 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/sentry/sentry_apps/api/endpoints/installation_external_issue_actions.py b/src/sentry/sentry_apps/api/endpoints/installation_external_issue_actions.py index 7416a9e5a29a81..67c5fdaeb96d12 100644 --- a/src/sentry/sentry_apps/api/endpoints/installation_external_issue_actions.py +++ b/src/sentry/sentry_apps/api/endpoints/installation_external_issue_actions.py @@ -33,7 +33,7 @@ def _extract_lazy_object(lo): return lo._wrapped -class SentryAppInstallationExternalIssueActionsSerializer(serializers.Serizlizer): +class SentryAppInstallationExternalIssueActionsSerializer(serializers.Serializer): groupId = serializers.CharField(required=True, allow_null=False) action = serializers.CharField(required=True, allow_null=False) uri = serializers.CharField(required=True, allow_null=False) @@ -49,7 +49,9 @@ class SentryAppInstallationExternalIssueActionsEndpoint(SentryAppInstallationBas def post(self, request: Request, installation) -> Response: data = request.data.copy() - external_issue_action_serializer = SentryAppInstallationExternalIssueActionsSerializer(data) + external_issue_action_serializer = SentryAppInstallationExternalIssueActionsSerializer( + data=data + ) if not external_issue_action_serializer.is_valid(): return Response(external_issue_action_serializer.errors, status=400) diff --git a/src/sentry/sentry_apps/external_requests/issue_link_requester.py b/src/sentry/sentry_apps/external_requests/issue_link_requester.py index a2bd823526ef6c..659d554709faf9 100644 --- a/src/sentry/sentry_apps/external_requests/issue_link_requester.py +++ b/src/sentry/sentry_apps/external_requests/issue_link_requester.py @@ -82,7 +82,7 @@ def run(self) -> dict[str, Any]: "response": body, "installation_uuid": self.install.uuid, }, - status_code=503, + status_code=500, ) except RequestException as e: error_type = "issue-link-requester.error" @@ -98,7 +98,7 @@ def run(self) -> dict[str, Any]: raise SentryAppIntegratorError( message=f"Issue occured while trying to contact {self.sentry_app.slug} to link issue", webhook_context={"error_type": error_type, **extras}, - status_code=503, + status_code=500, ) if not self._validate_response(response): @@ -110,7 +110,7 @@ def run(self) -> dict[str, Any]: "installation_uuid": self.install.uuid, "uri": self.uri, }, - status_code=503, + status_code=500, ) return response diff --git a/src/sentry/sentry_apps/external_requests/select_requester.py b/src/sentry/sentry_apps/external_requests/select_requester.py index 04982b2d00589b..9cb0ff5d0275c4 100644 --- a/src/sentry/sentry_apps/external_requests/select_requester.py +++ b/src/sentry/sentry_apps/external_requests/select_requester.py @@ -79,7 +79,7 @@ def run(self) -> SelectRequesterResult: raise SentryAppIntegratorError( message=f"Something went wrong while getting options for Select FormField from {self.sentry_app.slug}", webhook_context={"error_type": message, **extra}, - status_code=503, + status_code=500, ) from e if not self._validate_response(response): @@ -139,7 +139,7 @@ def _format_response(self, resp: Sequence[dict[str, Any]]) -> SelectRequesterRes "error_type": "select-requester.missing-fields", "response": resp, }, - status_code=503, + status_code=500, ) choices.append([option["value"], option["label"]]) diff --git a/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_installation_external_issue_actions.py b/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_installation_external_issue_actions.py index 658c6ec089d1e2..3972207fbccf8f 100644 --- a/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_installation_external_issue_actions.py +++ b/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_installation_external_issue_actions.py @@ -75,7 +75,7 @@ def test_external_issue_doesnt_get_created(self): ) response = self.client.post(self.url, data=data, format="json") - assert response.status_code == 503 + assert response.status_code == 500 assert ( response.content == b'{"detail":"Issue occured while trying to contact testin to link issue"}' diff --git a/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_installation_external_requests.py b/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_installation_external_requests.py index f73d3ec877efc1..1f73c0e52107de 100644 --- a/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_installation_external_requests.py +++ b/tests/sentry/sentry_apps/api/endpoints/test_sentry_app_installation_external_requests.py @@ -90,4 +90,4 @@ def test_external_request_fails(self): ) url = self.url + f"?uri={self.project.id}" response = self.client.get(url, format="json") - assert response.status_code == 503 + assert response.status_code == 500