From 8dd0d08322edafaec24624b72ed2f6045921cb7b Mon Sep 17 00:00:00 2001 From: Shivam Mishra Date: Thu, 16 Jul 2026 18:17:29 +0530 Subject: [PATCH] refactor: align conversation direct uploads with standard account auth (#15039) Conversation attachment uploads now go through the same authentication that every other account-scoped API endpoint uses. Agents continue to attach files exactly as before, and the upload request is now tied to the agent's dashboard session instead of a separately serialized access token. Because the upload request is now authenticated, the dashboard proves the agent's session directly instead of passing `currentUser.access_token`. This keeps uploads working alongside the profile access-token changes in #14973, including on accounts where that token is serialized as empty. ## What changed - `Api::V1::Accounts::Conversations::DirectUploadsController` now runs the standard account auth stack: API access token when the `api_access_token` header is present, dashboard session (devise-token-auth) otherwise, with agent-bot tokens rejected. Previously it inherited `ActiveStorage::DirectUploadsController` directly and did not run any authentication. - `EnsureCurrentAccountHelper#ensure_current_account` now returns `401` when a request has neither an authenticated user nor a bot resource, instead of continuing. This closes the same gap for any controller that relies on the helper. - The dashboard direct-upload paths (`useFileUpload.js` and the legacy `fileUploadMixin.js`) now attach the agent's session headers to the upload request via a new `directUploadsHelper.js`, instead of sending `currentUser.access_token`. ## How to test 1. As a logged-in agent, open a conversation and attach a file. Upload should succeed as before, on installs with direct uploads enabled. 2. Confirm attachments still work for an agent on an account whose profile access token is not serialized (e.g. a Cloud plan without `api_and_webhooks`). 3. Send a `POST` to `/api/v1/accounts/:account_id/conversations/:conversation_id/direct_uploads` with no credentials, an empty `api_access_token`, or an invalid token, and confirm it returns `401`. 4. Confirm a valid agent of the account (via API token or session) gets `200`, while an agent of a different account gets `401`. --- .../direct_uploads_controller.rb | 21 +++ .../concerns/ensure_current_account_helper.rb | 4 +- .../dashboard/composables/useFileUpload.js | 7 +- .../dashboard/helper/directUploadsHelper.js | 19 +++ .../helper/specs/directUploadsHelper.spec.js | 61 +++++++++ .../dashboard/mixins/fileUploadMixin.js | 6 +- config/locales/en.yml | 1 + .../direct_uploads_controller_spec.rb | 125 +++++++++++++++--- 8 files changed, 218 insertions(+), 26 deletions(-) create mode 100644 app/javascript/dashboard/helper/directUploadsHelper.js create mode 100644 app/javascript/dashboard/helper/specs/directUploadsHelper.spec.js diff --git a/app/controllers/api/v1/accounts/conversations/direct_uploads_controller.rb b/app/controllers/api/v1/accounts/conversations/direct_uploads_controller.rb index f4ac05d6e..915ade8a3 100644 --- a/app/controllers/api/v1/accounts/conversations/direct_uploads_controller.rb +++ b/app/controllers/api/v1/accounts/conversations/direct_uploads_controller.rb @@ -1,6 +1,17 @@ class Api::V1::Accounts::Conversations::DirectUploadsController < ActiveStorage::DirectUploadsController + include DeviseTokenAuth::Concerns::SetUserByToken + include RequestExceptionHandler + include AccessTokenAuthHelper include EnsureCurrentAccountHelper + + skip_before_action :verify_authenticity_token, if: :authenticate_by_access_token? + + around_action :handle_with_exception + before_action :authenticate_access_token!, if: :authenticate_by_access_token? + before_action :validate_bot_access_token!, if: :authenticate_by_access_token? + before_action :authenticate_user!, unless: :authenticate_by_access_token? before_action :current_account + before_action :validate_token_api_access, if: :authenticate_by_access_token? before_action :conversation def create @@ -11,6 +22,16 @@ class Api::V1::Accounts::Conversations::DirectUploadsController < ActiveStorage: private + def authenticate_by_access_token? + request.headers[:api_access_token].present? || request.headers[:HTTP_API_ACCESS_TOKEN].present? + end + + def validate_token_api_access + return if Current.account.api_and_webhooks_enabled? + + render json: { error: 'API access is not enabled for this account' }, status: :forbidden + end + def conversation @conversation ||= Current.account.conversations.find_by(display_id: params[:conversation_id]) end diff --git a/app/controllers/concerns/ensure_current_account_helper.rb b/app/controllers/concerns/ensure_current_account_helper.rb index ea36a48f2..7ed39fa98 100644 --- a/app/controllers/concerns/ensure_current_account_helper.rb +++ b/app/controllers/concerns/ensure_current_account_helper.rb @@ -14,6 +14,8 @@ module EnsureCurrentAccountHelper account_accessible_for_user?(account) elsif @resource.is_a?(AgentBot) account_accessible_for_bot?(account) + else + render_unauthorized(I18n.t('errors.account.not_authorized')) end account end @@ -21,7 +23,7 @@ module EnsureCurrentAccountHelper def account_accessible_for_user?(account) @current_account_user = account.account_users.find_by(user_id: current_user.id) Current.account_user = @current_account_user - render_unauthorized('You are not authorized to access this account') unless @current_account_user + render_unauthorized(I18n.t('errors.account.not_authorized')) unless @current_account_user end def account_accessible_for_bot?(account) diff --git a/app/javascript/dashboard/composables/useFileUpload.js b/app/javascript/dashboard/composables/useFileUpload.js index a0c7e3297..49809f606 100644 --- a/app/javascript/dashboard/composables/useFileUpload.js +++ b/app/javascript/dashboard/composables/useFileUpload.js @@ -2,6 +2,7 @@ import { useMapGetter } from 'dashboard/composables/store'; import { useAlert } from 'dashboard/composables'; import { useI18n } from 'vue-i18n'; import { DirectUpload } from 'activestorage'; +import { setDirectUploadAuthHeaders } from 'dashboard/helper/directUploadsHelper'; import { checkFileSizeLimit } from 'shared/helpers/FileHelper'; import { getMaxUploadSizeByChannel } from '@chatwoot/utils'; import { @@ -21,7 +22,6 @@ export const useFileUpload = ({ inbox, attachFile, isPrivateNote = false }) => { const { t } = useI18n(); const accountId = useMapGetter('getCurrentAccountId'); - const currentUser = useMapGetter('getCurrentUser'); const currentChat = useMapGetter('getSelectedChat'); const globalConfig = useMapGetter('globalConfig/get'); @@ -78,10 +78,7 @@ export const useFileUpload = ({ inbox, attachFile, isPrivateNote = false }) => { `/api/v1/accounts/${accountId.value}/conversations/${currentChat.value.id}/direct_uploads`, { directUploadWillCreateBlobWithXHR: xhr => { - xhr.setRequestHeader( - 'api_access_token', - currentUser.value.access_token - ); + setDirectUploadAuthHeaders(xhr); }, } ); diff --git a/app/javascript/dashboard/helper/directUploadsHelper.js b/app/javascript/dashboard/helper/directUploadsHelper.js new file mode 100644 index 000000000..6fafcee20 --- /dev/null +++ b/app/javascript/dashboard/helper/directUploadsHelper.js @@ -0,0 +1,19 @@ +import Auth from 'dashboard/api/auth'; + +export const setDirectUploadAuthHeaders = xhr => { + const { + 'access-token': accessToken, + 'token-type': tokenType, + client, + expiry, + uid, + } = Auth.getAuthData() || {}; + + if (!accessToken) return; + + xhr.setRequestHeader('access-token', accessToken); + xhr.setRequestHeader('token-type', tokenType); + xhr.setRequestHeader('client', client); + xhr.setRequestHeader('expiry', expiry); + xhr.setRequestHeader('uid', uid); +}; diff --git a/app/javascript/dashboard/helper/specs/directUploadsHelper.spec.js b/app/javascript/dashboard/helper/specs/directUploadsHelper.spec.js new file mode 100644 index 000000000..69c90a485 --- /dev/null +++ b/app/javascript/dashboard/helper/specs/directUploadsHelper.spec.js @@ -0,0 +1,61 @@ +import { setDirectUploadAuthHeaders } from '../directUploadsHelper'; +import Auth from 'dashboard/api/auth'; + +vi.mock('dashboard/api/auth', () => ({ + default: { getAuthData: vi.fn() }, +})); + +describe('setDirectUploadAuthHeaders', () => { + const buildXhr = () => ({ setRequestHeader: vi.fn() }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + it('sets the five session auth headers from the auth cookie', () => { + Auth.getAuthData.mockReturnValue({ + 'access-token': 'token-123', + 'token-type': 'Bearer', + client: 'client-123', + expiry: '9999', + uid: 'agent@example.com', + }); + const xhr = buildXhr(); + + setDirectUploadAuthHeaders(xhr); + + expect(xhr.setRequestHeader).toHaveBeenCalledTimes(5); + expect(xhr.setRequestHeader).toHaveBeenCalledWith( + 'access-token', + 'token-123' + ); + expect(xhr.setRequestHeader).toHaveBeenCalledWith('token-type', 'Bearer'); + expect(xhr.setRequestHeader).toHaveBeenCalledWith('client', 'client-123'); + expect(xhr.setRequestHeader).toHaveBeenCalledWith('expiry', '9999'); + expect(xhr.setRequestHeader).toHaveBeenCalledWith( + 'uid', + 'agent@example.com' + ); + }); + + it('does not set any header when there is no auth data', () => { + Auth.getAuthData.mockReturnValue(false); + const xhr = buildXhr(); + + setDirectUploadAuthHeaders(xhr); + + expect(xhr.setRequestHeader).not.toHaveBeenCalled(); + }); + + it('does not set any header when the access token is missing', () => { + Auth.getAuthData.mockReturnValue({ + client: 'client-123', + uid: 'agent@example.com', + }); + const xhr = buildXhr(); + + setDirectUploadAuthHeaders(xhr); + + expect(xhr.setRequestHeader).not.toHaveBeenCalled(); + }); +}); diff --git a/app/javascript/dashboard/mixins/fileUploadMixin.js b/app/javascript/dashboard/mixins/fileUploadMixin.js index 965846401..e4a2c9dce 100644 --- a/app/javascript/dashboard/mixins/fileUploadMixin.js +++ b/app/javascript/dashboard/mixins/fileUploadMixin.js @@ -3,6 +3,7 @@ import { useAlert } from 'dashboard/composables'; import { checkFileSizeLimit } from 'shared/helpers/FileHelper'; import { getMaxUploadSizeByChannel } from '@chatwoot/utils'; import { DirectUpload } from 'activestorage'; +import { setDirectUploadAuthHeaders } from 'dashboard/helper/directUploadsHelper'; import { DEFAULT_MAXIMUM_FILE_UPLOAD_SIZE, resolveMaximumFileUploadSize, @@ -77,10 +78,7 @@ export default { `/api/v1/accounts/${this.accountId}/conversations/${this.currentChat.id}/direct_uploads`, { directUploadWillCreateBlobWithXHR: xhr => { - xhr.setRequestHeader( - 'api_access_token', - this.currentUser.access_token - ); + setDirectUploadAuthHeaders(xhr); }, } ); diff --git a/config/locales/en.yml b/config/locales/en.yml index 011957fb4..1b66699f2 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -75,6 +75,7 @@ en: errors: account: + not_authorized: You are not authorized to access this account reporting_timezone: invalid: is not a valid timezone support_email: diff --git a/spec/controllers/api/v1/accounts/conversations/direct_uploads_controller_spec.rb b/spec/controllers/api/v1/accounts/conversations/direct_uploads_controller_spec.rb index 089b16b59..406f2e07c 100644 --- a/spec/controllers/api/v1/accounts/conversations/direct_uploads_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/conversations/direct_uploads_controller_spec.rb @@ -7,27 +7,120 @@ RSpec.describe '/api/v1/accounts/:account_id/conversations/:conversation_id/dire let(:contact) { create(:contact, account: account, email: nil) } let(:contact_inbox) { create(:contact_inbox, contact: contact, inbox: web_widget.inbox) } let(:conversation) { create(:conversation, contact: contact, account: account, inbox: web_widget.inbox, contact_inbox: contact_inbox) } + let(:blob_params) do + { + blob: { + filename: 'avatar.png', + byte_size: '1234', + checksum: 'dsjbsdhbfif3874823mnsdbf', + content_type: 'image/png' + } + } + end + + def create_direct_upload(headers) + post api_v1_account_conversation_direct_uploads_path(account_id: account.id, conversation_id: conversation.display_id), + params: blob_params, + headers: headers, + as: :json + end describe 'POST /api/v1/accounts/:account_id/conversations/:conversation_id/direct_uploads' do - context 'when post request is made' do - it 'creates attachment message in conversation' do - contact + context 'when it is an unauthenticated request' do + it 'returns unauthorized without any credentials' do + create_direct_upload({}) - post api_v1_account_conversation_direct_uploads_path(account_id: account.id, conversation_id: conversation.display_id), - params: { - blob: { - filename: 'avatar.png', - byte_size: '1234', - checksum: 'dsjbsdhbfif3874823mnsdbf', - content_type: 'image/png' - } - }, - headers: { api_access_token: agent.access_token.token }, - as: :json + expect(response).to have_http_status(:unauthorized) + end + + it 'returns unauthorized with an empty api_access_token header' do + create_direct_upload({ api_access_token: '' }) + + expect(response).to have_http_status(:unauthorized) + end + + it 'returns unauthorized with an invalid api_access_token header' do + create_direct_upload({ api_access_token: 'invalid-token' }) + + expect(response).to have_http_status(:unauthorized) + end + end + + context 'when it is an authenticated request with an api access token' do + it 'creates the blob for the direct upload' do + create_direct_upload({ api_access_token: agent.access_token.token }) expect(response).to have_http_status(:success) - json_response = response.parsed_body - expect(json_response['content_type']).to eq('image/png') + expect(response.parsed_body['content_type']).to eq('image/png') + end + + it 'returns unauthorized for an agent of another account' do + other_agent = create(:user, account: create(:account), role: :agent) + + create_direct_upload({ api_access_token: other_agent.access_token.token }) + + expect(response).to have_http_status(:unauthorized) + end + + it 'returns unauthorized for an agent bot token' do + agent_bot = create(:agent_bot, account: account) + + create_direct_upload({ api_access_token: agent_bot.access_token.token }) + + expect(response).to have_http_status(:unauthorized) + end + end + + context 'when the account api_and_webhooks feature is disabled' do + before do + allow(Account).to receive(:find).and_call_original + allow(Account).to receive(:find).with(account.id.to_s).and_return(account) + allow(account).to receive(:api_and_webhooks_enabled?).and_return(false) + end + + it 'returns forbidden for a token-authenticated request' do + create_direct_upload({ api_access_token: agent.access_token.token }) + + expect(response).to have_http_status(:forbidden) + end + + it 'still creates the blob for a session-authenticated request' do + create_direct_upload(agent.create_new_auth_token) + + expect(response).to have_http_status(:success) + expect(response.parsed_body['content_type']).to eq('image/png') + end + end + + context 'when it is an authenticated session request' do + it 'creates the blob for the direct upload' do + create_direct_upload(agent.create_new_auth_token) + + expect(response).to have_http_status(:success) + expect(response.parsed_body['content_type']).to eq('image/png') + end + + it 'creates the blob when the serialized api access token is empty' do + create_direct_upload(agent.create_new_auth_token.merge('api_access_token' => '')) + + expect(response).to have_http_status(:success) + expect(response.parsed_body['content_type']).to eq('image/png') + end + end + + context 'when forgery protection is enabled' do + around do |example| + original = ActionController::Base.allow_forgery_protection + ActionController::Base.allow_forgery_protection = true + example.run + ActionController::Base.allow_forgery_protection = original + end + + it 'creates the blob for a token-authenticated request without a CSRF token' do + create_direct_upload({ api_access_token: agent.access_token.token }) + + expect(response).to have_http_status(:success) + expect(response.parsed_body['content_type']).to eq('image/png') end end end