failure_app and other middlewares #2182

Closed
elado opened this Issue Dec 16, 2012 · 5 comments

Projects

None yet

2 participants

@elado
elado commented Dec 16, 2012

When there's an error (e.g. wrong username/password) and failure_app is firing, it's running another request after some Middlewares (depends on the order) had already finished their execution, i.e. after their @app.call(env).

I created a simple test where I have the following:

# custom_middleware.rb

class CustomMiddleware
  def initialize(app)
    @app = app
  end

  def call(env)
    env['running'] = true
    @app.call(env)
  ensure
    env['running'] = false
  end
end


# application_controller.rb

class ApplicationController ...
  before_filter { raise "No env['running']!" unless env['running'] }
end

And added the CustomMiddleware to the app.middleware.

The result is, when username or password are wrong, warden fires the devise failure_app after its app.call in the middleware, and it means that some of the middlewares had already finished their job too.

The failure_app generates another request to sessions#new, and now the env['running'] is set to false.

I know that one solution is to use app.middleware.insert_before Warden::Manager, CustomMiddleware, but if it's a gem that needs to be standalone it's not a good idea.

Any other suggestions?

@josevalim
Member

I know that one solution is to use app.middleware.insert_before Warden::Manager, CustomMiddleware, but if it's a gem that needs to be standalone it's not a good idea.

What do you mean? If a gem is inserting the middleware automatically?
You can always delete or swap the middleware inserted by the gem. I don't think we have another solution besides re-arranging the middleware.

@elado
elado commented Dec 16, 2012

Yes, some gems insert themselves to app.middleware in a Railtie and not aware of existence of Warden::Manager.

It seems like it's affecting all middlewared gems, probably up till now no one has encountered a bug related to that...

Maybe a redirect would make more sense (in case it's available at that point, before headers have sent to the client)
That way it will re-run all middlewares and won't try to execute new action after some middlewares have ended.

@josevalim
Member

@elado You can setup the redirect yourself. I don't think it makes sense to be the default in Devise since we already delay the inclusion of the middleware so we are likely to be last and such issues are unlikely to happen. In any case, thanks for the report!

@elado
elado commented Dec 17, 2012
  1. What's the right way to make a redirect instead of in-place rendering?
  2. How does devise delay the inclusion of the Middleware? I saw the generic app.middleware.use in the code.

Thanks

@josevalim
Member
  1. What's the right way to make a redirect instead of in-place rendering?

You can either remove the recall option from the SessionsController (by inheriting from it) or inherit from Devise::FailureApp and customize it.

  1. How does devise delay the inclusion of the Middleware? I saw the generic app.middleware.use in the code.

I thought we did but apparently we no longer do it. One trick is simply to include it in an initializer and move the initializer at the end. I will investigate later why we no longer do it.

@josevalim josevalim closed this Dec 17, 2012
@elado elado referenced this issue in drapergem/draper Dec 17, 2012
Closed

Use of Thread.current and ViewContext #390

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