Refactor the OneLogin::RubySaml::Metadata class#602
Refactor the OneLogin::RubySaml::Metadata class#602pitbulk merged 6 commits intoSAML-Toolkits:masterfrom
Conversation
…ass so it is easier to extend by breaking it into a series of methods. Also adds the #add_extras convenience method which is empty but can be extended. No change to behavior.
| if sp_sso_descriptor = self.elements["/md:EntityDescriptor"] | ||
| self.root.insert_before sp_sso_descriptor, signature_element | ||
| if sp_sso_descriptor = self.elements["/md:EntityDescriptor/md:SPSSODescriptor"] | ||
| self.root.insert_before(sp_sso_descriptor, signature_element) |
There was a problem hiding this comment.
@pitbulk the code as I have it here is correct with what you are saying. self.root is EntityDescriptor, so self.root.insert_before(sp_sso_descriptor, ...) means "Insert with EntityDescriptor as the parent, and before md:SPSSODescriptor as a sibling). I've tested it and it works correctly.
Here's the REXML documenation: https://ruby-doc.org/stdlib-2.5.1/libdoc/rexml/rdoc/REXML/Parent.html#method-i-insert_before
(It's a mystery that the previous code worked at all; it probably should have thrown an error.)
There was a problem hiding this comment.
Ok, in case of md:EntitiesDescriptor Signature needs to be added as first child which is covered later... I think you are right.... maybe we could add a unittest to verify where the signature is added on the different scenarios to assure it is not break in the future. Rather than that the PR seems ok, I just had a minus comment about a refactor. Good job.
There was a problem hiding this comment.
In that case we should force it to always insert explicitly as the first child of md:EntitiesDescriptor. The way the code is now, it "happens to work" because md:SPSSODescriptor happens to be the first child. But if one's subclass were to do some crazypants custom modification of the xml in the add_extras method, this might no longer be the case.
There was a problem hiding this comment.
I'm not an expert in REXML so I'll have to check the right way to do this. If you know offhand please let me know :)
There was a problem hiding this comment.
Not an REXML expert neither ;) , that why makes sense the extra unittests
There was a problem hiding this comment.
Unit test added and ready for final review. Note that unit test includes validate_xml! which catches most of errors.
In a follow-up PR we should consider to have the Metadata class itself do the XSD validation, since we're now allowing users to add custom elements and the validation is quite strict.
|
@pitbulk good to merge? |
Fixes #433
Example of extending the class: