diff --git a/activemodel/lib/active_model.rb b/activemodel/lib/active_model.rb index 026430fee3..5ed21a39c2 100644 --- a/activemodel/lib/active_model.rb +++ b/activemodel/lib/active_model.rb @@ -38,6 +38,7 @@ module ActiveModel autoload :EachValidator, 'active_model/validator' autoload :Errors autoload :Lint + autoload :MassAssignmentSecurity autoload :Name, 'active_model/naming' autoload :Naming autoload :Observer, 'active_model/observing' diff --git a/activerecord/lib/active_record/mass_assignment_security.rb b/activemodel/lib/active_model/mass_assignment_security.rb similarity index 94% rename from activerecord/lib/active_record/mass_assignment_security.rb rename to activemodel/lib/active_model/mass_assignment_security.rb index 8f4d6e1c74..c0549ba6c0 100644 --- a/activerecord/lib/active_record/mass_assignment_security.rb +++ b/activemodel/lib/active_model/mass_assignment_security.rb @@ -1,6 +1,7 @@ -require 'active_record/mass_assignment_security/permission_set' +require 'active_support/core_ext/class/attribute.rb' +require 'active_model/mass_assignment_security/permission_set' -module ActiveRecord +module ActiveModel # = Active Record Mass-Assignment Security module MassAssignmentSecurity extend ActiveSupport::Concern @@ -112,11 +113,13 @@ module ActiveRecord end def protected_attributes - self._protected_attributes ||= BlackList.new(attributes_protected_by_default).tap { |w| w.logger = logger } + self._protected_attributes ||= BlackList.new(attributes_protected_by_default).tap do |w| + w.logger = self.logger if self.respond_to?(:logger) + end end def accessible_attributes - self._accessible_attributes ||= WhiteList.new.tap { |w| w.logger = logger } + self._accessible_attributes ||= WhiteList.new.tap { |w| w.logger = self.logger if self.respond_to?(:logger) } end def active_authorizer diff --git a/activerecord/lib/active_record/mass_assignment_security/permission_set.rb b/activemodel/lib/active_model/mass_assignment_security/permission_set.rb similarity index 88% rename from activerecord/lib/active_record/mass_assignment_security/permission_set.rb rename to activemodel/lib/active_model/mass_assignment_security/permission_set.rb index 8446a4103b..978da493d7 100644 --- a/activerecord/lib/active_record/mass_assignment_security/permission_set.rb +++ b/activemodel/lib/active_model/mass_assignment_security/permission_set.rb @@ -1,6 +1,6 @@ -require 'active_record/mass_assignment_security/sanitizer' +require 'active_model/mass_assignment_security/sanitizer' -module ActiveRecord +module ActiveModel module MassAssignmentSecurity class PermissionSet < Set diff --git a/activerecord/lib/active_record/mass_assignment_security/sanitizer.rb b/activemodel/lib/active_model/mass_assignment_security/sanitizer.rb similarity index 81% rename from activerecord/lib/active_record/mass_assignment_security/sanitizer.rb rename to activemodel/lib/active_model/mass_assignment_security/sanitizer.rb index 11de35f9d6..275e481fb8 100644 --- a/activerecord/lib/active_record/mass_assignment_security/sanitizer.rb +++ b/activemodel/lib/active_model/mass_assignment_security/sanitizer.rb @@ -1,4 +1,4 @@ -module ActiveRecord +module ActiveModel module MassAssignmentSecurity module Sanitizer @@ -17,11 +17,11 @@ module ActiveRecord end def debug? - logger.present? + self.logger.present? end def warn!(attrs) - logger.debug "WARNING: Can't mass-assign protected attributes: #{attrs.join(', ')}" + self.logger.debug "WARNING: Can't mass-assign protected attributes: #{attrs.join(', ')}" end end diff --git a/activerecord/test/cases/mass_assignment_security/black_list_test.rb b/activemodel/test/cases/mass_assignment_security/black_list_test.rb similarity index 86% rename from activerecord/test/cases/mass_assignment_security/black_list_test.rb rename to activemodel/test/cases/mass_assignment_security/black_list_test.rb index 8b7f48a5f6..ed168bc016 100644 --- a/activerecord/test/cases/mass_assignment_security/black_list_test.rb +++ b/activemodel/test/cases/mass_assignment_security/black_list_test.rb @@ -1,9 +1,9 @@ require "cases/helper" -class BlackListTest < ActiveRecord::TestCase +class BlackListTest < ActiveModel::TestCase def setup - @black_list = ActiveRecord::MassAssignmentSecurity::BlackList.new + @black_list = ActiveModel::MassAssignmentSecurity::BlackList.new @included_key = 'admin' @black_list += [ @included_key ] end diff --git a/activerecord/test/cases/mass_assignment_security/permission_set_test.rb b/activemodel/test/cases/mass_assignment_security/permission_set_test.rb similarity index 85% rename from activerecord/test/cases/mass_assignment_security/permission_set_test.rb rename to activemodel/test/cases/mass_assignment_security/permission_set_test.rb index ca8985042a..d005b638e4 100644 --- a/activerecord/test/cases/mass_assignment_security/permission_set_test.rb +++ b/activemodel/test/cases/mass_assignment_security/permission_set_test.rb @@ -1,9 +1,9 @@ require "cases/helper" -class PermissionSetTest < ActiveRecord::TestCase +class PermissionSetTest < ActiveModel::TestCase def setup - @permission_list = ActiveRecord::MassAssignmentSecurity::PermissionSet.new + @permission_list = ActiveModel::MassAssignmentSecurity::PermissionSet.new end test "+ stringifies added collection values" do diff --git a/activerecord/test/cases/mass_assignment_security/sanitizer_test.rb b/activemodel/test/cases/mass_assignment_security/sanitizer_test.rb similarity index 87% rename from activerecord/test/cases/mass_assignment_security/sanitizer_test.rb rename to activemodel/test/cases/mass_assignment_security/sanitizer_test.rb index 122bc7e114..367207aab3 100644 --- a/activerecord/test/cases/mass_assignment_security/sanitizer_test.rb +++ b/activemodel/test/cases/mass_assignment_security/sanitizer_test.rb @@ -1,9 +1,10 @@ require "cases/helper" +require 'logger' -class SanitizerTest < ActiveRecord::TestCase +class SanitizerTest < ActiveModel::TestCase class SanitizingAuthorizer - include ActiveRecord::MassAssignmentSecurity::Sanitizer + include ActiveModel::MassAssignmentSecurity::Sanitizer attr_accessor :logger diff --git a/activerecord/test/cases/mass_assignment_security/white_list_test.rb b/activemodel/test/cases/mass_assignment_security/white_list_test.rb similarity index 86% rename from activerecord/test/cases/mass_assignment_security/white_list_test.rb rename to activemodel/test/cases/mass_assignment_security/white_list_test.rb index 4601263437..aa3596ad2a 100644 --- a/activerecord/test/cases/mass_assignment_security/white_list_test.rb +++ b/activemodel/test/cases/mass_assignment_security/white_list_test.rb @@ -1,9 +1,9 @@ require "cases/helper" -class WhiteListTest < ActiveRecord::TestCase +class WhiteListTest < ActiveModel::TestCase def setup - @white_list = ActiveRecord::MassAssignmentSecurity::WhiteList.new + @white_list = ActiveModel::MassAssignmentSecurity::WhiteList.new @included_key = 'first_name' @white_list += [ @included_key ] end diff --git a/activemodel/test/cases/mass_assignment_security_test.rb b/activemodel/test/cases/mass_assignment_security_test.rb new file mode 100644 index 0000000000..0f7a38b0bc --- /dev/null +++ b/activemodel/test/cases/mass_assignment_security_test.rb @@ -0,0 +1,52 @@ +require "cases/helper" +require 'models/mass_assignment_specific' + +class MassAssignmentSecurityTest < ActiveModel::TestCase + + def test_attribute_protection + user = User.new + expected = { "name" => "John Smith", "email" => "john@smith.com" } + sanitized = user.sanitize_for_mass_assignment(expected.merge("admin" => true)) + assert_equal expected, sanitized + end + + def test_attributes_accessible + user = Person.new + expected = { "name" => "John Smith", "email" => "john@smith.com" } + sanitized = user.sanitize_for_mass_assignment(expected.merge("super_powers" => true)) + assert_equal expected, sanitized + end + + def test_attributes_protected_by_default + firm = Firm.new + expected = { } + sanitized = firm.sanitize_for_mass_assignment({ "type" => "Client" }) + assert_equal expected, sanitized + end + + def test_mass_assignment_protection_inheritance + assert LoosePerson.accessible_attributes.blank? + assert_equal Set.new([ 'credit_rating', 'administrator']), LoosePerson.protected_attributes + + assert LooseDescendant.accessible_attributes.blank? + assert_equal Set.new([ 'credit_rating', 'administrator', 'phone_number']), LooseDescendant.protected_attributes + + assert LooseDescendantSecond.accessible_attributes.blank? + assert_equal Set.new([ 'credit_rating', 'administrator', 'phone_number', 'name']), LooseDescendantSecond.protected_attributes, + 'Running attr_protected twice in one class should merge the protections' + + assert (TightPerson.protected_attributes - TightPerson.attributes_protected_by_default).blank? + assert_equal Set.new([ 'name', 'address' ]), TightPerson.accessible_attributes + + assert (TightDescendant.protected_attributes - TightDescendant.attributes_protected_by_default).blank? + assert_equal Set.new([ 'name', 'address', 'phone_number' ]), TightDescendant.accessible_attributes + end + + def test_mass_assignment_multiparameter_protector + task = Task.new + attributes = { "starting(1i)" => "2004", "starting(2i)" => "6", "starting(3i)" => "24" } + sanitized = task.sanitize_for_mass_assignment(attributes) + assert_equal sanitized, { } + end + +end \ No newline at end of file diff --git a/activemodel/test/models/mass_assignment_specific.rb b/activemodel/test/models/mass_assignment_specific.rb new file mode 100644 index 0000000000..2a8fe170c2 --- /dev/null +++ b/activemodel/test/models/mass_assignment_specific.rb @@ -0,0 +1,57 @@ +class User + include ActiveModel::MassAssignmentSecurity + attr_protected :admin + + public :sanitize_for_mass_assignment +end + +class Person + include ActiveModel::MassAssignmentSecurity + attr_accessible :name, :email + + public :sanitize_for_mass_assignment +end + +class Firm + include ActiveModel::MassAssignmentSecurity + + public :sanitize_for_mass_assignment + + def self.attributes_protected_by_default + ["type"] + end +end + +class Task + include ActiveModel::MassAssignmentSecurity + attr_protected :starting + + public :sanitize_for_mass_assignment +end + +class LoosePerson + include ActiveModel::MassAssignmentSecurity + attr_protected :credit_rating, :administrator +end + +class LooseDescendant < LoosePerson + attr_protected :phone_number +end + +class LooseDescendantSecond< LoosePerson + attr_protected :phone_number + attr_protected :name +end + +class TightPerson + include ActiveModel::MassAssignmentSecurity + attr_accessible :name, :address + + def self.attributes_protected_by_default + ["mobile_number"] + end +end + +class TightDescendant < TightPerson + attr_accessible :phone_number +end \ No newline at end of file diff --git a/activerecord/lib/active_record.rb b/activerecord/lib/active_record.rb index 9ab05a0548..e2f2508ae8 100644 --- a/activerecord/lib/active_record.rb +++ b/activerecord/lib/active_record.rb @@ -64,7 +64,6 @@ 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 56b4ba8260..f22a9de7b1 100644 --- a/activerecord/lib/active_record/base.rb +++ b/activerecord/lib/active_record/base.rb @@ -1797,7 +1797,7 @@ MSG include AttributeMethods::PrimaryKey include AttributeMethods::TimeZoneConversion include AttributeMethods::Dirty - include MassAssignmentSecurity + include ActiveModel::MassAssignmentSecurity include Callbacks, ActiveModel::Observing, Timestamp include Associations, AssociationPreload, NamedScope diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index 2eecb6e344..5a72e9c6e0 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -17,7 +17,7 @@ require 'models/comment' require 'models/minimalistic' require 'models/warehouse_thing' require 'models/parrot' -require 'models/mass_assignment_specific' +require 'models/loose_person' require 'rexml/document' require 'active_support/core_ext/exception' diff --git a/activerecord/test/cases/mass_assignment_security_test.rb b/activerecord/test/cases/mass_assignment_security_test.rb index 07154da93b..025ec1d3fa 100644 --- a/activerecord/test/cases/mass_assignment_security_test.rb +++ b/activerecord/test/cases/mass_assignment_security_test.rb @@ -1,28 +1,11 @@ require "cases/helper" -require 'models/reply' require 'models/company' require 'models/subscriber' require 'models/keyboard' -require 'models/mass_assignment_specific' +require 'models/task' class MassAssignmentSecurityTest < ActiveRecord::TestCase - def test_mass_assignment_protection - firm = Firm.new - firm.attributes = { "name" => "Next Angle", "rating" => 5 } - assert_equal 1, firm.rating - end - - def test_mass_assignment_protection_against_class_attribute_writers - [:logger, :configurations, :primary_key_prefix_type, :table_name_prefix, :table_name_suffix, :pluralize_table_names, - :default_timezone, :schema_format, :lock_optimistically, :record_timestamps].each do |method| - assert_respond_to Task, method - assert_respond_to Task, "#{method}=" - assert_respond_to Task.new, method - assert !Task.new.respond_to?("#{method}=") - end - end - def test_customized_primary_key_remains_protected subscriber = Subscriber.new(:nick => 'webster123', :name => 'nice try') assert_nil subscriber.id @@ -47,50 +30,14 @@ class MassAssignmentSecurityTest < ActiveRecord::TestCase end end - def test_mass_assignment_protection_on_defaults - firm = Firm.new - firm.attributes = { "id" => 5, "type" => "Client" } - assert_nil firm.id - assert_equal "Firm", firm[:type] - end - - def test_mass_assignment_accessible - reply = Reply.new("title" => "hello", "content" => "world", "approved" => true) - reply.save - - assert reply.approved? - - reply.approved = false - reply.save - - assert !reply.approved? - end - - def test_mass_assignment_protection_inheritance - assert LoosePerson.accessible_attributes.blank? - assert_equal Set.new([ 'credit_rating', 'administrator', *LoosePerson.attributes_protected_by_default ]), LoosePerson.protected_attributes - - assert LooseDescendant.accessible_attributes.blank? - assert_equal Set.new([ 'credit_rating', 'administrator', 'phone_number', *LoosePerson.attributes_protected_by_default ]), LooseDescendant.protected_attributes - - assert LooseDescendantSecond.accessible_attributes.blank? - assert_equal Set.new([ 'credit_rating', 'administrator', 'phone_number', 'name', *LoosePerson.attributes_protected_by_default ]), - LooseDescendantSecond.protected_attributes, 'Running attr_protected twice in one class should merge the protections' - - assert (TightPerson.protected_attributes - TightPerson.attributes_protected_by_default).blank? - assert_equal Set.new([ 'name', 'address' ]), TightPerson.accessible_attributes - - assert (TightDescendant.protected_attributes - TightDescendant.attributes_protected_by_default).blank? - assert_equal Set.new([ 'name', 'address', 'phone_number' ]), TightDescendant.accessible_attributes - end - - def test_mass_assignment_multiparameter_protector - task = Task.new - time = Time.mktime(2000, 1, 1, 1) - task.starting = time - attributes = { "starting(1i)" => "2004", "starting(2i)" => "6", "starting(3i)" => "24" } - task.attributes = attributes - assert_equal time, task.starting + def test_protection_against_class_attribute_writers + [:logger, :configurations, :primary_key_prefix_type, :table_name_prefix, :table_name_suffix, :pluralize_table_names, + :default_timezone, :schema_format, :lock_optimistically, :record_timestamps].each do |method| + assert_respond_to Task, method + assert_respond_to Task, "#{method}=" + assert_respond_to Task.new, method + assert !Task.new.respond_to?("#{method}=") + end end end \ No newline at end of file diff --git a/activerecord/test/models/mass_assignment_specific.rb b/activerecord/test/models/loose_person.rb similarity index 73% rename from activerecord/test/models/mass_assignment_specific.rb rename to activerecord/test/models/loose_person.rb index 13a80e0197..256c281d0d 100644 --- a/activerecord/test/models/mass_assignment_specific.rb +++ b/activerecord/test/models/loose_person.rb @@ -1,6 +1,7 @@ class LoosePerson < ActiveRecord::Base self.table_name = 'people' self.abstract_class = true + attr_protected :credit_rating, :administrator end @@ -20,13 +21,4 @@ end class TightDescendant < TightPerson attr_accessible :phone_number -end - -class Task < ActiveRecord::Base - attr_protected :starting -end - -class TopicWithProtectedContent < ActiveRecord::Base - self.table_name = 'topics' - attr_protected :content end \ No newline at end of file