codeant-ai-for-open-source[bot] commented on code in PR #40769:
URL: https://github.com/apache/superset/pull/40769#discussion_r3357123604
##########
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)
Review 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]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Dashboard permalink hits execute two dashboard access checks.
- โ ๏ธ Extra query overhead negligible relative to request cost.
```
</details>
<details>
<summary><b>Steps of Reproduction โ
</b></summary>
```mdx
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.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f00f00328d744da3a8cd5b247a4512e6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f00f00328d744da3a8cd5b247a4512e6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
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
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40769&comment_hash=775015909110b4531fd1f2ad92a2802585524a5ee96f6d02bf2a95b62ed3fea2&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40769&comment_hash=775015909110b4531fd1f2ad92a2802585524a5ee96f6d02bf2a95b62ed3fea2&reaction=dislike'>๐</a>
--
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]
