sadpandajoe commented on code in PR #45342:
URL: https://github.com/apache/superset/pull/45342#discussion_r4288079316
##########
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:
On an overwrite that changes `database_uuid`, `import_from_dict` sets
`existing.database_id` but `dataset.database` can still be the previously
loaded `Database` (the overwrite path already read it for the uniqueness
check). `_table_binding` sees the new `database_id` and triggers this check,
but the check then runs against the old connection. A user with a schema grant
on another connection could create the dataset there, then re-import the same
UUID pointing at a database they can't read and have the table check pass
against the stale one. Should this use the database resolved from
`dataset.database_id` (and likewise for the virtual-SQL check below)? A test
that changes the database on overwrite would pin it down.
##########
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:
The Explore GET and the explore form-data step still authorize a `query`
datasource through `raise_for_access(query=...,
allow_query_authorship_bypass=True)` with editorship left on. For a user who
isn't the query's author, a `SELECT 1` decoy dataset named after a table the
stored query reads still authorizes
`/api/v1/explore/?datasource_type=query&datasource_id=<id>`, which returns that
query's SQL. Should those two callers pass `allow_dataset_editorship=False`
too, so a SQL Lab query is authorized the same way on every path?
##########
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:
This only runs when the SQL text changes, and `_validate_table_access` is
skipped whenever `sql` is non-empty. So `PUT {"schema": "restricted"}` (or a
`template_params`-only change) on a virtual dataset whose SQL is unchanged
re-resolves the same SQL against a different schema with no access check. The
decoy's editor can then read that table through chart-data. Should the SQL
check also run when the schema, catalog or template params change, as the
legacy `/datasource/save/` path now does for repoints?
--
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]
