diff --git a/auth/lti/auth.php b/auth/lti/auth.php index 0757570b5ba..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 { @@ -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); } } @@ -224,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 { @@ -381,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 { diff --git a/auth/lti/tests/auth_test.php b/auth/lti/tests/auth_test.php index 8690f555565..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); } @@ -249,17 +249,20 @@ 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. - $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); @@ -291,19 +294,22 @@ 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 migrated, verify the user account is reusing the legacy user account. - 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); - } - - // 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]); } } @@ -793,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, @@ -819,14 +825,17 @@ 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. - $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); @@ -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 (!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); - } - - // 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 + ] + ], ]; } 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. 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]); } /**