mikebridge commented on code in PR #43848:
URL: https://github.com/apache/superset/pull/43848#discussion_r4020563957
##########
superset/charts/filters.py:
##########
@@ -150,16 +155,40 @@ def _apply_viewers(self, query: Query) -> Query:
filters.append(Slice.id.in_(viewer_query))
# (C) No-viewer fallback: charts with no viewers → dataset-based access
+ layer_grant_clause = semantic_layer_grant_clause()
chart_has_viewers = Slice.viewers.any()
table_alias = aliased(SqlaTable)
no_viewer_query = (
db.session.query(Slice.id)
- .join(table_alias, Slice.datasource_id == table_alias.id)
- .join(models.Database, table_alias.database_id ==
models.Database.id)
+ # Type-aware datasource joins (mirroring DashboardAccessFilter):
+ # 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``. A chart whose
+ # datasource row is hard-deleted can still match its stale
+ # denormalized perm here; the object gate stays authoritative
+ # and denies (accepted edge, shared with the dashboard filter).
+ .join(table_alias, table_backed_slice_join(table_alias),
isouter=True)
+ .join(
+ models.Database,
+ table_alias.database_id == models.Database.id,
+ isouter=True,
Review Comment:
Could we narrow this one? The stale-`Slice.perm` mechanism as described
doesn't survive the lifecycle: ORM hard deletion fires `dataset_after_delete`,
which drops the dataset's `datasource_access` PVM per row, so there's no
residual perm string left to match.
What does survive deletion is `schema_perm` / `catalog_perm`, which is a
real but narrower residual. We'd rather pin that with a lifecycle test — delete
for real, fresh permission lookup, assert a user whose only data grant was the
deleted dataset doesn't list the retained chart — than restructure the joins
against the mechanism described here.
Flagging rather than resolving outright, in case we've misread the deletion
path.
--
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]
