From 653e72cf7c555fb75bc841ef7302e46afa0dbeba Mon Sep 17 00:00:00 2001 From: Michiel Kodde Date: Thu, 5 Sep 2024 08:16:50 +0200 Subject: [PATCH] Prevent adding empty renamed attribute Issue: #1321 describes it perfectly. When Manage configures an override for an attribute name. But the targetted attribute is not in the assertion, then Engine would add an empty attribute with the overridden value to the assertion. The SAML2 lib would then warn us that an empty attribute value can not be processed. To fix that. I added a test statement that verifies if the targetted attribute is present in the assertion. If not, the substitution is not made and a warning is logged. It is not allowed to rename an attribute with a null value. It is allowed to rename an empty attribute. See: https://github.com/OpenConext/OpenConext-engineblock/issues/1321 --- .../EngineBlock/Service/ReleaseAsEnforcer.php | 24 +++++ .../Service/ReleaseAsEnforcerTest.php | 100 +++++++++++++++++- 2 files changed, 123 insertions(+), 1 deletion(-) diff --git a/src/OpenConext/EngineBlock/Service/ReleaseAsEnforcer.php b/src/OpenConext/EngineBlock/Service/ReleaseAsEnforcer.php index e4d6b306c..feba35abc 100644 --- a/src/OpenConext/EngineBlock/Service/ReleaseAsEnforcer.php +++ b/src/OpenConext/EngineBlock/Service/ReleaseAsEnforcer.php @@ -19,6 +19,7 @@ namespace OpenConext\EngineBlock\Service; use Psr\Log\LoggerInterface; +use function is_null; class ReleaseAsEnforcer implements ReleaseAsEnforcerInterface { @@ -37,6 +38,29 @@ public function enforce(array $attributes, array $releaseAsOverrides) { foreach ($releaseAsOverrides as $oldAttributeName => $overrideValue) { $newAttributeName = $overrideValue[0]['release_as']; + if (!array_key_exists($oldAttributeName, $attributes)) { + $this->logger->notice( + sprintf( + 'Releasing "%s" as "%s" is not possible, "%s" is not in assertion', + $oldAttributeName, + $newAttributeName, + $oldAttributeName + ) + ); + continue; + } + if (is_null($attributes[$oldAttributeName])) { + $this->logger->warning( + sprintf( + 'Releasing "%s" as "%s" is not possible, value for "%s" is null', + $oldAttributeName, + $newAttributeName, + $oldAttributeName + ) + ); + unset($attributes[$oldAttributeName]); + continue; + } $attributeValue = $attributes[$oldAttributeName]; unset($attributes[$oldAttributeName]); $this->logger->notice( diff --git a/tests/unit/OpenConext/EngineBlock/Service/ReleaseAsEnforcerTest.php b/tests/unit/OpenConext/EngineBlock/Service/ReleaseAsEnforcerTest.php index 6d2829342..39dc21223 100644 --- a/tests/unit/OpenConext/EngineBlock/Service/ReleaseAsEnforcerTest.php +++ b/tests/unit/OpenConext/EngineBlock/Service/ReleaseAsEnforcerTest.php @@ -53,10 +53,21 @@ public function testEnforce($attributes, $releaseAsOverrides, $expectedResult, $ foreach ($releaseAsOverrides as $oldName => $override) { $this->assertArrayNotHasKey($oldName, $result); - $this->assertArrayHasKey($override[0]['release_as'], $result); } } + + /** + * @dataProvider enforceDataProviderWarnings + */ + public function testEnforceImpossible($attributes, $releaseAsOverrides, $expectedResult, $expectedLogMessage) + { + + $this->logger->shouldReceive('warning')->with($expectedLogMessage); + $result = $this->enforcer->enforce($attributes, $releaseAsOverrides); + $this->assertEquals($expectedResult, $result); + } + public function enforceDataProvider() { return [ @@ -88,6 +99,34 @@ public function enforceDataProvider() 'Releasing attribute "urn:mace:dir:attribute-def:cn" as "ComonNaam" as specified in the release_as ARP setting' ] ], + 'single attribute override, empty attribute value is allowed' => [ + 'attributes' => [ + "urn:mace:dir:attribute-def:displayName" => ["Ad Doe"], + "urn:mace:dir:attribute-def:cn" => [], + "urn:mace:dir:attribute-def:sn" => ["Doe"], + "urn:mace:dir:attribute-def:givenName" => ["Ad"], + "urn:mace:dir:attribute-def:mail" => ["ad@example.com"] + ], + 'releaseAsOverrides' => [ + "urn:mace:dir:attribute-def:cn" => [ + [ + "value" => "*", + "release_as" => "ComonNaam", + "use_as_nameid" => false + ] + ] + ], + 'expectedResult' => [ + "urn:mace:dir:attribute-def:displayName" => ["Ad Doe"], + "urn:mace:dir:attribute-def:sn" => ["Doe"], + "urn:mace:dir:attribute-def:givenName" => ["Ad"], + "urn:mace:dir:attribute-def:mail" => ["ad@example.com"], + "ComonNaam" => [] + ], + 'expectedLogMessages' => [ + 'Releasing attribute "urn:mace:dir:attribute-def:cn" as "ComonNaam" as specified in the release_as ARP setting' + ] + ], 'multiple attribute overrides' => [ 'attributes' => [ "urn:mace:dir:attribute-def:displayName" => ["John Smith"], @@ -139,6 +178,65 @@ public function enforceDataProvider() 'expectedLogMessages' => [ ] ], + 'targeted attribute not in assertion' => [ + 'attributes' => [ + "urn:mace:dir:attribute-def:displayName" => ["Ad Doe"], + "urn:mace:dir:attribute-def:cn" => ["Ad Doe"], + "urn:mace:dir:attribute-def:sn" => ["Doe"], + "urn:mace:dir:attribute-def:givenName" => ["Ad"], + "urn:mace:dir:attribute-def:mail" => ["ad@example.com"], + ], + 'releaseAsOverrides' => [ + "urn:mace:dir:attribute-def:eduPersonTargetedId" => [ + [ + "value" => "*", + "release_as" => "UserName", + "use_as_nameid" => false + ] + ] + ], + 'expectedResult' => [ + "urn:mace:dir:attribute-def:displayName" => ["Ad Doe"], + "urn:mace:dir:attribute-def:cn" => ["Ad Doe"], + "urn:mace:dir:attribute-def:sn" => ["Doe"], + "urn:mace:dir:attribute-def:givenName" => ["Ad"], + "urn:mace:dir:attribute-def:mail" => ["ad@example.com"] + ], + 'expectedLogMessages' => ['Releasing "urn:mace:dir:attribute-def:eduPersonTargetedId" as "UserName" is not possible, "urn:mace:dir:attribute-def:eduPersonTargetedId" is not in assertion'] + ], + ]; + } + + public function enforceDataProviderWarnings() + { + return [ + 'targeted attribute value is set to null in assertion' => [ + 'attributes' => [ + "urn:mace:dir:attribute-def:displayName" => ["Ad Doe"], + "urn:mace:dir:attribute-def:cn" => ["Ad Doe"], + "urn:mace:dir:attribute-def:sn" => ["Doe"], + "urn:mace:dir:attribute-def:eduPersonTargetedId" => null, + "urn:mace:dir:attribute-def:givenName" => ["Ad"], + "urn:mace:dir:attribute-def:mail" => ["ad@example.com"], + ], + 'releaseAsOverrides' => [ + "urn:mace:dir:attribute-def:eduPersonTargetedId" => [ + [ + "value" => "*", + "release_as" => "UserName", + "use_as_nameid" => false + ] + ] + ], + 'expectedResult' => [ + "urn:mace:dir:attribute-def:displayName" => ["Ad Doe"], + "urn:mace:dir:attribute-def:cn" => ["Ad Doe"], + "urn:mace:dir:attribute-def:sn" => ["Doe"], + "urn:mace:dir:attribute-def:givenName" => ["Ad"], + "urn:mace:dir:attribute-def:mail" => ["ad@example.com"] + ], + 'expectedLogMessages' => 'Releasing "urn:mace:dir:attribute-def:eduPersonTargetedId" as "UserName" is not possible, value for "urn:mace:dir:attribute-def:eduPersonTargetedId" is null' + ], ]; } }