From 908ce394cae2525fcace8c35137197703cda6899 Mon Sep 17 00:00:00 2001 From: Benjamin Walker Date: Tue, 20 Aug 2024 13:45:45 +1000 Subject: [PATCH] Add setting to restrict noredirect by ip #826 --- classes/auth.php | 26 ++++++++++++++++++--- lang/en/auth_saml2.php | 2 ++ settings.php | 7 ++++++ tests/auth_test.php | 52 ++++++++++++++++++++++++++++++++++++++++++ version.php | 4 ++-- 5 files changed, 86 insertions(+), 5 deletions(-) diff --git a/classes/auth.php b/classes/auth.php index 61aaedd3e..0663a4f4d 100644 --- 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 index c2906247f..9c5a3c2d7 100644 --- 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 index b1dacf0c3..161437a8c 100644 --- 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 index 6dc7b8230..522514123 100644 --- 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 index 62747df9b..d8a820ae3 100644 --- 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