OpenConext / OpenConext-engineblock

OpenConext SAML 2.0 IdP/SP Gateway
14 stars 22 forks source link

ResponseAnnotationDecorator::setCustomNameId() must be of the type array, object given #1447

Closed phavekes closed 4 days ago

phavekes commented 4 days ago

This issue is imported from pivotal - Originaly created at Mar 5, 2018 by Thijs Kinkhorst

To an SP set Attribute Manipulation code to:

// Restore the NameID that was used in the response by the IdP.
$response[\'__\'][\'CustomNameId\'] = $response[\'__\'][\'OriginalResponse\'][\'__\'][\'OriginalNameId\'];

and log in.

Be presented with: Catchable fatal error: Argument 1 passed to EngineBlock_Saml2_ResponseAnnotationDecorator::setCustomNameId() must be of the type array, object given, called in /opt/openconext/OpenConext-engineblock-5.6.2/library/EngineBlock/Corto/Mapper/Legacy/ResponseTranslator.php on line 101 and defined in /opt/openconext/OpenConext-engineblock-5.6.2/library/EngineBlock/Saml2/ResponseAnnotationDecorator.php on line 183

The given code is a quite commonly used attribute manipulation code in production (12 occurences). Might be worthwhile to explicitly test for this construct in the testsuite?

phavekes commented 4 days ago
@thijskh The PR attached in the \'CODE\' section of the story will provide support for old and new style NameId\'s (array and object). A seamless upgrade/rollback in this area is guaranteed.

Thanks to some digging in the AM code that was in production recently, we found another possible issue with NameID\'s.

Consider this manipulation code:

$attributes[\'urn:mace:dir:attribute-def:uid\'] = array($response[\'__\'][\'OriginalResponse\'][\'__\'][\'OriginalNameId\'][\'Value\']) ;

This will fail in 5.6 as the NameId is represented by an object. To make this code EB version agnostic, I propose the following change to the manipulation code:

$originalNameId = $response[\'__\'][\'OriginalResponse\'][\'__\'][\'OriginalNameId\'];

if (is_array($originalNameId)) { 
    $attributes[\'urn:mace:dir:attribute-def:uid\'] = $originalNameId[\'Value\']);
} else { 
    $attributes[\'urn:mace:dir:attribute-def:uid\'] = $originalNameId->value);
}

PS: this construction is in place for SP: http://www.surfspot.nl/surfspot2/sp (Michiel Kodde - Mar 8, 2018)

phavekes commented 4 days ago

Thanks. That SP is no longer in use. I\'ll ask to have it removed. (Thijs Kinkhorst - Mar 8, 2018)