gabotorresruiz commented on code in PR #45622:
URL: https://github.com/apache/superset/pull/45622#discussion_r4364192659
##########
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:
Confirmed, and the wider shape is safe. I re-ran the old commit first to be
sure the `500` was real, then rebuilt the matrix on this one. The refusal for a
repoint to a dataset the caller cannot read now comes from `validate()`'s own
`raise_for_access(datasource=...)` a few lines up, which runs before the
`dashboards` block, so `Gamma` still gets a `403` with nothing linked. I also
checked the one shape the deferral newly leans on, `datasource_type` with no
`datasource_id`: `validate()` falls back to the stored id there, so that is
still refused too. Closing this one.
##########
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:
Confirmed. On this commit the check never reaches the archived member:
`raise_for_datasources_access` is not called at all for that save, where on
`a19ee49f9b` it was called with the archived member's dataset and answered
`403`.
One thing worth knowing, and not yours to fix: the `Gamma` save still does
not complete here, it fails later in `set_dash_metadata` with an
`IntegrityError` on `dashboard_slices`. I measured the same `422` on `87d1abb2`
and on `a19ee49f9b` for an `Admin`, so it is pre-existing and it is exactly
what #45543 fixes. With #45543 merged into this head that save is a `200`.
Closing.
##########
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:
Confirmed, the `slices` shape is refused now: `DashboardForbiddenError` with
nothing linked, both for `Gamma` and for a custom `Alpha` role without
`all_datasource_access`, where both linked the chart on `a19ee49f9b`.
On the MCP claim I read both tools rather than take it, and it holds:
`add_chart_to_existing_dashboard.py:271` checks before it builds the `slices`
payload, and `generate_dashboard.py:272` checks each chart before
`dashboard.slices = fresh_charts` at line 392. Worth noting that the second one
never goes through a command at all, so it is covered by its own check and not
by this gate. Closing.
##########
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:
Confirmed. Deleting this block now turns
`test_dashboard_requires_datasource_access_for_added_chart` red, so the
endpoint is genuinely exercised, and the truthy flag is gone. Closing.
--
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]
