mirror_php/moodle
1
0
Fork 0
mirror of https://github.com/moodle/moodle.git synced 2025-04-15 05:25:08 +02:00
Code Issues Projects Releases Wiki Activity

Merge branch 'MDL-76842-401' of https://github.com/snake/moodle into MOODLE_401_STABLE

Browse Source
This commit is contained in:
Jun Pataleta 2023-06-08 10:34:17 +08:00
commit c8bb7a6911
4 changed files with 130 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
auth/lti/auth.php
View File

@ -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);
}
}
@ -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 {

50
auth/lti/tests/auth_test.php
View File

@ -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
]
],
];
}

25
enrol/lti/classes/local/ltiadvantage/repository/user_repository.php
View File

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

75
enrol/lti/tests/local/ltiadvantage/repository/user_repository_test.php
View File

@ -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]);
}
/**