minor changes to mass assignment security patch to bring it in line with rails standards

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

4 files changed, 99 insertions, 118 deletions

diff --git a/activerecord/lib/active_record/base.rb b/activerecord/lib/active_record/base.rb
index affb1fee3c..56b4ba8260 100644
--- a/activerecord/lib/active_record/base.rb
+++ b/activerecord/lib/active_record/base.rb
@@ -1480,7 +1480,7 @@ MSG
         attributes = new_attributes.stringify_keys
 
         multi_parameter_attributes = []
-        attributes = remove_attributes_protected_from_mass_assignment(attributes) if guard_protected_attributes
+        attributes = sanitize_for_mass_assignment(attributes) if guard_protected_attributes
 
         attributes.each do |k, v|
           if k.include?("(")
@@ -1797,7 +1797,7 @@ MSG
     include AttributeMethods::PrimaryKey
     include AttributeMethods::TimeZoneConversion
     include AttributeMethods::Dirty
-    extend MassAssignmentSecurity
+    include 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
index 07beb6405e..8f4d6e1c74 100644
--- a/activerecord/lib/active_record/mass_assignment_security.rb
+++ b/activerecord/lib/active_record/mass_assignment_security.rb
@@ -1,7 +1,16 @@
 require 'active_record/mass_assignment_security/permission_set'
 
 module ActiveRecord
+  # = Active Record Mass-Assignment Security
   module MassAssignmentSecurity
+    extend ActiveSupport::Concern
+
+    included do
+      class_attribute :_accessible_attributes
+      class_attribute :_protected_attributes
+      class_attribute :_active_authorizer
+    end
+
     # 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,
@@ -11,7 +20,7 @@ module ActiveRecord
     # on their role:
     #
     # class AccountsController < ApplicationController
-    #   extend ActiveRecord::MassAssignmentSecurity
+    #   include ActiveRecord::MassAssignmentSecurity
     #
     #   attr_accessible :first_name, :last_name
     #
@@ -28,7 +37,7 @@ module ActiveRecord
     #   protected
     #
     #   def account_params
-    #     remove_attributes_protected_from_mass_assignment(params[:account])
+    #     sanitize_for_mass_assignment(params[:account])
     #   end
     #
     #   def mass_assignment_authorizer
@@ -37,123 +46,96 @@ module ActiveRecord
     #
     # 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
+    module ClassMethods
+      # 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(*names)
+        self._protected_attributes = self.protected_attributes + names
+        self._active_authorizer = self._protected_attributes
+      end
 
-    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(*names)
+        self._accessible_attributes = self.accessible_attributes + names
+        self._active_authorizer = self._accessible_attributes
+      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
+      def protected_attributes
+        self._protected_attributes ||= BlackList.new(attributes_protected_by_default).tap { |w| w.logger = logger }
+      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
+      def accessible_attributes
+        self._accessible_attributes ||= WhiteList.new.tap { |w| w.logger = logger }
+      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)
+      def active_authorizer
+        self._active_authorizer ||= protected_attributes
       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)
+      def attributes_protected_by_default
+        []
       end
     end
 
-    def mass_assignment_authorizer
-      protected_attributes
-    end
+    protected
 
-    private
+      def sanitize_for_mass_assignment(attributes)
+        mass_assignment_authorizer.sanitize(attributes)
+      end
 
-      # 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
+      def mass_assignment_authorizer
+        self.class.active_authorizer
       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
index 1d34dce02e..8446a4103b 100644
--- a/activerecord/lib/active_record/mass_assignment_security/permission_set.rb
+++ b/activerecord/lib/active_record/mass_assignment_security/permission_set.rb
@@ -2,11 +2,11 @@ require 'active_record/mass_assignment_security/sanitizer'
 
 module ActiveRecord
   module MassAssignmentSecurity
-    class PermissionSet < Set
 
+    class PermissionSet < Set
       attr_accessor :logger
 
-      def merge(values)
+      def +(values)
         super(values.map(&:to_s))
       end
 
@@ -19,7 +19,6 @@ module ActiveRecord
         def remove_multiparameter_id(key)
           key.gsub(/\(.+/, '')
         end
-
     end
 
     class WhiteList < PermissionSet
@@ -28,7 +27,6 @@ module ActiveRecord
       def deny?(key)
         !include?(key)
       end
-
     end
 
     class BlackList < PermissionSet
@@ -37,7 +35,6 @@ module ActiveRecord
       def deny?(key)
         include?(key)
       end
-
     end
 
   end
diff --git a/activerecord/lib/active_record/mass_assignment_security/sanitizer.rb b/activerecord/lib/active_record/mass_assignment_security/sanitizer.rb
index 4a099a147c..11de35f9d6 100644
--- a/activerecord/lib/active_record/mass_assignment_security/sanitizer.rb
+++ b/activerecord/lib/active_record/mass_assignment_security/sanitizer.rb
@@ -13,15 +13,17 @@ module ActiveRecord
 
         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
+          warn!(removed_keys) if removed_keys.any?
         end
 
         def debug?
           logger.present?
         end
 
+        def warn!(attrs)
+          logger.debug "WARNING: Can't mass-assign protected attributes: #{attrs.join(', ')}"
+        end
+
     end
   end
 end