L-electron-Rare/moodle
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge branch 'MDL-76842-master' of https://github.com/snake/moodle

Browse Source
This commit is contained in:
Jun Pataleta
2023-06-08 10:34:46 +08:00
parent 7aee844d05 c25857a73f
commit f46cc228a5
4 changed files with 141 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files
auth/lti/auth.php
+7 -4
View File
@@ -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 {
auth/lti/tests/auth_test.php
+48 -20
View File
@@ -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
]
],
];
}
enrol/lti/classes/local/ltiadvantage/repository/user_repository.php
+21 -4
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.
enrol/lti/tests/local/ltiadvantage/repository/user_repository_test.php
+65 -10
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]);
}
/**