janko / rodauth-rails

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

Integrate with Rails `rescue_from` to support custom 5xx error pages #15

Closed bjeanes closed 3 years ago

bjeanes commented 3 years ago

Hey Janko,

Do you have any advice on how to re-use the existing styled 5xx pages on the Rails' side for exceptions which happen in Rodauth land? Is this something rodauth-rails could/should cover in its integration?

I wonder if the around_rodauth we discussed in https://github.com/janko/rodauth-rails/issues/2#issuecomment-700495426 might be necessary or even if that would simply work if process_actions is also responsible for Rails' rescue_from behaviour (I haven't checked).

janko commented 3 years ago

Yes, I agree it would make sense for rodauth-rails to call rescue_from handlers defined in the associated Rodauth controller. It doesn't seem these handlers are run as part of action callbacks, but it I believe it can be achieved easily by calling #rescue_with_handler on the controller instance. I'll work on this tomorrow.

bjeanes commented 3 years ago

Excellent! Thank you!

janko commented 3 years ago

We'll need the around_rodauth hook for this one 😃

bjeanes commented 3 years ago

Alright, I'll revise my PR for Rodauth as per Jeremy's feedback in the next day or so.

bjeanes commented 3 years ago

https://github.com/jeremyevans/rodauth/pull/129

janko commented 3 years ago

@bjeanes I've just pushed changes to the callbacks-and-rescues branch that utilize #around_rodauth to run rescue handlers and controller callbacks around Rodauth actions. Let me know whether it works for you 😉

bjeanes commented 3 years ago

I'll give it a go soon! :pray:

bjeanes commented 3 years ago

Hey Janko, I gave it a little spin this afternoon. Great work as always!

I don't use after_action often so I am actually not very sure how this should behave. But I suspect they should run in circumstances where they currently do not. That may also be considered "nice-to-have".

janko commented 3 years ago

Thank you very much for testing 👍

Ok, I checked the run_callbacks implementation in Active Support, and indeed it doesn't use any ensure for executing after_action callbacks. I agree we'll probably need to catch(:halt) manually, and then re-throw it later.

Regarding check_csrf?, it will be false, because we're skipping loading Roda's CSRF plugin, and Rodauth's default #check_csrf? is false when the CSRF Roda plugin is not loaded. I thought about still overriding it just in case, but ultimately I didn't see the need.

bjeanes commented 3 years ago

Ack. I thought that might be the case for CSRF but wasn't sure. Glad to see you've already thought of it :D.

One thing I thought of last night: it may be worth seeing if you can force rails_controller_instance.performed? to be true after Rodauth has created a response. I don't personally do anything like this, but in principle an around_action may modify a response or vary its behaviour by what that response is. In practice, this may be too difficult and niche to get right, so take it or leave it!

janko commented 3 years ago

I pushed new changes to callbacks-and-rescues branch, which should address the after_action callback as well. I forced-pushed, hopefully Bundler handles that.

Regarding allowing the controller to modify the response, I've allowed modifying the response headers at any point of the callback chain. This change also adds Action Dispatch default headers to Rodauth responses, which may be convenient as many of them are security-related.

At this moment changing response body or status isn't possible in callbacks, unless you want to take over from the controller and render your own response. I felt like that would be a too big change around how is in charge, I don't know if that would have other unintended consequences.

Could you give the branch another go?

bjeanes commented 3 years ago

Regarding allowing the controller to modify the response, I've allowed modifying the response headers at any point of the callback chain. This change also adds Action Dispatch default headers to Rodauth responses, which may be convenient as many of them are security-related.

:+1:

At this moment changing response body or status isn't possible in callbacks, unless you want to take over from the controller and render your own response. I felt like that would be a too big change around how is in charge, I don't know if that would have other unintended consequences.

TBH, I'm not even sure if/how Rails handles that. But I thing it really is such a niche use-case that I fully support not inheriting a lot of complexity here, at least until somebody actually communicates a desperate need for it!

Could you give the branch another go?

You bet -- I'll give it a spin on Monday!

bjeanes commented 3 years ago

Works a charm. I am going to push up this and make sure all the tests still run and then maybe we can see if Jeremy is ready to cut a new version which includes around_rodauth (and another fix of mine sitting in Rodauth master!).

bjeanes commented 3 years ago

Everything is green on my end.

How do you want to approach a release, given it depends on unreleased work in Rodauth?

@jeremyevans do you have a timeline or milestone in mind for cutting a new Rodauth release?

jeremyevans commented 3 years ago

Next Rodauth release will probably be on the 20th (5 days) or 23th (8 days). Happy to do the former if that is preferred.

bjeanes commented 3 years ago

I am in no urgent rush, so either suits me. I'm not presently seeing any 5xx errors in production on these pages anyway :)

Thanks for the update!

janko commented 3 years ago

I've just released rodauth-rails 0.6.0 with these changes 🚀

The release also includes a new Rodauth::Rails.rodauth method, which initializes a Rodauth instance outside of a request context, with Rack env set based on ActionMailer::Base.default_url_options (which is typically configured via config.action_mailer.default_url_options in Rails) – https://github.com/janko/rodauth-rails/commit/806fb220abc2dbef41665d904c3e98742922ec6a. That's a first step towards making it easier to programmatically call Rodauth operations.

bjeanes commented 3 years ago

Hah is this in response to my Reddit comment? Looks great! I will be afk for a week and then spotty for a few more weeks, but I will try to integrate with app ASAP :D