diff --git a/modules/saml/lib/Auth/Process/NameIDAttribute.php b/modules/saml/lib/Auth/Process/NameIDAttribute.php index 8020e30e41d1c3abff0b6e11af8759d58c5b1408..e33265b567e0a2690e44f536eb6ae01c896ee265 100644 --- a/modules/saml/lib/Auth/Process/NameIDAttribute.php +++ b/modules/saml/lib/Auth/Process/NameIDAttribute.php @@ -43,13 +43,13 @@ class NameIDAttribute extends \SimpleSAML\Auth\ProcessingFilter parent::__construct($config, $reserved); if (isset($config['attribute'])) { - $this->attribute = (string) $config['attribute']; + $this->attribute = strval($config['attribute']); } else { $this->attribute = 'nameid'; } if (isset($config['format'])) { - $format = (string) $config['format']; + $format = strval($config['format']); } else { $format = '%I!%S!%V'; } @@ -119,15 +119,16 @@ class NameIDAttribute extends \SimpleSAML\Auth\ProcessingFilter $rep = $state['saml:sp:NameID']; Assert::notNull($rep->getValue()); - $rep->{'%'} = '%'; - if ($rep->getFormat() !== null) { + + if ($rep->getFormat() === null) { $rep->setFormat(Constants::NAMEID_UNSPECIFIED); } - if ($rep->getNameQualifier() !== null) { - $rep->setNameQualifier($state['Source']['entityid']); + + if ($rep->getSPNameQualifier() === null) { + $rep->setSPNameQualifier($state['Source']['entityid']); } - if ($rep->getSPNameQualifier() !== null) { - $rep->setSPNameQualifier($state['Destination']['entityid']); + if ($rep->getNameQualifier() === null) { + $rep->setNameQualifier($state['Destination']['entityid']); } $value = ''; @@ -135,6 +136,8 @@ class NameIDAttribute extends \SimpleSAML\Auth\ProcessingFilter foreach ($this->format as $element) { if ($isString) { $value .= $element; + } elseif ($element === '%') { + $value .= '%'; } else { $value .= call_user_func([$rep, 'get' . $element]); } diff --git a/tests/modules/saml/lib/Auth/Process/NameIDAttributeTest.php b/tests/modules/saml/lib/Auth/Process/NameIDAttributeTest.php index 793d4f85a0ff4ee92d165f4623b5ef0ea76af1c1..fa45d32b42fa1a2345f636a6e5b90f246f61cc76 100644 --- a/tests/modules/saml/lib/Auth/Process/NameIDAttributeTest.php +++ b/tests/modules/saml/lib/Auth/Process/NameIDAttributeTest.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace SimpleSAML\Test\Module\saml\Auth\Process; use PHPUnit\Framework\TestCase; +use SimpleSAML\Error; use SimpleSAML\Module\saml\Auth\Process\NameIDAttribute; use SAML2\XML\saml\NameID; use SAML2\Constants; @@ -59,7 +60,7 @@ class NameIDAttributeTest extends TestCase 'saml:sp:NameID' => $nameId, ]; $result = $this->processFilter($config, $request); - $this->assertEquals("{$spId}!{$idpId}!{$nameId->getValue()}", $result['Attributes']['nameid'][0]); + $this->assertEquals("{$idpId}!{$spId}!{$nameId->getValue()}", $result['Attributes']['nameid'][0]); } @@ -91,7 +92,7 @@ class NameIDAttributeTest extends TestCase ]; $result = $this->processFilter($config, $request); $this->assertTrue(isset($result['Attributes'][$attributeName])); - $this->assertEquals("{$spId}!{$idpId}!{$nameId->getValue()}", $result['Attributes'][$attributeName][0]); + $this->assertEquals("{$idpId}!{$spId}!{$nameId->getValue()}", $result['Attributes'][$attributeName][0]); } @@ -101,7 +102,7 @@ class NameIDAttributeTest extends TestCase */ public function testFormat(): void { - $config = ['format' => '%V']; + $config = ['format' => '%V!%%']; $spId = 'eugeneSP'; $idpId = 'eugeneIdP'; @@ -121,7 +122,62 @@ class NameIDAttributeTest extends TestCase 'saml:sp:NameID' => $nameId, ]; $result = $this->processFilter($config, $request); - $this->assertEquals("{$nameId->getValue()}", $result['Attributes']['nameid'][0]); + $this->assertEquals("{$nameId->getValue()}!%", $result['Attributes']['nameid'][0]); + } + + + /** + * Test invalid format throws an exception. + * @return void + */ + public function testInvalidFormatThrowsException(): void + { + $config = ['format' => '%X']; + $spId = 'eugeneSP'; + $idpId = 'eugeneIdP'; + + $nameId = new NameID(); + $nameId->setValue('eugene@oombaas'); + + $request = [ + 'Source' => [ + 'entityid' => $spId, + ], + 'Destination' => [ + 'entityid' => $idpId, + ], + 'saml:sp:NameID' => $nameId, + ]; + + $this->expectException(Error\Exception::class); + $this->expectExceptionMessage('NameIDAttribute: Invalid replacement: "%X"'); + + $this->processFilter($config, $request); + } + + + /** + * Test invalid request silently continues, leaving the state untouched + * @return void + */ + public function testInvalidRequestLeavesStateUntouched(): void + { + $config = ['format' => '%V!%F']; + $spId = 'eugeneSP'; + $idpId = 'eugeneIdP'; + + $request = [ + 'Source' => [ + 'entityid' => $spId, + ], + 'Destination' => [ + 'entityid' => $idpId, + ], + ]; + + $pre = $request; + $this->processFilter($config, $request); + $this->assertEquals($pre, $request); } @@ -155,4 +211,32 @@ class NameIDAttributeTest extends TestCase $this->assertTrue(isset($result['Attributes'][$attributeName])); $this->assertEquals("{$nameId->getValue()}", $result['Attributes'][$attributeName][0]); } + + + /** + * Test overriding NameID Format/NameQualifier/SPNameQualifier with defaults. + * @return void + */ + public function testOverrideNameID(): void + { + $spId = 'eugeneSP'; + $idpId = 'eugeneIdP'; + + $nameId = new NameID(); + $nameId->setValue('eugene@oombaas'); + + $request = [ + 'Source' => [ + 'entityid' => $spId, + ], + 'Destination' => [ + 'entityid' => $idpId, + ], + 'saml:sp:NameID' => $nameId, + ]; + $this->processFilter(array(), $request); + $this->assertEquals("{$nameId->getFormat()}", Constants::NAMEID_UNSPECIFIED); + $this->assertEquals("{$nameId->getNameQualifier()}", $idpId); + $this->assertEquals("{$nameId->getSPNameQualifier()}", $spId); + } }