Issue #148: Fixed error with redirect loop explosions

Author: Peterburnett

.travis.yml

@@ -2,7 +2,7 @@ language: php
 
 addons:
   firefox: "47.0.1"
-  postgresql: "9.4"
+  postgresql: "9.5"
 
 cache:
   directories:

classes/manager.php

@@ -314,6 +314,12 @@ public static function should_require_mfa($url, $preventredirect) {
         // Remove all params before comparison.
         $url->remove_all_params();
 
+        // Dont redirect if already on auth.php
+        $authurl = new \moodle_url('/admin/tool/mfa/auth.php');
+        if ($url->compare($authurl)) {
+            return self::NO_REDIRECT;
+        }
+
         // Soft maintenance mode.
         if (!empty($CFG->maintenance_enabled)) {
             return self::NO_REDIRECT;
@@ -362,17 +368,20 @@ public static function should_require_mfa($url, $preventredirect) {
         }
 
         // Site policy.
-        if (isset($USER->policyagreed) && !$USER->policyagreed
-        && defined('NO_SITEPOLICY_CHECK') && !NO_SITEPOLICY_CHECK) {
+        if (isset($USER->policyagreed) && !$USER->policyagreed) {
             $manager = new \core_privacy\local\sitepolicy\manager();
             $policyurl = $manager->get_redirect_url(false);
-            if (!empty($policyurl)) {
+            if (!empty($policyurl) && $url->compare($policyurl)) {
                 return self::NO_REDIRECT;
             }
         }
 
         // WS/AJAX check.
         if (WS_SERVER || AJAX_SCRIPT) {
+            if (isset($SESSION->mfa_pending) && !empty($SESSION->mfa_pending)) {
+                // Allow AJAX and WS, but never from auth.php
+                return self::NO_REDIRECT;
+            }
             return self::REDIRECT_EXCEPTION;
         }
 
@@ -385,9 +394,8 @@ public static function should_require_mfa($url, $preventredirect) {
         }
 
         // Circular checks.
-        $authurl = new \moodle_url('/admin/tool/mfa/auth.php');
-        if (isset($SESSION->mfa_redir_referer) &&
-            $SESSION->mfa_redir_referer != $authurl) {
+        if (isset($SESSION->mfa_redir_referer)
+            && $SESSION->mfa_redir_referer != $authurl) {
             if ($SESSION->mfa_redir_referer == get_local_referer(true)) {
                 // Possible redirect loop.
                 if (!isset($SESSION->mfa_redir_count)) {
@@ -406,10 +414,6 @@ public static function should_require_mfa($url, $preventredirect) {
         // Set referer after checks.
         $SESSION->mfa_redir_referer = get_local_referer(true);
 
-        $safe = new \moodle_url('/admin/tool/mfa/auth.php');
-        if ($safe->compare($url)) {
-            return self::NO_REDIRECT;
-        }
         return self::REDIRECT;
     }
 
@@ -419,10 +423,10 @@ public static function should_require_mfa($url, $preventredirect) {
      * @return array
      */
     public static function get_factor_no_redirect_urls() {
-        $factors = \tool_mfa\plugininfo\factor::get_enabled_factors();
+        $factors = \tool_mfa\plugininfo\factor::get_factors();
         $urls = array();
         foreach ($factors as $factor) {
-            $urls += $factor->get_no_redirect_urls();
+            $urls = array_merge($urls, $factor->get_no_redirect_urls());
         }
         return $urls;
     }
@@ -464,7 +468,17 @@ public static function require_auth($courseorid = null, $autologinguest = null,
 
         if (empty($SESSION->tool_mfa_authenticated) || !$SESSION->tool_mfa_authenticated) {
             $cleanurl = new \moodle_url($ME);
+            $authurl = new \moodle_url('/admin/tool/mfa/auth.php');
+
             $redir = self::should_require_mfa($cleanurl, $preventredirect);
+
+            if ($redir == self::NO_REDIRECT && !$cleanurl->compare($authurl)) {
+                // A non-MFA page that should take precedence.
+                // This check is for any pages, such as site policy, that must occur before MFA.
+                // This check allows AJAX and WS requests to fire on these pages without throwing an exception.
+                $SESSION->mfa_pending = true;
+            }
+
             if ($redir == self::REDIRECT) {
                 if (empty($SESSION->wantsurl)) {
                     !empty($setwantsurltome)
@@ -473,9 +487,16 @@ public static function require_auth($courseorid = null, $autologinguest = null,
 
                     $SESSION->tool_mfa_setwantsurl = true;
                 }
-                redirect(new \moodle_url('/admin/tool/mfa/auth.php'));
+                // Remove pending status.
+                // We must now auth with MFA, now that pending statuses are resolved.
+                unset($SESSION->mfa_pending);
+                redirect($authurl);
             } else if ($redir == self::REDIRECT_EXCEPTION) {
-                throw new \moodle_exception('redirecterrordetected', 'error');
+                if (!empty($SESSION->mfa_redir_referer)) {
+                    throw new \moodle_exception('redirecterrordetected', 'tool_mfa', $SESSION->mfa_redir_referer);
+                } else {
+                    throw new \moodle_exception('redirecterrordetected', 'error');
+                }
             }
         }
     }

lang/en/tool_mfa.php

@@ -107,3 +107,4 @@
 $string['factorrevoked'] = 'Factor \'{$a}\' successfully revoked.';
 $string['connector'] = ' AND ';
 $string['pending'] = 'Pending';
+$string['redirecterrordetected'] = 'Unsupported redirect detected, script execution terminated. Redirection error occured between MFA and {$a}.';

version.php

@@ -25,7 +25,7 @@
 
 defined('MOODLE_INTERNAL') || die();
 
-$plugin->version   = 2019121900;      // The current plugin version (Date: YYYYMMDDXX).
+$plugin->version   = 2020021300;      // The current plugin version (Date: YYYYMMDDXX).
 $plugin->requires  = 2018051708.05;   // Requires MDL-60470 improvement.
 $plugin->component = 'tool_mfa';
 $plugin->release   = 'v1.0';