varvet
Support new params `expect` method to permit parameters
In Rails 8 the method expect has been introduced to mitigate some issues with the current params.require(:foo).permit(:bar) approach. One issue with the current approach is that if someone sends unexpected data, say POST { foo: "bam" } the Rails app will crash with a NoMethodError because the .permit method doesn't exist on String. The expect method handles this issue and will instead return a proper 400 error.
So, we should support this in upcoming versions of Pundit.
The expect method has a bit of a different syntax unfortunately so we can't just change it. I guess we either we have to make it required > some version, or configurable in Pundit. I haven't looked closely on how to best handle this yet.
Docs: https://api.rubyonrails.org/classes/ActionController/Parameters.html#method-i-expect
This is probably my fastest reply ever.
Luckily we're using it in a very limited capacity, a single call-site: https://github.com/varvet/pundit/blob/54cc64e45a3e4c606f555f05ce740e72c4f00946/lib/pundit/authorization.rb#L232-L238
Pundit should definitely support Rails 8.
Will ponder some more!
So if I understand correctly, behaviour doesn't change in an upgrade to Rails 8. It's just that expect is a new way of doing things.
However, expect has an entirely different set of parameters, so a change would affect permitted_attributes too: https://github.com/varvet/pundit/blob/54cc64e45a3e4c606f555f05ce740e72c4f00946/lib/pundit/authorization.rb#L222-L230
It effectively would reduce to something more like:
params.expect(PolicyFinder.new(record).param_key => policy.public_send(method_name))
... which I don't know if it works, but is indeed rather different and even makes pundit_params_for superfluous.
Two things important to me:
- Backwards-compatibility. We should not break it.
- A new way needs to be battle-tested.
We can achieve backwards-compatibility by adding to the API surface, e.g. def expected_attributes(record, action:) and then updating documentation to recommend the new and shiny.
However, what's important with that approach is that it's battle-tested before we recommend it. At the moment I'm not maintaining client projects with significant Pundit-usage (they don't need authorization), so I'm limited in my ability to battle-test it.
So if I understand correctly, behaviour doesn't change in an upgrade to Rails 8. It's just that expect is a new way of doing things.
Yes. Currently basically all Rails apps crash when you send some unexpected params (if they follow the recommended approach) which has been raised many times in the Rails repo. Now the expect syntax is the recommended style going forward. I don't know if the require(...).permit(...) will be deprecated and removed later or not.
Two things important to me:
- Backwards-compatibility. We should not break it.
For sure!
- A new way needs to be battle-tested.
We can achieve backwards-compatibility by adding to the API surface, e.g.
def expected_attributes(record, action:)and then updating documentation to recommend the new and shiny.
Good idea 👍
However, what's important with that approach is that it's battle-tested before we recommend it. At the moment I'm not maintaining client projects with significant Pundit-usage (they don't need authorization), so I'm limited in my ability to battle-test it.
Sure, but Rails is already recommending it so I think we can too? But yeah we can give the option now and then change any recommendations later :)
I think my main worry is how Pundit integrates with the API, i.e. how we hook up all the helper methods and how they relate to each other.
pundit_params_for(record) exists for some reason, allowing an override. Should we still have this kind of separation for the top-level key in expect too? I'm thinking we could skip it from the start, and add it if it turns out to be needed. We would find this out pretty quickly as we try to use it for real.
... maybe some more thoughts, but it's still a bit too vague in my mind.
Yeah, we don't need to rush anything :) I mainly wanted to open the issue to get the ball rolling a bit.
We will probably experiment with this a bit anyway, and I will get back later when we have some more experience with it. We already do some custom Pundit stuff so I don't think there will be any issues getting this to work in our app without changing pundit :)