search

pingidentity / ldapsdk

UnboundID LDAP SDK for Java
Other
334 stars 81 forks source link

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

Open markslater opened 2 years ago

markslater commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Wow, thanks for the fast turnaround!