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

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

Closed
bjeanes opened this issue Nov 5, 2020 · 18 comments
Closed

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

bjeanes opened this issue Nov 5, 2020 · 18 comments

Comments

@bjeanes
Copy link

bjeanes commented Nov 5, 2020

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 #2 (comment) 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
Copy link
Owner

janko commented Nov 5, 2020

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
Copy link
Author

bjeanes commented Nov 5, 2020

Excellent! Thank you!

@janko
Copy link
Owner

janko commented Nov 8, 2020

We'll need the around_rodauth hook for this one 😃

@bjeanes
Copy link
Author

bjeanes commented Nov 9, 2020

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

@bjeanes
Copy link
Author

bjeanes commented Nov 9, 2020

jeremyevans/rodauth#129

@janko
Copy link
Owner

janko commented Nov 10, 2020

@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
Copy link
Author

bjeanes commented Nov 11, 2020

I'll give it a go soon! 🙏

@bjeanes
Copy link
Author

bjeanes commented Nov 12, 2020

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

  • ✔️ before_action runs!

  • ✔️ rodauth action does NOT run if before_action creates a response

  • ✔️ rescue_from works as expected 🙏

  • ✔️ Caching controller makes sense. @ivars may be set by before_action etc. I actually already had to work around this in my integration of rodauth-rails to an existing app

  • after_action does NOT appear to run. I wonder if this is due to how Roda relies on halt/throw. Perhaps Rails' own run_callbacks does not use ensure. Potentially, you could do a catch(:halt) { yield } (or whatever it is), but I'm not sure how this should work. In any case, I don't personally have any after_action

  • ❌ Similarly around_action runs code before the action execution, but skips after.

    i.e.:

    around_action do |controller, action|
      # reached
      action.call
      # not reached
    ensure
      # reached
    end
  • I wonder if check_csrf should stay defined in feature, but return false. That would essentially skip Rodauth internal CSRF and rely on the before_action provided by the Rails controller.

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
Copy link
Owner

janko commented Nov 12, 2020

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
Copy link
Author

bjeanes commented Nov 12, 2020

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
Copy link
Owner

janko commented Nov 13, 2020

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
Copy link
Author

bjeanes commented Nov 14, 2020

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.

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
Copy link
Author

bjeanes commented Nov 15, 2020

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
Copy link
Author

bjeanes commented Nov 16, 2020

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?

@bjeanes bjeanes changed the title 5xx error pages Integrate with Rails rescue_from to support custom 5xx error pages Nov 16, 2020
@jeremyevans
Copy link

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
Copy link
Author

bjeanes commented Nov 16, 2020

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 janko closed this as completed in 2ca331e Nov 20, 2020
@janko
Copy link
Owner

janko commented Nov 22, 2020

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) – 806fb22. That's a first step towards making it easier to programmatically call Rodauth operations.

@bjeanes
Copy link
Author

bjeanes commented Nov 22, 2020

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

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

No branches or pull requests

3 participants