Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: attrValueMapper fails to parse complex AttributeValue tags (#245) #427

Conversation

dfdeagle47
Copy link

Context

This fixes an issue where the attrValueMapper would fail to properly map the value for complex AttributeValue tags. This handles the case where the AttributeValue contains a nested NameID tag.

One such example is the eduPersonTargetedID that is used as an identifier in eduGAIN which can return an Attribute of the form

<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://idp.example-university.fr/idp/shibboleth" SPNameQualifier="https://www.service-provider.com/shibboleth">a6c2c4d4-08b9-4ca7-8ff9-43d83e6e1d35</saml2:NameID>
  </saml2:AttributeValue>
</saml2:Attribute>

Limitations

Note that in reality, the AttributeValue tags can be much more complex. The Assertions and Protocols for the OASIS Security Assertion Markup Language (SAML) V2.0 uses the following schema for the Attribute tag:

<element name="Attribute" type="saml:AttributeType"/>
<complexType name="AttributeType">
  <sequence>
    <element ref="saml:AttributeValue" minOccurs="0" maxOccurs="unbounded"/>
  </sequence>
  <attribute name="Name" type="string" use="required"/>
  <attribute name="NameFormat" type="anyURI" use="optional"/>
  <attribute name="FriendlyName" type="string" use="optional"/>
  <anyAttribute namespace="##other" processContents="lax"/>
</complexType>

and the following schema for the AttributeValue:

<element name="AttributeValue" type="anyType" nillable="true"/>

which means it can take any type.

As pointed out in this issue, it is customary to use NameQualifier and the SPNameQualifier in addition to the actual value to create a unique identifier for the platform. That is why the AttributeValue is mapped to an object containing the attribute of the NameID tag as well as the string value for eduPersonTargetedID that is stored in the Value property.

Additional information regarding the fix

  • The attrValueMapper is extracted to the SAML prototype to make it easier to test and it is renamed to attributeValueMapper to make it more explicit.
  • Tests have been added to check that it correctly handles the use case where the AttributeValue contains a nested NameID tag.
  • Previously, the attrValueMapper would return undefined for this use case. Now, it returns an object where Value is the nested string value and the tag attributes are properties of that object.

Related PRs and issues

This should close the following PRs (although the mapping is different):

This should close the following issue:

@dfdeagle47 dfdeagle47 force-pushed the fix/complex-attribute-value-mapping branch 3 times, most recently from 45664c6 to 9bb8257 Compare March 9, 2020 16:51
@dfdeagle47
Copy link
Author

dfdeagle47 commented Mar 9, 2020

I still have to fix one issue before merging.

Edit: should be fixed now. I had an incorrect change in this PR because I'm trying to make it compatible for 0.35.x, but I added up making a separate commit on another branch starting from 0.35.0.

@dfdeagle47 dfdeagle47 force-pushed the fix/complex-attribute-value-mapping branch from 9bb8257 to 96a0edd Compare March 9, 2020 17:25
…-saml#245)

This fixes an issue where the `attrValueMapper` would fail to properly
map the value for complex `AttributeValue` tags. This handles the case
where the `AttributeValue` contains a nested `NameID` tag.

One such example is the `eduPersonTargetedID` that is used as an
identifier in [eduGAIN][1] which can return an Attribute of the form

```xml
<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://idp.example-university.fr/idp/shibboleth" SPNameQualifier="https://www.service-provider.com/shibboleth">a6c2c4d4-08b9-4ca7-8ff9-43d83e6e1d35</saml2:NameID>
  </saml2:AttributeValue>
</saml2:Attribute>
```

Note that in reality, the `AttributeValue` tags can be much more complex.
[The Assertions and Protocols for the OASIS Security Assertion Markup
Language (SAML) V2.0][2] uses the following schema for the `Attribute`
tag:

```xml
<element name="Attribute" type="saml:AttributeType"/>
<complexType name="AttributeType">
  <sequence>
    <element ref="saml:AttributeValue" minOccurs="0" maxOccurs="unbounded"/>
  </sequence>
  <attribute name="Name" type="string" use="required"/>
  <attribute name="NameFormat" type="anyURI" use="optional"/>
  <attribute name="FriendlyName" type="string" use="optional"/>
  <anyAttribute namespace="##other" processContents="lax"/>
</complexType>
```

and the following schema for the `AttributeValue`:

```xml
<element name="AttributeValue" type="anyType" nillable="true"/>
```

which means it can take any type.

As pointed in [3], it is customary to use `NameQualifier` and the
`SPNameQualifier` in addition to the actual value to create a unique
identifier for the platform. That is why the `AttributeValue` is mapped
to an object containing the attribute of the `NameID` tag as well as
the string value for `eduPersonTargetedID` that is stored in the `Value`
property.

[1]: https://wiki.geant.org/display/eduGAIN/Identifier+Attributes
[2]: https://docs.oasis-open.org/security/saml/v2.0/saml-core-2.0-os.pdf
[3]: node-saml#245 (comment)
@dfdeagle47 dfdeagle47 force-pushed the fix/complex-attribute-value-mapping branch from 96a0edd to c1801a4 Compare March 9, 2020 18:14
@cjbarth
Copy link
Collaborator

cjbarth commented Dec 15, 2020

Since #245 is closed, should this be closed too @dfdeagle47 ?

@dfdeagle47
Copy link
Author

@cjbarth

Since #245 is closed, should this be closed too @dfdeagle47 ?

Looks like it does, thanks for the update.

You might want to look at those PRs as well:

because they seem to be fixing the same issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants