mikebridge commented on code in PR #43848:
URL: https://github.com/apache/superset/pull/43848#discussion_r4020780820
##########
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).
Review Comment:
Agreed — narrowed in `8a5bd06f91`. The comment now names `schema_perm` /
`catalog_perm` explicitly and qualifies this as the table ORM-delete case,
rather than implying a stale `Slice.perm` can match after the PVM is dropped.
Added a lifecycle test covering it end to end: the dataset grant disappears
on delete, the schema/catalog list match remains, and the datasource-based
object gate denies. That pins the narrower residual you identified instead of
the mechanism originally described.
##########
superset/models/dashboard.py:
##########
@@ -272,6 +275,31 @@ def get_url(id_: int, slug: str | None = None) -> str:
def datasources(self) -> set[BaseDatasource]:
return {slc.datasource for slc in self.slices if slc.datasource}
+ def has_member_datasource(self, datasource: BaseDatasource | Explorable)
-> bool:
+ """Type-aware membership: does a member chart reference this
datasource?
+
+ A *member datasource* is the ``(datasource_type, datasource_id)``
+ pair a member chart references. Comparing the pair — never the bare
+ numeric id — makes the test immune to id collisions across
+ datasource types, and it needs no datasource resolution (zero
+ queries). ``datasources`` above stays table-shaped for its
+ export/thumbnail/dataset-payload consumers and must not be used for
+ membership checks: it silently omits every non-table datasource.
+ """
+ # ``type`` is duck-typed on purpose: drill entry points hand this
+ # method loosely-typed datasources, and an object with no type or no
+ # id is simply no member — fail closed. Ids are compared as-is: an
+ # explorable with a string id never matches the integer
+ # ``datasource_id`` column, which is likewise fail closed.
+ candidate_type = getattr(datasource, "type", None)
+ candidate_id = datasource.id
Review Comment:
Good catch — the docstring promised "no id is simply no member, fail closed"
while direct attribute access raised `AttributeError` instead.
Took your suggestion in `8a5bd06f91` with an explicit local annotation, and
added a missing-id regression that raised `AttributeError` before the change
and returns `False` now.
--
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]
