Repository navigation
fix(security): key column-values and annotation caches on RLS of tables inside virtual dataset SQL - #45814
Conversation
…ual dataset SQL The shared column-values cache keyed only on the outer dataset's RLS (security_manager.get_rls_cache_key). Executing a virtual dataset (or a SQL Lab query) also injects the RLS of every table its SQL reads, so two viewers with the same rules on the virtual dataset but different rules on an inner table shared one entry and one could be served the other's values. Move the inner-SQL predicate collection the chart cache key already uses into SqlaTable/Query.get_inner_rls_cache_key and include it in the column-values key. Collection failure falls back to the existing viewer-scoped marker. The field is only added when non-empty, so keys without inner rules keep their bytes.
…tasets and queries
… SQL Annotation data from a chart-backed layer is cached on the parent chart's entry, keyed by the requesting user id and the source datasource's own RLS. Embedded guests all have no user id, so two guests whose token rules differ only on a table read inside the source's virtual dataset SQL shared one entry and one could be served the other's annotation data. Add the source's inner-SQL RLS (get_inner_rls_cache_key, the collection the chart cache key uses) to the annotation context, failing closed to the viewer-scoped marker, and only when non-empty so other contexts keep their bytes. The source datasource lookup moves into its own helper. The column-values cache tests move to a module covering every inner-RLS cache key.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #45814 +/- ##
==========================================
- Coverage 84.62% 84.61% -0.01%
==========================================
Files 3069 3069
Lines 205465 205493 +28
Branches 48294 48301 +7
==========================================
+ Hits 173868 173886 +18
- Misses 28303 28313 +10
Partials 3294 3294
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
aminghadersohi
changed the title
…without one get_inner_rls_cache_key looked up the database's default schema before collecting the inner RLS predicates, even for datasets with a schema. The lookup opens a connection to the database, so every column-values request on a virtual dataset (or SQL Lab query) connected to it, cache hits included, and a cache hit failed with a 500 when the database was unreachable. Look it up only when the dataset has no schema.
|
Product acceptance run on a real stack (docker compose: Postgres metadata + warehouse, Redis data cache, embedded on, roles with RLS only on the physical tables read by the virtual datasets), comparing the merge base Result: all five cases pass on the branch and the leaks reproduce at the merge base:
One issue found and fixed in 1072cc7:
I also updated the description: binding predicates to their table changes the chart cache key for virtual datasets with inner rules (one-time recompute after upgrade), and it closes the swapped-rules collision in the chart-data cache too. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
gabotorresruiz
left a comment
gabotorresruiz
left a comment
There was a problem hiding this comment.
Hey @aminghadersohi, thanks for this one, the key itself checks out. Every shape I pushed through get_inner_rls_cache_key produced components matching what apply_rls actually injects, table for table: joins, CTEs, a CTE that is never selected from, nested and scalar subqueries, unions, and a CTE name that shadows a registered dataset. Unparseable SQL falls back to the viewer-scoped marker on both the column-values and the annotation path, and physical datasets plus virtual datasets with no inner rules keep byte-identical keys.
Approving, with three notes on the lines. None of them blocks: one is a gap in rls that predates this PR, and two are tests.
One housekeeping thing: the description's "How" section still describes include_table_identity and a rls-table:[...] component, both of which the merge with master replaced, so the mechanism a reviewer reads there no longer matches the code.
CI on 091ed90 is green: 57 successful checks, no failures and nothing still running.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
SUMMARY
What. Two shared caches now key on the RLS predicates of the tables read inside a virtual dataset's SQL (and a SQL Lab query's SQL), not only on the datasource's own RLS:
_column_values_cache_keyinsuperset/datasource/api.py), used by the filter-values endpoint and by the MCP dashboard-filter validation throughget_cached_column_values.QueryContextProcessor._annotation_cache_context), for chart-backed annotation layers whose source chart is on a virtual dataset.Why. Both keys carried only
security_manager.get_rls_cache_key(datasource), which is the datasource's own RLS. Executing a virtual dataset (get_from_clause) also injects the RLS of every table its SQL reads.How.
SqlaTable.get_extra_cache_keys/Query.get_extra_cache_keys, viacollect_rls_predicates_for_sql) moves into a newget_inner_rls_cache_key()method onSqlaTableandQuery. The chart cache key calls it too. The column-values key adds it asinner_rls, and the annotation context adds it per layer assource_inner_rls, so all three use the same predicates.rls-table:[catalog, schema, table, predicate]), so viewers whose rules are swapped between two inner tables get different keys. The chart cache key used the bare predicates, so it had the same collision: two such viewers shared a cached chart result. As a result, chart-data entries of virtual datasets (and annotations) with inner rules are recomputed once after upgrade.collect_rls_predicates_for_sqlalready returns a viewer-scoped marker (per user id, or per guest-token RLS for guests), so the keys fail closed to per-viewer._annotation_source_datasource(same behavior) to keep_annotation_cache_contextunder the complexity limit.{% if %}blocks) gets the viewer-scoped marker, so those entries are cached per viewer. The chart cache key already behaves this way for the same datasets, because it uses the same method.Cache keys built from
get_rls_cache_key(:superset/datasource/api.py_column_values_cache_key: fixed here.QueryContextProcessor._annotation_cache_context: fixed here.QueryContextProcessor.query_cache_key: not affected, it also addsdatasource.get_extra_cache_keys(...), which includes the inner RLS (now through the same method).semantic_layers/cache_host.py_execution_contextand_scope_material: not affected, semantic views have no Superset-executed inner SQL to inject RLS into.This touches the same block of
SqlaTable.get_extra_cache_keys/Query.get_extra_cache_keysas #45681; whichever merges second will need a small merge.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
New file
tests/unit_tests/security/inner_rls_cache_key_test.py, on real SQLite: a physical tablet(rowstenant=1 'alpha',tenant=2 'beta'), a virtual datasetSELECT tenant, name FROM t, viewers (or embedded guests whose token rule is scoped tot) withtenant = 1vstenant = 2, and no rules on the virtual dataset itself.Column values:
test_inner_table_rls_partitions_column_values_cache_keytest_inner_table_rls_viewers_only_see_their_own_cached_valuestest_query_inner_table_rls_partitions_column_values_cache_keytest_inner_rls_collection_failure_falls_back_to_viewer_scoped_keytest_same_inner_table_rls_shares_column_values_cache_keytest_physical_dataset_key_has_no_inner_rls_componenttest_cached_column_values_are_served_without_reaching_the_databaseChart cache key:
test_chart_cache_key_carries_the_same_inner_rls(pins the refactored path forSqlaTableandQuery)Annotation data:
test_annotation_context_partitions_guests_by_inner_table_rlstest_annotation_context_falls_back_to_viewer_scoped_keytest_annotation_context_shares_entry_for_equal_inner_table_rlstest_annotation_context_shape_unchanged_without_inner_rlsBefore the fix (source files at master
a22013d37945c66aab94a70b3234f22b595fbefc, same test file):The four that pass on master are non-regression pins: viewers with the same effective RLS still share an entry, and keys and contexts without inner rules are unchanged.
After the fix:
ADDITIONAL INFORMATION