diff --git a/src/sentry/api/endpoints/event_attachment_details.py b/src/sentry/api/endpoints/event_attachment_details.py index 002abed3a7b2..74d136db799f 100644 --- a/src/sentry/api/endpoints/event_attachment_details.py +++ b/src/sentry/api/endpoints/event_attachment_details.py @@ -1,7 +1,7 @@ import posixpath import sentry_sdk -from django.http import StreamingHttpResponse +from django.http import HttpResponseRedirect, StreamingHttpResponse from drf_spectacular.utils import OpenApiParameter, extend_schema from rest_framework.request import Request from rest_framework.response import Response @@ -44,7 +44,9 @@ description=( "If this parameter is present, the response will be a binary file download " "instead of JSON metadata. The value does not matter — any value (including " - "empty) triggers the download." + "empty) triggers the download. Depending on where the attachment is stored, " + "the response may be a redirect to the storage service, so clients must follow " + "redirects." ), ) @@ -87,7 +89,12 @@ class EventAttachmentDetailsEndpoint(ProjectEndpoint): } permission_classes = (EventAttachmentDetailsPermission,) - def download(self, attachment: EventAttachment, request: Request) -> StreamingHttpResponse: + def download( + self, attachment: EventAttachment, request: Request + ) -> HttpResponseRedirect | StreamingHttpResponse: + if attachment.uses_objectstore(): + return HttpResponseRedirect(attachment.get_objectstore_presigned_url(request)) + name = posixpath.basename(" ".join(attachment.name.split())) accept_encoding = parse_accept_encoding(request.headers.get("Accept-Encoding", "")) blob_stream = attachment.get_blob_stream(accept_encoding) @@ -131,6 +138,7 @@ def get( Response[EventAttachmentSerializerResponse] | Response[None] | Response[DetailResponse] + | HttpResponseRedirect | StreamingHttpResponse ): """ diff --git a/src/sentry/models/eventattachment.py b/src/sentry/models/eventattachment.py index e9a1672c4fe8..3f833e11c4a6 100644 --- a/src/sentry/models/eventattachment.py +++ b/src/sentry/models/eventattachment.py @@ -13,6 +13,7 @@ from django.db import models from django.db.models.expressions import DatabaseDefault from django.db.models.functions import Now +from django.http import HttpRequest from django.utils import timezone from objectstore_client import TimeToLive @@ -22,7 +23,11 @@ from sentry.db.models.fields.bounded import BoundedIntegerField from sentry.db.models.manager.base_query_set import BaseQuerySet from sentry.models.files.utils import get_size_and_checksum, get_storage -from sentry.objectstore import default_attachment_retention, get_attachments_session +from sentry.objectstore import ( + default_attachment_retention, + get_attachments_session, + get_download_redirect_url, +) from sentry.objectstore.metrics import measure_storage_operation from sentry.options.rollout import in_random_rollout @@ -159,6 +164,28 @@ def delete(self, *args: Any, **kwargs: Any) -> tuple[int, dict[str, int]]: return rv + def uses_objectstore(self) -> bool: + """Whether this attachment's payload is stored in Objectstore.""" + return self.blob_path is not None and self.blob_path.startswith(V2_PREFIX) + + def get_objectstore_presigned_url(self, request: HttpRequest) -> str: + """ + Returns the URL that `request` should be redirected to in order to download this + attachment directly from Objectstore. + + This function should only be called if this attachment is Objectstore-backed, it + will raise an exception otherwise. + """ + if not self.uses_objectstore(): + raise ValueError("attachment is not stored in Objectstore") + assert self.blob_path is not None + + organization_id = _get_organization(self.project_id) + session = get_attachments_session(organization_id, self.project_id) + return get_download_redirect_url( + request, session, organization_id, self.blob_path.removeprefix(V2_PREFIX) + ) + def getfile(self) -> IO[bytes]: if not self.blob_path: return BytesIO(b"") @@ -195,7 +222,8 @@ def get_blob_stream(self, accept_encoding: list[str]) -> BlobStream: bytes can be transferred directly to the client. For all other blob types (inline, V1), delegates to :meth:`getfile`. """ - if self.blob_path and self.blob_path.startswith(V2_PREFIX): + if self.uses_objectstore(): + assert self.blob_path is not None key = self.blob_path.removeprefix(V2_PREFIX) session = get_attachments_session(_get_organization(self.project_id), self.project_id) response = session.get(key, accept_encoding=accept_encoding or None) diff --git a/tests/sentry/api/endpoints/test_event_attachment_details.py b/tests/sentry/api/endpoints/test_event_attachment_details.py index da3ad3d9a688..1a2300f00124 100644 --- a/tests/sentry/api/endpoints/test_event_attachment_details.py +++ b/tests/sentry/api/endpoints/test_event_attachment_details.py @@ -1,4 +1,7 @@ +from unittest.mock import patch + import pytest +import requests from django.test import override_settings from sentry.attachments.base import CachedAttachment @@ -74,7 +77,11 @@ def test_simple(self) -> None: def test_download(self) -> None: self.login_as(user=self.user) - self.create_attachment() + # Attachments not backed by Objectstore are streamed through Sentry. + with override_options({"objectstore.enable_for.attachments": 0}): + self.create_attachment() + + assert not self.attachment.uses_objectstore() path1 = f"/api/0/projects/{self.organization.slug}/{self.project.slug}/events/{self.event.event_id}/attachments/{self.attachment.id}/?download" response = self.client.get(path1) @@ -87,54 +94,47 @@ def test_download(self) -> None: @with_feature("organizations:event-attachments") @requires_objectstore - def test_download_objectstore(self) -> None: + def test_download_objectstore_redirects_internal_to_objectstore(self) -> None: self.login_as(user=self.user) with override_options({"objectstore.enable_for.attachments": 1}): attachment = self.create_attachment() - - assert attachment.blob_path is not None - assert attachment.blob_path.startswith("v2/") - - path1 = f"/api/0/projects/{self.organization.slug}/{self.project.slug}/events/{self.event.event_id}/attachments/{attachment.id}/?download" - response = self.client.get(path1) - - assert response.status_code == 200, response.content - assert response.get("Content-Disposition") == 'attachment; filename="hello.png"' - assert response.get("Content-Length") == str(attachment.size) - assert response.get("Content-Type") == "image/png" - assert close_streaming_response(response) == ATTACHMENT_CONTENT + assert attachment.uses_objectstore() + + path = f"/api/0/projects/{self.organization.slug}/{self.project.slug}/events/{self.event.event_id}/attachments/{attachment.id}/?download" + with patch("sentry.auth.system.is_internal_ip", return_value=True): + response = self.client.get(path) + + assert response.status_code == 302 + location = response["Location"] + # Internal callers are redirected straight to Objectstore, not through the cell proxy. + assert "/organizations/" not in location + # In dev/test the host may be rewritten to the Docker-internal `objectstore` + # hostname (so Symbolicator can reach it); rewrite it back so we can follow it. + downloaded = requests.get(location.replace("://objectstore:", "://127.0.0.1:")) + assert downloaded.status_code == 200, downloaded.text + assert downloaded.content == ATTACHMENT_CONTENT @with_feature("organizations:event-attachments") @requires_objectstore - def test_download_objectstore_accept_encoding(self) -> None: - import zstandard - + def test_download_objectstore_redirects_external_to_cell_proxy(self) -> None: self.login_as(user=self.user) with override_options({"objectstore.enable_for.attachments": 1}): attachment = self.create_attachment() + assert attachment.uses_objectstore() - assert attachment.blob_path is not None - assert attachment.blob_path.startswith("v2/") - - path1 = f"/api/0/projects/{self.organization.slug}/{self.project.slug}/events/{self.event.event_id}/attachments/{attachment.id}/?download" - response = self.client.get(path1, HTTP_ACCEPT_ENCODING="zstd") + path = f"/api/0/projects/{self.organization.slug}/{self.project.slug}/events/{self.event.event_id}/attachments/{attachment.id}/?download" + with patch("sentry.auth.system.is_internal_ip", return_value=False): + response = self.client.get(path) - assert response.status_code == 200, response.content - assert response.get("Content-Disposition") == 'attachment; filename="hello.png"' - assert response.get("Content-Type") == "image/png" - - body = close_streaming_response(response) - if response.get("Content-Encoding") == "zstd": - # Object was stored compressed; verify we got valid compressed bytes - dctx = zstandard.ZstdDecompressor() - with dctx.stream_reader(body) as reader: - assert reader.read() == ATTACHMENT_CONTENT - else: - # Object was stored uncompressed; content-length should be present - assert response.get("Content-Length") == str(attachment.size) - assert body == ATTACHMENT_CONTENT + assert response.status_code == 302 + location = response["Location"] + assert ( + f"/organizations/{self.organization.id}/objectstore/v1/objects/attachments/" + in location + ) + assert "os_auth=" in location @with_feature("organizations:event-attachments") def test_zero_sized_attachment(self) -> None: @@ -207,7 +207,10 @@ def test_delete_activity_with_group(self) -> None: class EventAttachmentDetailsPermissionTest(PermissionTestCase, CreateAttachmentMixin): def setUp(self) -> None: super().setUp() - self.create_attachment() + # `PermissionTestCase` treats any 3xx as "access denied", so keep the attachment on + # the streaming path rather than the Objectstore redirect. + with override_options({"objectstore.enable_for.attachments": 0}): + self.create_attachment() self.path = f"/api/0/projects/{self.organization.slug}/{self.project.slug}/events/{self.event.event_id}/attachments/{self.attachment.id}/?download" @with_feature("organizations:event-attachments")