diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index 5566dd488e3..ca7e548a70d 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -40,7 +40,7 @@ class NotificationsController < ApplicationController end if notifications.present? && !(params.has_key?(:silent) || @readonly_mode) - if changed = current_user.bump_last_seen_notification! + if current_user.bump_last_seen_notification! current_user.reload current_user.publish_notifications_state end @@ -57,6 +57,8 @@ class NotificationsController < ApplicationController end end + notifications = filter_inaccessible_notifications(notifications) + json = { notifications: serialize_data(notifications, NotificationSerializer), seen_notification_id: current_user.seen_notification_id @@ -82,6 +84,7 @@ class NotificationsController < ApplicationController total_rows = notifications.dup.count notifications = notifications.offset(offset).limit(60) + notifications = filter_inaccessible_notifications(notifications) render_json_dump(notifications: serialize_data(notifications, NotificationSerializer), total_rows_notifications: total_rows, seen_notification_id: user.seen_notification_id, @@ -145,4 +148,9 @@ class NotificationsController < ApplicationController render_json_dump(NotificationSerializer.new(@notification, scope: guardian, root: false)) end + def filter_inaccessible_notifications(notifications) + topic_ids = notifications.map { |n| n.topic_id }.compact.uniq + accessible_topic_ids = guardian.can_see_topic_ids(topic_ids: topic_ids) + notifications.select { |n| n.topic_id.blank? || accessible_topic_ids.include?(n.topic_id) } + end end diff --git a/spec/requests/notifications_controller_spec.rb b/spec/requests/notifications_controller_spec.rb index f263fef37da..a087bb2f60b 100644 --- a/spec/requests/notifications_controller_spec.rb +++ b/spec/requests/notifications_controller_spec.rb @@ -339,6 +339,50 @@ RSpec.describe NotificationsController do expect(response.status).to eq(404) end end + + context "with notifications for inaccessible topics" do + fab!(:sender) { Fabricate.build(:topic_allowed_user, user: Fabricate(:coding_horror)) } + fab!(:allowed_user) { Fabricate.build(:topic_allowed_user, user: user) } + fab!(:another_allowed_user) { Fabricate.build(:topic_allowed_user, user: Fabricate(:user)) } + fab!(:allowed_pm) { Fabricate(:private_message_topic, topic_allowed_users: [sender, allowed_user, another_allowed_user]) } + fab!(:forbidden_pm) { Fabricate(:private_message_topic, topic_allowed_users: [sender, another_allowed_user]) } + fab!(:allowed_pm_notification) { Fabricate(:private_message_notification, user: user, topic: allowed_pm) } + fab!(:forbidden_pm_notification) { Fabricate(:private_message_notification, user: user, topic: forbidden_pm) } + + def expect_correct_notifications(response) + notification_ids = response.parsed_body["notifications"].map { |n| n["id"] } + expect(notification_ids).to include(allowed_pm_notification.id) + expect(notification_ids).to_not include(forbidden_pm_notification.id) + end + + context "with 'recent' filter" do + it "doesn't include notifications from topics the user isn't allowed to see" do + SiteSetting.enable_experimental_sidebar_hamburger = true + get "/notifications.json", params: { recent: true } + expect(response.status).to eq(200) + expect_correct_notifications(response) + + SiteSetting.enable_experimental_sidebar_hamburger = false + get "/notifications.json", params: { recent: true } + expect(response.status).to eq(200) + expect_correct_notifications(response) + end + end + + context "without 'recent' filter" do + it "doesn't include notifications from topics the user isn't allowed to see" do + SiteSetting.enable_experimental_sidebar_hamburger = true + get "/notifications.json" + expect(response.status).to eq(200) + expect_correct_notifications(response) + + SiteSetting.enable_experimental_sidebar_hamburger = false + get "/notifications.json" + expect(response.status).to eq(200) + expect_correct_notifications(response) + end + end + end end it 'should succeed' do