From 58c7411a0b223b5d39e8dcb6be9d4ffcb958de8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frank=20Tam=C3=A1s?= Date: Sun, 24 Aug 2025 12:35:40 +0200 Subject: [PATCH 1/5] Add Service Provider entity ID based filtering Allow restricting authorization rules to specific SPs by adding spEntityIDs arrays to attribute configurations. Rules with spEntityIDs only apply when the current SP matches the allowed list, enabling fine-grained access control per service provider. --- docs/authorize.md | 19 ++++ src/Auth/Process/Authorize.php | 49 +++++++++- tests/src/Auth/Process/AuthorizeTest.php | 119 +++++++++++++++++++++++ 3 files changed, 185 insertions(+), 2 deletions(-) diff --git a/docs/authorize.md b/docs/authorize.md index 9a7bb67..9986daa 100644 --- a/docs/authorize.md +++ b/docs/authorize.md @@ -155,4 +155,23 @@ Additionally, some helpful instructions are shown. ], ], ], + +You can restrict an attribute allowance to a list of Service Providers. + +```php +'authproc.sp' => [ + 60 => array[ + 'class' => 'authorize:Authorize', + 'uid' => [ + '/.*@students.example.edu$/', + '/^(stu1|stu2|stu3)@example.edu$/', + 'spEntityIDs' => [ + 'https://example.com/sp1', + 'https://example.com/sp2' + ] + ] + ] +] +``` + ``` diff --git a/src/Auth/Process/Authorize.php b/src/Auth/Process/Authorize.php index e9366b2..07665a0 100644 --- a/src/Auth/Process/Authorize.php +++ b/src/Auth/Process/Authorize.php @@ -61,6 +61,7 @@ class Authorize extends Auth\ProcessingFilter /** * Array of valid users. Each element is a regular expression. You should * use \ to escape special chars, like '.' etc. + * can also contain 'spEntityIDs' arrays to restrict rules to specific SPs. * * @var array */ @@ -137,6 +138,19 @@ public function __construct(array $config, $reserved) )); } + // Extract spEntityIDs if present + $spEntityIDs = null; + if (isset($values['spEntityIDs'])) { + if (!is_array($values['spEntityIDs'])) { + throw new Exception(sprintf( + 'Filter Authorize: spEntityIDs must be an array for attribute: %s', + var_export($attribute, true), + )); + } + $spEntityIDs = $values['spEntityIDs']; + unset($values['spEntityIDs']); + } + foreach ($values as $value) { if (!is_string($value)) { throw new Exception(sprintf( @@ -147,7 +161,20 @@ public function __construct(array $config, $reserved) )); } } - $this->valid_attribute_values[$attribute] = $values; + + // For backward compatibility, if no spEntityIDs were found, + // store the values directly in the old format + if ($spEntityIDs === null) { + $this->valid_attribute_values[$attribute] = [ + 'values' => $values, + 'spEntityIDs' => null, + ]; + } else { + $this->valid_attribute_values[$attribute] = [ + 'values' => $values, + 'spEntityIDs' => $spEntityIDs, + ]; + } } } @@ -171,9 +198,27 @@ public function process(array &$state): void } $state['authprocAuthorize_errorURL'] = $this->errorURL; $state['authprocAuthorize_allow_reauthentication'] = $this->allow_reauthentication; + // Get current SP EntityID from state + $currentSpEntityId = null; + if (isset($state['saml:sp:State']['core:SP'])) { + $currentSpEntityId = $state['saml:sp:State']['core:SP']; + } elseif (isset($state['Destination']['entityid'])) { + $currentSpEntityId = $state['Destination']['entityid']; + } + $arrayUtils = new Utils\Arrays(); - foreach ($this->valid_attribute_values as $name => $patterns) { + foreach ($this->valid_attribute_values as $name => $ruleConfig) { if (array_key_exists($name, $attributes)) { + $patterns = $ruleConfig['values']; + $spEntityIDs = $ruleConfig['spEntityIDs']; + + // If spEntityIDs is specified, check if current SP is in the list + if ($spEntityIDs !== null) { + if ($currentSpEntityId === null || !in_array($currentSpEntityId, $spEntityIDs, true)) { + continue; // Skip this rule if SP is not specified or not in allowed list + } + } + foreach ($patterns as $pattern) { $values = $arrayUtils->arrayize($attributes[$name]); foreach ($values as $value) { diff --git a/tests/src/Auth/Process/AuthorizeTest.php b/tests/src/Auth/Process/AuthorizeTest.php index a5e4755..27cd27f 100644 --- a/tests/src/Auth/Process/AuthorizeTest.php +++ b/tests/src/Auth/Process/AuthorizeTest.php @@ -240,4 +240,123 @@ public static function showUserAttributeScenarioProvider(): array [['uid' => 'stu3@example.edu', 'mail' => 'user@example.edu'], false, true, 'user@example.edu'], ]; } + + /** + * Test SP restriction functionality + * + * @param array $userAttributes The attributes to test + * @param string|null $spEntityId The SP Entity ID in the state + * @param bool $isAuthorized Should the user be authorized + */ + #[DataProvider('spRestrictionScenarioProvider')] + public function testSpRestriction(array $userAttributes, ?string $spEntityId, bool $isAuthorized): void + { + $attributeUtils = new Utils\Attributes(); + $userAttributes = $attributeUtils->normalizeAttributesArray($userAttributes); + $config = [ + 'uid' => [ + '/.*@example.com$/', + 'spEntityIDs' => [ + 'https://sp1.example.com', + 'https://sp2.example.com', + ], + ], + 'group' => [ + '/^admins$/', + 'spEntityIDs' => [ + 'https://admin.example.com', + ], + ], + ]; + + $state = ['Attributes' => $userAttributes]; + if ($spEntityId !== null) { + $state['saml:sp:State']['core:SP'] = $spEntityId; + } + + $resultState = $this->processFilter($config, $state); + $resultAuthorized = isset($resultState['NOT_AUTHORIZED']) ? false : true; + $this->assertEquals($isAuthorized, $resultAuthorized); + } + + /** + * @return array + */ + public static function spRestrictionScenarioProvider(): array + { + return [ + // Should be allowed - matching attribute and SP + [['uid' => 'user@example.com'], 'https://sp1.example.com', true], + [['uid' => 'user@example.com'], 'https://sp2.example.com', true], + [['group' => 'admins'], 'https://admin.example.com', true], + + // Should be denied - matching attribute but wrong SP + [['uid' => 'user@example.com'], 'https://wrong.example.com', false], + [['group' => 'admins'], 'https://sp1.example.com', false], + + // Should be denied - no SP specified but attribute would match + [['uid' => 'user@example.com'], null, false], + [['group' => 'admins'], null, false], + + // Should be denied - wrong attribute regardless of SP + [['uid' => 'user@wrong.com'], 'https://sp1.example.com', false], + [['group' => 'users'], 'https://admin.example.com', false], + ]; + } + + /** + * Test mixed SP and non-SP rules + * + * @param array $userAttributes The attributes to test + * @param string|null $spEntityId The SP Entity ID in the state + * @param bool $isAuthorized Should the user be authorized + */ + #[DataProvider('mixedRulesScenarioProvider')] + public function testMixedSpAndNonSpRules(array $userAttributes, ?string $spEntityId, bool $isAuthorized): void + { + $attributeUtils = new Utils\Attributes(); + $userAttributes = $attributeUtils->normalizeAttributesArray($userAttributes); + $config = [ + // Rule with SP restriction + 'uid' => [ + '/.*@restricted.com$/', + 'spEntityIDs' => ['https://restricted.example.com'], + ], + // Rule without SP restriction (should work for all SPs) + 'role' => [ + '/^admin$/', + '/^superuser$/', + ], + ]; + + $state = ['Attributes' => $userAttributes]; + if ($spEntityId !== null) { + $state['saml:sp:State']['core:SP'] = $spEntityId; + } + + $resultState = $this->processFilter($config, $state); + $resultAuthorized = isset($resultState['NOT_AUTHORIZED']) ? false : true; + $this->assertEquals($isAuthorized, $resultAuthorized); + } + + /** + * @return array + */ + public static function mixedRulesScenarioProvider(): array + { + return [ + // Should be allowed - role rule matches (no SP restriction) + [['role' => 'admin'], 'https://any.example.com', true], + [['role' => 'superuser'], null, true], + + // Should be allowed - uid rule matches and SP is correct + [['uid' => 'user@restricted.com'], 'https://restricted.example.com', true], + + // Should be denied - uid rule matches but SP is wrong + [['uid' => 'user@restricted.com'], 'https://other.example.com', false], + + // Should be denied - no matching rules + [['uid' => 'user@other.com', 'role' => 'user'], 'https://any.example.com', false], + ]; + } } From 1c15d8e5ee717efb1eb25d696f976edf1b0a2a9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frank=20Tam=C3=A1s?= Date: Mon, 25 Aug 2025 07:16:54 +0200 Subject: [PATCH 2/5] fix: remove unnecessary conditional block --- src/Auth/Process/Authorize.php | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/src/Auth/Process/Authorize.php b/src/Auth/Process/Authorize.php index 07665a0..48e1a75 100644 --- a/src/Auth/Process/Authorize.php +++ b/src/Auth/Process/Authorize.php @@ -162,19 +162,10 @@ public function __construct(array $config, $reserved) } } - // For backward compatibility, if no spEntityIDs were found, - // store the values directly in the old format - if ($spEntityIDs === null) { - $this->valid_attribute_values[$attribute] = [ - 'values' => $values, - 'spEntityIDs' => null, - ]; - } else { - $this->valid_attribute_values[$attribute] = [ - 'values' => $values, - 'spEntityIDs' => $spEntityIDs, - ]; - } + $this->valid_attribute_values[$attribute] = [ + 'values' => $values, + 'spEntityIDs' => $spEntityIDs, + ]; } } From 09353f7d1f3a80de4a594d03815907f2a868c4d6 Mon Sep 17 00:00:00 2001 From: Tim van Dijen Date: Mon, 25 Aug 2025 08:54:28 +0200 Subject: [PATCH 3/5] Fix markdown --- docs/authorize.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/authorize.md b/docs/authorize.md index 9986daa..a4a719a 100644 --- a/docs/authorize.md +++ b/docs/authorize.md @@ -155,6 +155,7 @@ Additionally, some helpful instructions are shown. ], ], ], +``` You can restrict an attribute allowance to a list of Service Providers. @@ -173,5 +174,3 @@ You can restrict an attribute allowance to a list of Service Providers. ] ] ``` - -``` From 647938283926abe8fd54be3782726a590d1e41a0 Mon Sep 17 00:00:00 2001 From: Tim van Dijen Date: Mon, 25 Aug 2025 21:43:55 +0200 Subject: [PATCH 4/5] Throw custom exception --- src/Auth/Process/Authorize.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Auth/Process/Authorize.php b/src/Auth/Process/Authorize.php index 48e1a75..127be9f 100644 --- a/src/Auth/Process/Authorize.php +++ b/src/Auth/Process/Authorize.php @@ -4,9 +4,9 @@ namespace SimpleSAML\Module\authorize\Auth\Process; -use Exception; use SimpleSAML\Assert\Assert; use SimpleSAML\Auth; +use SimpleSAML\Error; use SimpleSAML\Module; use SimpleSAML\Utils; @@ -132,7 +132,7 @@ public function __construct(array $config, $reserved) $arrayUtils = new Utils\Arrays(); $values = $arrayUtils->arrayize($values); } elseif (!is_array($values)) { - throw new Exception(sprintf( + throw new Error\Exception(sprintf( 'Filter Authorize: Attribute values is neither string nor array: %s', var_export($attribute, true), )); @@ -142,7 +142,7 @@ public function __construct(array $config, $reserved) $spEntityIDs = null; if (isset($values['spEntityIDs'])) { if (!is_array($values['spEntityIDs'])) { - throw new Exception(sprintf( + throw new Error\Exception(sprintf( 'Filter Authorize: spEntityIDs must be an array for attribute: %s', var_export($attribute, true), )); @@ -153,7 +153,7 @@ public function __construct(array $config, $reserved) foreach ($values as $value) { if (!is_string($value)) { - throw new Exception(sprintf( + throw new Error\Exception(sprintf( 'Filter Authorize: Each value should be a string for attribute: %s value: %s config: %s', var_export($attribute, true), var_export($value, true), From a08dbd8e65a66a3fda71df964495ac8d12e22d15 Mon Sep 17 00:00:00 2001 From: Tim van Dijen Date: Mon, 25 Aug 2025 21:52:35 +0200 Subject: [PATCH 5/5] Improved error handling: test entityIDs --- composer.json | 2 +- src/Auth/Process/Authorize.php | 14 +++++++++----- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/composer.json b/composer.json index 1b30c3c..a5af04e 100644 --- a/composer.json +++ b/composer.json @@ -36,7 +36,7 @@ }, "require": { "php": "^8.1", - "simplesamlphp/assert": "^1.0", + "simplesamlphp/saml2": "~5.0.2", "simplesamlphp/simplesamlphp": "^2.1", "symfony/http-foundation": "^6.4" }, diff --git a/src/Auth/Process/Authorize.php b/src/Auth/Process/Authorize.php index 127be9f..3c7bb80 100644 --- a/src/Auth/Process/Authorize.php +++ b/src/Auth/Process/Authorize.php @@ -4,10 +4,10 @@ namespace SimpleSAML\Module\authorize\Auth\Process; -use SimpleSAML\Assert\Assert; use SimpleSAML\Auth; use SimpleSAML\Error; use SimpleSAML\Module; +use SimpleSAML\SAML2\Assert\Assert; use SimpleSAML\Utils; use function array_diff; @@ -141,12 +141,16 @@ public function __construct(array $config, $reserved) // Extract spEntityIDs if present $spEntityIDs = null; if (isset($values['spEntityIDs'])) { - if (!is_array($values['spEntityIDs'])) { - throw new Error\Exception(sprintf( + Assert::isArray( + $values['spEntityIDs'], + sprintf( 'Filter Authorize: spEntityIDs must be an array for attribute: %s', var_export($attribute, true), - )); - } + ), + Error\Exception::class, + ); + Assert::allValidEntityID($values['spEntityIDs']); + $spEntityIDs = $values['spEntityIDs']; unset($values['spEntityIDs']); }