sha174n opened a new pull request, #44501:
URL: https://github.com/apache/superset/pull/44501
### SUMMARY
A virtual (query-backed) dataset's stored SQL is re-rendered with the
caller's Jinja context each time it is queried, so the tables its `FROM`/`JOIN`
clauses resolve to can depend on values supplied at request time (for example
`url_param`). The tables that a static render of the stored SQL declares are
already covered by the dataset grant and have row-level security applied, but a
table that appears only because of a request-time value was not being checked
against the caller.
This change makes the chart-data virtual-dataset path consistent with the
SQL Lab raw-query path: after the SQL is rendered, any table the rendered SQL
resolves to that the dataset does not declare statically is authorized against
the caller using the same per-table check SQL Lab uses (`raise_for_access(...,
force_dataset_match=True)`), before the query runs.
Mechanics:
- `get_from_clause()` already parses the rendered SQL and resolves its
tables (for RLS). After the read-only check, it now diffs the rendered tables
against the tables the stored SQL declares statically (computed by neutralizing
Jinja in the stored SQL and re-parsing) and authorizes only the difference.
- Non-templated virtual datasets, and templated ones whose rendered tables
match the declared set, are unaffected: the resolved tables equal the declared
tables, so nothing extra is checked. Row-level security and the SQL Lab path
are unchanged.
- Fails closed: if the declared set cannot be determined, every resolved
table is checked; unresolved or unauthorized tables are rejected.
Known trade-off: a stored template that selects different tables via control
blocks (e.g. `{% if %}FROM a{% else %}FROM b{% endif %}`) may not survive
static neutralization, in which case the resolved tables are all checked
against the caller. Callers holding only the dataset grant (not grants on the
underlying tables) would then be rejected for such a dataset. This mirrors the
strictness SQL Lab already applies and can be relaxed later by deriving the
declared set from the Jinja AST.
### TESTING INSTRUCTIONS
New unit tests in
`tests/unit_tests/models/test_virtual_dataset_table_access.py` cover:
- static virtual dataset: no per-table check;
- templating that only affects a filter value: no per-table check;
- a table introduced by a request-time value: authorized (allowed when
granted, rejected when not);
- a mix of a declared table and a request-introduced table: only the
introduced one is checked.
```
pytest tests/unit_tests/models/test_virtual_dataset_table_access.py \
tests/unit_tests/models/test_virtual_dataset_format.py \
tests/unit_tests/models/test_double_rls_virtual_dataset.py \
tests/unit_tests/models/helpers_test.py
```
### ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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]
