aminghadersohi commented on code in PR #45814:
URL: https://github.com/apache/superset/pull/45814#discussion_r4400997591
##########
superset/connectors/sqla/models.py:
##########
@@ -3239,6 +3227,35 @@ class and any keys added via `ExtraCache`.
return list(set(extra_cache_keys))
+ def get_inner_rls_cache_key(self) -> list[str]:
+ """
+ The table-bound RLS predicates execution injects into the tables read
+ by this dataset's SQL, as cache key components.
+
+ ``security_manager.get_rls_cache_key`` covers only the dataset's own
+ rules, so a cache of a virtual dataset's results must also key on
these,
+ or viewers whose RLS differs only on an inner table share entries.
Empty
+ for a physical dataset. If the predicates cannot be collected, a
+ viewer-scoped marker is returned instead.
+
+ :return: The RLS predicates, one cache key component each
+ """
+ from superset.utils.rls import collect_rls_predicates_for_sql
+
+ if not (self.is_virtual and self.sql):
+ return []
+ # The default schema is looked up only when the dataset has none: the
+ # lookup connects to the database, which serving a cached entry must
+ # not depend on.
+ return collect_rls_predicates_for_sql(
+ self.sql,
+ self.database,
+ self.catalog,
+ self.schema or self.database.get_default_schema(self.catalog) or
"",
Review Comment:
Fixed in 81c31623b150851b836e69a878888fc6aef189f2: default-schema resolution
runs inside the collector's guarded fallback for schema-less annotation/chart
keys.
Regression:
tests/unit_tests/security/inner_rls_cache_key_test.py::test_schema_lookup_outage_builds_guest_annotation_context,
added by this PR, fails at 1072cc722b2eced6c66f2ac6bbe337ca7db40ef6 with
get_default_schema raising ConnectionError and passes after this change. The
tests retain distinct user/guest scopes. Successful resolution still binds
predicates to their qualified tables; explicit schemas avoid this lookup.
A schema-resolution outage uses the existing viewer-scoped fallback
namespace. It can read an entry already in that namespace; it does not blindly
reuse a healthy predicate-keyed entry when the schema cannot be determined.
Siblings checked:
- superset/connectors/sqla/models.py:3247 — fixed in
81c31623b150851b836e69a878888fc6aef189f2
- superset/connectors/sqla/models.py:3205 — fixed in
81c31623b150851b836e69a878888fc6aef189f2
- superset/models/sql_lab.py:421 — fixed in
81c31623b150851b836e69a878888fc6aef189f2
- superset/models/sql_lab.py:408 — fixed in
81c31623b150851b836e69a878888fc6aef189f2
- superset/utils/rls.py:318 — fixed in
81c31623b150851b836e69a878888fc6aef189f2
- superset/datasource/api.py:110 — fixed in
81c31623b150851b836e69a878888fc6aef189f2
- superset/common/query_context_processor.py:519 — fixed in
81c31623b150851b836e69a878888fc6aef189f2
- superset/mcp_service/dashboard/tool/apply_dashboard_filters.py:214 — not
affected: delegates to the shared column-values cache; no separate schema
lookup.
- superset/utils/rls.py:222 — not affected: the conditional physical-table
schema lookup already runs inside the collector's guard.
- superset/models/helpers.py:4128 — not affected: query execution, not the
inner-key collector; execution still requires the database and enforces RLS.
The Jinja extra-cache-key execution branch is unchanged.
##########
superset/datasource/api.py:
##########
@@ -102,6 +103,13 @@ def _column_values_cache_key(
rls=security_manager.get_rls_cache_key(datasource),
changed_on=str(getattr(datasource, "changed_on", "")),
)
+ # Executing a virtual dataset (or a SQL Lab query) also injects the RLS of
+ # the tables its SQL reads, which ``get_rls_cache_key`` does not cover.
+ # Only added when non-empty, so keys without inner rules keep their bytes.
+ if isinstance(datasource, (SqlaTable, Query)) and (
+ inner_rls := datasource.get_inner_rls_cache_key()
+ ):
Review Comment:
Fixed in 81c31623b150851b836e69a878888fc6aef189f2: default-schema resolution
runs inside the collector's guarded fallback for schema-less virtual-dataset
and SQL Lab column-values keys.
Regression:
tests/unit_tests/security/inner_rls_cache_key_test.py::test_schema_lookup_outage_serves_viewer_scoped_column_values,
added by this PR, fails at 1072cc722b2eced6c66f2ac6bbe337ca7db40ef6 with
get_default_schema raising ConnectionError and passes after this change. The
tests retain distinct user/guest scopes. Successful resolution still binds
predicates to their qualified tables; explicit schemas avoid this lookup.
A schema-resolution outage uses the existing viewer-scoped fallback
namespace. It can read an entry already in that namespace; it does not blindly
reuse a healthy predicate-keyed entry when the schema cannot be determined.
Siblings checked:
- superset/connectors/sqla/models.py:3247 — fixed in
81c31623b150851b836e69a878888fc6aef189f2
- superset/connectors/sqla/models.py:3205 — fixed in
81c31623b150851b836e69a878888fc6aef189f2
- superset/models/sql_lab.py:421 — fixed in
81c31623b150851b836e69a878888fc6aef189f2
- superset/models/sql_lab.py:408 — fixed in
81c31623b150851b836e69a878888fc6aef189f2
- superset/utils/rls.py:318 — fixed in
81c31623b150851b836e69a878888fc6aef189f2
- superset/datasource/api.py:110 — fixed in
81c31623b150851b836e69a878888fc6aef189f2
- superset/common/query_context_processor.py:519 — fixed in
81c31623b150851b836e69a878888fc6aef189f2
- superset/mcp_service/dashboard/tool/apply_dashboard_filters.py:214 — not
affected: delegates to the shared column-values cache; no separate schema
lookup.
- superset/utils/rls.py:222 — not affected: the conditional physical-table
schema lookup already runs inside the collector's guard.
- superset/models/helpers.py:4128 — not affected: query execution, not the
inner-key collector; execution still requires the database and enforces RLS.
The Jinja extra-cache-key execution branch is unchanged.
--
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]
