Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(sentry apps): New error design for external request flow #83678

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 30 additions & 31 deletions src/sentry/sentry_apps/api/bases/sentryapps.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -103,10 +101,9 @@ def has_object_permission(self, request: Request, view, context: RpcUserOrganiza

# User must be a part of the Org they're trying to create the app in.
if context.organization.status != OrganizationStatus.ACTIVE or not context.member:
raise SentryAppIntegratorError(
APIUnauthorized(
"User must be a part of the Org they're trying to create the app in"
)
raise SentryAppError(
message="User must be a part of the Org they're trying to create the app in",
status_code=401,
)

assert request.method, "method must be present in request to get permissions"
Expand All @@ -131,7 +128,12 @@ def handle_exception_with_details(self, request, exc, handler_context=None, scop

def _handle_sentry_app_exception(self, exception: Exception):
if isinstance(exception, SentryAppIntegratorError) or isinstance(exception, SentryAppError):
response = Response({"detail": str(exception)}, status=exception.status_code)
response_body: dict[str, Any] = {"detail": exception.message}
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
return response

Expand All @@ -154,7 +156,7 @@ def _get_organization_slug(self, request: Request):
organization_slug = request.data.get("organization")
if not organization_slug or not isinstance(organization_slug, str):
error_message = "Please provide a valid value for the 'organization' field."
raise SentryAppError(ResourceDoesNotExist(error_message))
raise SentryAppError(message=error_message, status_code=404)
return organization_slug

def _get_organization_for_superuser_or_staff(
Expand All @@ -166,7 +168,7 @@ def _get_organization_for_superuser_or_staff(

if context is None:
error_message = f"Organization '{organization_slug}' does not exist."
raise SentryAppError(ResourceDoesNotExist(error_message))
raise SentryAppError(message=error_message, status_code=404)

return context

Expand All @@ -178,7 +180,7 @@ def _get_organization_for_user(
)
if context is None or context.member is None:
error_message = f"User does not belong to the '{organization_slug}' organization."
raise SentryAppIntegratorError(PermissionDenied(to_single_line_str(error_message)))
raise SentryAppError(message=to_single_line_str(error_message), status_code=403)
return context

def _get_org_context(self, request: Request) -> RpcUserOrganizationContext:
Expand Down Expand Up @@ -260,10 +262,13 @@ def has_object_permission(self, request: Request, view, sentry_app: RpcSentryApp
# if app is unpublished, user must be in the Org who owns the app.
if not sentry_app.is_published:
if not any(sentry_app.owner_id == org.id for org in organizations):
raise SentryAppIntegratorError(
APIUnauthorized(
"User must be in the app owner's organization for unpublished apps"
)
raise SentryAppError(
message="User must be in the app owner's organization for unpublished apps",
status_code=403,
public_context={
"integration": sentry_app.slug,
"user_organizations": [org.slug for org in organizations],
},
)

# TODO(meredith): make a better way to allow for public
Expand Down Expand Up @@ -299,9 +304,7 @@ def convert_args(
try:
sentry_app = SentryApp.objects.get(slug__id_or_slug=sentry_app_id_or_slug)
except SentryApp.DoesNotExist:
raise SentryAppIntegratorError(
ResourceDoesNotExist("Could not find the requested sentry app"), status_code=404
)
raise SentryAppError(message="Could not find the requested sentry app", status_code=404)

self.check_object_permissions(request, sentry_app)

Expand All @@ -320,9 +323,7 @@ def convert_args(
else:
sentry_app = app_service.get_sentry_app_by_slug(slug=sentry_app_id_or_slug)
if sentry_app is None:
raise SentryAppIntegratorError(
ResourceDoesNotExist("Could not find the requested sentry app"), status_code=404
)
raise SentryAppError(message="Could not find the requested sentry app", status_code=404)

self.check_object_permissions(request, sentry_app)

Expand Down Expand Up @@ -353,8 +354,10 @@ def has_object_permission(self, request: Request, view, organization):
else ()
)
if not any(organization.id == org.id for org in organizations):
raise SentryAppIntegratorError(
APIUnauthorized("User must belong to the given organization"), status_code=403
raise SentryAppError(
message="User must belong to the given organization",
status_code=403,
public_context={"user_organizations": [org.slug for org in organizations]},
)
assert request.method, "method must be present in request to get permissions"
return ensure_scoped_permission(request, self.scope_map.get(request.method))
Expand All @@ -379,9 +382,7 @@ def convert_args(self, request: Request, organization_id_or_slug, *args, **kwarg
)

if organization is None:
raise SentryAppIntegratorError(
ResourceDoesNotExist("Could not find requested organization"), status_code=404
)
raise SentryAppError(message="Could not find requested organization", status_code=404)
self.check_object_permissions(request, organization)

kwargs["organization"] = organization
Expand Down Expand Up @@ -437,9 +438,7 @@ def has_object_permission(self, request: Request, view, installation):
or not org_context.member
or org_context.organization.status != OrganizationStatus.ACTIVE
):
raise SentryAppIntegratorError(
ResourceDoesNotExist("Given organization is not valid"), status_code=404
)
raise SentryAppError(message="Given organization is not valid", status_code=404)

assert request.method, "method must be present in request to get permissions"
return ensure_scoped_permission(request, self.scope_map.get(request.method))
Expand All @@ -452,8 +451,8 @@ def convert_args(self, request: Request, uuid, *args, **kwargs):
installations = app_service.get_many(filter=dict(uuids=[uuid]))
installation = installations[0] if installations else None
if installation is None:
raise SentryAppIntegratorError(
ResourceDoesNotExist("Could not find given sentry app installation"),
raise SentryAppError(
message="Could not find given sentry app installation",
status_code=404,
)

Expand Down
Original file line number Diff line number Diff line change
@@ -1,21 +1,19 @@
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
from sentry.sentry_apps.api.serializers.platform_external_issue import (
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

Expand Down Expand Up @@ -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"]
Expand All @@ -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())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -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():
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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)
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +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(e) from e
raise SentryAppSentryError(
message="Failed to create external issue obj",
) from e
3 changes: 1 addition & 2 deletions src/sentry/sentry_apps/external_issues/issue_link_creator.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -33,7 +32,7 @@ def run(self) -> PlatformExternalIssue:

def _verify_action(self) -> None:
if self.action not in VALID_ACTIONS:
raise SentryAppSentryError(APIUnauthorized(f"Invalid action '{self.action}'"))
raise SentryAppSentryError(message=f"Invalid action: {self.action}", status_code=401)

def _make_external_request(self) -> dict[str, Any]:
response = IssueLinkRequester(
Expand Down
Loading
Loading