-
Notifications
You must be signed in to change notification settings - Fork 356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: return referrals for modify operation #375
Conversation
This implements returning the referral for the modify operation. Tested against a Microsoft Active Directory Read-only Domain Controller.
abf21d9
to
76b0db3
Compare
76b0db3
to
8da03e1
Compare
Thank you for your PR! 👍
I would utilize explicit errors instead of the safeguards (which discard the malformed packet). This way, the user has the option to atleast respond to it and acknlowedge that something went wrong in the background and act accordingly. The impact of this, perfomance-wise, is extremely low and only takes a few nanoseconds anyways. If we simply disregard it and return an empty string instead, the user doesn't have any way to know about the malformed response and therefore expects the ModifyResult to be valid.
Personally, I prefer to keep backwards compatibility and just utilize the new What do you think, @johnweldon & @vetinari ? 😊 |
Makes sense to me. Something like this which we check and immediately return if not nil so users can unwrap the original error if they want? func getReferral(err error, packet *ber.Packet) (string, error) {
if !IsErrorWithCode(err, LDAPResultReferral) {
return "", nil
}
if len(packet.Children) < 2 {
return "", fmt.Errorf("ldap: returned error indicates the packet contains a referral but it doesn't have sufficient child nodes: %w", err)
}
var (
ok bool
referral string
)
for _, child := range packet.Children[1].Children {
if child.Tag == ber.TagBitString && len(child.Children) >= 1 {
if referral, ok = child.Children[0].Value.(string); ok {
return referral, nil
}
}
}
return "", fmt.Errorf("ldap: returned error indicates the packet contains a referral but the referral couldn't be decoded: %w", err)
} |
I assume there's nothing wrong opting for the option with explicit error handling. @james-d-elliott Do you mind implementing this in your branch so we can get this merged? |
Yep SGTM! Just didn't know there was a reply sorry. |
I'm sorry for my delay in responding, alot going on currently. I'll have a look at your changes tomorrow! |
Yep no worries, same for me so I understand. Let me know if you want any changes, only thing I can think of is potentially making the method public since it could theoretically be used for other situations (apparently with openldap you can ensure people bind on specific servers for example). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I'll give the other maintainers a chance to review it as well. If no one objects I'll continue to merge this. Thank you for your efforts! 👍
No worries at all. No rush on our end, let me know if it needs a squash. We implemented this as a func downstream so we can operate as per normal without this being a blocker. My intent was just to share it so others can enjoy it. Oh I almost forgot, the error handling should be carefully thought about. Since it's returning a fmt error wrapping the ldap error it's technically a change to what the functions are returning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great
* feat: return referrals for modify operation This implements returning the referral for the modify operation. Tested against a Microsoft Active Directory Read-only Domain Controller.
This reverts commit a3dcdda.
* feat: return referrals for modify operation This implements returning the referral for the modify operation. Tested against a Microsoft Active Directory Read-only Domain Controller.
This implements returning the referral for the modify operation. Tested against a Microsoft Active Directory Read-only Domain Controller.
This also adds anti-panic guards in edge cases where the packet is malformed. This could be refactored to return an error or be removed entirely depending on what is considered to be desired/appropriate by maintainers. I also utilized the ASN.1 BER consts to check tag types in relevant code sections as I think this is more readable, and factorized the code into a single method rather than duplicating it.
Decisions: