mikebridge commented on code in PR #43781:
URL: https://github.com/apache/superset/pull/43781#discussion_r3935839586
##########
superset/security/manager.py:
##########
@@ -4786,11 +4786,36 @@ def has_promiscuous_chart_access() -> bool:
if dashboard.viewers:
if dashboard.published and self.is_viewer(dashboard):
return
- elif not dashboard.datasources or any(
- self.can_access_datasource(datasource)
- for datasource in dashboard.datasources
- ):
- return
+ else:
+ # Datasource-based fallback. Member chart datasources are
+ # resolved across datasource types via
+ # ``Slice.resolved_datasource`` — ``Dashboard.datasources``
+ # only ever contains SqlaTable-backed datasources, so an
+ # unqualified emptiness check would grant every authenticated
+ # user access to a dashboard composed solely of, e.g.,
+ # semantic-view charts. A dashboard with no charts remains
+ # accessible; a chart whose datasource cannot be resolved
+ # counts as inaccessible, never as absent. Resolution is
+ # lazy and deduplicated per (type, id) so the first
+ # accessible datasource short-circuits the remaining lookups.
+ member_slices = dashboard.slices
+
+ def member_datasource_accessible() -> bool:
+ seen: set[tuple[str | None, int | None]] = set()
+ for slc in member_slices:
+ key = (slc.datasource_type, slc.datasource_id)
+ if key in seen:
+ continue
+ seen.add(key)
+ resolved = slc.resolved_datasource
+ if resolved is not None and self.can_access_datasource(
Review Comment:
Fixed by the sc-119501 fold-in (`faea7d9866` + polish `27a1958d14`): the
generic datasource branch now carries `_semantic_layer_grant_allows`, mirroring
`SemanticView.raise_for_access`'s layer-perm fallback, so
`can_access_datasource(resolved)` honors layer-level grants for both the
dashboard gate and the standalone chart gate — and `DashboardAccessFilter`
gained the matching `SemanticLayer.perm` clause. Pinned by the three ALLOW
tests you asked for (`test_gate_allows_semantic_dashboard_for_layer_grant`,
`test_gate_allows_semantic_chart_for_layer_grant`,
`test_list_filter_layer_grant_lists_all_layer_dashboards`) plus wrong-layer and
table-inertness deny pins. Your narrower point about a blanket
`raise_for_access()` being unsafe for `Query` shaped the design — the fallback
consults only the layer perm, nothing type-specific beyond `SemanticView`.
Resolving.
##########
superset/security/manager.py:
##########
@@ -2218,6 +2218,24 @@ def can_access_schema(self, datasource: "BaseDatasource
| Explorable") -> bool:
# Non-SQL explorables don't have schema hierarchy
return False
+ def _semantic_layer_grant_allows(
+ self, datasource: "BaseDatasource | Explorable"
+ ) -> bool:
+ """True when a grant on a semantic view's parent layer covers it.
+
+ A ``datasource_access`` grant on a semantic layer covers every view
+ under it — the data path enforces this in
+ ``SemanticView.raise_for_access``; object authorization mirrors the
+ same fallback (sc-119501). Datasources without a parent layer resolve
+ to no perm and return False without a permission lookup, matching the
+ data path's ``if layer_perm and …`` guard.
+ """
+ layer = getattr(datasource, "semantic_layer", None)
+ layer_perm: str | None = getattr(layer, "perm", None)
+ if not layer_perm:
+ return False
+ return self.can_access("datasource_access", layer_perm)
Review Comment:
Fixed in `e0dd7eb200`, taking your suggestion nearly verbatim:
`_semantic_layer_grant_allows` isinstance-checks for `SemanticView` before any
lookup (the `getattr` survives only for the nullable relationship's `perm`),
and the unit test you asked for pins that a `MagicMock` with `__class__ =
SqlaTable` returns False with `can_access` never called
(`test_layer_fallback_never_consults_grants_for_non_semantic_datasource`). Both
lanes green on this head: test-sqlite and test-postgres (current) pass.
Resolving.
##########
superset/dashboards/filters.py:
##########
@@ -167,12 +167,30 @@ def _apply_viewers(self, query: Query) -> Query:
filters.append(Dashboard.id.in_(viewer_query))
# (C) No-viewer fallback: dashboards with no viewers → dataset-based
access
+ # Note: for ordinary users a dashboard with no charts is never yielded
+ # here (every access predicate is NULL-false after the outer joins)
+ # even though the object gate allows opening it — a deliberate,
+ # pre-existing asymmetry. For ``all_datasource_access`` holders the
+ # spliced literal True below yields such rows, matching the gate.
dashboard_has_viewers = Dashboard.viewers.any()
no_viewer_query = (
db.session.query(Dashboard.id)
.join(Dashboard.slices, isouter=True)
- .join(SqlaTable, Slice.datasource_id == SqlaTable.id)
- .join(Database, SqlaTable.database_id == Database.id)
+ # Type-aware datasource joins: the SqlaTable join is constrained
+ # to table-backed charts (an unconstrained id join can bind a
+ # semantic-view chart to an unrelated table sharing its numeric
+ # id) and kept outer so charts on other datasource types survive
+ # into the access filter — their access matches through the perm
+ # columns denormalized onto Slice by ``set_related_perm``.
Review Comment:
Brought into line in the same round rather than deferred: branch C now
carries the type-guarded outer join to `SemanticView`→`SemanticLayer` with
`SemanticLayer.perm` in the disjunction (since polished into the shared
`semantic_view_slice_join` / `semantic_view_layer_join` /
`semantic_layer_grant_clause` helpers), so a layer-level grant surfaces the
dashboards in the list too —
`test_list_filter_layer_grant_lists_all_layer_dashboards` asserts both
dashboards of the shared test layer appear. The gate and the list no longer
drift in opposite directions; the chart list gets the identical clause in the
stacked follow-up #43848. Resolving.
##########
superset/dashboards/filters.py:
##########
@@ -167,19 +168,62 @@ def _apply_viewers(self, query: Query) -> Query:
filters.append(Dashboard.id.in_(viewer_query))
# (C) No-viewer fallback: dashboards with no viewers → dataset-based
access
+ # A datasource_access grant on a parent semantic layer covers its
+ # views (sc-119501; the double perm fetch alongside
+ # get_dataset_access_filters is accepted until sc-119500 reworks the
+ # helper's signature for the chart-list mirror of this clause).
+ layer_grant_clause = SemanticLayer.perm.in_(
+ security_manager.user_view_menu_names("datasource_access")
+ )
+ # Note: for ordinary users a dashboard with no charts is never yielded
+ # here (every access predicate is NULL-false after the outer joins)
+ # even though the object gate allows opening it — a deliberate,
+ # pre-existing asymmetry. For ``all_datasource_access`` holders the
+ # spliced literal True below yields such rows, matching the gate.
dashboard_has_viewers = Dashboard.viewers.any()
no_viewer_query = (
db.session.query(Dashboard.id)
.join(Dashboard.slices, isouter=True)
- .join(SqlaTable, Slice.datasource_id == SqlaTable.id)
- .join(Database, SqlaTable.database_id == Database.id)
+ # Type-aware datasource joins: the SqlaTable join is constrained
+ # to table-backed charts (an unconstrained id join can bind a
+ # semantic-view chart to an unrelated table sharing its numeric
+ # id) and kept outer so charts on other datasource types survive
+ # into the access filter — their access matches through the perm
+ # columns denormalized onto Slice by ``set_related_perm``.
+ .join(
+ SqlaTable,
+ and_(
+ Slice.datasource_id == SqlaTable.id,
+ Slice.datasource_type == DatasourceType.TABLE,
+ ),
+ isouter=True,
+ )
+ .join(Database, SqlaTable.database_id == Database.id, isouter=True)
+ # A datasource_access grant on a semantic LAYER covers its views,
+ # as SemanticView.raise_for_access enforces on the data path
+ # (sc-119501) — surface those dashboards here too, through the
+ # same type-guarded outer-join shape as the SqlaTable join.
+ .join(
+ SemanticView,
+ and_(
+ Slice.datasource_id == SemanticView.id,
+ Slice.datasource_type == DatasourceType.SEMANTIC_VIEW,
+ ),
+ isouter=True,
+ )
+ .join(
+ SemanticLayer,
+ SemanticView.semantic_layer_uuid == SemanticLayer.uuid,
Review Comment:
Agreed on the mechanism and the tell — the layer clause is the only
predicate that survives a rename. This is deliberately out of scope here and
tracked as SC-119502, where your root cause (`semantic_view_before_update`
lacking the dataset path's `chart_table.update()` propagation), the suggested
fix, and the A/B rename evidence are now recorded so the implementer starts
from your analysis. It fails closed (gate admits, list hides), so nothing leaks
in the interim. Resolving as deferred-to-SC-119502.
##########
superset/dashboards/filters.py:
##########
@@ -167,19 +168,57 @@ def _apply_viewers(self, query: Query) -> Query:
filters.append(Dashboard.id.in_(viewer_query))
# (C) No-viewer fallback: dashboards with no viewers → dataset-based
access
+ # Note: for ordinary users a dashboard with no charts is never yielded
+ # here (every access predicate is NULL-false after the outer joins)
+ # even though the object gate allows opening it — a deliberate,
+ # pre-existing asymmetry. For ``all_datasource_access`` holders the
+ # spliced literal True below yields such rows, matching the gate.
dashboard_has_viewers = Dashboard.viewers.any()
no_viewer_query = (
db.session.query(Dashboard.id)
.join(Dashboard.slices, isouter=True)
- .join(SqlaTable, Slice.datasource_id == SqlaTable.id)
- .join(Database, SqlaTable.database_id == Database.id)
+ # Type-aware datasource joins: the SqlaTable join is constrained
+ # to table-backed charts (an unconstrained id join can bind a
+ # semantic-view chart to an unrelated table sharing its numeric
+ # id) and kept outer so charts on other datasource types survive
+ # into the access filter — their access matches through the perm
+ # columns denormalized onto Slice by ``set_related_perm``.
+ .join(
+ SqlaTable,
+ and_(
+ Slice.datasource_id == SqlaTable.id,
+ Slice.datasource_type == DatasourceType.TABLE,
+ ),
+ isouter=True,
+ )
+ .join(Database, SqlaTable.database_id == Database.id, isouter=True)
+ # A datasource_access grant on a semantic LAYER covers its views,
+ # as SemanticView.raise_for_access enforces on the data path
+ # (sc-119501) — surface those dashboards here too, through the
+ # same type-guarded outer-join shape as the SqlaTable join.
+ .join(
+ SemanticView,
+ and_(
+ Slice.datasource_id == SemanticView.id,
+ Slice.datasource_type == DatasourceType.SEMANTIC_VIEW,
+ ),
+ isouter=True,
+ )
+ .join(
+ SemanticLayer,
+ SemanticView.semantic_layer_uuid == SemanticLayer.uuid,
+ isouter=True,
+ )
.filter(
and_(
Dashboard.published.is_(True),
~dashboard_has_viewers,
get_dataset_access_filters(
Slice,
security_manager.can_access_all_datasources(),
+ SemanticLayer.perm.in_(
+
security_manager.user_view_menu_names("datasource_access")
+ ),
),
Review Comment:
Correct — view-level grants match through the denormalized `Slice.perm`,
which semantic-view renames/regenerations don't refresh. That's the known
follow-up SC-119502 (rename propagation in `semantic_view_before_update`,
mirroring the dataset path's `chart_table.update()`), deliberately out of scope
for this PR; the failure direction is closed (dashboards hide rather than
leak). Resolving as deferred-to-SC-119502.
--
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]
