Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Integrate with strong_parameters #260

Merged
merged 3 commits into from

7 participants

Taavo Smith Josemar Luedke Brendon Muir Dan Arnfield Ilya Vorontsov Vitaly Kushner Joel Moss
Taavo Smith

See also #236

We now check for a method permitted_params in build_resource_params, and use it instead of params if present. We're expecting permitted_params to be implemented using strong_parameters' permit method, like this:

def permitted_params
  params.permit(:widget => [:permitted_field, :other_permitted_field]) 
end

Note that this doesn't work if you use strong_parameters' require method, because whereas the above example returns a cleaned copy of params, params.require(:widget).permit(:permitted_field, :other_permitted_field) returns a cleaned copy of params[:widget].

Joel Moss joelmoss merged commit 995e282 into from
Brendon Muir

So weird how something like this is merged just when I needed it! :)

It might pay to add in the README that the permitted_params method should be protected and not private as otherwise it won't be found. The strong_parameters gem recommends a pattern whereby the allowed parameters are built in a private method.

Alternatively the check for the method could include the private space?

Dan Arnfield

Actually, it doesn't seem to work even when permitted_params is only protected. It isn't being called by IR in my app unless I move it to public.

Brendon Muir

You're correct @danbob. I just checked my code and my permitted_params is in the public space too. I must have figured that out, changed it, and forgot about my comment here.

Wait, actually, I've noticed that I have it in the public space in my Active Admin controller block, but in my main app I was able to put it in the protected space and it works fine. You don't happen to be using Active Admin?

Dan Arnfield

Nope, just IR and CanCan. I second the suggestion to allow it to live in private or at least protected, so I can keep all my non-action controller methods separate.

Brendon Muir

That's fairly strange. But yes, it seems more logical to allow for the recommended SP technique.

Ilya Vorontsov

Are there any reasons to use strong parameters without #require? May be this behavior should be fixed while not so much people use inherited_resources with rails 4?
I think it can be done simultaneously with sticking to standard rails 4 #{resource_name}_params method naming because it reduces number of error and simplified integration with such libraries as cancan and others. We can generate to methods: #permitted_params which works in exactly the same way it works now and #{resource_name}_params which overrides #permitted_params and allows params#require to be used.

Taavo Smith

Without #require, strong parameters still prohibits fields other than those marked by #permit, which means someone can update User#email without being able to update User#is_admin, which is most of the point of strong_parameters.

I still believe that #require should throw an error if the indicated parameter isn't present, but not drill down, but that's an issue for the strong_parameters team. It looks like this came up already, and someone may have come up with a plausible solution, but it's since been reverted.

Taavo Smith taavo deleted the branch
Ilya Vorontsov

@taavo I only say that it's simplier to allow one use both methods and use strong parameters exactly the same way they do it usually. When you refactor code to use inherited_resources - it's too simple to forget changing allowed parameters to use permitted_resource instead of, say user_params and remove #permit call in preference of deeper nesting which was working before. I just can't understand which profits this way of handling gives us comparing to default rails 4 way of handling params. The only benefit I can see - is that this method named the same for all controllers, but we can do both methods: #user_params (defined by user) and #permitted_params (defined and used in the gem) which will just delegate to #user_params if this method exists or fallback to params[#{resource_name}] (or probably raise an exception) if not.
My first concern here is compatibility of code. Because user won't need to change anything in rails4 scaffold code and in old code using inherited_resources too (permitted_params is just already redefined by the user and thus not trying to fallback to user_params). Also cancan and other libraries can rely on rails4 naming when trying to work with strong parameters.
I'll send a pull request soon to explain my idea with code more explicitly.

Ilya Vorontsov

@taavo Sent a pull request: #286

Vitaly Kushner

@joelmoss @taavo How is this better then just overriding resource_params in your controller? it doesn't do anything useful when we use strong_parameters as we don't need roles in this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
10 README.rdoc
View
@@ -539,6 +539,16 @@ And then you can rewrite the last example as:
end
end
+== Strong Parameters
+
+If your controller defines a method named permitted_params, Inherited Resources will call it where it would normally call params. This allows for easy integration with the strong_parameters gem:
+
+ def permitted_params
+ params.permit(:widget => [:permitted_field, :other_permitted_field])
+ end
+
+Note that this doesn't work if you use strong_parameters' require method instead of permit, because whereas permit returns the entire sanitized parameter hash, require returns only the sanitized params below the parameter you required.
+
== Bugs and Feedback
If you discover any bugs, please describe it in the issues tracker, including Rails and Inherited Resources versions.
3  lib/inherited_resources/base_helpers.rb
View
@@ -305,7 +305,8 @@ def resource_params
# extract attributes from params
def build_resource_params
- rparams = [params[resource_request_name] || params[resource_instance_name] || {}]
+ parameters = respond_to?(:permitted_params) ? permitted_params : params
+ rparams = [parameters[resource_request_name] || parameters[resource_instance_name] || {}]
if without_protection_given?
rparams << without_protection
else
34 test/strong_parameters_test.rb
View
@@ -0,0 +1,34 @@
+require File.expand_path('test_helper', File.dirname(__FILE__))
+
+class Widget
+ extend ActiveModel::Naming
+end
+
+class WidgetsController < InheritedResources::Base
+end
+
+class StrongParametersTest < ActionController::TestCase
+ def setup
+ @controller = WidgetsController.new
+ @controller.stubs(:widget_url).returns("/")
+ @controller.stubs(:permitted_params).returns(:widget => {:permitted => 'param'})
+ end
+
+ def test_permitted_params_from_new
+ Widget.expects(:new).with(:permitted => 'param')
+ get :new, :widget => { :permitted => 'param', :prohibited => 'param' }
+ end
+
+ def test_permitted_params_from_create
+ Widget.expects(:new).with(:permitted => 'param').returns(mock(:save => true))
+ post :create, :widget => { :permitted => 'param', :prohibited => 'param' }
+ end
+
+ def test_permitted_params_from_update
+ mock_widget = mock
+ mock_widget.stubs(:class).returns(Widget)
+ mock_widget.expects(:update_attributes).with(:permitted => 'param')
+ Widget.expects(:find).with('42').returns(mock_widget)
+ put :update, :id => '42', :widget => {:permitted => 'param', :prohibited => 'param'}
+ end
+end
0  test/views/widgets/new.html.erb
View
No changes.
Something went wrong with that request. Please try again.