feat(authz): add multi group claims support #2984
Conversation
- support multi group claims - refactor for efficiency - refactor for future casbin adapter support
Summary of ChangesHello @jrschumacher, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the authorization capabilities by introducing robust support for multiple group claims. This allows for more granular and flexible role-based access control, drawing user roles from various locations within authentication tokens or user profiles. The underlying Casbin integration has been optimized and modularized, paving the way for future extensibility with different policy storage mechanisms. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. A token arrives, claims nested deep, Roles for the user, secrets to keep. Casbin now scans, with logic so keen, Granting access, a well-oiled machine. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for multiple group claims in the authorization logic, which is a great enhancement. The refactoring efforts are commendable, especially the separation of CSV policy building into its own file (casbin_csv.go) and moving the dotNotation utility to a shared package. This improves modularity and prepares the codebase for future extensions, like supporting different Casbin adapters. The test suite has also been significantly improved with more comprehensive and table-driven tests, increasing confidence in the changes. My only suggestion is to address a piece of duplicated code in casbin.go to further improve maintainability.
| // extractRolesFromToken extracts roles from a jwt.Token based on the configured claim path | ||
| func (e *Enforcer) extractRolesFromToken(token jwt.Token) []string { | ||
| roles := make([]string, 0, defaultRolesCapacity) // preallocate for common case | ||
| for _, selectors := range e.groupClaimSelectors { | ||
| if len(selectors) == 0 { | ||
| continue | ||
| } | ||
| claim = dotNotation(claimMap, strings.Join(selectors[1:], ".")) | ||
| if claim == nil { | ||
| e.logger.Warn("claim not found", | ||
| slog.String("claim", roleClaim), | ||
| slog.Any("claims", claim), | ||
| ) | ||
| return nil | ||
| claim, exists := token.Get(selectors[0]) | ||
| if !exists { | ||
| continue // skip missing claim, don't log on hot path | ||
| } | ||
| if len(selectors) > 1 { | ||
| claimMap, ok := claim.(map[string]interface{}) | ||
| if !ok { | ||
| continue // skip invalid type | ||
| } | ||
| claim = util.Dotnotation(claimMap, strings.Join(selectors[1:], ".")) | ||
| if claim == nil { | ||
| continue | ||
| } | ||
| } | ||
| // Inline extractRolesFromClaim for efficiency | ||
| switch v := claim.(type) { | ||
| case string: | ||
| roles = append(roles, v) | ||
| case []interface{}: | ||
| for _, rr := range v { | ||
| if r, ok := rr.(string); ok { | ||
| roles = append(roles, r) | ||
| } | ||
| } | ||
| case []string: | ||
| roles = append(roles, v...) | ||
| } | ||
| } | ||
| return roles | ||
| } | ||
|
|
||
| // check the type of the role claim | ||
| switch v := claim.(type) { | ||
| case string: | ||
| roles = append(roles, v) | ||
| case []interface{}: | ||
| for _, rr := range v { | ||
| if r, ok := rr.(string); ok { | ||
| roles = append(roles, r) | ||
| // extractRolesFromUserInfo extracts roles from a userInfo JSON ([]byte) based on the configured claim path | ||
| func (e *Enforcer) extractRolesFromUserInfo(userInfo []byte) []string { | ||
| roles := make([]string, 0, defaultRolesCapacity) | ||
| if userInfo == nil || len(userInfo) == 0 { | ||
| return roles | ||
| } | ||
| var userInfoMap map[string]interface{} | ||
| if err := json.Unmarshal(userInfo, &userInfoMap); err != nil { | ||
| return roles // skip logging on hot path | ||
| } | ||
| for _, selectors := range e.groupClaimSelectors { | ||
| if len(selectors) == 0 { | ||
| continue | ||
| } | ||
| claim := util.Dotnotation(userInfoMap, strings.Join(selectors, ".")) | ||
| if claim == nil { | ||
| continue | ||
| } | ||
| switch v := claim.(type) { | ||
| case string: | ||
| roles = append(roles, v) | ||
| case []interface{}: | ||
| for _, rr := range v { | ||
| if r, ok := rr.(string); ok { | ||
| roles = append(roles, r) | ||
| } | ||
| } | ||
| case []string: | ||
| roles = append(roles, v...) | ||
| } | ||
| default: | ||
| e.logger.Warn("could not get claim type", | ||
| slog.String("selector", roleClaim), | ||
| slog.Any("claims", claim), | ||
| ) | ||
| return nil | ||
| } | ||
|
|
||
| return roles | ||
| } |
There was a problem hiding this comment.
The functions extractRolesFromToken and extractRolesFromUserInfo contain nearly identical logic for extracting role strings from a claim value. This duplication can be reduced by extracting the common logic into a helper function. While the comment on line 205 suggests inlining was done for efficiency, the performance gain is likely negligible for this small function, and extracting it would improve maintainability.
A potential helper function could look like this:
// extractRolesFromClaim is a helper to extract role strings from a claim value.
func (e *Enforcer) extractRolesFromClaim(claim interface{}) []string {
var roles []string
switch v := claim.(type) {
case string:
if v != "" {
roles = append(roles, v)
}
case []interface{}:
for _, rr := range v {
if r, ok := rr.(string); ok && r != "" {
roles = append(roles, r)
}
}
case []string:
for _, r := range v {
if r != "" {
roles = append(roles, r)
}
}
}
return roles
}This helper could then be called from both extractRolesFromToken and extractRolesFromUserInfo to handle the claim value, thus removing the duplicated code.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
NANOTDF Benchmark Results:
|
Checklist
Testing Instructions