gabotorresruiz commented on code in PR #43805: URL: https://github.com/apache/superset/pull/43805#discussion_r4050084226
########## superset/dashboards/excel_export/download_link.py: ########## @@ -0,0 +1,160 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Status tracking and long-lived download links for dashboard Excel exports. + +The download link shared with the requester is a Superset endpoint, never a +raw or signed storage URL: signed URLs are transferable bearer credentials +Superset cannot observe or revoke once issued, their real lifetime is bounded +by the signing credentials' own session (not just their nominal expiry), and +some ambient identities (e.g. direct workload identity federation) cannot +sign at all. The link's lifetime is enforced by this module via the +``key_value`` store's ``expires_on``, and the file itself streams through +Superset with the deployment's storage credentials at click time. + +The download endpoint (``download_xlsx``) intentionally requires no login: +the access-control decision for the underlying dashboard was already enforced +once, when the export was originally requested (see +``security_manager.raise_for_access`` in +``superset.dashboards.api.export_xlsx``); the unguessable key handed only to +that requester is the "possession of the link is the credential" model. + +Every entry is keyed by ``job_id`` -- the same id the ``export_xlsx`` POST +response hands back -- rather than a separately-generated identifier, so a +caller that only has the job id (e.g. a polling frontend for a session with +no email on file, such as an embedded/guest dashboard) can resolve both +status and, once ready, a download link from that one id. +""" + +from __future__ import annotations + +from datetime import datetime +from typing import Any +from uuid import UUID + +from flask import current_app + +from superset.daos.key_value import KeyValueDAO +from superset.key_value.types import JsonKeyValueCodec, KeyValueResource +from superset.utils.decorators import transaction +from superset.utils.urls import headless_url + +RESOURCE = KeyValueResource.EXCEL_EXPORT_DOWNLOAD +CODEC = JsonKeyValueCodec() + +DOWNLOAD_PATH = "/api/v1/dashboard/export_xlsx/download/{job_id}/" + +STATUS_READY = "ready" +STATUS_ERROR = "error" +STATUS_RUNNING = "running" + + +@transaction() +def _sweep_and_upsert( + job_id: UUID, value: dict[str, Any], expires_at: datetime +) -> None: + # Lazily sweep expired entries each time one is written; there is no + # dedicated cleanup job, so this resource keeps itself tidy on write. + # upsert (not create) so a retried/duplicate write for the same job_id + # overwrites cleanly instead of colliding on the primary key. + # @transaction commits now; the worker session otherwise only commits when + # the task settles, leaving statuses invisible to polling web pods. + KeyValueDAO.delete_expired_entries(RESOURCE) + KeyValueDAO.upsert_entry( + resource=RESOURCE, + value=value, + codec=CODEC, + key=job_id, + expires_on=expires_at, + ) + + +def download_path(job_id: UUID) -> str: + """Root-relative path of the download endpoint for ``job_id``, including + ``APPLICATION_ROOT`` when Superset is served under a subpath. Handed to the + polling frontend, which resolves it against its own origin (the one host + the user is provably reachable at).""" + path = DOWNLOAD_PATH.format(job_id=job_id) + app_root = current_app.config.get("APPLICATION_ROOT") or "/" + if app_root != "/" and not path.startswith(app_root): Review Comment: Good catch, fixed in 5e26086e71c452def59c173bf933ffdfc4bf6e9a. `DOWNLOAD_PATH` is the literal `/api/v1/dashboard/export_xlsx/download/<job_id>/`, so the `startswith` guard matched whenever `APPLICATION_ROOT` was a string prefix of it: `/api`, `/api/`, `/a`, `/api/v1`. Both the polled `download_url` and the emailed link then dropped the root and 404'd. The guard was never meaningful in the first place. `DOWNLOAD_PATH` is a constant that never carries the prefix, so there is nothing to make idempotent. The root is now prepended unconditionally, and an empty root falls through. Added `tests/unit_tests/dashboards/test_excel_export_download_link.py` covering `/`, `/superset`, the trailing slash form, and the two roots that regressed. I also diffed old against new across a spread of roots: the output is byte identical for every root that already worked, so the only behavior that changes is the cases that were 404ing. ########## superset/dashboards/api.py: ########## @@ -354,6 +391,9 @@ class DashboardRestApi( # menu item on it) instead of the ``can_export_xlsx`` FAB would otherwise # derive from the method name. "export_xlsx": "export", + # Polling status of an export you already requested is the same + # capability as requesting it, not a distinct permission. + "export_xlsx_status": "export", Review Comment: Agreed on the risk. I went at it from the other end in 6fd1781c63b7e9baac30ae2c6909d634fd11b65f and 9daab20c7f5ac52475ade8f4f347f8e37b9635f2, happy to switch if you would rather have the dedicated permission. The grant is the problem, as you say. `PUBLIC_ROLE_PERMISSIONS` carries neither `can_export` nor a data export permission, so enabling this for an embedded guest means an admin adds `can_export` on Dashboard by hand, and FAB derives that same permission for `/api/v1/dashboard/export/`. `DashboardAccessFilter` confines a guest to the dashboards in their token, so the reach is their own embedded dashboard rather than the instance, but the bundle still carries dataset SQL and database metadata the embedded view never shows. Chasing this down, `export_as_example` turned out to be a second door behind the same permission: it carries `@permission_name("export")` and emits dataset YAML alongside Parquet rows. Its row data is already constrained, since `dataset.raise_for_access()` runs per dataset and rows are fetched through `dataset.query()` so per-row filters apply, but the definitions are the same metadata, and the projection takes every non-expression column rather than the ones the dashboard renders. Both are now refused for guest principals. That closes the set: `export`, `export_as_example`, `export_xlsx` and `export_xlsx_status` are the only routes behind that permission, so the grant this feature needs can no longer be turned into a metadata read. Only the dashboard and chart list pages call those two endpoints, so no guest or embedded flow loses anything. On a dedicated permission proper, I did look at reusing `can_export_data` and `can_export_image`, since the two xlsx modes line up with them exactly. That does not work through `method_permission_name`: `@protect()` scopes to `class_permission_name` (`Dashboard`) while those live on the `Superset` view menu, so mapping the routes would mint a new `can_export_data` on Dashboard rather than reuse the existing one. Any dedicated permission here is therefore a new permission, and to avoid silently breaking custom roles that already hold `can_export` it wants a `security_converge` migration in the shape of `a1b2c3d4e5f6`, plus a bootstrap field since the menu gates on `dash_export_perm`. Happy to do that as a follow-up if you want the split as well. The one thing these guards do not buy is structural safety: a future route added under `can_export` would be exposed to guests again. -- 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]
