support storing attributes as \SAML2\XML\saml\Attribute objects

[tvdijen]

updated \SAML2\Assertion to use \SAML2\XML\saml\Attribute objects instead of an array of string|int|DOMElement.

This is a fairly major change, but the getAttributes and setAttributes methods have default params to handle backwards compatibility.
This is a "fix" for issue simplesamlphp/simplesamlphp#690

This PR is a continuation of #106 and the credits for it should go to @dub357

[coveralls]

Coverage decreased (-0.5%) to 67.986% when pulling d210b51 on tvdijen:feature-friendlynames into d809ffb on simplesamlphp:master.

[thijskh]

I have a hard time to follow what this tries to accomplish and how it relates to HoK and SOAP.

[tvdijen]

@dub357 Are you reading this?
I hope I haven't messed up reconstructing your original PR, but since the original commits are gone on your fork, there's no way for me to be sure..

[dub357]

So the stuff related to bindings was actually not supposed to be part of the original PR. My intent was to only have saml2:master pull in my changes to SAML2/Assertion.php and SAML2/XML/saml/AttributeValue.php. The binding stuff was committed after my original PR was created and since I'm new to GitHub and didn't really understand how the pull request process works, I didn't realize any additional commits on the same branch were going to be added to the open PR.

The binding changes were my attempt to make all bindings return their URN and should have probably not been committed/pushed to my default branch. During my testing with other SPs, I've run into the situation where they expect a response to come back using the same saml2 web browser profile binding (request vs post) that the original authn request was sent with (which is really a bug in their code). Because of this I was changing simplesamlphp to see if that could be 'fixed'. It did solve the problem, but I didn't mean for that stuff to end up in the PR. I attribute this to being a GitHub newbie LOL.

I also see that during the copy/merging/squashing/whitespace removal nightmare tvdijen and I were stuck in, some of the changes didn't make it over because the binding stuff looks incomplete as there was also a change to SAML2\Binding.php (adding "abstract public function getURN();") that I don't see in the commit. There may be other things that were missed, but like tvdijen stated, I can't tell now for some reason. I have absolutely no idea how all these changes have been removed in my fork. Its almost like my fork is even with simplesamlphp/saml2:master now as all the commits I pushed are gone now. wtf. I have no idea how that has happened. Somehow it happened after I had to fix all the whitespace issues directly in the fork because I couldn't figure out how to pull changes from master back into my fork. At least I still have the changes in my local repo...

Anyway - the intent for my original PR was only to have the Assertion object store an array of Attribute objects instead of scalar values and introduce a way to define name formats and friendly names per attribute. Should have really only been the 2 files I mentioned above.

[tvdijen]

@dub357 I've removed the unnecessary stuff now.
Could you please check this against your local repo and make sure nothing is missing?

[dub357]

Looks like one other change is needed...not sure how I didn't catch this, maybe there isn't a test for it, but in SAML2/XML/saml/AttributeValue.php, the "\XML\saml\NameID" references are not correct. They should probably just be "NameID" since that class is in the same namespace as AttributeValue. Other than that, it looks OK to me.

[tvdijen]

Okay, then it's up for review again!

[dub357]

I see there has been no movement on this PR...its been a couple months and nothing...no feedback, no approval, nothing. What can we do to get this back on track?

One of the biggest drawbacks with simplesamlphp is its limited ability to deal with attributes of assertions. I want to add more capability to simplesamlphp in this area, but the biggest hurdle is this library and its lack of treating attributes as objects. Folks have already pushed more and more code into the Assertion class in this library to 'control' attributes (overridding xsi types) when in all reality, none of that should really be in there at all. Its API should give you the ability to get/set the attributes as objects so that the developer has more control to do what they need with them....whether that is getting/setting their friendly names, nameformats or xsi types.
The code I wrote in this PR this is backwards compatible...any idea what the hold up is?

[thijskh]

Just a lack of time with the regular contributors I'm afraid. We'll get to it.

[tvdijen]

There's still a few conflicts left in Assertion.php that I don't know how to resolve

[thijskh]

This contains a lot of merge leftovers that make it hard to review properly. Needs to be cleaned up.

[tvdijen]

Two issues left in Assertion.php that I'm not sure of how to fix.

[tvdijen]

Hey @dub357 ! We're about to release v5 of this module and I really really really want to include this PR since it's been open for way too long..
I've finally managed to resolve all the merge conflicts that we had, but there still are some issues.. This library went through a lot of change over the last year..
You think you can give me helping hand here to get the tests to pass? The good thing is that, for a new major version, we don't have to keep BC-compatibility and can break stuff properly..