From fa61a759b3afc47abcc3b8a1dd4b0b6e68d7311b Mon Sep 17 00:00:00 2001 From: Lai Wei Date: Thu, 6 Aug 2026 16:11:21 +0100 Subject: [PATCH] fix OIDC login for manually matched users with mismatching Entra username Manually matched users are keyed by Entra UPN, which often differs from the Moodle username. handlelogin() found the matched user but then discarded it, continuing with the UPN as the username and failing with "User not found in Moodle." Now it binds to the matched user's actual username instead. Also normalizes entraidupn casing on write and read to avoid case-mismatch failures. --- auth/oidc/classes/loginflow/authcode.php | 10 +- auth/oidc/tests/loginflow/authcode_test.php | 146 ++++++++++++++++++++ local/o365/classes/page/acp.php | 15 +- 3 files changed, 165 insertions(+), 6 deletions(-) create mode 100644 auth/oidc/tests/loginflow/authcode_test.php diff --git a/auth/oidc/classes/loginflow/authcode.php b/auth/oidc/classes/loginflow/authcode.php index b12867dca..e79da2f63 100644 --- a/auth/oidc/classes/loginflow/authcode.php +++ b/auth/oidc/classes/loginflow/authcode.php @@ -574,7 +574,11 @@ protected function check_for_matched($entraidupn) { global $DB; if (auth_oidc_is_local_365_installed()) { - $match = $DB->get_record('local_o365_connections', ['entraidupn' => $entraidupn]); + $entraidupn = trim($entraidupn); + $sql = 'SELECT * + FROM {local_o365_connections} + WHERE ' . $DB->sql_equal('entraidupn', ':entraidupn', false); + $match = $DB->get_record_sql($sql, ['entraidupn' => $entraidupn]); if (!empty($match) && \local_o365\utils::is_o365_connected($match->muserid) !== true) { return $DB->get_record('user', ['id' => $match->muserid]); } @@ -850,6 +854,10 @@ protected function handlelogin(string $oidcuniqid, array $authparams, array $tok $matchedwith->entraidupn = $username; throw new moodle_exception('errorusermatched', 'auth_oidc', null, $matchedwith); } + // The matched Moodle user is already set to auth 'oidc': bind the login to that user's own + // username rather than the Microsoft-derived one, which may not match it (e.g. a manual match + // keyed on the full UPN while the Moodle username is only the UPN prefix). + $username = $matchedwith->username; } $username = trim(core_text::strtolower($username)); $tokenrec = $this->createtoken($oidcuniqid, $username, $authparams, $tokenparams, $idtoken, 0, $originalupn); diff --git a/auth/oidc/tests/loginflow/authcode_test.php b/auth/oidc/tests/loginflow/authcode_test.php new file mode 100644 index 000000000..d0caae076 --- /dev/null +++ b/auth/oidc/tests/loginflow/authcode_test.php @@ -0,0 +1,146 @@ +. + +namespace auth_oidc\loginflow; + +use advanced_testcase; +use auth_oidc\jwt; +use core\plugininfo\auth as auth_plugininfo; +use phpunit_util; + +/** + * Unit tests for the class \auth_oidc\loginflow\authcode. + * + * @package auth_oidc + * @copyright 2026 Enovation Solutions + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @group auth_oidc + * @group office365 + * @coversDefaultClass \auth_oidc\loginflow\authcode + */ +final class authcode_test extends advanced_testcase { + /** + * Set up test environment. + * + * @return void + */ + protected function setUp(): void { + parent::setUp(); + $this->resetAfterTest(true); + + auth_plugininfo::enable_plugin('oidc', 1); + set_config('bindingusernameclaim', 'upn', 'auth_oidc'); + } + + /** + * A manually matched user (auth already 'oidc', local_o365_connections keyed on the full Entra UPN, + * Moodle username only the UPN prefix) must complete on the very first OIDC login: the token must be + * bound to the matched Moodle user, not left dangling because the Moodle username differs from the UPN. + * + * Regression test for https://github.com/microsoft/o365-moodle/issues/2875. + * + * @return void + * @covers ::handlelogin + * @covers ::check_for_matched + */ + public function test_handlelogin_completes_manually_matched_user_with_differing_username(): void { + if (!auth_oidc_is_local_365_installed()) { + $this->markTestSkipped('This test requires local_o365 to be installed (local_o365_connections table).'); + } + + $this->assert_login_completes_for_matched_user('asmith@sc.school.edu.au', 'asmith@sc.school.edu.au'); + } + + /** + * A local_o365_connections row stored with mixed-case entraidupn (e.g. saved before UPNs were + * normalized to lower case, or entered that way by an admin) must still match a differently-cased + * UPN claim from the ID token, including on case-sensitive database collations such as PostgreSQL. + * + * @return void + * @covers ::check_for_matched + */ + public function test_handlelogin_completes_manually_matched_user_with_legacy_mixed_case_upn(): void { + if (!auth_oidc_is_local_365_installed()) { + $this->markTestSkipped('This test requires local_o365 to be installed (local_o365_connections table).'); + } + + $this->assert_login_completes_for_matched_user('ASmith@SC.School.edu.AU', 'asmith@sc.school.edu.au'); + } + + /** + * Creates a Moodle user manually matched to the given (as-stored) Entra UPN, drives handlelogin() with + * an ID token carrying the given (as-received) UPN claim, and asserts the login completed and bound + * the auth_oidc_token to the matched user. + * + * @param string $storedentraidupn The UPN as stored in local_o365_connections.entraidupn. + * @param string $tokenupn The UPN as received in the ID token's upn/preferred_username claims. + * @return void + */ + private function assert_login_completes_for_matched_user(string $storedentraidupn, string $tokenupn): void { + global $DB; + + $user = $this->getDataGenerator()->create_user(['username' => 'asmith', 'auth' => 'oidc']); + + // Simulate an admin manually matching the Moodle user to their Entra UPN via + // Manage User Connections, then flipping the account to auth 'oidc'. + $DB->insert_record('local_o365_connections', (object) [ + 'muserid' => $user->id, + 'entraidupn' => $storedentraidupn, + 'uselogin' => 0, + ]); + + $idtoken = new jwt(); + $idtoken->set_claims([ + 'sub' => 'sub-' . $user->id, + 'upn' => $tokenupn, + 'preferred_username' => $tokenupn, + ]); + + $oidcuniqid = 'oidcuniqid-' . $user->id; + $authparams = ['code' => 'authcode-' . $user->id]; + $tokenparams = [ + 'access_token' => 'access-token', + 'id_token' => 'id-token', + 'expires_in' => 3600, + 'resource' => 'resource', + 'scope' => 'scope', + ]; + + // The user_login() method validates the auth code against the current request, so it has to be + // available via optional_param() the same way it would be on a real OIDC callback request. + $_GET['code'] = $authparams['code']; + + try { + $loginflow = new authcode(); + phpunit_util::call_internal_method( + $loginflow, + 'handlelogin', + [$oidcuniqid, $authparams, $tokenparams, $idtoken], + authcode::class + ); + } finally { + unset($_GET['code']); + } + + $tokenrec = $DB->get_record('auth_oidc_token', ['oidcuniqid' => $oidcuniqid]); + $this->assertNotEmpty($tokenrec); + $this->assertEquals($user->id, $tokenrec->userid); + $this->assertEquals('asmith', $tokenrec->username); + + global $USER; + $this->assertEquals($user->id, $USER->id); + } +} diff --git a/local/o365/classes/page/acp.php b/local/o365/classes/page/acp.php index 089650bdc..7aa147f35 100644 --- a/local/o365/classes/page/acp.php +++ b/local/o365/classes/page/acp.php @@ -34,6 +34,7 @@ use core_course_category; use core_php_time_limit; use core_plugin_manager; +use core_text; use core_user; use finfo; use html_table; @@ -2077,11 +2078,15 @@ public function mode_userconnections_manualmatch() { $customdata = ['userid' => $userid]; $mform = new manualusermatch($redirect, $customdata); if ($fromform = $mform->get_data()) { - $o365username = trim($fromform->o365username); - - // Check existing matches for Microsoft user. - $existingmatchforo365user = $DB->get_record('local_o365_connections', ['entraidupn' => $o365username]); - if (!empty($existingmatchforo365user)) { + $o365username = core_text::strtolower(trim($fromform->o365username)); + + // Check existing matches for Microsoft user. Compared case-insensitively so legacy rows stored + // with different casing (e.g. before entraidupn values were normalized to lower case) are found + // too, rather than allowing a duplicate match to be created for the same Microsoft 365 user. + $sql = 'SELECT 1 + FROM {local_o365_connections} + WHERE ' . $DB->sql_equal('entraidupn', ':entraidupn', false); + if ($DB->record_exists_sql($sql, ['entraidupn' => $o365username])) { throw new moodle_exception('acp_userconnections_manualmatch_error_o365usermatched', 'local_o365'); }