sadpandajoe commented on code in PR #43336:
URL: https://github.com/apache/superset/pull/43336#discussion_r3817419744
##########
superset/dashboards/api.py:
##########
@@ -1787,12 +1803,9 @@ def export_xlsx(self, pk: int) -> WerkzeugResponse:
except SupersetSecurityException:
return self.response_403()
Review Comment:
Embedded guest requests now reach this path, but `GuestUser` has no
persisted numeric `id`; building the lock key (and later enqueueing the task)
raises before a job can be returned. Could this keep guest context separate
from the user-id task contract, or continue rejecting guests until the worker
can safely reconstruct that context?
##########
superset/dashboards/excel_export/download_link.py:
##########
@@ -0,0 +1,139 @@
+# 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.
+
+A raw S3 pre-signed URL is only valid for as long as *both* its own
+``ExpiresIn`` window and the credentials that signed it remain valid.
+Deployments whose S3 client authenticates via short-lived, auto-refreshed
+credentials (e.g. an EKS IRSA role assumed through
+``AssumeRoleWithWebIdentity``, which AWS caps at 12 hours and many clusters
+default to far less) can silently invalidate a pre-signed URL long before the
+``EXCEL_EXPORT_LINK_TTL_SECONDS`` window promised in the export email elapses,
+since the *credentials'* session -- not just the URL's own ``ExpiresIn`` --
+bounds how long it actually works.
+
+To keep that promise regardless of credential lifetime, the email links to a
+small Superset redirect endpoint instead of a raw S3 URL. The link's own
+lifetime is enforced by this module via the ``key_value`` store's
+``expires_on`` (independent of any credential session), and the actual
+pre-signed URL is generated fresh -- with then-current credentials -- at click
+time, valid only long enough to complete a single download.
+
+The redirect endpoint (``download_xlsx``) intentionally requires no login: a
+pre-signed S3 URL never did either, and 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 emailed only to
+that requester's own address is the same "possession of the link is the
+credential" model the raw pre-signed URL had; this module just re-signs it
+closer to when it is actually used.
+
+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 superset.daos.key_value import KeyValueDAO
+from superset.key_value.types import JsonKeyValueCodec, KeyValueResource
+from superset.utils.urls import headless_url
+
+RESOURCE = KeyValueResource.EXCEL_EXPORT_DOWNLOAD
+CODEC = JsonKeyValueCodec()
+
+# The fresh pre-signed URL generated at click time only needs to outlive the
+# redirect and the browser/S3 handshake that follows it, not the link's own
+# multi-hour lifetime.
+PRESIGNED_URL_TTL_SECONDS = 300
+
+DOWNLOAD_PATH = "/api/v1/dashboard/export_xlsx/download/{job_id}/"
+
+STATUS_READY = "ready"
+STATUS_ERROR = "error"
+
+
+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.
+ KeyValueDAO.delete_expired_entries(RESOURCE)
+ KeyValueDAO.upsert_entry(
Review Comment:
The ready/error status is only staged in the SQLAlchemy session here. With a
Redis distributed lock, releasing the lock does not commit that session, so
Celery teardown rolls the entry back: polling remains pending and the emailed
redirect returns 410. Could this write be committed transactionally before
reporting the export ready?
##########
superset/dashboards/excel_export/download_link.py:
##########
@@ -0,0 +1,139 @@
+# 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.
+
+A raw S3 pre-signed URL is only valid for as long as *both* its own
+``ExpiresIn`` window and the credentials that signed it remain valid.
+Deployments whose S3 client authenticates via short-lived, auto-refreshed
+credentials (e.g. an EKS IRSA role assumed through
+``AssumeRoleWithWebIdentity``, which AWS caps at 12 hours and many clusters
+default to far less) can silently invalidate a pre-signed URL long before the
+``EXCEL_EXPORT_LINK_TTL_SECONDS`` window promised in the export email elapses,
+since the *credentials'* session -- not just the URL's own ``ExpiresIn`` --
+bounds how long it actually works.
+
+To keep that promise regardless of credential lifetime, the email links to a
+small Superset redirect endpoint instead of a raw S3 URL. The link's own
+lifetime is enforced by this module via the ``key_value`` store's
+``expires_on`` (independent of any credential session), and the actual
+pre-signed URL is generated fresh -- with then-current credentials -- at click
+time, valid only long enough to complete a single download.
+
+The redirect endpoint (``download_xlsx``) intentionally requires no login: a
+pre-signed S3 URL never did either, and 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 emailed only to
+that requester's own address is the same "possession of the link is the
+credential" model the raw pre-signed URL had; this module just re-signs it
+closer to when it is actually used.
+
+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 superset.daos.key_value import KeyValueDAO
+from superset.key_value.types import JsonKeyValueCodec, KeyValueResource
+from superset.utils.urls import headless_url
+
+RESOURCE = KeyValueResource.EXCEL_EXPORT_DOWNLOAD
+CODEC = JsonKeyValueCodec()
+
+# The fresh pre-signed URL generated at click time only needs to outlive the
+# redirect and the browser/S3 handshake that follows it, not the link's own
+# multi-hour lifetime.
+PRESIGNED_URL_TTL_SECONDS = 300
+
+DOWNLOAD_PATH = "/api/v1/dashboard/export_xlsx/download/{job_id}/"
+
+STATUS_READY = "ready"
+STATUS_ERROR = "error"
+
+
+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.
+ KeyValueDAO.delete_expired_entries(RESOURCE)
+ KeyValueDAO.upsert_entry(
+ resource=RESOURCE,
+ value=value,
+ codec=CODEC,
+ key=job_id,
+ expires_on=expires_at,
+ )
+
+
+def build_download_url(job_id: UUID) -> str:
+ """The browser-facing URL that redirects to a freshly pre-signed S3 URL
+ for ``job_id``, once its export is ready."""
+ return headless_url(DOWNLOAD_PATH.format(job_id=job_id),
user_friendly=True)
Review Comment:
`headless_url` joins this absolute path to the host and bypasses
`APPLICATION_ROOT`. Deployments mounted below a prefix therefore email a
root-level download URL that does not route to Superset. Could this use the
prefix-aware URL helper (or otherwise include the application root)?
##########
superset/tasks/export_dashboard_excel.py:
##########
@@ -471,11 +498,14 @@ def export_dashboard_excel(
f"{current_app.config['EXCEL_EXPORT_S3_KEY_PREFIX']}"
f"{dashboard_id}/{job_id}.xlsx"
)
- ttl = current_app.config["EXCEL_EXPORT_LINK_TTL_SECONDS"]
s3.upload_file_to_s3(tmp_path, bucket, key)
- download_url = s3.generate_presigned_url(bucket, key, ttl)
expires_at = datetime.now(tz=timezone.utc) + timedelta(seconds=ttl)
+ # KeyValueEntry.expires_on comparisons use naive datetime.now(), so
+ # the stored expiry must be naive UTC too, not tz-aware.
+ download_url = create_download_link(
+ uuid.UUID(job_id), bucket, key, expires_at.replace(tzinfo=None)
Review Comment:
This stores a naive UTC timestamp, while `KeyValueEntry.is_expired()`
compares it with naive local `datetime.now()`. On non-UTC servers the
configured link lifetime is shifted by the timezone offset, so links can expire
early or stay valid too long. Could the stored value and expiry comparison use
the same timezone convention?
--
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]
