Merge branch 'vuln-hourly-rates-delete-17.0' into release/17.0

modules/costs/app/controllers/hourly_rates_controller.rb

@@ -32,13 +32,13 @@ class HourlyRatesController < ApplicationController
   helper :users
   helper :sort
   include SortHelper
+
   helper :hourly_rates
   include HourlyRatesHelper
 
-  before_action :find_user, only: %i[show edit update set_rate]
-
-  before_action :find_optional_project, only: %i[show edit update]
-  before_action :find_project, only: [:set_rate]
+  before_action :find_optional_project, only: %i[edit update]
+  before_action :find_project, only: %i[show]
+  before_action :find_user, only: %i[show edit update]
 
   # #show, #edit and #update have their own authorization
   before_action :authorize, except: %i[show edit update]
@@ -48,15 +48,10 @@ class HourlyRatesController < ApplicationController
 
   # TODO: this should be an index
   def show
-    if @project
-      return deny_access unless User.current.allowed_in_project?(:view_hourly_rates, @project)
+    return deny_access if @project.nil?
+    return deny_access unless User.current.allowed_in_project?(:view_hourly_rates, @project)
 
-      @rates = HourlyRate.where(user_id: @user, project_id: @project)
-               .order("#{HourlyRate.table_name}.valid_from desc")
-    else
-      @rates = HourlyRate.history_for_user(@user)
-      @rates_default = @rates.delete(nil)
-    end
+    @rates = HourlyRate.where(user_id: @user, project_id: @project).order("#{HourlyRate.table_name}.valid_from desc")
   end
 
   def edit # rubocop:disable Metrics/AbcSize, Metrics/PerceivedComplexity
@@ -129,32 +124,6 @@ class HourlyRatesController < ApplicationController
     end
   end
 
-  def set_rate
-    today = Time.zone.today
-
-    rate = @user.rate_at(today, @project)
-    rate = HourlyRate.new if rate.nil? || rate.valid_from != today
-
-    rate.tap do |hr|
-      hr.project    = @project
-      hr.user       = @user
-      hr.valid_from = today
-      hr.rate = parse_number_string_to_number(params[:rate])
-    end
-
-    if rate.save
-      if request.xhr?
-        render :update do |page|
-          page.replace_html "rate_for_#{@user.id}",
-                            link_to(number_to_currency(rate.rate), action: "edit", id: @user, project_id: @project)
-        end
-      else
-        flash[:notice] = t(:notice_successful_update)
-        redirect_to action: "index"
-      end
-    end
-  end
-
   private
 
   def update_rates(user, project, added_rates, changed_rates)
@@ -164,17 +133,23 @@ class HourlyRatesController < ApplicationController
 
   def delete_rates(user, project)
     if project.present?
-      user.rates.delete_all
+      user.rates.where(project:).delete_all
     else
       user.default_rates.delete_all
     end
   end
 
   def find_project
-    @project = Project.find(params[:project_id])
+    @project = Project.visible.find(params[:project_id])
   end
 
   def find_user
-    @user = params[:id] ? User.find(params[:id]) : User.current
+    @user = if params[:id].blank?
+              User.current
+            elsif @project
+              User.in_project(@project).visible.find(params[:id])
+            else
+              User.visible.find(params[:id])
+            end
   end
 end

modules/costs/spec/controllers/hourly_rates_controller_spec.rb

@@ -31,11 +31,60 @@ require_relative "../spec_helper"
 RSpec.describe HourlyRatesController do
   shared_let(:admin) { create(:admin) }
 
-  let(:user) { create(:user) }
+  let(:user) { create(:user, member_with_permissions: { project => permissions }) }
+  let(:permissions) { [:view_hourly_rates] }
+  let(:project) { create(:project) }
   let(:default_rate) { create(:default_hourly_rate, user:) }
 
-  describe "PUT update" do
-    describe "WHEN trying to update with an invalid rate value" do
+  describe "#show" do
+    before do
+      login_as(user)
+    end
+
+    context "when accessing the hourly rates of a user with a non exisiting project" do
+      it "responds with 404" do
+        get :show, params: { project_id: "this-does-not-exist", id: user.id }
+        expect(response).to have_http_status(:not_found)
+      end
+    end
+
+    context "when accessing the hourly rates of a user without being a member of the project" do
+      let(:user) { create(:user) }
+
+      it "responds with 404" do
+        get :show, params: { project_id: project.id, id: user.id }
+        expect(response).to have_http_status(:not_found)
+      end
+    end
+
+    context "when accessing the hourly rates of a user being a member of the project without permission to view hourly rates" do
+      let(:permissions) { [] }
+
+      it "responds with 403" do
+        get :show, params: { project_id: project.id, id: user.id }
+        expect(response).to have_http_status(:forbidden)
+      end
+    end
+
+    context "when accessing the hourly rates of a user with permission to view hourly rates in the project" do
+      it "responds with 200" do
+        get :show, params: { project_id: project.id, id: user.id }
+        expect(response).to have_http_status(:ok)
+      end
+    end
+
+    context "when accessing the hourly rates of a user that is not visible to me" do
+      let(:other_user) { create(:user) }
+
+      it "responds with 404" do
+        get :show, params: { project_id: project.id, id: other_user.id }
+        expect(response).to have_http_status(:not_found)
+      end
+    end
+  end
+
+  describe "#update" do
+    describe "when trying to update with an invalid rate value" do
       let(:params) do
         {
           id: user.id,
@@ -59,5 +108,144 @@ RSpec.describe HourlyRatesController do
         expect(actual_message).to eq(I18n.t("activerecord.errors.messages.not_a_number"))
       end
     end
+
+    context "when the user does not have the permission to edit hourly rates" do
+      let(:user) { create(:user, member_with_permissions: { project => [:view_hourly_rates] }) }
+      let(:params) do
+        {
+          id: user.id,
+          project_id: project.id,
+          user: {
+            "existing_rate_attributes" => {
+              default_rate.id.to_s => {
+                "valid_from" => default_rate.valid_from.to_s,
+                "rate" => "25"
+              }
+            }
+          }
+        }
+      end
+
+      before do
+        as_logged_in_user(user) do
+          post :update, params:
+        end
+      end
+
+      it "responds with 403 Forbidden" do
+        expect(response).to have_http_status(:forbidden)
+      end
+    end
+
+    context "when trying to update the rate of a user that is not a member of the project" do
+      let(:other_user) { create(:user) }
+      let(:params) do
+        {
+          id: other_user.id,
+          project_id: project.id,
+          user: {
+            "existing_rate_attributes" => {
+              default_rate.id.to_s => {
+                "valid_from" => default_rate.valid_from.to_s,
+                "rate" => "25"
+              }
+            }
+          }
+        }
+      end
+
+      before do
+        as_logged_in_user(admin) do
+          post :update, params:
+        end
+      end
+
+      it "responds with 404 Not Found" do
+        expect(response).to have_http_status(:not_found)
+      end
+    end
+
+    context "when updating and adding a new rate" do
+      let!(:default_rate) { create(:default_hourly_rate, user:, valid_from: 1.month.ago) }
+      let!(:hourly_rate) { create(:hourly_rate, user:, project:, valid_from: 1.day.ago) }
+
+      let(:params) do
+        {
+          id: user.id,
+          project_id: project.id,
+          user: {
+            "existing_rate_attributes" => {
+              hourly_rate.id.to_s => {
+                "valid_from" => 2.days.ago.to_s,
+                "rate" => "25"
+              }
+            },
+            "new_rate_attributes" => {
+              "0" => {
+                "valid_from" => 1.day.from_now.to_s,
+                "rate" => "30"
+              }
+            }
+          }
+        }
+      end
+
+      it "updates the existing rate and creates a new one" do
+        # testing before state
+
+        expect(HourlyRate.at_date_for_user_in_project(3.days.ago, user, project)).to eq(default_rate)
+        expect(HourlyRate.at_date_for_user_in_project(2.days.ago, user, project)).to eq(default_rate)
+        expect(HourlyRate.at_date_for_user_in_project(1.day.ago, user, project)).to eq(hourly_rate)
+        expect(HourlyRate.at_date_for_user_in_project(1.day.from_now, user, project)).to eq(hourly_rate)
+
+        login_as (admin)
+
+        expect do
+          post :update, params: params
+        end.to change { user.rates.reload.size }.from(1).to(2)
+
+        newest_rate = user.rates.last
+
+        expect(HourlyRate.at_date_for_user_in_project(3.days.ago, user, project)).to eq(default_rate)
+        expect(HourlyRate.at_date_for_user_in_project(2.days.ago, user, project)).to eq(hourly_rate)
+        expect(HourlyRate.at_date_for_user_in_project(1.day.ago, user, project)).to eq(hourly_rate)
+        expect(HourlyRate.at_date_for_user_in_project(1.day.from_now, user, project)).to eq(newest_rate)
+      end
+    end
+
+    context "when deleting all rates of a user" do
+      let!(:hourly_rate) { create(:hourly_rate, user:, project:) }
+
+      let(:params) do
+        {
+          id: user.id,
+          project_id: project.id
+        }
+      end
+
+      it "deletes all rates of the user for the project" do
+        login_as (admin)
+
+        expect do
+          post :update, params: params
+        end.to change { user.rates.reload.size }.from(1).to(0)
+
+        expect(user.rates.where(project:)).to be_empty
+      end
+
+      context "with rates in other projects" do
+        let(:other_project) { create(:project) }
+        let!(:other_rate) { create(:hourly_rate, user:, project: other_project) }
+
+        it "only deletes the rates for the specified project" do
+          login_as (admin)
+
+          post :update, params: params
+
+          expect(user.rates.where(project:)).to be_empty
+          expect(user.rates.where(project: other_project)).not_to be_empty
+        end
+      end
+    end
   end
 end