From 8b416e30ef8bcfb9b17a589b2b6fcb166b40fd08 Mon Sep 17 00:00:00 2001 From: Jon Roberts Date: Sat, 31 Aug 2024 11:20:59 -0400 Subject: [PATCH] Refactor ApplicationPolicy #same_org? (#6004) * Refactor ApplicationPolicy #same_org? Use model associations to check orgs match. Override in inherited Policy classes as needed (CasaOrg). Change index actions that went through same_org? and member actions that did not authorize record. * same_org? spec * specs for nil user and user.casa_org * record double org/other clarity --- app/controllers/contact_topics_controller.rb | 7 +-- app/models/case_assignment.rb | 1 + app/models/case_contact.rb | 1 + app/models/checklist_item.rb | 1 + app/models/contact_type.rb | 1 + app/models/court_date.rb | 1 + app/models/followup.rb | 1 + app/models/learning_hour.rb | 1 + app/policies/application_policy.rb | 24 ++------ app/policies/casa_case_policy.rb | 4 +- app/policies/casa_org_policy.rb | 4 ++ spec/policies/application_policy_spec.rb | 65 ++++++++++++++++++++ spec/policies/case_assignment_policy_spec.rb | 2 +- spec/requests/contact_topics_spec.rb | 8 ++- 14 files changed, 93 insertions(+), 28 deletions(-) diff --git a/app/controllers/contact_topics_controller.rb b/app/controllers/contact_topics_controller.rb index 3a10428b7b..3667fb69c2 100644 --- a/app/controllers/contact_topics_controller.rb +++ b/app/controllers/contact_topics_controller.rb @@ -4,9 +4,8 @@ class ContactTopicsController < ApplicationController # GET /contact_topics/new def new - authorize ContactTopic - contact_topic = ContactTopic.new(casa_org_id: current_user.casa_org_id) - @contact_topic = contact_topic + @contact_topic = ContactTopic.new(casa_org_id: current_user.casa_org_id) + authorize @contact_topic end # GET /contact_topics/1/edit @@ -16,8 +15,8 @@ def edit # POST /contact_topics or /contact_topics.json def create - authorize ContactTopic @contact_topic = ContactTopic.new(contact_topic_params) + authorize @contact_topic if @contact_topic.save redirect_to edit_casa_org_path(current_organization), notice: "Contact topic was successfully created." diff --git a/app/models/case_assignment.rb b/app/models/case_assignment.rb index 7a47bb6e95..0b92011c26 100644 --- a/app/models/case_assignment.rb +++ b/app/models/case_assignment.rb @@ -1,6 +1,7 @@ class CaseAssignment < ApplicationRecord belongs_to :casa_case belongs_to :volunteer, class_name: "User" + has_one :casa_org, through: :casa_case validates :casa_case_id, uniqueness: {scope: :volunteer_id} # only 1 row allowed per case-volunteer pair validates :volunteer, presence: true diff --git a/app/models/case_contact.rb b/app/models/case_contact.rb index 462b3e96cc..7a8451074b 100644 --- a/app/models/case_contact.rb +++ b/app/models/case_contact.rb @@ -33,6 +33,7 @@ class CaseContact < ApplicationRecord # Draft support requires the casa_case to be nil if the contact is in_progress belongs_to :casa_case, optional: true + has_one :casa_org, through: :casa_case validates :casa_case_id, presence: true, if: :active? validate :draft_case_ids_not_empty, unless: :started? diff --git a/app/models/checklist_item.rb b/app/models/checklist_item.rb index 713709d089..e73a6fa22b 100644 --- a/app/models/checklist_item.rb +++ b/app/models/checklist_item.rb @@ -1,5 +1,6 @@ class ChecklistItem < ApplicationRecord belongs_to :hearing_type + has_one :casa_org, through: :hearing_type validates :category, presence: true validates :description, presence: true end diff --git a/app/models/contact_type.rb b/app/models/contact_type.rb index d4f70ea0bd..02e34d58d5 100644 --- a/app/models/contact_type.rb +++ b/app/models/contact_type.rb @@ -1,5 +1,6 @@ class ContactType < ApplicationRecord belongs_to :contact_type_group + has_one :casa_org, through: :contact_type_group has_many :casa_case_contact_types has_many :casa_cases, through: :casa_case_contact_types diff --git a/app/models/court_date.rb b/app/models/court_date.rb index 424666fee9..b2bdaa9438 100644 --- a/app/models/court_date.rb +++ b/app/models/court_date.rb @@ -4,6 +4,7 @@ class CourtDate < ApplicationRecord belongs_to :casa_case + has_one :casa_org, through: :casa_case validates :date, presence: true validates :date, comparison: { less_than_or_equal_to: -> { 1.year.from_now }, diff --git a/app/models/followup.rb b/app/models/followup.rb index 440b581a12..ef09a5ed8f 100644 --- a/app/models/followup.rb +++ b/app/models/followup.rb @@ -1,6 +1,7 @@ class Followup < ApplicationRecord belongs_to :followupable, polymorphic: true, optional: true # TODO polymorph: remove optional after data is safely migrated belongs_to :case_contact + has_one :casa_org, through: :case_contact belongs_to :creator, class_name: "User" enum status: {requested: 0, resolved: 1} diff --git a/app/models/learning_hour.rb b/app/models/learning_hour.rb index 3b6ae5537e..8ea0e97a92 100644 --- a/app/models/learning_hour.rb +++ b/app/models/learning_hour.rb @@ -2,6 +2,7 @@ class LearningHour < ApplicationRecord belongs_to :user belongs_to :learning_hour_type belongs_to :learning_hour_topic, optional: true + has_one :casa_org, through: :user validates :duration_minutes, presence: true validates :duration_minutes, numericality: {greater_than: 0, message: "and hours (total duration) must be greater than 0"}, if: :zero_duration_hours? diff --git a/app/policies/application_policy.rb b/app/policies/application_policy.rb index 84b43ef090..3806a5955a 100644 --- a/app/policies/application_policy.rb +++ b/app/policies/application_policy.rb @@ -52,26 +52,10 @@ def is_admin? end def same_org? - case record - when CasaOrg - user.casa_org == record - when CasaAdmin, CasaCase, Volunteer, Supervisor, HearingType, ContactTypeGroup, ContactTopic - user.casa_org == record.casa_org - when CourtDate, CaseContact, CaseAssignment - user.casa_org == record&.casa_case&.casa_org - when LearningHour - user.casa_org == record&.user&.casa_org - when ChecklistItem - user.casa_org == record&.hearing_type&.casa_org - when ContactType - user.casa_org == record&.contact_type_group&.casa_org - when Followup - user.casa_org == record&.case_contact&.casa_case&.casa_org - when Class # Authorizing against collection, does not belong to org - true - else # Type not recognized, no auth since we can't verify the record - false - end + # NOTE: must have casa_org association on a Policy's associated Model + # that is: `has_one :casa_org, through: :some_association` (may need to define :some_association) + # do not use for collection actions (index), check user type & use policy_scope() on the collection + user&.casa_org.present? && user.casa_org == record&.casa_org end def is_admin_same_org? diff --git a/app/policies/casa_case_policy.rb b/app/policies/casa_case_policy.rb index e8f644ea0f..4b261e77a9 100644 --- a/app/policies/casa_case_policy.rb +++ b/app/policies/casa_case_policy.rb @@ -52,7 +52,7 @@ def assign_volunteers? end def can_see_filters? - admin_or_supervisor_same_org? + admin_or_supervisor? end alias_method :update_case_number?, :is_admin_same_org? @@ -96,7 +96,7 @@ def same_org_supervisor_admin? end def index? - admin_or_supervisor_or_volunteer_same_org? + admin_or_supervisor_or_volunteer? end alias_method :show?, :same_org_supervisor_admin_or_assigned? diff --git a/app/policies/casa_org_policy.rb b/app/policies/casa_org_policy.rb index 1943f4f91e..789a5b8aae 100644 --- a/app/policies/casa_org_policy.rb +++ b/app/policies/casa_org_policy.rb @@ -6,4 +6,8 @@ def edit? def update? record.users.include?(user) && is_admin? end + + def same_org? + user&.casa_org.present? && user.casa_org == record + end end diff --git a/spec/policies/application_policy_spec.rb b/spec/policies/application_policy_spec.rb index 300cf10ebc..53b5bcbcfa 100644 --- a/spec/policies/application_policy_spec.rb +++ b/spec/policies/application_policy_spec.rb @@ -7,6 +7,7 @@ let(:casa_admin) { build_stubbed(:casa_admin, casa_org: casa_org) } let(:supervisor) { build_stubbed(:supervisor, casa_org: casa_org) } let(:volunteer) { build_stubbed(:volunteer, casa_org: casa_org) } + let(:all_casa_admin) { build_stubbed(:all_casa_admin) } permissions :see_reports_page? do it "allows casa_admins" do @@ -87,4 +88,68 @@ end end end + + describe "#same_org?" do + let(:org_record) { double } + + before { allow(org_record).to receive(:casa_org).and_return(casa_org) } + + context "record with same casa_org" do + before { expect(org_record).to receive(:casa_org).and_return(casa_org) } + + permissions :same_org? do + it { is_expected.to permit(volunteer, org_record) } + it { is_expected.to permit(supervisor, org_record) } + it { is_expected.to permit(casa_admin, org_record) } + end + end + + context "record with different casa_org" do + let(:other_org_record) { double } + before { expect(other_org_record).to receive(:casa_org).and_return(build_stubbed(:casa_org)) } + + permissions :same_org? do + it { is_expected.to_not permit(volunteer, other_org_record) } + it { is_expected.to_not permit(supervisor, other_org_record) } + it { is_expected.to_not permit(casa_admin, other_org_record) } + end + end + + context "all_casa_admin user" do + it "raises a no method error for all_casa_admin.casa_org" do + expect { subject.new(all_casa_admin, org_record).same_org? }.to raise_error(NoMethodError) + end + end + + context "user with no casa_org" do + let(:volunteer) { build_stubbed(:volunteer, casa_org: nil) } + let(:supervisor) { build_stubbed(:supervisor, casa_org: nil) } + let(:casa_admin) { build_stubbed(:casa_admin, casa_org: nil) } + + permissions :same_org? do + it { is_expected.to_not permit(volunteer, org_record) } + it { is_expected.to_not permit(supervisor, org_record) } + it { is_expected.to_not permit(casa_admin, org_record) } + end + end + + context "no user" do + let(:user) { nil } + + permissions :same_org? do + it { is_expected.to_not permit(user, org_record) } + end + end + + context "called with a class instead of a record" do + let(:klass) { CasaCase } + + [:volunteer, :casa_admin, :supervisor].each do |user_type| + it "raises a no method error for #{user_type}" do + user = send(user_type) + expect { subject.new(user, klass).same_org? }.to raise_error(NoMethodError) + end + end + end + end end diff --git a/spec/policies/case_assignment_policy_spec.rb b/spec/policies/case_assignment_policy_spec.rb index d1e6e97ad1..2ef9186d77 100644 --- a/spec/policies/case_assignment_policy_spec.rb +++ b/spec/policies/case_assignment_policy_spec.rb @@ -5,7 +5,7 @@ let(:organization) { build(:casa_org) } let(:casa_admin) { build(:casa_admin, casa_org: organization) } - let(:casa_case) { build(:casa_case, casa_org: organization) } + let(:casa_case) { create(:casa_case, casa_org: organization) } let(:volunteer) { build(:volunteer, casa_org: organization) } let(:case_assignment) { build(:case_assignment, casa_case: casa_case, volunteer: volunteer) } let(:case_assignment_inactive) { build(:case_assignment, casa_case: casa_case, volunteer: volunteer, active: false) } diff --git a/spec/requests/contact_topics_spec.rb b/spec/requests/contact_topics_spec.rb index 01ee5fd4d0..4a7601dde2 100644 --- a/spec/requests/contact_topics_spec.rb +++ b/spec/requests/contact_topics_spec.rb @@ -56,7 +56,13 @@ end context "with invalid parameters" do - let(:attributes) { {casa_org_id: 0} } + let(:attributes) do + { + casa_org_id: casa_org.id, + question: "", + details: "" + } + end it "does not create a new ContactTopic" do expect do