diff --git a/CHANGELOG.md b/CHANGELOG.md index e82ba79038..c973327e32 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,12 @@ More information about our release strategy can be found in the [Development Guidelines](https://github.com/OpenConext/OpenConext-engineblock/wiki/Development-Guidelines#release-notes) on the EngineBlock wiki. +## 7.2.1 + +Features: + +* Added `external_subject_id`, `email` and `attributes` to `SramInterruptFilter` request + ## 7.2.0 Maintenance: diff --git a/library/EngineBlock/Corto/Filter/Command/SramInterruptFilter.php b/library/EngineBlock/Corto/Filter/Command/SramInterruptFilter.php index 77e5e4d6a7..688f9264a5 100644 --- a/library/EngineBlock/Corto/Filter/Command/SramInterruptFilter.php +++ b/library/EngineBlock/Corto/Filter/Command/SramInterruptFilter.php @@ -100,6 +100,19 @@ public function execute(): void } } + // TODO: duplicated from EngineBlockBundle/AttributeAggregation/Dto/Request.php + private static function filterNonStringValuesFromAttributes($attributes) + { + return array_filter($attributes, function ($attributeValues) { + foreach ($attributeValues as $attributeValue) { + if (!is_string($attributeValue)) { + return false; // drop the whole attribute + } + } + return true; + }); + } + private function buildRequest(ServiceProvider $serviceProvider): AuthzRequest { $attributes = $this->getResponseAttributes(); @@ -107,17 +120,21 @@ private function buildRequest(ServiceProvider $serviceProvider): AuthzRequest $userId = $this->_collabPersonId ?? ""; $eppn = $attributes['urn:mace:dir:attribute-def:eduPersonPrincipalName'][0] ?? ""; + $externalSubjectId = $attributes['urn:oasis:names:tc:SAML:attribute:subject-id'][0] ?? ""; + $email = $attributes['urn:mace:dir:attribute-def:mail'] ?? []; $continueUrl = $this->_server->getUrl('SramInterruptService', '') . "?ID=$id"; $serviceId = $serviceProvider->entityId; $issuerId = $this->_identityProvider->entityId; - return new AuthzRequest( - $userId, - $eppn, - $continueUrl, - $serviceId, - $issuerId + userId: $userId, + eduPersonPrincipalName: $eppn, + externalSubjectId: $externalSubjectId, + email: $email, + continueUrl: $continueUrl, + serviceId: $serviceId, + issuerId: $issuerId, + attributes: self::filterNonStringValuesFromAttributes($attributes) ); } diff --git a/src/OpenConext/EngineBlockBundle/Sbs/Dto/AuthzRequest.php b/src/OpenConext/EngineBlockBundle/Sbs/Dto/AuthzRequest.php index d8a403ed07..611ab80273 100644 --- a/src/OpenConext/EngineBlockBundle/Sbs/Dto/AuthzRequest.php +++ b/src/OpenConext/EngineBlockBundle/Sbs/Dto/AuthzRequest.php @@ -26,15 +26,21 @@ class AuthzRequest implements JsonSerializable public function __construct( public readonly string $userId, public readonly string $eduPersonPrincipalName, + public readonly string $externalSubjectId, + public readonly array $email, public readonly string $continueUrl, public readonly string $serviceId, - public readonly string $issuerId + public readonly string $issuerId, + public readonly array $attributes ) { Assertion::string($userId, 'The userId must be a string.'); Assertion::string($eduPersonPrincipalName, 'The eduPersonPrincipalName must be a string.'); + Assertion::string($externalSubjectId, 'The externalSubjectId must be a string.'); + Assertion::isArray($email, 'The email must be an array.'); Assertion::string($continueUrl, 'The continueUrl must be a string.'); Assertion::string($serviceId, 'The serviceId must be a string.'); Assertion::string($issuerId, 'The issuerId must be a string.'); + Assertion::isArray($attributes, 'The attributes must be an array.'); } public function jsonSerialize() : array @@ -42,9 +48,12 @@ public function jsonSerialize() : array return [ 'user_id' => $this->userId, 'eppn' => $this->eduPersonPrincipalName, + 'external_subject_id' => $this->externalSubjectId, + 'email' => $this->email, 'continue_url' => $this->continueUrl, 'service_id' => $this->serviceId, - 'issuer_id' => $this->issuerId + 'issuer_id' => $this->issuerId, + 'attributes' => $this->attributes ]; } } diff --git a/tests/library/EngineBlock/Test/Corto/Filter/Command/SramInterruptFilterTest.php b/tests/library/EngineBlock/Test/Corto/Filter/Command/SramInterruptFilterTest.php index d532e1e9be..97265a45d3 100644 --- a/tests/library/EngineBlock/Test/Corto/Filter/Command/SramInterruptFilterTest.php +++ b/tests/library/EngineBlock/Test/Corto/Filter/Command/SramInterruptFilterTest.php @@ -26,6 +26,7 @@ use OpenConext\EngineBlockBundle\Sbs\AuthzResponse; use OpenConext\EngineBlockBundle\Sbs\SbsClientInterface; use OpenConext\EngineBlockBundle\Sbs\SbsAttributeMerger; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; use Psr\Log\NullLogger; use SAML2\Assertion; @@ -115,7 +116,11 @@ public function testItAddsNonceWhenMessageInterrupt(): void new NullLogger(), ); - $initialAttributes = ['urn:mace:dir:attribute-def:uid' => ['userIdValue']]; + $initialAttributes = [ + 'urn:mace:dir:attribute-def:uid' => ['userIdValue'], + 'urn:oasis:names:tc:SAML:attribute:subject-id' => ['subjectIdValue'], + 'urn:mace:dir:attribute-def:mail' => ['user@example.org'], + ]; $sramFilter->setResponseAttributes($initialAttributes); $server = Mockery::mock(EngineBlock_Corto_ProxyServer::class); @@ -138,16 +143,27 @@ public function testItAddsNonceWhenMessageInterrupt(): void ] ]); - $expectedRequest = new AuthzRequest('', '', 'https://example.org?ID=', 'spEntityId', 'idpEntityId'); + $expectedRequest = new AuthzRequest( + userId: '', + eduPersonPrincipalName: '', + externalSubjectId: 'subjectIdValue', + email: ['user@example.org'], + continueUrl: 'https://example.org?ID=', + serviceId: 'spEntityId', + issuerId: 'idpEntityId', + attributes: $initialAttributes, + ); $sbsClient->shouldReceive('authz') ->withArgs(function ($args) use ($expectedRequest) { - return $args->userId === $expectedRequest->userId && $args->eduPersonPrincipalName === $expectedRequest->eduPersonPrincipalName + && $args->externalSubjectId === $expectedRequest->externalSubjectId + && $args->email === $expectedRequest->email && strpos($args->continueUrl, $expectedRequest->continueUrl) === 0 && $args->serviceId === $expectedRequest->serviceId - && $args->issuerId === $expectedRequest->issuerId; + && $args->issuerId === $expectedRequest->issuerId + && $args->attributes === $expectedRequest->attributes; }) ->andReturn($response); @@ -181,7 +197,11 @@ public function testItAddsSramAttributesOnStatusAuthorized(): void new NullLogger() ); - $initialAttributes = ['urn:mace:dir:attribute-def:uid' => ['userIdValue']]; + $initialAttributes = [ + 'urn:mace:dir:attribute-def:uid' => ['userIdValue'], + 'urn:oasis:names:tc:SAML:attribute:subject-id' => ['subjectIdValue'], + 'urn:mace:dir:attribute-def:mail' => ['user@example.org'], + ]; $sramFilter->setResponseAttributes($initialAttributes); $server = Mockery::mock(EngineBlock_Corto_ProxyServer::class); @@ -206,16 +226,28 @@ public function testItAddsSramAttributesOnStatusAuthorized(): void ], ]); - $expectedRequest = new AuthzRequest('', '', 'https://example.org?ID=', 'spEntityId', 'idpEntityId'); + $expectedRequest = new AuthzRequest( + userId: '', + eduPersonPrincipalName: '', + externalSubjectId: 'subjectIdValue', + email: ['user@example.org'], + continueUrl: 'https://example.org?ID=', + serviceId: 'spEntityId', + issuerId: 'idpEntityId', + attributes: $initialAttributes, + ); $sbsClient->shouldReceive('authz') ->withArgs(function ($args) use ($expectedRequest) { return $args->userId === $expectedRequest->userId && $args->eduPersonPrincipalName === $expectedRequest->eduPersonPrincipalName + && $args->externalSubjectId === $expectedRequest->externalSubjectId + && $args->email === $expectedRequest->email && str_starts_with($args->continueUrl, $expectedRequest->continueUrl) && $args->serviceId === $expectedRequest->serviceId - && $args->issuerId === $expectedRequest->issuerId; + && $args->issuerId === $expectedRequest->issuerId + && $args->attributes === $expectedRequest->attributes; }) ->andReturn($response); @@ -231,6 +263,8 @@ public function testItAddsSramAttributesOnStatusAuthorized(): void $expectedAttributes = [ 'urn:mace:dir:attribute-def:uid' => ['userIdValue'], + 'urn:oasis:names:tc:SAML:attribute:subject-id' => ['subjectIdValue'], + 'urn:mace:dir:attribute-def:mail' => ['user@example.org'], 'urn:mace:dir:attribute-def:eduPersonEntitlement' => 'attributes', ]; @@ -282,6 +316,48 @@ public function testThrowsEngineBlockExceptionIfPolicyCannotBeChecked() $sramFilter->execute(); } + #[DataProvider('filterNonStringValuesProvider')] + public function testFilterNonStringValuesFromAttributes(array $input, array $expected): void + { + $method = new ReflectionMethod( + EngineBlock_Corto_Filter_Command_SramInterruptFilter::class, + 'filterNonStringValuesFromAttributes' + ); + + $this->assertSame($expected, $method->invoke(null, $input)); + } + + public static function filterNonStringValuesProvider(): array + { + return [ + // ---- happy paths: array of (possibly empty) arrays of strings ---- + 'empty attribute set' => [[], []], + 'single string value' => [['a' => ['x']], ['a' => ['x']]], + 'multiple string values' => [['a' => ['x', 'y']], ['a' => ['x', 'y']]], + 'empty inner array kept' => [['a' => []], ['a' => []]], + 'several string attributes' => [ + ['a' => ['x'], 'b' => ['y', 'z'], 'c' => []], + ['a' => ['x'], 'b' => ['y', 'z'], 'c' => []], + ], + + // ---- failure paths: offending attribute dropped ---- + 'inner has integer' => [['a' => [1]], []], + 'inner has float' => [['a' => [1.5]], []], + 'inner has bool' => [['a' => [true]], []], + 'inner has null' => [['a' => [null]], []], + 'inner has nested array' => [['a' => [['nested']]], []], + 'inner has object' => [['a' => [new stdClass()]], []], + 'inner mixed string + int' => [['a' => ['x', 1]], []], + 'inner mixed string + obj' => [['a' => ['x', new stdClass()]], []], + + // ---- mixed set: only offending attributes dropped, keys preserved ---- + 'keeps good drops bad' => [ + ['good' => ['x'], 'bad' => ['x', 2], 'empty' => []], + ['good' => ['x'], 'empty' => []], + ], + ]; + } + private function mockServiceProvider(string $entityId): ServiceProvider { $sp = Mockery::mock(ServiceProvider::class);