FIX plataformatec/devise#4127

[a-barbieri]

I've been able to fix the issue for #4127 (comment) though I'd like some help to create the test.

It's pretty simple: the engine should point to a directory inside the engine, in order to trigger the redirect. Then the test would be:

test 'redirect to login page when browsing an internal route without being authenticated' do
    get '/mountable_engine/some_route'
    follow_redirect!
    assert_response :success
    assert_contain 'Rendered content of MyMountableEngine'
end

@basiszwo, how can I add to your code an internal route some_route in order to test it?

[tegon]

Hello @a-barbieri, thanks for your pull request.
Did you manage to write a test for this?

[a-barbieri]

Hi @tegon, I manage to create the test. Pretty straight forward. Let me know if it's ok on your side.

[a-barbieri]

Apparently, fixing the issue generated by the previous PR, brought back an even older issue which led to an infinite loop on Rails 4.2, Ruby 2.2.8 and ActiveRecord ORM.

My guess is that tests made on that PR weren't testing the issue properly as several people was complaining afterwards. Therefore, in the test spec, I replaced the Rack app with a Rails Engine to be sure tests are working properly and in fact now we can see them failing.

Unfortunately locally I cannot reproduce the error.

@tegon, how can I test just the version that fails on Travis locally?

I tried copy/pasting the gemfiles/Gemfile.rails-4.2-stable code into the current Gemfile and running bundle update on Ruby 2.2.8, but test are ok, no failure at all. Anyway I'm sure there is a proper way to the same thing that you guys use every time you need to test.

[tegon]

@a-barbieri you can use the BUNDLE_GEMFILE environment variable. Here's what I usually do:

rbenv local 2.2.8 # or rvm use 2.2.8
BUNDLE_GEMFILE=gemfiles/Gemfile.rails-4.2-stable bundle install
BUNDLE_GEMFILE=gemfiles/Gemfile.rails-4.2-stable rake test

[a-barbieri]

Thanks @tegon, running

BUNDLE_GEMFILE =gemfiles/Gemfile.rails-4.2-stable rake test

was throwing a error:

/path/to/devise/test/rails_app/gemfiles/Gemfile.rails-4.2-stable not found

I managed to make it work adding the local path to Devise repo

BUNDLE_GEMFILE=/path/to/devise/gemfiles/Gemfile.rails-4.2-stable rake test

---

Anyway nothing fails.

Finished in 19.669069s, 41.3848 runs/s, 110.2747 assertions/s.
814 runs, 2169 assertions, 0 failures, 0 errors, 0 skips

Can you try on your machine? Or run again Travis to confirm the failure?
I really don't know how to reproduce this failing test in order to debug it.

[a-barbieri]

The only thing that I noticed is that there is this message between the dots:

Expected string default value for '--test-framework'; got false (boolean)

What is the meaning of that? Any hint?

[tegon]

@a-barbieri Indeed, I triggered another build and this time everything passed. I'll review your PR later, I still need to read the related issues to fully understand the problem.

Thanks again for your contribution!

[feliperenan]

@a-barbieri I'm trying to understand the issue reproducing it locally and I've some things to share.

First, I created a new Engine using the last Rails version (5.2.2) based on that sample app created in the issue. Everything seems to be working using Devise 4.5.0.

When the user access the authenticated route, he/she is redirected to the sign in path. See:

Notice that the engine app is in the url: http://localhost:3000/blorgh/users/sign_in. As far as I understood, this is the correct behavior or am I missing something?

After, I tried to run my Engine against this branch but It doesn't work, I got the following error:

In the end, I tried to run that sample app) that is on Rails 4.2 using this branch and I got the same error above. Using the last Devise version (4.5.0), I got a different error and I think that's the issue:

TL;DR

If I reproduced the issue correctly, the issue only happens on Rails 4.2 and this code didn't fix the problem.

[a-barbieri]

Hi @tegon, it's been some time since I fixed this issue so I'm trying to catch up from where I left it.

When I first came across it I was using Rails 5.1 and 4.3. See #4127 (comment)

---

Notice that the engine app is in the url: http://localhost:3000/blorgh/users/sign_in. As far as I understood, this is the correct behavior or am I missing something?

You are right. Back then it didn't work because relative_url_root would not bring blorgh to opts. Mounting the engine relative_url_root didn't work. If it does now it means someone did fix it. See #4127 (comment)

Can you check that relative_url_root contains blorgh with Devise 4.5 and 5.2?

You should debug relative_url_root and the opts hash at those lines:
https://github.com/plataformatec/devise/blob/48aa20897f2a2022c970398c67495cfb04be5f2a/lib/devise/failure_app.rb#L145
https://github.com/plataformatec/devise/blob/cbbe932ee22947fb7fc741a4da3e6783091c88b0/lib/devise/failure_app.rb#L157

---

Regarding

undefined method root_path

Can you just double check that you have a root set within your routes? My if statement was actually pretty loose.

---

TL;DR

If Rails has fixed relative_url_root on a mounted engine we don't need this PR anymore.

[feliperenan]

Can you check that relative_url_root contains blorgh with Devise 4.5 and 5.2?

Both relative_url_root? and relative_url_root returns nil.

Can you just double check that you have a root set within your routes? My if statement was actually pretty loose.

Ohh, I had not the root_path configured in the Blorgh Engine 🙈. Now your code works well, even in Rails 4.2

For some reason it works without your patch on Rails 5.2 (even with relative_url_root), but I don't know if something has changed inside Rails to get it working on that version.

Here the app that I used to test it: https://github.com/feliperenan/engine-test-app

[a-barbieri]

Ohh, I had not the root_path configured in the Blorgh Engine 🙈. Now your code works well, even in Rails 4.2

Good.

I was thinking about changing elsif defined? context.routes to elsif defined? context.routes && defined? context.routes.url_helpers.root_path, but then you wont have that error anymore. You might want to get a undefined method root_path, as you might have forgotten to set the root.

For some reason it works without your patch on Rails 5.2 (even with relative_url_root), but I don't know if something has changed inside Rails to get it working on that version.

As soon as I've got some spare time I'll check what has changed. Meanwhile happy to know both versions are working.

[feliperenan]

I think I've found the issue.

ActionDispatch has changed the way it finds the script_name in Rails 5.1, we can see the different implementations among Rails 4.2 and Rails 5.2:

Rails 4.2 - router_proxy.rb
Rails 5.0 - router_proxy.rb
Rails 5.1 - router_proxy.rb
Rails 5.2 - router_proxy.rb

Debugging Devise with Rails 5.2

Debugging into Rails 5.2 app, I can see that context.send(route, opts) returns the correct path even with {script_name: nil}

context.send(rout, {script_name: nil})
# => "http://localhost:4000/blorgh/users/sign_in"

context.send(rout, {})
# => "http://localhost:4000/blorgh/users/sign_in"

The /blorgh path is returned by @script_namer.call(options) and after merged into options.

building script name on Rails 5.1+

Debugging Devise with Rails 4.2

On the other hand, on Rails 4.2, when context.send(route, opts) call happens with {script_name: nil}, it returns the wrong path.

context.send(rout, {script_name: nil})
# => "http://localhost:4000/users/sign_in"

context.send(rout, {})
# => "http://localhost:4000/my_engine/users/sign_in"

Looking into Rails 4.2 code, on the way, they check if the options have a script_name and if it does they call super and returns the wrong path since script_name is nil.

script_name = find_script_name options
# => "http://localhost:4000/users/sign_in"

options.delete(:script_name)
script_name = find_script_name options
# => "http://localhost:4000/my-engine/users/sign_in"

find_script_name call
find_script_name method definition

Where context.send is called on Devise code

https://github.com/plataformatec/devise/blob/c000b58c565f46cb63d52ade0807294943b20cd3/lib/devise/failure_app.rb#L153

TL;DR

We should apply your patch considering the change you've mentioned (checking for root_path), but also adding a comment to remove these checks after we remove support of Rails 5.0 or we could add these checks only for Rails versions lower than Rails 5.0.