sadpandajoe commented on code in PR #43499:
URL: https://github.com/apache/superset/pull/43499#discussion_r4344718672
##########
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:
A saved query with `catalog` unset on a database that has a default catalog
gets a chart `schema_perm` built from the default catalog, but `catalog_perm`
here is built from the raw `catalog`, so it is NULL. A non-admin granted
`catalog_access` on that default catalog then never sees a no-viewer chart in
list/get, and `can_access_schema` on the raw `catalog` misses the same grant on
direct access. Could `catalog_perm` fall back to `get_default_catalog()` the
same way `schema_perm` now does, here and in `_saved_query_chart_perm_values`?
##########
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(),
Review Comment:
The default-catalog fallback on this line is not pinned by the resync tests:
their databases either return no default catalog or their saved queries set a
`catalog`, so deleting `or database.get_default_catalog()` still passes. Could
one case in `tests/unit_tests/models/sql_lab_test.py` use a saved query with
`catalog=None` on a database whose default catalog is non-empty, move its
schema, and assert the dependent chart's fully qualified `schema_perm`? The
same tests also only create one saved query, so a second saved query's chart
(asserted unchanged) would cover the `datasource_id` predicates here and in
`saved_query_after_delete`.
--
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]
