sadpandajoe commented on code in PR #43499:
URL: https://github.com/apache/superset/pull/43499#discussion_r4372411855
##########
superset/commands/chart/utils.py:
##########
@@ -36,18 +38,58 @@
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 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(
Review Comment:
With editorship now off for saved queries, an owner whose only grant is
`catalog_access` on the database's default catalog gets a 403 binding a chart
to their own saved query that has `catalog` unset. `can_access_schema` checks
the raw `datasource.catalog`, which is `None` here, so the catalog grant is
skipped, even though `catalog_perm` and `schema_perm` on the same saved query
now resolve against the default catalog. Should the access check resolve the
effective catalog the same way, so the grant that the chart's own perm strings
match is honored?
##########
superset/commands/database/update.py:
##########
@@ -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):
Review Comment:
Nothing exercises this resync: `test_update_with_catalog_change` mocks
`_update_catalog_attribute` entirely, and the model tests drive the ORM
listeners, so deleting this call leaves the suite green. Could a test persist a
saved-query chart, run the real `_update_catalog_attribute` from catalog A to
B, and assert after expiring the session that the chart's `catalog_perm` and
`schema_perm` both carry B?
--
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]
