EnxDev commented on code in PR #45342:
URL: https://github.com/apache/superset/pull/45342#discussion_r4328548829
##########
superset/commands/dataset/importers/v1/utils.py:
##########
@@ -585,6 +610,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(
Review Comment:
Confirmed, and fixed in 75a2a5fdad. I reproduced it without mocking the
table check. A role with `datasource_access` on `[my_database].[t](id:N)` and
nothing else could re-import the UUID with `schema: restricted`, and the import
went through. The SQL check had the same problem for a virtual dataset whose
new SQL reads `restricted.t`.
The table and SQL checks now run on the uploaded config before
`import_from_dict` changes anything. Keys the config leaves out fall back to
the stored values, as they do in `import_from_dict`. That is the same order
`UpdateDatasetCommand` uses, so the lookup sees the dataset at its old binding
and can't use it to authorize the new one. The `datasource=dataset` check still
runs after the import, on the imported row.
The new test runs the real `raise_for_access` with only `can_access`
stubbed, for a physical and a virtual case. Both fail without the change.
##########
superset/commands/dataset/create.py:
##########
@@ -120,7 +125,11 @@ def validate(self) -> None: # noqa: C901
template_params_raw: str | None = self._properties.get(
"template_params"
)
- template_params = json_to_dict(template_params_raw or "")
+ # Render with the params chart execution exposes, so the
+ # derived schema comes from the SQL the access check sees.
+ template_params = strip_reserved_template_params(
Review Comment:
Added in 30dd12a321, next to
`test_create_dataset_schema_uses_template_params`, using your SQL and params.
It asserts the derived schema is `actual` and fails if the stripping is removed
from the derivation.
##########
superset/commands/dataset/create.py:
##########
@@ -225,6 +234,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:
This one is real, but the reserved-name handling and the params forwarding
don't cause it, and I don't think any save-time check can close it. Your
example has no params at all, and master behaves the same way: create checks
the SQL without params, renders the `else` branch, and chart-data later takes
the other one. Anything the template only sees at request time causes the same
split, like `url_param()`, `filter_values()`, `from_dttm`/`to_dttm`, `metrics`
or `current_username()`. With `url_param` the caller picks the branch on every
request. So whatever the check renders with, it can't bound what execution
renders.
The fix is the one you suggest. When a virtual dataset is authorized only by
editorship, chart execution should re-check the tables of the SQL it actually
rendered, with `allow_dataset_editorship=False`. That changes runtime
authorization for every virtual dataset an editor uses through #20382, and it
has to cover chart-data, samples, drill to detail, exports and reports. I'd
rather do it in a follow-up PR than grow this one further. Does that work for
you?
--
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]
