geido commented on code in PR #45266:
URL: https://github.com/apache/superset/pull/45266#discussion_r4294164795
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -600,6 +605,20 @@ def import_dataset( # noqa: C901
# references once the params are applied, so they are
# forwarded rather than left to default to empty.
template_params=_get_template_params(dataset),
+ allow_editorship=False,
+ )
+ else:
+ # Physical dataset: validate the bound table itself, so an
+ # overwrite that converts a decoy virtual dataset into a
+ # physical one cannot bind to a table on editorship alone.
+ security_manager.raise_for_access(
+ database=dataset.database,
+ table=Table(
+ dataset.table_name,
+ dataset.schema or None,
+ dataset.catalog or None,
+ ),
+ allow_editorship=False,
Review Comment:
Confirmed and fixed in cfbeef9 — this was the sharp one. Root cause exactly
as you traced: the check ran *after* `import_from_dict()` mutated the row, and
the dataset `perm` is schema-blind (`[db].[table](id:N)`), so the by-name
lookup resolved the dataset itself and its id-perm passed the schema swap.
The binding check now runs **before** `import_from_dict`, built from the
requested `config` values (`allow_editorship=False`), and only when the binding
actually changes — so the stored row is still at its old schema and can't
self-match. Verified driving the real `import_dataset()`: a `datasource_access`
holder on `public.orders` re-importing with `schema: private` is now **denied**
(`DatasetAccessDeniedError`) and the dataset is **not** repointed (schema stays
`public`). The virtual `sql=` variant goes through the same pre-mutation path.
This mirrors how update/create/legacy-save already resolve the *requested*
binding before mutating. Regression test:
`test_import_dataset_repoint_authorizes_requested_binding_pre_mutation`
(asserts the stored schema is still `public` at check time).
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -600,6 +605,20 @@ def import_dataset( # noqa: C901
# references once the params are applied, so they are
# forwarded rather than left to default to empty.
template_params=_get_template_params(dataset),
+ allow_editorship=False,
+ )
+ else:
+ # Physical dataset: validate the bound table itself, so an
+ # overwrite that converts a decoy virtual dataset into a
+ # physical one cannot bind to a table on editorship alone.
+ security_manager.raise_for_access(
Review Comment:
Right, and fixed in cfbeef9. The import check now mirrors
`UpdateDatasetCommand`: it compares the requested binding against the existing
row and runs the table/SQL check **only when the binding changes**
(table/schema/catalog/sql), so a metadata-only re-import no longer runs it.
Verified on the real `import_dataset()`: an editor with no data access
re-importing with only the `description` changed now **succeeds**. Regression
test: `test_import_dataset_metadata_only_skips_binding_check`. (Same commit
moved the check pre-mutation, which is what makes the change-comparison against
the stored row correct — see the schema-swap thread.)
##########
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:
Added in cfbeef9. `query_context_processor`:
`test_raise_for_access_query_datasource_disallows_editorship` asserts the
QUERY-path call is made with `allow_editorship=False` and that a
`SupersetSecurityException` stops `query.validate()`. `create`:
`test_create_dataset_passes_allow_editorship_false`. `import`: the two tests
above assert the wiring on both the physical (`table=`) and `sql=` branches
(flag + pre-mutation binding). So dropping the keyword from any of those sites
now fails a test. (Also honored `allow_editorship` in the
`datasource`/`query_context` branch so the flag isn't silently
branch-dependent.)
--
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]
