fix: authorize mark_as_read in Notification controller (rubyforgood#5850

)

When we upgraded Noticed we added skipping of callbacks to the
NotificationsController. This resulted in issues with unauthed users
hitting the notifications page.

To fix this we removed the skipping of callbacks, however, one of the
callbacks being skipped was a callback to authorize the Notification.
Authorization logic had never been added so now mark as read would not
work.

This PR added authorization logic as well as request and policy specs
for the controller.

app/controllers/notifications_controller.rb

@@ -1,13 +1,25 @@
 class NotificationsController < ApplicationController
+  after_action :verify_authorized
+  before_action :set_notification, only: %i[mark_as_read]
+
   def index
+    authorize Noticed::Notification, policy_class: NotificationPolicy
+
     @deploy_time = Health.instance.latest_deploy_time
     @notifications = current_user.notifications.includes([:event]).newest_first
     @patch_notes = PatchNote.notes_available_for_user(current_user)
   end
 
   def mark_as_read
-    @notification = Noticed::Notification.find(params[:id])
+    authorize @notification, policy_class: NotificationPolicy
+
     @notification.mark_as_read unless @notification.read?
     redirect_to @notification.event.url
   end
+
+  private
+
+  def set_notification
+    @notification = Noticed::Notification.find(params[:id])
+  end
 end

app/policies/notification_policy.rb

@@ -0,0 +1,9 @@
+class NotificationPolicy < ApplicationPolicy
+  def index?
+    admin_or_supervisor_or_volunteer?
+  end
+
+  def mark_as_read?
+    record&.recipient == user
+  end
+end

spec/policies/notification_policy_spec.rb

@@ -0,0 +1,44 @@
+require "rails_helper"
+
+RSpec.describe NotificationPolicy, type: :policy do
+  subject { described_class }
+
+  let(:recipient) { create(:volunteer) }
+  let(:casa_admin) { create(:casa_admin) }
+  let(:volunteer) { build(:volunteer) }
+  let(:supervisor) { build(:supervisor) }
+
+  permissions :index? do
+    it "allows any volunteer" do
+      is_expected.to permit(casa_admin)
+    end
+
+    it "allows any supervisor" do
+      is_expected.to permit(supervisor)
+    end
+
+    it "allows any admin" do
+      is_expected.to permit(volunteer)
+    end
+  end
+
+  permissions :mark_as_read? do
+    let(:notification) { create(:notification, recipient: recipient) }
+
+    it "allows recipient" do
+      is_expected.to permit(recipient, notification)
+    end
+
+    it "does not allow other volunteer" do
+      is_expected.to_not permit(volunteer, notification)
+    end
+
+    it "does not permit other supervisor" do
+      is_expected.to_not permit(supervisor, notification)
+    end
+
+    it "does not permit other admin" do
+      is_expected.to_not permit(casa_admin, notification)
+    end
+  end
+end

spec/requests/notifications_spec.rb

@@ -114,4 +114,49 @@
       end
     end
   end
+
+  describe "POST #mark_as_read" do
+    let(:user) { create(:volunteer) }
+    let(:notification) { create(:notification, :followup_with_note, recipient: user, read_at: nil) }
+
+    before { sign_in user }
+
+    context "when user is authorized" do
+      it "marks the notification as read" do
+        post mark_as_read_notification_path(notification)
+
+        expect(notification.reload.read_at).not_to be_nil
+      end
+
+      it "redirects to the notification event URL" do
+        post mark_as_read_notification_path(notification)
+
+        expect(response).to redirect_to(notification.event.url)
+      end
+    end
+
+    context "when user is not authorized" do
+      let(:other_user) { create(:volunteer) }
+
+      before { sign_in other_user }
+
+      it "does not mark the notification as read" do
+        post mark_as_read_notification_path(notification)
+
+        expect(notification.reload.read_at).to be_nil
+      end
+
+      it "redirects to root" do
+        post mark_as_read_notification_path(notification)
+
+        expect(response).to redirect_to(root_path)
+      end
+    end
+
+    it "does not mark the notification as read if it is already read" do
+      notification = create(:notification, :followup_read, recipient: user)
+
+      expect { post mark_as_read_notification_path(notification) }.not_to(change { notification.reload.read_at })
+    end
+  end
 end