From 2e0760f0836ca023cfe66b9379f649b27aec8eba Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Sun, 5 Jul 2026 20:26:33 +0100 Subject: [PATCH] feat: add IP-based anon reservation cleanup to admin reserve handler Extend AdminReserveSlotHandler's pre-overlap DELETE to also clean up anonymous RESERVATION:anon entries matching the admin's IP address. This handles the edge case where an admin previously reserved a slot without authentication. Also reorganise imports to follow goimports conventions. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- backend/handlers/bookings/admin_reserve.go | 22 ++- backend/handlers/bookings/overlap_test.go | 201 +++++++++++++++++++++ 2 files changed, 218 insertions(+), 5 deletions(-) diff --git a/backend/handlers/bookings/admin_reserve.go b/backend/handlers/bookings/admin_reserve.go index da06ffa..3430a27 100644 --- a/backend/handlers/bookings/admin_reserve.go +++ b/backend/handlers/bookings/admin_reserve.go @@ -2,15 +2,17 @@ package bookings import ( "context" - "crussell/db" + "crypto/md5" "crussell/clock" - "github.com/jackc/pgx/v5" + "crussell/db" "crussell/handlers/scheduling" "crussell/mw" "encoding/json" "errors" "fmt" + "github.com/jackc/pgx/v5" "log" + "net" "net/http" "time" ) @@ -140,11 +142,21 @@ func AdminReserveSlotHandler(w http.ResponseWriter, r *http.Request) { // using db.Conn.Exec so the delete is visible to the separate connection // used by CheckTimeBlockerOverlap. The in-transaction DELETE is kept // as a safety net for the insert-phase. + // Also clean up anonymous reservations matching this admin's IP + // (edge case: admin previously reserved without authentication). + ip := r.Header.Get("CF-Connecting-IP") + if ip == "" { + ip, _, _ = net.SplitHostPort(r.RemoteAddr) + if ip == "" { + ip = r.RemoteAddr + } + } + ipHash := fmt.Sprintf("%x", md5.Sum([]byte(ip)))[:8] if _, delErr := db.Conn.Exec(r.Context(), ` DELETE FROM time_blockers - WHERE description LIKE 'RESERVATION:admin:%' - AND created_by = $1 - `, adminID); delErr != nil { + WHERE (description LIKE 'RESERVATION:admin:%' AND created_by = $1) + OR (description LIKE 'RESERVATION:anon:' || $2 || ':%') + `, adminID, ipHash); delErr != nil { log.Printf("Failed to delete existing admin reservation: %v", delErr) http.Error(w, "Internal server error", http.StatusInternalServerError) return diff --git a/backend/handlers/bookings/overlap_test.go b/backend/handlers/bookings/overlap_test.go index 68dde2f..abfb521 100644 --- a/backend/handlers/bookings/overlap_test.go +++ b/backend/handlers/bookings/overlap_test.go @@ -1997,3 +1997,204 @@ func TestReserveSlot_CleansUpAnonReservation(t *testing.T) { t.Errorf("expected anonymous reservation to be cleaned up by pre-overlap DELETE, got %d remaining", remaining) } } + +// TestEditBooking_DoesNotSelfBlock_OwnReservation verifies that a user's own +// RESERVATION does not block EditBookingHandler — the excludeUserID parameter +// prevents the reservation from appearing as a time blocker conflict. +func TestEditBooking_DoesNotSelfBlock_OwnReservation(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + _, err = tx.Exec(ctx, "UPDATE users SET deposits_required = 0 WHERE id = $1", userID) + if err != nil { + t.Fatalf("failed to set deposits_required: %v", err) + } + + serviceID, err := fixtures.CreateTestService(tx) + if err != nil { + t.Fatalf("failed to create service: %v", err) + } + dur := durationMinutes(t, ctx, tx, serviceID) + token := jwt.GenerateUserToken(userID) + + baseTime := weekdayTime(time.Wednesday, 10) + + // Create a confirmed booking + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, baseTime) + if err != nil { + t.Fatalf("failed to create booking: %v", err) + } + _, err = tx.Exec(ctx, "UPDATE bookings SET status = 'confirmed' WHERE id = $1", bookingID) + if err != nil { + t.Fatalf("failed to confirm booking: %v", err) + } + + // Create a RESERVATION for this user at a time that overlaps with the edit target + reservationTime := baseTime.Add(time.Duration(dur) * time.Minute) + _, err = tx.Exec(ctx, ` + INSERT INTO time_blockers (start_time, duration_minutes, description, created_by) + VALUES ($1, 60, 'RESERVATION:user:' || $2 || ':' || EXTRACT(epoch FROM NOW())::bigint::text, $2) + `, reservationTime, userID) + if err != nil { + t.Fatalf("failed to create reservation: %v", err) + } + + // Edit the booking to a time that overlaps the reservation. + // Without excludeUserID, this would return 409. + overlapTime := reservationTime.Add(-time.Duration(dur/2) * time.Minute) + + handler := http.HandlerFunc(EditBookingHandler) + w := makeRequest(handler, "PUT", "/api/bookings/"+bookingID, map[string]interface{}{ + "start_time": overlapTime.Format(time.RFC3339), + "service_ids": []string{serviceID}, + }, token, ctx) + + if w.Code == http.StatusConflict { + t.Fatalf("own reservation should NOT self-block EditBookingHandler: got 409. body: %s", w.Body.String()) + } + if w.Code != http.StatusOK { + t.Fatalf("expected 200 for edit (own reservation excluded), got %d. body: %s", w.Code, w.Body.String()) + } +} + +// TestAdminRescheduleBooking_DoesNotSelfBlock verifies that an admin's own +// RESERVATION does not block AdminRescheduleBookingHandler. +func TestAdminRescheduleBooking_DoesNotSelfBlock(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + adminID, err := fixtures.CreateTestAdminUser(tx) + if err != nil { + t.Fatalf("failed to create admin: %v", err) + } + token := jwt.GenerateTestToken(adminID, "admin") + + serviceID, err := fixtures.CreateTestService(tx) + if err != nil { + t.Fatalf("failed to create service: %v", err) + } + + baseTime := weekdayTime(time.Wednesday, 10) + + // Create a booking belonging to a regular user + userID, err := fixtures.CreateTestUser(tx) + if err != nil { + t.Fatalf("failed to create user: %v", err) + } + bookingID, err := fixtures.CreateTestBookingAtTime(tx, userID, serviceID, baseTime) + if err != nil { + t.Fatalf("failed to create booking: %v", err) + } + _, err = tx.Exec(ctx, "UPDATE bookings SET status = 'confirmed' WHERE id = $1", bookingID) + if err != nil { + t.Fatalf("failed to confirm booking: %v", err) + } + + // Create an admin RESERVATION at a time that would overlap the reschedule target + reservationTime := baseTime.Add(2 * time.Hour) + _, err = tx.Exec(ctx, ` + INSERT INTO time_blockers (start_time, duration_minutes, description, created_by) + VALUES ($1, 60, 'RESERVATION:admin:walkin:guest:' || EXTRACT(epoch FROM NOW())::bigint::text, $2) + `, reservationTime, adminID) + if err != nil { + t.Fatalf("failed to create admin reservation: %v", err) + } + + // Reschedule to a time overlapping the admin's own reservation + overlapTime := reservationTime.Add(-30 * time.Minute) + + handler := http.HandlerFunc(AdminRescheduleBookingHandler) + w := makeAuthRequest(handler, "POST", "/api/admin/bookings/"+bookingID+"/reschedule", map[string]interface{}{ + "start_time": overlapTime.Format(time.RFC3339), + }, token, "", ctx) + + if w.Code == http.StatusConflict { + t.Fatalf("admin's own reservation should NOT self-block AdminRescheduleBookingHandler: got 409. body: %s", w.Body.String()) + } + if w.Code != http.StatusOK { + t.Fatalf("expected 200 for reschedule (own reservation excluded), got %d. body: %s", w.Code, w.Body.String()) + } +} + +// TestAdminReserveSlot_CleansUpAnonReservation verifies that AdminReserveSlotHandler +// cleans up anonymous RESERVATION:anon entries matching the admin's IP +// when the admin is authenticated. +func TestAdminReserveSlot_CleansUpAnonReservation(t *testing.T) { + t.Parallel() + ctx, tx := testutils.SetupTestTx(t) + + adminID, err := fixtures.CreateTestAdminUser(tx) + if err != nil { + t.Fatalf("failed to create admin: %v", err) + } + token := jwt.GenerateTestToken(adminID, "admin") + + serviceID, err := fixtures.CreateTestService(tx) + if err != nil { + t.Fatalf("failed to create service: %v", err) + } + + future := weekdayTime(time.Monday, 10) + testIP := "192.0.2.2" + ipHash := fmt.Sprintf("%x", md5.Sum([]byte(testIP)))[:8] + + // Create an anonymous reservation matching this IP + var blockerID string + err = tx.QueryRow(ctx, ` + INSERT INTO time_blockers (start_time, duration_minutes, description, created_by) + VALUES ($1, 60, $2, NULL) + RETURNING id + `, future, fmt.Sprintf("RESERVATION:anon:%s:%d", ipHash, clock.Now().UnixNano())).Scan(&blockerID) + if err != nil { + t.Fatalf("failed to create anon reservation: %v", err) + } + + makeIPRequest := func(handler http.Handler, method, path string, body interface{}, token, ip, userID string, requestCtx ...context.Context) *httptest.ResponseRecorder { + var req *http.Request + if body != nil { + bodyBytes, _ := json.Marshal(body) + req = httptest.NewRequest(method, path, bytes.NewReader(bodyBytes)) + req.Header.Set("Content-Type", "application/json") + } else { + req = httptest.NewRequest(method, path, nil) + } + if token != "" { + req.Header.Set("Authorization", "Bearer "+token) + } + req.Header.Set("CF-Connecting-IP", ip) + baseCtx := req.Context() + if len(requestCtx) > 0 { + baseCtx = requestCtx[0] + } + baseCtx = context.WithValue(baseCtx, mw.UserIDKey, userID) + baseCtx = context.WithValue(baseCtx, mw.UserRoleKey, "admin") + rctx := chi.NewRouteContext() + ctx := context.WithValue(baseCtx, chi.RouteCtxKey, rctx) + req = req.WithContext(ctx) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + return w + } + + w := makeIPRequest(http.HandlerFunc(AdminReserveSlotHandler), "POST", "/api/admin/bookings/reserve", + &AdminReserveSlotRequest{ + StartTime: future, + ServiceIDs: []string{serviceID}, + DurationMinutes: 60, + ReservationType: "walkin", + }, token, testIP, adminID, ctx) + + if w.Code != http.StatusCreated { + t.Fatalf("admin reserve: expected 201, got %d. body: %s", w.Code, w.Body.String()) + } + + var remaining int + tx.QueryRow(ctx, "SELECT COUNT(*) FROM time_blockers WHERE id = $1", blockerID).Scan(&remaining) + if remaining != 0 { + t.Errorf("expected anonymous reservation to be cleaned up by admin pre-overlap DELETE, got %d remaining", remaining) + } +}