Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
bilby91 opened this issue May 22, 2016 · 16 comments
Closed

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

bilby91 opened this issue May 22, 2016 · 16 comments

Comments

@bilby91
Copy link

bilby91 commented May 22, 2016

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
Copy link
Author

bilby91 commented Jul 4, 2016

Any help ?

@petrosp
Copy link

petrosp commented Aug 15, 2016

+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
Copy link

petrosp commented Sep 11, 2016

@bilby91, have you heard anything on this issue?

@bilby91
Copy link
Author

bilby91 commented Sep 11, 2016

@petrosp nothing yet. 🙁

@bilby91
Copy link
Author

bilby91 commented Sep 23, 2016

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

@fmh
Copy link

fmh commented Oct 10, 2016

+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
Copy link

ghost commented Dec 20, 2016

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

@a-barbieri
Copy link
Contributor

a-barbieri commented Jun 16, 2017

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
Copy link
Contributor

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
Copy link
Contributor

@a-barbieri (see the PR I made)[https://github.com//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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

basiszwo commented Jun 26, 2017 via email

@ouranos
Copy link

ouranos commented Nov 17, 2017

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
Copy link
Contributor

@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.

feliperenan added a commit that referenced this issue Jan 23, 2019
* Adding to check if rootpath is present on url_helpers.
* Run this code only for Rails versions lower than 5.1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

8 participants