-
Bug
-
Resolution: Deferred
-
Minor
-
None
-
3.5.2
When one LDAP user has additional spaces in their user_attribute, it will get trim() med before being written to the tmp_extuser table:
https://github.com/moodle/moodle/blob/master/auth/ldap/auth.php#L730:
if ($entry = @ldap_first_entry($ldapconnection, $ldap_result)) { |
do { |
$value = ldap_get_values_len($ldapconnection, $entry, $this->config->user_attribute); |
$value = core_text::convert($value[0], $this->config->ldapencoding, 'utf-8'); |
$value = trim($value); |
$this->ldap_bulk_insert($value); |
} while ($entry = ldap_next_entry($ldapconnection, $entry)); |
}
|
Therefore, the temporary entry for <johndoe@example.com > will have no end space in that temporary representation.
But later, said entry is used to find the LDAP entry from the LDAP server:
https://github.com/moodle/moodle/blob/master/auth/ldap/auth.php#L880 :
/// User Additions
|
// Find users missing in DB that are in LDAP |
// and gives me a nifty object I don't want. |
// note: we do not care about deleted accounts anymore, this feature was replaced by suspending to nologin auth plugin |
$sql = 'SELECT e.id, e.username |
FROM {tmp_extuser} e
|
LEFT JOIN {user} u ON (e.username = u.username AND e.mnethostid = u.mnethostid)
|
WHERE u.id IS NULL';
|
$add_users = $DB->get_records_sql($sql); |
if (!empty($add_users)) { |
print_string('userentriestoadd', 'auth_ldap', count($add_users)); |
$transaction = $DB->start_delegated_transaction(); |
foreach ($add_users as $user) { |
$user = $this->get_userinfo_asobj($user->username); |
|
|
// Prep a few params |
$user->modified = time(); |
… but! The result of $this->get_userinfo_asobj is not checked against being possibly false (which it can!). Then $user->username is empty user_create_user fails and the database transaction aborts; creating none of the users (although only one was "broken").
I understand Moodle usernames MUST BE trimmed, but failing a bunch of user creations for one wrong user is clearly a bug.
My quick workaround for this is to replace the first lines of above's foreach:
$username = $user->username;
|
$user = $this->get_userinfo_asobj($username);
|
if ($user === false) {
|
echo "\t Username '$username' not found in LDAP; skipping\n";
|
continue;
|
}
|
This at least allows the user creation to work for the righteous users, and logs what the problem was.
It's clear to me that if get_userinfo_asobj can return false, its return status MUST be checked, but I wonder if the trimming should not ONLY be done when inserting users in the user table.