From 27ea87bc7c6dac708b098a0bcdc4662c53a12600 Mon Sep 17 00:00:00 2001 From: Patrick Radtke Date: Tue, 15 Mar 2016 14:47:45 -0700 Subject: [PATCH 1/3] Add check for attributes suffixed scope --- README.md | 4 ++ lib/Auth/Process/FilterAttributes.php | 56 ++++++++++++++++++- .../lib/Auth/Process/FilterAttributesTest.php | 54 +++++++++++++++++- 3 files changed, 109 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index c795d9a..1c5eedd 100644 --- a/README.md +++ b/README.md @@ -41,5 +41,9 @@ _config/config.php_ ## Configurations Options * `attributesWithScope` an array of attributes that should be scoped and should match the scope from the metadata +* `attributesWithScopeSuffix` an array of attributes that have the scope as a suffix. For example, `user@department.example.com` +and `department.example.com` are both suffixed with `example.com`. Useful when an SP is reliant on `mail` attribute to identify users and +the IdP users various subdomains for mail. * `scopeAttributes` an array of attributes that should exactly match the scope from the metadata * `ignoreCheckForEntities` an array of IdP entity IDs to skip scope checking for. Useful when an IdP is a SAML proxy and is trusted to assert any scope. + diff --git a/lib/Auth/Process/FilterAttributes.php b/lib/Auth/Process/FilterAttributes.php index 0e28d08..c38b25a 100644 --- a/lib/Auth/Process/FilterAttributes.php +++ b/lib/Auth/Process/FilterAttributes.php @@ -27,6 +27,8 @@ class sspmod_attributescope_Auth_Process_FilterAttributes extends SimpleSAML_Aut private $ignoreCheckForEntities = array(); + private $attributesWithScopeSuffix = array(); + public function __construct($config, $reserved) { parent::__construct($config, $reserved); @@ -39,6 +41,9 @@ public function __construct($config, $reserved) if (array_key_exists('ignoreCheckForEntities', $config)) { $this->ignoreCheckForEntities = $config['ignoreCheckForEntities']; } + if (array_key_exists('attributesWithScopeSuffix', $config)) { + $this->attributesWithScopeSuffix = $config['attributesWithScopeSuffix']; + } } /** @@ -101,15 +106,41 @@ public function process(&$request) } } } + + 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 - * @param array $scopes + * @param string $value The attribute value to check + * @param array $scopes The array of scopes for the Idp * - * @return bool + * @return bool true if properly scoped */ private function isProperlyScoped($value, $scopes) { @@ -120,4 +151,23 @@ private function isProperlyScoped($value, $scopes) } } } + + /** + * 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) { + $preg = '/^(.*[@|.])?'.preg_quote($scope).'$/'; + if (preg_match($preg, $value) == 1) { + return true; + } + } + } } diff --git a/tests/lib/Auth/Process/FilterAttributesTest.php b/tests/lib/Auth/Process/FilterAttributesTest.php index e7fe4b3..a5389d7 100644 --- a/tests/lib/Auth/Process/FilterAttributesTest.php +++ b/tests/lib/Auth/Process/FilterAttributesTest.php @@ -24,13 +24,16 @@ private static function processFilter(array $config, array $request) */ public function testWrongScope($source) { - $config = array(); + $config = array( + 'attributesWithScopeSuffix' => array('sampleSuffixedAttribute') + ); $request = array( 'Attributes' => array( 'eduPersonPrincipalName' => array('joe@example.com'), 'nonScopedAttribute' => array('not-removed'), 'eduPersonScopedAffiliation' => array('student@example.com', 'staff@example.com', 'missing-scope'), - 'schacHomeOrganization' => array('example.com') + 'schacHomeOrganization' => array('example.com'), + 'sampleSuffixedAttribute' => array('joe@example.com'), ), 'Source' => $source, ); @@ -163,4 +166,51 @@ public function testIgnoreSourceScope() $attributes = $result['Attributes']; $this->assertEquals($expectedData, $attributes, "Scope check ignored"); } + + /** + * Test attributes values that need to end with the scope or some subdomain of the scope. + */ + public function testAttributeSuffix() + { + + $request = array( + 'Attributes' => array( + 'department' => array( + 'engineering.example.com', // Subdomain + 'example.com', // scope + 'invalid-example.com', // invalid: not subdomain + 'sexample.com', // invalid + 'examplecom' // invalid + ), + 'email' => array( + 'user@example.com', + 'user@gsb.example.com', + 'user@invalid-example.com', //invalid + 'user@examplecom' //invalid + ), + ), + 'Source' => array( + 'scope' => array('example.com'), + 'entityid' => 'https://example.com/idp' + ) + ); + + $config = array( + 'attributesWithScopeSuffix' => array('department', 'email') + ); + $result = self::processFilter($config, $request); + + $attributes = $result['Attributes']; + $expectedData = array( + 'department' => array( + 'engineering.example.com', + 'example.com', + ), + 'email' => array( + 'user@example.com', + 'user@gsb.example.com', + ), + ); + $this->assertEquals($expectedData, $attributes, "Incorrectly suffixed variables should be removed"); + } } From 46d01c5490eaa070cb939077c2751d80448fda54 Mon Sep 17 00:00:00 2001 From: Patrick Radtke Date: Mon, 21 Mar 2016 09:37:35 -0700 Subject: [PATCH 2/3] Handle scoped attributes with > 1 '@' Clarify behavior if attribute is used in multiple configuration options. --- README.md | 2 ++ lib/Auth/Process/FilterAttributes.php | 5 +-- .../lib/Auth/Process/FilterAttributesTest.php | 32 +++++++++++++++---- 3 files changed, 30 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 1c5eedd..119232d 100644 --- a/README.md +++ b/README.md @@ -14,6 +14,8 @@ Additionally, it is also capable to handle 'scope attributes' such as _schacHome * Regular expressions in `shibmd:Scope` are not supported. * It is recommended to run this filter after _oid2name_. Please note that attribute names in the module configuration are case sensitive and must match the names in attributemaps. * 'scope Attributes' must be singled valued, otherwise they are removed. +* Specifying an attribute in multiple configuration options is likely a user configuration issue. A value will only + pass if it conforms to the validation rule for each configured option. ## Installing the module You can install the module with composer: diff --git a/lib/Auth/Process/FilterAttributes.php b/lib/Auth/Process/FilterAttributes.php index c38b25a..79a5325 100644 --- a/lib/Auth/Process/FilterAttributes.php +++ b/lib/Auth/Process/FilterAttributes.php @@ -164,8 +164,9 @@ private function isProperlyScoped($value, $scopes) private function isProperlySuffixed($value, $scopes) { foreach ($scopes as $scope) { - $preg = '/^(.*[@|.])?'.preg_quote($scope).'$/'; - if (preg_match($preg, $value) == 1) { + $scopeRegex = '/^[^@]*@(.*\.)?'.preg_quote($scope).'$/'; + $subdomainRegex = '/^([^@]*\.)?'.preg_quote($scope).'$/'; + if (preg_match($subdomainRegex, $value) === 1 || preg_match($scopeRegex, $value) === 1) { return true; } } diff --git a/tests/lib/Auth/Process/FilterAttributesTest.php b/tests/lib/Auth/Process/FilterAttributesTest.php index a5389d7..fdb2168 100644 --- a/tests/lib/Auth/Process/FilterAttributesTest.php +++ b/tests/lib/Auth/Process/FilterAttributesTest.php @@ -75,7 +75,7 @@ public function testCorrectScope($source) $expectedData = array( 'eduPersonPrincipalName' => array('joe@example.com'), 'nonScopedAttribute' => array('not-removed'), - 'eduPersonScopedAffiliation' => array('student@example.com', 'staff@example.com'), + 'eduPersonScopedAffiliation' => array('student@example.com', 'staff@example.com', '@example.com'), 'schacHomeOrganization' => array('example.com') ); $config = array(); @@ -111,7 +111,12 @@ public function testMixedMultivaluedAttributes() $request = array( 'Attributes' => array( 'nonScopedAttribute' => array('not-removed'), - 'eduPersonScopedAffiliation' => array('faculty@abc.com', 'student@example.com', 'staff@other.com'), + 'eduPersonScopedAffiliation' => array( + 'faculty@abc.com', + 'student@example.com', + 'staff@other.com', + 'member@a@example.com' + ), // schacHomeOrganization is required to be single valued and gets filtered out if multi-valued 'schacHomeOrganization' => array('abc.com', 'example.com', 'other.com') ), @@ -176,17 +181,26 @@ public function testAttributeSuffix() $request = array( 'Attributes' => array( 'department' => array( + // Valid values 'engineering.example.com', // Subdomain 'example.com', // scope - 'invalid-example.com', // invalid: not subdomain - 'sexample.com', // invalid - 'examplecom' // invalid + '.example.com', + // Invalid values + 'invalid-example.com', // not subdomain + 'cexample.com', + 'examplecom' ), 'email' => array( + // Valid values 'user@example.com', 'user@gsb.example.com', - 'user@invalid-example.com', //invalid - 'user@examplecom' //invalid + '@example.com', + '@other.example.com', + // Invalid values + 'user@invalid-example.com', + 'user@examplecom', + 'user@cexample.com', + 'abc@efg@example.com' // double '@; ), ), 'Source' => array( @@ -205,10 +219,14 @@ public function testAttributeSuffix() 'department' => array( 'engineering.example.com', 'example.com', + '.example.com', ), 'email' => array( 'user@example.com', 'user@gsb.example.com', + '@example.com', + '@other.example.com', + ), ); $this->assertEquals($expectedData, $attributes, "Incorrectly suffixed variables should be removed"); From 721c52b6c4a84df6e8afc73b5f18721d340ae642 Mon Sep 17 00:00:00 2001 From: Patrick Radtke Date: Tue, 5 Apr 2016 15:41:10 -0400 Subject: [PATCH 3/3] Filter scoped attributes that have a value of '@scope'. Values must have some data before the '@' --- lib/Auth/Process/FilterAttributes.php | 4 ++-- tests/lib/Auth/Process/FilterAttributesTest.php | 17 ++++++++--------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/lib/Auth/Process/FilterAttributes.php b/lib/Auth/Process/FilterAttributes.php index 79a5325..195fe4b 100644 --- a/lib/Auth/Process/FilterAttributes.php +++ b/lib/Auth/Process/FilterAttributes.php @@ -145,7 +145,7 @@ public function process(&$request) private function isProperlyScoped($value, $scopes) { foreach ($scopes as $scope) { - $preg = '/^[^@]*@'.preg_quote($scope).'$/'; + $preg = '/^[^@]+@'.preg_quote($scope).'$/'; if (preg_match($preg, $value) == 1) { return true; } @@ -164,7 +164,7 @@ private function isProperlyScoped($value, $scopes) private function isProperlySuffixed($value, $scopes) { foreach ($scopes as $scope) { - $scopeRegex = '/^[^@]*@(.*\.)?'.preg_quote($scope).'$/'; + $scopeRegex = '/^[^@]+@(.*\.)?'.preg_quote($scope).'$/'; $subdomainRegex = '/^([^@]*\.)?'.preg_quote($scope).'$/'; if (preg_match($subdomainRegex, $value) === 1 || preg_match($scopeRegex, $value) === 1) { return true; diff --git a/tests/lib/Auth/Process/FilterAttributesTest.php b/tests/lib/Auth/Process/FilterAttributesTest.php index fdb2168..69b11ce 100644 --- a/tests/lib/Auth/Process/FilterAttributesTest.php +++ b/tests/lib/Auth/Process/FilterAttributesTest.php @@ -75,7 +75,7 @@ public function testCorrectScope($source) $expectedData = array( 'eduPersonPrincipalName' => array('joe@example.com'), 'nonScopedAttribute' => array('not-removed'), - 'eduPersonScopedAffiliation' => array('student@example.com', 'staff@example.com', '@example.com'), + 'eduPersonScopedAffiliation' => array('student@example.com', 'staff@example.com'), 'schacHomeOrganization' => array('example.com') ); $config = array(); @@ -115,7 +115,8 @@ public function testMixedMultivaluedAttributes() 'faculty@abc.com', 'student@example.com', 'staff@other.com', - 'member@a@example.com' + 'member@a@example.com', + '@example.com' ), // schacHomeOrganization is required to be single valued and gets filtered out if multi-valued 'schacHomeOrganization' => array('abc.com', 'example.com', 'other.com') @@ -188,19 +189,20 @@ public function testAttributeSuffix() // Invalid values 'invalid-example.com', // not subdomain 'cexample.com', - 'examplecom' + 'examplecom', ), 'email' => array( // Valid values 'user@example.com', 'user@gsb.example.com', - '@example.com', - '@other.example.com', // Invalid values 'user@invalid-example.com', 'user@examplecom', 'user@cexample.com', - 'abc@efg@example.com' // double '@; + 'abc@efg@example.com', // double '@' + // scoped values need data before the '@' + '@example.com', + '@other.example.com', ), ), 'Source' => array( @@ -224,9 +226,6 @@ public function testAttributeSuffix() 'email' => array( 'user@example.com', 'user@gsb.example.com', - '@example.com', - '@other.example.com', - ), ); $this->assertEquals($expectedData, $attributes, "Incorrectly suffixed variables should be removed");