Skip to content

chore: Move permissions checks outside of Analyzer #26869

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kevintang2022 merged 1 commit into from
Jan 21, 2026
Merged

chore: Move permissions checks outside of Analyzer #26869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion presto-main-base/src/main/java/com/facebook/presto/execution/AlterFunctionTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,15 @@
import com.facebook.presto.common.QualifiedObjectName;
import com.facebook.presto.common.type.TypeSignature;
import com.facebook.presto.metadata.Metadata;
import com.facebook.presto.spi.MaterializedViewDefinition;
import com.facebook.presto.spi.WarningCollector;
import com.facebook.presto.spi.analyzer.ViewDefinition;
import com.facebook.presto.spi.function.AlterRoutineCharacteristics;
import com.facebook.presto.spi.function.RoutineCharacteristics.NullCallClause;
import com.facebook.presto.spi.security.AccessControl;
import com.facebook.presto.spi.security.AccessControlContext;
import com.facebook.presto.spi.security.Identity;
import com.facebook.presto.sql.analyzer.Analysis;
import com.facebook.presto.sql.analyzer.Analyzer;
import com.facebook.presto.sql.parser.SqlParser;
import com.facebook.presto.sql.tree.AlterFunction;
Expand All @@ -36,6 +41,7 @@
import java.util.Optional;

import static com.facebook.presto.sql.analyzer.utils.ParameterUtils.parameterExtractor;
import static com.facebook.presto.util.AnalyzerUtil.checkAccessPermissions;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.util.concurrent.Futures.immediateFuture;
import static java.util.Objects.requireNonNull;
Expand Down Expand Up @@ -68,7 +74,8 @@ public ListenableFuture<?> execute(AlterFunction statement, TransactionManager t
{
Map<NodeRef<Parameter>, Expression> parameterLookup = parameterExtractor(statement, parameters);
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterLookup, warningCollector, query);
analyzer.analyze(statement);
Analysis analysis = analyzer.analyzeSemantic(statement, false);
checkAccessPermissions(analysis.getAccessControlReferences(), query, session.getPreparedStatements());

QualifiedObjectName functionName = metadata.getFunctionAndTypeManager().getFunctionAndTypeResolver().qualifyObjectName(statement.getFunctionName());
AlterRoutineCharacteristics alterRoutineCharacteristics = new AlterRoutineCharacteristics(
Expand All @@ -83,4 +90,7 @@ public ListenableFuture<?> execute(AlterFunction statement, TransactionManager t
alterRoutineCharacteristics);
return immediateFuture(null);
}

@Override
public void queryPermissionCheck(AccessControl accessControl, Identity identity, AccessControlContext context, String query, Map<String, String> preparedStatements, Map<QualifiedObjectName, ViewDefinition> viewDefinitions, Map<QualifiedObjectName, MaterializedViewDefinition> materializedViewDefinitions) {}
}
12 changes: 11 additions & 1 deletion presto-main-base/src/main/java/com/facebook/presto/execution/CreateFunctionTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,17 @@
import com.facebook.presto.common.type.Type;
import com.facebook.presto.common.type.TypeSignature;
import com.facebook.presto.metadata.Metadata;
import com.facebook.presto.spi.MaterializedViewDefinition;
import com.facebook.presto.spi.PrestoException;
import com.facebook.presto.spi.analyzer.ViewDefinition;
import com.facebook.presto.spi.function.Parameter;
import com.facebook.presto.spi.function.RoutineCharacteristics;
import com.facebook.presto.spi.function.SqlFunctionHandle;
import com.facebook.presto.spi.function.SqlFunctionId;
import com.facebook.presto.spi.function.SqlInvokedFunction;
import com.facebook.presto.spi.security.AccessControl;
import com.facebook.presto.spi.security.AccessControlContext;
import com.facebook.presto.spi.security.Identity;
import com.facebook.presto.sql.analyzer.Analysis;
import com.facebook.presto.sql.analyzer.Analyzer;
import com.facebook.presto.sql.parser.SqlParser;
Expand All @@ -50,6 +54,7 @@
import static com.facebook.presto.spi.function.FunctionVersion.notVersioned;
import static com.facebook.presto.sql.SqlFormatter.formatSql;
import static com.facebook.presto.sql.analyzer.utils.ParameterUtils.parameterExtractor;
import static com.facebook.presto.util.AnalyzerUtil.checkAccessPermissions;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.util.concurrent.Futures.immediateFuture;
import static java.lang.String.format;
Expand Down Expand Up @@ -84,7 +89,9 @@ public ListenableFuture<?> execute(CreateFunction statement, TransactionManager
Map<NodeRef<com.facebook.presto.sql.tree.Parameter>, Expression> parameterLookup = parameterExtractor(statement, parameters);
Session session = stateMachine.getSession();
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterLookup, stateMachine.getWarningCollector(), query);
Analysis analysis = analyzer.analyze(statement);
Analysis analysis = analyzer.analyzeSemantic(statement, false);
checkAccessPermissions(analysis.getAccessControlReferences(), query, session.getPreparedStatements());

if (analysis.getFunctionHandles().values().stream()
.anyMatch(SqlFunctionHandle.class::isInstance)) {
throw new PrestoException(NOT_SUPPORTED, "Invoking a dynamically registered function in SQL function body is not supported");
Expand All @@ -101,6 +108,9 @@ public ListenableFuture<?> execute(CreateFunction statement, TransactionManager
return immediateFuture(null);
}

@Override
public void queryPermissionCheck(AccessControl accessControl, Identity identity, AccessControlContext context, String query, Map<String, String> preparedStatements, Map<QualifiedObjectName, ViewDefinition> viewDefinitions, Map<QualifiedObjectName, MaterializedViewDefinition> materializedViewDefinitions) {}

private SqlInvokedFunction createSqlInvokedFunction(CreateFunction statement, Metadata metadata, Analysis analysis)
{
QualifiedObjectName functionName = statement.isTemporary() ?
Expand Down
4 changes: 3 additions & 1 deletion presto-main-base/src/main/java/com/facebook/presto/execution/CreateMaterializedViewTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
import static com.facebook.presto.sql.analyzer.SemanticErrorCode.MATERIALIZED_VIEW_ALREADY_EXISTS;
import static com.facebook.presto.sql.analyzer.SemanticErrorCode.NOT_SUPPORTED;
import static com.facebook.presto.sql.analyzer.utils.ParameterUtils.parameterExtractor;
import static com.facebook.presto.util.AnalyzerUtil.checkAccessPermissions;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.util.concurrent.Futures.immediateFuture;
import static java.util.Objects.requireNonNull;
Expand Down Expand Up @@ -92,7 +93,8 @@ public ListenableFuture<?> execute(CreateMaterializedView statement, Transaction

Map<NodeRef<Parameter>, Expression> parameterLookup = parameterExtractor(statement, parameters);
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterLookup, warningCollector, query);
Analysis analysis = analyzer.analyze(statement);
Analysis analysis = analyzer.analyzeSemantic(statement, false);
checkAccessPermissions(analysis.getAccessControlReferences(), query, session.getPreparedStatements());

List<ColumnMetadata> columnMetadata = analysis.getOutputDescriptor(statement.getQuery())
.getVisibleFields().stream()
Expand Down
13 changes: 12 additions & 1 deletion presto-main-base/src/main/java/com/facebook/presto/execution/CreateViewTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,12 @@
import com.facebook.presto.metadata.Metadata;
import com.facebook.presto.spi.ColumnMetadata;
import com.facebook.presto.spi.ConnectorTableMetadata;
import com.facebook.presto.spi.MaterializedViewDefinition;
import com.facebook.presto.spi.WarningCollector;
import com.facebook.presto.spi.analyzer.ViewDefinition;
import com.facebook.presto.spi.security.AccessControl;
import com.facebook.presto.spi.security.AccessControlContext;
import com.facebook.presto.spi.security.Identity;
import com.facebook.presto.spi.security.ViewSecurity;
import com.facebook.presto.sql.analyzer.Analysis;
import com.facebook.presto.sql.analyzer.Analyzer;
Expand All @@ -35,6 +38,7 @@
import jakarta.inject.Inject;

import java.util.List;
import java.util.Map;
import java.util.Optional;

import static com.facebook.presto.SystemSessionProperties.getDefaultViewSecurityMode;
Expand All @@ -44,6 +48,7 @@
import static com.facebook.presto.spi.security.ViewSecurity.INVOKER;
import static com.facebook.presto.sql.SqlFormatterUtil.getFormattedSql;
import static com.facebook.presto.sql.analyzer.utils.ParameterUtils.parameterExtractor;
import static com.facebook.presto.util.AnalyzerUtil.checkAccessPermissions;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.util.concurrent.Futures.immediateFuture;
import static java.util.Objects.requireNonNull;
Expand Down Expand Up @@ -115,9 +120,15 @@ public ListenableFuture<?> execute(CreateView statement, TransactionManager tran
return immediateFuture(null);
}

@Override
public void queryPermissionCheck(AccessControl accessControl, Identity identity, AccessControlContext context, String query, Map<String, String> preparedStatements, Map<QualifiedObjectName, ViewDefinition> viewDefinitions, Map<QualifiedObjectName, MaterializedViewDefinition> materializedViewDefinitions) {}

private Analysis analyzeStatement(Statement statement, Session session, Metadata metadata, AccessControl accessControl, List<Expression> parameters, WarningCollector warningCollector, String query)
{
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterExtractor(statement, parameters), warningCollector, query);
return analyzer.analyze(statement);
Analysis analysis = analyzer.analyzeSemantic(statement, false);
checkAccessPermissions(analysis.getAccessControlReferences(), query, session.getPreparedStatements());

return analysis;
}
}
3 changes: 1 addition & 2 deletions presto-main-base/src/main/java/com/facebook/presto/execution/DDLDefinitionExecution.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,7 @@ private DDLDefinitionExecution(
@Override
protected ListenableFuture<?> executeTask()
{
accessControl.checkQueryIntegrity(stateMachine.getSession().getIdentity(), stateMachine.getSession().getAccessControlContext(), query, stateMachine.getSession().getPreparedStatements(), ImmutableMap.of(), ImmutableMap.of());

task.queryPermissionCheck(accessControl, stateMachine.getSession().getIdentity(), stateMachine.getSession().getAccessControlContext(), query, stateMachine.getSession().getPreparedStatements(), ImmutableMap.of(), ImmutableMap.of());
return task.execute(statement, transactionManager, metadata, accessControl, stateMachine.getSession(), parameters, stateMachine.getWarningCollector(), query);
}

Expand Down
12 changes: 12 additions & 0 deletions presto-main-base/src/main/java/com/facebook/presto/execution/DataDefinitionTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,19 @@
*/
package com.facebook.presto.execution;

import com.facebook.presto.common.QualifiedObjectName;
import com.facebook.presto.spi.MaterializedViewDefinition;
import com.facebook.presto.spi.analyzer.ViewDefinition;
import com.facebook.presto.spi.security.AccessControl;
import com.facebook.presto.spi.security.AccessControlContext;
import com.facebook.presto.spi.security.Identity;
import com.facebook.presto.sql.SqlFormatter;
import com.facebook.presto.sql.tree.Expression;
import com.facebook.presto.sql.tree.Prepare;
import com.facebook.presto.sql.tree.Statement;

import java.util.List;
import java.util.Map;
import java.util.Optional;

public interface DataDefinitionTask<T extends Statement>
Expand All @@ -33,4 +40,9 @@ default String explain(T statement, List<Expression> parameters)

return SqlFormatter.formatSql(statement, Optional.of(parameters));
}

default void queryPermissionCheck(AccessControl accessControl, Identity identity, AccessControlContext context, String query, Map<String, String> preparedStatements, Map<QualifiedObjectName, ViewDefinition> viewDefinitions, Map<QualifiedObjectName, MaterializedViewDefinition> materializedViewDefinitions)
{
accessControl.checkQueryIntegrity(identity, context, query, preparedStatements, viewDefinitions, materializedViewDefinitions);
}
}
13 changes: 12 additions & 1 deletion presto-main-base/src/main/java/com/facebook/presto/execution/DropFunctionTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,13 @@
import com.facebook.presto.common.QualifiedObjectName;
import com.facebook.presto.common.type.TypeSignature;
import com.facebook.presto.metadata.Metadata;
import com.facebook.presto.spi.MaterializedViewDefinition;
import com.facebook.presto.spi.analyzer.ViewDefinition;
import com.facebook.presto.spi.function.SqlFunctionId;
import com.facebook.presto.spi.security.AccessControl;
import com.facebook.presto.spi.security.AccessControlContext;
import com.facebook.presto.spi.security.Identity;
import com.facebook.presto.sql.analyzer.Analysis;
import com.facebook.presto.sql.analyzer.Analyzer;
import com.facebook.presto.sql.parser.SqlParser;
import com.facebook.presto.sql.tree.DropFunction;
Expand All @@ -34,6 +39,7 @@

import static com.facebook.presto.metadata.SessionFunctionHandle.SESSION_NAMESPACE;
import static com.facebook.presto.sql.analyzer.utils.ParameterUtils.parameterExtractor;
import static com.facebook.presto.util.AnalyzerUtil.checkAccessPermissions;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.util.concurrent.Futures.immediateFuture;
import static java.lang.String.format;
Expand Down Expand Up @@ -68,7 +74,9 @@ public ListenableFuture<?> execute(DropFunction statement, TransactionManager tr
{
Map<NodeRef<Parameter>, Expression> parameterLookup = parameterExtractor(statement, parameters);
Analyzer analyzer = new Analyzer(stateMachine.getSession(), metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterLookup, stateMachine.getWarningCollector(), query);
analyzer.analyze(statement);
Analysis analysis = analyzer.analyzeSemantic(statement, false);
checkAccessPermissions(analysis.getAccessControlReferences(), query, stateMachine.getSession().getPreparedStatements());

Optional<List<TypeSignature>> parameterTypes = statement.getParameterTypes().map(types -> types.stream().map(TypeSignature::parseTypeSignature).collect(toImmutableList()));

if (statement.isTemporary()) {
Expand All @@ -87,4 +95,7 @@ public ListenableFuture<?> execute(DropFunction statement, TransactionManager tr

return immediateFuture(null);
}

@Override
public void queryPermissionCheck(AccessControl accessControl, Identity identity, AccessControlContext context, String query, Map<String, String> preparedStatements, Map<QualifiedObjectName, ViewDefinition> viewDefinitions, Map<QualifiedObjectName, MaterializedViewDefinition> materializedViewDefinitions) {}
}
3 changes: 1 addition & 2 deletions presto-main-base/src/main/java/com/facebook/presto/execution/SessionDefinitionExecution.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,7 @@ private SessionDefinitionExecution(
@Override
protected ListenableFuture<?> executeTask()
{
accessControl.checkQueryIntegrity(stateMachine.getSession().getIdentity(), stateMachine.getSession().getAccessControlContext(), query, stateMachine.getSession().getPreparedStatements(), ImmutableMap.of(), ImmutableMap.of());

task.queryPermissionCheck(accessControl, stateMachine.getSession().getIdentity(), stateMachine.getSession().getAccessControlContext(), query, stateMachine.getSession().getPreparedStatements(), ImmutableMap.of(), ImmutableMap.of());
return task.execute(statement, transactionManager, metadata, accessControl, stateMachine, parameters, query);
}

Expand Down
5 changes: 5 additions & 0 deletions ...rc/main/java/com/facebook/presto/security/DenyQueryIntegrityCheckSystemAccessControl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,11 @@ public void checkCanCreateTable(Identity identity, AccessControlContext context,
{
}

@Override
public void checkCanCreateView(Identity identity, AccessControlContext context, CatalogSchemaTableName view)
{
}

@Override
public void checkCanShowCreateTable(Identity identity, AccessControlContext context, CatalogSchemaTableName table)
{
Expand Down
18 changes: 0 additions & 18 deletions presto-main-base/src/main/java/com/facebook/presto/sql/analyzer/Analyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import com.facebook.presto.common.QualifiedObjectName;
import com.facebook.presto.metadata.Metadata;
import com.facebook.presto.spi.WarningCollector;
import com.facebook.presto.spi.analyzer.AccessControlReferences;
import com.facebook.presto.spi.function.FunctionHandle;
import com.facebook.presto.spi.security.AccessControl;
import com.facebook.presto.sql.parser.SqlParser;
Expand Down Expand Up @@ -46,8 +45,6 @@
import static com.facebook.presto.sql.analyzer.SemanticErrorCode.CANNOT_HAVE_AGGREGATIONS_WINDOWS_OR_GROUPING;
import static com.facebook.presto.sql.analyzer.SemanticErrorCode.NOT_SUPPORTED;
import static com.facebook.presto.sql.analyzer.UtilizedColumnsAnalyzer.analyzeForUtilizedColumns;
import static com.facebook.presto.util.AnalyzerUtil.checkAccessPermissionsForColumns;
import static com.facebook.presto.util.AnalyzerUtil.checkAccessPermissionsForTable;
import static java.util.Objects.requireNonNull;

public class Analyzer
Expand Down Expand Up @@ -102,21 +99,6 @@ public Analyzer(
this.query = requireNonNull(query, "query is null");
}

public Analysis analyze(Statement statement)
{
return analyze(statement, false);
}

// TODO: Remove this method once all calls are moved to analyzer interface, as this call is overloaded with analyze and columnCheckPermissions
public Analysis analyze(Statement statement, boolean isDescribe)
{
Analysis analysis = analyzeSemantic(statement, isDescribe);
AccessControlReferences accessControlReferences = analysis.getAccessControlReferences();
checkAccessPermissionsForTable(accessControlReferences);
checkAccessPermissionsForColumns(accessControlReferences);
return analysis;
}

public Analysis analyzeSemantic(Statement statement, boolean isDescribe)
{
return analyzeSemantic(statement, Optional.empty(), isDescribe);
Expand Down
8 changes: 7 additions & 1 deletion presto-main-base/src/main/java/com/facebook/presto/sql/analyzer/QueryExplainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.facebook.presto.spi.PrestoException;
import com.facebook.presto.spi.VariableAllocator;
import com.facebook.presto.spi.WarningCollector;
import com.facebook.presto.spi.analyzer.AccessControlReferences;
import com.facebook.presto.spi.plan.PlanNode;
import com.facebook.presto.spi.plan.PlanNodeIdAllocator;
import com.facebook.presto.spi.security.AccessControl;
Expand Down Expand Up @@ -58,6 +59,7 @@
import static com.facebook.presto.sql.planner.planPrinter.PlanPrinter.graphvizLogicalPlan;
import static com.facebook.presto.sql.planner.planPrinter.PlanPrinter.jsonDistributedPlan;
import static com.facebook.presto.sql.planner.planPrinter.PlanPrinter.jsonLogicalPlan;
import static com.facebook.presto.util.AnalyzerUtil.checkAccessPermissionsForTablesAndColumns;
import static java.lang.String.format;
import static java.util.Objects.requireNonNull;

Expand Down Expand Up @@ -122,7 +124,11 @@ public QueryExplainer(
public Analysis analyze(Session session, Statement statement, List<Expression> parameters, WarningCollector warningCollector, String query)
{
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.of(this), parameters, parameterExtractor(statement, parameters), warningCollector, query);
return analyzer.analyze(statement);
Analysis analysis = analyzer.analyzeSemantic(statement, false);
AccessControlReferences accessControlReferences = analysis.getAccessControlReferences();
checkAccessPermissionsForTablesAndColumns(accessControlReferences);
Copy link
Copy Markdown
Contributor Author

@kevintang2022 kevintang2022 Jan 20, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


return analysis;
}

public String getPlan(Session session, Statement statement, Type planType, List<Expression> parameters, boolean verbose, WarningCollector warningCollector, String query)
Expand Down
Loading
Loading