gabotorresruiz commented on code in PR #43805:
URL: https://github.com/apache/superset/pull/43805#discussion_r4084254811
##########
docs/static/resources/openapi.json:
##########
@@ -18435,6 +18438,76 @@
]
}
},
+ "/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": [],
+ "summary": "Download a completed dashboard Excel export",
+ "tags": [
+ "Dashboards"
+ ]
+ }
+ },
+ "/api/v1/dashboard/export_xlsx/status/{job_id}/": {
+ "get": {
+ "description": "For a session with no email address to be notified at
(e.g. an embedded/guest session), the frontend polls this endpoint with the
job_id from the export_xlsx response instead of waiting for an email. Behind
the same @protect() as the export request itself, unlike the login-free
download_xlsx stream (which also has to work when clicked from a plain email
link, possibly with no active session at all).",
+ "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": "Job status: {\"status\": \"pending\"} while
queued, {\"status\": \"running\"} once a worker has started executing,
{\"status\": \"ready\", \"download_url\": \"...\"} once the file is available,
or {\"status\": \"error\", \"message\": \"...\"} if the export failed."
+ },
+ "401": {
Review Comment:
Fixed: the status route now documents `403` alongside `200` and `401`, and
the spec is regenerated from source (the diff is exactly that entry).
##########
superset/dashboards/api.py:
##########
@@ -1844,21 +1914,162 @@ def export_xlsx(self, pk: int) -> WerkzeugResponse:
export_dashboard_excel.apply_async(
kwargs={
"dashboard_id": dashboard.id,
- "user_id": g.user.id,
+ "user_id": user_id,
"active_data_mask": payload.get("active_data_mask", {}),
"job_id": job_id,
"mode": payload.get("mode", "data"),
+ "guest_token": guest_token_payload,
+ "lock_token": acquire.token,
},
task_id=job_id,
)
except Exception:
# If enqueuing fails (e.g. broker down) the task will never run to
# release the lock, so free it now rather than block exports until
# the TTL expires.
- ReleaseDistributedLock(EXPORT_LOCK_NAMESPACE, lock_params).run()
+ ReleaseDistributedLock(
+ EXPORT_LOCK_NAMESPACE, lock_params, token=acquire.token
+ ).run()
raise
return self.response(202, job_id=job_id)
+ @expose("/export_xlsx/status/<uuid:job_id>/", methods=("GET",))
+ @protect()
+ @safe
+ @statsd_metrics
+ def export_xlsx_status(self, job_id: uuid.UUID) -> WerkzeugResponse:
+ """Poll the status of an in-flight or completed Excel export.
+ ---
+ get:
+ summary: Poll the status of a dashboard Excel export job
+ description: >-
+ For a session with no email address to be notified at (e.g. an
+ embedded/guest session), the frontend polls this endpoint with the
+ job_id from the export_xlsx response instead of waiting for an
+ email. Behind the same @protect() as the export request itself,
+ unlike the login-free download_xlsx stream (which also has to
+ work when clicked from a plain email link, possibly with no
+ active session at all).
+ parameters:
+ - in: path
+ schema:
+ type: string
+ format: uuid
+ name: job_id
+ description: The job_id from the export_xlsx response
+ responses:
+ 200:
+ description: >-
+ Job status: {"status": "pending"} while queued,
+ {"status": "running"} once a worker has started executing,
+ {"status": "ready", "download_url": "..."} once the file is
+ available, or {"status": "error", "message": "..."} if the
+ export failed.
+ 401:
+ $ref: '#/components/responses/401'
+ """
+ payload = get_export_status(job_id)
+ if payload is None:
+ return self.response(200, status="pending")
+ if payload.get("status") == STATUS_READY:
+ # Never report ready for a link the download endpoint will refuse:
+ # an unset backend (config cleared since upload) 501s there, and a
+ # mismatched backend 410s. Both mean the file cannot be served.
+ storage_backend =
current_app.config["EXPORT_STORAGE"].get("backend")
+ if storage_backend is None or not _link_backend_matches(
+ payload.get("backend")
+ ):
+ return self.response(
+ 200, status=STATUS_ERROR, message="This download has
expired."
+ )
+ return self.response(
+ 200, status=STATUS_READY, download_url=download_path(job_id)
+ )
+ if payload.get("status") == STATUS_ERROR:
+ return self.response(
+ 200, status=STATUS_ERROR, message=payload.get("message")
+ )
+ if payload.get("status") == STATUS_RUNNING:
+ return self.response(200, status=STATUS_RUNNING)
+ return self.response(200, status="pending")
+
+ def get_method_permission(self, method_name: str) -> str:
+ # download_xlsx is intentionally login-free (no @protect): the
+ # unguessable job_id is the credential, and dashboard access was
+ # already checked when the export was requested. Map it to no
+ # permission so FAB neither requires auth nor advertises a security
+ # requirement for it in the OpenAPI spec.
+ if method_name == "download_xlsx":
+ return ""
+ return super().get_method_permission(method_name)
+
+ @expose("/export_xlsx/download/<uuid:job_id>/", methods=("GET",))
+ @safe
+ @statsd_metrics
+ def download_xlsx(self, job_id: uuid.UUID) -> WerkzeugResponse:
+ """Stream a completed Excel export from storage.
+ ---
+ get:
+ summary: Download a completed dashboard Excel export
+ security: []
+ 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:
+ - in: path
+ schema:
+ type: string
+ format: uuid
+ name: job_id
+ description: The job_id from the export_xlsx response
+ 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
+ """
+ resolved = resolve_download_link(job_id)
+ if resolved is None:
+ return self.response(410, message="This download link has
expired.")
+ bucket, key, uploaded_backend = resolved
+ storage_backend = current_app.config["EXPORT_STORAGE"].get("backend")
+ if storage_backend is None:
+ # A link can only exist if a backend was configured when the export
+ # ran, so reaching this means the config was cleared since then.
+ return self.response(
+ 501, message="Excel export is not configured on this server."
+ )
+ if not _link_backend_matches(uploaded_backend):
+ # Don't read another backend's upload; expire the link instead.
+ return self.response(410, message="This download link has
expired.")
+ try:
+ # Existence is checked eagerly, so a missing object is a clean 410.
+ size, chunks = storage_backend.download(bucket, key)
+ except FileNotFoundError:
+ return self.response(410, message="This download link has
expired.")
+ return Response(
+ stream_with_context(chunks),
+ mimetype=(
+
"application/vnd.openxmlformats-officedocument.spreadsheetml.sheet"
+ ),
+ headers={
Review Comment:
Fixed. Every branch of `download_xlsx` now carries `Cache-Control: no-store,
no-cache, private, max-age=0` plus `Pragma: no-cache`, applied through
`after_this_request` so the `410` and `501` answers get it too, not only the
streamed `200`. It mirrors what `send_export_zip` already does for the bundle
export.
I applied the same to `export_xlsx_status`: a proxy caching a `"pending"`
answer would strand a poller on a stale status, and guests have nothing but
that poll.
Verified on a live instance: `GET` on a ready link answers with those
headers, `HEAD` answers them without a body, and an expired link answers `410`
with them as well.
##########
superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx:
##########
@@ -167,31 +213,134 @@ export const useDownloadMenuItems = (
}
};
+ const triggerExportDownload = (downloadUrl: string) => {
+ // Stream the file straight to disk via a hidden iframe. The endpoint sends
+ // Content-Disposition: attachment, so the browser saves it without
+ // navigating the dashboard away (fatal inside an embedded iframe) and
+ // without buffering the whole workbook in tab memory the way
fetch().blob()
+ // would. The status endpoint already confirmed the link is ready and
+ // backend-matched, so the only failure left is the narrow race where the
+ // object is removed between that check and this click; such an error
+ // response loads invisibly in the iframe and leaves the page untouched.
+ const iframe = document.createElement('iframe');
+ iframe.style.display = 'none';
+ iframe.src = downloadUrl;
Review Comment:
Agreed, and fixed. The download now goes through a plain anchor to the
attachment instead of a hidden iframe: no frame, so no `frame-src` exception,
the file streams to disk, and the page is not navigated because the response is
an attachment. `src/utils/export.ts` made the same move for the bundle export
for the same reason.
Two things I ran into while proving it, since they shaped the result. An
anchor carrying the `download` attribute is dropped silently by Chrome once
user activation has expired, which after a polling delay is always; against the
server access log, such a click never arrived at all while a plain anchor
fetched the file. And a plain anchor would navigate on an error response, which
was the objection to the earlier redirect, so the link is confirmed with a
`HEAD` request first and a failure becomes a toast rather than a navigation.
The success toast now follows that check instead of preceding it.
Reproduced your scenario end to end: a page under `frame-src 'none'` with
storage stalling 4.5 seconds logs a `frame-src` violation for the iframe and
the server never sees that request, while the anchor variant is delivered in
full. In the real component, without user activation, the trace is `POST 202`,
`GET status 200`, `HEAD download 200`, `GET download 200` with the workbook
delivered, and no frame is created.
One trade-off to be upfront about: the anchor starts a navigation that the
attachment then cancels, so a `beforeunload` guard fires on the way. The
dashboard installs one only while there are unsaved edits, and the Download
submenu is reachable in edit mode, so exporting in that exact state shows the
"unsaved changes" prompt: Leave still downloads without unloading the page,
Cancel aborts the download. The iframe did not have that property. If you would
rather not carry it, gating the Excel items on `!editMode` is a small
follow-up, and exporting from an unsaved edit is questionable anyway since the
export runs the saved query contexts.
##########
superset/dashboards/api.py:
##########
@@ -1623,13 +1663,20 @@ def export(self, **kwargs: Any) -> Response:
$ref: '#/components/responses/400'
401:
$ref: '#/components/responses/401'
+ 403:
+ $ref: '#/components/responses/403'
404:
$ref: '#/components/responses/404'
422:
$ref: '#/components/responses/422'
500:
$ref: '#/components/responses/500'
"""
+ # A bundle carries dataset SQL and database metadata the embedded view
+ # never exposes, and no guest flow consumes this endpoint.
+ if security_manager.is_guest_user():
Review Comment:
You are right, fixed. Both guards now key on `get_user_id() is None`, the
same predicate the image export uses. I confirmed the mechanism you describe in
FAB: `protect()` returns before any auth check when `is_item_public` is true
(`security/decorators.py`), so with `can_export` on Public the bundle and
example routes would have answered unauthenticated requests.
Verified live with Public holding `can_export`: an anonymous request gets
`403` from both routes, a guest token gets `403` from both, an admin still gets
`200` from both, and the Excel routes are untouched. The menu no longer offers
Export YAML or Export as Example to sessions without a user id, since the API
refuses them.
##########
superset/tasks/export_dashboard_excel.py:
##########
@@ -389,30 +447,121 @@ 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])
Review Comment:
Fixed. The grouping the email uses is now a shared `errored_groups()` in
`email.py`, and the summary sheet writes each group's note followed by its
charts instead of one flat list. Covered by a unit test that checks the "no
saved query context" note precedes the chart it applies to.
##########
superset/tasks/export_dashboard_excel.py:
##########
@@ -389,30 +447,121 @@ 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(
Review Comment:
Good catch, fixed. `_handle_export_failure` now reads the record first and
returns without writing (or emailing a failure) when it already reports ready,
so a soft time limit landing after `create_download_link` cannot replace the
bucket and key with an error.
While writing the test I found the window is narrower than it first looks:
the success email send is wrapped in `except Exception`, which swallows
`SoftTimeLimitExceeded` too, so the interrupt has to land between the upsert
committing inside `create_download_link` and its return, or on the lines right
after. The test models exactly that, and `mark_export_failed` is not called.
##########
superset/tasks/export_dashboard_excel.py:
##########
@@ -466,16 +629,22 @@ def export_dashboard_excel(
tmp_path, dashboard, active_data_mask, job_id, mode, user
)
- bucket = current_app.config["EXCEL_EXPORT_S3_BUCKET"]
- key = (
- f"{current_app.config['EXCEL_EXPORT_S3_KEY_PREFIX']}"
- f"{dashboard_id}/{job_id}.xlsx"
+ storage_backend, bucket, key =
_resolve_export_storage(dashboard_id, job_id)
+ storage_backend.upload_file(tmp_path, bucket, key)
+ # Naive local time to match KeyValueEntry.is_expired()'s naive
+ # datetime.now() comparison. Guests hold the link only for the
+ # polling window (no email fallback to revisit later), so their
+ # links need not outlive the short credential that authorized them.
+ link_ttl = min(ttl, GUEST_LINK_TTL_SECONDS) if guest_token else ttl
Review Comment:
Fixed: the clamp keys on `user_id is None`, so an anonymous requester gets
the same short link a guest does. Added the anonymous case next to the existing
guest one.
--
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]
