sadpandajoe commented on code in PR #45266:
URL: https://github.com/apache/superset/pull/45266#discussion_r4290795250
##########
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:
This new check runs on every non-virtual import, including a metadata-only
overwrite where the binding is unchanged. A dataset editor with import/write
permission but no separate datasource/schema/database grant can explore and
update that dataset today, and would now get `DatasetAccessDeniedError` after
exporting it, editing only the description, and re-importing. `update.py` only
revalidates when the table, database, or SQL actually changes. Should import
also compare against the pre-import binding and run this check only when it
changed?
##########
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:
`import_from_dict()` has already applied the uploaded binding by the time
this runs, so the by-name lookup resolves the just-mutated dataset itself.
Dataset perms omit the schema (`[db].[table](id:N)`), so a user with
`datasource_access` on `public.orders` could upload YAML for that dataset with
`schema: private` and pass this check, then read `private.orders` through
chart-data. Should the target binding be authorized against the pre-import
state, or the check be made independent of the dataset being modified?
##########
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:
This flips authorization for every query-backed chart-data fetch, but I
don't see a test that would notice if `allow_editorship=False` were dropped:
the existing QUERY-path test only injects a `TemplateSyntaxError`, independent
of arguments. Could a test at `QueryContextProcessor.raise_for_access()` with a
persisted Query assert the call is made with `allow_editorship=False` and that
a `SupersetSecurityException` stops `query.validate()`? The same goes for the
new import and create call sites, none of which assert the flag.
--
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]
