onheap opened a new pull request, #43005:
URL: https://github.com/apache/superset/pull/43005
### SUMMARY
Superset enforces row-level security on raw SQL by rewriting the query:
every table it reads is wrapped in a subquery that filters by the user's row
predicate. First it lists the tables a statement reads, with
`extract_tables_from_statement()`. That list drives both the RLS predicate and
the dataset access check. Then it rewrites each read. This PR tidies up three
edge cases in that path.
**1. A read whose name matches a CTE is treated as the CTE.** To tell a CTE
reference from a real table, extraction compares the reference's bare name to
the CTE names in scope. A real table read that shares a name with a CTE is
therefore misclassified as a CTE reference. It is omitted from the statement’s
table set, so neither an RLS predicate nor an access check is applied. This
happens for a schema- or catalog-qualified read, a non-recursive CTE's own name
inside its body, and a forward reference to a later `WITH` item:
```sql
WITH orders AS (SELECT 1 AS d)
SELECT *
FROM (SELECT * FROM public.orders) AS z -- a real read of public.orders;
matches CTE name `orders`
```
**2. The subquery rewrite picks the nodes to wrap by name.** The
`AS_SUBQUERY` rewrite walks the tree and wraps every table node whose name
matches a RLS rule. A CTE reference with the same name as the rule's table
matches too, so it also gets wrapped and the predicate is applied to it. If the
CTE does not select the predicate's column, the database cannot resolve the
query:
```sql
-- rule on orders: tenant = 'A'
WITH orders AS (SELECT id FROM orders) -- inner orders: the real read
SELECT * FROM orders -- outer orders: the CTE; matching
by name also wraps this one
```
**3. Aliases pass through the rewrite as strings.** Both RLS transformers
read the alias from `node.alias`. That is a string with the quoting removed,
and they pass it back as the alias. So a quoted alias loses its quotes, and a
column-list alias keeps only its name:
```sql
-- input
SELECT * FROM tbl_a AS "a b"
-- rewrite output: the quotes are gone, so `a b` is no longer a single
identifier
SELECT * FROM (SELECT * FROM tbl_a WHERE id = 42) AS a b
```
```sql
-- input
SELECT c1 FROM tbl_a AS x (c1, c2)
-- rewrite output: the (c1, c2) column list is dropped, so c1 resolves
against the table
SELECT c1 FROM (SELECT * FROM tbl_a WHERE id = 42) AS x
```
### BEFORE / AFTER
**CTE reference vs. real table** — `is_cte()` resolved by bare name (before)
vs. through the scope (after):
```sql
WITH orders AS (SELECT 1 AS d) SELECT * FROM (SELECT * FROM public.orders)
AS z
-- before: the read of public.orders is treated as the CTE `orders` and is
not listed
-- after: the read of public.orders is listed, filtered, and access-checked
```
**Subquery rewrite of a same-named CTE** (rule on `orders`):
```sql
-- input
WITH orders AS (SELECT id FROM orders) SELECT * FROM orders
-- before: both reads matched by name; the outer CTE reference is wrapped
too, and
-- the database cannot resolve `tenant` against a projection of just
`id`
-- after: only the real read inside the CTE body is wrapped; the CTE
reference is left as is
WITH orders AS (
SELECT id FROM (SELECT * FROM orders WHERE tenant = 'A') AS orders
)
SELECT * FROM orders
```
**Alias handling:**
```sql
-- input
SELECT * FROM tbl_a AS "a b"
-- before: SELECT * FROM (SELECT * FROM tbl_a WHERE id = 42) AS a b --
quotes dropped
-- after: SELECT * FROM (SELECT * FROM tbl_a WHERE id = 42) AS "a b"
-- input
SELECT c1 FROM tbl_a AS x (c1, c2)
-- before: SELECT c1 FROM (SELECT * FROM tbl_a WHERE id = 42) AS x
-- (c1, c2) dropped
-- after: SELECT c1 FROM (SELECT * FROM tbl_a WHERE id = 42) AS x(c1, c2)
```
### TESTING INSTRUCTIONS
Unit tests are added in `tests/unit_tests/sql/parse_tests.py` covering
extraction (CTE-vs-table shapes), both rewrite methods (quoted/column-list
aliases, a same-named CTE, correlated `LATERAL`, a read inside a DML subquery),
and a filtered-set invariant that checks each real read is wrapped exactly once.
```bash
pytest tests/unit_tests/sql/ --cov=superset/sql/ --cov-fail-under=100
```
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] 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]
