feat: return referrals for modify operation

[james-d-elliott]

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:

• We could export a method which takes the error and returns the referral if applicable. Would be useful in any instance where there may be referrals which have not been accounted for. This may also be the appropriate way to handle all instances, but I think that's a breaking change and consistency is also important for a cohesive API.
• Do we want to outright return errors in edge cases where the decoded BER packet has an unexpected structure, or do we want to just utilize the safeguards? Or do we want to remove the safeguards entirely since the chance of this happening is probably negligible? Also noteworthy is it's not specifically an LDAP error so that may stray from the API.

[cpuschma]

Thank you for your PR! 👍

Do we want to outright return errors in edge cases where the decoded BER packet has an unexpected structure, or do we want to just utilize the safeguards? Or do we want to remove the safeguards entirely since the chance of this happening is probably negligible? Also noteworthy is it's not specifically an LDAP error so that may stray from the API.

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.

We could export a method which takes the error and returns the referral if applicable. Would be useful in any instance where there may be referrals which have not been accounted for. This may also be the appropriate way to handle all instances, but I think that's a breaking change and consistency is also important for a cohesive API.

Personally, I prefer to keep backwards compatibility and just utilize the new getReferral function in the background to return the result as (additional) struct field. This way, users don't have to change their code.

What do you think, @johnweldon & @vetinari ? 😊

[james-d-elliott]

I would utilize explicit errors instead of the safeguards (which discard the malformed packet).

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)
}

[cpuschma]

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?

[james-d-elliott]

Yep SGTM! Just didn't know there was a reply sorry.

[cpuschma]

I'm sorry for my delay in responding, alot going on currently. I'll have a look at your changes tomorrow!

[james-d-elliott]

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).

[cpuschma]

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! 👍

[james-d-elliott]

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.

[johnweldon]

Looks great