Do not verify CSRF for every rodauth request

[HoneyryderChuck]

I'm having a problem integrating roda-oauth in a particular case where I need to allow a POST request to an /oauth-token URI; such requests are usually performed in the backend, and do not go through an html form, therefore the CSRF protection doesn't make sense.

This route is defined from inside the rodauth feature, however, rodauth-rails runs the verify_authenticity_token by default for every rodauth request, so it breaks my flow.

My suggestion would be to remove this line, or skip it given some circumstance around the request. The roda integration with csrf protection works, so there's something being done there that allows csrf protection to be skipped which hasn't been replicated in rodauth-rails.

[janko]

We should probably add a #rails_check_csrf? method that defaults to true, which can then be overridden manually for specific routes.

Though I'm wondering what Roda's CSRF protection does differently 🤔 You either require a valid CSRF token for a route or not, it should also require that you explicitly disable CSRF protection for a certain route IMO. Maybe there is more to CSRF protection than I think.

[HoneyryderChuck]

roda's csrf protection is just rack_csrf. I'm gonna investigate this a bit more today, but the bottom line is, when working on rails, testing controllers and performing post calls, the token verification is somehow skipped. I'll have to find the how.

[janko]

That's good to know, I thought you were using Roda's route_csrf plugin which doesn't use the rack_csrf gem. Thanks for investigating.

[HoneyryderChuck]

I think I found the issue (sort of).

rodauth-rails calls verify_authenticity_token inside the rodauth feature, as an extension to the csrf plugin; however, there is a rodauth controller, where this callback should/could be defined:

class RodauthController < ApplicationController
before_action :verify_authenticity_token
end

In order to make this more compliant with the rails way, rodauth should skip/disable the roda csrf plugin, and let the rodauth controller validate the authenticity token where rails expects it. Rails developers already know that before_action, and can choose to skip it if they desire so (or conditionally, as is my case).

Also, I found out that csrf protection is disabled in test mode by default, although for some reason that check doesn't pick up that variable.

[janko]

How rodauth-rails works is that, before every action, the following happens:

1. #verify_authenticity_token is called on RodauthController
2. Rodauth action is performed (entirely in Roda)
3. RodauthController is called when a template needs to be rendered

So, rodauth-rails doesn't execute "actions" on the controller, which means before_action callbacks are not applicable.

Note that rodauth-rails disables Roda's CSRF plugin: https://github.com/janko/rodauth-rails/blob/45ba96c3f1401ba92f0d13192c43b5439f1e19aa/lib/rodauth/rails/app.rb#L16

And in test mode the CRSF protection should be properly skipped, because #verify_authenticity_token calls #verified_request?, which checks #protect_against_forgery?, which read the allow_forgery_protection setting that's set to false in test environment. The rodauth-demo-rails app has some tests that work without CSRF protection.

[HoneyryderChuck]

I see now, thx for the details. The way I see it, it should then work like this:

• Instead of "#verify_authenticity_token is called on RodauthController", all before_action callbacks from the rodauth controller should be run in before_rodauth (which means that all after_actions should run in after_rodauth. From the perspective of someone coming from rails development, they won't expect to define such callbacks in rodauth , but rather in rails controller.

or

• rodauth-rails becomes a rails engine instead of a railtie. If you see how devise integrates with rails, all of the "rails low-level details" (devise controllers, views, etc) are hidden from the developer, until the developer explicitly requests them (i.e. there's a generator to copy devise views to the project main views directory). rodauth-rails could then provide similar functionality.

Of course, this is a totally separate "feature request", I guess :)

However, from the perspective of putting rails things in rails, I think that csrf protection should be altogether "removed" from rodauth and put into rails controls, as it's less of a burden for a rails developer, as the internet is full of "how do I disable csrf protection in rails" and this project would benefit of complying with the norm.

Let me know what you think.

[HoneyryderChuck]

The rodauth-demo-rails app has some tests that work without CSRF protection.

It's true, rails disables csrf protection in test mode by default. I managed to circumvent this by explicitly defining in application.rb:

config.action_controller.allow_forgery_protection = false

and it works now. However, my problem still stands (having rodauth routes that I want to skip csrf protection for).

[janko]

Thank you for your input. I'll try to provide an answer for the two suggestions:

1️⃣ I do agree the current behaviour is non-standard compared to other Rails gems, but I think that's a result of Rodauth's unique design, and I think it would be slippery slope to try to mask rodauth-rails into something it isn't. rodauth-rails doesn't run controller actions, so I don't think it's correct to try to run Action Controller callbacks. Note that AFAIK there is no after_rodauth hook at the moment, so after_action couldn't really be implemented.

I also don't know how to call Action Controller callbacks separately from executing a controller action, and we'd need to ensure that options like :only and :except work. I also bet that API might differ depending on the Rails version (rodauth-rails supports Rails 4.2+ at the moment). Note that the main reason rodauth-rails is using a separate controller is for rendering templates – I'd use Action View directly if it wasn't coupled to Action Controller. I tried rendering templates with ApplicationController, but view paths weren't being set correctly.

2️⃣ I've explored making rodauth-rails a Rails engine, I was going back on forth when deciding on the design, but in the end I didn't see how it would benefit from being an engine, and I chose a simple railtie instead. I wanted everything to integrate automatically, but without hiding things that aren't necessary to hide. For example, I wanted to show copy Rodauth controller in the codebase for transparency.

---

If we provide instructions on disabling CSRF protection, I don't see a benefit in having it on the controller level. It would add additional maintenance burden, and IMO it would lead away from understanding how rodauth-rails works. Note that you can already disable CSRF protection:

rails_check_csrf! { super() unless request.path == oauth_token_path }

[HoneyryderChuck]

Thx, your response is pretty valid. I get most of the concerns, I'll leave a few more comments taking these into consideration:

rodauth-rails doesn't run controller actions, so I don't think it's correct to try to run Action Controller callbacks.

Agreed, better not go that route then.

but in the end I didn't see how it would benefit from being an engine, and I chose a simple railtie instead.

I'd argue that, if it were an engine, you'd manage to keep RodauthController for compatibility, without having to expose it to the user application, which is what happens now. I think what you want to achieve is allow a user to configure rodauth through rodauth_app, and the RodauthController is just there as "burden" to render views. Devise uses the same approach using engines, and I think rodauth-rails could benefit from it (same thing with the models/views: the moment you need them, you run the generator to load them under app, a la devise).

I wanted to show copy Rodauth controller in the codebase for transparency.

I just read this now. What do you mean by "for transparency"? I think that you'd gain from not exposing this detail to the user, for the possibility of him doing smth "rails-y", like before actions.

Note that you can already disable CSRF protection:

The thing is, I already do this here. rodauth-rails doesn't seem to use it, but rodauth defines it. Do you think it makes sense to use it in your case?

[janko]

I understand what you mean. However, the transparency is useful when the user wants to change the directory of their templates, which requires renaming the Rodauth controller and changing rails_controller in the Rodauth config; this is straightforward when everything is already in your app.

It also allows the user to define additional helper methods via the current RodauthController, which they wouldn't be able to do if it was hidden inside an engine. I mean, they would have to define their own controller, but that might not be so obvious.

I have a comment in the generated Rodauth controller file that this controller is used only for rendering views, so I'm hoping that is enough. I feel like making rodauth-rails an engine would be an overkill just to hide RodauthController, though I'll give it some more thought.

The thing is, I already do this here. rodauth-rails doesn't seem to use it, but rodauth defines it. Do you think it makes sense to use it in your case?

Thank you for shedding light on this, rodauth-rails should definitely hook into #check_csrf?, I've opened a pull request that should make this possible – https://github.com/jeremyevans/rodauth/pull/96.

[HoneyryderChuck]

Thank you for shedding light on this, rodauth-rails should definitely hook into #check_csrf?, I've opened a pull request that should make this possible

Great, thx Janko, this will do perfect, thx for the patch!

I understand what you mean. However, the transparency is useful when the user wants to change the directory of their templates ...

Just to be clear, my suggestions are only based on what devise does and what rails users might expect or be confused with. Your concerns are perfectly valid to me. However, I can feel that you'd like rodauth-rails to become a worthy competitor to devise, which is still the rails auth standard, and in order to do so, it should achieve most of the goals that devise does, while doing everything else better. Those are, IMO:

• quick-and-dirty bootstrap: you run rails generate devise:install + 1/2 declarations and you can see the forms there, with relatively few devise-specific code next to yours.
• easy customization: I think rodauth-rails has a great shot at doing it way easier than devise.

Most of your concerns are amendable:

when the user wants to change the directory of their templates, which requires renaming the Rodauth controller and changing rails_controller in the Rodauth config...

True, but if rails_controller defaults to RodauthController, which lives in a rails engine, the user doesn't need to "understand" that command (and if he does, it is a customization step). Devise achieves this with DeviseController. It'd be easier to tell your user to rails_controller { YourController } and class YourController < RodauthController, thereby making RodauthController yours to tweak.

when the user wants to change the directory of their templates, which requires renaming the Rodauth controller and changing rails_controller in the Rodauth config

I'd argue that they could define a RodauthHelper for that.

I feel like making rodauth-rails an engine would be an overkill just to hide RodauthController...

Not only for that, it'd also "hide" your default views, thereby achieving that "quick-and-dirty bootstrap" and "customize as you go".

However, I like it already as is, so take my comments with a "speculative grain of salt", as I'm assuming that users will feel one way, but they might feel differently in the end, so this might already be the setup users want. Maybe it's worth to wait for a few more opinions and questions.

P.S.: A very useful thing for this project would be a "How to migrate from Devise to Rodauth-Rails" page :)

[janko]

Yes, I would like this gem to be equally easy to work with as Devise, so it's great to hear any feedback.

I agree that RodauthController can be considered an implementation detail, however, if I were to hide it, I might need to add additional settings, such as parent_controller which Devise has (I guess not every Rails app has an ApplicationController? 🤷). I feel like this was a simpler solution from the maintainability perspective, which I find important as well.

Yes, you're 100% percent correct regarding adding helper methods via RodauthHelper. I just thought that someone might be using some tricks to define helper methods only for views generated by a certain controller (because helper methods are global, the naming is just a convention). I don't know how to do it at the moment, but I thought if it's possible then it's probably configured on the controller level. It's a stretch for sure, though.

Not only for that, it'd also "hide" your default views, thereby achieving that "quick-and-dirty bootstrap" and "customize as you go".

Note that with rodauth-rails views already hidden, by default Rodauth's internal templates are rendered, until the user decides to run rails generate rodauth:views to copy the views into their app. With this I was hoping to achieve a similar customize-as-you-go approach that Devise has.

A very useful thing for this project would be a "How to migrate from Devise to Rodauth-Rails" page :)

Oh yeah, I haven't considered this, it sounds very useful indeed 👍 Personally I'm not experienced with Devise (I've created rodauth-rails because I didn't feel like learning Devise and then figuring out it's too complex for me 😛), but I will try to write a guide at some point.

[bjeanes]

Note that AFAIK there is no after_rodauth hook at the moment, so after_action couldn't really be implemented.

To actually support the full range of callbacks, there would need to be an around_rodauth. With such a hook point, the implementation to run all callbacks would actually be very straight forward:

around_rodauth do
  rails_controller_instance.run_callbacks(:process_action) do
    yield # or super(), depending on how `around_rodauth` would be defined
  end
end

That would be sufficient plus perhaps disabling the Roda-owned CSRF check or adding skip_before_filter in the Rodauth controller, so that this check isn't doubled up.

I expended some energy trying to do this myself with a little bit too much cleverness, but ultimately was foiled by the Rodauth lifecycle and when the route handlers are actually defined:

methods.grep(/^_handle_/).each do |route_handler|
  define_method(route_handler) do
    rails_controller_instance.run_callbacks(:process_action) do
      super()
    end
  end
end

It may still be possible but I opted for a simpler solution that actually addressed my immediate needs:

before_rodauth do
  super() if defined?(super)
  rails_controller_instance.instance_eval do
    track_session
    set_seo
    set_embedded_origin
    check_http_basic if Config.staging?
  end
end

EDIT: The above wasn't so graceful in the end because a render/redirect didn't interrupt the continuation of Rodauth handling. I managed to work around it, but I think it does make sense to have a closer integration here out-of-the-box.

[janko]

Interesting, thank you for sharing. The problem with implementing an around or after callback like this is that it won't work with redirects or early responses, because Rodauth uses throw for that, which would exit from any around block.

I think the correct way to do this would be to utilize Roda's hooks plugin. I'll see if it's possible to add around hooks there, and then possibly add after_rodauth and around_rodauth hooks that use that. Or maybe it's somehow possible to add them directly to Rodauth 🤔

[janko]

@bjeanes Actually, do you think you could make a proposal for the around_rodauth hook to Jeremy on the Rodauth google group? I realized it would be complex to start from Roda like I suggested, and that throwing will still execute any ensure blocks (which I'm assuming #run_callbacks uses), so it shouldn't actually be difficult to implement them inside Rodauth.

[bjeanes]

Actually, do you think you could make a proposal for the around_rodauth hook to Jeremy on the Rodauth google group?

I had a reminder to come back to this but then work got a bit busy. I will do that today, but first I'm going to have a short play in rodauth code-base to see if I can just implement at least a proof-of-concept for it within my window I have for this today.

[bjeanes]

Here is a proof-of-concept for the around_* hooks as a general purpose utility for any Rodauth feature to use. In the PoC, I switched from before_rodauth to around_rodauth handling.

https://github.com/jeremyevans/rodauth/compare/master...bjeanes:around_rodauth?expand=1

I'll email Jeremy on the mailing list now to discuss whether he'd be interested in moving in this direction.

[bjeanes]

https://groups.google.com/g/rodauth/c/kZfPJdVdQI8

[bjeanes]

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

[janko]

@HoneyryderChuck I'm preparing to release https://github.com/janko/rodauth-rails/commit/2ca331ea94e5daeea910cc2a41c7dc77c7b2c02c, which would run Action Controller action callbacks and rescue handlers around Rodauth actions. Because now CSRF token would be verified automatically as part of running controller action callbacks, I've removed explicit CSRF protection and overrides of #check_csrf and #check_csrf? that I had.

However, I think this can be problematic when integrating rodauth-oauth, because it overrides #check_csrf? to skip CSRF protection in some cases. With this new change that wouldn't work, because now the Rails controller decides when the CSRF token is verified, not the #check_csrf? method, so by default CSRF token would always be verified.

I'm not sure exactly how to solve this problem, I thought you might have ideas. My only idea was adding something like:

skip_before_action :verify_authenticity_token, unless: -> { rodauth.check_csrf? }

to the Rodauth controller, but the problem is this won't be possible for existing users of rodauth-rails, as their Rodauth controller has already been generated. That problem probably wouldn't exist if rodauth-rails was an engine like you initially suggested, though I still think that would bring its own challenges. Maybe there is another solution I'm not seeing.

[bjeanes]

@janko What about calling rails_controller_instance.skip_forgery_protection if check_csrf? is true. That won't require a modification from the controller. The downside is its hidden away and for a first-timer looking at it may seem backwards: "skip csrf if check_csrf?" will seem inverted. That could be mitigated by good naming I suppose...

[bjeanes]

I'll also note that performing CSRF protection twice may not be horrendous in most cases (personally, I emit a bunch of metrics that woudl be doubled up so it would be PITA, but I think for most it'll just be the wasted clock time)

[HoneyryderChuck]

@janko thanks for the heads-up! This would have gone under my radar otherwise.

That problem probably wouldn't exist if rodauth-rails was an engine like you initially suggested,...

I guess that one came back to haunt you :) eheh . For the record, although I still think an engine would be more appropriate, this was just one of the reasons.

I don't think one can do much about already existing controllers, besides documentation and deprecation warnings. Would a rodauth controlller callback be able to check for present of both rodauth-oauth and lack of a verify_authenticity_token declaration for RodauthController suffice? Because I'm assuming that existing applications have this optionn turned off, so you want them to turn it on.

However, when turning it on, maybe it's best to had a filter for skipping it for JSON requests? Something in the lines of:

skip_before_action :verify_authenticity_token, unless: -> { request.accepts_json? }

I guess you'd want this in the generator altogether, to make using rodauth-rails in JSON mode work out of the box?

[janko]

@bjeanes I like this idea. I wasn't sure whether CSRF token verification can be skipped at the controller instance level, but the allow_forgery_protection boolean attribute works on both class and instance level. So, if we add back #check_csrf and #check_csrf? overrides, then we could have Rodauth call into #verify_authenticity_token manually like before, and then when running the callbacks we could disable CSRF protection:

def rails_controller_callbacks
  rails_controller_instance.allow_forgery_protection = false
  rails_controller_instance.run_callbacks(:process_action) do
    yield
  end
end

I think this should work nicely, because Rodauth calls #check_csrf before the new #_around_rodauth. The #check_csrf? should already default to false for JSON requests, as that's the case for Rodauth's jwt feature.

[bjeanes]

Makes sense to me!