msyavuz commented on code in PR #44650:
URL: https://github.com/apache/superset/pull/44650#discussion_r4105931898
##########
superset/sql/parse.py:
##########
@@ -2794,6 +2822,75 @@ def resolve(scope: Scope, seen: frozenset[int]) ->
list[exp.Table]:
)
+def _find_subquery_scopes(scopes: list[Scope]) -> set[int]:
+ """
+ Find the scopes whose rows only reach a statement through a sub-query.
+
+ That is every uncorrelated ``SUBQUERY`` scope (a scalar, ``IN`` or
``EXISTS``
+ sub-query), every scope nested inside one, and every CTE one of them reads
from,
+ including the scopes nested inside that CTE. A CTE read both from a
sub-query and
+ from the statement's ``FROM`` counts as a sub-query, so its reads get the
stricter
+ rules. The body of a ``LATERAL`` or ``CROSS APPLY`` feeds the output like
a join,
+ so it is left out. A correlated sub-query is left out too: it is typically
a
+ lookup keyed to the enclosing rows, often over a table without the rule's
+ columns, which the rules would break the same way they would break a join.
The
+ outer query doesn't scope such a sub-query's tables either (UPDATING.md).
+
+ :param scopes: The scopes of the statement, as returned by
``traverse_scope``
+ :returns: The ``id`` of each scope found
+ """
+ found: set[int] = set()
+ pending = [
+ scope
+ for scope in scopes
+ if scope.scope_type == ScopeType.SUBQUERY
+ and not (scope.parent and scope.parent.scope_type == ScopeType.UDTF)
+ and not _is_correlated(scope)
+ ]
+ while pending:
+ scope = pending.pop()
+ if id(scope) in found:
+ continue
+ found.add(id(scope))
+ pending.extend(child for child in scopes if child.parent is scope)
+ pending.extend(
+ source
+ for source in scope.sources.values()
+ if isinstance(source, Scope) and source.scope_type == ScopeType.CTE
+ )
+ return found
+
+
+def _is_correlated(scope: Scope) -> bool:
+ """
+ Does a sub-query reference a table of an enclosing query?
+
+ Only a column qualified with an enclosing table's name or alias counts,
when the
+ sub-query has no table of its own under that name. An unqualified column
can't
+ be told apart from one of the sub-query's own, so it is treated as local,
which
+ errs toward the sub-query getting the stricter rules. (``Scope``'s own
+ ``is_correlated_subquery`` treats every unqualified column as external.)
+
+ Only the sub-query's own columns count, not those of a sub-query nested in
it
+ (which ``Scope.columns`` includes): a nested correlated sub-query doesn't
key the
+ wrapping sub-query's tables to the enclosing rows.
+
+ :param scope: A ``SUBQUERY`` scope
+ :returns: True if the sub-query is correlated
+ """
+ enclosing: set[str] = set()
+ parent = scope.parent
+ while parent:
+ enclosing.update(parent.sources)
+ parent = parent.parent
+ return any(
+ isinstance(node, exp.Column)
+ and node.table in enclosing
+ and node.table not in scope.sources
Review Comment:
CTE names are in the sub-query's `scope.sources`, so a sub-query correlated
to an outer CTE by name reads as uncorrelated: `WITH base AS (SELECT * FROM a)
SELECT base.*, (SELECT name FROM b WHERE b.id = base.lid) FROM base` still
injects `b.org_id = 1` under both methods. Fails closed, but it's the lookup
breakage this PR is avoiding; aliasing the CTE works around it.
##########
superset/sql/parse.py:
##########
@@ -2794,6 +2822,75 @@ def resolve(scope: Scope, seen: frozenset[int]) ->
list[exp.Table]:
)
+def _find_subquery_scopes(scopes: list[Scope]) -> set[int]:
+ """
+ Find the scopes whose rows only reach a statement through a sub-query.
+
+ That is every uncorrelated ``SUBQUERY`` scope (a scalar, ``IN`` or
``EXISTS``
+ sub-query), every scope nested inside one, and every CTE one of them reads
from,
+ including the scopes nested inside that CTE. A CTE read both from a
sub-query and
+ from the statement's ``FROM`` counts as a sub-query, so its reads get the
stricter
+ rules. The body of a ``LATERAL`` or ``CROSS APPLY`` feeds the output like
a join,
+ so it is left out. A correlated sub-query is left out too: it is typically
a
+ lookup keyed to the enclosing rows, often over a table without the rule's
+ columns, which the rules would break the same way they would break a join.
The
+ outer query doesn't scope such a sub-query's tables either (UPDATING.md).
+
+ :param scopes: The scopes of the statement, as returned by
``traverse_scope``
+ :returns: The ``id`` of each scope found
+ """
+ found: set[int] = set()
+ pending = [
+ scope
+ for scope in scopes
+ if scope.scope_type == ScopeType.SUBQUERY
+ and not (scope.parent and scope.parent.scope_type == ScopeType.UDTF)
+ and not _is_correlated(scope)
+ ]
+ while pending:
+ scope = pending.pop()
+ if id(scope) in found:
+ continue
+ found.add(id(scope))
+ pending.extend(child for child in scopes if child.parent is scope)
+ pending.extend(
+ source
+ for source in scope.sources.values()
+ if isinstance(source, Scope) and source.scope_type == ScopeType.CTE
+ )
+ return found
+
+
+def _is_correlated(scope: Scope) -> bool:
+ """
+ Does a sub-query reference a table of an enclosing query?
+
+ Only a column qualified with an enclosing table's name or alias counts,
when the
+ sub-query has no table of its own under that name. An unqualified column
can't
+ be told apart from one of the sub-query's own, so it is treated as local,
which
+ errs toward the sub-query getting the stricter rules. (``Scope``'s own
+ ``is_correlated_subquery`` treats every unqualified column as external.)
+
+ Only the sub-query's own columns count, not those of a sub-query nested in
it
+ (which ``Scope.columns`` includes): a nested correlated sub-query doesn't
key the
+ wrapping sub-query's tables to the enclosing rows.
+
+ :param scope: A ``SUBQUERY`` scope
+ :returns: True if the sub-query is correlated
+ """
+ enclosing: set[str] = set()
+ parent = scope.parent
+ while parent:
+ enclosing.update(parent.sources)
+ parent = parent.parent
+ return any(
+ isinstance(node, exp.Column)
+ and node.table in enclosing
Review Comment:
Should this compare case-insensitively on folding dialects? On Postgres
`SELECT b.*, (SELECT count(*) FROM secret AS B WHERE b.v > 0) FROM b` is
classified correlated since `"b" != "B"`, so `secret` skips the global guest
rule and an embedded guest sees all its rows (SQL has to be authored by
Alpha/Admin, not a regression vs master).
##########
superset/models/helpers.py:
##########
@@ -3799,8 +3799,13 @@ def get_from_clause(
),
self.database,
self.database.get_default_catalog(),
- exclude_dataset_id=self_id,
- include_global_guest_rls=False,
+ # at least as strict as apply_rls(), which injects
+ # this dataset's own RLS and the global guest rules
+ # into the inner SQL's sub-queries
+ exclude_dataset_id=(
+ None if statement.has_subquery() else self_id
Review Comment:
`test_get_from_clause_fail_closed_counts_global_guest_rls_in_subqueries`
only asserts `include_global_guest_rls`; could it also assert
`exclude_dataset_id is None` here? Otherwise a regression back to `self_id`
makes the fail-closed check looser than `apply_rls` without failing anything.
##########
superset/sql/parse.py:
##########
@@ -2794,6 +2822,75 @@ def resolve(scope: Scope, seen: frozenset[int]) ->
list[exp.Table]:
)
+def _find_subquery_scopes(scopes: list[Scope]) -> set[int]:
+ """
+ Find the scopes whose rows only reach a statement through a sub-query.
+
+ That is every uncorrelated ``SUBQUERY`` scope (a scalar, ``IN`` or
``EXISTS``
+ sub-query), every scope nested inside one, and every CTE one of them reads
from,
+ including the scopes nested inside that CTE. A CTE read both from a
sub-query and
+ from the statement's ``FROM`` counts as a sub-query, so its reads get the
stricter
+ rules. The body of a ``LATERAL`` or ``CROSS APPLY`` feeds the output like
a join,
+ so it is left out. A correlated sub-query is left out too: it is typically
a
+ lookup keyed to the enclosing rows, often over a table without the rule's
+ columns, which the rules would break the same way they would break a join.
The
+ outer query doesn't scope such a sub-query's tables either (UPDATING.md).
+
+ :param scopes: The scopes of the statement, as returned by
``traverse_scope``
+ :returns: The ``id`` of each scope found
+ """
+ found: set[int] = set()
+ pending = [
+ scope
+ for scope in scopes
+ if scope.scope_type == ScopeType.SUBQUERY
+ and not (scope.parent and scope.parent.scope_type == ScopeType.UDTF)
+ and not _is_correlated(scope)
+ ]
+ while pending:
+ scope = pending.pop()
+ if id(scope) in found:
+ continue
+ found.add(id(scope))
+ pending.extend(child for child in scopes if child.parent is scope)
+ pending.extend(
+ source
+ for source in scope.sources.values()
Review Comment:
`scope.sources` holds every CTE in lexical scope, not just the ones the
sub-query reads, so any sub-query pulls the strict rules into CTEs that are
only joined in the main FROM. On AS_PREDICATE, `WITH l AS (SELECT * FROM
lookup) SELECT a.*, l.name, (SELECT count(*) FROM b) FROM a JOIN l ON l.id =
a.lid` injects `lookup.org_id = 1`.
--
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]
