Copilot commented on code in PR #40769:
URL: https://github.com/apache/superset/pull/40769#discussion_r3364284730
##########
superset/views/core.py:
##########
@@ -847,17 +847,39 @@ def dashboard(
@has_access
@expose("/dashboard/p/<key>/", methods=("GET",))
- def dashboard_permalink(
+ def dashboard_permalink( # noqa: C901
self,
key: str,
) -> FlaskResponse:
try:
value = GetDashboardPermalinkCommand(key).run()
except (DashboardPermalinkGetFailedError, DashboardAccessDeniedError)
as ex:
+ if not get_current_user():
+ return redirect_to_login()
return json_error_response(__("Error: %(msg)s", msg=ex.message),
status=404)
if not value:
+ if not get_current_user():
+ return redirect_to_login()
return json_error_response(_("permalink state not found"),
status=404)
+ 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", {})
Review Comment:
GetDashboardPermalinkCommand.run() already validates the dashboard via
DashboardDAO.get_by_id_or_slug(value["dashboardId"]) (including access checks).
The additional Dashboard.get(...) + raise_for_access() logic duplicates that
work and adds another DB fetch on every permalink request; the downstream
/dashboard/<id_or_slug>/ view already handles published/access redirects.
Consider removing this extra lookup/access block here and relying on the
existing redirect target to enforce access/published behavior.
##########
superset/views/core.py:
##########
@@ -847,17 +847,39 @@ def dashboard(
@has_access
@expose("/dashboard/p/<key>/", methods=("GET",))
- def dashboard_permalink(
+ def dashboard_permalink( # noqa: C901
self,
key: str,
) -> FlaskResponse:
try:
value = GetDashboardPermalinkCommand(key).run()
except (DashboardPermalinkGetFailedError, DashboardAccessDeniedError)
as ex:
+ if not get_current_user():
+ return redirect_to_login()
Review Comment:
New unauthenticated behavior was added (redirect_to_login on access-denied /
missing permalink), but there are no tests asserting this for
/dashboard/p/<key>/ (existing integration coverage only checks the redirect URL
for an authenticated admin). Adding an integration test that hits a restricted
dashboard permalink while logged out and asserts a 302 to the FAB login URL
(with next=...) would prevent regressions and verify the issue is fixed.
--
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]
