From d570335f6779f859799eae08ab2f693eecbfe819 Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Tue, 11 Aug 2026 19:35:40 -0700 Subject: [PATCH] fix: bind permission-sync task to user id, use per-user RLS cache sentinel on parse failure (#42938) Co-authored-by: Amin Ghadersohi Co-authored-by: Claude Opus 4.8 --- .../commands/database/sync_permissions.py | 19 +- superset/utils/rls.py | 37 ++- .../integration_tests/databases/api_tests.py | 3 +- tests/integration_tests/sqla_models_tests.py | 16 +- .../sync_permissions_identity_test.py | 140 +++++++++++ .../databases/sync_permissions_test.py | 10 +- .../commands/databases/update_test.py | 4 +- tests/unit_tests/utils/rls_test.py | 225 ++++++++++++++++++ 8 files changed, 434 insertions(+), 20 deletions(-) create mode 100644 tests/unit_tests/commands/databases/sync_permissions_identity_test.py create mode 100644 tests/unit_tests/utils/rls_test.py diff --git a/superset/commands/database/sync_permissions.py b/superset/commands/database/sync_permissions.py index af5064ad3ea..e7790e25140 100644 --- a/superset/commands/database/sync_permissions.py +++ b/superset/commands/database/sync_permissions.py @@ -73,6 +73,7 @@ class SyncPermissionsCommand(BaseCommand): self.username = username self._old_db_connection_name: str | None = old_db_connection_name self._db_connection: Database | None = db_connection + self._user_id: int | None = None self.async_mode: bool = app.config["SYNC_DB_PERMISSIONS_IN_ASYNC_MODE"] @@ -99,11 +100,15 @@ class SyncPermissionsCommand(BaseCommand): if not self._db_connection: raise DatabaseNotFoundError() - # Need user info to impersonate for OAuth2 connections - if not self.username or not security_manager.get_user_by_username( - self.username + # Need user info to impersonate for OAuth2 connections. The id is + # captured here, at validation/enqueue time, so that an async run of + # this command binds to whoever held the username right now, rather + # than re-resolving the (mutable) username at execution time. + if not self.username or not ( + user := security_manager.get_user_by_username(self.username) ): raise UserNotFoundInSessionError() + self._user_id = user.id with self.db_connection.get_sqla_engine() as engine: try: @@ -126,7 +131,7 @@ class SyncPermissionsCommand(BaseCommand): self.validate() if self.async_mode: sync_database_permissions_task.delay( - self.db_connection_id, self.username, self.old_db_connection_name + self.db_connection_id, self._user_id, self.old_db_connection_name ) return @@ -313,14 +318,14 @@ class SyncPermissionsCommand(BaseCommand): @celery_app.task(name="sync_database_permissions", soft_time_limit=600) def sync_database_permissions_task( - database_id: int, username: str, old_db_connection_name: str + database_id: int, user_id: int, old_db_connection_name: str ) -> None: """ Celery task that triggers the SyncPermissionsCommand in async mode. """ with app.test_request_context(): try: - user = security_manager.get_user_by_username(username) + user = security_manager.get_user_by_id(user_id) if not user: raise UserNotFoundInSessionError() g.user = user @@ -336,7 +341,7 @@ def sync_database_permissions_task( SyncPermissionsCommand( database_id, - username, + user.username, old_db_connection_name=old_db_connection_name, db_connection=db_connection, ).sync_database_permissions() diff --git a/superset/utils/rls.py b/superset/utils/rls.py index b9fc1c9975e..48755229d3d 100644 --- a/superset/utils/rls.py +++ b/superset/utils/rls.py @@ -17,18 +17,41 @@ from __future__ import annotations +import hashlib from typing import Any, TYPE_CHECKING from sqlalchemy import and_, or_ -from superset import db +from superset import db, security_manager from superset.sql.parse import Table +from superset.utils import json +from superset.utils.core import get_user_id if TYPE_CHECKING: from superset.models.core import Database from superset.sql.parse import BaseSQLStatement +def _get_cache_identity() -> str: + """ + Build a stable per-session identity to key the parse-failure sentinel on. + + Logged-in users have a stable numeric id from ``get_user_id()``. Guest + users (embedded) don't -- ``get_user_id()`` always returns ``None`` for + them -- so different guest tokens with different RLS scopes would + otherwise all collapse onto the same "user-None" sentinel and share cache + entries. Key those on a hash of the guest token's own RLS rules instead, + so distinct guest scopes stay isolated from one another. + """ + if guest_user := security_manager.get_current_guest_user_if_guest(): + rls_rules = guest_user.guest_token.get("rls_rules", []) + digest = hashlib.sha256( + json.dumps(rls_rules, sort_keys=True).encode("utf-8") + ).hexdigest() + return f"guest-{digest}" + return str(get_user_id()) + + def apply_rls( database: Database, catalog: str | None, @@ -204,6 +227,12 @@ def collect_rls_predicates_for_sql( } ) except Exception: - # If we can't parse the SQL, return empty list - # This ensures RLS application failure doesn't break caching - return [] + # If we can't parse the SQL, we can't tell which (if any) RLS + # predicates would apply, so we can't contribute a meaningful cache + # key component. Returning an empty list here would make every + # user's failure collapse onto the same (missing) contribution, + # which is unsafe when different users have different RLS scopes on + # the underlying tables. Fall back to a per-user marker instead, so + # the cache key still varies by user even though we don't know the + # actual predicates. + return [f"rls-predicate-parse-failed-for-user-{_get_cache_identity()}"] diff --git a/tests/integration_tests/databases/api_tests.py b/tests/integration_tests/databases/api_tests.py index 73f6f69a417..b53f2c9d814 100644 --- a/tests/integration_tests/databases/api_tests.py +++ b/tests/integration_tests/databases/api_tests.py @@ -4608,8 +4608,9 @@ class TestDatabaseApi(SupersetTestCase): assert rv.status_code == 202 response = json.loads(rv.data.decode("utf-8")) assert response == {"message": "Async task created to sync permissions"} + admin_user = security_manager.find_user(username=ADMIN_USERNAME) mock_task.assert_called_once_with( - test_database.id, ADMIN_USERNAME, test_database.database_name + test_database.id, admin_user.id, test_database.database_name ) # Cleanup diff --git a/tests/integration_tests/sqla_models_tests.py b/tests/integration_tests/sqla_models_tests.py index 380e646881f..28e970b8001 100644 --- a/tests/integration_tests/sqla_models_tests.py +++ b/tests/integration_tests/sqla_models_tests.py @@ -835,7 +835,17 @@ def test_none_operand_in_filter(login_as_admin, physical_dataset): '{{ user_email }}' as email, '{{ current_user_roles()|tojson }}' as roles """, - {1, "abc", "abc@test.com", '["role1", "role2"]'}, + # The leading `{% set %}` block isn't valid SQL, so parsing this + # virtual dataset's SQL for RLS predicates fails and the cache key + # picks up the per-user parse-failure sentinel (no user is logged + # in for this test, hence "user-None"). + { + 1, + "abc", + "abc@test.com", + '["role1", "role2"]', + "rls-predicate-parse-failed-for-user-None", + }, True, ), ( @@ -845,7 +855,9 @@ def test_none_operand_in_filter(login_as_admin, physical_dataset): SELECT '{{ user_conditional_id }}' as conditional """, - {1, "abc@test.com"}, + # Same parse-failure sentinel as above: the leading `{% set %}` + # block breaks SQL parsing for RLS predicate collection. + {1, "abc@test.com", "rls-predicate-parse-failed-for-user-None"}, True, ), ( diff --git a/tests/unit_tests/commands/databases/sync_permissions_identity_test.py b/tests/unit_tests/commands/databases/sync_permissions_identity_test.py new file mode 100644 index 00000000000..43c950204b1 --- /dev/null +++ b/tests/unit_tests/commands/databases/sync_permissions_identity_test.py @@ -0,0 +1,140 @@ +# 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. +""" +Traces how ``sync_database_permissions_task`` binds an acting identity. + +The Celery task receives the immutable ``id`` of the user who enqueued it +(captured at enqueue time by ``SyncPermissionsCommand.validate``), not a +mutable username string. At execution time it resolves that id to a user +record via ``security_manager.get_user_by_id`` and binds the result to +``flask.g.user`` for the duration of the sync. Because resolution is by id, +a username change between enqueue and execution has no effect on which user +record the task acts as. + +That identity is not just used for logging: ``Database._get_sqla_engine`` +reads ``g.user.id`` to look up a per-user OAuth2 access token, and, for +databases with ``impersonate_user`` enabled, ``Database.get_effective_user`` +reads ``g.user.username`` (via ``get_username()``) to pick the identity the +outgoing connection impersonates at the external database. These tests pin +down both halves of that chain: the id-based resolution in the task, and the +fact that the resolved user is what a privileged, identity-sensitive +codepath consumes downstream. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock + +from flask import g +from pytest_mock import MockerFixture + +from superset.commands.database.sync_permissions import ( + sync_database_permissions_task, +) +from superset.models.core import Database + + +def test_task_binds_g_user_to_whoever_held_the_id_at_enqueue_time( + mocker: MockerFixture, +) -> None: + """ + The task resolves its acting identity from the user id captured at + enqueue time, via ``security_manager.get_user_by_id``. A username change + that happens between enqueue and execution has no effect on which user + record the task binds ``g.user`` to, because the id -- not the mutable + username -- is what crosses the enqueue/execute boundary. + """ + # Whoever enqueued the task saw this identity at enqueue time. Its id is + # what's passed to the task. + enqueuing_user = MagicMock() + enqueuing_user.id = 101 + enqueuing_user.username = "alice" + + get_user_mock = mocker.patch( + "superset.commands.database.sync_permissions.security_manager.get_user_by_id", + return_value=enqueuing_user, + ) + + mock_db_connection = MagicMock() + mocker.patch( + "superset.commands.database.sync_permissions.DatabaseDAO.find_by_id", + return_value=mock_db_connection, + ) + + observed_g_user: list[MagicMock] = [] + + def capture_g_user(self: object) -> None: + # Read g.user at the moment the sync logic actually runs, the same + # way privileged downstream code (e.g. _get_sqla_engine) would. + observed_g_user.append(g.user) + + mocker.patch( + "superset.commands.database.sync_permissions.SyncPermissionsCommand" + ".sync_database_permissions", + autospec=True, + side_effect=capture_g_user, + ) + + # By the time the task executes, "alice" has been renamed (and the + # username could even have been reassigned to someone else) -- but the + # task was enqueued with id 101, so the rename doesn't affect resolution. + enqueuing_user.username = "alice_renamed" + + sync_database_permissions_task(1, 101, "old_db_name") + + # Resolution happened purely off the immutable id... + get_user_mock.assert_called_once_with(101) + # ...and the sync ran under the same user captured at enqueue time, + # regardless of the username change in between. + assert observed_g_user == [enqueuing_user] + assert observed_g_user[0].id == 101 + + +def test_g_user_bound_by_the_task_drives_external_db_impersonation_identity( + mocker: MockerFixture, +) -> None: + """ + ``Database.get_effective_user`` -- consulted by ``_get_sqla_engine`` to + decide which identity an outgoing, ``impersonate_user``-enabled + connection impersonates at the external database -- reads + ``g.user.username``. Whatever user object the task bound to ``g.user`` + (per the previous test, the user resolved from the id captured at + enqueue time) is therefore the identity used to connect to the external + database. + """ + database = MagicMock(spec=Database) + database.impersonate_user = True + + object_url = MagicMock() + object_url.username = "url-embedded-user" + + # ``get_effective_user`` calls ``get_username()``, which reads + # ``g.user.username`` using the ``g`` imported into + # ``superset.utils.core`` (where ``get_username`` is defined) -- patch + # that module's ``g``, matching what the running task actually touches. + user_a = MagicMock() + user_a.username = "user_a" + mocker.patch("superset.utils.core.g", MagicMock(user=user_a)) + assert Database.get_effective_user(database, object_url) == "user_a" + + # A different user bound to g.user (as would happen if a different id + # had been captured at enqueue time) changes the impersonated identity + # for the exact same database configuration and target URL. + user_b = MagicMock() + user_b.username = "user_b" + mocker.patch("superset.utils.core.g", MagicMock(user=user_b)) + assert Database.get_effective_user(database, object_url) == "user_b" diff --git a/tests/unit_tests/commands/databases/sync_permissions_test.py b/tests/unit_tests/commands/databases/sync_permissions_test.py index 2400c726d03..f6b462b8131 100644 --- a/tests/unit_tests/commands/databases/sync_permissions_test.py +++ b/tests/unit_tests/commands/databases/sync_permissions_test.py @@ -100,7 +100,7 @@ def test_sync_permissions_command_async_mode( "superset.commands.database.sync_permissions.DatabaseDAO" ) mock_database_dao.find_by_id.return_value = database_with_catalog - mocker.patch( + mock_user = mocker.patch( "superset.commands.database.sync_permissions.security_manager.get_user_by_username" ) async_task_mock = mocker.patch( @@ -110,7 +110,7 @@ def test_sync_permissions_command_async_mode( cmmd = SyncPermissionsCommand(1, "admin") cmmd.run() - async_task_mock.delay.assert_called_once_with(1, "admin", "my_db") + async_task_mock.delay.assert_called_once_with(1, mock_user.return_value.id, "my_db") @with_config({"SYNC_DB_PERMISSIONS_IN_ASYNC_MODE": False}) @@ -219,7 +219,7 @@ def test_sync_permissions_command_async_mode_new_db_name( Test ``SyncPermissionsCommand`` in async mode when the database name changed. """ - mocker.patch( + mock_user = mocker.patch( "superset.commands.database.sync_permissions.security_manager.get_user_by_username" ) async_task_mock = mocker.patch( @@ -233,7 +233,9 @@ def test_sync_permissions_command_async_mode_new_db_name( ) cmmd.run() - async_task_mock.delay.assert_called_once_with(1, "admin", "Old Name") + async_task_mock.delay.assert_called_once_with( + 1, mock_user.return_value.id, "Old Name" + ) def test_sync_permissions_command_get_catalogs(database_with_catalog: MagicMock): diff --git a/tests/unit_tests/commands/databases/update_test.py b/tests/unit_tests/commands/databases/update_test.py index 32b816295be..07663df2450 100644 --- a/tests/unit_tests/commands/databases/update_test.py +++ b/tests/unit_tests/commands/databases/update_test.py @@ -122,11 +122,11 @@ def test_update_sync_perms_in_async_mode( "superset.commands.database.sync_permissions.sync_database_permissions_task.delay" ) mocker.patch("superset.commands.database.update.get_username", return_value="admin") - mocker.patch("superset.security_manager.get_user_by_username") + mock_user = mocker.patch("superset.security_manager.get_user_by_username") UpdateDatabaseCommand(1, {}).run() - sync_task.assert_called_once_with(1, "admin", "my_db") + sync_task.assert_called_once_with(1, mock_user.return_value.id, "my_db") def test_update_without_catalog( diff --git a/tests/unit_tests/utils/rls_test.py b/tests/unit_tests/utils/rls_test.py new file mode 100644 index 00000000000..19d26f36f0c --- /dev/null +++ b/tests/unit_tests/utils/rls_test.py @@ -0,0 +1,225 @@ +# 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. +""" +Traces the exact scope of the fallback in +``superset.utils.rls.collect_rls_predicates_for_sql``: a SQL-parse failure +there makes that function return a per-user marker instead of the real +predicates, but this module is wired in as a *cache-key* input only +(``SqlaTable.get_extra_cache_keys``), not as part of the code path that +actually attaches RLS predicates to a query's WHERE clause +(``BaseDatasource.get_sqla_row_level_filters``, consumed directly by +``get_sqla_query``). These tests pin down that separation: a parse failure +in the cache-key helper only ever affects the cache key contribution (kept +distinct per user via the marker), and never "RLS predicates stop being +applied to the query". +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +import pytest +from flask import Flask +from sqlalchemy.sql.elements import TextClause + +from superset.connectors.sqla.models import BaseDatasource +from superset.utils.rls import collect_rls_predicates_for_sql + + +@pytest.fixture +def mock_database() -> MagicMock: + database = MagicMock() + database.db_engine_spec.engine = "sqlite" + database.get_default_catalog.return_value = None + return database + + +def test_collect_rls_predicates_for_sql_returns_per_user_sentinel_on_parse_failure( + mock_database: MagicMock, +) -> None: + """ + A SQL-parse exception inside ``collect_rls_predicates_for_sql`` is + swallowed, and the function returns a marker derived from the current + user's id instead of propagating the exception or silently returning an + empty list. + """ + with ( + patch( + "superset.sql.parse.SQLScript", + side_effect=ValueError("cannot parse"), + ), + patch("superset.utils.rls.get_user_id", return_value=42), + ): + result = collect_rls_predicates_for_sql( + "SELECT * FROM some_table", + mock_database, + catalog=None, + schema="public", + ) + + assert result == ["rls-predicate-parse-failed-for-user-42"] + + +def test_parse_failure_produces_different_cache_contributions_for_different_users( + mock_database: MagicMock, +) -> None: + """ + Two virtual datasets whose underlying RLS predicates differ (one has a + predicate, the other has none) would normally contribute different + strings to the cache key. If SQL parsing fails before predicates are + even collected, the actual predicate difference never gets a chance to + be collected -- but each user still contributes a marker scoped to their + own id, so the two calls don't collapse onto the same cache key + contribution. + """ + with ( + patch( + "superset.sql.parse.SQLScript", + side_effect=ValueError("cannot parse"), + ), + patch( + "superset.utils.rls.get_predicates_for_table", + side_effect=[["tenant_id = 1"], []], + ) as mock_get_predicates, + patch( + "superset.utils.rls.get_user_id", + side_effect=[1, 2], + ), + ): + result_user_one = collect_rls_predicates_for_sql( + "SELECT * FROM some_table", + mock_database, + catalog=None, + schema="public", + ) + result_user_two = collect_rls_predicates_for_sql( + "SELECT * FROM some_table", + mock_database, + catalog=None, + schema="public", + ) + + # get_predicates_for_table was never reached: the parse exception fires + # first, so the per-user predicate difference never had a chance to be + # collected in the first place. + mock_get_predicates.assert_not_called() + assert result_user_one != result_user_two + assert result_user_one == ["rls-predicate-parse-failed-for-user-1"] + assert result_user_two == ["rls-predicate-parse-failed-for-user-2"] + + +def test_parse_failure_sentinel_distinguishes_guest_tokens_by_rls_scope( + mock_database: MagicMock, +) -> None: + """ + ``get_user_id()`` always returns ``None`` for guest users, so keying the + parse-failure sentinel on it alone would collapse every guest token onto + the same cache contribution regardless of the RLS rules baked into each + token. Guest sessions must instead be distinguished by (a hash of) their + own token's ``rls_rules``, so two guests with different row-level scopes + never share a cache entry, while two guests with the *same* scope do. + """ + + def _guest_user(rls_rules: list[dict[str, str]]) -> MagicMock: + guest_user = MagicMock() + guest_user.guest_token = {"rls_rules": rls_rules} + return guest_user + + scope_a = [{"dataset": "1", "clause": "tenant_id = 1"}] + scope_b = [{"dataset": "1", "clause": "tenant_id = 2"}] + + with ( + patch( + "superset.sql.parse.SQLScript", + side_effect=ValueError("cannot parse"), + ), + patch( + "superset.utils.rls.security_manager.get_current_guest_user_if_guest", + side_effect=[ + _guest_user(scope_a), + _guest_user(scope_b), + _guest_user(scope_a), + ], + ), + ): + result_guest_scope_a = collect_rls_predicates_for_sql( + "SELECT * FROM some_table", + mock_database, + catalog=None, + schema="public", + ) + result_guest_scope_b = collect_rls_predicates_for_sql( + "SELECT * FROM some_table", + mock_database, + catalog=None, + schema="public", + ) + result_guest_scope_a_again = collect_rls_predicates_for_sql( + "SELECT * FROM some_table", + mock_database, + catalog=None, + schema="public", + ) + + assert result_guest_scope_a[0].startswith( + "rls-predicate-parse-failed-for-user-guest-" + ) + assert result_guest_scope_a != result_guest_scope_b + assert result_guest_scope_a == result_guest_scope_a_again + + +def test_real_rls_enforcement_does_not_go_through_the_cache_key_helper( + app: Flask, +) -> None: + """ + ``get_sqla_row_level_filters`` -- the method ``get_sqla_query`` actually + calls to build a query's WHERE clause -- reaches the RLS rules directly + via ``security_manager.get_rls_filters`` and never touches + ``collect_rls_predicates_for_sql``. So even in a request where SQL + parsing inside the cache-key helper fails, the predicate is still + attached to the real, executed query: the failure mode is confined to + the cache key, not the query itself. + """ + datasource = MagicMock(spec=BaseDatasource) + datasource.get_template_processor.return_value = MagicMock() + datasource.get_template_processor.return_value.process_template = lambda x: x + datasource.text = lambda x: TextClause(x) + + configured_filter = MagicMock() + configured_filter.clause = "tenant_id = 1" + configured_filter.group_key = None + + with ( + patch( + "superset.connectors.sqla.models.security_manager.get_rls_filters", + return_value=[configured_filter], + ), + patch( + "superset.connectors.sqla.models.is_feature_enabled", + return_value=False, + ), + patch( + "superset.utils.rls.collect_rls_predicates_for_sql", + side_effect=AssertionError( + "get_sqla_row_level_filters must not call the cache-key helper" + ), + ), + ): + filters = BaseDatasource.get_sqla_row_level_filters(datasource) + + assert len(filters) == 1 + assert "tenant_id" in str(filters[0])