Only check access control on underlying MV tables on refresh

Author: codyzwief

core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java

@@ -714,7 +714,7 @@ protected Scope visitRefreshMaterializedView(RefreshMaterializedView refreshMate
 
             // analyze the query that creates the data
             Query query = parseView(view.getOriginalSql(), name, refreshMaterializedView);
-            Scope queryScope = process(query, scope);
+            Scope queryScope = analyzeView(query, name, view.getCatalog(), view.getSchema(), view.getRunAsIdentity(), view.getPath(), refreshMaterializedView.getTable(), false);
 
             // verify the insert destination columns match the query
             TableHandle targetTableHandle = metadata.getTableHandle(session, targetTable)
@@ -2260,10 +2260,10 @@ protected Scope visitTable(Table table, Optional<Scope> scope)
                     checkStorageTableNotRedirected(storageTableName);
                     TableHandle tableHandle = metadata.getTableHandle(session, storageTableName)
                             .orElseThrow(() -> semanticException(INVALID_VIEW, table, "Storage table '%s' does not exist", storageTableName));
-                    return createScopeForMaterializedView(table, name, scope, materializedViewDefinition, Optional.of(tableHandle));
+                    return createScopeForMaterializedView(table, name, scope, materializedViewDefinition, Optional.of(tableHandle), true);
                 }
                 // This is a stale materialized view and should be expanded like a logical view
-                return createScopeForMaterializedView(table, name, scope, materializedViewDefinition, Optional.empty());
+                return createScopeForMaterializedView(table, name, scope, materializedViewDefinition, Optional.empty(), true);
             }
 
             // This could be a reference to a logical view or a table
@@ -2474,7 +2474,7 @@ private Scope createScopeForCommonTableExpression(Table table, Optional<Scope> s
             return createAndAssignScope(table, scope, fields);
         }
 
-        private Scope createScopeForMaterializedView(Table table, QualifiedObjectName name, Optional<Scope> scope, MaterializedViewDefinition view, Optional<TableHandle> storageTable)
+        private Scope createScopeForMaterializedView(Table table, QualifiedObjectName name, Optional<Scope> scope, MaterializedViewDefinition view, Optional<TableHandle> storageTable, boolean bypassViewAccessControl)
         {
             return createScopeForView(
                     table,
@@ -2487,7 +2487,8 @@ private Scope createScopeForMaterializedView(Table table, QualifiedObjectName na
                     view.getPath(),
                     view.getColumns(),
                     storageTable,
-                    true);
+                    true,
+                    bypassViewAccessControl);
         }
 
         private Scope createScopeForView(Table table, QualifiedObjectName name, Optional<Scope> scope, ViewDefinition view)
@@ -2502,6 +2503,7 @@ private Scope createScopeForView(Table table, QualifiedObjectName name, Optional
                     view.getPath(),
                     view.getColumns(),
                     Optional.empty(),
+                    false,
                     false);
         }
 
@@ -2516,7 +2518,8 @@ private Scope createScopeForView(
                 List<CatalogSchemaName> path,
                 List<ViewColumn> columns,
                 Optional<TableHandle> storageTable,
-                boolean isMaterializedView)
+                boolean isMaterializedView,
+                boolean bypassViewAccessControl)
         {
             Statement statement = analysis.getStatement();
             if (statement instanceof CreateView viewStatement) {
@@ -2542,7 +2545,8 @@ private Scope createScopeForView(
             }
 
             analysis.registerTableForView(table, name, isMaterializedView);
-            RelationType descriptor = analyzeView(query, name, catalog, schema, owner, path, table);
+            RelationType descriptor = analyzeView(query, name, catalog, schema, owner, path, table, bypassViewAccessControl)
+                    .getRelationType().withAlias(name.objectName(), null);
             analysis.unregisterTableForView();
 
             checkViewStaleness(columns, descriptor.getVisibleFields(), name, table)
@@ -5041,43 +5045,46 @@ private void analyzeAggregations(
             }
         }
 
-        private RelationType analyzeView(
+        private Scope analyzeView(
                 Query query,
                 QualifiedObjectName name,
                 Optional<String> catalog,
                 Optional<String> schema,
                 Optional<Identity> owner,
                 List<CatalogSchemaName> path,
-                Table node)
+                Table node,
+                boolean bypassAccessControl)
         {
             try {
                 // run view as view owner if set; otherwise, run as session user
                 Identity identity;
-                AccessControl viewAccessControl;
+                AccessControl viewAccessControl = accessControl;
                 if (owner.isPresent()) {
                     identity = Identity.from(owner.get())
                             .withGroups(groupProvider.getGroups(owner.get().getUser()))
                             .build();
-                    if (owner.get().getUser().equals(session.getIdentity().getUser())) {
-                        // View owner does not need GRANT OPTION to grant access themselves
-                        viewAccessControl = accessControl;
+                    if (bypassAccessControl) {
+                        viewAccessControl = new AllowAllAccessControl();
                     }
-                    else {
+                    // View owner does not need GRANT OPTION to grant access themselves
+                    // All others do need GRANT OPTION
+                    else if (!owner.get().getUser().equals(session.getIdentity().getUser())) {
                         viewAccessControl = new ViewAccessControl(accessControl);
                     }
                 }
                 else {
                     identity = session.getIdentity();
-                    viewAccessControl = accessControl;
+                    if (bypassAccessControl) {
+                        viewAccessControl = new AllowAllAccessControl();
+                    }
                 }
 
                 Session viewSession = session.createViewSession(catalog, schema, identity, path);
 
                 StatementAnalyzer analyzer = statementAnalyzerFactory
                         .withSpecializedAccessControl(viewAccessControl)
                         .createStatementAnalyzer(analysis, viewSession, warningCollector, CorrelationSupport.ALLOWED);
-                Scope queryScope = analyzer.analyze(query);
-                return queryScope.getRelationType().withAlias(name.objectName(), null);
+                return analyzer.analyze(query);
             }
             catch (RuntimeException e) {
                 throw semanticException(INVALID_VIEW, node, e, "Failed analyzing stored view '%s': %s", name, e.getMessage());

testing/trino-tests/src/test/java/io/trino/execution/TestEventListenerBasic.java

@@ -630,7 +630,7 @@ public void testReferencedTablesInRefreshMaterializedView()
         TableInfo table = tables.get(0);
         assertThat(table)
                 .hasCatalogSchemaTable("tpch", "tiny", "nation")
-                .hasAuthorization("user")
+                .hasAuthorization("alice")
                 .isDirectlyReferenced()
                 .hasColumnsWithoutMasking("nationkey")
                 .hasNoRowFilters()

testing/trino-tests/src/test/java/io/trino/security/TestAccessControl.java

@@ -58,6 +58,7 @@
 import io.trino.spi.security.SystemSecurityContext;
 import io.trino.spi.security.TrinoPrincipal;
 import io.trino.spi.security.ViewExpression;
+import io.trino.spi.type.VarcharType;
 import io.trino.sql.SqlPath;
 import io.trino.testing.AbstractTestQueryFramework;
 import io.trino.testing.DistributedQueryRunner;
@@ -210,6 +211,19 @@ protected SystemAccessControl delegate()
                     @Override
                     public Map<SchemaTableName, ConnectorMaterializedViewDefinition> apply(ConnectorSession session, SchemaTablePrefix schemaTablePrefix)
                     {
+                        ConnectorMaterializedViewDefinition selectFromUnderlyingTableDefinition = new ConnectorMaterializedViewDefinition(
+                                "SELECT * FROM tpch.sf1.region",
+                                Optional.of(new CatalogSchemaTableName("tpch", "sf1", "region")),
+                                Optional.empty(),
+                                Optional.empty(),
+                                ImmutableList.of(
+                                        new ConnectorMaterializedViewDefinition.Column("regionkey", BIGINT.getTypeId(), Optional.empty()),
+                                        new ConnectorMaterializedViewDefinition.Column("name", VarcharType.createVarcharType(25).getTypeId(), Optional.empty()),
+                                        new ConnectorMaterializedViewDefinition.Column("comment", VarcharType.createVarcharType(152).getTypeId(), Optional.empty())),
+                                Optional.of(Duration.ZERO),
+                                Optional.of("comment"),
+                                Optional.of("test_mv_access_owner"),
+                                ImmutableList.of());
                         ConnectorMaterializedViewDefinition materializedViewDefinition = new ConnectorMaterializedViewDefinition(
                                 "SELECT 1 AS test",
                                 Optional.empty(),
@@ -221,6 +235,7 @@ public Map<SchemaTableName, ConnectorMaterializedViewDefinition> apply(Connector
                                 Optional.of("owner"),
                                 ImmutableList.of());
                         return ImmutableMap.of(
+                                new SchemaTableName("default", "test_materialized_view_select_from_region"), selectFromUnderlyingTableDefinition,
                                 new SchemaTableName("default", "test_materialized_view"), materializedViewDefinition);
                     }
                 })
@@ -860,6 +875,56 @@ public void testCommentOnRedirectedTable()
         assertQueryReturnsEmptyResult(query);
     }
 
+    @Test
+    public void testMaterializedViewAccessControl()
+    {
+        reset();
+
+        Session viewOwnerSession = TestingSession.testSessionBuilder()
+                .setIdentity(Identity.ofUser("test_mv_access_owner"))
+                .setCatalog(getSession().getCatalog())
+                .setSchema(getSession().getSchema())
+                .build();
+
+        // Verify MV owner can access
+        assertAccessAllowed(
+                viewOwnerSession,
+                "SELECT * FROM mock.default.test_materialized_view_select_from_region");
+
+        // Verify it's accessible to non-owner session as well if nothing's denied
+        assertAccessAllowed(
+                "SELECT * FROM mock.default.test_materialized_view_select_from_region");
+
+        // Verify non-owner cannot access if it doesn't have SELECT
+        assertAccessDenied("SELECT * FROM mock.default.test_materialized_view_select_from_region",
+                "Cannot select from columns .* in table or view mock.default.test_materialized_view_select_from_region",
+                privilege(getSession().getUser(), "test_materialized_view_select_from_region", SELECT_COLUMN));
+
+        // Verify an MV is treated as a table, not a view, for the purposes of selecting from it
+        assertAccessAllowed(
+                viewOwnerSession,
+                "SELECT * FROM mock.default.test_materialized_view_select_from_region",
+                privilege(viewOwnerSession.getUser(), "region", CREATE_VIEW_WITH_SELECT_COLUMNS));
+        assertAccessAllowed(
+                "SELECT * FROM mock.default.test_materialized_view_select_from_region",
+                privilege(viewOwnerSession.getUser(), "region", CREATE_VIEW_WITH_SELECT_COLUMNS));
+        assertAccessAllowed(
+                viewOwnerSession,
+                "SELECT * FROM mock.default.test_materialized_view_select_from_region",
+                privilege(viewOwnerSession.getUser(), "region", SELECT_COLUMN));
+        assertAccessAllowed(
+                "SELECT * FROM mock.default.test_materialized_view_select_from_region",
+                privilege(viewOwnerSession.getUser(), "region", SELECT_COLUMN));
+
+        // Verify only the owner can refresh the MV and the permissions on the underlying table are checked
+        assertAccessAllowed(
+                viewOwnerSession,
+                "REFRESH MATERIALIZED VIEW mock.default.test_materialized_view_select_from_region");
+        assertAccessDenied("REFRESH MATERIALIZED VIEW mock.default.test_materialized_view_select_from_region",
+                "View owner does not have sufficient privileges: View owner 'test_mv_access_owner' cannot create view that selects from tpch.sf1.region",
+                privilege(viewOwnerSession.getUser(), "region", CREATE_VIEW_WITH_SELECT_COLUMNS));
+    }
+
     @Test
     public void testViewWithTableFunction()
     {