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

ldap paged results connection closing in find_ext_enrolments() is incorrect

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.4.1
    • Component/s: Enrolments
    • Labels:
    • Testing Instructions:
      Hide

      0/ enable full debug
      1/ set up ldap enrol where one user gets different roles in different courses
      2/ login as that user
      3/ review error log and verify user was enrolled properly
      4/ add a lot more enrolments for that user and test if ldap paging works during login (you might want to lower the paging limit in enrol settings)

      Show
      0/ enable full debug 1/ set up ldap enrol where one user gets different roles in different courses 2/ login as that user 3/ review error log and verify user was enrolled properly 4/ add a lot more enrolments for that user and test if ldap paging works during login (you might want to lower the paging limit in enrol settings)
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      w51_MDL-37315_m25_ldappaging

      Description

      The trouble is that

      protected function find_ext_enrolments ($ldapconnection, $memberuid, $role) 

      closes connection and creates a new one, but the calling code continues to be using the CLOSED connection!

      PHP Warning: ldap_control_paged_result(): 90 is not a valid ldap link resource in moodle25/enrol/ldap/lib.php on line 732

      This may result in user unenrolment causing major data loss...

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            skodak Petr Skoda added a comment -

            Adding some watchers, please review, thanks.

            Show
            skodak Petr Skoda added a comment - Adding some watchers, please review, thanks.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks Petr - this has been integrated now.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks Petr - this has been integrated now.
            Hide
            iarenaza Iñaki Arenaza added a comment -

            Sorry for being so late at reviwing this

            While the fix is OK, I'd say we should fix the cause rather than the symptoms. We shouldn't be passing $ldapconnection around. We already have the active ldap connection in $this->ldapconnection, and we should use it everywhere. The $ldapconnection passing is a leftover from the old days when the auth and enrol LDAP plugins were not classes.

            The only places where we should pass $this->ldapconnection are to the lib/ldaplib.php functions calls.

            I guess this should go into a new bug.

            Saludos.
            Iñaki.

            Show
            iarenaza Iñaki Arenaza added a comment - Sorry for being so late at reviwing this While the fix is OK, I'd say we should fix the cause rather than the symptoms. We shouldn't be passing $ldapconnection around. We already have the active ldap connection in $this->ldapconnection, and we should use it everywhere. The $ldapconnection passing is a leftover from the old days when the auth and enrol LDAP plugins were not classes. The only places where we should pass $this->ldapconnection are to the lib/ldaplib.php functions calls. I guess this should go into a new bug. Saludos. Iñaki.
            Hide
            skodak Petr Skoda added a comment -

            I agree that we should not pass the connection around, I will create a new issue and fix it in master only, thanks a lot.

            Show
            skodak Petr Skoda added a comment - I agree that we should not pass the connection around, I will create a new issue and fix it in master only, thanks a lot.
            Hide
            fred Frédéric Massart added a comment -

            Test successful on master. The user is correctly assigned the roles on the matching courses, even with a very low paging. No errors were found in the logs.

            Show
            fred Frédéric Massart added a comment - Test successful on master. The user is correctly assigned the roles on the matching courses, even with a very low paging. No errors were found in the logs.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            And your fantastic code has met core, hope they become good friends for a long period.

            Closing, thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - And your fantastic code has met core, hope they become good friends for a long period. Closing, thanks!

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                8 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/Jan/13