Merge pull request #4 from finnlabs/feature/rails3_strong_parameters

Feature/rails3 strong parameters

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
 
@@ -36,17 +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
-    attrs.delete_if{|k, v| !self.respond_to?("#{k}=")} if attrs.is_a?(Hash)
-
-    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)

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 }
 

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,

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,

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)

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

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
 

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