Merge pull request #253 from ulferts/refactor/project_authorization

adapt to changes on Project.allowed_to in core

app/controllers/cost_objects_controller.rb

@@ -60,13 +60,11 @@ class CostObjectsController < ApplicationController
     sort_init 'id', 'desc'
     sort_update sort_columns
 
-    allowed_condition = Project.allowed_to(User.current,
-                                           :view_cost_objects,
-                                           project: @project)
-
-    @cost_objects = CostObject.order(sort_clause)
-                    .includes(:project, :author)
-                    .merge(allowed_condition)
+    @cost_objects = CostObject
+                    .visible(current_user)
+                    .order(sort_clause)
+                    .includes(:author)
+                    .where(project_id: @project.id)
                     .page(page_param)
                     .per_page(per_page_param)
 

app/controllers/costlog_controller.rb

@@ -47,7 +47,7 @@ class CostlogController < ApplicationController
 
     cond = ARCondition.new
     if @project.nil?
-      cond << Project.allowed_to_condition(User.current, :view_cost_entries)
+      cond << "project_id IN (#{Project.allowed_to(User.current, :view_cost_entries).to_sql})"
     elsif @work_package.nil?
       cond << @project.project_condition(Setting.display_subprojects_work_packages?)
     else
@@ -65,7 +65,7 @@ class CostlogController < ApplicationController
     respond_to do |format|
       format.html {
         @entries = CostEntry.includes(:project, :cost_type, :user, work_package: :type)
-                   .merge(Project.allowed_to(User.current, :view_cost_entries, project: @project))
+                   .merge(Project.allowed_to(User.current, :view_cost_entries))
                    .where(cond.conditions)
                    .order(sort_clause)
                    .page(page_param)
@@ -244,11 +244,11 @@ class CostlogController < ApplicationController
 
     @from, @to = @to, @from if @from && @to && @from > @to
     @from ||= (CostEntry.includes([:project, :user])
-              .where(Project.allowed_to_condition(User.current, :view_cost_entries))
-              .minimum(:spent_on) || Date.today) - 1
+                        .visible
+                        .minimum(:spent_on) || Date.today) - 1
     @to ||= (CostEntry.includes([:project, :user])
-            .where(Project.allowed_to_condition(User.current, :view_cost_entries))
-            .maximum(:spent_on) || Date.today)
+                      .visible
+                      .maximum(:spent_on) || Date.today)
   end
 
   def new_default_cost_entry

app/models/cost_entry.rb

@@ -37,27 +37,43 @@ class CostEntry < ActiveRecord::Base
   after_initialize :after_initialize
   validate :validate
 
-  scope :visible, lambda { |*args|
-    where(CostEntry.visible_condition(args[0] || User.current, args[1]))
-      .includes([:project, :user])
-      .references(:project)
-  }
+  scope :visible, -> (*args) {
+    user = args.first || User.current
+    project = args[1]
 
-  scope :on_work_packages, ->(work_packages) { where(work_package_id: work_packages) }
+    table = self.arel_table
 
-  def self.visible_condition(user, project)
-    %{ (#{Project.allowed_to_condition(user, :view_cost_entries, project: project)} OR
-         (#{Project.allowed_to_condition(user, :view_own_cost_entries, project: project)} AND #{CostEntry.table_name}.user_id = #{user.id})) }
-  end
+    view_allowed = Project.allowed_to(user, :view_cost_entries).select(:id)
+    view_own_allowed = Project.allowed_to(user, :view_own_cost_entries).select(:id)
+
+    view_or_view_own = table[:project_id]
+                       .in(view_allowed.arel)
+                       .or(table[:project_id]
+                           .in(view_own_allowed.arel)
+                           .and(table[:user_id].eq(user.id)))
+
+    scope = where(view_or_view_own)
+
+    if project
+      scope = scope.where(project_id: project.id)
+    end
+
+    scope
+  }
 
   scope :visible_costs, lambda{|*args|
-    view_cost_rates = Project.allowed_to_condition((args.first || User.current), :view_cost_rates, project: args[1])
-    view_cost_entries = CostEntry.visible_condition((args.first || User.current), args[1])
+    user = args.first || User.current
+    project = args[1]
+
+    table = self.arel_table
 
-    where([view_cost_entries, view_cost_rates].join(' AND '))
-      .includes([:project, :user])
+    view_allowed = Project.allowed_to(user, :view_cost_rates).select(:id)
+
+    visible(user, project).where(table[:project_id].in(view_allowed.arel))
   }
 
+  scope :on_work_packages, ->(work_packages) { where(work_package_id: work_packages) }
+
   def self.costs_of(work_packages:)
     # N.B. Because of an AR quirks the code below uses statements like
     #   where(work_package_id: ids)

app/models/cost_object.rb

@@ -44,6 +44,12 @@ class CostObject < ActiveRecord::Base
     CostObject.replace_author_with_deleted_user user
   end
 
+  def self.visible(user)
+    includes(:project)
+      .references(:projects)
+      .merge(Project.allowed_to(user, :view_cost_objects))
+  end
+
   def initialize(attributes = nil)
     super
     self.author = User.current if self.new_record?

app/models/labor_budget_item.rb

@@ -32,19 +32,29 @@ class LaborBudgetItem < ActiveRecord::Base
   include ActiveModel::ForbiddenAttributesProtection
   # user_id correctness is ensured in VariableCostObject#*_labor_budget_item_attributes=
 
-  def self.visible_condition(user, project)
-    %{ (#{Project.allowed_to_condition(user,
-                                       :view_hourly_rates,
-                                       project: project)} OR
-         (#{Project.allowed_to_condition(user,
-                                         :view_own_hourly_rate,
-                                         project: project)} AND #{LaborBudgetItem.table_name}.user_id = #{user.id})) }
+  def self.visible(user, project)
+    table = self.arel_table
+
+    view_allowed = Project.allowed_to(user, :view_hourly_rates).select(:id)
+    view_own_allowed = Project.allowed_to(user, :view_own_hourly_rates).select(:id)
+
+    view_or_view_own = table[:project_id]
+                       .in(view_allowed.arel)
+                       .or(table[:project_id]
+                           .in(view_own_allowed.arel)
+                           .and(table[:user_id].eq(user.id)))
+
+    scope = includes([{ cost_object: :project }, :user])
+            .references(:projects)
+            where(view_or_view_own)
+
+    if project
+      scope = scope.where(cost_object: { projects_id: project.id })
+    end
   end
 
   scope :visible_costs, lambda{|*args|
-    includes([{ cost_object: :project }, :user])
-      .where(LaborBudgetItem.visible_condition((args.first || User.current), args[1]))
-      .references(:projects)
+    visible((args.first || User.current), args[1])
   }
 
   def costs

app/models/material_budget_item.rb

@@ -26,16 +26,20 @@ class MaterialBudgetItem < ActiveRecord::Base
 
   include ActiveModel::ForbiddenAttributesProtection
 
-  def self.visible_condition(user, project)
-    Project.allowed_to_condition(user,
-                                 :view_cost_rates,
-                                 project: project)
+  def self.visible(user)
+    includes(cost_object: :project)
+      .references(:projects)
+      .merge(Project.allowed_to(user, :view_cost_rates))
   end
 
   scope :visible_costs, lambda { |*args|
-    where(MaterialBudgetItem.visible_condition((args.first || User.current), args[1]))
-      .includes(cost_object: :project)
-      .references(:projects)
+    scope = visible(args.first || User.current)
+
+    if args[1]
+      scope = scope.where(cost_object: { projects_id: args[1].id })
+    end
+
+    scope
   }
 
   def costs

lib/open_project/costs/patches/time_entry_patch.rb

@@ -24,36 +24,52 @@ module OpenProject::Costs::Patches::TimeEntryPatch
 
     base.send(:include, InstanceMethods)
 
-    # Same as typing in the class t.update_costs
+    # Same as typing in the class
     base.class_eval do
       belongs_to :rate, -> { where(type: ['HourlyRate', 'DefaultHourlyRate']) }, class_name: 'Rate'
 
       before_save :update_costs
 
-      def self.visible_condition(user, table_alias: nil, project: nil)
-        options = {}
-        options[:project_alias] = table_alias if table_alias
-        options[:project] = project if project
-
-        %{ (#{Project.allowed_to_condition(user,
-                                           :view_time_entries,
-                                           options)} OR
-             (#{Project.allowed_to_condition(user,
-                                             :view_own_time_entries,
-                                             options)} AND
-              #{TimeEntry.table_name}.user_id = #{user.id})) }
-      end
+      scope :visible, -> (*args) {
+        user = args.first || User.current
+        project = args[1]
+
+        table = self.arel_table
+
+        view_allowed = Project.allowed_to(user, :view_time_entries).select(:id)
+        view_own_allowed = Project.allowed_to(user, :view_own_time_entries).select(:id)
+
+        view_or_view_own = table[:project_id]
+                           .in(view_allowed.arel)
+                           .or(table[:project_id]
+                               .in(view_own_allowed.arel)
+                               .and(table[:user_id].eq(user.id)))
+
+        scope = where(view_or_view_own)
+
+        if project
+          scope = scope.where(project_id: project.id)
+        end
+
+        scope
+      }
 
       scope :visible_costs, lambda{|*args|
         user = args.first || User.current
         project = args[1]
 
-        view_hourly_rates = %{ (#{Project.allowed_to_condition(user, :view_hourly_rates, project: project)} OR
-                                (#{Project.allowed_to_condition(user, :view_own_hourly_rate, project: project)} AND #{TimeEntry.table_name}.user_id = #{user.id})) }
-        view_time_entries = TimeEntry.visible_condition(user, project: project)
+        table = self.arel_table
+
+        view_allowed = Project.allowed_to(user, :view_hourly_rates).select(:id)
+        view_own_allowed = Project.allowed_to(user, :view_own_hourly_rates).select(:id)
+
+        view_or_view_own = table[:project_id]
+                           .in(view_allowed.arel)
+                           .or(table[:project_id]
+                               .in(view_own_allowed.arel)
+                               .and(table[:user_id].eq(user.id)))
 
-        includes(:project, :user)
-          .where([view_time_entries, view_hourly_rates].join(' AND '))
+        visible(user, project).where(view_or_view_own)
       }
 
       def self.costs_of(work_packages:)

lib/open_project/costs/patches/user_patch.rb

@@ -31,9 +31,10 @@ module OpenProject::Costs::Patches::UserPatch
 
   module InstanceMethods
     def allowed_to_condition_with_project_id(permission, projects = nil)
-      project_condition = Project.allowed_to_condition(self, permission, project: projects)
+      scope = Project.allowed_to(self, permission)
+      scope = scope.where(id: projects) if projects
 
-      ids = Project.where(project_condition).pluck(:id)
+      ids = scope.pluck(:id)
 
       ids.empty? ?
         '1=0' :

lib/open_project/costs/patches/work_package_patch.rb

@@ -66,26 +66,30 @@ module OpenProject::Costs::Patches::WorkPackagePatch
 
         false
       when 'reassign'
-        reassign_to = WorkPackage
-                      .where(Project.allowed_to_condition(user, :edit_cost_entries))
-                      .includes(:project)
-                      .references(:projects)
-                      .find_by_id(to_do[:reassign_to_id])
-
-        if reassign_to.nil?
-          work_packages.each do |wp|
-            wp.errors.add(:base, :is_not_a_valid_target_for_cost_entries, id: to_do[:reassign_to_id])
-          end
-
-          false
-        else
-          WorkPackage.update_cost_entries(work_packages.map(&:id), "work_package_id = #{reassign_to.id}, project_id = #{reassign_to.project_id}")
-        end
+        reassign_cost_entries_before_destruction(work_packages, user, to_do[:reassign_to_id])
       else
         false
       end
     end
 
+    def reassign_cost_entries_before_destruction(work_packages, user, ids)
+      reassign_to = WorkPackage
+                    .joins(:project)
+                    .merge(Project.allowed_to(user, :edit_cost_entries))
+                    .find_by_id(ids)
+
+      if reassign_to.nil?
+        work_packages.each do |wp|
+          wp.errors.add(:base, :is_not_a_valid_target_for_cost_entries, id: ids)
+        end
+
+        false
+      else
+        condition = "work_package_id = #{reassign_to.id}, project_id = #{reassign_to.project_id}"
+        WorkPackage.update_cost_entries(work_packages.map(&:id), condition)
+      end
+    end
+
     protected
 
     def update_cost_entries(work_packages, action)