diff --git a/public/admin/tool/mfa/classes/manager.php b/public/admin/tool/mfa/classes/manager.php index 74e9ca7216a..df1a809dddd 100644 --- a/public/admin/tool/mfa/classes/manager.php +++ b/public/admin/tool/mfa/classes/manager.php @@ -598,6 +598,10 @@ class manager { $urls = [ new \moodle_url('/login/logout.php'), new \moodle_url('/admin/tool/mfa/guide.php'), + // Allow email self-registration confirmation to complete so that + // auth_email can restore wantsurl from the auth_email_wantsurl user + // preference before MFA intercepts on the next request. + new \moodle_url('/login/confirm.php'), ]; foreach ($factors as $factor) { $urls = array_merge($urls, $factor->get_no_redirect_urls()); diff --git a/public/admin/tool/mfa/tests/manager_test.php b/public/admin/tool/mfa/tests/manager_test.php index 16e97565198..674e61b38a6 100644 --- a/public/admin/tool/mfa/tests/manager_test.php +++ b/public/admin/tool/mfa/tests/manager_test.php @@ -437,6 +437,30 @@ final class manager_test extends \advanced_testcase { $this->assertTrue($SESSION->mfa_login_hook_test); } + /** + * Tests that /login/confirm.php is excluded from MFA redirection. + * + * When a user follows an email self-registration confirmation link, MFA must + * not intercept the request before auth_email::user_confirm() has had a chance + * to restore the wantsurl from the auth_email_wantsurl user preference. + * + * @covers ::should_require_mfa + * @covers ::get_no_redirect_urls + */ + public function test_confirm_url_no_redirect(): void { + $this->resetAfterTest(true); + $user = $this->getDataGenerator()->create_user(); + $this->setUser($user); + + $confirmurl = new \moodle_url('/login/confirm.php'); + $this->assertEquals( + \tool_mfa\manager::NO_REDIRECT, + \tool_mfa\manager::should_require_mfa($confirmurl, false), + '/login/confirm.php must not trigger an MFA redirect so that auth_email can ' . + 'restore wantsurl from the auth_email_wantsurl user preference first.' + ); + } + /** * Tests circular redirect auth * diff --git a/public/auth/email/auth.php b/public/auth/email/auth.php index 95e434e5a9e..580d702ea5c 100644 --- a/public/auth/email/auth.php +++ b/public/auth/email/auth.php @@ -160,7 +160,7 @@ class auth_plugin_email extends auth_plugin_base { * @param string $confirmsecret */ function user_confirm($username, $confirmsecret) { - global $DB, $SESSION; + global $DB; $user = get_complete_user_data('username', $username); if (!empty($user)) { @@ -168,17 +168,15 @@ class auth_plugin_email extends auth_plugin_base { return AUTH_CONFIRM_ERROR; } else if ($user->secret === $confirmsecret && $user->confirmed) { + // Clean up stale wantsurl preference if user clicks confirmation link again. + unset_user_preference('auth_email_wantsurl', $user); return AUTH_CONFIRM_ALREADY; } else if ($user->secret === $confirmsecret) { // They have provided the secret key to get in $DB->set_field("user", "confirmed", 1, array("id"=>$user->id)); - - if ($wantsurl = get_user_preferences('auth_email_wantsurl', false, $user)) { - // Ensure user gets returned to page they were trying to access before signing up. - $SESSION->wantsurl = $wantsurl; - unset_user_preference('auth_email_wantsurl', $user); - } - + // Clean up the wantsurl preference regardless of how confirmation was triggered + // (e.g. /login/confirm.php, admin single confirm, bulk confirm, web service). + unset_user_preference('auth_email_wantsurl', $user); return AUTH_CONFIRM_OK; } } else { diff --git a/public/auth/email/tests/auth_test.php b/public/auth/email/tests/auth_test.php new file mode 100644 index 00000000000..0e69a1333f9 --- /dev/null +++ b/public/auth/email/tests/auth_test.php @@ -0,0 +1,96 @@ +. + +namespace auth_email; + +/** + * Tests for email authentication plugin. + * + * @package auth_email + * @copyright 2026 Moodle Pty Ltd + * @license http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later + * @covers \auth_plugin_email + */ +final class auth_test extends \advanced_testcase { + /** + * Test that user_confirm() cleans up the auth_email_wantsurl preference + * when confirming a user for the first time (AUTH_CONFIRM_OK). + */ + public function test_user_confirm_cleans_up_wantsurl_preference(): void { + global $DB; + $this->resetAfterTest(true); + + // Create an unconfirmed user with the email auth method. + $user = $this->getDataGenerator()->create_user([ + 'auth' => 'email', + 'confirmed' => 0, + ]); + $secret = random_string(15); + $DB->set_field('user', 'secret', $secret, ['id' => $user->id]); + + // Simulate the wantsurl preference saved at signup time. + set_user_preference('auth_email_wantsurl', 'https://example.com/course/view.php?id=42', $user); + $this->assertTrue( + $DB->record_exists('user_preferences', ['userid' => $user->id, 'name' => 'auth_email_wantsurl']), + 'Preference should exist in DB before confirmation.' + ); + + $auth = get_auth_plugin('email'); + $result = $auth->user_confirm($user->username, $secret); + + $this->assertEquals(AUTH_CONFIRM_OK, $result); + $this->assertFalse( + $DB->record_exists('user_preferences', ['userid' => $user->id, 'name' => 'auth_email_wantsurl']), + 'auth_email_wantsurl preference must be removed from DB after successful confirmation.' + ); + } + + /** + * Test that user_confirm() cleans up the auth_email_wantsurl preference + * even when the user is already confirmed (AUTH_CONFIRM_ALREADY). + * + * This covers the edge case where a user clicks the confirmation link + * a second time — the stale preference should still be cleaned up. + */ + public function test_user_confirm_already_confirmed_cleans_up_wantsurl_preference(): void { + global $DB; + $this->resetAfterTest(true); + + // Create an already-confirmed user with the email auth method. + $user = $this->getDataGenerator()->create_user([ + 'auth' => 'email', + 'confirmed' => 1, + ]); + $secret = random_string(15); + $DB->set_field('user', 'secret', $secret, ['id' => $user->id]); + + // Simulate a stale wantsurl preference left over from signup. + set_user_preference('auth_email_wantsurl', 'https://example.com/course/view.php?id=42', $user); + $this->assertTrue( + $DB->record_exists('user_preferences', ['userid' => $user->id, 'name' => 'auth_email_wantsurl']), + 'Preference should exist in DB before re-confirmation.' + ); + + $auth = get_auth_plugin('email'); + $result = $auth->user_confirm($user->username, $secret); + + $this->assertEquals(AUTH_CONFIRM_ALREADY, $result); + $this->assertFalse( + $DB->record_exists('user_preferences', ['userid' => $user->id, 'name' => 'auth_email_wantsurl']), + 'auth_email_wantsurl preference must be removed from DB even when user is already confirmed.' + ); + } +} diff --git a/public/login/confirm.php b/public/login/confirm.php index fb85f91d47d..977d5995584 100644 --- a/public/login/confirm.php +++ b/public/login/confirm.php @@ -52,6 +52,11 @@ if (!empty($data) || (!empty($p) && !empty($s))) { $username = $s; } + // Read auth_email_wantsurl before user_confirm() cleans it up, so we can + // restore it into the session after complete_user_login() regenerates the session. + $earlyuser = get_complete_user_data('username', $username); + $emailwantsurl = $earlyuser ? get_user_preferences('auth_email_wantsurl', false, $earlyuser) : false; + $confirmed = $authplugin->user_confirm($username, $usersecret); if ($confirmed == AUTH_CONFIRM_ALREADY) { @@ -80,6 +85,14 @@ if (!empty($data) || (!empty($p) && !empty($s))) { \core\session\manager::apply_concurrent_login_limit($user->id, session_id()); + // Restore the originally requested URL saved at signup time. + // This must happen after complete_user_login() because session regeneration during login + // destroys any $SESSION data set before that point. The preference was already cleaned + // up by user_confirm(), so we use the value read before calling it. + if ($emailwantsurl) { + $SESSION->wantsurl = $emailwantsurl; + } + // Check where to go, $redirect has a higher preference. if (!empty($redirect)) { if (!empty($SESSION->wantsurl)) { @@ -96,7 +109,15 @@ if (!empty($data) || (!empty($p) && !empty($s))) { echo $OUTPUT->box_start('generalbox centerpara boxwidthnormal boxaligncenter'); echo "

".get_string("thanks").", ". fullname($USER) . "

\n"; echo "

".get_string("confirmed")."

\n"; - echo $OUTPUT->single_button(core_login_get_return_url(), get_string('continue')); + // Calling core_login_get_return_url() consumes $SESSION->wantsurl; restore it so + // that MFA intercepting the next page load can still redirect the user correctly. + // Skip restore when profile completion is required — core_login_get_return_url() + // intentionally preserves $SESSION->wantsurl in that case so it survives past user/edit.php. + $returnurl = core_login_get_return_url(); + if (!user_not_fully_set_up($USER, true)) { + $SESSION->wantsurl = $returnurl; + } + echo $OUTPUT->single_button($returnurl, get_string('continue')); echo $OUTPUT->box_end(); echo $OUTPUT->footer(); exit;