Skip to content

ext/ldap: Allow default host from ldap.conf to work. #2536

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

Merged
merged 2 commits into from
May 30, 2017

Conversation

caldwell
Copy link
Contributor

This fixes an regression introduced in e7af0fe. Previously, calling ldap_connect() with no parameters would pass NULL to ldap_init(), which causes it to use the default host specified in /etc/ldap/ldap.conf (on Ubuntu).

When the code changed to use ldap_initialize(), it initialized a uri, even if there were no parameters passed to ldap_connect(). Because of this, there's no way to pass a NULL into ldap_initialize(), making it impossible to use the default uri from ldap.conf.

This commit bypasses the uri creation when there is no host argument, passing on a NULL to ldap_initialize() which restores the old PHP 5.5 behavior.

This fixes an regression introduced in
e7af0fe. Previously, calling
ldap_connect() with no parameters would pass NULL to ldap_init(),
which causes it to use the default host specified in
/etc/ldap/ldap.conf (on Ubuntu).

When the code changed to use ldap_initialize(), it initialized a uri,
even if there were no parameters passed to ldap_connect(). Because of
this, there's no way to pass a NULL into ldap_initialize(), making it
impossible to use the default uri from ldap.conf.

This commit bypasses the uri creation when there is no host argument,
passing on a NULL to ldap_initialize() which restores the old PHP 5.5
behavior.
@andrewnester
Copy link
Contributor

@caldwell I guess it would great to add test to check this case

@caldwell
Copy link
Contributor Author

@andrewnester, I attempted a test and it appears to work. It's a little hairy since it needs to write an ldap.conf file and set an env var to test for it, so I'm worried about cross-platform stuff (is OpenLDAP a thing on Windows?) I'm also not super happy about the way I'm detecting whether PHP is using OpenLDAP (phpinfo() + a regexp). If there's a better way, I open to suggestions.

@andrewnester
Copy link
Contributor

@caldwell do we need setting conf file? what if we just skip this step?

as to checking for OpenLDAP - I am not sure that you need to check and skip if it's not because it looks like internal implementation of LDAP and we could ignore it just making sure that for end user it works as expected in any of configuration

@caldwell
Copy link
Contributor Author

The idea was to test that it would pick up defaults from the conf file when there are no arguments to ldap_connect(). I don't know how to test that without setting up a conf file. Without being limited to OpenLDAP and without the conf file, ldap_connect() would not choose a reliable default, and so the test wouldn't really be able to verify anything.

Maybe I'm misunderstanding what you want to test?

@andrewnester
Copy link
Contributor

@caldwell yes, i tried to dive deeper in this the code and it seems reasonable. Thanks!

@php-pulls php-pulls merged commit 49d1cdc into php:PHP-7.0 May 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants