From f7c3109eedbb5a99e2e5fd7d925992a06f52db75 Mon Sep 17 00:00:00 2001 From: Daniel Nottingham Date: Thu, 8 Jan 2026 18:34:21 -0300 Subject: [PATCH] fix: problem of two-step flow find and authorize --- .../api/v1/event_procedures_controller.rb | 5 +++- .../api/v1/hospitals_controller.rb | 5 +++- .../api/v1/medical_shifts_controller.rb | 5 +++- app/controllers/api/v1/patients_controller.rb | 5 +++- .../api/v1/procedures_controller.rb | 5 +++- app/operations/event_procedures/find.rb | 3 ++- app/operations/hospitals/find.rb | 3 ++- app/operations/medical_shifts/find.rb | 3 ++- app/operations/patients/find.rb | 3 ++- app/operations/procedures/find.rb | 3 ++- app/policies/hospital_policy.rb | 6 +++++ app/policies/procedure_policy.rb | 3 +++ spec/operations/event_procedures/find_spec.rb | 24 ++++++++++++++++++ spec/operations/hospitals/find_spec.rb | 23 +++++++++++++++++ spec/operations/medical_shifts/find_spec.rb | 24 ++++++++++++++++++ spec/operations/patients/find_spec.rb | 24 ++++++++++++++++++ spec/operations/procedures/find_spec.rb | 25 +++++++++++++++++++ .../api/v1/event_procedures_request_spec.rb | 4 +-- .../api/v1/medical_shifts_request_spec.rb | 4 +-- spec/requests/api/v1/patients_request_spec.rb | 24 +++--------------- 20 files changed, 166 insertions(+), 35 deletions(-) diff --git a/app/controllers/api/v1/event_procedures_controller.rb b/app/controllers/api/v1/event_procedures_controller.rb index 644cbc8c..4862ebd8 100644 --- a/app/controllers/api/v1/event_procedures_controller.rb +++ b/app/controllers/api/v1/event_procedures_controller.rb @@ -111,7 +111,10 @@ def event_procedure_params end def event_procedure - @event_procedure ||= EventProcedures::Find.result(id: params[:id]).event_procedure + @event_procedure ||= EventProcedures::Find.result( + id: params[:id], + scope: policy_scope(EventProcedure) + ).event_procedure end def serialized_event_procedures(event_procedures) diff --git a/app/controllers/api/v1/hospitals_controller.rb b/app/controllers/api/v1/hospitals_controller.rb index c50e31a8..5936eaa5 100644 --- a/app/controllers/api/v1/hospitals_controller.rb +++ b/app/controllers/api/v1/hospitals_controller.rb @@ -48,7 +48,10 @@ def destroy private def hospital - @hospital ||= Hospitals::Find.result(id: params[:id]).hospital + @hospital ||= Hospitals::Find.result( + id: params[:id], + scope: policy_scope(Hospital) + ).hospital end def hospital_params diff --git a/app/controllers/api/v1/medical_shifts_controller.rb b/app/controllers/api/v1/medical_shifts_controller.rb index bd997367..4f5b59c7 100644 --- a/app/controllers/api/v1/medical_shifts_controller.rb +++ b/app/controllers/api/v1/medical_shifts_controller.rb @@ -80,7 +80,10 @@ def destroy private def medical_shift - @medical_shift ||= MedicalShifts::Find.result(id: params[:id]).medical_shift + @medical_shift ||= MedicalShifts::Find.result( + id: params[:id], + scope: policy_scope(MedicalShift) + ).medical_shift end def medical_shift_params diff --git a/app/controllers/api/v1/patients_controller.rb b/app/controllers/api/v1/patients_controller.rb index a477a878..d6a38cd9 100644 --- a/app/controllers/api/v1/patients_controller.rb +++ b/app/controllers/api/v1/patients_controller.rb @@ -52,7 +52,10 @@ def destroy private def patient - @patient ||= Patients::Find.result(id: params[:id]).patient + @patient ||= Patients::Find.result( + id: params[:id], + scope: policy_scope(Patient) + ).patient end def patient_params diff --git a/app/controllers/api/v1/procedures_controller.rb b/app/controllers/api/v1/procedures_controller.rb index 7f2e1468..289eb1bd 100644 --- a/app/controllers/api/v1/procedures_controller.rb +++ b/app/controllers/api/v1/procedures_controller.rb @@ -50,7 +50,10 @@ def destroy private def procedure - @procedure ||= Procedures::Find.result(id: params[:id]).procedure + @procedure ||= Procedures::Find.result( + id: params[:id], + scope: policy_scope(Procedure) + ).procedure end def procedure_params diff --git a/app/operations/event_procedures/find.rb b/app/operations/event_procedures/find.rb index 3000128f..daabaee5 100644 --- a/app/operations/event_procedures/find.rb +++ b/app/operations/event_procedures/find.rb @@ -3,11 +3,12 @@ module EventProcedures class Find < Actor input :id, type: String + input :scope, type: Enumerable, default: -> { EventProcedure.all } output :event_procedure, type: EventProcedure def call - self.event_procedure = EventProcedure.find(id) + self.event_procedure = scope.find(id) end end end diff --git a/app/operations/hospitals/find.rb b/app/operations/hospitals/find.rb index 588ae114..a82b5359 100644 --- a/app/operations/hospitals/find.rb +++ b/app/operations/hospitals/find.rb @@ -3,11 +3,12 @@ module Hospitals class Find < Actor input :id, type: String + input :scope, type: Enumerable, default: -> { Hospital.all } output :hospital, type: Hospital def call - self.hospital = Hospital.find(id) + self.hospital = scope.find(id) end end end diff --git a/app/operations/medical_shifts/find.rb b/app/operations/medical_shifts/find.rb index 6d975a54..e23091d2 100644 --- a/app/operations/medical_shifts/find.rb +++ b/app/operations/medical_shifts/find.rb @@ -3,11 +3,12 @@ module MedicalShifts class Find < Actor input :id, type: String + input :scope, type: Enumerable, default: -> { MedicalShift.all } output :medical_shift, type: MedicalShift def call - self.medical_shift = MedicalShift.find(id) + self.medical_shift = scope.find(id) end end end diff --git a/app/operations/patients/find.rb b/app/operations/patients/find.rb index c7b69b34..616f8774 100644 --- a/app/operations/patients/find.rb +++ b/app/operations/patients/find.rb @@ -3,11 +3,12 @@ module Patients class Find < Actor input :id, type: String + input :scope, type: Enumerable, default: -> { Patient.all } output :patient, type: Patient def call - self.patient = Patient.find(id) + self.patient = scope.find(id) end end end diff --git a/app/operations/procedures/find.rb b/app/operations/procedures/find.rb index 9f9f9892..feb49f78 100644 --- a/app/operations/procedures/find.rb +++ b/app/operations/procedures/find.rb @@ -3,11 +3,12 @@ module Procedures class Find < Actor input :id, type: String + input :scope, type: Enumerable, default: -> { Procedure.all } output :procedure, type: Procedure def call - self.procedure = Procedure.find(id) + self.procedure = scope.find(id) end end end diff --git a/app/policies/hospital_policy.rb b/app/policies/hospital_policy.rb index 78ac57e8..f4204772 100644 --- a/app/policies/hospital_policy.rb +++ b/app/policies/hospital_policy.rb @@ -1,6 +1,12 @@ # frozen_string_literal: true class HospitalPolicy < ApplicationPolicy + class Scope < ApplicationScope + def resolve + scope.all + end + end + def index? user.present? end diff --git a/app/policies/procedure_policy.rb b/app/policies/procedure_policy.rb index 9a7f0baa..aead96b3 100644 --- a/app/policies/procedure_policy.rb +++ b/app/policies/procedure_policy.rb @@ -1,6 +1,9 @@ # frozen_string_literal: true class ProcedurePolicy < ApplicationPolicy + class Scope < CurrentUserScope + end + def index? user.present? end diff --git a/spec/operations/event_procedures/find_spec.rb b/spec/operations/event_procedures/find_spec.rb index d5670b60..5fd7e40b 100644 --- a/spec/operations/event_procedures/find_spec.rb +++ b/spec/operations/event_procedures/find_spec.rb @@ -27,5 +27,29 @@ end.to raise_error(ActiveRecord::RecordNotFound) end end + + context "when using a scope" do + let(:user) { create(:user) } + let(:event_procedure) { create(:event_procedure, user: user) } + let(:other_event_procedure) { create(:event_procedure) } + + it "returns found event_procedure when it exists in scope" do + result = described_class.result( + id: event_procedure.id.to_s, + scope: EventProcedure.where(user: user) + ) + + expect(result.event_procedure).to eq(event_procedure) + end + + it "raises ActiveRecord::RecordNotFound when record exists but not in scope" do + expect do + described_class.result( + id: other_event_procedure.id.to_s, + scope: EventProcedure.where(user: user) + ) + end.to raise_error(ActiveRecord::RecordNotFound) + end + end end end diff --git a/spec/operations/hospitals/find_spec.rb b/spec/operations/hospitals/find_spec.rb index d4201b51..f274602f 100644 --- a/spec/operations/hospitals/find_spec.rb +++ b/spec/operations/hospitals/find_spec.rb @@ -27,5 +27,28 @@ end.to raise_error(ActiveRecord::RecordNotFound) end end + + context "when using a scope" do + let!(:hospital) { create(:hospital, name: "Hospital A", address: "Address A") } + let!(:other_hospital) { create(:hospital, name: "Hospital B", address: "Address B") } + + it "returns found hospital when it exists in scope" do + result = described_class.result( + id: hospital.id.to_s, + scope: Hospital.where(name: "Hospital A") + ) + + expect(result.hospital).to eq(hospital) + end + + it "raises ActiveRecord::RecordNotFound when record exists but not in scope" do + expect do + described_class.result( + id: other_hospital.id.to_s, + scope: Hospital.where(name: "Hospital A") + ) + end.to raise_error(ActiveRecord::RecordNotFound) + end + end end end diff --git a/spec/operations/medical_shifts/find_spec.rb b/spec/operations/medical_shifts/find_spec.rb index e83d18c2..a29c212d 100644 --- a/spec/operations/medical_shifts/find_spec.rb +++ b/spec/operations/medical_shifts/find_spec.rb @@ -29,5 +29,29 @@ end.to raise_error(ActiveRecord::RecordNotFound) end end + + context "when using a scope" do + let(:user) { create(:user) } + let(:medical_shift) { create(:medical_shift, user: user) } + let(:other_medical_shift) { create(:medical_shift) } + + it "returns found medical_shift when it exists in scope" do + result = described_class.result( + id: medical_shift.id.to_s, + scope: MedicalShift.where(user: user) + ) + + expect(result.medical_shift).to eq(medical_shift) + end + + it "raises ActiveRecord::RecordNotFound when record exists but not in scope" do + expect do + described_class.result( + id: other_medical_shift.id.to_s, + scope: MedicalShift.where(user: user) + ) + end.to raise_error(ActiveRecord::RecordNotFound) + end + end end end diff --git a/spec/operations/patients/find_spec.rb b/spec/operations/patients/find_spec.rb index 61f24aaf..1ee5d07c 100644 --- a/spec/operations/patients/find_spec.rb +++ b/spec/operations/patients/find_spec.rb @@ -29,5 +29,29 @@ end.to raise_error(ActiveRecord::RecordNotFound) end end + + context "when using a scope" do + let(:user) { create(:user) } + let(:patient) { create(:patient, user: user) } + let(:other_patient) { create(:patient) } + + it "returns found patient when it exists in scope" do + result = described_class.result( + id: patient.id.to_s, + scope: Patient.where(user: user) + ) + + expect(result.patient).to eq(patient) + end + + it "raises ActiveRecord::RecordNotFound when record exists but not in scope" do + expect do + described_class.result( + id: other_patient.id.to_s, + scope: Patient.where(user: user) + ) + end.to raise_error(ActiveRecord::RecordNotFound) + end + end end end diff --git a/spec/operations/procedures/find_spec.rb b/spec/operations/procedures/find_spec.rb index e7dd6937..37f484e2 100644 --- a/spec/operations/procedures/find_spec.rb +++ b/spec/operations/procedures/find_spec.rb @@ -27,5 +27,30 @@ end.to raise_error(ActiveRecord::RecordNotFound) end end + + context "when using a scope" do + let(:user) { create(:user) } + let(:other_user) { create(:user) } + let(:procedure) { create(:procedure, custom: true, user: user) } + let(:other_procedure) { create(:procedure, custom: true, user: other_user) } + + it "returns found procedure when it exists in scope" do + result = described_class.result( + id: procedure.id.to_s, + scope: Procedure.where(user: user) + ) + + expect(result.procedure).to eq(procedure) + end + + it "raises ActiveRecord::RecordNotFound when record exists but not in scope" do + expect do + described_class.result( + id: other_procedure.id.to_s, + scope: Procedure.where(user: user) + ) + end.to raise_error(ActiveRecord::RecordNotFound) + end + end end end diff --git a/spec/requests/api/v1/event_procedures_request_spec.rb b/spec/requests/api/v1/event_procedures_request_spec.rb index 849c2941..c5d58e51 100644 --- a/spec/requests/api/v1/event_procedures_request_spec.rb +++ b/spec/requests/api/v1/event_procedures_request_spec.rb @@ -463,7 +463,7 @@ end context "with valid attributes and the record not belongs to the user" do - it "returns unauthorized" do + it "returns not_found to prevent ID enumeration" do other_user = create(:user) patient = create(:patient, user: other_user) health_insurance = create(:health_insurance) @@ -495,7 +495,7 @@ put "/api/v1/event_procedures/#{event_procedure.id}", params: params, headers: headers - expect(response).to have_http_status(:unauthorized) + expect(response).to have_http_status(:not_found) end end diff --git a/spec/requests/api/v1/medical_shifts_request_spec.rb b/spec/requests/api/v1/medical_shifts_request_spec.rb index 7c440bf8..7aaf04dc 100644 --- a/spec/requests/api/v1/medical_shifts_request_spec.rb +++ b/spec/requests/api/v1/medical_shifts_request_spec.rb @@ -325,12 +325,12 @@ context "when user is authenticated" do context "when updating another user's medical_shift" do - it "returns unauthorized status" do + it "returns not_found to prevent ID enumeration" do medical_shift = create(:medical_shift, workload: MedicalShifts::Workloads::SIX) params = { workload: MedicalShifts::Workloads::TWELVE } put api_v1_medical_shift_path(medical_shift), params: params, headers: headers - expect(response).to have_http_status(:unauthorized) + expect(response).to have_http_status(:not_found) end end diff --git a/spec/requests/api/v1/patients_request_spec.rb b/spec/requests/api/v1/patients_request_spec.rb index 8be36ad9..bec87374 100644 --- a/spec/requests/api/v1/patients_request_spec.rb +++ b/spec/requests/api/v1/patients_request_spec.rb @@ -168,30 +168,12 @@ context "when user is authenticated" do include_examples "delete request returns ok", Patient - context "when patient cannot be destroyed" do + context "when trying to destroy another user's patient" do let(:patient) { create(:patient) } - let(:errors) do - instance_double( - ActiveModel::Errors, - full_messages: ["Cannot delete record because of dependent event_procedures"] - ) - end - - before do - allow(Patient).to receive(:destroy).with(patient.id.to_s).and_return(patient) - allow(patient).to receive_messages(destroy: false, errors: errors) - allow(patient).to receive(:errors).and_return(errors) - end - it "returns unauthorized" do + it "returns not_found to prevent ID enumeration" do delete path, headers: headers - expect(response).to have_http_status(:unauthorized) - end - - it "returns errors" do - delete path, headers: headers - - expect(response.parsed_body).to include({ "error" => "not allowed to destroy? this Patient" }) + expect(response).to have_http_status(:not_found) end end