Mass assignment security refactoring

Signed-off-by: José Valim <jose.valim@gmail.com>

5 files changed, 243 insertions, 133 deletions

diff --git a/activerecord/lib/active_record.rb b/activerecord/lib/active_record.rb
index e2f2508ae8..9ab05a0548 100644
--- a/activerecord/lib/active_record.rb
+++ b/activerecord/lib/active_record.rb
@@ -64,6 +64,7 @@ module ActiveRecord
     autoload :CounterCache
     autoload :DynamicFinderMatch
     autoload :DynamicScopeMatch
+    autoload :MassAssignmentSecurity
     autoload :Migration
     autoload :Migrator, 'active_record/migration'
     autoload :NamedScope
diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
index 3f1015dd4b..affb1fee3c 100644
--- a/activerecord/lib/active_record/base.rb
+++ b/activerecord/lib/active_record/base.rb
@@ -24,7 +24,7 @@ require 'active_record/errors'
 require 'active_record/log_subscriber'
 
 module ActiveRecord #:nodoc:
-  # = Active Record 
+  # = Active Record
   #
   # Active Record objects don't specify their attributes directly, but rather infer them from the table definition with
   # which they're linked. Adding, removing, and changing attributes and their type is done directly in the database. Any change
@@ -476,112 +476,16 @@ module ActiveRecord #:nodoc:
         connection.select_value(sql, "#{name} Count").to_i
       end
 
-      # Attributes named in this macro are protected from mass-assignment,
-      # such as <tt>new(attributes)</tt>,
-      # <tt>update_attributes(attributes)</tt>, or
-      # <tt>attributes=(attributes)</tt>.
-      #
-      # Mass-assignment to these attributes will simply be ignored, to assign
-      # to them you can use direct writer methods. This is meant to protect
-      # sensitive attributes from being overwritten by malicious users
-      # tampering with URLs or forms.
-      #
-      #   class Customer < ActiveRecord::Base
-      #     attr_protected :credit_rating
-      #   end
-      #
-      #   customer = Customer.new("name" => David, "credit_rating" => "Excellent")
-      #   customer.credit_rating # => nil
-      #   customer.attributes = { "description" => "Jolly fellow", "credit_rating" => "Superb" }
-      #   customer.credit_rating # => nil
-      #
-      #   customer.credit_rating = "Average"
-      #   customer.credit_rating # => "Average"
-      #
-      # To start from an all-closed default and enable attributes as needed,
-      # have a look at +attr_accessible+.
-      #
-      # If the access logic of your application is richer you can use <tt>Hash#except</tt>
-      # or <tt>Hash#slice</tt> to sanitize the hash of parameters before they are
-      # passed to Active Record.
-      #
-      # For example, it could be the case that the list of protected attributes
-      # for a given model depends on the role of the user:
-      #
-      #   # Assumes plan_id is not protected because it depends on the role.
-      #   params[:account] = params[:account].except(:plan_id) unless admin?
-      #   @account.update_attributes(params[:account])
-      #
-      # Note that +attr_protected+ is still applied to the received hash. Thus,
-      # with this technique you can at most _extend_ the list of protected
-      # attributes for a particular mass-assignment call.
-      def attr_protected(*attributes)
-        write_inheritable_attribute(:attr_protected, Set.new(attributes.map {|a| a.to_s}) + (protected_attributes || []))
-      end
-
-      # Returns an array of all the attributes that have been protected from mass-assignment.
-      def protected_attributes # :nodoc:
-        read_inheritable_attribute(:attr_protected)
-      end
-
-      # Specifies a white list of model attributes that can be set via
-      # mass-assignment, such as <tt>new(attributes)</tt>,
-      # <tt>update_attributes(attributes)</tt>, or
-      # <tt>attributes=(attributes)</tt>
-      #
-      # This is the opposite of the +attr_protected+ macro: Mass-assignment
-      # will only set attributes in this list, to assign to the rest of
-      # attributes you can use direct writer methods. This is meant to protect
-      # sensitive attributes from being overwritten by malicious users
-      # tampering with URLs or forms. If you'd rather start from an all-open
-      # default and restrict attributes as needed, have a look at
-      # +attr_protected+.
-      #
-      #   class Customer < ActiveRecord::Base
-      #     attr_accessible :name, :nickname
-      #   end
-      #
-      #   customer = Customer.new(:name => "David", :nickname => "Dave", :credit_rating => "Excellent")
-      #   customer.credit_rating # => nil
-      #   customer.attributes = { :name => "Jolly fellow", :credit_rating => "Superb" }
-      #   customer.credit_rating # => nil
-      #
-      #   customer.credit_rating = "Average"
-      #   customer.credit_rating # => "Average"
-      #
-      # If the access logic of your application is richer you can use <tt>Hash#except</tt>
-      # or <tt>Hash#slice</tt> to sanitize the hash of parameters before they are
-      # passed to Active Record.
-      #
-      # For example, it could be the case that the list of accessible attributes
-      # for a given model depends on the role of the user:
-      #
-      #   # Assumes plan_id is accessible because it depends on the role.
-      #   params[:account] = params[:account].except(:plan_id) unless admin?
-      #   @account.update_attributes(params[:account])
-      #
-      # Note that +attr_accessible+ is still applied to the received hash. Thus,
-      # with this technique you can at most _narrow_ the list of accessible
-      # attributes for a particular mass-assignment call.
-      def attr_accessible(*attributes)
-        write_inheritable_attribute(:attr_accessible, Set.new(attributes.map(&:to_s)) + (accessible_attributes || []))
+      # Attributes listed as readonly can be set for a new record, but will be ignored in database updates afterwards.
+      def attr_readonly(*attributes)
+        write_inheritable_attribute(:attr_readonly, Set.new(attributes.map(&:to_s)) + (readonly_attributes || []))
       end
 
-      # Returns an array of all the attributes that have been made accessible to mass-assignment.
-      def accessible_attributes # :nodoc:
-        read_inheritable_attribute(:attr_accessible)
+      # Returns an array of all the attributes that have been specified as readonly.
+      def readonly_attributes
+        read_inheritable_attribute(:attr_readonly) || []
       end
 
-       # Attributes listed as readonly can be set for a new record, but will be ignored in database updates afterwards.
-       def attr_readonly(*attributes)
-         write_inheritable_attribute(:attr_readonly, Set.new(attributes.map(&:to_s)) + (readonly_attributes || []))
-       end
-
-       # Returns an array of all the attributes that have been specified as readonly.
-       def readonly_attributes
-         read_inheritable_attribute(:attr_readonly) || []
-       end
-
       # If you have an attribute that needs to be saved to the database as an object, and retrieved as the same object,
       # then specify the name of that attribute using this method and it will be handled automatically.
       # The serialization is done through YAML. If +class_name+ is specified, the serialized object must be of that
@@ -1716,27 +1620,6 @@ MSG
         end
       end
 
-      def remove_attributes_protected_from_mass_assignment(attributes)
-        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(/\(.+/, "")) || attributes_protected_by_default.include?(key.gsub(/\(.+/, "")) }
-          elsif self.class.accessible_attributes.nil?
-            attributes.reject { |key, value| self.class.protected_attributes.include?(key.gsub(/\(.+/,"")) || 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?
-          log_protected_attribute_removal(removed_attributes)
-        end
-
-        safe_attributes
-      end
-
       # Removes attributes which have been marked as readonly.
       def remove_readonly_attributes(attributes)
         unless self.class.readonly_attributes.nil?
@@ -1746,16 +1629,10 @@ MSG
         end
       end
 
-      def log_protected_attribute_removal(*attributes)
-        if logger
-          logger.debug "WARNING: Can't mass-assign these protected attributes: #{attributes.join(', ')}"
-        end
-      end
-
       # The primary key and inheritance column can never be set by mass-assignment for security reasons.
-      def attributes_protected_by_default
-        default = [ self.class.primary_key, self.class.inheritance_column ]
-        default << 'id' unless self.class.primary_key.eql? 'id'
+      def self.attributes_protected_by_default
+        default = [ primary_key, inheritance_column ]
+        default << 'id' unless primary_key.eql? 'id'
         default
       end
 
@@ -1920,6 +1797,7 @@ MSG
     include AttributeMethods::PrimaryKey
     include AttributeMethods::TimeZoneConversion
     include AttributeMethods::Dirty
+    extend MassAssignmentSecurity
     include Callbacks, ActiveModel::Observing, Timestamp
     include Associations, AssociationPreload, NamedScope
 
diff --git a/activerecord/lib/active_record/mass_assignment_security.rb b/activerecord/lib/active_record/mass_assignment_security.rb
new file mode 100644
index 0000000000..07beb6405e
--- /dev/null
+++ b/activerecord/lib/active_record/mass_assignment_security.rb
@@ -0,0 +1,160 @@
+require 'active_record/mass_assignment_security/permission_set'
+
+module ActiveRecord
+  module MassAssignmentSecurity
+    # Mass assignment security provides an interface for protecting attributes
+    # from end-user assignment. For more complex permissions, mass assignment security
+    # may be handled outside the model by extending a non-ActiveRecord class,
+    # such as a controller, with this behavior.
+    #
+    # For example, a logged in user may need to assign additional attributes depending
+    # on their role:
+    #
+    # class AccountsController < ApplicationController
+    #   extend ActiveRecord::MassAssignmentSecurity
+    #
+    #   attr_accessible :first_name, :last_name
+    #
+    #   def self.admin_accessible_attributes
+    #     accessible_attributes + [ :plan_id ]
+    #   end
+    #
+    #   def update
+    #     ...
+    #     @account.update_attributes(account_params)
+    #     ...
+    #   end
+    #
+    #   protected
+    #
+    #   def account_params
+    #     remove_attributes_protected_from_mass_assignment(params[:account])
+    #   end
+    #
+    #   def mass_assignment_authorizer
+    #     admin ? admin_accessible_attributes : super
+    #   end
+    #
+    # end
+    #
+    def self.extended(base)
+      base.send(:include, InstanceMethods)
+    end
+
+    module InstanceMethods
+
+      protected
+
+        def remove_attributes_protected_from_mass_assignment(attributes)
+          mass_assignment_authorizer.sanitize(attributes)
+        end
+
+        def mass_assignment_authorizer
+          self.class.mass_assignment_authorizer
+        end
+
+    end
+
+    # Attributes named in this macro are protected from mass-assignment,
+    # such as <tt>new(attributes)</tt>,
+    # <tt>update_attributes(attributes)</tt>, or
+    # <tt>attributes=(attributes)</tt>.
+    #
+    # Mass-assignment to these attributes will simply be ignored, to assign
+    # to them you can use direct writer methods. This is meant to protect
+    # sensitive attributes from being overwritten by malicious users
+    # tampering with URLs or forms.
+    #
+    #   class Customer < ActiveRecord::Base
+    #     attr_protected :credit_rating
+    #   end
+    #
+    #   customer = Customer.new("name" => David, "credit_rating" => "Excellent")
+    #   customer.credit_rating # => nil
+    #   customer.attributes = { "description" => "Jolly fellow", "credit_rating" => "Superb" }
+    #   customer.credit_rating # => nil
+    #
+    #   customer.credit_rating = "Average"
+    #   customer.credit_rating # => "Average"
+    #
+    # To start from an all-closed default and enable attributes as needed,
+    # have a look at +attr_accessible+.
+    #
+    # Note that using <tt>Hash#except</tt> or <tt>Hash#slice</tt> in place of +attr_protected+
+    # to sanitize attributes won't provide sufficient protection.
+    def attr_protected(*keys)
+      use_authorizer(:protected_attributes)
+      protected_attributes.merge(keys)
+    end
+
+    # Specifies a white list of model attributes that can be set via
+    # mass-assignment, such as <tt>new(attributes)</tt>,
+    # <tt>update_attributes(attributes)</tt>, or
+    # <tt>attributes=(attributes)</tt>
+    #
+    # This is the opposite of the +attr_protected+ macro: Mass-assignment
+    # will only set attributes in this list, to assign to the rest of
+    # attributes you can use direct writer methods. This is meant to protect
+    # sensitive attributes from being overwritten by malicious users
+    # tampering with URLs or forms. If you'd rather start from an all-open
+    # default and restrict attributes as needed, have a look at
+    # +attr_protected+.
+    #
+    #   class Customer < ActiveRecord::Base
+    #     attr_accessible :name, :nickname
+    #   end
+    #
+    #   customer = Customer.new(:name => "David", :nickname => "Dave", :credit_rating => "Excellent")
+    #   customer.credit_rating # => nil
+    #   customer.attributes = { :name => "Jolly fellow", :credit_rating => "Superb" }
+    #   customer.credit_rating # => nil
+    #
+    #   customer.credit_rating = "Average"
+    #   customer.credit_rating # => "Average"
+    #
+    # Note that using <tt>Hash#except</tt> or <tt>Hash#slice</tt> in place of +attr_accessible+
+    # to sanitize attributes won't provide sufficient protection.
+    def attr_accessible(*keys)
+      use_authorizer(:accessible_attributes)
+      accessible_attributes.merge(keys)
+    end
+
+    # Returns an array of all the attributes that have been protected from mass-assignment.
+    def protected_attributes
+      read_inheritable_attribute(:protected_attributes) || begin
+        authorizer = BlackList.new
+        authorizer += attributes_protected_by_default
+        authorizer.logger = logger
+        write_inheritable_attribute(:protected_attributes, authorizer)
+      end
+    end
+
+    # Returns an array of all the attributes that have been made accessible to mass-assignment.
+    def accessible_attributes
+      read_inheritable_attribute(:accessible_attributes) || begin
+        authorizer = WhiteList.new
+        authorizer.logger = logger
+        write_inheritable_attribute(:accessible_attributes, authorizer)
+      end
+    end
+
+    def mass_assignment_authorizer
+      protected_attributes
+    end
+
+    private
+
+      # Sets the active authorizer, (attr_protected or attr_accessible). Subsequent calls
+      # will raise an exception when using a different authorizer_id.
+      def use_authorizer(authorizer_id) # :nodoc:
+        if active_authorizer_id = read_inheritable_attribute(:active_authorizer_id)
+          unless authorizer_id == active_authorizer_id
+            raise("Already using #{active_authorizer_id}, cannot use #{authorizer_id}")
+          end
+        else
+          write_inheritable_attribute(:active_authorizer_id, authorizer_id)
+        end
+      end
+
+  end
+end
diff --git a/activerecord/lib/active_record/mass_assignment_security/permission_set.rb b/activerecord/lib/active_record/mass_assignment_security/permission_set.rb
new file mode 100644
index 0000000000..1d34dce02e
--- /dev/null
+++ b/activerecord/lib/active_record/mass_assignment_security/permission_set.rb
@@ -0,0 +1,44 @@
+require 'active_record/mass_assignment_security/sanitizer'
+
+module ActiveRecord
+  module MassAssignmentSecurity
+    class PermissionSet < Set
+
+      attr_accessor :logger
+
+      def merge(values)
+        super(values.map(&:to_s))
+      end
+
+      def include?(key)
+        super(remove_multiparameter_id(key))
+      end
+
+      protected
+
+        def remove_multiparameter_id(key)
+          key.gsub(/\(.+/, '')
+        end
+
+    end
+
+    class WhiteList < PermissionSet
+      include Sanitizer
+
+      def deny?(key)
+        !include?(key)
+      end
+
+    end
+
+    class BlackList < PermissionSet
+      include Sanitizer
+
+      def deny?(key)
+        include?(key)
+      end
+
+    end
+
+  end
+end
\ No newline at end of file
diff --git a/activerecord/lib/active_record/mass_assignment_security/sanitizer.rb b/activerecord/lib/active_record/mass_assignment_security/sanitizer.rb
new file mode 100644
index 0000000000..4a099a147c
--- /dev/null
+++ b/activerecord/lib/active_record/mass_assignment_security/sanitizer.rb
@@ -0,0 +1,27 @@
+module ActiveRecord
+  module MassAssignmentSecurity
+    module Sanitizer
+
+      # Returns all attributes not denied by the authorizer.
+      def sanitize(attributes)
+        sanitized_attributes = attributes.reject { |key, value| deny?(key) }
+        debug_protected_attribute_removal(attributes, sanitized_attributes) if debug?
+        sanitized_attributes
+      end
+
+      protected
+
+        def debug_protected_attribute_removal(attributes, sanitized_attributes)
+          removed_keys = attributes.keys - sanitized_attributes.keys
+          if removed_keys.any?
+            logger.debug "WARNING: Can't mass-assign protected attributes: #{removed_keys.join(', ')}"
+          end
+        end
+
+        def debug?
+          logger.present?
+        end
+
+    end
+  end
+end