From 22c346831ebd9151d3a8baf28b1be56be69a807b Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Fri, 5 May 2023 14:13:37 +0800 Subject: [PATCH 1/2] MDL-76821 enrol_lti: fix bug in repo causing return of legacy lti users If the enrolment method is updated from an LTI 1.1/2.0 tool to an LTI 1.3 tool, it may have associated enrol_lti_users records not having ltideploymentid values. These are legacy users and must not be returned by the repository, which deals only with LTI 1.3 LTI users. --- .../repository/user_repository.php | 7 +++- .../repository/user_repository_test.php | 39 +++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/enrol/lti/classes/local/ltiadvantage/repository/user_repository.php b/enrol/lti/classes/local/ltiadvantage/repository/user_repository.php index de0a7594096..ca072613750 100644 --- a/enrol/lti/classes/local/ltiadvantage/repository/user_repository.php +++ b/enrol/lti/classes/local/ltiadvantage/repository/user_repository.php @@ -218,7 +218,8 @@ class user_repository { FROM {{$this->ltiuserstable}} lu JOIN {user} u ON (u.id = lu.userid) - WHERE lu.id = :id"; + WHERE lu.id = :id + AND lu.ltideploymentid IS NOT NULL"; $record = $DB->get_record_sql($sql, ['id' => $id], MUST_EXIST); return $this->user_from_record($record); @@ -245,7 +246,8 @@ class user_repository { JOIN {user} u ON (u.id = lu.userid) WHERE lu.userid = :userid - AND lu.toolid = :resourceid"; + AND lu.toolid = :resourceid + AND lu.ltideploymentid IS NOT NULL"; $params = ['userid' => $userid, 'resourceid' => $resourceid]; $record = $DB->get_record_sql($sql, $params, MUST_EXIST); @@ -270,6 +272,7 @@ class user_repository { JOIN {user} u ON (u.id = lu.userid) WHERE lu.toolid = :resourceid + AND lu.ltideploymentid IS NOT NULL ORDER BY lu.lastaccess DESC"; $records = $DB->get_records_sql($sql, ['resourceid' => $resourceid]); diff --git a/enrol/lti/tests/local/ltiadvantage/repository/user_repository_test.php b/enrol/lti/tests/local/ltiadvantage/repository/user_repository_test.php index cd690379330..7a61103d1db 100644 --- a/enrol/lti/tests/local/ltiadvantage/repository/user_repository_test.php +++ b/enrol/lti/tests/local/ltiadvantage/repository/user_repository_test.php @@ -455,4 +455,43 @@ class user_repository_test extends \advanced_testcase { $this->assertEquals($saveduser->get_localid(), $saveduser2->get_localid()); $this->assertNotEquals($saveduser->get_id(), $saveduser2->get_id()); } + + /** + * Test confirming that any associated legacy lti user records are not returned by the repository. + * + * This test ensures that any enrolment methods (resources) updated in-place from legacy LTI to 1.3 only return LTI 1.3 users. + * + * @covers ::find + * @covers ::find_single_user_by_resource + * @covers ::find_by_resource + */ + public function test_find_filters_legacy_lti_users(): void { + $this->resetAfterTest(); + global $DB; + $user = $this->getDataGenerator()->create_user(); + $course = $this->getDataGenerator()->create_course(); + $resource = $this->getDataGenerator()->create_lti_tool((object)['courseid' => $course->id]); + $ltiuserdata = [ + 'userid' => $user->id, + 'toolid' => $resource->id, + 'sourceid' => '1001', + ]; + $ltiuserid = $DB->insert_record('enrol_lti_users', $ltiuserdata); + $userrepo = new user_repository(); + + $this->assertNull($userrepo->find($ltiuserid)); + $this->assertNull($userrepo->find_single_user_by_resource($user->id, $resource->id)); + $this->assertEmpty($userrepo->find_by_resource($resource->id)); + + // Set deploymentid, indicating the user originated from an LTI 1.3 launch and should now be returned. + $ltiuserdata['id'] = $ltiuserid; + $ltiuserdata['ltideploymentid'] = '234'; + $DB->update_record('enrol_lti_users', $ltiuserdata); + + $this->assertInstanceOf(user::class, $userrepo->find($ltiuserid)); + $this->assertInstanceOf(user::class, $userrepo->find_single_user_by_resource($user->id, $resource->id)); + $ltiusers = $userrepo->find_by_resource($resource->id); + $this->assertCount(1, $ltiusers); + $this->assertInstanceOf(user::class, reset($ltiusers)); + } } From b3feea921dccaa4036f4874ad506b5675f1e8b30 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Fri, 5 May 2023 14:24:15 +0800 Subject: [PATCH 2/2] MDL-76821 enrol_lti: prevent legacy launches via upgraded enrolment If the enrolment instance (the 'published resource') has been upgraded from LTI 1.1/2.0 to LTI 1.3 (i.e. a new instance was not created), prevent legacy launches which may occur from old resource links. Only LTI Advantage launches should be permitted through the method. --- enrol/lti/lang/en/enrol_lti.php | 1 + enrol/lti/tool.php | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/enrol/lti/lang/en/enrol_lti.php b/enrol/lti/lang/en/enrol_lti.php index 5808a0334d5..dce5d50a21a 100644 --- a/enrol/lti/lang/en/enrol_lti.php +++ b/enrol/lti/lang/en/enrol_lti.php @@ -54,6 +54,7 @@ $string['enrolenddate'] = 'End date'; $string['enrolenddate_help'] = 'If enabled, users can access until this date only.'; $string['enrolenddateerror'] = 'Enrolment end date cannot be earlier than start date'; $string['enrolisdisabled'] = 'The \'Publish as LTI tool\' plugin is disabled.'; +$string['enrolltiversionincorrect'] = 'The resource is not set up for use over legacy LTI (versions 1.1/2.0). Please contact the administrator of this tool.'; $string['enrolperiod'] = 'Enrolment duration'; $string['enrolperiod_help'] = 'Length of time that the enrolment is valid, starting with the moment the user enrols themselves from the remote system. If disabled, the enrolment duration will be unlimited.'; $string['enrolmentfinished'] = 'Enrolment finished.'; diff --git a/enrol/lti/tool.php b/enrol/lti/tool.php index 6730d474f1c..5aad2b3af55 100644 --- a/enrol/lti/tool.php +++ b/enrol/lti/tool.php @@ -53,6 +53,12 @@ if ($tool->status != ENROL_INSTANCE_ENABLED) { exit(); } +// Check if the enrolment instance has been upgraded to a newer LTI version. +if ($tool->ltiversion != 'LTI-1p0/LTI-2p0') { + throw new \moodle_exception('enrolltiversionincorrect', 'enrol_lti'); + exit(); +} + $consumerkey = required_param('oauth_consumer_key', PARAM_TEXT); $ltiversion = optional_param('lti_version', null, PARAM_TEXT); $messagetype = required_param('lti_message_type', PARAM_TEXT);