Merge pull request #454 from catalyst/453-grace-mode-unknown-factors-fix

[#453] Ignore unknown factors when calculating passing score in grace…

Author: Peterburnett

factor/grace/classes/factor.php

@@ -135,7 +135,7 @@ public function get_state($redirectable = true) {
                             }
 
                             // We should never redirect if we have already passed.
-                            if ($redirectable && \tool_mfa\manager::get_cumulative_weight() >= 100) {
+                            if ($redirectable && \tool_mfa\manager::get_total_weight() >= 100) {
                                 $redirectable = false;
                             }
 

factor/grace/tests/factor_test.php

@@ -23,6 +23,7 @@
  * @author      Peter Burnett <[email protected]>
  * @copyright   Catalyst IT
  * @license     http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ * @covers      \factor_grace
  */
 class factor_test extends \advanced_testcase {
 
@@ -125,4 +126,59 @@ public function test_gracemode_expires_noredirect() {
 
         $this->assertFalse($redirected, 'The function cause a redirect, where none was expected.');
     }
+
+    /**
+     * Tests that gracemode correctly calculates passing weight even when factors can return UNKNOWN
+     */
+    public function test_gracemode_unknown_factor_handling(): void {
+        $this->resetAfterTest(true);
+        $user1 = $this->getDataGenerator()->create_user();
+        $user2 = $this->getDataGenerator()->create_user();
+        $grace = \tool_mfa\plugininfo\factor::get_factor('grace');
+
+        set_config('enabled', 1, 'factor_totp');
+        set_config('enabled', 1, 'factor_exemption');
+        set_config('enabled', 1, 'factor_grace');
+        set_config('enabled', 1, 'factor_loginbanner');
+        set_config('forcesetup', 1, 'factor_grace');
+        set_config('graceperiod', -1, 'factor_grace'); // Grace period expired.
+
+        // Setup the order so that the loginbanner is at the top.
+        set_config('factor_order', 'loginbanner,exemption,totp,grace');
+
+        // Login banner should give no points.
+        set_config('weight', 0, 'factor_loginbanner');
+
+        // Add exemption for user 1, but NOT user 2.
+        \factor_exemption\factor::add_exemption($user1);
+
+        // Get state for user 1. This should return a neutral and not redirect.
+        $this->setUser($user1);
+        $this->assertEquals(\tool_mfa\plugininfo\factor::STATE_NEUTRAL, $grace->get_state(true));
+        $redirected = null;
+        try {
+            $grace->get_state(true);
+            $redirected = false;
+        } catch (\Throwable $e) {
+            $expected = get_string('redirecterrordetected', 'error');
+            if ($expected === $e->getMessage()) {
+                $redirected = true;
+            }
+        }
+        $this->assertFalse($redirected);
+
+        // Get state for user 2. This should redirect them to the totp setup since they do not pass the exemption.
+        $this->setUser($user2);
+        $redirected = null;
+        try {
+            $grace->get_state(true);
+            $redirected = false;
+        } catch (\Throwable $e) {
+            $expected = get_string('redirecterrordetected', 'error');
+            if ($expected === $e->getMessage()) {
+                $redirected = true;
+            }
+        }
+        $this->assertTrue($redirected);
+    }
 }