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 https://github.com/simplesamlphp/simplesamlphp/issues/690

This PR is a continuation of https://github.com/simplesamlphp/saml2/pull/106 and the credits for it should go to @dub357

[coveralls]

Coverage decreased (-0.5%) to 67.986% when pulling d210b51514143d1ab3435ff75d0fc66392b0111d on tvdijen:feature-friendlynames into d809ffbe2aa7260a0b3e72bf5be8cd6de0325d7c 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

[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..

[dub357]

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..

It has been open a long time...quite honestly I gave up on it over a year ago for a number of reasons, but mainly because I felt like no one was making an effort to review the PR and provide feedback or approve it. There were even other pull requests merged into the project while this one was just sitting out there, allowing it to divert further and further away from master. But in addition to that, the whitespace nightmare that I/we originally ran into (I'm on a Windows environment) and the "strangeness" related to having all the commits to my fork post-creation of the pull request becoming part of the actual pull request itself left me with a bad taste for GitHub LOL.

That being said, I'd still like to see this functionality make it into the library. My original goal was to make SSP more robust with respect to attribute friendly names, name formats and xsi types...basically all the stuff that SSP couldn't do because of its rudimentary attribute handling and all of that stemmed from this library.

I see you've been doing a lot in this fork lately. I appreciate you pushing to get this incorporated into the next major release and I'll do what I can to help. I can take a look at the tests, are some of them failing? Do we need more coverage?

[tvdijen]

I totally feel your pain.. It's not that we don't care, but I gave up on it as well because I couldn't get rid of some of the conflicts.. Today I just went in head-first and I think it went pretty well!

What we basically need to do is get it to pass the tests (we may even break BC to accomplish this).. See https://travis-ci.org/tvdijen/saml2/jobs/624802912

I have also expressed some concerns in code-comments.. I'm especially worried about the attributeValueTypes-property on the Assertion-class because it feels misplaced.. A value type is a characteristic of AttributeValue and should probably be on that class.

I think at some point you've tried to keep BC, but that has been broken due to changes in the library.. If you ask me, we should just work with Attribute-objects in Assertion->attributes and nothing else..

Let's make this PR (finally) happen!

[dub357]

So looking at this now, I think perhaps I need to start over from scratch with this PR because I dont see any of the changes I was trying to introduce 2 years ago. I can take a stab at it over the holiday break. Back when I did this, I only made changes to 10 files, so it might not be to difficult. What do you think? I haven't looked at master in quite some time so I dont know how far off I would be, or even how far off your fork is from master. So, if I wanted to help with this, should I fork your branch or master?

Ok maybe nevermind...it looks like your stuff does kinda work in the same manner, its just vastly different from what I originally pushed. Its gonna take me some time to wrap my head around the differences but I'll give it a shot

[tvdijen]

I'm not sure what you mean, because all the changes in this PR originate from your previous PR.

Let's get straight on the original purpose... You wanted FriendlyNames to be passed through the proxy by having the Assertion-class use Attribute-objects in the attributes-property, right?

Anyway, starting over may be the best way.. It probably already was after all the merging mess back in 2017/18, and it makes a lot of sense after the extensive amount of changes made to this library over time. I still think the changes in this PR make sense though..

@jaimeperez I imagine this PR with all the history is hard to follow, but could you give it a shot and give us some direction here?

[dub357]

@tvdijen So I forked your feature-friendlynames branch yesterday and starting doing some playing around. Turns out that some of the changes from my original proposed PR seem to be causing a lot of these test failures. Mainly not allowing the assertion->setAttributes to be called without anything other than an array of Attribute objects. From your original comment, it seems like this is because you don't want to have to worry about backwards compatibility. Is there any reason you don't want to allow this? Seems like its more work to have to change all the old tests rather than create new ones...

There are also a few other changes you've made that break the ability to use friendly names and name formats on a per attribute basis...especially your latest commits. This capability was the core functionality introduced by my original PR. The use of Attribute objects in the Assertion array was just a means to an end so to speak. This library is the core of SSP and if one cannot control the friendly names, name formats and xsd types of produced attributes from simple attribute-based arrays in SSP config or metadata, then all this work seems like a waste.

I also noticed that you committed/pushed a few other things to the branch after I forked. I had even made the same change locally that you committed in e92f28d. I don't want to be working on a moving target and I don't want one of us to be wasting our time. I'm tempted to simply fork master, make my changes there and push another PR, but I dont want to conflict with the existing PR you have in place - which was based on my original PR but has since deviated away quite a bit. In that respect, I dont really know what to do at this point and some guidance would be helpful...

[tvdijen]

Mainly not allowing the assertion->setAttributes to be called without anything other than an array of Attribute objects.

I think that's exactly what we want, because it makes life much easier if we can be sure about the contents of the array.. I would say we need a Attribute::fromArray() to cover conversion and repair our tests

[jaimeperez]

Thanks a lot for your patience @dub357! And sorry so much that this didn't progress as it should. It is mostly my fault because I have less and less time to spend on this, so I have to prioritize the stuff that's urgent.

In any case, as Tim was mentioning, I think it's a good idea to add this as a feature in a new major release, so I'll try my best to have a look into it during the Christmas holidays.

Tim, I guess we just need to change the interfaces and make them expect Attribute objects, or arrays of them. AFAIK, you can tell psalm you have such an array in a parameter, even though the data type will just be array. That would help us uncover possible bugs during the implementation. We'll need to adapt the tests as well, but I think you are already on track with that.

I'm also wondering how we could adopt this in SSP. I think it would definitely make a nice addition to 2.0, and we can't have it in place before because it definitely breaks bc. In that case, your idea to convert to/from arrays could be helpful to have an early implementation with backwards compatibility.

[dub357]

So I created pull request to this branch from my fork (https://github.com/tvdijen/saml2/pull/1). It fixes all the tests but I had to make a couple tweaks to Assertion, Attribute and AttributeValue to make it work. Take a look at let me know what you think.... I'm must say that anyone using this library outside of SSP will not be happy if/when they upgrade to 5.0 or whatever version these changes are targeted for because it breaks the current API pretty hard

[codecov[bot]]

Codecov Report

Merging #122 into master will increase coverage by 0.07%. The diff coverage is 97.05%.

@@             Coverage Diff              @@
##             master     #122      +/-   ##
============================================
+ Coverage     66.87%   66.95%   +0.07%     
- Complexity     1894     1911      +17     
============================================
  Files           119      119              
  Lines          4695     4690       -5     
============================================
  Hits           3140     3140              
+ Misses         1555     1550       -5