Devise::Strategies::SamlAuthenticatable#valid? doesn't pass settings

[rsl]

Devise::Strategies::SamlAuthenticatable#valid? doesn't pass settings which makes OneLogin::RubySaml::Response.new fail with encrypted assertions because #generate_decrypted_response cannot succeed because settings itself is nil. i've got a branch where i've made this small change and the tests pass but unfortunately there's no specs for testing encrypted assertions [and i'm not sure i follow how to make them in this setup]. i'd love to make a pr but would need some assistance on generating the specs needed to make the specs meaningful for this [ie, specs that fail with the current behavior and pass with the new].

also i believe references to OneLogin::RubySaml::Response.new(params[:SAMLResponse]).issuers.first in the idp readers would need to be changed to handle this as well, which means changing the arity of the reader's .entity_id method to take params and settings. this isn't needed for my use because we've got a custom database driven approach we'd already build and so use the standard Devise.saml_config [by having a custom idp reader return nil to .entity_id lookup].

[adamstegman]

i'd love to make a pr but would need some assistance on generating the specs needed to make the specs meaningful for this [ie, specs that fail with the current behavior and pass with the new].

I'm happy to help. It sounds like in this case unit tests in the strategy would be sufficient, we'd just need to generate an encrypted response to write them.

also i believe references to OneLogin::RubySaml::Response.new(params[:SAMLResponse]).issuers.first in the idp readers would need to be changed to handle this as well

👎 That's true, and I'm not sure how best to tackle this. The settings can't be passed to the entity_id reader, because we can't determine which settings to use without the entity_id. I think the entity_id reader implementation would need to handle trying different keys itself to figure out which one to use, which I'm not sure is something we can support with an API. It might just be up to the application developer.

[rsl]

sounds good on the former. and i agree on the latter. that's what we did. perhaps a note or something that this is an issue you have to address if you use encrypted assertions?

[adamstegman]

Sure, I can add it to the README where it brings up the entity ID reader; or it can be part of your PR if you'd like.

[brand-it]

This might be apart of the work that is being done here. However it sound like this might be a new issue.

undefined method 'issuers' using ruby-saml (1.4.0)

Got the following raised error

undefined method 'issuers' for #<OneLogin::RubySaml::Response:0x007fbb64234980>

Solution Use ruby-saml (1.3.1)

For a quick fix it might help to add a requirement of ruby-saml, '~> 1.3.0' to make sure it does not bump to the 1.4.0 version.

Current Gem lock file info

    devise_saml_authenticatable (1.3.0)
      devise (> 2.0.0)
      ruby-saml (~> 1.3)

ruby-saml 1.4.0 was added October 13, 2016.

[prosanelli]

I am seeing this issue with ruby-saml 1.3.1, devise_saml_authenticatable 1.3.0, and Rails 5.0.0.1

[adamstegman]

@prosanelli Can you make a new issue and explain more about what issue you're seeing?

[pitbulk]

issuers method is back on ruby-saml 1.4.1