Fourth fresh-eyes review pass (5 agents: goal, QA, code-quality, security, context-mining). All PASS on the money-safety core; this round closes the remaining MAJOR/MINOR items they surfaced. Webhooks: - Untracked disputes now raise ONE admin notification PER distinct chargeback: the notification id is derived deterministically from the square_dispute_id (SHA-256 truncated into the CHAR(12) slot) so a second untracked dispute is no longer silently suppressed by the first's dedup row. ON CONFLICT (id) keeps same-dispute replays idempotent; the booking-scoped NOT EXISTS guard is retained for the tracked path. Verified: distinct disputes -> distinct rows; re-delivered dispute -> one row. - The gift-card clawback SQL now lives in exactly ONE place: payments.RevertGiftCardFunding (new giftcard_clawback.go). till.go and the webhook path both call it — eliminating the byte-for-byte copy whose divergence would be a money-loss drift trap (the same two-sources-of-truth pattern this commit eliminated for GDPR scrubbing). 2FA: - Applied the lockout-coherence fix from the review: when a disable request must mint a fresh code (no valid pending one), the held attempt counter is reset so the locked-out user can use the freshly delivered code in the SAME request (no wasted round-trip). The reuse path keeps accumulating wrong attempts toward the 5-attempt lockout — the two behaviors no longer conflict. (The 'always-fresh on disable' suggestion was NOT adopted: it would break the out-of-band [2FA]-log delivery model, since a code generated by a request can never be submitted within that same request.) - New test pins the shared verify/disable lockout: 5 wrong verifies 429 and destroy the code; a stale code then 400s on disable while the freshly delivered code succeeds in the same request. - Startup now warns that 2FA codes travel in PLAINTEXT via the server log in enforced mode (operator must restrict log access + relay out-of-band until email/SMS lands). Docs: - Test counts updated to the current 2,154 across README + Technical Manual. - User Manual 2FA nav corrected: the settings live on the Account page, not an 'Admin' area. Tests: 2,154 (up from 2,151). Backend 25/26 packages green (crussell/db fails only in this environment: local postgres auth for the test role; package byte-identical to HEAD). Frontend builds; svelte-check 0 errors.
129 lines
5.8 KiB
Go
129 lines
5.8 KiB
Go
package payments
|
|
|
|
import (
|
|
"context"
|
|
"errors"
|
|
"fmt"
|
|
"log"
|
|
"log/slog"
|
|
|
|
"crussell/db"
|
|
|
|
"github.com/jackc/pgx/v5"
|
|
)
|
|
|
|
// RevertGiftCardFunding undoes the gift-card funding performed earlier in the
|
|
// SAME till-sale request after a definitive Square charge rejection, matching
|
|
// the gift_card_transactions accounting: a created card is deleted (with its
|
|
// purchase transaction) and any immediate redeem-to-account credit reversed; a
|
|
// topped-up card has the amount subtracted back out and its top-up transaction
|
|
// removed. The clawback is claim-first: it atomically claims the till sale
|
|
// with a gating `status='pending'` UPDATE whose row lock serializes against
|
|
// the handler's completion UPDATE, then runs the card mutation + failed-mark
|
|
// in the same transaction so a late same-key retry cannot re-complete a sale
|
|
// whose gift card no longer exists.
|
|
//
|
|
// This is the SINGLE money-reversal implementation shared by the till handler
|
|
// (revertGiftCardFunding), the stale-pending sweep (sweep.go) and the Square
|
|
// webhook clawback (handlers/webhooks/square.go). Do not fork it: a divergent
|
|
// clawback is real money loss.
|
|
func RevertGiftCardFunding(ctx context.Context, action, giftCardID string, amount float64, redeemToUserID *string, tillSaleID string) error {
|
|
tx, err := db.Conn.Begin(ctx)
|
|
if err != nil {
|
|
return fmt.Errorf("failed to begin clawback transaction: %w", err)
|
|
}
|
|
defer func() {
|
|
if err := tx.Rollback(ctx); err != nil && !errors.Is(err, pgx.ErrTxClosed) {
|
|
slog.Error("failed to rollback gift-card clawback transaction", "err", err)
|
|
}
|
|
}()
|
|
|
|
// Claim the sale first: the row lock serializes against the handler's
|
|
// completion UPDATE; a zero-row claim means the funding is not ours.
|
|
tag, err := tx.Exec(ctx, `
|
|
UPDATE till_sales SET status = 'failed', updated_at = NOW()
|
|
WHERE id = $1 AND status = 'pending'`, tillSaleID)
|
|
if err != nil {
|
|
return fmt.Errorf("failed to claim till sale for clawback: %w", err)
|
|
}
|
|
if tag.RowsAffected() == 0 {
|
|
return errTillSaleNotPending
|
|
}
|
|
|
|
if action == "create" {
|
|
// A newly created card's transactions are scoped to THIS sale's
|
|
// funding (reference_type='till_sale' AND reference_id=sale id) — never
|
|
// a wholesale delete, which would destroy the value of a different
|
|
// idempotency-keyed top-up sale that funded the same card before this
|
|
// create resolved. Then remove the card itself.
|
|
if _, err := tx.Exec(ctx, `DELETE FROM gift_card_transactions WHERE gift_card_id = $1 AND reference_type = 'till_sale' AND reference_id = $2`, giftCardID, tillSaleID); err != nil {
|
|
return fmt.Errorf("failed to delete gift card transaction: %w", err)
|
|
}
|
|
if _, err := tx.Exec(ctx, `DELETE FROM gift_cards WHERE id = $1`, giftCardID); err != nil {
|
|
return fmt.Errorf("failed to delete gift card: %w", err)
|
|
}
|
|
// If the card was immediately redeemed to a user balance in this
|
|
// request, reverse that credit (guarded so it can never go negative).
|
|
if redeemToUserID != nil && *redeemToUserID != "" {
|
|
bTag, bErr := tx.Exec(ctx, `
|
|
UPDATE user_giftcard_balances
|
|
SET balance = user_giftcard_balances.balance - $1, updated_at = NOW()
|
|
WHERE user_id = $2 AND balance >= $1
|
|
`, amount, *redeemToUserID)
|
|
if bErr != nil {
|
|
return fmt.Errorf("failed to reverse redeemed gift card balance: %w", bErr)
|
|
}
|
|
if bTag.RowsAffected() == 0 {
|
|
// The guard blocked the reversal because some of the credited
|
|
// balance was already spent. The sale is still marked failed
|
|
// below — do not fail the whole clawback tx — but the
|
|
// un-reversed credit must be flagged for manual reconciliation
|
|
// (mirrors the top-up branch).
|
|
log.Printf("CRITICAL: ... MANUAL RECONCILIATION REQUIRED: create-with-redeem clawback for gift card %s could not fully reverse the £%.2f balance credited to user %s (balance < amount)", giftCardID, amount, *redeemToUserID)
|
|
}
|
|
}
|
|
} else {
|
|
// Top-up: subtract the amount back out of the card. The guard keeps
|
|
// amount_remaining from ever going negative in the pathological case
|
|
// where some of the top-up was already spent before the charge failed.
|
|
tag, err := tx.Exec(ctx, `
|
|
UPDATE gift_cards
|
|
SET total_funds_added = total_funds_added - $1,
|
|
amount_remaining = amount_remaining - $1
|
|
WHERE id = $2 AND amount_remaining >= $1
|
|
`, amount, giftCardID)
|
|
if err != nil {
|
|
return fmt.Errorf("failed to reverse gift card top-up: %w", err)
|
|
}
|
|
if tag.RowsAffected() == 0 {
|
|
// The guard blocked the reversal because some of the top-up was
|
|
// already spent. The sale is still marked failed below — do not
|
|
// fail the whole clawback tx — but the unreversed money must be
|
|
// flagged for manual reconciliation.
|
|
log.Printf("CRITICAL: ... MANUAL RECONCILIATION REQUIRED: top-up %v on gift card %s could not be fully reversed (amount_remaining < top-up)", amount, giftCardID)
|
|
}
|
|
// Remove only this request's top-up transaction (reference_id = till
|
|
// sale) so prior sales' accounting on the same card is untouched.
|
|
if _, err := tx.Exec(ctx, `
|
|
DELETE FROM gift_card_transactions
|
|
WHERE gift_card_id = $1 AND reference_type = 'till_sale' AND reference_id = $2
|
|
`, giftCardID, tillSaleID); err != nil {
|
|
return fmt.Errorf("failed to delete gift card top-up transaction: %w", err)
|
|
}
|
|
}
|
|
|
|
if err := tx.Commit(ctx); err != nil {
|
|
return fmt.Errorf("failed to commit clawback transaction: %w", err)
|
|
}
|
|
return nil
|
|
}
|
|
|
|
// IsTillSaleNotPending reports whether err is the claim-first sentinel
|
|
// (errTillSaleNotPending): the gating `status='pending'` UPDATE matched zero
|
|
// rows, so the till sale is no longer pending and its gift card must be left
|
|
// untouched. Exported so cross-package clawback callers (the Square webhook)
|
|
// can detect the sentinel without reaching into the unexported error value.
|
|
func IsTillSaleNotPending(err error) bool {
|
|
return errors.Is(err, errTillSaleNotPending)
|
|
}
|