sadpandajoe commented on code in PR #45266:
URL: https://github.com/apache/superset/pull/45266#discussion_r4295800308
##########
tests/unit_tests/datasets/commands/importers/v1/import_test.py:
##########
@@ -748,6 +748,177 @@ def raise_for_access(**kwargs: object) -> None:
import_dataset(config)
+def test_import_dataset_repoint_authorizes_requested_binding_pre_mutation(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ Regression guard (schema-swap): an overwrite that repoints a dataset to a
+ new schema must authorize the REQUESTED binding with allow_editorship=False
+ and do so BEFORE the row is mutated -- otherwise the per-table lookup
+ resolves the dataset itself (whose perm omits the schema) and a
+ datasource_access holder could repoint it to a schema they can't read.
+ """
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+ database = Database(database_name="my_database",
sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+ ds_uuid = str(uuid.uuid4())
+ existing = SqlaTable(
+ table_name="orders",
+ schema="public",
+ catalog=None,
+ database=database,
+ uuid=ds_uuid,
+ )
+ db.session.add(existing)
+ db.session.flush()
+
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ seen: dict[str, Any] = {}
+
+ def record(**kwargs: Any) -> None:
+ if kwargs.get("table") is not None or kwargs.get("sql") is not None:
+ seen["kwargs"] = kwargs
+ seen["stored_schema"] = db.session.get(SqlaTable,
existing.id).schema
+
+ mocker.patch.object(security_manager, "raise_for_access",
side_effect=record)
+
+ config = copy.deepcopy(dataset_fixture)
+ config.update(
+ {
+ "uuid": ds_uuid,
+ "table_name": "orders",
+ "schema": "private",
+ "catalog": None,
+ "sql": "",
+ "database_id": database.id,
+ "metrics": [],
+ "columns": [],
+ }
+ )
+ import_dataset(config, overwrite=True)
+
+ assert seen["kwargs"].get("allow_editorship") is False
+ assert seen["kwargs"]["table"].schema == "private" # requested, not stored
+ assert seen["stored_schema"] == "public" # check ran before
import_from_dict
+
+
+def test_import_dataset_metadata_only_skips_binding_check(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ Regression guard (over-restriction): a metadata-only re-import (unchanged
+ table binding) must NOT run the table/SQL access check, so an editor
+ without separate data access can still re-import non-binding changes --
+ matching UpdateDatasetCommand, which only revalidates on a source change.
+ """
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+ database = Database(database_name="my_database",
sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+ ds_uuid = str(uuid.uuid4())
+ existing = SqlaTable(
+ table_name="orders",
+ schema="public",
+ catalog=None,
+ database=database,
+ uuid=ds_uuid,
+ description="old",
+ )
+ db.session.add(existing)
+ db.session.flush()
+
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "is_editor", return_value=True)
+ binding_checks: list[dict[str, Any]] = []
+
+ def record(**kwargs: Any) -> None:
+ if kwargs.get("table") is not None or kwargs.get("sql") is not None:
+ binding_checks.append(kwargs)
+
+ mocker.patch.object(security_manager, "raise_for_access",
side_effect=record)
+
+ config = copy.deepcopy(dataset_fixture)
+ config.update(
+ {
+ "uuid": ds_uuid,
+ "table_name": "orders",
+ "schema": "public",
+ "catalog": None,
+ "sql": "",
+ "database_id": database.id,
+ "description": "new description",
+ "metrics": [],
+ "columns": [],
+ }
+ )
+ import_dataset(config, overwrite=True)
+
+ assert binding_checks == [] # unchanged binding -> no table/SQL access
check
+
+
+def test_import_dataset_database_repoint_runs_binding_check(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ Regression guard: an overwrite that keeps table/schema/catalog/sql but
+ points at a DIFFERENT database still repoints the dataset, so the binding
+ check must run against the new database with allow_editorship=False -- the
+ database is part of the binding (matching UpdateDatasetCommand's repoint).
+ """
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+ db_a = Database(database_name="db_a", sqlalchemy_uri="sqlite://")
+ db_b = Database(database_name="db_b", sqlalchemy_uri="sqlite://")
+ db.session.add_all([db_a, db_b])
+ db.session.flush()
+ ds_uuid = str(uuid.uuid4())
+ existing = SqlaTable(
+ table_name="orders",
+ schema="public",
+ catalog=None,
+ database=db_a,
+ uuid=ds_uuid,
+ )
+ db.session.add(existing)
+ db.session.flush()
+
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ mocker.patch.object(security_manager, "is_editor", return_value=True)
+ # The access check runs before import_from_dict; stub the mutation so the
+ # test isolates whether the binding check ran (a real DB-identity change
+ # drives import_from_dict down an insert path that is orthogonal here).
+ mocker.patch.object(SqlaTable, "import_from_dict", return_value=existing)
+ binding_checks: list[dict[str, Any]] = []
+
+ def record(**kwargs: Any) -> None:
+ if kwargs.get("table") is not None or kwargs.get("sql") is not None:
+ binding_checks.append(kwargs)
+
+ mocker.patch.object(security_manager, "raise_for_access",
side_effect=record)
+
+ config = copy.deepcopy(dataset_fixture)
+ config.update(
+ {
+ "uuid": ds_uuid,
+ "table_name": "orders",
+ "schema": "public",
+ "catalog": None,
+ "sql": "",
+ "database_id": db_b.id, # repoint to a different database
+ "metrics": [],
+ "columns": [],
+ }
+ )
+ import_dataset(config, overwrite=True)
+
+ assert len(binding_checks) == 1
+ assert binding_checks[0]["table"].table == "orders"
+ assert binding_checks[0].get("allow_editorship") is False
Review Comment:
This guards that the database change triggers a check, but it would still
pass if the check authorized the existing database (`db_a`) instead of the
requested `db_b`: it asserts only the call count, the table name and the flag,
and both `import_from_dict` and `raise_for_access` are stubbed. Could it assert
`binding_checks[0]["database"] is db_b`?
Related: `test_import_dataset_access_check` further down raises from every
`raise_for_access` call, so it now fails inside the new pre-mutation binding
check and never reaches `raise_for_access(datasource=dataset)`. Deleting that
dataset-level gate would leave it green. A case that allows the
database/table/SQL calls and denies only the `datasource=` call would pin it.
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -248,23 +248,85 @@ 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
Review Comment:
When `sql` is omitted from the uploaded config this reads it as "no SQL",
but `import_from_dict` only applies the keys that are present, so the row keeps
its stored SQL. The check therefore authorizes a binding that is not the one
that ends up saved.
Example: an editor owns a virtual dataset whose SQL reads `private.orders`,
and holds `schema_access` only on `public` (same catalog). They
overwrite-import the same UUID with `schema: public` and no `sql` key. This
takes the physical-table branch, authorizes `public.<table_name>`, and the
stored SQL then runs with `public` as its default schema. In the other
direction, a metadata-only re-import of a virtual dataset that omits `sql` gets
a physical-table check on its own name and is denied, even though the binding
is unchanged.
Should an absent `sql` key resolve to `existing.sql` here (keeping an
explicit null/empty as a clear), so the check matches what is actually stored?
##########
superset/commands/dataset/update.py:
##########
@@ -283,6 +287,7 @@ def _validate_sql_access(
sql=sql,
catalog=catalog,
schema=schema,
+ allow_editorship=False,
Review Comment:
This SQL check only runs when the request carries SQL text that differs from
the stored text (the `sql and sql != self._model.sql` guard just above), and
`_validate_table_access` is skipped once the dataset has SQL. A `PUT
/api/v1/dataset/<id>` that changes only `schema` or `catalog` (or `database_id`
plus a schema the user holds) on a virtual dataset therefore re-checks nothing.
Example: the editor owns a virtual dataset with SQL `SELECT * FROM t` under
a schema they can read, sends `{"schema": "secret"}`, and chart-data then
resolves `t` against `secret`, where editorship is still accepted for the
datasource. The legacy `/datasource/save/` path now covers this through
`_repoints_sql`, and its docstring says this command "does not yet cover" it,
so the REST path the UI uses is left behind.
Should the SQL check also run whenever the dataset has SQL and the
connection, catalog or schema changes, using the effective SQL?
--
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]
