sha174n commented on code in PR #45622:
URL: https://github.com/apache/superset/pull/45622#discussion_r4355745850
##########
superset/commands/chart/update.py:
##########
@@ -155,6 +155,31 @@ def _validate_new_dashboard_access(
if dash.is_managed_externally or not
security_manager.is_editor(dash):
raise DashboardsForbiddenError()
+ # A chart's datasource can be served through a dashboard it belongs
+ # to (drill access, and promiscuous-viewer access), so linking the
+ # chart to a new dashboard must require access to that datasource.
+ # Use the datasource this request will leave on the chart: the
+ # incoming one when the same request repoints it, otherwise the
+ # stored one. (Admin and unresolvable-datasource exemptions live in
+ # ``raise_for_datasources_access``.)
+ datasource_type: str = self._properties.get("datasource_type", "")
Review Comment:
Addressed in 28a4528eae: when the request repoints the chart, the link check
now leaves the incoming datasource to `validate()` (already resolved and
checked there), so the second lookup is gone and a missing `datasource_id` with
a new dashboard returns 422, pinned by
`test_update_chart_new_dashboard_with_missing_datasource`.
##########
superset/daos/dashboard.py:
##########
@@ -609,6 +619,58 @@ def validate_update_slug_uniqueness(dashboard_id: int,
slug: str | None) -> bool
return not db.session.query(dashboard_query.exists()).scalar()
return True
+ @classmethod
+ def raise_for_added_member_datasource_access(
+ cls,
+ data: dict[str, Any],
+ existing_dashboard: Dashboard | None = None,
+ old_to_new_slice_ids: dict[int, int] | None = None,
+ ) -> None:
+ """Require the acting user's access to each chart newly added to a
dashboard.
+
+ A chart's datasource can be served through a dashboard it belongs to
+ (see ``can_drill_dataset_via_dashboard_access`` /
``has_drill_access``),
+ so adding one must not grant data the user could not otherwise read.
+ This runs from the command ``validate()`` layer (not the DAO write
+ path) so a refusal is translated to ``DashboardForbiddenError`` -> 403
+ before any side effects, and the rule lives in one place.
+
+ Datasources are resolved across types and deduplicated; the admin,
+ no-request-user and unresolvable-datasource exemptions live in
+ ``security_manager.raise_for_datasources_access``. The clone path (a
+ truthy ``old_to_new_slice_ids``) is exempt: it duplicates charts from a
+ source dashboard the caller already edits, so it grants nothing new;
+ the copy-with-layout mode passes no map and is checked like
+ create/update. Raises ``SupersetSecurityException`` when refused.
+ """
+ if old_to_new_slice_ids:
+ return
+ positions = data.get("positions")
+ if not positions:
+ return
+ slice_ids = _slice_ids_from_positions(positions)
+ if not slice_ids:
+ return
+ with skip_visibility_filter(db.session, Slice):
+ # Read the existing membership with the visibility filter lifted,
the
+ # same as ``set_dash_metadata``'s rebuild, so a soft-deleted member
+ # still referenced by the layout is recognised as already present
+ # and not mistaken for a new addition.
+ prior_slice_ids = (
+ {slc.id for slc in existing_dashboard.slices}
+ if existing_dashboard
+ else set()
+ )
Review Comment:
Fixed in 28a4528eae: prior membership is read from `dashboard_slices`, and
the soft-deleted test now loads the dashboard through
`db.session.query(Dashboard)` and asserts its `slices` hides the archived
member.
##########
superset/commands/dashboard/update.py:
##########
@@ -171,6 +171,18 @@ def validate(self) -> None:
except SupersetSecurityException as ex:
raise DashboardForbiddenError() from ex
+ # A chart added to the dashboard must be one whose datasource the
editor
+ # can access (its datasource can be served through the dashboard).
Check
+ # here, before ``run`` mutates anything, so a refusal is a 403 and no
+ # side effects (e.g. report deactivation) have fired yet.
+ if json_metadata := self._properties.get("json_metadata"):
Review Comment:
Not deliberate; `validate()` now also reads `slices` (28a4528eae), covered
in the integration test. Both MCP tools already run `check_chart_data_access`
on each chart before writing membership.
##########
superset/commands/dashboard/copy.py:
##########
@@ -62,3 +64,17 @@ def validate(self) -> None:
raise DashboardInvalidError()
if not security_manager.is_editor(self._original_dash):
raise DashboardForbiddenError()
+
+ # A copy that rebuilds membership from a caller-supplied layout must
not
+ # pull in a chart whose datasource the caller cannot access. The
+ # ``duplicate_slices`` (clone) mode is exempt: it duplicates charts
from
+ # the source dashboard the caller already edits, so it grants nothing
new.
+ try:
+ DashboardDAO.raise_for_added_member_datasource_access(
+ json.loads(self._properties["json_metadata"]),
+ old_to_new_slice_ids={0: 0}
+ if self._properties.get("duplicate_slices")
+ else None,
+ )
Review Comment:
Fixed in 28a4528eae: the flag is gone (copy gates on `duplicate_slices`
itself), and the integration test now hits `/copy/` with a layout: 403 without
the grant, 200 with it.
--
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]
