From 6f7a279c5b27e1d27a54046adda5037f996913f0 Mon Sep 17 00:00:00 2001 From: gyufi Date: Wed, 26 Oct 2022 14:18:21 +0200 Subject: [PATCH] refactor: refactoring for ssp 2.0.0 --- .php_cs.dist | 15 ++ .travis.yml | 2 + composer.json | 19 +- default-enable | 0 lib/Auth/Process/FilterAttributes.php | 179 -------------- phpunit.xml | 12 +- psalm.xml | 29 +++ src/Auth/Process/FilterAttributes.php | 229 ++++++++++++++++++ src/Auth/Process/codecov.yml | 17 ++ tests/_autoload_modules.php | 32 --- tests/bootstrap.php | 5 + .../lib/Auth/Process/FilterAttributesTest.php | 15 +- 12 files changed, 330 insertions(+), 224 deletions(-) create mode 100644 .php_cs.dist delete mode 100644 default-enable delete mode 100644 lib/Auth/Process/FilterAttributes.php create mode 100644 psalm.xml create mode 100644 src/Auth/Process/FilterAttributes.php create mode 100644 src/Auth/Process/codecov.yml delete mode 100644 tests/_autoload_modules.php create mode 100644 tests/bootstrap.php diff --git a/.php_cs.dist b/.php_cs.dist new file mode 100644 index 0000000..b46e7f8 --- /dev/null +++ b/.php_cs.dist @@ -0,0 +1,15 @@ +in([ + __DIR__ . '/src', + __DIR__ . '/tests', + ]) +; +return PhpCsFixer\Config::create() + ->setRules([ + '@PSR2' => true, + '@PSR4' => true, + '@PSR5' => true, + ]) + ->setFinder($finder) +; diff --git a/.travis.yml b/.travis.yml index 2fa6332..ef1f0ac 100644 --- a/.travis.yml +++ b/.travis.yml @@ -4,6 +4,8 @@ php: - 5.5 - 5.6 - 7.0 +- 7.4 +- 8.1 - hhvm matrix: allow_failures: diff --git a/composer.json b/composer.json index 1596bb1..19fea1f 100644 --- a/composer.json +++ b/composer.json @@ -3,18 +3,25 @@ "description": "Filter to remove attribute values which are not properly scoped.", "keywords": [ "simplesamlphp", "scope", "sp", "module", "filter" ], "type": "simplesamlphp-module", + "autoload": { + "psr-4": { + "SimpleSAML\\Module\\attributescope\\": "src/" + } + }, "require": { - "simplesamlphp/composer-module-installer": "^1.1" + "simplesamlphp/composer-module-installer": "^1.2", + "simplesamlphp/simplesamlphp": "2.0.0-rc2" }, "require-dev": { - "simplesamlphp/simplesamlphp": "1.14", - "phpunit/phpunit": "~4.8.35" - }, - "autoload-dev": { - "files": ["tests/_autoload_modules.php"] + "simplesamlphp/simplesamlphp-test-framework": "^1.2.1" }, "support": { "issues": "https://github.com/NIIF/simplesamlphp-module-attributescope/issues", "source": "https://github.com/NIIF/simplesamlphp-module-attributescope" + }, + "config": { + "allow-plugins": { + "simplesamlphp/composer-module-installer": true + } } } diff --git a/default-enable b/default-enable deleted file mode 100644 index e69de29..0000000 diff --git a/lib/Auth/Process/FilterAttributes.php b/lib/Auth/Process/FilterAttributes.php deleted file mode 100644 index 8e76b0d..0000000 --- a/lib/Auth/Process/FilterAttributes.php +++ /dev/null @@ -1,179 +0,0 @@ -attributesWithScope = $config['attributesWithScope']; - } - if (array_key_exists('scopeAttributes', $config)) { - $this->scopeAttributes = $config['scopeAttributes']; - } - if (array_key_exists('ignoreCheckForEntities', $config)) { - $this->ignoreCheckForEntities = $config['ignoreCheckForEntities']; - } - if (array_key_exists('attributesWithScopeSuffix', $config)) { - $this->attributesWithScopeSuffix = $config['attributesWithScopeSuffix']; - } - if (array_key_exists('ignoreCase', $config)) { - $this->ignoreCase = $config['ignoreCase']; - } - } - - /** - * Apply filter. - * - * @param array &$request the current request - */ - public function process(&$request) - { - $src = $request['Source']; - - if (isset($src['entityid']) && in_array($src['entityid'], $this->ignoreCheckForEntities, true)) { - SimpleSAML_Logger::debug('Ignoring scope checking for assertions from ' . $src['entityid']); - return; - } - - $noscope = false; - if (!isset($src['scope']) || - !is_array($src['scope']) || - !count($src['scope'])) { - SimpleSAML_Logger::warning('No scope extension in IdP metadata, all scoped attributes are filtered out!'); - $noscope = true; - } - $scopes = $noscope ? array() : $src['scope']; - - foreach ($this->attributesWithScope as $attributesWithScope) { - if (!isset($request['Attributes'][$attributesWithScope])) { - continue; - } - if ($noscope) { - SimpleSAML_Logger::info('Attribute '.$attributesWithScope.' is filtered out due to missing scope information in IdP metadata.'); - unset($request['Attributes'][$attributesWithScope]); - continue; - } - $values = $request['Attributes'][$attributesWithScope]; - $newValues = array(); - foreach ($values as $value) { - if ($this->isProperlyScoped($value, $scopes)) { - $newValues[] = $value; - } else { - SimpleSAML_Logger::warning('Attribute value ('.$value.') is removed by attributescope check.'); - } - } - - if (count($newValues)) { - $request['Attributes'][$attributesWithScope] = $newValues; - } else { - unset($request['Attributes'][$attributesWithScope]); - } - } - // Filter out scopeAttributes if the value does not match any scope values - foreach ($this->scopeAttributes as $scopeAttribute) { - if (array_key_exists($scopeAttribute, $request['Attributes'])) { - if (count($request['Attributes'][$scopeAttribute]) != 1) { - SimpleSAML_Logger::warning('$scopeAttribute (' . $scopeAttribute . ') must be single valued. Filtering out.'); - unset($request['Attributes'][$scopeAttribute]); - } elseif (!in_array($request['Attributes'][$scopeAttribute][0], $scopes)) { - SimpleSAML_Logger::warning('Scope attribute (' . $scopeAttribute . ') does not match metadata. Filtering out.'); - unset($request['Attributes'][$scopeAttribute]); - } - } - } - - foreach ($this->attributesWithScopeSuffix as $attributeWithSuffix) { - if (!isset($request['Attributes'][$attributeWithSuffix])) { - continue; - } - if ($noscope) { - SimpleSAML_Logger::info('Attribute '.$attributeWithSuffix.' is filtered out due to missing scope information in IdP metadata.'); - unset($request['Attributes'][$attributeWithSuffix]); - continue; - } - $values = $request['Attributes'][$attributeWithSuffix]; - $newValues = array(); - foreach ($values as $value) { - if ($this->isProperlySuffixed($value, $scopes)) { - $newValues[] = $value; - } else { - SimpleSAML_Logger::warning('Attribute value ('.$value.') is removed by attributeWithScopeSuffix check.'); - } - } - - if (count($newValues)) { - $request['Attributes'][$attributeWithSuffix] = $newValues; - } else { - unset($request['Attributes'][$attributeWithSuffix]); - } - } - } - - /** - * Determines whether an attribute value is properly scoped. - * - * @param string $value The attribute value to check - * @param array $scopes The array of scopes for the Idp - * - * @return bool true if properly scoped - */ - private function isProperlyScoped($value, $scopes) - { - foreach ($scopes as $scope) { - $preg = '/^[^@]+@'.preg_quote($scope).'$/' . ($this->ignoreCase ? 'i' : ''); - if (preg_match($preg, $value) == 1) { - return true; - } - } - } - - /** - * Determines whether an attribute value is properly suffixed with the scope. - * @ and (literal) . are used for suffix boundries - * - * @param string $value The attribute value to check - * @param array $scopes The array of scopes for the IdP - * - * @return bool true if attribute is suffixed with a scope - */ - private function isProperlySuffixed($value, $scopes) - { - foreach ($scopes as $scope) { - $scopeRegex = '/^[^@]+@(.*\.)?'.preg_quote($scope).'$/' . ($this->ignoreCase ? 'i' : ''); - $subdomainRegex = '/^([^@]*\.)?'.preg_quote($scope).'$/' . ($this->ignoreCase ? 'i' : ''); - if (preg_match($subdomainRegex, $value) === 1 || preg_match($scopeRegex, $value) === 1) { - return true; - } - } - } -} diff --git a/phpunit.xml b/phpunit.xml index 758398c..874490c 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -1,5 +1,15 @@ - + + + + ./src + + + + + + + ./tests diff --git a/psalm.xml b/psalm.xml new file mode 100644 index 0000000..c36957f --- /dev/null +++ b/psalm.xml @@ -0,0 +1,29 @@ + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/src/Auth/Process/FilterAttributes.php b/src/Auth/Process/FilterAttributes.php new file mode 100644 index 0000000..6e2932f --- /dev/null +++ b/src/Auth/Process/FilterAttributes.php @@ -0,0 +1,229 @@ + + * @author Gyula Szabo + * @author Gyula Szabo + * @author Tamas Frank + * @license http://www.gnu.org/copyleft/lesser.html LGPL License + * @link https://github.com/niif/simplesamlphp-module-attributescope + */ + +namespace SimpleSAML\Module\niif\attributescope\Auth\Process; +use SimpleSAML\Auth; +use SimpleSAML\Logger; + +/** + * Filter to remove + * * all attributes if there is no `shibmd:Scope` value for the IdP + * * attribute values which are not properly scoped + * * configured scopeAttribute if it doesn't match against + * a value from `shibmd:Scope`. + * + * Note: + * * regexp in scope values are not supported. + * * Configured attribute names MUST match with names in attributemaps. + * It is case-sensitive. + * + * @category SimpleSAML + * @package SimpleSAML\Module\niif + * @author Adam Lantos + * @author Gyula Szabo + * @author Gyula Szabo + * @author Tamas Frank + * @license http://www.gnu.org/copyleft/lesser.html LGPL License + * @link https://github.com/niif/simplesamlphp-module-attributescope + */ + + + +class FilterAttributes extends Auth\ProcessingFilter +{ + private array $_attributesWithScope = array( + 'eduPersonPrincipalName', + 'eduPersonScopedAffiliation', + ); + + private array $_scopeAttributes = array( + 'schacHomeOrganization', + ); + + private array $_ignoreCheckForEntities = array(); + + private array $_attributesWithScopeSuffix = array(); + + private bool $_ignoreCase = false; + + /** + * Constructor + * + * @param array $config simplesamlphp configuration + * @param mixed $reserved reserved + */ + public function __construct(array $config, $reserved) + { + parent::__construct($config, $reserved); + if (array_key_exists('attributesWithScope', $config)) { + $this->_attributesWithScope = $config['attributesWithScope']; + } + if (array_key_exists('scopeAttributes', $config)) { + $this->_scopeAttributes = $config['scopeAttributes']; + } + if (array_key_exists('ignoreCheckForEntities', $config)) { + $this->_ignoreCheckForEntities = $config['ignoreCheckForEntities']; + } + if (array_key_exists('attributesWithScopeSuffix', $config)) { + $this->_attributesWithScopeSuffix = $config['attributesWithScopeSuffix']; + } + if (array_key_exists('ignoreCase', $config)) { + $this->_ignoreCase = $config['ignoreCase']; + } + } + + /** + * Process the filter + * + * @param array $state the state array + * + * @return void + */ + public function process(array &$state): void + { + $src = $state['Source']; + + if (isset($src['entityid']) && in_array($src['entityid'], $this->_ignoreCheckForEntities, true)) { + Logger::debug('Ignoring scope checking for assertions from ' . $src['entityid']); + return; + } + + $noscope = false; + if (!isset($src['scope']) + || !is_array($src['scope']) + || !count($src['scope']) + ) { + Logger::warning('No scope extension in IdP metadata, all scoped attributes are filtered out!'); + $noscope = true; + } + $scopes = $noscope ? array() : $src['scope']; + + foreach ($this->_attributesWithScope as $attributesWithScope) { + if (!isset($state['Attributes'][$attributesWithScope])) { + continue; + } + if ($noscope) { + Logger::info('Attribute '.$attributesWithScope.' is filtered out due to missing scope information in IdP metadata.'); + unset($state['Attributes'][$attributesWithScope]); + continue; + } + $values = $state['Attributes'][$attributesWithScope]; + $newValues = array(); + foreach ($values as $value) { + if ($this->_isProperlyScoped($value, $scopes)) { + $newValues[] = $value; + } else { + Logger::warning('Attribute value ('.$value.') is removed by attributescope check.'); + } + } + + if (count($newValues)) { + $state['Attributes'][$attributesWithScope] = $newValues; + } else { + unset($state['Attributes'][$attributesWithScope]); + } + } + // Filter out scopeAttributes if the value does not match any scope values + foreach ($this->_scopeAttributes as $scopeAttribute) { + if (array_key_exists($scopeAttribute, $state['Attributes'])) { + if (count($state['Attributes'][$scopeAttribute]) != 1) { + Logger::warning('$scopeAttribute (' . $scopeAttribute . ') must be single valued. Filtering out.'); + unset($state['Attributes'][$scopeAttribute]); + } elseif (!in_array($state['Attributes'][$scopeAttribute][0], $scopes)) { + Logger::warning('Scope attribute (' . $scopeAttribute . ') does not match metadata. Filtering out.'); + unset($state['Attributes'][$scopeAttribute]); + } + } + } + + foreach ($this->_attributesWithScopeSuffix as $attributeWithSuffix) { + if (!isset($state['Attributes'][$attributeWithSuffix])) { + continue; + } + if ($noscope) { + Logger::info('Attribute '.$attributeWithSuffix.' is filtered out due to missing scope information in IdP metadata.'); + unset($state['Attributes'][$attributeWithSuffix]); + continue; + } + $values = $state['Attributes'][$attributeWithSuffix]; + $newValues = array(); + foreach ($values as $value) { + if ($this->_isProperlySuffixed($value, $scopes)) { + $newValues[] = $value; + } else { + Logger::warning('Attribute value ('.$value.') is removed by attributeWithScopeSuffix check.'); + } + } + + if (count($newValues)) { + $state['Attributes'][$attributeWithSuffix] = $newValues; + } else { + unset($state['Attributes'][$attributeWithSuffix]); + } + } + } + + /** + * Determines whether an attribute value is properly scoped. + * + * @param string $value The attribute value to check + * @param array $scopes The array of scopes for the Idp + * + * @return bool true if properly scoped + */ + private function _isProperlyScoped(string $value, array $scopes): bool + { + foreach ($scopes as $scope) { + $preg = '/^[^@]+@'.preg_quote($scope).'$/' . ($this->_ignoreCase ? 'i' : ''); + if (preg_match($preg, $value) == 1) { + return true; + } + } + + return false; + } + + /** + * Determines whether an attribute value is properly suffixed with the scope. + * @ and (literal) . are used for suffix boundries + * + * @param string $value The attribute value to check + * @param array $scopes The array of scopes for the IdP + * + * @return bool true if attribute is suffixed with a scope + */ + private function _isProperlySuffixed(string $value, array $scopes): bool + { + foreach ($scopes as $scope) { + $scopeRegex = '/^[^@]+@(.*\.)?'.preg_quote($scope).'$/' . ($this->_ignoreCase ? 'i' : ''); + $subdomainRegex = '/^([^@]*\.)?'.preg_quote($scope).'$/' . ($this->_ignoreCase ? 'i' : ''); + if (preg_match($subdomainRegex, $value) === 1 || preg_match($scopeRegex, $value) === 1) { + return true; + } + } + + return false; + } +} diff --git a/src/Auth/Process/codecov.yml b/src/Auth/Process/codecov.yml new file mode 100644 index 0000000..d01dd7d --- /dev/null +++ b/src/Auth/Process/codecov.yml @@ -0,0 +1,17 @@ +coverage: + status: + project: + default: + target: 0% + threshold: 2% + patch: off +comment: + layout: "diff" + behavior: once + require_changes: true + require_base: no + require_head: yes + branches: null + +github_checks: + annotations: false diff --git a/tests/_autoload_modules.php b/tests/_autoload_modules.php deleted file mode 100644 index a6fdea7..0000000 --- a/tests/_autoload_modules.php +++ /dev/null @@ -1,32 +0,0 @@ -process($request); return $request; } @@ -20,7 +23,7 @@ private static function processFilter(array $config, array $request) /** * Test scoped attributes don't match scope * @param array $source The IDP source info - * @dataProvider testWrongScopeDataProvider + * @dataProvider wrongScopeDataProvider */ public function testWrongScope($source) { @@ -47,7 +50,7 @@ public function testWrongScope($source) * Provide data for the tests * @return array test cases with each subtest being array of arguments */ - public function testWrongScopeDataProvider() + public function wrongScopeDataProvider() { return array( // Empty Source @@ -68,7 +71,7 @@ public function testWrongScopeDataProvider() /** * Test correct scope * @param array $source The IDP source info - * @dataProvider testCorrectScopeDataProvider + * @dataProvider correctScopeDataProvider */ public function testCorrectScope($source) { @@ -92,7 +95,7 @@ public function testCorrectScope($source) * Provide data for the tests * @return array test cases with each subtest being array of arguments */ - public function testCorrectScopeDataProvider() + public function correctScopeDataProvider() { return array( // Correct scope