codeant-ai-for-open-source[bot] commented on code in PR #43805:
URL: https://github.com/apache/superset/pull/43805#discussion_r3990317281
##########
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:
**Suggestion:** `guest_lock_slot` omits the token's `datasets` claim, so
guests with different dataset permissions can collide and one export is
incorrectly refused. [incorrect condition logic]
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7912e1aa81a940509e0b77ec1713d57a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7912e1aa81a940509e0b77ec1713d57a&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/tasks/export_dashboard_excel.py
**Line:** 134:141
**Comment:**
*Incorrect Condition Logic: `guest_lock_slot` omits the token's
`datasets` claim, so guests with different dataset permissions can collide and
one export is incorrectly refused.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43805&comment_hash=eae0b2979ea3351884872d8271331a5e5c28bd07cd001dd7bbabca8a0f1cecce&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43805&comment_hash=eae0b2979ea3351884872d8271331a5e5c28bd07cd001dd7bbabca8a0f1cecce&reaction=dislike'>๐</a>
##########
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:
**Suggestion:** Revoked guest tokens are checked only for expiration, so an
export queued before revocation still runs queries using claims that normal
request authentication would reject. [security]
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a0ef9143e24b485f9dc4205f1d8cfb63&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a0ef9143e24b485f9dc4205f1d8cfb63&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/tasks/export_dashboard_excel.py
**Line:** 551:551
**Comment:**
*Security: Revoked guest tokens are checked only for expiration, so an
export queued before revocation still runs queries using claims that normal
request authentication would reject.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43805&comment_hash=4c7fec7e977b4beadf9414c81c01c23454b900beda1bd6943a22c27c5deb673d&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43805&comment_hash=4c7fec7e977b4beadf9414c81c01c23454b900beda1bd6943a22c27c5deb673d&reaction=dislike'>๐</a>
--
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]
