From ba7e0053a223a98832c911fb2033065b6e26ce5c Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Wed, 21 Oct 2026 21:40:04 -0700 Subject: [PATCH 1/5] fix(charts): support saved_query datasource in chart create/update Rebased squash of the saved_query chart datasource work onto current master. Co-Authored-By: Evan Rusackas Co-Authored-By: Claude Sonnet 5.5 --- superset/commands/chart/create.py | 2 + superset/commands/chart/update.py | 2 + superset/commands/chart/utils.py | 32 +- superset/daos/datasource.py | 12 + ...9f48_allow_saved_query_chart_datasource.py | 83 +++++ superset/models/slice.py | 10 +- superset/models/sql_lab.py | 116 +++++++ superset/security/manager.py | 176 +++++++++- tests/integration_tests/charts/api_tests.py | 23 +- .../charts/semantic_save_tests.py | 9 +- .../unit_tests/commands/chart/create_test.py | 31 +- .../commands/chart/test_access_checks.py | 185 ++++++++++- .../unit_tests/commands/chart/update_test.py | 109 +++++- tests/unit_tests/datasource/dao_tests.py | 19 ++ tests/unit_tests/models/sql_lab_test.py | 312 ++++++++++++++++++ 15 files changed, 1076 insertions(+), 45 deletions(-) create mode 100644 superset/migrations/versions/2026-10-13_00-00_dfa683969f48_allow_saved_query_chart_datasource.py diff --git a/superset/commands/chart/create.py b/superset/commands/chart/create.py index ecdab6fb44db..0ad812b3de08 100644 --- a/superset/commands/chart/create.py +++ b/superset/commands/chart/create.py @@ -34,6 +34,7 @@ ) from superset.commands.chart.utils import ( touch_dashboards, + raise_for_saved_query_ownership, validate_chart_datasource_type, validate_query_context_datasource, ) @@ -82,6 +83,7 @@ def validate(self) -> None: validate_chart_datasource_type(datasource_type) datasource = get_datasource_by_id(datasource_id, datasource_type) self._properties["datasource_name"] = datasource.name + raise_for_saved_query_ownership(datasource) security_manager.raise_for_access(datasource=datasource) except SupersetSecurityException as ex: raise ChartForbiddenError() from ex diff --git a/superset/commands/chart/update.py b/superset/commands/chart/update.py index 5b67fdd58aa6..1f43e4a02334 100644 --- a/superset/commands/chart/update.py +++ b/superset/commands/chart/update.py @@ -36,6 +36,7 @@ ) from superset.commands.chart.utils import ( touch_dashboards, + raise_for_saved_query_ownership, validate_chart_datasource_type, validate_query_context_datasource, ) @@ -259,6 +260,7 @@ def validate(self) -> None: # noqa: C901 effective_id, datasource_type ) self._properties["datasource_name"] = datasource.name + raise_for_saved_query_ownership(datasource) security_manager.raise_for_access(datasource=datasource) except SupersetSecurityException as ex: raise ChartForbiddenError() from ex diff --git a/superset/commands/chart/utils.py b/superset/commands/chart/utils.py index e1a795017182..e4ce261db6b1 100644 --- a/superset/commands/chart/utils.py +++ b/superset/commands/chart/utils.py @@ -21,7 +21,9 @@ from flask import g from marshmallow import ValidationError +from superset import security_manager from superset.commands.chart.exceptions import ( + ChartForbiddenError, ChartQueryContextDatasourceMismatchValidationError, ) from superset.commands.exceptions import DatasourceTypeInvalidError @@ -36,18 +38,42 @@ def validate_chart_datasource_type(datasource_type: str) -> None: """Accept persistent chart sources with names and type-aware access policies. - SQL Lab queries are registered datasource types but are not persistent - chart sources; accepting all DatasourceDAO registrations would include them. + A raw SQL Lab ``query`` is a registered datasource type but not a + persistent chart source, so it's excluded here; accepting all + DatasourceDAO registrations would include it. A ``saved_query`` is + explicitly persisted and named by the user, so it's accepted alongside + ``table``/``semantic_view``. """ if ( datasource_type == DatasourceType.SEMANTIC_VIEW and not is_semantic_layers_enabled() ): raise ValidationError(SemanticLayersDisabledError.message) - if datasource_type not in (DatasourceType.TABLE, DatasourceType.SEMANTIC_VIEW): + if datasource_type not in ( + DatasourceType.TABLE, + DatasourceType.SEMANTIC_VIEW, + DatasourceType.SAVEDQUERY, + ): raise DatasourceTypeInvalidError() +def raise_for_saved_query_ownership(datasource: Any) -> None: + """ + Refuse a saved query the caller could not read through the saved-query API. + + ``raise_for_access`` accepts a schema grant without looking at the creator, + so without this check a user could bind a chart to another user's private + saved query. Mirrors ``SavedQueryFilter``: only the creator, or a user with + ``all_query_access``, may reference a saved query. + """ + if getattr(datasource, "type", None) != DatasourceType.SAVEDQUERY: + return + if security_manager.can_access_all_queries(): + return + if getattr(datasource, "created_by", None) != g.user: + raise ChartForbiddenError() + + def validate_query_context_datasource( raw_query_context: Any, expected_datasource_id: Any, diff --git a/superset/daos/datasource.py b/superset/daos/datasource.py index e072955336fc..08f502ff2d87 100644 --- a/superset/daos/datasource.py +++ b/superset/daos/datasource.py @@ -99,6 +99,18 @@ def get_datasource( ) raise DatasourceNotFound() + if isinstance(datasource, SavedQuery) and datasource.db_id is None: + # A saved query with no associated database can't back a chart: + # its perm/schema_perm/data properties all need `self.database`. + # Reject it here with a controlled validation error instead of + # letting callers hit an AttributeError further down the line. + logger.warning( + "SavedQuery %s has no associated database; refusing to use " + "it as a chart datasource", + database_id_or_uuid, + ) + raise DatasourceNotFound() + return datasource @classmethod diff --git a/superset/migrations/versions/2026-10-13_00-00_dfa683969f48_allow_saved_query_chart_datasource.py b/superset/migrations/versions/2026-10-13_00-00_dfa683969f48_allow_saved_query_chart_datasource.py new file mode 100644 index 000000000000..447d77011adf --- /dev/null +++ b/superset/migrations/versions/2026-10-13_00-00_dfa683969f48_allow_saved_query_chart_datasource.py @@ -0,0 +1,83 @@ +# 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. +"""allow saved_query chart datasource + +Revision ID: dfa683969f48 +Revises: d623a0cb6bb0 +Create Date: 2026-10-13 00:00:00.000000 + +""" + +import logging + +import sqlalchemy as sa +from alembic import op +from sqlalchemy.orm import declarative_base + +from superset import db + +# revision identifiers, used by Alembic. +revision = "dfa683969f48" +down_revision = "d623a0cb6bb0" + +Base = declarative_base() + +logger = logging.getLogger("alembic.env") + + +class Slice(Base): # type: ignore + __tablename__ = "slices" + + id = sa.Column(sa.Integer, primary_key=True) + datasource_type = sa.Column(sa.String(200)) + + +def upgrade() -> None: + # Update chart datasource constraint to allow saved_query, matching + # CreateChartCommand/UpdateChartCommand.validate_chart_datasource_type + # (apache/superset#29697), which now accepts a saved_query as a valid, + # persistent chart datasource alongside table and semantic_view. + with op.batch_alter_table("slices") as batch_op: + batch_op.drop_constraint("ck_chart_datasource", type_="check") + batch_op.create_check_constraint( + "ck_chart_datasource", + "datasource_type in ('table', 'semantic_view', 'saved_query')", + ) + + +def downgrade() -> None: + # Any chart persisted with datasource_type == saved_query would violate + # the stricter constraint being restored below, so reassign those rows + # first (mirrors the cleanup in 7e67aecbf3f1_chart_ds_constraint.py). + bind = op.get_bind() + session = db.Session(bind=bind) + for slc in session.query(Slice).filter(Slice.datasource_type == "saved_query"): + logger.warning( + "downgrading slice.id = %s from datasource_type saved_query to table", + slc.id, + ) + slc.datasource_type = "table" + session.add(slc) + session.commit() + session.close() + + with op.batch_alter_table("slices") as batch_op: + batch_op.drop_constraint("ck_chart_datasource", type_="check") + batch_op.create_check_constraint( + "ck_chart_datasource", + "datasource_type in ('table', 'semantic_view')", + ) diff --git a/superset/models/slice.py b/superset/models/slice.py index d866142f300b..48165ee987a4 100644 --- a/superset/models/slice.py +++ b/superset/models/slice.py @@ -221,7 +221,9 @@ def __repr__(self) -> str: def datasource(self) -> SqlaTable | None: return self.table - def _display_datasource(self) -> SqlaTable | SemanticView | None: + def _display_datasource( + self, + ) -> SqlaTable | SemanticView | Datasource | None: """Return the datasource used to name and link this chart in listings. Display-only counterpart of ``datasource``: it also resolves semantic @@ -232,6 +234,8 @@ def _display_datasource(self) -> SqlaTable | SemanticView | None: """ if self.datasource_type == utils.DatasourceType.SEMANTIC_VIEW: return self.semantic_view + if self.datasource_type == utils.DatasourceType.SAVEDQUERY: + return self.resolved_datasource return self.table @property @@ -369,7 +373,9 @@ def datasource_name_text(self) -> str | None: @property def datasource_edit_url(self) -> str | None: - return getattr(self._display_datasource(), "url", None) + url = getattr(self._display_datasource(), "url", None) + # ``SavedQuery.url`` is a method, not a property; only surface strings. + return url if isinstance(url, str) else None @property def description_markeddown(self) -> str: diff --git a/superset/models/sql_lab.py b/superset/models/sql_lab.py index 6b3bc409823c..fa2ee1adbcb3 100644 --- a/superset/models/sql_lab.py +++ b/superset/models/sql_lab.py @@ -87,6 +87,9 @@ from superset.utils.dates import naive_utcnow if TYPE_CHECKING: + from sqlalchemy.engine.base import Connection + from sqlalchemy.orm.mapper import Mapper + from superset.connectors.sqla.models import TableColumn from superset.db_engine_specs import BaseEngineSpec @@ -626,6 +629,84 @@ def to_dict(self) -> dict[str, Any]: "id": self.id, } + @property + def type(self) -> str: + """Datasource type discriminator, mirroring ``Query.type``.""" + return "saved_query" + + @property + def name(self) -> str: + """ + Expose ``label`` as ``name`` so callers that treat a ``SavedQuery`` as + a generic datasource (e.g. chart create/update commands) can rely on + a uniform ``name`` attribute across all datasource types. + + ``label`` is a nullable column, so fall back to an id-based name + rather than returning ``None`` from a property typed as ``str``. + """ + return self.label or f"Saved query {self.id}" + + @property + def schema_perm(self) -> Optional[str]: + """ + Schema-level permission string, mirroring ``Query.schema_perm``. + + Required so that ``SecurityManager.raise_for_access(datasource=...)`` + and the ``Slice`` ``before_insert``/``before_update`` listener (which + copies ``perm``/``catalog_perm``/``schema_perm`` onto the chart) can + treat a ``SavedQuery`` like any other datasource instead of raising + ``AttributeError``. + + ``catalog`` is nullable, same as on ``Query``: a saved query against a + database that has a default catalog can leave it unset, so fall back + to ``self.database.get_default_catalog()`` the same way + ``Query.schema_perm`` does -- otherwise this builds a ``[db].[schema]`` + permission string that never matches the ``[db].[catalog].[schema]`` + grant a schema-authorized user actually holds. + """ + if not self.database: + return None + return security_manager.get_schema_perm( + self.database.database_name, + self.catalog or self.database.get_default_catalog(), + self.schema, + ) + + @property + def catalog_perm(self) -> Optional[str]: + """Catalog-level permission string; see ``schema_perm`` above.""" + return security_manager.get_catalog_perm( + self.database.database_name, self.catalog + ) + + @property + def perm(self) -> str: + """Object-level permission string; see ``schema_perm`` above.""" + return f"[{self.database.database_name}].[{self.name}](id:{self.id})" + + @property + def data(self) -> ExplorableData: + """ + Minimal explorable payload. + + ``SecurityManager`` builds access-denied error messages (and a few + dashboard-RBAC lookups) off ``datasource.data["id"]``/``["name"]`` + for *every* datasource type it can be asked to authorize, not just + ones that are actually explorable; without this a denied saved-query + chart request fails with ``AttributeError`` instead of the intended + 403. + """ + result: ExplorableData = { + "id": self.id, + "name": self.name, + "type": "saved_query", + "schema": self.schema, + "catalog": self.catalog, + } + if self.database: + result["database"] = {"id": self.db_id, "backend": self.database.backend} + return result + @property def pop_tab_link(self) -> Markup: return Markup( @@ -659,6 +740,41 @@ def _last_run_delta_humanized(self) -> str: def last_run_delta_humanized(self) -> str: return self._last_run_delta_humanized + def load_database(self) -> None: + """Reload ``self.database`` from ``db_id`` if it's out of sync. + + Mirrors ``SqlaTable.load_database``: needed in ``before_update`` + because ``db_id`` may have just changed in-memory while the + ``database`` relationship still points at the previously loaded row. + """ + # pylint: disable=import-outside-toplevel + from superset.models.core import Database + + if self.db_id and (not self.database or self.database.id != self.db_id): + session = sqla.inspect(self).session # pylint: disable=disallowed-name + self.database = session.query(Database).filter_by(id=self.db_id).one() + + @staticmethod + def before_update( + mapper: "Mapper", + connection: "Connection", + target: "SavedQuery", + ) -> None: + target.load_database() + security_manager.saved_query_before_update(mapper, connection, target) + + @staticmethod + def after_delete( + mapper: "Mapper", + connection: "Connection", + target: "SavedQuery", + ) -> None: + security_manager.saved_query_after_delete(mapper, connection, target) + + +sqla.event.listen(SavedQuery, "before_update", SavedQuery.before_update) +sqla.event.listen(SavedQuery, "after_delete", SavedQuery.after_delete) + class TabState(AuditMixinNullable, ExtraJSONMixin, Model): __tablename__ = "tab_state" diff --git a/superset/security/manager.py b/superset/security/manager.py index e598ddacccc1..999a0c690582 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -132,7 +132,7 @@ from superset.models.core import Database from superset.models.dashboard import Dashboard from superset.models.slice import Slice - from superset.models.sql_lab import Query + from superset.models.sql_lab import Query, SavedQuery from superset.semantic_layers.models import SemanticLayer, SemanticView logger = logging.getLogger(__name__) @@ -3876,6 +3876,11 @@ def database_after_update( mapper, connection, old_database_name, target ) self._sync_loaded_dataset_perms(connection, target) + # Saved-query-backed charts carry the saved query's computed perm + # strings as well (see ``saved_query_before_update``), and the dataset + # walk above never reaches them: there is no PVM to rename, so rewrite + # the chart rows directly. + self._update_saved_query_chart_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 @@ -4888,6 +4893,175 @@ def semantic_view_before_update( .values(perm=new_perm) ) + def saved_query_before_update( + self, + mapper: Mapper, + connection: Connection, + target: "SavedQuery", + ) -> None: + """ + Keep a saved query's dependent charts' denormalized permission + columns in sync when its database, catalog, or schema changes. + + ``SavedQuery.perm``/``catalog_perm``/``schema_perm`` are computed + properties, not persisted columns (unlike ``SqlaTable`` and + ``SemanticView``, which each get a PVM-rename dance in + ``dataset_before_update``/``semantic_view_before_update``), so there + is no FAB ViewMenu here to rename -- saved queries never get a + ``datasource_access`` PVM of their own. But ``Slice``'s + ``before_insert``/``before_update`` listener (``set_related_perm``) + copies those computed strings onto the chart at save time, and + nothing re-copies them if the saved query moves afterwards: a chart + would otherwise keep matching stale schema/catalog grants + indefinitely. This is the dependent-chart half of + ``semantic_view_before_update``, without the PVM-rename half. + """ + # pylint: disable=import-outside-toplevel + from superset.models.sql_lab import SavedQuery + + sq_table = SavedQuery.__table__ # pylint: disable=no-member + current = connection.execute( + sq_table.select().where(sq_table.c.id == target.id) + ).one() + + if ( + current.db_id == target.db_id + and current.catalog == target.catalog + and current.schema == target.schema + ): + return + + if target.database is None: + return + + # Skip the perm computation (which may hit the warehouse via + # ``get_default_catalog``) when no chart references this saved query. + # pylint: disable=import-outside-toplevel + from superset.models.slice import Slice + + chart_table = Slice.__table__ # pylint: disable=no-member + has_charts = connection.execute( + select(chart_table.c.id) + .where( + chart_table.c.datasource_type == DatasourceType.SAVEDQUERY, + chart_table.c.datasource_id == target.id, + ) + .limit(1) + ).first() + if has_charts is None: + return + + self._sync_saved_query_chart_perms(connection, target, target.database) + + def _saved_query_chart_perm_values( + self, + saved_query: "SavedQuery", + database: "Database", + ) -> dict[str, Optional[str]]: + """ + Compute the denormalized ``perm``/``catalog_perm``/``schema_perm`` a + chart backed by ``saved_query`` should carry, given ``database``. + + ``database`` is passed explicitly rather than read off the saved query + because callers run inside a flush: ``SavedQuery.before_update`` may + have just reloaded it from a changed ``db_id``, and on a database + rename (``database_after_update``) the new ``database_name`` only + exists on the in-memory target. + """ + database_name = database.database_name + return { + "perm": f"[{database_name}].[{saved_query.name}](id:{saved_query.id})", + "catalog_perm": self.get_catalog_perm(database_name, saved_query.catalog), + "schema_perm": self.get_schema_perm( + database_name, + saved_query.catalog or database.get_default_catalog(), + saved_query.schema, + ), + } + + def _sync_saved_query_chart_perms( + self, + connection: Connection, + saved_query: "SavedQuery", + database: "Database", + ) -> None: + """Re-copy ``saved_query``'s perm strings onto its dependent charts.""" + # pylint: disable=import-outside-toplevel + from superset.models.slice import Slice + + chart_table = Slice.__table__ # pylint: disable=no-member + connection.execute( + chart_table.update() + .where( + chart_table.c.datasource_type == DatasourceType.SAVEDQUERY, + chart_table.c.datasource_id == saved_query.id, + ) + .values(**self._saved_query_chart_perm_values(saved_query, database)) + ) + + def _update_saved_query_chart_perms( + self, + connection: Connection, + target: "Database", + ) -> None: + """ + Rewrite the denormalized perm columns on every chart backed by one of + ``target``'s saved queries after the database is renamed. + + ``_update_vm_datasources_access`` only walks datasets, so saved-query + charts would otherwise keep their ``[old_name].[...]`` strings and + match a schema or catalog grant on an unrelated connection that later + takes the old name. + """ + # pylint: disable=import-outside-toplevel + from superset.models.sql_lab import SavedQuery + + saved_queries = ( + self.session.query(SavedQuery).filter(SavedQuery.db_id == target.id).all() + ) + for saved_query in saved_queries: + self._sync_saved_query_chart_perms(connection, saved_query, target) + + def saved_query_after_delete( + self, + mapper: Mapper, + connection: Connection, + target: "SavedQuery", + ) -> None: + """ + Detach a deleted saved query's dependent charts. + + ``slices`` has no foreign key to ``saved_query``, so an ORM delete of + a saved query would otherwise leave its charts pointing at a dead + ``datasource_id`` with the saved query's ``perm``/``catalog_perm``/ + ``schema_perm`` still denormalized on the row: a surviving schema or + catalog grant would keep listing a chart whose datasource no longer + resolves, and on a backend that reuses ids a later saved query could + be silently adopted by the orphaned chart. Clear the datasource + reference and the perm columns the way the dataset purge detaches a + purged dataset's charts (``_detach_dataset_charts``); no chart is + deleted. There is no PVM to drop here, unlike + ``dataset_after_delete``/``semantic_view_after_delete``, because a + saved query never gets a ``datasource_access`` ViewMenu of its own. + """ + # pylint: disable=import-outside-toplevel + from superset.models.slice import Slice + + chart_table = Slice.__table__ # pylint: disable=no-member + connection.execute( + chart_table.update() + .where( + chart_table.c.datasource_type == DatasourceType.SAVEDQUERY, + chart_table.c.datasource_id == target.id, + ) + .values( + datasource_id=None, + perm=None, + catalog_perm=None, + schema_perm=None, + ) + ) + def semantic_view_after_delete( self, mapper: Mapper, diff --git a/tests/integration_tests/charts/api_tests.py b/tests/integration_tests/charts/api_tests.py index 3a7cedb8c286..7a263effb308 100644 --- a/tests/integration_tests/charts/api_tests.py +++ b/tests/integration_tests/charts/api_tests.py @@ -674,15 +674,12 @@ def test_create_chart_validate_datasource(self): response = json.loads(rv.data.decode("utf-8")) assert response == {"message": {"datasource_id": ["Datasource does not exist"]}} - def test_create_chart_from_saved_query_rejected_cleanly(self): + def test_create_chart_from_saved_query(self): """ Chart API: creating a chart with datasource_type="saved_query" must - fail with a clean validation error, not the unhandled 500 "Fatal - error" reported in apache/superset#29697. Slice.datasource only - ever resolves the "table" relationship, so even a chart that - "created" successfully with this datasource_type could never - actually render -- "saved_query" is a real, existing row here - (not a bad ID), reproducing the original report exactly rather + succeed instead of raising the unhandled 500 "Fatal error" reported + in apache/superset#29697. "saved_query" is a real, existing row + here (not a bad ID), reproducing the original report exactly rather than a not-found case. """ self.login(ADMIN_USERNAME) @@ -703,15 +700,19 @@ def test_create_chart_from_saved_query_rejected_cleanly(self): "datasource_type": "saved_query", "viz_type": "table", } + chart_id = None try: rv = self.post_assert_metric("/api/v1/chart/", chart_data, "post") - assert rv.status_code == 422 + assert rv.status_code == 201 response = json.loads(rv.data.decode("utf-8")) - assert response == { - "message": {"datasource_type": ["Datasource type is invalid"]} - } + chart_id = response["id"] + chart = db.session.query(Slice).get(chart_id) + assert chart.datasource_type == "saved_query" + assert chart.datasource_id == saved_query_id finally: + if chart_id is not None: + db.session.delete(db.session.query(Slice).get(chart_id)) db.session.delete(db.session.query(SavedQuery).get(saved_query_id)) db.session.commit() diff --git a/tests/integration_tests/charts/semantic_save_tests.py b/tests/integration_tests/charts/semantic_save_tests.py index 1992ef1e7175..0ea292507caf 100644 --- a/tests/integration_tests/charts/semantic_save_tests.py +++ b/tests/integration_tests/charts/semantic_save_tests.py @@ -247,9 +247,14 @@ def test_semantic_chart_save_refused_when_disabled(self) -> None: db.session.delete(layer) db.session.commit() - @parameterized.expand([("query",), ("saved_query",)]) + @parameterized.expand([("query",), ("dataset",)]) def test_unsavable_datasource_stays_422(self, datasource_type: str) -> None: - """The unsupported-type crash protection from #43500 remains intact.""" + """The unsupported-type crash protection from #43500 remains intact. + + ``saved_query`` moved out of this list: apache/superset#29697 made it + a supported, persistent chart datasource type alongside ``table`` and + ``semantic_view`` (see ``api_tests.py::test_create_chart_from_saved_query``). + """ self.login("admin") response: Response = self.client.post( "/api/v1/chart/", diff --git a/tests/unit_tests/commands/chart/create_test.py b/tests/unit_tests/commands/chart/create_test.py index b0c059b2210d..959191d32293 100644 --- a/tests/unit_tests/commands/chart/create_test.py +++ b/tests/unit_tests/commands/chart/create_test.py @@ -18,7 +18,8 @@ Regression coverage for apache/superset#29697: unsupported SQL Lab query objects must be rejected before datasource lookup, not fail with an opaque -500. Table and semantic-view datasources are supported chart sources. +500. Table, semantic-view, and saved-query datasources are supported chart +sources. """ from datetime import datetime @@ -53,32 +54,30 @@ def _base_mocks(mocker: MockerFixture) -> None: ) -@pytest.mark.parametrize("datasource_type", ["saved_query", "query", "bogus"]) +@pytest.mark.parametrize("datasource_type", ["query", "bogus"]) def test_create_chart_rejects_non_table_datasource_type( mocker: MockerFixture, datasource_type: str ) -> None: """SQL Lab query objects must not become persistent chart datasources. - The two unsupported types can fail differently without validation, so - both are covered here: - - "saved_query": SavedQuery has no ``.name`` attribute, so validation - crashes with an unhandled AttributeError -- surfaced to API clients as - an opaque 500 "Fatal error" (apache/superset#29697). - - "query": Query *does* define a synthetic ``.name`` property (used for - CTAS table naming, not as a real display name), so this one doesn't - crash -- it silently "succeeds" and creates a chart with a nonsense - name and a datasource that Slice.datasource can never resolve. - - ``get_datasource_by_id`` is mocked with ``spec=`` the real model classes + "query" is the case worth covering here: Query *does* define a + synthetic ``.name`` property (used for CTAS table naming, not as a real + display name), so it doesn't crash outright -- it would silently + "succeed" and create a chart with a nonsense name and a datasource that + Slice.datasource can never resolve. ("saved_query" is a supported chart + datasource type -- see ``test_access_checks.py`` -- since it is + explicitly persisted and named by the user, unlike a raw SQL Lab query.) + + ``get_datasource_by_id`` is mocked with ``spec=`` the real model class so accessing ``.name`` on the mock behaves exactly like the real ORM - objects do if the new guard doesn't stop the code from getting there; + object does if the new guard doesn't stop the code from getting there; ``raise_for_access`` is mocked to a no-op so nothing downstream masks that behavior. """ - from superset.models.sql_lab import Query, SavedQuery + from superset.models.sql_lab import Query _base_mocks(mocker) - model_cls = SavedQuery if datasource_type == "saved_query" else Query + model_cls = Query get_datasource_by_id = mocker.patch( "superset.commands.chart.create.get_datasource_by_id", return_value=mocker.MagicMock(spec=model_cls), diff --git a/tests/unit_tests/commands/chart/test_access_checks.py b/tests/unit_tests/commands/chart/test_access_checks.py index 368e215779e3..8a3a6b6a9256 100644 --- a/tests/unit_tests/commands/chart/test_access_checks.py +++ b/tests/unit_tests/commands/chart/test_access_checks.py @@ -16,13 +16,18 @@ # under the License. """Unit tests for per-object datasource access checks in chart create/update.""" +from typing import Any from unittest.mock import MagicMock, patch import pytest -from superset.commands.chart.exceptions import ChartForbiddenError +from superset import security_manager +from superset.commands.chart.exceptions import ChartForbiddenError, ChartInvalidError +from superset.commands.exceptions import DatasourceNotFoundValidationError from superset.errors import ErrorLevel, SupersetError, SupersetErrorType from superset.exceptions import SupersetSecurityException +from superset.models.core import Database +from superset.models.sql_lab import SavedQuery def _security_exception() -> SupersetSecurityException: @@ -100,6 +105,161 @@ def test_create_chart_command_allowed_when_access_passes() -> None: command.validate() # should not raise +def test_create_chart_command_supports_saved_query_datasource() -> None: + """CreateChartCommand.validate() must populate ``datasource_name`` for a + ``saved_query`` datasource, which exposes ``label`` rather than ``name``. + + Regression test for https://github.com/apache/superset/issues/29697. + """ + from superset.commands.chart.create import CreateChartCommand + + saved_query = SavedQuery(label="My saved query") + + with ( + patch( + "superset.commands.chart.utils.security_manager.can_access_all_queries", + return_value=True, + ), + patch( + "superset.commands.chart.create.get_datasource_by_id", + return_value=saved_query, + ), + ): + with patch("superset.commands.chart.create.security_manager.raise_for_access"): + with patch( + "superset.commands.chart.create.populate_subjects", + return_value=[], + ): + with patch( + "superset.commands.chart.create.DashboardDAO.find_by_ids", + return_value=[], + ): + command = CreateChartCommand( + { + "slice_name": "test", + "viz_type": "bar", + "datasource_id": 1, + "datasource_type": "saved_query", + } + ) + command.validate() # should not raise AttributeError + + assert command._properties["datasource_name"] == "My saved query" + + +def test_create_chart_command_rejects_saved_query_without_database() -> None: + """A saved query with no associated database (``db_id`` is nullable) + can't back a chart: its ``perm``/``schema_perm``/``data`` properties all + dereference ``self.database``. ``CreateChartCommand.validate()`` must + surface this as a controlled ``ChartInvalidError`` -- via + ``DatasourceDAO.get_datasource`` raising ``DatasourceNotFound`` -- rather + than an ``AttributeError`` later on. + """ + from superset.commands.chart.create import CreateChartCommand + + with patch( + "superset.commands.chart.create.get_datasource_by_id", + side_effect=DatasourceNotFoundValidationError(), + ): + command = CreateChartCommand( + { + "slice_name": "test", + "viz_type": "bar", + "datasource_id": 1, + "datasource_type": "saved_query", + } + ) + with pytest.raises(ChartInvalidError): + command.validate() + + +def test_saved_query_exposes_perm_properties() -> None: + """``SavedQuery`` must expose ``perm``/``schema_perm``/``catalog_perm`` so + it can stand in for a generic datasource: ``Slice``'s ``before_insert``/ + ``before_update`` listener (``superset/models/slice.py::set_related_perm``) + unconditionally reads all three off the resolved datasource when + persisting a chart, regardless of ``datasource_type``. + """ + database = Database(database_name="my_db", sqlalchemy_uri="sqlite://") + saved_query = SavedQuery( + id=1, label="My saved query", schema="main", database=database + ) + + assert saved_query.perm == "[my_db].[My saved query](id:1)" + assert saved_query.schema_perm == "[my_db].[main]" + assert saved_query.catalog_perm is None + + +def test_saved_query_name_falls_back_when_label_is_none() -> None: + """``label`` is a nullable column; ``name`` (typed ``str``) must not + return ``None`` for a saved query that was persisted without one. + """ + saved_query = SavedQuery(id=7, label=None) + + assert saved_query.name == "Saved query 7" + + +def test_raise_for_access_does_not_crash_on_saved_query_datasource( + app_context: None, +) -> None: + """``SecurityManager.raise_for_access(datasource=...)`` must not raise + ``AttributeError`` for a ``saved_query`` datasource. + + Regression test for the ``datasource.perm`` lookup performed in the + non-admin branch of ``raise_for_access`` (previously unreachable for + ``SavedQuery`` since it had no ``perm`` attribute at all). Called for + real, without mocking ``raise_for_access`` itself, per the follow-up + review on https://github.com/apache/superset/issues/29697. + """ + from flask import g + + database = Database(database_name="my_db", sqlalchemy_uri="sqlite://") + saved_query = SavedQuery( + id=1, label="My saved query", schema="main", database=database + ) + + non_admin_user = MagicMock(is_anonymous=False, id=99, roles=[]) + g.user = non_admin_user + try: + with patch.object( + security_manager, "can_access_all_datasources", return_value=False + ): + with patch.object(security_manager, "can_access", return_value=False): + # A denied, non-admin caller must be rejected with the expected + # security exception, not an AttributeError from a missing + # `.perm`/`.schema_perm` attribute on SavedQuery. + with pytest.raises(SupersetSecurityException): + security_manager.raise_for_access(datasource=saved_query) + finally: + del g.user + + +def test_slice_set_related_perm_does_not_crash_on_saved_query_datasource() -> None: + """``Slice``'s ``before_insert``/``before_update`` listener + (``set_related_perm``) unconditionally reads ``perm``/``catalog_perm``/ + ``schema_perm`` off the resolved datasource for *every* chart flush, + independent of ``datasource_type`` — it must not crash for a + ``saved_query``-backed chart either. + """ + from superset.models.slice import set_related_perm, Slice + + database = Database(database_name="my_db", sqlalchemy_uri="sqlite://") + saved_query = SavedQuery( + id=1, label="My saved query", schema="main", database=database + ) + slice_ = Slice(datasource_type="saved_query", datasource_id=1) + + with patch("superset.models.slice.db") as mock_db: + mock_db.session.query.return_value.filter_by.return_value.first.return_value = ( + saved_query # noqa: E501 + ) + set_related_perm(MagicMock(), MagicMock(), slice_) + + assert slice_.perm == "[my_db].[My saved query](id:1)" + assert slice_.schema_perm == "[my_db].[main]" + assert slice_.catalog_perm is None + + def test_create_chart_command_delegates_editors_to_subjects() -> None: """CreateChartCommand.validate() must resolve editor subject IDs.""" from superset.commands.chart.create import CreateChartCommand @@ -181,3 +341,26 @@ def test_update_chart_command_forbidden_when_no_datasource_access() -> None: ) with pytest.raises(ChartForbiddenError): command.validate() + + +def test_raise_for_saved_query_ownership(mocker: Any) -> None: + from superset.commands.chart.exceptions import ChartForbiddenError + from superset.commands.chart.utils import raise_for_saved_query_ownership + from superset.utils.core import DatasourceType + + mocker.patch("superset.commands.chart.utils.g", user="me") + can_all = mocker.patch( + "superset.commands.chart.utils.security_manager.can_access_all_queries", + return_value=False, + ) + foreign = mocker.MagicMock(type=DatasourceType.SAVEDQUERY, created_by="other") + own = mocker.MagicMock(type=DatasourceType.SAVEDQUERY, created_by="me") + table = mocker.MagicMock(type=DatasourceType.TABLE, created_by="other") + + raise_for_saved_query_ownership(own) + raise_for_saved_query_ownership(table) + with pytest.raises(ChartForbiddenError): + raise_for_saved_query_ownership(foreign) + + can_all.return_value = True + raise_for_saved_query_ownership(foreign) diff --git a/tests/unit_tests/commands/chart/update_test.py b/tests/unit_tests/commands/chart/update_test.py index 7658a9524987..caa5af371b4f 100644 --- a/tests/unit_tests/commands/chart/update_test.py +++ b/tests/unit_tests/commands/chart/update_test.py @@ -411,19 +411,18 @@ def test_update_chart_query_context_without_datasource_is_allowed( ).validate() -@pytest.mark.parametrize("datasource_type", ["saved_query", "query", "bogus"]) +@pytest.mark.parametrize("datasource_type", ["query", "bogus"]) def test_update_chart_rejects_repointing_to_non_table_datasource( mocker: MockerFixture, datasource_type: str ) -> None: """Repointing a chart's datasource_id must be rejected the same way CreateChartCommand rejects it (apache/superset#29697): Slice.datasource - only ever resolves the ``table`` relationship, so repointing at a - saved_query or query datasource would "succeed" but leave the chart - permanently unable to render -- or, for saved_query specifically, crash - on SavedQuery's missing ``.name`` attribute before that point is even - reached. This is a regular (non-query-context) update, so it goes - through editorship + compute_subjects, unlike the query-context-only - tests above.""" + only ever resolves the ``table`` relationship, so repointing at a query + datasource would "succeed" but leave the chart permanently unable to + render. (``saved_query`` is a supported chart datasource type -- see + ``test_access_checks.py``.) This is a regular (non-query-context) + update, so it goes through editorship + compute_subjects, unlike the + query-context-only tests above.""" find_by_id = mocker.patch("superset.commands.chart.update.ChartDAO.find_by_id") find_by_id.return_value = mocker.MagicMock( is_managed_externally=False, id=1, tags=[], dashboards=[] @@ -484,6 +483,98 @@ def test_update_chart_accepts_semantic_view_datasource( assert cmd._properties["datasource_name"] == "my_semantic_view" +def test_update_chart_repoints_to_saved_query_datasource( + mocker: MockerFixture, +) -> None: + """Repointing a chart's ``datasource_id`` at a real ``SavedQuery`` must + be accepted and use the effective (newly-supplied) id for the lookup, + deriving ``datasource_name`` from the query's ``label``. + + Regression test for apache/superset#29697's update-command path, which + ``test_access_checks.py`` only covers for ``CreateChartCommand``. Uses a + real ``SavedQuery`` (not a mock) like ``test_access_checks.py`` does, so + an ``AttributeError`` from a missing ``.name`` would surface here too. + """ + from superset.models.sql_lab import SavedQuery + + find_by_id = mocker.patch("superset.commands.chart.update.ChartDAO.find_by_id") + find_by_id.return_value = mocker.MagicMock( + is_managed_externally=False, + id=1, + tags=[], + dashboards=[], + datasource_id=42, + datasource_type="table", + ) + mocker.patch("superset.commands.chart.update.security_manager.raise_for_editorship") + mocker.patch( + "superset.commands.chart.update.compute_subjects", + side_effect=lambda model, properties, exceptions: None, + ) + saved_query = SavedQuery(label="My saved query") + mocker.patch( + "superset.commands.chart.utils.security_manager.can_access_all_queries", + return_value=True, + ) + get_datasource_by_id = mocker.patch( + "superset.commands.chart.update.get_datasource_by_id", + return_value=saved_query, + ) + raise_for_access = mocker.patch( + "superset.commands.chart.update.security_manager.raise_for_access" + ) + + cmd = UpdateChartCommand(1, {"datasource_id": 11, "datasource_type": "saved_query"}) + cmd.validate() + + # The effective id is the newly-supplied 11, not the chart's stored 42. + get_datasource_by_id.assert_called_once_with(11, "saved_query") + raise_for_access.assert_called_once_with(datasource=saved_query) + assert cmd._properties["datasource_name"] == "My saved query" + + +def test_update_chart_rejects_access_denied_saved_query_datasource( + mocker: MockerFixture, +) -> None: + """Repointing a chart at a saved query the user cannot access must raise + ``ChartForbiddenError``, same as any other datasource type. + """ + from superset.models.sql_lab import SavedQuery + + find_by_id = mocker.patch("superset.commands.chart.update.ChartDAO.find_by_id") + find_by_id.return_value = mocker.MagicMock( + is_managed_externally=False, + id=1, + tags=[], + dashboards=[], + datasource_id=42, + datasource_type="table", + ) + mocker.patch("superset.commands.chart.update.security_manager.raise_for_editorship") + mocker.patch( + "superset.commands.chart.update.compute_subjects", + side_effect=lambda model, properties, exceptions: None, + ) + saved_query = SavedQuery(label="My saved query") + mocker.patch( + "superset.commands.chart.utils.security_manager.can_access_all_queries", + return_value=True, + ) + mocker.patch( + "superset.commands.chart.update.get_datasource_by_id", + return_value=saved_query, + ) + mocker.patch( + "superset.commands.chart.update.security_manager.raise_for_access", + side_effect=_access_exc(), + ) + + with pytest.raises(ChartForbiddenError): + UpdateChartCommand( + 1, {"datasource_id": 11, "datasource_type": "saved_query"} + ).validate() + + def test_update_chart_missing_datasource_type_keeps_required_error( mocker: MockerFixture, ) -> None: @@ -519,7 +610,7 @@ def test_update_chart_missing_datasource_type_keeps_required_error( get_datasource_by_id.assert_not_called() -@pytest.mark.parametrize("datasource_type", ["saved_query", "query", "bogus"]) +@pytest.mark.parametrize("datasource_type", ["query", "bogus"]) def test_update_chart_rejects_type_only_non_table_datasource( mocker: MockerFixture, datasource_type: str ) -> None: diff --git a/tests/unit_tests/datasource/dao_tests.py b/tests/unit_tests/datasource/dao_tests.py index 783796186f73..fc95272f8a0d 100644 --- a/tests/unit_tests/datasource/dao_tests.py +++ b/tests/unit_tests/datasource/dao_tests.py @@ -63,8 +63,14 @@ def session_with_data(session: Session) -> Iterator[Session]: ) saved_query = SavedQuery(database=database, sql="select * from foo") + # `db_id` is nullable, so a saved query can be persisted without an + # associated database; DatasourceDAO.get_datasource must reject it + # rather than hand callers a datasource whose perm/schema_perm/data + # properties all dereference `self.database`. + saved_query_no_database = SavedQuery(sql="select * from foo") session.add(saved_query) + session.add(saved_query_no_database) session.add(query_obj) session.add(database) session.add(sqla_table) @@ -111,6 +117,19 @@ def test_get_datasource_saved_query(session_with_data: Session) -> None: assert isinstance(result, SavedQuery) +def test_get_datasource_saved_query_without_database( + session_with_data: Session, +) -> None: + from superset.daos.datasource import DatasourceDAO + from superset.daos.exceptions import DatasourceNotFound + + with pytest.raises(DatasourceNotFound): + DatasourceDAO.get_datasource( + datasource_type=DatasourceType.SAVEDQUERY, + database_id_or_uuid=2, + ) + + def test_get_datasource_w_str_param(session_with_data: Session) -> None: from superset.connectors.sqla.models import SqlaTable from superset.daos.datasource import DatasourceDAO diff --git a/tests/unit_tests/models/sql_lab_test.py b/tests/unit_tests/models/sql_lab_test.py index 8b81b672acb4..169e6a76633b 100644 --- a/tests/unit_tests/models/sql_lab_test.py +++ b/tests/unit_tests/models/sql_lab_test.py @@ -219,6 +219,318 @@ def test_query_schema_perm_guards_unset_database() -> None: assert query.schema_perm == "" +@pytest.mark.parametrize( + ( + "catalog", + "schema", + "database_name", + "default_catalog", + "expected", + ), + [ + (None, "s1", "db", None, "[db].[s1]"), + ("cat", "s1", "db", None, "[db].[cat].[s1]"), + (None, "s1", "db", "defcat", "[db].[defcat].[s1]"), + ], +) +def test_saved_query_schema_perm( + catalog, + schema, + database_name, + default_catalog, + expected, +) -> None: + """SavedQuery.schema_perm mirrors Query.schema_perm's default-catalog + fallback. + + Regression test: a saved query against a database with a default + catalog can leave ``catalog`` unset (same as a raw SQL Lab ``Query``), + so this must fall back to ``database.get_default_catalog()`` -- not + building a ``[db].[schema]`` permission string that never matches the + ``[db].[catalog].[schema]`` grant a schema-authorized user actually + holds. + """ + database = MagicMock() + database.database_name = database_name + database.get_default_catalog.return_value = default_catalog + saved_query = SavedQuery(label="q", schema=schema, catalog=catalog) + saved_query.database = database + with patch.object( + sql_lab_module.security_manager, + "get_schema_perm", + return_value=expected or None, + ) as mock_get_schema_perm: + result = saved_query.schema_perm + assert result == expected + effective_catalog = catalog if catalog is not None else default_catalog + mock_get_schema_perm.assert_called_once_with( + database_name, effective_catalog, schema + ) + + +def test_saved_query_schema_perm_guards_unset_database() -> None: + """schema_perm returns None instead of raising when the database is + unset, mirroring Query.schema_perm_guards_unset_database. + """ + saved_query = SavedQuery(label="q", schema="s1", db_id=1) + assert saved_query.schema_perm is None + + +def test_saved_query_before_update_syncs_dependent_slice_perms(app: Any) -> None: + """Moving a saved query to a new schema also updates dependent charts' + denormalized perm/catalog_perm/schema_perm. + + ``SavedQuery`` has no persisted perm columns of its own and gets no + ``datasource_access`` PVM (unlike ``SqlaTable``/``SemanticView``), but + ``Slice``'s ``set_related_perm`` listener still copies its computed + perm strings onto the chart at save time. Without a ``before_update`` + listener on ``SavedQuery`` to re-copy them, a chart would keep matching + a stale schema/catalog grant after the saved query moves -- the + no-viewer chart-list fallback matches on ``Slice.schema_perm``/ + ``catalog_perm`` (see ``ChartFilter._apply_viewers``). + """ + from superset.extensions import db + from superset.models.core import Database + from superset.models.slice import Slice + from superset.utils.core import DatasourceType + + database = Database(database_name="syncdb", sqlalchemy_uri="sqlite://") + saved_query = SavedQuery(label="Sync Query", schema="old_schema", database=database) + + db.session.add(database) + db.session.add(saved_query) + db.session.flush() + + chart = Slice( + slice_name="On the saved query", + datasource_type=DatasourceType.SAVEDQUERY, + datasource_id=saved_query.id, + datasource_name="Sync Query", + viz_type="table", + params="{}", + ) + db.session.add(chart) + db.session.flush() + + try: + assert chart.schema_perm == saved_query.schema_perm + old_schema_perm = chart.schema_perm + + saved_query.schema = "new_schema" + db.session.flush() + db.session.expire(chart) + db.session.expire(saved_query) + + new_schema_perm = saved_query.schema_perm + assert new_schema_perm != old_schema_perm + assert chart.schema_perm == new_schema_perm + assert chart.catalog_perm == saved_query.catalog_perm + assert chart.perm == saved_query.perm + finally: + db.session.rollback() + + +def test_saved_query_before_update_syncs_perms_when_database_changes( + app: Any, +) -> None: + """Moving a saved query to a different database re-copies the new + database's name into dependent charts' perm/schema_perm, which only works + when ``before_update`` reloads ``database`` from the changed ``db_id``. + """ + from superset.extensions import db + from superset.models.core import Database + from superset.models.slice import Slice + from superset.utils.core import DatasourceType + + database_a = Database(database_name="move_db_a", sqlalchemy_uri="sqlite://") + database_b = Database(database_name="move_db_b", sqlalchemy_uri="sqlite://") + saved_query = SavedQuery(label="Move Query", schema="s1", database=database_a) + + db.session.add_all([database_a, database_b, saved_query]) + db.session.flush() + + chart = Slice( + slice_name="On the moving saved query", + datasource_type=DatasourceType.SAVEDQUERY, + datasource_id=saved_query.id, + datasource_name="Move Query", + viz_type="table", + params="{}", + ) + db.session.add(chart) + db.session.flush() + + try: + assert "move_db_a" in chart.perm + + saved_query.db_id = database_b.id + db.session.flush() + db.session.expire(chart) + + assert "move_db_b" in chart.perm + assert "move_db_a" not in chart.perm + assert "move_db_b" in (chart.schema_perm or "") + finally: + db.session.rollback() + + +def test_saved_query_before_update_syncs_catalog_perm(app: Any) -> None: + """Moving a saved query between catalogs replaces, not just preserves, + the dependent chart's ``catalog_perm`` and the catalog part of its + ``schema_perm``. + """ + from superset.extensions import db + from superset.models.core import Database + from superset.models.slice import Slice + from superset.utils.core import DatasourceType + + database = Database(database_name="catdb", sqlalchemy_uri="sqlite://") + saved_query = SavedQuery( + label="Catalog Query", catalog="cat_a", schema="s1", database=database + ) + db.session.add_all([database, saved_query]) + db.session.flush() + + chart = Slice( + slice_name="On the catalog-hopping saved query", + datasource_type=DatasourceType.SAVEDQUERY, + datasource_id=saved_query.id, + datasource_name="Catalog Query", + viz_type="table", + params="{}", + ) + db.session.add(chart) + db.session.flush() + + try: + assert chart.catalog_perm == "[catdb].[cat_a]" + assert chart.schema_perm == "[catdb].[cat_a].[s1]" + + saved_query.catalog = "cat_b" + db.session.flush() + db.session.expire(chart) + + assert chart.catalog_perm == "[catdb].[cat_b]" + assert chart.schema_perm == "[catdb].[cat_b].[s1]" + finally: + db.session.rollback() + + +def test_database_rename_syncs_saved_query_chart_perms(app: Any) -> None: + """Renaming a database rewrites the perm strings on charts backed by its + saved queries, not only on dataset-backed charts. + + ``database_after_update`` historically only walked ``SqlaTable`` rows, so + a saved-query chart kept ``[old].[...]`` strings after the rename and + would match a schema/catalog grant on an unrelated connection that later + took the old name. + """ + from superset.extensions import db + from superset.models.core import Database + from superset.models.slice import Slice + from superset.utils.core import DatasourceType + + database = Database(database_name="rename_old", sqlalchemy_uri="sqlite://") + saved_query = SavedQuery( + label="Rename Query", catalog="c1", schema="s1", database=database + ) + db.session.add_all([database, saved_query]) + db.session.flush() + + chart = Slice( + slice_name="On the renamed database's saved query", + datasource_type=DatasourceType.SAVEDQUERY, + datasource_id=saved_query.id, + datasource_name="Rename Query", + viz_type="table", + params="{}", + ) + db.session.add(chart) + db.session.flush() + + try: + saved_query_id = saved_query.id + assert chart.perm == f"[rename_old].[Rename Query](id:{saved_query_id})" + assert chart.catalog_perm == "[rename_old].[c1]" + assert chart.schema_perm == "[rename_old].[c1].[s1]" + + database.database_name = "rename_new" + db.session.flush() + db.session.expire(chart) + + assert chart.perm == f"[rename_new].[Rename Query](id:{saved_query_id})" + assert chart.catalog_perm == "[rename_new].[c1]" + assert chart.schema_perm == "[rename_new].[c1].[s1]" + finally: + db.session.rollback() + + +def test_saved_query_after_delete_detaches_dependent_slices(app: Any) -> None: + """Deleting a saved query clears its charts' datasource reference and + denormalized perm columns. + + ``slices`` has no foreign key to ``saved_query``, so without an + ``after_delete`` listener the chart would keep the dead ``datasource_id`` + and the saved query's perm strings: a surviving schema/catalog grant + would still list it (``ChartFilter._apply_viewers``) even though it can + no longer resolve, and a later saved query reusing the id would be + silently adopted. Mirrors ``_detach_dataset_charts`` in the dataset + purge. A table-backed chart sharing the numeric id must be left alone. + """ + from superset.extensions import db + from superset.models.core import Database + from superset.models.slice import Slice + from superset.utils.core import DatasourceType + + database = Database(database_name="del_db", sqlalchemy_uri="sqlite://") + saved_query = SavedQuery( + label="Doomed Query", catalog="c1", schema="s1", database=database + ) + + db.session.add_all([database, saved_query]) + db.session.flush() + + chart = Slice( + slice_name="On the doomed saved query", + datasource_type=DatasourceType.SAVEDQUERY, + datasource_id=saved_query.id, + datasource_name="Doomed Query", + viz_type="table", + params="{}", + ) + same_id_table_chart = Slice( + slice_name="Unrelated table chart with the same id", + datasource_type=DatasourceType.TABLE, + datasource_id=saved_query.id, + datasource_name="unrelated", + viz_type="table", + params="{}", + ) + db.session.add_all([chart, same_id_table_chart]) + db.session.flush() + + try: + saved_query_id = saved_query.id + assert chart.datasource_id == saved_query_id + assert chart.perm == saved_query.perm + assert chart.catalog_perm == "[del_db].[c1]" + assert chart.schema_perm == "[del_db].[c1].[s1]" + + db.session.delete(saved_query) + db.session.flush() + db.session.expire(chart) + db.session.expire(same_id_table_chart) + + assert chart.datasource_id is None + assert chart.perm is None + assert chart.catalog_perm is None + assert chart.schema_perm is None + + assert same_id_table_chart.datasource_id == saved_query_id + finally: + db.session.rollback() + + def test_adhoc_column_to_sqla_applies_time_grain_for_unsaved_dataset() -> None: """ Selecting a time grain on a chart backed by an unsaved SQL Lab query must From f14142d4fa5527a95f53ec96f335d80e60bdc896 Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Thu, 22 Oct 2026 21:39:32 -0700 Subject: [PATCH 2/5] style(chart): sort imports in chart create/update commands Co-Authored-By: Evan Rusackas Co-Authored-By: Claude Sonnet 5.5 --- superset/commands/chart/create.py | 2 +- superset/commands/chart/update.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/superset/commands/chart/create.py b/superset/commands/chart/create.py index 0ad812b3de08..afd3ebfc25f7 100644 --- a/superset/commands/chart/create.py +++ b/superset/commands/chart/create.py @@ -33,8 +33,8 @@ DashboardsNotFoundValidationError, ) from superset.commands.chart.utils import ( - touch_dashboards, raise_for_saved_query_ownership, + touch_dashboards, validate_chart_datasource_type, validate_query_context_datasource, ) diff --git a/superset/commands/chart/update.py b/superset/commands/chart/update.py index 1f43e4a02334..22de525f8d14 100644 --- a/superset/commands/chart/update.py +++ b/superset/commands/chart/update.py @@ -35,8 +35,8 @@ DatasourceTypeUpdateRequiredValidationError, ) from superset.commands.chart.utils import ( - touch_dashboards, raise_for_saved_query_ownership, + touch_dashboards, validate_chart_datasource_type, validate_query_context_datasource, ) From b73cacac3dc399d509d8eb46479df306139a4217 Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Sun, 25 Oct 2026 13:21:05 -0700 Subject: [PATCH 3/5] fix(charts): tighten saved-query chart perms and access checks Require a real database/schema grant (not saved-query editorship) when binding a chart to a saved query, fall back to the default catalog for catalog_perm, resync dependent charts on catalog changes, and limit database-rename resync to saved queries that have charts, resolving the default catalog once. Co-Authored-By: Evan Rusackas Co-Authored-By: Claude Sonnet 5.5 --- superset/commands/chart/create.py | 3 +- superset/commands/chart/update.py | 3 +- superset/commands/chart/utils.py | 16 ++++ superset/commands/database/update.py | 9 ++- superset/models/sql_lab.py | 5 +- superset/security/manager.py | 74 +++++++++++++++---- .../unit_tests/commands/chart/update_test.py | 10 ++- tests/unit_tests/models/sql_lab_test.py | 45 +++++++++++ 8 files changed, 145 insertions(+), 20 deletions(-) diff --git a/superset/commands/chart/create.py b/superset/commands/chart/create.py index afd3ebfc25f7..283423f3c9f2 100644 --- a/superset/commands/chart/create.py +++ b/superset/commands/chart/create.py @@ -33,6 +33,7 @@ DashboardsNotFoundValidationError, ) from superset.commands.chart.utils import ( + raise_for_chart_datasource_access, raise_for_saved_query_ownership, touch_dashboards, validate_chart_datasource_type, @@ -84,7 +85,7 @@ def validate(self) -> None: datasource = get_datasource_by_id(datasource_id, datasource_type) self._properties["datasource_name"] = datasource.name raise_for_saved_query_ownership(datasource) - security_manager.raise_for_access(datasource=datasource) + raise_for_chart_datasource_access(datasource) except SupersetSecurityException as ex: raise ChartForbiddenError() from ex except ValidationError as ex: diff --git a/superset/commands/chart/update.py b/superset/commands/chart/update.py index 22de525f8d14..86bf908e835b 100644 --- a/superset/commands/chart/update.py +++ b/superset/commands/chart/update.py @@ -35,6 +35,7 @@ DatasourceTypeUpdateRequiredValidationError, ) from superset.commands.chart.utils import ( + raise_for_chart_datasource_access, raise_for_saved_query_ownership, touch_dashboards, validate_chart_datasource_type, @@ -261,7 +262,7 @@ def validate(self) -> None: # noqa: C901 ) self._properties["datasource_name"] = datasource.name raise_for_saved_query_ownership(datasource) - security_manager.raise_for_access(datasource=datasource) + raise_for_chart_datasource_access(datasource) except SupersetSecurityException as ex: raise ChartForbiddenError() from ex except ValidationError as ex: diff --git a/superset/commands/chart/utils.py b/superset/commands/chart/utils.py index e4ce261db6b1..0f590b4476cc 100644 --- a/superset/commands/chart/utils.py +++ b/superset/commands/chart/utils.py @@ -74,6 +74,22 @@ def raise_for_saved_query_ownership(datasource: Any) -> None: raise ChartForbiddenError() +def raise_for_chart_datasource_access(datasource: Any) -> None: + """ + Authorize binding a chart to ``datasource``. + + A saved query is authored through an API that does not validate its + database, so editorship of one (being its creator) must not stand in for a + database, catalog, or schema grant: require the real grant. + """ + security_manager.raise_for_access( + datasource=datasource, + allow_editorship=( + getattr(datasource, "type", None) != DatasourceType.SAVEDQUERY + ), + ) + + def validate_query_context_datasource( raw_query_context: Any, expected_datasource_id: Any, diff --git a/superset/commands/database/update.py b/superset/commands/database/update.py index a0fe624b38a9..c2409f08e501 100644 --- a/superset/commands/database/update.py +++ b/superset/commands/database/update.py @@ -23,7 +23,7 @@ from flask_appbuilder.models.sqla import Model -from superset import db +from superset import db, security_manager from superset.commands.base import BaseCommand from superset.commands.database.exceptions import ( DatabaseConnectionFailedError, @@ -272,6 +272,13 @@ def _update_catalog_attribute( update = {"catalog": new_catalog} db.session.query(model).filter_by(**predicate).update(update) + # The bulk update above bypasses ``saved_query_before_update``, so + # re-copy the saved queries' perm strings onto their dependent charts. + if database := db.session.query(Database).get(database_id): + security_manager.sync_database_saved_query_chart_perms( + db.session.connection(), database + ) + def validate(self) -> None: if database_name := self._properties.get("database_name"): if not DatabaseDAO.validate_update_uniqueness( diff --git a/superset/models/sql_lab.py b/superset/models/sql_lab.py index fa2ee1adbcb3..d2f6516a12f6 100644 --- a/superset/models/sql_lab.py +++ b/superset/models/sql_lab.py @@ -675,8 +675,11 @@ def schema_perm(self) -> Optional[str]: @property def catalog_perm(self) -> Optional[str]: """Catalog-level permission string; see ``schema_perm`` above.""" + if not self.database: + return None return security_manager.get_catalog_perm( - self.database.database_name, self.catalog + self.database.database_name, + self.catalog or self.database.get_default_catalog(), ) @property diff --git a/superset/security/manager.py b/superset/security/manager.py index 999a0c690582..e0eef2815f30 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -4957,6 +4957,7 @@ def _saved_query_chart_perm_values( self, saved_query: "SavedQuery", database: "Database", + default_catalog: Optional[str] = None, ) -> dict[str, Optional[str]]: """ Compute the denormalized ``perm``/``catalog_perm``/``schema_perm`` a @@ -4967,15 +4968,21 @@ def _saved_query_chart_perm_values( have just reloaded it from a changed ``db_id``, and on a database rename (``database_after_update``) the new ``database_name`` only exists on the in-memory target. + + ``default_catalog`` lets callers that resolve many saved queries + against one database look the default catalog up once (it can be a + warehouse round trip); when omitted it is resolved lazily, and only + for a saved query whose own ``catalog`` is unset. """ database_name = database.database_name + catalog = saved_query.catalog or default_catalog + if catalog is None and not saved_query.catalog: + catalog = database.get_default_catalog() return { "perm": f"[{database_name}].[{saved_query.name}](id:{saved_query.id})", - "catalog_perm": self.get_catalog_perm(database_name, saved_query.catalog), + "catalog_perm": self.get_catalog_perm(database_name, catalog), "schema_perm": self.get_schema_perm( - database_name, - saved_query.catalog or database.get_default_catalog(), - saved_query.schema, + database_name, catalog, saved_query.schema ), } @@ -4984,6 +4991,7 @@ def _sync_saved_query_chart_perms( connection: Connection, saved_query: "SavedQuery", database: "Database", + default_catalog: Optional[str] = None, ) -> None: """Re-copy ``saved_query``'s perm strings onto its dependent charts.""" # pylint: disable=import-outside-toplevel @@ -4996,9 +5004,56 @@ def _sync_saved_query_chart_perms( chart_table.c.datasource_type == DatasourceType.SAVEDQUERY, chart_table.c.datasource_id == saved_query.id, ) - .values(**self._saved_query_chart_perm_values(saved_query, database)) + .values( + **self._saved_query_chart_perm_values( + saved_query, database, default_catalog + ) + ) ) + def sync_database_saved_query_chart_perms( + self, + connection: Connection, + database: "Database", + ) -> None: + """ + Re-copy perm strings onto the charts of every saved query on + ``database`` that has dependent charts. + + Only saved queries referenced by a chart are loaded, and the default + catalog is resolved at most once, so a database with many historical + saved queries does not trigger a per-query warehouse round trip. + """ + # pylint: disable=import-outside-toplevel + from superset.models.slice import Slice + from superset.models.sql_lab import SavedQuery + + chart_table = Slice.__table__ # pylint: disable=no-member + chart_ids = ( + select(chart_table.c.datasource_id) + .where( + chart_table.c.datasource_type == DatasourceType.SAVEDQUERY, + chart_table.c.datasource_id.is_not(None), + ) + .scalar_subquery() + ) + saved_queries = ( + self.session.query(SavedQuery) + .filter(SavedQuery.db_id == database.id, SavedQuery.id.in_(chart_ids)) + .all() + ) + if not saved_queries: + return + default_catalog = ( + database.get_default_catalog() + if any(not saved_query.catalog for saved_query in saved_queries) + else None + ) + for saved_query in saved_queries: + self._sync_saved_query_chart_perms( + connection, saved_query, database, default_catalog + ) + def _update_saved_query_chart_perms( self, connection: Connection, @@ -5013,14 +5068,7 @@ def _update_saved_query_chart_perms( match a schema or catalog grant on an unrelated connection that later takes the old name. """ - # pylint: disable=import-outside-toplevel - from superset.models.sql_lab import SavedQuery - - saved_queries = ( - self.session.query(SavedQuery).filter(SavedQuery.db_id == target.id).all() - ) - for saved_query in saved_queries: - self._sync_saved_query_chart_perms(connection, saved_query, target) + self.sync_database_saved_query_chart_perms(connection, target) def saved_query_after_delete( self, diff --git a/tests/unit_tests/commands/chart/update_test.py b/tests/unit_tests/commands/chart/update_test.py index caa5af371b4f..116fc934ba63 100644 --- a/tests/unit_tests/commands/chart/update_test.py +++ b/tests/unit_tests/commands/chart/update_test.py @@ -145,7 +145,7 @@ def test_type_only_update_checks_retained_datasource( else: lookup.assert_called_once_with(42, datasource_type) if outcome in {"allowed", "denied"}: - access.assert_called_once_with(datasource=datasource) + access.assert_called_once_with(datasource=datasource, allow_editorship=True) def _editorship_exc() -> SupersetSecurityException: @@ -479,7 +479,9 @@ def test_update_chart_accepts_semantic_view_datasource( cmd.validate() get_datasource_by_id.assert_called_once_with(11, "semantic_view") - raise_for_access.assert_called_once_with(datasource=datasource) + raise_for_access.assert_called_once_with( + datasource=datasource, allow_editorship=True + ) assert cmd._properties["datasource_name"] == "my_semantic_view" @@ -529,7 +531,9 @@ def test_update_chart_repoints_to_saved_query_datasource( # The effective id is the newly-supplied 11, not the chart's stored 42. get_datasource_by_id.assert_called_once_with(11, "saved_query") - raise_for_access.assert_called_once_with(datasource=saved_query) + raise_for_access.assert_called_once_with( + datasource=saved_query, allow_editorship=False + ) assert cmd._properties["datasource_name"] == "My saved query" diff --git a/tests/unit_tests/models/sql_lab_test.py b/tests/unit_tests/models/sql_lab_test.py index 169e6a76633b..bea31af490c0 100644 --- a/tests/unit_tests/models/sql_lab_test.py +++ b/tests/unit_tests/models/sql_lab_test.py @@ -465,6 +465,51 @@ def test_database_rename_syncs_saved_query_chart_perms(app: Any) -> None: db.session.rollback() +def test_saved_query_resync_uses_default_catalog_and_scopes_to_one_query( + app: Any, mocker: Any +) -> None: + """A saved query with no ``catalog`` resolves both ``catalog_perm`` and + ``schema_perm`` against the database's default catalog, and resyncing one + saved query leaves another saved query's chart untouched.""" + from superset.extensions import db + from superset.models.core import Database + from superset.models.slice import Slice + from superset.utils.core import DatasourceType + + mocker.patch.object(Database, "get_default_catalog", return_value="main") + + database = Database(database_name="dc_db", sqlalchemy_uri="sqlite://") + moved = SavedQuery(label="Moved", schema="s1", database=database) + other = SavedQuery(label="Other", schema="s1", database=database) + db.session.add_all([database, moved, other]) + db.session.flush() + + charts = {} + for key, saved_query in (("moved", moved), ("other", other)): + charts[key] = Slice( + slice_name=f"On {key}", + datasource_type=DatasourceType.SAVEDQUERY, + datasource_id=saved_query.id, + datasource_name=saved_query.label, + viz_type="table", + params="{}", + ) + db.session.add_all(charts.values()) + db.session.flush() + + try: + assert charts["moved"].catalog_perm == "[dc_db].[main]" + moved.schema = "s2" + db.session.flush() + db.session.expire_all() + + assert charts["moved"].catalog_perm == "[dc_db].[main]" + assert charts["moved"].schema_perm == "[dc_db].[main].[s2]" + assert charts["other"].schema_perm == "[dc_db].[main].[s1]" + finally: + db.session.rollback() + + def test_saved_query_after_delete_detaches_dependent_slices(app: Any) -> None: """Deleting a saved query clears its charts' datasource reference and denormalized perm columns. From a45ee1b9d164277869b591771d144025e15bb482 Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Mon, 26 Oct 2026 17:25:43 -0700 Subject: [PATCH 4/5] fix(charts): keep raise_for_access call shape for non-saved-query datasources Only pass allow_editorship=False when binding a chart to a saved query; tables and semantic views go through the same raise_for_access(datasource=...) call as before. The extra kwarg broke semantic_save_tests' call assertion, and because that test aborted before cleanup, its leftover semantic_view chart cascaded into the reports, dashboards export, and change_records failures later in the integration run. Co-Authored-By: Evan Rusackas Co-Authored-By: Claude Opus 5.5 --- superset/commands/chart/utils.py | 10 ++++------ tests/unit_tests/commands/chart/update_test.py | 6 ++---- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/superset/commands/chart/utils.py b/superset/commands/chart/utils.py index 0f590b4476cc..533a49c4ceae 100644 --- a/superset/commands/chart/utils.py +++ b/superset/commands/chart/utils.py @@ -82,12 +82,10 @@ def raise_for_chart_datasource_access(datasource: Any) -> None: database, so editorship of one (being its creator) must not stand in for a database, catalog, or schema grant: require the real grant. """ - security_manager.raise_for_access( - datasource=datasource, - allow_editorship=( - getattr(datasource, "type", None) != DatasourceType.SAVEDQUERY - ), - ) + if getattr(datasource, "type", None) == DatasourceType.SAVEDQUERY: + security_manager.raise_for_access(datasource=datasource, allow_editorship=False) + return + security_manager.raise_for_access(datasource=datasource) def validate_query_context_datasource( diff --git a/tests/unit_tests/commands/chart/update_test.py b/tests/unit_tests/commands/chart/update_test.py index 116fc934ba63..36f697dea195 100644 --- a/tests/unit_tests/commands/chart/update_test.py +++ b/tests/unit_tests/commands/chart/update_test.py @@ -145,7 +145,7 @@ def test_type_only_update_checks_retained_datasource( else: lookup.assert_called_once_with(42, datasource_type) if outcome in {"allowed", "denied"}: - access.assert_called_once_with(datasource=datasource, allow_editorship=True) + access.assert_called_once_with(datasource=datasource) def _editorship_exc() -> SupersetSecurityException: @@ -479,9 +479,7 @@ def test_update_chart_accepts_semantic_view_datasource( cmd.validate() get_datasource_by_id.assert_called_once_with(11, "semantic_view") - raise_for_access.assert_called_once_with( - datasource=datasource, allow_editorship=True - ) + raise_for_access.assert_called_once_with(datasource=datasource) assert cmd._properties["datasource_name"] == "my_semantic_view" From 0053e08a5c1f13e078e3ec55426ea9f863a3084e Mon Sep 17 00:00:00 2001 From: Evan Rusackas Date: Wed, 28 Oct 2026 15:10:49 -0700 Subject: [PATCH 5/5] fix(charts): re-point saved_query migration onto the current alembic head Co-Authored-By: Evan Rusackas Co-Authored-By: Claude Sonnet 5.5 --- ...3_00-00_dfa683969f48_allow_saved_query_chart_datasource.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/superset/migrations/versions/2026-10-13_00-00_dfa683969f48_allow_saved_query_chart_datasource.py b/superset/migrations/versions/2026-10-13_00-00_dfa683969f48_allow_saved_query_chart_datasource.py index 447d77011adf..fb238e52ef63 100644 --- a/superset/migrations/versions/2026-10-13_00-00_dfa683969f48_allow_saved_query_chart_datasource.py +++ b/superset/migrations/versions/2026-10-13_00-00_dfa683969f48_allow_saved_query_chart_datasource.py @@ -17,7 +17,7 @@ """allow saved_query chart datasource Revision ID: dfa683969f48 -Revises: d623a0cb6bb0 +Revises: f72b6e9c104a Create Date: 2026-10-13 00:00:00.000000 """ @@ -32,7 +32,7 @@ # revision identifiers, used by Alembic. revision = "dfa683969f48" -down_revision = "d623a0cb6bb0" +down_revision = "f72b6e9c104a" Base = declarative_base()