Can't logout from IdP

[llekn]

trafficstars

Hi guys!

I've been working with this gem and the login part is working just fine, but I can't make the logout on IdP to work.

Currently I followed the instructions on the wiki about multiple authentication strategies, but I'm logging out the user using the #destroy method of my custom SessionsController:

class Users::SessionsController < Devise::SessionsController
  def destroy
    @user = current_user
    super do
      return redirect_to destroy_saml_user_session_path
    end
  end
end

Unfortunately when I do return redirect_to destroy_saml_user_session_path I get a 404 Not found on /users/sign_out

Am I missing something or doing something wrong?

Thanks!

[adamstegman]

Can you share your routes file? At least, the parts with devise_for and devise_scope, and any other authentication routes you have. The output of bin/rails routes for those routes would also be helpful.

[llekn]

@adamstegman yes, of course.

routes.rb :

...
devise_for :users, controllers: { omniauth_callbacks: "users/omniauth_callbacks", sessions: 'users/sessions' }
...

and the relevant part of rails routes:

                      new_user_session GET      /users/sign_in(.:format)                                                                          users/sessions#new
                          user_session POST     /users/sign_in(.:format)                                                                          users/sessions#create
                  destroy_user_session DELETE   /users/sign_out(.:format)                                                                         users/sessions#destroy
                     new_user_password GET      /users/password/new(.:format)                                                                     devise/passwords#new
                    edit_user_password GET      /users/password/edit(.:format)                                                                    devise/passwords#edit
                         user_password PATCH    /users/password(.:format)                                                                         devise/passwords#update
                                       PUT      /users/password(.:format)                                                                         devise/passwords#update
                                       POST     /users/password(.:format)                                                                         devise/passwords#create
                 new_user_confirmation GET      /users/confirmation/new(.:format)                                                                 devise/confirmations#new
                     user_confirmation GET      /users/confirmation(.:format)                                                                     devise/confirmations#show
                                       POST     /users/confirmation(.:format)                                                                     devise/confirmations#create
                       new_user_unlock GET      /users/unlock/new(.:format)                                                                       devise/unlocks#new
                           user_unlock GET      /users/unlock(.:format)                                                                           devise/unlocks#show
                                       POST     /users/unlock(.:format)                                                                           devise/unlocks#create
                accept_user_invitation GET      /users/invitation/accept(.:format)                                                                devise/invitations#edit
                remove_user_invitation GET      /users/invitation/remove(.:format)                                                                devise/invitations#destroy
                   new_user_invitation GET      /users/invitation/new(.:format)                                                                   devise/invitations#new
                       user_invitation PATCH    /users/invitation(.:format)                                                                       devise/invitations#update
                                       PUT      /users/invitation(.:format)                                                                       devise/invitations#update
                                       POST     /users/invitation(.:format)                                                                       devise/invitations#create
                 new_saml_user_session GET      /users/saml/sign_in(.:format)                                                                     devise/saml_sessions#new
                     saml_user_session POST     /users/saml/auth(.:format)                                                                        devise/saml_sessions#create
             destroy_saml_user_session DELETE   /users/sign_out(.:format)                                                                         devise/saml_sessions#destroy
                 metadata_user_session GET      /users/saml/metadata(.:format)                                                                    devise/saml_sessions#metadata
         idp_destroy_saml_user_session GET|POST /users/saml/idp_sign_out(.:format)                                                                devise/saml_sessions#idp_sign_out

By the way, I'm using version 1.5.0 of the gem and using config.saml_route_helper_prefix = 'saml' on devise.rb.

Hope it helps!

[adamstegman]

I notice that destroy_user_session and destroy_saml_user_session actually point to the same route. I don't know if that could cause a 404, but it does seem wrong to try to redirect to the same route you're already on. Can you move your custom controller logout to a different route and see if that changes the result?

It would probably be good to have that routes as /users/saml/sign_out in the gem, but now that would be a breaking change 😓

[mjfrey]

I noticed the default route for sign_out is the same as the devise sign_out and not /users/saml/sign_out. Is this by design? I had to change it manually in my routes.

[adamstegman]

I think they should be separate, but that ship has already sailed on v1 of this gem. ☹️

[llekn]

Hi guys, sorry for the delay, I just couldn't check this out before.

Just for the record, what I ended up doing was adding:

get 'users/saml/sign_out', to: 'devise/saml_sessions#destroy', as: "destroy_saml_session"

to my routes.rb file.

Maybe there is a way to add /users/saml/sign_out to the gem without replacing the current route so we can maintain compatibility and support this case too?

[mattbaumann1]

We had a bunch of IDP logout issues because of the session_index. You can see what we did on this ticket. I have been meaning to create a PR for it, but haven't had a chance. #117

[hammady]

I think they should be separate, but that ship has already sailed on v1 of this gem. ☹️

I was about to create a PR to change the bug in the gem routes (I believe it is a bug because all routes use the prefix but the sign out does not). What's wrong in introducing a breaking change and bumping the major version? To be honest, I checked all related issues, and the workarounds are not clean where one must maintain custom routes.

[adamstegman]

We could do that; it shouldn't be too hard to maintain 1.x and 2.x branches. I think I was hoping to think of more breaking changes that would be helpful for a V2, but I just don't have the time.

[themilkman]

We also got issues (and real confusion ;-)) with the duplicated sign_out route target. A fix with a " saml_" prefixed route would be great, bumping to v2 wouldn't matter that much imho. I do understand the feelings regarding it but at the end of the day those are only numbers and SemVer is made exactly for this situations to help us devs to get aware of breaking changes.