Skip to content

Concat hostName and port for ldap_connect - #1386

Closed
arkraft wants to merge 1 commit into
BookStackApp:masterfrom
arkraft:fix/ldap_over_tls
Closed

arkraft wants to merge 1 commit into
BookStackApp:masterfrom
arkraft:fix/ldap_over_tls

Conversation

@arkraft

@arkraft arkraft commented Apr 11, 2019

Copy link
Copy Markdown

Instead of using two parameters for ldap_connect (hostName and port), this PR combines them to a single parameter. The docs of ldap_connect describe the host parameter as either a hostName or a LDAP URI. In case the latter is used, the second parameter (port) is ignored. When using a protocol, the parameters are combined, otherwise it stays the same.

It works for me with ldaps now, did not test it without the protocol.

resolves #1220

Instead of using two parameters for ldap_connect (hostName and port), this PR combines them to a single parameter. The docs of [ldap_connect](https://www.php.net/manual/en/function.ldap-connect.php) describe the host parameter as either a hostName or a LDAP URI. In case the latter is used, the second parameter (port) is ignored. When using a protocol, the parameters are combined, otherwise it stays the same.
@ssddanbrown ssddanbrown added this to the v0.26.0 milestone Apr 16, 2019
ssddanbrown added a commit that referenced this pull request Apr 16, 2019
- Aligns with PHP behaviour where ports is ignore for full LDAP URI.
- Added tests to check format being passed to LDAP is as expected.
- May be related to #1220
- Related to #1386 and #1278
@ssddanbrown

ssddanbrown commented Apr 16, 2019

Copy link
Copy Markdown
Member

Thanks @arkraft for offering this pull request.

We did already have a similar pull request open in #1278.
I've just commited c247640 to update & simplify the logic. Therefore will close this pull request.

This update will be in the next release (v0.26.0)

@arkraft
arkraft deleted the fix/ldap_over_tls branch May 4, 2019 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

LDAP over TLS: Unable to bind to server: Can't contact LDAP server

2 participants