diff --git a/auth/lti/auth.php b/auth/lti/auth.php index c5fb73ebb03..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. @@ -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') { 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 + ] + ], + ]; + } + } 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); } /**