From 9898ccee9eb5fb887b88d6a76063fb687f28940a Mon Sep 17 00:00:00 2001 From: Sojan Jose Date: Wed, 22 Oct 2025 20:23:37 -0700 Subject: [PATCH] chore: Enforce custom role permissions on conversation access (#12583) ## Summary - ensure conversation lookup uses the permission filter before fetching records - add request specs covering custom role access to unassigned conversations ## Testing - bundle exec rspec spec/enterprise/controllers/api/v1/accounts/conversations_controller_spec.rb ------ https://chatgpt.com/codex/tasks/task_e_68de1f62b9b883268a54882e608a8bb8 --- .../accounts/conversations/base_controller.rb | 2 +- .../v1/accounts/conversations_controller.rb | 2 +- .../accounts/integrations/dyte_controller.rb | 2 +- .../concerns/access_token_auth_helper.rb | 1 + app/policies/conversation_policy.rb | 40 ++++++++++- .../enterprise/conversation_policy.rb | 42 +++++++++++ spec/controllers/api/base_controller_spec.rb | 23 ++++++ .../accounts/conversations_controller_spec.rb | 71 +++++++++++++++++++ .../policies/conversation_policy_spec.rb | 65 +++++++++++++++++ spec/policies/conversation_policy_spec.rb | 45 +++++++++++- 10 files changed, 286 insertions(+), 7 deletions(-) create mode 100644 enterprise/app/policies/enterprise/conversation_policy.rb create mode 100644 spec/enterprise/policies/conversation_policy_spec.rb 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/controllers/api/v1/accounts/integrations/dyte_controller.rb b/app/controllers/api/v1/accounts/integrations/dyte_controller.rb index c5f795d34..845caab5e 100644 --- a/app/controllers/api/v1/accounts/integrations/dyte_controller.rb +++ b/app/controllers/api/v1/accounts/integrations/dyte_controller.rb @@ -22,7 +22,7 @@ class Api::V1::Accounts::Integrations::DyteController < Api::V1::Accounts::BaseC private def authorize_request - authorize @conversation.inbox, :show? + authorize @conversation, :show? end def render_response(response) diff --git a/app/controllers/concerns/access_token_auth_helper.rb b/app/controllers/concerns/access_token_auth_helper.rb index 9b0f9021f..338b290da 100644 --- a/app/controllers/concerns/access_token_auth_helper.rb +++ b/app/controllers/concerns/access_token_auth_helper.rb @@ -14,6 +14,7 @@ module AccessTokenAuthHelper ensure_access_token render_unauthorized('Invalid Access Token') && return if @access_token.blank? + # NOTE: This ensures that current_user is set and available for the rest of the controller actions @resource = @access_token.owner Current.user = @resource if allowed_current_user_type?(@resource) end diff --git a/app/policies/conversation_policy.rb b/app/policies/conversation_policy.rb index 931e17435..d23f29e54 100644 --- a/app/policies/conversation_policy.rb +++ b/app/policies/conversation_policy.rb @@ -4,6 +4,44 @@ class ConversationPolicy < ApplicationPolicy end def destroy? - @account_user&.administrator? + administrator? + end + + def show? + administrator? || agent_bot? || agent_can_view_conversation? + end + + private + + def agent_can_view_conversation? + inbox_access? || team_access? + end + + def administrator? + account_user&.administrator? + end + + def agent_bot? + user.is_a?(AgentBot) + end + + def inbox_access? + user.inboxes.where(account_id: account&.id).exists?(id: record.inbox_id) + end + + def team_access? + return false if record.team_id.blank? + + user.teams.where(account_id: account&.id).exists?(id: record.team_id) + end + + def assigned_to_user? + record.assignee_id == user.id + end + + def participant? + record.conversation_participants.exists?(user_id: user.id) end end + +ConversationPolicy.prepend_mod_with('ConversationPolicy') diff --git a/enterprise/app/policies/enterprise/conversation_policy.rb b/enterprise/app/policies/enterprise/conversation_policy.rb new file mode 100644 index 000000000..d956db2d7 --- /dev/null +++ b/enterprise/app/policies/enterprise/conversation_policy.rb @@ -0,0 +1,42 @@ +module Enterprise::ConversationPolicy + def show? + return false unless super + return true unless custom_role_permissions? + + permissions = custom_role_permissions + return true if manage_all_conversations?(permissions) + return true if permits_unassigned_manage?(permissions) + + permits_participating?(permissions) + end + + private + + def manage_all_conversations?(permissions) + permissions.include?('conversation_manage') + end + + def permits_unassigned_manage?(permissions) + return false unless permissions.include?('conversation_unassigned_manage') + + unassigned_conversation? || assigned_to_user? + end + + def permits_participating?(permissions) + return false unless permissions.include?('conversation_participating_manage') + + assigned_to_user? || participant? + end + + def unassigned_conversation? + record.assignee_id.nil? + end + + def custom_role_permissions? + account_user&.custom_role_id.present? + end + + def custom_role_permissions + account_user&.custom_role&.permissions || [] + end +end diff --git a/spec/controllers/api/base_controller_spec.rb b/spec/controllers/api/base_controller_spec.rb index ac2306d14..69e4f5cae 100644 --- a/spec/controllers/api/base_controller_spec.rb +++ b/spec/controllers/api/base_controller_spec.rb @@ -5,6 +5,29 @@ RSpec.describe 'API Base', type: :request do let!(:user) { create(:user, account: account) } describe 'request with api_access_token for user' do + context 'when accessing an account scoped resource' do + let!(:admin) { create(:user, :administrator, account: account) } + let!(:conversation) { create(:conversation, account: account) } + + it 'sets Current attributes for the request and then returns the response' do + # expect Current.account_user is set to the admin's account_user + allow(Current).to receive(:user=).and_call_original + allow(Current).to receive(:account=).and_call_original + allow(Current).to receive(:account_user=).and_call_original + + get "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}", + headers: { api_access_token: admin.access_token.token }, + as: :json + + expect(Current).to have_received(:user=).with(admin).at_least(:once) + expect(Current).to have_received(:account=).with(account).at_least(:once) + expect(Current).to have_received(:account_user=).with(admin.account_users.first).at_least(:once) + + expect(response).to have_http_status(:success) + expect(response.parsed_body['id']).to eq(conversation.display_id) + end + end + context 'when it is an invalid api_access_token' do it 'returns unauthorized' do get '/api/v1/profile', diff --git a/spec/enterprise/controllers/api/v1/accounts/conversations_controller_spec.rb b/spec/enterprise/controllers/api/v1/accounts/conversations_controller_spec.rb index 4d5269cde..bbb3f1eb3 100644 --- a/spec/enterprise/controllers/api/v1/accounts/conversations_controller_spec.rb +++ b/spec/enterprise/controllers/api/v1/accounts/conversations_controller_spec.rb @@ -30,5 +30,76 @@ RSpec.describe 'Conversations API', type: :request do expect(response.parsed_body.keys).not_to include('applied_sla') expect(response.parsed_body.keys).not_to include('sla_events') end + + context 'when agent has team access' do + let(:agent) { create(:user, account: account, role: :agent) } + let(:team) { create(:team, account: account) } + let(:conversation) { create(:conversation, account: account, team: team) } + + before do + create(:team_member, team: team, user: agent) + end + + it 'allows accessing the conversation via team membership' do + get "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}", headers: agent.create_new_auth_token + + expect(response).to have_http_status(:ok) + expect(response.parsed_body['id']).to eq(conversation.display_id) + end + end + + context 'when agent has a custom role' do + let(:agent) { create(:user, account: account, role: :agent) } + let(:conversation) { create(:conversation, account: account) } + + before do + create(:inbox_member, user: agent, inbox: conversation.inbox) + end + + it 'returns unauthorized for unassigned conversation without permission' do + custom_role = create(:custom_role, account: account, permissions: ['conversation_participating_manage']) + account.account_users.find_by(user_id: agent.id).update!(custom_role: custom_role) + + get "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}", headers: agent.create_new_auth_token + + expect(response).to have_http_status(:unauthorized) + end + + it 'returns the conversation when permission allows managing unassigned conversations, including when assigned to agent' do + custom_role = create(:custom_role, account: account, permissions: ['conversation_unassigned_manage']) + account_user = account.account_users.find_by(user_id: agent.id) + account_user.update!(custom_role: custom_role) + conversation.update!(assignee: agent) + + get "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}", headers: agent.create_new_auth_token + + expect(response).to have_http_status(:ok) + expect(response.parsed_body['id']).to eq(conversation.display_id) + end + + it 'returns the conversation when permission allows managing assigned conversations' do + custom_role = create(:custom_role, account: account, permissions: ['conversation_participating_manage']) + account_user = account.account_users.find_by(user_id: agent.id) + account_user.update!(custom_role: custom_role) + conversation.update!(assignee: agent) + + get "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}", headers: agent.create_new_auth_token + + expect(response).to have_http_status(:ok) + expect(response.parsed_body['id']).to eq(conversation.display_id) + end + + it 'returns the conversation when permission allows managing participating conversations' do + custom_role = create(:custom_role, account: account, permissions: ['conversation_participating_manage']) + account_user = account.account_users.find_by(user_id: agent.id) + account_user.update!(custom_role: custom_role) + create(:conversation_participant, conversation: conversation, account: account, user: agent) + + get "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}", headers: agent.create_new_auth_token + + expect(response).to have_http_status(:ok) + expect(response.parsed_body['id']).to eq(conversation.display_id) + end + end end end diff --git a/spec/enterprise/policies/conversation_policy_spec.rb b/spec/enterprise/policies/conversation_policy_spec.rb new file mode 100644 index 000000000..e48a84852 --- /dev/null +++ b/spec/enterprise/policies/conversation_policy_spec.rb @@ -0,0 +1,65 @@ +require 'rails_helper' + +RSpec.describe ConversationPolicy, type: :policy do + subject { described_class } + + let(:account) { create(:account) } + let(:agent) { create(:user, account: account, role: :agent) } + let(:inbox) { create(:inbox, account: account) } + let(:agent_account_user) { agent.account_users.find_by(account: account) } + let(:context) { { user: agent, account: account, account_user: agent_account_user } } + + before do + create(:inbox_member, user: agent, inbox: inbox) + end + + permissions :show? do + context 'when role grants conversation_unassigned_manage' do + let(:custom_role) { create(:custom_role, account: account, permissions: ['conversation_unassigned_manage']) } + + before do + agent_account_user.update!(role: :agent, custom_role: custom_role) + end + + it 'allows access to conversations assigned to the agent' do + conversation = create(:conversation, account: account, inbox: inbox, assignee: agent) + + expect(subject).to permit(context, conversation) + end + + it 'denies access to conversations assigned to someone else' do + other_agent = create(:user, account: account, role: :agent) + conversation = create(:conversation, account: account, inbox: inbox, assignee: other_agent) + + expect(subject).not_to permit(context, conversation) + end + end + + context 'when role grants conversation_participating_manage' do + let(:custom_role) { create(:custom_role, account: account, permissions: ['conversation_participating_manage']) } + + before do + agent_account_user.update!(role: :agent, custom_role: custom_role) + end + + it 'allows access to conversations assigned to the agent' do + conversation = create(:conversation, account: account, inbox: inbox, assignee: agent) + + expect(subject).to permit(context, conversation) + end + + it 'allows access to conversations where the agent is a participant' do + conversation = create(:conversation, account: account, inbox: inbox, assignee: nil) + create(:conversation_participant, conversation: conversation, account: account, user: agent) + + expect(subject).to permit(context, conversation) + end + + it 'denies access to unrelated conversations' do + conversation = create(:conversation, account: account, inbox: inbox, assignee: nil) + + expect(subject).not_to permit(context, conversation) + end + end + end +end diff --git a/spec/policies/conversation_policy_spec.rb b/spec/policies/conversation_policy_spec.rb index ecc3134fc..d75a6bc3a 100644 --- a/spec/policies/conversation_policy_spec.rb +++ b/spec/policies/conversation_policy_spec.rb @@ -4,11 +4,12 @@ RSpec.describe ConversationPolicy, type: :policy do subject { described_class } let(:account) { create(:account) } - let(:conversation) { create(:conversation, account: account) } let(:administrator) { create(:user, account: account, role: :administrator) } let(:agent) { create(:user, account: account, role: :agent) } - let(:administrator_context) { { user: administrator, account: account, account_user: administrator.account_users.first } } - let(:agent_context) { { user: agent, account: account, account_user: agent.account_users.first } } + let(:administrator_context) { { user: administrator, account: account, account_user: administrator.account_users.find_by(account: account) } } + let(:agent_context) { { user: agent, account: account, account_user: agent.account_users.find_by(account: account) } } + + let(:conversation) { create(:conversation, account: account) } permissions :destroy? do context 'when user is an administrator' do @@ -31,4 +32,42 @@ RSpec.describe ConversationPolicy, type: :policy do end end end + + permissions :show? do + context 'when user is an administrator' do + it 'allows access' do + expect(subject).to permit(administrator_context, conversation) + end + end + + context 'when agent has inbox access' do + let(:inbox) { create(:inbox, account: account) } + let(:conversation) { create(:conversation, account: account, inbox: inbox) } + + before { create(:inbox_member, user: agent, inbox: inbox) } + + it 'allows access' do + expect(subject).to permit(agent_context, conversation) + end + end + + context 'when agent has team access' do + let(:team) { create(:team, account: account) } + let(:conversation) { create(:conversation, :with_team, account: account, team: team) } + + before { create(:team_member, team: team, user: agent) } + + it 'allows access' do + expect(subject).to permit(agent_context, conversation) + end + end + + context 'when agent lacks inbox and team access' do + let(:conversation) { create(:conversation, account: account) } + + it 'denies access' do + expect(subject).not_to permit(agent_context, conversation) + end + end + end end