Possible conflict with the module argument of devise_for

[DanielDe]

Hi there, I've just started looking into using this library and I seem to be running into a conflict with our use of devise_for in our routes.rb:

devise_for :users, module: 'auth'

As far as I can tell, the problem is that this line tries to access the :saml_sessions key in controllers, but that key doesn't exist.

But that controllers hash is actually a default hash as defined here in Devise. Devise is using the value of the module argument from the devise_for call to namespace this controller, but Auth::SamlSessionsController doesn't exist, its still Devise::SamlSessionsController.

I think a fix here is to unconditionally namespace the controllers with devise in the devise_saml_authenticatable function I linked to above, but I was hoping to get some feedback on this before I implement this patch. If this is a reasonable approach, I'm super happy to submit a PR!

It's also possible that I've missed something that lets this library work well with the module argument in devise_for. Any help is appreciated!

[adamstegman]

Thanks for bringing this up! The module option is new to me. I can guess at what it does, but I don't see how it works in the devise code. Does it make e.g. Auth::SessionsController instead of Devise::SessionsController? Do you know how this works in devise?

[DanielDe]

Does it make e.g. Auth::SessionsController instead of Devise::SessionsController

Yep, exactly! We wanted to move the Devise controllers into their own directory (specifically app/controllers/auth) for organizational purposes, and this module option lets us do that.

So what do you think of my proposed fix? In our local version of this repo I've changed:

resource :session, only: [], controller: controllers[:saml_sessions], path: '' do

to:

resource :session, only: [], controller: 'devise/saml_sessions', path: '' do

(this would probably also have to be done a few lines lower in the else block).

Again, happy to propose this as a PR if you think this is the right approach.

Thanks!

[adamstegman]

That would be a fine short-term fix, but I'd like to understand the module option better and fully support it.