A while back was digging into upgrading our rather old 1.4.1 version of this gem to the latest version (1.9.1 I think at the time of the version bump), it got blocked due to a rather strange spec failure in our internal test suite.
The test looks something like:
context "when SAMLResponse is blank" do
it "authentication fails" do
post :create, params: { SAMLResponse: "" }
expect(response).to have_http_status(302)
path = URI.parse(response.headers["Location"]).path
expect(path).to eq "/session"
end
end
And when run with a newer version of devise_saml_authenticatable it breaks due to an exception begin thrown like so:
1) SamlSessionsController POST create when SAMLResponse is blank authentication fails
Failure/Error: post :create, params: { SAMLResponse: "" }
OneLogin::RubySaml::ValidationError:
Issuer of the Response not found or multiple.
# /Users/bnferguson/.gem/ruby/3.1.0/gems/i18n-1.14.4/lib/i18n.rb:322:in `with_locale'
# ./spec/controllers/saml_sessions_controller_spec.rb:47:in `block (4 levels) in <top (required)>'
# ./spec/rails_helper.rb:335:in `block (3 levels) in <top (required)>'
# ./spec/rails_helper.rb:344:in `with_n_plus_one_detection'
# ./spec/rails_helper.rb:334:in `block (2 levels) in <top (required)>'
Doing a bit of bisecting I found that this behavior was introduced in 1.6.0 with this PR. Specifically this line.
Swapping back to the previous code, settings: Devise.saml_config we see the test pass again. Originally we did have config.idp_settings_adapter but no config.idp_entity_id_reader set.
Setting the config.idp_entity_id_reader with our own reader that handles blank SAMLResponses seems to fix the issue but was unsure if this was a possible regression, or if we're just missing an updated setting or some such.
Here's our idp_entity_id_reader we've gone with in the mean time:
module Saml
class IdpEntityIdReader
def self.entity_id(params)
if params[:SAMLRequest].present?
OneLogin::RubySaml::SloLogoutrequest.new(
params[:SAMLRequest],
settings: Devise.saml_config,
allowed_clock_drift: Devise.allowed_clock_drift_in_seconds,
).issuer
elsif params[:SAMLResponse].present?
OneLogin::RubySaml::Response.new(
params[:SAMLResponse],
settings: Devise.saml_config,
allowed_clock_drift: Devise.allowed_clock_drift_in_seconds,
).issuers.first
end
end
end
end
Seems like adding the #present? calls to the DefaultIdpEntityIdReader could work. Was there an issue seen in real usage that caused you to add this test case?
Hello!
A while back was digging into upgrading our rather old 1.4.1 version of this gem to the latest version (1.9.1 I think at the time of the version bump), it got blocked due to a rather strange spec failure in our internal test suite.
The test looks something like:
And when run with a newer version of
devise_saml_authenticatable
it breaks due to an exception begin thrown like so:Doing a bit of bisecting I found that this behavior was introduced in 1.6.0 with this PR. Specifically this line.
Swapping back to the previous code,
settings: Devise.saml_config
we see the test pass again. Originally we did haveconfig.idp_settings_adapter
but noconfig.idp_entity_id_reader
set.Setting the
config.idp_entity_id_reader
with our own reader that handles blank SAMLResponses seems to fix the issue but was unsure if this was a possible regression, or if we're just missing an updated setting or some such.Here's our
idp_entity_id_reader
we've gone with in the mean time: