From fe8b33d778d4e23579fe727156be9062d0f9bc6e Mon Sep 17 00:00:00 2001
From: Yuri Bocharov <mail@yuribocharov.dev>
Date: Fri, 21 Jun 2024 01:20:21 -0400
Subject: [PATCH] fix: authorize mark_as_read in Notification controller
 (#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 | 14 ++++++-
 app/policies/notification_policy.rb         |  9 +++++
 spec/policies/notification_policy_spec.rb   | 44 ++++++++++++++++++++
 spec/requests/notifications_spec.rb         | 45 +++++++++++++++++++++
 4 files changed, 111 insertions(+), 1 deletion(-)
 create mode 100644 app/policies/notification_policy.rb
 create mode 100644 spec/policies/notification_policy_spec.rb

diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb
index 3a72da8667..0f7df4037d 100644
--- a/app/controllers/notifications_controller.rb
+++ b/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
diff --git a/app/policies/notification_policy.rb b/app/policies/notification_policy.rb
new file mode 100644
index 0000000000..db90976dce
--- /dev/null
+++ b/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
diff --git a/spec/policies/notification_policy_spec.rb b/spec/policies/notification_policy_spec.rb
new file mode 100644
index 0000000000..ae59fc6017
--- /dev/null
+++ b/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
diff --git a/spec/requests/notifications_spec.rb b/spec/requests/notifications_spec.rb
index ff1d0431db..b5b1be8740 100644
--- a/spec/requests/notifications_spec.rb
+++ b/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