gabotorresruiz commented on code in PR #45622:
URL: https://github.com/apache/superset/pull/45622#discussion_r4355197339
##########
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:
This block worries me a bit. `get_datasource_by_id` raises
`DatasourceNotFoundValidationError`, a `ValidationError`, and nothing here
catches it. The datasource block up at line 283 already resolved the same
datasource and appended that error to `exceptions`, so we arrive here and raise
it a second time with no handler: `on_error` only translates `SQLAlchemyError`
and `ChartRestApi.put` has no `except ValidationError`, so `@safe` answers `500
Fatal error`.
Concretely, a `PUT /api/v1/chart/<id>` carrying `datasource_type`, a
`datasource_id` that does not exist, and a new entry in `dashboards` returns
500 here and 422 `{"datasource_id": ["Datasource does not exist"]}` on master.
Drop just the `dashboards` key from that same request and this branch gives the
422 too, so it is the second lookup.
Smallest fix I could find, since `validate()` raises `ChartInvalidError`
anyway once `exceptions` is non-empty:
```suggestion
if exceptions:
return
datasource_type: str = self._properties.get("datasource_type",
"")
```
With that in place the 422 is back, every other cell of the matrix is
unchanged (repoint to a dataset the caller cannot read still 403s, link-only
still 200s), and all 74 new unit tests still pass. A case in
`test_update_dashboard_access.py` that combines a missing `datasource_id` with
a new `dashboards` entry would pin it.
Separately, this branch re-resolves and re-checks what line 283 already
resolved and already passed through `raise_for_access`, so reusing that object
instead would remove the duplicate lookup as well as the 500. That one needs
the repoint unit test retargeted at `validate()`, so the two-line guard above
may be the better trade for now.
##########
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:
This is the one I would most want changed. The comment says the lifted
filter makes a soft-deleted member count as already present, but for a
dashboard that came out of a query it does not. The soft-delete listener
attaches `with_loader_criteria(Slice, ...)` to the `Dashboard` SELECT, and that
option defaults to `propagate_to_loaders=True`, so the criteria is already
baked into `existing_dashboard.slices` before `skip_visibility_filter` ever
opens. On the `PUT` path `self._model` comes from `DashboardDAO.find_by_id`, so
that is the production shape; a dashboard built in Python carries no criteria,
which is why it looks fine from a unit test.
Measured on this branch with a dashboard holding one live and one archived
member: inside the `with` block, `{s.id for s in dashboard.slices}` returns
only the live id while `dashboard_slices` holds both. The archived member
therefore lands in `added_slices`, so an editor who cannot read its dataset
gets a 403 on every save of that dashboard for as long as the layout still
carries its slot. Dropping that slot from the layout saves fine, which is how I
isolated it.
On master that save already fails (a 422, which is what #45543 is for), so
nobody loses a working flow today. The part that concerns me is forward: I
merged #45543 into this branch and the save is still a 403, and disabling only
this check on the merged tree turns it into a 200. So this would sit on top of
that fix and keep the same dashboard unsaveable. #45538 already carries the
right primitive, `_member_chart_ids(dashboard_id)`, which reads
`dashboard_slices` instead of `Dashboard.slices` for exactly this reason.
Worth saying why the new tests do not catch it:
`test_added_member_datasource_access_skips_soft_deleted_member` builds its
dashboard in Python, and it still passes with the `skip_visibility_filter` call
deleted outright. A version that loads the dashboard through
`db.session.query(Dashboard)` before calling the helper would fail on this
baseline.
##########
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:
Question about the gate rather than the rule. This fires only when the
request carries `json_metadata`, but `UpdateDashboardCommand` also accepts
`slices`, and that key rebuilds membership straight through
`DashboardDAO.update` without ever reaching `set_dash_metadata`. `slices` is
not in `DashboardPutSchema`, so the REST API cannot send it, but
`superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py:345`
does exactly that (`{"position_json": ..., "slices": existing + [new_chart]}`,
no `json_metadata`) and only checks `raise_for_editorship` on the dashboard.
`generate_dashboard.py:392` assigns `dashboard.slices` directly from
`request.chart_ids` with no command at all.
I ran both shapes as the same `Gamma` editor with no grant on the chart's
dataset: the `json_metadata` shape is refused, the `slices` shape links the
chart. Is scoping to the request shape deliberate for now? Reading
`self._properties.get("slices")` here too, or checking where membership is
actually written, would close it. Happy to be told the MCP tools are out of
scope for this one.
##########
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:
Not a blocker, two small things. Deleting this whole block leaves every new
test passing: the DAO-level copy test calls the helper directly, so the
endpoint itself is never exercised, which matches what codecov flags on this
file. And `old_to_new_slice_ids={0: 0}` as a truthy flag is hard to read here;
a named parameter on the helper would say what it means. The behaviour itself
is right, for what it is worth: a `POST /api/v1/dashboard/<id>/copy/` whose
layout references an unreadable chart returns 403 and copies nothing.
--
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]
