EnxDev commented on code in PR #45342:
URL: https://github.com/apache/superset/pull/45342#discussion_r4288284089
##########
superset/commands/dataset/update.py:
##########
@@ -283,6 +291,7 @@ def _validate_sql_access(
sql=sql,
catalog=catalog,
schema=schema,
+ allow_dataset_editorship=False,
Review Comment:
Confirmed, and fixed in 2f33d895be. `_validate_sql_access` now runs when the
SQL text changes or when the SQL is kept but the connection, catalog, schema or
`template_params` change. It checks the effective SQL and passes the effective
template params, so Jinja SQL is checked against the table it actually renders
to. Empty values compare as `None`, so a client echoing `""` for an unset field
doesn't trigger a check and #20382 editors can still edit metadata.
The legacy `/datasource/save/` path compared schema and catalog but not
`template_params`, so I added it there too and pass the parsed params to the
check.
Tests: schema-only and template-params-only updates on a virtual dataset now
run the check with editorship off (both fail without the change), and a
metadata-only update or one that echoes the stored values skips it. The legacy
save tests got a template-params case.
##########
superset/common/query_context_processor.py:
##########
@@ -1031,7 +1031,10 @@ 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)
+ security_manager.raise_for_access(
+ query=self._qc_datasource,
+ allow_dataset_editorship=False,
Review Comment:
Yes, fixed in 2f33d895be. `GetExploreCommand` and `check_query_access` both
pass `allow_dataset_editorship=False`. The author bypass returns before the
table loop, so the query's own author still gets through. Everyone else now
needs real data access on the tables the query reads. Added a test where a
non-author is the editor of a same-named dataset and gets denied. It fails
without the change.
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -585,6 +605,22 @@ def import_dataset( # noqa: C901
if not ignore_permissions:
try:
security_manager.raise_for_access(datasource=dataset)
+ # The datasource check accepts editorship of the dataset itself,
+ # and an overwrite or restore only requires editorship of the row
+ # being replaced. Without this check an editor could re-import a
+ # ``SELECT 1`` decoy as a physical dataset over an arbitrary
+ # table. A physical dataset that is new or bound to a different
+ # table must be authorized by data access on that table.
+ if not dataset.sql and _table_binding(dataset) != previous_binding:
+ security_manager.raise_for_access(
+ database=dataset.database,
Review Comment:
I tried to reproduce this one and it doesn't get as far as the check.
`import_dataset` calls `SqlaTable.import_from_dict` without
`allow_reparenting`, so the lookup filters on `database_id == <new id>` as well
as the uuid/unique-constraint match. When the config points at a different
database it never matches the existing row. It builds a new `SqlaTable` with
the same uuid, and the `if dataset.id is None: db.session.flush()` just above
this block fails on `uq_tables_uuid` before any `raise_for_access` runs. So an
overwrite can't move a dataset to another connection, and `dataset.database`
can't be stale here.
I wrote the test you suggested (existing row with `database` loaded, then an
overwrite with a new `database_uuid`, for both physical and virtual). It fails
with `IntegrityError: UNIQUE constraint failed: tables.uuid` with or without
resolving the database by id, so I left the code as is. If you'd still like the
check to resolve `Database` from `database_id` as a guard in case reparenting
is ever allowed, I can add it.
--
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]
