From 841336653cbf2cd01220e4de9a5ddf070b51200e Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Fri, 30 Oct 2026 05:07:22 +0000 Subject: [PATCH 1/2] fix(security): keep loaded datasource perms in sync after Core perm rewrites The permission event hooks rewrite dataset, semantic view and chart perm strings with Core UPDATEs. A datasource instance already loaded in the session kept the previous strings, and set_related_perm copies a chart's perm from that instance, so a chart written later in the same transaction wrote the previous strings back. A viewer whose access came from the dataset grant then got 404 on the chart data call. Record the written values on loaded instances after a database rename, a dataset table/schema/database change and a semantic layer rename. --- superset/security/manager.py | 73 ++++- tests/integration_tests/commands_test.py | 48 ++++ .../security/perm_sync_after_rename_tests.py | 272 ++++++++++++++++++ .../unit_tests/semantic_layers/models_test.py | 50 ++++ 4 files changed, 442 insertions(+), 1 deletion(-) create mode 100644 tests/integration_tests/security/perm_sync_after_rename_tests.py diff --git a/superset/security/manager.py b/superset/security/manager.py index 952aa4a0000b..b58d34480f54 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -79,7 +79,8 @@ Table as SQLATable, ) from sqlalchemy.engine.base import Connection -from sqlalchemy.orm import joinedload +from sqlalchemy.orm import joinedload, object_session +from sqlalchemy.orm.attributes import set_committed_value from sqlalchemy.orm.exc import MultipleResultsFound from sqlalchemy.orm.mapper import Mapper from sqlalchemy.orm.query import Query as SqlaQuery @@ -3874,10 +3875,65 @@ def database_after_update( self._update_vm_datasources_access( mapper, connection, old_database_name, target ) + self._sync_loaded_dataset_perms(connection, target) # Note schema permissions are updated at the API level # (database.commands.update). Since we need to fetch all existing schemas from # the db + @staticmethod + def _set_committed_perms(instance: Any, values: dict[str, Optional[str]]) -> None: + """ + Record perm strings that a Core UPDATE wrote for an instance loaded in + the session. + + The permission event hooks rewrite ``perm``, ``schema_perm`` and + ``catalog_perm`` through the connection, so a loaded instance keeps + the previous strings until it is expired. ``set_related_perm`` copies + a chart's perm from its datasource instance, so a chart flushed later + in the same transaction would write the previous strings back. + + An attribute that is not loaded reads the new value from the database + anyway, and one with a pending change keeps that change. + + :param instance: The loaded datasource instance + :param values: The written value of each perm column + """ + state = inspect(instance) + for key, value in values.items(): + if key in state.unloaded or state.attrs[key].history.has_changes(): + continue + set_committed_value(instance, key, value) + + def _sync_loaded_dataset_perms( + self, + connection: Connection, + target: "Database", + ) -> None: + """ + Refresh the ``perm`` of the database's datasets loaded in the session + after a database rename rewrote it through the connection. + + :param connection: The SQLA connection + :param target: The renamed database + """ + from superset.connectors.sqla.models import ( # pylint: disable=import-outside-toplevel + SqlaTable, + ) + + if not (session := object_session(target)): + return + sqlatable_table = SqlaTable.__table__ # pylint: disable=no-member + rows = connection.execute( + select(sqlatable_table.c.id, sqlatable_table.c.perm).where( + sqlatable_table.c.database_id == target.id + ) + ) + for row in rows: + if dataset := session.identity_map.get( + session.identity_key(SqlaTable, row.id) + ): + self._set_committed_perms(dataset, {"perm": row.perm}) + def _delete_vm_database_access( self, mapper: Mapper, @@ -4312,6 +4368,13 @@ def _update_dataset_catalog_schema_perm( schema_perm=schema_permission_name, ) ) + self._set_committed_perms( + target, + { + "catalog_perm": catalog_permission_name, + "schema_perm": schema_permission_name, + }, + ) # Update charts (Slice schema_perm field) connection.execute( @@ -4390,6 +4453,7 @@ def _update_dataset_perm( # pylint: disable=too-many-arguments ) .values(perm=new_permission_name) ) + self._set_committed_perms(target, {"perm": new_permission_name}) # Update charts (Slice perm field) connection.execute( chart_table.update() @@ -4483,6 +4547,7 @@ def semantic_layer_before_update( views = connection.execute( sv_table.select().where(sv_table.c.semantic_layer_uuid == target.uuid) ).fetchall() + session = object_session(target) for view_row in views: new_view_perm = f"[{target.name}].[{view_row.name}](id:{view_row.id})" old_view_perm = view_row.perm @@ -4499,6 +4564,12 @@ def semantic_layer_before_update( .where(sv_table.c.id == view_row.id) .values(perm=new_view_perm) ) + if session and ( + view := session.identity_map.get( + session.identity_key(SemanticView, view_row.id) + ) + ): + self._set_committed_perms(view, {"perm": new_view_perm}) # Update dependent charts so their denormalized perm stays # in sync with the view: chart-list access filters match diff --git a/tests/integration_tests/commands_test.py b/tests/integration_tests/commands_test.py index 6f56fef4cb11..70d4eedc029a 100644 --- a/tests/integration_tests/commands_test.py +++ b/tests/integration_tests/commands_test.py @@ -15,6 +15,7 @@ # specific language governing permissions and limitations # under the License. import copy +from typing import Any from unittest.mock import patch from uuid import uuid4 @@ -122,6 +123,53 @@ def test_import_assets_resolves_native_annotation_layer(self, mock_add_permissio db.session.delete(database) db.session.commit() + @patch("superset.commands.database.importers.v1.utils.add_permissions") + def test_import_assets_dataset_change_keeps_chart_perms(self, mock_add_permissions): + """ + Re-importing a bundle whose dataset has a new table name and schema + gives its chart the dataset's new perm strings. The dataset update + rewrites them through the connection before the chart is written in + the same transaction. + """ + + def import_bundle(dataset: dict[str, Any]) -> None: + ImportAssetsCommand( + { + "metadata.yaml": yaml.safe_dump(metadata_config), + "databases/imported_database.yaml": yaml.safe_dump(database_config), + "datasets/imported_dataset.yaml": yaml.safe_dump(dataset), + "charts/imported_chart.yaml": yaml.safe_dump(chart_config), + } + ).run() + + import_bundle(dataset_config) + import_bundle( + { + **dataset_config, + "table_name": "imported_dataset_renamed", + "schema": "renamed_schema", + } + ) + + db.session.expire_all() + chart = db.session.query(Slice).filter_by(uuid=chart_config["uuid"]).one() + dataset = chart.table + database = dataset.database + try: + assert dataset.perm == ( + f"[imported_database].[imported_dataset_renamed](id:{dataset.id})" + ) + assert dataset.schema_perm == ( + "[imported_database].[default].[renamed_schema]" + ) + assert chart.perm == dataset.perm + assert chart.schema_perm == dataset.schema_perm + finally: + db.session.delete(chart) + db.session.delete(dataset) + db.session.delete(database) + db.session.commit() + @patch("superset.commands.database.importers.v1.utils.add_permissions") def test_import_assets(self, mock_add_permissions): """Test that we can import multiple assets""" diff --git a/tests/integration_tests/security/perm_sync_after_rename_tests.py b/tests/integration_tests/security/perm_sync_after_rename_tests.py new file mode 100644 index 000000000000..d4941c2cafd5 --- /dev/null +++ b/tests/integration_tests/security/perm_sync_after_rename_tests.py @@ -0,0 +1,272 @@ +# 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. +"""Permission strings of charts after their database or dataset changes.""" + +import gc +import os +import sqlite3 +import tempfile +from typing import Any +from uuid import uuid4 + +from superset import db, security_manager +from superset.connectors.sqla.models import SqlaTable, TableColumn +from superset.models.core import Database +from superset.models.slice import Slice +from superset.utils import json +from superset.utils.core import DatasourceType +from tests.integration_tests.base_tests import SupersetTestCase +from tests.integration_tests.constants import ADMIN_USERNAME + + +def _query_context(dataset_id: int) -> str: + return json.dumps( + { + "datasource": {"id": dataset_id, "type": "table"}, + "force": False, + "queries": [ + { + "filters": [], + "extras": {"having": "", "where": ""}, + "columns": ["a"], + "metrics": [], + "orderby": [], + "row_limit": 10, + } + ], + "result_format": "json", + "result_type": "full", + } + ) + + +class TestDatabaseRenamePermissions(SupersetTestCase): + def setUp(self) -> None: + super().setUp() + suffix = uuid4().hex[:8] + self.old_name = f"rename_perm_old_{suffix}" + self.new_name = f"rename_perm_new_{suffix}" + self.role_name = f"rename_perm_viewer_{suffix}" + self.username = f"rename_perm_viewer_{suffix}" + + fd, self.data_path = tempfile.mkstemp(suffix=".db") + os.close(fd) + with sqlite3.connect(self.data_path) as conn: + conn.executescript( + "CREATE TABLE t1 (a INTEGER); INSERT INTO t1 VALUES (1);" + "CREATE TABLE t2 (a INTEGER); INSERT INTO t2 VALUES (1);" + ) + + self.database = Database( + database_name=self.old_name, + sqlalchemy_uri=f"sqlite:///{self.data_path}", + ) + db.session.add(self.database) + db.session.flush() + + # One dataset with a schema and one without. + self.datasets = [ + SqlaTable( + table_name=table_name, + schema=schema, + database=self.database, + columns=[TableColumn(column_name="a", type="INTEGER")], + ) + for table_name, schema in (("t1", "main"), ("t2", None)) + ] + db.session.add_all(self.datasets) + db.session.flush() + + self.charts = [ + Slice( + slice_name=f"rename_perm_chart_{dataset.schema}_{suffix}", + datasource_id=dataset.id, + datasource_type=DatasourceType.TABLE, + viz_type="table", + params=json.dumps({}), + query_context=_query_context(dataset.id), + ) + for dataset in self.datasets + ] + db.session.add_all(self.charts) + db.session.commit() + + # A viewer whose only data access is the per-dataset grant. + security_manager.copy_role("Gamma", self.role_name, merge=False) + role = security_manager.find_role(self.role_name) + for dataset in self.datasets: + security_manager.add_permission_role( + role, + security_manager.find_permission_view_menu( + "datasource_access", dataset.perm + ), + ) + db.session.commit() + self.create_user_with_roles(self.username, [self.role_name]) + + self.database_id = self.database.id + self.dataset_ids = [dataset.id for dataset in self.datasets] + self.chart_ids = [chart.id for chart in self.charts] + # Start the request from a clean session, like a real API call: no + # instance loaded by the setup may linger in the identity map. + del self.database, self.datasets, self.charts + db.session.close() + gc.collect() + + def tearDown(self) -> None: + db.session.rollback() + for chart_id in self.chart_ids: + db.session.query(Slice).filter_by(id=chart_id).delete() + db.session.commit() + if user := security_manager.find_user(self.username): + db.session.delete(user) + if role := security_manager.find_role(self.role_name): + db.session.delete(role) + db.session.commit() + for dataset_id in self.dataset_ids: + if dataset := db.session.get(SqlaTable, dataset_id): + db.session.delete(dataset) + db.session.commit() + if database := db.session.get(Database, self.database_id): + db.session.delete(database) + db.session.commit() + os.unlink(self.data_path) + super().tearDown() + + def _rename_database_and_assert_access(self) -> None: + self.login(ADMIN_USERNAME) + rv = self.client.put( + f"/api/v1/database/{self.database_id}", + json={"database_name": self.new_name}, + ) + assert rv.status_code == 200, rv.data + self.logout() + + db.session.expire_all() + for chart_id, dataset_id in zip(self.chart_ids, self.dataset_ids, strict=True): + dataset = db.session.get(SqlaTable, dataset_id) + chart = db.session.get(Slice, chart_id) + expected_perm = f"[{self.new_name}].[{dataset.table_name}](id:{dataset_id})" + expected_schema_perm = ( + f"[{self.new_name}].[{dataset.schema}]" if dataset.schema else None + ) + assert dataset.perm == expected_perm + assert dataset.schema_perm == expected_schema_perm + assert chart.perm == expected_perm, dataset.schema + assert chart.schema_perm == expected_schema_perm, dataset.schema + + self.login(self.username) + for chart_id in self.chart_ids: + rv = self.client.get(f"/api/v1/chart/{chart_id}/data/") + assert rv.status_code == 200, (chart_id, rv.data) + + def test_rename_database_keeps_chart_perms_in_sync(self) -> None: + """ + Renaming a database through the API rewrites ``perm`` and + ``schema_perm`` of every chart of its datasets, with and without a + schema, so a viewer with the dataset grant keeps chart data access. + """ + self._rename_database_and_assert_access() + + def test_rename_database_with_loaded_datasets_keeps_chart_perms_in_sync( + self, + ) -> None: + """ + The rename also holds when the datasets are already loaded in the + session, as in a long-lived app context: the rename rewrites their + perm through the connection, and the chart schema perm sync must not + copy the previous perm back from the loaded instances. + """ + loaded = [db.session.get(SqlaTable, id_) for id_ in self.dataset_ids] + assert all(dataset.perm for dataset in loaded) + + self._rename_database_and_assert_access() + + +class TestDatasetChangePermissions(SupersetTestCase): + """ + A dataset change rewrites the dataset's perm strings through the + connection. A chart of that dataset written later in the same transaction + copies its perm strings from the dataset instance, so they must match. + """ + + def setUp(self) -> None: + super().setUp() + suffix = uuid4().hex[:8] + self.databases = [ + Database( + database_name=f"perm_sync_{name}_{suffix}", sqlalchemy_uri="sqlite://" + ) + for name in ("a", "b") + ] + db.session.add_all(self.databases) + db.session.flush() + self.dataset = SqlaTable( + table_name=f"perm_sync_{suffix}", + schema="s1", + database=self.databases[0], + ) + db.session.add(self.dataset) + db.session.flush() + self.chart = Slice( + slice_name=f"perm_sync_{suffix}", + datasource_id=self.dataset.id, + datasource_type=DatasourceType.TABLE, + viz_type="table", + params=json.dumps({}), + ) + db.session.add(self.chart) + db.session.commit() + + def tearDown(self) -> None: + db.session.rollback() + db.session.delete(self.chart) + db.session.delete(self.dataset) + for database in self.databases: + db.session.delete(database) + db.session.commit() + super().tearDown() + + def _change_dataset_then_write_chart(self, **changes: Any) -> None: + for key, value in changes.items(): + setattr(self.dataset, key, value) + db.session.flush() + self.chart.slice_name = f"{self.chart.slice_name} (edited)" + db.session.commit() + + db.session.expire_all() + assert self.dataset.perm == security_manager.get_dataset_perm( + self.dataset.id, + self.dataset.table_name, + self.dataset.database.database_name, + ) + assert self.dataset.schema_perm == security_manager.get_schema_perm( + self.dataset.database.database_name, None, self.dataset.schema + ) + assert self.chart.perm == self.dataset.perm + assert self.chart.schema_perm == self.dataset.schema_perm + + def test_table_rename_then_chart_write(self) -> None: + self._change_dataset_then_write_chart( + table_name=f"{self.dataset.table_name}_renamed" + ) + + def test_schema_change_then_chart_write(self) -> None: + self._change_dataset_then_write_chart(schema="s2") + + def test_database_change_then_chart_write(self) -> None: + self._change_dataset_then_write_chart(database=self.databases[1]) diff --git a/tests/unit_tests/semantic_layers/models_test.py b/tests/unit_tests/semantic_layers/models_test.py index 7e0c6d04eaf8..4b1448fcbeb0 100644 --- a/tests/unit_tests/semantic_layers/models_test.py +++ b/tests/unit_tests/semantic_layers/models_test.py @@ -2289,6 +2289,56 @@ def test_semantic_layer_rename_cascades_to_slice_perms(app: Any) -> None: db.session.rollback() +def test_semantic_layer_rename_then_chart_write_keeps_slice_perm(app: Any) -> None: + """A chart written after a layer rename gets the view's new perm. + + The rename rewrites the view and chart perms through the connection. The + chart's update hook then copies the perm from the view instance loaded + before the rename, so that instance must hold the new perm. + """ + from superset.extensions import db + from superset.models.slice import Slice + from superset.utils.core import DatasourceType + + layer = SemanticLayer() + layer.name = "Old Write Layer" + layer.uuid = uuid.UUID("eeee1111-2222-3333-4444-555566667777") + layer.type = "test" + + view = SemanticView() + view.name = "Write View" + view.semantic_layer_uuid = layer.uuid + + db.session.add(layer) + db.session.add(view) + db.session.flush() + + chart = Slice( + slice_name="On write view", + datasource_type=DatasourceType.SEMANTIC_VIEW, + datasource_id=view.id, + datasource_name="Write View", + viz_type="table", + params="{}", + ) + db.session.add(chart) + db.session.flush() + + try: + layer.name = "New Write Layer" + db.session.flush() + + # The chart loads after the rename; the view stays loaded from before. + db.session.expire(chart) + chart.slice_name = "On write view (edited)" + db.session.flush() + + db.session.refresh(chart) + assert chart.perm == f"[New Write Layer].[Write View](id:{view.id})" + finally: + db.session.rollback() + + # ============================================================================= # build_semantic_view_query dual perm tests # ============================================================================= From 8990688ef63f0cae2f7b39df719042752ffc8b5e Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Fri, 30 Oct 2026 05:17:38 +0000 Subject: [PATCH 2/2] fix(security): read back only loaded datasets after a database rename Restrict the perm read-back to datasets loaded in the session, and clean up the import test's assets even when an import fails. --- superset/security/manager.py | 10 ++++++- tests/integration_tests/commands_test.py | 38 ++++++++++++++---------- 2 files changed, 31 insertions(+), 17 deletions(-) diff --git a/superset/security/manager.py b/superset/security/manager.py index b58d34480f54..dc1026cd78e7 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -3922,10 +3922,18 @@ def _sync_loaded_dataset_perms( if not (session := object_session(target)): return + loaded_ids = [ + key[1][0] + for key in session.identity_map.keys() + if issubclass(key[0], SqlaTable) + ] + if not loaded_ids: + return sqlatable_table = SqlaTable.__table__ # pylint: disable=no-member rows = connection.execute( select(sqlatable_table.c.id, sqlatable_table.c.perm).where( - sqlatable_table.c.database_id == target.id + sqlatable_table.c.database_id == target.id, + sqlatable_table.c.id.in_(loaded_ids), ) ) for row in rows: diff --git a/tests/integration_tests/commands_test.py b/tests/integration_tests/commands_test.py index 70d4eedc029a..681f23ea87ae 100644 --- a/tests/integration_tests/commands_test.py +++ b/tests/integration_tests/commands_test.py @@ -26,7 +26,9 @@ from superset.commands.exceptions import CommandInvalidError from superset.commands.importers.v1.assets import ImportAssetsCommand from superset.commands.importers.v1.utils import is_valid_config +from superset.connectors.sqla.models import SqlaTable from superset.models.annotations import Annotation, AnnotationLayer +from superset.models.core import Database from superset.models.dashboard import Dashboard from superset.models.slice import Slice from superset.utils import json @@ -142,20 +144,19 @@ def import_bundle(dataset: dict[str, Any]) -> None: } ).run() - import_bundle(dataset_config) - import_bundle( - { - **dataset_config, - "table_name": "imported_dataset_renamed", - "schema": "renamed_schema", - } - ) - - db.session.expire_all() - chart = db.session.query(Slice).filter_by(uuid=chart_config["uuid"]).one() - dataset = chart.table - database = dataset.database try: + import_bundle(dataset_config) + import_bundle( + { + **dataset_config, + "table_name": "imported_dataset_renamed", + "schema": "renamed_schema", + } + ) + + db.session.expire_all() + chart = db.session.query(Slice).filter_by(uuid=chart_config["uuid"]).one() + dataset = chart.table assert dataset.perm == ( f"[imported_database].[imported_dataset_renamed](id:{dataset.id})" ) @@ -165,9 +166,14 @@ def import_bundle(dataset: dict[str, Any]) -> None: assert chart.perm == dataset.perm assert chart.schema_perm == dataset.schema_perm finally: - db.session.delete(chart) - db.session.delete(dataset) - db.session.delete(database) + db.session.rollback() + for model, uuid in ( + (Slice, chart_config["uuid"]), + (SqlaTable, dataset_config["uuid"]), + (Database, database_config["uuid"]), + ): + if obj := db.session.query(model).filter_by(uuid=uuid).one_or_none(): + db.session.delete(obj) db.session.commit() @patch("superset.commands.database.importers.v1.utils.add_permissions")