sadpandajoe commented on code in PR #45266:
URL: https://github.com/apache/superset/pull/45266#discussion_r4294196890
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -248,23 +248,78 @@ def validate_catalog(config: dict[str, Any]) -> None:
raise MultiCatalogDisabledValidationError()
-def _get_template_params(dataset: SqlaTable) -> dict[str, Any]:
+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 {}
+
+
+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``.
"""
- Return a dataset's template params as a dict for the access check.
+ 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
- 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.
- """
- try:
- params = dataset.template_params_dict
- except json.JSONDecodeError:
- logger.warning(
- "Unable to decode template_params for dataset %s",
dataset.table_name
+ requested_sql = config.get("sql") 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 existing is not None and (
Review Comment:
The unchanged-binding skip compares only sql/table/schema/catalog, but the
rendered query also depends on `template_params`. Re-importing a dataset whose
SQL is `SELECT * FROM {{ target }}` with only `template_params.target` changed
from `public.orders` to `private.secrets` returns early here, so the new table
is never authorized, `import_from_dict` stores the params, and the editor can
then read `private.secrets` through chart-data. The old post-import check
validated the resulting SQL on every import.
The helper also reads the request in isolation from the stored row: with
`template_params` omitted it authorizes against `{}` while the stored params
are kept, and with `sql` omitted it treats an existing virtual dataset as a
physical table named `table_name` (so a description-only re-import of a virtual
dataset is denied). Could the requested binding be resolved by merging the
config over the existing row (sql, template_params, schema, catalog), with
`template_params` included in the unchanged comparison?
##########
superset/common/query_context_processor.py:
##########
@@ -1031,7 +1031,18 @@ def raise_for_access(self) -> None:
# caller is not allowed to access.
if self._qc_datasource.type == DatasourceType.QUERY:
try:
- security_manager.raise_for_access(query=self._qc_datasource)
+ # Query-backed chart-data runs the stored query's raw SQL, so
+ # editorship of a same-named dataset must not authorize its
+ # tables -- otherwise a no-table decoy dataset named after a
+ # table would let its editor read that table through a
+ # persisted query. ``allow_editorship=False`` stops honoring
+ # editorship while keeping the regular catalog/schema/
+ # datasource_access checks this ongoing-fetch path has always
+ # used (it is deliberately not the stricter
+ # ``force_dataset_match`` that SQL Lab execution uses).
+ security_manager.raise_for_access(
+ query=self._qc_datasource, allow_editorship=False
Review Comment:
The added wiring tests close the query-backed chart-data, physical create,
and physical import paths, but several call sites would still pass if
`allow_editorship=False` were dropped: the virtual-create SQL check
(`create.py:228`; the new test only creates a physical dataset), the
database-only repoint and SQL-replacement calls in `update.py` (the existing
tests use mocks that ignore arguments), the virtual-import `sql=` branch, and
the `datasource=` branch in `raise_for_access` (every new manager test passes
`query=`). Could each get an assertion on `allow_editorship=False`, plus an
editor-only denial on the `datasource=` branch?
--
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]
