heartcombo / devise

Flexible authentication solution for Rails with Warden.
http://blog.plataformatec.com.br/tag/devise/
MIT License
24k stars 5.55k forks source link

Routes issue when devise engine is mounted in another engine #4127

Closed bilby91 closed 5 years ago

bilby91 commented 8 years ago

I'm building an engine that uses devise inside for user authentication. I have followed the instructions in the wiki (https://github.com/plataformatec/devise/wiki/How-To:-Use-devise-inside-a-mountable-engine) but I couldn't make it work as expected. It seems that router_name is being ignored. Maybe I'm miss understanding something.

I have created a simple engine with a dummy app that reproduces my issue. You can find it here: https://github.com/bilby91/devise_mountable_engine_issue. In the spec/dummy app there is a authenticated route under / path. When user is redirected to the sign_in path, the route doesn't build correctly and ignores the router_name.

I could make it work by providing a custom FailureApp to warden that handles the redirect.

module MyEngine

  class FailureApp < Devise::FailureApp

    def redirect_url
      engine_route = Rails
        .application
        .routes
        .routes
        .select { |r| r.app.app == MyEngine::Engine }
        .first

      "#{engine_route.path.spec.to_s}/users/sign_in"
    end
     #
    #  # You need to override respond to eliminate recall
    def respond
      if http_auth?
        http_auth
      else
        redirect
      end
    end

   end

end

I really don't like this solution :(. Any help is more than welcome!

Thanks!

bilby91 commented 8 years ago

Any help ?

petrosp commented 8 years ago

+100 for this. I have exactly the same issue and ended up using a FailureApp which is not really solution but a workaround.

Please also note that in ticket #3521, @josevalim suggests that if you use a namespace you should name the authenticate_user! method accordingly -- in our case should be authenticate_my_engine_user! -- but still this doesn't work.

Is anybody having a look at this?

petrosp commented 8 years ago

@bilby91, have you heard anything on this issue?

bilby91 commented 8 years ago

@petrosp nothing yet. 🙁

bilby91 commented 8 years ago

I know lots of people with this same problem. It would be awesome to have some feedback :)

fmh commented 8 years ago

+1, I think this wiki https://github.com/plataformatec/devise/wiki/How-To:-Use-devise-inside-a-mountable-engine is no longer works ... or is outdated

ghost commented 7 years ago

The same..... .o_0
-100 rep for devise.. ..

a-barbieri commented 7 years ago

I encountered the same problem after upgrading from Rails 5.0 to 5.1 and moving from Devise 4.1.1 to 4.3.0. That wiki needs to be updated or something needs to be fixed.

a-barbieri commented 7 years ago

I've been investigating and apparently the problem lies here.

@basiszwo: I've seen you created this piece of code, so I think you are the person we are looking for. With context.send(route, opts) Devise skips the route_name, but when remove the second parameter it works, i.e. context.send(route). What is the use of opts?

I'm temporarily monkey-patching my engine adding this code to config/initializers/devise_patch.rb:

require "action_controller/metal"

module Devise
  # Failure application that will be called every time :warden is thrown from
  # any strategy or hook. Responsible for redirect the user to the sign in
  # page based on current scope and mapping. If no scope is given, redirect
  # to the default_url.
  class FailureApp < ActionController::Metal

  protected

    def scope_url
      opts  = {}

      # Initialize script_name with nil to prevent infinite loops in
      # authenticated mounted engines in rails 4.2 and 5.0
      opts[:script_name] = nil

      route = route(scope)

      opts[:format] = request_format unless skip_format?

      config = Rails.application.config

      if config.respond_to?(:relative_url_root) && config.relative_url_root.present?
        opts[:script_name] = config.relative_url_root
      end

      router_name = Devise.mappings[scope].router_name || Devise.available_router_name

      context = send(router_name)

      if context.respond_to?(route)
        context.send(route)
      elsif respond_to?(:root_url)
        root_url(opts)
      else
        "/"
      end
    end
  end
end
basiszwo commented 7 years ago

@a-barbieri (see the PR I made)[https://github.com/plataformatec/devise/pull/4081/files] which addressed the original problem I and some others had with engines on authenticated routes. My PR also includes a test. Please note that the line you are referring to hasn't been changed touched at all in my PR.

I will try to verify the engines problem with a vanilla setup. Actually this is the only way to verify the functionality. (This btw. should be done through the test case anyway 😏 )

basiszwo commented 7 years ago

After debugging this for a couple of hours now I fear the problem actually comes from the PR I made. Before my PR opts[:script_name] wasn't even defined thus the call to generate the route (https://github.com/plataformatec/devise/blob/master/lib/devise/failure_app.rb#L151) was actually

context.send(route, {})

This results in a route like "http://localhost:3001/myengine/users/sign_in" (assuming the router_name is set to my_engine

After my PR the result is "http://localhost:3001/users/sign_in"

This was obviously not my intention but it fixed the error with infinite loop (mentioned in my PR).

At the moment I have no solution for this. First thing is to provide a test within devise we can code against. Everything else is not future proof.

a-barbieri commented 7 years ago

Good. I can try end of next week to add a test a see what comes out. Where did you test the infinite loop? I'd like to place this test there as well.

tomdracz commented 7 years ago

Just encountered the same problem and issue definitely lies in the change introduced by @basiszwo Would love to see example of infinite loop when not using opts[:script_name] so this could be investigated further

basiszwo commented 7 years ago

I am not available for the next 10 days.

you can go back to before my PR and verify the code is working there.

unfortunately neither the case I fixed nor your case has test coverage. so how could one know ...

Am 26. Juni 2017, 11:24 +0200 schrieb Tom Dracz notifications@github.com:

Just encountered the same problem and issue definitely lies in the change introduced by @basiszwo Would love to see example of infinite loop when not using opts[:script_name] so this could be investigated further — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

ouranos commented 6 years ago

Any updates on this one? I'm having a similar issue and I'm a bit reluctant to unset opts[:script_path] without fully understanding the loop issue. From what I gathered, the loop issue only occurs for authenticated routes, eg:

authenticate :admin do
  mount Sidekiq::Web => "/admin/sidekiq"
end

So if I'm not using authenticated routes, it should be safe to monkey patch to unset opts[:script_name]?

a-barbieri commented 6 years ago

@ouranos if you unset opts[:script_name] it works and if you are not encountering the infinite loop mentioned by @basiszwo in this PR you are good to go.


Going back to the issue:

opts[:script_name] should contain the path the engine is mounted on, in my case /admin_panel. Unfortunately relative_url_root doesn't return that path, as it's not defined when using mount MyEngine::Engine => '/admin_panel'. I'm trying to find a way to retrieve that path from within scope_url method, but it's pretty difficult. Any suggestion is welcome.