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

          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