From 19d1c814d0595bb1470fa9ebaa73181d29590299 Mon Sep 17 00:00:00 2001
From: Thijs Kinkhorst <thijs@kinkhorst.com>
Date: Fri, 6 Jan 2023 12:23:18 +0100
Subject: [PATCH] Rationalize NameIDFormat config. (#1741)
This was a mixture of legacy supported options, some of them
deprecated, and data types, and also implemented differently
in some places than others.
This changes it once more but hopefully for the better:
* The value is always an array or null/unset.
* You can set it to a specific array to get a specific policy.
* You can leave it unset/null to keep the default policy.
* You can set it to the empty array to signal that you do not
want any policy to be sent.
* The string and bool types are no longer allowed.
All in all this means that we can make code much simpler, a lot
less if-branching, and also typewise make use of correct typehints.
The behaviour changes are as follows:
- We drop the already deprecated option to set it as a string
(deprecated in 1.17).
- To not send the element you need to change false to [];
this was not deprecated before but I believe setting it to
false was already broken in master.
---
docs/simplesamlphp-reference-idp-remote.md | 8 +--
modules/saml/docs/sp.md | 8 +--
modules/saml/src/Auth/Source/SP.php | 28 ++-------
modules/saml/src/Message.php | 4 +-
src/SimpleSAML/Utils/Config/Metadata.php | 39 ++++++-------
tests/modules/saml/src/Auth/Source/SPTest.php | 21 +------
.../SimpleSAML/Utils/Config/MetadataTest.php | 58 ++++++++++++++-----
7 files changed, 78 insertions(+), 88 deletions(-)
diff --git a/docs/simplesamlphp-reference-idp-remote.md b/docs/simplesamlphp-reference-idp-remote.md
index fa517c370..4dc53b3e1 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 a868c6c70..6c5ee31f9 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 fb12fdfd3..916f7802d 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 3cbc6d69b..37c001a70 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 d47526213..95aeacb2a 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 a6f0431a1..ba591a93a 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 69f55762a..53701b15c 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)
+ );
+ }
}
--
GitLab