EnxDev commented on code in PR #45342:
URL: https://github.com/apache/superset/pull/45342#discussion_r4296124176
##########
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:
Confirmed, and fixed in 03fb5100a7. I went with ignoring the names rather
than rejecting them, since that's what execution does with them anyway.
The list ended up a bit longer than the three filter/cache names.
`SqlaTable.get_template_processor` passes the params through the same keywords
the processor takes itself, so `table`, `database` and `query` don't become
variables either. They just break at run time instead: `table` and `database`
raise `TypeError: got multiple values for keyword argument`, and `query` gets
bound as the processor's query and fails on `.schema`. The shared helper used
by create, update and `/datasource/save/`, and the importer's
`_get_template_params`, now drop all six names before the check renders. A few
existing tests used `{"table": ...}` as their template param, which describes a
dataset whose charts can't run, so I renamed that key in those fixtures.
Tests: one renders your exact SQL and params both ways. It builds the
processor the way `get_sqla_query` does and goes through `process_jinja_sql`
for the check, then asserts both see `public.customer_pii`. It fails without
the change. The helper table, the create test and the importer test also got a
reserved-name case.
##########
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:
Added in e7e9991a59. It overwrites an existing physical `safe_table` with a
config that only changes `table_name` to `customer_pii`, and asserts
`DatasetAccessDeniedError` and that the table check ran on
`Table("customer_pii", "public")` with `allow_dataset_editorship=False`. I
parametrized it with a schema-only change as well. Dropping `table_name` from
`_table_binding` makes the `table_name` case fail and leaves the other import
tests green, which is the gap you described.
--
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]
