rusackas commented on code in PR #40699:
URL: https://github.com/apache/superset/pull/40699#discussion_r3345993069
##########
superset/security/manager.py:
##########
@@ -3500,6 +3502,41 @@ def get_guest_user_from_request(self, req: Request) ->
Optional[GuestUser]:
return self.get_guest_user_from_token(cast(GuestToken, token))
+ def _guest_token_is_revoked(self, token: dict[str, Any]) -> bool:
+ """
+ Return True if any dashboard resource referenced by the token has been
+ revoked since the token was issued — i.e. the token's ``iat`` predates
+ that embedded dashboard's ``guest_token_revoked_before``.
+
+ Revocation is per embedded dashboard, so revoking one embed does not
+ affect guest tokens for other dashboards. Tokens with no ``iat`` (none
+ are issued without one) are treated as not revoked.
+ """
+ # pylint: disable=import-outside-toplevel
+ from datetime import timezone
+
+ from superset.daos.dashboard import EmbeddedDashboardDAO
+
+ iat = token.get("iat")
+ if iat is None:
+ return False
Review Comment:
Good catch — fixed in 0811472335. `_guest_token_is_revoked` now fails closed
for a token that lacks an `iat` claim (returns `True`) instead of silently
treating it as not-revoked. Superset always issues guest tokens with `iat`, so
legitimate tokens are unaffected, but a token that omits it can no longer be
positioned relative to a revocation instant, so it is rejected. Added a unit
test covering the missing-`iat` case.
##########
superset/security/manager.py:
##########
@@ -3500,6 +3502,41 @@ def get_guest_user_from_request(self, req: Request) ->
Optional[GuestUser]:
return self.get_guest_user_from_token(cast(GuestToken, token))
+ def _guest_token_is_revoked(self, token: dict[str, Any]) -> bool:
+ """
+ Return True if any dashboard resource referenced by the token has been
+ revoked since the token was issued — i.e. the token's ``iat`` predates
+ that embedded dashboard's ``guest_token_revoked_before``.
+
+ Revocation is per embedded dashboard, so revoking one embed does not
+ affect guest tokens for other dashboards. Tokens with no ``iat`` (none
+ are issued without one) are treated as not revoked.
+ """
+ # pylint: disable=import-outside-toplevel
+ from datetime import timezone
+
+ from superset.daos.dashboard import EmbeddedDashboardDAO
+
+ iat = token.get("iat")
+ if iat is None:
+ return False
+ for resource in token.get("resources") or []:
+ if resource.get("type") != GuestTokenResourceType.DASHBOARD.value:
+ continue
+ embedded = EmbeddedDashboardDAO.find_by_id(str(resource["id"]))
+ revoked_before = (
+ embedded.guest_token_revoked_before if embedded else None
+ )
+ if revoked_before is None:
+ continue
Review Comment:
Agreed, this was a real gap — fixed in 0811472335. `_guest_token_is_revoked`
now resolves the resource id both ways: it first tries
`EmbeddedDashboardDAO.find_by_id` (embedded uuid) and, failing that, resolves
the id as a dashboard id/slug via `Dashboard.get` and checks that dashboard's
embedded configs. So legacy dashboard-id guest tokens are now subject to
per-dashboard revocation, not just embedded-uuid tokens. Added an integration
test (`test_legacy_dashboard_id_token_is_revoked`).
--
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]
