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
          • Pull Master Diff URL:

            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

                Attachments

                  Issue Links

                  blocks

                  Sub-task - Issues are sometimes broken into multiple sub-tasks. MDL-35159 Add enrol_ldap restore support

                  • Minor - Minor loss of function where workaround is possible
                  • Closed
                  Discovered while testing

                  Sub-task - Issues are sometimes broken into multiple sub-tasks. MDL-37391 do not pass ldap connection around in enrol_ldap

                  • Minor - Minor loss of function where workaround is possible
                  • Closed
                  is a regression caused by

                  Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-36119 ldap auth sync - add paged support

                  • Minor - Minor loss of function where workaround is possible
                  • Closed
                  Testing discovered

                  Improvement - An enhancement to an existing Moodle feature. MDL-37314 cleanup enrol_ldap

                  • Minor - Minor loss of function where workaround is possible
                  • Open

                    Activity

                    Ascending order - Click to sort in descending order
                    Hide
                    Permalink
                    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
                    Permalink
                    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
                    Permalink
                    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
                    Permalink
                    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
                    Permalink
                    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
                    Permalink
                    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