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: [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]
