From 6c67eb9ba0e526da8bc6f99fc530dcab340967bc Mon Sep 17 00:00:00 2001 From: Pranav Date: Mon, 11 May 2026 23:57:29 -0700 Subject: [PATCH] fix(notifications): Respect conversation access when notifying agents (#14412) Agents with limited custom roles were receiving notifications (creation, assignment, mentions, new messages, SLA) for conversations they couldn't actually open. For example, an agent whose custom role only grants `conversation_unassigned_manage` was getting notified about conversations assigned to other agents. Notifications now go through the same `ConversationPolicy#show?` check that gates the conversation view itself, so an agent only gets notified for conversations they're permitted to see. Administrators and agents without custom roles are unaffected. --------- Co-authored-by: Sojan Jose Co-authored-by: Muhsin Keloth --- app/builders/notification_builder.rb | 15 +++ app/services/messages/mention_service.rb | 2 +- spec/builders/notification_builder_spec.rb | 65 ++++++++++- .../builders/notification_builder_spec.rb | 106 ++++++++++++++++++ spec/enterprise/models/sla_event_spec.rb | 1 + .../services/messages/mention_service_spec.rb | 20 ++++ 6 files changed, 207 insertions(+), 2 deletions(-) create mode 100644 spec/enterprise/builders/notification_builder_spec.rb diff --git a/app/builders/notification_builder.rb b/app/builders/notification_builder.rb index d5461eb4e..e09e44d50 100644 --- a/app/builders/notification_builder.rb +++ b/app/builders/notification_builder.rb @@ -27,6 +27,8 @@ class NotificationBuilder return if notification_type == 'conversation_creation' && !user_subscribed_to_notification? # skip notifications for blocked conversations except for user mentions return if primary_actor.contact.blocked? && notification_type != 'conversation_mention' + # respect conversation access (inbox/team membership and custom-role permissions) + return unless user_can_access_conversation? user.notifications.create!( notification_type: notification_type, @@ -36,4 +38,17 @@ class NotificationBuilder secondary_actor: secondary_actor || current_user ) end + + def user_can_access_conversation? + conversation = primary_actor.is_a?(Conversation) ? primary_actor : primary_actor.try(:conversation) + return true if conversation.blank? + + account_user = AccountUser.find_by(account_id: account.id, user_id: user.id) + return false if account_user.blank? + + ConversationPolicy.new( + { user: user, account: account, account_user: account_user }, + conversation + ).show? + end end diff --git a/app/services/messages/mention_service.rb b/app/services/messages/mention_service.rb index 8171f8168..1d42498a9 100644 --- a/app/services/messages/mention_service.rb +++ b/app/services/messages/mention_service.rb @@ -8,8 +8,8 @@ class Messages::MentionService return if validated_mentioned_ids.blank? Conversations::UserMentionJob.perform_later(validated_mentioned_ids, message.conversation.id, message.account.id) - generate_notifications_for_mentions(validated_mentioned_ids) add_mentioned_users_as_participants(validated_mentioned_ids) + generate_notifications_for_mentions(validated_mentioned_ids) end private diff --git a/spec/builders/notification_builder_spec.rb b/spec/builders/notification_builder_spec.rb index 1b28f6adf..f191f0485 100644 --- a/spec/builders/notification_builder_spec.rb +++ b/spec/builders/notification_builder_spec.rb @@ -6,9 +6,11 @@ describe NotificationBuilder do describe '#perform' do let!(:account) { create(:account) } let!(:user) { create(:user, account: account) } - let!(:primary_actor) { create(:conversation, account: account) } + let!(:inbox) { create(:inbox, account: account) } + let!(:primary_actor) { create(:conversation, account: account, inbox: inbox) } before do + create(:inbox_member, user: user, inbox: inbox) notification_setting = user.notification_settings.find_by(account_id: account.id) notification_setting.selected_email_flags = [:email_conversation_creation] notification_setting.selected_push_flags = [:push_conversation_creation] @@ -97,5 +99,66 @@ describe NotificationBuilder do ).perform end.to change { user.notifications.count }.by(1) end + + context 'when the user does not have access to the conversation' do + let!(:outsider) { create(:user, account: account) } + + it 'does not create a notification for an agent without inbox or team access' do + expect do + described_class.new( + notification_type: 'conversation_creation', + user: outsider, + account: account, + primary_actor: primary_actor + ).perform + end.not_to(change { outsider.notifications.count }) + end + + it 'still creates a notification for administrators regardless of inbox membership' do + admin = create(:user, account: account, role: :administrator) + admin_setting = admin.notification_settings.find_by(account_id: account.id) + admin_setting.selected_email_flags = [:email_conversation_creation] + admin_setting.selected_push_flags = [:push_conversation_creation] + admin_setting.save! + + expect do + described_class.new( + notification_type: 'conversation_creation', + user: admin, + account: account, + primary_actor: primary_actor + ).perform + end.to change { admin.notifications.count }.by(1) + end + + it 'does not create a notification when the user is not part of the account' do + unrelated_user = create(:user) + + expect do + described_class.new( + notification_type: 'conversation_creation', + user: unrelated_user, + account: account, + primary_actor: primary_actor + ).perform + end.not_to(change { unrelated_user.notifications.count }) + end + + it 'derives the conversation from a message primary_actor' do + outsider_inbox = create(:inbox, account: account) + message = create(:message, account: account, inbox: outsider_inbox, + conversation: create(:conversation, account: account, inbox: outsider_inbox)) + + expect do + described_class.new( + notification_type: 'conversation_mention', + user: outsider, + account: account, + primary_actor: message.conversation, + secondary_actor: message + ).perform + end.not_to(change { outsider.notifications.count }) + end + end end end diff --git a/spec/enterprise/builders/notification_builder_spec.rb b/spec/enterprise/builders/notification_builder_spec.rb new file mode 100644 index 000000000..e25034f0c --- /dev/null +++ b/spec/enterprise/builders/notification_builder_spec.rb @@ -0,0 +1,106 @@ +require 'rails_helper' + +describe NotificationBuilder do + describe '#perform with custom role permissions' do + let!(:account) { create(:account) } + let!(:agent) { create(:user, account: account, role: :agent) } + let!(:inbox) { create(:inbox, account: account) } + let!(:account_user) { agent.account_users.find_by(account: account) } + + before do + create(:inbox_member, user: agent, inbox: inbox) + notification_setting = agent.notification_settings.find_by(account_id: account.id) + notification_setting.selected_email_flags = [:email_conversation_creation] + notification_setting.selected_push_flags = [:push_conversation_creation] + notification_setting.save! + end + + def build_notification(conversation, type: 'conversation_creation') + described_class.new( + notification_type: type, + user: agent, + account: account, + primary_actor: conversation + ).perform + end + + context 'when the agent has conversation_manage permission' do + before do + custom_role = create(:custom_role, account: account, permissions: ['conversation_manage']) + account_user.update!(custom_role: custom_role) + end + + it 'creates a notification for any inbox conversation' do + conversation = create(:conversation, account: account, inbox: inbox) + + expect { build_notification(conversation) }.to change { agent.notifications.count }.by(1) + end + end + + context 'when the agent has conversation_unassigned_manage permission' do + before do + custom_role = create(:custom_role, account: account, permissions: ['conversation_unassigned_manage']) + account_user.update!(custom_role: custom_role) + end + + it 'creates a notification for unassigned conversations' do + conversation = create(:conversation, account: account, inbox: inbox, assignee: nil) + + expect { build_notification(conversation) }.to change { agent.notifications.count }.by(1) + end + + it 'creates a notification for conversations assigned to the agent' do + conversation = create(:conversation, account: account, inbox: inbox, assignee: agent) + + expect { build_notification(conversation) }.to change { agent.notifications.count }.by(1) + end + + it 'does not create a notification for conversations assigned to someone else' do + other_agent = create(:user, account: account, role: :agent) + create(:inbox_member, user: other_agent, inbox: inbox) + conversation = create(:conversation, account: account, inbox: inbox, assignee: other_agent) + + expect { build_notification(conversation) }.not_to(change { agent.notifications.count }) + end + end + + context 'when the agent has conversation_participating_manage permission' do + before do + custom_role = create(:custom_role, account: account, permissions: ['conversation_participating_manage']) + account_user.update!(custom_role: custom_role) + end + + it 'creates a notification for conversations assigned to the agent' do + conversation = create(:conversation, account: account, inbox: inbox, assignee: agent) + + expect { build_notification(conversation) }.to change { agent.notifications.count }.by(1) + end + + it 'creates a notification for conversations the agent participates in' do + conversation = create(:conversation, account: account, inbox: inbox, assignee: nil) + create(:conversation_participant, conversation: conversation, account: account, user: agent) + + expect { build_notification(conversation) }.to change { agent.notifications.count }.by(1) + end + + it 'does not create a notification for unassigned conversations the agent does not participate in' do + conversation = create(:conversation, account: account, inbox: inbox, assignee: nil) + + expect { build_notification(conversation) }.not_to(change { agent.notifications.count }) + end + end + + context 'when the custom role grants no conversation permissions' do + before do + custom_role = create(:custom_role, account: account, permissions: ['contact_manage']) + account_user.update!(custom_role: custom_role) + end + + it 'does not create a notification' do + conversation = create(:conversation, account: account, inbox: inbox, assignee: agent) + + expect { build_notification(conversation) }.not_to(change { agent.notifications.count }) + end + end + end +end diff --git a/spec/enterprise/models/sla_event_spec.rb b/spec/enterprise/models/sla_event_spec.rb index 8a839d626..0600fb80f 100644 --- a/spec/enterprise/models/sla_event_spec.rb +++ b/spec/enterprise/models/sla_event_spec.rb @@ -55,6 +55,7 @@ RSpec.describe SlaEvent, type: :model do before do # to ensure notifications are not sent to other users create(:user, account: account) + create(:inbox_member, inbox: inbox, user: assignee) create(:inbox_member, inbox: inbox, user: participant) create(:conversation_participant, conversation: conversation, user: participant) end diff --git a/spec/services/messages/mention_service_spec.rb b/spec/services/messages/mention_service_spec.rb index a7bddcc4e..e04a0e19b 100644 --- a/spec/services/messages/mention_service_spec.rb +++ b/spec/services/messages/mention_service_spec.rb @@ -117,6 +117,26 @@ describe Messages::MentionService do expect(conversation.conversation_participants.map(&:user_id)).to include(first_agent.id) end + + it 'adds the mentioned user as a participant before generating the notification' do + message = build( + :message, + conversation: conversation, + account: account, + content: "hi (mention://user/#{first_agent.id}/#{first_agent.name})", + private: true + ) + + participant_user_ids_when_notified = nil + allow(NotificationBuilder).to receive(:new) do |**_kwargs| + participant_user_ids_when_notified = conversation.conversation_participants.reload.map(&:user_id) + builder + end + + described_class.new(message: message).perform + + expect(participant_user_ids_when_notified).to include(first_agent.id) + end end context 'when message contains multiple user mentions' do