sadpandajoe commented on code in PR #45538:
URL: https://github.com/apache/superset/pull/45538#discussion_r4384125124
##########
superset/daos/dashboard.py:
##########
@@ -305,6 +305,59 @@ def _existing_chart_ids(chart_ids: set[int]) -> set[int]:
return {row[0] for row in rows}
+def _member_chart_ids(dashboard_id: int | None) -> set[int]:
+ """IDs of *dashboard_id*'s member charts, archived ones included.
+
+ Read from ``dashboard_slices`` rather than ``Dashboard.slices``, whose load
+ hides archived members.
+ """
+ if dashboard_id is None:
+ return set()
+ return set(
+ db.session.scalars(
+ select(dashboard_slices.c.slice_id).where(
+ dashboard_slices.c.dashboard_id == dashboard_id
+ )
+ )
+ )
+
+
+def _raise_for_unreadable_new_charts(
+ charts: list[Slice], allowed_chart_ids: set[int]
+) -> None:
+ """Refuse a layout that links a chart the caller cannot read.
+
+ A chart already allowed (an existing member, or a copy's source member)
+ is accepted as is, so a save never fails over a chart it does not add.
+ Dashboard membership feeds the datasource grant for dashboard viewers, so
+ a layout must not be a way to attach a chart the caller has no access to.
+ """
+ candidates: list[Slice] = [
+ chart for chart in charts if chart.id not in allowed_chart_ids
+ ]
+ if candidates and not security_manager.is_admin():
+ # The access check reads each chart's editors and viewers; load them
+ # in two queries rather than two lazy loads per chart.
+ db.session.query(Slice).options(
+ lazyload("*"),
+ selectinload(Slice.editors),
+ selectinload(Slice.viewers),
+ ).filter(Slice.id.in_([chart.id for chart in candidates])).all()
+ denied: list[int] = sorted(
+ chart.id for chart in candidates if not
security_manager.can_access_chart(chart)
+ )
+ if denied:
+ raise DashboardLayoutInvalidError(
+ exceptions=[
Review Comment:
This adds a new 422 on PUT, POST, and copy: a dashboard editor who saves a
layout containing a tile for a chart that was detached from the dashboard, and
that they cannot read, now gets `The layout references charts that are not
available`, where the same save used to reattach it. already documents the
copy-layout 422 under "Dashboard copy layout validation"; should this behavior
change get an entry there too, covering the affected endpoints, the
existing-member exemption, and the recovery step (remove the tile or get access
to the chart)?
##########
superset/daos/dashboard.py:
##########
@@ -305,6 +305,59 @@ def _existing_chart_ids(chart_ids: set[int]) -> set[int]:
return {row[0] for row in rows}
+def _member_chart_ids(dashboard_id: int | None) -> set[int]:
+ """IDs of *dashboard_id*'s member charts, archived ones included.
+
+ Read from ``dashboard_slices`` rather than ``Dashboard.slices``, whose load
+ hides archived members.
+ """
+ if dashboard_id is None:
+ return set()
+ return set(
+ db.session.scalars(
+ select(dashboard_slices.c.slice_id).where(
+ dashboard_slices.c.dashboard_id == dashboard_id
+ )
+ )
+ )
+
+
+def _raise_for_unreadable_new_charts(
+ charts: list[Slice], allowed_chart_ids: set[int]
+) -> None:
+ """Refuse a layout that links a chart the caller cannot read.
+
+ A chart already allowed (an existing member, or a copy's source member)
+ is accepted as is, so a save never fails over a chart it does not add.
+ Dashboard membership feeds the datasource grant for dashboard viewers, so
+ a layout must not be a way to attach a chart the caller has no access to.
+ """
+ candidates: list[Slice] = [
+ chart for chart in charts if chart.id not in allowed_chart_ids
+ ]
+ if candidates and not security_manager.is_admin():
+ # The access check reads each chart's editors and viewers; load them
+ # in two queries rather than two lazy loads per chart.
+ db.session.query(Slice).options(
+ lazyload("*"),
+ selectinload(Slice.editors),
+ selectinload(Slice.viewers),
+ ).filter(Slice.id.in_([chart.id for chart in candidates])).all()
+ denied: list[int] = sorted(
+ chart.id for chart in candidates if not
security_manager.can_access_chart(chart)
+ )
+ if denied:
+ raise DashboardLayoutInvalidError(
+ exceptions=[
Review Comment:
This adds a new 422 on PUT, POST, and copy: a dashboard editor who saves a
layout containing a tile for a chart that was detached from the dashboard, and
that they cannot read, now gets `The layout references charts that are not
available`, where the same save used to reattach it. `UPDATING.md` already
documents the copy-layout 422 under "Dashboard copy layout validation"; should
this behavior change get an entry there too, covering the affected endpoints,
the existing-member exemption, and the recovery step (remove the tile or get
access to the chart)?
--
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]
