Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add setting to restrict noredirect by ip #826 #831

Merged
merged 1 commit into from
Aug 20, 2024
Merged

Add setting to restrict noredirect by ip #826 #831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions classes/auth.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
Expand All @@ -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
*/
Expand Down
2 changes: 2 additions & 0 deletions lang/en/auth_saml2.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.';
Expand Down
7 changes: 7 additions & 0 deletions settings.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand Down
52 changes: 52 additions & 0 deletions tests/auth_test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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],
];
}

Expand Down
4 changes: 2 additions & 2 deletions version.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading