Add support for groups being members of groups. #81
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
pkg/connector/group.go(5 hunks)
🔇 Additional comments (3)
pkg/connector/group.go (3)
22-31: Object class to resource type mapping implemented correctly
The mapping between LDAP objectClass values and resource types is correctly established, enhancing resource type identification during grant creation.
160-168: Updated newGrantFromDN function to handle resource types
The modification of newGrantFromDN to accept a resourceType parameter and adjust grantOpts accordingly improves the flexibility and accuracy of grant creation based on resource types.
185-193: Added newGrantFromEntry function for dynamic grant creation
The new newGrantFromEntry function efficiently creates grants by determining the resource type from an LDAP entry's objectClass attributes, enhancing the adaptability of the grant creation process.
| member, _, err := g.client.LdapSearch( | ||
| ctx, | ||
| ldap3.ScopeWholeSubtree, | ||
| parsedDN, | ||
| "", | ||
| nil, | ||
| "", | ||
| 1, | ||
| ) | ||
| if err != nil { | ||
| l.Error("ldap-connector: failed to get group member", zap.String("group", groupDN.String()), zap.String("member_id", memberId), zap.Error(err)) | ||
| } |
There was a problem hiding this comment.
Handle LDAP search errors to prevent potential panics
If g.client.LdapSearch returns an error, the code logs the error but continues processing. This could lead to unexpected behavior or panics since member might be nil or invalid. It's important to handle the error appropriately by skipping the iteration when an error occurs.
Apply this diff to handle the error:
if err != nil {
l.Error("ldap-connector: failed to get group member", zap.String("group", groupDN.String()), zap.String("member_id", memberId), zap.Error(err))
+ continue
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| member, _, err := g.client.LdapSearch( | |
| ctx, | |
| ldap3.ScopeWholeSubtree, | |
| parsedDN, | |
| "", | |
| nil, | |
| "", | |
| 1, | |
| ) | |
| if err != nil { | |
| l.Error("ldap-connector: failed to get group member", zap.String("group", groupDN.String()), zap.String("member_id", memberId), zap.Error(err)) | |
| } | |
| member, _, err := g.client.LdapSearch( | |
| ctx, | |
| ldap3.ScopeWholeSubtree, | |
| parsedDN, | |
| "", | |
| nil, | |
| "", | |
| 1, | |
| ) | |
| if err != nil { | |
| l.Error("ldap-connector: failed to get group member", zap.String("group", groupDN.String()), zap.String("member_id", memberId), zap.Error(err)) | |
| continue | |
| } |
ggreer
force-pushed
the
ggreer/group-grant-expansion
branch
4 times, most recently
from
fb22b18 to
deab930
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
pkg/connector/group.go (1)
160-181: LGTM! Consider adding validation for resourceType.The function correctly handles both user and group principals, with proper annotation for expandable grants. Consider adding validation to ensure resourceType is not nil.
Add parameter validation:
func newGrantFromDN(groupResource *v2.Resource, dn string, resourceType *v2.ResourceType) *v2.Grant { + if resourceType == nil { + resourceType = resourceTypeUser // fallback to user type + } grantOpts := []grant.GrantOption{}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
.github/workflows/ci.yamlis excluded by none and included by nonescripts/get-baton.shis excluded by none and included by nonescripts/grant-revoke.shis excluded by none and included by none
📒 Files selected for processing (1)
pkg/connector/group.go(5 hunks)
🔇 Additional comments (4)
pkg/connector/group.go (4)
267-267: LGTM! Consistent grant creation for all member types.
The code properly handles grant creation for both direct members and POSIX group members using the new grant creation functions.
Also applies to: 297-297
237-255:
Fix error handling in member lookup.
The error handling needs improvement to prevent potential issues with nil member entries.
Apply this diff to properly handle the error:
if err != nil {
l.Error("ldap-connector: failed to get group member", zap.String("group", groupDN.String()), zap.String("member_id", memberId), zap.Error(err))
+ continue
}22-31: LGTM! Comprehensive mapping of LDAP object classes.
The mapping covers standard LDAP object classes and correctly associates them with appropriate resource types, enabling proper handling of both user and group members.
Let's verify that all object classes in use are covered:
✅ Verification successful
The object class mapping is complete and accurate
The verification confirms that all object classes used in LDAP filters (groupObjectClasses and userObjectClasses) are properly mapped in the objectClassesToResourceTypes variable:
Groups:
- group
- groupOfNames
- groupOfUniqueNames
- posixGroup
Users:
- inetOrgPerson
- person
- user
- organizationalPerson
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any unmapped object classes in LDAP entries
# that might be groups or users
# Search for object classes in group entries
rg -g '*.go' 'objectClass=' | grep -i 'group'
# Search for object classes in user entries
rg -g '*.go' 'objectClass=' | grep -i 'user\|person'
Length of output: 394
185-201: LGTM! Robust handling of LDAP entry to grant conversion.
The function properly handles DN canonicalization and object class mapping with a sensible fallback to user type.
Let's verify the object class handling:
✅ Verification successful
Object class to resource type mapping is complete and consistent
The verification confirms that:
- The
objectClassesToResourceTypesmapping correctly handles all LDAP object classes used in filters (groupOfUniqueNames,groupOfNames,posixGroup,group,inetOrgPerson,person,user,organizationalPerson) - Each object class maps to the appropriate resource type (
resourceTypeUserorresourceTypeGroup) - The fallback to
resourceTypeUserin the function is a safe default when no matching object class is found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential edge cases in object class handling
# Search for object class attribute usage
ast-grep --pattern 'entry.GetAttributeValues("objectClass")'
# Check for any non-standard object classes in use
rg -g '*.go' 'objectClass\s*=\s*[^)]+'
Length of output: 569
Script:
#!/bin/bash
# Let's check the objectClassesToResourceTypes mapping to ensure it aligns with the object classes we found
rg -g '*.go' 'objectClassesToResourceTypes\s*=\s*' -A 10
# Also check for any resource type definitions
rg -g '*.go' 'resourceType[A-Z][a-zA-Z]*\s*=\s*' -B 1 -A 1
Length of output: 2121
ggreer
force-pushed
the
ggreer/group-grant-expansion
branch
from
deab930 to
f3d56bb
Compare
Summary by CodeRabbit