chore(refactor): Improve conversation permission filtering (#11166)

1. Add permission filter service to separate permission filtering logic
from conversation queries
2. Implement hierarchical permissions with cleaner logic:
   - conversation_manage gives access to all conversations
- conversation_unassigned_manage gives access to unassigned and user's
conversations
- conversation_participating_manage gives access only to user's
conversations

---------

Co-authored-by: Pranav <pranav@chatwoot.com>
This commit is contained in:
Sojan Jose
2025-03-31 19:30:02 -07:00
committed by GitHub
parent f20a18b03f
commit ca83a27e95
12 changed files with 759 additions and 260 deletions

View File

@@ -0,0 +1,254 @@
## This spec is to ensure alignment between frontend and backend filters
# ref: https://github.com/chatwoot/chatwoot/pull/11111
require 'rails_helper'
describe Conversations::FilterService do
describe 'Frontend alignment tests' do
let!(:account) { create(:account) }
let!(:user_1) { create(:user, account: account, role: :administrator) }
let!(:inbox) { create(:inbox, account: account) }
let!(:params) { { payload: [], page: 1 } }
before do
account.conversations.destroy_all
# Create inbox membership
create(:inbox_member, user: user_1, inbox: inbox)
# Create custom attribute definition for conversation_type
create(:custom_attribute_definition,
attribute_key: 'conversation_type',
account: account,
attribute_model: 'conversation_attribute',
attribute_display_type: 'list',
attribute_values: %w[platinum silver gold regular])
end
context 'with A AND B OR C filter chain' do
let(:conversation) { create(:conversation, account: account, inbox: inbox, assignee: user_1) }
let(:filter_payload) do
[
{
attribute_key: 'status',
filter_operator: 'equal_to',
values: ['open'],
query_operator: 'AND'
}.with_indifferent_access,
{
attribute_key: 'priority',
filter_operator: 'equal_to',
values: ['urgent'],
query_operator: 'OR'
}.with_indifferent_access,
{
attribute_key: 'display_id',
filter_operator: 'equal_to',
values: ['12345'],
query_operator: nil
}.with_indifferent_access
]
end
before do
conversation.update!(
status: 'open',
priority: 'urgent',
display_id: '12345',
additional_attributes: { 'browser_language': 'en' }
)
end
it 'matches when all conditions are true' do
params[:payload] = filter_payload
result = described_class.new(params, user_1, account).perform
expect(result[:conversations].length).to be 1
end
it 'matches when first condition is false but third is true' do
conversation.update!(status: 'resolved', priority: 'urgent', display_id: '12345')
params[:payload] = filter_payload
result = described_class.new(params, user_1, account).perform
expect(result[:conversations].length).to be 1
end
it 'matches when first and second condition is false but third is true' do
conversation.update!(status: 'resolved', priority: 'low', display_id: '12345')
params[:payload] = filter_payload
result = described_class.new(params, user_1, account).perform
expect(result[:conversations].length).to be 1
end
it 'does not match when all conditions are false' do
conversation.update!(status: 'resolved', priority: 'low', display_id: '67890')
params[:payload] = filter_payload
result = described_class.new(params, user_1, account).perform
expect(result[:conversations].length).to be 0
end
end
context 'with A OR B AND C filter chain' do
let(:conversation) { create(:conversation, account: account, inbox: inbox, assignee: user_1) }
let(:filter_payload) do
[
{
attribute_key: 'status',
filter_operator: 'equal_to',
values: ['open'],
query_operator: 'OR'
}.with_indifferent_access,
{
attribute_key: 'priority',
filter_operator: 'equal_to',
values: ['low'],
query_operator: 'AND'
}.with_indifferent_access,
{
attribute_key: 'display_id',
filter_operator: 'equal_to',
values: ['67890'],
query_operator: nil
}.with_indifferent_access
]
end
before do
conversation.update!(
status: 'open',
priority: 'urgent',
display_id: '12345',
additional_attributes: { 'browser_language': 'en' }
)
end
it 'matches when first condition is true' do
params[:payload] = filter_payload
result = described_class.new(params, user_1, account).perform
expect(result[:conversations].length).to be 1
end
it 'matches when second and third conditions are true' do
conversation.update!(status: 'resolved', priority: 'low', display_id: '67890')
params[:payload] = filter_payload
result = described_class.new(params, user_1, account).perform
expect(result[:conversations].length).to be 1
end
end
context 'with complex filter chain A AND B OR C AND D' do
let(:conversation) { create(:conversation, account: account, inbox: inbox, assignee: user_1) }
let(:filter_payload) do
[
{
attribute_key: 'status',
filter_operator: 'equal_to',
values: ['open'],
query_operator: 'AND'
}.with_indifferent_access,
{
attribute_key: 'priority',
filter_operator: 'equal_to',
values: ['urgent'],
query_operator: 'OR'
}.with_indifferent_access,
{
attribute_key: 'display_id',
filter_operator: 'equal_to',
values: ['67890'],
query_operator: 'AND'
}.with_indifferent_access,
{
attribute_key: 'browser_language',
filter_operator: 'equal_to',
values: ['tr'],
query_operator: nil
}.with_indifferent_access
]
end
before do
conversation.update!(
status: 'open',
priority: 'urgent',
display_id: '12345',
additional_attributes: { 'browser_language': 'en' },
custom_attributes: { conversation_type: 'platinum' }
)
end
it 'matches when first two conditions are true' do
params[:payload] = filter_payload
result = described_class.new(params, user_1, account).perform
expect(result[:conversations].length).to be 1
end
it 'matches when last two conditions are true' do
conversation.update!(
status: 'resolved',
priority: 'low',
display_id: '67890',
additional_attributes: { 'browser_language': 'tr' }
)
params[:payload] = filter_payload
result = described_class.new(params, user_1, account).perform
expect(result[:conversations].length).to be 1
end
end
context 'with mixed operators filter chain' do
let(:conversation) { create(:conversation, account: account, inbox: inbox, assignee: user_1) }
let(:filter_payload) do
[
{
attribute_key: 'status',
filter_operator: 'equal_to',
values: ['open'],
query_operator: 'AND'
}.with_indifferent_access,
{
attribute_key: 'priority',
filter_operator: 'equal_to',
values: ['urgent'],
query_operator: 'OR'
}.with_indifferent_access,
{
attribute_key: 'display_id',
filter_operator: 'equal_to',
values: ['67890'],
query_operator: 'AND'
}.with_indifferent_access,
{
attribute_key: 'conversation_type',
filter_operator: 'equal_to',
values: ['platinum'],
custom_attribute_type: '',
query_operator: nil
}.with_indifferent_access
]
end
before do
conversation.update!(
status: 'open',
priority: 'urgent',
display_id: '12345',
additional_attributes: { 'browser_language': 'en' },
custom_attributes: { conversation_type: 'platinum' }
)
end
it 'matches when all conditions in the chain are true' do
params[:payload] = filter_payload
result = described_class.new(params, user_1, account).perform
expect(result[:conversations].length).to be 1
end
it 'does not match when the last condition is false' do
conversation.update!(custom_attributes: { conversation_type: 'silver' })
params[:payload] = filter_payload
result = described_class.new(params, user_1, account).perform
expect(result[:conversations].length).to be 1
end
end
end
end

View File

@@ -23,7 +23,6 @@ describe Conversations::FilterService do
before do
create(:inbox_member, user: user_1, inbox: inbox)
create(:inbox_member, user: user_2, inbox: inbox)
Current.account = account
en_conversation_1.update!(custom_attributes: { conversation_additional_information: 'test custom data' })
en_conversation_2.update!(custom_attributes: { conversation_additional_information: 'test custom data', conversation_type: 'platinum' })
@@ -72,7 +71,7 @@ describe Conversations::FilterService do
it 'filter conversations by additional_attributes and status' do
params[:payload] = payload
result = filter_service.new(params, user_1).perform
result = filter_service.new(params, user_1, account).perform
conversations = Conversation.where("additional_attributes ->> 'browser_language' IN (?) AND status IN (?)", ['en'], [1, 2])
expect(result[:count][:all_count]).to be conversations.count
end
@@ -88,7 +87,7 @@ describe Conversations::FilterService do
custom_attribute_type: ''
}.with_indifferent_access
]
result = filter_service.new(params, user_1).perform
result = filter_service.new(params, user_1, account).perform
expect(result[:conversations].length).to eq 1
expect(result[:conversations][0][:id]).to eq conversation.id
end
@@ -107,7 +106,7 @@ describe Conversations::FilterService do
custom_attribute_type: ''
}.with_indifferent_access
]
result = filter_service.new(params, user_1).perform
result = filter_service.new(params, user_1, account).perform
expect(result[:conversations].length).to eq 2
expect(result[:conversations].pluck(:id)).to include(high_priority.id, urgent_priority.id)
end
@@ -127,7 +126,7 @@ describe Conversations::FilterService do
custom_attribute_type: ''
}.with_indifferent_access
]
result = filter_service.new(params, user_1).perform
result = filter_service.new(params, user_1, account).perform
# Only include conversations with medium and low priority, excluding high and urgent
expect(result[:conversations].length).to eq 2
@@ -137,7 +136,7 @@ describe Conversations::FilterService do
it 'filter conversations by additional_attributes and status with pagination' do
params[:payload] = payload
params[:page] = 2
result = filter_service.new(params, user_1).perform
result = filter_service.new(params, user_1, account).perform
conversations = Conversation.where("additional_attributes ->> 'browser_language' IN (?) AND status IN (?)", ['en'], [1, 2])
expect(result[:count][:all_count]).to be conversations.count
end
@@ -156,7 +155,7 @@ describe Conversations::FilterService do
create(:conversation, account: account, inbox: inbox, assignee: user_1, campaign_id: campaign_1.id,
status: 'pending', additional_attributes: { 'browser_language': 'tr' })
result = filter_service.new(params, user_1).perform
result = filter_service.new(params, user_1, account).perform
expect(result[:count][:all_count]).to be 2
end
@@ -174,7 +173,7 @@ describe Conversations::FilterService do
create(:conversation, account: account, inbox: inbox, assignee: user_1, campaign_id: campaign_1.id,
status: 'pending', additional_attributes: { 'browser_language': 'tr' })
result = filter_service.new(params, user_1).perform
result = filter_service.new(params, user_1, account).perform
expect(result[:count][:all_count]).to be 1
expect(result[:conversations].first.additional_attributes['browser_language']).to eq 'fr'
@@ -184,7 +183,7 @@ describe Conversations::FilterService do
payload = [{ attribute_key: 'conversation_type', filter_operator: 'not_equal_to', values: 'platinum', query_operator: nil,
custom_attribute_type: 'conversation_attribute' }.with_indifferent_access]
params[:payload] = payload
result = filter_service.new(params, user_1).perform
result = filter_service.new(params, user_1, account).perform
conversations = Conversation.where(
"custom_attributes ->> 'conversation_type' NOT IN (?) OR custom_attributes ->> 'conversation_type' IS NULL", ['platinum']
)
@@ -213,7 +212,7 @@ describe Conversations::FilterService do
query_operator: nil
}.with_indifferent_access
]
result = filter_service.new(params, user_1).perform
result = filter_service.new(params, user_1, account).perform
expect(result[:count][:all_count]).to be 1
end
@@ -237,7 +236,7 @@ describe Conversations::FilterService do
custom_attribute_type: ''
}.with_indifferent_access
]
result = filter_service.new(params, user_1).perform
result = filter_service.new(params, user_1, account).perform
expect(result[:count][:all_count]).to be 2
expect(result[:conversations].pluck(:campaign_id).sort).to eq [campaign_2.id, campaign_1.id].sort
@@ -264,7 +263,7 @@ describe Conversations::FilterService do
}.with_indifferent_access
]
expect { filter_service.new(params, user_1).perform }.to raise_error(CustomExceptions::CustomFilter::InvalidQueryOperator)
expect { filter_service.new(params, user_1, account).perform }.to raise_error(CustomExceptions::CustomFilter::InvalidQueryOperator)
end
end
end
@@ -296,7 +295,7 @@ describe Conversations::FilterService do
query_operator: nil
}.with_indifferent_access
]
result = filter_service.new(params, user_1).perform
result = filter_service.new(params, user_1, account).perform
expect(result[:conversations].length).to be 1
expect(result[:conversations][0][:id]).to be user_2_assigned_conversation.id
end
@@ -324,7 +323,7 @@ describe Conversations::FilterService do
query_operator: nil
}.with_indifferent_access
]
result = filter_service.new(params, user_1).perform
result = filter_service.new(params, user_1, account).perform
expect(result[:conversations].length).to be 1
expect(result[:conversations][0][:id]).to be user_2_assigned_conversation.id
end
@@ -346,7 +345,7 @@ describe Conversations::FilterService do
custom_attribute_type: ''
}.with_indifferent_access
]
result = filter_service.new(params, user_1).perform
result = filter_service.new(params, user_1, account).perform
expect(result[:conversations].length).to be 1
end
@@ -367,7 +366,7 @@ describe Conversations::FilterService do
custom_attribute_type: nil
}.with_indifferent_access
]
result = filter_service.new(params, user_1).perform
result = filter_service.new(params, user_1, account).perform
expect(result[:conversations].length).to be 1
end
@@ -393,7 +392,7 @@ describe Conversations::FilterService do
custom_attribute_type: ''
}.with_indifferent_access
]
result = filter_service.new(params, user_1).perform
result = filter_service.new(params, user_1, account).perform
expect(result[:conversations].length).to be 1
end
end
@@ -413,7 +412,7 @@ describe Conversations::FilterService do
custom_attribute_type: ''
}.with_indifferent_access
]
result = filter_service.new(params, user_1).perform
result = filter_service.new(params, user_1, account).perform
expected_count = Conversation.where('created_at > ?', DateTime.parse('2022-01-20')).count
expect(result[:conversations].length).to be expected_count
end
@@ -435,7 +434,7 @@ describe Conversations::FilterService do
custom_attribute_type: ''
}.with_indifferent_access
]
result = filter_service.new(params, user_1).perform
result = filter_service.new(params, user_1, account).perform
expected_count = Conversation.where("created_at > ? AND custom_attributes->>'conversation_type' = ?", DateTime.parse('2022-01-20'),
'platinum').count
@@ -471,7 +470,7 @@ describe Conversations::FilterService do
expected_count = Conversation.where("last_activity_at < ? AND custom_attributes->>'conversation_type' = ?", (Time.zone.today - 3.days),
'platinum').count
result = filter_service.new(params, user_1).perform
result = filter_service.new(params, user_1, account).perform
expect(result[:conversations].length).to be expected_count
end
@@ -488,7 +487,7 @@ describe Conversations::FilterService do
expected_count = Conversation.where('last_activity_at < ?', (Time.zone.today - 2.days)).count
result = filter_service.new(params, user_1).perform
result = filter_service.new(params, user_1, account).perform
expect(result[:conversations].length).to be expected_count
end
end
@@ -522,240 +521,35 @@ describe Conversations::FilterService do
end
end
describe 'Frontend alignment tests' do
describe '#base_relation' do
let!(:account) { create(:account) }
let!(:user_1) { create(:user, account: account) }
let!(:inbox) { create(:inbox, account: account) }
let!(:user_1) { create(:user, account: account, role: :agent) }
let!(:admin) { create(:user, account: account, role: :administrator) }
let!(:inbox_1) { create(:inbox, account: account) }
let!(:inbox_2) { create(:inbox, account: account) }
let!(:params) { { payload: [], page: 1 } }
before do
account.conversations.destroy_all
# Make user_1 a regular agent with access to inbox_1 only
create(:inbox_member, user: user_1, inbox: inbox_1)
# Create conversations in both inboxes
create(:conversation, account: account, inbox: inbox_1)
create(:conversation, account: account, inbox: inbox_2)
end
context 'with A AND B OR C filter chain' do
let(:conversation) { create(:conversation, account: account, inbox: inbox, assignee: user_1) }
let(:filter_payload) do
[
{
attribute_key: 'status',
filter_operator: 'equal_to',
values: ['open'],
query_operator: 'AND'
}.with_indifferent_access,
{
attribute_key: 'priority',
filter_operator: 'equal_to',
values: ['urgent'],
query_operator: 'OR'
}.with_indifferent_access,
{
attribute_key: 'display_id',
filter_operator: 'equal_to',
values: ['12345'],
query_operator: nil
}.with_indifferent_access
]
end
before do
conversation.update!(
status: 'open',
priority: 'urgent',
display_id: '12345',
additional_attributes: { 'browser_language': 'en' }
)
end
it 'matches when all conditions are true' do
params[:payload] = filter_payload
result = described_class.new(params, user_1).perform
expect(result[:conversations].length).to be 1
end
it 'matches when first condition is false but third is true' do
conversation.update!(status: 'resolved', priority: 'urgent', display_id: '12345')
params[:payload] = filter_payload
result = described_class.new(params, user_1).perform
expect(result[:conversations].length).to be 1
end
it 'matches when first and second condition is false but third is true' do
conversation.update!(status: 'resolved', priority: 'low', display_id: '12345')
params[:payload] = filter_payload
result = described_class.new(params, user_1).perform
expect(result[:conversations].length).to be 1
end
it 'does not match when all conditions are false' do
conversation.update!(status: 'resolved', priority: 'low', display_id: '67890')
params[:payload] = filter_payload
result = described_class.new(params, user_1).perform
expect(result[:conversations].length).to be 0
end
it 'returns all conversations for administrators, even for inboxes they are not members of' do
service = filter_service.new(params, admin, account)
result = service.perform
expect(result[:conversations].count).to eq 2
end
context 'with A OR B AND C filter chain' do
let(:conversation) { create(:conversation, account: account, inbox: inbox, assignee: user_1) }
let(:filter_payload) do
[
{
attribute_key: 'status',
filter_operator: 'equal_to',
values: ['open'],
query_operator: 'OR'
}.with_indifferent_access,
{
attribute_key: 'priority',
filter_operator: 'equal_to',
values: ['low'],
query_operator: 'AND'
}.with_indifferent_access,
{
attribute_key: 'display_id',
filter_operator: 'equal_to',
values: ['67890'],
query_operator: nil
}.with_indifferent_access
]
end
before do
conversation.update!(
status: 'open',
priority: 'urgent',
display_id: '12345',
additional_attributes: { 'browser_language': 'en' }
)
end
it 'matches when first condition is true' do
params[:payload] = filter_payload
result = described_class.new(params, user_1).perform
expect(result[:conversations].length).to be 1
end
it 'matches when second and third conditions are true' do
conversation.update!(status: 'resolved', priority: 'low', display_id: '67890')
params[:payload] = filter_payload
result = described_class.new(params, user_1).perform
expect(result[:conversations].length).to be 1
end
end
context 'with complex filter chain A AND B OR C AND D' do
let(:conversation) { create(:conversation, account: account, inbox: inbox, assignee: user_1) }
let(:filter_payload) do
[
{
attribute_key: 'status',
filter_operator: 'equal_to',
values: ['open'],
query_operator: 'AND'
}.with_indifferent_access,
{
attribute_key: 'priority',
filter_operator: 'equal_to',
values: ['urgent'],
query_operator: 'OR'
}.with_indifferent_access,
{
attribute_key: 'display_id',
filter_operator: 'equal_to',
values: ['67890'],
query_operator: 'AND'
}.with_indifferent_access,
{
attribute_key: 'browser_language',
filter_operator: 'equal_to',
values: ['tr'],
query_operator: nil
}.with_indifferent_access
]
end
before do
conversation.update!(
status: 'open',
priority: 'urgent',
display_id: '12345',
additional_attributes: { 'browser_language': 'en' },
custom_attributes: { conversation_type: 'platinum' }
)
end
it 'matches when first two conditions are true' do
params[:payload] = filter_payload
result = described_class.new(params, user_1).perform
expect(result[:conversations].length).to be 1
end
it 'matches when last two conditions are true' do
conversation.update!(
status: 'resolved',
priority: 'low',
display_id: '67890',
additional_attributes: { 'browser_language': 'tr' }
)
params[:payload] = filter_payload
result = described_class.new(params, user_1).perform
expect(result[:conversations].length).to be 1
end
end
context 'with mixed operators filter chain' do
let(:conversation) { create(:conversation, account: account, inbox: inbox, assignee: user_1) }
let(:filter_payload) do
[
{
attribute_key: 'status',
filter_operator: 'equal_to',
values: ['open'],
query_operator: 'AND'
}.with_indifferent_access,
{
attribute_key: 'priority',
filter_operator: 'equal_to',
values: ['urgent'],
query_operator: 'OR'
}.with_indifferent_access,
{
attribute_key: 'display_id',
filter_operator: 'equal_to',
values: ['67890'],
query_operator: 'AND'
}.with_indifferent_access,
{
attribute_key: 'conversation_type',
filter_operator: 'equal_to',
values: ['platinum'],
custom_attribute_type: '',
query_operator: nil
}.with_indifferent_access
]
end
before do
conversation.update!(
status: 'open',
priority: 'urgent',
display_id: '12345',
additional_attributes: { 'browser_language': 'en' },
custom_attributes: { conversation_type: 'platinum' }
)
end
it 'matches when all conditions in the chain are true' do
params[:payload] = filter_payload
result = described_class.new(params, user_1).perform
expect(result[:conversations].length).to be 1
end
it 'does not match when the last condition is false' do
conversation.update!(custom_attributes: { conversation_type: 'silver' })
params[:payload] = filter_payload
result = described_class.new(params, user_1).perform
expect(result[:conversations].length).to be 1
end
it 'filters conversations by inbox membership for non-administrators' do
service = filter_service.new(params, user_1, account)
result = service.perform
expect(result[:conversations].count).to eq 1
end
end
end

View File

@@ -0,0 +1,47 @@
require 'rails_helper'
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(:admin) { create(:user, account: account, role: :administrator) }
let(:agent) { create(:user, account: account, role: :agent) }
let!(:inbox) { create(:inbox, account: account) }
# This inbox_member is used to establish the agent's access to the inbox
before { create(:inbox_member, user: agent, inbox: inbox) }
describe '#perform' do
context 'when user is an administrator' do
it 'returns all conversations' do
result = described_class.new(
account.conversations,
admin,
account
).perform
expect(result).to include(conversation)
expect(result).to include(another_conversation)
expect(result.count).to eq(2)
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)
# 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.count).to eq(2)
end
end
end
end