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 <clio-agent@sisyphuslabs.ai>
2026-07-05 20:26:33 +01:00
parent cc6ad7f00f
commit 2e0760f083
2 changed files with 218 additions and 5 deletions
@@ -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
@@ -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)
+	}
+}