PR#86: Fix handling attributes with multiple values (e.g. groups) - ipsilon

ipsilon

#86 Fix handling attributes with multiple values (e.g. groups)

Merged 7 years ago by puiterwijk. Opened 8 years ago by amishhammer.

amishhammer/ipsilon master into master

Fix handling attributes with multiple values (e.g. groups)

Tom Judge • 8 years ago

9bd14a3

file modified

+8 -4

		`@@ -319,18 +319,22 @@`
		`continue`
		`if not isinstance(values, list):`
		`values = [values]`
		`+ attr = lasso.Saml2Attribute()`
		`+ attr.name = key`
		`+ attr.nameFormat = lasso.SAML2_ATTRIBUTE_NAME_FORMAT_BASIC`
		`+ attr.attributeValue = []`
		`+ vals = []`
		`for value in values:`
		`- attr = lasso.Saml2Attribute()`
		`- attr.name = key`
		`- attr.nameFormat = lasso.SAML2_ATTRIBUTE_NAME_FORMAT_BASIC`
		`value = str(value).encode('utf-8')`
		`self.debug('value %s' % value)`
		`node = lasso.MiscTextNode.newWithString(value)`
		`node.textChild = True`
		`attrvalue = lasso.Saml2AttributeValue()`
		`attrvalue.any = [node]`
		`- attr.attributeValue = [attrvalue]`
		`attrstat.attribute = attrstat.attribute + (attr,)`
		`+ vals.append(attrvalue)`
		`+`
		`+ attr.attributeValue = vals`

		`self.debug('Assertion: %s' % login.assertion.dump())`

no initial comment

amishhammer commented 8 years ago

Fix handling attributes with multiple values (e.g. groups)

When handling attributes with more than one value provide a single
saml:Attribute with multiple saml:AttributeValue's.

I.e.

<saml:Attribute Name="groups" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic">
<saml:AttributeValue>group1</saml:AttributeValue>
<saml:AttributeValue>group2</saml:AttributeValue>
</saml:Attribute>

Not:

<saml:Attribute Name="groups" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic">
<saml:AttributeValue>group1</saml:AttributeValue>
</saml:Attribute>
<saml:Attribute Name="groups" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic">
<saml:AttributeValue>group2</saml:AttributeValue>
</saml:Attribute>

This fixes handling of attributes with more than one value for pac4j
based clients (Such as the Jenkins SAML plugin).

rcritten commented 8 years ago

Why use the temporary variable val and not just append directly? Otherwise looks ok to me.

amishhammer commented 8 years ago

I tried to get it to append directly but something in the saml attribute
class caused the list to be immutable when trying to append to it and I had
to convert it back and forwards to a mutable type or use a temp var and the
temp car seemed cleaner.

On Wednesday, April 20, 2016, pagure@pagure.io wrote:

rcritten commented on the pull-request: Fix handling attributes with multiple values (e.g. groups) that you are following:
Why use the temporary variable val and not just append directly? Otherwise looks ok to me.

To reply, visit the link belowor just reply to this email
https://pagure.io/ipsilon/pull-request/86

rcritten commented 8 years ago

The ldap test is failing:

ERROR: ValueError('Expected [Test Group;Test Group 2], got [Test Group;Test Group 2;Test Group;Test Group 2]',)

puiterwijk commented 8 years ago

You just need to move one line to make it work: please move ipsilon/providers/saml2/auth.py line 334 to line 338.
That will make the adding of the attributes happen after building the full attribute object.
If you could amend your commit and push the new one to your branch, I can apply your patch.

Also, it would be much appreciated if you add a Signed-off-by line to acknowledge you accept to publishing your patch under the license of Ipsilon (GPLv3).

puiterwijk commented 7 years ago

This has now been merged. Thanks!