From 887897ea986eca619e750e0651a54443d204c601 Mon Sep 17 00:00:00 2001 From: Sony Mathew Date: Wed, 22 Jul 2026 18:43:00 +0530 Subject: [PATCH] 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 --- app/builders/agent_builder.rb | 22 +++++++- .../api/v1/accounts/agents_controller.rb | 55 +++++++++---------- .../api/v1/accounts/agents_controller.rb | 2 + spec/builders/agent_builder_spec.rb | 18 ++++++ .../api/v1/accounts/agents_controller_spec.rb | 21 +++++++ 5 files changed, 87 insertions(+), 31 deletions(-) diff --git a/app/builders/agent_builder.rb b/app/builders/agent_builder.rb index d2715011c..af68eefc5 100644 --- a/app/builders/agent_builder.rb +++ b/app/builders/agent_builder.rb @@ -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 diff --git a/app/controllers/api/v1/accounts/agents_controller.rb b/app/controllers/api/v1/accounts/agents_controller.rb index 438944f04..864c50bb4 100644 --- a/app/controllers/api/v1/accounts/agents_controller.rb +++ b/app/controllers/api/v1/accounts/agents_controller.rb @@ -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) diff --git a/enterprise/app/controllers/enterprise/api/v1/accounts/agents_controller.rb b/enterprise/app/controllers/enterprise/api/v1/accounts/agents_controller.rb index b3a27c4ad..02d027b58 100644 --- a/enterprise/app/controllers/enterprise/api/v1/accounts/agents_controller.rb +++ b/enterprise/app/controllers/enterprise/api/v1/accounts/agents_controller.rb @@ -1,6 +1,8 @@ module Enterprise::Api::V1::Accounts::AgentsController def create super + return if @agent.blank? + associate_agent_with_custom_role end diff --git a/spec/builders/agent_builder_spec.rb b/spec/builders/agent_builder_spec.rb index f140f2f29..69cacb22b 100644 --- a/spec/builders/agent_builder_spec.rb +++ b/spec/builders/agent_builder_spec.rb @@ -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 diff --git a/spec/enterprise/controllers/api/v1/accounts/agents_controller_spec.rb b/spec/enterprise/controllers/api/v1/accounts/agents_controller_spec.rb index e5a8a5b7d..270150979 100644 --- a/spec/enterprise/controllers/api/v1/accounts/agents_controller_spec.rb +++ b/spec/enterprise/controllers/api/v1/accounts/agents_controller_spec.rb @@ -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