Skip to content

Commit

Permalink
MDL-57558 ldap: fix ldap_get_entries_moodle()
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@iarenaza @danpoltawski
iarenaza authored and danpoltawski committed May 16, 2017
1 parent 9695ca7 commit 67bebb6
Show file tree
Hide file tree
Showing 4 changed files with 289 additions and 15 deletions.
18 changes: 9 additions & 9 deletions auth/ldap/auth.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ function get_userinfo($username) {
}
$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;
Expand Down Expand Up @@ -634,7 +634,7 @@ function password_expire($username) {
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) {
Expand Down Expand Up @@ -1201,7 +1201,7 @@ function user_update($olduser, $newuser) {
return false;
}

$user_entry = array_change_key_case($user_entry[0], CASE_LOWER);
$user_entry = $user_entry[0];

foreach ($attrmap as $key => $ldapkeys) {
$profilefield = '';
Expand Down Expand Up @@ -1369,7 +1369,7 @@ function user_update_password($user, $newpassword) {
$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
Expand Down Expand Up @@ -1844,7 +1844,7 @@ function ldap_get_ad_pwdexpire($pwdlastset, $ldapconn, $user_dn){
}

$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.
Expand Down Expand Up @@ -1889,25 +1889,25 @@ function ldap_get_ad_pwdexpire($pwdlastset, $ldapconn, $user_dn){
}

$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.
// Grab the new maxpwdage from the msDS-MaximumPasswordAge attribute of the PSO.
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];
}
Expand Down
17 changes: 11 additions & 6 deletions lib/ldaplib.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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++;
Expand Down Expand Up @@ -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;
}
Expand Down
264 changes: 264 additions & 0 deletions lib/tests/ldaplib_test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,4 +207,268 @@ public function ldap_normalise_objectclass_provider() {
),
);
}

/**
* 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;
}
}
5 changes: 5 additions & 0 deletions lib/upgrade.txt
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down

0 comments on commit 67bebb6

Please sign in to comment.