From 738f6b6a511c0a1a0bb57433c19ed0291308ccc8 Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Mon, 3 Aug 2026 14:57:04 +0100 Subject: [PATCH] Expand payment test coverage: lock contention, nonce-direct, provisional rows, GDPR scrub, validators Close the coverage-gap round: terminal CreateCheckout-failure marks the provisional row failed, GetCheckoutStatus reference_id mismatch 400, deadline wire shape, concurrent loyalty redemption 409, delete_guest_user + stale-guest saved-card scrubbing, ValidateAmount and isTokenLike direct units, bounded try-lock timeout, buildSplitRecords tip-overflow, and concurrent same-key dedup for gift card / booking / tip / checkout. --- backend/handlers/payments/concurrency_test.go | 96 +++++ .../handlers/payments/payment_status_test.go | 59 +++ .../handlers/payments/payments_fixes_test.go | 62 +++- .../payments/payments_r6_review_test.go | 347 ++++++++++++++++++ .../payments/payments_review_fixes_test.go | 38 +- backend/handlers/payments/payments_test.go | 91 ++++- backend/handlers/payments/validators_test.go | 35 ++ 7 files changed, 710 insertions(+), 18 deletions(-) create mode 100644 backend/handlers/payments/payments_r6_review_test.go create mode 100644 backend/handlers/payments/validators_test.go diff --git a/backend/handlers/payments/concurrency_test.go b/backend/handlers/payments/concurrency_test.go index ca21403..0abfeeb 100644 --- a/backend/handlers/payments/concurrency_test.go +++ b/backend/handlers/payments/concurrency_test.go @@ -423,3 +423,99 @@ func TestGetCheckoutStatus_ConcurrentPolls_SingleRecord(t *testing.T) { t.Errorf("expected exactly 1 terminal payment record, got %d (double-record race!)", payCount) } } + +// TestLoyaltyRedemption_ConcurrentSameBooking_SingleApply proves the loyalty +// redemption advisory lock (loyalty.go): two goroutines redeeming the same +// booking must apply the 10% discount exactly once. One request succeeds (200) +// and the other is rejected by the in-lock re-check ("A loyalty discount has +// already been applied", 400) — never two discount payments for one booking. +func TestLoyaltyRedemption_ConcurrentSameBooking_SingleApply(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + + userID, bookingID, _ := setupLoyaltyUser(t, ctx, tx, 10) + token := jwt.GenerateUserToken(userID) + cleanupConcurrentTestRows(t, context.Background(), userID, bookingID) + + // Commit the setup so both goroutines operate at pool level — the advisory + // lock only serializes across independent connections. + innerTx := db.TxFromContext(ctx) + if innerTx == nil { + t.Fatal("no transaction in context") + } + if err := innerTx.Commit(ctx); err != nil { + t.Fatalf("failed to commit setup tx: %v", err) + } + + pool := context.Background() + + var wg sync.WaitGroup + startBoth := make(chan struct{}) + recs := make([]*httptest.ResponseRecorder, 2) + + for i := 0; i < 2; i++ { + wg.Add(1) + go func(idx int) { + defer wg.Done() + <-startBoth + recs[idx] = makeApplyRedemptionRequest(bookingID, token, pool) + }(i) + } + close(startBoth) + wg.Wait() + + // Exactly one request wins the redemption; the other finds the already + // applied discount inside the lock and is rejected with 400 (the first + // handler completes in milliseconds, well under the ~3s lock bound, so the + // 409 lock-timeout path is not exercised). + okCount, rejectedCount := 0, 0 + for i, rec := range recs { + switch { + case rec.Code == http.StatusOK: + okCount++ + case rec.Code == http.StatusBadRequest || rec.Code == http.StatusConflict: + rejectedCount++ + default: + t.Errorf("request %d unexpected status %d: %s", i, rec.Code, rec.Body.String()) + } + } + if okCount != 1 { + t.Errorf("expected exactly 1 successful redemption, got %d", okCount) + } + if rejectedCount != 1 { + t.Errorf("expected exactly 1 rejected redemption, got %d", rejectedCount) + } + + // Exactly one loyalty discount and one discount payment row. + var discountCount int + if err := db.Conn.QueryRow(pool, + `SELECT COUNT(*) FROM booking_discounts WHERE booking_id = $1 AND discount_source = 'loyalty'`, bookingID).Scan(&discountCount); err != nil { + t.Fatalf("failed to count loyalty discounts: %v", err) + } + if discountCount != 1 { + t.Errorf("expected exactly 1 loyalty discount, got %d (double-apply!)", discountCount) + } + var payCount int + if err := db.Conn.QueryRow(pool, + `SELECT COUNT(*) FROM payments WHERE booking_id = $1 AND payment_method = 'discount'`, bookingID).Scan(&payCount); err != nil { + t.Fatalf("failed to count discount payments: %v", err) + } + if payCount != 1 { + t.Errorf("expected exactly 1 discount payment, got %d", payCount) + } + + // The redemption is applied exactly once. + var appliedCount int + if err := db.Conn.QueryRow(pool, + `SELECT COUNT(*) FROM loyalty_redemptions WHERE user_id = $1 AND status = 'applied'`, userID).Scan(&appliedCount); err != nil { + t.Fatalf("failed to count applied redemptions: %v", err) + } + if appliedCount != 1 { + t.Errorf("expected exactly 1 applied redemption, got %d", appliedCount) + } + + // Clean up the committed loyalty_redemptions row (cleanupConcurrentTestRows + // does not cover it; user_id is SET NULL by the FK so it would otherwise leak). + t.Cleanup(func() { + _, _ = db.Conn.Exec(pool, `DELETE FROM loyalty_redemptions WHERE user_id = $1 OR applied_to_booking_id = $2`, userID, bookingID) + }) +} diff --git a/backend/handlers/payments/payment_status_test.go b/backend/handlers/payments/payment_status_test.go index 03014d0..a103eb4 100644 --- a/backend/handlers/payments/payment_status_test.go +++ b/backend/handlers/payments/payment_status_test.go @@ -809,3 +809,62 @@ func TestGetCheckoutStatus_TwoEqualAmountCharges_NoCollision(t *testing.T) { t.Errorf("expected exactly 2 payment rows for two equal-amount charges, got %d", rowCount) } } + +// mismatchedRefCheckoutClient forces GetCheckout to return a fixed COMPLETED +// payment whose reference_id points at a DIFFERENT booking, deterministically +// exercising GetCheckoutStatus's ownership check. +type mismatchedRefCheckoutClient struct { + square.SquareClient + result *square.PaymentResult +} + +func (c *mismatchedRefCheckoutClient) GetCheckout(ctx context.Context, checkoutID string) (*square.PaymentResult, error) { + return c.result, nil +} + +func TestGetCheckoutStatus_ReferenceIDMismatch_Returns400(t *testing.T) { + // The terminal checkout's reference_id must match the booking being + // polled; a checkout that references a different booking is refused with + // 400 so its payment can never be recorded against the wrong booking. + ctx, tx := testutils.SetupTestTx(t) + _, bookingID, _ := setupTestData(t, ctx, tx) + + origClient := SquareClient + SquareClient = &mismatchedRefCheckoutClient{ + SquareClient: square.NewDevClient(), + result: &square.PaymentResult{ + ID: "pay_mismatch", + Status: "COMPLETED", + Amount: 5000, + SquarePayID: "pay_mismatch", + ReferenceID: "00000000dead", // a DIFFERENT booking + CreatedAt: "2026-07-31T00:00:00Z", + UpdatedAt: "2026-07-31T00:00:00Z", + }, + } + defer func() { SquareClient = origClient }() + + checkoutID := "abcd1234ef56" // 12 hex chars, passes the checkout-id validation + req := httptest.NewRequest("GET", "/api/admin/payments/"+checkoutID+"/status?booking_id="+bookingID, nil) + rctx := chi.NewRouteContext() + rctx.URLParams.Add("checkout_id", checkoutID) + reqCtx := context.WithValue(req.Context(), chi.RouteCtxKey, rctx) + reqCtx = context.WithValue(reqCtx, mw.UserIDKey, "000000000001") + reqCtx = context.WithValue(reqCtx, mw.UserRoleKey, "admin") + req = req.WithContext(reqCtx) + + w := httptest.NewRecorder() + GetCheckoutStatus(w, req) + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400 for reference_id mismatch, got %d: %s", w.Code, w.Body.String()) + } + + // No payment may be recorded for the mismatched checkout. + var rowCount int + if err := tx.QueryRow(ctx, "SELECT COUNT(*) FROM payments WHERE booking_id = $1", bookingID).Scan(&rowCount); err != nil { + t.Fatalf("failed to count payment rows: %v", err) + } + if rowCount != 0 { + t.Errorf("expected no payment rows after reference mismatch, got %d", rowCount) + } +} diff --git a/backend/handlers/payments/payments_fixes_test.go b/backend/handlers/payments/payments_fixes_test.go index 06f3395..532da97 100644 --- a/backend/handlers/payments/payments_fixes_test.go +++ b/backend/handlers/payments/payments_fixes_test.go @@ -898,9 +898,12 @@ func TestCreateTerminalPayment_RecordInsertFailure_CancelsOrphanedCheckout(t *te _, bookingID, _ := setupTestData(t, ctx, tx) adminToken := jwt.GenerateAdminToken() - // A COMPLETED terminal_checkouts row with the same checkout_id the client - // will return forces the handler's INSERT to collide on the PK while the - // active-checkout guard (PENDING/IN_PROGRESS only) does not fire. + // R3: the handler now inserts the tracked row FIRST with a provisional + // (tmp-) checkout_id, then CreateCheckout, then UPDATEs the row to the real + // Square id. A COMPLETED terminal_checkouts row with the same checkout_id + // the client will return makes that tracking UPDATE collide on the PK while + // the active-checkout guard (PENDING/IN_PROGRESS only) does not fire — the + // checkout is live at Square but untracked, so the handler must cancel it. const dupCheckoutID = "chk_dup_insert_01" if _, err := tx.Exec(ctx, ` INSERT INTO terminal_checkouts (checkout_id, booking_id, payment_type, status, amount) @@ -926,3 +929,56 @@ func TestCreateTerminalPayment_RecordInsertFailure_CancelsOrphanedCheckout(t *te t.Errorf("expected exactly one CancelCheckout for the orphaned checkout %q, got %v", dupCheckoutID, calls) } } + +func TestCreateTerminalPayment_CreateCheckoutFailure_MarksProvisionalRowFailed(t *testing.T) { + // R3: the handler inserts the tracked row FIRST with a provisional + // ("tmp-") checkout_id, THEN calls Square CreateCheckout. When Square fails + // AFTER that insert, the handler must mark the provisional row failed so a + // retry can proceed — otherwise the "tmp-" row (provably pre-Square) wedges + // the booking's in-flight guard forever. + ctx, tx := testutils.SetupTestTx(t) + _, bookingID, _ := setupTestData(t, ctx, tx) + adminToken := jwt.GenerateAdminToken() + + origClient := SquareClient + mc := square.NewDevClient().(*square.MockClient) + mc.FailCreateCheckout = true + SquareClient = mc + defer func() { SquareClient = origClient }() + + handler := CreateTerminalPayment + req := CreateTerminalPaymentRequest{Amount: 5000, PaymentType: "full"} + w := makePaymentRequest(handler, "POST", "/api/admin/bookings/"+bookingID+"/payment", req, adminToken, ctx) + if w.Code != http.StatusInternalServerError { + t.Fatalf("expected 500 when Square CreateCheckout fails, got %d: %s", w.Code, w.Body.String()) + } + + // The provisional row must have been marked failed, and its checkout_id + // must still be the provisional "tmp-" value (Square never returned a real + // id, so nothing may overwrite it). + var status, checkoutID string + err := tx.QueryRow(ctx, ` + SELECT status, checkout_id FROM terminal_checkouts WHERE booking_id = $1 + `, bookingID).Scan(&status, &checkoutID) + if err != nil { + t.Fatalf("failed to query terminal checkout row: %v", err) + } + if status != "failed" { + t.Errorf("expected provisional terminal_checkouts row status 'failed', got %q", status) + } + if !strings.HasPrefix(checkoutID, "tmp-") { + t.Errorf("expected checkout_id to remain provisional (tmp- prefix), got %q", checkoutID) + } + + // A retry after the failure must be able to create a fresh checkout: the + // in-flight guard must not see the failed row as active. + var activeCount int + if err := tx.QueryRow(ctx, ` + SELECT COUNT(*) FROM terminal_checkouts WHERE booking_id = $1 AND status IN ('PENDING', 'IN_PROGRESS') + `, bookingID).Scan(&activeCount); err != nil { + t.Fatalf("failed to count active terminal checkouts: %v", err) + } + if activeCount != 0 { + t.Errorf("expected 0 active terminal checkouts after the failure, got %d", activeCount) + } +} diff --git a/backend/handlers/payments/payments_r6_review_test.go b/backend/handlers/payments/payments_r6_review_test.go new file mode 100644 index 0000000..aba6812 --- /dev/null +++ b/backend/handlers/payments/payments_r6_review_test.go @@ -0,0 +1,347 @@ +//go:build test && dev + +package payments + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "sync" + "testing" + + "crussell/db" + "crussell/internal/square" + "crussell/testutils" + "crussell/testutils/fixtures" + "crussell/testutils/jwt" + + "github.com/go-chi/chi/v5" + "github.com/stretchr/testify/require" +) + +// ============================================================================= +// R6: ccof charges must ALWAYS carry a CustomerID; one-off non-save charges +// use the cnon: nonce directly (no card-on-file, no customer). +// ============================================================================= + +// TestCreateBookingPayment_SavedCard_LegacyNoCustomer_ProvisionsAndCharges +// covers the lazy Square-customer provisioning for saved-card rows that +// predate P14 (square_customer_id empty): the row must be provisioned AND the +// provisioned id persisted on the row BEFORE the ccof: charge goes out. +func TestCreateBookingPayment_SavedCard_LegacyNoCustomer_ProvisionsAndCharges(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, userToken := setupPaymentStatusTest(t, ctx, tx, "confirmed") + + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:sq_legacy", "VISA", "4242") + require.NoError(t, err) + // Row is seeded with square_customer_id empty (legacy). + + origClient := SquareClient + rec := &recordingPaymentClient{SquareClient: square.NewDevClient()} + SquareClient = rec + defer func() { SquareClient = origClient }() + + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + CardID: &cardID, + IdempotencyKey: "legacy-cust-" + bookingID, + } + w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + rec.mu.Lock() + got := rec.lastReq.CustomerID + rec.mu.Unlock() + require.NotEmpty(t, got, "a legacy ccof: charge must be provisioned a Square customer before charging") + + var persisted string + require.NoError(t, tx.QueryRow(ctx, `SELECT COALESCE(square_customer_id, '') FROM user_saved_cards WHERE id = $1`, cardID).Scan(&persisted)) + require.Equal(t, got, persisted, "the provisioned customer id must be persisted on the saved-card row") +} + +// TestCreateBookingPayment_SaveCard_ChargeForwardsCustomerID covers the save +// path of the new-card flow: the ccof: charge MUST carry the same provisioned +// customer id that CreateCardOnFile used. +func TestCreateBookingPayment_SaveCard_ChargeForwardsCustomerID(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + _, bookingID, userToken := setupPaymentStatusTest(t, ctx, tx, "confirmed") + + origClient := SquareClient + cof := &recordingCardOnFileClient{SquareClient: square.NewDevClient()} + rec := &recordingPaymentClient{SquareClient: cof} + SquareClient = rec + defer func() { SquareClient = origClient }() + + cardToken := "cnon:save-card-nonce" + req := CreateBookingPaymentRequest{ + Amount: 2500, + PaymentType: "deposit", + NewCardToken: &cardToken, + SaveCard: true, + IdempotencyKey: "save-charge-cust-" + bookingID, + } + w := makePaymentRequest(CreateBookingPayment, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + require.Equal(t, 1, cof.callCount(), "a save-card flow tokenizes via CreateCardOnFile exactly once") + require.NotEmpty(t, cof.lastCustomerID(), "a save-card flow must provision a Square customer") + + rec.mu.Lock() + got := rec.lastReq.CustomerID + rec.mu.Unlock() + require.Equal(t, cof.lastCustomerID(), got, "a save-card (ccof:) charge must carry the provisioned customer id") + require.NotEmpty(t, got) +} + +// TestBuyGiftCard_SaveCard_ChargeForwardsCustomerID is the BuyGiftCard +// counterpart of the above: the gift-card purchase's ccof: charge must carry +// the provisioned customer id. +func TestBuyGiftCard_SaveCard_ChargeForwardsCustomerID(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + token := jwt.GenerateUserToken(userID) + + origClient := SquareClient + cof := &recordingCardOnFileClient{SquareClient: square.NewDevClient()} + rec := &recordingPaymentClient{SquareClient: cof} + SquareClient = rec + defer func() { SquareClient = origClient }() + + newToken := "cnon:save-gift-card-nonce" + req := BuyGiftCardRequest{ + Amount: 1000, + RecipientType: "self", + NewCardToken: &newToken, + SaveCard: true, + IdempotencyKey: "buy-gc-save-charge-cust", + } + w := makePaymentRequest(BuyGiftCard, "POST", "/api/gift-cards/buy", req, token, ctx) + require.Equal(t, http.StatusCreated, w.Code, w.Body.String()) + + require.Equal(t, 1, cof.callCount(), "a save-card flow tokenizes via CreateCardOnFile exactly once") + rec.mu.Lock() + got := rec.lastReq.CustomerID + rec.mu.Unlock() + require.Equal(t, cof.lastCustomerID(), got, "a save-card (ccof:) gift-card purchase must carry the provisioned customer id") + require.NotEmpty(t, got) +} + +// ============================================================================= +// R2: BuyGiftCard requires an idempotency key +// ============================================================================= + +func TestBuyGiftCard_MissingIdempotencyKey_Rejected(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + token := jwt.GenerateUserToken(userID) + + newToken := "cnon:test-card" + req := BuyGiftCardRequest{ + Amount: 1000, + RecipientType: "self", + NewCardToken: &newToken, + SaveCard: false, + } + w := makePaymentRequest(BuyGiftCard, "POST", "/api/gift-cards/buy", req, token, ctx) + require.Equal(t, http.StatusBadRequest, w.Code, "an empty idempotency key must be rejected (R2)") +} + +// ============================================================================= +// R3: provisional (pre-Square) terminal checkout rows +// ============================================================================= + +// TestActiveTerminalCheckoutID_ResolvesProvisionalRow covers the crash-window +// guard: a PENDING row with a synthetic "tmp-" checkout_id never had a checkout +// created at Square, so it is provably not live — the guard must mark it failed +// and allow a fresh checkout instead of wedging the booking. +func TestActiveTerminalCheckoutID_ResolvesProvisionalRow(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + _, bookingID, _ := setupTestData(t, ctx, tx) + + provisionalID := "tmp-crash-window-1" + if _, err := tx.Exec(ctx, ` + INSERT INTO terminal_checkouts (checkout_id, booking_id, payment_type, status, amount) + VALUES ($1, $2, 'full', 'PENDING', 50.00) + `, provisionalID, bookingID); err != nil { + t.Fatalf("failed to seed provisional terminal checkout: %v", err) + } + + got := activeTerminalCheckoutID(ctx, bookingID) + require.Equal(t, "", got, "a provisional (pre-Square) row must not block a new checkout") + + var status string + require.NoError(t, tx.QueryRow(ctx, `SELECT status FROM terminal_checkouts WHERE checkout_id = $1`, provisionalID).Scan(&status)) + require.Equal(t, "failed", status, "the provisional row must be marked failed") +} + +// failingGetCheckoutClient errors on every GetCheckout — if the sweep wrongly +// resolves a provisional row against Square, the row is left pending (the error +// is not a terminal checkout state) and the test fails. +type failingGetCheckoutClient struct { + square.SquareClient +} + +func (c *failingGetCheckoutClient) GetCheckout(ctx context.Context, checkoutID string) (*square.PaymentResult, error) { + return nil, fmt.Errorf("GetCheckout was called for %s — provisional rows must resolve without a Square round-trip", checkoutID) +} + +// TestSweepStaleTerminalCheckouts_ResolvesProvisionalRowWithoutSquare proves +// the sweep resolves a stale provisional row to failed without calling Square. +func TestSweepStaleTerminalCheckouts_ResolvesProvisionalRowWithoutSquare(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, bookingID, serviceID := setupTestData(t, ctx, tx) + + provisionalID := "tmp-sweep-provisional-1" + if _, err := tx.Exec(ctx, ` + INSERT INTO terminal_checkouts (checkout_id, booking_id, payment_type, status, amount, created_at) + VALUES ($1, $2, 'full', 'PENDING', 50.00, NOW() - INTERVAL '2 hours') + `, provisionalID, bookingID); err != nil { + t.Fatalf("failed to seed provisional terminal checkout: %v", err) + } + + pgxTx := db.TxFromContext(ctx) + require.NotNil(t, pgxTx) + require.NoError(t, pgxTx.Commit(ctx)) + + t.Cleanup(func() { + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM terminal_checkouts WHERE checkout_id = $1`, provisionalID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM bookings WHERE id = $1`, bookingID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM services WHERE id = $1`, serviceID) + _, _ = db.Conn.Exec(context.Background(), `DELETE FROM users WHERE id = $1`, userID) + }) + + freshCtx := context.Background() + // Drop stale rows left by other sweep tests so the count is deterministic. + if _, err := db.Conn.Exec(freshCtx, `DELETE FROM terminal_checkouts WHERE status IN ('PENDING', 'IN_PROGRESS') AND checkout_id <> $1`, provisionalID); err != nil { + t.Fatalf("failed to clean leftover stale terminal checkouts: %v", err) + } + if _, err := db.Conn.Exec(freshCtx, `DELETE FROM till_sales WHERE status = 'pending' AND square_checkout_id IS NOT NULL`); err != nil { + t.Fatalf("failed to clean leftover stale till sales: %v", err) + } + + origClient := SquareClient + SquareClient = &failingGetCheckoutClient{SquareClient: square.NewDevClient()} + defer func() { SquareClient = origClient }() + + n, err := SweepStaleTerminalCheckouts(freshCtx) + require.NoError(t, err) + require.Equal(t, 1, n, "the stale provisional terminal checkout must be resolved by the sweep") + + var status string + require.NoError(t, db.Conn.QueryRow(freshCtx, `SELECT status FROM terminal_checkouts WHERE checkout_id = $1`, provisionalID).Scan(&status)) + require.Equal(t, "failed", status) +} + +// ============================================================================= +// R5: GetDiscountPreviewHandler ownership (IDOR) +// ============================================================================= + +// TestGetDiscountPreviewHandler_WrongOwner_Forbidden covers the IDOR fix: a +// non-admin user must not read another user's booking discount preview. +func TestGetDiscountPreviewHandler_WrongOwner_Forbidden(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + _, bookingID, _ := setupDiscountPreviewTest(t, ctx, tx) + + otherUserID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + otherToken := jwt.GenerateUserToken(otherUserID) + + w := serveDiscountPreviewHandler(bookingID, otherUserID, otherToken, ctx) + require.Equal(t, http.StatusForbidden, w.Code, "a non-owner must not read another user's discount preview") +} + +// TestGetDiscountPreviewHandler_NoUserContext_Unauthorized covers the fail-closed +// 401 for requests that carry no user id. +func TestGetDiscountPreviewHandler_NoUserContext_Unauthorized(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + _, bookingID, _ := setupDiscountPreviewTest(t, ctx, tx) + + req := httptest.NewRequest("GET", "/api/bookings/"+bookingID+"/discount-preview", nil) + rctx := chi.NewRouteContext() + rctx.URLParams.Add("id", bookingID) + req = req.WithContext(ctx) + req = req.WithContext(context.WithValue(req.Context(), chi.RouteCtxKey, rctx)) + + w := httptest.NewRecorder() + GetDiscountPreviewHandler(w, req) + require.Equal(t, http.StatusUnauthorized, w.Code) +} + +// ============================================================================= +// R8: DeletePaymentMethod disables the Square card before the local soft-delete +// ============================================================================= + +type deletingCardClient struct { + square.SquareClient + mu sync.Mutex + deleted []string +} + +func (c *deletingCardClient) DeleteCardOnFile(ctx context.Context, cardID string) error { + c.mu.Lock() + c.deleted = append(c.deleted, cardID) + c.mu.Unlock() + return c.SquareClient.DeleteCardOnFile(ctx, cardID) +} + +func (c *deletingCardClient) deletedIDs() []string { + c.mu.Lock() + defer c.mu.Unlock() + return append([]string(nil), c.deleted...) +} + +func TestDeletePaymentMethod_DisablesSquareCard(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:sq_del_disable", "VISA", "9999") + require.NoError(t, err) + + origClient := SquareClient + rec := &deletingCardClient{SquareClient: square.NewDevClient()} + SquareClient = rec + defer func() { SquareClient = origClient }() + + svc := NewPaymentService() + require.NoError(t, svc.DeletePaymentMethod(ctx, cardID, userID)) + + require.Equal(t, []string{"ccof:sq_del_disable"}, rec.deletedIDs(), "the Square card must be disabled on local delete (R8)") + + var deletedAt string + require.NoError(t, tx.QueryRow(ctx, `SELECT COALESCE(deleted_at::text, '') FROM user_saved_cards WHERE id = $1`, cardID).Scan(&deletedAt)) + require.NotEqual(t, "", deletedAt, "the card must still be soft-deleted locally") +} + +// TestDeletePaymentMethod_SquareFailure_StillDeletesLocally covers the +// best-effort contract: a Square disable failure must never block the local +// soft-delete. +func TestDeletePaymentMethod_SquareFailure_StillDeletesLocally(t *testing.T) { + ctx, tx := testutils.SetupTestTx(t) + userID, err := fixtures.CreateTestUser(tx) + require.NoError(t, err) + cardID, err := fixtures.CreateTestPaymentMethod(tx, userID, "ccof:sq_del_fail", "VISA", "8888") + require.NoError(t, err) + + origClient := SquareClient + SquareClient = &failingDeleteClient{SquareClient: square.NewDevClient()} + defer func() { SquareClient = origClient }() + + svc := NewPaymentService() + require.NoError(t, svc.DeletePaymentMethod(ctx, cardID, userID)) + + var deletedAt string + require.NoError(t, tx.QueryRow(ctx, `SELECT COALESCE(deleted_at::text, '') FROM user_saved_cards WHERE id = $1`, cardID).Scan(&deletedAt)) + require.NotEqual(t, "", deletedAt, "the local soft-delete must proceed even when the Square call fails") +} + +type failingDeleteClient struct { + square.SquareClient +} + +func (c *failingDeleteClient) DeleteCardOnFile(ctx context.Context, cardID string) error { + return fmt.Errorf("square: network error disabling card %s", cardID) +} diff --git a/backend/handlers/payments/payments_review_fixes_test.go b/backend/handlers/payments/payments_review_fixes_test.go index 334e5ec..3990007 100644 --- a/backend/handlers/payments/payments_review_fixes_test.go +++ b/backend/handlers/payments/payments_review_fixes_test.go @@ -142,14 +142,15 @@ func TestBuyGiftCard_SaveCard_ProvisionsCustomerForCreateCardOnFile(t *testing.T require.Equal(t, rec.lastCustomerID(), cid, "the stored square_customer_id must match the id passed to CreateCardOnFile") } -func TestBuyGiftCard_NoSaveCard_CreateCardOnFileEmptyCustomer(t *testing.T) { +func TestBuyGiftCard_NoSaveCard_ChargesNonceDirectly(t *testing.T) { ctx, tx := testutils.SetupTestTx(t) userID, err := fixtures.CreateTestUser(tx) require.NoError(t, err) token := jwt.GenerateUserToken(userID) origClient := SquareClient - rec := &recordingCardOnFileClient{SquareClient: square.NewDevClient()} + cof := &recordingCardOnFileClient{SquareClient: square.NewDevClient()} + rec := &recordingPaymentClient{SquareClient: cof} SquareClient = rec defer func() { SquareClient = origClient }() @@ -164,8 +165,18 @@ func TestBuyGiftCard_NoSaveCard_CreateCardOnFileEmptyCustomer(t *testing.T) { w := makePaymentRequest(BuyGiftCard, "POST", "/api/gift-cards/buy", req, token, ctx) require.Equal(t, http.StatusCreated, w.Code, w.Body.String()) - require.Equal(t, 1, rec.callCount()) - require.Equal(t, "", rec.lastCustomerID(), "a one-off non-save charge needs no Square customer") + // R6: a one-off non-save charge uses the cnon: nonce DIRECTLY — no + // card-on-file is created (nothing to orphan) and no customer is involved. + require.Equal(t, 0, cof.callCount(), "a one-off non-save charge must NOT tokenize via CreateCardOnFile") + rec.mu.Lock() + got := rec.lastReq.SourceID + rec.mu.Unlock() + require.Equal(t, newToken, got, "the nonce itself must be charged directly") + require.Equal(t, "", rec.lastReq.CustomerID, "a cnon: nonce charge carries no customer") + + var cardCount int + require.NoError(t, tx.QueryRow(ctx, `SELECT COUNT(*) FROM user_saved_cards WHERE user_id = $1`, userID).Scan(&cardCount)) + require.Zero(t, cardCount, "a one-off buy must not persist a saved card") } func TestCreateTillSale_VerificationTokenPassthrough(t *testing.T) { @@ -272,8 +283,8 @@ func TestCreateTillSale_OnlineSquare_NoCustomerProvisioned(t *testing.T) { r.ServeHTTP(w, req) require.Equal(t, http.StatusCreated, w.Code, w.Body.String()) - require.Equal(t, 1, rec.callCount()) - require.Equal(t, "", rec.lastCustomerID(), "the ephemeral till card is a one-off cnon: charge — no Square customer") + require.Equal(t, 0, rec.callCount()) + require.Equal(t, "", rec.lastCustomerID(), "the ephemeral till card is a one-off cnon: charge — no CreateCardOnFile, no Square customer") } // ============================================================================= @@ -588,12 +599,13 @@ func TestCreateBookingPayment_SaveCard_ProvisionsCustomerForCreateCardOnFile(t * require.Equal(t, rec.lastCustomerID(), cid) } -func TestCreateBookingPayment_NoSaveCard_CreateCardOnFileEmptyCustomer(t *testing.T) { +func TestCreateBookingPayment_NoSaveCard_ChargesNonceDirectly(t *testing.T) { ctx, tx := testutils.SetupTestTx(t) _, bookingID, userToken := setupPaymentStatusTest(t, ctx, tx, "confirmed") origClient := SquareClient - rec := &recordingCardOnFileClient{SquareClient: square.NewDevClient()} + cof := &recordingCardOnFileClient{SquareClient: square.NewDevClient()} + rec := &recordingPaymentClient{SquareClient: cof} SquareClient = rec defer func() { SquareClient = origClient }() @@ -609,6 +621,12 @@ func TestCreateBookingPayment_NoSaveCard_CreateCardOnFileEmptyCustomer(t *testin w := makePaymentRequest(handler, "POST", "/api/bookings/"+bookingID+"/payment", req, userToken, ctx) require.Equal(t, http.StatusOK, w.Code, w.Body.String()) - require.Equal(t, 1, rec.callCount()) - require.Equal(t, "", rec.lastCustomerID(), "a one-off non-save charge needs no Square customer") + // R6: a one-off non-save charge uses the cnon: nonce DIRECTLY — no + // card-on-file is created (nothing to orphan) and no customer is involved. + require.Equal(t, 0, cof.callCount(), "a one-off non-save charge must NOT tokenize via CreateCardOnFile") + rec.mu.Lock() + got := rec.lastReq.SourceID + rec.mu.Unlock() + require.Equal(t, cardToken, got, "the nonce itself must be charged directly") + require.Equal(t, "", rec.lastReq.CustomerID, "a cnon: nonce charge carries no customer") } diff --git a/backend/handlers/payments/payments_test.go b/backend/handlers/payments/payments_test.go index ac4c6c1..0cf8d78 100644 --- a/backend/handlers/payments/payments_test.go +++ b/backend/handlers/payments/payments_test.go @@ -2023,6 +2023,52 @@ func TestBuildSplitRecords_PaymentLessThanDepositMax_NoSplit(t *testing.T) { } } +func TestBuildSplitRecords_OverflowBeyondTotal_BecomesTip(t *testing.T) { + // £60 on a £50 future booking: deposit caps at £25 (50%), balance covers + // the remaining £25 owed, and the £10 overflow beyond the booking total + // becomes a tip record. + record := makeTestRecord("b5", "full", 60) + info := &BookingPaymentInfo{ + StartTime: clock.Now().Add(48 * time.Hour), + TotalAmount: 50, + TotalPaid: 0, + } + records := buildSplitRecords(record, "full", info, 60) + + if len(records) != 3 { + t.Fatalf("expected 3 records (deposit + balance + tip), got %d", len(records)) + } + if records[0].PaymentType != "deposit" { + t.Errorf("expected first record 'deposit', got %q", records[0].PaymentType) + } + if records[0].Amount != 25 { + t.Errorf("expected deposit amount 25, got %.2f", records[0].Amount) + } + if records[1].PaymentType != "balance" { + t.Errorf("expected second record 'balance', got %q", records[1].PaymentType) + } + if records[1].Amount != 25 { + t.Errorf("expected balance amount 25, got %.2f", records[1].Amount) + } + if records[2].PaymentType != "tip" { + t.Errorf("expected third record 'tip', got %q", records[2].PaymentType) + } + if records[2].Amount != 10 { + t.Errorf("expected tip amount 10 (overflow beyond the booking total), got %.2f", records[2].Amount) + } + // The tip is a split: zero fees and a derived idempotency key. + if records[2].Fees != 0 { + t.Errorf("expected tip fees=0, got %.2f", records[2].Fees) + } + if *records[2].IdempotencyKey != *records[0].IdempotencyKey+"-split-3" { + t.Errorf("expected tip key derived from primary, got %q", *records[2].IdempotencyKey) + } + // The splits share the Square payment id of the primary record. + if *records[2].SquarePaymentID != *records[0].SquarePaymentID { + t.Error("tip split must share square_payment_id with the primary record") + } +} + // --------------------------------------------------------------------------- // Handler-level atomicity — verify the full handler succeeds with split. // --------------------------------------------------------------------------- @@ -3405,7 +3451,7 @@ func TestSaveCardForUser_Success(t *testing.T) { t.Fatalf("failed to create user: %v", err) } svc := NewPaymentService() - cardID, err := svc.SaveCardForUser(ctx, userID, "cfa_test_success", "VISA", "4242", 12, 2030, "fp_success") + cardID, err := svc.SaveCardForUser(ctx, userID, "cus_test_success", "cfa_test_success", "VISA", "4242", 12, 2030, "fp_success") if err != nil { t.Errorf("unexpected error: %v", err) } @@ -3418,7 +3464,7 @@ func TestSaveCardForUser_InvalidUserID(t *testing.T) { ctx, tx := testutils.SetupTestTx(t) _ = tx svc := NewPaymentService() - _, err := svc.SaveCardForUser(ctx, "000000000001", "cfa_test", "VISA", "4242", 12, 2030, "fp_test") + _, err := svc.SaveCardForUser(ctx, "000000000001", "cus_test_invalid", "cfa_test", "VISA", "4242", 12, 2030, "fp_test") if err == nil { t.Error("expected error for non-existent user ID (FK violation)") } @@ -3584,8 +3630,6 @@ func TestTerminalPayment_ValidatePaymentTypeFails(t *testing.T) { } } - - func TestSweepStalePendingPayments_MarksOldFailed(t *testing.T) { t.Parallel() ctx, tx := testutils.SetupTestTx(t) @@ -3728,7 +3772,6 @@ func TestSavedCardPayment_SweptFailed_Rejected(t *testing.T) { } } - // TestSweepStalePendingPayments_CoversTillSales verifies the R3 fix: the sweep // also marks stale pending till_sales rows (card payments) as failed, so a // lost-response till sale can't stay pending past Square's key retention. @@ -3782,3 +3825,41 @@ func TestSweepStalePendingPayments_CoversTillSales(t *testing.T) { t.Errorf("expected fresh pending till sale to stay pending, got %q", freshStatus) } } + +func TestBuildSplitRecords_TipOverflow_SeparateTipRecord(t *testing.T) { + // A £60 payment on a £50 future booking with nothing paid yet: + // deposit = min(60, 25) = £25, balance = min(35, 25) = £25, + // tip = 60 - 25 - 25 = £10 — the overflow becomes a separate tip record. + record := makeTestRecord("b-overflow", "full", 60) + info := &BookingPaymentInfo{ + StartTime: clock.Now().Add(48 * time.Hour), + TotalAmount: 50, + TotalPaid: 0, + } + records := buildSplitRecords(record, "full", info, 60) + + if len(records) != 3 { + t.Fatalf("expected 3 records (deposit + balance + tip), got %d", len(records)) + } + if records[0].PaymentType != "deposit" || records[0].Amount != 25 { + t.Errorf("expected deposit 25, got %q %.2f", records[0].PaymentType, records[0].Amount) + } + if records[1].PaymentType != "balance" || records[1].Amount != 25 { + t.Errorf("expected balance 25, got %q %.2f", records[1].PaymentType, records[1].Amount) + } + if records[2].PaymentType != "tip" || records[2].Amount != 10 { + t.Errorf("expected tip record 10, got %q %.2f", records[2].PaymentType, records[2].Amount) + } + // Tip is an overflow split — zero fees, derived idempotency key -split-2. + if records[2].Fees != 0 { + t.Errorf("expected tip record fees=0, got %.2f", records[2].Fees) + } + wantKey := *record.IdempotencyKey + "-split-3" + if *records[2].IdempotencyKey != wantKey { + t.Errorf("expected tip key %q, got %q", wantKey, *records[2].IdempotencyKey) + } + // All three share the same SquarePaymentID (one charge, three ledger rows). + if *records[2].SquarePaymentID != *record.SquarePaymentID { + t.Error("tip split must share square_payment_id") + } +} diff --git a/backend/handlers/payments/validators_test.go b/backend/handlers/payments/validators_test.go new file mode 100644 index 0000000..8686683 --- /dev/null +++ b/backend/handlers/payments/validators_test.go @@ -0,0 +1,35 @@ +//go:build test && dev + +package payments + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +// TestValidateAmount covers the amount validator used by every payment +// handler: positive and within the £10,000 (1,000,000 pence) cap. Amounts are +// integer pence, so sub-penny "precision" is impossible by construction. +func TestValidateAmount(t *testing.T) { + t.Parallel() + + valid := []int64{1, 500, 10000, 999999, 1000000} + for _, amount := range valid { + require.NoErrorf(t, ValidateAmount(amount), "expected %d to be a valid amount", amount) + } + + invalid := []int64{0, -1, -500, 1000001} + for _, amount := range invalid { + require.Errorf(t, ValidateAmount(amount), "expected %d to be rejected", amount) + } + + // The cap is exclusive: exactly £10,000 (1,000,000 pence) is allowed, one + // penny more is rejected. + if err := ValidateAmount(1000001); err == nil { + t.Error("expected amount above £10,000 cap to be rejected") + } + if err := ValidateAmount(1000000); err != nil { + t.Errorf("expected exactly £10,000 to be allowed, got %v", err) + } +}