Merge pull request #3900 from jfturcot/accessible_wrap_params

ParamsWrapper only wrap the accessible attributes when they were set

3 files changed, 41 insertions, 2 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index 8844d6e6f6..473115dc6d 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -106,6 +106,11 @@
     persistent between requests so if you need to manipulate the environment
     for your test you need to do it before the cookie jar is created.
 
+*   ActionController::ParamsWrapper on ActiveRecord models now only wrap
+    attr_accessible attributes if they were set, if not, only the attributes
+    returned by the class method attribute_names will be wrapped. This fixes
+    the wrapping of nested attributes by adding them to attr_accessible.
+
 ## Rails 3.1.4 (unreleased) ##
 
 *   Allow to use asset_path on named_routes aliasing RailsHelper's
diff --git a/actionpack/lib/action_controller/metal/params_wrapper.rb b/actionpack/lib/action_controller/metal/params_wrapper.rb
index 9dcea86253..5c28a8074f 100644
--- a/actionpack/lib/action_controller/metal/params_wrapper.rb
+++ b/actionpack/lib/action_controller/metal/params_wrapper.rb
@@ -43,6 +43,11 @@ module ActionController
   #       wrap_parameters :person, :include => [:username, :password]
   #     end
   #
+  # On ActiveRecord models with no +:include+ or +:exclude+ option set, 
+  # if attr_accessible is set on that model, it will only wrap the accessible
+  # parameters, else it will only wrap the parameters returned by the class 
+  # method attribute_names.
+  #
   # If you're going to pass the parameters to an +ActiveModel+ object (such as
   # +User.new(params[:user])+), you might consider passing the model class to
   # the method instead. The +ParamsWrapper+ will actually try to determine the
@@ -162,7 +167,9 @@ module ActionController
 
         unless options[:include] || options[:exclude]
           model ||= _default_wrap_model
-          if model.respond_to?(:attribute_names) && model.attribute_names.present?
+          if model.respond_to?(:accessible_attributes) && model.accessible_attributes.present?
+            options[:include] = model.accessible_attributes.to_a
+          elsif model.respond_to?(:attribute_names) && model.attribute_names.present?
             options[:include] = model.attribute_names
           end
         end
diff --git a/actionpack/test/controller/params_wrapper_test.rb b/actionpack/test/controller/params_wrapper_test.rb
index 0102f66dfe..a4c6c08abb 100644
--- a/actionpack/test/controller/params_wrapper_test.rb
+++ b/actionpack/test/controller/params_wrapper_test.rb
@@ -26,7 +26,7 @@ class ParamsWrapperTest < ActionController::TestCase
       self.class.last_parameters = request.params.except(:controller, :action)
       head :ok
     end
-  end
+end
 
   class User; end
   class Person; end
@@ -147,6 +147,7 @@ class ParamsWrapperTest < ActionController::TestCase
   end
 
   def test_derived_wrapped_keys_from_matching_model
+    User.expects(:respond_to?).with(:accessible_attributes).returns(false)
     User.expects(:respond_to?).with(:attribute_names).returns(true)
     User.expects(:attribute_names).twice.returns(["username"])
 
@@ -159,6 +160,7 @@ class ParamsWrapperTest < ActionController::TestCase
 
   def test_derived_wrapped_keys_from_specified_model
     with_default_wrapper_options do
+      Person.expects(:respond_to?).with(:accessible_attributes).returns(false)
       Person.expects(:respond_to?).with(:attribute_names).returns(true)
       Person.expects(:attribute_names).twice.returns(["username"])
 
@@ -169,8 +171,33 @@ class ParamsWrapperTest < ActionController::TestCase
       assert_parameters({ 'username' => 'sikachu', 'title' => 'Developer', 'person' => { 'username' => 'sikachu' }})
     end
   end
+  
+  def test_accessible_wrapped_keys_from_matching_model
+    User.expects(:respond_to?).with(:accessible_attributes).returns(true)
+    User.expects(:accessible_attributes).twice.returns(["username"])
+    
+    with_default_wrapper_options do
+      @request.env['CONTENT_TYPE'] = 'application/json'
+      post :parse, { 'username' => 'sikachu', 'title' => 'Developer' }
+      assert_parameters({ 'username' => 'sikachu', 'title' => 'Developer', 'user' => { 'username' => 'sikachu' }})
+    end
+  end
+  
+  def test_accessible_wrapped_keys_from_specified_model
+    with_default_wrapper_options do
+      Person.expects(:respond_to?).with(:accessible_attributes).returns(true)
+      Person.expects(:accessible_attributes).twice.returns(["username"])
+
+      UsersController.wrap_parameters Person
+
+      @request.env['CONTENT_TYPE'] = 'application/json'
+      post :parse, { 'username' => 'sikachu', 'title' => 'Developer' }
+      assert_parameters({ 'username' => 'sikachu', 'title' => 'Developer', 'person' => { 'username' => 'sikachu' }})
+    end
+  end
 
   def test_not_wrapping_abstract_model
+    User.expects(:respond_to?).with(:accessible_attributes).returns(false)
     User.expects(:respond_to?).with(:attribute_names).returns(true)
     User.expects(:attribute_names).returns([])