Skip to content
Browse files

Revert "AM::MassAssingmentSecurity: improve performance"

It introduces backwards incompatible changes in the API.

This reverts commit 7d1379f.
  • Loading branch information...
1 parent f961ec4 commit eb8f0ddb67440d26eb0e179a0c43df8ea2a53b1e @josevalim josevalim committed Mar 15, 2012
View
34 activemodel/lib/active_model/mass_assignment_security/sanitizer.rb
@@ -3,18 +3,20 @@ module MassAssignmentSecurity
class Sanitizer
# Returns all attributes not denied by the authorizer.
def sanitize(attributes, authorizer)
- attributes.reject do |attr, value|
- if authorizer.deny?(attr)
- process_removed_attribute(attr)
- true
- end
- end
+ sanitized_attributes = attributes.reject { |key, value| authorizer.deny?(key) }
+ debug_protected_attribute_removal(attributes, sanitized_attributes)
+ sanitized_attributes
end
protected
- def process_removed_attribute(attr)
- raise NotImplementedError, "#process_removed_attribute(attr) suppose to be overwritten"
+ def debug_protected_attribute_removal(attributes, sanitized_attributes)
+ removed_keys = attributes.keys - sanitized_attributes.keys
+ process_removed_attributes(removed_keys) if removed_keys.any?
+ end
+
+ def process_removed_attributes(attrs)
+ raise NotImplementedError, "#process_removed_attributes(attrs) suppose to be overwritten"
end
end
@@ -32,8 +34,8 @@ def logger?
@target.respond_to?(:logger) && @target.logger
end
- def process_removed_attribute(attr)
- logger.warn "Can't mass-assign protected attribute: #{attr}" if logger?
+ def process_removed_attributes(attrs)
+ logger.warn "Can't mass-assign protected attributes: #{attrs.join(', ')}" if logger?
end
end
@@ -42,19 +44,19 @@ def initialize(target = nil)
super()
end
- def process_removed_attribute(attr)
- return if insensitive_attributes.include?(attr)
- raise ActiveModel::MassAssignmentSecurity::Error.new(attr)
+ def process_removed_attributes(attrs)
+ return if (attrs - insensitive_attributes).empty?
+ raise ActiveModel::MassAssignmentSecurity::Error.new(attrs)
end
def insensitive_attributes
- @insensitive_attributes ||= ['id']
+ ['id']
end
end
class Error < StandardError
- def initialize(attr)
- super("Can't mass-assign protected attribute: #{attr}")
+ def initialize(attrs)
+ super("Can't mass-assign protected attributes: #{attrs.join(', ')}")
end
end
end
View
2 activemodel/test/cases/mass_assignment_security_test.rb
@@ -4,7 +4,7 @@
class CustomSanitizer < ActiveModel::MassAssignmentSecurity::Sanitizer
- def process_removed_attribute(attr)
+ def process_removed_attributes(attrs)
raise StandardError
end

3 comments on commit eb8f0dd

@bogdan
bogdan commented on eb8f0dd Mar 15, 2012

What kind of compatibility problem? Can we fix it?

@josevalim
Ruby on Rails member

The problem is that we haven't specified a public API but the documentation and examples show it is ok to inherit from the sanitizer and override a method like process_removed_attributes. Considering that many people are writing plugins due to the mass assignment issue, I don't think we should change this API.

Also, it makes the error messages worse, because know we just know which attributes are being mass assigned one by one, instead of having the whole list at once.

Maybe we could optimize the debug method in some other way?

@bogdan
bogdan commented on eb8f0dd Mar 15, 2012

Ok, I'll try.

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