Merge pull request #759 from johnnyshields/signed-document-info-refactor

[WIP] v2.x - Nokogiri Refactor Part 6 - Conversion of SignedDocument and final removal of REXML 🎉

Author: pitbulk

README.md

@@ -929,22 +929,24 @@ or underscore, and can only contain letters, digits, underscores, hyphens, and p
 
 Some IdPs may require to add SPs to add additional fields (Organization, ContactPerson, etc.)
 into the SP metadata. This can be done by extending the `RubySaml::Metadata` class and
-overriding the `#add_extras` method using a Nokogiri XML builder as per the following example:
+overriding the `#add_extras` method where the first arg is a
+[Nokogiri::XML::Builder](https://nokogiri.org/rdoc/Nokogiri/XML/Builder.html) object as per
+the following example:
 
 ```ruby
 class MyMetadata < RubySaml::Metadata
   private
 
   def add_extras(xml, _settings)
-    xml['md'].Organization do
-      xml['md'].OrganizationName('ACME Inc.', 'xml:lang' => 'en-US')
-      xml['md'].OrganizationDisplayName('ACME', 'xml:lang' => 'en-US')
-      xml['md'].OrganizationURL('https://www.acme.com', 'xml:lang' => 'en-US')
+    xml.Organization do
+      xml.OrganizationName('xml:lang' => 'en-US') { xml.text 'ACME Inc.' }
+      xml.OrganizationDisplayName('xml:lang' => 'en-US') { xml.text 'ACME' }
+      xml.OrganizationURL('xml:lang' => 'en-US') { xml.text 'https://www.acme.com' }
     end
 
-    xml['md'].ContactPerson('contactType' => 'technical') do
-      xml['md'].GivenName('ACME SAML Team')
-      xml['md'].EmailAddress('saml@acme.com')
+    xml.ContactPerson('contactType' => 'technical') do
+      xml.GivenName { xml.text 'ACME SAML Team' }
+      xml.EmailAddress { xml.text 'saml@acme.com' }
     end
   end
 end

UPGRADING.md

@@ -1,6 +1,6 @@
 # Ruby SAML Migration Guide
 
-## Updating from 1.x to 2.0.0
+## Upgrading from 1.x to 2.0.0
 
 **IMPORTANT: Please read this section carefully as it contains breaking changes!**
 
@@ -34,7 +34,7 @@ Note that the project folder structure has also been updated accordingly. Notabl
 `lib/onelogin/schemas` is now `lib/ruby_saml/schemas`.
 
 For backward compatibility, the alias `OneLogin = Object` has been set, so `OneLogin::RubySaml::` will still work
-as before. This alias will be removed in RubySaml version `2.1.0`.
+as before. This alias will be removed in RubySaml version `3.0.0`.
 
 ### Deprecation and removal of "XMLSecurity" namespace
 
@@ -74,24 +74,33 @@ settings.security[:signature_method] = RubySaml::XML::RSA_SHA1
 ### Replacement of REXML with Nokogiri
 
 RubySaml `1.x` used a combination of REXML and Nokogiri for XML parsing and generation.
-In `2.0.0`, REXML has been replaced with Nokogiri. This change should be transparent
-to most users, however, see note about Custom Metadata Fields below.
+In `2.0.0`, REXML has been replaced with Nokogiri. As a result, there are minor differences
+in how XML is generated, ncluding SAML requests and SP Metadata: 
+
+1. All XML namespace declarations will be on the root node of the XML. Previously,
+   some declarations such as `xmlns:ds` were done on child nodes.
+2. The ordering of attributes on each node may be different.
+
+These differences should not affect how the XML is parsed by various XML parsing libraries.
+However, if you are strictly asserting that the generated XML is an exact string in your tests,
+such tests may need to be adjusted accordingly.
 
 ### Custom Metadata Fields now use Nokogiri XML Builder
 
 If you have added custom fields to your SP metadata generation by overriding
-the `RubySaml::Metadata#add_extras` method, you will need to update your code to use
-[Nokogiri::XML::Builder](https://nokogiri.org/rdoc/Nokogiri/XML/Builder.html) format
-instead of REXML. Here is an example of the new format:
+the `RubySaml::Metadata#add_extras` method, you will need to update your code
+so that the first arg of the method is a
+[Nokogiri::XML::Builder](https://nokogiri.org/rdoc/Nokogiri/XML/Builder.html)
+object. Here is an example of the new format:
 
 ```ruby
 class MyMetadata < RubySaml::Metadata
   private
 
-  def add_extras(xml, _settings)
-    xml['md'].ContactPerson('contactType' => 'technical') do
-      xml['md'].GivenName('ACME SAML Team')
-      xml['md'].EmailAddress('saml@acme.com')
+  def add_extras(builder, _settings)
+    builder.ContactPerson('contactType' => 'technical') do
+      builder.GivenName { builder.text 'ACME SAML Team' }
+      builder.EmailAddress { builder.text 'saml@acme.com' }
     end
   end
 end
@@ -101,7 +110,7 @@ end
 
 RubySaml now always uses double quotes for attribute values when generating XML.
 The `settings.double_quote_xml_attribute_values` parameter now always behaves as `true`,
-and will be removed in RubySaml 2.1.0.
+and will be removed in RubySaml 3.0.0.
 
 The reasons for this change are:
 - RubySaml will use Nokogiri instead of REXML to generate XML. Nokogiri does not support
@@ -154,7 +163,7 @@ a different `sp_uuid_prefix` is passed-in on subsequent calls.
 ### Deprecation of compression settings
 
 The `settings.compress_request` and `settings.compress_response` parameters have been deprecated
-and are no longer functional. They will be removed in RubySaml 2.1.0. Please remove `compress_request`
+and are no longer functional. They will be removed in RubySaml 3.0.0. Please remove `compress_request`
 and `compress_response` everywhere within your project code.
 
 The SAML SP request/response message compression behavior is now controlled automatically by the
@@ -166,13 +175,15 @@ compression may be achieved by enabling `Content-Encoding: gzip` on your webserv
 ### Deprecation of IdP certificate fingerprint settings
 
 The `settings.idp_cert_fingerprint` and `settings.idp_cert_fingerprint_algorithm` are deprecated
-and will be removed in RubySaml 2.1.0. Please use `settings.idp_cert` or `settings.idp_cert_multi` instead.
-The reasons for this deprecation are that (1) fingerprint cannot be used with HTTP-Redirect binding,
-and (2) fingerprint is theoretically susceptible to collision attacks.
+and will be removed in RubySaml 3.0.0. Please use `settings.idp_cert` or `settings.idp_cert_multi` instead.
+
+The reasons for this deprecation are:
+- Fingerprint cannot be used with HTTP-Redirect binding
+- Fingerprint is theoretically susceptible to collision attacks.
 
 ### Other settings deprecations
 
-The following parameters in `RubySaml::Settings` are deprecated and will be removed in RubySaml 2.1.0:
+The following parameters in `RubySaml::Settings` are deprecated and will be removed in RubySaml 3.0.0:
 
 - `#issuer` is deprecated and replaced 1:1 by `#sp_entity_id`
 - `#idp_sso_target_url` is deprecated and replaced 1:1 by `#idp_sso_service_url`
@@ -212,7 +223,7 @@ and `#format_private_key` methods. Specifically:
   stripped out.
 - Case 7: If no valid certificates are found, the entire original string will be returned.
 
-## Updating from 1.17.x to 1.18.0
+## Upgrading from 1.17.x to 1.18.0
 
 Version `1.18.0` changes the way the toolkit validates SAML signatures. There is a new order
 how validation happens in the toolkit and also the toolkit by default will check malformed doc
@@ -222,7 +233,7 @@ The SignedDocument class defined at xml_security.rb experienced several changes.
 We don't expect compatibilty issues if you use the main methods offered by ruby-saml, but if
 you use a fork or customized usage, is possible that you need to adapt your code.
 
-## Updating from 1.12.x to 1.13.0
+## Upgrading from 1.12.x to 1.13.0
 
 Version `1.13.0` adds `settings.idp_sso_service_binding` and `settings.idp_slo_service_binding`, and
 deprecates `settings.security[:embed_sign]`. If specified, new binding parameters will be used in place of `:embed_sign`

lib/ruby_saml.rb

@@ -20,5 +20,5 @@
 require 'ruby_saml/utils'
 require 'ruby_saml/version'
 
-# @deprecated This alias adds compatibility with v1.x and will be removed in v2.1.0
+# @deprecated This alias adds compatibility with v1.x and will be removed in v3.0.0
 OneLogin = Object

lib/ruby_saml/idp_metadata_parser.rb

@@ -161,7 +161,7 @@ def parse_to_array(idp_metadata, options = {})
     end
 
     def parse_to_idp_metadata_array(idp_metadata, options = {})
-      @document = Nokogiri::XML(idp_metadata)
+      @document = Nokogiri::XML(idp_metadata) # TODO: RubySaml::XML.safe_load_nokogiri
       @options = options
 
       idpsso_descriptors = self.class.get_idps(@document, options[:entity_id])
@@ -348,14 +348,14 @@ def certificates
           unless signing_nodes.empty?
             certs['signing'] = []
             signing_nodes.each do |cert_node|
-              certs['signing'] << cert_node.content
+              certs['signing'] << cert_node.text
             end
           end
 
           unless encryption_nodes.empty?
             certs['encryption'] = []
             encryption_nodes.each do |cert_node|
-              certs['encryption'] << cert_node.content
+              certs['encryption'] << cert_node.text
             end
           end
           certs

lib/ruby_saml/logoutresponse.rb

@@ -41,7 +41,7 @@ def initialize(response, settings = nil, options = {})
 
       @options = options
       @response = RubySaml::XML::Decoder.decode_message(response, @settings&.message_max_bytesize)
-      @document = RubySaml::XML::SignedDocument.new(@response)
+      @document = RubySaml::XML.safe_load_nokogiri(@response)
       super()
     end
 
@@ -60,47 +60,35 @@ def success?
     # @return [String|nil] Gets the InResponseTo attribute from the Logout Response if exists.
     #
     def in_response_to
-      @in_response_to ||= begin
-        node = REXML::XPath.first(
-          document,
-          "/p:LogoutResponse",
-          { "p" => RubySaml::XML::NS_PROTOCOL }
-        )
-        node.nil? ? nil : node.attributes['InResponseTo']
-      end
+      @in_response_to ||= document.at_xpath(
+        "/p:LogoutResponse",
+        { "p" => RubySaml::XML::NS_PROTOCOL }
+      )&.[]('InResponseTo')
     end
 
     # @return [String] Gets the Issuer from the Logout Response.
     #
     def issuer
-      @issuer ||= begin
-        node = REXML::XPath.first(
-          document,
-          "/p:LogoutResponse/a:Issuer",
-          { "p" => RubySaml::XML::NS_PROTOCOL, "a" => RubySaml::XML::NS_ASSERTION }
-        )
-        Utils.element_text(node)
-      end
+      @issuer ||= document.at_xpath(
+        "/p:LogoutResponse/a:Issuer",
+        { "p" => RubySaml::XML::NS_PROTOCOL, "a" => RubySaml::XML::NS_ASSERTION }
+      )&.text
     end
 
     # @return [String] Gets the StatusCode from a Logout Response.
     #
     def status_code
-      @status_code ||= begin
-        node = REXML::XPath.first(document, "/p:LogoutResponse/p:Status/p:StatusCode", { "p" => RubySaml::XML::NS_PROTOCOL })
-        node.nil? ? nil : node.attributes["Value"]
-      end
+      @status_code ||= document.at_xpath(
+        "/p:LogoutResponse/p:Status/p:StatusCode",
+        { "p" => RubySaml::XML::NS_PROTOCOL }
+      )&.[]('Value')
     end
 
     def status_message
-      @status_message ||= begin
-        node = REXML::XPath.first(
-          document,
-          "/p:LogoutResponse/p:Status/p:StatusMessage",
-          { "p" => RubySaml::XML::NS_PROTOCOL }
-        )
-        Utils.element_text(node)
-      end
+      @status_message ||= document.at_xpath(
+        "/p:LogoutResponse/p:Status/p:StatusMessage",
+        { "p" => RubySaml::XML::NS_PROTOCOL }
+      )&.text
     end
 
     # Aux function to validate the Logout Response

lib/ruby_saml/memoizable.rb

@@ -0,0 +1,66 @@
+# frozen_string_literal: true
+
+module RubySaml
+  # Mixin for memoizing methods
+  module Memoizable
+    # Creates a memoized method
+    #
+    # @param method_name [Symbol] the name of the method to memoize
+    # @param original_method [Symbol, nil] the original method to memoize (defaults to method_name)
+    def self.included(base)
+      base.extend(ClassMethods)
+    end
+
+    private
+
+    # Memoizes the result of a block using the given name as the cache key
+    #
+    # @param cache_key [Symbol, String] the name to use as the cache key
+    # @yield the block whose result will be cached
+    # @return [Object] the cached result or the result of the block
+    def memoize(cache_key)
+      cache_key = "@#{cache_key.to_s.delete_prefix('@')}"
+      return instance_variable_get(cache_key) if instance_variable_defined?(cache_key)
+
+      instance_variable_set(cache_key, yield)
+    end
+
+    # Class methods for memoization
+    module ClassMethods
+      # Defines multiple memoized methods
+      #
+      # @param method_names [Array<Symbol>] the names of the methods to memoize
+      # @raise [ArgumentError] if any method has an arity greater than 0
+      def memoize_method(*method_names)
+        method_names.each do |method_name|
+          method_obj = instance_method(method_name)
+
+          # Check method arity
+          if method_obj.arity > 0 # rubocop:disable Style/IfUnlessModifier
+            raise ArgumentError.new("Cannot memoize method '#{method_name}' with arity > 0")
+          end
+
+          # Store the original method
+          original_method_name = "#{method_name}_without_memoization"
+          alias_method original_method_name, method_name
+          private original_method_name
+
+          # Define the memoized version
+          define_method(method_name) do |&block|
+            cache_key = "@memoized_#{method_name}"
+            memoize(cache_key) do
+              send(original_method_name, &block)
+            end
+          end
+
+          # Preserve method visibility
+          if private_method_defined?(original_method_name)
+            private method_name
+          elsif protected_method_defined?(original_method_name)
+            protected method_name
+          end
+        end
+      end
+    end
+  end
+end