Better interaction with strong parameters

Gemfile

@@ -1,3 +1,3 @@
-source :rubygems
+source 'https://rubygems.org'
 
-gemspec
+gemspec

lib/resources_controller.rb

@@ -22,6 +22,9 @@
 #
 #   class ForumsController < ApplicationController
 #     resources_controller_for :forums
+#     def forum_params
+#       params.fetch('forum',{}).permit(%i(name description ...))
+#     end
 #   end
 #
 # Your controller will get the standard CRUD actions, @forum will be set in member actions, @forums in
@@ -30,9 +33,15 @@
 # ==== Example 2: Specifying enclosing resources
 #   class PostsController < ApplicationController
 #     resources_controller_for :posts, :in => :forum
+#     def resource_params
+#       params.fetch('post',{}).permit(%i(title body ...))
+#     end
 #   end
 #
-# As above, but the controller will load @forum on every action, and use @forum to find and create @posts
+# As above, but the controller will load @forum on every action, and use @forum to find and create @posts.
+# Note also the use of resource_params instead of post_params (named by the resource
+# name); either version works, but one must be defined.  See the discussion of
+# strong parameters below.
 #
 # ==== Wildcard enclosing resources
 # All of the above examples will work for any routes that match what it specified
@@ -76,6 +85,7 @@
 #     resources_controller_for :account, :class => User, :singleton => true do
 #       @current_user
 #     end
+#     def account_params; ...; end
 #   end
 #
 # Your controller will use the block to find the resource.  The @account will be assigned to @current_user
@@ -85,6 +95,7 @@
 #
 #   class PostsController < ApplicationController
 #     resources_controller_for :posts
+#     def post_params; ...; end
 #   end
 #
 # This will now work for /users/2/posts.
@@ -112,6 +123,7 @@
 #
 #   class ImageController < ApplicationController
 #     resources_controller_for :image, :singleton => true
+#     def image_params; ...; end
 #   end
 #
 # When invoked with /users/3/image RC will find @user, and use @user.image to find the resource, and
@@ -159,26 +171,32 @@
 #    map_enclosing_resource :account, :singleton => true, :find => :current_user
 #
 #    def current_user # get it from session or whatnot
+#    end
 #  end
 #
 #  class ForumsController < AplicationController
 #    resources_controller_for :forums
+#    def forum_params; ...; end
 #  end
 #
 #  class PostsController < AplicationController
 #    resources_controller_for :posts
+#    def post_params; ...; end
 #  end
 #
 #  class UsersController < AplicationController
 #    resources_controller_for :users
+#    def user_params; ...; end
 #  end
 #
 #  class ImageController < AplicationController
 #    resources_controller_for :image, :singleton => true
+#    def image_params; ...; end
 #  end
 #
 #  class AccountController < ApplicationController
 #    resources_controller_for :account, :singleton => true, :find => :current_user
+#    def account_params; ...; end
 #  end
 #
 # This is how the app will handle the following routes:
@@ -212,6 +230,42 @@
 #                                       @post = @account.posts.find(3)
 #                                       @post.update_attributes(params[:post])
 #
+# === Strong parameters
+#
+# Note that in all the examples above, the controller must define a method
+# resource_params, or #{resource_name}_params, which will be used to select
+# parameters out of the params fed into the controller by the standard
+# create and update actions.  (This is invoked by a parent resource_params
+# method in lib/resource_controller/resource_methods.rb, which dispatches
+# to whichever available, with #{resource_name}_params preferred if they
+# both are.)
+#
+# The method can be inherited from a base class, not defined by a controller
+# itself.  Thus, if you have several controllers that take a common set of
+# attributes, you can define resource_params in a controller base class and not
+# bother with a bunch of identical resource_params definitions in the
+# definitions of the individual controllers.
+#
+# (It also means that if, for some reason, you want to avoid defining any
+# such methods at all, you can put
+#
+#    def resource_params
+#      params.fetch(resource_name, {}).permit!
+#    end
+#
+# in ApplicationController.  However, this is not recommended for the same
+# reasons permit! (which permits whatever the client supplied) is not
+# recommended -- a hostile user can use browser dev tools to add extra
+# inputs to your forms, and set whatever attributes they like, whether you
+# expected them to or not.  Easy exploits for this are setting 'is_admin' on a
+# User object, or altering the blog_id on a Post to put it on a blog where the
+# user doesn't have publication rights.  And that's just within the context
+# of the sample app above.  A reasonable rule of thumb might be to not give
+# any user access to a controller with permit! unless the same user has
+# access to the production database SQL prompt.  And then remember that a
+# web client with access to your trusted user's credentials might not be
+# your trusted user!)
+#
 # === Views
 #
 # Ok - so how do I write the views?

lib/resources_controller/actions.rb

@@ -121,14 +121,4 @@ def destroy
     end
   end
 
-
-  # Never trust parameters from the scary internet, only allow the white list through.
-  def resource_params
-    if self.respond_to?("#{resource_name}_params", true)
-      return self.send("#{resource_name}_params")
-    else
-      return params.require(resource_name).permit( *(resource_service.content_columns.map(&:name) - [ 'updated_at', 'created_at' ]) )
-    end
-  end
-
 end

lib/resources_controller/resource_methods.rb

@@ -13,12 +13,11 @@ def find_resource(id = nil)
       resource_service.find id
     end
 
-    # makes a new resource, if attributes are not supplied, determine them from the
-    # params hash and the current resource_class, or resource_name (the latter left in for BC)
+    # makes a new resource.  If attributes are not supplied, we try to get them
+    # from 'resource_params', if available.
     def new_resource(attributes = nil, &block)
-      if attributes.blank? && respond_to?(:params) && params.is_a?(ActionController::Parameters)
-        resource_form_name = ActiveModel::Naming.singular(resource_class)
-        attributes = params[resource_form_name] || params[resource_name] || {}
+      if attributes.blank? && respond_to?(:resource_params)
+        attributes = resource_params
       end
       resource_service.new attributes, &block
     end
@@ -34,8 +33,10 @@ def destroy_resource(id = nil)
     def resource_params
       if self.respond_to?("#{resource_name}_params", true)
         return self.send("#{resource_name}_params")
+      elsif defined?(super)
+        return super
       else
-        return params.fetch(resource_name, {}).permit( *(resource_service.content_columns.map(&:name) - [ 'updated_at', 'created_at' ]) )
+        raise NoMethodError, "resource_params and #{resource_name}_params both unimplemented"
       end
     end
 

spec/app.rb

@@ -1,3 +1,4 @@
+# coding: utf-8
 # Testing app setup
 
 module ResourcesControllerTest
@@ -95,6 +96,7 @@ class Application < Rails::Application
 
     create_table :infos, :force => true do |t|
       t.column "user_id", :integer
+      t.column "info", :string
     end
 
     create_table :addresses, :force => true do |t|
@@ -200,16 +202,14 @@ class ApplicationController < ActionController::Base
   def current_user
     @current_user
   end
-
-  def resource_params
-    params
-  end
 end
 
 module Admin
   class ForumsController < ApplicationController
     resources_controller_for :forums
-
+    def resource_params
+      params['forum'].try(:permit, %i(owner_id))
+    end
   end
 
   class InterestsController < ApplicationController
@@ -238,10 +238,16 @@ def account_params
 
 class InfosController < ApplicationController
   resources_controller_for :info, :singleton => true, :only => [:show, :edit, :update]
+  def resource_params
+    params['info'].try(:permit, %i(attr other_attr)) # no real attrs!
+  end
 end
 
 class TagsController < ApplicationController
   resources_controller_for :tags
+  def resource_params
+    params['tag'].try(:permit, :name)
+  end
 end
 
 class UsersController < ApplicationController

spec/controllers/admin_forums_controller_spec.rb

@@ -433,11 +433,11 @@ def do_get
     end
 
     def do_post
-      post :create, params: { :forum => {:name => 'Forum'} }
+      post :create, params: { :forum => {:owner_id => '1'} }
     end
 
     it "should create a new forum" do
-      expect(Forum).to receive(:new).with({'name' => 'Forum'}).and_return(@mock_forum)
+      expect(Forum).to receive(:new).with(ac_permitted(:owner_id => '1')).and_return(@mock_forum)
       do_post
     end
 
@@ -463,11 +463,11 @@ def do_post
     end
 
     def do_post
-      post :create, params: { :forum => {:name => 'Forum'} }, xhr: true
+      post :create, params: { :forum => {:owner_id => '1'} }, xhr: true
     end
 
     it "should create a new forum" do
-      expect(Forum).to receive(:new).with({'name' => 'Forum'}).and_return(@mock_forum)
+      expect(Forum).to receive(:new).with(ac_permitted(:owner_id => '1')).and_return(@mock_forum)
       do_post
     end
 
@@ -492,12 +492,13 @@ def do_post
 
     before(:each) do
       @mock_forum = double('Forum').as_null_object
+      @update_params = { :owner_id => "1" }
       allow(@mock_forum).to receive(:to_param).and_return("1")
       allow(Forum).to receive(:find).and_return(@mock_forum)
     end
 
     def do_update
-      put :update, params: { :id => "1" }
+      put :update, params: { :id => "1", :forum => @update_params }
     end
 
     it "should find the forum requested" do
@@ -511,7 +512,9 @@ def do_update
     end
 
     it "should update the found forum" do
-      expect(@mock_forum).to receive(:update).and_return(true)
+      expected_params = ActionController::Parameters.new(@update_params)
+      expected_params.permit(:owner_id)
+      expect(@mock_forum).to receive(:update).with(expected_params).and_return(true)
       do_update
       expect(assigns(:forum)).to eq(@mock_forum)
     end

spec/controllers/comments_controller_spec.rb

@@ -298,11 +298,11 @@ def do_get
     end
 
     def do_post
-      post :create, params: { :comment => {:name => 'Comment'}, :forum_id => '3', :post_id => '2' }
+      post :create, params: { :comment => {:user_id => '3'}, :forum_id => '3', :post_id => '2' }
     end
 
     it "should build a new comment" do
-      expect(@post_comments).to receive(:build).with({'name' => 'Comment'}).and_return(@comment)
+      expect(@post_comments).to receive(:build).with(ac_permitted('user_id' => '3')).and_return(@comment)
       do_post
     end
 

spec/controllers/forums_controller_spec.rb

@@ -204,12 +204,12 @@ def do_get
     end
 
     def do_post
-      post :create, params: { :forum => {:name => 'Forum'}, :resource_path => '/forums', :resource_method => :post }
+      post :create, params: { :forum => {:title => 'Forum'}, :resource_path => '/forums', :resource_method => :post }
 
     end
 
     it "should create a new forum" do
-      expect(Forum).to receive(:new).with({'name' => 'Forum'}).and_return(@mock_forum)
+      expect(Forum).to receive(:new).with(ac_permitted('title' => 'Forum')).and_return(@mock_forum)
       do_post
     end
 
@@ -490,11 +490,11 @@ def do_get
     end
 
     def do_post
-      post :create, params: { :forum => {:name => 'Forum'} }
+      post :create, params: { :forum => {:title => 'Forum'} }
     end
 
     it "should create a new forum" do
-      expect(Forum).to receive(:new).with({'name' => 'Forum'}).and_return(@mock_forum)
+      expect(Forum).to receive(:new).with(ac_permitted('title' => 'Forum')).and_return(@mock_forum)
       do_post
     end
 
@@ -520,11 +520,11 @@ def do_post
     end
 
     def do_post
-      post :create, params: { :forum => {:name => 'Forum'} }, xhr: true
+      post :create, params: { :forum => {:title => 'Forum'} }, xhr: true
     end
 
     it "should create a new forum" do
-      expect(Forum).to receive(:new).with({'name' => 'Forum'}).and_return(@mock_forum)
+      expect(Forum).to receive(:new).with(ac_permitted('title' => 'Forum')).and_return(@mock_forum)
       do_post
     end
 

spec/controllers/tags_controller_via_forum_spec.rb

@@ -151,7 +151,7 @@ def do_get
     end
 
     it "should build a new tag with params" do
-      expect(@forum_tags).to receive(:build).with("name" => "hello").and_return(@tag)
+      expect(@forum_tags).to receive(:build).with(ac_permitted("name" => "hello")).and_return(@tag)
       do_get
     end
 

spec/spec_helper.rb

@@ -9,11 +9,18 @@
 # just about every test fail.
 ActionController::Base.view_paths = [File.join(File.dirname(__FILE__), "app/views")]
 
+module SpecConveniences
+  def ac_permitted(hsh)
+    ActionController::Parameters.new(hsh).permit!
+  end
+end
+
 RSpec.configure do |config|
   config.use_transactional_fixtures = true
   config.use_instantiated_fixtures  = false
   config.infer_spec_type_from_file_location!
   config.expect_with(:rspec) { |c| c.syntax = [ :should, :expect ] }
+  config.include SpecConveniences # defined above
 end
 
 require 'rails-controller-testing'