From 3bf35d89a3ad3bfdaaf6607f8f9ce7415472f37b Mon Sep 17 00:00:00 2001 From: Pranav Date: Wed, 13 Aug 2025 17:30:40 -0700 Subject: [PATCH] feat: Add support for team based permissions along with inbox --- .../accounts/conversations/base_controller.rb | 2 +- .../v1/accounts/conversations_controller.rb | 2 +- app/finders/conversation_finder.rb | 2 +- app/policies/conversation_policy.rb | 20 ++++ .../permission_filter_service.rb | 1 + spec/policies/conversation_policy_spec.rb | 73 +++++++++++++++ .../permission_filter_service_spec.rb | 92 ++++++++++++++++--- 7 files changed, 175 insertions(+), 17 deletions(-) diff --git a/app/controllers/api/v1/accounts/conversations/base_controller.rb b/app/controllers/api/v1/accounts/conversations/base_controller.rb index 500c7772f..223530e27 100644 --- a/app/controllers/api/v1/accounts/conversations/base_controller.rb +++ b/app/controllers/api/v1/accounts/conversations/base_controller.rb @@ -5,6 +5,6 @@ class Api::V1::Accounts::Conversations::BaseController < Api::V1::Accounts::Base def conversation @conversation ||= Current.account.conversations.find_by!(display_id: params[:conversation_id]) - authorize @conversation.inbox, :show? + authorize @conversation, :show? end end diff --git a/app/controllers/api/v1/accounts/conversations_controller.rb b/app/controllers/api/v1/accounts/conversations_controller.rb index e27869d82..4301eaa4a 100644 --- a/app/controllers/api/v1/accounts/conversations_controller.rb +++ b/app/controllers/api/v1/accounts/conversations_controller.rb @@ -160,7 +160,7 @@ class Api::V1::Accounts::ConversationsController < Api::V1::Accounts::BaseContro def conversation @conversation ||= Current.account.conversations.find_by!(display_id: params[:id]) - authorize @conversation.inbox, :show? + authorize @conversation, :show? end def inbox diff --git a/app/finders/conversation_finder.rb b/app/finders/conversation_finder.rb index d43ed31e7..e746a88df 100644 --- a/app/finders/conversation_finder.rb +++ b/app/finders/conversation_finder.rb @@ -72,7 +72,7 @@ class ConversationFinder def set_inboxes @inbox_ids = if params[:inbox_id] - @current_user.assigned_inboxes.where(id: params[:inbox_id]) + @current_user.assigned_inboxes.where(id: params[:inbox_id]).pluck(:id) else @current_user.assigned_inboxes.pluck(:id) end diff --git a/app/policies/conversation_policy.rb b/app/policies/conversation_policy.rb index 931e17435..d38d5f301 100644 --- a/app/policies/conversation_policy.rb +++ b/app/policies/conversation_policy.rb @@ -3,7 +3,27 @@ class ConversationPolicy < ApplicationPolicy true end + def show? + return true if @account_user&.administrator? + + # Check if user has access to the conversation through inbox membership + return true if user_assigned_inboxes.include?(record.inbox) + + # Check if user has access to the conversation through team membership + user_assigned_teams.include?(record.team) if record.team.present? + end + def destroy? @account_user&.administrator? end + + private + + def user_assigned_inboxes + @user.inboxes.where(account_id: @account.id) + end + + def user_assigned_teams + @user.teams.where(account_id: @account.id) + end end diff --git a/app/services/conversations/permission_filter_service.rb b/app/services/conversations/permission_filter_service.rb index 31dd011d7..82c702275 100644 --- a/app/services/conversations/permission_filter_service.rb +++ b/app/services/conversations/permission_filter_service.rb @@ -17,6 +17,7 @@ class Conversations::PermissionFilterService def accessible_conversations conversations.where(inbox: user.inboxes.where(account_id: account.id)) + .or(conversations.where(team: user.teams.where(account_id: account.id))) end def account_user diff --git a/spec/policies/conversation_policy_spec.rb b/spec/policies/conversation_policy_spec.rb index ecc3134fc..4f67ef2af 100644 --- a/spec/policies/conversation_policy_spec.rb +++ b/spec/policies/conversation_policy_spec.rb @@ -31,4 +31,77 @@ RSpec.describe ConversationPolicy, type: :policy do end end end + + permissions :show? do + let(:inbox) { create(:inbox, account: account) } + let(:team) { create(:team, account: account) } + let(:conversation_with_inbox) { create(:conversation, account: account, inbox: inbox) } + let(:conversation_with_team) { create(:conversation, account: account, inbox: other_inbox, team: team) } + let(:other_inbox) { create(:inbox, account: account) } + let(:inaccessible_conversation) { create(:conversation, account: account, inbox: other_inbox) } + + context 'when user is an administrator' do + it 'allows show for any conversation' do + expect(subject).to permit(administrator_context, conversation_with_inbox) + expect(subject).to permit(administrator_context, conversation_with_team) + expect(subject).to permit(administrator_context, inaccessible_conversation) + end + end + + context 'when user is an agent' do + context 'with inbox access' do + before do + create(:inbox_member, user: agent, inbox: inbox) + end + + it 'allows show for conversations in assigned inbox' do + expect(subject).to permit(agent_context, conversation_with_inbox) + end + + it 'denies show for conversations in non-assigned inbox' do + expect(subject).not_to permit(agent_context, conversation_with_team) + expect(subject).not_to permit(agent_context, inaccessible_conversation) + end + end + + context 'with team access' do + before do + create(:team_member, user: agent, team: team) + end + + it 'allows show for conversations assigned to user team' do + expect(subject).to permit(agent_context, conversation_with_team) + end + + it 'denies show for conversations not assigned to user team' do + expect(subject).not_to permit(agent_context, conversation_with_inbox) + expect(subject).not_to permit(agent_context, inaccessible_conversation) + end + end + + context 'with both inbox and team access' do + before do + create(:inbox_member, user: agent, inbox: inbox) + create(:team_member, user: agent, team: team) + end + + it 'allows show for conversations from both inbox and team' do + expect(subject).to permit(agent_context, conversation_with_inbox) + expect(subject).to permit(agent_context, conversation_with_team) + end + + it 'denies show for inaccessible conversations' do + expect(subject).not_to permit(agent_context, inaccessible_conversation) + end + end + + context 'with no access' do + it 'denies show for all conversations' do + expect(subject).not_to permit(agent_context, conversation_with_inbox) + expect(subject).not_to permit(agent_context, conversation_with_team) + expect(subject).not_to permit(agent_context, inaccessible_conversation) + end + end + end + end end diff --git a/spec/services/conversations/permission_filter_service_spec.rb b/spec/services/conversations/permission_filter_service_spec.rb index 194387afc..443895a4d 100644 --- a/spec/services/conversations/permission_filter_service_spec.rb +++ b/spec/services/conversations/permission_filter_service_spec.rb @@ -4,12 +4,20 @@ RSpec.describe Conversations::PermissionFilterService do let(:account) { create(:account) } let!(:conversation) { create(:conversation, account: account, inbox: inbox) } let!(:another_conversation) { create(:conversation, account: account, inbox: inbox) } + let!(:team_conversation) { create(:conversation, account: account, inbox: other_inbox, team: team) } + let!(:inaccessible_conversation) { create(:conversation, account: account, inbox: other_inbox) } let(:admin) { create(:user, account: account, role: :administrator) } let(:agent) { create(:user, account: account, role: :agent) } + let(:team_agent) { create(:user, account: account, role: :agent) } let!(:inbox) { create(:inbox, account: account) } + let!(:other_inbox) { create(:inbox, account: account) } + let!(:team) { create(:team, account: account) } # This inbox_member is used to establish the agent's access to the inbox - before { create(:inbox_member, user: agent, inbox: inbox) } + before do + create(:inbox_member, user: agent, inbox: inbox) + create(:team_member, user: team_agent, team: team) + end describe '#perform' do context 'when user is an administrator' do @@ -22,25 +30,81 @@ RSpec.describe Conversations::PermissionFilterService do expect(result).to include(conversation) expect(result).to include(another_conversation) - expect(result.count).to eq(2) + expect(result).to include(team_conversation) + expect(result).to include(inaccessible_conversation) + expect(result.count).to eq(4) end end context 'when user is an agent' do - it 'returns all conversations with no further filtering' do - inbox_ids = agent.inboxes.where(account_id: account.id).pluck(:id) + context 'with inbox access only' do + it 'returns conversations from assigned inboxes' do + result = described_class.new( + account.conversations, + agent, + account + ).perform - # The base implementation returns all conversations - # expecting the caller to filter by assigned inboxes - result = described_class.new( - account.conversations.where(inbox_id: inbox_ids), - agent, - account - ).perform + expect(result).to include(conversation) + expect(result).to include(another_conversation) + expect(result).not_to include(team_conversation) + expect(result).not_to include(inaccessible_conversation) + expect(result.count).to eq(2) + end + end - expect(result).to include(conversation) - expect(result).to include(another_conversation) - expect(result.count).to eq(2) + context 'with team access only' do + it 'returns conversations from assigned teams' do + result = described_class.new( + account.conversations, + team_agent, + account + ).perform + + expect(result).to include(team_conversation) + expect(result).not_to include(conversation) + expect(result).not_to include(another_conversation) + expect(result).not_to include(inaccessible_conversation) + expect(result.count).to eq(1) + end + end + + context 'with both inbox and team access' do + let(:agent_with_both) { create(:user, account: account, role: :agent) } + + before do + create(:inbox_member, user: agent_with_both, inbox: inbox) + create(:team_member, user: agent_with_both, team: team) + end + + it 'returns conversations from both assigned inboxes and teams' do + result = described_class.new( + account.conversations, + agent_with_both, + account + ).perform + + expect(result).to include(conversation) + expect(result).to include(another_conversation) + expect(result).to include(team_conversation) + expect(result).not_to include(inaccessible_conversation) + expect(result.count).to eq(3) + end + end + + context 'with no access' do + let(:agent_no_access) { create(:user, account: account, role: :agent) } + + it 'returns no conversations' do + result = described_class.new( + account.conversations, + agent_no_access, + account + ).perform + + expect(result).to be_empty + expect(result.count).to eq(0) + end end end end