attrValueMapper fails to parse complex AttributeValue tags

[vpalmisano]

attrValueMapper function parse correctly this AttributeStatement:

<saml2:Attribute FriendlyName="NAME" Name="urn:oid:x.x.x.x" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"> <saml2:AttributeValue>VALUE</saml2:AttributeValue> </saml2:Attribute>

but it fails with this kind of AttributeStatement:

<saml2:Attribute FriendlyName="NAME" Name="urn:oid:x.x.x.x" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri"> <saml2:AttributeValue> <saml2:NameID Format="urn:oasis:names:tc:SAML:2.0:nameid-format:persistent" NameQualifier="XXX" SPNameQualifier="XX">VALUE</saml2:NameID> </saml2:AttributeValue> </saml2:Attribute>

[huntdm07]

The PR I've submitted #249 should fix this with support for existing usage.

[alexstuart]

Hello, it's Alex from the UK federation support team here. Just to expand on what @huntdm07 says in #249.

The UK federation uses eduPersonTargetedID to uniquely identify subjects. In SAML 2 eduPersonTargetedID is called urn:oid:1.3.6.1.4.1.5923.1.1.1.10 and, as far as I know, it's the only attribute from the eduPerson schema which has a value that is a saml2:NameID. So I think your code should only be trying to extract parts of the attribute if its Name starts with urn:oid:1.3.6.1.4.1.5923.1.1.1. and its Name == urn:oid:1.3.6.1.4.1.5923.1.1.1.10.

Secondly, the UK federation advises storing eduPersonTargetedID by concatenating the following elements:

1. the entity name of the identity provider
2. a single ‘!’ character
3. the entity name of the service provider
4. a single ‘!’ character
5. the opaque string value

As an example, the attribute received as

<saml2:Attribute
           FriendlyName="eduPersonTargetedID"
           Name="urn:oid:1.3.6.1.4.1.5923.1.1.1.10"
           NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri">
          <saml2:AttributeValue>
                <saml2:NameID
                   Format="urn:oasis:names:tc:SAML:2.0:nameid-format:persistent"
                   NameQualifier="https://test-idp.ukfederation.org.uk/idp/shibboleth"
                   SPNameQualifier="https://test.ukfederation.org.uk/entity">
                  xY1234567890ACdFGGhjmkklljA=
                </saml2:NameID>
          </saml2:AttributeValue>
</saml2:Attribute>

could be flattened to https://test-idp.ukfederation.org.uk/idp/shibboleth!https://test.ukfederation.org.uk/entity!xY1234567890ACdFGGhjmkklljA=

Sorry, I'd write a pull request myself but I don't speak Javascript or know the structure of passport-saml.

[mans0954]

saml-core-2.0-os Section 2.7.3.1.1 says

The <AttributeValue> element supplies the value of a specified SAML attribute. It is of the
xs:anyType type, which allows any well-formed XML to appear as the content of the element.

It doesn't, as far as I can see, specify any particular interpretation of XML appearing in an AttributeValue, even if that XML is a valid SAML fragment.

I note that the latest eduPerson schema says:

NOTE: eduPersonTargetedID is DEPRECATED and will be marked as obsolete in a 
future version of this specification. Its equivalent definition in SAML 2.0 has 
been replaced by a new specification for standard Subject Identifier attributes 
[https://docs.oasis-open.org/security/saml-subject-id-attr/v1.0/saml-subject-id-attr-v1.0.html],
one of which ("urn:oasis:names:tc:SAML:attribute:pairwise-id") is a direct 
replacement for this identifier with a simpler syntax and safer comparison 
rules. Existing use of this attribute in SAML 1.1 or SAML 2.0 should be phased 
out in favor of the new Subject Identifier attributes.

[mans0954]

Just to further muddy the waters, I've suggested a third PR for solving this #447. Since the SAML specification allows any well formed XML to form the content of an AttributeValue, it seems to me that the correct response is to return that XML (or rather an object parsed from it) as the value of the attribute.

Much as I'm a fan of eduPerson, as I understand it passport-saml is targeting the SAML specification rather than eduPerson so we should avoid including eduPerson specific code in passport-saml, particularly when eduPersonTargetedID is deprecated, and the transform to a string suggested by @alexstuart is UKAMF specific (if I have understood correctly) rather than part of eduPerson.

I would be in favour of adding support for https://docs.oasis-open.org/security/saml-subject-id-attr/v1.0/saml-subject-id-attr-v1.0.html in future since that is an OASIS SAML specification.

[dfdeagle47]

Much as I'm a fan of eduPerson, as I understand it passport-saml is targeting the SAML specification rather than eduPerson so we should avoid including eduPerson specific code in passport-saml, particularly when eduPersonTargetedID is deprecated, and the transform to a string suggested by @alexstuart is UKAMF specific (if I have understood correctly) rather than part of eduPerson.

Much appreciated. Perfectly agree it's much better to have a scalable solution which doesn't rely on eduPerson-related attributes (though I didn't have time to propose a cleaner solution).

(Didn't know eduPersonTargetedID was deprecated, thanks for pointing it out, though how wonder how many years it will take for universities to update their configuration ^^)