From 00b875deab7f0800ecf0677b9a2e2b2c74a6d1ab Mon Sep 17 00:00:00 2001 From: Klaus Zanders Date: Thu, 12 Feb 2026 13:11:15 +0100 Subject: [PATCH] Correctly scope hourly rate access and prevent deletion in other projects --- .../controllers/hourly_rates_controller.rb | 57 ++--- .../hourly_rates_controller_spec.rb | 194 +++++++++++++++++- 2 files changed, 207 insertions(+), 44 deletions(-) diff --git a/modules/costs/app/controllers/hourly_rates_controller.rb b/modules/costs/app/controllers/hourly_rates_controller.rb index fbed7718979..9401bfefdc3 100644 --- a/modules/costs/app/controllers/hourly_rates_controller.rb +++ b/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 diff --git a/modules/costs/spec/controllers/hourly_rates_controller_spec.rb b/modules/costs/spec/controllers/hourly_rates_controller_spec.rb index f5ce4ecb4e0..fa1879a4f32 100644 --- a/modules/costs/spec/controllers/hourly_rates_controller_spec.rb +++ b/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