Merge remote-tracking branch 'remotes/johnnyshields/v2.x-double-quote-xml' into v2.x-partial-nokogiri-upgrade

Author: johnnyshields

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)

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)

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)

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
@@ -165,8 +165,8 @@ class AuthrequestTest < Minitest::Test
       it "uuid is initialized to nil" do
         request = RubySaml::Authrequest.new
 
-        assert_nil(request.uuid)
-        assert_equal request.request_id, request.uuid
+        assert_nil request.uuid
+        assert_nil request.request_id
       end
 
       it "creates request with ID prefixed with default '_'" do
@@ -199,8 +199,9 @@ class AuthrequestTest < Minitest::Test
 
       it "can mutate the uuid" do
         request = RubySaml::Authrequest.new
-        request_id = request.request_id
-        assert_equal request_id, request.uuid
+        assert_nil request.uuid
+        assert_nil request.request_id
+
         request.uuid = "new_uuid"
         assert_equal "new_uuid", request.uuid
         assert_equal request.uuid, request.request_id
@@ -224,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
 
@@ -233,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
 
@@ -253,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
@@ -280,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

test/logoutrequest_test.rb

@@ -78,7 +78,7 @@ class RequestTest < Minitest::Test
         settings.idp_slo_service_url = "http://example.com?field=value"
 
         unauth_url = RubySaml::Logoutrequest.new.create(settings)
-        assert_match(/^http:\/\/example.com\?field=value&SAMLRequest/, unauth_url)
+        assert_match(/^http:\/\/example\.com\?field=value&SAMLRequest/, unauth_url)
       end
     end
 
@@ -90,16 +90,16 @@ class RequestTest < Minitest::Test
         unauth_url = unauth_req.create(settings)
 
         inflated = decode_saml_request_payload(unauth_url)
-        assert_match %r[ID='#{unauth_req.uuid}'], inflated
+        assert_match %r[ID="#{unauth_req.uuid}"], inflated
       end
     end
 
     describe "uuid" do
       it "uuid is initialized to nil" do
         request = RubySaml::Logoutrequest.new
 
-        assert_nil(request.uuid)
-        assert_equal request.request_id, request.uuid
+        assert_nil request.uuid
+        assert_nil request.request_id
       end
 
       it "creates request with ID prefixed with default '_'" do
@@ -132,8 +132,9 @@ class RequestTest < Minitest::Test
 
       it "can mutate the uuid" do
         request = RubySaml::Logoutrequest.new
-        request_id = request.request_id
-        assert_equal request_id, request.uuid
+        assert_nil request.uuid
+        assert_nil request.request_id
+
         request.uuid = "new_uuid"
         assert_equal "new_uuid", request.uuid
         assert_equal request.uuid, request.request_id

test/settings_test.rb

@@ -16,8 +16,7 @@ class SettingsTest < Minitest::Test
         :idp_cert, :idp_cert_fingerprint, :idp_cert_fingerprint_algorithm, :idp_cert_multi,
         :idp_attribute_names, :issuer, :assertion_consumer_service_url, :single_logout_service_url,
         :sp_name_qualifier, :name_identifier_format, :name_identifier_value, :name_identifier_value_requested,
-        :sessionindex, :attributes_index, :passive, :force_authn,
-        :double_quote_xml_attribute_values, :message_max_bytesize,
+        :sessionindex, :attributes_index, :passive, :force_authn, :message_max_bytesize,
         :security, :certificate, :private_key, :certificate_new, :sp_cert_multi,
         :authn_context, :authn_context_comparison, :authn_context_decl_ref,
         :assertion_consumer_logout_service_url
@@ -556,7 +555,6 @@ class SettingsTest < Minitest::Test
         assert_equal expected_encryption, actual[:encryption].map { |ary| ary.map(&:to_pem) }
       end
 
-
       it 'handles OpenSSL::PKey::PKey objects for single case' do
         @settings.certificate = cert_text1
         @settings.private_key = OpenSSL::PKey::RSA.new(key_text1)

test/slo_logoutresponse_test.rb

@@ -54,7 +54,7 @@ class SloLogoutresponseTest < Minitest::Test
       unauth_url = RubySaml::SloLogoutresponse.new.create(settings, logout_request.id)
 
       inflated = decode_saml_response_payload(unauth_url)
-      assert_match(/InResponseTo='_c0348950-935b-0131-1060-782bcb56fcaa'/, inflated)
+      assert_match(/InResponseTo="_c0348950-935b-0131-1060-782bcb56fcaa"/, inflated)
     end
 
     it "set a custom successful logout message on the response" do
@@ -69,7 +69,7 @@ class SloLogoutresponseTest < Minitest::Test
 
       inflated = decode_saml_response_payload(unauth_url)
       assert_match(/<samlp:StatusMessage>Custom Logout Message<\/samlp:StatusMessage>/, inflated)
-      assert_match(/<samlp:StatusCode Value='urn:oasis:names:tc:SAML:2.0:status:PartialLogout/, inflated)
+      assert_match(/<samlp:StatusCode Value="urn:oasis:names:tc:SAML:2\.0:status:PartialLogout/, inflated)
     end
 
     it "uses the response location when set" do
@@ -79,15 +79,15 @@ class SloLogoutresponseTest < Minitest::Test
       assert_match(/^http:\/\/unauth\.com\/logout\/return\?SAMLResponse=/, unauth_url)
 
       inflated = decode_saml_response_payload(unauth_url)
-      assert_match(/Destination='http:\/\/unauth.com\/logout\/return'/, inflated)
+      assert_match(/Destination="http:\/\/unauth\.com\/logout\/return"/, inflated)
     end
 
     describe "uuid" do
       it "uuid is initialized to nil" do
         response = RubySaml::SloLogoutresponse.new
 
-        assert_nil(response.uuid)
-        assert_equal response.response_id, response.uuid
+        assert_nil response.uuid
+        assert_nil response.response_id
       end
 
       it "creates response with ID prefixed with default '_'" do
@@ -120,8 +120,9 @@ class SloLogoutresponseTest < Minitest::Test
 
       it "can mutate the uuid" do
         response = RubySaml::SloLogoutresponse.new
-        response_id = response.response_id
-        assert_equal response_id, response.uuid
+        assert_nil response.uuid
+        assert_nil response.response_id
+
         response.uuid = "new_uuid"
         assert_equal "new_uuid", response.uuid
         assert_equal response.uuid, response.response_id