gabotorresruiz commented on code in PR #43805:
URL: https://github.com/apache/superset/pull/43805#discussion_r3990273984
##########
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:
Addressed in ae78c7fac6. The worker now revalidates the guest token's `exp`
before entering `override_user` and aborts (recorded failure, lock released) if
it has passed, so a job picked up after expiry never runs chart queries under
stale claims. This deliberately goes a step beyond
`superset.tasks.async_queries` (which reconstructs from the queued payload
without rechecking). Two honest limits: the raw JWT isn't queued (only the
decoded payload is), so signature re-verification isn't possible here without
changing that contract, and stateless guest tokens have no revocation list, so
beyond `exp` the only invalidation is secret rotation (which invalidates
everything). A guest whose token expired also can't reach the `@protect`-ed
status endpoint to fetch the result, so nothing servable is lost by aborting.
##########
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:
Fixed in ae78c7fac6. `download_xlsx` is intentionally login-free (no
`@protect`), so it now maps to no permission and the operation declares
`security: []` in the regenerated spec. FAB stamps `jwt`/`api_key` on every
permissioned method regardless of the docstring, so the fix was to give this
method no permission via `get_method_permission` (correct, since it's genuinely
public), which lets the empty security requirement through. Verified: the
download op now shows `security: []` while `export_xlsx_status` still requires
auth.
--
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]
