rusackas commented on code in PR #43499:
URL: https://github.com/apache/superset/pull/43499#discussion_r4372256740
##########
superset/commands/chart/create.py:
##########
@@ -78,6 +79,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)
Review Comment:
@sadpandajoe good catch. Chart create/update now call `raise_for_access`
with `allow_editorship=False` for saved queries, so a real
database/catalog/schema grant is required. Tables keep the existing behavior.
##########
superset/security/manager.py:
##########
@@ -3763,6 +3763,11 @@ def database_after_update(
self._update_vm_datasources_access(
mapper, connection, old_database_name, 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)
Review Comment:
@sadpandajoe done. `UpdateDatabaseCommand._update_catalog_attribute` now
re-syncs the dependent charts perms after the bulk catalog update.
##########
superset/security/manager.py:
##########
@@ -4682,6 +4687,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 = (
Review Comment:
@sadpandajoe done. The rename path now only loads saved queries that have
charts and resolves the default catalog once.
##########
superset/models/sql_lab.py:
##########
@@ -602,6 +605,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(
Review Comment:
@sadpandajoe done. `catalog_perm` now falls back to the default catalog,
both on `SavedQuery` and in the chart resync.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
