From 58fab0dea49da448e8efcccbfd656ce45d1e7308 Mon Sep 17 00:00:00 2001 From: Sebastian Schuster Date: Tue, 7 May 2013 16:09:25 +0200 Subject: [PATCH 1/3] Removed attr_accessible for cost models and replaced with strong parameters style permitting of mass assignments, added specs for testing the assignment --- app/models/cost_object.rb | 2 +- app/models/cost_type.rb | 2 +- app/models/labor_budget_item.rb | 2 +- app/models/material_budget_item.rb | 2 +- app/models/rate.rb | 2 +- .../costs/patches/permitted_params_patch.rb | 38 ++++ spec/models/permitted_params_spec.rb | 184 ++++++++++++++++++ 7 files changed, 227 insertions(+), 5 deletions(-) diff --git a/app/models/cost_object.rb b/app/models/cost_object.rb index 7fa23cb594..e9a9f1772f 100644 --- a/app/models/cost_object.rb +++ b/app/models/cost_object.rb @@ -10,7 +10,7 @@ class CostObject < ActiveRecord::Base has_many :cost_entries, :through => :issues has_many :time_entries, :through => :issues - attr_accessible :subject, :description, :fixed_date, :project_manager_signoff, :client_signoff + include ActiveModel::ForbiddenAttributesProtection acts_as_attachable :after_remove => :attachment_removed diff --git a/app/models/cost_type.rb b/app/models/cost_type.rb index 9f284aceec..e2fe15509a 100644 --- a/app/models/cost_type.rb +++ b/app/models/cost_type.rb @@ -8,7 +8,7 @@ class CostType < ActiveRecord::Base after_update :save_rates - attr_accessible :name, :unit, :unit_plural, :default, :new_rate_attributes, :existing_rate_attributes + include ActiveModel::ForbiddenAttributesProtection scope :active, :conditions => { :deleted_at => nil } diff --git a/app/models/labor_budget_item.rb b/app/models/labor_budget_item.rb index 317ce20d9b..195567d452 100644 --- a/app/models/labor_budget_item.rb +++ b/app/models/labor_budget_item.rb @@ -10,8 +10,8 @@ class LaborBudgetItem < ActiveRecord::Base validates_presence_of :cost_object validates_numericality_of :hours, :allow_nil => false + include ActiveModel::ForbiddenAttributesProtection # user_id correctness is ensured in VariableCostObject#*_labor_budget_item_attributes= - attr_accessible :hours, :comments, :budget, :user_id def self.visible_condition(user, project) %Q{ (#{Project.allowed_to_condition(user, diff --git a/app/models/material_budget_item.rb b/app/models/material_budget_item.rb index daabb64bd5..d25a22ffe4 100644 --- a/app/models/material_budget_item.rb +++ b/app/models/material_budget_item.rb @@ -7,7 +7,7 @@ class MaterialBudgetItem < ActiveRecord::Base validates_length_of :comments, :maximum => 255, :allow_nil => true validates_presence_of :cost_type - attr_accessible :units, :comments, :budget, :cost_type, :cost_type_id + include ActiveModel::ForbiddenAttributesProtection def self.visible_condition(user, project) Project.allowed_to_condition(user, diff --git a/app/models/rate.rb b/app/models/rate.rb index da879bcd45..bc1a3723f5 100644 --- a/app/models/rate.rb +++ b/app/models/rate.rb @@ -6,7 +6,7 @@ class Rate < ActiveRecord::Base include ::OpenProject::Costs::DeletedUserFallback belongs_to :project - attr_accessible :rate, :project, :valid_from + include ActiveModel::ForbiddenAttributesProtection def self.clean_currency(value) if value && value.is_a?(String) diff --git a/lib/open_project/costs/patches/permitted_params_patch.rb b/lib/open_project/costs/patches/permitted_params_patch.rb index f1d7331bc0..0ac6b28766 100644 --- a/lib/open_project/costs/patches/permitted_params_patch.rb +++ b/lib/open_project/costs/patches/permitted_params_patch.rb @@ -13,6 +13,44 @@ module OpenProject::Costs::Patches::PermittedParamsPatch :overridden_costs, :spent_on) end + + def cost_object + params.require(:cost_object).permit(:subject, + :description, + :fixed_date, + :project_manager_signoff, + :client_signoff) + end + + def cost_type + params.require(:cost_type).permit(:name, + :unit, + :unit_plural, + :default, + :new_rate_attributes, + :existing_rate_attributes) + end + + def labor_budget_item + params.require(:labor_budget_item).permit(:hours, + :comments, + :budget, + :user_id) + end + + def material_budget_item + params.require(:material_budget_item).permit(:units, + :comments, + :budget, + :cost_type, + :cost_type_id) + end + + def rate + params.require(:rate).permit(:rate, + :project, + :valid_from) + end end end diff --git a/spec/models/permitted_params_spec.rb b/spec/models/permitted_params_spec.rb index fdd876cdff..87b8780669 100644 --- a/spec/models/permitted_params_spec.rb +++ b/spec/models/permitted_params_spec.rb @@ -27,5 +27,189 @@ describe PermittedParams do PermittedParams.new(params, user).cost_entry.should == { "spent_on" => Date.today.to_s } end + + it "should not return project_id" do + params = ActionController::Parameters.new(:cost_entry => { "project_id" => 42 } ) + + PermittedParams.new(params, user).cost_entry.should == { } + end + end + + describe :cost_object do + it "should return comments" do + params = ActionController::Parameters.new(:cost_object => { "subject" => "subject_test" } ) + + PermittedParams.new(params, user).cost_object.should == { "subject" => "subject_test" } + end + + it "should return description" do + params = ActionController::Parameters.new(:cost_object => { "description" => "description_test" } ) + + PermittedParams.new(params, user).cost_object.should == { "description" => "description_test" } + end + + it "should return fixed_date" do + params = ActionController::Parameters.new(:cost_object => { "fixed_date" => "2013-05-06" } ) + + PermittedParams.new(params, user).cost_object.should == { "fixed_date" => "2013-05-06" } + end + + it "should return project_manager_signoff" do + params = ActionController::Parameters.new(:cost_object => { "project_manager_signoff" => true } ) + + PermittedParams.new(params, user).cost_object.should == { "project_manager_signoff" => true } + end + + it "should return client_signoff" do + params = ActionController::Parameters.new(:cost_object => { "client_signoff" => true } ) + + PermittedParams.new(params, user).cost_object.should == { "client_signoff" => true } + end + + it "should not return project_id" do + params = ActionController::Parameters.new(:cost_object => { "project_id" => 42 } ) + + PermittedParams.new(params, user).cost_object.should == { } + end + end + + describe :cost_type do + it "should return name" do + params = ActionController::Parameters.new(:cost_type => { "name" => "name_test" } ) + + PermittedParams.new(params, user).cost_type.should == { "name" => "name_test" } + end + + it "should return unit" do + params = ActionController::Parameters.new(:cost_type => { "unit" => "unit_test" } ) + + PermittedParams.new(params, user).cost_type.should == { "unit" => "unit_test" } + end + + it "should return unit_plural" do + params = ActionController::Parameters.new(:cost_type => { "unit_plural" => "unit_plural_test" } ) + + PermittedParams.new(params, user).cost_type.should == { "unit_plural" => "unit_plural_test" } + end + + it "should return default" do + params = ActionController::Parameters.new(:cost_type => { "default" => 7 } ) + + PermittedParams.new(params, user).cost_type.should == { "default" => 7 } + end + + it "should return new_rate_attributes" do + params = ActionController::Parameters.new(:cost_type => { "new_rate_attributes" => "new_rate_attributes_test" } ) + + PermittedParams.new(params, user).cost_type.should == { "new_rate_attributes" => "new_rate_attributes_test" } + end + + it "should return existing_rate_attributes" do + params = ActionController::Parameters.new(:cost_type => { "existing_rate_attributes" => "new_rate_attributes_test" } ) + + PermittedParams.new(params, user).cost_type.should == { "existing_rate_attributes" => "new_rate_attributes_test" } + end + + it "should not return project_id" do + params = ActionController::Parameters.new(:cost_type => { "project_id" => 42 } ) + + PermittedParams.new(params, user).cost_type.should == { } + end + end + + describe :labor_budget_item do + it "should return hours" do + params = ActionController::Parameters.new(:labor_budget_item => { "hours" => 42.42 } ) + + PermittedParams.new(params, user).labor_budget_item.should == { "hours" => 42.42 } + end + + it "should return comments" do + params = ActionController::Parameters.new(:labor_budget_item => { "comments" => "comments_test" } ) + + PermittedParams.new(params, user).labor_budget_item.should == { "comments" => "comments_test" } + end + + it "should return budget" do + params = ActionController::Parameters.new(:labor_budget_item => { "budget" => 42.4242 } ) + + PermittedParams.new(params, user).labor_budget_item.should == { "budget" => 42.4242 } + end + + it "should return user_id" do + params = ActionController::Parameters.new(:labor_budget_item => { "user_id" => 42 } ) + + PermittedParams.new(params, user).labor_budget_item.should == { "user_id" => 42 } + end + + it "should not return project_id" do + params = ActionController::Parameters.new(:labor_budget_item => { "project_id" => 42 } ) + + PermittedParams.new(params, user).labor_budget_item.should == { } + end + end + + describe :material_budget_item do + it "should return hours" do + params = ActionController::Parameters.new(:material_budget_item => { "units" => 42.42 } ) + + PermittedParams.new(params, user).material_budget_item.should == { "units" => 42.42 } + end + + it "should return comments" do + params = ActionController::Parameters.new(:material_budget_item => { "comments" => "comments_test" } ) + + PermittedParams.new(params, user).material_budget_item.should == { "comments" => "comments_test" } + end + + it "should return budget" do + params = ActionController::Parameters.new(:material_budget_item => { "budget" => 42.4242 } ) + + PermittedParams.new(params, user).material_budget_item.should == { "budget" => 42.4242 } + end + + it "should return cost_type" do + params = ActionController::Parameters.new(:material_budget_item => { "cost_type" => "cost_type_test" } ) + + PermittedParams.new(params, user).material_budget_item.should == { "cost_type" => "cost_type_test" } + end + + it "should return cost_type_id" do + params = ActionController::Parameters.new(:material_budget_item => { "cost_type_id" => 42 } ) + + PermittedParams.new(params, user).material_budget_item.should == { "cost_type_id" => 42 } + end + + it "should not return project_id" do + params = ActionController::Parameters.new(:material_budget_item => { "project_id" => 42 } ) + + PermittedParams.new(params, user).material_budget_item.should == { } + end + end + + describe :rate do + it "should return rate" do + params = ActionController::Parameters.new(:rate => { "rate" => 42.42 } ) + + PermittedParams.new(params, user).rate.should == { "rate" => 42.42 } + end + + it "should return project" do + params = ActionController::Parameters.new(:rate => { "project" => "project_test" } ) + + PermittedParams.new(params, user).rate.should == { "project" => "project_test" } + end + + it "should return valid_from" do + params = ActionController::Parameters.new(:rate => { "valid_from" => "2013-05-07" } ) + + PermittedParams.new(params, user).rate.should == { "valid_from" => "2013-05-07" } + end + + it "should not return project_id" do + params = ActionController::Parameters.new(:rate => { "project_id" => 42 } ) + + PermittedParams.new(params, user).rate.should == { } + end end end From 97128d3964f8b91c12ed0e30c1e05d120eb81ced Mon Sep 17 00:00:00 2001 From: Sebastian Schuster Date: Tue, 7 May 2013 16:09:54 +0200 Subject: [PATCH 2/3] Manually took id and type out of copying cost objects so they are not prevented by the mass assignment protection --- app/models/cost_object.rb | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/app/models/cost_object.rb b/app/models/cost_object.rb index e9a9f1772f..baddaf4b22 100644 --- a/app/models/cost_object.rb +++ b/app/models/cost_object.rb @@ -39,8 +39,14 @@ class CostObject < ActiveRecord::Base def attributes=(attrs) # Remove any attributes which can not be assigned. # This is to protect from exceptions during change of cost object type - attrs.delete_if{|k, v| !self.respond_to?("#{k}=")} if attrs.is_a?(Hash) - + if(attrs.is_a?(Hash)) + attrs.delete_if{|k, v| !self.respond_to?("#{k}=")} + #also remove any attributes that cannot be mass assigned like id and type, set type manually instead + attrs.delete("id") + if attrs.has_key?("type") + self.type = attrs.delete("type") + end + end super(attrs) end From 2a83bf44348200487e66e857d0091a9f17bf07c7 Mon Sep 17 00:00:00 2001 From: Sebastian Schuster Date: Tue, 7 May 2013 17:41:10 +0200 Subject: [PATCH 3/3] Adapted the copying of values from other cost objects to not work around the mass assignment protection for all assignments but just for copying --- app/models/cost_object.rb | 23 +++++++---------------- app/models/variable_cost_object.rb | 24 +++++++++++------------- 2 files changed, 18 insertions(+), 29 deletions(-) diff --git a/app/models/cost_object.rb b/app/models/cost_object.rb index baddaf4b22..abd6b69635 100644 --- a/app/models/cost_object.rb +++ b/app/models/cost_object.rb @@ -36,23 +36,14 @@ class CostObject < ActiveRecord::Base self.author = User.current if self.new_record? end - def attributes=(attrs) - # Remove any attributes which can not be assigned. - # This is to protect from exceptions during change of cost object type - if(attrs.is_a?(Hash)) - attrs.delete_if{|k, v| !self.respond_to?("#{k}=")} - #also remove any attributes that cannot be mass assigned like id and type, set type manually instead - attrs.delete("id") - if attrs.has_key?("type") - self.type = attrs.delete("type") - end - end - super(attrs) - end - def copy_from(arg) - cost_object = arg.is_a?(CostObject) ? arg : CostObject.find(arg) - self.attributes = cost_object.attributes.dup + if !arg.is_a?(Hash) + #turn args into an attributes hash if it is not already (which is the case when called from VariableCostObject) + arg = (arg.is_a?(CostObject) ? arg : self.class.find(arg)).attributes.dup + end + arg.delete("id") + self.type = arg.delete("type") + self.attributes = arg end # Wrap type column to make it usable in views (especially in a select tag) diff --git a/app/models/variable_cost_object.rb b/app/models/variable_cost_object.rb index 5ccf9b18f8..caed55dcf5 100644 --- a/app/models/variable_cost_object.rb +++ b/app/models/variable_cost_object.rb @@ -25,17 +25,6 @@ class VariableCostObject < CostObject :activity_permission => :view_cost_objects end - def attributes=(attrs) - if attrs - [:new_material_budget_item_attributes, :new_labor_budget_item_attributes, - :existing_material_budget_item_attributes, :existing_labor_budget_item_attributes].each do |attribute| - if (value = attrs.delete(attribute.to_s)).present? - self.send(:"#{attribute}=", value) - end - end - end - super(attrs) - end # override acts_as_journalized method def activity_type @@ -43,8 +32,17 @@ class VariableCostObject < CostObject end def copy_from(arg) - cost_object = arg.is_a?(VariableCostObject) ? arg : VariableCostObject.find(arg) - self.attributes = cost_object.attributes.dup + cost_object = (arg.is_a?(VariableCostObject) ? arg : self.class.find(arg)) + attrs = cost_object.attributes.dup + #do single assignments of attributes not allowed for mass assignment + [:new_material_budget_item_attributes, :new_labor_budget_item_attributes, + :existing_material_budget_item_attributes, :existing_labor_budget_item_attributes].each do |attribute| + if (value = attrs.delete(attribute.to_s)).present? + self.send(:"#{attribute}=", value) + end + end + #pass the remaining attributes to base class which will set them + super(attrs) self.material_budget_items = cost_object.material_budget_items.collect {|v| v.clone} self.labor_budget_items = cost_object.labor_budget_items.collect {|v| v.clone} end