sadpandajoe commented on code in PR #45342:
URL: https://github.com/apache/superset/pull/45342#discussion_r4299376456
##########
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:
The access check here renders the SQL with only the stored
`template_params`, but chart execution adds its own variables on top
(`get_sqla_query` passes `columns`, `groupby`, `metrics`, `filter`,
`time_grain`, etc. to the template processor). With template processing on, a
user with dataset write and no grants on `public.customer_pii` can save `{% if
columns is defined %}SELECT secret FROM public.customer_pii{% else %}SELECT 1
AS secret{% endif %}` with no params. Creation (and column sync) renders only
the `SELECT 1` branch and passes; chart-data then renders the restricted branch
and is authorized by `is_editor(datasource)`. Update, import and
`/datasource/save/` forward the same params, so they look affected too. Could
dataset-backed chart execution re-check the tables of the SQL it actually
renders instead of trusting editorship, or is there another way to close this
branch-dependent case?
##########
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:
This check runs after `import_from_dict` has already changed the existing
row, so on an overwrite that changes only `schema` the table lookup finds that
same dataset under the new schema. Its `datasource_access` still matches
because the dataset permission name is `[db].[table](id:N)` and omits the
schema, so `allow_dataset_editorship=False` doesn't reject it. A role with
write/import and `datasource_access` on `public.t` (but nothing on
`restricted.t`) could re-import that UUID with `schema: restricted` and then
read `restricted.t` through chart-data. Can the target binding be authorized
before the row is mutated, or the dataset being rebound be excluded from the
rows that can authorize its new binding? The rejection test mocks
`raise_for_access`, so it wouldn't catch this.
##########
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:
Nothing seems to cover this stripping: the new create tests only assert what
is forwarded to the mocked `raise_for_access`, and the helper test never goes
through this derivation. If `strip_reserved_template_params` were dropped here,
would any test fail? A case next to
`test_create_dataset_schema_uses_template_params` with
`template_params='{"removed_filters": []}'` and `{% if removed_filters is
defined %}SELECT * FROM decoy.t{% else %}SELECT * FROM actual.t{% endif %}`,
asserting the derived schema is `actual`, would pin it.
--
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]
