From 1656e8ad4045a6f51899a7d8cc42d6edece73ddd Mon Sep 17 00:00:00 2001 From: Kevin Pham Date: Mon, 27 Nov 2023 16:46:55 +1100 Subject: [PATCH 1/3] feat: add a way to prevent gracemode redirects if user has passed certain factors --- classes/local/factor/object_factor_base.php | 9 ++++++ classes/manager.php | 8 ++--- factor/grace/classes/factor.php | 35 ++++++++++++++++++++- factor/grace/lang/en/factor_grace.php | 2 ++ factor/grace/settings.php | 9 ++++++ 5 files changed, 58 insertions(+), 5 deletions(-) diff --git a/classes/local/factor/object_factor_base.php b/classes/local/factor/object_factor_base.php index 6152ca21..b9296dc4 100644 --- a/classes/local/factor/object_factor_base.php +++ b/classes/local/factor/object_factor_base.php @@ -635,4 +635,13 @@ public function global_validation($data, $files): array { public function global_submit($data) { return; } + + /** + * Returns whether or not the factor has passed + * + * @return bool + */ + public function passed(): bool { + return $this->get_state() == \tool_mfa\plugininfo\factor::STATE_PASS; + } } diff --git a/classes/manager.php b/classes/manager.php index b61d6afd..937cf32f 100644 --- a/classes/manager.php +++ b/classes/manager.php @@ -81,7 +81,7 @@ public static function display_debug_notification() { // Stop adding weight if 100 achieved. if (!$weighttoggle) { - $achieved = $factor->get_state() == \tool_mfa\plugininfo\factor::STATE_PASS ? $factor->get_weight() : 0; + $achieved = $factor->passed() ? $factor->get_weight() : 0; $achieved = '+'.$achieved; $runningtotal += $achieved; } else { @@ -147,7 +147,7 @@ public static function get_total_weight() { $factors = \tool_mfa\plugininfo\factor::get_active_user_factor_types(); foreach ($factors as $factor) { - if ($factor->get_state() == \tool_mfa\plugininfo\factor::STATE_PASS) { + if ($factor->passed()) { $totalweight += $factor->get_weight(); } } @@ -818,7 +818,7 @@ public static function get_cumulative_weight() { $factors = \tool_mfa\plugininfo\factor::get_active_user_factor_types(); $totalweight = 0; foreach ($factors as $factor) { - if ($factor->get_state() == \tool_mfa\plugininfo\factor::STATE_PASS) { + if ($factor->passed()) { $totalweight += $factor->get_weight(); // If over 100, break. Dont care about >100. if ($totalweight >= 100) { @@ -851,7 +851,7 @@ public static function check_factor_pending($factorname) { continue; } - if ($factor->get_state() == \tool_mfa\plugininfo\factor::STATE_PASS) { + if ($factor->passed()) { $totalweight += $factor->get_weight(); if ($totalweight >= 100) { $weighttoggle = true; diff --git a/factor/grace/classes/factor.php b/factor/grace/classes/factor.php index 97724e24..48ab128c 100644 --- a/factor/grace/classes/factor.php +++ b/factor/grace/classes/factor.php @@ -113,7 +113,7 @@ public function get_state($redirectable = true) { // If gracemode would have given points, but now doesnt, // Jump out of the loop and force a factor setup. // We will return once there is a setup, or the user tries to leave. - if (get_config('factor_grace', 'forcesetup') && $redirectable) { + if ($redirectable && get_config('factor_grace', 'forcesetup')) { if (empty($SESSION->mfa_gracemode_recursive)) { // Set a gracemode lock so any further recursive gets fall past any recursive calls. $SESSION->mfa_gracemode_recursive = true; @@ -124,9 +124,15 @@ public function get_state($redirectable = true) { foreach ($factorurls as $factorurl) { if ($factorurl->compare($cleanurl)) { $redirectable = false; + break; } } + // Checking if there are contributing factors, that should not cause a redirect. + if ($redirectable && $this->should_skip_force_setup()) { + $redirectable = false; + } + // We should never redirect if we have already passed. if ($redirectable && \tool_mfa\manager::get_cumulative_weight() >= 100) { $redirectable = false; @@ -302,4 +308,31 @@ public function get_affecting_factors(): array { }); return $factors; } + + /** + * Returns whether or not the force setup of MFA is skippable + * + * @return bool + */ + private function should_skip_force_setup(): bool { + // Checking if there are contributing factors, that should not cause a redirect. + $active = \tool_mfa\plugininfo\factor::get_active_user_factor_types(); + $noredirectlist = get_config('factor_grace', 'noredirectlist'); + if (!$noredirectlist) { + return false; + } + + $values = explode(',', $noredirectlist); + $keys = array_flip($values); + + foreach ($active as $factor) { + if (isset($keys[$factor->name]) + && $factor->passed() + && $factor->get_weight() > 0) { + return true; + } + } + + return false; + } } diff --git a/factor/grace/lang/en/factor_grace.php b/factor/grace/lang/en/factor_grace.php index c2f2f7d0..568995b9 100644 --- a/factor/grace/lang/en/factor_grace.php +++ b/factor/grace/lang/en/factor_grace.php @@ -33,6 +33,8 @@ $string['settings:customwarning_help'] = 'Add content here to replace the grace warning notification with custom HTML contents. Adding {timeremaining} in text will replace it with the current grace duration for the user, and {setuplink} will replace with the URL of the setup page for the user.'; $string['settings:forcesetup'] = 'Force factor setup'; $string['settings:forcesetup_help'] = 'Forces a user to the preferences page to setup MFA when the gracemode period expires. If set to off, users will be unable to authenticate when the grace period expires.'; +$string['settings:noredirectlist'] = 'Force factor skip list'; +$string['settings:noredirectlist_help'] = 'In cases where certain users DO NOT need to set up MFA. If any selected factors contribute points, grace mode will NOT force factor setup.'; $string['settings:graceperiod'] = 'Grace period'; $string['settings:graceperiod_help'] = 'Period of time when users can access Moodle without configured and enabled factors'; $string['settings:ignorelist'] = 'Ignored factors'; diff --git a/factor/grace/settings.php b/factor/grace/settings.php index eb1448bd..6e41c1a8 100644 --- a/factor/grace/settings.php +++ b/factor/grace/settings.php @@ -41,6 +41,15 @@ new lang_string('settings:forcesetup', 'factor_grace'), new lang_string('settings:forcesetup_help', 'factor_grace'), 0)); +$allfactors = \tool_mfa\plugininfo\factor::get_factors(); +$noredirectlistfactors = []; +foreach ($allfactors as $factor) { + $noredirectlistfactors[$factor->name] = $factor->get_display_name(); +} +$settings->add(new admin_setting_configmultiselect('factor_grace/noredirectlist', + new lang_string('settings:noredirectlist', 'factor_grace'), + new lang_string('settings:noredirectlist_help', 'factor_grace'), [], $noredirectlistfactors)); + $settings->add(new admin_setting_configduration('factor_grace/graceperiod', new lang_string('settings:graceperiod', 'factor_grace'), new lang_string('settings:graceperiod_help', 'factor_grace'), '604800')); From 271c64043e19a9abab466063a83a524d877f4ca6 Mon Sep 17 00:00:00 2001 From: Kevin Pham Date: Tue, 28 Nov 2023 14:16:21 +1100 Subject: [PATCH 2/3] test: add unit tests for grace mode redirects handling --- factor/grace/classes/factor.php | 3 +- factor/grace/tests/factor_test.php | 70 ++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/factor/grace/classes/factor.php b/factor/grace/classes/factor.php index 48ab128c..5d97b4a3 100644 --- a/factor/grace/classes/factor.php +++ b/factor/grace/classes/factor.php @@ -110,6 +110,7 @@ public function get_state($redirectable = true) { if (!empty($duration)) { if (time() > $starttime + $duration) { + // If gracemode would have given points, but now doesnt, // Jump out of the loop and force a factor setup. // We will return once there is a setup, or the user tries to leave. @@ -316,7 +317,6 @@ public function get_affecting_factors(): array { */ private function should_skip_force_setup(): bool { // Checking if there are contributing factors, that should not cause a redirect. - $active = \tool_mfa\plugininfo\factor::get_active_user_factor_types(); $noredirectlist = get_config('factor_grace', 'noredirectlist'); if (!$noredirectlist) { return false; @@ -325,6 +325,7 @@ private function should_skip_force_setup(): bool { $values = explode(',', $noredirectlist); $keys = array_flip($values); + $active = \tool_mfa\plugininfo\factor::get_active_user_factor_types(); foreach ($active as $factor) { if (isset($keys[$factor->name]) && $factor->passed() diff --git a/factor/grace/tests/factor_test.php b/factor/grace/tests/factor_test.php index f5d8b86a..2ea89ca6 100644 --- a/factor/grace/tests/factor_test.php +++ b/factor/grace/tests/factor_test.php @@ -55,4 +55,74 @@ public function test_affecting_factors() { $affecting = $grace->get_affecting_factors(); $this->assertEquals(0, count($affecting)); } + + /** + * Test factors leading to a redirect. + */ + public function test_redirect_factors() { + $this->resetAfterTest(true); + $user = $this->getDataGenerator()->create_user(); + $this->setUser($user); + + $grace = \tool_mfa\plugininfo\factor::get_factor('grace'); + + set_config('enabled', 1, 'factor_totp'); + set_config('enabled', 1, 'factor_grace'); + set_config('forcesetup', 1, 'factor_grace'); + set_config('graceperiod', -1, 'factor_grace'); // Grace period expired. + + // Set up exemption factor for a person. + set_config('duration', 100, 'factor_exemption'); + \factor_exemption\factor::add_exemption($user); + + $redirected = false; + try { + $grace->get_state(true); + } catch (\Throwable $e) { + $expected = get_string('redirecterrordetected', 'error'); + if ($expected === $e->getMessage()) { + $redirected = true; + } + } + + $this->assertTrue($redirected, 'No redirect detected, but was expected.'); + } + + /** + * Test factors leading to a redirect, but avoiding it + */ + public function test_gracemode_expires_noredirect() { + $this->resetAfterTest(true); + $user = $this->getDataGenerator()->create_user(); + $this->setUser($user); + + $grace = \tool_mfa\plugininfo\factor::get_factor('grace'); + $totp = \tool_mfa\plugininfo\factor::get_factor('totp'); + $exemption = \tool_mfa\plugininfo\factor::get_factor('exemption'); + + set_config('enabled', 1, 'factor_totp'); + set_config('enabled', 1, 'factor_exemption'); + set_config('enabled', 1, 'factor_grace'); + set_config('forcesetup', 1, 'factor_grace'); + set_config('graceperiod', -1, 'factor_grace'); // Grace period expired. + + // Set up exemption factor for a person. + set_config('duration', 100, 'factor_exemption'); + \factor_exemption\factor::add_exemption($user); + + // Set exemption as a factor that should prevent redirects. + set_config('noredirectlist', 'exemption', 'factor_grace'); + + $redirected = false; + try { + $grace->get_state(true); + } catch (\Throwable $e) { + $expected = get_string('redirecterrordetected', 'error'); + if ($expected === $e->getMessage()) { + $redirected = true; + } + } + + $this->assertFalse($redirected, 'The function cause a redirect, where none was expected.'); + } } From dd631162dba34aab9eb867c9a609a37532e7d361 Mon Sep 17 00:00:00 2001 From: Kevin Pham Date: Tue, 28 Nov 2023 14:29:58 +1100 Subject: [PATCH 3/3] build: bump exemption factor --- factor/exemption | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/factor/exemption b/factor/exemption index abc74c4f..b9245161 160000 --- a/factor/exemption +++ b/factor/exemption @@ -1 +1 @@ -Subproject commit abc74c4f1837ae09db229b00bf5c9dc9a63af51b +Subproject commit b9245161085ca509ec85dc237c99459c2c8e392c