LDAP Utility – »Has pagination« and »Allow to use pagination« is not the same

[pixelbrackets]

The LdapUtility checks if a pagination feature exists and tries to use it later on.

Expected behaviour: Paginate the results.

Actual behaviour: Throws a warning, that pagination is not possible.

Reason

The »LdapUtility« sets the flag hasPagination to true if the method exists and the server reponds to a given request:

https://github.com/xperseguers/t3ext-ig_ldap_sso_auth/blob/master/Classes/Utility/LdapUtility.php#L311-L317

Line 315 is critial here: It does not expect the pagination to be allowed (since it is not used in this method):

https://github.com/xperseguers/t3ext-ig_ldap_sso_auth/blob/master/Classes/Utility/LdapUtility.php#L315

This flag is then used again if a entry is fetched:

https://github.com/xperseguers/t3ext-ig_ldap_sso_auth/blob/master/Classes/Utility/LdapUtility.php#L381-L383

If the user is not allowed to paginate for whatever reason, then this line will throw a PHP warning:

ldap_control_paged_result_response(): No server controls in result

Solution idea

The check in line 315 should require the pagination. If the result is empty, then the flag should be false and another search without a pagination should be started.

[xperseguers]

You do not provide a PR so I tried to do what you suggested. I come up with that patch:

diff --git a/Classes/Utility/LdapUtility.php b/Classes/Utility/LdapUtility.php
index d3e8ac9..5e22d43 100644
--- a/Classes/Utility/LdapUtility.php
+++ b/Classes/Utility/LdapUtility.php
@@ -325,7 +325,7 @@ class LdapUtility
                 @ldap_control_paged_result(
                     $this->connection,
                     static::PAGE_SIZE,
-                    false,  // Pagination is not critical for search to work anyway
+                    true,  // Test for pagination is critical because method could be available while not allowed for the user
                     $this->paginationCookie
                 );
             if (!($this->searchResult = @ldap_search($this->connection, $baseDn, $filter, $attributes, $attributesOnly, $sizeLimit, $timeLimit, $dereferenceAliases))) {

is it what you mean?

I'm not sure about the result though because the phpDoc for ldap_control_paged_result says:

• iscritical: Indicates whether the pagination is critical or not. If true and if the server doesn't support pagination, the search will return no result.

This sounds like if you mark it as "critical" and it is not supported, then no search results are given back. And I suspect in your case where the method is available but disabled, you won't find anything anymore.

[janit42]

@xperseguers, I'm not sure if I understand correctly what the setting of "hasPagination" should represent in the code:

My first impression was that it should tell whether the LDAP server used supports "pagination" (more precisely: pagedResultsControl aka 1.2.840.113556.1.4.319) - is that correct? Or is it rather intended as a check whether we're currently in a "paginated" search?

If it's meant to check for general pagination support, the detection doesn't work as intended like @pixelbrackets has pointed out. We see a similar warning (typo3 8.7, extension 3.3.1, php 7.0):

TYPO3\CMS\Core\Error\ErrorHandler::handleError(2, "ldap_control_paged_result_response(): No server controls in result", "/path/to/typo3conf/ext/ig_ldap_sso_auth/Classes/Utility/LdapUtility.php", 382, array)

If we comment out Line 382 we don't get any errors from the LDAP server without pagedResults support and the extension just seems to work, but that feels dirty and dangerous as we don't understand the code good enough to be sure not to break something.

The proposed fix (set pagination to critical in ldap_control_paged_result), while it seems to make sense on first glance, fixed the warning, but then we don't get any results at all (an therefore no FE or BE login possible with an LDAP user).

ldap_control_paged_result($this->connection,static::PAGE_SIZE,true,$this->paginationCookie)

still returnes true - even with pagination marked as critical while questioning an LDAP server that doesn't support pagedResults at all.

A solution might be to explicitly ask, whether the LDAP server has pagedResult Support and don't use pagination if not. Unfortunately I'm not a PHP guy, but I've found some implementations which might be on track, e.g. function ldap_paged_results_supported($ldapversion, $ldapconnection = null) of the moodle learning platform code and some examples on php.net: example 1 example 5.

I've tested a check based on example 1 linked above against an Active Directory (with pagedResults support) and an Oracle Directory Server EE (no pagedResults support) and it detected pagination Support (or the lack of it) in the respective LDAP servers correctly. (tests run with php 7.0):

$ldapconn = ldap_connect($ldapuri)
$ldapbind = ldap_bind($ldapconn, $ldaprdn, $ldappass);

//ask for supported controls after successful ldapconnect and ldapbind
$result = ldap_read($ldapconn, '', '(objectClass=*)', ['supportedControl']);

// from PHP 7.3 on LDAP_CONTROL_PAGEDRESULTS is predefined, see https://www.php.net/manual/en/ldap.constants.php
// in earlier version we need to define it
define('LDAP_CONTROL_PAGEDRESULTS','1.2.840.113556.1.4.319');

// Taken from https://www.php.net/manual/en/ldap.controls.php
if (!in_array(LDAP_CONTROL_PAGEDRESULTS, ldap_get_entries($ldapconn, $result)[0]['supportedcontrol'])) {
  die("This server does not support paged result control\n");
} else {
  echo "This server does support paged result control\n";
  }
}

Unfortunately I'm not sure where to put a check for general pagedResults support in the extension code, but maybe something could be worked out nevertheless ... :-). Please let me know, if I can provide more information or test something!

[janit42]

I've created a patch against the 3.3.1 version of the extension which we're currently running.

Not sure whether the position of the check for supported controls has a better fit somewhere else in the code, but so far it works fine for us.

--- LdapUtility.php        2020-01-24 09:57:19.156092740 +0100
+++ LdapUtility.php.patched     2020-01-24 10:01:04.173815540 +0100
@@ -300,21 +300,27 @@
             return false;
         }

-        if ($this->connection) {
-            if (!$continueLastSearch) {
-                // Reset the pagination cookie
-                $this->paginationCookie = null;
+       if ($this->connection) {
+           //ask for supported controls
+           $supportedControls = ldap_read($this->connection, '', '(objectClass=*)', ['supportedControl']);
+           // from PHP 7.3 on LDAP_CONTROL_PAGEDRESULTS is predefined
+           defined('LDAP_CONTROL_PAGEDRESULTS') or define('LDAP_CONTROL_PAGEDRESULTS','1.2.840.113556.1.4.319');
+           if (in_array(LDAP_CONTROL_PAGEDRESULTS, ldap_get_entries($this->connection, $supportedControls)[0]['supportedcontrol'])) {
+                // supports pagedResults
+                if (!$continueLastSearch) {
+                    // Reset the pagination cookie
+                    $this->paginationCookie = null;
+                }
+                // It was reported that ldap_control_paged_result() may not be available;
+                // we thus check for existence before proceeding
+                $this->hasPagination = function_exists('ldap_control_paged_result') &&
+                    @ldap_control_paged_result(
+                        $this->connection,
+                        static::PAGE_SIZE,
+                        false,  // Pagination is not critical for search to work anyway
+                        $this->paginationCookie
+               );
             }
-
-            // It was reported that ldap_control_paged_result() may not be available;
-            // we thus check for existence before proceeding
-            $this->hasPagination = function_exists('ldap_control_paged_result') &&
-                @ldap_control_paged_result(
-                    $this->connection,
-                    static::PAGE_SIZE,
-                    false,  // Pagination is not critical for search to work anyway
-                    $this->paginationCookie
-                );
             if (!($this->searchResult = @ldap_search($this->connection, $baseDn, $filter, $attributes, $attributesOnly, $sizeLimit, $timeLimit, $dereferenceAliases))) {
                 // Search failed.
                 $this->status['search']['status'] = ldap_error($this->connection);

[vendytjung]

@janit42 i have also the same issue with version 3.3.1. Your Patch help me a lot, but for me it should be not in array. if (!in_array(LDAP_CONTROL_PAGEDRESULTS, ldap_get_entries($this->connection, $supportedControls)[0]['supportedcontrol'])) {

Here is my patch:

diff --git a/Classes/Utility/LdapUtility.php b/Classes/Utility/LdapUtility.php
--- a/Classes/Utility/LdapUtility.php
+++ b/Classes/Utility/LdapUtility.php
@@ -301,20 +301,27 @@ class LdapUtility
         }

         if ($this->connection) {
-            if (!$continueLastSearch) {
-                // Reset the pagination cookie
-                $this->paginationCookie = null;
+            $supportedControls = ldap_read($this->connection, '', '(objectClass=*)', ['supportedControl']);
+            // from PHP 7.3 on LDAP_CONTROL_PAGEDRESULTS is predefined
+            defined('LDAP_CONTROL_PAGEDRESULTS') or define('LDAP_CONTROL_PAGEDRESULTS','1.2.840.113556.1.4.319');
+
+            if (!in_array(LDAP_CONTROL_PAGEDRESULTS, ldap_get_entries($this->connection, $supportedControls)[0]['supportedcontrol'])) {
+                // supports pagedResults
+                if (!$continueLastSearch) {
+                    // Reset the pagination cookie
+                    $this->paginationCookie = null;
+                }
+                // It was reported that ldap_control_paged_result() may not be available;
+                // we thus check for existence before proceeding
+                $this->hasPagination = function_exists('ldap_control_paged_result') &&
+                    @ldap_control_paged_result(
+                        $this->connection,
+                        static::PAGE_SIZE,
+                        false,  // Pagination is not critical for search to work anyway
+                        $this->paginationCookie
+                    );
             }

-            // It was reported that ldap_control_paged_result() may not be available;
-            // we thus check for existence before proceeding
-            $this->hasPagination = function_exists('ldap_control_paged_result') &&
-                @ldap_control_paged_result(
-                    $this->connection,
-                    static::PAGE_SIZE,
-                    false,  // Pagination is not critical for search to work anyway
-                    $this->paginationCookie
-                );
             if (!($this->searchResult = @ldap_search($this->connection, $baseDn, $filter, $attributes, $attributesOnly, $sizeLimit, $timeLimit, $dereferenceAliases))) {
                 // Search failed.
                 $this->status['search']['status'] = ldap_error($this->connection);

[xperseguers]

Something was changed for PHP 8 compatibility in that regard, please check again.

[janit42]

my webmaster tells me the issue is fixed - thank you! :-)