MDL-57558 ldap: fix ldap_get_entries_moodle()
authorIñaki Arenaza <iarenaza@mondragon.edu>
Fri, 30 Dec 2016 23:01:52 +0000 (00:01 +0100)
committerDan Poltawski <dan@moodle.com>
Tue, 16 May 2017 09:48:27 +0000 (10:48 +0100)
While ldap_get_entries_moodle() PHPdocs state that it returns "array
ldap-entries with lower-cased attributes as indexes.", this is not true. It
uses ldap_get_attributes() internally, which returns both numerically indexed
attribute names, and dictionary-like entries indexed by attribute names.

Current code lowercases the dictionary-like entries, but then uses the
numerically indexed entries for the attribute names used as keys in the
returned array. The numerically indexed names might or might not be lowercased,
depending on the LDAP server and PHP version) version. E.g., OpenLDAP 2.x,
Novell eDirectory 8.x and MS Active Directory return mixed-cased attribute
names, and PHP 5.x and PHP 7.x don't lowercase them inside ldap_get_entries().

This is probably why all calls to ldap_get_entries_moodle() are followed by
calls to array_change_key_case(), even if that shouldn't be necessary.

So make sure we always return lower-cased attributs as indexes and add some
unit tests to avoid regressions in the future.

auth/ldap/auth.php
lib/ldaplib.php
lib/tests/ldaplib_test.php
lib/upgrade.txt

index 7e6fd3b..7f59282 100644 (file)
@@ -276,7 +276,7 @@ class auth_plugin_ldap extends auth_plugin_base {
             }
             $ldapval = NULL;
             foreach ($values as $value) {
-                $entry = array_change_key_case($user_entry[0], CASE_LOWER);
+                $entry = $user_entry[0];
                 if (($value == 'dn') || ($value == 'distinguishedname')) {
                     $result[$key] = $user_dn;
                     continue;
@@ -634,7 +634,7 @@ class auth_plugin_ldap extends auth_plugin_base {
         if ($sr)  {
             $info = ldap_get_entries_moodle($ldapconnection, $sr);
             if (!empty ($info)) {
-                $info = array_change_key_case($info[0], CASE_LOWER);
+                $info = $info[0];
                 if (isset($info[$this->config->expireattr][0])) {
                     $expiretime = $this->ldap_expirationtime2unix($info[$this->config->expireattr][0], $ldapconnection, $user_dn);
                     if ($expiretime != 0) {
@@ -1201,7 +1201,7 @@ class auth_plugin_ldap extends auth_plugin_base {
                 return false;
             }
 
-            $user_entry = array_change_key_case($user_entry[0], CASE_LOWER);
+            $user_entry = $user_entry[0];
 
             foreach ($attrmap as $key => $ldapkeys) {
                 $profilefield = '';
@@ -1369,7 +1369,7 @@ class auth_plugin_ldap extends auth_plugin_base {
                 $sr = ldap_read($ldapconnection, $user_dn, '(objectClass=*)', $search_attribs);
                 if ($sr) {
                     $entry = ldap_get_entries_moodle($ldapconnection, $sr);
-                    $info = array_change_key_case($entry[0], CASE_LOWER);
+                    $info = $entry[0];
                     $newattrs = array();
                     if (!empty($info[$this->config->expireattr][0])) {
                         // Set expiration time only if passwordExpirationInterval is defined
@@ -1844,7 +1844,7 @@ class auth_plugin_ldap extends auth_plugin_base {
         }
 
         $entry = ldap_get_entries_moodle($ldapconn, $sr);
-        $info = array_change_key_case($entry[0], CASE_LOWER);
+        $info = $entry[0];
         $useraccountcontrol = $info['useraccountcontrol'][0];
         if ($useraccountcontrol & UF_DONT_EXPIRE_PASSWD) {
             // Password doesn't expire.
@@ -1889,17 +1889,17 @@ class auth_plugin_ldap extends auth_plugin_base {
         }
 
         $entry = ldap_get_entries_moodle($ldapconn, $sr);
-        $info = array_change_key_case($entry[0], CASE_LOWER);
+        $info = $entry[0];
         $domaindn = $info['defaultnamingcontext'][0];
 
         $sr = ldap_read ($ldapconn, $domaindn, '(objectClass=*)',
                          array('maxPwdAge'));
         $entry = ldap_get_entries_moodle($ldapconn, $sr);
-        $info = array_change_key_case($entry[0], CASE_LOWER);
+        $info = $entry[0];
         $maxpwdage = $info['maxpwdage'][0];
         if ($sr = ldap_read($ldapconn, $user_dn, '(objectClass=*)', array('msDS-ResultantPSO'))) {
             if ($entry = ldap_get_entries_moodle($ldapconn, $sr)) {
-                $info = array_change_key_case($entry[0], CASE_LOWER);
+                $info = $entry[0];
                 $userpso = $info['msds-resultantpso'][0];
 
                 // If a PSO exists, FGPP is being utilized.
@@ -1907,7 +1907,7 @@ class auth_plugin_ldap extends auth_plugin_base {
                 if (!empty($userpso)) {
                     $sr = ldap_read($ldapconn, $userpso, '(objectClass=*)', array('msDS-MaximumPasswordAge'));
                     if ($entry = ldap_get_entries_moodle($ldapconn, $sr)) {
-                        $info = array_change_key_case($entry[0], CASE_LOWER);
+                        $info = $entry[0];
                         // Default value of msds-maximumpasswordage is 42 and is always set.
                         $maxpwdage = $info['msds-maximumpasswordage'][0];
                     }
index 8a5c965..78923c7 100644 (file)
@@ -339,13 +339,18 @@ function ldap_get_entries_moodle($ldapconnection, $searchresult) {
         return array();
     }
     do {
-        $attributes = array_change_key_case(ldap_get_attributes($ldapconnection, $entry), CASE_LOWER);
-        for ($j = 0; $j < $attributes['count']; $j++) {
-            $values = ldap_get_values_len($ldapconnection, $entry, $attributes[$j]);
+        $attributes = array();
+        $attribute = ldap_first_attribute($ldapconnection, $entry);
+        while ($attribute !== false) {
+            $attributes[] = strtolower($attribute); // Attribute names don't usually contain non-ASCII characters.
+            $attribute = ldap_next_attribute($ldapconnection, $entry);
+        }
+        foreach ($attributes as $attribute) {
+            $values = ldap_get_values_len($ldapconnection, $entry, $attribute);
             if (is_array($values)) {
-                $result[$i][$attributes[$j]] = $values;
+                $result[$i][$attribute] = $values;
             } else {
-                $result[$i][$attributes[$j]] = array($values);
+                $result[$i][$attribute] = array($values);
             }
         }
         $i++;
@@ -488,7 +493,7 @@ function ldap_paged_results_supported($ldapversion, $ldapconnection = null) {
     if (empty($entries)) {
         return false;
     }
-    $info = array_change_key_case($entries[0], CASE_LOWER);
+    $info = $entries[0];
     if (isset($info['supportedcontrol']) && in_array(LDAP_PAGED_RESULTS_CONTROL, $info['supportedcontrol'])) {
         return true;
     }
index 909c3f7..57909b9 100644 (file)
@@ -207,4 +207,268 @@ class core_ldaplib_testcase extends advanced_testcase {
             ),
         );
     }
+
+    /**
+     * Tests for ldap_get_entries_moodle.
+     *
+     * NOTE: in order to execute this test you need to set up OpenLDAP server with core,
+     *       cosine, nis and internet schemas and add configuration constants to
+     *       config.php or phpunit.xml configuration file.  The bind users *needs*
+     *       permissions to create objects in the LDAP server, under the bind domain.
+     *
+     * define('TEST_LDAPLIB_HOST_URL', 'ldap://127.0.0.1');
+     * define('TEST_LDAPLIB_BIND_DN', 'cn=someuser,dc=example,dc=local');
+     * define('TEST_LDAPLIB_BIND_PW', 'somepassword');
+     * define('TEST_LDAPLIB_DOMAIN',  'dc=example,dc=local');
+     *
+     */
+    public function test_ldap_get_entries_moodle() {
+        $this->resetAfterTest();
+
+        if (!defined('TEST_LDAPLIB_HOST_URL') or !defined('TEST_LDAPLIB_BIND_DN') or
+                !defined('TEST_LDAPLIB_BIND_PW') or !defined('TEST_LDAPLIB_DOMAIN')) {
+            $this->markTestSkipped('External LDAP test server not configured.');
+        }
+
+        // Make sure we can connect the server.
+        $debuginfo = '';
+        if (!$connection = ldap_connect_moodle(TEST_LDAPLIB_HOST_URL, 3, 'rfc2307', TEST_LDAPLIB_BIND_DN,
+                                               TEST_LDAPLIB_BIND_PW, LDAP_DEREF_NEVER, $debuginfo, false)) {
+            $this->markTestSkipped('Cannot connect to LDAP test server: '.$debuginfo);
+        }
+
+        // Create new empty test container.
+        if (!($containerdn = $this->create_test_container($connection, 'moodletest'))) {
+            $this->markTestSkipped('Can not create test LDAP container.');
+        }
+
+        // Add all the test objects.
+        $testobjects = $this->get_ldap_get_entries_moodle_test_objects();
+        if (!$this->add_test_objects($connection, $containerdn, $testobjects)) {
+            $this->markTestSkipped('Can not create LDAP test objects.');
+        }
+
+        // Now query about them and compare results.
+        foreach ($testobjects as $object) {
+            $dn = $this->get_object_dn($object, $containerdn);
+            $filter = $object['query']['filter'];
+            $attributes = $object['query']['attributes'];
+
+            $sr = ldap_read($connection, $dn, $filter, $attributes);
+            if (!$sr) {
+                $this->markTestSkipped('Cannot retrieve test objects from LDAP test server.');
+            }
+
+            $entries = ldap_get_entries_moodle($connection, $sr);
+            $actual = array_keys($entries[0]);
+            $expected = $object['expected'];
+
+            // We need to sort both arrays to be able to compare them, as the LDAP server
+            // might return attributes in any order.
+            sort($expected);
+            sort($actual);
+            $this->assertEquals($expected, $actual);
+        }
+
+        // Clean up test objects and container.
+        $this->remove_test_objects($connection, $containerdn, $testobjects);
+        $this->remove_test_container($connection, $containerdn);
+    }
+
+    /**
+     * Provide the array of test objects for the ldap_get_entries_moodle test case.
+     *
+     * @return array of test objects
+     */
+    protected function get_ldap_get_entries_moodle_test_objects() {
+        $testobjects = array(
+            // Test object 1.
+            array(
+                // Add/remove this object to LDAP directory? There are existing standard LDAP
+                // objects that we might want to test, but that we shouldn't add/remove ourselves.
+                'addremove' => true,
+                // Relative (to test container) or absolute distinguished name (DN).
+                'relativedn' => true,
+                // Distinguished name for this object (interpretation depends on 'relativedn').
+                'dn' => 'cn=test1',
+                // Values to add to LDAP directory.
+                'values' => array(
+                    'objectClass' => array('inetOrgPerson', 'organizationalPerson', 'person', 'posixAccount'),
+                    'cn' => 'test1',  // We don't care about the actual values, as long as they are unique.
+                    'sn' => 'test1',
+                    'givenName' => 'test1',
+                    'uid' => 'test1',
+                    'uidNumber' => '20001',  // Start from 20000, then add test number.
+                    'gidNumber' => '20001',  // Start from 20000, then add test number.
+                    'homeDirectory' => '/',
+                    'userPassword' => '*',
+                ),
+                // Attributes to query the object for.
+                'query' => array(
+                    'filter' => '(objectClass=posixAccount)',
+                    'attributes' => array(
+                        'cn',
+                        'sn',
+                        'givenName',
+                        'uid',
+                        'uidNumber',
+                        'gidNumber',
+                        'homeDirectory',
+                        'userPassword'
+                    ),
+                ),
+                // Expected values for the queried attributes' names.
+                'expected' => array(
+                    'cn',
+                    'sn',
+                    'givenname',
+                    'uid',
+                    'uidnumber',
+                    'gidnumber',
+                    'homedirectory',
+                    'userpassword'
+                ),
+            ),
+            // Test object 2.
+            array(
+                'addremove' => true,
+                'relativedn' => true,
+                'dn' => 'cn=group2',
+                'values' => array(
+                    'objectClass' => array('top', 'posixGroup'),
+                    'cn' => 'group2',  // We don't care about the actual values, as long as they are unique.
+                    'gidNumber' => '20002',  // Start from 20000, then add test number.
+                    'memberUid' => '20002',  // Start from 20000, then add test number.
+                ),
+                'query' => array(
+                    'filter' => '(objectClass=posixGroup)',
+                    'attributes' => array(
+                        'cn',
+                        'gidNumber',
+                        'memberUid'
+                    ),
+                ),
+                'expected' => array(
+                    'cn',
+                    'gidnumber',
+                    'memberuid'
+                ),
+            ),
+            // Test object 3.
+            array(
+                'addremove' => false,
+                'relativedn' => false,
+                'dn' => '',  // To query the RootDSE, we must specify the empty string as the absolute DN.
+                'values' => array(
+                ),
+                'query' => array(
+                    'filter' => '(objectClass=*)',
+                    'attributes' => array(
+                        'supportedControl',
+                        'namingContexts'
+                    ),
+                ),
+                'expected' => array(
+                    'supportedcontrol',
+                    'namingcontexts'
+                ),
+            ),
+        );
+
+        return $testobjects;
+    }
+
+    /**
+     * Create a new container in the LDAP domain, to hold the test objects. The
+     * container is created as a domain component (dc) + organizational unit (ou) object.
+     *
+     * @param object $connection Valid LDAP connection
+     * @param string $container Name of the test container to create.
+     *
+     * @return string or false Distinguished name for the created container, or false on error.
+     */
+    protected function create_test_container($connection, $container) {
+        $object = array();
+        $object['objectClass'] = array('dcObject', 'organizationalUnit');
+        $object['dc'] = $container;
+        $object['ou'] = $container;
+        $containerdn = 'dc='.$container.','.TEST_LDAPLIB_DOMAIN;
+        if (!ldap_add($connection, $containerdn, $object)) {
+            return false;
+        }
+        return $containerdn;
+    }
+
+    /**
+     * Remove the container in the LDAP domain root that holds the test objects. The container
+     * *must* be empty before trying to remove it. Otherwise this function fails.
+     *
+     * @param object $connection Valid LDAP connection
+     * @param string $containerdn The distinguished of the container to remove.
+     */
+    protected function remove_test_container($connection, $containerdn) {
+        ldap_delete($connection, $containerdn);
+    }
+
+    /**
+     * Add the test objects to the test container.
+     *
+     * @param resource $connection Valid LDAP connection
+     * @param string $containerdn The distinguished name of the container for the created objects.
+     * @param array $testobjects Array of the tests objects to create. The structure of
+     *              the array elements *must* follow the structure of the value returned
+     *              by ldap_get_entries_moodle_test_objects() member function.
+     *
+     * @return boolean True on success, false otherwise.
+     */
+    protected function add_test_objects($connection, $containerdn, $testobjects) {
+        foreach ($testobjects as $object) {
+            if ($object['addremove'] !== true) {
+                continue;
+            }
+            $dn = $this->get_object_dn($object, $containerdn);
+            $entry = $object['values'];
+            if (!ldap_add($connection, $dn, $entry)) {
+                return false;
+            }
+        }
+        return true;
+    }
+
+    /**
+     * Remove the test objects from the test container.
+     *
+     * @param resource $connection Valid LDAP connection
+     * @param string $containerdn The distinguished name of the container for the objects to remove.
+     * @param array $testobjects Array of the tests objects to create. The structure of
+     *              the array elements *must* follow the structure of the value returned
+     *              by ldap_get_entries_moodle_test_objects() member function.
+     *
+     */
+    protected function remove_test_objects($connection, $containerdn, $testobjects) {
+        foreach ($testobjects as $object) {
+            if ($object['addremove'] !== true) {
+                continue;
+            }
+            $dn = $this->get_object_dn($object, $containerdn);
+            ldap_delete($connection, $dn);
+        }
+    }
+
+    /**
+     * Get the distinguished name (DN) for a given object.
+     *
+     * @param object $object The LDAP object to calculate the DN for.
+     * @param string $containerdn The DN of the container to use for objects with relative DNs.
+     *
+     * @return string The calculated DN.
+     */
+    protected function get_object_dn($object, $containerdn) {
+        if ($object['relativedn']) {
+            $dn = $object['dn'].','.$containerdn;
+        } else {
+            $dn = $object['dn'];
+        }
+        return $dn;
+    }
 }
index 79ee13b..c923bfb 100644 (file)
@@ -1,6 +1,11 @@
 This files describes API changes in core libraries and APIs,
 information provided here is intended especially for developers.
 
+=== 3.3.1 ===
+
+* ldap_get_entries_moodle() now always returns lower-cased attribute names in the returned entries.
+  It was suppposed to do so before, but it actually didn't.
+
 === 3.3 ===
 
 * Behat compatibility changes are now being documented at