From a58f0045455e929d833f0ea9a25215b877f82d54 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Mon, 15 May 2023 09:48:43 +0800 Subject: [PATCH 1/3] MDL-78219 auth_lti: force empty session before launch user creation If an existing session is active at the time of launch, that user id appears in the user_created log, incorrectly (it should be system/0). This patch makes sure that any existing session is terminated before creation of a new user during launches. --- auth/lti/auth.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/auth/lti/auth.php b/auth/lti/auth.php index c5fb73ebb03..d68f3eb1b77 100644 --- a/auth/lti/auth.php +++ b/auth/lti/auth.php @@ -247,6 +247,10 @@ 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); @@ -301,6 +305,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. * From 796935efc2e878c7f419bdaeae8b6a63e6822071 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Mon, 15 May 2023 12:07:27 +0800 Subject: [PATCH 2/3] MDL-78219 auth_lti: ensure user updates happen after authentication If an existing session is active at the time of launch, that user id appears in the user_updated log. This log should show the launching user updating their own record, not someone else. To resolve this, auth the user before making any record updates so that the event gets the correct user id. --- auth/lti/auth.php | 37 ++--- auth/lti/login.php | 3 +- auth/lti/tests/auth_test.php | 306 ++++++++++++++++++++++++++--------- 3 files changed, 243 insertions(+), 103 deletions(-) diff --git a/auth/lti/auth.php b/auth/lti/auth.php index d68f3eb1b77..0b60dfc24a9 100644 --- a/auth/lti/auth.php +++ b/auth/lti/auth.php @@ -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. @@ -252,8 +246,7 @@ class auth_plugin_lti extends \auth_plugin_base { $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); } } @@ -376,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') { diff --git a/auth/lti/login.php b/auth/lti/login.php index 39fd2d32ca9..41084332ca4 100644 --- a/auth/lti/login.php +++ b/auth/lti/login.php @@ -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')); diff --git a/auth/lti/tests/auth_test.php b/auth/lti/tests/auth_test.php index 37e291fb7c3..2ac87189f57 100644 --- a/auth/lti/tests/auth_test.php +++ b/auth/lti/tests/auth_test.php @@ -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 + ] + ], + ]; + } + } From 631d94a8720d2a11bf92624d018fa2ae565603f5 Mon Sep 17 00:00:00 2001 From: Jake Dallimore Date: Mon, 15 May 2023 12:11:54 +0800 Subject: [PATCH 3/3] MDL-78219 enrol_lti: fix auth method usage in base testcase --- enrol/lti/tests/local/ltiadvantage/lti_advantage_testcase.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/enrol/lti/tests/local/ltiadvantage/lti_advantage_testcase.php b/enrol/lti/tests/local/ltiadvantage/lti_advantage_testcase.php index ca844c26a85..bd0c59b5dbd 100644 --- a/enrol/lti/tests/local/ltiadvantage/lti_advantage_testcase.php +++ b/enrol/lti/tests/local/ltiadvantage/lti_advantage_testcase.php @@ -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); } /**