fix: scope Captain assistant responses to account (#15386)
## Description Fixes account scoping for Captain assistant responses. Create and update accepted an `assistant_id` from the request. The model then set the response account from that assistant. The controller lookup read the top level parameter, while the API sends the ID inside `assistant_response`, and create did not use the lookup result. The controller now resolves the nested assistant ID through `Current.account`, removes `assistant_id` before assigning request fields, and assigns the scoped assistant directly. The model now fills the account only when it is blank and rejects a response when its account and assistant do not match. Linear issue: [CW-7913](https://linear.app/chatwoot/issue/CW-7913/ghsa-phpm-m2mf-r8r9-captain-assistant-responses-writes-into-another) ## Type of change - [x] Bug fix ## How has this been tested? - Ran `bundle exec rspec spec/enterprise/controllers/api/v1/accounts/captain/assistant_responses_controller_spec.rb spec/enterprise/models/captain/assistant_response_spec.rb`. All 19 examples passed. - Ran the two new account isolation examples against the original code. Both failed and reproduced the create and update issue. Both pass with this fix. - Ran RuboCop on the five changed Ruby files. It found no offenses. ## Checklist - [x] My code follows the style guidelines of this project - [x] I have performed a self review of my code - [x] I have added tests that prove the fix is effective - [x] New and existing unit tests pass locally with my changes
This commit is contained in:
@@ -2,7 +2,7 @@ class Api::V1::Accounts::Captain::AssistantResponsesController < Api::V1::Accoun
|
||||
before_action -> { check_authorization(Captain::Assistant) }
|
||||
|
||||
before_action :set_current_page, only: [:index]
|
||||
before_action :set_assistant, only: [:create]
|
||||
before_action :set_assistant, only: [:create, :update]
|
||||
before_action :set_responses, except: [:create]
|
||||
before_action :set_response, only: [:show, :update, :destroy]
|
||||
|
||||
@@ -17,13 +17,15 @@ class Api::V1::Accounts::Captain::AssistantResponsesController < Api::V1::Accoun
|
||||
def show; end
|
||||
|
||||
def create
|
||||
@response = Current.account.captain_assistant_responses.new(response_params)
|
||||
@response = Current.account.captain_assistant_responses.new(response_params.except(:assistant_id))
|
||||
@response.assistant = @assistant
|
||||
@response.documentable = Current.user
|
||||
@response.save!
|
||||
end
|
||||
|
||||
def update
|
||||
@response.update!(response_params)
|
||||
@response.assistant = @assistant if response_params.key?(:assistant_id)
|
||||
@response.update!(response_params.except(:assistant_id))
|
||||
end
|
||||
|
||||
def destroy
|
||||
@@ -55,7 +57,7 @@ class Api::V1::Accounts::Captain::AssistantResponsesController < Api::V1::Accoun
|
||||
end
|
||||
|
||||
def set_assistant
|
||||
@assistant = Current.account.captain_assistants.find_by(id: params[:assistant_id])
|
||||
@assistant = Current.account.captain_assistants.find_by(id: response_params[:assistant_id])
|
||||
end
|
||||
|
||||
def set_responses
|
||||
|
||||
@@ -33,6 +33,7 @@ class Captain::AssistantResponse < ApplicationRecord
|
||||
|
||||
validates :question, presence: true
|
||||
validates :answer, presence: true
|
||||
validate :assistant_belongs_to_account
|
||||
|
||||
before_validation :ensure_account
|
||||
before_validation :ensure_status
|
||||
@@ -66,7 +67,13 @@ class Captain::AssistantResponse < ApplicationRecord
|
||||
end
|
||||
|
||||
def ensure_account
|
||||
self.account = assistant&.account
|
||||
self.account ||= assistant&.account
|
||||
end
|
||||
|
||||
def assistant_belongs_to_account
|
||||
return if assistant.blank? || assistant.account_id == account_id
|
||||
|
||||
errors.add(:assistant, :invalid)
|
||||
end
|
||||
|
||||
def update_response_embedding
|
||||
|
||||
@@ -192,6 +192,20 @@ RSpec.describe 'Api::V1::Accounts::Captain::AssistantResponses', type: :request
|
||||
expect(Captain::AssistantResponse.last).to be_approved
|
||||
end
|
||||
|
||||
it 'does not create a response for an assistant in another account' do
|
||||
other_assistant = create(:captain_assistant)
|
||||
params = valid_params.deep_merge(assistant_response: { assistant_id: other_assistant.id })
|
||||
|
||||
expect do
|
||||
post "/api/v1/accounts/#{account.id}/captain/assistant_responses",
|
||||
params: params,
|
||||
headers: admin.create_new_auth_token,
|
||||
as: :json
|
||||
end.not_to change(Captain::AssistantResponse, :count)
|
||||
|
||||
expect(response).to have_http_status(:unprocessable_entity)
|
||||
end
|
||||
|
||||
context 'with invalid params' do
|
||||
let(:invalid_params) do
|
||||
{
|
||||
@@ -236,6 +250,20 @@ RSpec.describe 'Api::V1::Accounts::Captain::AssistantResponses', type: :request
|
||||
expect(json_response[:answer]).to eq('Updated answer')
|
||||
end
|
||||
|
||||
it 'does not move a response to an assistant in another account' do
|
||||
other_assistant = create(:captain_assistant)
|
||||
params = update_params.deep_merge(assistant_response: { assistant_id: other_assistant.id })
|
||||
|
||||
patch "/api/v1/accounts/#{account.id}/captain/assistant_responses/#{response_record.id}",
|
||||
params: params,
|
||||
headers: admin.create_new_auth_token,
|
||||
as: :json
|
||||
|
||||
expect(response).to have_http_status(:unprocessable_entity)
|
||||
expect(response_record.reload.assistant).to eq(assistant)
|
||||
expect(response_record.question).not_to eq('Updated question?')
|
||||
end
|
||||
|
||||
context 'with invalid params' do
|
||||
let(:invalid_params) do
|
||||
{
|
||||
|
||||
21
spec/enterprise/models/captain/assistant_response_spec.rb
Normal file
21
spec/enterprise/models/captain/assistant_response_spec.rb
Normal file
@@ -0,0 +1,21 @@
|
||||
require 'rails_helper'
|
||||
|
||||
RSpec.describe Captain::AssistantResponse, type: :model do
|
||||
describe 'account validation' do
|
||||
it 'uses the assistant account when the account is not set' do
|
||||
assistant = create(:captain_assistant)
|
||||
assistant_response = build(:captain_assistant_response, assistant: assistant, account: nil)
|
||||
|
||||
expect(assistant_response).to be_valid
|
||||
expect(assistant_response.account).to eq(assistant.account)
|
||||
end
|
||||
|
||||
it 'rejects an assistant from another account' do
|
||||
account = create(:account)
|
||||
assistant_response = build(:captain_assistant_response, account: account)
|
||||
|
||||
expect(assistant_response).not_to be_valid
|
||||
expect(assistant_response.errors[:assistant]).to include('is invalid')
|
||||
end
|
||||
end
|
||||
end
|
||||
@@ -1,7 +1,7 @@
|
||||
FactoryBot.define do
|
||||
factory :captain_assistant_response, class: 'Captain::AssistantResponse' do
|
||||
association :assistant, factory: :captain_assistant
|
||||
association :account
|
||||
account { assistant.account }
|
||||
sequence(:question) { |n| "Test question #{n}?" }
|
||||
sequence(:answer) { |n| "Test answer #{n}" }
|
||||
embedding { Array.new(1536) { rand(-1.0..1.0) } }
|
||||
|
||||
Reference in New Issue
Block a user