aminghadersohi commented on code in PR #43781:
URL: https://github.com/apache/superset/pull/43781#discussion_r3931059119
##########
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:
The new `SemanticLayer.perm` join is the robust path — but the
**view-level** grant still matches through the denormalized `Slice.perm`, and
unlike the dataset path, semantic-view renames never refresh it.
`semantic_view_before_update` (`superset/security/manager.py:4030-4054`)
renames the `ViewMenu` and writes the new perm back to `SemanticView.perm`, but
it has no counterpart to the `chart_table.update()` the dataset path performs
at `superset/security/manager.py:3846-3854`. So `Slice.perm` keeps the
pre-rename string.
Executed at this head, renaming a semantic view and a dataset in the same
transaction as an A/B control:
```
AFTER RENAME
SEMANTIC: view.perm = '[ops].[newname](id:401)' Slice.perm =
'[ops].[oldname](id:401)' *** STALE ***
TABLE : tbl.perm = '[dbr].[newtable](id:402)' Slice.perm =
'[dbr].[newtable](id:402)' IN SYNC
holder of the NEW (live) perms
semantic: gate=True list=[] <-- dashboard disappears from the
list
table : gate=True list=['tbl dash']
layer : gate=True list=['sem dash'] <-- layer grant unaffected,
matches live SemanticLayer.perm
```
So after a rename, a user holding the view's own `datasource_access` is
admitted by the object gate but the dashboard vanishes from their list. It
fails closed, so it is not a security issue.
The root cause is pre-existing and off-diff, but this diff is what makes it
reachable: before the type-aware join, a semantic-view chart matched only by
colliding on `datasource_id` with some `SqlaTable`, so `Slice.perm` was never
the load-bearing predicate for these rows. It is now. The last row above is the
tell — the clause you added this round is the only one that survives a rename.
Cheapest fix is the missing propagation in `semantic_view_before_update`,
mirroring the dataset path:
```python
connection.execute(
chart_table.update()
.where(
chart_table.c.datasource_type == DatasourceType.SEMANTIC_VIEW,
chart_table.c.datasource_id == target.id,
)
.values(perm=new_perm)
)
```
Reasonable to split into a follow-up given it lives outside this PR's files
— but the PR's own list tests would pass either way, so it will not be caught
here.
##########
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:
**This `getattr` duck-typing is what turns CI red.**
`test-sqlite` and `test-postgres (current)` both fail on
`tests/integration_tests/security_tests.py::TestSecurityManager::test_all_database_access`:
```
sqlalchemy.exc.ProgrammingError: (sqlite3.ProgrammingError)
Error binding parameter 1: type 'MagicMock' is not supported
[parameters: (<MagicMock name='mock.semantic_layer.perm' id=...>,
'datasource_access', 5)]
```
That test passes `SupersetTestCase.get_datasource_mock()`
(`tests/integration_tests/base_tests.py:380`), a `MagicMock` with `__class__`
reassigned to `SqlaTable`. Reassigning `__class__` does not stop
`MagicMock.__getattr__` from auto-creating attributes, so:
- `getattr(datasource, "semantic_layer", None)` → a fresh child mock, not
`None`
- `getattr(layer, "perm", None)` → another child mock
- `if not layer_perm:` → **False**, because a `MagicMock` is truthy
- `can_access("datasource_access", <MagicMock>)` → the mock reaches the
driver as a bind parameter
Reproduced locally against this head:
```
getattr(ds,'semantic_layer') : <MagicMock name='mock.semantic_layer' ...>
getattr(layer,'perm') : <MagicMock name='mock.semantic_layer.perm'
...>
bool(layer_perm) -> truthy? : True
can_access called with : [('datasource_access', 'MagicMock')]
isinstance(ds, SemanticView) : False
```
An `isinstance` guard fixes the test *and* removes the duck-typing, which is
the more durable half: today only `SemanticView` declares a `semantic_layer`
attribute (`superset/semantic_layers/models.py:254`), so the current code is
correct in production — but it is correct by coincidence of naming, and any
future model that grows a `semantic_layer` attribute with a truthy `perm` would
silently start consulting a layer grant.
```suggestion
from superset.semantic_layers.models import ( # pylint:
disable=import-outside-toplevel
SemanticView,
)
if not isinstance(datasource, SemanticView):
return False
layer_perm: str | None = getattr(datasource.semantic_layer, "perm",
None)
if not layer_perm:
return False
return self.can_access("datasource_access", layer_perm)
```
The `getattr` on `.semantic_layer` is still worth keeping for the `perm`
lookup since the relationship is nullable in practice. Worth a unit test
pinning that a non-`SemanticView` datasource never reaches `can_access` — that
is the assertion the integration failure is making the hard way.
--
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]
