Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor the OneLogin::RubySaml::Metadata class #602

Merged
merged 6 commits into from
Aug 16, 2021

Conversation

johnnyshields
Copy link
Collaborator

@johnnyshields johnnyshields commented Aug 7, 2021

Fixes #433

  • Refactor the OneLogin::RubySaml::Metadata class so it is easier to extend by breaking it into a series of smaller methods.
  • Also adds the #add_extras convenience method which is empty but can be extended.
  • No change to behavior or to public method surface area.

Example of extending the class:

  class MyMetadata < OneLogin::RubySaml::Metadata
    def add_extras(root, _settings)
      org = root.add_element("md:Organization")
      org.add_element("md:OrganizationName", 'xml:lang' => "en-US").text = 'ACME Inc.'
      org.add_element("md:OrganizationDisplayName", 'xml:lang' => "en-US").text = 'ACME'
      org.add_element("md:OrganizationURL", 'xml:lang' => "en-US").text = 'https://www.acme.com'

      cp = root.add_element("md:ContactPerson", 'contactType' => 'technical')
      cp.add_element("md:GivenName").text = 'ACME SAML Team'
      cp.add_element("md:EmailAddress").text = 'saml@acme.com'
    end
  end

…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)
Copy link
Collaborator Author

@johnnyshields johnnyshields Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@johnnyshields johnnyshields Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Collaborator

@pitbulk pitbulk Aug 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not an REXML expert neither ;) , that why makes sense the extra unittests

Copy link
Collaborator Author

@johnnyshields johnnyshields Aug 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@johnnyshields
Copy link
Collaborator Author

@pitbulk good to merge?

@pitbulk pitbulk merged commit c6489cc into SAML-Toolkits:master Aug 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enable additional SP metadata
2 participants