Add socket api #1
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
createConnection is a parameter that lets the user supply their own method of creating a socket to the LDAP server, as an alternative for the existing logic, which uses net.connect or tls.connect depending on whether the URL is ldaps or ldap. The primary use case I have in mind is connecting to an LDAP server through an explicit proxy which uses the HTTP CONNECT method. In this case, the user can supply a createConnection function handles making the HTTP CONNECT request and (for ldaps) doing a TLS handshake over the resulting connection.
taylor-sutton
force-pushed
the
add-socket-api
branch
from
e7f00fc
to
1f4ded3
Compare
The type declarations are reproduced (under MIT license) from DefinitelyTyped, which underpins the @types/ldapjs package, except with a one line modification, the addition of the createConnection parameter to ClientOptions, which is specific to this fork.
johnhuangclever
approved these changes
Oct 14, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds a new way to construct LDAP clients which allows the user (meaning the user of this lib, the person creating the LDAP client) to use custom logic for constructing a network connection to the LDAP server. Previously, this connection was always created by
ldapjscalling eithernet.connectortls.connect(from Node stdlib) to the provided URL. Most of the time, this is sufficient, and it remains the default. In rare cases (described in a sec), we may want to specify some custom way of creating the socket to the server. This doesn't change any of the LDAP logic - all that occurs at a higher level of abstraction on the network hierarchy (layer 7), so we can just make a small change here to change where the connection comes from.We plan to use this feature to connect to an LDAP server over an explicit proxy i.e. the client will first establish a connection to the proxy, then use that connection for LDAP. Clever-internal details about this setup are at https://github.com/Clever/ldap-user-service/pull/65.
About the actual code changes. There was already an
if-elsefor calling the appropriateconnectmethod, which seemed like a good place to add a third branch to use the user-defined logic. However, it required a little refactor - moving some logic into theonSocketDefinedfunction - because there was no longer a line of code in the client where we could be sure thesocketvariable was set to a real vaue.In addition, in order to modify the TypeScript declarations, I pulled in the existing type declarations from https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/ldapjs and made the one-line modification (adding the parameter to
ClientOptionsat https://github.com/Clever/node-ldapjs/pull/1/files#diff-cfc7807f6b174e1c3705c5c2747f56e49a889a52999dc96e4080a3af0131a534R60 .Discussed merging into upstream in ldapjs#761 - seems like not gonna happen any time soon.