From fcfc1144362e0ff59653b23bfcff90bfacdb88a9 Mon Sep 17 00:00:00 2001 From: Rohit Jain Date: Fri, 16 Feb 2024 15:43:43 -0800 Subject: [PATCH] Add config for default view security mode Presto has a default view creation security mode as 'DEFINER'. However, some administrators may prefer to use 'INVOKER' as the default. To address this, a new configuration flag `default-view-security-mode` has been added that allows the default view security mode to be changed according to the administrator's preference. --- .../src/main/sphinx/sql/create-view.rst | 7 ++++ .../presto/SystemSessionProperties.java | 21 +++++++++- .../presto/execution/CreateViewTask.java | 6 ++- .../presto/sql/analyzer/FeaturesConfig.java | 16 ++++++++ .../sql/analyzer/StatementAnalyzer.java | 1 + .../sql/analyzer/TestFeaturesConfig.java | 9 +++- .../tests/AbstractTestDistributedQueries.java | 41 +++++++++++++++++++ 7 files changed, 96 insertions(+), 5 deletions(-) diff --git a/presto-docs/src/main/sphinx/sql/create-view.rst b/presto-docs/src/main/sphinx/sql/create-view.rst index 5a9778c948a74..eec68eaa20d5d 100644 --- a/presto-docs/src/main/sphinx/sql/create-view.rst +++ b/presto-docs/src/main/sphinx/sql/create-view.rst @@ -38,6 +38,9 @@ In the ``INVOKER`` security mode, tables referenced in the view are accessed using the permissions of the query user (the *invoker* of the view). A view created in this mode is simply a stored query. +The ``default-view-security-mode`` can be used to configure the default +security mode for view creation. + Examples -------- @@ -60,6 +63,10 @@ Create a view that replaces an existing view:: SELECT orderkey, orderstatus, totalprice / 4 AS quarter FROM orders +Set the default view security mode to ``INVOKER``:: + + SET SESSION default_view_security_mode='INVOKER' + See Also -------- diff --git a/presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java b/presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java index c8be99a72297f..d0356a15b1165 100644 --- a/presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java +++ b/presto-main/src/main/java/com/facebook/presto/SystemSessionProperties.java @@ -43,6 +43,7 @@ import com.facebook.presto.sql.analyzer.FeaturesConfig.ShardedJoinStrategy; import com.facebook.presto.sql.analyzer.FeaturesConfig.SingleStreamSpillerChoice; import com.facebook.presto.sql.planner.CompilerConfig; +import com.facebook.presto.sql.tree.CreateView; import com.facebook.presto.tracing.TracingConfig; import com.google.common.base.Splitter; import com.google.common.collect.ImmutableList; @@ -341,6 +342,7 @@ public final class SystemSessionProperties public static final String NATIVE_EXECUTION_PROGRAM_ARGUMENTS = "native_execution_program_arguments"; public static final String NATIVE_EXECUTION_PROCESS_REUSE_ENABLED = "native_execution_process_reuse_enabled"; public static final String NATIVE_DEBUG_VALIDATE_OUTPUT_FROM_OPERATORS = "native_debug_validate_output_from_operators"; + public static final String DEFAULT_VIEW_SECURITY_MODE = "default_view_security_mode"; private final List> sessionProperties; @@ -1929,7 +1931,19 @@ public SystemSessionProperties( REWRITE_EXPRESSION_WITH_CONSTANT_EXPRESSION, "Rewrite left join with is null check to semi join", featuresConfig.isRewriteExpressionWithConstantVariable(), - false)); + false), + new PropertyMetadata<>( + DEFAULT_VIEW_SECURITY_MODE, + format("Set default view security mode. Options are: %s", + Stream.of(CreateView.Security.values()) + .map(CreateView.Security::name) + .collect(joining(","))), + VARCHAR, + CreateView.Security.class, + featuresConfig.getDefaultViewSecurityMode(), + false, + value -> CreateView.Security.valueOf(((String) value).toUpperCase()), + CreateView.Security::name)); } public static boolean isSpoolingOutputBufferEnabled(Session session) @@ -3197,4 +3211,9 @@ public static boolean isRewriteExpressionWithConstantEnabled(Session session) { return session.getSystemProperty(REWRITE_EXPRESSION_WITH_CONSTANT_EXPRESSION, Boolean.class); } + + public static CreateView.Security getDefaultViewSecurityMode(Session session) + { + return session.getSystemProperty(DEFAULT_VIEW_SECURITY_MODE, CreateView.Security.class); + } } diff --git a/presto-main/src/main/java/com/facebook/presto/execution/CreateViewTask.java b/presto-main/src/main/java/com/facebook/presto/execution/CreateViewTask.java index 8cbf593d40d60..8e90d6613902c 100644 --- a/presto-main/src/main/java/com/facebook/presto/execution/CreateViewTask.java +++ b/presto-main/src/main/java/com/facebook/presto/execution/CreateViewTask.java @@ -37,6 +37,7 @@ import java.util.List; import java.util.Optional; +import static com.facebook.presto.SystemSessionProperties.getDefaultViewSecurityMode; import static com.facebook.presto.metadata.MetadataUtil.createQualifiedObjectName; import static com.facebook.presto.metadata.MetadataUtil.toSchemaTableName; import static com.facebook.presto.spi.analyzer.ViewDefinition.ViewColumn; @@ -97,9 +98,10 @@ public ListenableFuture execute(CreateView statement, TransactionManager tran .collect(toImmutableList()); ConnectorTableMetadata viewMetadata = new ConnectorTableMetadata(toSchemaTableName(name), columnMetadata); - // use DEFINER security by default + + CreateView.Security defaultViewSecurityMode = getDefaultViewSecurityMode(session); Optional owner = Optional.of(session.getUser()); - if (statement.getSecurity().orElse(null) == INVOKER) { + if (statement.getSecurity().orElse(defaultViewSecurityMode) == INVOKER) { owner = Optional.empty(); } diff --git a/presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java b/presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java index 34c980fbf83aa..2c4a8d9857193 100644 --- a/presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java +++ b/presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java @@ -22,6 +22,7 @@ import com.facebook.presto.operator.aggregation.histogram.HistogramGroupImplementation; import com.facebook.presto.operator.aggregation.multimapagg.MultimapAggGroupImplementation; import com.facebook.presto.spi.function.FunctionMetadata; +import com.facebook.presto.sql.tree.CreateView; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Splitter; import com.google.common.collect.ImmutableList; @@ -43,6 +44,7 @@ import static com.facebook.presto.sql.analyzer.FeaturesConfig.JoinNotNullInferenceStrategy.NONE; import static com.facebook.presto.sql.analyzer.FeaturesConfig.TaskSpillingStrategy.ORDER_BY_CREATE_TIME; import static com.facebook.presto.sql.analyzer.RegexLibrary.JONI; +import static com.facebook.presto.sql.tree.CreateView.Security.DEFINER; import static com.google.common.collect.ImmutableList.toImmutableList; import static io.airlift.units.DataSize.Unit.KILOBYTE; import static io.airlift.units.DataSize.Unit.MEGABYTE; @@ -302,6 +304,7 @@ public class FeaturesConfig private long kHyperLogLogAggregationGroupNumberLimit; private boolean limitNumberOfGroupsForKHyperLogLogAggregations = true; private boolean generateDomainFilters; + private CreateView.Security defaultViewSecurityMode = DEFINER; public enum PartitioningPrecisionStrategy { @@ -3055,4 +3058,17 @@ public FeaturesConfig setRewriteExpressionWithConstantVariable(boolean rewriteEx this.rewriteExpressionWithConstantVariable = rewriteExpressionWithConstantVariable; return this; } + + public CreateView.Security getDefaultViewSecurityMode() + { + return this.defaultViewSecurityMode; + } + + @Config("default-view-security-mode") + @ConfigDescription("Sets the default security mode for view creation. The options are definer/invoker.") + public FeaturesConfig setDefaultViewSecurityMode(CreateView.Security securityMode) + { + this.defaultViewSecurityMode = securityMode; + return this; + } } diff --git a/presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java b/presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java index ad3211e18e90a..7138ea567c6e3 100644 --- a/presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java +++ b/presto-main/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java @@ -2803,6 +2803,7 @@ private RelationType analyzeView(Query query, QualifiedObjectName name, Optional Identity identity; AccessControl viewAccessControl; if (owner.isPresent() && !owner.get().equals(session.getIdentity().getUser())) { + // definer mode identity = new Identity(owner.get(), Optional.empty(), session.getIdentity().getExtraCredentials()); viewAccessControl = new ViewAccessControl(accessControl); } diff --git a/presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestFeaturesConfig.java b/presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestFeaturesConfig.java index 3037eee78fa30..727e4bd3907a0 100644 --- a/presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestFeaturesConfig.java +++ b/presto-main/src/test/java/com/facebook/presto/sql/analyzer/TestFeaturesConfig.java @@ -49,6 +49,8 @@ import static com.facebook.presto.sql.analyzer.FeaturesConfig.TaskSpillingStrategy.PER_TASK_MEMORY_THRESHOLD; import static com.facebook.presto.sql.analyzer.RegexLibrary.JONI; import static com.facebook.presto.sql.analyzer.RegexLibrary.RE2J; +import static com.facebook.presto.sql.tree.CreateView.Security.DEFINER; +import static com.facebook.presto.sql.tree.CreateView.Security.INVOKER; import static io.airlift.units.DataSize.Unit.GIGABYTE; import static io.airlift.units.DataSize.Unit.KILOBYTE; import static io.airlift.units.DataSize.Unit.MEGABYTE; @@ -265,7 +267,8 @@ public void testDefaults() .setLimitNumberOfGroupsForKHyperLogLogAggregations(true) .setGenerateDomainFilters(false) .setRewriteExpressionWithConstantVariable(true) - .setDefaultWriterReplicationCoefficient(3.0)); + .setDefaultWriterReplicationCoefficient(3.0) + .setDefaultViewSecurityMode(DEFINER)); } @Test @@ -477,6 +480,7 @@ public void testExplicitPropertyMappings() .put("optimizer.generate-domain-filters", "true") .put("optimizer.rewrite-expression-with-constant-variable", "false") .put("optimizer.default-writer-replication-coefficient", "5.0") + .put("default-view-security-mode", INVOKER.name()) .build(); FeaturesConfig expected = new FeaturesConfig() @@ -685,7 +689,8 @@ public void testExplicitPropertyMappings() .setLimitNumberOfGroupsForKHyperLogLogAggregations(false) .setGenerateDomainFilters(true) .setRewriteExpressionWithConstantVariable(false) - .setDefaultWriterReplicationCoefficient(5.0); + .setDefaultWriterReplicationCoefficient(5.0) + .setDefaultViewSecurityMode(INVOKER); assertFullMapping(properties, expected); } diff --git a/presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestDistributedQueries.java b/presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestDistributedQueries.java index 7677da793adfe..0f1f6af379e1f 100644 --- a/presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestDistributedQueries.java +++ b/presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestDistributedQueries.java @@ -50,6 +50,7 @@ import static com.facebook.presto.SystemSessionProperties.VERBOSE_OPTIMIZER_INFO_ENABLED; import static com.facebook.presto.common.type.VarcharType.VARCHAR; import static com.facebook.presto.connector.informationSchema.InformationSchemaMetadata.INFORMATION_SCHEMA; +import static com.facebook.presto.sql.tree.CreateView.Security.INVOKER; import static com.facebook.presto.testing.MaterializedResult.resultBuilder; import static com.facebook.presto.testing.TestingAccessControlManager.TestingPrivilegeType.ADD_COLUMN; import static com.facebook.presto.testing.TestingAccessControlManager.TestingPrivilegeType.CREATE_TABLE; @@ -1093,6 +1094,46 @@ public void testNonQueryAccessControl() } } + @Test + public void testViewAccessControlInvokerDefault() + { + skipTestUnless(supportsViews()); + + Session viewOwnerSession = TestingSession.testSessionBuilder() + .setIdentity(new Identity("test_view_access_owner", Optional.empty())) + .setCatalog(getSession().getCatalog().get()) + .setSchema(getSession().getSchema().get()) + .setSystemProperty("default_view_security_mode", INVOKER.name()) + .build(); + + assertAccessAllowed( + viewOwnerSession, + "CREATE VIEW test_view_access AS SELECT * FROM orders", + privilege("orders", CREATE_VIEW_WITH_SELECT_COLUMNS)); + + assertAccessAllowed( + "SELECT * FROM test_view_access", + privilege(viewOwnerSession.getUser(), "orders", SELECT_COLUMN)); + + assertAccessDenied( + "SELECT * FROM test_view_access", + "Cannot select from columns.*", + privilege(getSession().getUser(), "orders", SELECT_COLUMN)); + + assertAccessAllowed( + viewOwnerSession, + "CREATE VIEW test_view_access1 SECURITY DEFINER AS SELECT * FROM orders", + privilege("orders", CREATE_VIEW_WITH_SELECT_COLUMNS)); + + assertAccessAllowed( + "SELECT * FROM test_view_access1", + privilege(viewOwnerSession.getUser(), "orders", SELECT_COLUMN)); + + assertAccessAllowed( + "SELECT * FROM test_view_access1", + privilege(getSession().getUser(), "orders", SELECT_COLUMN)); + } + @Test public void testViewAccessControl() {