aminghadersohi commented on code in PR #45814:
URL: https://github.com/apache/superset/pull/45814#discussion_r4405775222
##########
superset/datasource/api.py:
##########
@@ -102,6 +103,13 @@ def _column_values_cache_key(
rls=security_manager.get_rls_cache_key(datasource),
Review Comment:
Fixed in 3f65d928bbb45686b912c8130b9ccfe2b3b8895e.
Regression tests added by this PR:
`tests/unit_tests/security/inner_rls_cache_key_test.py::test_templated_own_rls_partitions_column_values_cache_key`
and `test_templated_own_rls_partitions_annotation_context`, each for physical
and virtual datasets. Running `pytest
tests/unit_tests/security/inner_rls_cache_key_test.py -k templated_own_rls`
with the API implementation from 091ed907c456f2645b2ee6523be20baa37965a73
produced 4 failures: identical value keys / annotation contexts despite
different rendered own rules. With this commit those cases pass, including the
second viewer's cache MISS and own values.
I used sorted literal-bound compiled own predicates, shared by the value and
annotation keys. The condition uses the same `ExtraCache.regex` detection as
chart caching, but scans only own-RLS clauses (including guest clauses), not
`has_extra_cache_key_calls({})` on the whole datasource. The latter
unnecessarily loads columns/metrics and can autoflush pending metadata during
key construction. Static-rule keys keep their shape. I also narrowed the
comment so it describes the individual fields rather than claiming complete
isolation from raw `rls` alone.
Siblings checked:
- superset/datasource/api.py:103 — fixed in
3f65d928bbb45686b912c8130b9ccfe2b3b8895e
- superset/connectors/sqla/models.py:3257 — fixed in
3f65d928bbb45686b912c8130b9ccfe2b3b8895e
- superset/common/query_context_processor.py:515 — fixed in
3f65d928bbb45686b912c8130b9ccfe2b3b8895e
- superset/common/query_context_processor.py:462 — not affected: chart keys
also collect rendered macro values through datasource.get_extra_cache_keys; the
annotation regression checks that path too.
- superset/models/sql_lab.py:395 — not affected: Query has no own RLS; its
table-bound inner keys include global guest rules.
- superset/security/manager.py:6117 — not affected: this helper
intentionally returns raw clauses/grouping; rendering belongs to the consuming
cache path.
- superset/semantic_layers/cache_host.py:80 — not affected: identity also
hashes the principal, effective roles and guest token, including token
attributes.
- superset/semantic_layers/cache_host.py:192 — not affected: this is an
unsupported-RLS rejection, not a raw-clause-only cache key.
- superset/mcp_service/dashboard/tool/apply_dashboard_filters.py:214 — not
affected: delegates to the updated shared get_cached_column_values helper.
- superset/common/utils/query_cache_manager.py:208 — not affected: reads
annotation payloads using the already-scoped query key; does not construct
another RLS key.
- superset/common/query_object.py:725 — not affected: serializes
annotation-layer definitions plus the caller's annotation_context; no separate
raw-RLS fingerprint.
Searches: `rg -n 'get_rls_cache_key|"col_values:"|_annotation_cache_context'
superset --glob '*.py'` and `rg -n
'annotation.*cache|cache.*annotation|get_cached_column_values' superset --glob
'*.py'`.
##########
tests/unit_tests/security/inner_rls_cache_key_test.py:
##########
@@ -0,0 +1,754 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Shared caches must keep viewers apart when their RLS differs only on a table
+read inside a virtual dataset's SQL.
+
+Executing a virtual dataset injects the RLS of every table its SQL reads, so
+two viewers with the same rules on the virtual dataset itself but different
+rules on an inner table see different rows, and must not share an entry. This
+covers the column-values cache, the chart cache key, and the annotation data
+cached on a parent chart's entry.
+"""
+
+from __future__ import annotations
+
+from collections.abc import Iterator
+from contextlib import contextmanager
+from types import SimpleNamespace
+from typing import Any, TYPE_CHECKING
+from unittest.mock import MagicMock
+
+import pytest
+from flask_caching.backends import SimpleCache
+from pytest_mock import MockerFixture
+from sqlalchemy import create_engine
+from sqlalchemy.orm.session import Session
+from sqlalchemy.pool import StaticPool
+
+from tests.unit_tests.conftest import with_feature_flags
+
+if TYPE_CHECKING:
+ from superset.connectors.sqla.models import SqlaTable
+ from superset.models.slice import Slice
+ from superset.models.sql_lab import Query
+
+# Per-viewer RLS rules on the physical table ``t``; neither viewer has a rule
on
+# the virtual dataset that reads it.
+INNER_RLS: dict[str, str] = {
+ "alice": "tenant = 1",
+ "bob": "tenant = 2",
+ "carol": "tenant = 1",
+}
+
+
[email protected]
+def viewer(mocker: MockerFixture) -> SimpleNamespace:
+ """Current viewer, and RLS rules that depend on it and on the dataset."""
+ from superset import security_manager
+
+ current = SimpleNamespace(name="alice")
+
+ def get_rls_filters(table: Any) -> list[SimpleNamespace]:
+ if getattr(table, "table_name", None) != "t":
+ return []
+ return [SimpleNamespace(id=1, clause=INNER_RLS[current.name],
group_key=None)]
+
+ mocker.patch.object(security_manager, "get_rls_filters",
new=get_rls_filters)
+ return current
+
+
[email protected]
+def data_cache(mocker: MockerFixture) -> SimpleCache:
+ cache = SimpleCache()
+ mocker.patch("superset.datasource.api.cache_manager", data_cache=cache)
+ return cache
+
+
[email protected]
+def virtual_dataset(mocker: MockerFixture, session: Session) ->
Iterator[SqlaTable]:
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+ from superset.models.core import Database
+
+ SqlaTable.metadata.create_all(session.get_bind())
+
+ engine = create_engine(
+ "sqlite://",
+ connect_args={"check_same_thread": False},
+ poolclass=StaticPool,
+ )
+ connection = engine.raw_connection()
+ connection.execute("CREATE TABLE t (tenant INTEGER, name TEXT)")
+ connection.execute("INSERT INTO t VALUES (1, 'alpha'), (2, 'beta')")
+ connection.commit()
+
+ database = Database(database_name="db", sqlalchemy_uri="sqlite://")
+
+ @contextmanager
+ def get_sqla_engine(*args: Any, **kwargs: Any) -> Iterator[Any]:
+ yield engine
+
+ mocker.patch.object(database, "get_sqla_engine", new=get_sqla_engine)
+
+ physical = SqlaTable(
+ database=database,
+ schema="main",
+ table_name="t",
+ columns=[TableColumn(column_name="tenant"),
TableColumn(column_name="name")],
+ )
+ virtual = SqlaTable(
+ database=database,
+ schema="main",
+ table_name="v",
+ sql="SELECT tenant, name FROM t",
+ columns=[TableColumn(column_name="tenant"),
TableColumn(column_name="name")],
+ )
+ session.add_all([database, physical, virtual])
+ session.flush()
+ yield virtual
+ session.rollback()
+
+
+def _inner_key(predicate: str) -> str:
+ """Expected component binding an RLS predicate to the physical table."""
+ from superset.utils import json
+
+ return json.dumps(["rls", None, "main", "t", predicate])
+
+
+def _key(datasource: Any) -> str:
+ from superset.datasource.api import _column_values_cache_key
+
+ return _column_values_cache_key(datasource, {"col": "name"})
+
+
+def test_inner_table_rls_partitions_column_values_cache_key(
+ virtual_dataset: SqlaTable, viewer: SimpleNamespace
+) -> None:
+ """Different RLS on a table inside the virtual SQL yields different
keys."""
+ viewer.name = "alice"
+ alice_key = _key(virtual_dataset)
+ viewer.name = "bob"
+ bob_key = _key(virtual_dataset)
+
+ assert alice_key != bob_key
+
+
+def test_inner_table_rls_viewers_only_see_their_own_cached_values(
+ virtual_dataset: SqlaTable, viewer: SimpleNamespace, data_cache:
SimpleCache
+) -> None:
+ """A viewer is never served values cached for another viewer's inner
RLS."""
+ from superset.datasource.api import get_cached_column_values
+
+ def values() -> tuple[list[Any], bool]:
+ return get_cached_column_values(
+ virtual_dataset,
+ column_name="name",
+ limit=100,
+ denormalize_column=False,
+ )
+
+ viewer.name = "alice"
+ assert values() == (["alpha"], False)
+ assert values() == (["alpha"], True)
+
+ viewer.name = "bob"
+ assert values() == (["beta"], False)
+ assert values() == (["beta"], True)
+
+
+def test_cached_column_values_are_served_without_reaching_the_database(
+ virtual_dataset: SqlaTable,
+ viewer: SimpleNamespace,
+ data_cache: SimpleCache,
+ mocker: MockerFixture,
+) -> None:
+ """
+ Building the key of a dataset with a schema does not connect to its
+ database, so a cache hit still serves values when the database is down.
+ """
+ from superset.datasource.api import get_cached_column_values
+ from superset.models.sql_lab import Query
+
+ def values() -> tuple[list[Any], bool]:
+ return get_cached_column_values(
+ virtual_dataset,
+ column_name="name",
+ limit=100,
+ denormalize_column=False,
+ )
+
+ viewer.name = "alice"
+ assert values() == (["alpha"], False)
+
+ # Looking up the default schema opens a connection to the database.
+ unreachable = mocker.patch.object(
+ virtual_dataset.database,
+ "get_default_schema",
+ side_effect=ConnectionError("database unreachable"),
+ )
+ assert values() == (["alpha"], True)
+
+ query: Query = Query(
+ database=virtual_dataset.database,
+ schema="main",
+ sql="SELECT tenant, name FROM t",
+ client_id="client",
+ )
+ assert query.get_inner_rls_cache_key() == [_inner_key("(tenant = 1)")]
+ unreachable.assert_not_called()
Review Comment:
Fixed the regression test in 3f65d928bbb45686b912c8130b9ccfe2b3b8895e:
`tests/unit_tests/security/inner_rls_cache_key_test.py::test_cached_column_values_are_served_without_reaching_the_database`
uses a PostgreSQL/dynamic-schema datasource and patches `get_inspector`, not
the unused SQLite instance method. Its docstring explicitly scopes the
no-connection guarantee to dynamic-schema engines with an explicit schema. It
checks both the cached value request and the Query inner-key path.
Before/after evidence: forcing `get_rls_schema` to discover the default
before honoring an explicit schema leaves the original SQLite test passing (1
passed), but the strengthened test fails (1 failed). Restoring production code
makes the strengthened test pass; the full focused file has 38 passing cases.
These are local pytest runs with the same test fixture and a single deliberate
production mutation.
Remaining: SQLite/non-dynamic engines still discover the connection's actual
schema through the inspector. This commit does not promise warehouse-free keys
for those engines or reuse a healthy-key entry after schema discovery fails.
Siblings checked:
- tests/unit_tests/security/inner_rls_cache_key_test.py — fixed in
3f65d928bbb45686b912c8130b9ccfe2b3b8895e
- superset/utils/rls.py:58 — not affected: existing schema policy honors
explicit schemas only for dynamic-schema engines; no production schema-policy
change is needed for this test correction.
- superset/connectors/sqla/models.py:3278 — not affected: virtual keys use
the shared schema resolver, covered by the strengthened test.
- superset/models/sql_lab.py:426 — not affected: Query keys use the same
resolver, also checked by the strengthened test.
- superset/models/helpers.py:4116 — not affected: delegates to
get_rls_schema; expression-key outage coverage remains in
test_expression_schema_outage_keeps_chart_keys_viewer_scoped.
Search: `rg -n
'get_rls_schema|get_inspector|get_default_schema|served_without_reaching'
superset/utils/rls.py superset/models/helpers.py superset/models/sql_lab.py
superset/connectors/sqla/models.py
tests/unit_tests/security/inner_rls_cache_key_test.py`.
##########
tests/unit_tests/security/inner_rls_cache_key_test.py:
##########
@@ -0,0 +1,754 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Shared caches must keep viewers apart when their RLS differs only on a table
+read inside a virtual dataset's SQL.
+
+Executing a virtual dataset injects the RLS of every table its SQL reads, so
+two viewers with the same rules on the virtual dataset itself but different
+rules on an inner table see different rows, and must not share an entry. This
+covers the column-values cache, the chart cache key, and the annotation data
+cached on a parent chart's entry.
+"""
+
+from __future__ import annotations
+
+from collections.abc import Iterator
+from contextlib import contextmanager
+from types import SimpleNamespace
+from typing import Any, TYPE_CHECKING
+from unittest.mock import MagicMock
+
+import pytest
+from flask_caching.backends import SimpleCache
+from pytest_mock import MockerFixture
+from sqlalchemy import create_engine
+from sqlalchemy.orm.session import Session
+from sqlalchemy.pool import StaticPool
+
+from tests.unit_tests.conftest import with_feature_flags
+
+if TYPE_CHECKING:
+ from superset.connectors.sqla.models import SqlaTable
+ from superset.models.slice import Slice
+ from superset.models.sql_lab import Query
+
+# Per-viewer RLS rules on the physical table ``t``; neither viewer has a rule
on
+# the virtual dataset that reads it.
+INNER_RLS: dict[str, str] = {
+ "alice": "tenant = 1",
+ "bob": "tenant = 2",
+ "carol": "tenant = 1",
+}
+
+
[email protected]
+def viewer(mocker: MockerFixture) -> SimpleNamespace:
+ """Current viewer, and RLS rules that depend on it and on the dataset."""
+ from superset import security_manager
+
+ current = SimpleNamespace(name="alice")
+
+ def get_rls_filters(table: Any) -> list[SimpleNamespace]:
+ if getattr(table, "table_name", None) != "t":
+ return []
+ return [SimpleNamespace(id=1, clause=INNER_RLS[current.name],
group_key=None)]
+
+ mocker.patch.object(security_manager, "get_rls_filters",
new=get_rls_filters)
+ return current
+
+
[email protected]
+def data_cache(mocker: MockerFixture) -> SimpleCache:
+ cache = SimpleCache()
+ mocker.patch("superset.datasource.api.cache_manager", data_cache=cache)
+ return cache
+
+
[email protected]
+def virtual_dataset(mocker: MockerFixture, session: Session) ->
Iterator[SqlaTable]:
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+ from superset.models.core import Database
+
+ SqlaTable.metadata.create_all(session.get_bind())
+
+ engine = create_engine(
+ "sqlite://",
+ connect_args={"check_same_thread": False},
+ poolclass=StaticPool,
+ )
+ connection = engine.raw_connection()
+ connection.execute("CREATE TABLE t (tenant INTEGER, name TEXT)")
+ connection.execute("INSERT INTO t VALUES (1, 'alpha'), (2, 'beta')")
+ connection.commit()
+
+ database = Database(database_name="db", sqlalchemy_uri="sqlite://")
+
+ @contextmanager
+ def get_sqla_engine(*args: Any, **kwargs: Any) -> Iterator[Any]:
+ yield engine
+
+ mocker.patch.object(database, "get_sqla_engine", new=get_sqla_engine)
+
+ physical = SqlaTable(
+ database=database,
+ schema="main",
+ table_name="t",
+ columns=[TableColumn(column_name="tenant"),
TableColumn(column_name="name")],
+ )
+ virtual = SqlaTable(
+ database=database,
+ schema="main",
+ table_name="v",
+ sql="SELECT tenant, name FROM t",
+ columns=[TableColumn(column_name="tenant"),
TableColumn(column_name="name")],
+ )
+ session.add_all([database, physical, virtual])
+ session.flush()
+ yield virtual
+ session.rollback()
+
+
+def _inner_key(predicate: str) -> str:
+ """Expected component binding an RLS predicate to the physical table."""
+ from superset.utils import json
+
+ return json.dumps(["rls", None, "main", "t", predicate])
+
+
+def _key(datasource: Any) -> str:
+ from superset.datasource.api import _column_values_cache_key
+
+ return _column_values_cache_key(datasource, {"col": "name"})
+
+
+def test_inner_table_rls_partitions_column_values_cache_key(
+ virtual_dataset: SqlaTable, viewer: SimpleNamespace
+) -> None:
+ """Different RLS on a table inside the virtual SQL yields different
keys."""
+ viewer.name = "alice"
+ alice_key = _key(virtual_dataset)
+ viewer.name = "bob"
+ bob_key = _key(virtual_dataset)
+
+ assert alice_key != bob_key
+
+
+def test_inner_table_rls_viewers_only_see_their_own_cached_values(
+ virtual_dataset: SqlaTable, viewer: SimpleNamespace, data_cache:
SimpleCache
+) -> None:
+ """A viewer is never served values cached for another viewer's inner
RLS."""
+ from superset.datasource.api import get_cached_column_values
+
+ def values() -> tuple[list[Any], bool]:
+ return get_cached_column_values(
+ virtual_dataset,
+ column_name="name",
+ limit=100,
+ denormalize_column=False,
+ )
+
+ viewer.name = "alice"
+ assert values() == (["alpha"], False)
+ assert values() == (["alpha"], True)
+
+ viewer.name = "bob"
+ assert values() == (["beta"], False)
+ assert values() == (["beta"], True)
+
+
+def test_cached_column_values_are_served_without_reaching_the_database(
+ virtual_dataset: SqlaTable,
+ viewer: SimpleNamespace,
+ data_cache: SimpleCache,
+ mocker: MockerFixture,
+) -> None:
+ """
+ Building the key of a dataset with a schema does not connect to its
+ database, so a cache hit still serves values when the database is down.
+ """
+ from superset.datasource.api import get_cached_column_values
+ from superset.models.sql_lab import Query
+
+ def values() -> tuple[list[Any], bool]:
+ return get_cached_column_values(
+ virtual_dataset,
+ column_name="name",
+ limit=100,
+ denormalize_column=False,
+ )
+
+ viewer.name = "alice"
+ assert values() == (["alpha"], False)
+
+ # Looking up the default schema opens a connection to the database.
+ unreachable = mocker.patch.object(
+ virtual_dataset.database,
+ "get_default_schema",
+ side_effect=ConnectionError("database unreachable"),
+ )
+ assert values() == (["alpha"], True)
+
+ query: Query = Query(
+ database=virtual_dataset.database,
+ schema="main",
+ sql="SELECT tenant, name FROM t",
+ client_id="client",
+ )
+ assert query.get_inner_rls_cache_key() == [_inner_key("(tenant = 1)")]
+ unreachable.assert_not_called()
+
+
+def test_same_inner_table_rls_shares_column_values_cache_key(
+ virtual_dataset: SqlaTable, viewer: SimpleNamespace
+) -> None:
+ """Different viewers with identical effective RLS keep sharing one
entry."""
+ viewer.name = "alice"
+ alice_key = _key(virtual_dataset)
+ viewer.name = "carol"
+ carol_key = _key(virtual_dataset)
+
+ assert alice_key == carol_key
+
+
+def test_physical_dataset_key_has_no_inner_rls_component(
+ virtual_dataset: SqlaTable, viewer: SimpleNamespace
+) -> None:
+ """A physical dataset's key bytes are unchanged: its own RLS covers it."""
+ import hashlib
+
+ from superset import db
+ from superset.connectors.sqla.models import SqlaTable
+ from superset.utils import json
+
+ physical = db.session.query(SqlaTable).filter_by(table_name="t").one()
+
+ def legacy_key(rls: list[str]) -> str:
+ material = {
+ "col": "name",
+ "uid": physical.uid,
+ "rls": rls,
+ "changed_on": str(physical.changed_on or ""),
+ }
+ digest = hashlib.sha256(json.dumps(material, sort_keys=True).encode())
+ return "col_values:" + digest.hexdigest()
+
+ viewer.name = "alice"
+ assert _key(physical) == legacy_key(["tenant = 1-"])
+ viewer.name = "bob"
+ assert _key(physical) == legacy_key(["tenant = 2-"])
+
+
+def test_query_inner_table_rls_partitions_column_values_cache_key(
+ virtual_dataset: SqlaTable, viewer: SimpleNamespace
+) -> None:
+ """A SQL Lab query datasource keys on the RLS of the tables it reads."""
+ from superset.models.sql_lab import Query
+
+ query: Query = Query(
+ database=virtual_dataset.database,
+ schema="main",
+ sql="SELECT tenant, name FROM t",
+ client_id="client",
+ )
+
+ viewer.name = "alice"
+ alice_key = _key(query)
+ viewer.name = "bob"
+ bob_key = _key(query)
+
+ assert alice_key != bob_key
+
+
+def test_inner_rls_collection_failure_falls_back_to_viewer_scoped_key(
+ virtual_dataset: SqlaTable, viewer: SimpleNamespace, mocker: MockerFixture
+) -> None:
+ """If the inner predicates can't be collected, the key is viewer-scoped."""
+ mocker.patch("superset.sql.parse.SQLScript", side_effect=ValueError("bad
sql"))
+ user_ids = iter([1, 2])
+ mocker.patch("superset.utils.rls.get_user_id", side_effect=lambda:
next(user_ids))
+
+ viewer.name = "alice"
+ alice_key = _key(virtual_dataset)
+ viewer.name = "bob"
+ bob_key = _key(virtual_dataset)
+
+ assert alice_key != bob_key
+
+
+def test_chart_cache_key_carries_the_same_inner_rls(
+ virtual_dataset: SqlaTable, viewer: SimpleNamespace
+) -> None:
+ """The chart cache key keys on the same inner predicates as column
values."""
+ from superset.models.sql_lab import Query
+
+ query: Query = Query(
+ database=virtual_dataset.database,
+ schema="main",
+ sql="SELECT tenant, name FROM t",
+ client_id="client",
+ )
+ for datasource in (virtual_dataset, query):
+ viewer.name = "alice"
+ assert _inner_key("(tenant = 1)") in
datasource.get_extra_cache_keys({})
+ assert datasource.get_inner_rls_cache_key() == [_inner_key("(tenant =
1)")]
+ viewer.name = "bob"
+ assert _inner_key("(tenant = 2)") in
datasource.get_extra_cache_keys({})
+ assert datasource.get_inner_rls_cache_key() == [_inner_key("(tenant =
2)")]
+
+
[email protected]
+def guest(mocker: MockerFixture, virtual_dataset: SqlaTable) ->
SimpleNamespace:
+ """
+ Current embedded guest, whose token scopes one RLS rule to the physical
+ table ``t`` and none to the virtual dataset that reads it.
+ """
+ from superset import db, security_manager
+ from superset.connectors.sqla import models
+ from superset.connectors.sqla.models import SqlaTable
+
+ physical_id =
db.session.query(SqlaTable).filter_by(table_name="t").one().id
+ current = SimpleNamespace(clause="tenant = 1")
+
+ def get_current_guest_user_if_guest() -> SimpleNamespace:
+ rules = [{"dataset": physical_id, "clause": current.clause}]
+ return SimpleNamespace(rls=rules, guest_token={"rls_rules": rules})
+
+ mocker.patch.object(
+ security_manager,
+ "get_current_guest_user_if_guest",
+ new=get_current_guest_user_if_guest,
+ )
+ mocker.patch.object(security_manager, "get_rls_filters", return_value=[])
+ # Guest rules only apply with embedding on. Patched where the RLS filters
+ # read it, so a flag callback left by another test cannot override it.
+ is_feature_enabled = models.is_feature_enabled
+ mocker.patch.object(
+ models,
+ "is_feature_enabled",
+ new=lambda flag: flag == "EMBEDDED_SUPERSET" or
is_feature_enabled(flag),
+ )
+ # Guests have no user id, so it cannot tell two guests' entries apart.
+ mocker.patch(
+ "superset.common.query_context_processor.get_user_id",
return_value=None
+ )
+ return current
+
+
[email protected]
+def source_chart(
+ mocker: MockerFixture, session: Session, virtual_dataset: SqlaTable
+) -> Slice:
+ """An annotation source chart on the virtual dataset."""
+ from superset.models.slice import Slice
+
+ chart = Slice(
+ slice_name="source",
+ viz_type="line",
+ datasource_type="table",
+ datasource_id=virtual_dataset.id,
+ )
+ session.add(chart)
+ session.flush()
+ mocker.patch(
+ "superset.common.query_context_processor.ChartDAO.find_by_id",
+ return_value=chart,
+ )
+ return chart
+
+
+def _annotation_context(chart: Slice) -> dict[str, Any]:
+ from superset.common.query_context_processor import QueryContextProcessor
+
+ query = MagicMock(
+ annotation_layers=[{"sourceType": "line", "name": "a", "value":
chart.id}]
+ )
+ # The context reads nothing from the processor's query context.
+ processor = QueryContextProcessor.__new__(QueryContextProcessor)
+ return processor._annotation_cache_context(query)
+
+
+def test_annotation_context_partitions_guests_by_inner_table_rls(
+ guest: SimpleNamespace, source_chart: Slice
+) -> None:
+ """Guests with different rules on the source's inner table get different
keys."""
+ guest.clause = "tenant = 1"
+ first = _annotation_context(source_chart)
+ guest.clause = "tenant = 2"
+ second = _annotation_context(source_chart)
+
+ # The source dataset's own rules are the same for both guests, so only the
+ # inner-table rules can keep their annotation data apart.
+ assert first["source_rls"] == second["source_rls"]
+ assert first != second
+ assert first["source_inner_rls"] == {
+ str(source_chart.id): [_inner_key("(tenant = 1)")]
+ }
+ assert second["source_inner_rls"] == {
+ str(source_chart.id): [_inner_key("(tenant = 2)")]
+ }
+
+
+def test_annotation_context_shares_entry_for_equal_inner_table_rls(
+ guest: SimpleNamespace, source_chart: Slice
+) -> None:
+ """Guests with identical effective rules keep sharing one entry."""
+ guest.clause = "tenant = 1"
+ first = _annotation_context(source_chart)
+ second = _annotation_context(source_chart)
+
+ assert first == second
+
+
+def test_annotation_context_falls_back_to_viewer_scoped_key(
+ guest: SimpleNamespace, source_chart: Slice, mocker: MockerFixture
+) -> None:
+ """If the inner predicates can't be collected, the key is viewer-scoped."""
+ mocker.patch("superset.sql.parse.SQLScript", side_effect=ValueError("bad
sql"))
+
+ guest.clause = "tenant = 1"
+ first = _annotation_context(source_chart)
+ guest.clause = "tenant = 2"
+ second = _annotation_context(source_chart)
+
+ assert first != second
+
+
+def test_annotation_context_shape_unchanged_without_inner_rls(
+ viewer: SimpleNamespace, source_chart: Slice
+) -> None:
+ """A source with no inner-table rules keeps the existing context shape."""
+ from superset import db
+ from superset.connectors.sqla.models import SqlaTable
+
+ physical = db.session.query(SqlaTable).filter_by(table_name="t").one()
+ source_chart.datasource_id = physical.id
+ db.session.flush()
+ db.session.expire(source_chart)
+
+ assert set(_annotation_context(source_chart)) == {
+ "user_id",
+ "source_rls",
+ "semantic_filter_protocol",
+ "source_versions",
+ }
+
+
[email protected](params=[False, True], ids=["user", "guest"])
+def swapped_inner_rules(
+ request: pytest.FixtureRequest,
+ mocker: MockerFixture,
+ virtual_dataset: SqlaTable,
+ session: Session,
+) -> SimpleNamespace:
+ """Two scopes swap the same predicates between two different inner
tables."""
+ from superset import security_manager
+ from superset.connectors.sqla import models
+ from superset.connectors.sqla.models import SqlaTable, TableColumn
+
+ with virtual_dataset.database.get_sqla_engine() as engine:
+ connection = engine.raw_connection()
+ connection.execute("CREATE TABLE u (tenant INTEGER)")
+ connection.execute("INSERT INTO u VALUES (1), (2)")
+ connection.commit()
+ other = SqlaTable(
+ database=virtual_dataset.database,
+ schema="main",
+ table_name="u",
+ columns=[TableColumn(column_name="tenant")],
+ )
+ session.add(other)
+ virtual_dataset.sql = "SELECT t.tenant, t.name FROM t CROSS JOIN u"
Review Comment:
Fixed the coverage gap in 3f65d928bbb45686b912c8130b9ccfe2b3b8895e with
`tests/unit_tests/security/inner_rls_cache_key_test.py::test_every_inner_table_contributes_its_own_component`.
The new assertion requires the exact sorted list of both qualified
table/predicate components, rather than just different keys. It runs for user
and guest rules, and for both virtual and Query datasources (4 cases).
Before/after evidence: mutating `collect_rls_predicates_for_sql` to keep
only the first table leaves the original file passing (30 passed), but all four
new cases fail (4 failed). Restoring the collector makes those cases pass; the
focused file has 38 passing cases. No production collector change was required.
Siblings checked:
- tests/unit_tests/security/inner_rls_cache_key_test.py — fixed in
3f65d928bbb45686b912c8130b9ccfe2b3b8895e
- superset/utils/rls.py:349 — not affected: production collection already
emits both components; the added regression detects dropping one.
- superset/connectors/sqla/models.py:3278 — not affected: virtual wrapper
preserves both components, tested for user and guest rules.
- superset/models/sql_lab.py:426 — not affected: Query wrapper preserves
both components, also tested for user and guest rules.
- superset/models/helpers.py:4415 — not affected: expression-only collection
uses the same collector and retains table-tagged components; this fixture
targets the inner SQL's two registered tables.
- superset/common/query_context_processor.py:524 — not affected: annotation
context stores the complete inner-key list; swapped-rule annotation regressions
remain in this file.
- superset/datasource/api.py:115 — not affected: column-value keys serialize
the complete inner-key list; swapped-rule cache-read regressions remain in this
file.
Search: `rg -n
'collect_rls_predicates_for_sql|get_inner_rls_cache_key|swapped_table_rules'
superset tests/unit_tests/security/inner_rls_cache_key_test.py`.
--
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]
