sadpandajoe commented on code in PR #43340:
URL: https://github.com/apache/superset/pull/43340#discussion_r3866888425
##########
superset/tasks/export_dashboard_excel.py:
##########
@@ -451,31 +491,45 @@ def _handle_export_failure(
def export_dashboard_excel(
self: Any, # pylint: disable=unused-argument
dashboard_id: int,
- user_id: int,
+ user_id: int | None,
active_data_mask: dict[str, Any],
job_id: str,
mode: str = EXPORT_MODE_DATA,
+ guest_token: GuestToken | None = None,
) -> None:
"""
Export a dashboard's charts to an ``.xlsx`` and record a download link.
:param dashboard_id: The dashboard to export
- :param user_id: The requesting user (the task runs with their permissions)
+ :param user_id: The requesting user (the task runs with their permissions),
+ or ``None`` for a guest/embedded requester
:param active_data_mask: Live dashboard filter state keyed by native
filter id
- :param job_id: Correlation id, also the Celery task id and S3 object name
+ :param job_id: Correlation id, also the Celery task id and storage object
name
:param mode: ``"data"`` streams every chart's tabular result; ``"images"``
embeds non-table charts as rendered images and keeps tables tabular
+ :param guest_token: The guest token payload when the requester is an
+ embedded guest; the guest user is reconstructed from it so the export
+ runs under the token's RLS rules and resource claims, never under an
+ elevated identity
"""
# pylint: disable=import-outside-toplevel
from superset.models.dashboard import Dashboard
requested_at = datetime.now(tz=timezone.utc)
- user = security_manager.get_user_by_id(user_id)
+ user = None
dashboard_title = ""
tmp_path: str | None = None
ttl = current_app.config["EXCEL_EXPORT_LINK_TTL_SECONDS"]
try:
+ # Resolve the user inside the protected block: if this raises (e.g. the
+ # guest role lookup fails), the ``finally`` below must still release
the
+ # lock the API acquired, and the failure status must still be recorded
+ # for pollers.
+ if user_id is not None:
+ user = security_manager.get_user_by_id(user_id)
+ elif guest_token:
Review Comment:
For a Public/anonymous requester, `get_user_id()` is also `None` but no
guest token is present, so this leaves `user` unset. The worker then runs chart
queries without the Public role; a permitted anonymous export can complete with
failures or an empty workbook. Should this load the anonymous user on the
no-token branch?
##########
superset/dashboards/api.py:
##########
@@ -1922,17 +1937,27 @@ def download_xlsx(self, job_id: uuid.UUID) ->
WerkzeugResponse:
description: The job_id from the export_xlsx response
responses:
302:
- description: Redirect to a pre-signed S3 download URL
+ description: Redirect to a signed storage download URL
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 = resolved
- return redirect(
- s3.generate_presigned_url(bucket, key, PRESIGNED_URL_TTL_SECONDS)
+ storage_backend = current_app.config["EXPORT_STORAGE"].get("backend")
Review Comment:
The job stores no backend identity, so a file uploaded under S3 will be
signed with the GCS backend after a storage migration or mixed rollout.
Existing links then fail during their advertised lifetime. Could the upload
backend be captured with the job and used for the download redirect?
##########
docs/docs/using-superset/exporting-dashboard-data.mdx:
##########
@@ -72,23 +91,25 @@ will not register.
## Configuration keys
-| Key | Default | Description
|
-| ------------------------------- | ---------------------- |
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
|
-| `EXCEL_EXPORT_S3_BUCKET` | `None` | Destination
bucket. Required; `501` if unset.
|
-| `EXCEL_EXPORT_S3_KEY_PREFIX` | `"dashboard-exports/"` | Key prefix:
`{prefix}{dashboard_id}/{job_id}.xlsx`.
|
-| `EXCEL_EXPORT_LINK_TTL_SECONDS` | `86400` | Lifetime of the
pre-signed download URL (24h).
|
-| `EXCEL_EXPORT_S3_CLIENT_KWARGS` | `{}` | Extra kwargs for
`boto3.client("s3", ...)` — e.g. `region_name`, or `endpoint_url` for
MinIO/LocalStack.
|
-| `EXCEL_EXPORT_TABLE_VIZ_TYPES` | `None` | Viz types kept
tabular in **Export Images to Excel** mode; every other type is embedded as an
image. `None` uses the built-in default (`table`, `pivot_table`,
`pivot_table_v2`). |
-| `EXCEL_EXPORT_QUERY_CONTEXT_BUILDER` | `None` | Optional
`Callable[[form_data_dict], dict \| None]` to build a query context for a chart
missing a saved one, tried before the built-in form-data rebuild. Point it at a
service that runs the chart's real frontend `buildQuery` to faithfully export
viz types the built-in rebuild can't handle. Must return `None` when it can't
build faithfully, so the export falls back. |
-
-Credentials and region resolve through the standard boto3 chain (environment
-variables, shared config, or instance role) unless overridden via
-`EXCEL_EXPORT_S3_CLIENT_KWARGS`. The worker needs `s3:PutObject` on the bucket.
+| Key | Default |
Description
|
+| -------------------------------------- | ---------------------- |
---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
|
+| `EXPORT_STORAGE["bucket"]` | unset |
Destination bucket. Required; `501` if unset.
|
+| `EXPORT_STORAGE["backend"]` | unset | Storage
backend instance: `S3ExportStorage()` (`superset.utils.s3`),
`GCSExportStorage()` (`superset.utils.gcs`), or a custom
`superset.utils.export_storage.ExportStorage` implementation. Required; `501`
if unset. |
Review Comment:
This updated configuration section still leaves the Limitations section
below saying guest-token exports are unsupported and lists SMTP as required,
although this change adds guest-token task reconstruction and polling. Could
the docs distinguish the no-email guest delivery path and document the
supported flow?
##########
superset-frontend/src/dashboard/components/menu/DownloadMenuItems/index.tsx:
##########
@@ -83,9 +89,28 @@ export const useDownloadMenuItems = (
canExportImage,
} = props;
- const { addDangerToast, addSuccessToast } = useToasts();
+ const { addDangerToast, addSuccessToast, addInfoToast } = useToasts();
const dataMask = useSelector((state: RootState) => state.dataMask);
- const SCREENSHOT_NODE_SELECTOR = ".dashboard";
+ // Embedded (iframe) sessions may have no email address, so they get
+ // delivery-neutral copy and a poll window that outlives the task budget.
+ const isEmbedded = isEmbeddedDashboard();
Review Comment:
This treats only iframe embedding as no-email. A top-level Public/anonymous
user is also returned as `user_id=None` and cannot receive email, but gets the
five-minute poll timeout while the task can run for ten minutes; a successful
export can then be orphaned. Could the delivery and polling behavior use the
requester identity/email rather than iframe detection?
--
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]
