sadpandajoe commented on code in PR #45342:
URL: https://github.com/apache/superset/pull/45342#discussion_r4293113257
##########
superset/commands/dataset/create.py:
##########
@@ -225,6 +226,12 @@ def validate(self) -> None: # noqa: C901
sql=sql,
catalog=catalog,
schema=schema,
+ # Jinja-templated SQL only reveals the tables it reads
+ # once rendered with the params the dataset is saved with.
+ template_params=template_params_for_access_check(
Review Comment:
Authorization now renders this SQL with the dataset's own `template_params`,
but chart execution doesn't see those params the same way: `get_sqla_query`
hands `removed_filters`, `applied_filters` and `extra_cache_keys` to the
processor as constructor arguments, so they are not template variables at run
time, while here a `template_params` entry with that name is a defined
variable. With template processing on, a user with dataset write and no grants
on `public.customer_pii` can save `{% if removed_filters is defined %}SELECT 1
AS secret{% else %}SELECT secret FROM public.customer_pii{% endif %}` with
`template_params = {"removed_filters": []}`. The check renders only `SELECT 1`
and passes, then chart-data on that dataset renders the second branch and is
authorized by `is_editor(datasource)`. Before this forwarding the check
rendered without that variable and took the restricted branch, so it was
denied. The same forwarding is in the update, `/datasource/save/` and import
checks. C
ould the access check reject (or ignore) those reserved processor names in
`template_params`, so it renders what execution renders?
##########
tests/unit_tests/datasets/commands/importers/v1/import_test.py:
##########
@@ -703,10 +704,112 @@ def
test_import_dataset_virtual_sql_check_receives_template_params(
call for call in raise_for_access.call_args_list if
call.kwargs.get("sql")
)
assert sql_call.kwargs["template_params"] == {"flag": True}
+ # A dataset named after a referenced table must not stand in for data
+ # access to it.
+ assert sql_call.kwargs["allow_dataset_editorship"] is False
# Empty strings in the export payload must not be forwarded verbatim.
assert sql_call.kwargs["schema"] is None
+def _decoy_dataset(database: Database) -> SqlaTable:
+ """A ``SELECT 1`` virtual dataset named after a table it can't read."""
+ decoy = SqlaTable(
+ uuid=uuid.uuid4(),
+ table_name="customer_pii",
+ schema="public",
+ sql="SELECT 1",
+ database_id=database.id,
+ )
+ db.session.add(decoy)
+ db.session.flush()
+ return decoy
+
+
+def _physical_config(dataset: SqlaTable, database: Database) -> dict[str, Any]:
+ return {
+ "table_name": dataset.table_name,
+ "schema": dataset.schema,
+ "sql": None,
+ "uuid": dataset.uuid,
+ "metrics": [],
+ "columns": [],
+ "database_uuid": database.uuid,
+ "database_id": database.id,
+ }
+
+
+def test_import_dataset_overwrite_decoy_as_physical_requires_table_access(
Review Comment:
The new import rebinding gate depends on `table_name` (and
catalog/schema/database) in `_table_binding`, but the only rejection test here
overwrites a virtual decoy with a physical config, which trips the
`bool(dataset.sql)` difference. The unchanged re-import test leaves a physical
binding alone. Overwriting an existing physical dataset's UUID with only
`table_name` changed (say `safe_table` -> `customer_pii`) by an editor without
access to the target is not asserted anywhere, so dropping `table_name` from
`_table_binding` would keep both tests green and leave that path on the
datasource-only check. Could you add a case next to this one that calls
`import_dataset(..., overwrite=True)` for that rename and asserts
`DatasetAccessDeniedError` with the `table` check called with
`allow_dataset_editorship=False`?
--
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]
