sadpandajoe commented on code in PR #43805:
URL: https://github.com/apache/superset/pull/43805#discussion_r3988384606
##########
docs/static/resources/openapi.json:
##########
@@ -18296,6 +18296,83 @@
]
}
},
+ "/api/v1/dashboard/export_xlsx/download/{job_id}/": {
+ "get": {
+ "description": "Intentionally requires no login: the unguessable
job_id, emailed only to the original requester (or handed to their own session
via export_xlsx_status), is the credential. The dashboard access check already
ran once, when the export was requested -- see
security_manager.raise_for_access in export_xlsx. The file streams through
Superset with the deployment's own storage credentials instead of redirecting
to a signed storage URL, so it works for ambient identities that cannot sign
(e.g. workload identity federation) and never mints a bearer URL Superset
cannot observe or revoke.",
+ "parameters": [
+ {
+ "description": "The job_id from the export_xlsx response",
+ "in": "path",
+ "name": "job_id",
+ "required": true,
+ "schema": {
+ "format": "uuid",
+ "type": "string"
+ }
+ }
+ ],
+ "responses": {
+ "200": {
+ "description": "The .xlsx file as an attachment"
+ },
+ "410": {
+ "description": "The link is unknown, expired, or the export failed"
+ },
+ "501": {
+ "description": "Excel export is not configured on this server"
+ }
+ },
+ "security": [
Review Comment:
This spec still requires JWT or API-key auth for the intentionally
login-free bearer endpoint, so generated clients or gateways enforcing it can
reject emailed downloads with no active session. Could this operation declare
an empty security requirement?
##########
superset/tasks/export_dashboard_excel.py:
##########
@@ -389,30 +441,108 @@ def _build_workbook(
)
errored.setdefault(email.ERROR_GENERAL, []).append(label)
- if writer.sheet_count == 0:
+ # The workbook itself carries the skipped-charts list: sessions with
+ # no email (guests, Public role) have no other way to learn that part
+ # of the requested workbook was omitted.
+ if writer.sheet_count == 0 or errored:
flat = [label for labels in errored.values() for label in labels]
- writer.add_summary_sheet(
- "Export Summary",
- ["No chart data could be exported.", *flat],
+ header = (
+ "No chart data could be exported."
+ if writer.sheet_count == 0
+ else "Charts that could not be exported:"
)
+ writer.add_summary_sheet("Export Summary", [header, *flat])
finally:
writer.close()
return errored
-def _send_failure_email(
- user: Any, dashboard_title: str, requested_at: datetime
+_GENERIC_FAILURE_MESSAGE = (
+ "An error occurred while generating the file. Please try again, or "
+ "contact your administrator if the problem persists."
+)
+
+
+def _handle_export_failure(
+ user: Any, dashboard_title: str, requested_at: datetime, job_id: str, ttl:
int
) -> None:
- if not (user and getattr(user, "email", None)):
- return
+ """Notify the requester their export failed: email them if they have an
+ address on file, and record a pollable failure status either way (a
+ session with no email, e.g. an embedded/guest dashboard, has no other way
+ to learn the export failed than polling ``export_xlsx/status/<job_id>/``).
+ """
+ # Status first: on a soft timeout only 60s remain before the hard kill,
+ # and a slow SMTP send must not cost pollers the failure status.
try:
- email.send_export_email(
- user.email,
- email.build_subject(dashboard_title, success=False),
- email.build_failure_email(dashboard_title, requested_at),
+ # Naive local time: KeyValueEntry.is_expired() compares against naive
+ # datetime.now(), like every other writer into this store.
+ mark_export_failed(
+ uuid.UUID(job_id),
+ _GENERIC_FAILURE_MESSAGE,
+ datetime.now() + timedelta(seconds=ttl),
+ )
+ except Exception: # pylint: disable=broad-except
+ logger.exception("Failed to record export failure status for %s",
job_id)
+ if user and getattr(user, "email", None):
+ try:
+ email.send_export_email(
+ user.email,
+ email.build_subject(dashboard_title, success=False),
+ email.build_failure_email(dashboard_title, requested_at),
+ )
+ except Exception: # pylint: disable=broad-except
+ logger.exception("Failed to send export failure email")
+
+
+def _resolve_export_storage(
+ dashboard_id: int, job_id: str
+) -> tuple[ExportStorage, str, str]:
+ """The configured storage backend, bucket, and this export's object key.
+
+ The API already rejects the request with 501 when either the bucket or
+ the backend is unset, so reaching this unconfigured normally means
+ EXPORT_STORAGE was cleared after the job was enqueued (or the task
+ was invoked directly, bypassing the API). Fail with a clear message
+ instead of an opaque storage-SDK error.
+ """
+ storage_config = current_app.config["EXPORT_STORAGE"]
+ bucket = storage_config.get("bucket")
+ storage_backend = storage_config.get("backend")
+ if not bucket or storage_backend is None:
+ raise SupersetException(
+ "Excel export is not configured on this server: "
+ "EXPORT_STORAGE needs both a 'bucket' and a 'backend' "
+ "(e.g. superset.utils.s3.S3ExportStorage())."
)
+ key_prefix = storage_config.get("key_prefix", "dashboard-exports/")
+ if callable(key_prefix):
+ # A callable prefix is resolved per export, for deployments where it
+ # is only known in task context (e.g. a multi-tenant installation
+ # scoping a shared bucket per tenant).
+ key_prefix = key_prefix()
+ return storage_backend, bucket, f"{key_prefix}{dashboard_id}/{job_id}.xlsx"
+
+
+def _mark_running(job_id: str) -> None:
+ """Tell pollers execution has begun (vs. queued); best-effort, the export
+ must not fail over a status write."""
+ try:
+ expires_at = datetime.now() + timedelta(seconds=EXPORT_HARD_TIME_LIMIT
+ 300)
+ mark_export_running(uuid.UUID(job_id), expires_at)
except Exception: # pylint: disable=broad-except
- logger.exception("Failed to send export failure email")
+ logger.exception("Failed to record running status for %s", job_id)
+
+
+def _resolve_requesting_user(
+ user_id: int | None, guest_token: GuestToken | None
+) -> Any:
+ if user_id is not None:
+ return security_manager.get_user_by_id(user_id)
+ if guest_token:
+ return security_manager.get_guest_user_from_token(guest_token)
Review Comment:
This rebuilds the guest principal directly from queued claims, so a job
picked up after `exp`—or after a revocation cutoff/version bump—still runs
chart queries under the stale resource and RLS claims. Could the worker
revalidate expiry and revocation before entering `override_user`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
