Prevent adding empty renamed attribute #1322
Conversation
MKodde
force-pushed
the
bugfix/prevent-invalid-release-as
branch
from
272a819 to
0d700e4
Compare
| $oldAttributeName | ||
| ) | ||
| ); | ||
| unset($attributes[$oldAttributeName]); |
There was a problem hiding this comment.
@thijskh in this testcase; edupersontargetedId is in the assertion but is set to be an empty array as value.
The enforcer now tests if the value is empty, and pops it out of the assertion. Is that expected behavior for Engine? I think, prior to this change. EB would allow this.
There was a problem hiding this comment.
Indeed. The problem only occurs when the attribute is completely unset. So I would limit the solution to that only.
| $oldAttributeName | ||
| ) | ||
| ); | ||
| unset($attributes[$oldAttributeName]); |
There was a problem hiding this comment.
Indeed. The problem only occurs when the attribute is completely unset. So I would limit the solution to that only.
| @@ -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->warning( | |||
There was a problem hiding this comment.
I think this is in principle also 'normal' behaviour. The ARP specification of release_as basically should be read as "If this attribute is released, then use this name". So if it's not present it should not be causing warning level messages. Info is probably fine.
MKodde
force-pushed
the
bugfix/prevent-invalid-release-as
branch
from
0d700e4 to
4a1307c
Compare
MKodde
force-pushed
the
bugfix/prevent-invalid-release-as
branch
from
4a1307c to
653e72c
Compare
tests/unit/OpenConext/EngineBlock/Service/ReleaseAsEnforcerTest.php
Outdated
Show resolved
Hide resolved
MKodde
force-pushed
the
bugfix/prevent-invalid-release-as
branch
from
653e72c to
9f9ca82
Compare
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
MKodde
force-pushed
the
bugfix/prevent-invalid-release-as
branch
from
9f9ca82 to
550c91a
Compare
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.
See, Fixes: #1321