fix(auth): redirect to login on failure to access dashboard permalink#40769
fix(auth): redirect to login on failure to access dashboard permalink#40769dmunozv04 wants to merge 2 commits into
Conversation
…dashboard permalink
Code Review Agent Run #8276b3Actionable 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 AI Code Review powered by |
dosubot
Bot
added
authentication
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40769 +/- ##
==========================================
- Coverage 64.06% 55.95% -8.11%
==========================================
Files 2664 2664
Lines 143922 143856 -66
Branches 33096 33066 -30
==========================================
- Hits 92204 80499 -11705
- Misses 50108 62639 +12531
+ Partials 1610 718 -892
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| dashboard = Dashboard.get(value["dashboardId"]) | ||
|
|
||
| if not dashboard: | ||
| if not get_current_user(): | ||
| return redirect_to_login() | ||
| abort(404) | ||
|
|
||
| # Redirect anonymous users to login for unpublished dashboards, | ||
| # in the edge case where a dataset has been shared with public | ||
| if not get_current_user() and not dashboard.published: | ||
| return redirect_to_login() | ||
|
|
||
| try: | ||
| dashboard.raise_for_access() | ||
| except SupersetSecurityException: | ||
| if not get_current_user(): | ||
| return redirect_to_login() | ||
| abort(404) |
There was a problem hiding this comment.
Suggestion: This adds a second dashboard fetch and a second permission check even though GetDashboardPermalinkCommand.run() already resolves the dashboard and enforces access via DashboardDAO.get_by_id_or_slug. The duplicate query/check adds avoidable overhead on every permalink hit and can lead to inconsistent outcomes if permissions/state change between checks. Reuse the command's access decision instead of rechecking with another DB round trip. [performance]
Severity Level: Minor 🧹
- ⚠️ Dashboard permalink hits execute two dashboard access checks.
- ⚠️ Extra query overhead negligible relative to request cost.Steps of Reproduction ✅
1. A user follows a dashboard permalink such as `GET /superset/dashboard/p/<key>/`, which
is handled by `dashboard_permalink` in `superset/views/core.py:848-853`.
2. At `superset/views/core.py:35-40` (hunk lines 855-860), `dashboard_permalink` calls
`GetDashboardPermalinkCommand(key).run()`, whose implementation in
`superset/commands/dashboard/permalink/get.py:42-49` performs `KeyValueDAO.get_value(...)`
and then `DashboardDAO.get_by_id_or_slug(value["dashboardId"])`
(`superset/daos/dashboard.py:135-161`), querying the `Dashboard` table and calling
`dashboard.raise_for_access()` once.
3. If `run()` returns a non-`None` value without raising, `dashboard_permalink` then
executes `dashboard = Dashboard.get(value["dashboardId"])` followed by
`dashboard.raise_for_access()` in the block at `superset/views/core.py:865-882`, issuing a
second database lookup for the same dashboard and repeating the access check within the
same request.
4. This double lookup and double authorization check occur on every successful dashboard
permalink hit, adding an extra DB round-trip and redundant permission evaluation compared
to reusing the command's existing `DashboardDAO.get_by_id_or_slug` access decision.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/views/core.py
**Line:** 865:882
**Comment:**
*Performance: This adds a second dashboard fetch and a second permission check even though `GetDashboardPermalinkCommand.run()` already resolves the dashboard and enforces access via `DashboardDAO.get_by_id_or_slug`. The duplicate query/check adds avoidable overhead on every permalink hit and can lead to inconsistent outcomes if permissions/state change between checks. Reuse the command's access decision instead of rechecking with another DB round trip.
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
|
The flagged issue is correct. The You can resolve this by reusing the dashboard object if the command returns it, or by ensuring the command's result is sufficient. Since the command already performs the necessary authorization, you can simplify the view logic by removing the redundant database lookup and permission check. Would you like me to check the rest of the comments on this PR for similar issues? superset/views/core.py |
There was a problem hiding this comment.
Pull request overview
This PR fixes an authentication UX bug where opening a dashboard permalink for a private dashboard while logged out returned a JSON access error instead of redirecting to the login page (aligning permalink behavior with the regular /dashboard/<id>/ route).
Changes:
- Update
dashboard_permalinkto redirect unauthenticated users to the login view when access is denied (and in some not-found cases). - Add dashboard existence / published checks and an explicit
raise_for_access()gate before redirecting to the dashboard route.
| dashboard = Dashboard.get(value["dashboardId"]) | ||
|
|
||
| if not dashboard: | ||
| if not get_current_user(): | ||
| return redirect_to_login() | ||
| abort(404) | ||
|
|
||
| # Redirect anonymous users to login for unpublished dashboards, | ||
| # in the edge case where a dataset has been shared with public | ||
| if not get_current_user() and not dashboard.published: | ||
| return redirect_to_login() | ||
|
|
||
| try: | ||
| dashboard.raise_for_access() | ||
| except SupersetSecurityException: | ||
| if not get_current_user(): | ||
| return redirect_to_login() | ||
| abort(404) | ||
| dashboard_id, state = value["dashboardId"], value.get("state", {}) |
| except (DashboardPermalinkGetFailedError, DashboardAccessDeniedError) as ex: | ||
| if not get_current_user(): | ||
| return redirect_to_login() |
SUMMARY
Fixes #40768, where accessing a permalink to a private dashboard without being logged in would return an error instead of redirecting to the login page.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
{"error": "Error: You don't have access to this dashboard."}After: redirect to login page
TESTING INSTRUCTIONS
Enable RBAC, share a dashboard with a role.
Share -> copy permalink
Open that link on a private tab (or log out before)
You should be redirected to the login page instead of getting Error
{"error": "Error: You don't have access to this dashboard."}ADDITIONAL INFORMATION
Copies functionality from #30380. I've set noqa: C901 to be able to pass listing while copying the section of code verbatim.
If wanted, this could be refactored into a helper function to reduce complexity