codeant-ai-for-open-source[bot] commented on code in PR #40026:
URL: https://github.com/apache/superset/pull/40026#discussion_r3220154131
##########
superset/security/api.py:
##########
@@ -171,6 +171,19 @@ def guest_token(self) -> Response:
try:
body = guest_token_create_schema.load(request.json)
self.appbuilder.sm.validate_guest_token_resources(body["resources"])
+ # Verify requesting user has access to each specified resource
+ for resource in body["resources"]:
+ if resource["type"] == GuestTokenResourceType.DASHBOARD.value:
+ from superset.models.embedded_dashboard import
EmbeddedDashboard # noqa: PLC0415
+ embedded = (
+ db.session.query(EmbeddedDashboard)
+ .filter_by(uuid=resource["id"])
+ .one_or_none()
+ )
+ if embedded and not
self.appbuilder.sm.can_access_dashboard(
+ embedded.dashboard
+ ):
+ raise ForbiddenError()
Review Comment:
Yes — the review comment is **correct**.
### Why this is a real issue
The new check only validates access when the resource maps to an
`EmbeddedDashboard` UUID.
If the request uses a legacy dashboard ID, `embedded` is `None`, so the code
skips the authorization check entirely.
That means a user who can call `guest_token` could still mint a guest token
for a dashboard they **shouldn’t** be able to access, which defeats the intent
of the fix.
### Minimal fix
Apply the access check to the resolved dashboard in **both** cases:
- legacy dashboard ID
- embedded dashboard UUID
For example, resolve the dashboard using the same logic already used by
`validate_guest_token_resources`, then always call `can_access_dashboard(...)`
on the resolved dashboard.
```python
for resource in body["resources"]:
if resource["type"] == GuestTokenResourceType.DASHBOARD.value:
# resolve dashboard by legacy ID or embedded UUID
dashboard = ...
if not self.appbuilder.sm.can_access_dashboard(dashboard):
raise ForbiddenError()
```
### Recommendation
I’d keep the change localized in `guest_token()` and add an integration test
that verifies:
- a caller without dashboard access gets `403`
- this works for both dashboard ID and embedded UUID inputs
So the fix is necessary, and the current patch should be adjusted rather
than merged as-is.
--
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]
