Short explanation

The enrol_lti\task\sync_members task currently relies on an internal array (namely currentusers) to keep record of users that were retrieved from the LTI membership service.

It is used as sanity check before both sync_member_information (enrol) and sync_unenrol (unenrol) methods.

In short, the problem happens because the currentusers class variable never gets reset after each LTI Consumer iteration. What then happens is that the sanity check in the sync_unenrol fails for all the subsequente iterations, and it will unenrol users from a LTI Consumer even when that consumer doesn't provide a membership service.

Complete explanation

We'be been experiencing an issue with the wrong unenrolment of users when you have the following setup:

Two LTI consumers for the same LTI Tool:

• Provider - any Moodle platform configured to publish a course as a LTI Tool
• Consumer A - registered with Provider with a membership URL
• Consumer B - registered with Provider without a membership URL

On the Provider website, we need to have the following processing order on the LTI enrol_lti\task\sync_members task:

1. Consumer A (with membership url)
2. Consumer B (without the membership url)

The following scenarios WORKS:

• Consumer A
  • An "User A" coming from the consumer platform launches the LTI Tool and is enrolled into the Provider
• Consumer B
  • An "User B" coming from the consumer platform launches the LTI Tool and is enrolled into the Provider
• Provider (running the sync_members task)
  • Start processing Consumer A
    • Checks membership URL - returns 0 members
    • Skip new enrolments (since it returned 0 members)
    • Skip unenrolments (since task's internal currentusers variable is empty)
  • Start processing Consumer B
    • Checks membership URL - fails because it doesn't have one
    • Skip new enrolments (since it failed at fetching members)
    • Skip unenrolments (since task's internal currentusers variable is empty)

The following scenarios BREAKS:

• Consumer A
  • An "User A" coming from the consumer platform launches the LTI Tool and is enrolled into the Provider
• Consumer B
  • An "User B" coming from the consumer platform launches the LTI Tool and is enrolled into the Provider
• Provider (running the sync_members task)
  • Start processing Consumer A
    • Checks membership URL - returns 1 member (that "User A")
    • Updates "User A" enrolment with its membership information and adds him into the currentusers internal task class variable.
    • Processes unenrolments but unenrols no one, since "User A" is in the currentusers array.
  • Start processing Consumer B
    • Checks membership URL - fails because it doesn't have one
    • Skip new enrolments (since it failed at fetching members)
    • Wrongly processes unenrolments, since currentusers is currently not empty (as it was filled with "User A" by the previous Consumer A processing). Since "User B" is not in the currentusers array, the task unenrols him.

The interesting bits of code are all in the enrol/lti/classes/task/sync_members.php file:

line 54 (task class definition):

protected $currentusers = [];

lines 240 to 270 (enrol/update routine):

$dbuser = core_user::get_user_by_username($user->username, 'id');

if ($dbuser) {

    // If email is empty remove it, so we don't update the user with an empty email.

    if (empty($user->email)) {

        unset($user->email);

    }

    $user->id = $dbuser->id;

    user_update_user($user);

    // Add the information to the necessary arrays.

    if (!in_array($user->id, $this->currentusers)) {

        $this->currentusers[] = $user->id;

    }

    $this->userphotos[$user->id] = $member->image;

} else {

    if ($this->should_sync_enrol($tool->membersyncmode)) {

        // If the email was stripped/not set then fill it with a default one. This

        // stops the user from being redirected to edit their profile page.

        if (empty($user->email)) {

            $user->email = $user->username .  "@example.com";

        }

        $user->auth = 'lti';

        $user->id = user_create_user($user);

        // Add the information to the necessary arrays.

        $this->currentusers[] = $user->id;

        $this->userphotos[$user->id] = $member->image;

    }

}

line 311 to 330 (un-enrol routine):

if (empty($this->currentusers)) {

    return 0;

}

$unenrolcount = 0;

$ltiusersrs = $DB->get_recordset('enrol_lti_users', array('toolid' => $tool->id), 'lastaccess DESC', 'userid');

// Go through the users and check if any were never listed, if so, remove them.

foreach ($ltiusersrs as $ltiuser) {

    if (!in_array($ltiuser->userid, $this->currentusers)) {

        $instance = new stdClass();

        $instance->id = $tool->enrolid;

        $instance->courseid = $tool->courseid;

        $instance->enrol = 'lti';

        $ltiplugin->unenrol_user($instance, $ltiuser->userid);

        // Increment unenrol count.

        $unenrolcount++;

    }

}

$ltiusersrs->close();

Here is a very dirty but quick fix for this issue, for testing purposes:

line 88 to 90:

foreach ($tools as $tool) {

+   $this->currentusers = [];

    mtrace("Starting - Member sync for published tool '$tool->id' for course '$tool->courseid'.");

For posterity, the original patch attached to the issue (thanks to Denis):
Repo:
https://github.com/denisjuergen/moodle

Branch:
MDL-69251-MOODLE_39_STABLE

Diff:
https://github.com/moodle/moodle/compare/MOODLE_39_STABLE...denisjuergen:MDL-69251-MOODLE_39_STABLE