Skip to content

Added controls support to php-ldap #2640

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 31 commits into from
Sep 21, 2017
Merged

Conversation

MCMic
Copy link
Contributor

@MCMic MCMic commented Jul 20, 2017

Adapted from https://bugs.php.net/patch-display.php?bug_id=61921&patch=ldap_parse_result.patch&revision=latest
This also changes the way controls are set/get as BER is decoded/encoded
to an array.
ldap_set_option will accept both array or ber-encoded as a string so no BC there.
ldap_get_option return for controls changes a bit but the support for controls in ldap_get_option was only added in PHP 7.2.

The test uses pagination controls, if anyone has other suggestion of controls which can be found in ldap result objects I’d be happy to add them to the test.

@krakjoe
Copy link
Member

krakjoe commented Jul 25, 2017

It's not totally clear what you want to target here, master is now 7.3, and it looks like this should target 7.2 ... but then this is not a good time for 7.2 because feature freeze ... so I defer to @remicollet and @sgolemon

@MCMic
Copy link
Contributor Author

MCMic commented Jul 25, 2017

No this was for 7.3 I know that 7.2 is frozen.
And since I created this PR I’ve found that it does not work in many cases the conversion to array code cannot handle BER context tagged value which are in a lot of controls.

I’m working on it to have a consistent and complete control support to push in 7.3 (or later if I’m too slow)

@MCMic MCMic force-pushed the ldap_parse_controls branch from 6746194 to 825fb9c Compare July 26, 2017 07:43
@MCMic
Copy link
Contributor Author

MCMic commented Jul 26, 2017

Ok, so I reworked the code and it now works like this:
Controls are converted to/from arrays like before, but the value can be passed as an array as well if the control is a known one.
The known controls are ppolicy response and paged results for now.
You can look at the tests for examples.

They can be passed to ldap_set_option, they can be get by ldap_get_option, ldap_parse_result and ldap_exop_passwd.

For ldap_exop_passwd this allows to do a password change on a ppolicy enabled ldap and get the error from the control if any.

Once this is merged I intend to add support for all known controls as arrays, and support for passing them to most of ldap operations (search and such).
We should also find a way to pass and get controls to and from bind operations which does not return the result object in the current implementation.

@MCMic MCMic force-pushed the ldap_parse_controls branch from 825fb9c to 98fd782 Compare July 26, 2017 15:02
@MCMic MCMic force-pushed the ldap_parse_controls branch 3 times, most recently from 40d23c0 to e50ac56 Compare September 7, 2017 14:48
@MCMic
Copy link
Contributor Author

MCMic commented Sep 14, 2017

Control support is pretty complete and functional now so for me this is good to merge.
Can I merge it as-is or should I squash it as one commit? Seems to big to be squashed.

Côme Chilliet added 19 commits September 21, 2017 10:05
Known controls are parsed to and from associative arrays.
Only ppolicy and paged results are implemented for now.
Also added workaround for a bug in ldap_create_assertion_control_value
Made sure failed control creation aborts the operation
And added test for assertion control on ldap_modify
Note: for functions like ldap_compare, ldap_delete, ldap_modify,
 a way to get the result object back will need to be added
 so that controls returned by the server may be analyzed.
Client controls are ldap client lib specific and all the one
 I could find are ignoring client controls anyway.
ldap_bind_ext allows to pass controls and get result object
 from bind operation
Also added a test for it, pretty basic as tests cannot depend upon
 ppolicy overlay
@MCMic MCMic changed the title Added controls support to ldap_parse_result Added controls support to php-ldap Sep 21, 2017
@MCMic MCMic force-pushed the ldap_parse_controls branch from 73cbf7a to 9a4f350 Compare September 21, 2017 08:17
@php-pulls php-pulls merged commit 9a4f350 into php:master Sep 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants