Skip to content
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

Merged
merged 6 commits into from
Jun 5, 2022

Conversation

james-d-elliott
Copy link
Contributor

@james-d-elliott james-d-elliott commented Apr 30, 2022

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.

This implements returning the referral for the modify operation. Tested against a Microsoft Active Directory Read-only Domain Controller.
@cpuschma
Copy link
Member

cpuschma commented Apr 30, 2022

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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

@cpuschma
Copy link
Member

cpuschma commented Jun 1, 2022

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

@james-d-elliott
Copy link
Contributor Author

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

Copy link
Member

@cpuschma cpuschma left a 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! 👍

@james-d-elliott
Copy link
Contributor Author

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.

Copy link
Member

@johnweldon johnweldon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great

passwdmodify.go Outdated Show resolved Hide resolved
v3/modify.go Outdated Show resolved Hide resolved
@cpuschma cpuschma merged commit a3dcdda into go-ldap:master Jun 5, 2022
@james-d-elliott james-d-elliott deleted the feat-modify-referral branch June 5, 2022 22:44
m-vinc pushed a commit to m-vinc/ldap that referenced this pull request Jun 15, 2022
* 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.
james-d-elliott added a commit to james-d-elliott/ldap that referenced this pull request Jan 10, 2023
inv2004 pushed a commit to inv2004/ldap that referenced this pull request Jan 17, 2023
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants