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

Merge branch 'MDL-78219-403' of https://github.com/snake/moodle into MOODLE_403_STABLE

Browse Source
This commit is contained in:
Jun Pataleta
2024-01-23 11:59:26 +08:00
parent da9bbf5a96 631d94a872
commit d132858820
4 changed files with 259 additions and 104 deletions
Download Patch File Download Diff File Expand all files Collapse all files
auth/lti/auth.php
+30 -22
View File
@@ -110,8 +110,7 @@ class auth_plugin_lti extends \auth_plugin_base {
// The platform user is already linked with a user account.
if ($this->get_user_binding($launchdata['iss'], $launchdata['sub'])) {
// Always sync the PII, regardless of whether we're already authenticated as this user or not.
$user = $this->find_or_create_user_from_launch($launchdata, true);
$user = $this->find_or_create_user_from_launch($launchdata);
if (isloggedin()) {
// If a different user is currently logged in, authenticate the linked user instead.
@@ -121,11 +120,13 @@ class auth_plugin_lti extends \auth_plugin_base {
}
// If the linked user is already logged in, skip the call to complete_user_login() because this affects deep linking
// workflows on sites publishing and consuming resources on the same site, due to the regenerated sesskey.
return;
} else {
complete_user_login($user);
return;
}
// Always sync the PII, regardless of whether we're already authenticated as this user or not.
$this->update_user_account($user, $launchdata, $launchdata['iss']);
return;
}
// The platform user is not bound to a user account, check provisioning mode now.
@@ -136,7 +137,9 @@ class auth_plugin_lti extends \auth_plugin_base {
switch ($provisioningmode) {
case self::PROVISIONING_MODE_AUTO_ONLY:
// Automatic provisioning - this will create/migrate a user account and log the user in.
complete_user_login($this->find_or_create_user_from_launch($launchdata, true, $legacyconsumersecrets));
$user = $this->find_or_create_user_from_launch($launchdata, $legacyconsumersecrets);
complete_user_login($user);
$this->update_user_account($user, $launchdata, $launchdata['iss']);
break;
case self::PROVISIONING_MODE_PROMPT_NEW_EXISTING:
case self::PROVISIONING_MODE_PROMPT_EXISTING_ONLY:
@@ -211,21 +214,13 @@ class auth_plugin_lti extends \auth_plugin_base {
* itself and pass relevant data in - as auth_plugin_lti::complete_login() does.
*
* @param array $launchdata all data in the decoded JWT including iss and sub.
* @param bool $syncpicture whether to sync the user's picture with the picture sent in the launch.
* @param array $legacyconsumersecrets all secrets found for the legacy consumer, facilitating user migration.
* @return stdClass the Moodle user who is mapped to the platform user identified in the JWT data.
*/
public function find_or_create_user_from_launch(array $launchdata, bool $syncpicture = false,
array $legacyconsumersecrets = []): stdClass {
if (!$syncpicture) {
unset($launchdata['picture']);
}
public function find_or_create_user_from_launch(array $launchdata, array $legacyconsumersecrets = []): stdClass {
if ($binduser = $this->get_user_binding($launchdata['iss'], $launchdata['sub'])) {
$user = \core_user::get_user($binduser);
$this->update_user_account($user, $launchdata, $launchdata['iss']);
return \core_user::get_user($user->id);
return \core_user::get_user($binduser);
} else {
// Is the intent to migrate a user account used in legacy launches?
if (!empty($legacyconsumersecrets)) {
@@ -234,11 +229,10 @@ class auth_plugin_lti extends \auth_plugin_base {
$usermigrationclaim = new user_migration_claim($launchdata, $legacyconsumersecrets);
$username = 'enrol_lti' .
sha1($usermigrationclaim->get_consumer_key() . '::' .
$usermigrationclaim->get_consumer_key() .':' .$usermigrationclaim->get_user_id());
if ($user = \core_user::get_user_by_username($username)) {
$usermigrationclaim->get_consumer_key() . ':' . $usermigrationclaim->get_user_id());
if ($user = core_user::get_user_by_username($username)) {
$this->create_user_binding($launchdata['iss'], $launchdata['sub'], $user->id);
$this->update_user_account($user, $launchdata, $launchdata['iss']);
return \core_user::get_user($user->id);
return core_user::get_user($user->id);
}
} catch (Exception $e) {
// There was an issue validating the user migration claim. We don't want to fail auth entirely though.
@@ -247,9 +241,12 @@ class auth_plugin_lti extends \auth_plugin_base {
"'{$launchdata['iss']}'. The migration claim could not be validated. A new account will be created.");
}
}
// At the point of the creation, to ensure the user_created event correctly reflects the creating user of '0' (the user
// performing the action), ensure any active session is terminated and an empty session initialised.
$this->empty_session();
$user = $this->create_new_account($launchdata, $launchdata['iss']);
$this->update_user_account($user, $launchdata, $launchdata['iss']);
return \core_user::get_user($user->id);
return core_user::get_user($user->id);
}
}
@@ -301,6 +298,17 @@ class auth_plugin_lti extends \auth_plugin_base {
return $binduser;
}
/**
* If there's an existing session, inits an empty session.
*
* @return void
*/
protected function empty_session(): void {
if (isloggedin()) {
\core\session\manager::init_empty_session();
}
}
/**
* Check whether a provisioning mode is valid or not.
*
@@ -361,7 +369,7 @@ class auth_plugin_lti extends \auth_plugin_base {
* @param array $userdata the user data coming from either a launch or membership service call.
* @param string $iss the issuer to which the user belongs.
*/
protected function update_user_account(stdClass $user, array $userdata, string $iss): void {
public function update_user_account(stdClass $user, array $userdata, string $iss): void {
global $CFG;
require_once($CFG->dirroot.'/user/lib.php');
if ($user->auth !== 'lti') {
auth/lti/login.php
+2 -1
View File
@@ -72,8 +72,9 @@ if ($newaccount) {
} else {
// Create a new account and link it, logging the user in.
$auth = get_auth_plugin('lti');
$newuser = $auth->find_or_create_user_from_launch($launchdata, true);
$newuser = $auth->find_or_create_user_from_launch($launchdata);
complete_user_login($newuser);
$auth->update_user_account($newuser, $launchdata, $launchdata['iss']);
$PAGE->set_context(context_system::instance());
$PAGE->set_url(new moodle_url('/auth/lti/login.php'));
auth/lti/tests/auth_test.php
+226 -80
View File
@@ -214,7 +214,7 @@ class auth_test extends \advanced_testcase {
* @param array $expected the test case expectations.
* @covers ::find_or_create_user_from_launch
*/
public function test_find_or_create_user_from_launch(?array $legacydata, array $launchdata, array $expected) {
public function test_find_or_create_user_from_launch(?array $legacydata, array $launchdata, array $expected = []) {
$this->resetAfterTest();
global $DB;
$auth = get_auth_plugin('lti');
@@ -251,7 +251,7 @@ class auth_test extends \advanced_testcase {
// Authenticate the platform user.
$sink = $this->redirectEvents();
$countusersbefore = $DB->count_records('user');
$user = $auth->find_or_create_user_from_launch($mockjwtdata, true, $legacysecrets);
$user = $auth->find_or_create_user_from_launch($mockjwtdata, $legacysecrets);
if (!empty($expected['migration_debugging'])) {
$this->assertDebuggingCalled();
}
@@ -266,47 +266,15 @@ class auth_test extends \advanced_testcase {
0 : $numnewusers;
$this->assertEquals($numnewusers, $countusersafter - $countusersbefore);
// Verify PII is updated appropriately.
switch ($expected['PII']) {
case self::PII_ALL:
$this->assertEquals($launchdata['user']['given_name'], $user->firstname);
$this->assertEquals($launchdata['user']['family_name'], $user->lastname);
$this->assertEquals($launchdata['user']['email'], $user->email);
break;
case self::PII_NAMES_ONLY:
$this->assertEquals($launchdata['user']['given_name'], $user->firstname);
$this->assertEquals($launchdata['user']['family_name'], $user->lastname);
$email = 'enrol_lti_13_' . sha1($mockjwtdata['iss'] . '_' . $mockjwtdata['sub']) . "@example.com";
$this->assertEquals($email, $user->email);
break;
case self::PII_EMAILS_ONLY:
$this->assertEquals($mockjwtdata['iss'], $user->lastname);
$this->assertEquals($mockjwtdata['sub'], $user->firstname);
$this->assertEquals($launchdata['user']['email'], $user->email);
break;
default:
case self::PII_NONE:
$this->assertEquals($mockjwtdata['iss'], $user->lastname);
$this->assertEquals($mockjwtdata['sub'], $user->firstname);
$email = 'enrol_lti_13_' . sha1($mockjwtdata['iss'] . '_' . $mockjwtdata['sub']) . "@example.com";
$this->assertEquals($email, $user->email);
break;
}
// Verify picture sync occurs, if expected.
if (!empty($expected['syncpicture'])) {
$this->verify_user_profile_image_updated($user->id);
}
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);
$this->assertInstanceOf(\core\event\user_updated::class, $events[0]);
$this->assertEmpty($events); // No updates as part of this method.
} 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.
$this->assertEmpty($events); // No updates as part of this method.
} 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]);
@@ -329,9 +297,6 @@ class auth_test extends \advanced_testcase {
)[0],
'migration_claim' => null
],
'expected' => [
'PII' => self::PII_ALL,
]
],
'New (unlinked) platform learner excluding names, no legacy user, no migration claim' => [
'legacy_data' => null,
@@ -343,9 +308,6 @@ class auth_test extends \advanced_testcase {
)[0],
'migration_claim' => null
],
'expected' => [
'PII' => self::PII_EMAILS_ONLY,
]
],
'New (unlinked) platform learner excluding emails, no legacy user, no migration claim' => [
'legacy_data' => null,
@@ -358,9 +320,6 @@ class auth_test extends \advanced_testcase {
)[0],
'migration_claim' => null
],
'expected' => [
'PII' => self::PII_NAMES_ONLY,
]
],
'New (unlinked) platform learner excluding all PII, no legacy user, no migration claim' => [
'legacy_data' => null,
@@ -373,9 +332,6 @@ class auth_test extends \advanced_testcase {
)[0],
'migration_claim' => null
],
'expected' => [
'PII' => self::PII_NONE,
]
],
'New (unlinked) platform learner including PII, existing legacy user, valid migration claim' => [
'legacy_data' => [
@@ -403,7 +359,6 @@ class auth_test extends \advanced_testcase {
]
],
'expected' => [
'PII' => self::PII_ALL,
'migrated' => true
]
],
@@ -426,7 +381,6 @@ class auth_test extends \advanced_testcase {
'migration_claim' => null,
],
'expected' => [
'PII' => self::PII_ALL,
'migrated' => false,
]
],
@@ -455,7 +409,6 @@ class auth_test extends \advanced_testcase {
]
],
'expected' => [
'PII' => self::PII_ALL,
'migrated' => false,
'migration_debugging' => true,
]
@@ -486,7 +439,6 @@ class auth_test extends \advanced_testcase {
]
],
'expected' => [
'PII' => self::PII_ALL,
'migrated' => false,
]
],
@@ -516,7 +468,6 @@ class auth_test extends \advanced_testcase {
]
],
'expected' => [
'PII' => self::PII_ALL,
'migrated' => false
]
],
@@ -546,7 +497,6 @@ class auth_test extends \advanced_testcase {
]
],
'expected' => [
'PII' => self::PII_ALL,
'migrated' => true
]
],
@@ -576,7 +526,6 @@ class auth_test extends \advanced_testcase {
]
],
'expected' => [
'PII' => self::PII_ALL,
'migrated' => false,
'migration_debugging' => true,
]
@@ -605,7 +554,6 @@ class auth_test extends \advanced_testcase {
]
],
'expected' => [
'PII' => self::PII_ALL,
'migrated' => false
]
],
@@ -637,7 +585,6 @@ class auth_test extends \advanced_testcase {
]
],
'expected' => [
'PII' => self::PII_NONE,
'migrated' => true
]
],
@@ -650,9 +597,6 @@ class auth_test extends \advanced_testcase {
)[0],
'migration_claim' => null
],
'expected' => [
'PII' => self::PII_ALL,
]
],
'New (unlinked) platform instructor excluding PII, no legacy user, no migration claim' => [
'legacy_data' => null,
@@ -665,9 +609,6 @@ class auth_test extends \advanced_testcase {
)[0],
'migration_claim' => null
],
'expected' => [
'PII' => self::PII_NONE,
]
],
'New (unlinked) platform instructor including PII, existing legacy user, valid migration claim' => [
'legacy_data' => [
@@ -695,7 +636,6 @@ class auth_test extends \advanced_testcase {
]
],
'expected' => [
'PII' => self::PII_ALL,
'migrated' => true
]
],
@@ -709,9 +649,6 @@ class auth_test extends \advanced_testcase {
)[0],
'migration_claim' => null
],
'expected' => [
'PII' => self::PII_ALL,
]
],
'Existing (linked) platform learner excluding PII, no legacy user, no migration claim' => [
'legacy_data' => null,
@@ -725,9 +662,6 @@ class auth_test extends \advanced_testcase {
)[0],
'migration_claim' => null
],
'expected' => [
'PII' => self::PII_NONE,
]
],
'Existing (linked) platform instructor including PII, no legacy user, no migration claim' => [
'legacy_data' => null,
@@ -739,9 +673,6 @@ class auth_test extends \advanced_testcase {
)[0],
'migration_claim' => null
],
'expected' => [
'PII' => self::PII_ALL,
]
],
'Existing (linked) platform instructor excluding PII, no legacy user, no migration claim' => [
'legacy_data' => null,
@@ -755,9 +686,6 @@ class auth_test extends \advanced_testcase {
)[0],
'migration_claim' => null
],
'expected' => [
'PII' => self::PII_NONE,
]
],
'New (unlinked) platform instructor excluding PII, picture included' => [
'legacy_data' => null,
@@ -772,10 +700,6 @@ class auth_test extends \advanced_testcase {
)[0],
'migration_claim' => null
],
'expected' => [
'PII' => self::PII_NONE,
'syncpicture' => true
]
]
];
}
@@ -1154,4 +1078,226 @@ class auth_test extends \advanced_testcase {
// Assert idempotency of the bind call.
$this->assertNull($auth->create_user_binding($mockiss, $mocksub, $user->id));
}
/**
* Test updating a user account based on a given set of launchdata.
*
* @param array $firstlaunchdata the data from the first launch the user made.
* @param array $launchdata the current launch data, which will dictate what data is updated.
* @param array $expected array of test expectations
* @dataProvider update_user_account_provider
* @covers ::update_user_account
*/
public function test_update_user_account(array $firstlaunchdata, array $launchdata, array $expected): void {
$this->resetAfterTest();
$auth = get_auth_plugin('lti');
// Mock the first authentication of the user.
$firstmockjwtdata = $this->get_mock_launchdata_for_user($firstlaunchdata['user']);
$user = $auth->find_or_create_user_from_launch($firstmockjwtdata);
// Now, mock the recent authentication, confirming updates.
$mockjwtdata = $this->get_mock_launchdata_for_user($launchdata['user']);
$sink = $this->redirectEvents();
$auth->update_user_account($user, $mockjwtdata, $mockjwtdata['iss']);
$user = \core_user::get_user($user->id);
$events = $sink->get_events();
$sink->close();
if (!empty($expected['user_updated'])) {
$this->assertInstanceOf(\core\event\user_updated::class, $events[0]);
} else {
$this->assertEmpty($events);
}
// Verify PII is updated appropriately.
switch ($expected['PII']) {
case self::PII_ALL:
$this->assertEquals($launchdata['user']['given_name'], $user->firstname);
$this->assertEquals($launchdata['user']['family_name'], $user->lastname);
$this->assertEquals($launchdata['user']['email'], $user->email);
break;
case self::PII_NAMES_ONLY:
$this->assertEquals($launchdata['user']['given_name'], $user->firstname);
$this->assertEquals($launchdata['user']['family_name'], $user->lastname);
$email = 'enrol_lti_13_' . sha1($mockjwtdata['iss'] . '_' . $mockjwtdata['sub']) . "@example.com";
$this->assertEquals($email, $user->email);
break;
case self::PII_EMAILS_ONLY:
$this->assertEquals($mockjwtdata['iss'], $user->lastname);
$this->assertEquals($mockjwtdata['sub'], $user->firstname);
$this->assertEquals($launchdata['user']['email'], $user->email);
break;
default:
case self::PII_NONE:
$this->assertEquals($mockjwtdata['iss'], $user->lastname);
$this->assertEquals($mockjwtdata['sub'], $user->firstname);
$email = 'enrol_lti_13_' . sha1($mockjwtdata['iss'] . '_' . $mockjwtdata['sub']) . "@example.com";
$this->assertEquals($email, $user->email);
break;
}
// Verify picture sync occurs, if expected.
if (!empty($expected['picture_updated'])) {
$this->verify_user_profile_image_updated($user->id);
}
}
/**
* Data provider for testing user user_update_account.
*
* @return array the test case data.
*/
public function update_user_account_provider(): array {
return [
'Full PII included in both auths, no picture in either' => [
'first_launch_data' => [
'user' => $this->get_mock_users_with_ids(
['1'],
'http://purl.imsglobal.org/vocab/lis/v2/membership#Learner'
)[0]
],
'launch_data' => [
'user' => $this->get_mock_users_with_ids(
['1'],
'http://purl.imsglobal.org/vocab/lis/v2/membership#Learner'
)[0],
],
'expected' => [
'PII' => self::PII_ALL,
'user_updated' => false,
'picture_updated' => false
]
],
'No PII included in both auths, no picture in either' => [
'first_launch_data' => [
'user' => $this->get_mock_users_with_ids(
['1'],
'http://purl.imsglobal.org/vocab/lis/v2/membership#Learner',
false,
false
)[0]
],
'launch_data' => [
'user' => $this->get_mock_users_with_ids(
['1'],
'http://purl.imsglobal.org/vocab/lis/v2/membership#Learner',
false,
false
)[0],
],
'expected' => [
'PII' => self::PII_NONE,
'user_updated' => false,
'picture_updated' => false
]
],
'First auth no PII, second auth including PII, no picture in either' => [
'first_launch_data' => [
'user' => $this->get_mock_users_with_ids(
['1'],
'http://purl.imsglobal.org/vocab/lis/v2/membership#Learner',
false,
false
)[0]
],
'launch_data' => [
'user' => $this->get_mock_users_with_ids(
['1'],
'http://purl.imsglobal.org/vocab/lis/v2/membership#Learner'
)[0],
],
'expected' => [
'PII' => self::PII_ALL,
'user_updated' => true,
'picture_updated' => false
]
],
'First auth full PII, second auth no PII, no picture in either' => [
'first_launch_data' => [
'user' => $this->get_mock_users_with_ids(
['1'],
'http://purl.imsglobal.org/vocab/lis/v2/membership#Learner',
)[0]
],
'launch_data' => [
'user' => $this->get_mock_users_with_ids(
['1'],
'http://purl.imsglobal.org/vocab/lis/v2/membership#Learner',
false,
false
)[0],
],
'expected' => [
'PII' => self::PII_NONE,
'user_updated' => true,
'picture_updated' => false
]
],
'First auth full PII, second auth emails only, no picture in either' => [
'first_launch_data' => [
'user' => $this->get_mock_users_with_ids(
['1'],
'http://purl.imsglobal.org/vocab/lis/v2/membership#Learner',
)[0]
],
'launch_data' => [
'user' => $this->get_mock_users_with_ids(
['1'],
'http://purl.imsglobal.org/vocab/lis/v2/membership#Learner',
false
)[0],
],
'expected' => [
'PII' => self::PII_EMAILS_ONLY,
'user_updated' => true,
'picture_updated' => false
]
],
'First auth full PII, second auth names only, no picture in either' => [
'first_launch_data' => [
'user' => $this->get_mock_users_with_ids(
['1'],
'http://purl.imsglobal.org/vocab/lis/v2/membership#Learner',
)[0]
],
'launch_data' => [
'user' => $this->get_mock_users_with_ids(
['1'],
'http://purl.imsglobal.org/vocab/lis/v2/membership#Learner',
true,
false
)[0],
],
'expected' => [
'PII' => self::PII_NAMES_ONLY,
'user_updated' => true,
'picture_updated' => false
]
],
'Full PII included in both auths, picture included in the second auth' => [
'first_launch_data' => [
'user' => $this->get_mock_users_with_ids(
['1'],
'http://purl.imsglobal.org/vocab/lis/v2/membership#Learner'
)[0]
],
'launch_data' => [
'user' => $this->get_mock_users_with_ids(
['1'],
'http://purl.imsglobal.org/vocab/lis/v2/membership#Learner',
true,
true,
true
)[0],
],
'expected' => [
'PII' => self::PII_ALL,
'user_updated' => false,
'picture_updated' => false
]
],
];
}
}
enrol/lti/tests/local/ltiadvantage/lti_advantage_testcase.php
+1 -1
View File
@@ -89,7 +89,7 @@ abstract class lti_advantage_testcase extends \advanced_testcase {
}
$secrets = !empty($migrationclaiminfo['signing_secret']) ? [$migrationclaiminfo['signing_secret']] : [];
return $auth->find_or_create_user_from_launch($mockjwt, false, $secrets);
return $auth->find_or_create_user_from_launch($mockjwt, $secrets);
}
/**