Merge pull request catalyst#449 from catalyst/grace-skip-force-factor

grace skip force factor

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;
+    }
 }

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;

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;
+    }
 }

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';

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'));

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.');
+    }
 }