From dd507afe4bd927773f58c3bdb21e02a960ab8582 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Wed, 10 May 2023 09:39:43 +0800 Subject: [PATCH 1/6] MDL-76842 auth_lti: test confirming the erroneous user_updated events This test will fail until the fix - only updating users when data has changed - is put in place in the following commit. --- auth/lti/tests/auth_test.php | 50 ++++++++++++++++++++++++++++-------- 1 file changed, 39 insertions(+), 11 deletions(-) diff --git a/auth/lti/tests/auth_test.php b/auth/lti/tests/auth_test.php index 8690f555565..167aed38f9e 100644 --- a/auth/lti/tests/auth_test.php +++ b/auth/lti/tests/auth_test.php @@ -249,12 +249,15 @@ class auth_test extends \advanced_testcase { $mockjwtdata = $this->get_mock_launchdata_for_user($launchdata['user'], $launchdata['migration_claim'] ?? []); // Authenticate the platform user. + $sink = $this->redirectEvents(); $countusersbefore = $DB->count_records('user'); $user = $auth->find_or_create_user_from_launch($mockjwtdata, true, $legacysecrets); if (!empty($expected['migration_debugging'])) { $this->assertDebuggingCalled(); } $countusersafter = $DB->count_records('user'); + $events = $sink->get_events(); + $sink->close(); // Verify user count is correct. i.e. no user is created when migration claim is correctly processed or when // the user has authenticated with the tool before. @@ -295,15 +298,18 @@ class auth_test extends \advanced_testcase { $this->verify_user_profile_image_updated($user->id); } - // If migrated, verify the user account is reusing the legacy user account. if (!empty($expected['migrated']) && $expected['migrated']) { + // If migrated, verify the user account is reusing the legacy user account. $legacyuserids = array_column($legacyusers, 'id'); $this->assertContains($user->id, $legacyuserids); - } - - // If the user is authenticating a second time, confirm the same account is being returned. - if (isset($firstauthuser)) { + $this->assertInstanceOf(\core\event\user_updated::class, $events[0]); + } else if (isset($firstauthuser)) { + // If the user is authenticating a second time, confirm the same account is being returned. $this->assertEquals($firstauthuser->id, $user->id); + $this->assertEmpty($events); // The user authenticated with the same data once before, so we don't expect an update. + } else { + // The user wasn't migrated and hasn't launched before, so we expect a user_created event. + $this->assertInstanceOf(\core\event\user_created::class, $events[0]); } } @@ -819,9 +825,12 @@ class auth_test extends \advanced_testcase { $mockmemberdata = $this->get_mock_member_data_for_user($memberdata['user'], $memberdata['legacy_user_id'] ?? ''); // Authenticate the platform user. + $sink = $this->redirectEvents(); $countusersbefore = $DB->count_records('user'); $user = $auth->find_or_create_user_from_membership($mockmemberdata, $iss, $legacyconsumerkey ?? ''); $countusersafter = $DB->count_records('user'); + $events = $sink->get_events(); + $sink->close(); // Verify user count is correct. i.e. no user is created when migration claim is correctly processed or when // the user has authenticated with the tool before. @@ -857,15 +866,18 @@ class auth_test extends \advanced_testcase { break; } - // If migrated, verify the user account is reusing the legacy user account. if (!empty($expected['migrated']) && $expected['migrated']) { + // If migrated, verify the user account is reusing the legacy user account. $legacyuserids = array_column($legacyusers, 'id'); $this->assertContains($user->id, $legacyuserids); - } - - // If the user is authenticating a second time, confirm the same account is being returned. - if (isset($firstauthuser)) { + $this->assertInstanceOf(\core\event\user_updated::class, $events[0]); + } else if (isset($firstauthuser)) { + // If the user is authenticating a second time, confirm the same account is being returned. $this->assertEquals($firstauthuser->id, $user->id); + $this->assertEmpty($events); // The user authenticated with the same data once before, so we don't expect an update. + } else { + // The user wasn't migrated and hasn't launched before, so we expect a user_created event. + $this->assertInstanceOf(\core\event\user_created::class, $events[0]); } } @@ -1090,7 +1102,23 @@ class auth_test extends \advanced_testcase { 'PII' => self::PII_NONE, 'migrated' => false ] - ] + ], + 'Existing (linked) platform learner including PII, no legacy data, no consumer key bound, no legacy id' => [ + 'legacy_data' => null, + 'launch_data' => [ + 'has_authenticated_before' => true, + 'user' => $this->get_mock_users_with_ids( + ['1'], + 'http://purl.imsglobal.org/vocab/lis/v2/membership#Learner' + )[0], + ], + 'iss' => $this->issuer, + 'legacy_consumer_key' => null, + 'expected' => [ + 'PII' => self::PII_ALL, + 'migrated' => false + ] + ], ]; } From 3208e7f182809b1240cd042295287c683917bfca Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Tue, 9 May 2023 16:06:14 +0800 Subject: [PATCH 2/6] MDL-76842 auth_lti: don't update the user after membership creation This isn't required and generates excess events. The only thing update does, other than update the user fields, is to update the picture, but in this case, that's unset in membership-based auths anyway, so it's entirely safe to remove this. --- auth/lti/auth.php | 1 - 1 file changed, 1 deletion(-) diff --git a/auth/lti/auth.php b/auth/lti/auth.php index 0757570b5ba..dbca92ab8a2 100644 --- a/auth/lti/auth.php +++ b/auth/lti/auth.php @@ -199,7 +199,6 @@ class auth_plugin_lti extends \auth_plugin_base { } } $user = $this->create_new_account($member, $iss); - $this->update_user_account($user, $member, $iss); return \core_user::get_user($user->id); } } From d9fbe7c8660ad2b77f75cd2530300a8476d66529 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Wed, 10 May 2023 09:41:38 +0800 Subject: [PATCH 3/6] MDL-76842 auth_lti: don't update the user unless data has changed When receiving data from the LTI launch, or service call, only update the user record when we know something has changed. This prevents the creation of many \core\event\user_updated events. --- auth/lti/auth.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/auth/lti/auth.php b/auth/lti/auth.php index dbca92ab8a2..9bf8975868e 100644 --- a/auth/lti/auth.php +++ b/auth/lti/auth.php @@ -380,7 +380,11 @@ class auth_plugin_lti extends \auth_plugin_base { 'lastname' => $userdata['family_name'] ?? $iss, 'email' => $email ]; - user_update_user($update); + $userfieldstocompare = array_intersect_key((array) $user, $update); + + if (!empty(array_diff($update, $userfieldstocompare))) { + user_update_user($update); // Only update if there's a change. + } if (!empty($userdata['picture'])) { try { From b1e550a2292a1a0f32726a4e94f2fc9da60dfda3 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Wed, 10 May 2023 11:05:49 +0800 Subject: [PATCH 4/6] MDL-76842 enrol_lti: test confirming the erroneous user_updated events This test will fail until the fix - only updating users when data has changed - is put in place in the following commit. --- .../repository/user_repository_test.php | 75 ++++++++++++++++--- 1 file changed, 65 insertions(+), 10 deletions(-) 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..3ba923237da 100644 --- a/enrol/lti/tests/local/ltiadvantage/repository/user_repository_test.php +++ b/enrol/lti/tests/local/ltiadvantage/repository/user_repository_test.php @@ -31,9 +31,10 @@ class user_repository_test extends \advanced_testcase { * Helper to generate a new user instance. * * @param int $mockresourceid used to spoof a published resource, to which this user is associated. + * @param array $userfields user information like city, timezone which would normally come from the tool configuration. * @return user a user instance */ - protected function generate_user(int $mockresourceid = 1): user { + protected function generate_user(int $mockresourceid = 1, array $userfields = []): user { global $CFG; $registration = application_registration::create( 'Test', @@ -63,16 +64,35 @@ class user_repository_test extends \advanced_testcase { $savedcontext->get_id()); $savedresourcelink = $resourcelinkrepo->save($resourcelink); - $user = $this->getDataGenerator()->create_user(); + // Create a user using the DB defaults to simulate what would have occurred during an auth_lti user auth. + $user = $this->getDataGenerator()->create_user([ + 'city' => '', + 'country' => '', + 'institution' => '', + 'timezone' => '99', + 'maildisplay' => 2, + 'lang' => 'en' + ]); + + $userdefaultvalues = [ + 'lang' => $CFG->lang, + 'city' => '', + 'country' => '', + 'institution' => '', + 'timezone' => '99', + 'maildisplay' => 2 + ]; + if (empty($userfields)) { + // If userfields is omitted, assume the tool default configuration values (as if 'User default values' are unchanged). + $userfields = $userdefaultvalues; + } else { + // If they have been provided, merge and override the defaults. + $userfields = array_merge($userdefaultvalues, $userfields); + } $ltiuser = $savedresourcelink->add_user( $user->id, 'source-id-123', - $CFG->lang, - 'Perth', - 'AU', - 'An Example Institution', - '99', - 2, + ...array_values($userfields) ); $ltiuser->set_lastgrade(67.33333333); @@ -141,19 +161,45 @@ class user_repository_test extends \advanced_testcase { } /** - * Tests adding a user to the store. + * Tests adding a user to the store, assuming that the user has been created using the default 'user default values'. * * @covers ::save */ - public function test_save_new() { + public function test_save_new_unchanged_user_defaults() { $this->resetAfterTest(); $user = $this->generate_user(); $userrepo = new user_repository(); + $sink = $this->redirectEvents(); $saveduser = $userrepo->save($user); + $events = $sink->get_events(); + $sink->close(); $this->assertIsInt($saveduser->get_id()); $this->assert_same_user_values($user, $saveduser, true); $this->assert_user_db_values($saveduser); + // No change to underlying user: city, etc. take on default values matching those of the existing user record. + $this->assertEmpty($events); + } + + /** + * Tests adding a user to the store, assuming that the user has been created using modified 'user default values'. + * + * @covers ::save + */ + public function test_save_new_changed_user_defaults() { + $this->resetAfterTest(); + $user = $this->generate_user(1, ['city' => 'Perth']); + $userrepo = new user_repository(); + $sink = $this->redirectEvents(); + $saveduser = $userrepo->save($user); + $events = $sink->get_events(); + $sink->close(); + + $this->assertIsInt($saveduser->get_id()); + $this->assert_same_user_values($user, $saveduser, true); + $this->assert_user_db_values($saveduser); + // The underlying user record will change: city ('Perth') differs from that of the existing user (''). + $this->assertInstanceOf(\core\event\user_updated::class, $events[0]); } /** @@ -165,16 +211,25 @@ class user_repository_test extends \advanced_testcase { $this->resetAfterTest(); $user = $this->generate_user(); $userrepo = new user_repository(); + $sink = $this->redirectEvents(); $saveduser = $userrepo->save($user); + $events = $sink->get_events(); + $sink->close(); + $this->assertEmpty($events); // No event for the first save, since the underlying user record is unchanged. $saveduser->set_city('New City'); $saveduser->set_country('NZ'); $saveduser->set_lastgrade(99.99999999); + $sink = $this->redirectEvents(); $saveduser2 = $userrepo->save($saveduser); + $events = $sink->get_events(); + $sink->close(); $this->assertEquals($saveduser->get_id(), $saveduser2->get_id()); $this->assert_same_user_values($saveduser, $saveduser2, true); $this->assert_user_db_values($saveduser2); + // The underlying user record will change now, since city and country have changed. + $this->assertInstanceOf(\core\event\user_updated::class, $events[0]); } /** From 22b0565ef1274d3c7268a106c2238c933279ca78 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Wed, 10 May 2023 11:07:48 +0800 Subject: [PATCH 5/6] MDL-76842 enrol_lti: fix user record updates in user_repository Only call user_update_user when the relevant user data has changed, preventing unnecessary user_updated events. This also removes the line setting timemodified on the user since user_update_user already handles this. --- .../repository/user_repository.php | 25 ++++++++++++++++--- 1 file changed, 21 insertions(+), 4 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..16829cafada 100644 --- a/enrol/lti/classes/local/ltiadvantage/repository/user_repository.php +++ b/enrol/lti/classes/local/ltiadvantage/repository/user_repository.php @@ -162,10 +162,18 @@ class user_repository { "to user '{$ltiuser->userid}' and can't be associated with another user '{$userrecord->id}'."); } - $userrecord->timemodified = $timenow; - $ltiuserrecord->timemodified = $timenow; - \user_update_user($userrecord); + // Only update the Moodle user record if something has changed. + $rawuser = \core_user::get_user($userrecord->id); + $userfieldstocompare = array_intersect_key( + (array) $rawuser, + (array) $userrecord + ); + if (!empty(array_diff((array) $userrecord, $userfieldstocompare))) { + \user_update_user($userrecord); + } unset($userrecord->id); + + $ltiuserrecord->timemodified = $timenow; $DB->update_record($this->ltiuserstable, $ltiuserrecord); } else { // Validate uniqueness of the lti user, in the case of a stale object coming in to be saved. @@ -173,8 +181,17 @@ class user_repository { throw new \coding_exception("Cannot create duplicate LTI user '{$user->get_localid()}' for resource " . "'{$user->get_resourceid()}'."); } + + // Only update the Moodle user record if something has changed. $userid = $userrecord->id; - \user_update_user($userrecord); + $rawuser = \core_user::get_user($userid); + $userfieldstocompare = array_intersect_key( + (array) $rawuser, + (array) $userrecord + ); + if (!empty(array_diff((array) $userrecord, $userfieldstocompare))) { + \user_update_user($userrecord); + } unset($userrecord->id); // Create the lti_user record, holding details that have a lifespan equal to that of the enrolment instance. From c25857a73fdb23c50907f49a1253ff4fd56b8644 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Wed, 10 May 2023 10:02:38 +0800 Subject: [PATCH 6/6] MDL-76842 auth_lti: remove redundant conditional logic and cast --- auth/lti/auth.php | 4 ++-- auth/lti/tests/auth_test.php | 18 +++++++++--------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/auth/lti/auth.php b/auth/lti/auth.php index 9bf8975868e..c5fb73ebb03 100644 --- a/auth/lti/auth.php +++ b/auth/lti/auth.php @@ -181,7 +181,7 @@ class auth_plugin_lti extends \auth_plugin_base { unset($member['picture']); if ($binduser = $this->get_user_binding($iss, $member['user_id'])) { - $user = \core_user::get_user((int) $binduser); + $user = \core_user::get_user($binduser); $this->update_user_account($user, $member, $iss); return \core_user::get_user($user->id); } else { @@ -223,7 +223,7 @@ class auth_plugin_lti extends \auth_plugin_base { } if ($binduser = $this->get_user_binding($launchdata['iss'], $launchdata['sub'])) { - $user = \core_user::get_user((int) $binduser); + $user = \core_user::get_user($binduser); $this->update_user_account($user, $launchdata, $launchdata['iss']); return \core_user::get_user($user->id); } else { diff --git a/auth/lti/tests/auth_test.php b/auth/lti/tests/auth_test.php index 167aed38f9e..37e291fb7c3 100644 --- a/auth/lti/tests/auth_test.php +++ b/auth/lti/tests/auth_test.php @@ -220,7 +220,7 @@ class auth_test extends \advanced_testcase { $auth = get_auth_plugin('lti'); // When testing platform users who have authenticated before, make that first auth call. - if (!empty($launchdata['has_authenticated_before']) && $launchdata['has_authenticated_before']) { + if (!empty($launchdata['has_authenticated_before'])) { $mockjwtdata = $this->get_mock_launchdata_for_user($launchdata['user']); $firstauthuser = $auth->find_or_create_user_from_launch($mockjwtdata); } @@ -261,8 +261,8 @@ class auth_test extends \advanced_testcase { // Verify user count is correct. i.e. no user is created when migration claim is correctly processed or when // the user has authenticated with the tool before. - $numnewusers = (!empty($expected['migrated']) && $expected['migrated']) ? 0 : 1; - $numnewusers = (!empty($launchdata['has_authenticated_before']) && $launchdata['has_authenticated_before']) ? + $numnewusers = (!empty($expected['migrated'])) ? 0 : 1; + $numnewusers = (!empty($launchdata['has_authenticated_before'])) ? 0 : $numnewusers; $this->assertEquals($numnewusers, $countusersafter - $countusersbefore); @@ -294,11 +294,11 @@ class auth_test extends \advanced_testcase { } // Verify picture sync occurs, if expected. - if (!empty($expected['syncpicture']) && $expected['syncpicture']) { + if (!empty($expected['syncpicture'])) { $this->verify_user_profile_image_updated($user->id); } - if (!empty($expected['migrated']) && $expected['migrated']) { + if (!empty($expected['migrated'])) { // If migrated, verify the user account is reusing the legacy user account. $legacyuserids = array_column($legacyusers, 'id'); $this->assertContains($user->id, $legacyuserids); @@ -799,7 +799,7 @@ class auth_test extends \advanced_testcase { $auth = get_auth_plugin('lti'); // When testing platform users who have authenticated before, make that first auth call. - if (!empty($memberdata['has_authenticated_before']) && $memberdata['has_authenticated_before']) { + if (!empty($memberdata['has_authenticated_before'])) { $mockmemberdata = $this->get_mock_member_data_for_user($memberdata['user'], $memberdata['legacy_user_id'] ?? ''); $firstauthuser = $auth->find_or_create_user_from_membership($mockmemberdata, $iss, @@ -834,8 +834,8 @@ class auth_test extends \advanced_testcase { // Verify user count is correct. i.e. no user is created when migration claim is correctly processed or when // the user has authenticated with the tool before. - $numnewusers = (!empty($expected['migrated']) && $expected['migrated']) ? 0 : 1; - $numnewusers = (!empty($memberdata['has_authenticated_before']) && $memberdata['has_authenticated_before']) ? + $numnewusers = (!empty($expected['migrated'])) ? 0 : 1; + $numnewusers = (!empty($memberdata['has_authenticated_before'])) ? 0 : $numnewusers; $this->assertEquals($numnewusers, $countusersafter - $countusersbefore); @@ -866,7 +866,7 @@ class auth_test extends \advanced_testcase { break; } - if (!empty($expected['migrated']) && $expected['migrated']) { + if (!empty($expected['migrated'])) { // If migrated, verify the user account is reusing the legacy user account. $legacyuserids = array_column($legacyusers, 'id'); $this->assertContains($user->id, $legacyuserids);