Skip to content

Commit

Permalink
Prevent adding empty renamed attribute
Browse files Browse the repository at this point in the history 
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: #1321
  • Loading branch information
@MKodde
MKodde committed Sep 5, 2024
1 parent 4828d21 commit 550c91a
Show file tree
Hide file tree
Showing 2 changed files with 121 additions and 1 deletion.
23 changes: 23 additions & 0 deletions src/OpenConext/EngineBlock/Service/ReleaseAsEnforcer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,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(
Expand Down
99 changes: 98 additions & 1 deletion tests/unit/OpenConext/EngineBlock/Service/ReleaseAsEnforcerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,20 @@ 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 [
Expand Down Expand Up @@ -88,6 +98,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" => ["[email protected]"]
],
'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" => ["[email protected]"],
"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"],
Expand Down Expand Up @@ -139,6 +177,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" => ["[email protected]"],
],
'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" => ["[email protected]"]
],
'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" => ["[email protected]"],
],
'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" => ["[email protected]"]
],
'expectedLogMessages' => 'Releasing "urn:mace:dir:attribute-def:eduPersonTargetedId" as "UserName" is not possible, value for "urn:mace:dir:attribute-def:eduPersonTargetedId" is null'
],
];
}
}

0 comments on commit 550c91a

Please sign in to comment.