Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Added assign_attributes to Active Record which accepts a mass-assignm…

…ent security scope using the :as option, while also allowing mass-assignment security to be bypassed using :with_protected
  • Loading branch information...
commit a08d04bedfd01cc0a517ccedf74f2ceac70eb28d 1 parent 1054ebd
Josh Kalderimis joshk authored
41 activerecord/lib/active_record/base.rb
View
@@ -1640,10 +1640,49 @@ def attribute_names
# user.is_admin? # => true
def attributes=(new_attributes, guard_protected_attributes = true)
return unless new_attributes.is_a?(Hash)
+ if guard_protected_attributes
+ assign_attributes(new_attributes)
+ else
+ assign_attributes(new_attributes, :without_protection => true)
+ end
+ end
+
+ # Allows you to set all the attributes for a particular mass-assignment
+ # security scope by passing in a hash of attributes with keys matching
+ # the attribute names (which again matches the column names) and the scope
+ # name using the :as option.
+ #
+ # To bypass mass-assignment security you can use the :without_protection => true
+ # option.
+ #
+ # class User < ActiveRecord::Base
+ # attr_accessible :name
+ # attr_accessible :name, :is_admin, :as => :admin
+ # end
+ #
+ # user = User.new
+ # user.assign_attributes({ :name => 'Josh', :is_admin => true })
+ # user.name # => "Josh"
+ # user.is_admin? # => false
+ #
+ # user = User.new
+ # user.assign_attributes({ :name => 'Josh', :is_admin => true }, :as => :admin)
+ # user.name # => "Josh"
+ # user.is_admin? # => true
+ #
+ # user = User.new
+ # user.assign_attributes({ :name => 'Josh', :is_admin => true }, :without_protection => true)
+ # user.name # => "Josh"
+ # user.is_admin? # => true
+ def assign_attributes(new_attributes, options = {})
attributes = new_attributes.stringify_keys
+ scope = options[:as] || :default
multi_parameter_attributes = []
- attributes = sanitize_for_mass_assignment(attributes) if guard_protected_attributes
+
+ unless options[:without_protection]
+ attributes = sanitize_for_mass_assignment(attributes, scope)
+ end
attributes.each do |k, v|
if k.include?("(")
2  activerecord/test/cases/base_test.rb
View
@@ -18,7 +18,7 @@
require 'models/minimalistic'
require 'models/warehouse_thing'
require 'models/parrot'
-require 'models/loose_person'
+require 'models/person'
require 'models/edge'
require 'models/joke'
require 'rexml/document'
71 activerecord/test/cases/mass_assignment_security_test.rb
View
@@ -3,6 +3,7 @@
require 'models/subscriber'
require 'models/keyboard'
require 'models/task'
+require 'models/person'
class MassAssignmentSecurityTest < ActiveRecord::TestCase
@@ -30,6 +31,66 @@ def test_mass_assigning_invalid_attribute
end
end
+ def test_assign_attributes_uses_default_scope_when_no_scope_is_provided
+ p = LoosePerson.new
+ p.assign_attributes(attributes_hash)
+
+ assert_equal nil, p.id
+ assert_equal 'Josh', p.first_name
+ assert_equal 'male', p.gender
+ assert_equal nil, p.comments
+ end
+
+ def test_assign_attributes_skips_mass_assignment_security_protection_when_without_protection_is_used
+ p = LoosePerson.new
+ p.assign_attributes(attributes_hash, :without_protection => true)
+
+ assert_equal 5, p.id
+ assert_equal 'Josh', p.first_name
+ assert_equal 'male', p.gender
+ assert_equal 'rides a sweet bike', p.comments
+ end
+
+ def test_assign_attributes_with_default_scope_and_attr_protected_attributes
+ p = LoosePerson.new
+ p.assign_attributes(attributes_hash, :as => :default)
+
+ assert_equal nil, p.id
+ assert_equal 'Josh', p.first_name
+ assert_equal 'male', p.gender
+ assert_equal nil, p.comments
+ end
+
+ def test_assign_attributes_with_admin_scope_and_attr_protected_attributes
+ p = LoosePerson.new
+ p.assign_attributes(attributes_hash, :as => :admin)
+
+ assert_equal nil, p.id
+ assert_equal 'Josh', p.first_name
+ assert_equal 'male', p.gender
+ assert_equal 'rides a sweet bike', p.comments
+ end
+
+ def test_assign_attributes_with_default_scope_and_attr_accessible_attributes
+ p = TightPerson.new
+ p.assign_attributes(attributes_hash, :as => :default)
+
+ assert_equal nil, p.id
+ assert_equal 'Josh', p.first_name
+ assert_equal 'male', p.gender
+ assert_equal nil, p.comments
+ end
+
+ def test_assign_attributes_with_admin_scope_and_attr_accessible_attributes
+ p = TightPerson.new
+ p.assign_attributes(attributes_hash, :as => :admin)
+
+ assert_equal nil, p.id
+ assert_equal 'Josh', p.first_name
+ assert_equal 'male', p.gender
+ assert_equal 'rides a sweet bike', p.comments
+ end
+
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|
@@ -40,4 +101,14 @@ def test_protection_against_class_attribute_writers
end
end
+ private
+
+ def attributes_hash
+ {
+ :id => 5,
+ :first_name => 'Josh',
+ :gender => 'male',
+ :comments => 'rides a sweet bike'
+ }
+ end
end
2  activerecord/test/cases/persistence_test.rb
View
@@ -12,7 +12,7 @@
require 'models/warehouse_thing'
require 'models/parrot'
require 'models/minivan'
-require 'models/loose_person'
+require 'models/person'
require 'rexml/document'
require 'active_support/core_ext/exception'
24 activerecord/test/models/loose_person.rb
View
@@ -1,24 +0,0 @@
-class LoosePerson < ActiveRecord::Base
- self.table_name = 'people'
- self.abstract_class = true
-
- 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 < ActiveRecord::Base
- self.table_name = 'people'
- attr_accessible :name, :address
-end
-
-class TightDescendant < TightPerson
- attr_accessible :phone_number
-end
19 activerecord/test/models/person.rb
View
@@ -48,3 +48,22 @@ class PersonWithDependentNullifyJobs < ActiveRecord::Base
has_many :references, :foreign_key => :person_id
has_many :jobs, :source => :job, :through => :references, :dependent => :nullify
end
+
+
+class LoosePerson < ActiveRecord::Base
+ self.table_name = 'people'
+ self.abstract_class = true
+
+ attr_protected :comments
+ attr_protected :as => :admin
+end
+
+class LooseDescendant < LoosePerson; end
+
+class TightPerson < ActiveRecord::Base
+ self.table_name = 'people'
+ attr_accessible :first_name, :gender
+ attr_accessible :first_name, :gender, :comments, :as => :admin
+end
+
+class TightDescendant < TightPerson; end

6 comments on commit a08d04b

José Valim
Owner

Sweet! Is deprecating attributes= under discussion? :)

José Valim
Owner

And are we going to extend update_attributes and new to support this as well? :D

Josh Kalderimis

Both good questions. Deprecating attributes= hasn't been discussed, at the very least we should deprecate the guard_protected_attributes argument.

+1 for for extending update_attributes and new to support this.

David Heinemeier Hansson
Owner

-1 on deprecating attributes= (it's a much nicer api when you don't need options), but +1 on deprecating that nasty guard_protected_attributes argument.

I would like to extend this to update_attributes too. Feels completely natural. I guess we have both new/create to think about as well. I'd like to see how the API would look for those.

Josh Kalderimis

I'm more than happy to deprecate guard_protected_attributes

I'll also extend :as to update_attributes.

If its alright with you both I will extend to new and create to show what the api would look like?

David Heinemeier Hansson
Owner

Sure, let's see what it looks like. I'm a little worried that this, somewhat minority, feature is going to creep too far. But let's see what it looks like.

Please sign in to comment.
Something went wrong with that request. Please try again.