Raise ProtectedAttributeAssignmentError in development and test environments when mass-assigning to an attr_protected attribute. Closes #9699.

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@7777 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

activerecord/CHANGELOG

@@ -1,5 +1,7 @@
 *SVN*
 
+* Raise ProtectedAttributeAssignmentError in development and test environments when mass-assigning to an attr_protected attribute.  #9699 [Henrik N]
+
 * MySQL: speedup date/time parsing.  [Jeremy Kemper]
 
 * Fix calling .clear on a has_many :dependent=>:delete_all association. [tarmo]

activerecord/lib/active_record/base.rb

@@ -35,10 +35,11 @@ module ActiveRecord #:nodoc:
   end
   class Rollback < ActiveRecordError #:nodoc:
   end
-  
+  class ProtectedAttributeAssignmentError < ActiveRecordError #:nodoc:
+  end
   class DangerousAttributeError < ActiveRecordError #:nodoc:
   end
-  
+
   # Raised when you've tried to access a column, which wasn't
   # loaded by your finder.  Typically this is because :select
   # has been specified
@@ -327,13 +328,13 @@ module ActiveRecord #:nodoc:
     cattr_accessor :table_name_suffix, :instance_writer => false
     @@table_name_suffix = ""
 
-    # Indicates whether or not table names should be the pluralized versions of the corresponding class names.
+    # Indicates whether table names should be the pluralized versions of the corresponding class names.
     # If true, the default table name for a +Product+ class will be +products+. If false, it would just be +product+.
     # See table_name for the full rules on table/class naming. This is true, by default.
     cattr_accessor :pluralize_table_names, :instance_writer => false
     @@pluralize_table_names = true
 
-    # Determines whether or not to use ANSI codes to colorize the logging statements committed by the connection adapter. These colors
+    # Determines whether to use ANSI codes to colorize the logging statements committed by the connection adapter. These colors
     # make it much easier to overview things during debugging (when used through a reader like +tail+ and on a black background), but
     # may complicate matters if you use software like syslog. This is true, by default.
     cattr_accessor :colorize_logging, :instance_writer => false
@@ -344,7 +345,7 @@ module ActiveRecord #:nodoc:
     cattr_accessor :default_timezone, :instance_writer => false
     @@default_timezone = :local
 
-    # Determines whether or not to use a connection for each thread, or a single shared connection for all threads.
+    # Determines whether to use a connection for each thread, or a single shared connection for all threads.
     # Defaults to false. Set to true if you're writing a threaded application.
     cattr_accessor :allow_concurrency, :instance_writer => false
     @@allow_concurrency = false
@@ -358,6 +359,11 @@ module ActiveRecord #:nodoc:
     cattr_accessor :schema_format , :instance_writer => false
     @@schema_format = :ruby
 
+    # Determines whether to raise an exception on mass-assignment to protected
+    # attribute. Defaults to true.
+    cattr_accessor :whiny_protected_attributes, :instance_writer => false
+    @@whiny_protected_attributes = true
+
     class << self # Class methods
       # Find operates with three different retrieval approaches:
       #
@@ -2022,17 +2028,31 @@ module ActiveRecord #:nodoc:
       end
 
       def remove_attributes_protected_from_mass_assignment(attributes)
-        if self.class.accessible_attributes.nil? && self.class.protected_attributes.nil?
-          attributes.reject { |key, value| attributes_protected_by_default.include?(key.gsub(/\(.+/, "")) }
-        elsif self.class.protected_attributes.nil?
-          attributes.reject { |key, value| !self.class.accessible_attributes.include?(key.gsub(/\(.+/, "").intern) || attributes_protected_by_default.include?(key.gsub(/\(.+/, "")) }
-        elsif self.class.accessible_attributes.nil?
-          attributes.reject { |key, value| self.class.protected_attributes.include?(key.gsub(/\(.+/,"").intern) || attributes_protected_by_default.include?(key.gsub(/\(.+/, "")) }
-        else
-          raise "Declare either attr_protected or attr_accessible for #{self.class}, but not both."
+        safe_attributes =
+          if self.class.accessible_attributes.nil? && self.class.protected_attributes.nil?
+            attributes.reject { |key, value| attributes_protected_by_default.include?(key.gsub(/\(.+/, "")) }
+          elsif self.class.protected_attributes.nil?
+            attributes.reject { |key, value| !self.class.accessible_attributes.include?(key.gsub(/\(.+/, "").intern) || attributes_protected_by_default.include?(key.gsub(/\(.+/, "")) }
+          elsif self.class.accessible_attributes.nil?
+            attributes.reject { |key, value| self.class.protected_attributes.include?(key.gsub(/\(.+/,"").intern) || attributes_protected_by_default.include?(key.gsub(/\(.+/, "")) }
+          else
+            raise "Declare either attr_protected or attr_accessible for #{self.class}, but not both."
+          end
+
+        removed_attributes = attributes.keys - safe_attributes.keys
+
+        if removed_attributes.any?
+          error_message = "Can't mass-assign these protected attributes: #{removed_attributes.join(', ')}"
+          if self.class.whiny_protected_attributes
+            raise ProtectedAttributeAssignmentError, error_message
+          else
+            logger.error error_message
+          end
         end
+
+        safe_attributes
       end
-      
+
       # Removes attributes which have been marked as readonly.
       def remove_readonly_attributes(attributes)
         unless self.class.readonly_attributes.nil?

activerecord/test/base_test.rb

@@ -67,6 +67,13 @@ end
 class BasicsTest < Test::Unit::TestCase
   fixtures :topics, :companies, :developers, :projects, :computers, :accounts, :minimalistics
 
+  # whiny_protected_attributes is turned off since several tests were
+  # not written with it in mind, and would otherwise raise exceptions
+  # as an irrelevant side-effect.
+  def setup
+    ActiveRecord::Base.whiny_protected_attributes = false
+  end
+
   def test_table_exists
     assert !NonExistentTable.table_exists?
     assert Topic.table_exists?
@@ -860,6 +867,17 @@ class BasicsTest < Test::Unit::TestCase
     assert_equal [ :name, :address, :phone_number  ], TightDescendant.accessible_attributes
   end
   
+  def test_whiny_protected_attributes
+    ActiveRecord::Base.whiny_protected_attributes = true
+    assert_raise(ActiveRecord::ProtectedAttributeAssignmentError) do
+      LoosePerson.create!(:administrator => true)
+    end
+    ActiveRecord::Base.whiny_protected_attributes = false
+    assert_nothing_raised do
+      LoosePerson.create!(:administrator => true)
+    end
+  end
+
   def test_readonly_attributes
     assert_equal [ :title ], ReadonlyTitlePost.readonly_attributes
     

railties/environments/production.rb

@@ -16,3 +16,6 @@ config.action_controller.perform_caching             = true
 
 # Disable delivery errors, bad email addresses will be ignored
 # config.action_mailer.raise_delivery_errors = false
+
+# Disable raising errors when mass-assigning to a protected attribute
+config.whiny_protected_attributes = false

railties/lib/initializer.rb

@@ -72,6 +72,7 @@ module Rails
     # * #initialize_framework_views
     # * #initialize_dependency_mechanism
     # * #initialize_whiny_nils
+    # * #initialize_whiny_protected_attributes
     # * #initialize_temporary_directories
     # * #initialize_framework_settings
     # * #add_support_load_paths
@@ -95,6 +96,7 @@ module Rails
       initialize_framework_views
       initialize_dependency_mechanism
       initialize_whiny_nils
+      initialize_whiny_protected_attributes
       initialize_temporary_directories
       initialize_framework_settings
 
@@ -298,6 +300,12 @@ module Rails
     def initialize_whiny_nils
       require('active_support/whiny_nil') if configuration.whiny_nils
     end
+    
+    # Sets +ActiveRecord::Base#whiny_protected_attributes+ which determines whether to
+    # raise on mass-assigning attributes protected with +attr_protected+/+attr_accessible+.
+    def initialize_whiny_protected_attributes
+      ActiveRecord::Base.whiny_protected_attributes = configuration.whiny_protected_attributes
+    end
 
     def initialize_temporary_directories
       if configuration.frameworks.include?(:action_controller)
@@ -423,6 +431,11 @@ module Rails
     # Set to +true+ if you want to be warned (noisily) when you try to invoke
     # any method of +nil+. Set to +false+ for the standard Ruby behavior.
     attr_accessor :whiny_nils
+    
+    # The default value of +true+ means an exception will be raised on attempts
+    # to mass-assign to protected attributes. Set to +false+ to discard them
+    # without raising (an error will be logged instead).
+    attr_accessor :whiny_protected_attributes
 
     # The list of plugins to load. If this is set to <tt>nil</tt>, all plugins will
     # be loaded. If this is set to <tt>[]</tt>, no plugins will be loaded. Otherwise,
@@ -471,6 +484,7 @@ module Rails
       self.controller_paths             = default_controller_paths
       self.cache_classes                = default_cache_classes
       self.whiny_nils                   = default_whiny_nils
+      self.whiny_protected_attributes   = default_whiny_protected_attributes
       self.plugins                      = default_plugins
       self.plugin_paths                 = default_plugin_paths
       self.plugin_locators              = default_plugin_locators
@@ -630,6 +644,10 @@ module Rails
       def default_whiny_nils
         false
       end
+      
+      def default_whiny_protected_attributes
+        true
+      end
 
       def default_plugins
         nil