korbit-ai[bot] commented on code in PR #35523:
URL: https://github.com/apache/superset/pull/35523#discussion_r2406046389
##########
superset/commands/chart/update.py:
##########
@@ -71,6 +72,28 @@ def run(self) -> Model:
return ChartDAO.update(self._model, self._properties)
+ def _validate_new_dashboard_access(
+ self, requested_dashboards: list[Dashboard], exceptions:
list[Exception]
+ ) -> None:
+ """
+ Validate user has access to any NEW dashboard relationships.
+ Existing relationships are preserved to maintain chart ownership
rights.
+ """
+ if not self._model:
+ return
+
+ existing_dashboard_ids = {d.id for d in self._model.dashboards}
+ requested_dashboard_ids = {d.id for d in requested_dashboards}
+
+ if new_dashboard_ids := requested_dashboard_ids -
existing_dashboard_ids:
+ # For NEW dashboard relationships, verify user has access
+ accessible_dashboards =
DashboardDAO.find_by_ids(list(new_dashboard_ids))
+ accessible_dashboard_ids = {d.id for d in accessible_dashboards}
+ unauthorized_dashboard_ids = new_dashboard_ids -
accessible_dashboard_ids
Review Comment:
### Redundant access validation for non-existent dashboards <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The access validation logic incorrectly assumes that if a dashboard is not
returned by DashboardDAO.find_by_ids(), the user lacks access to it, when it
could simply mean the dashboard doesn't exist.
###### Why this matters
This creates a confusing user experience where attempting to add a
non-existent dashboard to a chart results in a
DashboardsNotFoundValidationError being raised twice - once for the existence
check and once for the access check, even though the real issue is that the
dashboard doesn't exist at all.
###### Suggested change ∙ *Feature Preview*
Remove the redundant access validation since the existence check already
handles non-existent dashboards. The `_validate_new_dashboard_access` method
should only be called after confirming all dashboards exist:
```python
def _validate_new_dashboard_access(
self, requested_dashboards: list[Dashboard], exceptions: list[Exception]
) -> None:
"""
Validate user has access to any NEW dashboard relationships.
Existing relationships are preserved to maintain chart ownership rights.
"""
if not self._model:
return
existing_dashboard_ids = {d.id for d in self._model.dashboards}
requested_dashboard_ids = {d.id for d in requested_dashboards}
if new_dashboard_ids := requested_dashboard_ids - existing_dashboard_ids:
# For NEW dashboard relationships, verify user has access
# Note: All dashboards are guaranteed to exist at this point
accessible_dashboards =
DashboardDAO.find_by_ids(list(new_dashboard_ids))
accessible_dashboard_ids = {d.id for d in accessible_dashboards}
unauthorized_dashboard_ids = new_dashboard_ids -
accessible_dashboard_ids
if unauthorized_dashboard_ids:
exceptions.append(DashboardForbiddenError()) # Use appropriate
access error
```
And modify the validation flow:
```python
if dashboard_ids is not None:
# First, verify all requested dashboards exist
dashboards = DashboardDAO.find_by_ids(
dashboard_ids,
skip_base_filter=True,
)
if len(dashboards) != len(dashboard_ids):
exceptions.append(DashboardsNotFoundValidationError())
else:
# Only validate access if all dashboards exist
self._validate_new_dashboard_access(dashboards, exceptions)
self._properties["dashboards"] = dashboards
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc548e1b-486b-4ca9-9692-2c9323d9d3bb/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc548e1b-486b-4ca9-9692-2c9323d9d3bb?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc548e1b-486b-4ca9-9692-2c9323d9d3bb?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc548e1b-486b-4ca9-9692-2c9323d9d3bb?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/cc548e1b-486b-4ca9-9692-2c9323d9d3bb)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:58c964c6-63de-4487-997c-bd2ea59d5232 -->
[](58c964c6-63de-4487-997c-bd2ea59d5232)
--
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]
