Moodle
  1. Moodle
  2. MDL-15039

Apostrophes preventing any users in course being unenrolled via LDAP

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.2, 1.9
    • Fix Version/s: 1.8.6, 1.9.2
    • Component/s: Enrolments
    • Labels:
      None
    • Environment:
      MS Active Directory
    • Database:
      MySQL
    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE

      Description

      If any member of a course has an ID number containing an apostrophe (eg. CN=Paddy O'Brien,OU=Students,DC=etc...), no other members will ever be unenrolled via LDAP.

      In enrol/ldap/enrol.php, this is due to the array $ldapmembers not accounting for apostrophes and therefore get_records_sql($sql) fails to return any members due to the resultant syntax error.

      It doesn't just affect the name with an apostrophe - it affects any members of the same course.

      We fixed by replacing the following code around line 264 (v1.8.2)

      $sql .= 'AND usr.idnumber NOT IN (\''. join('\',\'', $ldapmembers).'\')';

      with...

      $sql .= 'AND usr.idnumber NOT IN (\''. join('\',\'', str_replace("'", "\'",$ldapmembers)).'\')';

        Gliffy Diagrams

          Activity

          Hide
          Michael Woods added a comment -

          Update - we tested on our 1.9 test instance and the bug is there also.

          Show
          Michael Woods added a comment - Update - we tested on our 1.9 test instance and the bug is there also.
          Hide
          Michael Woods added a comment -

          Forgot to mention an earlier change we made which should probably come under the same bug report.

          Around line 285 of enrol/ldap/enrol.php (v1.8.2), current enrolments are inserted, but this fails for anyone with an apostrophe.

          We replaced:

          ." WHERE idnumber='$ldapmember'";

          with

          ." WHERE idnumber='".str_replace("'", "\'",$ldapmember)."'";

          I think this is still outstanding in 1.9.

          Show
          Michael Woods added a comment - Forgot to mention an earlier change we made which should probably come under the same bug report. Around line 285 of enrol/ldap/enrol.php (v1.8.2), current enrolments are inserted, but this fails for anyone with an apostrophe. We replaced: ." WHERE idnumber='$ldapmember'"; with ." WHERE idnumber='".str_replace("'", "\'",$ldapmember)."'"; I think this is still outstanding in 1.9.
          Hide
          Iñaki Arenaza added a comment -

          Hi Michael,

          this should be fixed in the latest CVS versions of 1.8, 1.9 and HEAD.

          Could you confirm it, please?

          Saludos. Iñaki.

          Show
          Iñaki Arenaza added a comment - Hi Michael, this should be fixed in the latest CVS versions of 1.8, 1.9 and HEAD. Could you confirm it, please? Saludos. Iñaki.
          Hide
          Michael Blake added a comment -

          test

          Show
          Michael Blake added a comment - test
          Hide
          Michael Blake added a comment -

          should not have been resolved.

          Show
          Michael Blake added a comment - should not have been resolved.
          Hide
          Petr Skoda added a comment -

          Hi,
          what is the status? should we resolve this?

          Show
          Petr Skoda added a comment - Hi, what is the status? should we resolve this?
          Hide
          Iñaki Arenaza added a comment -

          All my tests show that this should be fixed in 1.8, 1.9 and 2.0 (with your patch from MDL-14679, in revision 1.25), but I was waiting for comfirmation from Michael Woods.

          Saludos. Iñaki.

          Show
          Iñaki Arenaza added a comment - All my tests show that this should be fixed in 1.8, 1.9 and 2.0 (with your patch from MDL-14679 , in revision 1.25), but I was waiting for comfirmation from Michael Woods. Saludos. Iñaki.
          Hide
          Petr Skoda added a comment -

          thanks, my +1 to Resolve it now and wait with Closing a bit

          Show
          Petr Skoda added a comment - thanks, my +1 to Resolve it now and wait with Closing a bit
          Hide
          Michael Woods added a comment -

          Hi all,

          Inaki - thank you for the fix. Unfortunately, I still haven't had a chance to test it. If your tests show that this is resolved, feel free to close it. When I eventually get around to testing it, I can let you know if there any problems (which I doubt).

          Thanks again,
          Michael

          Show
          Michael Woods added a comment - Hi all, Inaki - thank you for the fix. Unfortunately, I still haven't had a chance to test it. If your tests show that this is resolved, feel free to close it. When I eventually get around to testing it, I can let you know if there any problems (which I doubt). Thanks again, Michael
          Hide
          Iñaki Arenaza added a comment -

          Resolving now. Michael, please reopen if it doesn't fix your case

          Saludos. Iñaki.

          Show
          Iñaki Arenaza added a comment - Resolving now. Michael, please reopen if it doesn't fix your case Saludos. Iñaki.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: