Support sending user principal to OPA #27204
Open
+118
−13
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 GuideAdded optional inclusion of the user’s Principal in authorization requests to OPA via a new Entity relationship diagram for TrinoIdentity principal inclusionerDiagram
IDENTITY {
string user
string[] groups
principal principal
}
TRINO_IDENTITY {
string user
string[] groups
principal principal
}
PRINCIPAL {
string details
}
IDENTITY ||--o| PRINCIPAL : has
TRINO_IDENTITY ||--o| PRINCIPAL : has
Class diagram for updated TrinoIdentity and TrinoUserclassDiagram
class Identity {
+String user
+Set<String> groups
+Optional<Principal> principal
}
class TrinoIdentity {
+String user
+Set<String> groups
+Optional<Principal> principal
+static fromTrinoIdentity(identity: Identity, includeUserPrincipal: boolean): TrinoIdentity
}
class TrinoUser {
+TrinoUser(identity: Identity, includeUserPrincipal: boolean)
}
Identity <|-- TrinoIdentity
TrinoIdentity <|-- TrinoUser
Class diagram for updated OpaConfig and OpaAccessControlclassDiagram
class OpaConfig {
-boolean includeUserPrincipal
+boolean getIncludeUserPrincipal()
+OpaConfig setIncludeUserPrincipal(boolean)
}
class OpaAccessControl {
-boolean includeUserPrincipal
+OpaAccessControl(..., OpaConfig config, ...)
}
OpaConfig <|-- OpaAccessControl
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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `plugin/trino-opa/src/test/java/io/trino/plugin/opa/TestOpaAccessControl.java:694-695` </location>
<code_context>
assertStringRequestsEqual(ImmutableSet.of(expectedRequest), mockClient.getRequests(), "/input");
}
+ @Test
+ void testIncludeUserPrincipal() {
+ InstrumentedHttpClient mockClient = createMockHttpClient(OPA_SERVER_URI, request -> OK_RESPONSE);
+ OpaAccessControl authorizer = (OpaAccessControl) OpaAccessControlFactory.create(
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding negative and edge case tests for principal inclusion.
Please add tests for: (1) config set to false, ensuring principal is excluded; (2) principal is null or missing; (3) principal with special characters or large size. This will improve coverage of edge cases.
Suggested implementation:
```java
@Test
void testIncludeUserPrincipal() {
InstrumentedHttpClient mockClient = createMockHttpClient(OPA_SERVER_URI, request -> OK_RESPONSE);
OpaAccessControl authorizer = (OpaAccessControl) OpaAccessControlFactory.create(
ImmutableMap.of("opa.policy.uri", OPA_SERVER_URI.toString(), "opa.include-user-principal", "true"),
Optional.of(mockClient),
Optional.empty());
Identity sampleIdentityWithGroupsAndPrincipal = Identity.forUser("test_user").withGroups(ImmutableSet.of("some_group")).withPrincipal(new SerializableTestPrincipal("test_principal")).build();
authorizer.checkCanExecuteQuery(sampleIdentityWithGroupsAndPrincipal, TEST_QUERY_ID);
String expectedRequest =
"""
{
"action": {
```
```java
}
""";
assertStringRequestsEqual(ImmutableSet.of(expectedRequest), mockClient.getRequests(), "/input");
}
@Test
void testExcludeUserPrincipalWhenConfigFalse() {
InstrumentedHttpClient mockClient = createMockHttpClient(OPA_SERVER_URI, request -> OK_RESPONSE);
OpaAccessControl authorizer = (OpaAccessControl) OpaAccessControlFactory.create(
ImmutableMap.of("opa.policy.uri", OPA_SERVER_URI.toString(), "opa.include-user-principal", "false"),
Optional.of(mockClient),
Optional.empty());
Identity sampleIdentityWithPrincipal = Identity.forUser("test_user").withGroups(ImmutableSet.of("some_group")).withPrincipal(new SerializableTestPrincipal("test_principal")).build();
authorizer.checkCanExecuteQuery(sampleIdentityWithPrincipal, TEST_QUERY_ID);
String expectedRequest =
"""
{
"action": {
"type": "EXECUTE_QUERY",
"queryId": "test_query_id"
},
"user": {
"user": "test_user",
"groups": ["some_group"]
}
}
""";
assertStringRequestsEqual(ImmutableSet.of(expectedRequest), mockClient.getRequests(), "/input");
}
@Test
void testPrincipalIsNullOrMissing() {
InstrumentedHttpClient mockClient = createMockHttpClient(OPA_SERVER_URI, request -> OK_RESPONSE);
OpaAccessControl authorizer = (OpaAccessControl) OpaAccessControlFactory.create(
ImmutableMap.of("opa.policy.uri", OPA_SERVER_URI.toString(), "opa.include-user-principal", "true"),
Optional.of(mockClient),
Optional.empty());
Identity sampleIdentityWithoutPrincipal = Identity.forUser("test_user").withGroups(ImmutableSet.of("some_group")).build();
authorizer.checkCanExecuteQuery(sampleIdentityWithoutPrincipal, TEST_QUERY_ID);
String expectedRequest =
"""
{
"action": {
"type": "EXECUTE_QUERY",
"queryId": "test_query_id"
},
"user": {
"user": "test_user",
"groups": ["some_group"]
}
}
""";
assertStringRequestsEqual(ImmutableSet.of(expectedRequest), mockClient.getRequests(), "/input");
}
@Test
void testPrincipalWithSpecialCharactersAndLargeSize() {
InstrumentedHttpClient mockClient = createMockHttpClient(OPA_SERVER_URI, request -> OK_RESPONSE);
OpaAccessControl authorizer = (OpaAccessControl) OpaAccessControlFactory.create(
ImmutableMap.of("opa.policy.uri", OPA_SERVER_URI.toString(), "opa.include-user-principal", "true"),
Optional.of(mockClient),
Optional.empty());
String specialPrincipal = "user!@#$%^&*()_+-=[]{},.<>?/|~`" + "A".repeat(1000);
Identity sampleIdentityWithSpecialPrincipal = Identity.forUser("test_user").withGroups(ImmutableSet.of("some_group")).withPrincipal(new SerializableTestPrincipal(specialPrincipal)).build();
authorizer.checkCanExecuteQuery(sampleIdentityWithSpecialPrincipal, TEST_QUERY_ID);
String expectedRequest =
String.format("""
{
"action": {
"type": "EXECUTE_QUERY",
"queryId": "test_query_id"
},
"user": {
"user": "test_user",
"groups": ["some_group"],
"principal": "%s"
}
}
""", specialPrincipal);
assertStringRequestsEqual(ImmutableSet.of(expectedRequest), mockClient.getRequests(), "/input");
}
```
</issue_to_address>
### Comment 2
<location> `plugin/trino-opa/src/test/java/io/trino/plugin/opa/SerializableTestPrincipal.java:8-10` </location>
<code_context>
+
+import java.security.Principal;
+
+// A Principal for tests that can be serialized using Jackson
+@JsonSerialize
+public class SerializableTestPrincipal implements Principal {
+ private String name;
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for serialization/deserialization of SerializableTestPrincipal.
Adding a unit test with Jackson will ensure SerializableTestPrincipal's serialization works as expected and prevent future regressions.
Suggested implementation:
```java
package io.trino.plugin.opa;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.junit.jupiter.api.Test;
import static org.assertj.core.api.Assertions.assertThat;
public class SerializableTestPrincipalTest {
@Test
public void testSerializationDeserialization() throws Exception {
ObjectMapper objectMapper = new ObjectMapper();
SerializableTestPrincipal principal = new SerializableTestPrincipal("test-user");
// Serialize to JSON
String json = objectMapper.writeValueAsString(principal);
// Deserialize from JSON
SerializableTestPrincipal deserialized = objectMapper.readValue(json, SerializableTestPrincipal.class);
// Assert that the name is preserved
assertThat(deserialized.getName()).isEqualTo("test-user");
assertThat(deserialized.getName()).isEqualTo(principal.getName());
}
}
```
You may need to ensure that `SerializableTestPrincipal` has a public constructor accepting a `String name` and a public getter for `name` (e.g., `getName()`), as well as any necessary Jackson annotations for proper serialization/deserialization. If these are missing, add them to `SerializableTestPrincipal.java`.
</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
8
to
23
| // A Principal for tests that can be serialized using Jackson | ||
| @JsonSerialize | ||
| public class SerializableTestPrincipal implements Principal { |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for serialization/deserialization of SerializableTestPrincipal.
Adding a unit test with Jackson will ensure SerializableTestPrincipal's serialization works as expected and prevent future regressions.
Suggested implementation:
package io.trino.plugin.opa;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.junit.jupiter.api.Test;
import static org.assertj.core.api.Assertions.assertThat;
public class SerializableTestPrincipalTest {
@Test
public void testSerializationDeserialization() throws Exception {
ObjectMapper objectMapper = new ObjectMapper();
SerializableTestPrincipal principal = new SerializableTestPrincipal("test-user");
// Serialize to JSON
String json = objectMapper.writeValueAsString(principal);
// Deserialize from JSON
SerializableTestPrincipal deserialized = objectMapper.readValue(json, SerializableTestPrincipal.class);
// Assert that the name is preserved
assertThat(deserialized.getName()).isEqualTo("test-user");
assertThat(deserialized.getName()).isEqualTo(principal.getName());
}
}You may need to ensure that SerializableTestPrincipal has a public constructor accepting a String name and a public getter for name (e.g., getName()), as well as any necessary Jackson annotations for proper serialization/deserialization. If these are missing, add them to SerializableTestPrincipal.java.
sbernauer
force-pushed
the
feat/opa-send-user-principal
branch
from
c792b22 to
dd4f02d
Compare
Open
6 tasks
sbernauer
force-pushed
the
feat/opa-send-user-principal
branch
from
dd4f02d to
70d3a96
Compare
lfrancke
suggested changes
Nov 5, 2025
plugin/trino-opa/src/main/java/io/trino/plugin/opa/OpaConfig.java
Outdated
Show resolved
Hide resolved
| - Configure if permission management operations are allowed. Find more details in | ||
| [](opa-permission-management). Defaults to `false`. | ||
| * - `opa.include-user-principal` | ||
| - Whether to include the users principal when sending authorization requests to OPA. Defaults to `false`. |
Member
There was a problem hiding this comment.
Suggested change
| - Whether to include the users principal when sending authorization requests to OPA. Defaults to `false`. | |
| - Whether to include the user's principal when sending authorization requests to OPA. Defaults to `false`. |
Member
There was a problem hiding this comment.
In addition I think this could use a few more details. What exactly is it it's sending? I guess it depends on the authentication method?
Member
Author
There was a problem hiding this comment.
Picked the suggestion. Honestly, I don't know. I also assume it depends on the authentication method used. I'm no Java expert, this code only sees the java.security.Principal interface.
| private final LifeCycleManager lifeCycleManager; | ||
| private final OpaHighLevelClient opaHighLevelClient; | ||
| private final boolean allowPermissionManagementOperations; | ||
| protected final boolean includeUserPrincipal; |
Member
There was a problem hiding this comment.
Personally I'd prefer if this could be made private and the create an access method.
Member
Author
There was a problem hiding this comment.
No opinion. Moved it behind this function
protected boolean allowPermissionManagementOperations()
{
return this.allowPermissionManagementOperations;
}| String user, | ||
| Set<String> groups) | ||
| Set<String> groups, | ||
| Optional<Principal> principal) |
Member
There was a problem hiding this comment.
I checked how these are sent and it seems the OpaHttpClient serializes everything to JSON. Are we sure that all principals can be serialized properly?
I don't have a really good answer myself for how to test this to be honest.
Member
Author
There was a problem hiding this comment.
Yeah, we only have the java.security.Principal interface here. In Rust I would simply add a trait bound on Serialize and the compiler would ensure all passed types can be serialized :P
This is the main reason this is opt-in, so only users that really need access to the principal in OPA need to make sure it's serializable.
It would be great to find out what possible concrete types the Trino source code passes to authorizers.
sbernauer
force-pushed
the
feat/opa-send-user-principal
branch
from
70d3a96 to
f2a7e3c
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.
Description
Sometimes more context than the username and/or groups is helpful to make an authorization decision.
This PR adds support to additionally send the users principal to the OPA server.
As this increases the payload size for every OPA request, we made this opt-in using the configuration property
opa.include-user-principalRelease 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:
Summary by Sourcery
Add optional support for including the user principal in OPA authorization requests based on a new configuration property.
New Features:
Enhancements:
Documentation:
Tests:
Chores: