sadpandajoe commented on code in PR #45538:
URL: https://github.com/apache/superset/pull/45538#discussion_r4340075316
##########
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:
Both successful new-chart cases (`own` here and in the copy case below) are
charts Gamma edits, so `can_access_chart` short-circuits on editorship. An
implementation that only accepted owned charts would pass every case while
rejecting charts Gamma can read through a granted dataset. Should there be a
case with a chart Gamma doesn't edit but whose dataset Gamma has been granted,
asserting 200 and that the chart is persisted as a member?
##########
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:
This rejection now fires after `UpdateDashboardCommand.run()` has already
run `process_tab_diff()` / `process_native_filter_diff()`, which deactivate the
affected reports and synchronously send the "deactivated" emails. If a save
removes a native filter or tab used by an active report and also adds an
unreadable chart, the caller gets a 422 and the DB rolls back (report stays
active), but report owners have already been emailed that it was deactivated.
Should the chart check run before those side effects, or should the emails be
deferred until the save commits?
##########
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:
Agreed—`can_access_chart` reads `Slice.editors` / `Slice.viewers` lazily and
the `Slice` query here doesn't preload them, so a non-admin save adding N
readable charts the caller doesn't own issues roughly 2N extra SELECTs
(existing members short-circuit, so unchanged layouts are unaffected).
`prefetch_chart_access()` already solves this for dashboard reads; should the
new-chart candidates use 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]
