From 919a6c102cd36c9278529cc0c3f212bf511a8471 Mon Sep 17 00:00:00 2001
From: Thijs Kinkhorst <thijs@kinkhorst.com>
Date: Fri, 27 Aug 2021 13:03:17 +0000
Subject: [PATCH] Fix filterscopes using hostname when explicit scopes set.
FilterScopes documented behaviour is that hostname is only used as
a fallback when there are no explicit scopes (makes sense). However
the code does not do that but always includes the hostname to match.
This refactors the code so that hostname is only evaluated when
no explicit scopes are there. Also it takes the whole fetching and
parsing of the SSOlocation outside of the inner loop whcih is
inefficient. This is now only done once, and only if needed (when
no explicit scopes present).
To confirm the changes testcases have been added for this and also
for some other cases to reach 100% coverage for the filter.
---
.../saml/lib/Auth/Process/FilterScopes.php | 68 ++++++-------
.../lib/Auth/Process/FilterScopesTest.php | 97 +++++++++++++++++--
2 files changed, 118 insertions(+), 47 deletions(-)
diff --git a/modules/saml/lib/Auth/Process/FilterScopes.php b/modules/saml/lib/Auth/Process/FilterScopes.php
index 9748a9c61..f6c0a8f92 100644
--- a/modules/saml/lib/Auth/Process/FilterScopes.php
+++ b/modules/saml/lib/Auth/Process/FilterScopes.php
@@ -25,7 +25,6 @@ class FilterScopes extends ProcessingFilter
'eduPersonPrincipalName'
];
-
/**
* Constructor for the processing filter.
*
@@ -41,7 +40,6 @@ class FilterScopes extends ProcessingFilter
}
}
-
/**
* This method applies the filter, removing any values
*
@@ -50,53 +48,45 @@ class FilterScopes extends ProcessingFilter
public function process(array &$request): void
{
$src = $request['Source'];
- if (!count($this->scopedAttributes)) {
- // paranoia, should never happen
- Logger::warning('No scoped attributes configured.');
- return;
- }
+
$validScopes = [];
+ $host = '';
if (array_key_exists('scope', $src) && is_array($src['scope']) && !empty($src['scope'])) {
$validScopes = $src['scope'];
+ } else {
+ $ep = Utils\Config\Metadata::getDefaultEndpoint($request['Source']['SingleSignOnService']);
+ $host = parse_url($ep['Location'], PHP_URL_HOST) ?? '';
}
- $ep = Utils\Config\Metadata::getDefaultEndpoint($request['Source']['SingleSignOnService']);
- if ($ep !== null) {
- foreach ($this->scopedAttributes as $attribute) {
- if (!isset($request['Attributes'][$attribute])) {
- continue;
- }
+ foreach ($this->scopedAttributes as $attribute) {
+ if (!isset($request['Attributes'][$attribute])) {
+ continue;
+ }
- $values = $request['Attributes'][$attribute];
- $newValues = [];
- foreach ($values as $value) {
- $loc = $ep['Location'];
- $host = parse_url($loc, PHP_URL_HOST);
- if ($host === null) {
- $host = '';
- }
- $value_a = explode('@', $value, 2);
- if (count($value_a) < 2) {
- $newValues[] = $value;
- continue; // there's no scope
- }
- $scope = $value_a[1];
- if (in_array($scope, $validScopes, true)) {
- $newValues[] = $value;
- } elseif (strpos($host, $scope) === strlen($host) - strlen($scope)) {
- $newValues[] = $value;
- } else {
- Logger::warning("Removing value '$value' for attribute '$attribute'. Undeclared scope.");
- }
+ $values = $request['Attributes'][$attribute];
+ $newValues = [];
+ foreach ($values as $value) {
+ $value_a = explode('@', $value, 2);
+ if (count($value_a) < 2) {
+ $newValues[] = $value;
+ continue; // there's no scope
}
-
- if (empty($newValues)) {
- Logger::warning("No suitable values for attribute '$attribute', removing it.");
- unset($request['Attributes'][$attribute]); // remove empty attributes
+ $scope = $value_a[1];
+ if (in_array($scope, $validScopes, true)) {
+ $newValues[] = $value;
+ } elseif (strpos($host, $scope) === strlen($host) - strlen($scope)) {
+ $newValues[] = $value;
} else {
- $request['Attributes'][$attribute] = $newValues;
+ Logger::warning("Removing value '$value' for attribute '$attribute'. Undeclared scope.");
}
}
+
+ if (empty($newValues)) {
+ Logger::warning("No suitable values for attribute '$attribute', removing it.");
+ unset($request['Attributes'][$attribute]); // remove empty attributes
+ } else {
+ $request['Attributes'][$attribute] = $newValues;
+ }
}
}
}
diff --git a/tests/modules/saml/lib/Auth/Process/FilterScopesTest.php b/tests/modules/saml/lib/Auth/Process/FilterScopesTest.php
index 724eebe57..2730453c3 100644
--- a/tests/modules/saml/lib/Auth/Process/FilterScopesTest.php
+++ b/tests/modules/saml/lib/Auth/Process/FilterScopesTest.php
@@ -68,19 +68,12 @@ class FilterScopesTest extends TestCase
$result = $this->processFilter($config, $request);
$this->assertEquals($request['Attributes'], $result['Attributes']);
- // test implicit scope
- $request['Attributes'] = [
- 'eduPersonPrincipalName' => ['jdoe@example.org'],
- ];
- $result = $this->processFilter($config, $request);
- $this->assertEquals($request['Attributes'], $result['Attributes']);
-
// test alternative attributes
$config['attributes'] = [
'mail',
];
$request['Attributes'] = [
- 'mail' => ['john.doe@example.org'],
+ 'mail' => ['john.doe@example.com'],
];
$result = $this->processFilter($config, $request);
$this->assertEquals($request['Attributes'], $result['Attributes']);
@@ -91,6 +84,35 @@ class FilterScopesTest extends TestCase
$this->assertEquals($request['Attributes'], $result['Attributes']);
}
+ /**
+ * Test implict scope matching on IdP hostname
+ */
+ public function testImplicitScopes(): void
+ {
+ $config = [];
+ $request = [
+ 'Source' => [
+ 'SingleSignOnService' => [
+ [
+ 'Binding' => 'urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect',
+ 'Location' => 'https://example.org/saml2/idp/SSOService.php',
+ ],
+ ],
+ ],
+ 'Attributes' => [
+ 'eduPersonPrincipalName' => ['jdoe@example.org'],
+ ],
+ ];
+
+ $result = $this->processFilter($config, $request);
+ $this->assertEquals($request['Attributes'], $result['Attributes']);
+
+ $request['Attributes'] = [
+ 'eduPersonPrincipalName' => ['jdoe@example.com'],
+ ];
+ $result = $this->processFilter($config, $request);
+ $this->assertEquals([], $result['Attributes']);
+ }
/**
* Test invalid scopes.
@@ -137,5 +159,64 @@ class FilterScopesTest extends TestCase
];
$result = $this->processFilter($config, $request);
$this->assertEquals($request['Attributes'], $result['Attributes']);
+
+ }
+
+ /**
+ * Test that implicit matching is not done when explicit scopes present
+ */
+ public function testNoImplicitMatchingWhenExplicitScopes(): void
+ {
+ // test declared scopes
+ $config = [];
+ $request = [
+ 'Source' => [
+ 'SingleSignOnService' => [
+ [
+ 'Binding' => 'urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect',
+ 'Location' => 'https://example.org/saml2/idp/SSOService.php',
+ ],
+ ],
+ 'scope' => [
+ 'example.com',
+ 'example.net',
+ ],
+ ],
+ 'Attributes' => [
+ 'eduPersonPrincipalName' => ['jdoe@example.org'],
+ ],
+ ];
+ $result = $this->processFilter($config, $request);
+ $this->assertEquals([], $result['Attributes']);
+ }
+
+ /**
+ * Test that the scope is considered to be the part after the first @ sign
+ */
+ public function testAttributeValueMultipleAt(): void
+ {
+ $config = [];
+ $request = [
+ 'Source' => [
+ 'SingleSignOnService' => [
+ [
+ 'Binding' => 'urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect',
+ 'Location' => 'https://example.org/saml2/idp/SSOService.php',
+ ],
+ ],
+ 'scope' => [
+ 'example.com',
+ ],
+ ],
+ 'Attributes' => [
+ 'eduPersonPrincipalName' => ['jdoe@gmail.com@example.com'],
+ ],
+ ];
+ $result = $this->processFilter($config, $request);
+ $this->assertEquals([], $result['Attributes']);
+
+ $request['Source']['scope'] = ['gmail.com@example.com'];
+ $result = $this->processFilter($config, $request);
+ $this->assertEquals($request['Attributes'], $result['Attributes']);
}
}
--
GitLab