Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
chore: Ensure privilege validations for API endpoints (#2224)
Co-authored-by: Pranav Raj S <pranav@chatwoot.com>
  • Loading branch information
sojan-official and pranavrajs committed Jun 11, 2021
1 parent 5a95c74 commit 534acfb
Show file tree
Hide file tree
Showing 27 changed files with 340 additions and 124 deletions.
4 changes: 4 additions & 0 deletions app/controllers/api/base_controller.rb
Expand Up @@ -16,4 +16,8 @@ def check_authorization(model = nil)

authorize(model)
end

def check_admin_authorization?
raise Pundit::NotAuthorizedError unless Current.account_user.administrator?
end
end
Expand Up @@ -11,6 +11,7 @@ def create

def ensure_inbox
@inbox = Current.account.inboxes.find(params[:inbox_id])
authorize @inbox, :show?
end

def ensure_contact
Expand Down
Expand Up @@ -8,9 +8,7 @@ def index
private

def inbox_ids
if Current.user.administrator?
Current.account.inboxes.pluck(:id)
elsif Current.user.agent?
if Current.user.administrator? || Current.user.agent?
Current.user.assigned_inboxes.pluck(:id)
else
[]
Expand Down
3 changes: 2 additions & 1 deletion app/controllers/api/v1/accounts/contacts_controller.rb
Expand Up @@ -48,7 +48,8 @@ def active
def show; end

def contactable_inboxes
@contactable_inboxes = Contacts::ContactableInboxesService.new(contact: @contact).get
@all_contactable_inboxes = Contacts::ContactableInboxesService.new(contact: @contact).get
@contactable_inboxes = @all_contactable_inboxes.select { |contactable_inbox| policy(contactable_inbox[:inbox]).show? }
end

def create
Expand Down
Expand Up @@ -5,5 +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?
end
end
11 changes: 8 additions & 3 deletions app/controllers/api/v1/accounts/conversations_controller.rb
@@ -1,7 +1,7 @@
class Api::V1::Accounts::ConversationsController < Api::V1::Accounts::BaseController
include Events::Types

before_action :conversation, except: [:index]
before_action :conversation, except: [:index, :meta, :search, :create]
before_action :contact_inbox, only: [:create]

def index
Expand Down Expand Up @@ -79,21 +79,26 @@ def trigger_typing_event(event)
end

def conversation
@conversation ||= Current.account.conversations.find_by(display_id: params[:id])
@conversation ||= Current.account.conversations.find_by!(display_id: params[:id])
authorize @conversation.inbox, :show?
end

def contact_inbox
@contact_inbox = build_contact_inbox

@contact_inbox ||= ::ContactInbox.find_by!(source_id: params[:source_id])
authorize @contact_inbox.inbox, :show?
end

def build_contact_inbox
return if params[:contact_id].blank? || params[:inbox_id].blank?

inbox = Current.account.inboxes.find(params[:inbox_id])
authorize inbox, :show?

ContactInboxBuilder.new(
contact_id: params[:contact_id],
inbox_id: params[:inbox_id],
inbox_id: inbox.id,
source_id: params[:source_id]
).perform
end
Expand Down
16 changes: 10 additions & 6 deletions app/controllers/api/v1/accounts/inbox_members_controller.rb
Expand Up @@ -3,15 +3,19 @@ class Api::V1::Accounts::InboxMembersController < Api::V1::Accounts::BaseControl
before_action :current_agents_ids, only: [:create]

def create
# update also done via same action
update_agents_list
head :ok
rescue StandardError => e
Rails.logger.debug "Rescued: #{e.inspect}"
render_could_not_create_error('Could not add agents to inbox')
authorize @inbox, :create?
begin
# update also done via same action
update_agents_list
head :ok
rescue StandardError => e
Rails.logger.debug "Rescued: #{e.inspect}"
render_could_not_create_error('Could not add agents to inbox')
end
end

def show
authorize @inbox, :show?
@agents = Current.account.users.where(id: @inbox.members.select(:user_id))
end

Expand Down
1 change: 1 addition & 0 deletions app/controllers/api/v1/accounts/inboxes_controller.rb
Expand Up @@ -62,6 +62,7 @@ def destroy

def fetch_inbox
@inbox = Current.account.inboxes.find(params[:id])
authorize @inbox, :show?
end

def fetch_agent_bot
Expand Down
@@ -1,4 +1,5 @@
class Api::V1::Accounts::Integrations::AppsController < Api::V1::Accounts::BaseController
before_action :check_admin_authorization?
before_action :fetch_apps, only: [:index]
before_action :fetch_app, only: [:show]

Expand Down
@@ -1,4 +1,5 @@
class Api::V1::Accounts::Integrations::SlackController < Api::V1::Accounts::BaseController
before_action :check_admin_authorization?
before_action :fetch_hook, only: [:update, :destroy]

def create
Expand Down
6 changes: 6 additions & 0 deletions app/controllers/api/v2/accounts/reports_controller.rb
@@ -1,4 +1,6 @@
class Api::V2::Accounts::ReportsController < Api::V1::Accounts::BaseController
before_action :check_authorization

def account
builder = V2::ReportBuilder.new(Current.account, account_report_params)
data = builder.build
Expand All @@ -23,6 +25,10 @@ def inboxes

private

def check_authorization
raise Pundit::NotAuthorizedError unless Current.account_user.administrator?
end

def account_summary_params
{
type: :account,
Expand Down
14 changes: 5 additions & 9 deletions app/finders/conversation_finder.rb
Expand Up @@ -48,15 +48,11 @@ def perform
private

def set_inboxes
if params[:inbox_id]
@inbox_ids = current_account.inboxes.where(id: params[:inbox_id])
else
if @current_user.administrator?
@inbox_ids = current_account.inboxes.pluck(:id)
elsif @current_user.agent?
@inbox_ids = @current_user.assigned_inboxes.pluck(:id)
end
end
@inbox_ids = if params[:inbox_id]
current_account.inboxes.where(id: params[:inbox_id])
else
@current_user.assigned_inboxes.pluck(:id)
end
end

def set_assignee_type
Expand Down
2 changes: 1 addition & 1 deletion app/models/user.rb
Expand Up @@ -122,7 +122,7 @@ def account
end

def assigned_inboxes
inboxes.where(account_id: Current.account.id)
administrator? ? Current.account.inboxes : inboxes.where(account_id: Current.account.id)
end

def administrator?
Expand Down
5 changes: 5 additions & 0 deletions app/policies/conversation_policy.rb
@@ -0,0 +1,5 @@
class ConversationPolicy < ApplicationPolicy
def index?
true
end
end
13 changes: 8 additions & 5 deletions app/policies/inbox_policy.rb
Expand Up @@ -11,18 +11,21 @@ def initialize(user_context, scope)
end

def resolve
if @account_user.administrator?
scope.all
elsif @account_user.agent?
user.assigned_inboxes
end
user.assigned_inboxes
end
end

def index?
true
end

def show?
# FIXME: for agent bots, lets bring this validation to policies as well in future
return true if @user.blank?

Current.user.assigned_inboxes.include? record
end

def assignable_agents?
true
end
Expand Down
32 changes: 27 additions & 5 deletions spec/controllers/api/v1/accounts/agents_controller_spec.rb
Expand Up @@ -2,6 +2,8 @@

RSpec.describe 'Agents API', type: :request do
let(:account) { create(:account) }
let(:admin) { create(:user, account: account, role: :administrator) }
let(:agent) { create(:user, account: account, role: :agent) }

describe 'GET /api/v1/accounts/{account.id}/agents' do
context 'when it is an unauthenticated user' do
Expand Down Expand Up @@ -38,7 +40,13 @@
end

context 'when it is an authenticated user' do
let(:admin) { create(:user, account: account, role: :administrator) }
it 'returns unauthorized for agents' do
delete "/api/v1/accounts/#{account.id}/agents/#{other_agent.id}",
headers: agent.create_new_auth_token,
as: :json

expect(response).to have_http_status(:unauthorized)
end

it 'deletes an agent' do
delete "/api/v1/accounts/#{account.id}/agents/#{other_agent.id}",
Expand All @@ -63,10 +71,17 @@
end

context 'when it is an authenticated user' do
let(:admin) { create(:user, account: account, role: :administrator) }

params = { name: 'TestUser' }

it 'returns unauthorized for agents' do
put "/api/v1/accounts/#{account.id}/agents/#{other_agent.id}",
params: params,
headers: agent.create_new_auth_token,
as: :json

expect(response).to have_http_status(:unauthorized)
end

it 'modifies an agent' do
put "/api/v1/accounts/#{account.id}/agents/#{other_agent.id}",
params: params,
Expand All @@ -91,10 +106,17 @@
end

context 'when it is an authenticated user' do
let(:admin) { create(:user, account: account, role: :administrator) }

params = { name: 'NewUser', email: Faker::Internet.email, role: :agent }

it 'returns unauthorized for agents' do
post "/api/v1/accounts/#{account.id}/agents",
params: params,
headers: agent.create_new_auth_token,
as: :json

expect(response).to have_http_status(:unauthorized)
end

it 'creates a new agent' do
post "/api/v1/accounts/#{account.id}/agents",
params: params,
Expand Down
Expand Up @@ -5,7 +5,7 @@
let(:contact) { create(:contact, account: account) }
let(:channel_twilio_sms) { create(:channel_twilio_sms, account: account) }
let(:channel_api) { create(:channel_api, account: account) }
let(:user) { create(:user, account: account) }
let(:agent) { create(:user, account: account) }

describe 'GET /api/v1/accounts/{account.id}/contacts/:id/contact_inboxes' do
context 'when unauthenticated user' do
Expand All @@ -15,12 +15,13 @@
end
end

context 'when user is logged in' do
context 'when authenticated user with access to inbox' do
it 'creates a contact inbox' do
create(:inbox_member, inbox: channel_api.inbox, user: agent)
expect do
post "/api/v1/accounts/#{account.id}/contacts/#{contact.id}/contact_inboxes",
params: { inbox_id: channel_api.inbox.id },
headers: user.create_new_auth_token,
headers: agent.create_new_auth_token,
as: :json
end.to change(ContactInbox, :count).by(1)

Expand All @@ -29,10 +30,11 @@
end

it 'throws error for invalid source id' do
create(:inbox_member, inbox: channel_twilio_sms.inbox, user: agent)
expect do
post "/api/v1/accounts/#{account.id}/contacts/#{contact.id}/contact_inboxes",
params: { inbox_id: channel_twilio_sms.inbox.id },
headers: user.create_new_auth_token,
headers: agent.create_new_auth_token,
as: :json
end.to change(ContactInbox, :count).by(0)

Expand Down
21 changes: 16 additions & 5 deletions spec/controllers/api/v1/accounts/contacts_controller_spec.rb
Expand Up @@ -201,18 +201,29 @@
end

context 'when it is an authenticated user' do
let(:admin) { create(:user, account: account, role: :administrator) }
let(:agent) { create(:user, account: account, role: :agent) }
let!(:twilio_sms) { create(:channel_twilio_sms, account: account) }
let!(:twilio_sms_inbox) { create(:inbox, channel: twilio_sms, account: account) }
let!(:twilio_whatsapp) { create(:channel_twilio_sms, medium: :whatsapp, account: account) }
let!(:twilio_whatsapp_inbox) { create(:inbox, channel: twilio_whatsapp, account: account) }

it 'shows the contactable inboxes which the user has access to' do
create(:inbox_member, user: agent, inbox: twilio_whatsapp_inbox)

it 'shows the contact' do
inbox_service = double
allow(Contacts::ContactableInboxesService).to receive(:new).and_return(inbox_service)
allow(inbox_service).to receive(:get).and_return({})
expect(inbox_service).to receive(:get).and_return({})
allow(inbox_service).to receive(:get).and_return([
{ source_id: '1123', inbox: twilio_sms_inbox },
{ source_id: '1123', inbox: twilio_whatsapp_inbox }
])
expect(inbox_service).to receive(:get)
get "/api/v1/accounts/#{account.id}/contacts/#{contact.id}/contactable_inboxes",
headers: admin.create_new_auth_token,
headers: agent.create_new_auth_token,
as: :json

expect(response).to have_http_status(:success)
# only the inboxes which agent has access to are shown
expect(JSON.parse(response.body)['payload'].pluck('inbox').pluck('id')).to eq([twilio_whatsapp_inbox.id])
end
end
end
Expand Down
Expand Up @@ -14,10 +14,14 @@
end
end

context 'when it is an authenticated user' do
context 'when it is an authenticated user with access to the inbox' do
let(:agent) { create(:user, account: account, role: :agent) }
let(:team) { create(:team, account: account) }

before do
create(:inbox_member, inbox: conversation.inbox, user: agent)
end

it 'assigns a user to the conversation' do
params = { assignee_id: agent.id }

Expand Down Expand Up @@ -48,6 +52,7 @@

before do
conversation.update!(assignee: agent)
create(:inbox_member, inbox: conversation.inbox, user: agent)
end

it 'unassigns the assignee from the conversation' do
Expand All @@ -69,6 +74,7 @@

before do
conversation.update!(team: team)
create(:inbox_member, inbox: conversation.inbox, user: agent)
end

it 'unassigns the team from the conversation' do
Expand Down

0 comments on commit 534acfb

Please sign in to comment.