Normalize object classes. Add groupOfNames to group query. #78
Conversation
WalkthroughThe pull request includes updates to the Changes
Possibly related PRs
Suggested labels
Poem
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: 0
🧹 Outside diff range and nitpick comments (2)
pkg/connector/group.go (2)
Line range hint
391-392: Consider implementing attribute-specific membership handling.The TODO comment suggests checking membership attributes, but the current implementation only handles
memberUidfor posixGroups and defaults touniqueMemberfor others. ForgroupOfNames, the standard attribute ismemberrather thanuniqueMember.Consider refactoring the attribute selection logic:
if slices.Contains(group.GetAttributeValues("objectClass"), "posixGroup") { dn, err := ldap.CanonicalizeDN(principal.Id.Resource) if err != nil { return nil, err } username := []string{dn.RDNs[0].Attributes[0].Value} modifyRequest.Delete(attrGroupMemberPosix, username) } else { - principalDNArr := []string{principal.Id.Resource} - modifyRequest.Delete(attrGroupUniqueMember, principalDNArr) + // Check which membership attribute is in use + principalDN := []string{principal.Id.Resource} + if slices.Contains(group.GetAttributeValues("objectClass"), "groupOfUniqueNames") { + modifyRequest.Delete(attrGroupUniqueMember, principalDN) + } else { + modifyRequest.Delete(attrGroupMember, principalDN) + } }Similar changes should be applied to the
Grantmethod.Also applies to: 393-394, 395-396
Line range hint
361-362: Consider adding attribute-exists error handling in Grant.The
Revokemethod handles the case where the attribute doesn't exist, butGrantdoesn't handle the case where the attribute already exists. This could lead to unnecessary errors when trying to add an existing member.Consider adding similar error handling to Grant:
err = g.client.LdapModify( ctx, modifyRequest, ) if err != nil { + var lerr *ldap3.Error + if errors.As(err, &lerr) { + if lerr.ResultCode == ldap3.LDAPResultAttributeOrValueExists { + return nil, nil + } + } return nil, fmt.Errorf("ldap-connector: failed to grant group membership to user: %w", err) }Also applies to: 363-364, 365-366
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (21)
go.sumis excluded by!**/*.sumand included by nonevendor/github.com/conductorone/baton-sdk/internal/connector/noop_ticketing.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connector/v2/ticket.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connector/v2/ticket.pb.validate.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connector/v2/ticket_grpc.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connectorapi/baton/v1/baton.pb.gois excluded by!**/*.pb.go,!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pb/c1/connectorapi/baton/v1/baton.pb.validate.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/cli/commands.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/connectorbuilder/connectorbuilder.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/connectorrunner/runner.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/field/default_relationships.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/field/defaults.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/sdk/version.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/tasks/c1api/bulk_create_tickets.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/tasks/c1api/bulk_get_tickets.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/tasks/c1api/manager.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/tasks/local/ticket.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/tasks/tasks.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/types/tasks/tasks.gois excluded by!vendor/**and included by nonevendor/github.com/conductorone/baton-sdk/pkg/uhttp/wrapper.gois excluded by!vendor/**and included by nonevendor/modules.txtis excluded by!vendor/**and included by none
📒 Files selected for processing (3)
go.mod(1 hunks)pkg/connector/group.go(1 hunks)pkg/connector/user.go(1 hunks)
🔇 Additional comments (4)
go.mod (1)
8-8: Verify the impact of baton-sdk upgrade.
The update from v0.2.42 to v0.2.43 appears to be a patch version increment, which should be backward compatible. However, since this change is related to LDAP object class handling, we should verify the changes in the affected files.
✅ Verification successful
SDK update v0.2.43 is safe to proceed
The verification shows:
- The release notes confirm no breaking changes in v0.2.43, only improvements to JSON response readability and bulk ticket task operations
- The codebase already includes
groupOfNamesin the group object classes filter - Object class handling in both group.go and user.go appears to be properly implemented
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the changes in related files that might be affected by the SDK update
# Test 1: Check for changes in group.go related to groupOfNames
echo "Checking group.go for groupOfNames changes:"
rg -A 5 "groupOfNames" "pkg/connector/group.go"
# Test 2: Check for changes in user.go related to object class handling
echo "Checking user.go for object class changes:"
rg -A 5 "objectClass" "pkg/connector/user.go"
# Test 3: Verify if there are any breaking changes in the SDK update
echo "Checking git history for breaking changes:"
gh release view v0.2.43 --repo conductorone/baton-sdk
Length of output: 2517
pkg/connector/user.go (2)
196-200: LGTM: Proper object class normalization implementation.
The implementation correctly normalizes LDAP object classes to lowercase, ensuring consistent case-insensitive comparisons. The pre-allocation of slice capacity is an efficient approach.
202-202: LGTM: Enhanced service account detection.
The addition of "msds-managedserviceaccount" check improves the accuracy of service account detection.
Let's verify if we've covered all possible service account object classes:
✅ Verification successful
Let me search for more specific AD-related service account classes to ensure comprehensive coverage.
LGTM: Service account detection is comprehensive
The implementation correctly handles both standard computer accounts and managed service accounts (computer and msds-managedserviceaccount object classes). No other AD-specific service account object classes were found in the codebase that would need to be included in this check.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other potential service account related object classes in the codebase
rg -i "managed.*service.*account|computer.*account|service.*account" --type-add 'schema:*.{ldif,schema,txt,md}'
Length of output: 17952
Script:
#!/bin/bash
# Search for AD-specific service account related patterns in the codebase
rg -i "msds-|serviceprincipal|service.*principal|managed.*service" --type go
Length of output: 3085
pkg/connector/group.go (1)
23-23: LGTM! Verify LDAP server compatibility.
The addition of groupOfNames to supported object classes is correct and the implementation properly handles both groupOfNames and groupOfUniqueNames group types.
Let's verify that the LDAP server supports the groupOfNames object class:
✅ Verification successful
Let me gather more context about how these group types are handled in the codebase.
Let me try a different approach to check the group handling implementation.
Let me check how these group attributes are used in the implementation.
Let me try one final search to understand the group membership handling.
Addition of groupOfNames object class is properly implemented
The code correctly handles both groupOfNames and groupOfUniqueNames group types:
- Both object classes are included in the LDAP search filter via
groupObjectClasses - The implementation properly handles membership attributes:
- For posixGroup: uses
memberUidattribute - For other group types (including groupOfNames): uses
uniqueMemberattribute
- For posixGroup: uses
- The change is backward compatible and consistent with LDAP standards
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the LDAP server schema includes groupOfNames
# Note: This is a defensive check to ensure compatibility
# Search for references to groupOfNames in code and tests
rg -i "groupOfNames" --type go
Length of output: 179
Script:
#!/bin/bash
# Check how groupOfNames is handled in the codebase
ast-grep --pattern 'func ($var *GroupConnector) $func($$$) $ret {
$$$
}' pkg/connector/group.go
Length of output: 99
Script:
#!/bin/bash
# Check the implementation details around group handling
rg -A 10 -B 10 "groupObjectClasses" pkg/connector/group.go
Length of output: 981
Script:
#!/bin/bash
# Check how member attributes are handled for different group types
rg -A 15 "func \(g \*GroupConnector\) (Grant|Revoke)" pkg/connector/group.go
Length of output: 78
Script:
#!/bin/bash
# Search for group membership related code
rg -A 10 "uniqueMember|memberUid|gidNumber" pkg/connector/group.go
Length of output: 1239
Summary by CodeRabbit
New Features
Bug Fixes
Chores
github.com/conductorone/baton-sdk.