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/exemption b/factor/exemption index abc74c4f..b9245161 160000 --- a/factor/exemption +++ b/factor/exemption @@ -1 +1 @@ -Subproject commit abc74c4f1837ae09db229b00bf5c9dc9a63af51b +Subproject commit b9245161085ca509ec85dc237c99459c2c8e392c diff --git a/factor/grace/classes/factor.php b/factor/grace/classes/factor.php index 97724e24..5d97b4a3 100644 --- a/factor/grace/classes/factor.php +++ b/factor/grace/classes/factor.php @@ -110,10 +110,11 @@ 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. - 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 +125,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 +309,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. + $noredirectlist = get_config('factor_grace', 'noredirectlist'); + if (!$noredirectlist) { + return false; + } + + $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() + && $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')); 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.'); + } }