gabotorresruiz commented on code in PR #43805:
URL: https://github.com/apache/superset/pull/43805#discussion_r3990468842
##########
superset/tasks/export_dashboard_excel.py:
##########
@@ -98,6 +118,32 @@ def export_lock_params(user_id: int, dashboard_id: int) ->
dict[str, int]:
return {"user_id": user_id, "dashboard_id": dashboard_id}
+def guest_lock_slot(guest_token: GuestToken | None) -> int:
+ """A stable per-guest lock slot derived from the token's identity
(username,
+ resources, and RLS rules), so concurrent guests on the same dashboard
+ throttle independently instead of all sharing slot 0 (where the second
+ guest's export is refused with no job id and no email fallback). RLS is
part
+ of the fingerprint because it is what distinguishes embedded guests
sharing a
+ dashboard when the username is shared or absent. Anonymous requesters (no
+ token) share 0.
+ """
+ if not guest_token:
+ return 0
+ user = guest_token.get("user") or {}
+ resources = guest_token.get("resources") or []
+ fingerprint = json.dumps(
+ {
+ "username": user.get("username"),
+ "resources": resources,
+ "rls": guest_token.get("rls_rules") or [],
+ },
+ sort_keys=True,
+ default=str,
Review Comment:
Good catch, fixed in 55fc9783ee. `guest_lock_slot` now folds the token's
`datasets` allowlist into the fingerprint alongside `resources` and
`rls_rules`, so two guests differing only in dataset access get distinct lock
slots. Covered by an added case in the slot test.
##########
superset/tasks/export_dashboard_excel.py:
##########
@@ -389,30 +442,116 @@ 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 send export failure email")
+ 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 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:
+ # The token's signature/exp were verified at request time, but the
+ # export can sit queued: refuse to run chart queries under a guest
+ # token that has since expired. (Such a guest can no longer reach the
+ # @protect-ed status endpoint to retrieve the result anyway, and has no
+ # email fallback, so nothing servable is lost.)
+ exp = guest_token.get("exp")
+ if exp is not None and exp < time.time():
+ raise SupersetException("The guest token has expired.")
+ return security_manager.get_guest_user_from_token(guest_token)
Review Comment:
The expiry recheck added in ae78c7fac6 is the actionable part here. Full
revocation isn't checkable for these tokens: guest tokens are stateless JWTs
with no server-side revocation list, so beyond `exp` the only invalidation is
rotating `GUEST_TOKEN_JWT_SECRET` (which invalidates every token at once). The
raw JWT also isn't queued, only the decoded payload, so the worker can't re-run
signature verification without changing that contract. This matches
`superset.tasks.async_queries`, which reconstructs from the queued payload the
same way.
--
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]
