Serialise the xsi:type on WebServiceDescriptorType

[cicnavi]

md:RoleDescriptor carries its element type in xsi:type, and AbstractRoleDescriptor::fromXML() rejects the element without it, but nothing wrote it for objects built from scratch. SecurityTokenServiceType::fromXML($sts->toXML()) threw a SchemaViolationException on the object's own output.

[codecov]

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

[tvdijen]

Why not just pass it in the $namepacedAttributes ?

[cicnavi]

Hm, the element already holds that type, it is the element own type, passed as first constructor parameter...

[tvdijen]

Yes, so my idea was to convert QName to XMLAttribute and then add it to the namespacedAttributes-array. The code for adding the attribute and the xmlns-namespace then becomes redundant. It's already covered by the ExtendableAttributesTrait

[cicnavi]

I'll try

[cicnavi]

Please check now.

[tvdijen]

Is this really true? It should be a QNameValue, if not we have a bug...
This filter should not be necessary at all with the exclusion-constant in place

[cicnavi]

Yes, everything is a StringValue. Try this on master branch:

<?php
require 'vendor/autoload.php';

use SimpleSAML\WebServices\Federation\XML\fed\SecurityTokenServiceType;
use SimpleSAML\XML\DOMDocumentFactory;

$xml = <<<XML
<md:RoleDescriptor xmlns:md="urn:oasis:names:tc:SAML:2.0:metadata"
                   xmlns:fed="http://docs.oasis-open.org/wsfed/federation/200706"
                   xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                   xmlns:wsa10="http://www.w3.org/2005/08/addressing"
                   xmlns:ex="urn:example:extension"
                   xsi:type="fed:SecurityTokenServiceType"
                   ex:flag="whatever"
                   protocolSupportEnumeration="http://docs.oasis-open.org/wsfed/federation/200706">
  <fed:SecurityTokenServiceEndpoint>
    <wsa10:EndpointReference><wsa10:Address>https://idp.example.org/sts</wsa10:Address></wsa10:EndpointReference>
  </fed:SecurityTokenServiceEndpoint>
</md:RoleDescriptor>
XML;

$parsed = SecurityTokenServiceType::fromXML(DOMDocumentFactory::fromString($xml)->documentElement);

foreach ($parsed->getAttributesNS() as $a) {
    printf("  {%s}%s = %-24s stored as %s\n", $a->getNamespaceURI(), $a->getAttrName(),
        $a->getAttrValue()->getValue(), (new ReflectionClass($a->getAttrValue()))->getShortName());
}
printf("  xsi:type (as \$type)          stored as %s\n",
    (new ReflectionClass($parsed->getXsiType()))->getShortName());

This is from xml-common, ExtendableAttributesTrait::getAttributesNSFromXML():

$attributes[] = new Attribute(
    $a->namespaceURI, $a->prefix, $a->localName,
    StringValue::fromString($a->nodeValue),
);

Second test that you can run on this branch, which throws as rejected:

<?php
require 'vendor/autoload.php';

use SimpleSAML\SAML2\Type\SAMLAnyURIListValue;
use SimpleSAML\WebServices\Addressing\XML\wsa_200508\{Address, EndpointReference};
use SimpleSAML\WebServices\Federation\Constants as C_FED;
use SimpleSAML\WebServices\Federation\XML\fed\{AbstractSecurityTokenServiceType,
    SecurityTokenServiceEndpoint, SecurityTokenServiceType};
use SimpleSAML\XML\Attribute as XMLAttribute;
use SimpleSAML\XMLSchema\Constants as C;
use SimpleSAML\XMLSchema\Type\{AnyURIValue, NCNameValue, QNameValue};

$type = QNameValue::fromParts(
    NCNameValue::fromString(AbstractSecurityTokenServiceType::XSI_TYPE_NAME),
    AnyURIValue::fromString(AbstractSecurityTokenServiceType::XSI_TYPE_NAMESPACE),
    NCNameValue::fromString(AbstractSecurityTokenServiceType::XSI_TYPE_PREFIX),
);

try {
    new SecurityTokenServiceType(
        $type,
        SAMLAnyURIListValue::fromString(C_FED::NS_FED),
        namespacedAttributes: [new XMLAttribute(C::NS_XSI, 'xsi', 'type', $type)],
        securityTokenServiceEndpoint: [new SecurityTokenServiceEndpoint([
            new EndpointReference(new Address(AnyURIValue::fromString('https://idp.example.org/sts'))),
        ])],
    );
    echo "accepted\n";
} catch (Throwable $e) {
    printf("REJECTED: %s\n", $e::class);
}

So, exclusion constant and the bucket can't coexist.

How would you handle this? Should I keep the exclusion,and write the attribute explicitly in toUnsignedXML().

[tvdijen]

Second test shows as accepted for me.. XMLAttribute accepts any value type. What are we missing here?

[cicnavi]

Ahh, sorry, I had the exclusion locally, and the branch has them deleted upstream... I've now pushed together with how Claude explains it (I'm using it to get to the bottom of this). Claude says:

Pushed as e0f7cea. This goes back to the exclusion constant and an explicit write, but keeps
your point about the redundant xmlns declaration — AbstractSignedMdElement::toXML() already
declares xmlns:<XSI_TYPE_PREFIX> unconditionally, so the write is now a single line:

$xsiType = QNameValue::fromParts(
    $this->getXsiType()->getLocalName(),
    $this->getXsiType()->getNamespaceURI(),
    static::getXsiTypePrefix(),
);

(new XMLAttribute(C::NS_XSI, 'xsi', 'type', $xsiType))->toXML($e);

And yes — you're right that XMLAttribute takes any value type, which is exactly what this
relies on: $xsiType is a QNameValue, so the resolved namespace survives. The two things are
independent though. What XMLAttribute accepts is one question; what
getAttributesNSFromXML() produces when reading is another, and that's hardcoded to
StringValue::fromString($a->nodeValue). It can't really do better — with a DOMAttr and no
schema, guessing which values are QNames would misfire on anything containing a colon, a URI
being the obvious case. Which I think argues for your exclusion constant rather than against
it: if the bucket can't carry a typed value, xsi:type shouldn't travel through it.

Sorry about the earlier script — its precondition was the exclusion constant being present,
and the commit you tested was the one that removed it, so of course it was accepted. I should
have said which state to run it in.

Here's a self-checking script instead. Drop it in the repo root on this branch and run
php verify-xsi-type.php; it prints the exclusion constant before testing against it, and
exits non-zero if anything fails:

<?php

/**
 * Verification for simplesamlphp/xml-ws-federation#8.
 *
 * Run from the repo root on branch fix-roledescriptor-xsi-type:
 *     php verify-xsi-type.php
 */

declare(strict_types=1);

require 'vendor/autoload.php';

use SimpleSAML\SAML2\Type\{SAMLAnyURIListValue, SAMLStringValue};
use SimpleSAML\WebServices\Addressing\XML\wsa_200508\{Address, EndpointReference};
use SimpleSAML\WebServices\Federation\Constants as C_FED;
use SimpleSAML\WebServices\Federation\XML\fed\{AbstractSecurityTokenServiceType,
    PassiveRequestorEndpoint, SecurityTokenServiceEndpoint, SecurityTokenServiceType};
use SimpleSAML\XML\Attribute as XMLAttribute;
use SimpleSAML\XML\DOMDocumentFactory;
use SimpleSAML\XMLSchema\Constants as C;
use SimpleSAML\XMLSchema\Type\{AnyURIValue, NCNameValue, QNameValue, StringValue};
use SimpleSAML\XMLSchema\XML\Documentation;

$pass = 0;
$fail = 0;

function check(string $label, bool $ok, string $detail = ''): void
{
    global $pass, $fail;
    $ok ? $pass++ : $fail++;
    printf("  [%s] %s%s\n", $ok ? ' ok ' : 'FAIL', $label, $detail === '' ? '' : "\n         $detail");
}

function sts(?QNameValue $type = null, array $namespacedAttributes = []): SecurityTokenServiceType
{
    return new SecurityTokenServiceType(
        $type ?? QNameValue::fromParts(
            NCNameValue::fromString(AbstractSecurityTokenServiceType::XSI_TYPE_NAME),
            AnyURIValue::fromString(AbstractSecurityTokenServiceType::XSI_TYPE_NAMESPACE),
            NCNameValue::fromString(AbstractSecurityTokenServiceType::XSI_TYPE_PREFIX),
        ),
        SAMLAnyURIListValue::fromString(C_FED::NS_FED),
        namespacedAttributes: $namespacedAttributes,
        serviceDisplayName: SAMLStringValue::fromString('SimpleSAMLphp ADFS IdP'),
        securityTokenServiceEndpoint: [new SecurityTokenServiceEndpoint([
            new EndpointReference(new Address(AnyURIValue::fromString('https://idp.example.org/sts'))),
        ])],
        passiveRequestorEndpoint: [new PassiveRequestorEndpoint([
            new EndpointReference(new Address(AnyURIValue::fromString('https://idp.example.org/ls/'))),
        ])],
    );
}


echo "\n1. The bug this PR fixes: an object built from scratch writes its xsi:type\n";

$element = sts()->toXML();
check(
    'xsi:type is present on the serialised element',
    $element->getAttributeNS(C::NS_XSI, 'type') === 'fed:SecurityTokenServiceType',
    'got: ' . var_export($element->getAttributeNS(C::NS_XSI, 'type'), true),
);
check(
    'the prefix used in that value is bound on the element',
    $element->lookupNamespaceURI('fed') === C_FED::NS_FED,
);

try {
    SecurityTokenServiceType::fromXML(sts()->toXML());
    check('the element can parse its own output (round-trip)', true);
} catch (Throwable $e) {
    check('the element can parse its own output (round-trip)', false, $e::class . ': ' . $e->getMessage());
}


echo "\n2. An unprefixed xsi:type keeps its namespace\n";

$xml = <<<XML
<md:RoleDescriptor xmlns:md="urn:oasis:names:tc:SAML:2.0:metadata"
                   xmlns="http://docs.oasis-open.org/wsfed/federation/200706"
                   xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
                   xmlns:wsa10="http://www.w3.org/2005/08/addressing"
                   xsi:type="SecurityTokenServiceType"
                   protocolSupportEnumeration="http://docs.oasis-open.org/wsfed/federation/200706">
  <SecurityTokenServiceEndpoint>
    <wsa10:EndpointReference><wsa10:Address>https://idp.example.org/sts</wsa10:Address></wsa10:EndpointReference>
  </SecurityTokenServiceEndpoint>
</md:RoleDescriptor>
XML;

$parsed = SecurityTokenServiceType::fromXML(DOMDocumentFactory::fromString($xml)->documentElement);
check('parsed with no prefix, as written', $parsed->getXsiType()->getNamespacePrefix() === null);

$out = $parsed->toXML();
check(
    're-serialised with this type\'s own prefix',
    $out->getAttributeNS(C::NS_XSI, 'type') === 'fed:SecurityTokenServiceType',
    'got: ' . var_export($out->getAttributeNS(C::NS_XSI, 'type'), true),
);
check('no default namespace introduced', $out->lookupNamespaceURI(null) === null);
check(
    'the QName still denotes the same namespace',
    SecurityTokenServiceType::fromXML($out)->getXsiType()->getNamespaceURI()?->getValue() === C_FED::NS_FED,
);


echo "\n3. Why the exclusion constant rules out the \$namespacedAttributes route\n";

printf("     XS_ANY_ATTR_EXCLUSIONS = %s\n", json_encode(SecurityTokenServiceType::XS_ANY_ATTR_EXCLUSIONS));

check('xsi:type is kept out of the bucket when parsing', sts()->getAttributesNS() === []);

try {
    sts(namespacedAttributes: [new XMLAttribute(
        C::NS_XSI,
        'xsi',
        'type',
        QNameValue::fromParts(
            NCNameValue::fromString(AbstractSecurityTokenServiceType::XSI_TYPE_NAME),
            AnyURIValue::fromString(AbstractSecurityTokenServiceType::XSI_TYPE_NAMESPACE),
            NCNameValue::fromString(AbstractSecurityTokenServiceType::XSI_TYPE_PREFIX),
        ),
    )]);
    check('passing xsi:type in $namespacedAttributes is rejected', false, 'it was accepted');
} catch (Throwable $e) {
    check(
        'passing xsi:type in $namespacedAttributes is rejected',
        str_contains($e::class, 'InvalidDOMAttributeException'),
        $e::class,
    );
}


echo "\n4. The same write-side ban, with no WS-Federation code involved\n";
echo "     xs:documentation models xml:lang as \$lang and excludes it, exactly as\n";
echo "     md:RoleDescriptor models xsi:type as \$type.\n";

$empty = DOMDocumentFactory::fromString('<root/>')->documentElement->childNodes;

foreach ([
    ['http://www.w3.org/XML/1998/namespace', 'lang', true],
    ['urn:example:extension', 'flag', false],
] as [$ns, $name, $shouldReject]) {
    try {
        new Documentation($empty, namespacedAttributes: [
            new XMLAttribute($ns, 'p', $name, StringValue::fromString('v')),
        ]);
        check(sprintf('{%s}%s %s', $ns, $name, $shouldReject ? 'rejected' : 'accepted'), !$shouldReject);
    } catch (Throwable $e) {
        check(
            sprintf('{%s}%s %s', $ns, $name, $shouldReject ? 'rejected' : 'accepted'),
            $shouldReject,
            $e::class,
        );
    }
}


echo "\n5. What a generic sweep can and cannot recover\n";

$withExtension = str_replace(
    'xsi:type="SecurityTokenServiceType"',
    'xsi:type="SecurityTokenServiceType" xmlns:ex="urn:example:extension" ex:flag="whatever"',
    $xml,
);
$swept = SecurityTokenServiceType::fromXML(
    DOMDocumentFactory::fromString($withExtension)->documentElement,
);

foreach ($swept->getAttributesNS() as $a) {
    printf(
        "     bucket: {%s}%s = %s  -> %s\n",
        $a->getNamespaceURI(),
        $a->getAttrName(),
        $a->getAttrValue()->getValue(),
        (new ReflectionClass($a->getAttrValue()))->getShortName(),
    );
}
printf(
    "     \$type : xsi:type = %s  -> %s\n",
    $swept->getXsiType()->getValue(),
    (new ReflectionClass($swept->getXsiType()))->getShortName(),
);
check(
    'swept attributes arrive as StringValue, the modelled type as QNameValue',
    $swept->getAttributesNS()[0]->getAttrValue() instanceof StringValue
        && $swept->getXsiType() instanceof QNameValue,
);


printf("\n%d passed, %d failed\n\n", $pass, $fail);
exit($fail === 0 ? 0 : 1);

Section 4 doesn't touch WS-Federation code at all — it uses xs:documentation, which models
xml:lang as $lang and excludes it from the bucket, the same shape as md:RoleDescriptor
modelling xsi:type as $type. Passing xml:lang in namespacedAttributes gets
InvalidDOMAttributeException; an unexcluded attribute is accepted. That's the write-side ban
in setAttributesNS() (ExtendableAttributesTrait:246-252), independent of this PR.

Unrelated to this change, but worth mentioning: Scrutinizer has been failing on this branch
since f53e333 ("Analysis: Errored – Tests: failed") while master is green. That commit only
touched the pre-commit script in composer.json, so I suspect it's environmental rather than
a real regression — I can't see the inspection details without access.

[tvdijen]

Much cleaner now! Thanks @cicnavi !

[tvdijen]

PS: Scrutinizer is dead (unless you pay for it).. It lacks proper PHP 8.1+ support

[tvdijen]

We had one tiny little issue on the new major version of this library, which uses the PHP 8.4 new DOM-API, which is much stricter:

https://github.com/simplesamlphp/xml-ws-federation/actions/runs/32058589797/job/95474529720

I got it covered now where it belongs:
simplesamlphp/xml-common@c50b33d