janko / rodauth-rails

Rails integration for Rodauth authentication framework
https://github.com/jeremyevans/rodauth
MIT License
599 stars 40 forks source link

`after_action` is not called after `rodauth.login` #40

Closed renchap closed 3 years ago

renchap commented 3 years ago

If you have a around_action or after_action configured in your RodauthController, and calling rodauth.login in your action, then the processing stops and the callbacks are not called

  around_action :around_test
  after_action { puts "After action" }

  def around_test
    logger.info "Before around"
    yield
    logger.info "After around"
  end

  def test
    rodauth.account_from_login("email@example.com")
    rodauth.login("omniauth")
    puts "End of action" # not clear if this would be executed or not
  end

Output:

Started POST "/auth/test" for 127.0.0.1 at 2021-04-29 21:55:53 +0200
Processing by RodauthController#test as */*
  Parameters: {"rodauth"=>{}}
Before around
Completed   in 178ms (ActiveRecord: 69.1ms | Allocations: 62204)
janko commented 3 years ago

This happens the #login Rodauth method redirects via throw, which bubbles up all the way to the Rodauth middleware. throw/catch does execute any ensure blocks, but Active Support callbacks (which are used to implement Action Controller callbacks) don't execute callbacks inside an ensure block, which causes them not to execute.

Technically, this behaviour is probably correct, because the action doesn't really complete executing, because the execution flow breaks out of the controller action, and the Rodauth middleware then returns the response.

Any easy workaround should be to add a catch around the #login call inside the controller, and redirect using Rails instead:

# ...
catch(:halt) do
  rodauth.login("omniauth")
end
redirect_to rodauth.login_redirect
renchap commented 3 years ago

Ok understood. I feel this is not obvious and it might bite some people as rodauth-rails handles it with rodauth actions, but not here. Maybe at least we should add some documentation about it?

In my case, we have a way to emit events from controllers (new login, account…) and the events are grouped and emitted in bulk in an after_action, and this was silently not triggered in our omniauth login action, causing some events to be lost.

janko commented 3 years ago

I'm considering converting a Rodauth redirect into standard Rails controller redirect when calling Rodauth methods inside controllers, but I'm not sure yet how to design it sanely. I agree it would be convenient in terms of executing after_action and around_action callbacks, especially considering that calling methods such as rodauth.require_authentication in controllers is recommended in the docs.

If I choose not to do it, I'll definitely add the caveat to the documentation.

bjeanes commented 3 years ago

I think another simple option is simply to make your around_action use ensure, like so:

def around_test
  logger.info "Before around"
  yield
ensure
  logger.info "After around"
end

(this won't work for after_action though)

janko commented 3 years ago

I've finally been looking into this today. I initially thought that we need to find a generic way how to convert halting in any rodauth method into a controller response, but then I realized that Devise's #authenticate_user! is throwing as well (as it's calling Warden's #authenticate! internally), and people have obviously been fine with that.

So, it seems that it needs to be possible only for selected methods, such as #login. So I've come up with a #rodauth_response method, which catches :halt and writes the response data on the controller.

rodauth_response do
  rodauth.login("omniauth")
end

It's a bit cumbersome, I know, but I couldn't come up with anything better at this time. I initially started writing a macro that defines controller methods that delegate to rodauth, with rodauth calls automatically wrapped inside a rodauth_response { ... } block. However, since I didn't find good enough use cases outside of this one, I chose to keep only the #rodauth_response method for now.