Introduce hidden legacy_catalog_roles session property

Enable legacy role management syntax that assumed all roles are catalog scoped.
ribrewguy · Oct 5, 2021 · 8cc300b · 8cc300b
1 parent 39cf4fd
commit 8cc300b
Show file tree

Hide file tree

Showing 8 changed files with 20 additions and 62 deletions.
diff --git a/core/trino-main/src/main/java/io/trino/SystemSessionProperties.java b/core/trino-main/src/main/java/io/trino/SystemSessionProperties.java
@@ -143,6 +143,7 @@ public final class SystemSessionProperties
     public static final String MAX_UNACKNOWLEDGED_SPLITS_PER_TASK = "max_unacknowledged_splits_per_task";
     public static final String MERGE_PROJECT_WITH_VALUES = "merge_project_with_values";
     public static final String TIME_ZONE_ID = "time_zone_id";
+    public static final String LEGACY_CATALOG_ROLES = "legacy_catalog_roles";
 
     private final List<PropertyMetadata<?>> sessionProperties;
 
@@ -660,6 +661,11 @@ public SystemSessionProperties(
                                 getTimeZoneKey(value);
                             }
                         },
+                        true),
+                booleanProperty(
+                        LEGACY_CATALOG_ROLES,
+                        "Enable legacy role management syntax that assumed all roles are catalog scoped",
+                        featuresConfig.isLegacyCatalogRoles(),
                         true));
     }
 
@@ -1173,4 +1179,9 @@ public static Optional<String> getTimeZoneId(Session session)
     {
         return Optional.ofNullable(session.getSystemProperty(TIME_ZONE_ID, String.class));
     }
+
+    public static boolean isLegacyCatalogRoles(Session session)
+    {
+        return session.getSystemProperty(LEGACY_CATALOG_ROLES, Boolean.class);
+    }
 }
diff --git a/core/trino-main/src/main/java/io/trino/execution/CreateRoleTask.java b/core/trino-main/src/main/java/io/trino/execution/CreateRoleTask.java
@@ -19,14 +19,11 @@
 import io.trino.metadata.Metadata;
 import io.trino.security.AccessControl;
 import io.trino.spi.security.TrinoPrincipal;
-import io.trino.sql.analyzer.FeaturesConfig;
 import io.trino.sql.tree.CreateRole;
 import io.trino.sql.tree.Expression;
 import io.trino.sql.tree.Identifier;
 import io.trino.transaction.TransactionManager;
 
-import javax.inject.Inject;
-
 import java.util.List;
 import java.util.Optional;
 
@@ -41,14 +38,6 @@
 public class CreateRoleTask
         implements DataDefinitionTask<CreateRole>
 {
-    private final boolean legacyCatalogRoles;
-
-    @Inject
-    public CreateRoleTask(FeaturesConfig featuresConfig)
-    {
-        legacyCatalogRoles = featuresConfig.isLegacyCatalogRoles();
-    }
-
     @Override
     public String getName()
     {
@@ -66,7 +55,7 @@ public ListenableFuture<Void> execute(
             WarningCollector warningCollector)
     {
         Session session = stateMachine.getSession();
-        Optional<String> catalog = processRoleCommandCatalog(metadata, session, statement, statement.getCatalog().map(Identifier::getValue), legacyCatalogRoles);
+        Optional<String> catalog = processRoleCommandCatalog(metadata, session, statement, statement.getCatalog().map(Identifier::getValue));
         String role = statement.getName().getValue().toLowerCase(ENGLISH);
         Optional<TrinoPrincipal> grantor = statement.getGrantor().map(specification -> createPrincipal(session, specification));
         accessControl.checkCanCreateRole(session.toSecurityContext(), role, grantor, catalog);

diff --git a/core/trino-main/src/main/java/io/trino/execution/DropRoleTask.java b/core/trino-main/src/main/java/io/trino/execution/DropRoleTask.java
@@ -18,14 +18,11 @@
 import io.trino.execution.warnings.WarningCollector;
 import io.trino.metadata.Metadata;
 import io.trino.security.AccessControl;
-import io.trino.sql.analyzer.FeaturesConfig;
 import io.trino.sql.tree.DropRole;
 import io.trino.sql.tree.Expression;
 import io.trino.sql.tree.Identifier;
 import io.trino.transaction.TransactionManager;
 
-import javax.inject.Inject;
-
 import java.util.List;
 import java.util.Optional;
 
@@ -37,14 +34,6 @@
 public class DropRoleTask
         implements DataDefinitionTask<DropRole>
 {
-    private final boolean legacyCatalogRoles;
-
-    @Inject
-    public DropRoleTask(FeaturesConfig featuresConfig)
-    {
-        legacyCatalogRoles = featuresConfig.isLegacyCatalogRoles();
-    }
-
     @Override
     public String getName()
     {
@@ -62,7 +51,7 @@ public ListenableFuture<Void> execute(
             WarningCollector warningCollector)
     {
         Session session = stateMachine.getSession();
-        Optional<String> catalog = processRoleCommandCatalog(metadata, session, statement, statement.getCatalog().map(Identifier::getValue), legacyCatalogRoles);
+        Optional<String> catalog = processRoleCommandCatalog(metadata, session, statement, statement.getCatalog().map(Identifier::getValue));
         String role = statement.getName().getValue().toLowerCase(ENGLISH);
         accessControl.checkCanDropRole(session.toSecurityContext(), role, catalog);
         checkRoleExists(session, statement, metadata, role, catalog);

diff --git a/core/trino-main/src/main/java/io/trino/execution/GrantRolesTask.java b/core/trino-main/src/main/java/io/trino/execution/GrantRolesTask.java
@@ -20,14 +20,11 @@
 import io.trino.metadata.MetadataUtil;
 import io.trino.security.AccessControl;
 import io.trino.spi.security.TrinoPrincipal;
-import io.trino.sql.analyzer.FeaturesConfig;
 import io.trino.sql.tree.Expression;
 import io.trino.sql.tree.GrantRoles;
 import io.trino.sql.tree.Identifier;
 import io.trino.transaction.TransactionManager;
 
-import javax.inject.Inject;
-
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Locale;
@@ -44,14 +41,6 @@
 public class GrantRolesTask
         implements DataDefinitionTask<GrantRoles>
 {
-    private final boolean legacyCatalogRoles;
-
-    @Inject
-    public GrantRolesTask(FeaturesConfig featuresConfig)
-    {
-        legacyCatalogRoles = featuresConfig.isLegacyCatalogRoles();
-    }
-
     @Override
     public String getName()
     {
@@ -76,7 +65,7 @@ public ListenableFuture<Void> execute(
                 .collect(toImmutableSet());
         boolean adminOption = statement.isAdminOption();
         Optional<TrinoPrincipal> grantor = statement.getGrantor().map(specification -> createPrincipal(session, specification));
-        Optional<String> catalog = processRoleCommandCatalog(metadata, session, statement, statement.getCatalog().map(Identifier::getValue), legacyCatalogRoles);
+        Optional<String> catalog = processRoleCommandCatalog(metadata, session, statement, statement.getCatalog().map(Identifier::getValue));
 
         Set<String> specifiedRoles = new LinkedHashSet<>();
         specifiedRoles.addAll(roles);

diff --git a/core/trino-main/src/main/java/io/trino/execution/RevokeRolesTask.java b/core/trino-main/src/main/java/io/trino/execution/RevokeRolesTask.java
@@ -20,14 +20,11 @@
 import io.trino.metadata.MetadataUtil;
 import io.trino.security.AccessControl;
 import io.trino.spi.security.TrinoPrincipal;
-import io.trino.sql.analyzer.FeaturesConfig;
 import io.trino.sql.tree.Expression;
 import io.trino.sql.tree.Identifier;
 import io.trino.sql.tree.RevokeRoles;
 import io.trino.transaction.TransactionManager;
 
-import javax.inject.Inject;
-
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Locale;
@@ -44,14 +41,6 @@
 public class RevokeRolesTask
         implements DataDefinitionTask<RevokeRoles>
 {
-    private final boolean legacyCatalogRoles;
-
-    @Inject
-    public RevokeRolesTask(FeaturesConfig featuresConfig)
-    {
-        legacyCatalogRoles = featuresConfig.isLegacyCatalogRoles();
-    }
-
     @Override
     public String getName()
     {
@@ -76,7 +65,7 @@ public ListenableFuture<Void> execute(
                 .collect(toImmutableSet());
         boolean adminOption = statement.isAdminOption();
         Optional<TrinoPrincipal> grantor = statement.getGrantor().map(specification -> createPrincipal(session, specification));
-        Optional<String> catalog = processRoleCommandCatalog(metadata, session, statement, statement.getCatalog().map(Identifier::getValue), legacyCatalogRoles);
+        Optional<String> catalog = processRoleCommandCatalog(metadata, session, statement, statement.getCatalog().map(Identifier::getValue));
 
         Set<String> specifiedRoles = new LinkedHashSet<>();
         specifiedRoles.addAll(roles);

diff --git a/core/trino-main/src/main/java/io/trino/execution/SetRoleTask.java b/core/trino-main/src/main/java/io/trino/execution/SetRoleTask.java
@@ -22,14 +22,11 @@
 import io.trino.spi.security.RoleGrant;
 import io.trino.spi.security.SelectedRole;
 import io.trino.spi.security.TrinoPrincipal;
-import io.trino.sql.analyzer.FeaturesConfig;
 import io.trino.sql.tree.Expression;
 import io.trino.sql.tree.Identifier;
 import io.trino.sql.tree.SetRole;
 import io.trino.transaction.TransactionManager;
 
-import javax.inject.Inject;
-
 import java.util.List;
 import java.util.Optional;
 import java.util.Set;
@@ -45,14 +42,6 @@
 public class SetRoleTask
         implements DataDefinitionTask<SetRole>
 {
-    private final boolean legacyCatalogRoles;
-
-    @Inject
-    public SetRoleTask(FeaturesConfig featuresConfig)
-    {
-        legacyCatalogRoles = featuresConfig.isLegacyCatalogRoles();
-    }
-
     @Override
     public String getName()
     {
@@ -70,7 +59,7 @@ public ListenableFuture<Void> execute(
             WarningCollector warningCollector)
     {
         Session session = stateMachine.getSession();
-        Optional<String> catalog = processRoleCommandCatalog(metadata, session, statement, statement.getCatalog().map(Identifier::getValue), legacyCatalogRoles);
+        Optional<String> catalog = processRoleCommandCatalog(metadata, session, statement, statement.getCatalog().map(Identifier::getValue));
         if (statement.getType() == SetRole.Type.ROLE) {
             String role = statement.getRole().map(c -> c.getValue().toLowerCase(ENGLISH)).orElseThrow();
             if (!metadata.roleExists(session, role, catalog)) {

diff --git a/core/trino-main/src/main/java/io/trino/metadata/MetadataUtil.java b/core/trino-main/src/main/java/io/trino/metadata/MetadataUtil.java
@@ -36,6 +36,7 @@
 import java.util.Optional;
 
 import static com.google.common.base.Preconditions.checkArgument;
+import static io.trino.SystemSessionProperties.isLegacyCatalogRoles;
 import static io.trino.spi.StandardErrorCode.CATALOG_NOT_FOUND;
 import static io.trino.spi.StandardErrorCode.MISSING_CATALOG_NAME;
 import static io.trino.spi.StandardErrorCode.MISSING_SCHEMA_NAME;
@@ -216,8 +217,9 @@ public static void checkRoleExists(Session session, Node node, Metadata metadata
         }
     }
 
-    public static Optional<String> processRoleCommandCatalog(Metadata metadata, Session session, Node node, Optional<String> catalog, boolean legacyCatalogRoles)
+    public static Optional<String> processRoleCommandCatalog(Metadata metadata, Session session, Node node, Optional<String> catalog)
     {
+        boolean legacyCatalogRoles = isLegacyCatalogRoles(session);
         // old role commands use only supported catalog roles and used session catalog as the default
         if (catalog.isEmpty() && legacyCatalogRoles) {
             catalog = session.getCatalog();

diff --git a/core/trino-main/src/test/java/io/trino/execution/TestSetRoleTask.java b/core/trino-main/src/test/java/io/trino/execution/TestSetRoleTask.java
@@ -202,7 +202,7 @@ private QueryStateMachine executeSetRole(String statement)
                 metadata,
                 WarningCollector.NOOP,
                 Optional.empty());
-        new SetRoleTask(new FeaturesConfig()).execute(setRole, transactionManager, metadata, accessControl, stateMachine, ImmutableList.of(), WarningCollector.NOOP);
+        new SetRoleTask().execute(setRole, transactionManager, metadata, accessControl, stateMachine, ImmutableList.of(), WarningCollector.NOOP);
         return stateMachine;
     }
 }