Skip to content

Conversation

@ashimaathri
Copy link
Contributor

@ashimaathri ashimaathri commented Oct 5, 2016

This fixes the issue when integrating with onelogin's python3-saml client:

Element '{urn:oasis:names:tc:SAML:2.0:assertion}AttributeValue', attribute '{http://www.w3.org/2001/XMLSchema-instance}type': The QName value 'xs:string' has no corresponding namespace declaration in scope.

This fixes the issue when integrating with onelogin's python3-saml
client:
Element '{urn:oasis:names:tc:SAML:2.0:assertion}AttributeValue',
attribute '{http://www.w3.org/2001/XMLSchema-instance}type': The QName
value 'xs:string' has no corresponding namespace declaration in
scope.
@ashimaathri
Copy link
Contributor Author

Oops! This fixes the same issue as #336, but in a different way.

@jaustinpage
Copy link

Either #336 or this pr should be merged, not both. both look good to me 👍

Copy link

@jaustinpage jaustinpage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link

@jaustinpage jaustinpage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either #336 or this pr should be merged, not both. both look good to me 👍

@jgehrcke
Copy link
Contributor

jgehrcke commented Oct 5, 2016

  1. I think it's cleaner to just conditionally add this attribute, as is done in AttributeValueBase: include xs namespace definition in set_type #336.

  2. This PR here sets self._extatt['xmlns:xs'] in addition to self.extension_attributes["xmlns:xs"]. In my tests (when I was working on AttributeValueBase: include xs namespace definition in set_type #336) this was not required and it is too long ago for me to understand the difference. @ashimaathri are you able to explain, hopefully having this at the top of your head? :-)


try:
self.extension_attributes[XSI_TYPE] = typ
self.extension_attributes['xmlns:xs'] = XS_NAMESPACE

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaustinpage
Copy link

jaustinpage commented Oct 5, 2016

@jgehrcke it looks like a fallback. The access that ashima is doing looks consistent with other code in here... https://github.com/rohe/pysaml2/blob/master/src/saml2/saml.py#L241

self._extatt gets set on the object init to be empty. self.extension_attributes gets set to this empty object. https://github.com/rohe/pysaml2/blob/master/src/saml2/saml.py#L140 (i am guessing this is for legacy use of this object)

self.extension_attributes is from SamlBase, the class this extends. https://github.com/rohe/pysaml2/blob/master/src/saml2/__init__.py#L302

@ashimaathri
Copy link
Contributor Author

@jgehrcke Thanks for you comment. I've updated my code to conditionally add this attribute. And it looks like @jaustinpage has answered your other questions. I'm ok with either PR getting in as long as the issue is fixed.

@rohe rohe merged commit cfad434 into IdentityPython:master Oct 12, 2016
@ashimaathri ashimaathri deleted the add-namespace-to-attribute-value branch October 19, 2016 19:36
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.

4 participants