Send view security on view creation to OPA authorizer #27029
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 the view‐creation authorization API to include the view security mode, propagates that optional ViewSecurity parameter from the SQL analyzer through execution into the AccessControlManager, updates all forwarding/delegating and plugin‐toolkit access control implementations to accept and pass it along, integrates it into the OPA authorizer (including query builders and enforcement), and updates existing file‐based and Ranger tests to use the new signature. ER diagram for OpaQueryInputResource including ViewSecurityerDiagram
OpaQueryInputResource {
string catalog
string schema
string table
string column
enum security
}
ViewSecurity {
enum INVOKER
enum DEFINER
}
OpaQueryInputResource ||--o| ViewSecurity : has
Class diagram for updated AccessControl and related interfacesclassDiagram
class AccessControl {
+void checkCanCreateView(SecurityContext context, QualifiedObjectName viewName, Optional<ViewSecurity> security)
}
class ForwardingAccessControl {
+void checkCanCreateView(SecurityContext context, QualifiedObjectName viewName, Optional<ViewSecurity> security)
}
class InjectedConnectorAccessControl {
+void checkCanCreateView(ConnectorSecurityContext context, SchemaTableName viewName, Optional<ViewSecurity> security)
}
class TracingAccessControl {
+void checkCanCreateView(SecurityContext context, QualifiedObjectName viewName, Optional<ViewSecurity> security)
}
class AllowAllAccessControl {
+void checkCanCreateView(SecurityContext context, QualifiedObjectName viewName, Optional<ViewSecurity> security)
}
class DenyAllAccessControl {
+void checkCanCreateView(SecurityContext context, QualifiedObjectName viewName, Optional<ViewSecurity> security)
}
class TestingAccessControlManager {
+void checkCanCreateView(SecurityContext context, QualifiedObjectName viewName, Optional<ViewSecurity> security)
}
class ConnectorAccessControl {
+void checkCanCreateView(ConnectorSecurityContext context, SchemaTableName viewName, Optional<ViewSecurity> security)
}
class SystemAccessControl {
+void checkCanCreateView(SystemSecurityContext context, CatalogSchemaTableName view, Optional<ViewSecurity> security)
}
class ViewSecurity {
<<enum>>
INVOKER
DEFINER
}
AccessControl <|.. ForwardingAccessControl
AccessControl <|.. InjectedConnectorAccessControl
AccessControl <|.. TracingAccessControl
AccessControl <|.. AllowAllAccessControl
AccessControl <|.. DenyAllAccessControl
AccessControl <|.. TestingAccessControlManager
ConnectorAccessControl <|.. SystemAccessControl
AccessControl o-- ViewSecurity
ConnectorAccessControl o-- ViewSecurity
SystemAccessControl o-- ViewSecurity
Class diagram for OpaAccessControl and OpaQueryInputResource changesclassDiagram
class OpaAccessControl {
+void checkCanCreateView(SystemSecurityContext context, CatalogSchemaTableName view, Optional<ViewSecurity> security)
-void checkViewOperation(SystemSecurityContext context, String actionName, CatalogSchemaTableName view, Optional<ViewSecurity> security, Consumer<String> deny)
}
class OpaQueryInputResource {
+NamedEntity catalog
+TrinoSchema schema
+TrinoTable table
+TrinoColumn column
+ViewSecurity security
}
class Builder {
+Builder security(ViewSecurity security)
+OpaQueryInputResource build()
}
OpaAccessControl o-- OpaQueryInputResource
OpaQueryInputResource o-- ViewSecurity
OpaQueryInputResource o-- Builder
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 - here's some feedback:
- Add a test case covering the DEFINER security mode to ensure that OPA correctly rejects or handles views defined with DEFINER security as intended.
- Since you’ve updated the AccessControl SPI signature, make sure to bump the SPI version and verify that existing connectors still compile and run against the new default method overload.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Add a test case covering the DEFINER security mode to ensure that OPA correctly rejects or handles views defined with DEFINER security as intended.
- Since you’ve updated the AccessControl SPI signature, make sure to bump the SPI version and verify that existing connectors still compile and run against the new default method overload.
## Individual Comments
### Comment 1
<location> `plugin/trino-opa/src/test/java/io/trino/plugin/opa/TestOpaAccessControl.java:239` </location>
<code_context>
+ }
+
+ @Test
+ public void testCreateViewWithInvokerSecurity()
+ {
+ CatalogSchemaTableName viewName = new CatalogSchemaTableName("my_catalog", "my_schema", "my_view");
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for CreateView with DEFINER security mode.
Please add a test for CreateView with DEFINER security to verify OPA authorizer behavior for both modes.
Suggested implementation:
```java
@Test
public void testCreateViewWithDefinerSecurity()
{
CatalogSchemaTableName viewName = new CatalogSchemaTableName("my_catalog", "my_schema", "my_view");
ThrowingMethodWrapper wrappedMethod = new ThrowingMethodWrapper(
accessControl -> accessControl.checkCanCreateViewWithSecurityMode(
TEST_SECURITY_CONTEXT, viewName, Optional.empty(), SecurityMode.DEFINER));
String expectedRequest =
"""
{
"operation": "CreateView",
"resource": {
"table": {
"catalogName": "%s",
"schemaName": "%s",
"tableName": "%s"
}
},
"securityMode": "DEFINER"
}
""".formatted(
viewName.getCatalogName(),
viewName.getSchemaTableName().getSchemaName(),
viewName.getSchemaTableName().getTableName());
assertAccessControlMethodBehaviour(wrappedMethod, ImmutableSet.of(expectedRequest));
}
@Test
ThrowingMethodWrapper wrappedMethod = new ThrowingMethodWrapper(
accessControl -> accessControl.checkCanCreateView(TEST_SECURITY_CONTEXT, viewName, Optional.empty()));
String expectedRequest =
"""
{
"operation": "CreateView",
"resource": {
"table": {
"catalogName": "%s",
"schemaName": "%s",
```
1. If `checkCanCreateViewWithSecurityMode` or `SecurityMode.DEFINER` do not exist, you will need to implement or import them accordingly.
2. If the OPA authorizer expects a different field for security mode, adjust the `"securityMode": "DEFINER"` part of the expected request.
3. Ensure that the INVOKER test also includes `"securityMode": "INVOKER"` in its expected request for consistency.
</issue_to_address>
### Comment 2
<location> `core/trino-main/src/test/java/io/trino/security/TestFileBasedSystemAccessControl.java:558` </location>
<code_context>
SecurityContext nonAsciiContext = new SecurityContext(transactionId, nonAsciiUser, queryId, queryStart);
- accessControlManager.checkCanCreateView(aliceContext, aliceView);
+ accessControlManager.checkCanCreateView(aliceContext, aliceView, Optional.empty());
accessControlManager.checkCanDropView(aliceContext, aliceView);
accessControlManager.checkCanSelectFromColumns(aliceContext, aliceView, ImmutableSet.of());
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test coverage for CreateView with non-empty security mode.
Please add tests for CreateView using Optional.of(ViewSecurity.INVOKER) and Optional.of(ViewSecurity.DEFINER) to ensure correct behavior for both security modes.
Suggested implementation:
```java
accessControlManager.checkCanCreateView(aliceContext, aliceView, Optional.empty());
accessControlManager.checkCanCreateView(aliceContext, aliceView, Optional.of(ViewSecurity.INVOKER));
accessControlManager.checkCanCreateView(aliceContext, aliceView, Optional.of(ViewSecurity.DEFINER));
```
```java
accessControlManager.checkCanCreateView(aliceContext, staffView, Optional.empty());
accessControlManager.checkCanCreateView(aliceContext, staffView, Optional.of(ViewSecurity.INVOKER));
accessControlManager.checkCanCreateView(aliceContext, staffView, Optional.of(ViewSecurity.DEFINER));
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
|
|
||
| @Test | ||
| public void testCreateViewWithInvokerSecurity() |
There was a problem hiding this comment.
suggestion (testing): Missing test for CreateView with DEFINER security mode.
Please add a test for CreateView with DEFINER security to verify OPA authorizer behavior for both modes.
Suggested implementation:
@Test
public void testCreateViewWithDefinerSecurity()
{
CatalogSchemaTableName viewName = new CatalogSchemaTableName("my_catalog", "my_schema", "my_view");
ThrowingMethodWrapper wrappedMethod = new ThrowingMethodWrapper(
accessControl -> accessControl.checkCanCreateViewWithSecurityMode(
TEST_SECURITY_CONTEXT, viewName, Optional.empty(), SecurityMode.DEFINER));
String expectedRequest =
"""
{
"operation": "CreateView",
"resource": {
"table": {
"catalogName": "%s",
"schemaName": "%s",
"tableName": "%s"
}
},
"securityMode": "DEFINER"
}
""".formatted(
viewName.getCatalogName(),
viewName.getSchemaTableName().getSchemaName(),
viewName.getSchemaTableName().getTableName());
assertAccessControlMethodBehaviour(wrappedMethod, ImmutableSet.of(expectedRequest));
}
@Test
ThrowingMethodWrapper wrappedMethod = new ThrowingMethodWrapper(
accessControl -> accessControl.checkCanCreateView(TEST_SECURITY_CONTEXT, viewName, Optional.empty()));
String expectedRequest =
"""
{
"operation": "CreateView",
"resource": {
"table": {
"catalogName": "%s",
"schemaName": "%s",- If
checkCanCreateViewWithSecurityModeorSecurityMode.DEFINERdo not exist, you will need to implement or import them accordingly. - If the OPA authorizer expects a different field for security mode, adjust the
"securityMode": "DEFINER"part of the expected request. - Ensure that the INVOKER test also includes
"securityMode": "INVOKER"in its expected request for consistency.
sbernauer
force-pushed
the
opa-view-security
branch
from
796a93c to
305bc9d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Description
When creating views you can define two security modes: Invoker and definer, see the docs
We only want to allow views with invoker security using OPA authorization.
For that to work we need to pass the information of the view security to OPA, what exactly is what this PR does.
Going forward other authorizers (e.g. file based or Ranger) can also profit from this information.
Additional context and related issues
First commit does the SPI change.
Second commit uses the new attribute in the OPA authorizer.
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:
This is a breaking change for third-party plugins that implement the
AccessControlinterfaces. I guess we need to also mention that?Summary by Sourcery
Extend view creation authorization to carry and enforce the SQL SECURITY mode by introducing a new ViewSecurity SPI parameter, propagating it through the engine and plugins, and integrating it into the OPA policy checks
New Features:
Enhancements:
Tests: