Remove create access checks without properties

Remove `ConnectorAccessControl` and `SystemAccessControl`'s `checkCanCreateTable` and `checkCanCreateMaterializedView` checks that do not take properties that were deprecated some time ago. Remove associated fallback configuration toggle. Among other things, this forces plugin implementors to implement the correct method. This is important, because the old method did not delegate to the new, nor vice versa.
trinodb · Feb 5, 2022 · 72ca4dc · 72ca4dc
1 parent 05f6e8f
commit 72ca4dc
Show file tree

Hide file tree

Showing 29 changed files with 44 additions and 376 deletions.
diff --git a/core/trino-main/src/main/java/io/trino/FeaturesConfig.java b/core/trino-main/src/main/java/io/trino/FeaturesConfig.java
@@ -44,6 +44,7 @@
 @DefunctConfig({
         "analyzer.experimental-syntax-enabled",
         "arrayagg.implementation",
+        "deprecated.disable-set-properties-security-check-for-create-ddl",
         "deprecated.group-by-uses-equal",
         "deprecated.legacy-char-to-varchar-coercion",
         "deprecated.legacy-join-using",
@@ -144,7 +145,6 @@ public class FeaturesConfig
     private int maxGroupingSets = 2048;
 
     private boolean legacyCatalogRoles;
-    private boolean disableSetPropertiesSecurityCheckForCreateDdl;
     private boolean incrementalHashArrayLoadFactorEnabled = true;
     private boolean allowSetViewAuthorization;
 
@@ -1061,18 +1061,6 @@ public FeaturesConfig setLegacyCatalogRoles(boolean 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;
-    }
-
     @Deprecated
     public boolean isIncrementalHashArrayLoadFactorEnabled()
     {

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
@@ -54,7 +54,6 @@ public class CreateMaterializedViewTask
     private final SqlParser sqlParser;
     private final AnalyzerFactory analyzerFactory;
     private final MaterializedViewPropertyManager materializedViewPropertyManager;
-    private final boolean disableSetPropertiesSecurityCheckForCreateDdl;
 
     @Inject
     public CreateMaterializedViewTask(
@@ -70,7 +69,6 @@ public CreateMaterializedViewTask(
         this.sqlParser = requireNonNull(sqlParser, "sqlParser is null");
         this.analyzerFactory = requireNonNull(analyzerFactory, "analyzerFactory is null");
         this.materializedViewPropertyManager = requireNonNull(materializedViewPropertyManager, "materializedViewPropertyManager is null");
-        this.disableSetPropertiesSecurityCheckForCreateDdl = featuresConfig.isDisableSetPropertiesSecurityCheckForCreateDdl();
     }
 
     @Override
@@ -121,12 +119,7 @@ public ListenableFuture<Void> execute(
                 Optional.empty(),
                 properties);
 
-        if (!disableSetPropertiesSecurityCheckForCreateDdl) {
-            accessControl.checkCanCreateMaterializedView(session.toSecurityContext(), name, properties);
-        }
-        else {
-            accessControl.checkCanCreateMaterializedView(session.toSecurityContext(), name);
-        }
+        accessControl.checkCanCreateMaterializedView(session.toSecurityContext(), name, properties);
         plannerContext.getMetadata().createMaterializedView(session, name, definition, statement.isReplace(), statement.isNotExists());
 
         stateMachine.setOutput(analysis.getTarget());

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
@@ -18,7 +18,6 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.util.concurrent.ListenableFuture;
-import io.trino.FeaturesConfig;
 import io.trino.Session;
 import io.trino.connector.CatalogName;
 import io.trino.execution.warnings.WarningCollector;
@@ -89,21 +88,18 @@ public class CreateTableTask
     private final AccessControl accessControl;
     private final ColumnPropertyManager columnPropertyManager;
     private final TablePropertyManager tablePropertyManager;
-    private final boolean disableSetPropertiesSecurityCheckForCreateDdl;
 
     @Inject
     public CreateTableTask(
             PlannerContext plannerContext,
             AccessControl accessControl,
             ColumnPropertyManager columnPropertyManager,
-            TablePropertyManager tablePropertyManager,
-            FeaturesConfig featuresConfig)
+            TablePropertyManager tablePropertyManager)
     {
         this.plannerContext = requireNonNull(plannerContext, "plannerContext is null");
         this.accessControl = requireNonNull(accessControl, "accessControl is null");
         this.columnPropertyManager = requireNonNull(columnPropertyManager, "columnPropertyManager is null");
         this.tablePropertyManager = requireNonNull(tablePropertyManager, "tablePropertyManager is null");
-        this.disableSetPropertiesSecurityCheckForCreateDdl = featuresConfig.isDisableSetPropertiesSecurityCheckForCreateDdl();
     }
 
     @Override
@@ -252,12 +248,7 @@ else if (element instanceof LikeClause) {
                 parameterLookup,
                 true);
 
-        if (!disableSetPropertiesSecurityCheckForCreateDdl) {
-            accessControl.checkCanCreateTable(session.toSecurityContext(), tableName, properties);
-        }
-        else {
-            accessControl.checkCanCreateTable(session.toSecurityContext(), tableName);
-        }
+        accessControl.checkCanCreateTable(session.toSecurityContext(), tableName, properties);
 
         Set<String> specifiedPropertyKeys = statement.getProperties().stream()
                 .map(property -> property.getName().getValue())

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
@@ -161,15 +161,6 @@ public interface AccessControl
      */
     void checkCanShowCreateTable(SecurityContext context, QualifiedObjectName tableName);
 
-    /**
-     * 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.
      *
@@ -338,15 +329,6 @@ default void checkCanSetViewAuthorization(SecurityContext context, QualifiedObje
      */
     void checkCanCreateViewWithSelectFromColumns(SecurityContext context, QualifiedObjectName tableName, Set<String> columnNames);
 
-    /**
-     * Check if identity is allowed to create the specified materialized view.
-     *
-     * @throws AccessDeniedException if not allowed
-     * @deprecated use {@link #checkCanCreateMaterializedView(SecurityContext, QualifiedObjectName, Map<String, Object>) instead}
-     */
-    @Deprecated
-    void checkCanCreateMaterializedView(SecurityContext context, QualifiedObjectName materializedViewName);
-
     /**
      * Check if identity is allowed to create the specified materialized view.
      *

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
@@ -414,19 +414,6 @@ public void checkCanShowCreateTable(SecurityContext securityContext, QualifiedOb
         catalogAuthorizationCheck(tableName.getCatalogName(), securityContext, (control, context) -> control.checkCanShowCreateTable(context, tableName.asSchemaTableName()));
     }
 
-    @Override
-    public void checkCanCreateTable(SecurityContext securityContext, QualifiedObjectName tableName)
-    {
-        requireNonNull(securityContext, "securityContext is null");
-        requireNonNull(tableName, "tableName is null");
-
-        checkCanAccessCatalog(securityContext, tableName.getCatalogName());
-
-        systemAuthorizationCheck(control -> control.checkCanCreateTable(securityContext.toSystemSecurityContext(), tableName.asCatalogSchemaTableName()));
-
-        catalogAuthorizationCheck(tableName.getCatalogName(), securityContext, (control, context) -> control.checkCanCreateTable(context, tableName.asSchemaTableName()));
-    }
-
     @Override
     public void checkCanCreateTable(SecurityContext securityContext, QualifiedObjectName tableName, Map<String, Object> properties)
     {
@@ -754,19 +741,6 @@ public void checkCanCreateViewWithSelectFromColumns(SecurityContext securityCont
         catalogAuthorizationCheck(tableName.getCatalogName(), securityContext, (control, context) -> control.checkCanCreateViewWithSelectFromColumns(context, tableName.asSchemaTableName(), columnNames));
     }
 
-    @Override
-    public void checkCanCreateMaterializedView(SecurityContext securityContext, QualifiedObjectName materializedViewName)
-    {
-        requireNonNull(securityContext, "securityContext is null");
-        requireNonNull(materializedViewName, "materializedViewName is null");
-
-        checkCanAccessCatalog(securityContext, materializedViewName.getCatalogName());
-
-        systemAuthorizationCheck(control -> control.checkCanCreateMaterializedView(securityContext.toSystemSecurityContext(), materializedViewName.asCatalogSchemaTableName()));
-
-        catalogAuthorizationCheck(materializedViewName.getCatalogName(), securityContext, (control, context) -> control.checkCanCreateMaterializedView(context, materializedViewName.asSchemaTableName()));
-    }
-
     @Override
     public void checkCanCreateMaterializedView(SecurityContext securityContext, QualifiedObjectName materializedViewName, Map<String, Object> properties)
     {

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
@@ -118,11 +118,6 @@ public void checkCanShowCreateTable(SecurityContext context, QualifiedObjectName
     {
     }
 
-    @Override
-    public void checkCanCreateTable(SecurityContext context, QualifiedObjectName tableName)
-    {
-    }
-
     @Override
     public void checkCanCreateTable(SecurityContext context, QualifiedObjectName tableName, Map<String, Object> properties)
     {
@@ -240,11 +235,6 @@ public void checkCanCreateViewWithSelectFromColumns(SecurityContext context, Qua
     {
     }
 
-    @Override
-    public void checkCanCreateMaterializedView(SecurityContext context, QualifiedObjectName materializedViewName)
-    {
-    }
-
     @Override
     public void checkCanCreateMaterializedView(SecurityContext context, QualifiedObjectName materializedViewName, Map<String, Object> properties)
     {

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
@@ -185,12 +185,6 @@ public void checkCanSetSchemaAuthorization(SecurityContext context, CatalogSchem
         denySetSchemaAuthorization(schemaName.toString(), principal);
     }
 
-    @Override
-    public void checkCanCreateTable(SecurityContext context, QualifiedObjectName tableName)
-    {
-        denyCreateTable(tableName.toString());
-    }
-
     @Override
     public void checkCanCreateTable(SecurityContext context, QualifiedObjectName tableName, Map<String, Object> properties)
     {
@@ -341,12 +335,6 @@ public void checkCanCreateViewWithSelectFromColumns(SecurityContext context, Qua
         denyCreateViewWithSelect(tableName.toString(), context.getIdentity());
     }
 
-    @Override
-    public void checkCanCreateMaterializedView(SecurityContext context, QualifiedObjectName materializedViewName)
-    {
-        denyCreateMaterializedView(materializedViewName.toString());
-    }
-
     @Override
     public void checkCanCreateMaterializedView(SecurityContext context, QualifiedObjectName materializedViewName, Map<String, Object> properties)
     {

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
@@ -154,12 +154,6 @@ public void checkCanShowCreateTable(SecurityContext context, QualifiedObjectName
         delegate().checkCanShowCreateTable(context, tableName);
     }
 
-    @Override
-    public void checkCanCreateTable(SecurityContext context, QualifiedObjectName tableName)
-    {
-        delegate().checkCanCreateTable(context, tableName);
-    }
-
     @Override
     public void checkCanCreateTable(SecurityContext context, QualifiedObjectName tableName, Map<String, Object> properties)
     {
@@ -298,12 +292,6 @@ public void checkCanCreateViewWithSelectFromColumns(SecurityContext context, Qua
         delegate().checkCanCreateViewWithSelectFromColumns(context, tableName, columnNames);
     }
 
-    @Override
-    public void checkCanCreateMaterializedView(SecurityContext context, QualifiedObjectName materializedViewName)
-    {
-        delegate().checkCanCreateMaterializedView(context, materializedViewName);
-    }
-
     @Override
     public void checkCanCreateMaterializedView(SecurityContext context, QualifiedObjectName materializedViewName, Map<String, Object> properties)
     {

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
@@ -104,13 +104,6 @@ public void checkCanShowCreateTable(ConnectorSecurityContext context, SchemaTabl
         accessControl.checkCanShowCreateTable(securityContext, getQualifiedObjectName(tableName));
     }
 
-    @Override
-    public void checkCanCreateTable(ConnectorSecurityContext context, SchemaTableName tableName)
-    {
-        checkArgument(context == null, "context must be null");
-        accessControl.checkCanCreateTable(securityContext, getQualifiedObjectName(tableName));
-    }
-
     @Override
     public void checkCanCreateTable(ConnectorSecurityContext context, SchemaTableName tableName, Map<String, Object> properties)
     {
@@ -279,13 +272,6 @@ public void checkCanCreateViewWithSelectFromColumns(ConnectorSecurityContext con
         accessControl.checkCanCreateViewWithSelectFromColumns(securityContext, getQualifiedObjectName(tableName), columnNames);
     }
 
-    @Override
-    public void checkCanCreateMaterializedView(ConnectorSecurityContext context, SchemaTableName materializedViewName)
-    {
-        checkArgument(context == null, "context must be null");
-        accessControl.checkCanCreateMaterializedView(securityContext, getQualifiedObjectName(materializedViewName));
-    }
-
     @Override
     public void checkCanCreateMaterializedView(ConnectorSecurityContext context, SchemaTableName materializedViewName, Map<String, Object> properties)
     {

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
@@ -813,7 +813,6 @@ protected Scope visitCreateTableAsSelect(CreateTableAsSelect node, Optional<Scop
                     accessControl,
                     analysis.getParameters(),
                     true);
-            // TODO respect FeaturesConfig.isDisableSetPropertiesSecurityCheckForCreateDdl
             accessControl.checkCanCreateTable(session.toSecurityContext(), targetTable, properties);
 
             // analyze the query that creates the table

diff --git a/core/trino-main/src/main/java/io/trino/testing/AllowAllAccessControlManager.java b/core/trino-main/src/main/java/io/trino/testing/AllowAllAccessControlManager.java
@@ -92,9 +92,6 @@ public void checkCanShowCreateSchema(SecurityContext context, CatalogSchemaName
     @Override
     public void checkCanShowCreateTable(SecurityContext context, QualifiedObjectName tableName) {}
 
-    @Override
-    public void checkCanCreateTable(SecurityContext context, QualifiedObjectName tableName) {}
-
     @Override
     public void checkCanCreateTable(SecurityContext context, QualifiedObjectName tableName, Map<String, Object> properties) {}
 
@@ -167,9 +164,6 @@ public void checkCanDropView(SecurityContext context, QualifiedObjectName viewNa
     @Override
     public void checkCanCreateViewWithSelectFromColumns(SecurityContext context, QualifiedObjectName tableName, Set<String> columnNames) {}
 
-    @Override
-    public void checkCanCreateMaterializedView(SecurityContext context, QualifiedObjectName materializedViewName) {}
-
     @Override
     public void checkCanCreateMaterializedView(SecurityContext context, QualifiedObjectName materializedViewName, Map<String, Object> properties) {}
 

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
@@ -334,17 +334,6 @@ public void checkCanShowCreateTable(SecurityContext context, QualifiedObjectName
         }
     }
 
-    @Override
-    public void checkCanCreateTable(SecurityContext context, QualifiedObjectName tableName)
-    {
-        if (shouldDenyPrivilege(context.getIdentity().getUser(), tableName.getObjectName(), CREATE_TABLE)) {
-            denyCreateTable(tableName.toString());
-        }
-        if (denyPrivileges.isEmpty()) {
-            super.checkCanCreateTable(context, tableName);
-        }
-    }
-
     @Override
     public void checkCanCreateTable(SecurityContext context, QualifiedObjectName tableName, Map<String, Object> properties)
     {
@@ -543,17 +532,6 @@ public void checkCanCreateViewWithSelectFromColumns(SecurityContext context, Qua
         }
     }
 
-    @Override
-    public void checkCanCreateMaterializedView(SecurityContext context, QualifiedObjectName materializedViewName)
-    {
-        if (shouldDenyPrivilege(context.getIdentity().getUser(), materializedViewName.getObjectName(), CREATE_MATERIALIZED_VIEW)) {
-            denyCreateMaterializedView(materializedViewName.toString());
-        }
-        if (denyPrivileges.isEmpty()) {
-            super.checkCanCreateMaterializedView(context, materializedViewName);
-        }
-    }
-
     @Override
     public void checkCanCreateMaterializedView(SecurityContext context, QualifiedObjectName materializedViewName, Map<String, Object> properties)
     {