aminghadersohi commented on code in PR #43781:
URL: https://github.com/apache/superset/pull/43781#discussion_r3928617447
##########
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:
**Semantic-layer grants are dropped by this predicate.**
`can_access_datasource(resolved)` routes to
`SupersetSecurityManager.raise_for_access(datasource=...)`, which authorizes
through `can_access_schema(...)`, `can_access("datasource_access",
datasource.perm or "")` and `is_editor(...)`. For a `SemanticView`,
`can_access_schema` short-circuits to `False` (it is not a `BaseDatasource`),
so this reduces to **the view's own `perm` only**.
Everywhere else, a semantic view is accessible via *either* its own perm
*or* its parent layer's:
- `SemanticView.raise_for_access()` —
`superset/semantic_layers/models.py:576-581`
- `SemanticLayerDAO.find_accessible()` — `or_(SemanticView.perm.in_(perms),
SemanticLayer.perm.in_(perms))`, `superset/daos/semantic_layer.py:228-232`
- `DatasourceDAO.build_semantic_view_query()` — same `or_`,
`superset/daos/datasource.py:168-170`
Both are real `datasource_access` PVMs — `semantic_layer_after_insert`
(`superset/security/manager.py:3853-3854`) and `semantic_view_after_insert`
(`:3974-3975`) each create one — so a role can hold the layer grant alone.
Reproduced at this head with layer `finance` (`perm = [finance](id:<hex>)`)
and view `revenue` (`perm = [finance].[revenue](id:101)`), granting **only**
the layer perm:
| check | result |
| --- | --- |
| `view.raise_for_access()` | allowed |
| `raise_for_access(dashboard=...)` | **denied** |
| `raise_for_access(chart=...)` | **denied** |
For the dashboard gate that is a regression: pre-PR the same user was
admitted by the `not dashboard.datasources` fail-open. For the chart gate it is
an incomplete fix rather than a regression (semantic-view charts were already
denied there).
A blanket "call the datasource's own `raise_for_access()`" is not safe —
`Query.raise_for_access()` (`superset/models/sql_lab.py:311`) re-parses
`executed_sql` and enforces per-table dataset matching, which is both heavier
and stricter than what this gate wants. Targeting the one type whose
entitlement rule differs keeps it minimal:
```python
def _datasource_accessible(self, datasource: Any) -> bool:
from superset.semantic_layers.models import SemanticView
if isinstance(datasource, SemanticView):
try:
datasource.raise_for_access()
return True
except SupersetSecurityException:
return False
return self.can_access_datasource(datasource)
```
Both call sites need it — here and the chart branch below
(`chart.resolved_datasource`). Worth a test with the view's own perm withheld
and only the layer perm granted; the current fixtures give every view its own
perm and leave the layer's unset, so this path is not exercised.
##########
UPDATING.md:
##########
@@ -39,6 +39,7 @@ payload. Clients must display the new impact and obtain
renewed confirmation
before retrying. Preview or recheck failures fail closed rather than treating
unknown impact as zero. Chart and dashboard purge endpoints are unchanged.
+- The dashboard datasource-based visibility fallback now fails closed: a
dashboard whose member charts’ datasources cannot be resolved (deleted
datasource rows, missing `datasource_id`, or unsupported datasource types) is
no longer accessible to users without explicit editor/viewer rights, and a
dashboard composed of semantic-view charts now requires `datasource_access` on
(at least one of) its semantic views — previously any authenticated user could
open such a dashboard’s shell. Dashboards with no charts remain accessible, and
dashboards with explicit viewers are unaffected. Conversely, holders of
`all_datasource_access` now see every published no-viewer dashboard in the
dashboard list — including chart-less ones previously hidden by the inner joins
— matching what the object-level gate already allowed them to open.
Review Comment:
This documents the restrictive direction and the `all_datasource_access`
list widening, but not the object-gate widening from round 1: on a **mixed**
dashboard the fallback is now an `any()` over *all* member charts rather than
over table-backed ones only.
Verified at this head — dashboard with one table chart (`perm =
[d90].[t91](id:91)`) the user cannot access plus one semantic-view chart (`perm
= [L2].[v90](id:90)`) they can:
- pre-PR: `dashboard.datasources == {t91}` → `any(...)` is `False` → **403**
- at head: the semantic view resolves and matches → **granted**
So a user holding only semantic-view grants gains access to mixed dashboards
that previously denied them. That follows from applying the pre-existing
`any()` semantics consistently and looks intended, but it is user-visible and
belongs in this note. One clause after the semantic-view sentence, e.g.:
> Because the fallback now considers every member chart rather than only
table-backed ones, a user holding `datasource_access` on any single member
datasource — including a semantic view — can open a mixed dashboard that
previously denied them.
##########
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:
This claim holds for a view-level grant but not a layer-level one.
`set_related_perm` copies `ds.perm` onto `Slice.perm`, and for a semantic-view
chart that is the **view's** perm — `SemanticView.catalog_perm` and
`schema_perm` are both hardcoded `None`
(`superset/semantic_layers/models.py:515-520`), so the other two denormalized
columns never match either.
A user holding only the parent layer's `datasource_access` therefore never
matches branch C, and the dashboard stays out of their list. That is
pre-existing here, but it becomes visible if the object gate is corrected as
described on `superset/security/manager.py` — the gate would admit them while
the list still hides the dashboard.
Bringing this branch into line needs the layer perm in the disjunction too,
e.g. an outer join to `SemanticView`/`SemanticLayer` on `Slice.datasource_type
== 'semantic_view'` with `SemanticLayer.perm.in_(perms)` added — mirroring
`DatasourceDAO.build_semantic_view_query`. Fine to defer to a follow-up, but
the two surfaces should not drift in opposite directions.
--
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]
