Merge pull request #744 from johnnyshields/v2.x-double-quote-xml

V2.x - Nokogiri Upgrade Part 1 - Always double quote XML

Author: pitbulk

README.md

@@ -585,8 +585,8 @@ to specify different certificates for each function.
 You may also globally set the SP signature and digest method, to be used in SP signing (functions 1 and 2 above):
 
 ```ruby
-settings.security[:digest_method]    = RubySaml::XML::Document::SHA1
-settings.security[:signature_method] = RubySaml::XML::Document::RSA_SHA1
+settings.security[:digest_method]    = RubySaml::XML::Crypto::SHA1
+settings.security[:signature_method] = RubySaml::XML::Crypto::RSA_SHA1
 ```
 
 #### Signing SP Metadata

UPGRADING.md

@@ -59,6 +59,18 @@ settings.security[:digest_method] = RubySaml::XML::Crypto::SHA1
 settings.security[:signature_method] = RubySaml::XML::Crypto::RSA_SHA1
 ```
 
+### Behavior change of double_quote_xml_attribute_values setting
+
+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.
+
+The reasons for this change are:
+- RubySaml will use Nokogiri instead of REXML to generate XML. Nokogiri does not support
+  generating XML with single quotes.
+- Double-quoted XML attributes are more commonly used than single quotes. There are no known
+  SAML clients which cannot support double-quoted XML.
+
 ### Removal of embed_sign setting
 
 The deprecated `settings.security[:embed_sign]` parameter has been removed. If you were using it, please instead switch

lib/ruby_saml/authrequest.rb

@@ -54,7 +54,7 @@ def create_params(settings, params={})
       end
 
       request_doc = create_authentication_xml_doc(settings)
-      request_doc.context[:attribute_quote] = :quote if settings.double_quote_xml_attribute_values
+      request_doc.context[:attribute_quote] = :quote
 
       request = +""
       request_doc.write(request)
@@ -100,6 +100,7 @@ def create_xml_document(settings)
       assign_uuid(settings)
 
       request_doc = RubySaml::XML::Document.new
+      request_doc.context[:attribute_quote] = :quote
       request_doc.uuid = uuid
 
       root = request_doc.add_element "samlp:AuthnRequest", { "xmlns:samlp" => "urn:oasis:names:tc:SAML:2.0:protocol", "xmlns:saml" => "urn:oasis:names:tc:SAML:2.0:assertion" }

lib/ruby_saml/logoutrequest.rb

@@ -52,7 +52,7 @@ def create_params(settings, params={})
       end
 
       request_doc = create_logout_request_xml_doc(settings)
-      request_doc.context[:attribute_quote] = :quote if settings.double_quote_xml_attribute_values
+      request_doc.context[:attribute_quote] = :quote
 
       request = +""
       request_doc.write(request)
@@ -98,6 +98,7 @@ def create_xml_document(settings)
       assign_uuid(settings)
 
       request_doc = RubySaml::XML::Document.new
+      request_doc.context[:attribute_quote] = :quote
       request_doc.uuid = uuid
 
       root = request_doc.add_element "samlp:LogoutRequest", { "xmlns:samlp" => "urn:oasis:names:tc:SAML:2.0:protocol", "xmlns:saml" => "urn:oasis:names:tc:SAML:2.0:assertion" }

lib/ruby_saml/metadata.rb

@@ -22,6 +22,7 @@ class Metadata
     #
     def generate(settings, pretty_print=false, valid_until=nil, cache_duration=nil)
       meta_doc = RubySaml::XML::Document.new
+      meta_doc.context[:attribute_quote] = :quote
       add_xml_declaration(meta_doc)
       root = add_root_element(meta_doc, settings, valid_until, cache_duration)
       sp_sso = add_sp_sso_element(root, settings)

lib/ruby_saml/settings.rb

@@ -54,7 +54,6 @@ def initialize(overrides = {}, keep_security_attributes = false)
     attr_accessor :name_identifier_value
     attr_accessor :name_identifier_value_requested
     attr_accessor :sessionindex
-    attr_accessor :double_quote_xml_attribute_values
     attr_accessor :message_max_bytesize
     attr_accessor :passive
     attr_reader   :protocol_binding
@@ -230,7 +229,6 @@ def get_binding(value)
       idp_cert_fingerprint_algorithm: RubySaml::XML::Crypto::SHA256,
       message_max_bytesize: 250_000,
       soft: true,
-      double_quote_xml_attribute_values: false,
       security: {
         authn_requests_signed: false,
         logout_requests_signed: false,
@@ -248,6 +246,22 @@ def get_binding(value)
       }.freeze
     }.freeze
 
+    {
+      double_quote_xml_attribute_values: true
+    }.each do |old_param, new_value|
+      # @deprecated Will be removed in v2.1.0
+      define_method(old_param) do
+        removed_deprecation(old_param, new_value)
+        new_value
+      end
+
+      # @deprecated Will be removed in v2.1.0
+      define_method(:"#{old_param}=") do |_|
+        removed_deprecation(old_param, new_value)
+        new_value
+      end
+    end
+
     {
       issuer: :sp_entity_id,
       idp_sso_target_url: :idp_sso_service_url,
@@ -355,6 +369,12 @@ def compress_response=(value)
 
     private
 
+    # @deprecated Will be removed in v2.1.0
+    def removed_deprecation(old_param, new_value)
+      Logging.deprecate "`RubySaml::Settings##{old_param}` is deprecated and will be removed in RubySaml 2.1.0. " \
+                        "It no longer has any effect, and will behave as if always set to #{new_value.inspect}."
+    end
+
     # @deprecated Will be removed in v2.1.0
     def replaced_deprecation(old_param, new_param)
       Logging.deprecate "`RubySaml::Settings##{old_param}` is deprecated and will be removed in RubySaml 2.1.0. " \

lib/ruby_saml/slo_logoutresponse.rb

@@ -60,7 +60,7 @@ def create_params(settings, request_id = nil, logout_message = nil, params = {},
       end
 
       response_doc = create_logout_response_xml_doc(settings, request_id, logout_message, logout_status_code)
-      response_doc.context[:attribute_quote] = :quote if settings.double_quote_xml_attribute_values
+      response_doc.context[:attribute_quote] = :quote
 
       response = +""
       response_doc.write(response)
@@ -109,6 +109,7 @@ def create_xml_document(settings, request_id = nil, logout_message = nil, status
       assign_uuid(settings)
 
       response_doc = RubySaml::XML::Document.new
+      response_doc.context[:attribute_quote] = :quote
       response_doc.uuid = uuid
 
       destination = settings.idp_slo_response_service_url || settings.idp_slo_service_url

test/authrequest_test.rb

@@ -36,7 +36,7 @@ class AuthrequestTest < Minitest::Test
       zstream.finish
       zstream.close
 
-      assert_match(/<samlp:AuthnRequest[^<]* Destination='http:\/\/example.com'/, inflated)
+      assert_match(/<samlp:AuthnRequest[^<]* Destination="http:\/\/example\.com"/, inflated)
     end
 
     it "create the SAMLRequest URL parameter without deflating" do
@@ -61,7 +61,7 @@ class AuthrequestTest < Minitest::Test
       zstream.finish
       zstream.close
 
-      assert_match(/<samlp:AuthnRequest[^<]* IsPassive='true'/, inflated)
+      assert_match(/<samlp:AuthnRequest[^<]* IsPassive="true"/, inflated)
     end
 
     it "create the SAMLRequest URL parameter with ProtocolBinding" do
@@ -76,7 +76,7 @@ class AuthrequestTest < Minitest::Test
       zstream.finish
       zstream.close
 
-      assert_match(/<samlp:AuthnRequest[^<]* ProtocolBinding='urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST'/, inflated)
+      assert_match(/<samlp:AuthnRequest[^<]* ProtocolBinding="urn:oasis:names:tc:SAML:2\.0:bindings:HTTP-POST"/, inflated)
     end
 
     it "create the SAMLRequest URL parameter with AttributeConsumingServiceIndex" do
@@ -90,7 +90,7 @@ class AuthrequestTest < Minitest::Test
       inflated = zstream.inflate(decoded)
       zstream.finish
       zstream.close
-      assert_match(/<samlp:AuthnRequest[^<]* AttributeConsumingServiceIndex='30'/, inflated)
+      assert_match(/<samlp:AuthnRequest[^<]* AttributeConsumingServiceIndex="30"/, inflated)
     end
 
     it "create the SAMLRequest URL parameter with ForceAuthn" do
@@ -104,7 +104,7 @@ class AuthrequestTest < Minitest::Test
       inflated = zstream.inflate(decoded)
       zstream.finish
       zstream.close
-      assert_match(/<samlp:AuthnRequest[^<]* ForceAuthn='true'/, inflated)
+      assert_match(/<samlp:AuthnRequest[^<]* ForceAuthn="true"/, inflated)
     end
 
     it "create the SAMLRequest URL parameter with NameID Format" do
@@ -118,8 +118,8 @@ class AuthrequestTest < Minitest::Test
       zstream.finish
       zstream.close
 
-      assert_match(/<samlp:NameIDPolicy[^<]* AllowCreate='true'/, inflated)
-      assert_match(/<samlp:NameIDPolicy[^<]* Format='urn:oasis:names:tc:SAML:2.0:nameid-format:transient'/, inflated)
+      assert_match(/<samlp:NameIDPolicy[^<]* AllowCreate="true"/, inflated)
+      assert_match(/<samlp:NameIDPolicy[^<]* Format="urn:oasis:names:tc:SAML:2\.0:nameid-format:transient"/, inflated)
     end
 
     it "create the SAMLRequest URL parameter with Subject" do
@@ -135,8 +135,8 @@ class AuthrequestTest < Minitest::Test
       zstream.close
 
       assert inflated.include?('<saml:Subject>')
-      assert inflated.include?("<saml:NameID Format='urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress'>testuser@example.com</saml:NameID>")
-      assert inflated.include?("<saml:SubjectConfirmation Method='urn:oasis:names:tc:SAML:2.0:cm:bearer'/>")
+      assert inflated.include?('<saml:NameID Format="urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress">testuser@example.com</saml:NameID>')
+      assert inflated.include?('<saml:SubjectConfirmation Method="urn:oasis:names:tc:SAML:2.0:cm:bearer"/>')
     end
 
     it "accept extra parameters" do
@@ -199,9 +199,9 @@ class AuthrequestTest < Minitest::Test
 
       it "can mutate the uuid" do
         request = RubySaml::Authrequest.new
-        request_id = request.request_id
         assert_nil request.uuid
-        assert_nil request_id
+        assert_nil request.request_id
+
         request.uuid = "new_uuid"
         assert_equal "new_uuid", request.uuid
         assert_equal request.uuid, request.request_id
@@ -225,7 +225,7 @@ class AuthrequestTest < Minitest::Test
       it "create the SAMLRequest parameter correctly" do
 
         auth_url = RubySaml::Authrequest.new.create(settings)
-        assert_match(/^http:\/\/example.com\?SAMLRequest/, auth_url)
+        assert_match(/^http:\/\/example\.com\?SAMLRequest/, auth_url)
       end
     end
 
@@ -234,7 +234,7 @@ class AuthrequestTest < Minitest::Test
         settings.idp_sso_service_url = "http://example.com?field=value"
 
         auth_url = RubySaml::Authrequest.new.create(settings)
-        assert_match(/^http:\/\/example.com\?field=value&SAMLRequest/, auth_url)
+        assert_match(/^http:\/\/example\.com\?field=value&SAMLRequest/, auth_url)
       end
     end
 
@@ -254,22 +254,22 @@ class AuthrequestTest < Minitest::Test
     it "create the saml:AuthnContextClassRef with comparison exact" do
       settings.authn_context = 'secure/name/password/uri'
       auth_doc = RubySaml::Authrequest.new.create_authentication_xml_doc(settings)
-      assert_match(/<samlp:RequestedAuthnContext[\S ]+Comparison='exact'/, auth_doc.to_s)
+      assert_match(/<samlp:RequestedAuthnContext[\S ]+Comparison="exact"/, auth_doc.to_s)
       assert_match(/<saml:AuthnContextClassRef>secure\/name\/password\/uri<\/saml:AuthnContextClassRef>/, auth_doc.to_s)
     end
 
     it "create the saml:AuthnContextClassRef with comparison minimun" do
       settings.authn_context = 'secure/name/password/uri'
       settings.authn_context_comparison = 'minimun'
       auth_doc = RubySaml::Authrequest.new.create_authentication_xml_doc(settings)
-      assert_match(/<samlp:RequestedAuthnContext[\S ]+Comparison='minimun'/, auth_doc.to_s)
+      assert_match(/<samlp:RequestedAuthnContext[\S ]+Comparison="minimun"/, auth_doc.to_s)
       assert_match(/<saml:AuthnContextClassRef>secure\/name\/password\/uri<\/saml:AuthnContextClassRef>/, auth_doc.to_s)
     end
 
     it "create the saml:AuthnContextDeclRef element correctly" do
       settings.authn_context_decl_ref = 'urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport'
       auth_doc = RubySaml::Authrequest.new.create_authentication_xml_doc(settings)
-      assert_match(/<saml:AuthnContextDeclRef>urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport<\/saml:AuthnContextDeclRef>/, auth_doc.to_s)
+      assert_match(/<saml:AuthnContextDeclRef>urn:oasis:names:tc:SAML:2\.0:ac:classes:PasswordProtectedTransport<\/saml:AuthnContextDeclRef>/, auth_doc.to_s)
     end
 
     it "create the saml:AuthnContextClassRef element correctly" do
@@ -281,22 +281,22 @@ class AuthrequestTest < Minitest::Test
     it "create the saml:AuthnContextClassRef with comparison exact" do
       settings.authn_context = 'secure/name/password/uri'
       auth_doc = RubySaml::Authrequest.new.create_authentication_xml_doc(settings)
-      assert auth_doc.to_s =~ /<samlp:RequestedAuthnContext[\S ]+Comparison='exact'/
+      assert auth_doc.to_s =~ /<samlp:RequestedAuthnContext[\S ]+Comparison="exact"/
       assert auth_doc.to_s =~ /<saml:AuthnContextClassRef>secure\/name\/password\/uri<\/saml:AuthnContextClassRef>/
     end
 
     it "create the saml:AuthnContextClassRef with comparison minimun" do
       settings.authn_context = 'secure/name/password/uri'
       settings.authn_context_comparison = 'minimun'
       auth_doc = RubySaml::Authrequest.new.create_authentication_xml_doc(settings)
-      assert auth_doc.to_s =~ /<samlp:RequestedAuthnContext[\S ]+Comparison='minimun'/
+      assert auth_doc.to_s =~ /<samlp:RequestedAuthnContext[\S ]+Comparison="minimun"/
       assert auth_doc.to_s =~ /<saml:AuthnContextClassRef>secure\/name\/password\/uri<\/saml:AuthnContextClassRef>/
     end
 
     it "create the saml:AuthnContextDeclRef element correctly" do
       settings.authn_context_decl_ref = 'urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport'
       auth_doc = RubySaml::Authrequest.new.create_authentication_xml_doc(settings)
-      assert auth_doc.to_s =~ /<saml:AuthnContextDeclRef>urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport<\/saml:AuthnContextDeclRef>/
+      assert auth_doc.to_s =~ /<saml:AuthnContextDeclRef>urn:oasis:names:tc:SAML:2\.0:ac:classes:PasswordProtectedTransport<\/saml:AuthnContextDeclRef>/
     end
 
     it "create multiple saml:AuthnContextDeclRef elements correctly " do
@@ -329,7 +329,7 @@ class AuthrequestTest < Minitest::Test
         unless sp_hash_algo == :sha256
           it 'using mixed signature and digest methods (signature SHA256)' do
             # RSA is ignored here; only the hash sp_key_algo is used
-            settings.security[:signature_method] = RubySaml::XML::Document::RSA_SHA256
+            settings.security[:signature_method] = RubySaml::XML::Crypto::RSA_SHA256
             params = RubySaml::Authrequest.new.create_params(settings)
             request_xml = Base64.decode64(params["SAMLRequest"])
 
@@ -339,7 +339,7 @@ class AuthrequestTest < Minitest::Test
           end
 
           it 'using mixed signature and digest methods (digest SHA256)' do
-            settings.security[:digest_method] = RubySaml::XML::Document::SHA256
+            settings.security[:digest_method] = RubySaml::XML::Crypto::SHA256
             params = RubySaml::Authrequest.new.create_params(settings)
             request_xml = Base64.decode64(params["SAMLRequest"])
 
@@ -428,7 +428,7 @@ class AuthrequestTest < Minitest::Test
         unless sp_hash_algo == :sha256
           it 'using mixed signature and digest methods (signature SHA256)' do
             # RSA is ignored here; only the hash sp_key_algo is used
-            settings.security[:signature_method] = RubySaml::XML::Document::RSA_SHA256
+            settings.security[:signature_method] = RubySaml::XML::Crypto::RSA_SHA256
             params = RubySaml::Authrequest.new.create_params(settings, :RelayState => 'http://example.com')
 
             assert params['SAMLRequest']
@@ -444,7 +444,7 @@ class AuthrequestTest < Minitest::Test
           end
 
           it 'using mixed signature and digest methods (digest SHA256)' do
-            settings.security[:digest_method] = RubySaml::XML::Document::SHA256
+            settings.security[:digest_method] = RubySaml::XML::Crypto::SHA256
             params = RubySaml::Authrequest.new.create_params(settings, :RelayState => 'http://example.com')
 
             assert params['SAMLRequest']
@@ -461,7 +461,7 @@ class AuthrequestTest < Minitest::Test
         end
 
         it "create a signature parameter using the first certificate and key" do
-          settings.security[:signature_method] = RubySaml::XML::Document::RSA_SHA1
+          settings.security[:signature_method] = RubySaml::XML::Crypto::RSA_SHA1
           settings.certificate = nil
           settings.private_key = nil
           cert, pkey = CertificateHelper.generate_pair(sp_key_algo)

test/idp_metadata_parser_test.rb

@@ -157,27 +157,27 @@ def initialize; end
       settings = idp_metadata_parser.parse(idp_metadata, {
         :settings => {
           :security => {
-            :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
           }
         }
       })
       assert_equal "C4:C6:BD:41:EC:AD:57:97:CE:7B:7D:80:06:C3:E4:30:53:29:02:0B:DD:2D:47:02:9E:BD:85:AD:93:02:45:21", settings.idp_cert_fingerprint
-      assert_equal RubySaml::XML::Document::SHA256, settings.get_sp_digest_method
-      assert_equal RubySaml::XML::Document::RSA_SHA256, settings.get_sp_signature_method
+      assert_equal RubySaml::XML::Crypto::SHA256, settings.get_sp_digest_method
+      assert_equal RubySaml::XML::Crypto::RSA_SHA256, settings.get_sp_signature_method
     end
 
     it "merges results into given settings object" do
       settings = RubySaml::Settings.new(:security => {
-        :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
       })
 
       RubySaml::IdpMetadataParser.new.parse(idp_metadata_descriptor, :settings => settings)
 
       assert_equal "C4:C6:BD:41:EC:AD:57:97:CE:7B:7D:80:06:C3:E4:30:53:29:02:0B:DD:2D:47:02:9E:BD:85:AD:93:02:45:21", settings.idp_cert_fingerprint
-      assert_equal RubySaml::XML::Document::SHA256, settings.get_sp_digest_method
-      assert_equal RubySaml::XML::Document::RSA_SHA256, settings.get_sp_signature_method
+      assert_equal RubySaml::XML::Crypto::SHA256, settings.get_sp_digest_method
+      assert_equal RubySaml::XML::Crypto::RSA_SHA256, settings.get_sp_signature_method
     end
   end
 
@@ -256,8 +256,8 @@ def initialize; end
       parsed_metadata = idp_metadata_parser.parse_to_hash(idp_metadata, {
         :settings => {
           :security => {
-            :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
           }
         }
       })