gabotorresruiz commented on code in PR #45815:
URL: https://github.com/apache/superset/pull/45815#discussion_r4403750249
##########
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:
Not a blocker. `Query.get_extra_cache_keys` still passes
`include_global_guest_rls=False` with the comment "As `get_from_clause` applies
them to the inner SQL" (`superset/models/sql_lab.py`), and this line makes that
the opposite of true for a `Query`. The chart key does still partition:
`query_cache_key` adds `security_manager.get_rls_cache_key(datasource)`, and
`get_guest_rls_filters_str` returns the unscoped rules for any datasource, so a
guest's global clause lands there instead of in a table-tagged component. Do we
want to flip that flag to `True` so the key mirrors execution, or just update
the comment? I might be missing a reason to keep the key narrower than what
runs.
--
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]
