feat(analyzer): Add invoker and definer rights access control to materialized views #26521
+899
−88
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR extends materialized view support with SQL-standard SECURITY INVOKER/DEFINER control by tracking an optional owner in the view definition and wiring it through parsing, formatting, analysis, execution, and access control. The parser and AST builder now recognize an optional SECURITY clause, the SQL formatter emits it, and CreateMaterializedViewTask computes and stores an owner field (absent for INVOKER mode). Analyzer and StatementAnalyzer use the owner field to switch between invoker and definer sessions for queries and refreshes. Legacy mode bypasses the SECURITY clause and always applies definer behavior. The memory connector is updated to load file-based access control, and extensive unit and integration tests validate all new behaviors. Sequence diagram for materialized view creation with SECURITY clausesequenceDiagram
actor User
participant "SQL Parser"
participant "AST Builder"
participant "CreateMaterializedViewTask"
participant "MaterializedViewDefinition"
User->>"SQL Parser": CREATE MATERIALIZED VIEW ... SECURITY DEFINER/INVOKER ...
"SQL Parser"->>"AST Builder": Parse SECURITY clause
"AST Builder"->>"CreateMaterializedViewTask": Pass security mode
"CreateMaterializedViewTask"->>"MaterializedViewDefinition": Store owner if DEFINER, none if INVOKER
"CreateMaterializedViewTask"-->>User: Error if legacy mode and SECURITY clause
Sequence diagram for materialized view query/refresh with SECURITY modesequenceDiagram
actor User
participant "StatementAnalyzer"
participant "Session"
participant "AccessControl"
participant "MaterializedViewDefinition"
User->>"StatementAnalyzer": Query/refresh materialized view
"StatementAnalyzer"->>"MaterializedViewDefinition": Check owner field
alt SECURITY DEFINER
"StatementAnalyzer"->>"Session": Build session as owner
"StatementAnalyzer"->>"AccessControl": Use owner's permissions
else SECURITY INVOKER
"StatementAnalyzer"->>"Session": Use current user session
"StatementAnalyzer"->>"AccessControl": Use invoker's permissions
end
ER diagram for MaterializedViewDefinition owner fielderDiagram
MATERIALIZED_VIEW_DEFINITION {
string schema
string objectName
string sql
string[] baseTables
string[] columnMappings
string[] directColumnMappings
string[] baseTablesOnOuterJoinSide
string owner
}
MATERIALIZED_VIEW_DEFINITION ||--o| USER : "owner (optional)"
USER {
string userName
}
Class diagram for ViewSecurity and CreateMaterializedView changesclassDiagram
class ViewSecurity {
<<enum>>
INVOKER
DEFINER
}
class CreateMaterializedView {
+QualifiedName name
+Query query
+boolean notExists
+Optional<ViewSecurity> security
+List<Property> properties
+Optional<String> comment
+getSecurity()
}
ViewSecurity <.. CreateMaterializedView : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
tdcmeehan
changed the title
tdcmeehan
force-pushed
the
matview-pr-access-control
branch
9 times, most recently
from
bb678f7 to
d097da7
Compare
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `presto-main-base/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java:884-885` </location>
<code_context>
+ querySession = session;
+ }
+ else {
+ checkState(isLegacyMaterializedViews(session) || view.getOwner().isPresent(),
+ "Expected materialized view to have owner (DEFINER security) when legacy materialized views are disabled");
+ querySession = buildOwnerSession(session, view.getOwner(), metadata.getSessionPropertyManager(), viewName.getCatalogName(), view.getSchema());
+ }
+
</code_context>
<issue_to_address>
**suggestion:** The checkState message could be more actionable for debugging.
Include details like the view name or session information in the error message to make failures easier to diagnose.
```suggestion
checkState(isLegacyMaterializedViews(session) || view.getOwner().isPresent(),
String.format(
"Expected materialized view '%s' to have owner (DEFINER security) when legacy materialized views are disabled. Session user: '%s', Catalog: '%s', Schema: '%s'",
viewName,
session.getUser(),
viewName.getCatalogName(),
view.getSchema()));
```
</issue_to_address>
### Comment 2
<location> `presto-main-base/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java:2360-2361` </location>
<code_context>
+ Identity queryIdentity;
+ AccessControl queryAccessControl;
+
+ if (owner.isPresent()) {
+ queryIdentity = new Identity(owner.get(), Optional.empty(), session.getIdentity().getExtraCredentials());
+ queryAccessControl = new ViewAccessControl(accessControl);
+ }
+ else {
</code_context>
<issue_to_address>
**suggestion:** ViewAccessControl is used only when owner is present; clarify rationale.
Consider adding a comment explaining why ViewAccessControl is only assigned when owner is present and how legacy mode affects this logic, to aid future maintainers.
```suggestion
Identity queryIdentity;
AccessControl queryAccessControl;
// ViewAccessControl is only assigned when owner is present (definer rights).
// In legacy mode, owner may be absent and invoker rights are used (no ViewAccessControl).
// This ensures correct security semantics for materialized views depending on legacy mode and owner presence.
```
</issue_to_address>
### Comment 3
<location> `presto-main-base/src/main/java/com/facebook/presto/execution/CreateMaterializedViewTask.java:142-147` </location>
<code_context>
MaterializedViewColumnMappingExtractor extractor = new MaterializedViewColumnMappingExtractor(analysis, session, metadata);
+
+ if (isLegacyMaterializedViews(session) && statement.getSecurity().isPresent()) {
+ throw new SemanticException(
+ NOT_SUPPORTED,
+ statement,
+ "SECURITY clause is not supported when legacy_materialized_views is enabled");
+ }
+
</code_context>
<issue_to_address>
**suggestion:** Blocking use of SECURITY clause in legacy mode is appropriate, but error message could be more informative.
Including the rejected SECURITY clause value or relevant session details in the error message would make it clearer for users.
```suggestion
if (isLegacyMaterializedViews(session) && statement.getSecurity().isPresent()) {
String securityClause = statement.getSecurity().get().toString();
String sessionInfo = session.getUser() + "@" + session.getSource();
throw new SemanticException(
NOT_SUPPORTED,
statement,
String.format(
"SECURITY clause '%s' is not supported when legacy_materialized_views is enabled (session: %s)",
securityClause,
sessionInfo));
}
```
</issue_to_address>
### Comment 4
<location> `presto-memory/src/test/java/com/facebook/presto/plugin/memory/TestMemoryMaterializedViews.java:677` </location>
<code_context>
+ }
+
+ @Test
+ public void testDefaultSecurityModeIsDefiner()
+ {
+ Session adminSession = createSessionForUser("admin");
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for explicit SECURITY clause precedence over default_view_security_mode.
Please add a test case where both are set to confirm the SECURITY clause overrides default_view_security_mode.
</issue_to_address>
### Comment 5
<location> `presto-memory/src/test/java/com/facebook/presto/plugin/memory/TestMemoryMaterializedViews.java:837` </location>
<code_context>
+ }
+
+ @Test
+ public void testAccessControlOnMaterializedViewObject()
+ {
+ Session adminSession = createSessionForUser("admin");
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for access control on materialized view with SECURITY INVOKER.
Please add a test for SECURITY INVOKER to verify correct access control enforcement on materialized views with INVOKER rights.
Suggested implementation:
```java
@Test
public void testAccessControlOnMaterializedViewSecurityInvoker()
{
Session adminSession = createSessionForUser("admin");
Session restrictedSession = createSessionForUser("restricted_user");
Session allowedSession = createSessionForUser("allowed_user");
// Admin creates base table and grants SELECT to allowed_user only
assertUpdate(adminSession, "CREATE TABLE secure_base_invoker (id BIGINT, secret VARCHAR, value BIGINT)");
assertUpdate(adminSession, "INSERT INTO secure_base_invoker VALUES (1, 'confidential', 100), (2, 'classified', 200)", 2);
assertUpdate(adminSession, "GRANT SELECT ON secure_base_invoker TO allowed_user");
// Admin creates materialized view with SECURITY INVOKER
assertUpdate(adminSession,
"CREATE MATERIALIZED VIEW mv_invoker " +
"SECURITY INVOKER AS " +
"SELECT id, secret, value FROM secure_base_invoker");
// allowed_user can query the view
assertQuery(allowedSession, "SELECT * FROM mv_invoker", "VALUES (1, 'confidential', 100), (2, 'classified', 200)");
// restricted_user cannot query the view due to lack of SELECT on base table
assertQueryFails(restrictedSession, "SELECT * FROM mv_invoker", "Access Denied: Cannot select from table secure_base_invoker");
// Cleanup
assertUpdate(adminSession, "DROP MATERIALIZED VIEW mv_invoker");
assertUpdate(adminSession, "DROP TABLE secure_base_invoker");
}
Session restrictedSession = createSessionForUser("restricted_user");
assertUpdate(adminSession, "CREATE TABLE secure_base (id BIGINT, secret VARCHAR, value BIGINT)");
assertUpdate(adminSession, "INSERT INTO secure_base VALUES (1, 'confidential', 100), (2, 'classified', 200)", 2);
assertUpdate(adminSession,
"CREATE MATERIALIZED VIEW mv_definer " +
"SECURITY DEFINER AS " +
"SELECT id, secret, value FROM secure_base");
```
You may need to ensure that the users "allowed_user" and "restricted_user" exist and have the correct privileges in your test setup. Adjust the user creation and privilege granting logic as needed to match your environment.
</issue_to_address>
### Comment 6
<location> `presto-main-base/src/test/java/com/facebook/presto/execution/TestCreateMaterializedViewTask.java:210` </location>
<code_context>
+ }
+
+ @Test
+ public void testCreateMaterializedViewWithDefaultDefinerSecurity()
+ {
+ SqlParser parser = new SqlParser();
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for invalid default_view_security_mode value.
Add a test to confirm that an invalid default_view_security_mode triggers the correct error, ensuring configuration validation works as intended.
</issue_to_address>
### Comment 7
<location> `presto-parser/src/test/java/com/facebook/presto/sql/parser/TestSqlParser.java:1786-1787` </location>
<code_context>
- assertStatement("CREATE VIEW a SECURITY DEFINER AS SELECT * FROM t", new CreateView(QualifiedName.of("a"), query, false, Optional.of(CreateView.Security.DEFINER)));
- assertStatement("CREATE VIEW a SECURITY INVOKER AS SELECT * FROM t", new CreateView(QualifiedName.of("a"), query, false, Optional.of(CreateView.Security.INVOKER)));
+ assertStatement("CREATE VIEW a SECURITY DEFINER AS SELECT * FROM t", new CreateView(QualifiedName.of("a"), query, false, Optional.of(DEFINER)));
+ assertStatement("CREATE VIEW a SECURITY INVOKER AS SELECT * FROM t", new CreateView(QualifiedName.of("a"), query, false, Optional.of(INVOKER)));
assertStatement("CREATE VIEW bar.foo AS SELECT * FROM t", new CreateView(QualifiedName.of("bar", "foo"), query, false, Optional.empty()));
</code_context>
<issue_to_address>
**nitpick (testing):** Nitpick: Consider adding parser tests for materialized views with SECURITY clause.
Adding tests for CREATE MATERIALIZED VIEW with SECURITY DEFINER and SECURITY INVOKER would enhance test coverage and maintain consistency with existing view tests.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+884
to
+885
| checkState(isLegacyMaterializedViews(session) || view.getOwner().isPresent(), | ||
| "Expected materialized view to have owner (DEFINER security) when legacy materialized views are disabled"); |
There was a problem hiding this comment.
suggestion: The checkState message could be more actionable for debugging.
Include details like the view name or session information in the error message to make failures easier to diagnose.
Suggested change
| checkState(isLegacyMaterializedViews(session) || view.getOwner().isPresent(), | |
| "Expected materialized view to have owner (DEFINER security) when legacy materialized views are disabled"); | |
| checkState(isLegacyMaterializedViews(session) || view.getOwner().isPresent(), | |
| String.format( | |
| "Expected materialized view '%s' to have owner (DEFINER security) when legacy materialized views are disabled. Session user: '%s', Catalog: '%s', Schema: '%s'", | |
| viewName, | |
| session.getUser(), | |
| viewName.getCatalogName(), | |
| view.getSchema())); |
Comment on lines
+2360
to
+2361
| Identity queryIdentity; | ||
| AccessControl queryAccessControl; |
There was a problem hiding this comment.
suggestion: ViewAccessControl is used only when owner is present; clarify rationale.
Consider adding a comment explaining why ViewAccessControl is only assigned when owner is present and how legacy mode affects this logic, to aid future maintainers.
Suggested change
| Identity queryIdentity; | |
| AccessControl queryAccessControl; | |
| Identity queryIdentity; | |
| AccessControl queryAccessControl; | |
| // ViewAccessControl is only assigned when owner is present (definer rights). | |
| // In legacy mode, owner may be absent and invoker rights are used (no ViewAccessControl). | |
| // This ensures correct security semantics for materialized views depending on legacy mode and owner presence. |
Comment on lines
+142
to
+147
| if (isLegacyMaterializedViews(session) && statement.getSecurity().isPresent()) { | ||
| throw new SemanticException( | ||
| NOT_SUPPORTED, | ||
| statement, | ||
| "SECURITY clause is not supported when legacy_materialized_views is enabled"); | ||
| } |
There was a problem hiding this comment.
suggestion: Blocking use of SECURITY clause in legacy mode is appropriate, but error message could be more informative.
Including the rejected SECURITY clause value or relevant session details in the error message would make it clearer for users.
Suggested change
| if (isLegacyMaterializedViews(session) && statement.getSecurity().isPresent()) { | |
| throw new SemanticException( | |
| NOT_SUPPORTED, | |
| statement, | |
| "SECURITY clause is not supported when legacy_materialized_views is enabled"); | |
| } | |
| if (isLegacyMaterializedViews(session) && statement.getSecurity().isPresent()) { | |
| String securityClause = statement.getSecurity().get().toString(); | |
| String sessionInfo = session.getUser() + "@" + session.getSource(); | |
| throw new SemanticException( | |
| NOT_SUPPORTED, | |
| statement, | |
| String.format( | |
| "SECURITY clause '%s' is not supported when legacy_materialized_views is enabled (session: %s)", | |
| securityClause, | |
| sessionInfo)); | |
| } |
| } | ||
|
|
||
| @Test | ||
| public void testDefaultSecurityModeIsDefiner() |
There was a problem hiding this comment.
suggestion (testing): Missing test for explicit SECURITY clause precedence over default_view_security_mode.
Please add a test case where both are set to confirm the SECURITY clause overrides default_view_security_mode.
| } | ||
|
|
||
| @Test | ||
| public void testAccessControlOnMaterializedViewObject() |
There was a problem hiding this comment.
suggestion (testing): Missing test for access control on materialized view with SECURITY INVOKER.
Please add a test for SECURITY INVOKER to verify correct access control enforcement on materialized views with INVOKER rights.
Suggested implementation:
@Test
public void testAccessControlOnMaterializedViewSecurityInvoker()
{
Session adminSession = createSessionForUser("admin");
Session restrictedSession = createSessionForUser("restricted_user");
Session allowedSession = createSessionForUser("allowed_user");
// Admin creates base table and grants SELECT to allowed_user only
assertUpdate(adminSession, "CREATE TABLE secure_base_invoker (id BIGINT, secret VARCHAR, value BIGINT)");
assertUpdate(adminSession, "INSERT INTO secure_base_invoker VALUES (1, 'confidential', 100), (2, 'classified', 200)", 2);
assertUpdate(adminSession, "GRANT SELECT ON secure_base_invoker TO allowed_user");
// Admin creates materialized view with SECURITY INVOKER
assertUpdate(adminSession,
"CREATE MATERIALIZED VIEW mv_invoker " +
"SECURITY INVOKER AS " +
"SELECT id, secret, value FROM secure_base_invoker");
// allowed_user can query the view
assertQuery(allowedSession, "SELECT * FROM mv_invoker", "VALUES (1, 'confidential', 100), (2, 'classified', 200)");
// restricted_user cannot query the view due to lack of SELECT on base table
assertQueryFails(restrictedSession, "SELECT * FROM mv_invoker", "Access Denied: Cannot select from table secure_base_invoker");
// Cleanup
assertUpdate(adminSession, "DROP MATERIALIZED VIEW mv_invoker");
assertUpdate(adminSession, "DROP TABLE secure_base_invoker");
}
Session restrictedSession = createSessionForUser("restricted_user");
assertUpdate(adminSession, "CREATE TABLE secure_base (id BIGINT, secret VARCHAR, value BIGINT)");
assertUpdate(adminSession, "INSERT INTO secure_base VALUES (1, 'confidential', 100), (2, 'classified', 200)", 2);
assertUpdate(adminSession,
"CREATE MATERIALIZED VIEW mv_definer " +
"SECURITY DEFINER AS " +
"SELECT id, secret, value FROM secure_base");You may need to ensure that the users "allowed_user" and "restricted_user" exist and have the correct privileges in your test setup. Adjust the user creation and privilege granting logic as needed to match your environment.
| } | ||
|
|
||
| @Test | ||
| public void testCreateMaterializedViewWithDefaultDefinerSecurity() |
There was a problem hiding this comment.
suggestion (testing): Missing test for invalid default_view_security_mode value.
Add a test to confirm that an invalid default_view_security_mode triggers the correct error, ensuring configuration validation works as intended.
Comment on lines
+1786
to
+1787
| assertStatement("CREATE VIEW a SECURITY DEFINER AS SELECT * FROM t", new CreateView(QualifiedName.of("a"), query, false, Optional.of(DEFINER))); | ||
| assertStatement("CREATE VIEW a SECURITY INVOKER AS SELECT * FROM t", new CreateView(QualifiedName.of("a"), query, false, Optional.of(INVOKER))); |
There was a problem hiding this comment.
nitpick (testing): Nitpick: Consider adding parser tests for materialized views with SECURITY clause.
Adding tests for CREATE MATERIALIZED VIEW with SECURITY DEFINER and SECURITY INVOKER would enhance test coverage and maintain consistency with existing view tests.
tdcmeehan
force-pushed
the
matview-pr-access-control
branch
from
d097da7 to
ff01dae
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Depends on: #26492
Description
Adds SQL standard
SECURITY DEFINERandSECURITY INVOKERsyntax to materialized views. When specified, controls whether views execute with creator's permissions (DEFINER) or querying user's permissions (INVOKER). Defaults todefault_view_security_modewhen theSECURITYis not set (which is defaulted toDEFINER).Motivation and Context
Well-defined access control that is consistent with views are industry standard. Materialized views should have an access control model that is consistent with views and configurable.
Impact
Adds optional
SECURITYclause toCREATE MATERIALIZED VIEW:If
legacy_materialized_views=true, theSECURITYclause will throw if attempted for use in aCREATEstatement, and there are no changes in behavior for refresh or query.Test Plan
Unit and integration tests are included in this PR.
Contributor checklist
Release Notes