diff --git a/core/trino-main/src/main/java/io/trino/execution/AddColumnTask.java b/core/trino-main/src/main/java/io/trino/execution/AddColumnTask.java index b1599bfd05a2..b2cde3aeea4e 100644 --- a/core/trino-main/src/main/java/io/trino/execution/AddColumnTask.java +++ b/core/trino-main/src/main/java/io/trino/execution/AddColumnTask.java @@ -114,7 +114,8 @@ public ListenableFuture execute( session, metadata, accessControl, - parameterExtractor(statement, parameters)); + parameterExtractor(statement, parameters), + true); ColumnMetadata column = ColumnMetadata.builder() .setName(element.getName().getValue()) diff --git a/core/trino-main/src/main/java/io/trino/execution/CreateMaterializedViewTask.java b/core/trino-main/src/main/java/io/trino/execution/CreateMaterializedViewTask.java index 6570fea076cb..2031f239abcf 100644 --- a/core/trino-main/src/main/java/io/trino/execution/CreateMaterializedViewTask.java +++ b/core/trino-main/src/main/java/io/trino/execution/CreateMaterializedViewTask.java @@ -110,7 +110,8 @@ public ListenableFuture execute( session, metadata, accessControl, - parameterLookup); + parameterLookup, + true); ConnectorMaterializedViewDefinition definition = new ConnectorMaterializedViewDefinition( sql, diff --git a/core/trino-main/src/main/java/io/trino/execution/CreateSchemaTask.java b/core/trino-main/src/main/java/io/trino/execution/CreateSchemaTask.java index 5a19097d4dae..6974dd3fa293 100644 --- a/core/trino-main/src/main/java/io/trino/execution/CreateSchemaTask.java +++ b/core/trino-main/src/main/java/io/trino/execution/CreateSchemaTask.java @@ -96,7 +96,8 @@ static ListenableFuture internalExecute(CreateSchema statement, Metadata m session, metadata, accessControl, - parameterExtractor(statement, parameters)); + parameterExtractor(statement, parameters), + true); TrinoPrincipal principal = getCreatePrincipal(statement, session, metadata, catalogName.getCatalogName()); try { diff --git a/core/trino-main/src/main/java/io/trino/execution/CreateTableTask.java b/core/trino-main/src/main/java/io/trino/execution/CreateTableTask.java index 8059d176cf8f..dff8c57e0434 100644 --- a/core/trino-main/src/main/java/io/trino/execution/CreateTableTask.java +++ b/core/trino-main/src/main/java/io/trino/execution/CreateTableTask.java @@ -33,6 +33,7 @@ import io.trino.spi.security.AccessDeniedException; import io.trino.spi.type.Type; import io.trino.spi.type.TypeNotFoundException; +import io.trino.sql.analyzer.FeaturesConfig; import io.trino.sql.analyzer.Output; import io.trino.sql.analyzer.OutputColumn; import io.trino.sql.tree.ColumnDefinition; @@ -44,6 +45,8 @@ import io.trino.sql.tree.TableElement; import io.trino.transaction.TransactionManager; +import javax.inject.Inject; + import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; @@ -81,6 +84,14 @@ public class CreateTableTask implements DataDefinitionTask { + private final boolean disableSetPropertiesSecurityCheckForCreateDdl; + + @Inject + public CreateTableTask(FeaturesConfig featuresConfig) + { + this.disableSetPropertiesSecurityCheckForCreateDdl = featuresConfig.isDisableSetPropertiesSecurityCheckForCreateDdl(); + } + @Override public String getName() { @@ -155,7 +166,8 @@ ListenableFuture internalExecute(CreateTable statement, Metadata metadata, session, metadata, accessControl, - parameterLookup); + parameterLookup, + true); columns.put(name, ColumnMetadata.builder() .setName(name) @@ -230,8 +242,6 @@ else if (element instanceof LikeClause) { } } - accessControl.checkCanCreateTable(session.toSecurityContext(), tableName); - Map sqlProperties = mapFromProperties(statement.getProperties()); Map properties = metadata.getTablePropertyManager().getProperties( catalogName, @@ -240,7 +250,15 @@ else if (element instanceof LikeClause) { session, metadata, accessControl, - parameterLookup); + parameterLookup, + true); + + if (!disableSetPropertiesSecurityCheckForCreateDdl && !properties.isEmpty()) { + accessControl.checkCanCreateTable(session.toSecurityContext(), tableName, properties); + } + else { + accessControl.checkCanCreateTable(session.toSecurityContext(), tableName); + } Map finalProperties = combineProperties(sqlProperties.keySet(), properties, inheritedProperties); diff --git a/core/trino-main/src/main/java/io/trino/execution/SetPropertiesTask.java b/core/trino-main/src/main/java/io/trino/execution/SetPropertiesTask.java new file mode 100644 index 000000000000..d023c9a5b7f3 --- /dev/null +++ b/core/trino-main/src/main/java/io/trino/execution/SetPropertiesTask.java @@ -0,0 +1,102 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.execution; + +import com.google.common.util.concurrent.ListenableFuture; +import io.trino.Session; +import io.trino.execution.warnings.WarningCollector; +import io.trino.metadata.Metadata; +import io.trino.metadata.QualifiedObjectName; +import io.trino.metadata.TableHandle; +import io.trino.security.AccessControl; +import io.trino.sql.tree.Expression; +import io.trino.sql.tree.SetProperties; +import io.trino.transaction.TransactionManager; + +import java.util.List; +import java.util.Map; +import java.util.Optional; + +import static com.google.common.util.concurrent.Futures.immediateVoidFuture; +import static io.trino.metadata.MetadataUtil.createQualifiedObjectName; +import static io.trino.metadata.MetadataUtil.getRequiredCatalogHandle; +import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED; +import static io.trino.spi.StandardErrorCode.TABLE_NOT_FOUND; +import static io.trino.sql.NodeUtils.mapFromProperties; +import static io.trino.sql.ParameterUtils.parameterExtractor; +import static io.trino.sql.analyzer.SemanticExceptions.semanticException; + +public class SetPropertiesTask + implements DataDefinitionTask +{ + @Override + public String getName() + { + return "SET PROPERTIES"; + } + + @Override + public ListenableFuture execute( + SetProperties statement, + TransactionManager transactionManager, + Metadata metadata, + AccessControl accessControl, + QueryStateMachine stateMachine, + List parameters, + WarningCollector warningCollector) + { + Session session = stateMachine.getSession(); + QualifiedObjectName tableName = createQualifiedObjectName(session, statement, statement.getName()); + + Map sqlProperties = mapFromProperties(statement.getProperties()); + + if (statement.getType() == SetProperties.Type.TABLE) { + Map properties = metadata.getTablePropertyManager().getProperties( + getRequiredCatalogHandle(metadata, session, statement, tableName.getCatalogName()), + tableName.getCatalogName(), + sqlProperties, + session, + metadata, + accessControl, + parameterExtractor(statement, parameters), + false); // skip setting of default properties since they should not be stored explicitly + setTableProperties(statement, tableName, metadata, accessControl, session, properties); + } + else { + throw semanticException(NOT_SUPPORTED, statement, "Unsupported target type: %s", statement.getType()); + } + + return immediateVoidFuture(); + } + + private void setTableProperties(SetProperties statement, QualifiedObjectName tableName, Metadata metadata, AccessControl accessControl, Session session, Map properties) + { + if (metadata.getMaterializedView(session, tableName).isPresent()) { + throw semanticException(NOT_SUPPORTED, statement, "Cannot set properties to a materialized view in ALTER TABLE"); + } + + if (metadata.getView(session, tableName).isPresent()) { + throw semanticException(NOT_SUPPORTED, statement, "Cannot set properties to a view in ALTER TABLE"); + } + + Optional tableHandle = metadata.getTableHandle(session, tableName); + if (tableHandle.isEmpty()) { + throw semanticException(TABLE_NOT_FOUND, statement, "Table does not exist: %s", tableName); + } + + accessControl.checkCanSetTableProperties(session.toSecurityContext(), tableName, properties); + + metadata.setTableProperties(session, tableHandle.get(), properties); + } +} diff --git a/core/trino-main/src/main/java/io/trino/metadata/AbstractPropertyManager.java b/core/trino-main/src/main/java/io/trino/metadata/AbstractPropertyManager.java index 5acbd549a2b7..82555437888f 100644 --- a/core/trino-main/src/main/java/io/trino/metadata/AbstractPropertyManager.java +++ b/core/trino-main/src/main/java/io/trino/metadata/AbstractPropertyManager.java @@ -77,7 +77,8 @@ public final Map getProperties( Session session, Metadata metadata, AccessControl accessControl, - Map, Expression> parameters) + Map, Expression> parameters, + boolean setDefaultProperties) { Map> supportedProperties = connectorProperties.get(catalogName); if (supportedProperties == null) { @@ -134,6 +135,9 @@ public final Map getProperties( } Map userSpecifiedProperties = properties.build(); + if (!setDefaultProperties) { + return properties.build(); + } // Fill in the remaining properties with non-null defaults for (PropertyMetadata propertyMetadata : supportedProperties.values()) { if (!userSpecifiedProperties.containsKey(propertyMetadata.getName())) { diff --git a/core/trino-main/src/main/java/io/trino/metadata/Metadata.java b/core/trino-main/src/main/java/io/trino/metadata/Metadata.java index 01efbbc8757a..441a8afe6e86 100644 --- a/core/trino-main/src/main/java/io/trino/metadata/Metadata.java +++ b/core/trino-main/src/main/java/io/trino/metadata/Metadata.java @@ -200,6 +200,11 @@ public interface Metadata */ void renameTable(Session session, TableHandle tableHandle, QualifiedObjectName newTableName); + /** + * Set properties to the specified table. + */ + void setTableProperties(Session session, TableHandle tableHandle, Map properties); + /** * Comments to the specified table. */ diff --git a/core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java b/core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java index 894677c4eb12..9974fdeb794c 100644 --- a/core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java +++ b/core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java @@ -773,6 +773,14 @@ public void renameTable(Session session, TableHandle tableHandle, QualifiedObjec metadata.renameTable(session.toConnectorSession(catalog), tableHandle.getConnectorHandle(), newTableName.asSchemaTableName()); } + @Override + public void setTableProperties(Session session, TableHandle tableHandle, Map properties) + { + CatalogName catalogName = tableHandle.getCatalogName(); + ConnectorMetadata metadata = getMetadataForWrite(session, catalogName); + metadata.setTableProperties(session.toConnectorSession(catalogName), tableHandle.getConnectorHandle(), properties); + } + @Override public void setTableComment(Session session, TableHandle tableHandle, Optional comment) { diff --git a/core/trino-main/src/main/java/io/trino/security/AccessControl.java b/core/trino-main/src/main/java/io/trino/security/AccessControl.java index 673aa5c6b575..a6b58d61c5c6 100644 --- a/core/trino-main/src/main/java/io/trino/security/AccessControl.java +++ b/core/trino-main/src/main/java/io/trino/security/AccessControl.java @@ -27,6 +27,7 @@ import java.security.Principal; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.Set; @@ -163,9 +164,18 @@ public interface AccessControl * Check if identity is allowed to create the specified table. * * @throws AccessDeniedException if not allowed + * @deprecated use {@link #checkCanCreateTable(SecurityContext context, QualifiedObjectName tableName, Map properties)} */ + @Deprecated void checkCanCreateTable(SecurityContext context, QualifiedObjectName tableName); + /** + * Check if identity is allowed to create the specified table with properties. + * + * @throws AccessDeniedException if not allowed + */ + void checkCanCreateTable(SecurityContext context, QualifiedObjectName tableName, Map properties); + /** * Check if identity is allowed to drop the specified table. * @@ -180,6 +190,13 @@ public interface AccessControl */ void checkCanRenameTable(SecurityContext context, QualifiedObjectName tableName, QualifiedObjectName newTableName); + /** + * Check if identity is allowed to set properties to the specified table. + * + * @throws AccessDeniedException if not allowed + */ + void checkCanSetTableProperties(SecurityContext context, QualifiedObjectName tableName, Map properties); + /** * Check if identity is allowed to comment the specified table. * diff --git a/core/trino-main/src/main/java/io/trino/security/AccessControlManager.java b/core/trino-main/src/main/java/io/trino/security/AccessControlManager.java index 65552f986e46..324447885e26 100644 --- a/core/trino-main/src/main/java/io/trino/security/AccessControlManager.java +++ b/core/trino-main/src/main/java/io/trino/security/AccessControlManager.java @@ -426,6 +426,19 @@ public void checkCanCreateTable(SecurityContext securityContext, QualifiedObject catalogAuthorizationCheck(tableName.getCatalogName(), securityContext, (control, context) -> control.checkCanCreateTable(context, tableName.asSchemaTableName())); } + @Override + public void checkCanCreateTable(SecurityContext securityContext, QualifiedObjectName tableName, Map properties) + { + requireNonNull(securityContext, "securityContext is null"); + requireNonNull(tableName, "tableName is null"); + + checkCanAccessCatalog(securityContext, tableName.getCatalogName()); + + systemAuthorizationCheck(control -> control.checkCanCreateTable(securityContext.toSystemSecurityContext(), tableName.asCatalogSchemaTableName(), properties)); + + catalogAuthorizationCheck(tableName.getCatalogName(), securityContext, (control, context) -> control.checkCanCreateTable(context, tableName.asSchemaTableName(), properties)); + } + @Override public void checkCanDropTable(SecurityContext securityContext, QualifiedObjectName tableName) { @@ -453,6 +466,20 @@ public void checkCanRenameTable(SecurityContext securityContext, QualifiedObject catalogAuthorizationCheck(tableName.getCatalogName(), securityContext, (control, context) -> control.checkCanRenameTable(context, tableName.asSchemaTableName(), newTableName.asSchemaTableName())); } + @Override + public void checkCanSetTableProperties(SecurityContext securityContext, QualifiedObjectName tableName, Map properties) + { + requireNonNull(securityContext, "securityContext is null"); + requireNonNull(tableName, "tableName is null"); + requireNonNull(properties, "properties is null"); + + checkCanAccessCatalog(securityContext, tableName.getCatalogName()); + + systemAuthorizationCheck(control -> control.checkCanSetTableProperties(securityContext.toSystemSecurityContext(), tableName.asCatalogSchemaTableName(), properties)); + + catalogAuthorizationCheck(tableName.getCatalogName(), securityContext, (control, context) -> control.checkCanSetTableProperties(context, tableName.asSchemaTableName(), properties)); + } + @Override public void checkCanSetTableComment(SecurityContext securityContext, QualifiedObjectName tableName) { diff --git a/core/trino-main/src/main/java/io/trino/security/AllowAllAccessControl.java b/core/trino-main/src/main/java/io/trino/security/AllowAllAccessControl.java index 745c861e4e9b..5f0327c90b49 100644 --- a/core/trino-main/src/main/java/io/trino/security/AllowAllAccessControl.java +++ b/core/trino-main/src/main/java/io/trino/security/AllowAllAccessControl.java @@ -22,6 +22,7 @@ import io.trino.spi.security.TrinoPrincipal; import java.security.Principal; +import java.util.Map; import java.util.Optional; import java.util.Set; @@ -121,6 +122,11 @@ public void checkCanCreateTable(SecurityContext context, QualifiedObjectName tab { } + @Override + public void checkCanCreateTable(SecurityContext context, QualifiedObjectName tableName, Map properties) + { + } + @Override public void checkCanDropTable(SecurityContext context, QualifiedObjectName tableName) { @@ -131,6 +137,11 @@ public void checkCanRenameTable(SecurityContext context, QualifiedObjectName tab { } + @Override + public void checkCanSetTableProperties(SecurityContext context, QualifiedObjectName tableName, Map properties) + { + } + @Override public void checkCanSetTableComment(SecurityContext context, QualifiedObjectName tableName) { diff --git a/core/trino-main/src/main/java/io/trino/security/DenyAllAccessControl.java b/core/trino-main/src/main/java/io/trino/security/DenyAllAccessControl.java index 0803060c89fd..1a1741e971f8 100644 --- a/core/trino-main/src/main/java/io/trino/security/DenyAllAccessControl.java +++ b/core/trino-main/src/main/java/io/trino/security/DenyAllAccessControl.java @@ -23,6 +23,7 @@ import io.trino.spi.security.TrinoPrincipal; import java.security.Principal; +import java.util.Map; import java.util.Optional; import java.util.Set; @@ -68,6 +69,7 @@ import static io.trino.spi.security.AccessDeniedException.denySetSchemaAuthorization; import static io.trino.spi.security.AccessDeniedException.denySetSystemSessionProperty; import static io.trino.spi.security.AccessDeniedException.denySetTableAuthorization; +import static io.trino.spi.security.AccessDeniedException.denySetTableProperties; import static io.trino.spi.security.AccessDeniedException.denySetUser; import static io.trino.spi.security.AccessDeniedException.denySetViewAuthorization; import static io.trino.spi.security.AccessDeniedException.denyShowColumns; @@ -182,6 +184,12 @@ public void checkCanCreateTable(SecurityContext context, QualifiedObjectName tab denyCreateTable(tableName.toString()); } + @Override + public void checkCanCreateTable(SecurityContext context, QualifiedObjectName tableName, Map properties) + { + denyCreateTable(tableName.toString()); + } + @Override public void checkCanDropTable(SecurityContext context, QualifiedObjectName tableName) { @@ -194,6 +202,12 @@ public void checkCanRenameTable(SecurityContext context, QualifiedObjectName tab denyRenameTable(tableName.toString(), newTableName.toString()); } + @Override + public void checkCanSetTableProperties(SecurityContext context, QualifiedObjectName tableName, Map properties) + { + denySetTableProperties(tableName.toString()); + } + @Override public void checkCanSetTableComment(SecurityContext context, QualifiedObjectName tableName) { diff --git a/core/trino-main/src/main/java/io/trino/security/ForwardingAccessControl.java b/core/trino-main/src/main/java/io/trino/security/ForwardingAccessControl.java index f1421d9be99f..2d3d952888d0 100644 --- a/core/trino-main/src/main/java/io/trino/security/ForwardingAccessControl.java +++ b/core/trino-main/src/main/java/io/trino/security/ForwardingAccessControl.java @@ -25,6 +25,7 @@ import java.security.Principal; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.function.Supplier; @@ -158,6 +159,12 @@ public void checkCanCreateTable(SecurityContext context, QualifiedObjectName tab delegate().checkCanCreateTable(context, tableName); } + @Override + public void checkCanCreateTable(SecurityContext context, QualifiedObjectName tableName, Map properties) + { + delegate().checkCanCreateTable(context, tableName, properties); + } + @Override public void checkCanDropTable(SecurityContext context, QualifiedObjectName tableName) { @@ -170,6 +177,12 @@ public void checkCanRenameTable(SecurityContext context, QualifiedObjectName tab delegate().checkCanRenameTable(context, tableName, newTableName); } + @Override + public void checkCanSetTableProperties(SecurityContext context, QualifiedObjectName tableName, Map properties) + { + delegate().checkCanSetTableProperties(context, tableName, properties); + } + @Override public void checkCanSetTableComment(SecurityContext context, QualifiedObjectName tableName) { diff --git a/core/trino-main/src/main/java/io/trino/security/InjectedConnectorAccessControl.java b/core/trino-main/src/main/java/io/trino/security/InjectedConnectorAccessControl.java index bc6cf1e9db54..556f8bca4d51 100644 --- a/core/trino-main/src/main/java/io/trino/security/InjectedConnectorAccessControl.java +++ b/core/trino-main/src/main/java/io/trino/security/InjectedConnectorAccessControl.java @@ -26,6 +26,7 @@ import io.trino.spi.security.ViewExpression; import io.trino.spi.type.Type; +import java.util.Map; import java.util.Optional; import java.util.Set; @@ -110,6 +111,13 @@ public void checkCanCreateTable(ConnectorSecurityContext context, SchemaTableNam accessControl.checkCanCreateTable(securityContext, getQualifiedObjectName(tableName)); } + @Override + public void checkCanCreateTable(ConnectorSecurityContext context, SchemaTableName tableName, Map properties) + { + checkArgument(context == null, "context must be null"); + accessControl.checkCanCreateTable(securityContext, getQualifiedObjectName(tableName), properties); + } + @Override public void checkCanDropTable(ConnectorSecurityContext context, SchemaTableName tableName) { @@ -124,6 +132,13 @@ public void checkCanRenameTable(ConnectorSecurityContext context, SchemaTableNam accessControl.checkCanRenameTable(securityContext, getQualifiedObjectName(tableName), getQualifiedObjectName(tableName)); } + @Override + public void checkCanSetTableProperties(ConnectorSecurityContext context, SchemaTableName tableName, Map properties) + { + checkArgument(context == null, "context must be null"); + accessControl.checkCanSetTableProperties(securityContext, getQualifiedObjectName(tableName), properties); + } + @Override public void checkCanSetTableComment(ConnectorSecurityContext context, SchemaTableName tableName) { diff --git a/core/trino-main/src/main/java/io/trino/server/QueryExecutionFactoryModule.java b/core/trino-main/src/main/java/io/trino/server/QueryExecutionFactoryModule.java index b0241a9b9bca..9e48b25bc008 100644 --- a/core/trino-main/src/main/java/io/trino/server/QueryExecutionFactoryModule.java +++ b/core/trino-main/src/main/java/io/trino/server/QueryExecutionFactoryModule.java @@ -50,6 +50,7 @@ import io.trino.execution.RevokeTask; import io.trino.execution.RollbackTask; import io.trino.execution.SetPathTask; +import io.trino.execution.SetPropertiesTask; import io.trino.execution.SetRoleTask; import io.trino.execution.SetSchemaAuthorizationTask; import io.trino.execution.SetSessionTask; @@ -88,6 +89,7 @@ import io.trino.sql.tree.RevokeRoles; import io.trino.sql.tree.Rollback; import io.trino.sql.tree.SetPath; +import io.trino.sql.tree.SetProperties; import io.trino.sql.tree.SetRole; import io.trino.sql.tree.SetSchemaAuthorization; import io.trino.sql.tree.SetSession; @@ -146,6 +148,7 @@ public void configure(Binder binder) bindDataDefinitionTask(binder, executionBinder, RevokeRoles.class, RevokeRolesTask.class); bindDataDefinitionTask(binder, executionBinder, Rollback.class, RollbackTask.class); bindDataDefinitionTask(binder, executionBinder, SetPath.class, SetPathTask.class); + bindDataDefinitionTask(binder, executionBinder, SetProperties.class, SetPropertiesTask.class); bindDataDefinitionTask(binder, executionBinder, SetTimeZone.class, SetTimeZoneTask.class); bindDataDefinitionTask(binder, executionBinder, SetRole.class, SetRoleTask.class); bindDataDefinitionTask(binder, executionBinder, SetSchemaAuthorization.class, SetSchemaAuthorizationTask.class); diff --git a/core/trino-main/src/main/java/io/trino/sql/analyzer/FeaturesConfig.java b/core/trino-main/src/main/java/io/trino/sql/analyzer/FeaturesConfig.java index 70ecd09a87b8..200452b8f76a 100644 --- a/core/trino-main/src/main/java/io/trino/sql/analyzer/FeaturesConfig.java +++ b/core/trino-main/src/main/java/io/trino/sql/analyzer/FeaturesConfig.java @@ -142,6 +142,7 @@ public class FeaturesConfig private int maxGroupingSets = 2048; private boolean legacyCatalogRoles; + private boolean disableSetPropertiesSecurityCheckForCreateDdl; public enum JoinReorderingStrategy { @@ -1103,4 +1104,16 @@ public FeaturesConfig setLegacyCatalogRoles(boolean legacyCatalogRoles) this.legacyCatalogRoles = legacyCatalogRoles; return this; } + + public boolean isDisableSetPropertiesSecurityCheckForCreateDdl() + { + return disableSetPropertiesSecurityCheckForCreateDdl; + } + + @Config("deprecated.disable-set-properties-security-check-for-create-ddl") + public FeaturesConfig setDisableSetPropertiesSecurityCheckForCreateDdl(boolean disableSetPropertiesSecurityCheckForCreateDdl) + { + this.disableSetPropertiesSecurityCheckForCreateDdl = disableSetPropertiesSecurityCheckForCreateDdl; + return this; + } } diff --git a/core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java b/core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java index 638aaad2739a..3e099be329fe 100644 --- a/core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java +++ b/core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java @@ -157,6 +157,7 @@ import io.trino.sql.tree.Select; import io.trino.sql.tree.SelectItem; import io.trino.sql.tree.SetOperation; +import io.trino.sql.tree.SetProperties; import io.trino.sql.tree.SetSchemaAuthorization; import io.trino.sql.tree.SetSession; import io.trino.sql.tree.SetTableAuthorization; @@ -726,7 +727,8 @@ protected Scope visitAnalyze(Analyze node, Optional scope) session, metadata, accessControl, - analysis.getParameters()); + analysis.getParameters(), + true); TableHandle tableHandle = metadata.getTableHandleForStatisticsCollection(session, tableName, analyzeProperties) .orElseThrow(() -> semanticException(TABLE_NOT_FOUND, node, "Table '%s' does not exist", tableName)); @@ -815,7 +817,8 @@ protected Scope visitCreateTableAsSelect(CreateTableAsSelect node, Optional scope) return createAndAssignScope(node, scope); } + @Override + protected Scope visitSetProperties(SetProperties node, Optional context) + { + return createAndAssignScope(node, context); + } + @Override protected Scope visitComment(Comment node, Optional scope) { diff --git a/core/trino-main/src/main/java/io/trino/testing/LocalQueryRunner.java b/core/trino-main/src/main/java/io/trino/testing/LocalQueryRunner.java index 795e6912a12a..cde4cb22f6e4 100644 --- a/core/trino-main/src/main/java/io/trino/testing/LocalQueryRunner.java +++ b/core/trino-main/src/main/java/io/trino/testing/LocalQueryRunner.java @@ -74,6 +74,7 @@ import io.trino.execution.RollbackTask; import io.trino.execution.ScheduledSplit; import io.trino.execution.SetPathTask; +import io.trino.execution.SetPropertiesTask; import io.trino.execution.SetSessionTask; import io.trino.execution.SetTimeZoneTask; import io.trino.execution.StartTransactionTask; @@ -184,6 +185,7 @@ import io.trino.sql.tree.ResetSession; import io.trino.sql.tree.Rollback; import io.trino.sql.tree.SetPath; +import io.trino.sql.tree.SetProperties; import io.trino.sql.tree.SetSession; import io.trino.sql.tree.SetTimeZone; import io.trino.sql.tree.StartTransaction; @@ -449,7 +451,7 @@ private LocalQueryRunner( defaultSession.getProtocolHeaders()); dataDefinitionTask = ImmutableMap., DataDefinitionTask>builder() - .put(CreateTable.class, new CreateTableTask()) + .put(CreateTable.class, new CreateTableTask(featuresConfig)) .put(CreateView.class, new CreateViewTask(sqlParser, groupProvider, statsCalculator)) .put(DropTable.class, new DropTableTask()) .put(DropView.class, new DropViewTask()) @@ -466,6 +468,7 @@ private LocalQueryRunner( .put(Commit.class, new CommitTask()) .put(Rollback.class, new RollbackTask()) .put(SetPath.class, new SetPathTask()) + .put(SetProperties.class, new SetPropertiesTask()) .put(SetTimeZone.class, new SetTimeZoneTask(sqlParser, groupProvider, statsCalculator)) .build(); diff --git a/core/trino-main/src/main/java/io/trino/testing/TestingAccessControlManager.java b/core/trino-main/src/main/java/io/trino/testing/TestingAccessControlManager.java index 5292c6259b2c..61436468bb23 100644 --- a/core/trino-main/src/main/java/io/trino/testing/TestingAccessControlManager.java +++ b/core/trino-main/src/main/java/io/trino/testing/TestingAccessControlManager.java @@ -74,6 +74,7 @@ import static io.trino.spi.security.AccessDeniedException.denySelectColumns; import static io.trino.spi.security.AccessDeniedException.denySetCatalogSessionProperty; import static io.trino.spi.security.AccessDeniedException.denySetSystemSessionProperty; +import static io.trino.spi.security.AccessDeniedException.denySetTableProperties; import static io.trino.spi.security.AccessDeniedException.denySetUser; import static io.trino.spi.security.AccessDeniedException.denyShowColumns; import static io.trino.spi.security.AccessDeniedException.denyShowCreateTable; @@ -107,6 +108,7 @@ import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.RENAME_VIEW; import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.SELECT_COLUMN; import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.SET_SESSION; +import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.SET_TABLE_PROPERTIES; import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.SET_USER; import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.SHOW_COLUMNS; import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.SHOW_CREATE_TABLE; @@ -338,6 +340,20 @@ public void checkCanCreateTable(SecurityContext context, QualifiedObjectName tab } } + @Override + public void checkCanCreateTable(SecurityContext context, QualifiedObjectName tableName, Map properties) + { + if (shouldDenyPrivilege(context.getIdentity().getUser(), tableName.getObjectName(), CREATE_TABLE)) { + denyCreateTable(tableName.toString()); + } + if (shouldDenyPrivilege(context.getIdentity().getUser(), tableName.getObjectName(), SET_TABLE_PROPERTIES)) { + denySetTableProperties(tableName.toString()); + } + if (denyPrivileges.isEmpty()) { + super.checkCanCreateTable(context, tableName, properties); + } + } + @Override public void checkCanDropTable(SecurityContext context, QualifiedObjectName tableName) { @@ -360,6 +376,17 @@ public void checkCanRenameTable(SecurityContext context, QualifiedObjectName tab } } + @Override + public void checkCanSetTableProperties(SecurityContext context, QualifiedObjectName tableName, Map properties) + { + if (shouldDenyPrivilege(context.getIdentity().getUser(), tableName.getObjectName(), SET_TABLE_PROPERTIES)) { + denySetTableProperties(tableName.toString()); + } + if (denyPrivileges.isEmpty()) { + super.checkCanSetTableProperties(context, tableName, properties); + } + } + @Override public void checkCanSetTableComment(SecurityContext context, QualifiedObjectName tableName) { @@ -660,7 +687,7 @@ public enum TestingPrivilegeType EXECUTE_QUERY, VIEW_QUERY, KILL_QUERY, EXECUTE_FUNCTION, CREATE_SCHEMA, DROP_SCHEMA, RENAME_SCHEMA, - SHOW_CREATE_TABLE, CREATE_TABLE, DROP_TABLE, RENAME_TABLE, COMMENT_TABLE, COMMENT_COLUMN, INSERT_TABLE, DELETE_TABLE, UPDATE_TABLE, SHOW_COLUMNS, + SHOW_CREATE_TABLE, CREATE_TABLE, DROP_TABLE, RENAME_TABLE, COMMENT_TABLE, COMMENT_COLUMN, INSERT_TABLE, DELETE_TABLE, UPDATE_TABLE, SET_TABLE_PROPERTIES, SHOW_COLUMNS, ADD_COLUMN, DROP_COLUMN, RENAME_COLUMN, SELECT_COLUMN, CREATE_VIEW, RENAME_VIEW, DROP_VIEW, CREATE_VIEW_WITH_SELECT_COLUMNS, CREATE_MATERIALIZED_VIEW, REFRESH_MATERIALIZED_VIEW, DROP_MATERIALIZED_VIEW, RENAME_MATERIALIZED_VIEW, diff --git a/core/trino-main/src/main/java/io/trino/util/StatementUtils.java b/core/trino-main/src/main/java/io/trino/util/StatementUtils.java index 8f7301db7d18..05ab819814ab 100644 --- a/core/trino-main/src/main/java/io/trino/util/StatementUtils.java +++ b/core/trino-main/src/main/java/io/trino/util/StatementUtils.java @@ -54,6 +54,7 @@ import io.trino.sql.tree.RevokeRoles; import io.trino.sql.tree.Rollback; import io.trino.sql.tree.SetPath; +import io.trino.sql.tree.SetProperties; import io.trino.sql.tree.SetRole; import io.trino.sql.tree.SetSchemaAuthorization; import io.trino.sql.tree.SetSession; @@ -154,6 +155,7 @@ private StatementUtils() {} .put(SetRole.class, DATA_DEFINITION) .put(SetSchemaAuthorization.class, DATA_DEFINITION) .put(SetSession.class, DATA_DEFINITION) + .put(SetProperties.class, DATA_DEFINITION) .put(SetTableAuthorization.class, DATA_DEFINITION) .put(SetTimeZone.class, DATA_DEFINITION) .put(SetViewAuthorization.class, DATA_DEFINITION) diff --git a/core/trino-main/src/test/java/io/trino/connector/MockConnector.java b/core/trino-main/src/test/java/io/trino/connector/MockConnector.java index 05cc6f62136d..b131d97ed0ef 100644 --- a/core/trino-main/src/test/java/io/trino/connector/MockConnector.java +++ b/core/trino-main/src/test/java/io/trino/connector/MockConnector.java @@ -383,6 +383,9 @@ public void dropTable(ConnectorSession session, ConnectorTableHandle tableHandle @Override public void renameTable(ConnectorSession session, ConnectorTableHandle tableHandle, SchemaTableName newTableName) {} + @Override + public void setTableProperties(ConnectorSession session, ConnectorTableHandle tableHandle, Map properties) {} + @Override public void setTableComment(ConnectorSession session, ConnectorTableHandle tableHandle, Optional comment) {} diff --git a/core/trino-main/src/test/java/io/trino/execution/TestCreateTableTask.java b/core/trino-main/src/test/java/io/trino/execution/TestCreateTableTask.java index 68ccacb579b2..0b115bc9ad94 100644 --- a/core/trino-main/src/test/java/io/trino/execution/TestCreateTableTask.java +++ b/core/trino-main/src/test/java/io/trino/execution/TestCreateTableTask.java @@ -40,6 +40,7 @@ import io.trino.spi.type.Type; import io.trino.spi.type.TypeId; import io.trino.spi.type.TypeSignature; +import io.trino.sql.analyzer.FeaturesConfig; import io.trino.sql.planner.TestingConnectorTransactionHandle; import io.trino.sql.tree.ColumnDefinition; import io.trino.sql.tree.CreateTable; @@ -144,7 +145,7 @@ public void testCreateTableNotExistsTrue() ImmutableList.of(), Optional.empty()); - getFutureValue(new CreateTableTask().internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, emptyList(), output -> {})); + getFutureValue(new CreateTableTask(new FeaturesConfig()).internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, emptyList(), output -> {})); assertEquals(metadata.getCreateTableCallCount(), 1); } @@ -157,7 +158,7 @@ public void testCreateTableNotExistsFalse() ImmutableList.of(), Optional.empty()); - assertTrinoExceptionThrownBy(() -> getFutureValue(new CreateTableTask().internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, emptyList(), output -> {}))) + assertTrinoExceptionThrownBy(() -> getFutureValue(new CreateTableTask(new FeaturesConfig()).internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, emptyList(), output -> {}))) .hasErrorCode(ALREADY_EXISTS) .hasMessage("Table already exists"); @@ -173,7 +174,7 @@ public void testCreateTableWithMaterializedViewPropertyFails() ImmutableList.of(new Property(new Identifier("foo"), new StringLiteral("bar"))), Optional.empty()); - assertTrinoExceptionThrownBy(() -> getFutureValue(new CreateTableTask().internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, emptyList(), output -> {}))) + assertTrinoExceptionThrownBy(() -> getFutureValue(new CreateTableTask(new FeaturesConfig()).internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, emptyList(), output -> {}))) .hasErrorCode(INVALID_TABLE_PROPERTY) .hasMessage("Catalog 'catalog' does not support table property 'foo'"); @@ -190,7 +191,7 @@ public void testCreateWithNotNullColumns() new ColumnDefinition(identifier("c"), toSqlType(VARBINARY), false, emptyList(), Optional.empty())); CreateTable statement = new CreateTable(QualifiedName.of("test_table"), inputColumns, true, ImmutableList.of(), Optional.empty()); - getFutureValue(new CreateTableTask().internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, emptyList(), output -> {})); + getFutureValue(new CreateTableTask(new FeaturesConfig()).internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, emptyList(), output -> {})); assertEquals(metadata.getCreateTableCallCount(), 1); List columns = metadata.getReceivedTableMetadata().get(0).getColumns(); assertEquals(columns.size(), 3); @@ -223,7 +224,7 @@ public void testCreateWithUnsupportedConnectorThrowsWhenNotNull() Optional.empty()); assertTrinoExceptionThrownBy(() -> - getFutureValue(new CreateTableTask().internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, emptyList(), output -> {}))) + getFutureValue(new CreateTableTask(new FeaturesConfig()).internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, emptyList(), output -> {}))) .hasErrorCode(NOT_SUPPORTED) .hasMessage("Catalog 'catalog' does not support non-null column for column name 'b'"); } @@ -233,7 +234,7 @@ public void testCreateLike() { CreateTable statement = getCreatleLikeStatement(false); - getFutureValue(new CreateTableTask().internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, List.of(), output -> {})); + getFutureValue(new CreateTableTask(new FeaturesConfig()).internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, List.of(), output -> {})); assertEquals(metadata.getCreateTableCallCount(), 1); assertThat(metadata.getReceivedTableMetadata().get(0).getColumns()) @@ -246,7 +247,7 @@ public void testCreateLikeWithProperties() { CreateTable statement = getCreatleLikeStatement(true); - getFutureValue(new CreateTableTask().internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, List.of(), output -> {})); + getFutureValue(new CreateTableTask(new FeaturesConfig()).internalExecute(statement, metadata, new AllowAllAccessControl(), testSession, List.of(), output -> {})); assertEquals(metadata.getCreateTableCallCount(), 1); assertThat(metadata.getReceivedTableMetadata().get(0).getColumns()) @@ -263,7 +264,7 @@ public void testCreateLikeDenyPermission() TestingAccessControlManager accessControl = new TestingAccessControlManager(transactionManager, new EventListenerManager(new EventListenerConfig())); accessControl.deny(privilege("parent_table", SELECT_COLUMN)); - assertThatThrownBy(() -> getFutureValue(new CreateTableTask().internalExecute(statement, metadata, accessControl, testSession, List.of(), output -> {}))) + assertThatThrownBy(() -> getFutureValue(new CreateTableTask(new FeaturesConfig()).internalExecute(statement, metadata, accessControl, testSession, List.of(), output -> {}))) .isInstanceOf(AccessDeniedException.class) .hasMessageContaining("Cannot reference columns of table"); } @@ -276,7 +277,7 @@ public void testCreateLikeWithPropertiesDenyPermission() TestingAccessControlManager accessControl = new TestingAccessControlManager(transactionManager, new EventListenerManager(new EventListenerConfig())); accessControl.deny(privilege("parent_table", SHOW_CREATE_TABLE)); - assertThatThrownBy(() -> getFutureValue(new CreateTableTask().internalExecute(statement, metadata, accessControl, testSession, List.of(), output -> {}))) + assertThatThrownBy(() -> getFutureValue(new CreateTableTask(new FeaturesConfig()).internalExecute(statement, metadata, accessControl, testSession, List.of(), output -> {}))) .isInstanceOf(AccessDeniedException.class) .hasMessageContaining("Cannot reference properties of table"); } diff --git a/core/trino-main/src/test/java/io/trino/metadata/AbstractMockMetadata.java b/core/trino-main/src/test/java/io/trino/metadata/AbstractMockMetadata.java index cc48b4900f38..6eb4f04de582 100644 --- a/core/trino-main/src/test/java/io/trino/metadata/AbstractMockMetadata.java +++ b/core/trino-main/src/test/java/io/trino/metadata/AbstractMockMetadata.java @@ -246,6 +246,12 @@ public void renameTable(Session session, TableHandle tableHandle, QualifiedObjec throw new UnsupportedOperationException(); } + @Override + public void setTableProperties(Session session, TableHandle tableHandle, Map properties) + { + throw new UnsupportedOperationException(); + } + @Override public void setTableComment(Session session, TableHandle tableHandle, Optional comment) { diff --git a/core/trino-main/src/test/java/io/trino/security/TestFileBasedSystemAccessControl.java b/core/trino-main/src/test/java/io/trino/security/TestFileBasedSystemAccessControl.java index 3d84f00b7c8f..af11db997865 100644 --- a/core/trino-main/src/test/java/io/trino/security/TestFileBasedSystemAccessControl.java +++ b/core/trino-main/src/test/java/io/trino/security/TestFileBasedSystemAccessControl.java @@ -327,6 +327,7 @@ public void testTableOperations() accessControlManager.checkCanCreateViewWithSelectFromColumns(aliceContext, aliceTable, ImmutableSet.of()); accessControlManager.checkCanInsertIntoTable(aliceContext, aliceTable); accessControlManager.checkCanDeleteFromTable(aliceContext, aliceTable); + accessControlManager.checkCanSetTableProperties(aliceContext, aliceTable, ImmutableMap.of()); accessControlManager.checkCanAddColumns(aliceContext, aliceTable); accessControlManager.checkCanRenameColumn(aliceContext, aliceTable); @@ -336,6 +337,7 @@ public void testTableOperations() accessControlManager.checkCanCreateViewWithSelectFromColumns(aliceContext, staffTable, ImmutableSet.of()); accessControlManager.checkCanInsertIntoTable(aliceContext, staffTable); accessControlManager.checkCanDeleteFromTable(aliceContext, staffTable); + accessControlManager.checkCanSetTableProperties(aliceContext, staffTable, ImmutableMap.of()); accessControlManager.checkCanAddColumns(aliceContext, staffTable); accessControlManager.checkCanRenameColumn(aliceContext, staffTable); @@ -357,6 +359,9 @@ public void testTableOperations() assertThatThrownBy(() -> accessControlManager.checkCanDeleteFromTable(bobContext, aliceTable)) .isInstanceOf(AccessDeniedException.class) .hasMessage("Access Denied: Cannot access catalog alice-catalog"); + assertThatThrownBy(() -> accessControlManager.checkCanSetTableProperties(bobContext, aliceTable, ImmutableMap.of())) + .isInstanceOf(AccessDeniedException.class) + .hasMessage("Access Denied: Cannot access catalog alice-catalog"); assertThatThrownBy(() -> accessControlManager.checkCanAddColumns(bobContext, aliceTable)) .isInstanceOf(AccessDeniedException.class) .hasMessage("Access Denied: Cannot access catalog alice-catalog"); @@ -370,6 +375,7 @@ public void testTableOperations() accessControlManager.checkCanCreateViewWithSelectFromColumns(bobContext, staffTable, ImmutableSet.of()); accessControlManager.checkCanInsertIntoTable(bobContext, staffTable); accessControlManager.checkCanDeleteFromTable(bobContext, staffTable); + accessControlManager.checkCanSetTableProperties(bobContext, staffTable, ImmutableMap.of()); accessControlManager.checkCanAddColumns(bobContext, staffTable); accessControlManager.checkCanRenameColumn(bobContext, staffTable); @@ -391,6 +397,9 @@ public void testTableOperations() assertThatThrownBy(() -> accessControlManager.checkCanDeleteFromTable(nonAsciiContext, aliceTable)) .isInstanceOf(AccessDeniedException.class) .hasMessage("Access Denied: Cannot access catalog alice-catalog"); + assertThatThrownBy(() -> accessControlManager.checkCanSetTableProperties(nonAsciiContext, aliceTable, ImmutableMap.of())) + .isInstanceOf(AccessDeniedException.class) + .hasMessage("Access Denied: Cannot access catalog alice-catalog"); assertThatThrownBy(() -> accessControlManager.checkCanAddColumns(nonAsciiContext, aliceTable)) .isInstanceOf(AccessDeniedException.class) .hasMessage("Access Denied: Cannot access catalog alice-catalog"); @@ -416,6 +425,9 @@ public void testTableOperations() assertThatThrownBy(() -> accessControlManager.checkCanDeleteFromTable(nonAsciiContext, staffTable)) .isInstanceOf(AccessDeniedException.class) .hasMessage("Access Denied: Cannot access catalog staff-catalog"); + assertThatThrownBy(() -> accessControlManager.checkCanSetTableProperties(nonAsciiContext, staffTable, ImmutableMap.of())) + .isInstanceOf(AccessDeniedException.class) + .hasMessage("Access Denied: Cannot access catalog staff-catalog"); assertThatThrownBy(() -> accessControlManager.checkCanAddColumns(nonAsciiContext, staffTable)) .isInstanceOf(AccessDeniedException.class) .hasMessage("Access Denied: Cannot access catalog staff-catalog"); @@ -460,6 +472,11 @@ public void testTableOperationsReadOnly() })).isInstanceOf(AccessDeniedException.class) .hasMessage("Access Denied: Cannot delete from table alice-catalog.schema.table"); + assertThatThrownBy(() -> transaction(transactionManager, accessControlManager).execute(transactionId -> { + accessControlManager.checkCanSetTableProperties(new SecurityContext(transactionId, alice, queryId), aliceTable, ImmutableMap.of()); + })).isInstanceOf(AccessDeniedException.class) + .hasMessage("Access Denied: Cannot set table properties to alice-catalog.schema.table"); + assertThatThrownBy(() -> transaction(transactionManager, accessControlManager).execute(transactionId -> { accessControlManager.checkCanAddColumns(new SecurityContext(transactionId, alice, queryId), aliceTable); })).isInstanceOf(AccessDeniedException.class) diff --git a/core/trino-main/src/test/java/io/trino/sql/analyzer/TestFeaturesConfig.java b/core/trino-main/src/test/java/io/trino/sql/analyzer/TestFeaturesConfig.java index fcb7b26f88e6..9fbbe3829ae6 100644 --- a/core/trino-main/src/test/java/io/trino/sql/analyzer/TestFeaturesConfig.java +++ b/core/trino-main/src/test/java/io/trino/sql/analyzer/TestFeaturesConfig.java @@ -114,7 +114,8 @@ public void testDefaults() .setUseTableScanNodePartitioning(true) .setTableScanNodePartitioningMinBucketToTaskRatio(0.5) .setMergeProjectWithValues(true) - .setLegacyCatalogRoles(false)); + .setLegacyCatalogRoles(false) + .setDisableSetPropertiesSecurityCheckForCreateDdl(false)); } @Test @@ -194,6 +195,7 @@ public void testExplicitPropertyMappings() .put("optimizer.table-scan-node-partitioning-min-bucket-to-task-ratio", "0.0") .put("optimizer.merge-project-with-values", "false") .put("deprecated.legacy-catalog-roles", "true") + .put("deprecated.disable-set-properties-security-check-for-create-ddl", "true") .build(); FeaturesConfig expected = new FeaturesConfig() @@ -269,7 +271,8 @@ public void testExplicitPropertyMappings() .setUseTableScanNodePartitioning(false) .setTableScanNodePartitioningMinBucketToTaskRatio(0.0) .setMergeProjectWithValues(false) - .setLegacyCatalogRoles(true); + .setLegacyCatalogRoles(true) + .setDisableSetPropertiesSecurityCheckForCreateDdl(true); assertFullMapping(properties, expected); } } diff --git a/core/trino-parser/src/main/antlr4/io/trino/sql/parser/SqlBase.g4 b/core/trino-parser/src/main/antlr4/io/trino/sql/parser/SqlBase.g4 index 0167fa00907c..93ddf7fc7c82 100644 --- a/core/trino-parser/src/main/antlr4/io/trino/sql/parser/SqlBase.g4 +++ b/core/trino-parser/src/main/antlr4/io/trino/sql/parser/SqlBase.g4 @@ -70,6 +70,7 @@ statement | ALTER TABLE (IF EXISTS)? tableName=qualifiedName DROP COLUMN (IF EXISTS)? column=qualifiedName #dropColumn | ALTER TABLE tableName=qualifiedName SET AUTHORIZATION principal #setTableAuthorization + | ALTER TABLE tableName=qualifiedName SET PROPERTIES properties #setTableProperties | ANALYZE qualifiedName (WITH properties)? #analyze | CREATE (OR REPLACE)? MATERIALIZED VIEW (IF NOT EXISTS)? qualifiedName diff --git a/core/trino-parser/src/main/java/io/trino/sql/SqlFormatter.java b/core/trino-parser/src/main/java/io/trino/sql/SqlFormatter.java index 853619175fc8..d6b2f61ea5ed 100644 --- a/core/trino-parser/src/main/java/io/trino/sql/SqlFormatter.java +++ b/core/trino-parser/src/main/java/io/trino/sql/SqlFormatter.java @@ -97,6 +97,7 @@ import io.trino.sql.tree.Select; import io.trino.sql.tree.SelectItem; import io.trino.sql.tree.SetPath; +import io.trino.sql.tree.SetProperties; import io.trino.sql.tree.SetRole; import io.trino.sql.tree.SetSchemaAuthorization; import io.trino.sql.tree.SetSession; @@ -1248,12 +1249,7 @@ private String formatPropertiesSingleLine(List properties) return ""; } - String propertyList = properties.stream() - .map(element -> formatExpression(element.getName()) + " = " + - formatExpression(element.getValue())) - .collect(joining(", ")); - - return " WITH ( " + propertyList + " )"; + return " WITH ( " + joinProperties(properties) + " )"; } private String formatColumnDefinition(ColumnDefinition column) @@ -1323,7 +1319,27 @@ protected Void visitRenameTable(RenameTable node, Integer indent) } @Override - protected Void visitComment(Comment node, Integer indent) + protected Void visitSetProperties(SetProperties node, Integer context) + { + builder.append("ALTER TABLE "); + builder.append(node.getName()) + .append(" SET PROPERTIES ( "); + builder.append(joinProperties(node.getProperties())); + builder.append(" )"); + + return null; + } + + private String joinProperties(List properties) + { + return properties.stream() + .map(element -> formatExpression(element.getName()) + " = " + + formatExpression(element.getValue())) + .collect(joining(", ")); + } + + @Override + protected Void visitComment(Comment node, Integer context) { String comment = node.getComment().isPresent() ? formatStringLiteral(node.getComment().get()) : "NULL"; diff --git a/core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java b/core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java index 2f9122f7c206..41a4d6d826e4 100644 --- a/core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java +++ b/core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java @@ -176,6 +176,7 @@ import io.trino.sql.tree.Select; import io.trino.sql.tree.SelectItem; import io.trino.sql.tree.SetPath; +import io.trino.sql.tree.SetProperties; import io.trino.sql.tree.SetRole; import io.trino.sql.tree.SetSchemaAuthorization; import io.trino.sql.tree.SetSession; @@ -565,6 +566,17 @@ public Node visitRenameTable(SqlBaseParser.RenameTableContext context) return new RenameTable(getLocation(context), getQualifiedName(context.from), getQualifiedName(context.to), context.EXISTS() != null); } + @Override + public Node visitSetTableProperties(SqlBaseParser.SetTablePropertiesContext context) + { + List properties = ImmutableList.of(); + if (context.properties() != null) { + properties = visit(context.properties().property(), Property.class); + } + + return new SetProperties(getLocation(context), SetProperties.Type.TABLE, getQualifiedName(context.qualifiedName()), properties); + } + @Override public Node visitCommentTable(SqlBaseParser.CommentTableContext context) { diff --git a/core/trino-parser/src/main/java/io/trino/sql/tree/AstVisitor.java b/core/trino-parser/src/main/java/io/trino/sql/tree/AstVisitor.java index a064856ae61d..f5cd29efcd3c 100644 --- a/core/trino-parser/src/main/java/io/trino/sql/tree/AstVisitor.java +++ b/core/trino-parser/src/main/java/io/trino/sql/tree/AstVisitor.java @@ -637,6 +637,11 @@ protected R visitSetViewAuthorization(SetViewAuthorization node, C context) return visitStatement(node, context); } + protected R visitSetProperties(SetProperties node, C context) + { + return visitStatement(node, context); + } + protected R visitComment(Comment node, C context) { return visitStatement(node, context); diff --git a/core/trino-parser/src/main/java/io/trino/sql/tree/SetProperties.java b/core/trino-parser/src/main/java/io/trino/sql/tree/SetProperties.java new file mode 100644 index 000000000000..69153551fd03 --- /dev/null +++ b/core/trino-parser/src/main/java/io/trino/sql/tree/SetProperties.java @@ -0,0 +1,112 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.sql.tree; + +import com.google.common.collect.ImmutableList; + +import java.util.List; +import java.util.Objects; +import java.util.Optional; + +import static com.google.common.base.MoreObjects.toStringHelper; +import static java.util.Objects.requireNonNull; + +public final class SetProperties + extends Statement +{ + public enum Type + { + TABLE + } + + private final Type type; + private final QualifiedName name; + private final List properties; + + public SetProperties(Type type, QualifiedName name, List properties) + { + this(Optional.empty(), type, name, properties); + } + + public SetProperties(NodeLocation location, Type type, QualifiedName name, List properties) + { + this(Optional.of(location), type, name, properties); + } + + private SetProperties(Optional location, Type type, QualifiedName name, List properties) + { + super(location); + this.type = requireNonNull(type, "type is null"); + this.name = requireNonNull(name, "name is null"); + this.properties = ImmutableList.copyOf(requireNonNull(properties, "properties is null")); + } + + public Type getType() + { + return type; + } + + public QualifiedName getName() + { + return name; + } + + public List getProperties() + { + return properties; + } + + @Override + public R accept(AstVisitor visitor, C context) + { + return visitor.visitSetProperties(this, context); + } + + @Override + public List getChildren() + { + return ImmutableList.of(); + } + + @Override + public int hashCode() + { + return Objects.hash(type, name, properties); + } + + @Override + public boolean equals(Object obj) + { + if (this == obj) { + return true; + } + if ((obj == null) || (getClass() != obj.getClass())) { + return false; + } + SetProperties o = (SetProperties) obj; + return type == o.type && + Objects.equals(name, o.name) && + Objects.equals(properties, o.properties); + } + + @Override + public String toString() + { + return toStringHelper(this) + .add("type", type) + .add("name", name) + .add("properties", properties) + .toString(); + } +} diff --git a/core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java b/core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java index 9fb6306255a5..f7aba4ecd714 100644 --- a/core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java +++ b/core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java @@ -145,6 +145,7 @@ import io.trino.sql.tree.Select; import io.trino.sql.tree.SelectItem; import io.trino.sql.tree.SetPath; +import io.trino.sql.tree.SetProperties; import io.trino.sql.tree.SetRole; import io.trino.sql.tree.SetSession; import io.trino.sql.tree.SetTableAuthorization; @@ -1804,6 +1805,19 @@ public void testRenameTable() assertStatement("ALTER TABLE IF EXISTS a RENAME TO b", new RenameTable(QualifiedName.of("a"), QualifiedName.of("b"), true)); } + @Test + public void testSetTableProperties() + { + assertStatement("ALTER TABLE a SET PROPERTIES (foo='bar')", new SetProperties(SetProperties.Type.TABLE, QualifiedName.of("a"), ImmutableList.of(new Property(new Identifier("foo"), new StringLiteral("bar"))))); + assertStatement("ALTER TABLE a SET PROPERTIES (foo=true)", new SetProperties(SetProperties.Type.TABLE, QualifiedName.of("a"), ImmutableList.of(new Property(new Identifier("foo"), new BooleanLiteral("true"))))); + assertStatement("ALTER TABLE a SET PROPERTIES (foo=123)", new SetProperties(SetProperties.Type.TABLE, QualifiedName.of("a"), ImmutableList.of(new Property(new Identifier("foo"), new LongLiteral("123"))))); + assertStatement("ALTER TABLE a SET PROPERTIES (foo=123, bar=456)", new SetProperties(SetProperties.Type.TABLE, QualifiedName.of("a"), ImmutableList.of(new Property(new Identifier("foo"), new LongLiteral("123")), new Property(new Identifier("bar"), new LongLiteral("456"))))); + assertStatement("ALTER TABLE a SET PROPERTIES (\" s p a c e \"='bar')", new SetProperties(SetProperties.Type.TABLE, QualifiedName.of("a"), ImmutableList.of(new Property(new Identifier(" s p a c e "), new StringLiteral("bar"))))); + + assertStatementIsInvalid("ALTER TABLE a SET PROPERTIES ()") + .withMessage("line 1:31: mismatched input ')'. Expecting: "); + } + @Test public void testCommentTable() { diff --git a/core/trino-parser/src/test/java/io/trino/sql/parser/TestStatementBuilder.java b/core/trino-parser/src/test/java/io/trino/sql/parser/TestStatementBuilder.java index a1796face542..3861c2fc881a 100644 --- a/core/trino-parser/src/test/java/io/trino/sql/parser/TestStatementBuilder.java +++ b/core/trino-parser/src/test/java/io/trino/sql/parser/TestStatementBuilder.java @@ -205,6 +205,9 @@ public void testStatementBuilder() printStatement("alter table a.b.c rename column x to y"); + printStatement("alter table foo set properties (a='1')"); + printStatement("alter table a.b.c set properties (a=true, b=123, c='x')"); + printStatement("alter table a.b.c add column x bigint"); printStatement("alter table a.b.c add column x bigint comment 'large x'"); diff --git a/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorAccessControl.java b/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorAccessControl.java index 3a4477149f54..9c927826ab21 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorAccessControl.java +++ b/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorAccessControl.java @@ -18,6 +18,7 @@ import io.trino.spi.security.ViewExpression; import io.trino.spi.type.Type; +import java.util.Map; import java.util.Optional; import java.util.Set; @@ -56,6 +57,7 @@ import static io.trino.spi.security.AccessDeniedException.denySetRole; import static io.trino.spi.security.AccessDeniedException.denySetSchemaAuthorization; import static io.trino.spi.security.AccessDeniedException.denySetTableAuthorization; +import static io.trino.spi.security.AccessDeniedException.denySetTableProperties; import static io.trino.spi.security.AccessDeniedException.denySetViewAuthorization; import static io.trino.spi.security.AccessDeniedException.denyShowColumns; import static io.trino.spi.security.AccessDeniedException.denyShowCreateSchema; @@ -157,12 +159,24 @@ default void checkCanShowCreateTable(ConnectorSecurityContext context, SchemaTab * Check if identity is allowed to create the specified table. * * @throws io.trino.spi.security.AccessDeniedException if not allowed + * @deprecated use {@link #checkCanCreateTable(ConnectorSecurityContext context, SchemaTableName tableName, Map properties)} instead */ + @Deprecated default void checkCanCreateTable(ConnectorSecurityContext context, SchemaTableName tableName) { denyCreateTable(tableName.toString()); } + /** + * Check if identity is allowed to create the specified table with properties. + * + * @throws io.trino.spi.security.AccessDeniedException if not allowed + */ + default void checkCanCreateTable(ConnectorSecurityContext context, SchemaTableName tableName, Map properties) + { + denyCreateTable(tableName.toString()); + } + /** * Check if identity is allowed to drop the specified table. * @@ -183,6 +197,16 @@ default void checkCanRenameTable(ConnectorSecurityContext context, SchemaTableNa denyRenameTable(tableName.toString(), newTableName.toString()); } + /** + * Check if identity is allowed to set properties to the specified table. + * + * @throws io.trino.spi.security.AccessDeniedException if not allowed + */ + default void checkCanSetTableProperties(ConnectorSecurityContext context, SchemaTableName tableName, Map properties) + { + denySetTableProperties(tableName.toString()); + } + /** * Check if identity is allowed to comment the specified table. * diff --git a/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java b/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java index aec03b2c8e5a..cf4463ff8b1d 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java +++ b/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java @@ -325,6 +325,14 @@ default void renameTable(ConnectorSession session, ConnectorTableHandle tableHan throw new TrinoException(NOT_SUPPORTED, "This connector does not support renaming tables"); } + /** + * Set properties to the specified table + */ + default void setTableProperties(ConnectorSession session, ConnectorTableHandle tableHandle, Map properties) + { + throw new TrinoException(NOT_SUPPORTED, "This connector does not support setting table properties"); + } + /** * Comments to the specified table */ diff --git a/core/trino-spi/src/main/java/io/trino/spi/security/AccessDeniedException.java b/core/trino-spi/src/main/java/io/trino/spi/security/AccessDeniedException.java index 20529940713d..2293eb46d69c 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/security/AccessDeniedException.java +++ b/core/trino-spi/src/main/java/io/trino/spi/security/AccessDeniedException.java @@ -208,6 +208,16 @@ public static void denyRenameTable(String tableName, String newTableName, String throw new AccessDeniedException(format("Cannot rename table from %s to %s%s", tableName, newTableName, formatExtraInfo(extraInfo))); } + public static void denySetTableProperties(String tableName) + { + denySetTableProperties(tableName, null); + } + + public static void denySetTableProperties(String tableName, String extraInfo) + { + throw new AccessDeniedException(format("Cannot set table properties to %s%s", tableName, formatExtraInfo(extraInfo))); + } + public static void denyCommentTable(String tableName) { denyCommentTable(tableName, null); diff --git a/core/trino-spi/src/main/java/io/trino/spi/security/SystemAccessControl.java b/core/trino-spi/src/main/java/io/trino/spi/security/SystemAccessControl.java index 438b429c3fad..02160550a1d3 100644 --- a/core/trino-spi/src/main/java/io/trino/spi/security/SystemAccessControl.java +++ b/core/trino-spi/src/main/java/io/trino/spi/security/SystemAccessControl.java @@ -22,6 +22,7 @@ import java.security.Principal; import java.util.Locale; +import java.util.Map; import java.util.Optional; import java.util.Set; @@ -65,6 +66,7 @@ import static io.trino.spi.security.AccessDeniedException.denySetSchemaAuthorization; import static io.trino.spi.security.AccessDeniedException.denySetSystemSessionProperty; import static io.trino.spi.security.AccessDeniedException.denySetTableAuthorization; +import static io.trino.spi.security.AccessDeniedException.denySetTableProperties; import static io.trino.spi.security.AccessDeniedException.denySetUser; import static io.trino.spi.security.AccessDeniedException.denySetViewAuthorization; import static io.trino.spi.security.AccessDeniedException.denyShowColumns; @@ -283,12 +285,24 @@ default void checkCanShowCreateTable(SystemSecurityContext context, CatalogSchem * Check if identity is allowed to create the specified table in a catalog. * * @throws AccessDeniedException if not allowed + * @deprecated use {@link #checkCanCreateTable(SystemSecurityContext context, CatalogSchemaTableName table, Map properties)} instead */ + @Deprecated default void checkCanCreateTable(SystemSecurityContext context, CatalogSchemaTableName table) { denyCreateTable(table.toString()); } + /** + * Check if identity is allowed to create the specified table with properties in a catalog. + * + * @throws AccessDeniedException if not allowed + */ + default void checkCanCreateTable(SystemSecurityContext context, CatalogSchemaTableName table, Map properties) + { + denyCreateTable(table.toString()); + } + /** * Check if identity is allowed to drop the specified table in a catalog. * @@ -309,6 +323,16 @@ default void checkCanRenameTable(SystemSecurityContext context, CatalogSchemaTab denyRenameTable(table.toString(), newTable.toString()); } + /** + * Check if identity is allowed to alter properties to the specified table in a catalog. + * + * @throws AccessDeniedException if not allowed + */ + default void checkCanSetTableProperties(SystemSecurityContext context, CatalogSchemaTableName table, Map properties) + { + denySetTableProperties(table.toString()); + } + /** * Check if identity is allowed to comment the specified table in a catalog. * diff --git a/docs/src/main/sphinx/security/file-system-access-control.rst b/docs/src/main/sphinx/security/file-system-access-control.rst index e7b65ad2397b..7f6ee18102a8 100644 --- a/docs/src/main/sphinx/security/file-system-access-control.rst +++ b/docs/src/main/sphinx/security/file-system-access-control.rst @@ -66,6 +66,7 @@ ALTER SCHEMA ... SET AUTHORIZATION all owner CREATE TABLE all owner DROP TABLE all owner ALTER TABLE ... RENAME TO all owner* Ownership is required on both old and new tables +ALTER TABLE ... SET PROPERTIES all owner CREATE VIEW all owner DROP VIEW all owner ALTER VIEW ... RENAME TO all owner* Ownership is required on both old and new views diff --git a/docs/src/main/sphinx/sql/alter-table.rst b/docs/src/main/sphinx/sql/alter-table.rst index d87b9c784124..3ac68c0ca249 100644 --- a/docs/src/main/sphinx/sql/alter-table.rst +++ b/docs/src/main/sphinx/sql/alter-table.rst @@ -14,6 +14,7 @@ Synopsis ALTER TABLE [ IF EXISTS ] name DROP COLUMN [ IF EXISTS ] column_name ALTER TABLE [ IF EXISTS ] name RENAME COLUMN [ IF EXISTS ] old_name TO new_name ALTER TABLE name SET AUTHORIZATION ( user | USER user | ROLE role ) + ALTER TABLE SET PROPERTIES ( property_name = expression [, ...] ) Description ----------- @@ -69,6 +70,10 @@ Allow everyone with role public to drop and alter table ``people``:: ALTER TABLE people SET AUTHORIZATION ROLE PUBLIC +Set table properties (``x=y``) to table ``users``:: + + ALTER TABLE people SET PROPERTIES (x = 'y') + See also -------- diff --git a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorAccessControl.java b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorAccessControl.java index b5aaf346e943..0265c3dc233e 100644 --- a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorAccessControl.java +++ b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorAccessControl.java @@ -25,6 +25,7 @@ import javax.inject.Inject; +import java.util.Map; import java.util.Optional; import java.util.Set; @@ -115,6 +116,14 @@ public void checkCanCreateTable(ConnectorSecurityContext context, SchemaTableNam } } + @Override + public void checkCanCreateTable(ConnectorSecurityContext context, SchemaTableName tableName, Map properties) + { + try (ThreadContextClassLoader ignored = new ThreadContextClassLoader(classLoader)) { + delegate.checkCanCreateTable(context, tableName, properties); + } + } + @Override public void checkCanDropTable(ConnectorSecurityContext context, SchemaTableName tableName) { @@ -131,6 +140,14 @@ public void checkCanRenameTable(ConnectorSecurityContext context, SchemaTableNam } } + @Override + public void checkCanSetTableProperties(ConnectorSecurityContext context, SchemaTableName tableName, Map properties) + { + try (ThreadContextClassLoader ignored = new ThreadContextClassLoader(classLoader)) { + delegate.checkCanSetTableProperties(context, tableName, properties); + } + } + @Override public void checkCanSetTableComment(ConnectorSecurityContext context, SchemaTableName tableName) { diff --git a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorMetadata.java b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorMetadata.java index 0845c47d4fa4..a0c5f51786ff 100644 --- a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorMetadata.java +++ b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/classloader/ClassLoaderSafeConnectorMetadata.java @@ -392,6 +392,14 @@ public void renameTable(ConnectorSession session, ConnectorTableHandle tableHand } } + @Override + public void setTableProperties(ConnectorSession session, ConnectorTableHandle tableHandle, Map properties) + { + try (ThreadContextClassLoader ignored = new ThreadContextClassLoader(classLoader)) { + delegate.setTableProperties(session, tableHandle, properties); + } + } + @Override public void setTableComment(ConnectorSession session, ConnectorTableHandle tableHandle, Optional comment) { diff --git a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/AllowAllAccessControl.java b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/AllowAllAccessControl.java index aa14a5bb118d..856ccf629315 100644 --- a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/AllowAllAccessControl.java +++ b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/AllowAllAccessControl.java @@ -22,6 +22,7 @@ import io.trino.spi.security.ViewExpression; import io.trino.spi.type.Type; +import java.util.Map; import java.util.Optional; import java.util.Set; @@ -74,6 +75,11 @@ public void checkCanCreateTable(ConnectorSecurityContext context, SchemaTableNam { } + @Override + public void checkCanCreateTable(ConnectorSecurityContext context, SchemaTableName tableName, Map properties) + { + } + @Override public void checkCanDropTable(ConnectorSecurityContext context, SchemaTableName tableName) { @@ -89,6 +95,11 @@ public void checkCanSetTableComment(ConnectorSecurityContext context, SchemaTabl { } + @Override + public void checkCanSetTableProperties(ConnectorSecurityContext context, SchemaTableName tableName, Map properties) + { + } + @Override public void checkCanSetColumnComment(ConnectorSecurityContext context, SchemaTableName tableName) { diff --git a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/AllowAllSystemAccessControl.java b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/AllowAllSystemAccessControl.java index 9aaefeb100e8..d89e3c66cacc 100644 --- a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/AllowAllSystemAccessControl.java +++ b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/AllowAllSystemAccessControl.java @@ -163,6 +163,11 @@ public void checkCanCreateTable(SystemSecurityContext context, CatalogSchemaTabl { } + @Override + public void checkCanCreateTable(SystemSecurityContext context, CatalogSchemaTableName table, Map properties) + { + } + @Override public void checkCanDropTable(SystemSecurityContext context, CatalogSchemaTableName table) { @@ -173,6 +178,11 @@ public void checkCanRenameTable(SystemSecurityContext context, CatalogSchemaTabl { } + @Override + public void checkCanSetTableProperties(SystemSecurityContext context, CatalogSchemaTableName table, Map properties) + { + } + @Override public void checkCanSetTableComment(SystemSecurityContext context, CatalogSchemaTableName table) { diff --git a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedAccessControl.java b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedAccessControl.java index 3a1de62a8c79..f1d8a46f7fc1 100644 --- a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedAccessControl.java +++ b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedAccessControl.java @@ -28,6 +28,7 @@ import java.nio.file.Paths; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.function.Function; @@ -76,6 +77,7 @@ import static io.trino.spi.security.AccessDeniedException.denySetRole; import static io.trino.spi.security.AccessDeniedException.denySetSchemaAuthorization; import static io.trino.spi.security.AccessDeniedException.denySetTableAuthorization; +import static io.trino.spi.security.AccessDeniedException.denySetTableProperties; import static io.trino.spi.security.AccessDeniedException.denySetViewAuthorization; import static io.trino.spi.security.AccessDeniedException.denyShowColumns; import static io.trino.spi.security.AccessDeniedException.denyShowCreateSchema; @@ -189,6 +191,15 @@ public void checkCanCreateTable(ConnectorSecurityContext context, SchemaTableNam } } + @Override + public void checkCanCreateTable(ConnectorSecurityContext context, SchemaTableName tableName, Map properties) + { + // check if user will be an owner of the table after creation + if (!checkTablePermission(context, tableName, OWNERSHIP)) { + denyCreateTable(tableName.toString()); + } + } + @Override public void checkCanDropTable(ConnectorSecurityContext context, SchemaTableName tableName) { @@ -257,6 +268,14 @@ public void checkCanRenameTable(ConnectorSecurityContext context, SchemaTableNam } } + @Override + public void checkCanSetTableProperties(ConnectorSecurityContext context, SchemaTableName tableName, Map properties) + { + if (!checkTablePermission(context, tableName, OWNERSHIP)) { + denySetTableProperties(tableName.toString()); + } + } + @Override public void checkCanSetTableComment(ConnectorSecurityContext identity, SchemaTableName tableName) { diff --git a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedSystemAccessControl.java b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedSystemAccessControl.java index 2e3767611cbe..c4d8cb760ae8 100644 --- a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedSystemAccessControl.java +++ b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/FileBasedSystemAccessControl.java @@ -97,6 +97,7 @@ import static io.trino.spi.security.AccessDeniedException.denySetSchemaAuthorization; import static io.trino.spi.security.AccessDeniedException.denySetSystemSessionProperty; import static io.trino.spi.security.AccessDeniedException.denySetTableAuthorization; +import static io.trino.spi.security.AccessDeniedException.denySetTableProperties; import static io.trino.spi.security.AccessDeniedException.denySetUser; import static io.trino.spi.security.AccessDeniedException.denySetViewAuthorization; import static io.trino.spi.security.AccessDeniedException.denyShowColumns; @@ -507,6 +508,15 @@ public void checkCanCreateTable(SystemSecurityContext context, CatalogSchemaTabl } } + @Override + public void checkCanCreateTable(SystemSecurityContext context, CatalogSchemaTableName table, Map properties) + { + // check if user will be an owner of the table after creation + if (!checkTablePermission(context, table, OWNERSHIP)) { + denyCreateTable(table.toString()); + } + } + @Override public void checkCanDropTable(SystemSecurityContext context, CatalogSchemaTableName table) { @@ -524,6 +534,14 @@ public void checkCanRenameTable(SystemSecurityContext context, CatalogSchemaTabl } } + @Override + public void checkCanSetTableProperties(SystemSecurityContext context, CatalogSchemaTableName table, Map properties) + { + if (!checkTablePermission(context, table, OWNERSHIP)) { + denySetTableProperties(table.toString()); + } + } + @Override public void checkCanSetTableComment(SystemSecurityContext context, CatalogSchemaTableName table) { diff --git a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ForwardingConnectorAccessControl.java b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ForwardingConnectorAccessControl.java index 099628f7ab44..680a3cce4d32 100644 --- a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ForwardingConnectorAccessControl.java +++ b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ForwardingConnectorAccessControl.java @@ -22,6 +22,7 @@ import io.trino.spi.security.ViewExpression; import io.trino.spi.type.Type; +import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.function.Supplier; @@ -100,6 +101,12 @@ public void checkCanCreateTable(ConnectorSecurityContext context, SchemaTableNam delegate().checkCanCreateTable(context, tableName); } + @Override + public void checkCanCreateTable(ConnectorSecurityContext context, SchemaTableName tableName, Map properties) + { + delegate().checkCanCreateTable(context, tableName, properties); + } + @Override public void checkCanDropTable(ConnectorSecurityContext context, SchemaTableName tableName) { @@ -112,6 +119,12 @@ public void checkCanRenameTable(ConnectorSecurityContext context, SchemaTableNam delegate().checkCanRenameTable(context, tableName, newTableName); } + @Override + public void checkCanSetTableProperties(ConnectorSecurityContext context, SchemaTableName tableName, Map properties) + { + delegate().checkCanSetTableProperties(context, tableName, properties); + } + @Override public void checkCanSetTableComment(ConnectorSecurityContext context, SchemaTableName tableName) { diff --git a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ForwardingSystemAccessControl.java b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ForwardingSystemAccessControl.java index 395af5d2fa26..1f2c6a229177 100644 --- a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ForwardingSystemAccessControl.java +++ b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ForwardingSystemAccessControl.java @@ -26,6 +26,7 @@ import io.trino.spi.type.Type; import java.security.Principal; +import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.function.Supplier; @@ -170,6 +171,12 @@ public void checkCanCreateTable(SystemSecurityContext context, CatalogSchemaTabl delegate().checkCanCreateTable(context, table); } + @Override + public void checkCanCreateTable(SystemSecurityContext context, CatalogSchemaTableName table, Map properties) + { + delegate().checkCanCreateTable(context, table, properties); + } + @Override public void checkCanDropTable(SystemSecurityContext context, CatalogSchemaTableName table) { @@ -182,6 +189,12 @@ public void checkCanRenameTable(SystemSecurityContext context, CatalogSchemaTabl delegate().checkCanRenameTable(context, table, newTable); } + @Override + public void checkCanSetTableProperties(SystemSecurityContext context, CatalogSchemaTableName table, Map properties) + { + delegate().checkCanSetTableProperties(context, table, properties); + } + @Override public void checkCanSetTableComment(SystemSecurityContext context, CatalogSchemaTableName table) { diff --git a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ReadOnlyAccessControl.java b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ReadOnlyAccessControl.java index c313c073645d..8def3c94c1e4 100644 --- a/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ReadOnlyAccessControl.java +++ b/lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/security/ReadOnlyAccessControl.java @@ -19,6 +19,7 @@ import io.trino.spi.security.Privilege; import io.trino.spi.security.TrinoPrincipal; +import java.util.Map; import java.util.Set; import static io.trino.spi.security.AccessDeniedException.denyAddColumn; @@ -40,6 +41,7 @@ import static io.trino.spi.security.AccessDeniedException.denyRenameTable; import static io.trino.spi.security.AccessDeniedException.denyRenameView; import static io.trino.spi.security.AccessDeniedException.denyRevokeTablePrivilege; +import static io.trino.spi.security.AccessDeniedException.denySetTableProperties; import static io.trino.spi.security.AccessDeniedException.denyUpdateTableColumns; public class ReadOnlyAccessControl @@ -84,6 +86,12 @@ public void checkCanCreateTable(ConnectorSecurityContext context, SchemaTableNam denyCreateTable(tableName.toString()); } + @Override + public void checkCanCreateTable(ConnectorSecurityContext context, SchemaTableName tableName, Map properties) + { + denyCreateTable(tableName.toString()); + } + @Override public void checkCanDropTable(ConnectorSecurityContext context, SchemaTableName tableName) { @@ -96,6 +104,12 @@ public void checkCanRenameTable(ConnectorSecurityContext context, SchemaTableNam denyRenameTable(tableName.toString(), newTableName.toString()); } + @Override + public void checkCanSetTableProperties(ConnectorSecurityContext context, SchemaTableName tableName, Map properties) + { + denySetTableProperties(tableName.toString()); + } + @Override public void checkCanSetTableComment(ConnectorSecurityContext context, SchemaTableName tableName) { diff --git a/lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/security/TestFileBasedAccessControl.java b/lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/security/TestFileBasedAccessControl.java index 9366ff0668b1..adec6f567d4a 100644 --- a/lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/security/TestFileBasedAccessControl.java +++ b/lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/security/TestFileBasedAccessControl.java @@ -13,6 +13,7 @@ */ package io.trino.plugin.base.security; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import io.trino.plugin.base.CatalogName; import io.trino.spi.QueryId; @@ -282,10 +283,14 @@ public void testTableRules() accessControl.checkCanRenameMaterializedView(ADMIN, new SchemaTableName("bobschema", "bobmaterializedview"), new SchemaTableName("aliceschema", "newbobaterializedview")); accessControl.checkCanRenameMaterializedView(ALICE, new SchemaTableName("aliceschema", "alicevaterializediew"), new SchemaTableName("aliceschema", "newaliceaterializedview")); + accessControl.checkCanSetTableProperties(ADMIN, bobTable, ImmutableMap.of()); + accessControl.checkCanSetTableProperties(ALICE, aliceTable, ImmutableMap.of()); + assertDenied(() -> accessControl.checkCanInsertIntoTable(ALICE, bobTable)); assertDenied(() -> accessControl.checkCanDropTable(BOB, bobTable)); assertDenied(() -> accessControl.checkCanRenameTable(BOB, bobTable, new SchemaTableName("bobschema", "newbobtable"))); assertDenied(() -> accessControl.checkCanRenameTable(ALICE, aliceTable, new SchemaTableName("bobschema", "newalicetable"))); + assertDenied(() -> accessControl.checkCanSetTableProperties(BOB, bobTable, ImmutableMap.of())); assertDenied(() -> accessControl.checkCanInsertIntoTable(BOB, testTable)); assertDenied(() -> accessControl.checkCanSelectFromColumns(ADMIN, new SchemaTableName("secret", "secret"), ImmutableSet.of())); assertDenied(() -> accessControl.checkCanSelectFromColumns(JOE, new SchemaTableName("secret", "secret"), ImmutableSet.of())); diff --git a/lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/security/TestFileBasedSystemAccessControl.java b/lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/security/TestFileBasedSystemAccessControl.java index eea21541e646..065051d9d1e9 100644 --- a/lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/security/TestFileBasedSystemAccessControl.java +++ b/lib/trino-plugin-toolkit/src/test/java/io/trino/plugin/base/security/TestFileBasedSystemAccessControl.java @@ -103,6 +103,7 @@ public class TestFileBasedSystemAccessControl private static final String DROP_TABLE_ACCESS_DENIED_MESSAGE = "Access Denied: Cannot drop table .*"; private static final String CREATE_TABLE_ACCESS_DENIED_MESSAGE = "Access Denied: Cannot show create table for .*"; private static final String RENAME_TABLE_ACCESS_DENIED_MESSAGE = "Access Denied: Cannot rename table .*"; + private static final String SET_TABLE_PROPERTIES_ACCESS_DENIED_MESSAGE = "Access Denied: Cannot set table properties to .*"; private static final String CREATE_VIEW_ACCESS_DENIED_MESSAGE = "Access Denied: View owner '.*' cannot create view that selects from .*"; private static final String CREATE_MATERIALIZED_VIEW_ACCESS_DENIED_MESSAGE = "Access Denied: Cannot create materialized view .*"; private static final String DROP_MATERIALIZED_VIEW_ACCESS_DENIED_MESSAGE = "Access Denied: Cannot drop materialized view .*"; @@ -673,6 +674,16 @@ public void testTableRulesForCheckCanRenameTable() assertAccessDenied(() -> accessControl.checkCanRenameTable(ALICE, new CatalogSchemaTableName("some-catalog", "aliceschema", "alicetable"), new CatalogSchemaTableName("some-catalog", "bobschema", "newalicetable")), RENAME_TABLE_ACCESS_DENIED_MESSAGE); } + @Test + public void testTableRulesForCheckCanSetTableProperties() + { + SystemAccessControl accessControl = newFileBasedSystemAccessControl("file-based-system-access-table.json"); + + accessControl.checkCanSetTableProperties(ADMIN, new CatalogSchemaTableName("some-catalog", "bobschema", "bobtable"), ImmutableMap.of()); + accessControl.checkCanSetTableProperties(ALICE, new CatalogSchemaTableName("some-catalog", "aliceschema", "alicetable"), ImmutableMap.of()); + assertAccessDenied(() -> accessControl.checkCanSetTableProperties(BOB, new CatalogSchemaTableName("some-catalog", "bobschema", "bobtable"), ImmutableMap.of()), SET_TABLE_PROPERTIES_ACCESS_DENIED_MESSAGE); + } + @Test public void testCanSetUserOperations() { diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java index 3a0aeca2ff96..b0edcbfdd857 100644 --- a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java +++ b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java @@ -400,6 +400,13 @@ public void renameTable(ConnectorSession session, JdbcTableHandle handle, Schema invalidateTableCaches(newTableName); } + @Override + public void setTableProperties(ConnectorSession session, JdbcTableHandle handle, Map properties) + { + delegate.setTableProperties(session, handle, properties); + invalidateTableCaches(handle.asPlainTable().getSchemaTableName()); + } + @Override public void createTable(ConnectorSession session, ConnectorTableMetadata tableMetadata) { diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java index cef672bf8fd7..142c45794d7c 100644 --- a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java +++ b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java @@ -758,6 +758,14 @@ public void renameTable(ConnectorSession session, ConnectorTableHandle table, Sc jdbcClient.renameTable(session, tableHandle, newTableName); } + @Override + public void setTableProperties(ConnectorSession session, ConnectorTableHandle table, Map properties) + { + JdbcTableHandle tableHandle = (JdbcTableHandle) table; + verify(!tableHandle.isSynthetic(), "Not a table reference: %s", tableHandle); + jdbcClient.setTableProperties(session, tableHandle, properties); + } + @Override public TableStatistics getTableStatistics(ConnectorSession session, ConnectorTableHandle tableHandle, Constraint constraint) { diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/ForwardingJdbcClient.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/ForwardingJdbcClient.java index 566e659b7f08..171529d0243f 100644 --- a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/ForwardingJdbcClient.java +++ b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/ForwardingJdbcClient.java @@ -286,6 +286,12 @@ public void renameTable(ConnectorSession session, JdbcTableHandle handle, Schema delegate().renameTable(session, handle, newTableName); } + @Override + public void setTableProperties(ConnectorSession session, JdbcTableHandle handle, Map properties) + { + delegate().setTableProperties(session, handle, properties); + } + @Override public void createTable(ConnectorSession session, ConnectorTableMetadata tableMetadata) { diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcClient.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcClient.java index 15736b93110a..c605082bf79a 100644 --- a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcClient.java +++ b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcClient.java @@ -129,6 +129,11 @@ default void setColumnComment(ConnectorSession session, JdbcTableHandle handle, void renameTable(ConnectorSession session, JdbcTableHandle handle, SchemaTableName newTableName); + default void setTableProperties(ConnectorSession session, JdbcTableHandle handle, Map properties) + { + throw new TrinoException(NOT_SUPPORTED, "This connector does not support setting table properties"); + } + void createTable(ConnectorSession session, ConnectorTableMetadata tableMetadata); JdbcOutputTableHandle beginCreateTable(ConnectorSession session, ConnectorTableMetadata tableMetadata); diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/jmx/JdbcClientStats.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/jmx/JdbcClientStats.java index 5d4a1cec59ad..33432bf6a045 100644 --- a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/jmx/JdbcClientStats.java +++ b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/jmx/JdbcClientStats.java @@ -45,6 +45,7 @@ public final class JdbcClientStats private final JdbcApiStats getTableStatistics = new JdbcApiStats(); private final JdbcApiStats renameColumn = new JdbcApiStats(); private final JdbcApiStats renameTable = new JdbcApiStats(); + private final JdbcApiStats setTableProperties = new JdbcApiStats(); private final JdbcApiStats rollbackCreateTable = new JdbcApiStats(); private final JdbcApiStats schemaExists = new JdbcApiStats(); private final JdbcApiStats toPrestoType = new JdbcApiStats(); @@ -243,6 +244,13 @@ public JdbcApiStats getRenameTable() return renameTable; } + @Managed + @Nested + public JdbcApiStats getSetTableProperties() + { + return setTableProperties; + } + @Managed @Nested public JdbcApiStats getRollbackCreateTable() diff --git a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/jmx/StatisticsAwareJdbcClient.java b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/jmx/StatisticsAwareJdbcClient.java index f55ebb2de3c5..09afb44c5e35 100644 --- a/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/jmx/StatisticsAwareJdbcClient.java +++ b/plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/jmx/StatisticsAwareJdbcClient.java @@ -219,6 +219,12 @@ public void renameTable(ConnectorSession session, JdbcTableHandle handle, Schema stats.getRenameTable().wrap(() -> delegate().renameTable(session, handle, newTableName)); } + @Override + public void setTableProperties(ConnectorSession session, JdbcTableHandle handle, Map properties) + { + stats.getSetTableProperties().wrap(() -> delegate().setTableProperties(session, handle, properties)); + } + @Override public void createTable(ConnectorSession session, ConnectorTableMetadata tableMetadata) { diff --git a/plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java b/plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java index 19a3677684ed..889a794ef6ae 100644 --- a/plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java +++ b/plugin/trino-clickhouse/src/main/java/io/trino/plugin/clickhouse/ClickHouseClient.java @@ -66,7 +66,9 @@ import java.util.UUID; import java.util.function.BiFunction; +import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Strings.isNullOrEmpty; +import static io.trino.plugin.clickhouse.ClickHouseTableProperties.SAMPLE_BY_PROPERTY; import static io.trino.plugin.jdbc.DecimalConfig.DecimalMapping.ALLOW_OVERFLOW; import static io.trino.plugin.jdbc.DecimalSessionSessionProperties.getDecimalDefaultScale; import static io.trino.plugin.jdbc.DecimalSessionSessionProperties.getDecimalRounding; @@ -214,6 +216,27 @@ protected String createTableSql(RemoteTableName remoteTableName, List co return format("CREATE TABLE %s (%s) %s", quoted(remoteTableName), join(", ", columns), join(" ", tableOptions.build())); } + @Override + public void setTableProperties(ConnectorSession session, JdbcTableHandle handle, Map properties) + { + // TODO: Support other table properties + checkArgument(properties.size() == 1 && properties.containsKey(SAMPLE_BY_PROPERTY), "Only support setting 'sample_by' property"); + + ImmutableList.Builder tableOptions = ImmutableList.builder(); + ClickHouseTableProperties.getSampleBy(properties).ifPresent(value -> tableOptions.add("SAMPLE BY " + value)); + + try (Connection connection = connectionFactory.openConnection(session)) { + String sql = format( + "ALTER TABLE %s MODIFY %s", + quoted(handle.asPlainTable().getRemoteTableName()), + join(" ", tableOptions.build())); + execute(connection, sql); + } + catch (SQLException e) { + throw new TrinoException(JDBC_ERROR, e); + } + } + @Override protected String getColumnDefinitionSql(ConnectorSession session, ColumnMetadata column, String columnName) { diff --git a/plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java b/plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java index 855c2ddfaa27..a1777da4958e 100644 --- a/plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java +++ b/plugin/trino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java @@ -24,10 +24,21 @@ import org.testng.SkipException; import org.testng.annotations.Test; +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; import java.util.List; +import java.util.Map; import java.util.Optional; import static io.trino.plugin.clickhouse.ClickHouseQueryRunner.createClickHouseQueryRunner; +import static io.trino.plugin.clickhouse.ClickHouseTableProperties.ENGINE_PROPERTY; +import static io.trino.plugin.clickhouse.ClickHouseTableProperties.ORDER_BY_PROPERTY; +import static io.trino.plugin.clickhouse.ClickHouseTableProperties.PARTITION_BY_PROPERTY; +import static io.trino.plugin.clickhouse.ClickHouseTableProperties.PRIMARY_KEY_PROPERTY; +import static io.trino.plugin.clickhouse.ClickHouseTableProperties.SAMPLE_BY_PROPERTY; import static io.trino.spi.type.VarcharType.VARCHAR; import static io.trino.testing.MaterializedResult.resultBuilder; import static io.trino.testing.assertions.Assert.assertEquals; @@ -321,6 +332,46 @@ public void testTableProperty() "Invalid value for table property 'partition_by': .*"); } + @Test + public void testSetTableProperties() + throws Exception + { + try (TestTable table = new TestTable( + getQueryRunner()::execute, + "test_alter_table_properties", + "(p1 int NOT NULL, p2 int NOT NULL, x VARCHAR) WITH (engine = 'MergeTree', order_by = ARRAY['p1', 'p2'], primary_key = ARRAY['p1', 'p2'])")) { + assertThat(getTableProperties("tpch", table.getName())) + .containsExactlyEntriesOf(ImmutableMap.of( + "engine", "MergeTree", + "order_by", "p1, p2", + "partition_by", "", + "primary_key", "p1, p2", + "sample_by", "")); + + assertUpdate("ALTER TABLE " + table.getName() + " SET PROPERTIES (sample_by = 'p2')"); + assertThat(getTableProperties("tpch", table.getName())) + .containsExactlyEntriesOf(ImmutableMap.of( + "engine", "MergeTree", + "order_by", "p1, p2", + "partition_by", "", + "primary_key", "p1, p2", + "sample_by", "p2")); + } + } + + @Test + public void testAlterInvalidTableProperties() + { + try (TestTable table = new TestTable( + getQueryRunner()::execute, + "test_alter_table_properties", + "(p1 int NOT NULL, p2 int NOT NULL, x VARCHAR) WITH (engine = 'MergeTree', order_by = ARRAY['p1', 'p2'], primary_key = ARRAY['p1', 'p2'])")) { + assertQueryFails( + "ALTER TABLE " + table.getName() + " SET PROPERTIES (invalid_property = 'p2')", + "Catalog 'clickhouse' does not support table property 'invalid_property'"); + } + } + @Override protected TestTable createTableWithUnsupportedColumn() { @@ -391,4 +442,26 @@ protected SqlExecutor onRemoteDatabase() { return clickhouseServer::execute; } + + private Map getTableProperties(String schemaName, String tableName) + throws SQLException + { + String sql = "SELECT * FROM system.tables WHERE database = ? AND name = ?"; + try (Connection connection = DriverManager.getConnection(clickhouseServer.getJdbcUrl()); + PreparedStatement preparedStatement = connection.prepareStatement(sql)) { + preparedStatement.setString(1, schemaName); + preparedStatement.setString(2, tableName); + + ResultSet resultSet = preparedStatement.executeQuery(); + ImmutableMap.Builder properties = new ImmutableMap.Builder<>(); + while (resultSet.next()) { + properties.put(ENGINE_PROPERTY, resultSet.getString("engine")); + properties.put(ORDER_BY_PROPERTY, resultSet.getString("sorting_key")); + properties.put(PARTITION_BY_PROPERTY, resultSet.getString("partition_key")); + properties.put(PRIMARY_KEY_PROPERTY, resultSet.getString("primary_key")); + properties.put(SAMPLE_BY_PROPERTY, resultSet.getString("sampling_key")); + } + return properties.build(); + } + } } diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/security/LegacyAccessControl.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/security/LegacyAccessControl.java index 54f4a23ff710..09225332e952 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/security/LegacyAccessControl.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/security/LegacyAccessControl.java @@ -26,6 +26,7 @@ import javax.inject.Inject; +import java.util.Map; import java.util.Optional; import java.util.Set; @@ -114,6 +115,11 @@ public void checkCanCreateTable(ConnectorSecurityContext context, SchemaTableNam { } + @Override + public void checkCanCreateTable(ConnectorSecurityContext context, SchemaTableName tableName, Map properties) + { + } + @Override public void checkCanDropTable(ConnectorSecurityContext context, SchemaTableName tableName) { @@ -140,6 +146,11 @@ public void checkCanRenameTable(ConnectorSecurityContext context, SchemaTableNam } } + @Override + public void checkCanSetTableProperties(ConnectorSecurityContext context, SchemaTableName tableName, Map properties) + { + } + @Override public void checkCanSetTableComment(ConnectorSecurityContext context, SchemaTableName tableName) { diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/security/SqlStandardAccessControl.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/security/SqlStandardAccessControl.java index e62cdebd1324..4c834b5e900f 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/security/SqlStandardAccessControl.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/security/SqlStandardAccessControl.java @@ -34,6 +34,7 @@ import javax.inject.Inject; +import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.stream.Stream; @@ -84,6 +85,7 @@ import static io.trino.spi.security.AccessDeniedException.denySetRole; import static io.trino.spi.security.AccessDeniedException.denySetSchemaAuthorization; import static io.trino.spi.security.AccessDeniedException.denySetTableAuthorization; +import static io.trino.spi.security.AccessDeniedException.denySetTableProperties; import static io.trino.spi.security.AccessDeniedException.denySetViewAuthorization; import static io.trino.spi.security.AccessDeniedException.denyShowColumns; import static io.trino.spi.security.AccessDeniedException.denyShowCreateSchema; @@ -184,6 +186,14 @@ public void checkCanCreateTable(ConnectorSecurityContext context, SchemaTableNam } } + @Override + public void checkCanCreateTable(ConnectorSecurityContext context, SchemaTableName tableName, Map properties) + { + if (!isDatabaseOwner(context, tableName.getSchemaName())) { + denyCreateTable(tableName.toString()); + } + } + @Override public void checkCanDropTable(ConnectorSecurityContext context, SchemaTableName tableName) { @@ -200,6 +210,14 @@ public void checkCanRenameTable(ConnectorSecurityContext context, SchemaTableNam } } + @Override + public void checkCanSetTableProperties(ConnectorSecurityContext context, SchemaTableName tableName, Map properties) + { + if (!isTableOwner(context, tableName)) { + denySetTableProperties(tableName.toString()); + } + } + @Override public void checkCanSetTableComment(ConnectorSecurityContext context, SchemaTableName tableName) { diff --git a/service/trino-verifier/src/main/java/io/trino/verifier/VerifyCommand.java b/service/trino-verifier/src/main/java/io/trino/verifier/VerifyCommand.java index 7685e4ab9a38..abdaf1356248 100644 --- a/service/trino-verifier/src/main/java/io/trino/verifier/VerifyCommand.java +++ b/service/trino-verifier/src/main/java/io/trino/verifier/VerifyCommand.java @@ -47,6 +47,7 @@ import io.trino.sql.tree.RenameMaterializedView; import io.trino.sql.tree.RenameTable; import io.trino.sql.tree.RenameView; +import io.trino.sql.tree.SetProperties; import io.trino.sql.tree.ShowCatalogs; import io.trino.sql.tree.ShowColumns; import io.trino.sql.tree.ShowFunctions; @@ -425,6 +426,9 @@ private static QueryType statementToQueryType(Statement statement) if (statement instanceof RenameView) { return MODIFY; } + if (statement instanceof SetProperties) { + return MODIFY; + } if (statement instanceof Comment) { return MODIFY; } diff --git a/testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java b/testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java index d65410ca01aa..6bde8a726ac0 100644 --- a/testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java +++ b/testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java @@ -53,6 +53,7 @@ import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.RENAME_TABLE; import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.SELECT_COLUMN; import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.SET_SESSION; +import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.SET_TABLE_PROPERTIES; import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.SET_USER; import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.SHOW_COLUMNS; import static io.trino.testing.TestingAccessControlManager.TestingPrivilegeType.SHOW_CREATE_TABLE; @@ -98,6 +99,7 @@ public void testAccessControl() assertAccessDenied("INSERT INTO orders SELECT * FROM orders", "Cannot insert into table .*.orders.*", privilege("orders", INSERT_TABLE)); assertAccessDenied("DELETE FROM orders", "Cannot delete from table .*.orders.*", privilege("orders", DELETE_TABLE)); assertAccessDenied("CREATE TABLE foo AS SELECT * FROM orders", "Cannot create table .*.foo.*", privilege("foo", CREATE_TABLE)); + assertAccessDenied("ALTER TABLE orders SET PROPERTIES (field_length = 32)", "Cannot set table properties to .*.orders.*", privilege("orders", SET_TABLE_PROPERTIES)); assertAccessDenied("SELECT * FROM nation", "Cannot select from columns \\[nationkey, regionkey, name, comment] in table .*.nation.*", privilege("nation.nationkey", SELECT_COLUMN)); assertAccessDenied("SELECT * FROM (SELECT * FROM nation)", "Cannot select from columns \\[nationkey, regionkey, name, comment] in table .*.nation.*", privilege("nation.nationkey", SELECT_COLUMN)); assertAccessDenied("SELECT name FROM (SELECT * FROM nation)", "Cannot select from columns \\[nationkey, regionkey, name, comment] in table .*.nation.*", privilege("nation.nationkey", SELECT_COLUMN)); @@ -301,6 +303,14 @@ public void testAnalyzeAccessControl() assertAccessDenied("ANALYZE nation", "Cannot select from columns \\[.*nationkey.*] in table or view .*.nation", privilege("nation.nationkey", SELECT_COLUMN)); } + @Test + public void testSetTableProperties() + { + assertAccessDenied("ALTER TABLE orders SET PROPERTIES (field_length = 32)", "Cannot set table properties to .*.orders.*", privilege("orders", SET_TABLE_PROPERTIES)); + assertThatThrownBy(() -> getQueryRunner().execute(getSession(), "ALTER TABLE orders SET PROPERTIES (field_length = 32)")) + .hasMessageContaining("This connector does not support setting table properties"); + } + @Test public void testDeleteAccessControl() { @@ -327,6 +337,7 @@ public void testNonQueryAccessControl() assertAccessDenied("CREATE TABLE foo (pk bigint)", "Cannot create table .*.foo.*", privilege("foo", CREATE_TABLE)); assertAccessDenied("DROP TABLE orders", "Cannot drop table .*.orders.*", privilege("orders", DROP_TABLE)); assertAccessDenied("ALTER TABLE orders RENAME TO foo", "Cannot rename table .*.orders.* to .*.foo.*", privilege("orders", RENAME_TABLE)); + assertAccessDenied("ALTER TABLE orders SET PROPERTIES (field_length = 32)", "Cannot set table properties to .*.orders.*", privilege("orders", SET_TABLE_PROPERTIES)); assertAccessDenied("ALTER TABLE orders ADD COLUMN foo bigint", "Cannot add a column to table .*.orders.*", privilege("orders", ADD_COLUMN)); assertAccessDenied("ALTER TABLE orders DROP COLUMN foo", "Cannot drop a column from table .*.orders.*", privilege("orders", DROP_COLUMN)); assertAccessDenied("ALTER TABLE orders RENAME COLUMN orderkey TO foo", "Cannot rename a column in table .*.orders.*", privilege("orders", RENAME_COLUMN)); diff --git a/testing/trino-tests/src/test/java/io/trino/tests/AbstractTestEngineOnlyQueries.java b/testing/trino-tests/src/test/java/io/trino/tests/AbstractTestEngineOnlyQueries.java index d1df481b9115..29d27d5103f3 100644 --- a/testing/trino-tests/src/test/java/io/trino/tests/AbstractTestEngineOnlyQueries.java +++ b/testing/trino-tests/src/test/java/io/trino/tests/AbstractTestEngineOnlyQueries.java @@ -1479,6 +1479,7 @@ public void testDescribeOutputNonSelect() assertDescribeOutputEmpty("ALTER TABLE foo ADD COLUMN y bigint"); assertDescribeOutputEmpty("ALTER TABLE foo SET AUTHORIZATION bar"); assertDescribeOutputEmpty("ALTER TABLE foo RENAME TO bar"); + assertDescribeOutputEmpty("ALTER TABLE foo SET PROPERTIES ( x = 'y' )"); assertDescribeOutputEmpty("DROP TABLE foo"); assertDescribeOutputEmpty("CREATE VIEW foo AS SELECT * FROM nation"); assertDescribeOutputEmpty("DROP VIEW foo");