Files
sure/app/controllers/transfers_controller.rb
T
47c46843e1 fix(transfers): prevent duplicate creation on double-submit (#3342)
* fix(transfers): prevent duplicate creation on double-submit

TransfersController#create -> Transfer::Creator had no protection
against a repeated form submission - a double-click, a browser retry,
or two near-simultaneous requests could each create a separate,
identical transfer (and its 2-4 underlying Entry/Transaction rows).

Adds a per-form idempotency key, the same approach already used for
TransactionsController#create: a UUID hidden field generated fresh on
page load, tagging the outflow/inflow (and fee, with a distinguishing
suffix since a fee leg shares its account with its primary leg) entries
via the existing entries(account_id, source, external_id) partial
unique index. A pre-check handles the sequential double-submit case;
rescue ActiveRecord::RecordNotUnique is the authoritative backstop for
genuine concurrent requests - the whole Transfer.transaction block
rolls back cleanly on conflict, so there's no risk of a half-created
transfer.

A same-day duplicate transfer can be legitimate (unlike a duplicate
valuation, see #3339/PR #3340), so this uses the same per-submission
token approach as #3334/PR #3338 rather than a natural-key DB
constraint.

Fixes #3341.

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(transfers): store the idempotency key in its own column, isolate the retry with a savepoint

Same two review findings as PR #3338 (transactions) and #3340
(valuations), applied here since this branch shares the same
mechanism:

- Reusing external_id/source for the web-form idempotency token made
  every leg of a manually-created transfer satisfy Entry#linked?,
  incorrectly making it look provider-synced. Uses the same dedicated
  entries.idempotency_key column added in
  db/migrate/20260902180400_add_idempotency_key_to_entries.rb (cherry-picked
  identically from PR #3338 - this branch depends on that migration;
  please merge #3338 first, or merge this after it lands so the
  duplicate migration file is a no-op).

- Transfer::Creator now wraps the actual save in
  Transfer.transaction(requires_new: true) so a RecordNotUnique only
  rolls back to a savepoint rather than aborting any transaction the
  caller might already be in, keeping the rescue's retry lookup usable
  (mirrors the fix already applied to Account::ReconciliationManager
  in PR #3340).

Added a regression test asserting neither leg of a transfer created
via this path is linked? or has external_id/source set.

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(transfers): distinct idempotency key per leg, rebuild invalid index on retry

Two more review findings:

- CodeRabbit: the form doesn't prevent selecting the same account as
  both source and destination. The outflow and inflow legs shared the
  bare idempotency key, so on that same-account path they'd collide
  with each other under the same account-scoped unique index (as
  would both fee legs, which shared a single "-fee" suffix). Every
  leg now gets a distinct, role-specific suffix (outflow stays bare -
  that's what find_existing_transfer looks up by - inflow/source_fee/
  destination_fee each get their own).

- Codex (same finding already fixed once for entries.idempotency_key's
  sibling migration, recurring here since this branch carries an
  identical copy): index_exists? alone doesn't distinguish a valid
  index from an INVALID one left behind by an interrupted CREATE INDEX
  CONCURRENTLY, so a retry after a failed build would short-circuit
  and record the migration as applied while the constraint was still
  missing. Now checks pg_index.indisvalid directly before deciding to
  skip.

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(transfers): clear idempotency key on destroy so a retry doesn't 500

Codex flagged that Transfer#destroy! (used by reject!) preserves the
outflow/inflow entries but not the Transfer join row - a retried
create request with the same idempotency_key would find no Transfer
via find_existing_transfer, attempt another insert, hit the stale
entry's unique key, and re-raise RecordNotUnique instead of finding
a match. Clear the key on the surviving entries when a transfer is
destroyed.

Also adds a regression test for the CodeRabbit-flagged per-leg key
collision concern (already fixed by role-specific suffixes in the
prior commit) to lock in that fee legs never share a key with their
primary leg.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(transfers): verify idempotency key matches the request, fix stale doc comment

jjmata review on #3342:
- find_existing_transfer matched on idempotency_key + source_account only,
  so a stale key from a cached form could silently return a different,
  older transfer instead of creating the one actually requested. Now
  verifies destination account, date, and amount before treating a key
  match as the same request; a genuine mismatch surfaces as a new
  StaleIdempotencyKeyError (422 + message) instead of a false success or
  a raw 500.
- Removed a comment claiming parity with a
  TransactionsController#new_transaction_idempotency_key method that
  doesn't exist in the codebase.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* fix(transfers): preserve from_account_id on error, match fees/exchange rate in idempotency check

coderabbitai review on #3342:
- All three create rescue blocks (exchange rate unavailable, invalid date,
  stale idempotency key) failed to set @from_account_id, so the re-rendered
  form lost the user's selected source account.
- matches_request? only compared accounts/date/outflow amount, so a retry
  with the same key but a different exchange_rate or fee would be reported
  as success while silently keeping the old inflow amount and fee entries.
  Now recomputes the request's effective inflow amount and compares derived
  fee totals too; a mismatch raises StaleIdempotencyKeyError like other
  stale-key mismatches instead of silently returning the old transfer.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

---------

Signed-off-by: Juan José Mata <juanjo.mata@gmail.com>
Co-authored-by: Gerald <248542187+gfr-free@users.noreply.github.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Juan José Mata <juanjo.mata@gmail.com>
2026-09-04 06:25:34 +02:00

326 lines
12 KiB
Ruby

class TransfersController < ApplicationController
include StreamExtensions
before_action :set_transfer, only: %i[show destroy update update_tags mark_as_recurring]
before_action :set_accounts, only: %i[new create]
helper_method :new_transfer_idempotency_key
def new
@transfer = Transfer.new
@from_account_id = params[:from_account_id]
@tags = Current.family.tags.alphabetically
end
def show
@categories = Current.family.categories.alphabetically_by_hierarchy
@tags = Current.family.tags.alphabetically
# Whether the current user can hit `mark_as_recurring`: feature flag on,
# AND they have write access to BOTH transfer endpoints. Gating the
# view button on this avoids showing a CTA that the controller would
# reject via `require_account_permission!` for read-only sharers.
endpoint_ids = [ @transfer.from_account&.id, @transfer.to_account&.id ].compact
writable_endpoint_count = Account.writable_by(Current.user).where(id: endpoint_ids).distinct.count
@can_mark_as_recurring_transfer =
!Current.family.recurring_transactions_disabled? &&
endpoint_ids.size == 2 &&
writable_endpoint_count == 2
end
def create
# Validate user has write access to both accounts
source_account = accessible_accounts.find(transfer_params[:from_account_id])
destination_account = accessible_accounts.find(transfer_params[:to_account_id])
return unless require_account_permission!(source_account, redirect_path: transactions_path)
return unless require_account_permission!(destination_account, redirect_path: transactions_path)
@transfer = Transfer::Creator.new(
family: Current.family,
source_account_id: source_account.id,
destination_account_id: destination_account.id,
date: transfer_params[:date].present? ? Date.parse(transfer_params[:date]) : Date.current,
amount: transfer_params[:amount].to_d,
exchange_rate: transfer_params[:exchange_rate].presence&.to_d,
source_fee_amount: transfer_params[:source_fee_amount],
destination_fee_amount: transfer_params[:destination_fee_amount],
tag_ids: transfer_params[:tag_ids],
idempotency_key: submitted_idempotency_key
).create
if @transfer.persisted?
success_message = "Transfer created"
respond_to do |format|
format.html { redirect_back_or_to transactions_path, notice: success_message }
format.turbo_stream { stream_redirect_back_or_to transactions_path, notice: success_message }
end
else
@from_account_id = transfer_params[:from_account_id]
@tags = Current.family.tags.alphabetically
render :new, status: :unprocessable_entity
end
rescue Money::ConversionError
@transfer ||= Transfer.new
@transfer.tag_ids = transfer_params[:tag_ids]
@transfer.errors.add(:base, t(".exchange_rate_unavailable"))
@from_account_id = transfer_params[:from_account_id]
set_accounts
@tags = Current.family.tags.alphabetically
render :new, status: :unprocessable_entity
rescue ArgumentError
@transfer ||= Transfer.new
@transfer.tag_ids = transfer_params[:tag_ids]
@transfer.errors.add(:date, t(".date_invalid"))
@from_account_id = transfer_params[:from_account_id]
set_accounts
@tags = Current.family.tags.alphabetically
render :new, status: :unprocessable_entity
rescue Transfer::Creator::StaleIdempotencyKeyError
@transfer ||= Transfer.new
@transfer.tag_ids = transfer_params[:tag_ids]
@transfer.errors.add(:base, t(".stale_form"))
@from_account_id = transfer_params[:from_account_id]
set_accounts
@tags = Current.family.tags.alphabetically
render :new, status: :unprocessable_entity
end
def update
outflow_account = @transfer.outflow_transaction.entry.account
return unless require_account_permission!(outflow_account, redirect_path: transactions_url)
Transfer.transaction do
update_transfer_status
update_transfer_fees_and_amount
update_transfer_details unless transfer_update_params[:status] == "rejected"
end
respond_to do |format|
format.html { redirect_back_or_to transactions_url, notice: t(".success") }
format.turbo_stream
end
end
def update_tags
outflow_account = @transfer.outflow_transaction.entry.account
inflow_account = @transfer.inflow_transaction.entry.account
return unless require_account_permission!(outflow_account, :annotate, redirect_path: transactions_url)
return unless require_account_permission!(inflow_account, :annotate, redirect_path: transactions_url)
resolved_ids = Current.family.tags.where(id: Array(params[:tag_ids]).reject(&:blank?)).pluck(:id)
Transfer.transaction do
[ @transfer.outflow_transaction, @transfer.inflow_transaction ].each do |transaction|
transaction.tag_ids = resolved_ids
transaction.lock_attr!(:tag_ids)
end
end
render json: { tag_ids: @transfer.outflow_transaction.reload.tag_ids }
end
def destroy
outflow_account = @transfer.outflow_transaction.entry.account
return unless require_account_permission!(outflow_account, redirect_path: transactions_url)
@transfer.destroy!
redirect_back_or_to transactions_url, notice: t(".success")
end
def mark_as_recurring
if Current.family.recurring_transactions_disabled?
flash[:alert] = t("recurring_transactions.transfer_feature_disabled")
redirect_back_or_to transactions_path
return
end
source_account = @transfer.from_account
destination_account = @transfer.to_account
if source_account.nil? || destination_account.nil?
flash[:alert] = t("recurring_transactions.unexpected_error")
redirect_back_or_to transactions_path
return
end
return unless require_account_permission!(source_account)
return unless require_account_permission!(destination_account)
existing = Current.family.recurring_transactions.find_by(
account_id: source_account.id,
destination_account_id: destination_account.id,
amount: @transfer.outflow_transaction.entry.amount,
currency: @transfer.outflow_transaction.entry.currency
)
if existing
flash[:alert] = t("recurring_transactions.transfer_already_exists")
respond_to do |format|
format.html { redirect_back_or_to transactions_path }
end
return
end
begin
RecurringTransaction.create_from_transfer(@transfer)
flash[:notice] = t("recurring_transactions.transfer_marked_as_recurring")
respond_to do |format|
format.html { redirect_back_or_to transactions_path }
end
rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotUnique
# RecordNotUnique covers the race window between `find_by` and `create!`
# (the partial unique index protects us at the DB level).
flash[:alert] = t("recurring_transactions.transfer_creation_failed")
respond_to do |format|
format.html { redirect_back_or_to transactions_path }
end
rescue StandardError => e
Rails.logger.error(
"transfers#mark_as_recurring failed: #{e.class} #{e.message} " \
"(transfer=#{@transfer&.id} family=#{Current.family&.id} user=#{Current.user&.id})"
)
flash[:alert] = t("recurring_transactions.unexpected_error")
respond_to do |format|
format.html { redirect_back_or_to transactions_path }
end
end
end
private
def set_transfer
# Finds the transfer and ensures the user has access to it
accessible_transaction_ids = Current.family.transactions
.joins(entry: :account)
.merge(Account.accessible_by(Current.user))
.select(:id)
@transfer = Transfer
.where(id: params[:id])
.where(inflow_transaction_id: accessible_transaction_ids)
.first!
end
def transfer_params
params.require(:transfer).permit(:from_account_id, :to_account_id, :amount, :date, :name, :excluded, :exchange_rate, :source_fee_amount, :destination_fee_amount, tag_ids: [])
end
# Anti-double-submit token: a random UUID rendered fresh on every "new
# transfer" form load, echoed back on submit, only ever trusted to look
# like something we could have generated (see
# Transfer::Creator#find_existing_transfer for how it's used to
# de-duplicate).
UUID_FORMAT = /\A[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\z/i
private_constant :UUID_FORMAT
def submitted_idempotency_key
key = params.dig(:transfer, :idempotency_key)
key if key.is_a?(String) && key.match?(UUID_FORMAT)
end
def new_transfer_idempotency_key
@new_transfer_idempotency_key ||= submitted_idempotency_key || SecureRandom.uuid
end
def set_accounts
@accounts = accessible_accounts
.alphabetically
.includes(
:account_providers,
logo_attachment: :blob
)
end
def transfer_update_params
params.require(:transfer).permit(:notes, :status, :category_id, :amount, :source_fee_amount, :destination_fee_amount)
end
def update_transfer_status
if transfer_update_params[:status] == "rejected"
@transfer.reject!
elsif transfer_update_params[:status] == "confirmed"
@transfer.confirm!
end
end
def update_transfer_details
@transfer.outflow_transaction.update!(category_id: transfer_update_params[:category_id])
@transfer.update!(notes: transfer_update_params[:notes])
end
def update_transfer_fees_and_amount
new_amount = transfer_update_params[:amount]
new_source_fee = transfer_update_params[:source_fee_amount]
new_destination_fee = transfer_update_params[:destination_fee_amount]
current_source_fee = @transfer.derived_source_fee_amount
current_destination_fee = @transfer.derived_destination_fee_amount
source_fee_changed = new_source_fee.present? && new_source_fee.to_d != current_source_fee
dest_fee_changed = new_destination_fee.present? && new_destination_fee.to_d != current_destination_fee
amount_changed = new_amount.present? && new_amount.to_d != @transfer.amount.to_d
return unless amount_changed || source_fee_changed || dest_fee_changed
@transfer.amount = new_amount.to_d if amount_changed
if amount_changed
outflow_entry = @transfer.outflow_transaction.entry
outflow_entry.amount = @transfer.amount
outflow_entry.save!
inflow_entry = @transfer.inflow_transaction.entry
converted = Money.new(@transfer.amount, @transfer.from_account.currency)
.exchange_to(@transfer.to_account.currency, date: @transfer.date)
inflow_entry.amount = -(converted.amount)
inflow_entry.save!
end
if source_fee_changed
update_fee_transaction(
account: @transfer.from_account,
old_fee: current_source_fee,
new_fee: new_source_fee.to_d,
name: "Transfer fee — #{@transfer.name}"
)
end
if dest_fee_changed
update_fee_transaction(
account: @transfer.to_account,
old_fee: current_destination_fee,
new_fee: new_destination_fee.to_d,
name: "Transfer fee — #{@transfer.name}"
)
end
@transfer.save!
end
def update_fee_transaction(account:, old_fee:, new_fee:, name:)
if old_fee > 0 && new_fee > 0
fee_tx = @transfer.fee_transactions.find { |t| t.entry.account_id == account.id }
if fee_tx
fee_tx.entry.update!(amount: new_fee)
end
elsif old_fee > 0 && new_fee == 0
fee_tx = @transfer.fee_transactions.find { |t| t.entry.account_id == account.id }
fee_tx&.destroy!
elsif old_fee == 0 && new_fee > 0
fee_category = account.family.categories.find_or_create_by!(name: I18n.t("models.category.defaults.fees"))
fee_tx = Transaction.new(
kind: "standard",
category: fee_category,
entry: account.entries.build(
amount: new_fee,
currency: account.currency,
date: @transfer.date,
name: name,
)
)
fee_tx.save!
@transfer.fee_transactions << fee_tx
end
end
end