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

LDAPInterface.search methods and SearchRequest constructors should accept a com.unboundid.ldap.sdk.DN as the baseDN #128

Open
markslater opened this issue May 16, 2022 · 2 comments

Comments

@markslater
Copy link

The base DN for search has to be a valid DN, which is something that the SDK models with the DN class, but the search methods on LDAPInterface only accept a String for the baseDN argument.

I notice that SearchRequest does have an overloaded setBaseDN method that takes an argument of type DN, though this is strictly for mutating SearchRequests: you cannot construct a SearchRequest without provinding a String base DN.

It would be great for type safety, readability, and intuitiveness if methods that take a base DN argument were overloaded to accept a base DN of type DN (IMHO it would be even better if base DN arguments could only be of type DN, but I appreciate that would be a significant breaking change).

@dirmgr
Copy link
Collaborator

dirmgr commented May 16, 2022

I've just committed a change that updates SearchRequest to provide variants of the constructor that take DN objects as an alternative to Strings. I hadn't added those in the first place because (a) using the string representation is more convenient in some cases, (b) it's easy enough to get the string representation of a DN object by calling its toString() method, and (c) I was trying to avoid having a proliferation of constructors. However, I do think that it makes sense to have an option to create a SearchRequest with a base DN as a DN object, so I've gone ahead and added a few new constructors that allow that.

I'm more hesitant to update LDAPInterface because even though it's marked with the @NotExtensible annotation, there are implementations outside the LDAP SDK (including in the Ping Identity Directory Server), and adding a new signatures to that interface would be a little more involved without breaking the existing usages. However, there are methods that allow you to issue a search from a SearchRequest, so you can use the new constructors in that class, or you can use the DN.toString method to get the string representation.

We have a very strong no-backward-compatibility guarantee in the LDAP SDK, so we won't make changes to break existing APIs, or existing usages that allow you to provide strings that don't represent valid DNs. When the search request is sent to the server, the base DN is sent as a string (as opposed to the filter, which is sent as an encoded ASN.1 element). Even though the protocol requires the base DN to be a valid DN, it's possible that some servers may use invalid DN values in some cases. I'm not aware of any cases in which that happens for search requests, but I know that Active Directory chose to violate the specification for simple bind requests by allowing you to provide non-DN values as the bind DN, so it's possible that they allow non-DN usages in other places, too. I'm definitely not a fan of that "embrace and extend" approach (especially when they could have just used the existing SASL PLAIN mechanism to achieve the same result in a completely standards compliant manner), but I don't want to inadvertently and unnecessarily break an application that works even if it uses non-standards-compliant behavior.

@markslater
Copy link
Author

Wow, thanks for the fast turnaround!

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

No branches or pull requests

2 participants