-
Notifications
You must be signed in to change notification settings - Fork 26
Support X-Trino-Role header
#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?
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. |
| } | ||
|
|
||
| if settings.Role != "" { | ||
| ctx = context.WithValue(ctx, trinoRoleHeader, "system=ROLE{"+settings.Role+"}") |
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'm not sure if system=ROLE{ has to be hardcoded here.
It makes configuration of the plugin more user friendly, but limits specifying roles for catalogs.
Of course, as this PR does not verify input, kind of sql injection may be utilised to specify more roles 🤣
solves
Summary by Sourcery
Support the X-Trino-Role header by allowing users to specify a role in the data source config and passing it through backend context into SQL requests.
New Features:
X-Trino-Roleheader in SQL queriesEnhancements:
rolefield