aminghadersohi commented on code in PR #45815:
URL: https://github.com/apache/superset/pull/45815#discussion_r4403891166
##########
superset/models/helpers.py:
##########
@@ -4529,20 +4529,27 @@ def get_from_clause(
# For each table referenced in the virtual dataset, apply its RLS
filters
if parsed_script.statements:
default_schema = self._get_rls_schema()
+ # Only a dataset applies its own RLS and the global guest rules on
+ # the outer WHERE, so only a dataset leaves them out of its inner
+ # SQL. Other subclasses (e.g. a SQL Lab Query) apply no RLS
outside,
+ # and their ``id`` is not a dataset id: excluding it would drop the
+ # RLS of whichever dataset shares that number. Imported locally to
+ # avoid a circular import with the connectors package.
+ from superset.connectors.sqla.models import SqlaTable
+
+ dataset = self if isinstance(self, SqlaTable) else None
+ is_dataset = dataset is not None
+ self_id = dataset.id if dataset is not None else None
try:
rls_applied = False
- # ``id`` lives on concrete subclasses (e.g. SqlaTable), not on
- # ExploreMixin itself. getattr keeps this safe for non-dataset
- # subclasses (e.g. SQL Lab Query), which have no RLS to dedupe.
- self_id = getattr(self, "id", None)
for statement in parsed_script.statements:
if apply_rls(
self.database,
self.catalog,
default_schema,
statement,
exclude_dataset_id=self_id,
- include_global_guest_rls=False,
+ include_global_guest_rls=not is_dataset,
Review Comment:
I chose to mirror execution: fixed in
b512bf1800708ddfc99375a930d2bec29665cce7 by passing
`include_global_guest_rls=True` for Query. A Query has no outer RLS
application, and `get_from_clause` includes global guest rules in its inner
SQL. You are right that the overall chart key already includes the unscoped
guest clause through `get_rls_cache_key`; this change aligns the table-tagged
extra keys, rather than claiming a chart-cache isolation gap.
`tests/unit_tests/models/query_datasource_rls_test.py::test_query_datasource_cache_key_has_global_guest_rls`
is added by this PR. With the old False flag it fails (`[]` instead of the
table-tagged guest predicate), while the execution assertion succeeds. With
True it passes. Targeted pytest across that file plus virtual-dataset
formatting, double-RLS, RLS utilities and guest-RLS tests: 56 passed.
Pre-commit on branch-changed files: MyPy and other applicable hooks passed
except Ruff S704 at the unchanged `superset/models/sql_lab.py:615`; the same
failure reproduces on the pre-change HEAD.
Search: `grep -rn --include="*.py" "include_global_guest_rls" superset`
(every source hit below; binary bytecode hits are not datasource code).
Siblings checked:
- superset/connectors/sqla/models.py:1161 — not affected: parameter defaults
to True; callers control inner versus outer application
- superset/connectors/sqla/models.py:1168 — not affected: documents the
filtering parameter
- superset/connectors/sqla/models.py:1190 — not affected: excludes global
rules only when explicitly requested
- superset/connectors/sqla/models.py:3234 — not affected: SqlaTable leaves
globals to its outer WHERE, so False matches execution
- superset/models/sql_lab.py:423 — fixed in
b512bf1800708ddfc99375a930d2bec29665cce7
- superset/models/helpers.py:2210 — not affected: Query mixin stub returns
no outer RLS
- superset/models/helpers.py:4491 — not affected: independent expression
subqueries already include globals
- superset/models/helpers.py:4552 — not affected: execution already includes
globals for Query and excludes them only for SqlaTable
- superset/models/helpers.py:4587 — not affected: fail-closed recount
includes globals for Query and for dataset subqueries
- superset/utils/rls.py:86 — not affected: shared helper defaults to
including global guest rules
- superset/utils/rls.py:98 — not affected: documents the shared
execution/cache flag contract
- superset/utils/rls.py:119 — not affected: shared RLS helper forwards the
caller-selected flag consistently for execution and cache collection
- superset/utils/rls.py:148 — not affected: shared helper defaults to
including global guest rules
- superset/utils/rls.py:173 — not affected: shared RLS helper forwards the
caller-selected flag consistently for execution and cache collection
- superset/utils/rls.py:177 — not affected: shared RLS helper forwards the
caller-selected flag consistently for execution and cache collection
- superset/utils/rls.py:185 — not affected: subquery handling restores
globals where an outer dataset filter cannot constrain reads
- superset/utils/rls.py:297 — not affected: shared helper defaults to
including global guest rules
- superset/utils/rls.py:306 — not affected: documents the shared
execution/cache flag contract
- superset/utils/rls.py:344 — not affected: shared RLS helper forwards the
caller-selected flag consistently for execution and cache collection
- superset/utils/rls.py:356 — not affected: shared helper defaults to
including global guest rules
- superset/utils/rls.py:374 — not affected: documents the shared
execution/cache flag contract
- superset/utils/rls.py:396 — not affected: shared RLS helper forwards the
caller-selected flag consistently for execution and cache collection
--
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]
