Skip to content

Commit 19d1c81

Browse files
thijskhthijskh
authored
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.
1 parent cb0e9ac commit 19d1c81

7 files changed

Lines changed: 78 additions & 88 deletions

File tree

‎docs/simplesamlphp-reference-idp-remote.md‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -111,11 +111,9 @@ $metadata['entity-id-2'] = [
111111
`NameIDPolicy`
112112
: The format of the NameID we request from this IdP: an array in the form of
113113
`[ 'Format' => the format, 'AllowCreate' => true or false ]`.
114-
Set to `false` instead of an array to omit sending any specific NameIDPolicy
115-
in the AuthnRequest.
116-
117-
: For compatibility purposes, `null` is equivalent to Transient and a format
118-
can be defined as a string instead of an array. These variants are deprecated.
114+
Set to an empty array `[]` to omit sending any specific NameIDPolicy element
115+
in the AuthnRequest. When the entire option or either array key is unset,
116+
the defaults are transient and true respectively.
119117

120118
`OrganizationName`
121119
: The name of the organization responsible for this SPP.

‎modules/saml/docs/sp.md‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -266,11 +266,9 @@ The following attributes are available:
266266
`NameIDPolicy`
267267
: The format of the NameID we request from the idp: an array in the form of
268268
`[ 'Format' => the format, 'AllowCreate' => true or false ]`.
269-
Set to `false` instead of an array to omit sending any specific NameIDPolicy
270-
in the AuthnRequest.
271-
272-
: For compatibility purposes, `null` is equivalent to transient and a format
273-
can be defined as a string instead of an array. These variants are deprecated.
269+
Set to an empty array `[]` to omit sending any specific NameIDPolicy element
270+
in the AuthnRequest. When the entire option or either array key is unset,
271+
the defaults are transient and true respectively.
274272

275273
`OrganizationName`, `OrganizationDisplayName`, `OrganizationURL`
276274
: The name and URL of the organization responsible for this IdP.

‎modules/saml/src/Auth/Source/SP.php‎

Lines changed: 5 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -159,14 +159,9 @@ public function getHostedMetadata(): array
159159

160160
// add NameIDPolicy
161161
if ($this->metadata->hasValue('NameIDPolicy')) {
162-
$format = $this->metadata->getValue('NameIDPolicy');
163-
if (is_array($format)) {
164-
$metadata['NameIDFormat'] = Configuration::loadFromArray($format)->getOptionalString(
165-
'Format',
166-
Constants::NAMEID_TRANSIENT
167-
);
168-
} elseif (is_string($format)) {
169-
$metadata['NameIDFormat'] = $format;
162+
$format = $this->metadata->getArray('NameIDPolicy');
163+
if ($format !== []) {
164+
$metadata['NameIDFormat'] = $format['Format'] ?? Constants::NAMEID_TRANSIENT;
170165
}
171166
}
172167

@@ -558,21 +553,8 @@ private function startSSO2(Configuration $idpMetadata, array $state): void
558553
$ar->setNameId($nid);
559554
}
560555

561-
if (isset($state['saml:NameIDPolicy'])) {
562-
$policy = null;
563-
if (is_string($state['saml:NameIDPolicy'])) {
564-
$policy = [
565-
'Format' => $state['saml:NameIDPolicy'],
566-
'AllowCreate' => true,
567-
];
568-
} elseif (is_array($state['saml:NameIDPolicy'])) {
569-
$policy = $state['saml:NameIDPolicy'];
570-
} elseif ($state['saml:NameIDPolicy'] === null) {
571-
$policy = ['Format' => Constants::NAMEID_TRANSIENT];
572-
}
573-
if ($policy !== null) {
574-
$ar->setNameIdPolicy($policy);
575-
}
556+
if (!empty($state['saml:NameIDPolicy'])) {
557+
$ar->setNameIdPolicy($policy);
576558
}
577559

578560
$requesterID = [];

‎modules/saml/src/Message.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -477,8 +477,8 @@ public static function buildAuthnRequest(
477477
}
478478

479479
$policy = Utils\Config\Metadata::parseNameIdPolicy($nameIdPolicy);
480-
if ($policy !== null) {
481-
// either we have a policy set, or we used the transient default
480+
// empty array signals not to set any NameIdPolicy element
481+
if ($policy !== []) {
482482
$ar->setNameIdPolicy($policy);
483483
}
484484

‎src/SimpleSAML/Utils/Config/Metadata.php‎

Lines changed: 19 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -261,29 +261,28 @@ public static function isHiddenFromDiscovery(array $metadata): bool
261261

262262
/**
263263
* This method parses the different possible values of the NameIDPolicy metadata configuration.
264-
*
265-
* @param null|array|false $nameIdPolicy
266-
*
267-
* @return null|array
268264
*/
269-
public static function parseNameIdPolicy($nameIdPolicy): ?array
265+
public static function parseNameIdPolicy(array $nameIdPolicy = null): array
270266
{
271-
$policy = null;
267+
if ($nameIdPolicy === null) {
268+
// when NameIDPolicy is unset or set to null, default to transient
269+
return ['Format' => Constants::NAMEID_TRANSIENT, 'AllowCreate' => true];
270+
}
272271

273-
if (is_array($nameIdPolicy)) {
274-
// handle current configurations specifying an array in the NameIDPolicy config option
275-
$nameIdPolicy_cf = Configuration::loadFromArray($nameIdPolicy);
276-
$policy = [
277-
'Format' => $nameIdPolicy_cf->getOptionalString('Format', Constants::NAMEID_TRANSIENT),
278-
'AllowCreate' => $nameIdPolicy_cf->getOptionalBoolean('AllowCreate', true),
279-
];
280-
$spNameQualifier = $nameIdPolicy_cf->getOptionalString('SPNameQualifier', null);
281-
if ($spNameQualifier !== null) {
282-
$policy['SPNameQualifier'] = $spNameQualifier;
283-
}
284-
} elseif ($nameIdPolicy === null) {
285-
// when NameIDPolicy is unset or set to null, default to transient as before
286-
$policy = ['Format' => Constants::NAMEID_TRANSIENT, 'AllowCreate' => true];
272+
if ($nameIdPolicy === []) {
273+
// empty array means not to send any NameIDPolicy element
274+
return [];
275+
}
276+
277+
// handle configurations specifying an array in the NameIDPolicy config option
278+
$nameIdPolicy_cf = Configuration::loadFromArray($nameIdPolicy);
279+
$policy = [
280+
'Format' => $nameIdPolicy_cf->getOptionalString('Format', Constants::NAMEID_TRANSIENT),
281+
'AllowCreate' => $nameIdPolicy_cf->getOptionalBoolean('AllowCreate', true),
282+
];
283+
$spNameQualifier = $nameIdPolicy_cf->getOptionalString('SPNameQualifier', null);
284+
if ($spNameQualifier !== null) {
285+
$policy['SPNameQualifier'] = $spNameQualifier;
287286
}
288287

289288
return $policy;

‎tests/modules/saml/src/Auth/Source/SPTest.php‎

Lines changed: 1 addition & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1077,26 +1077,7 @@ public function testMetadataHostedNameIDPolicy(): void
10771077
}
10781078

10791079
/**
1080-
* SP config option NameIDPolicy specified in legacy string form is reflected in metadata
1081-
*/
1082-
public function testMetadataHostedNameIDPolicyString(): void
1083-
{
1084-
$spId = 'myhosted-sp';
1085-
$info = ['AuthId' => $spId];
1086-
1087-
$config = [
1088-
'entityID' => 'urn:x-simplesamlphp:example-sp',
1089-
'NameIDPolicy' => 'urn:mace:shibboleth:1.0:nameIdentifier',
1090-
];
1091-
$as = new SpTester($info, $config);
1092-
1093-
$md = $as->getHostedMetadata();
1094-
$this->assertArrayHasKey('NameIDFormat', $md);
1095-
$this->assertEquals('urn:mace:shibboleth:1.0:nameIdentifier', $md['NameIDFormat']);
1096-
}
1097-
1098-
/**
1099-
* SP config option NameIDPolicy specified in deprecated form without Format is reflected in metadata
1080+
* SP config option NameIDPolicy specified without Format is reflected in metadata
11001081
*/
11011082
public function testMetadataHostedNameIDPolicyNullFormat(): void
11021083
{

‎tests/src/SimpleSAML/Utils/Config/MetadataTest.php‎

Lines changed: 45 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
/**
1616
* Tests related to SAML metadata.
1717
*
18-
* @covers \SimpleSAML\Utils\Config
18+
* @covers \SimpleSAML\Utils\Config\Metadata
1919
*/
2020
class MetadataTest extends TestCase
2121
{
@@ -222,21 +222,10 @@ public function testIsHiddenFromDiscovery(): void
222222

223223
/**
224224
* Test \SimpleSAML\Utils\Config\Metadata::parseNameIdPolicy().
225+
* Set to specific arrays.
225226
*/
226227
public function testParseNameIdPolicy(): void
227228
{
228-
// Test null or unset
229-
$nameIdPolicy = null;
230-
$this->assertEquals(
231-
['Format' => Constants::NAMEID_TRANSIENT, 'AllowCreate' => true],
232-
Metadata::parseNameIdPolicy($nameIdPolicy)
233-
);
234-
235-
// Test false
236-
$nameIdPolicy = false;
237-
$this->assertEquals(null, Metadata::parseNameIdPolicy($nameIdPolicy));
238-
239-
// Test array
240229
$nameIdPolicy = [
241230
'Format' => 'urn:oasis:names:tc:SAML:1.1:nameid-format:persistent',
242231
'AllowCreate' => false
@@ -257,4 +246,47 @@ public function testParseNameIdPolicy(): void
257246
'SPNameQualifier' => 'TEST'
258247
], Metadata::parseNameIdPolicy($nameIdPolicy));
259248
}
249+
250+
/**
251+
* Test \SimpleSAML\Utils\Config\Metadata::parseNameIdPolicy().
252+
* Test with settings that produce the fallback defaults.
253+
*/
254+
public function testParseNameIdPolicyDefaults(): void
255+
{
256+
// Test null or unset
257+
$nameIdPolicy = null;
258+
$this->assertEquals([
259+
'Format' => Constants::NAMEID_TRANSIENT,
260+
'AllowCreate' => true
261+
], Metadata::parseNameIdPolicy($nameIdPolicy));
262+
263+
$nameIdPolicy = [
264+
'Format' => 'urn:oasis:names:tc:SAML:1.1:nameid-format:persistent',
265+
];
266+
$this->assertEquals([
267+
'Format' => 'urn:oasis:names:tc:SAML:1.1:nameid-format:persistent',
268+
'AllowCreate' => true
269+
], Metadata::parseNameIdPolicy($nameIdPolicy));
270+
271+
$nameIdPolicy = [
272+
'AllowCreate' => false,
273+
];
274+
$this->assertEquals([
275+
'Format' => Constants::NAMEID_TRANSIENT,
276+
'AllowCreate' => false
277+
], Metadata::parseNameIdPolicy($nameIdPolicy));
278+
}
279+
280+
/**
281+
* Test \SimpleSAML\Utils\Config\Metadata::parseNameIdPolicy().
282+
* Test with setting to empty array (meaning to not send any NameIdPolicy).
283+
*/
284+
public function testParseNameIdPolicyEmpty(): void
285+
{
286+
$nameIdPolicy = [];
287+
$this->assertEquals(
288+
[],
289+
Metadata::parseNameIdPolicy($nameIdPolicy)
290+
);
291+
}
260292
}

0 commit comments

Comments
 (0)