geido commented on code in PR #45266:
URL: https://github.com/apache/superset/pull/45266#discussion_r4288481477
##########
superset/commands/dataset/update.py:
##########
@@ -260,7 +262,9 @@ def _validate_table_access(
self, db: Database, table: Table, exceptions: list[ValidationError]
) -> None:
try:
- security_manager.raise_for_access(database=db, table=table)
+ security_manager.raise_for_access(
+ database=db, table=table, allow_editorship=False
Review Comment:
Good catch — you're right, those exact-args assertions would have failed CI.
Fixed in 8a24cc5: `update_test.py` (the `assert_called_once_with(database=...,
table=Table(...))`) and `views_test.py` (both the single SQL
`assert_called_once_with` ~660 and the `call_args_list` at ~695) now include
`allow_editorship=False`. Ran all three locally — `update_test.py` 32 passed,
`views_test.py` 45 passed. As you note, this also gives the
create/update/repoint call-site wiring its first coverage: dropping the keyword
from those sites would now fail these assertions.
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -600,6 +600,12 @@ 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),
+ # Editorship of a same-named dataset must not authorize
+ # the referenced tables here, matching the create/update
+ # commands: otherwise importing (or overwriting) a virtual
+ # dataset whose SQL reads a table the importer only edits a
+ # decoy for would stand in for real data access.
+ allow_editorship=False,
Review Comment:
Right — the physical-overwrite path was open. The importer only ran the
`sql=` check (virtual); re-importing a decoy's UUID with `overwrite` and no SQL
converts it to a **physical** dataset on the real table, authorized by the
`raise_for_access(datasource=dataset)` editorship accept alone. Closed in
8a24cc5: the `else` branch now runs
`raise_for_access(database=dataset.database, table=Table(table_name, schema,
catalog), allow_editorship=False)`, matching the update command. Verified: 68
importer unit tests pass, and the `table=` + `allow_editorship=False` form
denies decoy editorship while a `schema_access` holder still passes (probe hops
F and E in the decoy repro).
--
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]
