From d732c59c970bce81a0d1a479362743e745271f09 Mon Sep 17 00:00:00 2001 From: baarkerlounger <5101747+baarkerlounger@users.noreply.github.com> Date: Wed, 3 Aug 2022 10:01:06 +0100 Subject: [PATCH] Return 404 when scheme not found (#804) * Return not found for schemes when... not found * Redundant check * Apply to all member actions * Add not found checks for location --- app/controllers/locations_controller.rb | 17 +++++++---- app/controllers/schemes_controller.rb | 33 ++++++++++++++++++++-- spec/requests/locations_controller_spec.rb | 27 +++++++++++++++++- spec/requests/schemes_controller_spec.rb | 16 +++++++++++ 4 files changed, 83 insertions(+), 10 deletions(-) diff --git a/app/controllers/locations_controller.rb b/app/controllers/locations_controller.rb index 46aceb650..de8a2d835 100644 --- a/app/controllers/locations_controller.rb +++ b/app/controllers/locations_controller.rb @@ -39,13 +39,18 @@ class LocationsController < ApplicationController end end - def edit; end + def edit + render_not_found and return unless @location && @scheme + end - def edit_name; end + def edit_name + render_not_found and return unless @location && @scheme + end def update - page = params[:location][:page] + render_not_found and return unless @location && @scheme + page = params[:location][:page] if @location.update(location_params) case page when "edit" @@ -77,12 +82,12 @@ private @scheme = if %w[new create index edit_name].include?(action_name) Scheme.find(params[:id]) else - @location.scheme + @location&.scheme end end def find_location - @location = params[:location_id].present? ? Location.find(params[:location_id]) : Location.find(params[:id]) + @location = params[:location_id].present? ? Location.find_by(id: params[:location_id]) : Location.find_by(id: params[:id]) end def authenticate_scope! @@ -90,7 +95,7 @@ private end def authenticate_action! - if %w[new edit update create index edit_name].include?(action_name) && !((current_user.organisation == @scheme.owning_organisation) || current_user.support?) + if %w[new edit update create index edit_name].include?(action_name) && !((current_user.organisation == @scheme&.owning_organisation) || current_user.support?) render_not_found and return end end diff --git a/app/controllers/schemes_controller.rb b/app/controllers/schemes_controller.rb index 2bae8ae94..c10784c2d 100644 --- a/app/controllers/schemes_controller.rb +++ b/app/controllers/schemes_controller.rb @@ -18,6 +18,7 @@ class SchemesController < ApplicationController def show @scheme = Scheme.find_by(id: params[:id]) + render_not_found and return unless @scheme end def new @@ -42,7 +43,13 @@ class SchemesController < ApplicationController end end + def edit + render_not_found and return unless @scheme + end + def update + render_not_found and return unless @scheme + check_answers = params[:scheme][:check_answers] page = params[:scheme][:page] @@ -64,34 +71,50 @@ class SchemesController < ApplicationController end def primary_client_group + render_not_found and return unless @scheme + render "schemes/primary_client_group" end def confirm_secondary_client_group + render_not_found and return unless @scheme + render "schemes/confirm_secondary" end def secondary_client_group + render_not_found and return unless @scheme + render "schemes/secondary_client_group" end def support + render_not_found and return unless @scheme + render "schemes/support" end def details + render_not_found and return unless @scheme + render "schemes/details" end def check_answers + render_not_found and return unless @scheme + render "schemes/check_answers" end def edit_name + render_not_found and return unless @scheme + render "schemes/edit_name" end def support_services_provider + render_not_found and return unless @scheme + render "schemes/support_services_provider" end @@ -193,11 +216,15 @@ private end def arrangement_type_set_to_same_org?(required_params) - arrangement_type_value(required_params[:arrangement_type]) == "D" || (required_params[:arrangement_type].blank? && @scheme.present? && @scheme.arrangement_type_same?) + return unless @scheme + + arrangement_type_value(required_params[:arrangement_type]) == "D" || (required_params[:arrangement_type].blank? && @scheme.arrangement_type_same?) end def arrangement_type_changed_to_different_org?(required_params) - @scheme.present? && @scheme.arrangement_type_same? && arrangement_type_value(required_params[:arrangement_type]) != "D" && required_params[:managing_organisation_id].blank? + return unless @scheme + + @scheme.arrangement_type_same? && arrangement_type_value(required_params[:arrangement_type]) != "D" && required_params[:managing_organisation_id].blank? end def arrangement_type_value(key) @@ -215,7 +242,7 @@ private def authenticate_scope! head :unauthorized and return unless current_user.data_coordinator? || current_user.support? - if %w[show locations primary_client_group confirm_secondary_client_group secondary_client_group support details check_answers edit_name].include?(action_name) && !((current_user.organisation == @scheme.owning_organisation) || current_user.support?) + if %w[show locations primary_client_group confirm_secondary_client_group secondary_client_group support details check_answers edit_name].include?(action_name) && !((current_user.organisation == @scheme&.owning_organisation) || current_user.support?) render_not_found and return end end diff --git a/spec/requests/locations_controller_spec.rb b/spec/requests/locations_controller_spec.rb index 29387ffb3..ed2132809 100644 --- a/spec/requests/locations_controller_spec.rb +++ b/spec/requests/locations_controller_spec.rb @@ -454,7 +454,7 @@ RSpec.describe LocationsController, type: :request do expect(page).to have_content("Add a location to this scheme") end - context "when trying to new location to a scheme that belongs to another organisation" do + context "when trying to edit a location that belongs to another organisation" do let(:another_scheme) { FactoryBot.create(:scheme) } let(:another_location) { FactoryBot.create(:location, scheme: another_scheme) } @@ -463,6 +463,14 @@ RSpec.describe LocationsController, type: :request do expect(response).to have_http_status(:not_found) end end + + context "when the requested location does not exist" do + let(:location) { OpenStruct.new(id: (Location.maximum(:id) || 0) + 1) } + + it "returns not found" do + expect(response).to have_http_status(:not_found) + end + end end context "when signed in as a support user" do @@ -635,6 +643,15 @@ RSpec.describe LocationsController, type: :request do expect(page).to have_content(I18n.t("activerecord.errors.models.location.attributes.type_of_unit.blank")) end end + + context "when the requested location does not exist" do + let(:location) { OpenStruct.new(id: (Location.maximum(:id) || 0) + 1) } + let(:params) { {} } + + it "returns not found" do + expect(response).to have_http_status(:not_found) + end + end end context "when signed in as a support user" do @@ -1060,6 +1077,14 @@ RSpec.describe LocationsController, type: :request do expect(response).to have_http_status(:ok) expect(page).to have_content("Location name for #{location.postcode}") end + + context "when the requested location does not exist" do + let(:location) { OpenStruct.new(id: (Location.maximum(:id) || 0) + 1) } + + it "returns not found" do + expect(response).to have_http_status(:not_found) + end + end end end end diff --git a/spec/requests/schemes_controller_spec.rb b/spec/requests/schemes_controller_spec.rb index 47ba637e3..ce6805f35 100644 --- a/spec/requests/schemes_controller_spec.rb +++ b/spec/requests/schemes_controller_spec.rb @@ -250,6 +250,13 @@ RSpec.describe SchemesController, type: :request do expect(response).to have_http_status(:not_found) end end + + context "when the requested scheme does not exist" do + it "returns not found" do + get "/schemes/#{Scheme.maximum(:id) + 1}" + expect(response).to have_http_status(:not_found) + end + end end context "when signed in as a support user" do @@ -864,6 +871,15 @@ RSpec.describe SchemesController, type: :request do expect(scheme_to_update.reload.sensitive).to eq("Yes") end end + + context "when the requested scheme does not exist" do + let(:scheme_to_update) { OpenStruct.new(id: Scheme.maximum(:id) + 1) } + let(:params) { {} } + + it "returns not found" do + expect(response).to have_http_status(:not_found) + end + end end context "when signed in as a support" do