Allow ViewExpression use session user explicitly

Before the change if access control is not returning any dedicated user to evaluate the view expression they just pass the session user. However for the engine it is not clear that is a session user and so engine needs to retrieve groups for that user again and possibly some session roles are lost. After this change access control may decide to return empty identity. That would be in line of view SECURITY INVOKER. Then engine can simply reuse the session identity.
trinodb · Mar 13, 2023 · 16c3e44 · 16c3e44
1 parent 4492850
commit 16c3e44
Show file tree

Hide file tree

Showing 14 changed files with 240 additions and 137 deletions.
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
@@ -2279,7 +2279,7 @@ private void analyzeFiltersAndMasks(Table table, QualifiedObjectName name, Relat
         private void analyzeCheckConstraints(Table table, QualifiedObjectName name, Scope accessControlScope, List<String> constraints)
         {
             for (String constraint : constraints) {
-                ViewExpression expression = new ViewExpression(session.getIdentity().getUser(), Optional.of(name.getCatalogName()), Optional.of(name.getSchemaName()), constraint);
+                ViewExpression expression = new ViewExpression(Optional.empty(), Optional.of(name.getCatalogName()), Optional.of(name.getSchemaName()), constraint);
                 analyzeCheckConstraint(table, name, accessControlScope, expression);
             }
         }
@@ -4663,9 +4663,11 @@ private void analyzeRowFilter(String currentIdentity, Table table, QualifiedObje
 
             ExpressionAnalysis expressionAnalysis;
             try {
-                Identity filterIdentity = Identity.forUser(filter.getIdentity())
-                        .withGroups(groupProvider.getGroups(filter.getIdentity()))
-                        .build();
+                Identity filterIdentity = filter.getSecurityIdentity()
+                        .map(filterUser -> Identity.forUser(filterUser)
+                                .withGroups(groupProvider.getGroups(filterUser))
+                                .build())
+                        .orElseGet(session::getIdentity);
                 expressionAnalysis = ExpressionAnalyzer.analyzeExpression(
                         createViewSession(filter.getCatalog(), filter.getSchema(), filterIdentity, session.getPath()), // TODO: path should be included in row filter
                         plannerContext,
@@ -4714,11 +4716,13 @@ private void analyzeCheckConstraint(Table table, QualifiedObjectName name, Scope
 
             ExpressionAnalysis expressionAnalysis;
             try {
-                Identity filterIdentity = Identity.forUser(constraint.getIdentity())
-                        .withGroups(groupProvider.getGroups(constraint.getIdentity()))
-                        .build();
+                Identity constraintIdentity = constraint.getSecurityIdentity()
+                        .map(user -> Identity.forUser(user)
+                            .withGroups(groupProvider.getGroups(user))
+                            .build())
+                        .orElseGet(session::getIdentity);
                 expressionAnalysis = ExpressionAnalyzer.analyzeExpression(
-                        createViewSession(constraint.getCatalog(), constraint.getSchema(), filterIdentity, session.getPath()),
+                        createViewSession(constraint.getCatalog(), constraint.getSchema(), constraintIdentity, session.getPath()),
                         plannerContext,
                         statementAnalyzerFactory,
                         accessControl,
@@ -4777,9 +4781,11 @@ private void analyzeColumnMask(String currentIdentity, Table table, QualifiedObj
             verifyNoAggregateWindowOrGroupingFunctions(session, metadata, expression, format("Column mask for '%s.%s'", table.getName(), column));
 
             try {
-                Identity maskIdentity = Identity.forUser(mask.getIdentity())
-                        .withGroups(groupProvider.getGroups(mask.getIdentity()))
-                        .build();
+                Identity maskIdentity = mask.getSecurityIdentity()
+                        .map(maskUser -> Identity.forUser(maskUser)
+                                .withGroups(groupProvider.getGroups(maskUser))
+                                .build())
+                        .orElseGet(session::getIdentity);
                 expressionAnalysis = ExpressionAnalyzer.analyzeExpression(
                         createViewSession(mask.getCatalog(), mask.getSchema(), maskIdentity, session.getPath()), // TODO: path should be included in row filter
                         plannerContext,

diff --git a/core/trino-main/src/test/java/io/trino/security/TestAccessControlManager.java b/core/trino-main/src/test/java/io/trino/security/TestAccessControlManager.java
@@ -231,7 +231,7 @@ public SystemAccessControl create(Map<String, String> config)
                         @Override
                         public List<ViewExpression> getColumnMasks(SystemSecurityContext context, CatalogSchemaTableName tableName, String column, Type type)
                         {
-                            return ImmutableList.of(new ViewExpression("user", Optional.empty(), Optional.empty(), "system mask"));
+                            return ImmutableList.of(new ViewExpression(Optional.of("user"), Optional.empty(), Optional.empty(), "system mask"));
                         }
 
                         @Override
@@ -249,7 +249,7 @@ public void checkCanSetSystemSessionProperty(SystemSecurityContext context, Stri
                 @Override
                 public List<ViewExpression> getColumnMasks(ConnectorSecurityContext context, SchemaTableName tableName, String column, Type type)
                 {
-                    return ImmutableList.of(new ViewExpression("user", Optional.empty(), Optional.empty(), "connector mask"));
+                    return ImmutableList.of(new ViewExpression(Optional.of("user"), Optional.empty(), Optional.empty(), "connector mask"));
                 }
 
                 @Override

diff --git a/core/trino-main/src/test/java/io/trino/sql/planner/TestMaterializedViews.java b/core/trino-main/src/test/java/io/trino/sql/planner/TestMaterializedViews.java
@@ -236,7 +236,7 @@ public void testMaterializedViewWithCasts()
                 new QualifiedObjectName(TEST_CATALOG_NAME, SCHEMA, "materialized_view_with_casts"),
                 "a",
                 "user",
-                new ViewExpression("user", Optional.empty(), Optional.empty(), "a + 1"));
+                new ViewExpression(Optional.empty(), Optional.empty(), Optional.empty(), "a + 1"));
         assertPlan("SELECT * FROM materialized_view_with_casts",
                 anyTree(
                         project(