Possible regression introduced in 1.6.0 series (and after) when SAMLResponse is empty

[bnferguson]

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:

    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

[adamstegman]

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?