Fix bug in saml:NameIDAttribute authproc filter (#1325)

Fix several bugs in saml:NameIDAttribute authproc filter & raise coverage

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]);
             }

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);
+    }
 }