-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Fix authenticated engine routes #4081
Conversation
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? |
Temporary fix until heartcombo#4081 is merged
Is this error still happening on Rails 5 rc? |
@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. |
4b3f61f
to
645fd6f
Compare
@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. |
@basiszwo awesome test, thanks ❤️ |
In my application it doesn't work: rails 5.0.0.1 authenticate :user, lambda { |u| u.admin? } do mount Sidekiq::Web => '/sidekiq' end Can you help me? |
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? |
Without lambda also doesn't work. |
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. |
I'm seeing this fail with Sidekiq as well. Rails 4.2.8 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 |
Also fails for me
|
didn't work for me either: |
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]
withnil
.When investigating the behaviour I found the following related issues:
authenticate
in routes.rb (plus pull request that caused it) #3643FailureApp
sscript_name: nil
breaks route generation within mounted engines #3705There'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!