Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-63611

LDAP Sync: usernames with surrounding spaces break user creation transaction




      When one LDAP user has additional spaces in their user_attribute, it will get trim() med before being written to the tmp_extuser table:


                      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);
                          } 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";

      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.




            Unassigned Unassigned
            odyx Didier Raboud
            Component watchers:
            Adrian Greeve, Jake Dallimore, Mathew May, Mihail Geshoski, Peter Dias, Amaia Anabitarte, Carlos Escobedo, Ferran Recio, Sara Arjona (@sarjona)
            0 Vote for this issue
            3 Start watching this issue