From 54e2658b893854f9a98cf4fa348628dd162f5738 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Fri, 14 Aug 2026 15:30:49 +0200 Subject: [PATCH 1/4] Serialise the xsi:type on WebServiceDescriptorType --- .../fed/AbstractWebServiceDescriptorType.php | 57 +++++ .../XML/fed/SecurityTokenServiceTypeTest.php | 207 ++++++++++++++++++ .../xml/fed/SecurityTokenServiceType.xml | 12 + 3 files changed, 276 insertions(+) create mode 100644 tests/Federation/XML/fed/SecurityTokenServiceTypeTest.php create mode 100644 tests/resources/xml/fed/SecurityTokenServiceType.xml diff --git a/src/XML/fed/AbstractWebServiceDescriptorType.php b/src/XML/fed/AbstractWebServiceDescriptorType.php index 011c3ef..eb8986e 100644 --- a/src/XML/fed/AbstractWebServiceDescriptorType.php +++ b/src/XML/fed/AbstractWebServiceDescriptorType.php @@ -12,6 +12,8 @@ use SimpleSAML\SAML2\XML\md\AbstractRoleDescriptor; use SimpleSAML\SAML2\XML\md\Extensions; use SimpleSAML\SAML2\XML\md\Organization; +use SimpleSAML\XML\Attribute as XMLAttribute; +use SimpleSAML\XMLSchema\Constants as C; use SimpleSAML\XMLSchema\Type\DurationValue; use SimpleSAML\XMLSchema\Type\IDValue; use SimpleSAML\XMLSchema\Type\QNameValue; @@ -23,6 +25,24 @@ */ abstract class AbstractWebServiceDescriptorType extends AbstractRoleDescriptor { + /** + * The element is md:RoleDescriptor, but its content model lives in the WS-Federation schema — that is + * what the xsi:type points at. Validating against the inherited metadata schema alone can never resolve + * it, because md:RoleDescriptorType is abstract. ws-federation.xsd imports the metadata namespace. + */ + public const string SCHEMA = 'resources/schemas/ws-federation.xsd'; + + /** + * The exclusions for the xs:anyAttribute element + * + * xsi:type is modelled by AbstractRoleDescriptor as $type and returned by getXsiType(); without this + * exclusion it would also be swept into the extendable-attributes bucket and written a second time. + */ + public const array XS_ANY_ATTR_EXCLUSIONS = [ + [C::NS_XSI, 'type'], + ]; + + /** * WebServiceDescriptorType constructor. * @@ -198,6 +218,43 @@ public function toUnsignedXML(?DOMElement $parent = null): DOMElement { $e = parent::toUnsignedXML($parent); + // md:RoleDescriptor requires an xsi:type. AbstractRoleDescriptor stores it and demands it back in + // fromXML(), but nothing ever writes it, so the element cannot round-trip its own output. + $xsiType = $this->getXsiType(); + + // An unprefixed QName — from a document that carried this namespace on xmlns — can only resolve + // through the default namespace, and declaring one here would silently capture any unqualified + // descendant. Re-express it with this type's own prefix instead: a QName is identified by its + // {namespace, local name}, so the prefix is lexical only and the value keeps its meaning. + if ($xsiType->getNamespaceURI() !== null && $xsiType->getNamespacePrefix() === null) { + $xsiType = QNameValue::fromParts( + $xsiType->getLocalName(), + $xsiType->getNamespaceURI(), + static::getXsiTypePrefix(), + ); + } + + $namespaceURI = $xsiType->getNamespaceURI(); + $namespacePrefix = $xsiType->getNamespacePrefix(); + + // The prefix appears only inside the attribute *value*, so DOM will not declare it for us. Bind it + // unless it is already in scope for this very namespace. AbstractSignedMdElement::toXML() separately + // declares the *canonical* prefix once this method returns; this covers a caller-supplied one. + if ( + $namespaceURI !== null && + $namespacePrefix !== null && + $e->lookupNamespaceURI($namespacePrefix->getValue()) !== $namespaceURI->getValue() + ) { + $e->setAttributeNS( + 'http://www.w3.org/2000/xmlns/', + 'xmlns:' . $namespacePrefix->getValue(), + $namespaceURI->getValue(), + ); + } + + $type = new XMLAttribute(C::NS_XSI, 'xsi', 'type', $xsiType); + $type->toXML($e); + $this->getLogicalServiceNamesOffered()?->toXML($e); $this->getTokenTypesOffered()?->toXML($e); $this->getClaimDialectsOffered()?->toXML($e); diff --git a/tests/Federation/XML/fed/SecurityTokenServiceTypeTest.php b/tests/Federation/XML/fed/SecurityTokenServiceTypeTest.php new file mode 100644 index 0000000..2e7f875 --- /dev/null +++ b/tests/Federation/XML/fed/SecurityTokenServiceTypeTest.php @@ -0,0 +1,207 @@ +assertEquals( + self::$xmlRepresentation->saveXML(self::$xmlRepresentation->documentElement), + strval(self::buildSecurityTokenServiceType()), + ); + } + + + /** + * md:RoleDescriptor carries its element type in xsi:type, and AbstractRoleDescriptor::fromXML() + * rejects the element outright when it is absent. Serialising has to emit it. + */ + public function testMarshallingWritesXsiType(): void + { + $element = self::buildSecurityTokenServiceType()->toXML(); + + $this->assertEquals( + 'fed:SecurityTokenServiceType', + $element->getAttributeNS(C_XSI::NS_XSI, 'type'), + ); + + // The prefix used inside the attribute value only counts if it is bound on the element. + $this->assertEquals(C::NS_FED, $element->lookupNamespaceURI('fed')); + } + + + /** + * The element must be able to read back what it writes. Before the xsi:type was emitted this threw + * a SchemaViolationException on the object's own output. + */ + public function testRoundTrip(): void + { + $original = self::buildSecurityTokenServiceType(); + + $this->assertEquals( + strval($original), + strval(SecurityTokenServiceType::fromXML($original->toXML())), + ); + } + + + /** + * An xsi:type may legally be unprefixed, resolving through the document's default namespace. The + * element must not lose that namespace on the way out — a default namespace cannot be re-declared + * here without capturing unqualified descendants, so the type is re-expressed with its own prefix. + */ + public function testUnprefixedXsiTypeKeepsItsNamespace(): void + { + $xml = << + + + https://idp.example.org/adfs/services/trust + + + + XML; + + $parsed = SecurityTokenServiceType::fromXML( + DOMDocumentFactory::fromString($xml)->documentElement, + ); + $this->assertNull($parsed->getXsiType()->getNamespacePrefix()); + + $element = $parsed->toXML(); + $this->assertEquals( + 'fed:SecurityTokenServiceType', + $element->getAttributeNS(C_XSI::NS_XSI, 'type'), + ); + + // No default namespace may be introduced, or unqualified descendants would change meaning. + $this->assertNull($element->lookupNamespaceURI(null)); + + // What matters is that the QName still denotes the same {namespace, local name}. + $this->assertEquals( + C::NS_FED, + SecurityTokenServiceType::fromXML($element)->getXsiType()->getNamespaceURI()->getValue(), + ); + } + + + // test unmarshalling + + + /** + * Test creating a SecurityTokenServiceType object from XML. + */ + public function testUnmarshalling(): void + { + $securityTokenServiceType = SecurityTokenServiceType::fromXML( + self::$xmlRepresentation->documentElement, + ); + + $this->assertEquals( + 'fed:SecurityTokenServiceType', + strval($securityTokenServiceType->getXsiType()), + ); + + // xsi:type is modelled as the element's own type, not as one of its extendable attributes. + $this->assertEmpty($securityTokenServiceType->getAttributesNS()); + $this->assertCount(1, $securityTokenServiceType->getSecurityTokenServiceEndpoint()); + $this->assertCount(1, $securityTokenServiceType->getPassiveRequestorEndpoint()); + $this->assertEquals( + 'SimpleSAMLphp ADFS IdP', + strval($securityTokenServiceType->getServiceDisplayName()), + ); + } +} diff --git a/tests/resources/xml/fed/SecurityTokenServiceType.xml b/tests/resources/xml/fed/SecurityTokenServiceType.xml new file mode 100644 index 0000000..8d429cf --- /dev/null +++ b/tests/resources/xml/fed/SecurityTokenServiceType.xml @@ -0,0 +1,12 @@ + + + + https://idp.example.org/adfs/services/trust + + + + + https://idp.example.org/adfs/ls/ + + + From f53e333f1630df63cc2f97975542f1591dbb0b29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Fri, 14 Aug 2026 15:52:21 +0200 Subject: [PATCH 2/4] Make pre-commit runnable --- composer.json | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/composer.json b/composer.json index 4b961f4..9a16e76 100644 --- a/composer.json +++ b/composer.json @@ -51,9 +51,8 @@ "scripts": { "pre-commit": [ "vendor/bin/phpcs -p", - "vendor/bin/composer-require-checker check composer.json", - "vendor/bin/phpstan analyze -c phpstan.neon", - "vendor/bin/phpstan analyze -c phpstan-dev.neon", + "vendor/bin/phpstan analyze -c phpstan.neon --memory-limit=1024M", + "vendor/bin/phpstan analyze -c phpstan-dev.neon --memory-limit=1024M", "vendor/bin/composer-unused", "vendor/bin/phpunit --no-coverage --testdox" ] From 33bb0f7b0529744f0f971027f94ba3e375c252b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Mon, 17 Aug 2026 09:46:54 +0200 Subject: [PATCH 3/4] Serialise the xsi:type via the extendable-attributes bucket --- .../fed/AbstractWebServiceDescriptorType.php | 77 ++++++++----------- .../XML/fed/SecurityTokenServiceTypeTest.php | 10 ++- .../xml/fed/SecurityTokenServiceType.xml | 11 +-- 3 files changed, 44 insertions(+), 54 deletions(-) diff --git a/src/XML/fed/AbstractWebServiceDescriptorType.php b/src/XML/fed/AbstractWebServiceDescriptorType.php index eb8986e..ac666b2 100644 --- a/src/XML/fed/AbstractWebServiceDescriptorType.php +++ b/src/XML/fed/AbstractWebServiceDescriptorType.php @@ -18,6 +18,9 @@ use SimpleSAML\XMLSchema\Type\IDValue; use SimpleSAML\XMLSchema\Type\QNameValue; +use function array_filter; +use function array_values; + /** * An WebServiceDescriptorType * @@ -32,16 +35,6 @@ abstract class AbstractWebServiceDescriptorType extends AbstractRoleDescriptor */ public const string SCHEMA = 'resources/schemas/ws-federation.xsd'; - /** - * The exclusions for the xs:anyAttribute element - * - * xsi:type is modelled by AbstractRoleDescriptor as $type and returned by getXsiType(); without this - * exclusion it would also be swept into the extendable-attributes bucket and written a second time. - */ - public const array XS_ANY_ATTR_EXCLUSIONS = [ - [C::NS_XSI, 'type'], - ]; - /** * WebServiceDescriptorType constructor. @@ -96,6 +89,33 @@ protected function __construct( protected ?SAMLStringValue $serviceDisplayName = null, protected ?SAMLStringValue $serviceDescription = null, ) { + // md:RoleDescriptor requires an xsi:type. AbstractRoleDescriptor stores it and demands it back in + // fromXML(), but nothing ever writes it, so the element cannot round-trip its own output. Hand it + // to the extendable-attributes bucket, which the parent already serializes. + $xsiType = $type; + + // An unprefixed QName — from a document that carried this namespace on xmlns — can only resolve + // through the default namespace, and declaring one here would silently capture any unqualified + // descendant. Re-express it with this type's own prefix instead: a QName is identified by its + // {namespace, local name}, so the prefix is lexical only and the value keeps its meaning. + if ($xsiType->getNamespaceURI() !== null && $xsiType->getNamespacePrefix() === null) { + $xsiType = QNameValue::fromParts( + $xsiType->getLocalName(), + $xsiType->getNamespaceURI(), + static::getXsiTypePrefix(), + ); + } + + // fromXML() also sweeps xsi:type into this bucket as a plain StringValue, so drop that copy + // rather than let two entries claim the same attribute name — the typed one carries the + // resolved namespace, the swept one only the lexical form it happened to be written with. + $namespacedAttributes = array_values(array_filter( + $namespacedAttributes, + static fn (XMLAttribute $attr): bool + => $attr->getNamespaceURI() !== C::NS_XSI || $attr->getAttrName() !== 'type', + )); + $namespacedAttributes[] = new XMLAttribute(C::NS_XSI, 'xsi', 'type', $xsiType); + parent::__construct( $type, $protocolSupportEnumeration, @@ -218,43 +238,6 @@ public function toUnsignedXML(?DOMElement $parent = null): DOMElement { $e = parent::toUnsignedXML($parent); - // md:RoleDescriptor requires an xsi:type. AbstractRoleDescriptor stores it and demands it back in - // fromXML(), but nothing ever writes it, so the element cannot round-trip its own output. - $xsiType = $this->getXsiType(); - - // An unprefixed QName — from a document that carried this namespace on xmlns — can only resolve - // through the default namespace, and declaring one here would silently capture any unqualified - // descendant. Re-express it with this type's own prefix instead: a QName is identified by its - // {namespace, local name}, so the prefix is lexical only and the value keeps its meaning. - if ($xsiType->getNamespaceURI() !== null && $xsiType->getNamespacePrefix() === null) { - $xsiType = QNameValue::fromParts( - $xsiType->getLocalName(), - $xsiType->getNamespaceURI(), - static::getXsiTypePrefix(), - ); - } - - $namespaceURI = $xsiType->getNamespaceURI(); - $namespacePrefix = $xsiType->getNamespacePrefix(); - - // The prefix appears only inside the attribute *value*, so DOM will not declare it for us. Bind it - // unless it is already in scope for this very namespace. AbstractSignedMdElement::toXML() separately - // declares the *canonical* prefix once this method returns; this covers a caller-supplied one. - if ( - $namespaceURI !== null && - $namespacePrefix !== null && - $e->lookupNamespaceURI($namespacePrefix->getValue()) !== $namespaceURI->getValue() - ) { - $e->setAttributeNS( - 'http://www.w3.org/2000/xmlns/', - 'xmlns:' . $namespacePrefix->getValue(), - $namespaceURI->getValue(), - ); - } - - $type = new XMLAttribute(C::NS_XSI, 'xsi', 'type', $xsiType); - $type->toXML($e); - $this->getLogicalServiceNamesOffered()?->toXML($e); $this->getTokenTypesOffered()?->toXML($e); $this->getClaimDialectsOffered()?->toXML($e); diff --git a/tests/Federation/XML/fed/SecurityTokenServiceTypeTest.php b/tests/Federation/XML/fed/SecurityTokenServiceTypeTest.php index 2e7f875..8267a02 100644 --- a/tests/Federation/XML/fed/SecurityTokenServiceTypeTest.php +++ b/tests/Federation/XML/fed/SecurityTokenServiceTypeTest.php @@ -195,8 +195,14 @@ public function testUnmarshalling(): void strval($securityTokenServiceType->getXsiType()), ); - // xsi:type is modelled as the element's own type, not as one of its extendable attributes. - $this->assertEmpty($securityTokenServiceType->getAttributesNS()); + // xsi:type reaches the document through the extendable-attributes bucket, and lands there exactly + // once — the copy fromXML() sweeps in as a plain StringValue is replaced by the typed one. + $attributes = $securityTokenServiceType->getAttributesNS(); + $this->assertCount(1, $attributes); + $this->assertEquals(C_XSI::NS_XSI, $attributes[0]->getNamespaceURI()); + $this->assertEquals('type', $attributes[0]->getAttrName()); + $this->assertEquals('fed:SecurityTokenServiceType', strval($attributes[0]->getAttrValue())); + $this->assertCount(1, $securityTokenServiceType->getSecurityTokenServiceEndpoint()); $this->assertCount(1, $securityTokenServiceType->getPassiveRequestorEndpoint()); $this->assertEquals( diff --git a/tests/resources/xml/fed/SecurityTokenServiceType.xml b/tests/resources/xml/fed/SecurityTokenServiceType.xml index 8d429cf..e63ad78 100644 --- a/tests/resources/xml/fed/SecurityTokenServiceType.xml +++ b/tests/resources/xml/fed/SecurityTokenServiceType.xml @@ -1,11 +1,12 @@ - - - + + + + https://idp.example.org/adfs/services/trust - - + + https://idp.example.org/adfs/ls/ From e0f7ceafb35483443ee2fe3f50ad09e42ac91fba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Ivan=C4=8Di=C4=87?= Date: Mon, 17 Aug 2026 14:51:27 +0200 Subject: [PATCH 4/4] Write the xsi:type directly, keeping the exclusion constant The previous commit routed the xsi:type through $namespacedAttributes, which required dropping XS_ANY_ATTR_EXCLUSIONS: setAttributesNS() enforces the exclusion list on the write side too, so an excluded attribute cannot be passed back in. That in turn let fromXML()'s swept StringValue copy into the bucket and needed a filter to remove it again. Keep the exclusion constant and write the attribute directly instead. The manual namespace declaration really was redundant, as review pointed out -- AbstractSignedMdElement::toXML() already declares xmlns: unconditionally -- so the write is a single line. Normalise the QName to static::getXsiTypePrefix() in all cases rather than only when it arrives unprefixed. That is precisely the prefix the declaration above guarantees to bind, so it also covers a caller passing a prefix that nothing declares. Co-Authored-By: Claude Opus 5 --- .../fed/AbstractWebServiceDescriptorType.php | 55 +++++++++---------- .../XML/fed/SecurityTokenServiceTypeTest.php | 10 +--- 2 files changed, 27 insertions(+), 38 deletions(-) diff --git a/src/XML/fed/AbstractWebServiceDescriptorType.php b/src/XML/fed/AbstractWebServiceDescriptorType.php index ac666b2..b054d76 100644 --- a/src/XML/fed/AbstractWebServiceDescriptorType.php +++ b/src/XML/fed/AbstractWebServiceDescriptorType.php @@ -18,9 +18,6 @@ use SimpleSAML\XMLSchema\Type\IDValue; use SimpleSAML\XMLSchema\Type\QNameValue; -use function array_filter; -use function array_values; - /** * An WebServiceDescriptorType * @@ -35,6 +32,16 @@ abstract class AbstractWebServiceDescriptorType extends AbstractRoleDescriptor */ public const string SCHEMA = 'resources/schemas/ws-federation.xsd'; + /** + * The exclusions for the xs:anyAttribute element + * + * xsi:type is modeled by AbstractRoleDescriptor as $type and returned by getXsiType(); without this + * exclusion it would also be swept into the extendable-attributes bucket and written a second time. + */ + public const array XS_ANY_ATTR_EXCLUSIONS = [ + [C::NS_XSI, 'type'], + ]; + /** * WebServiceDescriptorType constructor. @@ -89,33 +96,6 @@ protected function __construct( protected ?SAMLStringValue $serviceDisplayName = null, protected ?SAMLStringValue $serviceDescription = null, ) { - // md:RoleDescriptor requires an xsi:type. AbstractRoleDescriptor stores it and demands it back in - // fromXML(), but nothing ever writes it, so the element cannot round-trip its own output. Hand it - // to the extendable-attributes bucket, which the parent already serializes. - $xsiType = $type; - - // An unprefixed QName — from a document that carried this namespace on xmlns — can only resolve - // through the default namespace, and declaring one here would silently capture any unqualified - // descendant. Re-express it with this type's own prefix instead: a QName is identified by its - // {namespace, local name}, so the prefix is lexical only and the value keeps its meaning. - if ($xsiType->getNamespaceURI() !== null && $xsiType->getNamespacePrefix() === null) { - $xsiType = QNameValue::fromParts( - $xsiType->getLocalName(), - $xsiType->getNamespaceURI(), - static::getXsiTypePrefix(), - ); - } - - // fromXML() also sweeps xsi:type into this bucket as a plain StringValue, so drop that copy - // rather than let two entries claim the same attribute name — the typed one carries the - // resolved namespace, the swept one only the lexical form it happened to be written with. - $namespacedAttributes = array_values(array_filter( - $namespacedAttributes, - static fn (XMLAttribute $attr): bool - => $attr->getNamespaceURI() !== C::NS_XSI || $attr->getAttrName() !== 'type', - )); - $namespacedAttributes[] = new XMLAttribute(C::NS_XSI, 'xsi', 'type', $xsiType); - parent::__construct( $type, $protocolSupportEnumeration, @@ -238,6 +218,21 @@ public function toUnsignedXML(?DOMElement $parent = null): DOMElement { $e = parent::toUnsignedXML($parent); + // md:RoleDescriptor requires an xsi:type. AbstractRoleDescriptor stores it and demands it back in + // fromXML(), but nothing ever writes it, so the element cannot round-trip its own output. + // + // Re-express it with this type's own prefix rather than the caller's: a QName is identified by its + // {namespace, local name}, so the prefix is lexical only and the value keeps its meaning — and this + // is the one prefix AbstractSignedMdElement::toXML() is guaranteed to declare for us. A caller's + // prefix, or none at all, would otherwise be left unbound and the QName unresolvable. + $xsiType = QNameValue::fromParts( + $this->getXsiType()->getLocalName(), + $this->getXsiType()->getNamespaceURI(), + static::getXsiTypePrefix(), + ); + + (new XMLAttribute(C::NS_XSI, 'xsi', 'type', $xsiType))->toXML($e); + $this->getLogicalServiceNamesOffered()?->toXML($e); $this->getTokenTypesOffered()?->toXML($e); $this->getClaimDialectsOffered()?->toXML($e); diff --git a/tests/Federation/XML/fed/SecurityTokenServiceTypeTest.php b/tests/Federation/XML/fed/SecurityTokenServiceTypeTest.php index 8267a02..2e7f875 100644 --- a/tests/Federation/XML/fed/SecurityTokenServiceTypeTest.php +++ b/tests/Federation/XML/fed/SecurityTokenServiceTypeTest.php @@ -195,14 +195,8 @@ public function testUnmarshalling(): void strval($securityTokenServiceType->getXsiType()), ); - // xsi:type reaches the document through the extendable-attributes bucket, and lands there exactly - // once — the copy fromXML() sweeps in as a plain StringValue is replaced by the typed one. - $attributes = $securityTokenServiceType->getAttributesNS(); - $this->assertCount(1, $attributes); - $this->assertEquals(C_XSI::NS_XSI, $attributes[0]->getNamespaceURI()); - $this->assertEquals('type', $attributes[0]->getAttrName()); - $this->assertEquals('fed:SecurityTokenServiceType', strval($attributes[0]->getAttrValue())); - + // xsi:type is modelled as the element's own type, not as one of its extendable attributes. + $this->assertEmpty($securityTokenServiceType->getAttributesNS()); $this->assertCount(1, $securityTokenServiceType->getSecurityTokenServiceEndpoint()); $this->assertCount(1, $securityTokenServiceType->getPassiveRequestorEndpoint()); $this->assertEquals(