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

Fix authenticated engine routes #4081

Merged

Conversation

basiszwo
Copy link
Contributor

@basiszwo basiszwo commented May 3, 2016

Using Rails 5.0.0.beta4, Devise Edge (latest master), Warden 1.2.6

This pull request solves the issue of an infinite redirects for mounted engines when not being logged in.

This happens in devises' failure app. I was using rails 5 beta 3 when addressing the issue. I could reproduce the broken behaviour in rails 4.2.6 also.

First I addressed rails 5 by relaxing the rails 4.2 constraint. Then fixed the infinite loop by initializing opts[:script_name] with nil.

When investigating the behaviour I found the following related issues:

There's also #3807 which also addresses the main issue but imo with unnecessary conditionals.

The test suite is green as can be seen at https://travis-ci.org/basiszwo/devise/builds/127469964
I am sure the devise travis will also report green state.

Anyway I am not sure how to cover this issue in the test suite. You may point me to the spot?

I love to see feedback from you. Thank you very much!

@lucasmazza
Copy link
Contributor

Anyway I am not sure how to cover this issue in the test suite. You may point me to the spot?

That's the hard question 😩 maybe we should have a mountable engine on on our test app and have a test that hits one of its URLs?

@lucasmazza lucasmazza added this to the 4.2.0 milestone May 3, 2016
mtarnovan pushed a commit to mtarnovan/devise that referenced this pull request May 20, 2016
Temporary fix until heartcombo#4081
is merged
@ulissesalmeida
Copy link
Contributor

ulissesalmeida commented May 21, 2016

Is this error still happening on Rails 5 rc?

@basiszwo
Copy link
Contributor Author

@ulissesalmeida just tried. latest devise from master with rails rc 1 still has the problem. I try to provide a test within the next few days.

@basiszwo basiszwo force-pushed the fix-authenticated-engine-routes branch from 4b3f61f to 645fd6f Compare May 21, 2016 06:58
@basiszwo
Copy link
Contributor Author

@ulissesalmeida please see the test in my last commit. Runs on my machine. Waiting for travis to pass. In case you like it that way I squash the commits.

@lucasmazza
Copy link
Contributor

@basiszwo awesome test, thanks ❤️

@lucasmazza lucasmazza merged commit cbbe932 into heartcombo:master May 21, 2016
@basiszwo basiszwo deleted the fix-authenticated-engine-routes branch May 22, 2016 11:04
@rubydev
Copy link

rubydev commented Nov 17, 2016

In my application it doesn't work:

rails 5.0.0.1
devise 4.2.0
sidekiq 4.2.6

  authenticate :user, lambda { |u| u.admin? } do
    mount Sidekiq::Web => '/sidekiq'
  end

Can you help me?

@basiszwo
Copy link
Contributor Author

I didn't have time to look into this with a blank slate application but I am using this in six different applications and it works with the versions from above.

I have to add that we use this as follows

authenticate :user do
  mount Sidekiq::Web => '/sidekiq'
end

Meaning without the additional lambda you are passing in.

Can you verify that it works without the lambda block?

@rubydev
Copy link

rubydev commented Dec 14, 2016

Without lambda also doesn't work.

@basiszwo
Copy link
Contributor Author

We have a spec proofing that exactly this is working.

maybe some other gem or configuration is interfering? you may verify this with a vanilla rails app including only devise and sidekiq.

sorry that I can't help you more. As said there is a spec in devise ensuring this scenario plus I run six applications with the versions from above and they work fine and as expected.

@ghost
Copy link

ghost commented Apr 20, 2017

I'm seeing this fail with Sidekiq as well.

Rails 4.2.8
Devise 4.2.1
Sidekiq 4.2.9

I get too many redirects when not logged in for both:

authenticate :user do
  mount Sidekiq::Web => '/sidekiq'
end

and

authenticate :user, -> (u) { u.super_admin? } do
  mount Sidekiq::Web => '/sidekiq'
end

@anthonyms
Copy link

Also fails for me

Using sidekiq 4.2.10
Using rails 4.0.13
Using devise 3.5.10

@carinabs8
Copy link

didn't work for me either:
sidekiq (5.0.0)
rails (5.0.3)
devise (4.3.0)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

6 participants