mikebridge commented on code in PR #45538:
URL: https://github.com/apache/superset/pull/45538#discussion_r4344296169
##########
superset/daos/dashboard.py:
##########
@@ -660,6 +713,10 @@ def set_dash_metadata( # noqa: C901
current_slices = (
db.session.query(Slice).filter(Slice.id.in_(slice_ids)).all()
)
+ _raise_for_unreadable_new_charts(
Review Comment:
Good catch, thanks. The chart check now runs in the update command's
`validate()`, before the tab and native-filter diffs, so a rejected save never
deactivates reports or emails anyone. The check in `set_dash_metadata` stays as
a backstop for copy and the other callers.
There's a new test where a PUT drops a tab and a native filter used by an
active report and also adds an unreadable chart: it gets a 422, no email goes
out, the report stays active, and the layout, metadata and chart list are
unchanged.
##########
tests/integration_tests/dashboards/api_tests.py:
##########
@@ -5441,6 +5441,140 @@ def test_copy_dashboard_layout_without_current_charts(
db.session.delete(chart)
db.session.commit()
+ @contextmanager
+ def _layout_access_fixture(self) -> Iterator[dict[str, int]]:
+ """A dashboard Gamma edits, whose member chart Gamma cannot read,
+ plus a chart Gamma owns, an unrelated chart Gamma cannot read, and
+ an archived member."""
+ from superset.connectors.sqla.models import SqlaTable
+ from superset.models.helpers import skip_visibility_filter
+
+ admin: User = self.get_user(ADMIN_USERNAME)
+ gamma: User = self.get_user(GAMMA_USERNAME)
+ dataset_id: int = (
+ db.session.query(SqlaTable.id).order_by(SqlaTable.id).first()[0]
+ )
+ member: Slice = self.insert_chart(
+ "layout-access-member", [admin.id], dataset_id
+ )
+ archived: Slice = self.insert_chart(
+ "layout-access-archived", [admin.id], dataset_id
+ )
+ own: Slice = self.insert_chart("layout-access-own", [gamma.id],
dataset_id)
+ foreign: Slice = self.insert_chart(
+ "layout-access-foreign", [admin.id], dataset_id
+ )
+ dashboard: Dashboard = self.insert_dashboard(
+ "layout-access", None, [gamma.id], slices=[member, archived]
+ )
+ ids: dict[str, int] = {
+ "dashboard": dashboard.id,
+ "member": member.id,
+ "archived": archived.id,
+ "own": own.id,
+ "foreign": foreign.id,
+ }
+ archived.deleted_at = datetime.now()
+ db.session.commit()
+ try:
+ yield ids
+ finally:
+ db.session.rollback()
+ with skip_visibility_filter(db.session, Slice):
+ for dash in db.session.query(Dashboard).filter(
+ Dashboard.dashboard_title.like("layout-access%")
+ ):
+ db.session.delete(dash)
+ for chart in db.session.query(Slice).filter(
+ Slice.slice_name.like("layout-access-%")
+ ):
+ db.session.delete(chart)
+ db.session.commit()
+
+ @staticmethod
+ def _chart_layout(*chart_ids: int) -> dict[str, Any]:
+ return {
+ f"CHART-{chart_id}": {
+ "id": f"CHART-{chart_id}",
+ "type": "CHART",
+ "children": [],
+ "meta": {"chartId": chart_id, "width": 4, "height": 50},
+ }
+ for chart_id in chart_ids
+ }
+
+ @parameterized.expand(
+ [
+ ("existing_and_readable", ("member", "archived", "own"), 200),
Review Comment:
Agreed, both new-chart cases short-circuited on editorship. I added a chart
Gamma doesn't edit, on a dataset granted to Gamma, to the update and copy
tests. Both assert a 200 and that the chart is saved as a member. I also
checked that an editor-only check fails exactly those two cases.
##########
superset/daos/dashboard.py:
##########
@@ -660,6 +713,10 @@ def set_dash_metadata( # noqa: C901
current_slices = (
db.session.query(Slice).filter(Slice.id.in_(slice_ids)).all()
)
+ _raise_for_unreadable_new_charts(
+ current_slices,
+ _member_chart_ids(dashboard.id) | (allowed_chart_ids or
set()),
+ )
Review Comment:
Applied. `prefetch_chart_access()` loads the dashboard's existing members,
and those are the charts this check skips, so I used the same pattern scoped to
the new charts instead: one `selectinload` for editors and viewers before the
access check, skipped for admins.
--
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]
