fix(roles): allow Public role to read themes#37295
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Code Review Agent Run #a8245cActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
Sequence DiagramShows the added permission check that lets unauthenticated (Public) users access a dashboard's theme so public dashboards remain viewable when a theme is assigned. sequenceDiagram
participant Unauthenticated User
participant Web App
participant SupersetSecurityManager
participant Storage
Unauthenticated User->>Web App: Request public dashboard
Web App->>SupersetSecurityManager: Authorize as Public (check can_read Theme)
SupersetSecurityManager-->>Web App: Allowed (can_read Theme)
Web App->>Storage: Fetch dashboard and associated theme
Storage-->>Web App: Dashboard + Theme
Web App-->>Unauthenticated User: 200 OK (themed dashboard)
Generated by CodeAnt AI |
|
CodeAnt AI finished reviewing your PR. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where public dashboards become inaccessible when assigned a theme. The fix adds the can_read permission on Theme to the built-in Public role's permission set, enabling unauthenticated users to view dashboards with themes applied.
Changes:
- Added
("can_read", "Theme")permission toPUBLIC_ROLE_PERMISSIONS - Updated comment to reflect that both CSS and themes are for dashboard styling
Comments suppressed due to low confidence (3)
superset/security/manager.py:1262
- Testing for None should use the 'is' operator.
PermissionView.permission # pylint: disable=singleton-comparison
== None, # noqa: E711
superset/security/manager.py:1264
- Testing for None should use the 'is' operator.
PermissionView.view_menu # pylint: disable=singleton-comparison
== None, # noqa: E711
superset/security/manager.py:261
- Nested for statement uses loop variable 'key' of enclosing for statement.
for key in equivalent:
| # CSS for dashboard styling | ||
| # CSS and themes for dashboard styling | ||
| ("can_read", "CssTemplate"), | ||
| ("can_read", "Theme"), |
There was a problem hiding this comment.
Consider adding a test assertion for the Theme permission in the existing test_public_role_permissions method (e.g., assert ("can_read", "Theme") in public_perm_set). This would ensure the Theme permission is explicitly verified alongside CssTemplate and other styling-related permissions, providing better regression protection for the fix in issue #37294.
|
The suggestion to add a test assertion for the Theme permission in |
SUMMARY
Fixes #37294
In #36548 a canned set of permissions for a Public role was added. This is great! We need to also add
can read Theme, as without that, a dashboard becomes un-Public if it is assigned a theme.TESTING INSTRUCTIONS
Create a public dashboard and verify it's accessible to unauthenticated user
Assign it a theme
Verify it's accessible (after this fix is applied) and inaccessible in Superset 6.0.0.