Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

MassAssignmentSecurity: add ability to specify your own sanitizer #1334

Closed
wants to merge 1 commit into from

3 participants

@bogdan

With respect to discussion:
#1320

Added an ability to specify your own behavior on mass assingment
protection, controlled by option:

ActiveModel::MassAssignmentSecurity.mass_assignment_sanitizer

I hope this is right way to do it. Because I don't understand how the customizations can be done without monkey patch or configuration option.

BTW code seems more clean now even without customization benefits.

If there is still something I need to fix please let me know.

@bogdan bogdan MassAssignmentSecurity: add ability to specify your own sanitizer
Added an ability to specify your own behavior on mass assingment
protection, controlled by option:
ActiveModel::MassAssignmentSecurity.mass_assignment_sanitizer
c7567c9
@josevalim
Owner

I will review properly when I get home, but this looks good!

@josevalim
Owner

Merged (github did not close this automatically). Thanks for your work, I think the final solution is quite good.

@josevalim josevalim closed this
@dasch dasch commented on the diff
...tive_model/mass_assignment_security/permission_set.rb
@@ -14,6 +12,10 @@ module ActiveModel
super(remove_multiparameter_id(key))
end
+ def deny?(key)
+ raise NotImplementedError, "#deny?(key) suppose to be overwritten"
@dasch
dasch added a note

It's "supposed" :-)

@bogdan
bogdan added a note

This is my Java habit :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bogdan

My plan now is to make StrictSanitizer that will raise ActiveModel::MassAssingmentSecurity::Error and use it by default in development and test envs like:

R3config::Application.configure do
....
config.active_record.mass_assingment_sanitizer = ActiveModel::MassAssignmentProtection::StrictSanitizer.new
....
end

That do you think, guys?

@josevalim
Owner

I think it is a good idea, but, if we are going to have a main API for that, we should support symbols to be given:

config.active_record.mass_assignment_sanitizer = :strict

And then we would do a constant lookup for StrictSanitizer in the current class, what do you think? And also what do you think about renaming DefaultSanitizer to LoggerSanitizer?

@bogdan

Agreed with everything.

Using symbols in config is good idea.

@jake3030 jake3030 referenced this pull request from a commit in jake3030/rails
stopdropandrew Ensure calculations respect scoped :select [#1334 state:resolved]
Signed-off-by: Pratik Naik <pratiknaik@gmail.com>
6543426
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 26, 2011
  1. @bogdan

    MassAssignmentSecurity: add ability to specify your own sanitizer

    bogdan authored
    Added an ability to specify your own behavior on mass assingment
    protection, controlled by option:
    ActiveModel::MassAssignmentSecurity.mass_assignment_sanitizer
This page is out of date. Refresh to see the latest.
View
14 activemodel/lib/active_model/mass_assignment_security.rb
@@ -1,5 +1,6 @@
require 'active_support/core_ext/class/attribute.rb'
require 'active_model/mass_assignment_security/permission_set'
+require 'active_model/mass_assignment_security/sanitizer'
module ActiveModel
# = Active Model Mass-Assignment Security
@@ -10,6 +11,7 @@ module MassAssignmentSecurity
class_attribute :_accessible_attributes
class_attribute :_protected_attributes
class_attribute :_active_authorizer
+ class_attribute :mass_assignment_sanitizer
end
# Mass assignment security provides an interface for protecting attributes
@@ -181,16 +183,14 @@ def attributes_protected_by_default
def protected_attributes_configs
self._protected_attributes ||= begin
- default_black_list = BlackList.new(attributes_protected_by_default).tap do |w|
- w.logger = self.logger if self.respond_to?(:logger)
- end
+ default_black_list = BlackList.new(attributes_protected_by_default)
Hash.new(default_black_list)
end
end
def accessible_attributes_configs
self._accessible_attributes ||= begin
- default_white_list = WhiteList.new.tap { |w| w.logger = self.logger if self.respond_to?(:logger) }
+ default_white_list = WhiteList.new
Hash.new(default_white_list)
end
end
@@ -199,7 +199,11 @@ def accessible_attributes_configs
protected
def sanitize_for_mass_assignment(attributes, role = :default)
- mass_assignment_authorizer(role).sanitize(attributes)
+ (mass_assignment_sanitizer || default_mass_assignment_sanitizer).sanitize(attributes, mass_assignment_authorizer(role))
+ end
+
+ def default_mass_assignment_sanitizer
+ DefaultSanitizer.new(self.respond_to?(:logger) && self.logger)
end
def mass_assignment_authorizer(role = :default)
View
8 activemodel/lib/active_model/mass_assignment_security/permission_set.rb
@@ -1,10 +1,8 @@
require 'set'
-require 'active_model/mass_assignment_security/sanitizer'
module ActiveModel
module MassAssignmentSecurity
class PermissionSet < Set
- attr_accessor :logger
def +(values)
super(values.map(&:to_s))
@@ -14,6 +12,10 @@ def include?(key)
super(remove_multiparameter_id(key))
end
+ def deny?(key)
+ raise NotImplementedError, "#deny?(key) suppose to be overwritten"
@dasch
dasch added a note

It's "supposed" :-)

@bogdan
bogdan added a note

This is my Java habit :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ end
+
protected
def remove_multiparameter_id(key)
@@ -22,7 +24,6 @@ def remove_multiparameter_id(key)
end
class WhiteList < PermissionSet
- include Sanitizer
def deny?(key)
!include?(key)
@@ -30,7 +31,6 @@ def deny?(key)
end
class BlackList < PermissionSet
- include Sanitizer
def deny?(key)
include?(key)
View
24 activemodel/lib/active_model/mass_assignment_security/sanitizer.rb
@@ -1,9 +1,9 @@
module ActiveModel
module MassAssignmentSecurity
- module Sanitizer
+ class Sanitizer
# Returns all attributes not denied by the authorizer.
- def sanitize(attributes)
- sanitized_attributes = attributes.reject { |key, value| deny?(key) }
+ def sanitize(attributes, authorizer)
+ sanitized_attributes = attributes.reject { |key, value| authorizer.deny?(key) }
debug_protected_attribute_removal(attributes, sanitized_attributes)
sanitized_attributes
end
@@ -12,10 +12,24 @@ def sanitize(attributes)
def debug_protected_attribute_removal(attributes, sanitized_attributes)
removed_keys = attributes.keys - sanitized_attributes.keys
- warn!(removed_keys) if removed_keys.any?
+ 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
+ class DefaultSanitizer < Sanitizer
- def warn!(attrs)
+ attr_accessor :logger
+
+ def initialize(logger = nil)
+ self.logger = logger
+ super()
+ end
+
+ def process_removed_attributes(attrs)
self.logger.debug "WARNING: Can't mass-assign protected attributes: #{attrs.join(', ')}" if self.logger
end
end
View
8 activemodel/test/cases/mass_assignment_security/black_list_test.rb
@@ -16,13 +16,5 @@ def setup
assert_equal false, @black_list.deny?('first_name')
end
- test "sanitize attributes" do
- original_attributes = { 'first_name' => 'allowed', 'admin' => 'denied', 'admin(1)' => 'denied' }
- attributes = @black_list.sanitize(original_attributes)
-
- assert attributes.key?('first_name'), "Allowed key shouldn't be rejected"
- assert !attributes.key?('admin'), "Denied key should be rejected"
- assert !attributes.key?('admin(1)'), "Multi-parameter key should be detected"
- end
end
View
13 activemodel/test/cases/mass_assignment_security/sanitizer_test.rb
@@ -4,24 +4,21 @@
class SanitizerTest < ActiveModel::TestCase
- class SanitizingAuthorizer
- include ActiveModel::MassAssignmentSecurity::Sanitizer
-
- attr_accessor :logger
+ class Authorizer < ActiveModel::MassAssignmentSecurity::PermissionSet
def deny?(key)
key.in?(['admin'])
end
-
end
def setup
- @sanitizer = SanitizingAuthorizer.new
+ @sanitizer = ActiveModel::MassAssignmentSecurity::DefaultSanitizer.new
+ @authorizer = Authorizer.new
end
test "sanitize attributes" do
original_attributes = { 'first_name' => 'allowed', 'admin' => 'denied' }
- attributes = @sanitizer.sanitize(original_attributes)
+ attributes = @sanitizer.sanitize(original_attributes, @authorizer)
assert attributes.key?('first_name'), "Allowed key shouldn't be rejected"
assert !attributes.key?('admin'), "Denied key should be rejected"
@@ -31,7 +28,7 @@ def setup
original_attributes = { 'first_name' => 'allowed', 'admin' => 'denied' }
log = StringIO.new
@sanitizer.logger = Logger.new(log)
- @sanitizer.sanitize(original_attributes)
+ @sanitizer.sanitize(original_attributes, @authorizer)
assert_match(/admin/, log.string, "Should log removed attributes: #{log.string}")
end
View
9 activemodel/test/cases/mass_assignment_security/white_list_test.rb
@@ -16,13 +16,4 @@ def setup
assert_equal true, @white_list.deny?('admin')
end
- test "sanitize attributes" do
- original_attributes = { 'first_name' => 'allowed', 'admin' => 'denied', 'admin(1)' => 'denied' }
- attributes = @white_list.sanitize(original_attributes)
-
- assert attributes.key?('first_name'), "Allowed key shouldn't be rejected"
- assert !attributes.key?('admin'), "Denied key should be rejected"
- assert !attributes.key?('admin(1)'), "Multi-parameter key should be detected"
- end
-
end
View
20 activemodel/test/cases/mass_assignment_security_test.rb
@@ -1,6 +1,15 @@
require "cases/helper"
require 'models/mass_assignment_specific'
+
+class CustomSanitizer < ActiveModel::MassAssignmentSecurity::Sanitizer
+
+ def process_removed_attributes(attrs)
+ raise StandardError
+ end
+
+end
+
class MassAssignmentSecurityTest < ActiveModel::TestCase
def test_attribute_protection
@@ -76,4 +85,15 @@ def test_mass_assignment_multiparameter_protector
assert_equal sanitized, { }
end
+ def test_custom_sanitizer
+ user = User.new
+ User.mass_assignment_sanitizer = CustomSanitizer.new
+ assert_raise StandardError do
+ user.sanitize_for_mass_assignment("admin" => true)
+ end
+ ensure
+ User.mass_assignment_sanitizer = nil
+
+ end
+
end
Something went wrong with that request. Please try again.