geido commented on code in PR #45266:
URL: https://github.com/apache/superset/pull/45266#discussion_r4308641369
##########
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:
Covered in 4b47324. Each call site you listed now has an assertion that
fails if `allow_editorship=False` is dropped:
- virtual create: `test_create_dataset_passes_template_params` (the `sql=`
check)
- update: exact-argument assertions in
`test_update_dataset_sql_authorized_schema` (SQL replacement) and
`test_update_dataset_database_id_change_checks_new_database_access` /
`test_update_dataset_database_id_change_allowed_with_access` (database-only
repoint)
- virtual import `sql=` branch:
`test_import_dataset_template_params_only_change_runs_binding_check`, plus the
new binding-change and omitted-field import tests
- `datasource=` branch:
`test_raise_for_access_datasource_branch_honors_allow_editorship`, where an
editor with no schema/datasource grant is refused with `allow_editorship=False`
and keeps the default accept.
I also ran a mutation pass that flips each of these flags and removes each
new guard; every mutant now fails at least one test.
##########
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:
Fixed in 829fbc5 and tightened in 4b47324. The helper merges the upload over
the stored row, so an omitted `sql`, `schema`, `catalog` or `template_params`
keeps its stored value, and `template_params` is part of the unchanged-binding
comparison, so a params-only overwrite is re-checked against the new params.
Params are compared and rendered as execution will read them: config params go
through the same `json.dumps` → `json_to_dict` round trip as storage, and
anything that isn't a JSON object fails the import as a bad payload. Tests:
`test_import_dataset_template_params_only_change_runs_binding_check`,
`test_import_dataset_omitted_fields_keep_stored_values`,
`test_import_dataset_params_checked_as_execution_reads_them`.
##########
superset/commands/dataset/create.py:
##########
@@ -225,6 +225,7 @@ def validate(self) -> None: # noqa: C901
sql=sql,
catalog=catalog,
schema=schema,
+ allow_editorship=False,
Review Comment:
Fixed in 829fbc5: create, update and `/datasource/save/` render the SQL with
the params that will be stored (for update, the effective post-update params,
so a params-only change is re-checked too), all with `allow_editorship=False`.
Tests: `test_create_dataset_passes_template_params`,
`test_update_dataset_template_params_only_change_is_authorized`,
`test_save_rejects_template_params_repoint_without_access`.
##########
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:
Fixed in 829fbc5 (the helper resolves the requested binding by merging the
upload over the stored row), with a test in 4b47324:
`test_import_dataset_omitted_sql_authorizes_stored_sql_at_requested_schema`
overwrites a virtual dataset with `schema: public` and no `sql` key, and
asserts the stored SQL is checked on the `sql=` branch at the requested schema.
A metadata-only re-import that omits `sql` compares equal and skips the check.
##########
superset/commands/dataset/update.py:
##########
@@ -283,6 +287,7 @@ def _validate_sql_access(
sql=sql,
catalog=catalog,
schema=schema,
+ allow_editorship=False,
Review Comment:
Fixed in 4b47324. `_validate_sql_access` now also runs when the connection,
catalog or schema changes, not only the SQL or params, and checks the stored
SQL against the effective post-update values. The change is detected before
catalog normalization, with `""` and `None` treated alike, so an editor's
ordinary save isn't treated as a move. `table_name` alone is still a label for
virtual datasets, as in `/datasource/save/`. Tests:
`test_update_virtual_dataset_location_change_rechecks_sql` (schema, catalog,
database), `test_update_virtual_dataset_metadata_change_skips_access_check`,
`test_update_dataset_empty_schema_round_trip_is_not_a_move`.
##########
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:
Both done in 4b47324:
`test_import_dataset_database_repoint_runs_binding_check` now asserts
`binding_checks[0]["database"] is db_b`, and `test_import_dataset_access_check`
allows the table/SQL checks and denies only `raise_for_access(datasource=...)`,
asserting that call happened for the imported dataset, so removing the
dataset-level gate now fails it.
##########
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:
Good catch, fixed in 4b47324. Params are parsed only when the dataset has
SQL (a physical dataset's params are left unparsed, as before), with the same
parser execution uses, and a value that isn't a JSON object fails validation on
`template_params` (422) instead of escaping `validate()`. Update,
`/datasource/save/` and import handle it the same way, and an error rendering
the templated SQL during the check now maps to a 422 on `sql` rather than a
500. Tests: `test_create_dataset_unreadable_template_params_rejected`,
`test_create_physical_dataset_template_params_not_parsed`,
`test_update_dataset_unreadable_template_params_rejected`,
`test_save_rejects_unreadable_template_params`.
##########
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:
Agreed, fixed in 4b47324: when the requested and stored datasets are both
virtual, `table_name` is left out of the unchanged-binding comparison, matching
PUT and `/datasource/save/`. It still counts for physical datasets, where it is
the binding. Tests: `test_import_dataset_virtual_rename_skips_binding_check`,
`test_import_dataset_physical_rename_runs_binding_check`.
##########
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:
This one is out of scope for this PR. It predates the change and doesn't
involve editorship: on master the same create passes with `datasource_access`
on `public.orders` alone, because the non-strict lookup drops the schema filter
when the reference's schema is unresolved. I've kept this PR to editorship
standing in for data access and left that lookup as is; the unresolved-schema
matching is tracked separately.
--
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]
