sadpandajoe commented on code in PR #45266:
URL: https://github.com/apache/superset/pull/45266#discussion_r4297388961
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -248,25 +248,112 @@ def validate_catalog(config: dict[str, Any]) -> None:
raise MultiCatalogDisabledValidationError()
-def _get_template_params(dataset: SqlaTable) -> dict[str, Any]:
- """
- Return a dataset's template params as a dict for the access check.
+def _config_template_params(config: dict[str, Any]) -> dict[str, Any]:
+ """Read an import config's template params as a dict for the access
check."""
+ raw = config.get("template_params")
+ if isinstance(raw, dict):
+ return raw
+ if isinstance(raw, str) and raw:
+ try:
+ parsed = json.loads(raw)
+ except json.JSONDecodeError:
+ return {}
+ return parsed if isinstance(parsed, dict) else {}
+ return {}
- Delegates to ``template_params_dict`` so the check reads the params the
- same way rendering does, but tolerates the two inputs that property does
- not guard: it raises on malformed JSON and passes non-dict JSON through.
- Either yields an empty dict so the check still runs against the raw SQL.
- """
+
+def _existing_template_params(dataset: SqlaTable) -> dict[str, Any]:
+ """A stored dataset's template params as a dict, tolerating bad JSON."""
try:
params = dataset.template_params_dict
except json.JSONDecodeError:
- logger.warning(
- "Unable to decode template_params for dataset %s",
dataset.table_name
- )
return {}
return params if isinstance(params, dict) else {}
+def _raise_for_import_binding_access(
+ config: dict[str, Any], existing: SqlaTable | None
+) -> None:
+ """Authorize the table binding a dataset import will apply.
+
+ Runs BEFORE ``import_from_dict`` mutates the row, against the *requested*
+ (``config``) binding rather than the stored dataset: the per-table lookup
+ would otherwise resolve the dataset being imported itself (whose ``perm``
+ is schema-blind), letting a ``datasource_access`` holder repoint it to
+ another schema and read it through chart-data. ``allow_editorship=False``
+ means editorship of a same-named dataset is not accepted -- defining or
+ repointing a dataset over a table requires real data access to that
+ table. Skipped when the binding is unchanged (a metadata-only re-import is
+ not a repoint), matching ``UpdateDatasetCommand``.
+ """
+ database =
db.session.query(Database).filter_by(id=config["database_id"]).first()
+ if database is None:
+ # A missing database is reported downstream by import_from_dict; there
+ # is no table binding to authorize here.
+ return
+
+ # Resolve the requested binding by MERGING the config over the stored row:
+ # an overwrite only applies the keys the upload carries, so an omitted
+ # field keeps its stored value. Resolving in isolation would mis-authorize
+ # (e.g. omitted ``sql`` -> treat a virtual dataset as physical; omitted
+ # ``template_params`` -> render against ``{}`` while the stored params are
+ # kept).
+ requested_sql = (
+ config["sql"] if "sql" in config else (existing.sql if existing else
None)
+ ) or None
+ requested_table_name = config.get("table_name") or (
+ existing.table_name if existing else None
+ )
+ requested_schema = config.get("schema", existing.schema if existing else
None)
+ requested_catalog = config.get("catalog", existing.catalog if existing
else None)
+ if "template_params" in config:
+ requested_params = _config_template_params(config)
+ elif existing is not None:
+ requested_params = _existing_template_params(existing)
+ else:
+ requested_params = {}
+
+ if existing is not None and (
+ config["database_id"] == existing.database_id
+ and requested_sql == (existing.sql or None)
+ and requested_table_name == existing.table_name
Review Comment:
For a virtual dataset `table_name` is only a display label, but it is part
of the unchanged-binding comparison here. An editor who exports a virtual
dataset, changes only its name, and re-imports it with `overwrite` now falls
through to `raise_for_access(sql=..., allow_editorship=False)` and gets
`DatasetAccessDeniedError` unless they also hold a grant on the underlying
tables, while the same rename through `PUT /api/v1/dataset/<id>` or
`/datasource/save/` is allowed. Should `table_name` be ignored in this
comparison when the requested and stored datasets are both virtual?
##########
superset/views/datasource/views.py:
##########
@@ -142,10 +145,14 @@ def _repoints_sql(
or requested_sql != datasource.sql
or (requested_table.schema, requested_table.catalog)
!= (datasource.schema or None, datasource.catalog or None)
+ or json_to_dict(requested_template_params or "")
Review Comment:
I don't see a test for this new `template_params` comparison. The existing
`views_test.py` save cases change SQL, schema or database and only assert
`template_params={}`, so deleting these two lines would leave them green. Could
you add a case that saves a virtual dataset with `SELECT * FROM {{ t }}` and
unchanged SQL/database/schema/catalog but a different `t`, with the SQL access
check denying, and assert `DatasetForbiddenError`, that `raise_for_access` got
the new params with `allow_editorship=False`, and that `update_from_object` was
not called?
##########
superset/commands/dataset/create.py:
##########
@@ -103,6 +103,11 @@ def validate(self) -> None: # noqa: C901
schema = self._properties.get("schema")
table_name = self._properties["table_name"]
sql = self._properties.get("sql")
+ # The access check below must render the SQL with the same template
+ # params that will be stored and executed, so a templated table
+ # reference is authorized as the table it actually resolves to rather
+ # than its ``default()``.
+ template_params = json_to_dict(self._properties.get("template_params")
or "")
Review Comment:
Moving `json_to_dict` out of the guarded block means a malformed
`template_params` string now raises `JSONDecodeError` straight out of
`validate()`. `DatasetPostSchema.template_params` is a plain string, and
`on_error` here only handles `SQLAlchemyError`, so `POST /api/v1/dataset/` with
`template_params: "{bad"` would come back as an unhandled 500 (including for
physical datasets with no `sql`, which never parsed the params before). Failing
closed is fine, but should this be turned into a validation error instead?
##########
superset/security/manager.py:
##########
@@ -5376,10 +5397,37 @@ def raise_for_access( # noqa: C901
catalog=effective_catalog,
)
for datasource_ in datasources:
+ # ``datasource_access`` on a registered dataset is always
+ # sufficient. Editorship of the dataset is accepted only on
+ # the non-strict path (chart-data, ``/table_metadata/``,
+ # ``/select_star/``), where it is the dataset-owner
+ # "explore your own dataset" grant from #20382.
+ #
+ # It is excluded on two kinds of call:
+ # * strict scoping (``force_dataset_match``) -- SQL Lab
+ # raw queries, MetaDB, MCP ``execute_sql`` and
+ # alert/report queries -- which return raw rows and
+ # also drop the catalog/schema fallthroughs;
+ # * ``allow_editorship=False`` -- the paths that define
+ # or repoint a dataset over a table (create, update, v1
+ # import, and the legacy ``/datasource/save/`` repoint)
+ # and query-backed chart-data (which runs a stored
+ # query's raw SQL) -- which keep the catalog/schema
+ # fallthroughs but refuse editorship.
+ # Both must follow the user's live data-access grants, not
+ # editorship of a same-named dataset: any principal able to
+ # create a dataset can make themselves an editor of one
+ # named after an arbitrary table, so accepting editorship
+ # here would let dataset creation stand in for data access
+ # and would outlive revocation of every real grant.
if self.can_access(
"datasource_access",
datasource_.perm or "",
- ) or self.is_editor(datasource_):
+ ) or (
+ not force_dataset_match
+ and allow_editorship
Review Comment:
With `allow_editorship=False` the guard above that refuses an unresolved
schema (`force_dataset_match and not table_.schema`) is still off, so
`query_datasources_by_name(schema=None)` matches a same-named dataset in any
schema. Example: a role with dataset write and `datasource_access` only on
`public.orders` creates a virtual dataset with `sql: "SELECT * FROM orders"`
and no schema on a MySQL connection whose default database is `private`. The
lookup finds `public.orders` and passes, but the dataset then executes against
`private.orders`. Since create/update/import now rely on this flag to require
real access to the referenced table, should an unresolved schema be denied (or
resolved the way execution resolves it) in this mode too?
--
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]
