Skip to content

Commit 90e4fd7

Browse files
codyzwiefcodyzwief
committed
Only check access control on underlying MV tables on refresh
1 parent 3da3772 commit 90e4fd7

File tree

3 files changed

+90
-18
lines changed

3 files changed

+90
-18
lines changed

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

Lines changed: 24 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -713,7 +713,7 @@ protected Scope visitRefreshMaterializedView(RefreshMaterializedView refreshMate
713713

714714
// analyze the query that creates the data
715715
Query query = parseView(view.getOriginalSql(), name, refreshMaterializedView);
716-
Scope queryScope = process(query, scope);
716+
Scope queryScope = analyzeView(query, name, view.getCatalog(), view.getSchema(), view.getRunAsIdentity(), view.getPath(), refreshMaterializedView.getTable(), false);
717717

718718
// verify the insert destination columns match the query
719719
TableHandle targetTableHandle = metadata.getTableHandle(session, targetTable)
@@ -2259,10 +2259,10 @@ protected Scope visitTable(Table table, Optional<Scope> scope)
22592259
checkStorageTableNotRedirected(storageTableName);
22602260
TableHandle tableHandle = metadata.getTableHandle(session, storageTableName)
22612261
.orElseThrow(() -> semanticException(INVALID_VIEW, table, "Storage table '%s' does not exist", storageTableName));
2262-
return createScopeForMaterializedView(table, name, scope, materializedViewDefinition, Optional.of(tableHandle));
2262+
return createScopeForMaterializedView(table, name, scope, materializedViewDefinition, Optional.of(tableHandle), true);
22632263
}
22642264
// This is a stale materialized view and should be expanded like a logical view
2265-
return createScopeForMaterializedView(table, name, scope, materializedViewDefinition, Optional.empty());
2265+
return createScopeForMaterializedView(table, name, scope, materializedViewDefinition, Optional.empty(), true);
22662266
}
22672267

22682268
// This could be a reference to a logical view or a table
@@ -2473,7 +2473,7 @@ private Scope createScopeForCommonTableExpression(Table table, Optional<Scope> s
24732473
return createAndAssignScope(table, scope, fields);
24742474
}
24752475

2476-
private Scope createScopeForMaterializedView(Table table, QualifiedObjectName name, Optional<Scope> scope, MaterializedViewDefinition view, Optional<TableHandle> storageTable)
2476+
private Scope createScopeForMaterializedView(Table table, QualifiedObjectName name, Optional<Scope> scope, MaterializedViewDefinition view, Optional<TableHandle> storageTable, boolean bypassViewAccessControl)
24772477
{
24782478
return createScopeForView(
24792479
table,
@@ -2486,7 +2486,8 @@ private Scope createScopeForMaterializedView(Table table, QualifiedObjectName na
24862486
view.getPath(),
24872487
view.getColumns(),
24882488
storageTable,
2489-
true);
2489+
true,
2490+
bypassViewAccessControl);
24902491
}
24912492

24922493
private Scope createScopeForView(Table table, QualifiedObjectName name, Optional<Scope> scope, ViewDefinition view)
@@ -2501,6 +2502,7 @@ private Scope createScopeForView(Table table, QualifiedObjectName name, Optional
25012502
view.getPath(),
25022503
view.getColumns(),
25032504
Optional.empty(),
2505+
false,
25042506
false);
25052507
}
25062508

@@ -2515,7 +2517,8 @@ private Scope createScopeForView(
25152517
List<CatalogSchemaName> path,
25162518
List<ViewColumn> columns,
25172519
Optional<TableHandle> storageTable,
2518-
boolean isMaterializedView)
2520+
boolean isMaterializedView,
2521+
boolean bypassViewAccessControl)
25192522
{
25202523
Statement statement = analysis.getStatement();
25212524
if (statement instanceof CreateView viewStatement) {
@@ -2541,7 +2544,8 @@ private Scope createScopeForView(
25412544
}
25422545

25432546
analysis.registerTableForView(table, name, isMaterializedView);
2544-
RelationType descriptor = analyzeView(query, name, catalog, schema, owner, path, table);
2547+
RelationType descriptor = analyzeView(query, name, catalog, schema, owner, path, table, bypassViewAccessControl)
2548+
.getRelationType().withAlias(name.objectName(), null);
25452549
analysis.unregisterTableForView();
25462550

25472551
checkViewStaleness(columns, descriptor.getVisibleFields(), name, table)
@@ -5045,43 +5049,46 @@ private void analyzeAggregations(
50455049
}
50465050
}
50475051

5048-
private RelationType analyzeView(
5052+
private Scope analyzeView(
50495053
Query query,
50505054
QualifiedObjectName name,
50515055
Optional<String> catalog,
50525056
Optional<String> schema,
50535057
Optional<Identity> owner,
50545058
List<CatalogSchemaName> path,
5055-
Table node)
5059+
Table node,
5060+
boolean bypassAccessControl)
50565061
{
50575062
try {
50585063
// run view as view owner if set; otherwise, run as session user
50595064
Identity identity;
5060-
AccessControl viewAccessControl;
5065+
AccessControl viewAccessControl = accessControl;
50615066
if (owner.isPresent()) {
50625067
identity = Identity.from(owner.get())
50635068
.withGroups(groupProvider.getGroups(owner.get().getUser()))
50645069
.build();
5065-
if (owner.get().getUser().equals(session.getIdentity().getUser())) {
5066-
// View owner does not need GRANT OPTION to grant access themselves
5067-
viewAccessControl = accessControl;
5070+
if (bypassAccessControl) {
5071+
viewAccessControl = new AllowAllAccessControl();
50685072
}
5069-
else {
5073+
// View owner does not need GRANT OPTION to grant access themselves
5074+
// All others do need GRANT OPTION
5075+
else if (!owner.get().getUser().equals(session.getIdentity().getUser())) {
50705076
viewAccessControl = new ViewAccessControl(accessControl);
50715077
}
50725078
}
50735079
else {
50745080
identity = session.getIdentity();
5075-
viewAccessControl = accessControl;
5081+
if (bypassAccessControl) {
5082+
viewAccessControl = new AllowAllAccessControl();
5083+
}
50765084
}
50775085

50785086
Session viewSession = session.createViewSession(catalog, schema, identity, path);
50795087

50805088
StatementAnalyzer analyzer = statementAnalyzerFactory
50815089
.withSpecializedAccessControl(viewAccessControl)
50825090
.createStatementAnalyzer(analysis, viewSession, warningCollector, CorrelationSupport.ALLOWED);
5083-
Scope queryScope = analyzer.analyze(query);
5084-
return queryScope.getRelationType().withAlias(name.objectName(), null);
5091+
return analyzer.analyze(query);
50855092
}
50865093
catch (RuntimeException e) {
50875094
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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -630,7 +630,7 @@ public void testReferencedTablesInRefreshMaterializedView()
630630
TableInfo table = tables.get(0);
631631
assertThat(table)
632632
.hasCatalogSchemaTable("tpch", "tiny", "nation")
633-
.hasAuthorization("user")
633+
.hasAuthorization("alice")
634634
.isDirectlyReferenced()
635635
.hasColumnsWithoutMasking("nationkey")
636636
.hasNoRowFilters()

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

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@
5858
import io.trino.spi.security.SystemSecurityContext;
5959
import io.trino.spi.security.TrinoPrincipal;
6060
import io.trino.spi.security.ViewExpression;
61+
import io.trino.spi.type.VarcharType;
6162
import io.trino.sql.SqlPath;
6263
import io.trino.testing.AbstractTestQueryFramework;
6364
import io.trino.testing.DistributedQueryRunner;
@@ -210,6 +211,19 @@ protected SystemAccessControl delegate()
210211
@Override
211212
public Map<SchemaTableName, ConnectorMaterializedViewDefinition> apply(ConnectorSession session, SchemaTablePrefix schemaTablePrefix)
212213
{
214+
ConnectorMaterializedViewDefinition selectFromUnderlyingTableDefinition = new ConnectorMaterializedViewDefinition(
215+
"SELECT * FROM tpch.sf1.region",
216+
Optional.of(new CatalogSchemaTableName("tpch", "sf1", "region")),
217+
Optional.empty(),
218+
Optional.empty(),
219+
ImmutableList.of(
220+
new ConnectorMaterializedViewDefinition.Column("regionkey", BIGINT.getTypeId(), Optional.empty()),
221+
new ConnectorMaterializedViewDefinition.Column("name", VarcharType.createVarcharType(25).getTypeId(), Optional.empty()),
222+
new ConnectorMaterializedViewDefinition.Column("comment", VarcharType.createVarcharType(152).getTypeId(), Optional.empty())),
223+
Optional.of(Duration.ZERO),
224+
Optional.of("comment"),
225+
Optional.of("test_mv_access_owner"),
226+
ImmutableList.of());
213227
ConnectorMaterializedViewDefinition materializedViewDefinition = new ConnectorMaterializedViewDefinition(
214228
"SELECT 1 AS test",
215229
Optional.empty(),
@@ -221,6 +235,7 @@ public Map<SchemaTableName, ConnectorMaterializedViewDefinition> apply(Connector
221235
Optional.of("owner"),
222236
ImmutableList.of());
223237
return ImmutableMap.of(
238+
new SchemaTableName("default", "test_materialized_view_select_from_region"), selectFromUnderlyingTableDefinition,
224239
new SchemaTableName("default", "test_materialized_view"), materializedViewDefinition);
225240
}
226241
})
@@ -860,6 +875,56 @@ public void testCommentOnRedirectedTable()
860875
assertQueryReturnsEmptyResult(query);
861876
}
862877

878+
@Test
879+
public void testMaterializedViewAccessControl()
880+
{
881+
reset();
882+
883+
Session viewOwnerSession = TestingSession.testSessionBuilder()
884+
.setIdentity(Identity.ofUser("test_mv_access_owner"))
885+
.setCatalog(getSession().getCatalog())
886+
.setSchema(getSession().getSchema())
887+
.build();
888+
889+
// Verify MV owner can access
890+
assertAccessAllowed(
891+
viewOwnerSession,
892+
"SELECT * FROM mock.default.test_materialized_view_select_from_region");
893+
894+
// Verify it's accessible to non-owner session as well if nothing's denied
895+
assertAccessAllowed(
896+
"SELECT * FROM mock.default.test_materialized_view_select_from_region");
897+
898+
// Verify non-owner cannot access if it doesn't have SELECT
899+
assertAccessDenied("SELECT * FROM mock.default.test_materialized_view_select_from_region",
900+
"Cannot select from columns .* in table or view mock.default.test_materialized_view_select_from_region",
901+
privilege(getSession().getUser(), "test_materialized_view_select_from_region", SELECT_COLUMN));
902+
903+
// Verify an MV is treated as a table, not a view, for the purposes of selecting from it
904+
assertAccessAllowed(
905+
viewOwnerSession,
906+
"SELECT * FROM mock.default.test_materialized_view_select_from_region",
907+
privilege(viewOwnerSession.getUser(), "region", CREATE_VIEW_WITH_SELECT_COLUMNS));
908+
assertAccessAllowed(
909+
"SELECT * FROM mock.default.test_materialized_view_select_from_region",
910+
privilege(viewOwnerSession.getUser(), "region", CREATE_VIEW_WITH_SELECT_COLUMNS));
911+
assertAccessAllowed(
912+
viewOwnerSession,
913+
"SELECT * FROM mock.default.test_materialized_view_select_from_region",
914+
privilege(viewOwnerSession.getUser(), "region", SELECT_COLUMN));
915+
assertAccessAllowed(
916+
"SELECT * FROM mock.default.test_materialized_view_select_from_region",
917+
privilege(viewOwnerSession.getUser(), "region", SELECT_COLUMN));
918+
919+
// Verify only the owner can refresh the MV and the permissions on the underlying table are checked
920+
assertAccessAllowed(
921+
viewOwnerSession,
922+
"REFRESH MATERIALIZED VIEW mock.default.test_materialized_view_select_from_region");
923+
assertAccessDenied("REFRESH MATERIALIZED VIEW mock.default.test_materialized_view_select_from_region",
924+
"View owner does not have sufficient privileges: View owner 'test_mv_access_owner' cannot create view that selects from tpch.sf1.region",
925+
privilege(viewOwnerSession.getUser(), "region", CREATE_VIEW_WITH_SELECT_COLUMNS));
926+
}
927+
863928
@Test
864929
public void testViewWithTableFunction()
865930
{

0 commit comments

Comments
 (0)