Centralize DN Canoicalization and minor improvements #74
Conversation
WalkthroughThe changes in this pull request involve significant updates to several files primarily focused on integrating a new LDAP package, modifying data types, and enhancing error handling across various components. Key modifications include the replacement of LDAP-related types and methods, updates to user and group filtering logic, and the introduction of new functions for handling distinguished names (DNs). The overall structure of the code remains intact while improving clarity, reliability, and maintainability. Changes
Possibly related PRs
Suggested reviewers
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 (
|
pquerna
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (14)
pkg/ldap/dn.go (2)
9-26: Add a period to the comment for consistency.The variable declaration looks good and covers common LDAP attributes efficiently. However, to address the static analysis hint and maintain consistency:
-// List of attribute types whose values should be made lowercase +// List of attribute types whose values should be made lowercase.🧰 Tools
🪛 GitHub Check: go-lint
[failure] 9-9:
Comment should end in a period (godot)
28-46: Consider handling empty input strings.The
CanonicalizeDNfunction looks well-implemented and correctly handles the canonicalization process. It properly standardizes attribute types, trims whitespace, and selectively lowercases attribute values based on thecaseInsensitiveAttrsmap.Consider adding a check for empty input strings at the beginning of the function:
func CanonicalizeDN(dn string) (*ldap3.DN, error) { + if dn == "" { + return nil, errors.New("empty DN provided") + } dnv, err := ldap3.ParseDN(dn) if err != nil { return nil, err } // ... rest of the function }This addition would provide more explicit handling for edge cases and improve the function's robustness.
pkg/connector/connector_test.go (1)
Line range hint
93-97: Well-implementedmustParseDNfunctionThe
mustParseDNfunction is a great addition that encapsulates DN parsing and error handling. It improves code clarity and ensures consistent error handling across tests.Consider adding a custom error message to
require.NoErrorto provide more context if the parsing fails:func mustParseDN(t *testing.T, input string) *ldap.DN { dn, err := ldap.ParseDN(input) - require.NoError(t, err) + require.NoError(t, err, "Failed to parse DN: %s", input) return dn }This change will make it easier to identify which DN failed to parse if an error occurs.
pkg/connector/helpers.go (2)
46-46: Update function comment to reflect new return type.The change in return type from
[]stringtomapset.Set[string]is appropriate for the new implementation. However, the function comment should be updated to reflect this change.Consider updating the function comment as follows:
// Parses the values of targeted attributes from an LDAP entry and returns them as a set of strings. func parseValues(entry *ldap.Entry, targetAttrs []string) mapset.Set[string] {
47-53: LGTM: Improved implementation using a set.The new implementation using a set is more efficient and ensures uniqueness of values. This is a good improvement over the previous slice-based approach.
Consider a minor optimization by using
AddAllmethod of the set:func parseValues(entry *ldap.Entry, targetAttrs []string) mapset.Set[string] { rv := mapset.NewSet[string]() for _, targetAttr := range targetAttrs { rv.AddAll(stringSliceToInterfaceSlice(entry.GetAttributeValues(targetAttr))...) } return rv }This change would reduce the number of method calls on the set and potentially improve performance for large attribute values.
pkg/config/config.go (5)
Line range hint
107-111: **Type Mismatch: Assigning ldap.DN to ldap3.DNAt line 107~,
bindDNis obtained fromldap.CanonicalizeDN(bindDNValue)as a*ldap.DN, butrv.BindDNis of type*ldap3.DN. This leads to a type mismatch during assignment.Consistently use the same DN type throughout the code by following the resolution provided earlier.
Line range hint
119-125: **Type Mismatch: Assigning ldap.DN to ldap3.DNAt line 119~,
userSearchDNis assigned torv.UserSearchDN, which is of type*ldap3.DN, butuserSearchDNis a*ldap.DN. This results in a type mismatch error.Ensure that
rv.UserSearchDNand the assigned value share the same DN type as per the previous recommendations.
Line range hint
129-135: **Type Mismatch: Assigning ldap.DN to ldap3.DNAt line 129~, you're assigning
groupSearchDN(of type*ldap.DN) torv.GroupSearchDN(of type*ldap3.DN), causing a type mismatch.Maintain consistent DN types between the variables and the struct fields as advised earlier.
Line range hint
139-145: **Type Mismatch: Assigning ldap.DN to ldap3.DNAt line 139~,
roleSearchDN(type*ldap.DN) is assigned torv.RoleSearchDN(type*ldap3.DN), leading to a type mismatch error.Apply the same type consistency fixes as previously suggested.
8-10: Consider Renaming Import Aliases for ClarityAt lines 8~ and 10~, the packages are imported with similar aliases:
"github.com/conductorone/baton-ldap/pkg/ldap" ldap3 "github.com/go-ldap/ldap/v3"Using
ldapandldap3can be confusing due to their similarity. Consider renaming the import aliases to improve code readability and maintainability.For example:
import ( batonldap "github.com/conductorone/baton-ldap/pkg/ldap" oldldap "github.com/go-ldap/ldap/v3" )This makes it clearer which package is being referenced in the code, reducing potential confusion.
pkg/connector/role.go (1)
210-215: Consider logging when attribute does not exist during revokeIn the
Revokemethod, when the LDAP errorLDAPResultNoSuchAttributeoccurs, the function returns without logging. Logging this scenario can aid in understanding why the attribute was not found, especially during debugging.Optionally, you could add a log statement to inform that the attribute did not exist:
if lerr.ResultCode == ldap3.LDAPResultNoSuchAttribute { + l.Info("attribute does not exist; role membership may have already been revoked", zap.String("attribute", attrRoleMember)) return nil, nil }pkg/connector/group.go (3)
234-238: Handle cases where no user is found for a member IDCurrently, when a user is not found for a given
memberId, an error is logged, and the loop continues. Consider whether this is the desired behavior or if additional handling is needed, such as notifying about missing users or cleaning up group memberships.Ensure that the absence of a user entry for a
memberIdis acceptable and doesn't indicate data inconsistency.
Line range hint
293-300: Simplify extraction of username from DNIn the
Grantmethod, when adding a user to aposixGroup, the code extracts the username from the user's DN. If the username attribute is consistently theuid, consider extracting it directly from the user's attributes instead of parsing the DN, which can be error-prone.Modify the code to retrieve the username from the user's
uidattribute: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} + userEntry, err := g.client.LdapGet( + ctx, + ldap.CanonicalizeDN(principal.Id.Resource), + userFilter, + []string{"uid"}, + ) + if err != nil { + return nil, err + } + username := userEntry.GetAttributeValues("uid") modifyRequest.Add(attrGroupMemberPosix, username) } else {
Line range hint
335-342: Simplify username extraction during revocationSimilarly, in the
Revokemethod, when removing a user from aposixGroup, consider retrieving the username from the user's attributes instead of parsing the DN.Update the code to fetch the
uidattribute: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} + userEntry, err := g.client.LdapGet( + ctx, + ldap.CanonicalizeDN(principal.Id.Resource), + userFilter, + []string{"uid"}, + ) + if err != nil { + return nil, err + } + username := userEntry.GetAttributeValues("uid") modifyRequest.Delete(attrGroupMemberPosix, username) } else {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- pkg/config/config.go (6 hunks)
- pkg/connector/connector_test.go (1 hunks)
- pkg/connector/group.go (9 hunks)
- pkg/connector/helpers.go (2 hunks)
- pkg/connector/role.go (4 hunks)
- pkg/connector/user.go (6 hunks)
- pkg/ldap/client.go (1 hunks)
- pkg/ldap/dn.go (1 hunks)
🧰 Additional context used
🪛 GitHub Check: go-lint
pkg/connector/group.go
[failure] 27-27:
var-naming: const groupMemberUidFilter should be groupMemberUIDFilter (revive)
[failure] 169-169:
var-naming: var numericUidRe should be numericUIDRe (revive)pkg/ldap/dn.go
[failure] 9-9:
Comment should end in a period (godot)
🔇 Additional comments (17)
pkg/ldap/dn.go (1)
1-46: Well-structured implementation of LDAP DN canonicalization.This new file introduces a robust implementation for canonicalizing LDAP Distinguished Names. The
CanonicalizeDNfunction effectively standardizes the case of attributes and values, which is crucial for consistent LDAP operations. The use of a pre-defined map for case-insensitive attributes is an efficient approach.The code is well-organized and follows good practices. With the minor suggestions implemented, this file will provide a solid foundation for LDAP-related operations in the project.
🧰 Tools
🪛 GitHub Check: go-lint
[failure] 9-9:
Comment should end in a period (godot)pkg/connector/connector_test.go (2)
86-89: Excellent refactoring of DN parsing!The replacement of
ldap.ParseDNcalls withmustParseDNis a great improvement. This change:
- Centralizes DN parsing logic, aligning with the PR objective.
- Simplifies error handling in the
createConnectorfunction.- Improves code readability and reduces duplication.
- Ensures that any DN parsing errors will cause the test to fail immediately, improving test reliability.
Line range hint
1-97: Summary: Excellent improvements to DN handlingThe changes in this file successfully centralize DN canonicalization, as stated in the PR objectives. The introduction of the
mustParseDNfunction and its usage increateConnectorimprove code readability, maintainability, and error handling. These changes will make the tests more robust and easier to understand.The modifications align well with best practices for test code organization and error handling in Go. Great job on this refactoring!
pkg/connector/helpers.go (1)
9-9: LGTM: New import for mapset package.The addition of the
mapsetimport is appropriate for the changes made to theparseValuesfunction. Using an alias and specifying the version is a good practice.pkg/ldap/client.go (1)
Line range hint
1-138: Overall assessment of the changesThe addition of the
LdapGetmethod to theClientstruct is a valuable improvement to the LDAP client implementation. It provides a convenient way to retrieve a single LDAP entry, which is a common operation in LDAP-based systems.The method is well-integrated with the existing
LdapSearchmethod and follows the error handling patterns established in the rest of the file. With the suggested improvements to error handling and documentation, this change will enhance the usability and maintainability of the LDAP client.The rest of the file remains unchanged, maintaining the existing functionality and structure of the LDAP client.
pkg/connector/user.go (6)
13-13: LGTM: Import statement for mapset added.The addition of the
mapsetimport is appropriate for the changes made in theparseUserLoginfunction. This package provides efficient set operations, which will improve the handling of user aliases.
25-26: LGTM: Constants refactored for better readability and maintainability.The introduction of the
userObjectClassesconstant and its use inuserFilteris a good refactoring. It improves code readability and makes future modifications to the object classes easier to manage.
Line range hint
98-118: LGTM: Improved efficiency inparseUserLoginfunction.The refactoring of
parseUserLoginfunction is well done:
- Using
mapset.Setinstead of a slice improves the efficiency of managing unique aliases.- The new implementation correctly ensures that the login is not included in the returned aliases.
- The changes enhance both the performance and correctness of the function.
These modifications align well with best practices for handling sets of unique values.
222-227: LGTM: Improved DN handling inuserResourcefunction.The changes to the
userResourcefunction enhance the handling of Distinguished Names (DNs):
- The use of
ldap.CanonicalizeDNensures consistent DN formatting.- Logging the canonicalized DN improves traceability and debugging.
- Using the canonicalized DN in resource creation maintains consistency across the system.
- Proper error handling for the canonicalization process is in place.
These modifications improve the robustness and consistency of DN handling in the connector.
Also applies to: 232-232
275-275: LGTM: SimplifiedListmethod inuserResourceType.The modification in the
Listmethod, whereuserEntryis directly passed touserResourcefunction, is a good optimization:
- It simplifies the code by removing unnecessary variable assignment.
- This change may slightly improve performance by avoiding redundant object creation.
This is a good example of clean and efficient coding practices.
Line range hint
1-305: Overall assessment: Excellent improvements to code quality and consistency.The changes in this file align well with the PR objectives of centralizing DN canonicalization and improving logs. Key improvements include:
- Enhanced efficiency in handling user aliases using
mapset.Set.- Consistent canonicalization of Distinguished Names (DNs).
- Improved logging for better traceability and debugging.
- Code simplification and potential performance improvements in the
Listmethod.- Better constant management for user object classes and filters.
These modifications collectively enhance the code's readability, maintainability, and efficiency without introducing any major architectural changes or new features. The changes are well-integrated and consistent throughout the file, demonstrating a thoughtful approach to code improvement.
pkg/connector/role.go (5)
5-5: Approved: Added import for theerrorspackageThe addition of the
errorspackage import is appropriate for improved error handling.
15-16: Approved: Added imports for enhanced loggingThe imports of
ctxzapandzappackages enhance logging capabilities and are correctly implemented.
123-128: Approved: Proper logger extraction and role DN canonicalizationThe extraction of the logger from context and the canonicalization of
roleDNare correctly implemented. Addingrole_dnto the logger's context will aid in debugging.
130-135: Approved: Retrieval of LDAP role objectUsing
r.client.LdapGetto retrieve the LDAP role object with the specified parameters is appropriate and correctly implemented.
142-145: Approved: Parsing role members and iterating over themThe parsing of role members using
parseValuesand the iteration overmembers.Iter()are correctly implemented to retrieve and process each member DN.pkg/connector/group.go (1)
215-218: Ensure proper handling of member ID filtersWhen constructing LDAP filters for member IDs, make sure that the attributes used (
uidandcn) match the directory schema. If the directory uses different attributes for user identification, the filters may not return the correct entries.Consider verifying that the attributes
uidandcnare appropriate for your LDAP schema.Run the following script to check available user attributes:
| func (c *Client) LdapGet(ctx context.Context, | ||
| searchDN *ldap.DN, | ||
| filter string, | ||
| attrNames []string, | ||
| ) (*ldap.Entry, error) { | ||
| entries, _, err := c.LdapSearch(ctx, ldap.ScopeBaseObject, searchDN, filter, attrNames, "", 1) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if len(entries) == 0 { | ||
| return nil, fmt.Errorf("entry not found: %s", searchDN.String()) | ||
| } | ||
| if len(entries) > 1 { | ||
| return nil, fmt.Errorf("multiple entries found: %s", searchDN.String()) | ||
| } | ||
| return entries[0], nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the LdapGet method with comments and improved error handling.
The implementation of LdapGet is solid, but we can make a few improvements:
- Add a comment explaining the purpose of the method.
- Improve error messages to be more specific.
- Consider using custom error types for "not found" and "multiple entries" cases.
Here's a suggested improvement:
+// LdapGet retrieves a single LDAP entry based on the provided distinguished name (DN),
+// filter, and attribute names. It returns an error if no entry or multiple entries are found.
func (c *Client) LdapGet(ctx context.Context,
searchDN *ldap.DN,
filter string,
attrNames []string,
) (*ldap.Entry, error) {
entries, _, err := c.LdapSearch(ctx, ldap.ScopeBaseObject, searchDN, filter, attrNames, "", 1)
if err != nil {
return nil, err
}
if len(entries) == 0 {
- return nil, fmt.Errorf("entry not found: %s", searchDN.String())
+ return nil, fmt.Errorf("ldap entry not found for DN: %s, filter: %s", searchDN.String(), filter)
}
if len(entries) > 1 {
- return nil, fmt.Errorf("multiple entries found: %s", searchDN.String())
+ return nil, fmt.Errorf("multiple ldap entries found for DN: %s, filter: %s", searchDN.String(), filter)
}
return entries[0], nil
}Consider defining custom error types for more granular error handling:
var (
ErrLdapEntryNotFound = errors.New("ldap entry not found")
ErrMultipleLdapEntriesFound = errors.New("multiple ldap entries found")
)Then use these custom errors in the LdapGet method:
if len(entries) == 0 {
return nil, fmt.Errorf("%w: DN: %s, filter: %s", ErrLdapEntryNotFound, searchDN.String(), filter)
}
if len(entries) > 1 {
return nil, fmt.Errorf("%w: DN: %s, filter: %s", ErrMultipleLdapEntriesFound, searchDN.String(), filter)
}This approach allows callers to use errors.Is for more precise error handling.
pquerna
force-pushed
the
pq/improved_dns_and_logs
branch
from
e44260a to
a918af0
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores