chore: Move permissions checks outside of Analyzer#26869
Merged
kevintang2022 merged 1 commit intoprestodb:masterfrom Jan 21, 2026
Merged
chore: Move permissions checks outside of Analyzer#26869kevintang2022 merged 1 commit intoprestodb:masterfrom
kevintang2022 merged 1 commit intoprestodb:masterfrom
Conversation
kevintang2022
requested review from
a team,
feilong-liu,
jaystarshot,
shrinidhijoshi and
vivek-bharathan
as code owners
Contributor
Reviewer's GuideDecouples semantic analysis from access-permission checks by removing the overloaded Analyzer.analyze() entry point, pushing explicit permission checking to callers (including DDL tasks, query explain/describe rewrites, and Presto Spark), and standardizing query-integrity checks for DDL/Session executions, with corresponding test updates for access control coverage. Sequence diagram for DDLDefinitionExecution query permission checkssequenceDiagram
participant Client
participant QueryStateMachine as QueryStateMachine
participant DDLDefExec as DDLDefinitionExecution
participant Task as DDLDefinitionTask
participant AccessControl as AccessControl
participant Analyzer as Analyzer
participant AnalyzerUtil as AnalyzerUtil
Client->>QueryStateMachine: submit DDL statement
QueryStateMachine->>DDLDefExec: create execution(statement, query)
DDLDefExec->>Task: executeTask()
activate DDLDefExec
DDLDefExec->>Task: queryPermissionCheck(accessControl, identity, context, query)
alt default DataDefinitionTask implementation
Task->>AccessControl: checkQueryIntegrity(identity, context, query, emptyMap, emptyMap)
AccessControl-->>Task: ok
else DDL task override (e.g. CreateViewTask)
Task-->>DDLDefExec: no-op (permission already handled with analyze)
end
DDLDefExec->>Task: execute(statement, transactionManager, metadata, accessControl, session, parameters, warningCollector, query)
activate Task
Task->>Analyzer: new Analyzer(session, metadata, sqlParser, accessControl, optionalQueryExplainer, parameters, parameterLookup, warningCollector, query)
Task->>Analyzer: analyzeSemantic(statement, isDescribe=false)
Analyzer-->>Task: Analysis
Task->>AnalyzerUtil: checkAccessPermissions(analysis.getAccessControlReferences(), query)
AnalyzerUtil-->>Task: ok
Task-->>DDLDefExec: ListenableFuture
deactivate Task
DDLDefExec-->>QueryStateMachine: ListenableFuture
deactivate DDLDefExec
QueryStateMachine-->>Client: DDL result
Sequence diagram for QueryExplainer and Describe rewrites permission checkssequenceDiagram
participant Client
participant QueryExplainer as QueryExplainer
participant DescribeInputRewrite as DescribeInputRewrite
participant DescribeOutputRewrite as DescribeOutputRewrite
participant Analyzer as Analyzer
participant AnalyzerUtil as AnalyzerUtil
Client->>QueryExplainer: analyze(session, statement, parameters, warningCollector, query)
QueryExplainer->>Analyzer: new Analyzer(..., accessControl, optionalQueryExplainer, parameters, parameterLookup, warningCollector, query)
QueryExplainer->>Analyzer: analyzeSemantic(statement, isDescribe=false)
Analyzer-->>QueryExplainer: Analysis
QueryExplainer->>AnalyzerUtil: checkAccessPermissionsForTable(analysis.getAccessControlReferences())
QueryExplainer->>AnalyzerUtil: checkAccessPermissionsForColumns(analysis.getAccessControlReferences())
AnalyzerUtil-->>QueryExplainer: ok
QueryExplainer-->>Client: Analysis
Client->>DescribeInputRewrite: rewrite(DescribeInput)
DescribeInputRewrite->>Analyzer: new Analyzer(...)
DescribeInputRewrite->>Analyzer: analyzeSemantic(innerStatement, isDescribe=true)
Analyzer-->>DescribeInputRewrite: Analysis
DescribeInputRewrite->>AnalyzerUtil: checkAccessPermissionsForTable(analysis.getAccessControlReferences())
DescribeInputRewrite->>AnalyzerUtil: checkAccessPermissionsForColumns(analysis.getAccessControlReferences())
AnalyzerUtil-->>DescribeInputRewrite: ok
DescribeInputRewrite-->>Client: rewritten DescribeInput
Client->>DescribeOutputRewrite: rewrite(DescribeOutput)
DescribeOutputRewrite->>Analyzer: new Analyzer(...)
DescribeOutputRewrite->>Analyzer: analyzeSemantic(innerStatement, isDescribe=true)
Analyzer-->>DescribeOutputRewrite: Analysis
DescribeOutputRewrite->>AnalyzerUtil: checkAccessPermissionsForTable(analysis.getAccessControlReferences())
DescribeOutputRewrite->>AnalyzerUtil: checkAccessPermissionsForColumns(analysis.getAccessControlReferences())
AnalyzerUtil-->>DescribeOutputRewrite: ok
DescribeOutputRewrite-->>Client: rewritten DescribeOutput
Class diagram for Analyzer and DDL task permission responsibilitiesclassDiagram
class Analyzer {
- Session session
- Metadata metadata
- SqlParser sqlParser
- AccessControl accessControl
- Optional queryExplainer
- List~Expression~ parameters
- Map parameterLookup
- WarningCollector warningCollector
- String query
+ Analyzer(Session session, Metadata metadata, SqlParser sqlParser, AccessControl accessControl, Optional queryExplainer, List parameters, Map parameterLookup, WarningCollector warningCollector, String query)
+ Analysis analyzeSemantic(Statement statement, boolean isDescribe)
+ Analysis analyzeSemantic(Statement statement, Optional queryExplainer, boolean isDescribe)
}
class Analysis {
+ AccessControlReferences getAccessControlReferences()
+ Scope getRootScope()
+ RelationType getOutputDescriptor(Node node)
+ Map getFunctionHandles()
}
class AccessControlReferences {
}
class AnalyzerUtil {
+ void checkAccessPermissions(AccessControlReferences references, String query)
+ void checkAccessPermissionsForTable(AccessControlReferences references)
+ void checkAccessPermissionsForColumns(AccessControlReferences references)
}
class DataDefinitionTask~T~ {
<<interface>>
+ String explain(T statement, List parameters)
+ void queryPermissionCheck(AccessControl accessControl, Identity identity, AccessControlContext context, String query)
}
class DDLDefinitionTask~T~ {
<<interface>>
+ ListenableFuture execute(T statement, TransactionManager transactionManager, Metadata metadata, AccessControl accessControl, Session session, List parameters, WarningCollector warningCollector, String query)
}
class DDLDefinitionExecution~T~ {
- DDLDefinitionTask~T~ task
- QueryStateMachine stateMachine
- AccessControl accessControl
- String query
+ ListenableFuture executeTask()
}
class SessionDefinitionTask~T~ {
<<interface>>
+ void queryPermissionCheck(AccessControl accessControl, Identity identity, AccessControlContext context, String query)
}
class SessionDefinitionExecution~T~ {
- SessionDefinitionTask~T~ task
- QueryStateMachine stateMachine
- AccessControl accessControl
- String query
+ ListenableFuture executeTask()
}
class CreateViewTask {
+ ListenableFuture execute(CreateView statement, TransactionManager transactionManager, Metadata metadata, AccessControl accessControl, Session session, List parameters, WarningCollector warningCollector, String query)
+ void queryPermissionCheck(AccessControl accessControl, Identity identity, AccessControlContext context, String query)
- Analysis analyzeStatement(Statement statement, Session session, Metadata metadata, AccessControl accessControl, List parameters, WarningCollector warningCollector, String query)
}
class CreateFunctionTask {
+ ListenableFuture execute(CreateFunction statement, TransactionManager transactionManager, Metadata metadata, AccessControl accessControl, QueryStateMachine stateMachine, List parameters, String query)
+ void queryPermissionCheck(AccessControl accessControl, Identity identity, AccessControlContext context, String query)
}
class DropFunctionTask {
+ ListenableFuture execute(DropFunction statement, TransactionManager transactionManager, Metadata metadata, AccessControl accessControl, QueryStateMachine stateMachine, List parameters, String query)
+ void queryPermissionCheck(AccessControl accessControl, Identity identity, AccessControlContext context, String query)
}
class AlterFunctionTask {
+ ListenableFuture execute(AlterFunction statement, TransactionManager transactionManager, Metadata metadata, AccessControl accessControl, Session session, List parameters, WarningCollector warningCollector, String query)
+ void queryPermissionCheck(AccessControl accessControl, Identity identity, AccessControlContext context, String query)
}
class CreateMaterializedViewTask {
+ ListenableFuture execute(CreateMaterializedView statement, TransactionManager transactionManager, Metadata metadata, AccessControl accessControl, Session session, List parameters, WarningCollector warningCollector, String query)
}
class QueryExplainer {
+ Analysis analyze(Session session, Statement statement, List parameters, WarningCollector warningCollector, String query)
}
class DescribeInputRewrite {
+ Node rewrite(DescribeInput node, Void context)
}
class DescribeOutputRewrite {
+ Node rewrite(DescribeOutput node, Void context)
}
class PrestoSparkQueryPlanner {
+ PlanAndMore createQueryPlan(Session session, BuiltInPreparedQuery preparedQuery, WarningCollector warningCollector, String query)
}
class AccessControl {
+ void checkQueryIntegrity(Identity identity, AccessControlContext context, String query, Map tags, Map extraInfo)
+ void checkCanCreateView(Identity identity, AccessControlContext context, CatalogSchemaTableName view)
}
class DenyQueryIntegrityCheckSystemAccessControl {
+ void checkQueryIntegrity(Identity identity, AccessControlContext context, String query, Map tags, Map extraInfo)
+ void checkCanCreateView(Identity identity, AccessControlContext context, CatalogSchemaTableName view)
}
DataDefinitionTask <|.. DDLDefinitionTask
DDLDefinitionTask <|-- CreateViewTask
DDLDefinitionTask <|-- CreateFunctionTask
DDLDefinitionTask <|-- DropFunctionTask
DDLDefinitionTask <|-- AlterFunctionTask
DDLDefinitionTask <|-- CreateMaterializedViewTask
DDLDefinitionExecution --> DDLDefinitionTask : uses task
SessionDefinitionExecution --> SessionDefinitionTask : uses task
CreateViewTask ..> Analyzer : creates
CreateViewTask ..> AnalyzerUtil : calls checkAccessPermissions
CreateFunctionTask ..> Analyzer : creates
CreateFunctionTask ..> AnalyzerUtil : calls checkAccessPermissions
DropFunctionTask ..> Analyzer : creates
DropFunctionTask ..> AnalyzerUtil : calls checkAccessPermissions
AlterFunctionTask ..> Analyzer : creates
AlterFunctionTask ..> AnalyzerUtil : calls checkAccessPermissions
CreateMaterializedViewTask ..> Analyzer : creates
CreateMaterializedViewTask ..> AnalyzerUtil : calls checkAccessPermissions
QueryExplainer ..> Analyzer : creates
QueryExplainer ..> AnalyzerUtil : calls table and column checks
DescribeInputRewrite ..> Analyzer : creates
DescribeInputRewrite ..> AnalyzerUtil : calls table and column checks
DescribeOutputRewrite ..> Analyzer : creates
DescribeOutputRewrite ..> AnalyzerUtil : calls table and column checks
PrestoSparkQueryPlanner ..> Analyzer : creates
PrestoSparkQueryPlanner ..> AnalyzerUtil : calls checkAccessPermissions
AccessControl <|-- DenyQueryIntegrityCheckSystemAccessControl
Flow diagram for query permission checks across componentsflowchart TD
A[Client submits statement] --> B{Statement type}
B --> C[Query SELECT or DML]
B --> D[DDL statement]
B --> E[Session statement]
B --> F[Explain or Describe]
B --> G[Presto Spark query]
C --> H[Analyzer.analyzeSemantic]
H --> I[Planner and access control checks<br/>checkAccessPermissionsForTable/Columns]
D --> J[DDLDefinitionExecution]
J --> K[task.queryPermissionCheck]
K --> L[DataDefinitionTask default<br/>AccessControl.checkQueryIntegrity]
J --> M[task.execute]
M --> N[Task creates Analyzer]
N --> O[Analyzer.analyzeSemantic]
O --> P[checkAccessPermissions<br/>for DDL Analysis]
E --> Q[SessionDefinitionExecution]
Q --> R[task.queryPermissionCheck]
R --> S[AccessControl.checkQueryIntegrity]
Q --> T[task.execute]
F --> U[QueryExplainer or Describe rewrites]
U --> V[Analyzer.analyzeSemantic]
V --> W[checkAccessPermissionsForTable/Columns]
G --> X[PrestoSparkQueryPlanner]
X --> Y[Analyzer.analyzeSemantic]
Y --> Z[checkAccessPermissions<br/>for Spark Analysis]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
kevintang2022
added a commit
to kevintang2022/presto
that referenced
this pull request
Dec 28, 2025
Summary: - Remove overloaded analyze call in Analyzer - Add query permission check for DDL executions that call Analyze - Add query permission check for Presto spark Differential Revision: D89869624
kevintang2022
force-pushed
the
export-D89869624
branch
from
7b0d919 to
2f9658e
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- There is now a lot of repeated
analyzeSemantic(...)+getAccessControlReferences()+checkAccessPermissions*boilerplate across tasks, explainers, and tests; consider adding a small helper/utility to centralize this pattern and reduce duplication. - The no-op
queryPermissionCheckoverrides in some DDL tasks override the default implementation that callscheckQueryIntegrity, which may be surprising to future maintainers; consider either reusing the default behavior or adding a brief comment/rationale where the check is intentionally disabled.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There is now a lot of repeated `analyzeSemantic(...)` + `getAccessControlReferences()` + `checkAccessPermissions*` boilerplate across tasks, explainers, and tests; consider adding a small helper/utility to centralize this pattern and reduce duplication.
- The no-op `queryPermissionCheck` overrides in some DDL tasks override the default implementation that calls `checkQueryIntegrity`, which may be surprising to future maintainers; consider either reusing the default behavior or adding a brief comment/rationale where the check is intentionally disabled.
## Individual Comments
### Comment 1
<location> `presto-main-base/src/main/java/com/facebook/presto/execution/CreateViewTask.java:124-125` </location>
<code_context>
return immediateFuture(null);
}
+
+ @Override
+ public void queryPermissionCheck(AccessControl accessControl, Identity identity, AccessControlContext context, String query) {}
}
</code_context>
<issue_to_address>
**🚨 issue (security):** Overriding `queryPermissionCheck` with a no-op likely disables query integrity checks for this DDL path
Previously, `DDLDefinitionExecution` always invoked `accessControl.checkQueryIntegrity(...)` for DDL statements. By overriding `queryPermissionCheck` with an empty method, `CreateView` now skips these checks. If you intended to extend the check, please delegate to the default implementation or reimplement an equivalent check. If the goal is to bypass integrity checks for this path, this should be clearly documented and carefully validated for safety.
</issue_to_address>
### Comment 2
<location> `presto-main-base/src/main/java/com/facebook/presto/execution/CreateFunctionTask.java:112-113` </location>
<code_context>
return immediateFuture(null);
}
+
+ @Override
+ public void queryPermissionCheck(AccessControl accessControl, Identity identity, AccessControlContext context, String query) {}
}
</code_context>
<issue_to_address>
**🚨 issue (security):** No-op `queryPermissionCheck` here also skips integrity checks for CREATE FUNCTION
This override replaces the default `DataDefinitionTask.queryPermissionCheck` (which runs `checkQueryIntegrity`) with a no-op, changing behavior from the previous implementation where `DDLDefinitionExecution` always enforced query integrity for this statement type. Unless this relaxation is explicitly required, please either call the default implementation or add an equivalent function-specific integrity check rather than disabling it entirely.
</issue_to_address>
### Comment 3
<location> `presto-main-base/src/main/java/com/facebook/presto/execution/DropFunctionTask.java:100-101` </location>
<code_context>
return immediateFuture(null);
}
+
+ @Override
+ public void queryPermissionCheck(AccessControl accessControl, Identity identity, AccessControlContext context, String query) {}
}
</code_context>
<issue_to_address>
**🚨 issue (security):** DROP FUNCTION also bypasses `checkQueryIntegrity` via an empty `queryPermissionCheck`
This adds another special case that bypasses query integrity checks. Since other DDL still goes through `accessControl.checkQueryIntegrity(...)`, either reuse the default `queryPermissionCheck` implementation or document a strong rationale for exempting `DropFunction`. Otherwise this could unintentionally weaken integrity enforcement.
</issue_to_address>
### Comment 4
<location> `presto-main-base/src/main/java/com/facebook/presto/execution/AlterFunctionTask.java:95-96` </location>
<code_context>
return immediateFuture(null);
}
+
+ @Override
+ public void queryPermissionCheck(AccessControl accessControl, Identity identity, AccessControlContext context, String query) {}
}
</code_context>
<issue_to_address>
**🚨 issue (security):** ALTER FUNCTION now opts out of query integrity checks as well
This override bypasses the global `checkQueryIntegrity` enforcement that applied before the refactor, which may weaken security around function modification. Please either rely on the default `queryPermissionCheck` implementation or add an equivalent integrity check instead of a no-op.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+124
to
+125
| @Override | ||
| public void queryPermissionCheck(AccessControl accessControl, Identity identity, AccessControlContext context, String query) {} |
Contributor
There was a problem hiding this comment.
🚨 issue (security): Overriding queryPermissionCheck with a no-op likely disables query integrity checks for this DDL path
Previously, DDLDefinitionExecution always invoked accessControl.checkQueryIntegrity(...) for DDL statements. By overriding queryPermissionCheck with an empty method, CreateView now skips these checks. If you intended to extend the check, please delegate to the default implementation or reimplement an equivalent check. If the goal is to bypass integrity checks for this path, this should be clearly documented and carefully validated for safety.
Comment on lines
+112
to
+113
| @Override | ||
| public void queryPermissionCheck(AccessControl accessControl, Identity identity, AccessControlContext context, String query) {} |
Contributor
There was a problem hiding this comment.
🚨 issue (security): No-op queryPermissionCheck here also skips integrity checks for CREATE FUNCTION
This override replaces the default DataDefinitionTask.queryPermissionCheck (which runs checkQueryIntegrity) with a no-op, changing behavior from the previous implementation where DDLDefinitionExecution always enforced query integrity for this statement type. Unless this relaxation is explicitly required, please either call the default implementation or add an equivalent function-specific integrity check rather than disabling it entirely.
Comment on lines
+100
to
+101
| @Override | ||
| public void queryPermissionCheck(AccessControl accessControl, Identity identity, AccessControlContext context, String query) {} |
Contributor
There was a problem hiding this comment.
🚨 issue (security): DROP FUNCTION also bypasses checkQueryIntegrity via an empty queryPermissionCheck
This adds another special case that bypasses query integrity checks. Since other DDL still goes through accessControl.checkQueryIntegrity(...), either reuse the default queryPermissionCheck implementation or document a strong rationale for exempting DropFunction. Otherwise this could unintentionally weaken integrity enforcement.
Comment on lines
+95
to
+96
| @Override | ||
| public void queryPermissionCheck(AccessControl accessControl, Identity identity, AccessControlContext context, String query) {} |
Contributor
There was a problem hiding this comment.
🚨 issue (security): ALTER FUNCTION now opts out of query integrity checks as well
This override bypasses the global checkQueryIntegrity enforcement that applied before the refactor, which may weaken security around function modification. Please either rely on the default queryPermissionCheck implementation or add an equivalent integrity check instead of a no-op.
kevintang2022
added a commit
to kevintang2022/presto
that referenced
this pull request
Dec 29, 2025
Summary: - Remove overloaded analyze call in Analyzer - Add query permission check for DDL executions that call Analyze - Add query permission check for Presto spark Differential Revision: D89869624
kevintang2022
force-pushed
the
export-D89869624
branch
from
2f9658e to
1348e4b
Compare
kevintang2022
changed the title
kevintang2022
added a commit
to kevintang2022/presto
that referenced
this pull request
Dec 29, 2025
kevintang2022
force-pushed
the
export-D89869624
branch
from
1348e4b to
d8ee8a3
Compare
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The pattern
analyzeSemantic(...); AccessControlReferences refs = analysis.getAccessControlReferences(); checkAccessPermissions...is now duplicated across multiple call sites (QueryExplainer, Describe* rewrites, tests, Spark planner, various DDL tasks); consider reintroducing a small helper (e.g., a static utility or a thin wrapper aroundAnalyzer) to centralize this sequence while still avoiding the old overloadedanalyzeAPI. - Several DDL tasks override
queryPermissionCheckas a no-op and instead invokecheckAccessPermissionsinside theiranalyze*methods; it would help future maintainers if this divergence from theDataDefinitionTaskdefault were either documented or funneled through a common helper to make it clear that query integrity is intentionally handled via analysis rather than the executor hook.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The pattern `analyzeSemantic(...); AccessControlReferences refs = analysis.getAccessControlReferences(); checkAccessPermissions...` is now duplicated across multiple call sites (QueryExplainer, Describe* rewrites, tests, Spark planner, various DDL tasks); consider reintroducing a small helper (e.g., a static utility or a thin wrapper around `Analyzer`) to centralize this sequence while still avoiding the old overloaded `analyze` API.
- Several DDL tasks override `queryPermissionCheck` as a no-op and instead invoke `checkAccessPermissions` inside their `analyze*` methods; it would help future maintainers if this divergence from the `DataDefinitionTask` default were either documented or funneled through a common helper to make it clear that query integrity is intentionally handled via analysis rather than the executor hook.
## Individual Comments
### Comment 1
<location> `presto-main-base/src/main/java/com/facebook/presto/sql/rewrite/DescribeInputRewrite.java:122-125` </location>
<code_context>
// create analysis for the query we are describing.
Analyzer analyzer = new Analyzer(session, metadata, parser, accessControl, queryExplainer, parameters, parameterLookup, warningCollector, query);
- Analysis analysis = analyzer.analyze(statement, true);
+ Analysis analysis = analyzer.analyzeSemantic(statement, true);
+ AccessControlReferences accessControlReferences = analysis.getAccessControlReferences();
+ checkAccessPermissionsForTable(accessControlReferences);
+ checkAccessPermissionsForColumns(accessControlReferences);
// get all parameters in query
</code_context>
<issue_to_address>
**suggestion:** DescribeInputRewrite uses direct table/column checks instead of the new unified checkAccessPermissions helper.
Other call sites now rely on `AnalyzerUtil.checkAccessPermissions(analysis.getAccessControlReferences(), query)`, but this one still invokes the table/column checks directly. To avoid this path diverging if additional checks are added to the unified helper, please consider using the same helper here for consistent permission evaluation across query types, unless DESCRIBE is explicitly intended to behave differently.
Suggested implementation:
```java
import static com.facebook.presto.sql.analyzer.utils.ParameterExtractor.getParameters;
import static com.facebook.presto.util.AnalyzerUtil.checkAccessPermissions;
import static com.facebook.presto.util.AnalyzerUtil.createParsingOptions;
import static java.util.Objects.requireNonNull;
```
```java
// create analysis for the query we are describing.
Analyzer analyzer = new Analyzer(session, metadata, parser, accessControl, queryExplainer, parameters, parameterLookup, warningCollector, query);
Analysis analysis = analyzer.analyzeSemantic(statement, true);
checkAccessPermissions(analysis.getAccessControlReferences(), query);
// get all parameters in query
List<Parameter> parameters = getParameters(statement);
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
kevintang2022
added a commit
to kevintang2022/presto
that referenced
this pull request
Dec 30, 2025
kevintang2022
force-pushed
the
export-D89869624
branch
from
d8ee8a3 to
91a162b
Compare
kevintang2022
added a commit
to kevintang2022/presto
that referenced
this pull request
Dec 31, 2025
kevintang2022
force-pushed
the
export-D89869624
branch
from
91a162b to
c3b1f58
Compare
kevintang2022
added a commit
to kevintang2022/presto
that referenced
this pull request
Jan 5, 2026
kevintang2022
force-pushed
the
export-D89869624
branch
from
c3b1f58 to
f410642
Compare
kevintang2022
commented
Jan 20, 2026
| Analysis analysis = analyzer.analyzeSemantic(statement, false); | ||
| AccessControlReferences accessControlReferences = analysis.getAccessControlReferences(); | ||
| checkAccessPermissionsForTable(accessControlReferences); | ||
| checkAccessPermissionsForColumns(accessControlReferences); |
Contributor
Author
There was a problem hiding this comment.
Notice that in this file, analyze is called, but checkAccessPermissions is not called. Onl y the Table and Column checks are called. The query check is not called here.
This is the one example where query permission check should not be invoked, even though analyzer was run. This is due to the explain rewrite.
If query permission check was added here, then it would be invoked multiple times for an explain query, and once for each nesting of explain. However, the query permissions check should only be run once
kaikalur
reviewed
Jan 20, 2026
presto-main-base/src/main/java/com/facebook/presto/sql/rewrite/DescribeOutputRewrite.java
Outdated
Show resolved
Hide resolved
kevintang2022
force-pushed
the
export-D89869624
branch
from
f410642 to
7e51b19
Compare
kevintang2022
commented
Jan 20, 2026
| return analyzer.analyze(statement); | ||
| Analysis analysis = analyzer.analyzeSemantic(statement, false); | ||
| AccessControlReferences accessControlReferences = analysis.getAccessControlReferences(); | ||
| checkAccessPermissionsForTablesAndColumns(accessControlReferences); |
Contributor
Author
There was a problem hiding this comment.
Notice that in this file, analyze is called, but checkAccessPermissions is not called. Onl y the Table and Column checks are called. The query check is not called here.
This is the one example where query permission check should not be invoked, even though analyzer was run. This is due to the explain rewrite.
If query permission check was added here, then it would be invoked multiple times for an explain query, and once for each nesting of explain. However, the query permissions check should only be run once.
This logic will be applied in any Query Rewriter that calls analyze, like DescribeInputRewrite/DescribeOutputRewrite/ExplainRewrite. These rewriters do another analysis in their subqueries, which can result in repeated query permissions checks
kaikalur
approved these changes
Jan 21, 2026
NikhilCollooru
approved these changes
Jan 21, 2026
This was referenced Mar 31, 2026
15 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Address the TODO comment in Analyzer class to remove the overloaded analyze method and decouple the analyzeSemantic and permissionChecking behaviors.
Motivation and Context
The overloaded analyze method makes it difficult to ensure that permissions checks are run exactly once for every query type. For example, for nested EXPLAIN statements, the permissions checks being run for each level of nesting because each level of nesting creates a new rewrite and Analysis. By removing these overloaded analyze method calls, it gives flexibility to remove redundant permissions checks.
The new interface method added to
DataDefinitionTask#queryPermissionCheckis needed because sometimes, aDataDefinitionTaskimplementation (likeCreateViewTask) will callanalyzeSemanticfollowed bycheckAccessPermissionson theAnalysisinside of theexecutemethod for the task. This ensures that the utilized resources will be permission checked. But some implementations ofDataDefinitionTaskdo not ever callanalyzeSemantic(likeCreateTableTask), but we still need to be running the query permission check for these query shapes. So for non-Analyze tasks (CreateTable), we can just run the default implementation of query permissions check, and for Analyze tasks (CreateView), we override the default implementation to be NOOP and run thecheckAccessPermissionsafter the analyze.Impact
Test Plan
Add unit test cases for checkQueryIntegrity and ensure that each shape runs the check at least once.
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.
Differential Revision: D89869624