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

LDAP authentication uses a mix of username and idnumber

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.6
    • Fix Version/s: 1.8, 1.9
    • Component/s: Authentication
    • Labels:
      None
    • Environment:
      All
    • Database:
      Any
    • Affected Branches:
      MOODLE_16_STABLE
    • Fixed Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE

      Description

      when auth_ldap_sync_users is called it fails if you are not using the username as the idnumber. Thus the temporary table {$CFG->prefix}ext_user gets the idnumber field populated with usernames. Then when the subsequent 'LEFT JOIN' is called the idnumber field returned is NULL for all users. This results in all of the users being deleted and then readded. This causes lots of extraneous work and problems in certain cases.

      To resolve the issue the code either needs to be switched to use only idnumber as the keying attribute or username. Even though the other external authentication methods use idnumber as the outside keying entry I think it is correct to use username in the case of LDAP.

      One solution is to change the LEFT JOINS, there are two queries, to use the username field from {$CFG->prefix}user rather than idnumber.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              dougiamas Martin Dougiamas added a comment -

              From Jeff Graham (jjg7 at humboldt.edu) Tuesday, 20 June 2006, 02:33 AM:

              attached is a diff from that should fix this bug. I chose to go with username as the rest of the authentication code is keying off of username rather than idnumber.

              Show
              dougiamas Martin Dougiamas added a comment - From Jeff Graham (jjg7 at humboldt.edu) Tuesday, 20 June 2006, 02:33 AM: attached is a diff from that should fix this bug. I chose to go with username as the rest of the authentication code is keying off of username rather than idnumber.
              Hide
              danmarsden Dan Marsden added a comment -

              Hi Martin, - any chance on gettting this patched? - I could commit it to head if you have no objections!

              this is the change I'd suggest

              $sql = 'SELECT u.id, u.username
              FROM ' . $CFG->prefix .'user u LEFT JOIN ' . $CFG->prefix .'extuser e
              ON u.username = e.idnumber
              WHERE u.auth=\'' . AUTH_LDAP_NAME . '\' AND u.deleted=\'0\' AND e.idnumber IS NULL';

              Instead of:

              $sql = 'SELECT u.id, u.username
              FROM ' . $CFG->prefix .'user u LEFT JOIN ' . $CFG->prefix .'extuser e
              ON u.idnumber = e.idnumber
              WHERE u.auth=\'' . AUTH_LDAP_NAME . '\' AND u.deleted=\'0\' AND e.idnumber IS NULL';

              Show
              danmarsden Dan Marsden added a comment - Hi Martin, - any chance on gettting this patched? - I could commit it to head if you have no objections! this is the change I'd suggest $sql = 'SELECT u.id, u.username FROM ' . $CFG->prefix .'user u LEFT JOIN ' . $CFG->prefix .'extuser e ON u.username = e.idnumber WHERE u.auth=\'' . AUTH_LDAP_NAME . '\' AND u.deleted=\'0\' AND e.idnumber IS NULL'; Instead of: $sql = 'SELECT u.id, u.username FROM ' . $CFG->prefix .'user u LEFT JOIN ' . $CFG->prefix .'extuser e ON u.idnumber = e.idnumber WHERE u.auth=\'' . AUTH_LDAP_NAME . '\' AND u.deleted=\'0\' AND e.idnumber IS NULL';
              Hide
              martinlanghoff Martín Langhoff added a comment -

              Dan - I agree with the idea but... your "patch" looks backwards?

              Show
              martinlanghoff Martín Langhoff added a comment - Dan - I agree with the idea but... your "patch" looks backwards?
              Hide
              jgraham Jeff Graham added a comment -

              Bringing attached diff from old tracker over.

              Show
              jgraham Jeff Graham added a comment - Bringing attached diff from old tracker over.
              Hide
              jgraham Jeff Graham added a comment -

              Martin,

              I believe Dan's patch is correct, but there are 2 queries that need to be adjusted. I have brought over the diff from the old tracker.

              The other option would be to populate prefix_extuser.idnumber with the same value as user.idnumber or change that fieldname in extuser to username.

              Show
              jgraham Jeff Graham added a comment - Martin, I believe Dan's patch is correct, but there are 2 queries that need to be adjusted. I have brought over the diff from the old tracker. The other option would be to populate prefix_extuser.idnumber with the same value as user.idnumber or change that fieldname in extuser to username.
              Hide
              danmarsden Dan Marsden added a comment -

              Hi Martin,

              Nope - not backwards! - it makes the thing work in our environment! - what looks wrong - will this not work everywhere?

              Dan

              Show
              danmarsden Dan Marsden added a comment - Hi Martin, Nope - not backwards! - it makes the thing work in our environment! - what looks wrong - will this not work everywhere? Dan
              Hide
              skodak Petr Skoda added a comment -

              should be fixed as part of the auth cleanup, only usernames are now used for sync, synchronization of removed users is now fully configurable - allows full delete/suspend/nothing actions.

              please report any problems with the new code, closing now - thanks!

              Show
              skodak Petr Skoda added a comment - should be fixed as part of the auth cleanup, only usernames are now used for sync, synchronization of removed users is now fully configurable - allows full delete/suspend/nothing actions. please report any problems with the new code, closing now - thanks!
              Hide
              ppollet Patrick Pollet added a comment -

              Although revamped code in 1.82 DO fix the previous issue (it is a pity it was not backported in CVS in 1.6 and 1.7 in Novembre 2006 , it would have saved us a lot of headache ;-(() , use of username as the Ldap unique user's information to sync Moodle's user database brings new problems.

              See http://moodle.org/mod/forum/discuss.php?d=80652#p358210

              Show
              ppollet Patrick Pollet added a comment - Although revamped code in 1.82 DO fix the previous issue (it is a pity it was not backported in CVS in 1.6 and 1.7 in Novembre 2006 , it would have saved us a lot of headache ;-(() , use of username as the Ldap unique user's information to sync Moodle's user database brings new problems. See http://moodle.org/mod/forum/discuss.php?d=80652#p358210

                People

                • Votes:
                  1 Vote for this issue
                  Watchers:
                  2 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    31/Mar/07