From aa489cdc2082e7f896983f1879bb44d89ba85d77 Mon Sep 17 00:00:00 2001 From: Kevin Pratt Date: Wed, 28 Feb 2018 09:49:02 -0700 Subject: [PATCH 1/3] Self closing tags returning "" instead of nil It was discovered that self closing tags return "" now instead of nil. Discussed the possible fixes on: https://github.com/onelogin/ruby-saml/issues/443 --- lib/onelogin/ruby-saml/utils.rb | 2 +- test/response_test.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/onelogin/ruby-saml/utils.rb b/lib/onelogin/ruby-saml/utils.rb index a135d697e..450ad9154 100644 --- a/lib/onelogin/ruby-saml/utils.rb +++ b/lib/onelogin/ruby-saml/utils.rb @@ -286,7 +286,7 @@ def self.original_uri_match?(destination_url, settings_url) # that there all children other than text nodes can be ignored (e.g. comments). If nil is # passed, nil will be returned. def self.element_text(element) - element.texts.join if element + element.texts.join if element && !element.texts.empty? end end end diff --git a/test/response_test.rb b/test/response_test.rb index 2b3121d4c..3b5ba16a0 100644 --- a/test/response_test.rb +++ b/test/response_test.rb @@ -1129,12 +1129,12 @@ class RubySamlTest < Minitest::Test it "return multiple values including nil and empty string" do response = OneLogin::RubySaml::Response.new(fixture(:response_with_multiple_attribute_values)) - assert_equal ["", "valuePresent", nil, nil], response.attributes.multi(:attribute_with_nils_and_empty_strings) + assert_equal [nil, "valuePresent", nil, nil], response.attributes.multi(:attribute_with_nils_and_empty_strings) end it "return multiple values from [] when not in compatibility mode off" do OneLogin::RubySaml::Attributes.single_value_compatibility = false - assert_equal ["", "valuePresent", nil, nil], response_multiple_attr_values.attributes[:attribute_with_nils_and_empty_strings] + assert_equal [nil, "valuePresent", nil, nil], response_multiple_attr_values.attributes[:attribute_with_nils_and_empty_strings] OneLogin::RubySaml::Attributes.single_value_compatibility = true end From 9400c453b20a4e82dda8e812aa549fce8c56ae96 Mon Sep 17 00:00:00 2001 From: Kevin Pratt Date: Wed, 28 Feb 2018 10:33:53 -0700 Subject: [PATCH 2/3] Test for self closed audience tag After discussion on: https://github.com/onelogin/ruby-saml/issues/443 We determined that it was related to the way that the text is being returned and checked. In my previous commit I applied the change discussed in the issue above. This commit adds a new response and test that fails before the and passes once the change is added. I used a SAML tool to produce a new response that has the unexpected selfclosing tag and included it in the test/responses folder. --- test/response_test.rb | 8 ++++++++ .../response_audience_self_closed_tag.xml.base64 | 1 + 2 files changed, 9 insertions(+) create mode 100644 test/responses/response_audience_self_closed_tag.xml.base64 diff --git a/test/response_test.rb b/test/response_test.rb index 3b5ba16a0..dbb78e890 100644 --- a/test/response_test.rb +++ b/test/response_test.rb @@ -34,6 +34,7 @@ class RubySamlTest < Minitest::Test let(:response_encrypted_attrs) { OneLogin::RubySaml::Response.new(response_document_encrypted_attrs) } let(:response_no_signed_elements) { OneLogin::RubySaml::Response.new(read_invalid_response("no_signature.xml.base64")) } let(:response_multiple_signed) { OneLogin::RubySaml::Response.new(read_invalid_response("multiple_signed.xml.base64")) } + let(:response_audience_self_closed) { OneLogin::RubySaml::Response.new(read_response("response_audience_self_closed_tag.xml.base64")) } let(:response_invalid_audience) { OneLogin::RubySaml::Response.new(read_invalid_response("invalid_audience.xml.base64")) } let(:response_invalid_signed_element) { OneLogin::RubySaml::Response.new(read_invalid_response("response_invalid_signed_element.xml.base64")) } let(:response_invalid_issuer_assertion) { OneLogin::RubySaml::Response.new(read_invalid_response("invalid_issuer_assertion.xml.base64")) } @@ -428,6 +429,13 @@ class RubySamlTest < Minitest::Test assert_empty response.errors end + it "return true when the audience is self closing" do + response_audience_self_closed.settings = settings + response_audience_self_closed.settings.issuer = '{audience}' + assert response_audience_self_closed.send(:validate_audience) + assert_empty response_audience_self_closed.errors + end + it "return false when the audience is valid" do response.settings = settings response.settings.issuer = 'invalid_audience' diff --git a/test/responses/response_audience_self_closed_tag.xml.base64 b/test/responses/response_audience_self_closed_tag.xml.base64 new file mode 100644 index 000000000..1bac221e3 --- /dev/null +++ b/test/responses/response_audience_self_closed_tag.xml.base64 @@ -0,0 +1 @@ +tVdbc7JIE77fqv0Plu9lynBUhEpSi6DGAwqCp9xscRgQ5SQDgvz6b8BDTEyy+27Vd6U03T3P8/T09PAEdd+LuBmAURhAUMt9L4BcZXyup3HAhTp0IRfoPoBcYnIqL4058hHnojhMQjP06jchP0foEII4ccOgXhuIz/XIzi3atHSGajVaeoto4CzVbjC4CRpUG2+DtsnaTQvUawsQQxT1XEdJUCiEKRgEMNGDBJlwAgW2GgSjETTXpDmCfqvXRAATN9CTKmqTJBGHYTBJbfvRDH0MBFYUukECb/7pJnyMNhFKH1yk0MLn+t9NxjANm8EbjEHYDZwgQcNsk0SDaZPoBUEZBtWuv/z5R632VArAVejil/OarhU9glz3Iw9UKz9ht05PFuRU10E40/givAWvgLMse8yoxzB2MBLHcQxnMeRjQdf5dV7xHA+sQWCHVTpBD8LANXXPLSr2Ekg2oVXjPSeM3WTjf5OcwAi8TN4AudkwCTr4VceqJd4XqUD+y3QfsMZQb8CNTpwzlvlmwAYxCExQm88Gz/Vf/3YnVBS1WA+gHcY+/Pj4e6hAcABeGAGrAS/kEMDfS/ilak/YPUbRddCG/C/inYV7T7LQvRS8QCZfazNsO57s11lOE4HdabnxA0ilwXMF4Na5MlwlPz1+2jbXAp8iNLu1c/qddqyZCbbM4dzx5mE4XmbEdtjVZcPrFSvzQY7CjjOcGTE207diRyX94pgdX2eOHUz3vmU/OMkILBXYksJk7so7fGPsRvJq6NjOzmHhwc4cbCVPx1NsnB6T42D+Ng2yV2X3uqaWbkSHKq7mUbpz1ON8uysYeZhNV7sMBjGlHtR0XiSENMefr3Ru8P/5R0lqBI5Xgqsmzop6ol8fhPIkslGnJOBFGgyEfiEI/FoRBEWYTExlFfoUuRD5ScfZ7Tc7t89meIdX5j0eEZUUmAnKWlwoSr+bDRfzojuW+F2fJ+ZdoSMJ6pzOexqvdZzJosObmtj10re+tzH8XmaQeWQW3ZnEt0/+G2mgkU1P67OBvqRzUeRHpzio8fgifSt40MvwfCLyuLSV8qmo5JLY0yub9tEm9Qa5UPDDU/xa472FdoN1gLAOtW13IXWkau1OLknqsrnTl710veocjL5XSLNuJmaV/6ibbWTDX0gGaUXGtitJfHjCjFabLZuBNOjbEo/3BXXfVwcGJSrdUiOep/sTXhQ6roKoKGLY8h+Ut8NawfIx3vXGThqNCEU012PabsomKunEUdloflgwdqHthdddW9uMc21Hk5JCSuYQTsF8uJAZsj3aHP1tT9jnjkwv0my2Y+OI9/AHf3NsSbJ13B75Vp+gt7G4VFvNIxybmxGdYIcs6ooFripeN+wTO6nwPNVnijeVWsXA4RlxO9Hna2WfDURe4TufOXVOnDr8WCIPfcFVsIP/oLcONP6wWJGFtX/lSQUjDj3CNZRi2KSloTBdLA/w4aCs7LfexvLxqSyRg1icM+H+MJL3ciuf8mA4ar3iR5mR6PWYsvVEPQzWvm8fVwK7S9AuL7DJMX4t5M08GOJHgMG5usqDQskz08dYOeP3saLzlBKhpmkyR6OHTcP4KIMHclO1xeeNfjWeWgG7bZIPbfQ+0CJOTZAJXqbBrU0ILVCrOu7nuQ8rb05NTRNAeJkD2F360wTlL7eE80TMofvt+buSxqq5Ab7ecKsLgQnq16h/DrpeQ5g2TbAsgQ5xhrIbNE41G4AEaNITaAbhgLAoQP+Ha8itZr91MbiNU1NjC8zkbLtYJ0jkgVhT5fKPkqJ5b7sgRpSB54Vlxnqth8aPnnxfGeKRqCyu1bArVw5J4nq8ZcVllV5g6IMwAH/dwDyjPC3+CdEZpxAGtltmKwt4mng/bw7T5wygxyCuXxP+kLLcuLVJmEyDaczbSUmZxEn8Rnr2dAOcAdONXFDW5v9//8PetcC+Q34pKvZFVU90kbfllq6wZNgBqCqg2lm39CiOLundSUDcSfCpPnxqueX8R+yS2DVvIN373PH5Pvjs8I79AyU+TTZB2eHAR6WoVY/f9wzOIGYq2nwoz7cESfJ6yz+7DgIL5GWpCANQDNtq2sBoNhnWauMUQRM402KplmWSgLUIi20Dw77XBuFCFBKQJ1+I8v5S8NA3DLpKvfz4mWNyZumHzDL6ycLYusr4Rao7qe+w3L65ivlR5wSVxUgT8Pn1nUOtbN7netnp9er/Px0TFSMUXVovB4WB/Mz7fr0uUs2EGjq4ueQYodVy9HWIXgXOT6fKx+g7WS5vP2nyNfHL28soKW+E2McP3Zf/AQ== From e3dc5bdbcf5572746556f5ef2aa43fbb9e5ce2ce Mon Sep 17 00:00:00 2001 From: Kevin Pratt Date: Wed, 28 Feb 2018 11:04:25 -0700 Subject: [PATCH 3/3] Restore logic for Utils#element_text In this commit I'm putting the tests back the way they were and restoring the logic in the Utils.element_text. Instead I'm applying the change that @brianswko suggested at the start of https://github.com/onelogin/ruby-saml/issues/443 This still allows for my new test to pass. --- lib/onelogin/ruby-saml/response.rb | 2 +- lib/onelogin/ruby-saml/utils.rb | 2 +- test/response_test.rb | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/onelogin/ruby-saml/response.rb b/lib/onelogin/ruby-saml/response.rb index 0eb79c740..c78946f23 100644 --- a/lib/onelogin/ruby-saml/response.rb +++ b/lib/onelogin/ruby-saml/response.rb @@ -322,7 +322,7 @@ def destination def audiences @audiences ||= begin nodes = xpath_from_signed_assertion('/a:Conditions/a:AudienceRestriction/a:Audience') - nodes.map { |node| Utils.element_text(node) }.compact + nodes.map { |node| Utils.element_text(node) }.reject(&:empty?) end end diff --git a/lib/onelogin/ruby-saml/utils.rb b/lib/onelogin/ruby-saml/utils.rb index 450ad9154..a135d697e 100644 --- a/lib/onelogin/ruby-saml/utils.rb +++ b/lib/onelogin/ruby-saml/utils.rb @@ -286,7 +286,7 @@ def self.original_uri_match?(destination_url, settings_url) # that there all children other than text nodes can be ignored (e.g. comments). If nil is # passed, nil will be returned. def self.element_text(element) - element.texts.join if element && !element.texts.empty? + element.texts.join if element end end end diff --git a/test/response_test.rb b/test/response_test.rb index dbb78e890..28314d61b 100644 --- a/test/response_test.rb +++ b/test/response_test.rb @@ -1137,12 +1137,12 @@ class RubySamlTest < Minitest::Test it "return multiple values including nil and empty string" do response = OneLogin::RubySaml::Response.new(fixture(:response_with_multiple_attribute_values)) - assert_equal [nil, "valuePresent", nil, nil], response.attributes.multi(:attribute_with_nils_and_empty_strings) + assert_equal ["", "valuePresent", nil, nil], response.attributes.multi(:attribute_with_nils_and_empty_strings) end it "return multiple values from [] when not in compatibility mode off" do OneLogin::RubySaml::Attributes.single_value_compatibility = false - assert_equal [nil, "valuePresent", nil, nil], response_multiple_attr_values.attributes[:attribute_with_nils_and_empty_strings] + assert_equal ["", "valuePresent", nil, nil], response_multiple_attr_values.attributes[:attribute_with_nils_and_empty_strings] OneLogin::RubySaml::Attributes.single_value_compatibility = true end