aminghadersohi opened a new pull request, #45814: URL: https://github.com/apache/superset/pull/45814
### SUMMARY **What.** The shared column-values cache (`_column_values_cache_key` in `superset/datasource/api.py`, used by the filter-values endpoint and by the MCP dashboard-filter validation through `get_cached_column_values`) now also keys on the RLS predicates of the tables read inside a virtual dataset's SQL (and a SQL Lab query's SQL). **Why.** The key only carried `security_manager.get_rls_cache_key(datasource)`, which is the dataset's own RLS. Executing a virtual dataset (`get_from_clause`) also injects the RLS of every table its SQL reads. Two viewers with the same rules on the virtual dataset but different rules on an inner table therefore got the same key, and the second viewer was served the first viewer's cached filter values. **How.** - The inner-SQL predicate collection the chart cache key already uses (`SqlaTable.get_extra_cache_keys` / `Query.get_extra_cache_keys`, via `collect_rls_predicates_for_sql`) moves into a new `get_inner_rls_cache_key()` method on `SqlaTable` and `Query`. The chart cache key calls it unchanged, and the column-values key adds it as `inner_rls`, so both keys use the same predicates. - If the predicates cannot be collected, `collect_rls_predicates_for_sql` already returns a viewer-scoped marker, so the key fails closed to per-viewer. - `inner_rls` is only added when non-empty, so physical datasets, semantic views and virtual datasets with no inner rules keep their existing key bytes (no cache churn). - No extra work on cache hits beyond what the chart cache key already does for the same dataset (one metadata lookup of the RLS rules for the inner tables). Other cache keys built from `get_rls_cache_key(`: - `QueryContextProcessor.query_cache_key`: not affected, it also adds `datasource.get_extra_cache_keys(...)`, which includes the inner RLS (now through the same method). - `QueryContextProcessor._annotation_cache_context`: not affected, the context includes the requesting user id. - `semantic_layers/cache_host.py` `_execution_context` and `_scope_material`: not affected, semantic views have no Superset-executed inner SQL to inject RLS into; the execution context is also keyed on the principal. This touches the same block of `SqlaTable.get_extra_cache_keys` / `Query.get_extra_cache_keys` as https://github.com/apache/superset/pull/45681; whichever merges second will need a small merge. ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF N/A ### TESTING INSTRUCTIONS New file `tests/unit_tests/datasource/column_values_cache_test.py`, on real SQLite: a physical table `t` (rows `tenant=1 'alpha'`, `tenant=2 'beta'`), a virtual dataset `SELECT tenant, name FROM t`, viewer A with RLS `tenant = 1` on `t`, viewer B with `tenant = 2`, neither with rules on the virtual dataset. - `test_inner_table_rls_partitions_column_values_cache_key` - `test_inner_table_rls_viewers_only_see_their_own_cached_values` - `test_query_inner_table_rls_partitions_column_values_cache_key` - `test_inner_rls_collection_failure_falls_back_to_viewer_scoped_key` - `test_same_inner_table_rls_shares_column_values_cache_key` - `test_physical_dataset_key_has_no_inner_rls_component` Before the fix (master `a22013d37945c66aab94a70b3234f22b595fbefc`), viewer B is served viewer A's cached values as a cache hit: ``` > assert values() == (["beta"], False) E AssertionError: assert (['alpha'], True) == (['beta'], False) FAILED ...::test_inner_table_rls_partitions_column_values_cache_key FAILED ...::test_inner_table_rls_viewers_only_see_their_own_cached_values FAILED ...::test_query_inner_table_rls_partitions_column_values_cache_key FAILED ...::test_inner_rls_collection_failure_falls_back_to_viewer_scoped_key 4 failed, 2 passed ``` After the fix: ``` 6 passed ``` ``` pytest tests/unit_tests/datasource/column_values_cache_test.py pytest tests/unit_tests/datasource tests/unit_tests/models tests/unit_tests/connectors tests/unit_tests/utils/rls_test.py tests/unit_tests/semantic_layers tests/unit_tests/common tests/unit_tests/security tests/unit_tests/sql_lab_test.py tests/unit_tests/mcp_service/dashboard/tool/test_apply_dashboard_filters.py pytest tests/unit_tests/sql --cov=superset/sql --cov-fail-under=100 ``` ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Migration is atomic, supports rollback & is backwards-compatible - [ ] Confirm DB migration upgrade and downgrade tested - [ ] Runtime estimates and downtime expectations provided - [ ] Introduces new feature or API - [ ] Removes existing feature or API -- 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]
