Sanitize mdattr: tests

[tvdijen]

Add serialization tests as well

[tvdijen]

While working on this I noticed that we have implemented this element wrong.. It may only contain Attribute- or Assertion-elements, but right now we handle everything that's not an Attribute as Chunk.

Also before my sanitizing, we had tests is place that would test for the presence of <saml:Assertion with some attributes and self-closing tag />. This is however illegal behaviour, because the Assertion MUST contain several elements and even a signature is mandatory.

See: http://docs.oasis-open.org/security/saml/Post2.0/sstc-metadata-attr-cs-01.pdf

[jaimeperez]

Just to confirm here what we have already discussed: yes, our implementation was wrong.

I think we should also complete the implementation here with the restriction for attribute statements in the assertions:

Assertions that appear MUST conform to the profile in section 2.4, and will contain only attribute statements.

We can also add checks in SAML2\XML\md\Extensions:

Finally, this element MUST NOT appear more than once within a given <md:Extensions> element.

There are also quite a lot of restrictions imposed depending on whether this is included in an <md:EntityDescriptor> or <md:EntitiesDescriptor> element, according to the document.

[codecov[bot]]

Codecov Report

Merging #218 (eb0bfc0) into master (cb0a4ca) will decrease coverage by 12.57%. The diff coverage is 22.72%.

:exclamation: Current head eb0bfc0 differs from pull request most recent head 55e309c. Consider uploading reports for the commit 55e309c to get more accurate results

@@              Coverage Diff              @@
##             master     #218       +/-   ##
=============================================
- Coverage     88.89%   76.32%   -12.58%     
- Complexity     1622     2409      +787     
=============================================
Files           162      166        +4     
Lines          5125     6035      +910     
=============================================
+ Hits           4556     4606       +50     
- Misses          569     1429      +860     

[jaimeperez]

One minor detail I just realized about: Assertions don't need to be signed. An assertion can be included in a Response which is signed in turn, and that must suffice.

[tvdijen]

Normally, yes.. However:

The assertion MUST be independently signed (rather than inheriting a signature from the metadata itself)

[jaimeperez]

But that's business logic, not the schema itself. I mean, that's mandatory for certain uses, but not always. Therefore, we cannot enforce a signature to be always present.

[tvdijen]

This still needs tests with a <saml:Assertion> included, but I'm keen to know what you think of the changes I've made today to enforce the specs as far as we can

[tvdijen]

This one needs a bit more work as soon as #230 is merged