devise: Authentication in routes breaks URL helpers

Environment

Ruby 2.5.1
Rails 5.2.2
Devise 4.5.0

Current behavior

In my routes.rb, I have a snippet that looks like this:

authenticate :user, ->(_u) { current_user&.admin? } do
  mount Sidekiq::Web => '/sidekiq'
end

In a test, I have a snippet that looks like this:

p new_user_session_path
get '/sidekiq'
p new_user_session_path

However, the two print statements gives an output like this:

"/users/sign_in"
"/sidekiq/users/sign_in"

If I remove the authenticate line in routes.rb, I get the expected output.

Expected behavior

I expect the path to never change, i.e. output should look like this:

"/users/sign_in"
"/users/sign_in"

About this issue

Original URL
State: open
Created 5 years ago
Comments: 15 (2 by maintainers)

Most upvoted comments

I’ve more finds to share 🙈.

When we do a request to "/sidekiq" that needs authentication, Devise redirects the user to the /users/sign_in with the FailureApp that behaves as a controller. Once the request is finished, the ActionDispatch put the FailureApp into the session controller instance. Also, notice that in this request process, ActionDispatch sets the @url_options to nil.

So, when we ask for a path using some path helper, ActionDispatch uses the session cached through memoization to build the correct path. When url_options method is called, as @url_options is nil, ActionDispatch is going to merge the controller#url_options with the session#url_options.

As I said before, FailureApp behaves as a controller, so it has the request information. When we inspect this controller asking for a path after a request for “/sidekiq”, we got:

From: /Users/feliperenan/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/gems/actionpack-5.2.2/lib/action_dispatch/testing/integration.rb @ line 126 ActionDispatch::Integration::Session#url_options:

    123: def url_options
    124:   @url_options ||= default_url_options.dup.tap do |url_options|
    125:     require 'pry'; binding.pry
 => 126:     url_options.reverse_merge!(controller.url_options) if controller
    127:
    128:     if @app.respond_to?(:routes)
    129:       url_options.reverse_merge!(@app.routes.default_url_options)
    130:     end
    131:
    132:     url_options.reverse_merge!(host: host, protocol: https? ? "https" : "http")
    133:   end
    134: end

[1] pry(#<#<Class:0x00007fc99adab108>>)> controller.class.name
=> "Devise::FailureApp"
[2] pry(#<#<Class:0x00007fc99adab108>>)> controller.url_options
=> {:host=>"www.example.com", :port=>nil, :protocol=>"http://", :_recall=>{}, :script_name=>"/sidekiq"}

This controller#url_options isn’t a FailureApp method. It’s included by ActionController::UrlFor. https://github.com/plataformatec/devise/blob/20e299bce0307d79895b05b13530f3c74a0ca0e0/lib/devise/failure_app.rb#L11

Then, as the script_name is there, the path will be concatenated with it as I said in this previous comment https://github.com/plataformatec/devise/issues/5019#issuecomment-463264252

It doesn’t happen in a get request that give 200 or when we use a follow_redirect! (that makes another get under the hood), because it makes another request that cleans up the @url_options and sets another controller into sessions instance.

From: /Users/feliperenan/.rbenv/versions/2.5.1/lib/ruby/gems/2.5.0/gems/actionpack-5.2.2/lib/action_dispatch/testing/integration.rb @ line 126 ActionDispatch::Integration::Session#url_options:

    123: def url_options
    124:   @url_options ||= default_url_options.dup.tap do |url_options|
    125:     require 'pry'; binding.pry
 => 126:     url_options.reverse_merge!(controller.url_options) if controller
    127:
    128:     if @app.respond_to?(:routes)
    129:       url_options.reverse_merge!(@app.routes.default_url_options)
    130:     end
    131:
    132:     url_options.reverse_merge!(host: host, protocol: https? ? "https" : "http")
    133:   end
    134: end

[1] pry(#<#<Class:0x00007fc99b43c7b0>>)> controller.class.name
=> "DemoController"
[2] pry(#<#<Class:0x00007fc99b43c7b0>>)> controller.url_options
=> {:host=>"www.example.com", :port=>nil, :protocol=>"http://", :_recall=>{:controller=>"demo", :action=>"index"}, :script_name=>""}

Now that we have all this information, I’m not sure if it’s a Devise issue. Maybe we should do something in the FailureApp, but I don’t know what. Maybe the issue is with ActionDispatch since they memoize the session and relies on it to build paths in the test environment.

I’m open for suggestions on how to proceed with this issue.

feliperenan on Feb 18, 2019

Just wanted to follow up and mention that as of Rails 5.2.5 and Devise 4.7.1 this is still an active bug.

glennfu on Jun 10, 2021

@tegon I was thinking about the same thing. As far as I understood, a new session is created on every single test, so developers could use follow_redirect! approach only in the test that they want to use a path returned by the path helper after a Devise redirect in this specific case.

If we don’t do a patch for it, we may need to document this behavior at some place 🤔.

feliperenan on Feb 18, 2019

@feliperenan Great work on this!

My 2 cents here is that since this only happens in the test environment - e.g. because the session is memoized - I believe there’s not much we can do. I’m not even sure if this should be addressed in Rails or not in some way. I’d go with following the redirect in the test in order to make it more close to a real request/response flow.

tegon on Feb 18, 2019

@feliperenan Wow that’s a really detailed report! Thanks for taking the time to look into this! I’ll try to do more digging into this to determine the source of the issue (in both devise and in Rails). I’ll post an update here if I find anything interesting.

peterzhu2118 on Feb 15, 2019

I’ve created a project to demo this. The test that demonstrates this can be found here.

peterzhu2118 on Feb 4, 2019