Flyte Admin RBAC + Project/Domain Isolation #6190
Conversation
Code Review Agent Run Status
|
Code Review Agent Run Status
|
Code Review Agent Run Status
|
Code Review Agent Run Status
|
1 similar comment
Code Review Agent Run Status
|
Sovietaced
changed the title
| return scopes | ||
| } | ||
|
|
||
| func (a *ResourceIsolationFilter) toDbScopes(resourceScope isolation.ResourceScope) func(db *gorm.DB) *gorm.DB { |
There was a problem hiding this comment.
One key implementation detail is that we use GORM's scopes in order to dynamically alter queries: https://gorm.io/docs/advanced_query.html#Scopes
| }).Take(&execution) | ||
| }) | ||
| if isolationFilter != nil { | ||
| cleanSession := tx.Session(&gorm.Session{NewDB: true}) |
There was a problem hiding this comment.
Another key implementation detail is that a new session must be declared on the transaction in order to properly separate the newly injected where clauses within a paranthesis. If this is not done we've see gorm try and be smart and combine/deduplicate clauses which causes bugs.
Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Jason Parraga <[email protected]>
Signed-off-by: Jason Parraga <[email protected]>
Code Review Agent Run #c2b01cActionable Suggestions - 18
Additional Suggestions - 10
Review Details
AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
| isolationFilter := util.GetIsolationFilter(ctx, isolation.DomainTargetResourceScopeDepth, workflowColumnNames) | ||
| tx, err := applyFilters(tx, input.InlineFilters, input.MapFilters, isolationFilter) |
There was a problem hiding this comment.
Consider adding error handling for the GetIsolationFilter() call. While the function may not return an error currently, it's good practice to handle potential errors from security-related functions.
Code suggestion
Check the AI-generated fix before applying
| isolationFilter := util.GetIsolationFilter(ctx, isolation.DomainTargetResourceScopeDepth, workflowColumnNames) | |
| tx, err := applyFilters(tx, input.InlineFilters, input.MapFilters, isolationFilter) | |
| isolationFilter, err := util.GetIsolationFilter(ctx, isolation.DomainTargetResourceScopeDepth, workflowColumnNames) | |
| if err != nil { | |
| return interfaces.WorkflowCollectionOutput{}, err | |
| } | |
| tx, err = applyFilters(tx, input.InlineFilters, input.MapFilters, isolationFilter) |
Code Review Run #c2b01c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| cmdFlags.Bool(fmt.Sprintf("%v%v", prefix, "rbac.enabled"), DefaultConfig.Rbac.Enabled, "Enables RBAC.") | ||
| cmdFlags.StringSlice(fmt.Sprintf("%v%v", prefix, "rbac.bypassMethodPatterns"), DefaultConfig.Rbac.BypassMethodPatterns, "List of regex patterns to match against method names to bypass RBAC.") | ||
| cmdFlags.Bool(fmt.Sprintf("%v%v", prefix, "rbac.tokenScopeRoleResolver.enabled"), DefaultConfig.Rbac.TokenScopeRoleResolver.Enabled, "Enables token scope based role resolution.") | ||
| cmdFlags.Bool(fmt.Sprintf("%v%v", prefix, "rbac.tokenClaimRoleResolver.enabled"), DefaultConfig.Rbac.TokenClaimRoleResolver.Enabled, "Enables token claim based role resolution.") |
There was a problem hiding this comment.
Consider adding validation for rbac.bypassMethodPatterns to ensure valid regex patterns. Invalid regex patterns could cause runtime errors when attempting pattern matching.
Code suggestion
Check the AI-generated fix before applying
| cmdFlags.Bool(fmt.Sprintf("%v%v", prefix, "rbac.enabled"), DefaultConfig.Rbac.Enabled, "Enables RBAC.") | |
| cmdFlags.StringSlice(fmt.Sprintf("%v%v", prefix, "rbac.bypassMethodPatterns"), DefaultConfig.Rbac.BypassMethodPatterns, "List of regex patterns to match against method names to bypass RBAC.") | |
| cmdFlags.Bool(fmt.Sprintf("%v%v", prefix, "rbac.tokenScopeRoleResolver.enabled"), DefaultConfig.Rbac.TokenScopeRoleResolver.Enabled, "Enables token scope based role resolution.") | |
| cmdFlags.Bool(fmt.Sprintf("%v%v", prefix, "rbac.tokenClaimRoleResolver.enabled"), DefaultConfig.Rbac.TokenClaimRoleResolver.Enabled, "Enables token claim based role resolution.") | |
| cmdFlags.Bool(fmt.Sprintf("%v%v", prefix, "rbac.enabled"), DefaultConfig.Rbac.Enabled, "Enables RBAC.") | |
| bypassPatterns := cmdFlags.StringSlice(fmt.Sprintf("%v%v", prefix, "rbac.bypassMethodPatterns"), DefaultConfig.Rbac.BypassMethodPatterns, "List of regex patterns to match against method names to bypass RBAC.") | |
| cmdFlags.VisitAll(func(f *pflag.Flag) { | |
| if f.Name == fmt.Sprintf("%v%v", prefix, "rbac.bypassMethodPatterns") { | |
| f.Validate = func(val string) error { | |
| _, err := regexp.Compile(val) | |
| return err | |
| } | |
| } | |
| }) | |
| cmdFlags.Bool(fmt.Sprintf("%v%v", prefix, "rbac.tokenScopeRoleResolver.enabled"), DefaultConfig.Rbac.TokenScopeRoleResolver.Enabled, "Enables token scope based role resolution.") | |
| cmdFlags.Bool(fmt.Sprintf("%v%v", prefix, "rbac.tokenClaimRoleResolver.enabled"), DefaultConfig.Rbac.TokenClaimRoleResolver.Enabled, "Enables token claim based role resolution.") |
Code Review Run #c2b01c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| cleanSession := tx.Session(&gorm.Session{NewDB: true}) | ||
| tx = tx.Where(cleanSession.Scopes(isolationFilter.GetScopes()...)) |
There was a problem hiding this comment.
Consider using tx.WithContext(ctx) when creating a new session to ensure context propagation. The current implementation may lose context information when creating a new session.
Code suggestion
Check the AI-generated fix before applying
| cleanSession := tx.Session(&gorm.Session{NewDB: true}) | |
| tx = tx.Where(cleanSession.Scopes(isolationFilter.GetScopes()...)) | |
| cleanSession := tx.Session(&gorm.Session{NewDB: true}).WithContext(ctx) | |
| tx = tx.Where(cleanSession.Scopes(isolationFilter.GetScopes()...)) |
Code Review Run #c2b01c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| func ruleMatchesRequest(rule config.Rule, info *grpc.UnaryServerInfo) (bool, error) { | ||
| pattern, err := regexp.Compile(rule.MethodPattern) |
There was a problem hiding this comment.
Consider adding error handling for invalid regex patterns in ruleMatchesRequest. The current implementation could panic if rule.MethodPattern contains an invalid regex pattern.
Code suggestion
Check the AI-generated fix before applying
| func ruleMatchesRequest(rule config.Rule, info *grpc.UnaryServerInfo) (bool, error) { | |
| pattern, err := regexp.Compile(rule.MethodPattern) | |
| func ruleMatchesRequest(rule config.Rule, info *grpc.UnaryServerInfo) (bool, error) { | |
| if rule.MethodPattern == "" { | |
| return false, fmt.Errorf("empty method pattern in rule %s", rule.Name) | |
| } | |
| pattern, err := regexp.Compile(rule.MethodPattern) |
Code Review Run #c2b01c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| ) | ||
| } | ||
|
|
||
| if authCtx.Options().Rbac.Enabled { |
There was a problem hiding this comment.
Consider adding error handling for the case when authCtx.Options() returns nil. The current code assumes authCtx.Options() always returns a valid value, which could lead to a nil pointer dereference if it doesn't.
Code suggestion
Check the AI-generated fix before applying
| if authCtx.Options().Rbac.Enabled { | |
| opts := authCtx.Options() | |
| if opts == nil { | |
| return nil, fmt.Errorf("auth context options not initialized") | |
| } | |
| if opts.Rbac.Enabled { |
Code Review Run #c2b01c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| isolationFilter := util.GetIsolationFilter(ctx, isolation.DomainTargetResourceScopeDepth, descriptionEntityResourceColumns) | ||
| tx, err = applyFilters(tx, filters, nil, isolationFilter) |
There was a problem hiding this comment.
Consider adding error handling for GetIsolationFilter(). The function could potentially return errors that should be handled before passing the filter to applyFilters().
Code suggestion
Check the AI-generated fix before applying
| isolationFilter := util.GetIsolationFilter(ctx, isolation.DomainTargetResourceScopeDepth, descriptionEntityResourceColumns) | |
| tx, err = applyFilters(tx, filters, nil, isolationFilter) | |
| isolationFilter, err := util.GetIsolationFilter(ctx, isolation.DomainTargetResourceScopeDepth, descriptionEntityResourceColumns) | |
| if err != nil { | |
| return models.DescriptionEntity{}, err | |
| } | |
| tx, err = applyFilters(tx, filters, nil, isolationFilter) |
Code Review Run #c2b01c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| @@ -52,6 +68,8 @@ | |||
| func (r *ProjectRepo) List(ctx context.Context, input interfaces.ListResourceInput) ([]models.Project, error) { | |||
| var projects []models.Project | |||
|
|
|||
| isolationFilter := util.GetIsolationFilter(ctx, isolation.ProjectTargetResourceScopeDepth, projectResourceColumns) | |||
There was a problem hiding this comment.
Consider adding error handling for GetIsolationFilter() call. The function may return nil but there's no validation of the error case before using the filter.
Code suggestion
Check the AI-generated fix before applying
| isolationFilter := util.GetIsolationFilter(ctx, isolation.ProjectTargetResourceScopeDepth, projectResourceColumns) | |
| isolationFilter, err := util.GetIsolationFilter(ctx, isolation.ProjectTargetResourceScopeDepth, projectResourceColumns) | |
| if err != nil { | |
| return models.Project{}, r.errorTransformer.ToFlyteAdminError(err) | |
| } |
Code Review Run #c2b01c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| cleanSession := tx.Session(&gorm.Session{NewDB: true}) | ||
| tx = tx.Where(cleanSession.Scopes(isolationFilter.GetScopes()...)) |
There was a problem hiding this comment.
Consider using tx.WithContext(ctx) before creating a new session to ensure context propagation. The current implementation may lose context information when creating a new session.
Code suggestion
Check the AI-generated fix before applying
| cleanSession := tx.Session(&gorm.Session{NewDB: true}) | |
| tx = tx.Where(cleanSession.Scopes(isolationFilter.GetScopes()...)) | |
| cleanSession := tx.WithContext(ctx).Session(&gorm.Session{NewDB: true}) | |
| tx = tx.WithContext(ctx).Where(cleanSession.Scopes(isolationFilter.GetScopes()...)) |
Code Review Run #c2b01c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| func (ap AuthorizationPolicy) UnmarshalJSON(b []byte) error { | ||
| err := json.Unmarshal(b, &ap) |
There was a problem hiding this comment.
The UnmarshalJSON method for AuthorizationPolicy appears to be unmarshaling into a local variable ap rather than the receiver. Consider using a pointer receiver to modify the actual struct.
Code suggestion
Check the AI-generated fix before applying
| func (ap AuthorizationPolicy) UnmarshalJSON(b []byte) error { | |
| err := json.Unmarshal(b, &ap) | |
| func (ap *AuthorizationPolicy) UnmarshalJSON(b []byte) error { | |
| err := json.Unmarshal(b, ap) |
Code Review Run #c2b01c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| func (tc TokenClaim) UnmarshalJSON(b []byte) error { | ||
| err := json.Unmarshal(b, &tc) |
There was a problem hiding this comment.
Similar to the AuthorizationPolicy issue, the UnmarshalJSON method for TokenClaim has the same problem with using a value receiver instead of a pointer receiver.
Code suggestion
Check the AI-generated fix before applying
| func (tc TokenClaim) UnmarshalJSON(b []byte) error { | |
| err := json.Unmarshal(b, &tc) | |
| func (tc *TokenClaim) UnmarshalJSON(b []byte) error { | |
| err := json.Unmarshal(b, tc) |
Code Review Run #c2b01c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| cleanSession := tx.Session(&gorm.Session{NewDB: true}) | ||
| tx = tx.Where(cleanSession.Scopes(isolationFilter.GetScopes()...)) |
There was a problem hiding this comment.
Consider using tx.WithContext(ctx) when creating a new session to ensure context propagation. The current implementation with cleanSession may lose the context information.
Code suggestion
Check the AI-generated fix before applying
| cleanSession := tx.Session(&gorm.Session{NewDB: true}) | |
| tx = tx.Where(cleanSession.Scopes(isolationFilter.GetScopes()...)) | |
| cleanSession := tx.Session(&gorm.Session{NewDB: true}).WithContext(ctx) | |
| tx = tx.Where(cleanSession.Scopes(isolationFilter.GetScopes()...)) |
Code Review Run #c2b01c
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Tracking issue
Closes:
Related: #5871
Why are the changes needed?
See: #5871
What changes were proposed in this pull request?
This pull request adds an optional rbac/authorization interceptor. This interceptor does the following:
2 Evaluates whether the target RPC is allowed to be called based on the authorization policy rules associated with any matching roles.
This pull request also modified the DB layer such that mutative operations on resources are validated against the isolation context scope (if present) and filters resources based on the isolation context scope (if present). There are key implementation details where where clauses are carefully injected into queries being made against the DB.
How was this patch tested?
Unit tests. I could add unit tests to the DB layer but this would require a lot of coverage.
Hasn't been tested end to end yet.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
This PR implements Role-Based Access Control (RBAC) and resource isolation in Flyte Admin, introducing an authorization interceptor for validating user roles and permissions. The implementation includes configuration structures, command-line flags, and deployment updates across different environments. The changes enable fine-grained access control at project and domain levels, with support for bypass patterns and flexible role resolution mechanisms.Unit tests added: True
Estimated effort to review (1-5, lower is better): 5