simplesamlphp / saml2

SimpleSAMLphp low-level SAML2 PHP library
https://www.simplesamlphp.org
GNU Lesser General Public License v2.1
286 stars 135 forks source link

Add Subject-class & restructure Identifier-classes #221

Closed tvdijen closed 4 years ago

codecov[bot] commented 4 years ago

Codecov Report

Merging #221 into master will increase coverage by 0.23%. The diff coverage is 92.07%.

@@             Coverage Diff              @@
##             master     #221      +/-   ##
============================================
+ Coverage     87.37%   87.61%   +0.23%     
- Complexity     2122     2159      +37     
============================================
  Files           143      146       +3     
  Lines          5395     5481      +86     
============================================
+ Hits           4714     4802      +88     
+ Misses          681      679       -2     
tvdijen commented 4 years ago

@jaimeperez I went with the BaseID implementation equal to OpenSAML.. Intentionally left out the implementation of the encryption classes to keep this easy to review.

Other than that, I'm thinking we could use a IdentifierTrait holding the properties and getter/setter methods for BaseID/NameID/EncryptedID because the combination of the three is common throughout the specs.. What do you reckon?

tvdijen commented 4 years ago

The BaseID was a little hard to understand, but we finally figured it out according to the following articles: https://www.mendix.com/blog/xml-inheritance-extension-mapping-documents/ https://www.informit.com/articles/article.aspx?p=1398625&seqNum=4 https://referencesource.microsoft.com/#System.IdentityModel/System/IdentityModel/Tokens/Saml2SecurityTokenHandler.cs (around line 2300)

jaimeperez commented 4 years ago

Oh my, this one was hard! 😅 Not because it was hard to review, because this functionality in particular is quite difficult to model in a way that makes sense.

tvdijen commented 4 years ago

I know.. It's horrible :D I'll try and work on the suggested changes.. Not all comments make sense immediately..

jaimeperez commented 4 years ago

I know.. It's horrible :D I'll try and work on the suggested changes.. Not all comments make sense immediately..

That's not a surprise, I'm aware I'm not particularly clear today. Just let me know if you need me to clarify something. I can also provide examples, of course.