Add setting to restrict noredirect by ip #826

catalyst · Aug 20, 2024 · 908ce39 · 908ce39
1 parent 64070b3
commit 908ce39
Show file tree

Hide file tree

Showing 5 changed files with 86 additions and 5 deletions.
diff --git a/classes/auth.php b/classes/auth.php
@@ -478,7 +478,7 @@ public function should_login_redirect() {
         }
 
         // Never redirect if requested so.
-        if ($saml === 0) {
+        if ($saml === 0 && $this->can_skip_redirect()) {
             $SESSION->saml = $saml;
             $this->log(__FUNCTION__ . ' skipping due to saml=off parameter');
             return false;
@@ -532,7 +532,8 @@ public function should_login_redirect() {
         //
         // This isn't needed when duallogin is on because $saml will default to 0
         // and duallogin is not part of the request.
-        if ((isset($SESSION->saml) && $SESSION->saml == 0) && $this->config->duallogin == saml2_settings::OPTION_DUAL_LOGIN_NO) {
+        if ((isset($SESSION->saml) && $SESSION->saml == 0) && $this->config->duallogin == saml2_settings::OPTION_DUAL_LOGIN_NO
+                && $this->can_skip_redirect()) {
             $this->log(__FUNCTION__ . ' skipping due to no sso session');
             return false;
         }
@@ -554,7 +555,7 @@ public function should_login_redirect() {
             $saml = 0;
         }
 
-        if ($saml == 0) {
+        if ($saml == 0 && $this->can_skip_redirect()) {
             $SESSION->saml = $saml;
             $this->log(__FUNCTION__ . ' skipping due to ?saml=off');
             return false;
@@ -569,6 +570,25 @@ public function should_login_redirect() {
         return true;
     }
 
+    /**
+     * Checks whether a user is allowed to skip redirect by using ?saml=off and noredirect params.
+     *
+     * @return bool whether the user can use these flags.
+     */
+    public function can_skip_redirect() {
+        // Allow if duallogin is enabled or a whitelist hasn't been set.
+        if ($this->config->duallogin != saml2_settings::OPTION_DUAL_LOGIN_NO || empty($this->config->noredirectips)) {
+            return true;
+        }
+
+        // Otherwise only allow this for users with matching IPs.
+        if (remoteip_in_list($this->config->noredirectips)) {
+            return true;
+        }
+
+        return false;
+    }
+
     /**
      * All the checking happens before the login page in this hook
      */

diff --git a/lang/en/auth_saml2.php b/lang/en/auth_saml2.php
@@ -151,6 +151,8 @@
 $string['nameidasattrib_help'] = 'The NameID claim will be exposed to SSPHP as an attribute named nameid';
 $string['noattribute'] = 'You have logged in successfully but we could not find your \'{$a}\' attribute to associate you to an account in Moodle.';
 $string['noidpfound'] = 'The IdP \'{$a}\' was not found as a configured IdP.';
+$string['noredirectips'] = 'Restrict noredirect by IP';
+$string['noredirectips_help'] = 'When dual login is turned off and IPs are set, this will restrict the use of ?saml=off and ?noredirect=1 during SAML login to users with matching IP subnets.';
 $string['nouser'] = 'You have logged in successfully as \'{$a}\' but do not have an account in Moodle.';
 $string['nullprivatecert'] = 'Creation of Private Certificate failed.';
 $string['nullpubliccert'] = 'Creation of Public Certificate failed.';

diff --git a/settings.php b/settings.php
@@ -246,6 +246,13 @@
         ));
     }
 
+    $settings->add(new admin_setting_configiplist(
+        'auth_saml2/noredirectips',
+        get_string('noredirectips', 'auth_saml2'),
+        get_string('noredirectips_help', 'auth_saml2'),
+        ''
+    ));
+
     // Auto login.
     $autologinoptions = [
         saml2_settings::OPTION_AUTO_LOGIN_NO => get_string('no'),

diff --git a/tests/auth_test.php b/tests/auth_test.php
@@ -788,6 +788,9 @@ public function test_should_login_redirect($cfg, $config, $param, $multiidp, $se
             $_GET['multiidp'] = true;
         }
 
+        // Setting an ip to use for testing against different configs.
+        $_SERVER['REMOTE_ADDR'] = '1.2.3.4';
+
         /** @var auth_plugin_saml2 $auth */
         $auth = get_auth_plugin('saml2');
         $result = $auth->should_login_redirect();
@@ -934,6 +937,55 @@ public function provider_should_login_redirect(): array {
                 ['duallogin' => true],
                 'on', true, false,
                 $midp],
+
+            // Restrict noredirect flags by ip.
+            // IP restrictions for ?saml=off should only take effect when dual is off.
+            "21. dual: y, ips: no match, param: off, multiidp: false, session: false" => [
+                [],
+                ['duallogin' => true, 'noredirectips' => '4.3.2.1'],
+                'off', false, false,
+                false],
+
+            // Ignore ?saml=off when ip restrictions are set and there's no matching ip.
+            "22. dual: n, ips: no match, param: off, multiidp: false, session: false" => [
+                [],
+                ['duallogin' => false, 'noredirectips' => '4.3.2.1'],
+                'off', false, false,
+                true],
+
+            // Allow ?saml=off when ip restrictions are set and there's a matching ip.
+            "23. dual: n, ips: match, param: off, multiidp: false, session: false" => [
+                [],
+                ['duallogin' => false, 'noredirectips' => '1.2.3.4'],
+                'off', false, false,
+                false],
+
+            // Matching ip subsets.
+            "24. dual: n, ips: match subset, param: off, multiidp: false, session: false" => [
+                [],
+                ['duallogin' => false, 'noredirectips' => '1.2'],
+                'off', false, false,
+                false],
+
+            // Multiple lines.
+            "25. dual: n, ips: match line, param: off, multiidp: false, session: false" => [
+                [],
+                ['duallogin' => false, 'noredirectips' => '4.3.2.1' . PHP_EOL . '1.2.3.4'],
+                'off', false, false,
+                false],
+
+            // Confirm this works the same for sessions.
+            "26. dual: n, ips: no match, param: off, multiidp: false, session: true" => [
+                [],
+                ['duallogin' => false, 'noredirectips' => '4.3.2.1'],
+                'off', false, true,
+                true],
+
+            "27. dual: n, ips: match, param: off, multiidp: false, session: true" => [
+                [],
+                ['duallogin' => false, 'noredirectips' => '1.2.3.4'],
+                'off', false, true,
+                false],
         ];
     }
 

diff --git a/version.php b/version.php
@@ -24,8 +24,8 @@
 
 defined('MOODLE_INTERNAL') || die();
 
-$plugin->version   = 2024071101;    // The current plugin version (Date: YYYYMMDDXX).
-$plugin->release   = 2024071101;    // Match release exactly to version.
+$plugin->version   = 2024082000;    // The current plugin version (Date: YYYYMMDDXX).
+$plugin->release   = 2024082000;    // Match release exactly to version.
 $plugin->requires  = 2017051509;    // Requires PHP 7, 2017051509 = T12. M3.3
                                     // Strictly we require either Moodle 3.5 OR
                                     // we require Totara 3.3, but the version number