Moodle

ldap paged results connection closing in find_ext_enrolments() is incorrect

Details

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

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...

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 - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
discovered while testing

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

  • Minor - Minor loss of function where workaround is possible
  • Open - The issue is open and ready for the assignee to start work on it.
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 - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
testing discovered

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 - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Petr Škoda added a comment -

Adding some watchers, please review, thanks.

Show
Petr Škoda added a comment - Adding some watchers, please review, thanks.
Hide
Permalink
Sam Hemelryk added a comment -

Thanks Petr - this has been integrated now.

Show
Sam Hemelryk added a comment - Thanks Petr - this has been integrated now.
Hide
Permalink
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
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
Petr Škoda 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
Petr Škoda 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
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
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
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
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

Vote (0)
Watch (8)

Dates

  • Created:
    Updated:
    Resolved: