diff --git a/docs/simplesamlphp-reference-idp-remote.md b/docs/simplesamlphp-reference-idp-remote.md index fa517c37002bad5b89cbc5147fe52aea3cca8c1d..4dc53b3e1fa02ac699d96018945d51f1ec07e605 100644 --- a/docs/simplesamlphp-reference-idp-remote.md +++ b/docs/simplesamlphp-reference-idp-remote.md @@ -111,11 +111,9 @@ $metadata['entity-id-2'] = [ `NameIDPolicy` : The format of the NameID we request from this IdP: an array in the form of `[ 'Format' => the format, 'AllowCreate' => true or false ]`. - Set to `false` instead of an array to omit sending any specific NameIDPolicy - in the AuthnRequest. - -: For compatibility purposes, `null` is equivalent to Transient and a format - can be defined as a string instead of an array. These variants are deprecated. + Set to an empty array `[]` to omit sending any specific NameIDPolicy element + in the AuthnRequest. When the entire option or either array key is unset, + the defaults are transient and true respectively. `OrganizationName` : The name of the organization responsible for this SPP. diff --git a/modules/saml/docs/sp.md b/modules/saml/docs/sp.md index a868c6c70d8627c11e70f5103d62569dd31202aa..6c5ee31f93c288ff00f6951fc3a60b5f73432c1a 100644 --- a/modules/saml/docs/sp.md +++ b/modules/saml/docs/sp.md @@ -266,11 +266,9 @@ The following attributes are available: `NameIDPolicy` : The format of the NameID we request from the idp: an array in the form of `[ 'Format' => the format, 'AllowCreate' => true or false ]`. - Set to `false` instead of an array to omit sending any specific NameIDPolicy - in the AuthnRequest. - -: For compatibility purposes, `null` is equivalent to transient and a format - can be defined as a string instead of an array. These variants are deprecated. + Set to an empty array `[]` to omit sending any specific NameIDPolicy element + in the AuthnRequest. When the entire option or either array key is unset, + the defaults are transient and true respectively. `OrganizationName`, `OrganizationDisplayName`, `OrganizationURL` : The name and URL of the organization responsible for this IdP. diff --git a/modules/saml/src/Auth/Source/SP.php b/modules/saml/src/Auth/Source/SP.php index fb12fdfd3234586e95f258374cc1b0cce43143cd..916f7802d79101468ccf2fcc6d1870425314c1b3 100644 --- a/modules/saml/src/Auth/Source/SP.php +++ b/modules/saml/src/Auth/Source/SP.php @@ -159,14 +159,9 @@ class SP extends \SimpleSAML\Auth\Source // add NameIDPolicy if ($this->metadata->hasValue('NameIDPolicy')) { - $format = $this->metadata->getValue('NameIDPolicy'); - if (is_array($format)) { - $metadata['NameIDFormat'] = Configuration::loadFromArray($format)->getOptionalString( - 'Format', - Constants::NAMEID_TRANSIENT - ); - } elseif (is_string($format)) { - $metadata['NameIDFormat'] = $format; + $format = $this->metadata->getArray('NameIDPolicy'); + if ($format !== []) { + $metadata['NameIDFormat'] = $format['Format'] ?? Constants::NAMEID_TRANSIENT; } } @@ -558,21 +553,8 @@ class SP extends \SimpleSAML\Auth\Source $ar->setNameId($nid); } - if (isset($state['saml:NameIDPolicy'])) { - $policy = null; - if (is_string($state['saml:NameIDPolicy'])) { - $policy = [ - 'Format' => $state['saml:NameIDPolicy'], - 'AllowCreate' => true, - ]; - } elseif (is_array($state['saml:NameIDPolicy'])) { - $policy = $state['saml:NameIDPolicy']; - } elseif ($state['saml:NameIDPolicy'] === null) { - $policy = ['Format' => Constants::NAMEID_TRANSIENT]; - } - if ($policy !== null) { - $ar->setNameIdPolicy($policy); - } + if (!empty($state['saml:NameIDPolicy'])) { + $ar->setNameIdPolicy($policy); } $requesterID = []; diff --git a/modules/saml/src/Message.php b/modules/saml/src/Message.php index 3cbc6d69b65d8dfd8b21c6c39513eda85dfc266c..37c001a70935ff0d62406c36de5720fe741944f6 100644 --- a/modules/saml/src/Message.php +++ b/modules/saml/src/Message.php @@ -477,8 +477,8 @@ class Message } $policy = Utils\Config\Metadata::parseNameIdPolicy($nameIdPolicy); - if ($policy !== null) { - // either we have a policy set, or we used the transient default + // empty array signals not to set any NameIdPolicy element + if ($policy !== []) { $ar->setNameIdPolicy($policy); } diff --git a/src/SimpleSAML/Utils/Config/Metadata.php b/src/SimpleSAML/Utils/Config/Metadata.php index d47526213791eb83fdf44b6ab5ac338aa3338934..95aeacb2af7187e81037fc0cc4f7a564fd29cf75 100644 --- a/src/SimpleSAML/Utils/Config/Metadata.php +++ b/src/SimpleSAML/Utils/Config/Metadata.php @@ -261,29 +261,28 @@ class Metadata /** * This method parses the different possible values of the NameIDPolicy metadata configuration. - * - * @param null|array|false $nameIdPolicy - * - * @return null|array */ - public static function parseNameIdPolicy($nameIdPolicy): ?array + public static function parseNameIdPolicy(array $nameIdPolicy = null): array { - $policy = null; + if ($nameIdPolicy === null) { + // when NameIDPolicy is unset or set to null, default to transient + return ['Format' => Constants::NAMEID_TRANSIENT, 'AllowCreate' => true]; + } - if (is_array($nameIdPolicy)) { - // handle current configurations specifying an array in the NameIDPolicy config option - $nameIdPolicy_cf = Configuration::loadFromArray($nameIdPolicy); - $policy = [ - 'Format' => $nameIdPolicy_cf->getOptionalString('Format', Constants::NAMEID_TRANSIENT), - 'AllowCreate' => $nameIdPolicy_cf->getOptionalBoolean('AllowCreate', true), - ]; - $spNameQualifier = $nameIdPolicy_cf->getOptionalString('SPNameQualifier', null); - if ($spNameQualifier !== null) { - $policy['SPNameQualifier'] = $spNameQualifier; - } - } elseif ($nameIdPolicy === null) { - // when NameIDPolicy is unset or set to null, default to transient as before - $policy = ['Format' => Constants::NAMEID_TRANSIENT, 'AllowCreate' => true]; + if ($nameIdPolicy === []) { + // empty array means not to send any NameIDPolicy element + return []; + } + + // handle configurations specifying an array in the NameIDPolicy config option + $nameIdPolicy_cf = Configuration::loadFromArray($nameIdPolicy); + $policy = [ + 'Format' => $nameIdPolicy_cf->getOptionalString('Format', Constants::NAMEID_TRANSIENT), + 'AllowCreate' => $nameIdPolicy_cf->getOptionalBoolean('AllowCreate', true), + ]; + $spNameQualifier = $nameIdPolicy_cf->getOptionalString('SPNameQualifier', null); + if ($spNameQualifier !== null) { + $policy['SPNameQualifier'] = $spNameQualifier; } return $policy; diff --git a/tests/modules/saml/src/Auth/Source/SPTest.php b/tests/modules/saml/src/Auth/Source/SPTest.php index a6f0431a113a09f9da335127b2188da153424f43..ba591a93ae10765d77cacdcb90b52564719cb1f1 100644 --- a/tests/modules/saml/src/Auth/Source/SPTest.php +++ b/tests/modules/saml/src/Auth/Source/SPTest.php @@ -1077,26 +1077,7 @@ class SPTest extends ClearStateTestCase } /** - * SP config option NameIDPolicy specified in legacy string form is reflected in metadata - */ - public function testMetadataHostedNameIDPolicyString(): void - { - $spId = 'myhosted-sp'; - $info = ['AuthId' => $spId]; - - $config = [ - 'entityID' => 'urn:x-simplesamlphp:example-sp', - 'NameIDPolicy' => 'urn:mace:shibboleth:1.0:nameIdentifier', - ]; - $as = new SpTester($info, $config); - - $md = $as->getHostedMetadata(); - $this->assertArrayHasKey('NameIDFormat', $md); - $this->assertEquals('urn:mace:shibboleth:1.0:nameIdentifier', $md['NameIDFormat']); - } - - /** - * SP config option NameIDPolicy specified in deprecated form without Format is reflected in metadata + * SP config option NameIDPolicy specified without Format is reflected in metadata */ public function testMetadataHostedNameIDPolicyNullFormat(): void { diff --git a/tests/src/SimpleSAML/Utils/Config/MetadataTest.php b/tests/src/SimpleSAML/Utils/Config/MetadataTest.php index 69f55762a05417f0c8146ada46da449bb6a435dc..53701b15ccca171c5e2bfb19d192ea45531cf376 100644 --- a/tests/src/SimpleSAML/Utils/Config/MetadataTest.php +++ b/tests/src/SimpleSAML/Utils/Config/MetadataTest.php @@ -15,7 +15,7 @@ use TypeError; /** * Tests related to SAML metadata. * - * @covers \SimpleSAML\Utils\Config + * @covers \SimpleSAML\Utils\Config\Metadata */ class MetadataTest extends TestCase { @@ -222,21 +222,10 @@ class MetadataTest extends TestCase /** * Test \SimpleSAML\Utils\Config\Metadata::parseNameIdPolicy(). + * Set to specific arrays. */ public function testParseNameIdPolicy(): void { - // Test null or unset - $nameIdPolicy = null; - $this->assertEquals( - ['Format' => Constants::NAMEID_TRANSIENT, 'AllowCreate' => true], - Metadata::parseNameIdPolicy($nameIdPolicy) - ); - - // Test false - $nameIdPolicy = false; - $this->assertEquals(null, Metadata::parseNameIdPolicy($nameIdPolicy)); - - // Test array $nameIdPolicy = [ 'Format' => 'urn:oasis:names:tc:SAML:1.1:nameid-format:persistent', 'AllowCreate' => false @@ -257,4 +246,47 @@ class MetadataTest extends TestCase 'SPNameQualifier' => 'TEST' ], Metadata::parseNameIdPolicy($nameIdPolicy)); } + + /** + * Test \SimpleSAML\Utils\Config\Metadata::parseNameIdPolicy(). + * Test with settings that produce the fallback defaults. + */ + public function testParseNameIdPolicyDefaults(): void + { + // Test null or unset + $nameIdPolicy = null; + $this->assertEquals([ + 'Format' => Constants::NAMEID_TRANSIENT, + 'AllowCreate' => true + ], Metadata::parseNameIdPolicy($nameIdPolicy)); + + $nameIdPolicy = [ + 'Format' => 'urn:oasis:names:tc:SAML:1.1:nameid-format:persistent', + ]; + $this->assertEquals([ + 'Format' => 'urn:oasis:names:tc:SAML:1.1:nameid-format:persistent', + 'AllowCreate' => true + ], Metadata::parseNameIdPolicy($nameIdPolicy)); + + $nameIdPolicy = [ + 'AllowCreate' => false, + ]; + $this->assertEquals([ + 'Format' => Constants::NAMEID_TRANSIENT, + 'AllowCreate' => false + ], Metadata::parseNameIdPolicy($nameIdPolicy)); + } + + /** + * Test \SimpleSAML\Utils\Config\Metadata::parseNameIdPolicy(). + * Test with setting to empty array (meaning to not send any NameIdPolicy). + */ + public function testParseNameIdPolicyEmpty(): void + { + $nameIdPolicy = []; + $this->assertEquals( + [], + Metadata::parseNameIdPolicy($nameIdPolicy) + ); + } }