Small clean-ups for v2.x release

Author: johnnyshields

lib/ruby_saml/idp_metadata_parser.rb

@@ -39,7 +39,7 @@ module Vocabulary
     attr_reader :options
 
     # fetch IdP descriptors from a metadata document
-    def self.get_idps(metadata_document, only_entity_id=nil)
+    def self.get_idps(metadata_document, only_entity_id = nil)
       path = "//md:EntityDescriptor#{"[@entityID=\"#{only_entity_id}\"]" if only_entity_id}/md:IDPSSODescriptor"
       REXML::XPath.match(
         metadata_document,
@@ -190,7 +190,7 @@ def parse_to_idp_metadata_array(idp_metadata, options = {})
         raise ArgumentError.new("idp_metadata must contain an IDPSSODescriptor element")
       end
 
-      idpsso_descriptors.map {|id| IdpMetadata.new(id, id.parent.attributes["entityID"])}
+      idpsso_descriptors.map { |id| IdpMetadata.new(id, id.parent.attributes["entityID"]) }
     end
 
     # Retrieve the remote IdP metadata from the URL or a cached copy.
@@ -205,6 +205,7 @@ def parse_to_idp_metadata_array(idp_metadata, options = {})
     def get_idp_metadata(url, validate_cert, options = {})
       uri = URI.parse(url)
       raise ArgumentError.new("url must begin with http or https") unless /^https?/.match?(uri.scheme)
+
       http = Net::HTTP.new(uri.host, uri.port)
 
       if uri.scheme == "https"
@@ -218,13 +219,12 @@ def get_idp_metadata(url, validate_cert, options = {})
       http.max_retries = options[:max_retries] if options[:max_retries]
 
       get = Net::HTTP::Get.new(uri.request_uri)
-      get.basic_auth uri.user, uri.password if uri.user
+      get.basic_auth(uri.user, uri.password) if uri.user
+
       @response = http.request(get)
-      return response.body if response.is_a? Net::HTTPSuccess
+      return response.body if response.is_a?(Net::HTTPSuccess)
 
-      raise RubySaml::HttpError.new(
-        "Failed to fetch idp metadata: #{response.code}: #{response.message}"
-      )
+      raise RubySaml::HttpError.new("Failed to fetch idp metadata: #{response.code}: #{response.message}")
     end
 
     private
@@ -240,6 +240,7 @@ def initialize(idpsso_descriptor, entity_id)
       def to_hash(options = {})
         sso_binding = options[:sso_binding]
         slo_binding = options[:slo_binding]
+
         {
           idp_entity_id: @entity_id,
           name_identifier_format: idp_name_id_format(options[:name_id_format]),
@@ -392,15 +393,13 @@ def certificates
 
       # @return [String|nil] the fingerpint of the X509Certificate if it exists
       #
-      def fingerprint(certificate, fingerprint_algorithm = RubySaml::XML::Document::SHA256)
-        @fingerprint ||= begin
-          return unless certificate
+      def fingerprint(certificate, fingerprint_algorithm = RubySaml::XML::Crypto::SHA256)
+        return unless certificate
 
-          cert = OpenSSL::X509::Certificate.new(Base64.decode64(certificate))
+        cert = OpenSSL::X509::Certificate.new(Base64.decode64(certificate))
 
-          fingerprint_alg = RubySaml::XML::Crypto.hash_algorithm(fingerprint_algorithm).new
-          fingerprint_alg.hexdigest(cert.to_der).upcase.scan(/../).join(":")
-        end
+        fingerprint_alg = RubySaml::XML::Crypto.hash_algorithm(fingerprint_algorithm).new
+        fingerprint_alg.hexdigest(cert.to_der).upcase.scan(/../).join(":")
       end
 
       # @return [Array] the names of all SAML attributes if any exist

lib/ruby_saml/logoutresponse.rb

@@ -271,6 +271,5 @@ def validate_signature
       end
       true
     end
-
   end
 end

lib/ruby_saml/metadata.rb

@@ -40,7 +40,7 @@ def add_xml_declaration(meta_doc)
 
     def add_root_element(meta_doc, settings, valid_until, cache_duration)
       namespaces = {
-          "xmlns:md" => "urn:oasis:names:tc:SAML:2.0:metadata"
+        "xmlns:md" => "urn:oasis:names:tc:SAML:2.0:metadata"
       }
 
       if settings.attribute_consuming_service.configured?

lib/ruby_saml/response.rb

@@ -2,7 +2,6 @@
 
 require "ruby_saml/xml"
 require "ruby_saml/attributes"
-
 require "time"
 require "nokogiri"
 
@@ -166,16 +165,16 @@ def attributes
               raise ValidationError.new("Found an Attribute element with duplicated Name")
             end
 
-            values = node.elements.collect  do |e|
+            values = node.elements.map do |e|
               if e.elements.nil? || e.elements.empty?
                 # SAMLCore requires that nil AttributeValues MUST contain xsi:nil XML attribute set to "true" or "1"
                 # otherwise the value is to be regarded as empty.
                 %w[true 1].include?(e.attributes['xsi:nil']) ? nil : Utils.element_text(e)
-              # explicitly support saml2:NameID with saml2:NameQualifier if supplied in attributes
-              # this is useful for allowing eduPersonTargetedId to be passed as an opaque identifier to use to
-              # identify the subject in an SP rather than email or other less opaque attributes
-              # NameQualifier, if present is prefixed with a "/" to the value
               else
+                # Explicitly support saml2:NameID with saml2:NameQualifier if supplied in attributes
+                # this is useful for allowing eduPersonTargetedId to be passed as an opaque identifier to use to
+                # identify the subject in an SP rather than email or other less opaque attributes
+                # NameQualifier, if present is prefixed with a "/" to the value
                 REXML::XPath.match(e,'a:NameID', { "a" => ASSERTION }).collect do |n|
                   base_path = n.attributes['NameQualifier'] ? "#{n.attributes['NameQualifier']}/" : ''
                   "#{base_path}#{Utils.element_text(n)}"
@@ -186,6 +185,7 @@ def attributes
             attributes.add(name, values.flatten)
           end
         end
+
         attributes
       end
     end
@@ -594,7 +594,7 @@ def validate_signed_elements
         signed_elements << signed_element
       end
 
-      unless signature_nodes.length < 3 && !signed_elements.empty?
+      unless signature_nodes.size < 3 && !signed_elements.empty?
         return append_error("Found an unexpected number of Signature Element. SAML Response rejected")
       end
 
@@ -619,10 +619,10 @@ def validate_in_response_to
       append_error(error_msg)
     end
 
-    # Validates the Audience, (If the Audience match the Service Provider EntityID)
+    # Validates the Audience, (If the Audience matches the Service Provider EntityID)
     # If the response was initialized with the :skip_audience option, this validation is skipped,
     # If fails, the error is added to the errors array
-    # @return [Boolean] True if there is an Audience Element that match the Service Provider EntityID, otherwise False if soft=True
+    # @return [Boolean] True if there is an Audience Element that matches the Service Provider EntityID, otherwise False if soft=True
     # @raise [ValidationError] if soft == false and validation fails
     #
     def validate_audience
@@ -726,7 +726,7 @@ def validate_conditions
 
     # Validates the Issuer (Of the SAML Response and the SAML Assertion)
     # @param soft [Boolean] soft Enable or Disable the soft mode (In order to raise exceptions when the response is invalid or not)
-    # @return [Boolean] True if the Issuer matchs the IdP entityId, otherwise False if soft=True
+    # @return [Boolean] True if the Issuer matches the IdP entityId, otherwise False if soft=True
     # @raise [ValidationError] if soft == false and validation fails
     #
     def validate_issuer
@@ -863,9 +863,9 @@ def validate_signature
 
       if sig_elements.size != 1
         if sig_elements.empty?
-           append_error("Signed element id ##{doc.signed_element_id} is not found")
+          append_error("Signed element id ##{doc.signed_element_id} is not found")
         else
-           append_error("Signed element id ##{doc.signed_element_id} is found more than once")
+          append_error("Signed element id ##{doc.signed_element_id} is found more than once")
         end
         return append_error(error_msg)
       end
@@ -874,16 +874,16 @@ def validate_signature
 
       idp_certs = settings.get_idp_cert_multi
       if idp_certs.nil? || idp_certs[:signing].empty?
-        opts = {}
-        opts[:fingerprint_alg] = settings.idp_cert_fingerprint_algorithm
         idp_cert = settings.get_idp_cert
         fingerprint = settings.get_fingerprint
-        opts[:cert] = idp_cert
+        opts = {
+          cert: idp_cert,
+          fingerprint_alg: settings.idp_cert_fingerprint_algorithm
+        }
 
         if fingerprint && doc.validate_document(fingerprint, @soft, opts)
           if settings.security[:check_idp_cert_expiration] && RubySaml::Utils.is_cert_expired(idp_cert)
-            error_msg = "IdP x509 certificate expired"
-            return append_error(error_msg)
+            return append_error("IdP x509 certificate expired")
           end
         else
           return append_error(error_msg)
@@ -894,18 +894,20 @@ def validate_signature
         idp_certs[:signing].each do |idp_cert|
           valid = doc.validate_document_with_cert(idp_cert, true)
           next unless valid
+
           if settings.security[:check_idp_cert_expiration] && RubySaml::Utils.is_cert_expired(idp_cert)
-              expired = true
+            expired = true
           end
 
           # At least one certificate is valid, restore the old accumulated errors
           @errors = old_errors
           break
         end
+
         if expired
-          error_msg = "IdP x509 certificate expired"
-          return append_error(error_msg)
+          return append_error("IdP x509 certificate expired")
         end
+
         unless valid
           # Remove duplicated errors
           @errors = @errors.uniq
@@ -933,7 +935,7 @@ def name_id_node
     # @param subelt [String] The XPath pattern
     # @return [REXML::Element | nil] If any matches, return the Element
     #
-    def xpath_first_from_signed_assertion(subelt=nil)
+    def xpath_first_from_signed_assertion(subelt = nil)
       doc = decrypted_document.nil? ? document : decrypted_document
       node = REXML::XPath.first(
           doc,
@@ -1035,14 +1037,14 @@ def decrypt_attribute(encrypted_attribute_node)
     #
     def decrypt_element(encrypt_node, regexp)
       if settings.nil? || settings.get_sp_decryption_keys.empty?
-        raise ValidationError.new('An ' + encrypt_node.name + ' found and no SP private key found on the settings to decrypt it')
+        raise ValidationError.new("An #{encrypt_node.name} found and no SP private key found on the settings to decrypt it.")
       end
 
-      if encrypt_node.name == 'EncryptedAttribute'
-        node_header = '<node xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">'
-      else
-        node_header = '<node xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion">'
-      end
+      node_header = if encrypt_node.name == 'EncryptedAttribute'
+                      '<node xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">'
+                    else
+                      '<node xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion">'
+                    end
 
       elem_plaintext = RubySaml::Utils.decrypt_multi(encrypt_node, settings.get_sp_decryption_keys)
 
@@ -1063,8 +1065,9 @@ def decrypt_element(encrypt_node, regexp)
     # @return [Time|nil] The parsed value
     #
     def parse_time(node, attribute)
-      return unless node && node.attributes[attribute]
-      Time.parse(node.attributes[attribute])
+      return unless (value = node&.attributes&.[](attribute))
+
+      Time.parse(value)
     end
   end
 end

lib/ruby_saml/saml_message.rb

@@ -19,7 +19,7 @@ class SamlMessage
     ASSERTION = "urn:oasis:names:tc:SAML:2.0:assertion"
     PROTOCOL  = "urn:oasis:names:tc:SAML:2.0:protocol"
 
-    BASE64_FORMAT = %r(\A([A-Za-z0-9+/]{4})*([A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?\Z)
+    BASE64_FORMAT = %r{\A([A-Za-z0-9+/]{4})*([A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?\Z}
     @@mutex = Mutex.new
 
     # @return [Nokogiri::XML::Schema] Gets the schema object of the SAML 2.0 Protocol schema
@@ -74,10 +74,12 @@ def valid_saml?(document, soft = true)
         raise ValidationError.new("XML load failed: #{error.message}")
       end
 
-      SamlMessage.schema.validate(xml).map do |schema_error|
+      SamlMessage.schema.validate(xml).each do |schema_error|
         return false if soft
         raise ValidationError.new("#{schema_error.message}\n\n#{xml}")
       end
+
+      true
     end
 
     private
@@ -89,9 +91,9 @@ def valid_saml?(document, soft = true)
     def decode_raw_saml(saml, settings = nil)
       return saml unless base64_encoded?(saml)
 
-      settings = RubySaml::Settings.new if settings.nil?
+      settings ||= RubySaml::Settings.new
       if saml.bytesize > settings.message_max_bytesize
-        raise ValidationError.new("Encoded SAML Message exceeds " + settings.message_max_bytesize.to_s + " bytes, so was rejected")
+        raise ValidationError.new("Encoded SAML Message exceeds #{settings.message_max_bytesize} bytes, so was rejected")
       end
 
       decoded = decode(saml)
@@ -157,7 +159,7 @@ def inflate(deflated)
     # @return [String] The deflated string
     #
     def deflate(inflated)
-      Zlib::Deflate.deflate(inflated, 9)[2..-5]
+      Zlib::Deflate.deflate(inflated, Zlib::BEST_COMPRESSION)[2..-5]
     end
   end
 end

lib/ruby_saml/settings.rb

@@ -226,7 +226,7 @@ def get_binding(value)
     DEFAULTS = {
       assertion_consumer_service_binding: Utils::BINDINGS[:post],
       single_logout_service_binding: Utils::BINDINGS[:redirect],
-      idp_cert_fingerprint_algorithm: RubySaml::XML::Document::SHA256,
+      idp_cert_fingerprint_algorithm: RubySaml::XML::Crypto::SHA256,
       message_max_bytesize: 250_000,
       soft: true,
       double_quote_xml_attribute_values: false,
@@ -238,8 +238,8 @@ def get_binding(value)
         want_assertions_encrypted: false,
         want_name_id: false,
         metadata_signed: false,
-        digest_method: RubySaml::XML::Document::SHA256,
-        signature_method: RubySaml::XML::Document::RSA_SHA256,
+        digest_method: RubySaml::XML::Crypto::SHA256,
+        signature_method: RubySaml::XML::Crypto::RSA_SHA256,
         check_idp_cert_expiration: false,
         check_sp_cert_expiration: false,
         strict_audience_validation: false,

lib/ruby_saml/slo_logoutrequest.rb

@@ -95,12 +95,12 @@ def name_id_node
     # @return [REXML::Document] The decrypted EncrypedtID element
     #
     def decrypt_nameid(encrypted_id_node)
-
       if settings.nil? || settings.get_sp_decryption_keys.empty?
-        raise ValidationError.new('An ' + encrypted_id_node.name + ' found and no SP private key found on the settings to decrypt it')
+        raise ValidationError.new("An #{encrypted_id_node.name} found and no SP private key found on the settings to decrypt it")
       end
 
       elem_plaintext = RubySaml::Utils.decrypt_multi(encrypted_id_node, settings.get_sp_decryption_keys)
+
       # If we get some problematic noise in the plaintext after decrypting.
       # This quick regexp parse will grab only the Element and discard the noise.
       elem_plaintext = elem_plaintext.match(/(.*<\/(\w+:)?NameID>)/m)[0]
@@ -135,16 +135,17 @@ def issuer
     # @return [Time|nil] Gets the NotOnOrAfter Attribute value if exists.
     #
     def not_on_or_after
-      @not_on_or_after ||= begin
-        node = REXML::XPath.first(
-          document,
-          "/p:LogoutRequest",
-          { "p" => PROTOCOL }
-        )
-        if node && node.attributes["NotOnOrAfter"]
-          Time.parse(node.attributes["NotOnOrAfter"])
-        end
-      end
+      return @not_on_or_after if defined?(@not_on_or_after)
+
+      node = REXML::XPath.first(
+        document,
+        "/p:LogoutRequest",
+        { "p" => PROTOCOL }
+      )
+
+      @not_on_or_after = if (value = node&.attributes&.[]("NotOnOrAfter"))
+                           Time.parse(value)
+                         end
     end
 
     # @return [Array] Gets the SessionIndex if exists (Supported multiple values). Empty Array if none found