Refactor ApplicationPolicy #same_org? (rubyforgood#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

@@ -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."

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

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?
 

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

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

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 },

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}
 

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?

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?

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?

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

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

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) }

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