diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 1514a944e98c..b49e9ba4d337 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -740,15 +740,19 @@ def text(self, clause: str) -> TextClause: def get_sqla_row_level_filters( self, template_processor: Optional[BaseTemplateProcessor] = None, + include_global_guest_rls: bool = True, ) -> list[TextClause]: """ Return the appropriate row level security filters for this table and the - current user. A custom username can be passed when the user is not present in the - Flask global namespace. + current user. :param template_processor: The template processor to apply to the filters. + :param include_global_guest_rls: Whether to include global (unscoped) guest + RLS filters. Set to False for underlying tables in virtual datasets to + prevent double application of global guest rules. Dataset-scoped guest + rules are always included regardless of this parameter. :returns: A list of SQL clauses to be ANDed together. - """ # noqa: E501 + """ template_processor = template_processor or self.get_template_processor() all_filters: list[TextClause] = [] @@ -765,6 +769,8 @@ def get_sqla_row_level_filters( if is_feature_enabled("EMBEDDED_SUPERSET"): for rule in security_manager.get_guest_rls_filters(self): + if not include_global_guest_rls and not rule.get("dataset"): + continue clause = self.text( f"({template_processor.process_template(rule['clause'])})" ) diff --git a/superset/models/helpers.py b/superset/models/helpers.py index f6f28f63850d..704b0f4c56a3 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -903,6 +903,7 @@ def get_fetch_values_predicate( def get_sqla_row_level_filters( self, template_processor: Optional[BaseTemplateProcessor] = None, # pylint: disable=unused-argument + include_global_guest_rls: bool = True, # pylint: disable=unused-argument ) -> list[TextClause]: # TODO: We should refactor this mixin and remove this method # as it exists in the BaseDatasource and is not applicable diff --git a/superset/utils/rls.py b/superset/utils/rls.py index 43f7da9ffe81..a2b379caec08 100644 --- a/superset/utils/rls.py +++ b/superset/utils/rls.py @@ -98,6 +98,11 @@ def get_predicates_for_table( if not dataset: return [] + # Exclude global (unscoped) guest RLS to prevent double application in + # virtual datasets. Global guest rules will be applied to the outer query + # via get_sqla_row_level_filters() on the virtual dataset itself. + # Dataset-scoped guest rules are still included here because they target + # this specific physical dataset and won't match on the outer query. return [ str( predicate.compile( @@ -105,7 +110,9 @@ def get_predicates_for_table( compile_kwargs={"literal_binds": True}, ) ) - for predicate in dataset.get_sqla_row_level_filters() + for predicate in dataset.get_sqla_row_level_filters( + include_global_guest_rls=False, + ) ] diff --git a/tests/integration_tests/security/row_level_security_tests.py b/tests/integration_tests/security/row_level_security_tests.py index cab80b49476e..22652023eeb2 100644 --- a/tests/integration_tests/security/row_level_security_tests.py +++ b/tests/integration_tests/security/row_level_security_tests.py @@ -845,3 +845,47 @@ def test_dataset_id_can_be_string(self): sql = dataset.get_query_str(self.query_obj) assert re.search(RLS_ALICE_REGEX, sql) + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + @pytest.mark.usefixtures("load_energy_table_with_slice") + def test_global_rls_not_applied_to_virtual_dataset_inner_tables(self): + """ + Global guest RLS (no dataset ID) should only be applied to the outer + query of a virtual dataset, not to individual inner tables. This + prevents SQL errors when the RLS column doesn't exist on all inner + tables (e.g., a virtual dataset JOINing tables where only one has the + filtered column). + """ + birth_names = self.get_table(name="birth_names") + + virtual_dataset = SqlaTable( + table_name="virtual_birth_names_rls_test", + database=birth_names.database, + schema=birth_names.schema, + sql=f"SELECT * FROM {birth_names.table_name}", # noqa: S608 + ) + db.session.add(virtual_dataset) + db.session.commit() + + try: + # Global RLS rule (no dataset ID) — should only apply to outer + # query, not to the inner birth_names table reference + g.user = self.guest_user_with_rls(rules=[{"clause": "name = 'Alice'"}]) + query_obj = { + "groupby": [], + "metrics": None, + "filter": [], + "is_timeseries": False, + "columns": ["name"], + "granularity": None, + "from_dttm": None, + "to_dttm": None, + "extras": {}, + } + sql = virtual_dataset.get_query_str(query_obj) + + # Guest RLS should appear in the outer WHERE clause + assert re.search(RLS_ALICE_REGEX, sql) + finally: + db.session.delete(virtual_dataset) + db.session.commit() diff --git a/tests/unit_tests/sql_lab_test.py b/tests/unit_tests/sql_lab_test.py index 9fd3a0ac0e24..cdf0c81bdce8 100644 --- a/tests/unit_tests/sql_lab_test.py +++ b/tests/unit_tests/sql_lab_test.py @@ -326,3 +326,31 @@ def test_get_predicates_for_table(mocker: MockerFixture) -> None: table = Table("t1", "public", "examples") assert get_predicates_for_table(table, database, "examples") == ["c1 = 1"] + dataset.get_sqla_row_level_filters.assert_called_once_with( + include_global_guest_rls=False, + ) + + +def test_get_predicates_for_table_excludes_global_guest_rls( + mocker: MockerFixture, +) -> None: + """ + Test that ``get_predicates_for_table`` passes + ``include_global_guest_rls=False`` to ``dataset.get_sqla_row_level_filters`` + to prevent double application of global guest RLS in virtual datasets. + """ + database = mocker.MagicMock() + dataset = mocker.MagicMock() + predicate = mocker.MagicMock() + predicate.compile.return_value = "c1 = 1" + dataset.get_sqla_row_level_filters.return_value = [predicate] + db = mocker.patch("superset.utils.rls.db") + db.session.query().filter().one_or_none.return_value = dataset + + table = Table("t1", "public", "examples") + result = get_predicates_for_table(table, database, "examples") + + assert result == ["c1 = 1"] + dataset.get_sqla_row_level_filters.assert_called_once_with( + include_global_guest_rls=False, + )