You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/06/02 05:23:02 UTC
[PR] feat(security): support guest-token revocation per embedded dashboard [superset]
rusackas opened a new pull request, #40676:
URL: https://github.com/apache/superset/pull/40676
> **Draft / `hold:testing`** — adds a DB migration and a per-request check on the guest-token path. Needs validation under embedded load before merge.
### SUMMARY
Guest tokens (embedded dashboards) are self-contained JWTs validated only for signature, `exp`, and `aud` — there is **no revocation**. When an admin revokes a guest's access, existing tokens remain valid until expiry (ASVS 7.4.1, CWE-613).
This implements the SIP's **Part A3**:
- Adds `guest_token_revoked_before` (epoch seconds) to `embedded_dashboards` (migration `c8d2e3f4a5b6`).
- `get_guest_user_from_request` now rejects any guest token whose `iat` predates the revocation cutoff of one of its embedded-dashboard resources.
- `SecurityManager.revoke_guest_token_access(embedded_uuid)` sets the cutoff to now — revoking all currently-issued tokens for that dashboard while leaving later-issued ones valid.
Guest tokens already carry `iat`, so no token-format change is required. Default behavior is unchanged (`NULL` cutoff ⇒ no revocation).
### WHY DRAFT (`hold:testing`)
Wiring `revoke_guest_token_access` into an admin UI / REST action is a follow-up (this PR provides the mechanism). The revocation check adds a DAO lookup per guest request — needs validation under embedded load.
### TESTING INSTRUCTIONS
```
pytest tests/unit_tests/security/test_guest_token_revocation.py
```
Tests: token issued before the cutoff is rejected; token issued after is still valid; no cutoff ⇒ valid; missing `iat` ⇒ not revoked.
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags: EMBEDDED_SUPERSET
- [ ] Changes UI
- [x] Includes DB Migration (adds `embedded_dashboards.guest_token_revoked_before`; reversible)
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(security): support guest-token revocation per embedded dashboard [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40676:
URL: https://github.com/apache/superset/pull/40676#issuecomment-4598954314
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: support guest-token revocation per embedded dashboard [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40676:
URL: https://github.com/apache/superset/pull/40676#issuecomment-4665468921
<!-- Bito Reply -->
The flagged issue is correct. The current implementation of `_is_guest_token_revoked` only attempts to resolve resources using `EmbeddedDashboardDAO.find_by_id` with the resource ID, which assumes the ID is already a UUID. If the resource ID is a legacy integer dashboard ID, the lookup fails, and the revocation check is skipped.
To resolve this, I have updated `_is_guest_token_revoked` to implement a fallback mechanism: it first attempts to resolve the resource as a dashboard (using `Dashboard.get`), and if that fails, it proceeds to look up the resource as an embedded dashboard (using `EmbeddedDashboardDAO.find_by_id`).
**superset/security/manager.py**
```
for resource in token.get("resources") or []:
if resource.get("type") != GuestTokenResourceType.DASHBOARD.value:
continue
# Fallback: try to resolve as dashboard ID, then embedded UUID
resource_id = str(resource.get("id"))
embedded = Dashboard.get(resource_id) or EmbeddedDashboardDAO.find_by_id(resource_id)
revoked_before = getattr(embedded, "guest_token_revoked_before", None)
if revoked_before is not None and issued_at < revoked_before:
return True
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: support guest-token revocation per embedded dashboard [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40676:
URL: https://github.com/apache/superset/pull/40676#discussion_r3384762610
##########
superset/security/manager.py:
##########
@@ -3681,6 +3684,43 @@ def get_guest_user_from_request(self, req: Request) -> Optional[GuestUser]:
return self.get_guest_user_from_token(cast(GuestToken, token))
+ @staticmethod
+ def _is_guest_token_revoked(token: dict[str, Any]) -> bool:
+ """Return True if the token predates a revocation on any of its
+ embedded-dashboard resources (``guest_token_revoked_before``)."""
+ issued_at = token.get("iat")
+ if not issued_at:
+ return False
+
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.dashboard import EmbeddedDashboardDAO
+
+ for resource in token.get("resources") or []:
+ if resource.get("type") != GuestTokenResourceType.DASHBOARD.value:
+ continue
+ embedded = EmbeddedDashboardDAO.find_by_id(str(resource.get("id")))
+ revoked_before = getattr(embedded, "guest_token_revoked_before", None)
+ if revoked_before is not None and issued_at < revoked_before:
Review Comment:
**🟠 Architect Review — HIGH**
Guest-token revocation only looks up embedded dashboards by UUID via EmbeddedDashboardDAO.find_by_id(resource["id"]), so tokens whose dashboard resources still carry a dashboard ID (not an embedded UUID) never see a guest_token_revoked_before cutoff and remain valid until exp.
**Suggestion:** Align _is_guest_token_revoked with the existing resource-resolution pattern (Dashboard.get(...) fallback to EmbeddedDashboardDAO.find_by_id(...)), so revocation consults guest_token_revoked_before for both dashboard-ID and embedded-UUID resources during the UUID migration period.
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5c986fd005714ff99883ded45c9f9b37&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=5c986fd005714ff99883ded45c9f9b37&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** superset/security/manager.py
**Line:** 3698:3703
**Comment:**
*HIGH: Guest-token revocation only looks up embedded dashboards by UUID via EmbeddedDashboardDAO.find_by_id(resource["id"]), so tokens whose dashboard resources still carry a dashboard ID (not an embedded UUID) never see a guest_token_revoked_before cutoff and remain valid until exp.
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.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
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>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: support guest-token revocation per embedded dashboard [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #40676:
URL: https://github.com/apache/superset/pull/40676
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: support guest-token revocation per embedded dashboard [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40676:
URL: https://github.com/apache/superset/pull/40676#discussion_r3385389668
##########
superset/security/manager.py:
##########
@@ -3681,6 +3685,67 @@ def get_guest_user_from_request(self, req: Request) -> Optional[GuestUser]:
return self.get_guest_user_from_token(cast(GuestToken, token))
+ @staticmethod
+ def _is_guest_token_revoked(token: dict[str, Any]) -> bool:
+ """Return True if the token predates a revocation on any of its
+ embedded-dashboard resources (``guest_token_revoked_before``).
+
+ A token missing ``iat`` cannot prove it was issued after a revocation
+ cutoff, so it is treated as revoked whenever any of its dashboard
+ resources has an active cutoff; otherwise it is not revoked.
+ """
+ issued_at = token.get("iat")
+
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.dashboard import EmbeddedDashboardDAO
+ from superset.models.dashboard import Dashboard
+
+ for resource in token.get("resources") or []:
+ if resource.get("type") != GuestTokenResourceType.DASHBOARD.value:
+ continue
+ resource_id = str(resource.get("id"))
+ # A dashboard resource id may be an embedded UUID or, during the
+ # UUID migration, a legacy dashboard id. Resolve the embedded
+ # config(s) for either form (mirrors validate_guest_token_resources).
+ embedded = EmbeddedDashboardDAO.find_by_id(resource_id)
+ if embedded:
+ embedded_configs = [embedded]
+ else:
+ dashboard = Dashboard.get(resource_id)
+ embedded_configs = dashboard.embedded if dashboard else []
+ for embedded_config in embedded_configs:
+ revoked_before = getattr(
+ embedded_config, "guest_token_revoked_before", None
+ )
+ if revoked_before is None:
+ continue
+ # Without an issued-at claim the token cannot be shown to
+ # postdate the cutoff, so fail closed and treat it as revoked.
+ if not issued_at or issued_at < revoked_before:
+ return True
+ return False
+
+ @transaction()
+ def revoke_guest_token_access(
+ self, embedded_uuid: str, before: Optional[int] = None
+ ) -> None:
+ """Revoke all guest tokens issued for an embedded dashboard before
+ ``before`` (epoch seconds, default: now). Subsequent tokens are
+ unaffected."""
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.dashboard import EmbeddedDashboardDAO
+
+ embedded = EmbeddedDashboardDAO.find_by_id(str(embedded_uuid))
+ if embedded is None:
+ return
+ # Round the cutoff up to the next whole second so that tokens whose
+ # fractional ``iat`` falls within the current second are reliably
+ # revoked (the column stores integer seconds). Rounding up fails
+ # closed: at worst it revokes a token issued slightly after the call.
+ embedded.guest_token_revoked_before = (
+ before if before is not None else ceil(self._get_current_epoch_time())
+ )
Review Comment:
Good call — added unit tests for `revoke_guest_token_access` in 506c70eb42 covering the explicit `before` argument, the default ceil-of-now cutoff (verifying fractional times round up so same-second tokens fail closed), and the no-op path when the embedded UUID does not resolve.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: support guest-token revocation per embedded dashboard [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40676:
URL: https://github.com/apache/superset/pull/40676#discussion_r3384770101
##########
superset/security/manager.py:
##########
@@ -3681,6 +3684,43 @@ def get_guest_user_from_request(self, req: Request) -> Optional[GuestUser]:
return self.get_guest_user_from_token(cast(GuestToken, token))
+ @staticmethod
+ def _is_guest_token_revoked(token: dict[str, Any]) -> bool:
+ """Return True if the token predates a revocation on any of its
+ embedded-dashboard resources (``guest_token_revoked_before``)."""
+ issued_at = token.get("iat")
+ if not issued_at:
+ return False
+
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.dashboard import EmbeddedDashboardDAO
+
+ for resource in token.get("resources") or []:
+ if resource.get("type") != GuestTokenResourceType.DASHBOARD.value:
+ continue
+ embedded = EmbeddedDashboardDAO.find_by_id(str(resource.get("id")))
+ revoked_before = getattr(embedded, "guest_token_revoked_before", None)
+ if revoked_before is not None and issued_at < revoked_before:
Review Comment:
Good catch. A guest-token dashboard resource id can be either an embedded UUID or, during the UUID migration, a legacy dashboard id (per `validate_guest_token_resources`, which falls back to `Dashboard.get(...)`). The revocation check only resolved the UUID form, so legacy-id resources skipped the cutoff. Fixed in 153c4ece3d: `_is_guest_token_revoked` now resolves the embedded config(s) for both forms (UUID via `EmbeddedDashboardDAO.find_by_id`, else `Dashboard.get(id).embedded`) and honors `guest_token_revoked_before` in both cases. Added unit tests for the legacy-id path and the unresolvable-resource case.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: support guest-token revocation per embedded dashboard [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40676:
URL: https://github.com/apache/superset/pull/40676#discussion_r3385390272
##########
superset/security/manager.py:
##########
@@ -3681,6 +3685,67 @@ def get_guest_user_from_request(self, req: Request) -> Optional[GuestUser]:
return self.get_guest_user_from_token(cast(GuestToken, token))
+ @staticmethod
+ def _is_guest_token_revoked(token: dict[str, Any]) -> bool:
+ """Return True if the token predates a revocation on any of its
+ embedded-dashboard resources (``guest_token_revoked_before``).
+
+ A token missing ``iat`` cannot prove it was issued after a revocation
+ cutoff, so it is treated as revoked whenever any of its dashboard
+ resources has an active cutoff; otherwise it is not revoked.
+ """
+ issued_at = token.get("iat")
+
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.dashboard import EmbeddedDashboardDAO
+ from superset.models.dashboard import Dashboard
+
+ for resource in token.get("resources") or []:
+ if resource.get("type") != GuestTokenResourceType.DASHBOARD.value:
+ continue
+ resource_id = str(resource.get("id"))
+ # A dashboard resource id may be an embedded UUID or, during the
+ # UUID migration, a legacy dashboard id. Resolve the embedded
+ # config(s) for either form (mirrors validate_guest_token_resources).
+ embedded = EmbeddedDashboardDAO.find_by_id(resource_id)
+ if embedded:
+ embedded_configs = [embedded]
+ else:
+ dashboard = Dashboard.get(resource_id)
+ embedded_configs = dashboard.embedded if dashboard else []
+ for embedded_config in embedded_configs:
+ revoked_before = getattr(
+ embedded_config, "guest_token_revoked_before", None
+ )
+ if revoked_before is None:
+ continue
+ # Without an issued-at claim the token cannot be shown to
+ # postdate the cutoff, so fail closed and treat it as revoked.
+ if not issued_at or issued_at < revoked_before:
+ return True
+ return False
+
+ @transaction()
+ def revoke_guest_token_access(
+ self, embedded_uuid: str, before: Optional[int] = None
+ ) -> None:
+ """Revoke all guest tokens issued for an embedded dashboard before
+ ``before`` (epoch seconds, default: now). Subsequent tokens are
+ unaffected."""
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.dashboard import EmbeddedDashboardDAO
+
+ embedded = EmbeddedDashboardDAO.find_by_id(str(embedded_uuid))
+ if embedded is None:
+ return
+ # Round the cutoff up to the next whole second so that tokens whose
+ # fractional ``iat`` falls within the current second are reliably
+ # revoked (the column stores integer seconds). Rounding up fails
+ # closed: at worst it revokes a token issued slightly after the call.
+ embedded.guest_token_revoked_before = (
+ before if before is not None else ceil(self._get_current_epoch_time())
+ )
Review Comment:
<!-- Bito Reply -->
The addition of unit tests for `revoke_guest_token_access` addresses the previous review suggestion regarding missing test coverage. The implementation covers the specified edge cases, including the `before` argument, default behavior for fractional timestamps, and the no-op path for unresolved UUIDs, which aligns with the requirement for comprehensive testing of new features.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: support guest-token revocation per embedded dashboard [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40676:
URL: https://github.com/apache/superset/pull/40676#issuecomment-4666206346
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: support guest-token revocation per embedded dashboard [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40676:
URL: https://github.com/apache/superset/pull/40676#discussion_r3385353482
##########
superset/security/manager.py:
##########
@@ -3681,6 +3685,67 @@ def get_guest_user_from_request(self, req: Request) -> Optional[GuestUser]:
return self.get_guest_user_from_token(cast(GuestToken, token))
+ @staticmethod
+ def _is_guest_token_revoked(token: dict[str, Any]) -> bool:
+ """Return True if the token predates a revocation on any of its
+ embedded-dashboard resources (``guest_token_revoked_before``).
+
+ A token missing ``iat`` cannot prove it was issued after a revocation
+ cutoff, so it is treated as revoked whenever any of its dashboard
+ resources has an active cutoff; otherwise it is not revoked.
+ """
+ issued_at = token.get("iat")
+
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.dashboard import EmbeddedDashboardDAO
+ from superset.models.dashboard import Dashboard
+
+ for resource in token.get("resources") or []:
+ if resource.get("type") != GuestTokenResourceType.DASHBOARD.value:
+ continue
+ resource_id = str(resource.get("id"))
+ # A dashboard resource id may be an embedded UUID or, during the
+ # UUID migration, a legacy dashboard id. Resolve the embedded
+ # config(s) for either form (mirrors validate_guest_token_resources).
+ embedded = EmbeddedDashboardDAO.find_by_id(resource_id)
+ if embedded:
+ embedded_configs = [embedded]
+ else:
+ dashboard = Dashboard.get(resource_id)
+ embedded_configs = dashboard.embedded if dashboard else []
+ for embedded_config in embedded_configs:
+ revoked_before = getattr(
+ embedded_config, "guest_token_revoked_before", None
+ )
+ if revoked_before is None:
+ continue
+ # Without an issued-at claim the token cannot be shown to
+ # postdate the cutoff, so fail closed and treat it as revoked.
+ if not issued_at or issued_at < revoked_before:
+ return True
+ return False
+
+ @transaction()
+ def revoke_guest_token_access(
+ self, embedded_uuid: str, before: Optional[int] = None
+ ) -> None:
+ """Revoke all guest tokens issued for an embedded dashboard before
+ ``before`` (epoch seconds, default: now). Subsequent tokens are
+ unaffected."""
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.dashboard import EmbeddedDashboardDAO
+
+ embedded = EmbeddedDashboardDAO.find_by_id(str(embedded_uuid))
+ if embedded is None:
+ return
+ # Round the cutoff up to the next whole second so that tokens whose
+ # fractional ``iat`` falls within the current second are reliably
+ # revoked (the column stores integer seconds). Rounding up fails
+ # closed: at worst it revokes a token issued slightly after the call.
+ embedded.guest_token_revoked_before = (
+ before if before is not None else ceil(self._get_current_epoch_time())
+ )
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests</b></div>
<div id="fix">
The `revoke_guest_token_access` method lacks unit test coverage. Per BITO.md rule [11730], new tools/features should have comprehensive unit tests covering success paths, error scenarios, and edge cases. Existing tests in `test_guest_token_revocation.py` only cover `_is_guest_token_revoked`.
</div>
</div>
<small><i>Code Review Run #e835c6</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: support guest-token revocation per embedded dashboard [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40676:
URL: https://github.com/apache/superset/pull/40676#issuecomment-4667494923
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: support guest-token revocation per embedded dashboard [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #40676:
URL: https://github.com/apache/superset/pull/40676#discussion_r3384913376
##########
superset/security/manager.py:
##########
@@ -3681,6 +3684,43 @@ def get_guest_user_from_request(self, req: Request) -> Optional[GuestUser]:
return self.get_guest_user_from_token(cast(GuestToken, token))
+ @staticmethod
+ def _is_guest_token_revoked(token: dict[str, Any]) -> bool:
+ """Return True if the token predates a revocation on any of its
+ embedded-dashboard resources (``guest_token_revoked_before``)."""
+ issued_at = token.get("iat")
+ if not issued_at:
+ return False
Review Comment:
Good point — fixed in 6f07238acf. `_is_guest_token_revoked` now fails closed: when a dashboard resource has an active `guest_token_revoked_before` cutoff and the token lacks an `iat` claim, the token is treated as revoked (it cannot prove it was issued after the cutoff). Tokens without `iat` are only allowed through when no cutoff is set. Added unit tests for both the cutoff and no-cutoff cases.
##########
superset/security/manager.py:
##########
@@ -3681,6 +3684,43 @@ def get_guest_user_from_request(self, req: Request) -> Optional[GuestUser]:
return self.get_guest_user_from_token(cast(GuestToken, token))
+ @staticmethod
+ def _is_guest_token_revoked(token: dict[str, Any]) -> bool:
+ """Return True if the token predates a revocation on any of its
+ embedded-dashboard resources (``guest_token_revoked_before``)."""
+ issued_at = token.get("iat")
+ if not issued_at:
+ return False
+
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.dashboard import EmbeddedDashboardDAO
+
+ for resource in token.get("resources") or []:
+ if resource.get("type") != GuestTokenResourceType.DASHBOARD.value:
+ continue
+ embedded = EmbeddedDashboardDAO.find_by_id(str(resource.get("id")))
+ revoked_before = getattr(embedded, "guest_token_revoked_before", None)
+ if revoked_before is not None and issued_at < revoked_before:
+ return True
+ return False
+
+ @transaction()
+ def revoke_guest_token_access(
+ self, embedded_uuid: str, before: Optional[int] = None
+ ) -> None:
+ """Revoke all guest tokens issued for an embedded dashboard before
+ ``before`` (epoch seconds, default: now). Subsequent tokens are
+ unaffected."""
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.dashboard import EmbeddedDashboardDAO
+
+ embedded = EmbeddedDashboardDAO.find_by_id(str(embedded_uuid))
+ if embedded is None:
+ return
+ embedded.guest_token_revoked_before = (
+ before if before is not None else int(self._get_current_epoch_time())
Review Comment:
Agreed — fixed in 6f07238acf. `revoke_guest_token_access` now rounds the cutoff up to the next whole second with `ceil(...)` (the column stores integer seconds), so any token whose fractional `iat` falls within the revocation second is reliably `< revoked_before` and gets rejected. Rounding up fails closed.
##########
superset/security/manager.py:
##########
@@ -3681,6 +3684,43 @@ def get_guest_user_from_request(self, req: Request) -> Optional[GuestUser]:
return self.get_guest_user_from_token(cast(GuestToken, token))
+ @staticmethod
+ def _is_guest_token_revoked(token: dict[str, Any]) -> bool:
+ """Return True if the token predates a revocation on any of its
+ embedded-dashboard resources (``guest_token_revoked_before``)."""
+ issued_at = token.get("iat")
+ if not issued_at:
+ return False
+
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.dashboard import EmbeddedDashboardDAO
+
+ for resource in token.get("resources") or []:
+ if resource.get("type") != GuestTokenResourceType.DASHBOARD.value:
+ continue
+ embedded = EmbeddedDashboardDAO.find_by_id(str(resource.get("id")))
+ revoked_before = getattr(embedded, "guest_token_revoked_before", None)
+ if revoked_before is not None and issued_at < revoked_before:
+ return True
Review Comment:
This is already handled. `_is_guest_token_revoked` resolves both resource id forms: it tries `EmbeddedDashboardDAO.find_by_id(resource_id)` (embedded UUID) and falls back to `Dashboard.get(resource_id).embedded` for legacy dashboard-id resources, then honors `guest_token_revoked_before` on whichever config(s) resolve. So a revoked dashboard cannot be accessed via a legacy dashboard-id token. Covered by `test_guest_token_revoked_via_legacy_dashboard_id_resource`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: support guest-token revocation per embedded dashboard [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40676:
URL: https://github.com/apache/superset/pull/40676#issuecomment-4676243342
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(security): support guest-token revocation per embedded dashboard [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #40676:
URL: https://github.com/apache/superset/pull/40676#issuecomment-4598966922
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 985bfdc752f9d71d3df4a240e87944b5403e5805 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a1e68b63373e60008e517aa |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-40676--superset-docs-preview.netlify.app](https://deploy-preview-40676--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpLDkAwEADQu8yaqDSGuIFjjHZ8gk5TQyLi7rp7i_dCTHJEhR6ggDFRcEv2xKTVfPGppcrGoUx8iyNdJeTmOe7yDD5HpJqxG9Ha1jIaYzpu6pYIvh8) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-40676--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(security): support guest-token revocation per embedded dashboard [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #40676:
URL: https://github.com/apache/superset/pull/40676#issuecomment-4598979982
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/40676?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `13.33333%` with `26 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.11%. Comparing base ([`a183582`](https://app.codecov.io/gh/apache/superset/commit/a183582291ea020bae95bc10969c73c0470459d3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`985bfdc`](https://app.codecov.io/gh/apache/superset/commit/985bfdc752f9d71d3df4a240e87944b5403e5805?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 184 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/40676?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/security/manager.py](https://app.codecov.io/gh/apache/superset/pull/40676?src=pr&el=tree&filepath=superset%2Fsecurity%2Fmanager.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvc2VjdXJpdHkvbWFuYWdlci5weQ==) | 10.34% | [26 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/40676?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (a183582) and HEAD (985bfdc). Click for more details.
>
> <details><summary>HEAD has 150 uploads less than BASE</summary>
>
>| Flag | BASE (a183582) | HEAD (985bfdc) |
>|------|------|------|
>|python|138|1|
>|presto|14|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #40676 +/- ##
==========================================
- Coverage 64.18% 55.11% -9.08%
==========================================
Files 2591 2651 +60
Lines 138471 142188 +3717
Branches 32120 32575 +455
==========================================
- Hits 88883 78372 -10511
- Misses 48056 63127 +15071
+ Partials 1532 689 -843
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/40676/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/40676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/40676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/40676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/40676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.35% <13.33%> (+0.26%)` | :arrow_up: |
| [python](https://app.codecov.io/gh/apache/superset/pull/40676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.35% <13.33%> (-19.22%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/40676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/40676/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/40676?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat: support guest-token revocation per embedded dashboard [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40676:
URL: https://github.com/apache/superset/pull/40676#discussion_r3384772256
##########
superset/security/manager.py:
##########
@@ -3681,6 +3684,43 @@ def get_guest_user_from_request(self, req: Request) -> Optional[GuestUser]:
return self.get_guest_user_from_token(cast(GuestToken, token))
+ @staticmethod
+ def _is_guest_token_revoked(token: dict[str, Any]) -> bool:
+ """Return True if the token predates a revocation on any of its
+ embedded-dashboard resources (``guest_token_revoked_before``)."""
+ issued_at = token.get("iat")
+ if not issued_at:
+ return False
+
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.dashboard import EmbeddedDashboardDAO
+
+ for resource in token.get("resources") or []:
+ if resource.get("type") != GuestTokenResourceType.DASHBOARD.value:
+ continue
+ embedded = EmbeddedDashboardDAO.find_by_id(str(resource.get("id")))
+ revoked_before = getattr(embedded, "guest_token_revoked_before", None)
+ if revoked_before is not None and issued_at < revoked_before:
+ return True
Review Comment:
**Suggestion:** Revocation can be bypassed for valid guest tokens that still use dashboard numeric IDs in `resources`: this check always treats `resource.id` as an embedded UUID and never resolves dashboard IDs to their embedded config before reading the revocation cutoff. Since resource validation and access checks still accept dashboard IDs, revoked dashboards can continue to be accessed with those tokens. Resolve dashboard IDs to their related embedded row (or check both forms) before comparing against the revocation timestamp. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Revoked embedded dashboards stay accessible via dashboard-ID tokens.
- ❌ Revocation logic inconsistent with existing access checks behavior.
- ⚠️ Legacy clients bypass new revocation without any error.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Issue a guest token via the REST API `POST /api/v1/security/guest_token/` implemented
in `superset/security/api.py:149-207`, passing a payload where `resources = [{"type":
"dashboard", "id": "<dashboard_id>"}]` and `<dashboard_id>` is the numeric dashboard ID
(or slug) rather than the embedded UUID. `ResourceSchema` at
`superset/security/api.py:62-65` accepts `id` as a string, and
`validate_guest_token_resources` at `superset/security/manager.py:3607-24` resolves this
ID using `Dashboard.get(str(resource["id"]))`, so the token is accepted and created with a
dashboard-based resource identifier.
2. Configure embedding for that dashboard using `EmbeddedDashboardDAO.upsert` as in
`superset/daos/dashboard.py:610-21` and the embedded dashboard API at
`superset/dashboards/api.py:2103-2107`, then obtain the embedded UUID for use with
revocation. Call `SupersetSecurityManager.revoke_guest_token_access(embedded_uuid)` at
`superset/security/manager.py:68-83`, which looks up the `EmbeddedDashboard` row by UUID
via `EmbeddedDashboardDAO.find_by_id(str(embedded_uuid))` and sets
`embedded.guest_token_revoked_before`.
3. Later, send an embedded HTTP request carrying the previously issued guest token in the
header or form field that `get_guest_user_from_request` reads
(`superset/security/manager.py:3652-66`). The Flask-Login request loader at
`superset/security/manager.py:660-665` calls `get_guest_user_from_request`, which
successfully decodes the token and then invokes `_is_guest_token_revoked(token)` at
`superset/security/manager.py:48-66`.
4. Inside `_is_guest_token_revoked`, the code at `superset/security/manager.py:59-65`
iterates over resources and, for dashboard resources, calls
`EmbeddedDashboardDAO.find_by_id(str(resource.get("id")))`. Because `resource["id"]` is
the dashboard ID (not the embedded UUID), `EmbeddedDashboardDAO.find_by_id` (configured
with `id_column_name = "uuid"` at `superset/daos/dashboard.py:605-608`) returns `None`, so
`revoked_before = getattr(embedded, "guest_token_revoked_before", None)` yields `None`.
The comparison `issued_at < revoked_before` at line 64 is never true,
`_is_guest_token_revoked` returns `False`, and `get_guest_user_from_request` returns a
`GuestUser` even though revocation was configured. Downstream, `has_guest_access` at
`superset/security/manager.py:3764-21` still grants access because it explicitly supports
dashboard-ID tokens by checking `str(resource["id"]) == str(dashboard.id)` before checking
the embedded UUID, so the revoked embedded dashboard remains accessible with legacy
dashboard-ID tokens.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=04f76c0c183c4571922eb8354406ac7b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=04f76c0c183c4571922eb8354406ac7b&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/security/manager.py
**Line:** 3698:3704
**Comment:**
*Security: Revocation can be bypassed for valid guest tokens that still use dashboard numeric IDs in `resources`: this check always treats `resource.id` as an embedded UUID and never resolves dashboard IDs to their embedded config before reading the revocation cutoff. Since resource validation and access checks still accept dashboard IDs, revoked dashboards can continue to be accessed with those tokens. Resolve dashboard IDs to their related embedded row (or check both forms) before comparing against the revocation timestamp.
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%2F40676&comment_hash=f929231633a68e847b1a4035f26948e3a33f2bf69ca3e100fffbabc61707d092&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40676&comment_hash=f929231633a68e847b1a4035f26948e3a33f2bf69ca3e100fffbabc61707d092&reaction=dislike'>👎</a>
##########
superset/security/manager.py:
##########
@@ -3681,6 +3684,43 @@ def get_guest_user_from_request(self, req: Request) -> Optional[GuestUser]:
return self.get_guest_user_from_token(cast(GuestToken, token))
+ @staticmethod
+ def _is_guest_token_revoked(token: dict[str, Any]) -> bool:
+ """Return True if the token predates a revocation on any of its
+ embedded-dashboard resources (``guest_token_revoked_before``)."""
+ issued_at = token.get("iat")
+ if not issued_at:
+ return False
Review Comment:
**Suggestion:** Returning `False` when `iat` is missing means tokens without an issued-at claim are always treated as not revoked, even when a dashboard has an active revocation cutoff. That weakens the revocation control and allows bypass for any token lacking `iat`. Reject missing `iat` (or treat it as revoked once a cutoff exists) to enforce revocation consistently. [security]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Guest tokens without iat ignore configured revocation cutoffs.
- ❌ Administrators cannot revoke such externally minted tokens.
- ⚠️ Behavior conflicts with stated revocation security objectives.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Construct a JWT for embedded access using the configured secret and algorithm (used by
`SupersetSecurityManager.parse_jwt_guest_token` at `superset/security/manager.py:91-101`).
The payload includes: `"type": "guest"`, a valid `"user"` object, a `"resources"` list
with at least one dashboard resource (e.g. `{"type": "dashboard", "id":
"<embedded_uuid_or_dashboard_id>"}`), `"rls_rules": []`, and a valid `"exp"` claim, but
intentionally omits the `"iat"` claim.
2. Send this JWT to Superset as part of an embedded request, in the header or form field
read by `SupersetSecurityManager.get_guest_user_from_request`
(`superset/security/manager.py:62-64`). `parse_jwt_guest_token` decodes the token without
requiring `"iat"` (it only enforces standard JWT validity and audience), and
`get_guest_user_from_request` at `superset/security/manager.py:69-37` verifies the
presence of `"user"`, `"resources"`, `"rls_rules"`, and `"type" == "guest"`, but does not
require `"iat"`, so the token proceeds to revocation checking.
3. `get_guest_user_from_request` calls `_is_guest_token_revoked(token)` at
`superset/security/manager.py:48-66`. Inside this function, line 52 reads `issued_at =
token.get("iat")`, which is `None` for this crafted token, and line 53-54 immediately
returns `False` when `if not issued_at:` is true. No `EmbeddedDashboardDAO` lookup or
comparison to `guest_token_revoked_before` is performed, regardless of whether revocation
has been configured for the embedded dashboard.
4. Because `_is_guest_token_revoked` returns `False`, `get_guest_user_from_request`
returns a `GuestUser` (line 46) and downstream access checks like `has_guest_access` at
`superset/security/manager.py:3764-21` can still authorize access to the embedded
dashboard when the resource IDs match. The unit test
`test_guest_token_without_iat_is_not_revoked` at
`tests/unit_tests/security/test_guest_token_revocation.py:62-64` currently codifies this
behavior, but it also means that any guest token lacking `"iat"` is effectively exempt
from revocation, weakening the control's effectiveness.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0f9513b5cb5540239aad45b815c3e9c3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0f9513b5cb5540239aad45b815c3e9c3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/security/manager.py
**Line:** 3691:3693
**Comment:**
*Security: Returning `False` when `iat` is missing means tokens without an issued-at claim are always treated as not revoked, even when a dashboard has an active revocation cutoff. That weakens the revocation control and allows bypass for any token lacking `iat`. Reject missing `iat` (or treat it as revoked once a cutoff exists) to enforce revocation consistently.
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%2F40676&comment_hash=af11721cf35836e92321f3106684ccb7d67403bd80c00936be211aa9b26a7bd8&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40676&comment_hash=af11721cf35836e92321f3106684ccb7d67403bd80c00936be211aa9b26a7bd8&reaction=dislike'>👎</a>
##########
superset/security/manager.py:
##########
@@ -3681,6 +3684,43 @@ def get_guest_user_from_request(self, req: Request) -> Optional[GuestUser]:
return self.get_guest_user_from_token(cast(GuestToken, token))
+ @staticmethod
+ def _is_guest_token_revoked(token: dict[str, Any]) -> bool:
+ """Return True if the token predates a revocation on any of its
+ embedded-dashboard resources (``guest_token_revoked_before``)."""
+ issued_at = token.get("iat")
+ if not issued_at:
+ return False
+
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.dashboard import EmbeddedDashboardDAO
+
+ for resource in token.get("resources") or []:
+ if resource.get("type") != GuestTokenResourceType.DASHBOARD.value:
+ continue
+ embedded = EmbeddedDashboardDAO.find_by_id(str(resource.get("id")))
+ revoked_before = getattr(embedded, "guest_token_revoked_before", None)
+ if revoked_before is not None and issued_at < revoked_before:
+ return True
+ return False
+
+ @transaction()
+ def revoke_guest_token_access(
+ self, embedded_uuid: str, before: Optional[int] = None
+ ) -> None:
+ """Revoke all guest tokens issued for an embedded dashboard before
+ ``before`` (epoch seconds, default: now). Subsequent tokens are
+ unaffected."""
+ # pylint: disable=import-outside-toplevel
+ from superset.daos.dashboard import EmbeddedDashboardDAO
+
+ embedded = EmbeddedDashboardDAO.find_by_id(str(embedded_uuid))
+ if embedded is None:
+ return
+ embedded.guest_token_revoked_before = (
+ before if before is not None else int(self._get_current_epoch_time())
Review Comment:
**Suggestion:** The revocation cutoff is truncated to an integer second, while issued-at values are created from `time.time()` and can contain fractional seconds. This creates a same-second gap where tokens issued just before revocation are not invalidated because their `iat` is greater than the floored cutoff. Store/use consistent precision (or round cutoff up) so all tokens issued before revocation are reliably rejected. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Recently revoked guest tokens can remain valid briefly.
- ⚠️ Revocation semantics differ from intuitive "revoke immediately".
- ⚠️ Difficult to reason about exact revocation cutoff behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Issue a guest token using `SupersetSecurityManager.create_guest_access_token` at
`superset/security/manager.py:25-51`, either directly (as in
`tests/integration_tests/security_tests.py:11-17`) or via the REST API `POST
/api/v1/security/guest_token/` (`superset/security/api.py:149-207`).
`create_guest_access_token` calls `_get_current_epoch_time()`
(`superset/security/manager.py:1-3`), which returns `time.time()` as a float, and stores
this exact float in the JWT `iat` claim (`"iat": now` at line 44).
2. Shortly after issuing the token—within the same wall-clock second—invoke
`SupersetSecurityManager.revoke_guest_token_access(embedded_uuid)` at
`superset/security/manager.py:68-83` for the corresponding embedded dashboard UUID. This
method sets `embedded.guest_token_revoked_before = before if before is not None else
int(self._get_current_epoch_time())` (lines 81-82), truncating the revocation cutoff to an
integer number of seconds via `int(...)`.
3. On a subsequent HTTP request where the client presents the previously issued token,
Flask-Login calls `SupersetSecurityManager.get_guest_user_from_request` as a request
loader (`superset/security/manager.py:660-665` and 3652-46). This decodes the token, then
calls `_is_guest_token_revoked(token)` (`superset/security/manager.py:48-66`), which reads
`issued_at = token.get("iat")` (float) and, for the relevant embedded dashboard resource,
reads `revoked_before = embedded.guest_token_revoked_before` (integer floor of the
revocation time).
4. Because the comparison in `_is_guest_token_revoked` is `if revoked_before is not None
and issued_at < revoked_before:` (lines 63-65), any tokens issued in the interval
`[floor(revocation_time), revocation_time)` have `issued_at >= revoked_before` even though
they were created before the actual revocation moment. For such tokens
`_is_guest_token_revoked` returns `False`, `get_guest_user_from_request` accepts the
token, and the user retains access despite revocation having been requested.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=37e112c31f2b493ea22d36ad54bbd2e3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=37e112c31f2b493ea22d36ad54bbd2e3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/security/manager.py
**Line:** 3720:3721
**Comment:**
*Logic Error: The revocation cutoff is truncated to an integer second, while issued-at values are created from `time.time()` and can contain fractional seconds. This creates a same-second gap where tokens issued just before revocation are not invalidated because their `iat` is greater than the floored cutoff. Store/use consistent precision (or round cutoff up) so all tokens issued before revocation are reliably rejected.
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%2F40676&comment_hash=8a73aae98e47a891263a02c2134acfb6dc9bb26f4283abb01db20261821a46ea&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40676&comment_hash=8a73aae98e47a891263a02c2134acfb6dc9bb26f4283abb01db20261821a46ea&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org