sadpandajoe commented on code in PR #45266:
URL: https://github.com/apache/superset/pull/45266#discussion_r4287703068
##########
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:
This only runs when `dataset.sql` is set, and the preceding
`raise_for_access(datasource=dataset)` still accepts editorship. If someone
owns a virtual decoy dataset named like a private table (`SELECT 1`) and
re-imports the same UUID with `overwrite` and no `sql`, it becomes a physical
dataset on that table and passes on editorship alone, and chart-data then
accepts the same editorship. Should the physical-table binding also get a
`database`+`table` check with `allow_editorship=False`, as the update command
does?
##########
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:
Adding `allow_editorship=False` here changes the exact call these existing
unit tests assert, so they should now fail:
`tests/unit_tests/commands/dataset/update_test.py` (the
`assert_called_once_with(database=mock_database, table=Table(...))` around line
329) and `tests/unit_tests/views/datasource/views_test.py` (the exact-args
assertions around lines 660 and 694, where `call(database=new_database,
table=ANY)` and the SQL call also omit the flag). Could you update those
expectations to include `allow_editorship=False`? That would also give the
call-site wiring here and in `views.py` its first coverage; as it stands,
nothing would catch the keyword being dropped from the
create/update/repoint/chart-data call sites.
--
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]
