fix: lock agent quota checks (#15029)
# Pull Request Template ## Description Locks the agent quota check to the account row while creating account users. This fixes a race where concurrent agent-create requests could all observe the same remaining seat before any `account_users` row was inserted. The API continues to return the existing `402 Account limit exceeded. Please purchase more licenses` response when the limit is reached. Bulk create now preflights the requested email count while holding the account lock, then creates each agent through the same locked builder path. The Enterprise custom-role hook now no-ops when create did not produce an agent. Fixes: [CW-7039](https://linear.app/chatwoot/issue/CW-7039/race-condition-in-agent-creation-bypasses-plan-agent-seat-limit) ## Type of change - [x] Bug fix (non-breaking change which fixes an issue) ## How Has This Been Tested? - `POSTGRES_DATABASE=chatwoot_test_c20f_agent_quota REDIS_DB=9 bundle exec rspec spec/builders/agent_builder_spec.rb spec/enterprise/builders/agent_builder_spec.rb spec/controllers/api/v1/accounts/agents_controller_spec.rb spec/enterprise/controllers/api/v1/accounts/agents_controller_spec.rb spec/enterprise/controllers/enterprise/api/v1/accounts/agents_controller_spec.rb` - `bundle exec rubocop app/builders/agent_builder.rb app/controllers/api/v1/accounts/agents_controller.rb enterprise/app/controllers/enterprise/api/v1/accounts/agents_controller.rb spec/builders/agent_builder_spec.rb spec/enterprise/controllers/api/v1/accounts/agents_controller_spec.rb` - `git diff --check` - One-off threaded Rails validation with 8 concurrent `AgentBuilder` calls against an account with one remaining seat: `created: 1`, `limited: 7`, final `count=2`, `limit=2`. ## Checklist: - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my code - [x] I have commented on my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [ ] Any dependent changes have been merged and published in downstream modules Co-authored-by: Muhsin Keloth <muhsinkeramam@gmail.com>
This commit is contained in:
co-authored by
Muhsin Keloth
parent
8aee518149
commit
887897ea98
@@ -2,6 +2,14 @@
|
||||
# It initializes with necessary attributes and provides a perform method
|
||||
# to create a user and account user in a transaction.
|
||||
class AgentBuilder
|
||||
LIMIT_EXCEEDED_MESSAGE = 'Account limit exceeded. Please purchase more licenses'.freeze
|
||||
|
||||
class LimitExceededError < StandardError
|
||||
def initialize
|
||||
super(AgentBuilder::LIMIT_EXCEEDED_MESSAGE)
|
||||
end
|
||||
end
|
||||
|
||||
# Initializes an AgentBuilder with necessary attributes.
|
||||
# @param email [String] the email of the user.
|
||||
# @param name [String] the name of the user.
|
||||
@@ -14,15 +22,23 @@ class AgentBuilder
|
||||
# Creates a user and account user in a transaction.
|
||||
# @return [User] the created user.
|
||||
def perform
|
||||
ActiveRecord::Base.transaction do
|
||||
@user = find_or_create_user
|
||||
create_account_user
|
||||
account.with_lock do
|
||||
raise LimitExceededError unless can_add_agent?
|
||||
|
||||
ActiveRecord::Base.transaction do
|
||||
@user = find_or_create_user
|
||||
create_account_user
|
||||
end
|
||||
end
|
||||
@user
|
||||
end
|
||||
|
||||
private
|
||||
|
||||
def can_add_agent?
|
||||
account.usage_limits[:agents] > account.account_users.count
|
||||
end
|
||||
|
||||
# Finds a user by email or creates a new one with a temporary password.
|
||||
# @return [User] the found or created user.
|
||||
def find_or_create_user
|
||||
|
||||
@@ -1,8 +1,6 @@
|
||||
class Api::V1::Accounts::AgentsController < Api::V1::Accounts::BaseController
|
||||
before_action :fetch_agent, except: [:create, :index, :bulk_create]
|
||||
before_action :check_authorization
|
||||
before_action :validate_limit, only: [:create]
|
||||
before_action :validate_limit_for_bulk_create, only: [:bulk_create]
|
||||
|
||||
def index
|
||||
@agents = agents
|
||||
@@ -20,6 +18,8 @@ class Api::V1::Accounts::AgentsController < Api::V1::Accounts::BaseController
|
||||
)
|
||||
|
||||
@agent = builder.perform
|
||||
rescue AgentBuilder::LimitExceededError => e
|
||||
render_payment_required(e.message)
|
||||
end
|
||||
|
||||
def update
|
||||
@@ -36,25 +36,13 @@ class Api::V1::Accounts::AgentsController < Api::V1::Accounts::BaseController
|
||||
def bulk_create
|
||||
emails = params[:emails]
|
||||
|
||||
emails.each do |email|
|
||||
builder = AgentBuilder.new(
|
||||
email: email,
|
||||
name: email.split('@').first,
|
||||
inviter: current_user,
|
||||
account: Current.account
|
||||
)
|
||||
begin
|
||||
builder.perform
|
||||
rescue ActiveRecord::RecordInvalid => e
|
||||
Rails.logger.info "[Agent#bulk_create] ignoring email #{email}, errors: #{e.record.errors}"
|
||||
end
|
||||
end
|
||||
|
||||
bulk_create_agents(emails)
|
||||
# This endpoint is used to bulk create agents during onboarding
|
||||
# onboarding_step key in present in Current account custom attributes, since this is a one time operation
|
||||
Current.account.custom_attributes.delete('onboarding_step')
|
||||
Current.account.save!
|
||||
clear_onboarding_step
|
||||
head :ok
|
||||
rescue AgentBuilder::LimitExceededError => e
|
||||
render_payment_required(e.message)
|
||||
end
|
||||
|
||||
private
|
||||
@@ -87,22 +75,33 @@ class Api::V1::Accounts::AgentsController < Api::V1::Accounts::BaseController
|
||||
@agents ||= Current.account.users.order_by_full_name.includes(:account_users, { avatar_attachment: [:blob] })
|
||||
end
|
||||
|
||||
def validate_limit_for_bulk_create
|
||||
limit_available = params[:emails].count <= available_agent_count
|
||||
def bulk_create_agents(emails)
|
||||
Current.account.with_lock do
|
||||
raise AgentBuilder::LimitExceededError if emails.count > available_agent_count
|
||||
|
||||
render_payment_required('Account limit exceeded. Please purchase more licenses') unless limit_available
|
||||
emails.each { |email| create_agent_from_email(email) }
|
||||
end
|
||||
end
|
||||
|
||||
def validate_limit
|
||||
render_payment_required('Account limit exceeded. Please purchase more licenses') unless can_add_agent?
|
||||
def create_agent_from_email(email)
|
||||
builder = AgentBuilder.new(
|
||||
email: email,
|
||||
name: email.split('@').first,
|
||||
inviter: current_user,
|
||||
account: Current.account
|
||||
)
|
||||
builder.perform
|
||||
rescue ActiveRecord::RecordInvalid => e
|
||||
Rails.logger.info "[Agent#bulk_create] ignoring email #{email}, errors: #{e.record.errors}"
|
||||
end
|
||||
|
||||
def clear_onboarding_step
|
||||
Current.account.custom_attributes.delete('onboarding_step')
|
||||
Current.account.save!
|
||||
end
|
||||
|
||||
def available_agent_count
|
||||
Current.account.usage_limits[:agents] - agents.count
|
||||
end
|
||||
|
||||
def can_add_agent?
|
||||
available_agent_count.positive?
|
||||
Current.account.usage_limits[:agents] - Current.account.account_users.count
|
||||
end
|
||||
|
||||
def delete_user_record(agent)
|
||||
|
||||
@@ -1,6 +1,8 @@
|
||||
module Enterprise::Api::V1::Accounts::AgentsController
|
||||
def create
|
||||
super
|
||||
return if @agent.blank?
|
||||
|
||||
associate_agent_with_custom_role
|
||||
end
|
||||
|
||||
|
||||
@@ -23,6 +23,12 @@ RSpec.describe AgentBuilder, type: :model do
|
||||
end
|
||||
|
||||
describe '#perform' do
|
||||
it 'locks the account while checking and creating the agent' do
|
||||
expect(account).to receive(:with_lock).and_call_original
|
||||
|
||||
agent_builder.perform
|
||||
end
|
||||
|
||||
context 'when user does not exist' do
|
||||
it 'creates a new user' do
|
||||
expect { agent_builder.perform }.to change(User, :count).by(1)
|
||||
@@ -67,5 +73,17 @@ RSpec.describe AgentBuilder, type: :model do
|
||||
expect(user.encrypted_password).not_to be_empty
|
||||
end
|
||||
end
|
||||
|
||||
context 'when the account has reached its agent limit' do
|
||||
before do
|
||||
allow(account).to receive(:usage_limits).and_return({ agents: account.account_users.count })
|
||||
end
|
||||
|
||||
it 'raises a limit exceeded error without creating a user' do
|
||||
expect { agent_builder.perform }.to raise_error(described_class::LimitExceededError, described_class::LIMIT_EXCEEDED_MESSAGE)
|
||||
|
||||
expect(User.from_email(email)).to be_nil
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
@@ -21,6 +21,27 @@ RSpec.describe 'Agents API', type: :request do
|
||||
expect(response).to have_http_status(:payment_required)
|
||||
expect(response.body).to include('Account limit exceeded. Please purchase more licenses')
|
||||
end
|
||||
|
||||
it 'prevents adding an agent if the last seat is consumed before creation' do
|
||||
account.update!(limits: { agents: account.account_users.count + 1 })
|
||||
competing_agent_created = false
|
||||
|
||||
allow(AgentBuilder).to receive(:new).and_wrap_original do |method, *args|
|
||||
unless competing_agent_created
|
||||
create(:user, account: account, role: :agent)
|
||||
competing_agent_created = true
|
||||
end
|
||||
|
||||
method.call(*args)
|
||||
end
|
||||
|
||||
post "/api/v1/accounts/#{account.id}/agents", params: params, headers: admin.create_new_auth_token, as: :json
|
||||
|
||||
expect(response).to have_http_status(:payment_required)
|
||||
expect(response.body).to include('Account limit exceeded. Please purchase more licenses')
|
||||
expect(User.from_email(params[:email])).to be_nil
|
||||
expect(account.account_users.count).to eq(account.usage_limits[:agents])
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
|
||||
Reference in New Issue
Block a user