Skip to content

Conversation

@codez
Copy link
Contributor

@codez codez commented Sep 13, 2024

fixes #253

@adamstegman
Copy link
Collaborator

@codez if you update from master, I've fixed these test failures. There's a few that do apply to this PR after that:

Failures:

  1) Devise::SamlSessionsController#idp_sign_out when receiving an IdP logout request direct the resource to reset the session key
     Failure/Error: Devise.sign_out_all_scopes ? sign_out : sign_out(resource_name)
     
     Devise::MissingWarden:
       Devise could not find the `Warden::Proxy` instance on your request environment.
       Make sure that your application is loading Devise and Warden as expected and that the `Warden::Manager` middleware is present in your middleware stack.
       If you are seeing this on one of your tests, ensure that your tests are either executing the Rails middleware stack or that your tests are using the `Devise::Test::ControllerHelpers` module to inject the `request.env['warden']` object for you.
     # ./app/controllers/devise/saml_sessions_controller.rb:26:in `idp_sign_out'
     # ./spec/controllers/devise/saml_sessions_controller_spec.rb:359:in `block (4 levels) in <top (required)>'
     # ./spec/controllers/devise/saml_sessions_controller_spec.rb:376:in `block (4 levels) in <top (required)>'

  2) Devise::SamlSessionsController#idp_sign_out when receiving an IdP logout request with a specified idp accepts a LogoutResponse for the associated slo_target_url and redirects to sign_in
     Failure/Error: Devise.sign_out_all_scopes ? sign_out : sign_out(resource_name)
     
     Devise::MissingWarden:
       Devise could not find the `Warden::Proxy` instance on your request environment.
       Make sure that your application is loading Devise and Warden as expected and that the `Warden::Manager` middleware is present in your middleware stack.
       If you are seeing this on one of your tests, ensure that your tests are either executing the Rails middleware stack or that your tests are using the `Devise::Test::ControllerHelpers` module to inject the `request.env['warden']` object for you.
     # ./app/controllers/devise/saml_sessions_controller.rb:26:in `idp_sign_out'
     # ./spec/controllers/devise/saml_sessions_controller_spec.rb:359:in `block (4 levels) in <top (required)>'
     # ./spec/controllers/devise/saml_sessions_controller_spec.rb:388:in `block (5 levels) in <top (required)>'

  3) Devise::SamlSessionsController#idp_sign_out when receiving an IdP logout request with a relay_state lambda defined includes the RelayState param in the request to the IdP
     Failure/Error: Devise.sign_out_all_scopes ? sign_out : sign_out(resource_name)
     
     Devise::MissingWarden:
       Devise could not find the `Warden::Proxy` instance on your request environment.
       Make sure that your application is loading Devise and Warden as expected and that the `Warden::Manager` middleware is present in your middleware stack.
       If you are seeing this on one of your tests, ensure that your tests are either executing the Rails middleware stack or that your tests are using the `Devise::Test::ControllerHelpers` module to inject the `request.env['warden']` object for you.
     # ./app/controllers/devise/saml_sessions_controller.rb:26:in `idp_sign_out'
     # ./spec/controllers/devise/saml_sessions_controller_spec.rb:359:in `block (4 levels) in <top (required)>'
     # ./spec/controllers/devise/saml_sessions_controller_spec.rb:401:in `block (5 levels) in <top (required)>'

@codez codez force-pushed the multiple-user-sessions branch from 94d6def to cf17df1 Compare October 15, 2024 12:36
@codez
Copy link
Contributor Author

codez commented Oct 15, 2024

I pushed a change the should fix those specs. Can you please re-start the action workflow?

@adamstegman
Copy link
Collaborator

I think we might be missing a test case for saml_session_index_key being set to nil, like in the README.

This also seems like a major version bump, since it enables IdP sign out by default, and changes its behavior pretty significantly. I'll take care of the version after merging this.

@codez codez force-pushed the multiple-user-sessions branch from cf17df1 to f33506e Compare October 17, 2024 06:35
@codez
Copy link
Contributor Author

codez commented Oct 17, 2024

I added one in strategy_spec.rb, another in saml_sessions_controller_spec.rb already exists.

I agree that this will be a major version bump.

@adamstegman adamstegman merged commit 248d8b6 into apokalipto:master Nov 11, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow multiple sessions per user

2 participants