-
Notifications
You must be signed in to change notification settings - Fork 26
Support trino roles #322
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Support trino roles #322
Conversation
Reviewer's GuideThis PR implements support for the X-Trino-Role header by extending the frontend configuration UI and type definitions, adding a Role field in the backend settings model, propagating the role value through the datasource context, and injecting it into SQL query arguments. Entity relationship diagram for updated TrinoDatasourceSettings modelerDiagram
TrinoDatasourceSettings {
string ClientId
string ClientSecret
string ImpersonationUser
string Role
string ClientTags
}
Class diagram for updated TrinoDataSourceOptions and TrinoDatasourceSettingsclassDiagram
class TrinoDataSourceOptions {
tokenUrl?: string
clientId?: string
impersonationUser?: string
role?: string
clientTags?: string
}
class TrinoDatasourceSettings {
ClientId string
ClientSecret string
ImpersonationUser string
Role string
ClientTags string
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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> `pkg/trino/datasource-context.go:45-46` </location>
<code_context>
ctx = context.WithValue(ctx, trinoUserHeader, user)
}
+ if settings.Role != "" {
+ ctx = context.WithValue(ctx, trinoRoleHeader, "system=ROLE{"+settings.Role+"}")
+ }
+
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider validating the Role value before formatting.
If settings.Role is user-supplied, ensure it is validated or sanitized to prevent malformed or unexpected header values.
Suggested implementation:
```golang
if settings.Role != "" {
role := sanitizeRole(settings.Role)
if role != "" {
ctx = context.WithValue(ctx, trinoRoleHeader, "system=ROLE{"+role+"}")
}
}
```
```golang
const (
accessTokenKey = "accessToken"
trinoUserHeader = "X-Trino-User"
trinoRoleHeader = "X-Trino-Role"
trinoClientTagsKey = "X-Trino-Client-Tags"
bearerPrefix = "Bearer "
)
// sanitizeRole ensures the role value is safe for use in the header.
// Only allows alphanumeric and underscores, returns empty string if invalid.
func sanitizeRole(role string) string {
for _, r := range role {
if !(r >= 'a' && r <= 'z') && !(r >= 'A' && r <= 'Z') && !(r >= '0' && r <= '9') && r != '_' {
return ""
}
}
return role
}
```
</issue_to_address>
### Comment 2
<location> `pkg/trino/datasource.go:97-98` </location>
<code_context>
args = append(args, sql.Named(accessTokenKey, accessToken.(string)))
}
+ if role != nil {
+ args = append(args, sql.Named(trinoRoleHeader, role.(string)))
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Type assertion on role could panic if not a string.
Use a type check or type switch before asserting role as a string to prevent runtime panics.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Can you add an e2e test that'd verify this works? |
|
in progress. |
a6f9187 to
599f44b
Compare
599f44b to
69759bd
Compare
|
@nineinchnick Could you let me know when you might have time to review this? |
69759bd to
91e6b3e
Compare
582d9b3 to
27a8ed1
Compare
ssheikin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've addressed all comments. Please approve.
27a8ed1 to
943e388
Compare
6d49664 to
98ba0f2
Compare
98ba0f2 to
8ad13e6
Compare
solves
Summary
Support trino roles by allowing users to specify a role in the data source config and passing it through backend context into SQL requests.
rolefield