Permalink
Browse files

AccessibleFor: new name, new API, ruby 1.8 compatible

  • Loading branch information...
1 parent ec9d281 commit 935d11d0a38009e67332e1a7bcd363fd207da97f @zenhob committed Mar 6, 2012
View
@@ -1,19 +1,31 @@
-# MassAssignmentBackport
+# AccessibleFor: key-based hash sanitizer for Ruby
This is a simple mass-assignment security module loosely based on
[ActiveModel::MassAssignmentSecurity][1]. It attempts to steal the good ideas
and some of the API while being compatible with Rails 2.3-based applications.
-Only attr_accessible is implemented, because attr_protected is just a bad
-ActiveRecord API that hung around for some reason, and we don't want it
-stinking up the place.
+Only attr_accessible (or its equivalent, keep reading) is implemented, because
+attr_protected is just a bad ActiveRecord API that hung around for some reason,
+and we don't want it stinking up the place.
+
+There are actually two available APIs, the ActiveModel-workalike and a new one
+called accessible_for. They provide identical functionality.
# Usage
+## ActiveModel-workalike API
+
class TacoShop < Controller
include MassAssignmentBackport
- attr_accessible :topping
- attr_accessible :price, :topping, as: :manager
+
+ # when no role is specified, :default is used
+ attr_accessible :rating
+
+ # you can specify multiple roles
+ attr_accessible :filling, :topping, :as => [:default, :manager]
+
+ # and add to existing roles
+ attr_accessible :price, :as => :manager
def update
Taco.find(params[:id]).update_attributes!(taco_params)
@@ -22,10 +34,38 @@ stinking up the place.
protected
def taco_params
+ # use sanitize_for_mass_assignment to build a safe hash given a role.
+ # when nothing/nil is passed for the role, :default is used
sanitize_for_mass_assignment params[:taco], current_user.manager? ? :manager : nil
end
end
+## accessible_for API
+
+ class TacoShop < Controller
+ include AccessibleFor
+
+ # there are no implicit roles and you can declare only one group at a time
+ accessible_for :default => [ :filling, :topping, :rating ]
+ accessible_for :manager => [ :filling, :topping, :price ]
+
+ def update
+ Taco.find(params[:id]).update_attributes!(taco_params)
+ end
+
+ protected
+
+ def taco_params
+ # use sanitize_for(role, params) to build a safe hash
+ # again, there is no implicit role
+ if current_user.manager?
+ sanitize_for :manager, params[:taco]
+ else
+ sanitize_for :default, params[:taco]
+ end
+ end
+ end
+
# Rationale
There are two things I've never liked about ActiveRecord's attr_* API:
@@ -40,8 +80,18 @@ Another problem with ActiveRecord is that it provides attr_protected.
Blacklisting instead of whitelisting is just a bad idea, and I see no reason
to allow/support it when security is the primary goal.
-This small package attempts to address both of those issues with a module that
-borrows/steals the excellent ActiveModel API for the same purpose.
+So once we address those two things we have something that looks a bit like
+ActiveModel's implementation minus attr_protected, which is the purpose of the
+ActiveModel-workalike API. However there are problems with this API as well:
+
+The role is optional, leading to lack of clarity. Sometimes you need to
+specify :default, sometimes it's implicit. I think an API designed for
+hardening should be more transparent.
+
+The way the role is specified is also suboptimal. It's at the end of the
+declaration so you have to hunt for it. It uses the key :as implying a
+user-based access role, but the fact is this value is really just a scope and
+can mean anything.
# Author
View
@@ -1,9 +1,14 @@
desc 'run tests'
task :test do
- sh 'ruby -Ilib test/*_test.rb'
+ # This should work in my opinion, but it only runs the last loaded test.
+ #sh 'ruby -Ilib -rminitest/autorun test/*_test.rb'
+ require 'minitest/unit'
+ $:.unshift("./lib")
+ Dir['test/*_test.rb'].each {|t| load t}
+ MiniTest::Unit.autorun
end
desc 'build the gem'
task :gem do
- sh 'gem build mass_assignment_backport.gemspec'
+ sh 'gem build accessible_for.gemspec'
end
@@ -1,9 +1,9 @@
$:.unshift File.expand_path("./lib")
-require 'mass_assignment_backport'
+require 'accessible_for'
Gem::Specification.new do |s|
- s.name = "mass_assignment_backport"
- s.version = MassAssignmentBackport::VERSION
+ s.name = "accessible_for"
+ s.version = AccessibleFor::VERSION
s.summary = 'Simple API for sanitizing hashes by input key'
s.description = <<-EOD
This is a simple mass-assignment security module loosely based on
View
@@ -0,0 +1,24 @@
+require 'mass_assignment_backport'
+
+module AccessibleFor
+ VERSION = "0.1.0"
+
+ def self.included(mod)
+ mod.send :include, MassAssignmentBackport
+ mod.extend ClassMethods
+ end
+
+ module ClassMethods
+ def accessible_for params
+ params.each do |role, attrs|
+ attr_accessible *([attrs].flatten.push(:as => role))
+ end
+ end
+ end
+
+ def sanitize_for role, values
+ sanitize_for_mass_assignment values, role
+ end
+end
+
+
@@ -0,0 +1,49 @@
+require 'accessible_for'
+
+class AccessibleForTest < MiniTest::Unit::TestCase
+ include AccessibleFor
+ accessible_for :default => :topping
+ accessible_for :manager => [:price, :topping]
+
+ def test_accessible_default
+ default = sanitize_for :default, :topping => 'salsa', :price => 123, :extra => 'foo'
+ assert default.has_key?(:topping), "default gets accessible key"
+ assert !default.has_key?(:price), "default does not get inaccessible key"
+ assert !default.has_key?(:extra), "default does not get extra key"
+ end
+
+ def test_accessible_role
+ manager = sanitize_for :manager, :topping => 'salsa', :price => 123, :extra => 'foo'
+ assert manager.has_key?(:topping), "role gets accessible key"
+ assert manager.has_key?(:price), "role gets second accessible key"
+ assert !manager.has_key?(:extra), "role does not get extra key"
+ end
+
+ class SubTest
+ include AccessibleFor
+ accessible_for :default => :toasted
+ accessible_for :manager => :free
+ end
+ class SubclassTest < SubTest
+ accessible_for :default => :rating
+ accessible_for :manager => :toasted
+ end
+
+ def test_ignores_parent_settings_default
+ sub = SubclassTest.new
+ default = sub.sanitize_for :default, :toasted => true, :rating => 'good'
+ assert !default.has_key?(:toasted), "inherited default does not get parent accessible key"
+ assert default.has_key?(:rating), "inherited default gets accessible key"
+ assert !default.has_key?(:free), "inherited default does not get inaccessible key"
+ assert !default.has_key?(:extra), "inherited default does not get extra key"
+ end
+
+ def test_ignores_parent_settings_role
+ sub = SubclassTest.new
+ manager = sub.sanitize_for :manager, :toasted => true, :free => true, :rating => 'good'
+ assert !manager.has_key?(:free), "inherited role does not get parent accessible key"
+ assert manager.has_key?(:toasted), "inherited role gets accessible key"
+ assert !manager.has_key?(:rating), "inherited role does not get inaccessible key"
+ assert !manager.has_key?(:extra), "inherited role does not get extra key"
+ end
+end
@@ -1,23 +1,49 @@
require 'mass_assignment_backport'
-require 'minitest/autorun'
class MassAssignmentTest < MiniTest::Unit::TestCase
include MassAssignmentBackport
- attr_accessible :topping
- attr_accessible :price, :topping, as: :manager
+ attr_accessible :topping, :as => [:default, :manager]
+ attr_accessible :price, :as => :manager
def test_accessible_default
- default = sanitize_for_mass_assignment topping: 'salsa', price: 123, extra: 'foo'
+ default = sanitize_for_mass_assignment :topping => 'salsa', :price => 123, :extra => 'foo'
assert default.has_key?(:topping), "default gets accessible key"
assert !default.has_key?(:price), "default does not get inaccessible key"
assert !default.has_key?(:extra), "default does not get extra key"
end
def test_accessible_role
- manager = sanitize_for_mass_assignment({ topping: 'salsa', price: 123, extra: 'foo' }, :manager)
+ manager = sanitize_for_mass_assignment({ :topping => 'salsa', :price => 123, :extra => 'foo' }, :manager)
assert manager.has_key?(:topping), "role gets accessible key"
assert manager.has_key?(:price), "role gets second accessible key"
assert !manager.has_key?(:extra), "role does not get extra key"
end
+ class SubTest
+ include MassAssignmentBackport
+ attr_accessible :toasted
+ attr_accessible :free, :as => :manager
+ end
+ class SubclassTest < SubTest
+ attr_accessible :rating
+ attr_accessible :toasted, :as => :manager
+ end
+
+ def test_ignores_parent_settings_default
+ sub = SubclassTest.new
+ default = sub.sanitize_for_mass_assignment :toasted => true, :rating => 'good'
+ assert !default.has_key?(:toasted), "inherited default does not get parent accessible key"
+ assert default.has_key?(:rating), "inherited default gets accessible key"
+ assert !default.has_key?(:free), "inherited default does not get inaccessible key"
+ assert !default.has_key?(:extra), "inherited default does not get extra key"
+ end
+
+ def test_ignores_parent_settings_role
+ sub = SubclassTest.new
+ manager = sub.sanitize_for_mass_assignment({ :toasted => true, :free => true, :rating => 'good' }, :manager)
+ assert !manager.has_key?(:free), "inherited role does not get parent accessible key"
+ assert manager.has_key?(:toasted), "inherited role gets accessible key"
+ assert !manager.has_key?(:rating), "inherited role does not get inaccessible key"
+ assert !manager.has_key?(:extra), "inherited role does not get extra key"
+ end
end

0 comments on commit 935d11d

Please sign in to comment.