Skip to content

Added support for inclusive namespaces #1

Merged
merged 4 commits into from Jul 3, 2011

3 participants

@bluemango

I working on getting the ruby-saml gem to play nicely with Salesforce. Salesforce SAML assertions use inclusive namespaces so I had to add support to the canonix gem for this. I have included inclusive namespace support as well as some additional tests for verify the update.

Thanks,
Greg DeVore

@brendon
Owner

Hi Greg, thanks for the pull request. I've adopted this gem but have next to no idea of the detail of how the code works. I've had a look through though and wondered if you could fix a couple of things (or let me know why they shouldn't be fixed) :D

There appears to be some abandoned code around the @inclusive_namespaces variable that you've abandoned in your patch. Would you be able to remove those references also? They exist on line 135 and also in the initialiser line 111.

Also, would you be able to remove the reference to salesforce in the test and just say should "canonicalize a saml file with inclusive ns assertions" :)

This is my first pull request, so let me know if I'm doing this all wrong :)

Owner

Also, if you're making changes/adding features to the ruby-saml gem itself, it might pay to check out relevances fork of that project as they've already done heaps of enhancements and I'm just waiting on them to push them up to the master.

Would you be able to link me to some documentation on ns assertions? I've tried to find some info but have come up dry :) Either that or a quick explanation of what's going on in the patch would be great :) Sorry for the ignorance :)

@brendon
Owner
brendon commented Jun 29, 2011

Hi Greg, thanks for the info. I agree, I find this whole SAML thing a huge drama for something so simple! The mere fact that ruby support is so underdeveloped speaks volumes as to its adoption in the trendier real world!

I just had a look back at the code and that mention of @inclusive_namespaces in canonicalize_element is only invoked if @prefix_list is not false. I've traced @prefix_list through the whole script and it is initialised as nil, and never gets changed or used, so I guess it's safe to remove both references to @inclusive_namespaces and @prefix_list. Would you be happy to do that? Definitely have a doublecheck also just in case I've missed something big :)

@brendon
Owner
brendon commented Jun 29, 2011

You're right regarding the relevance fork, but it would still pay to double check it before you reinvent the wheel just in case :) I'll give the guy a message and see where he's at with prepping the code to be pushed up to master :)

@bluemango
@brendon
Owner
brendon commented Jul 3, 2011

Hi Greg, thanks for that. I just meant for you to remove lines 111, 112 and 134. I'm pretty sure that no one currently uses the canonicalize_element function, but just incase someone decides to drop their usage of xmlcanonicalizer in favour of this gem, I'd prefer to keep the functionality just in case. I'll just accept this commit and revert those changes for you :D

Thanks heaps for your help! :D

Brendon

@brendon brendon merged commit 92880e9 into brendon:master Jul 3, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.