Allow duplicate named attributes in SAML2 assertions#261
Allow duplicate named attributes in SAML2 assertions#261pitbulk merged 8 commits intoSAML-Toolkits:masterfrom
Conversation
|
In the php-saml toolkit, we added a new setting to allow/disallow duplicated Names/Friendlynames Are you able to extend your PR to allow duplicate names or friendlynames, when the new setting is enabled? |
|
Certainly. I'll work on it more next week. Should the setting be part of
the SP config, and should it default to true or false?
…On Fri, Jun 4, 2021, 6:01 PM Sixto Martin ***@***.***> wrote:
In the php-saml toolkit, we added a new setting to allow/disallow
duplicated Names/Friendlynames
In order to be aligned, we should implement it here in a similar way.
Are you able to extend your PR to allow duplicate names or friendlynames,
when the new setting is enabled?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#261 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIYCGXG7WRXWZPBFW76VYLTRFEMHANCNFSM46DB5PUA>
.
|
|
At php-saml it was a new setting parameter at the advanced section: allowRepeatAttributeName Here is the commit that introduced the feature on php-saml: SAML-Toolkits/php-saml@370a5d9 |
|
I have updated the MR to pass the style checks, added tests for both the positive and negative cases, and condensed the attribute retrieval logic into one method. I added a setting under security that mirrors the name in the php-saml repo, and a settings file called settings11.json to ensure the attribute loading works as intended. |
doticatto
changed the title
|
@pitbulk Any changes requested? |
| if attr_text: | ||
| values.append(attr_text) | ||
|
|
||
| # Parse any nested NameID children |
There was a problem hiding this comment.
You removed the support of the nested NameID children, we need to keep supporting it.
|
Ah, yes that was an oversight on my part. Will add it back and add tests
for it
…On Mon, Jun 14, 2021, 10:31 AM Sixto Martin ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/onelogin/saml2/response.py
<#261 (comment)>:
> - attr_name = attribute_node.get('Name')
- if attr_name in attributes.keys():
- raise OneLogin_Saml2_ValidationError(
- 'Found an Attribute element with duplicated Name',
- OneLogin_Saml2_ValidationError.DUPLICATED_ATTRIBUTE_NAME_FOUND
- )
-
- values = []
- for attr in attribute_node.iterchildren('{%s}AttributeValue' % OneLogin_Saml2_Constants.NSMAP['saml']):
- attr_text = OneLogin_Saml2_XML.element_text(attr)
- if attr_text:
- attr_text = attr_text.strip()
- if attr_text:
- values.append(attr_text)
-
- # Parse any nested NameID children
You removed the support of the nested NameID children
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#261 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIYCGXFYGRIHYLYK7YFN4LTSYVFJANCNFSM46DB5PUA>
.
|
|
@pitbulk there is an existing test for getting nested NameID attributes, and the code is still included in the _get_attributes() method. The testGetNestedNameIDAttributes test on line 760 of response_test.py tests this scenario and still passes as expected. Unless I am misunderstanding the issue I do not believe it breaks support for this case |
|
I added a test for nested NameID attributes when retrieving FriendlyName attributes, all tests pass as expected. Please let me know if I have missed something |
4 participants
In the SAML spec, the "friendlyName" is defined as a completely optional for attribute naming, and should not be relied upon for retrieving attributes. It exists simply to provide a more human-readable method to decipher attributes. From the spec (https://docs.oasis-open.org/security/saml/v2.0/saml-core-2.0-os.pdf):
FriendlyName [Optional]
A string that provides a more human-readable form of the attribute's name, which may be useful in cases in which the actual Name is complex or opaque, such as an OID or a UUID. This attribute's value MUST NOT be used as a basis for formally identifying SAML attributes.
This MR adds the possibility of multiple attributes with the same FriendlyName, and creates a list with all of the attributes with the same FriendlyName appended to it.