fix(security): gate SLA APIs and automation on the sla feature (#15209)
SLA is a premium feature, but several SLA surfaces never checked the
account's `sla` flag. An account without SLA — or one whose plan was
downgraded and had the flag revoked — could still read SLA breach
reporting, attach an SLA policy to a conversation through the
conversation update endpoint, and keep applying SLA policies from
automation rules. All three paths now require the feature, matching the
SLA policies API which already checked it.
## How to reproduce
1. Disable the `sla` feature for an account that has SLA policies and
applied SLAs.
2. `GET /api/v1/accounts/{id}/applied_slas/metrics` (also `index` and
`download`) — returns the account's SLA breach data instead of denying.
3. `PATCH /api/v1/accounts/{id}/conversations/{display_id}` with
`sla_policy_id` — the policy is attached.
4. Trigger an automation rule with an "Add SLA" action — the SLA is
applied and starts tracking.
## What changed
- `Api::V1::Accounts::AppliedSlasController` gains the same
`ensure_sla_feature_enabled` guard the SLA policies controller uses,
covering `index`, `metrics` and `download`.
-
`Enterprise::Api::V1::Accounts::ConversationsController#permitted_update_params`
only permits `sla_policy_id` when the feature is enabled. When it is off
the parameter is dropped, so an existing SLA association is preserved
rather than cleared.
- `Enterprise::ActionService#add_sla` returns early when the feature is
off, so automation rules stop applying SLAs on revoked accounts.
This commit is contained in:
committed by
GitHub
parent
e7ded47753
commit
8448001fdc
@@ -58,6 +58,6 @@ json.last_non_activity_message conversation.messages.where(account_id: conversat
|
||||
json.last_activity_at conversation.last_activity_at.to_i
|
||||
json.priority conversation.priority
|
||||
json.waiting_since conversation.waiting_since.to_i.to_i
|
||||
sla_applicable = !conversation.respond_to?(:sla_applicable?) || conversation.sla_applicable?
|
||||
sla_applicable = conversation.account.feature_enabled?('sla') && (!conversation.respond_to?(:sla_applicable?) || conversation.sla_applicable?)
|
||||
json.sla_policy_id sla_applicable ? conversation.sla_policy_id : nil
|
||||
json.partial! 'enterprise/api/v1/conversations/partials/conversation', conversation: conversation if ChatwootApp.enterprise?
|
||||
|
||||
@@ -4,6 +4,7 @@ class Api::V1::Accounts::AppliedSlasController < Api::V1::Accounts::EnterpriseAc
|
||||
|
||||
RESULTS_PER_PAGE = 25
|
||||
|
||||
before_action :ensure_sla_feature_enabled
|
||||
before_action :set_applied_slas, only: [:index, :metrics, :download]
|
||||
before_action :set_current_page, only: [:index]
|
||||
before_action :check_admin_authorization?
|
||||
@@ -30,6 +31,10 @@ class Api::V1::Accounts::AppliedSlasController < Api::V1::Accounts::EnterpriseAc
|
||||
|
||||
private
|
||||
|
||||
def ensure_sla_feature_enabled
|
||||
raise Pundit::NotAuthorizedError unless Current.account.feature_enabled?('sla')
|
||||
end
|
||||
|
||||
def total_applied_slas
|
||||
@total_applied_slas ||= @applied_slas.count
|
||||
end
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
class Api::V1::Accounts::SlaPoliciesController < Api::V1::Accounts::EnterpriseAccountsController
|
||||
before_action :ensure_sla_feature_enabled
|
||||
before_action :fetch_sla, only: [:show, :update, :destroy]
|
||||
before_action :check_authorization
|
||||
|
||||
@@ -29,4 +30,8 @@ class Api::V1::Accounts::SlaPoliciesController < Api::V1::Accounts::EnterpriseAc
|
||||
def fetch_sla
|
||||
@sla_policy = Current.account.sla_policies.find_by(id: params[:id])
|
||||
end
|
||||
|
||||
def ensure_sla_feature_enabled
|
||||
raise Pundit::NotAuthorizedError unless Current.account.feature_enabled?('sla')
|
||||
end
|
||||
end
|
||||
|
||||
@@ -16,6 +16,9 @@ module Enterprise::Api::V1::Accounts::ConversationsController
|
||||
end
|
||||
|
||||
def permitted_update_params
|
||||
# SLA is a premium feature; only accept sla_policy_id assignment when it is enabled for the account.
|
||||
return super unless Current.account.feature_enabled?('sla')
|
||||
|
||||
super.merge(params.permit(:sla_policy_id))
|
||||
end
|
||||
|
||||
|
||||
@@ -2,6 +2,9 @@ class Sla::ProcessAccountAppliedSlasJob < ApplicationJob
|
||||
queue_as :medium
|
||||
|
||||
def perform(account)
|
||||
# The scheduler filters on the feature, but this job can already be queued when a plan is downgraded.
|
||||
return unless account.feature_enabled?('sla')
|
||||
|
||||
account.applied_slas.with_sla_applicable_conversation.where(sla_status: %w[active active_with_misses]).each do |applied_sla|
|
||||
Sla::ProcessAppliedSlaJob.perform_later(applied_sla)
|
||||
end
|
||||
|
||||
@@ -2,6 +2,9 @@ class Sla::ProcessAppliedSlaJob < ApplicationJob
|
||||
queue_as :medium
|
||||
|
||||
def perform(applied_sla)
|
||||
# This job can already be queued when a plan is downgraded, so re-check before evaluating.
|
||||
return unless applied_sla.account.feature_enabled?('sla')
|
||||
|
||||
Sla::EvaluateAppliedSlaService.new(applied_sla: applied_sla).perform
|
||||
end
|
||||
end
|
||||
|
||||
@@ -2,7 +2,8 @@ class Sla::TriggerSlasForAccountsJob < ApplicationJob
|
||||
queue_as :scheduled_jobs
|
||||
|
||||
def perform
|
||||
Account.joins(:sla_policies).distinct.find_each do |account|
|
||||
# SLA is a premium feature; skip accounts that have policies left over from a downgrade.
|
||||
Account.feature_sla.joins(:sla_policies).distinct.find_each do |account|
|
||||
Rails.logger.info "Enqueuing ProcessAccountAppliedSlasJob for account #{account.id}"
|
||||
Sla::ProcessAccountAppliedSlasJob.perform_later(account)
|
||||
end
|
||||
|
||||
@@ -1,6 +1,8 @@
|
||||
module Enterprise::ActionService
|
||||
def add_sla(sla_policy_id)
|
||||
return if sla_policy_id.blank?
|
||||
# SLA is a premium feature; automation rules must not keep applying SLAs once it is disabled.
|
||||
return unless @account.feature_enabled?('sla')
|
||||
|
||||
sla_policy = @account.sla_policies.find_by(id: sla_policy_id.first)
|
||||
return if sla_policy.nil?
|
||||
|
||||
@@ -12,6 +12,7 @@ RSpec.describe 'Applied SLAs API', type: :request do
|
||||
let(:sla_policy2) { create(:sla_policy, account: account) }
|
||||
|
||||
before do
|
||||
account.enable_features!('sla')
|
||||
AppliedSla.destroy_all
|
||||
end
|
||||
|
||||
@@ -117,6 +118,18 @@ RSpec.describe 'Applied SLAs API', type: :request do
|
||||
expect(body).to include('hit_rate' => '50.0%')
|
||||
end
|
||||
end
|
||||
|
||||
context 'when the sla feature is disabled' do
|
||||
it 'returns unauthorized' do
|
||||
account.disable_features!('sla')
|
||||
create(:applied_sla, sla_policy: sla_policy1, conversation: conversation1, sla_status: 'missed')
|
||||
|
||||
get "/api/v1/accounts/#{account.id}/applied_slas/metrics",
|
||||
headers: administrator.create_new_auth_token
|
||||
|
||||
expect(response).to have_http_status(:unauthorized)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
describe 'GET /api/v1/accounts/{account.id}/applied_slas/download' do
|
||||
|
||||
@@ -35,16 +35,18 @@ RSpec.describe 'Conversations API', type: :request do
|
||||
end
|
||||
|
||||
it 'does not return SLA data for the conversation if the feature is disabled' do
|
||||
account.enable_features!('sla')
|
||||
sla_policy = create(:sla_policy, account: account)
|
||||
conversation = create(:conversation, account: account, sla_policy: sla_policy)
|
||||
create(:sla_event, conversation: conversation, applied_sla: conversation.applied_sla)
|
||||
account.disable_features!('sla')
|
||||
conversation = create(:conversation, account: account)
|
||||
create(:applied_sla, conversation: conversation)
|
||||
create(:sla_event, conversation: conversation)
|
||||
|
||||
get "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}", headers: administrator.create_new_auth_token
|
||||
|
||||
expect(response).to have_http_status(:ok)
|
||||
expect(response.parsed_body.keys).not_to include('applied_sla')
|
||||
expect(response.parsed_body.keys).not_to include('sla_events')
|
||||
expect(response.parsed_body['sla_policy_id']).to be_nil
|
||||
end
|
||||
|
||||
context 'when agent has team access' do
|
||||
|
||||
@@ -6,6 +6,7 @@ RSpec.describe 'Enterprise SLA API', type: :request do
|
||||
let(:agent) { create(:user, account: account, role: :agent) }
|
||||
|
||||
before do
|
||||
account.enable_features!('sla')
|
||||
create(:sla_policy, account: account, name: 'SLA 1')
|
||||
end
|
||||
|
||||
|
||||
@@ -13,6 +13,7 @@ RSpec.describe 'Enterprise Conversations API', type: :request do
|
||||
let(:agent) { create(:user, account: account, role: :agent) }
|
||||
|
||||
before do
|
||||
account.enable_features!('sla')
|
||||
create(:inbox_member, user: agent, inbox: conversation.inbox)
|
||||
end
|
||||
|
||||
@@ -50,5 +51,27 @@ RSpec.describe 'Enterprise Conversations API', type: :request do
|
||||
.to eq('Sla policy cannot be assigned to conversations with blocked contacts')
|
||||
end
|
||||
end
|
||||
|
||||
context 'when the sla feature is disabled' do
|
||||
let(:agent) { create(:user, account: account, role: :agent) }
|
||||
let(:existing_sla_policy) { create(:sla_policy, account: account) }
|
||||
|
||||
before do
|
||||
account.enable_features!('sla')
|
||||
conversation.update!(sla_policy: existing_sla_policy)
|
||||
account.disable_features!('sla')
|
||||
create(:inbox_member, user: agent, inbox: conversation.inbox)
|
||||
end
|
||||
|
||||
it 'ignores the sla assignment and keeps the existing policy' do
|
||||
patch "/api/v1/accounts/#{account.id}/conversations/#{conversation.display_id}",
|
||||
params: params,
|
||||
headers: agent.create_new_auth_token,
|
||||
as: :json
|
||||
|
||||
expect(response).to have_http_status(:success)
|
||||
expect(conversation.reload.sla_policy_id).to eq(existing_sla_policy.id)
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
@@ -11,6 +11,7 @@ RSpec.describe Sla::ProcessAccountAppliedSlasJob do
|
||||
let!(:blocked_contact_applied_sla) { create(:applied_sla, account: account, sla_policy: sla_policy, sla_status: 'active') }
|
||||
|
||||
before do
|
||||
account.enable_features!('sla')
|
||||
blocked_contact_applied_sla.conversation.contact.update!(blocked: true)
|
||||
end
|
||||
|
||||
@@ -32,5 +33,12 @@ RSpec.describe Sla::ProcessAccountAppliedSlasJob do
|
||||
expect(Sla::ProcessAppliedSlaJob).not_to receive(:perform_later).with(miss_applied_sla)
|
||||
described_class.perform_now(account)
|
||||
end
|
||||
|
||||
it 'does not call the ProcessAppliedSlaJob when the sla feature is disabled' do
|
||||
account.disable_features!('sla')
|
||||
|
||||
expect(Sla::ProcessAppliedSlaJob).not_to receive(:perform_later)
|
||||
described_class.perform_now(account)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
@@ -5,6 +5,8 @@ RSpec.describe Sla::ProcessAppliedSlaJob do
|
||||
let(:account) { create(:account) }
|
||||
let(:applied_sla) { create(:applied_sla, account: account) }
|
||||
|
||||
before { account.enable_features!('sla') }
|
||||
|
||||
it 'enqueues the job' do
|
||||
expect { described_class.perform_later(applied_sla) }.to have_enqueued_job(described_class)
|
||||
.with(applied_sla)
|
||||
@@ -13,7 +15,14 @@ RSpec.describe Sla::ProcessAppliedSlaJob do
|
||||
|
||||
it 'calls the EvaluateAppliedSlaService' do
|
||||
expect(Sla::EvaluateAppliedSlaService).to receive(:new).with(applied_sla: applied_sla).and_call_original
|
||||
described_class.perform_now(applied_sla)
|
||||
described_class.perform_now(applied_sla.reload)
|
||||
end
|
||||
|
||||
it 'does not call the EvaluateAppliedSlaService when the sla feature is disabled' do
|
||||
account.disable_features!('sla')
|
||||
|
||||
expect(Sla::EvaluateAppliedSlaService).not_to receive(:new)
|
||||
described_class.perform_now(applied_sla.reload)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
@@ -3,8 +3,10 @@ RSpec.describe Sla::TriggerSlasForAccountsJob do
|
||||
context 'when perform is called' do
|
||||
let(:account_with_sla) { create(:account) }
|
||||
let(:account_without_sla) { create(:account) }
|
||||
let(:downgraded_account) { create(:account) }
|
||||
|
||||
before do
|
||||
account_with_sla.enable_features!('sla')
|
||||
create(:sla_policy, account: account_with_sla)
|
||||
end
|
||||
|
||||
@@ -22,5 +24,13 @@ RSpec.describe Sla::TriggerSlasForAccountsJob do
|
||||
expect(Sla::ProcessAccountAppliedSlasJob).not_to receive(:perform_later).with(account_without_sla)
|
||||
described_class.perform_now
|
||||
end
|
||||
|
||||
it 'does not call the ProcessAccountAppliedSlasJob for accounts with the sla feature disabled' do
|
||||
create(:sla_policy, account: downgraded_account)
|
||||
downgraded_account.disable_features!('sla')
|
||||
|
||||
expect(Sla::ProcessAccountAppliedSlasJob).not_to receive(:perform_later).with(downgraded_account)
|
||||
described_class.perform_now
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
@@ -8,6 +8,8 @@ describe ActionService do
|
||||
let(:conversation) { create(:conversation, account: account) }
|
||||
let(:action_service) { described_class.new(conversation) }
|
||||
|
||||
before { account.enable_features!('sla') }
|
||||
|
||||
context 'when sla_policy_id is present' do
|
||||
it 'adds the sla policy to the conversation and create applied_sla entry' do
|
||||
action_service.add_sla([sla_policy.id])
|
||||
|
||||
Reference in New Issue
Block a user