feat(backend): update auth middleware

Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
2026-06-18 16:26:02 +01:00
parent b5c1eef8c2
commit 37e1069404
2 changed files with 484 additions and 3 deletions
@@ -2,6 +2,7 @@ package mw

 import (
 	"context"
+	"log"
 	"net/http"
 	"strings"

@@ -50,7 +51,9 @@ func OptionalAuth(next http.Handler) http.Handler {
 			tokenString := strings.TrimPrefix(authHeader, "Bearer ")

 			userID, role, jti, err := auth.VerifyToken(tokenString, r.Context())
-			if err == nil {
+			if err != nil {
+				log.Printf("OptionalAuth: invalid token: %v", err)
+			} else {
 				ctx := context.WithValue(r.Context(), UserIDKey, userID)
 				ctx = context.WithValue(ctx, UserRoleKey, role)
 				ctx = context.WithValue(ctx, JTIKey, jti)
@@ -97,9 +100,18 @@ func RequireVerified(next http.Handler) http.Handler {
 	return RequireRole("verified_email", "admin")(next)
 }

-// RequireAdmin middleware - only allows admin
+// RequireAdmin middleware - only allows admin and validates the user ID is present
+// in context, so handlers don't need to re-check. The user ID is always set by
+// RequireAuth before this runs, but this adds a defensive safety net.
 func RequireAdmin(next http.Handler) http.Handler {
-	return RequireRole("admin")(next)
+	return RequireRole("admin")(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		uid, ok := r.Context().Value(UserIDKey).(string)
+		if !ok || uid == "" {
+			http.Error(w, "Authentication required", http.StatusUnauthorized)
+			return
+		}
+		next.ServeHTTP(w, r)
+	}))
 }

 // Helper functions to get user info from context
@@ -0,0 +1,469 @@
+//go:build test
+// +build test
+
+package mw
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"crussell/testutils/jwt"
+)
+
+func testHandler() http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		userID, _ := GetUserID(r.Context())
+		role, _ := GetUserRole(r.Context())
+		jti, _ := GetJTI(r.Context())
+		w.WriteHeader(http.StatusOK)
+		w.Write([]byte(userID + "|" + role + "|" + jti))
+	})
+}
+
+// =============================================================================
+// RequireAuth
+// =============================================================================
+
+func TestRequireAuth_NoHeader(t *testing.T) {
+	req := httptest.NewRequest("GET", "/", nil)
+	w := httptest.NewRecorder()
+	RequireAuth(testHandler()).ServeHTTP(w, req)
+
+	if w.Code != http.StatusUnauthorized {
+		t.Errorf("expected 401, got %d", w.Code)
+	}
+}
+
+func TestRequireAuth_MalformedHeader(t *testing.T) {
+	req := httptest.NewRequest("GET", "/", nil)
+	req.Header.Set("Authorization", "NotBearer token123")
+	w := httptest.NewRecorder()
+	RequireAuth(testHandler()).ServeHTTP(w, req)
+
+	if w.Code != http.StatusUnauthorized {
+		t.Errorf("expected 401, got %d", w.Code)
+	}
+}
+
+func TestRequireAuth_EmptyToken(t *testing.T) {
+	req := httptest.NewRequest("GET", "/", nil)
+	req.Header.Set("Authorization", "Bearer ")
+	w := httptest.NewRecorder()
+	RequireAuth(testHandler()).ServeHTTP(w, req)
+
+	if w.Code != http.StatusUnauthorized {
+		t.Errorf("expected 401, got %d", w.Code)
+	}
+}
+
+func TestRequireAuth_InvalidToken(t *testing.T) {
+	req := httptest.NewRequest("GET", "/", nil)
+	req.Header.Set("Authorization", "Bearer this.is.not.a.valid.jwt")
+	w := httptest.NewRecorder()
+	RequireAuth(testHandler()).ServeHTTP(w, req)
+
+	if w.Code != http.StatusUnauthorized {
+		t.Errorf("expected 401, got %d", w.Code)
+	}
+}
+
+func TestRequireAuth_ValidToken(t *testing.T) {
+	token := jwt.GenerateTestToken("testuser123", "verified_email")
+
+	req := httptest.NewRequest("GET", "/", nil)
+	req.Header.Set("Authorization", "Bearer "+token)
+	w := httptest.NewRecorder()
+	RequireAuth(testHandler()).ServeHTTP(w, req)
+
+	if w.Code != http.StatusOK {
+		t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String())
+	}
+
+	body := w.Body.String()
+	if body == "" || body == "|" {
+		t.Errorf("expected user info in response, got %q", body)
+	}
+}
+
+func TestRequireAuth_AdminToken(t *testing.T) {
+	token := jwt.GenerateAdminToken()
+
+	req := httptest.NewRequest("GET", "/", nil)
+	req.Header.Set("Authorization", "Bearer "+token)
+	w := httptest.NewRecorder()
+	RequireAuth(testHandler()).ServeHTTP(w, req)
+
+	if w.Code != http.StatusOK {
+		t.Errorf("expected 200, got %d: %s", w.Code, w.Body.String())
+	}
+}
+
+func TestRequireAuth_SetsAllContextFields(t *testing.T) {
+	token := jwt.GenerateTestToken("testuser123", "admin")
+
+	var capturedID, capturedRole, capturedJTI string
+	collector := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		capturedID, _ = GetUserID(r.Context())
+		capturedRole, _ = GetUserRole(r.Context())
+		capturedJTI, _ = GetJTI(r.Context())
+		w.WriteHeader(http.StatusOK)
+	})
+
+	req := httptest.NewRequest("GET", "/", nil)
+	req.Header.Set("Authorization", "Bearer "+token)
+	w := httptest.NewRecorder()
+	RequireAuth(collector).ServeHTTP(w, req)
+
+	if w.Code != http.StatusOK {
+		t.Fatalf("expected 200, got %d", w.Code)
+	}
+	if capturedID != "testuser123" {
+		t.Errorf("expected userID 'testuser123', got %q", capturedID)
+	}
+	if capturedRole != "admin" {
+		t.Errorf("expected role 'admin', got %q", capturedRole)
+	}
+	if capturedJTI == "" {
+		t.Error("expected non-empty JTI")
+	}
+}
+
+// =============================================================================
+// OptionalAuth
+// =============================================================================
+
+func TestOptionalAuth_NoHeader(t *testing.T) {
+	req := httptest.NewRequest("GET", "/", nil)
+	w := httptest.NewRecorder()
+	OptionalAuth(testHandler()).ServeHTTP(w, req)
+
+	if w.Code != http.StatusOK {
+		t.Errorf("expected 200 (pass-through), got %d", w.Code)
+	}
+
+	body := w.Body.String()
+	if body != "||" {
+		t.Errorf("expected empty context (||), got %q", body)
+	}
+}
+
+func TestOptionalAuth_ValidToken(t *testing.T) {
+	token := jwt.GenerateTestToken("testuser123", "verified_email")
+
+	req := httptest.NewRequest("GET", "/", nil)
+	req.Header.Set("Authorization", "Bearer "+token)
+	w := httptest.NewRecorder()
+	OptionalAuth(testHandler()).ServeHTTP(w, req)
+
+	if w.Code != http.StatusOK {
+		t.Errorf("expected 200, got %d", w.Code)
+	}
+
+	body := w.Body.String()
+	if body == "||" {
+		t.Error("expected context to be set with valid token")
+	}
+}
+
+func TestOptionalAuth_InvalidToken(t *testing.T) {
+	req := httptest.NewRequest("GET", "/", nil)
+	req.Header.Set("Authorization", "Bearer obviously.invalid.token")
+	w := httptest.NewRecorder()
+	OptionalAuth(testHandler()).ServeHTTP(w, req)
+
+	// Should pass through without setting context
+	if w.Code != http.StatusOK {
+		t.Errorf("expected 200 (pass-through on invalid token), got %d", w.Code)
+	}
+
+	body := w.Body.String()
+	if body != "||" {
+		t.Errorf("expected empty context on invalid token, got %q", body)
+	}
+}
+
+func TestOptionalAuth_NoBearer(t *testing.T) {
+	req := httptest.NewRequest("GET", "/", nil)
+	req.Header.Set("Authorization", "Basic somecreds")
+	w := httptest.NewRecorder()
+	OptionalAuth(testHandler()).ServeHTTP(w, req)
+
+	if w.Code != http.StatusOK {
+		t.Errorf("expected 200 (pass-through without Bearer prefix), got %d", w.Code)
+	}
+}
+
+// =============================================================================
+// RequireRole
+// =============================================================================
+
+func TestRequireRole_Allowed(t *testing.T) {
+	handler := RequireRole("admin")(testHandler())
+
+	req := httptest.NewRequest("GET", "/", nil)
+	ctx := context.WithValue(req.Context(), UserRoleKey, "admin")
+	req = req.WithContext(ctx)
+	w := httptest.NewRecorder()
+	handler.ServeHTTP(w, req)
+
+	if w.Code != http.StatusOK {
+		t.Errorf("expected 200 for admin role, got %d", w.Code)
+	}
+}
+
+func TestRequireRole_MultipleRoles(t *testing.T) {
+	handler := RequireRole("admin", "verified_email")(testHandler())
+
+	req := httptest.NewRequest("GET", "/", nil)
+	ctx := context.WithValue(req.Context(), UserRoleKey, "verified_email")
+	req = req.WithContext(ctx)
+	w := httptest.NewRecorder()
+	handler.ServeHTTP(w, req)
+
+	if w.Code != http.StatusOK {
+		t.Errorf("expected 200 for verified_email in allowed list, got %d", w.Code)
+	}
+}
+
+func TestRequireRole_Forbidden(t *testing.T) {
+	handler := RequireRole("admin")(testHandler())
+
+	req := httptest.NewRequest("GET", "/", nil)
+	ctx := context.WithValue(req.Context(), UserRoleKey, "verified_email")
+	req = req.WithContext(ctx)
+	w := httptest.NewRecorder()
+	handler.ServeHTTP(w, req)
+
+	if w.Code != http.StatusForbidden {
+		t.Errorf("expected 403 for non-admin role, got %d", w.Code)
+	}
+}
+
+func TestRequireRole_NoRoleInContext(t *testing.T) {
+	handler := RequireRole("admin")(testHandler())
+
+	req := httptest.NewRequest("GET", "/", nil)
+	// No UserRoleKey in context
+	w := httptest.NewRecorder()
+	handler.ServeHTTP(w, req)
+
+	if w.Code != http.StatusUnauthorized {
+		t.Errorf("expected 401 when no role in context, got %d", w.Code)
+	}
+}
+
+func TestRequireRole_EmptyAllowedList(t *testing.T) {
+	handler := RequireRole()(testHandler())
+
+	req := httptest.NewRequest("GET", "/", nil)
+	ctx := context.WithValue(req.Context(), UserRoleKey, "admin")
+	req = req.WithContext(ctx)
+	w := httptest.NewRecorder()
+	handler.ServeHTTP(w, req)
+
+	if w.Code != http.StatusForbidden {
+		t.Errorf("expected 403 with empty allowed list, got %d", w.Code)
+	}
+}
+
+// =============================================================================
+// RequireVerified (wraps RequireRole("verified_email", "admin"))
+// =============================================================================
+
+func TestRequireVerified_VerifiedEmail(t *testing.T) {
+	req := httptest.NewRequest("GET", "/", nil)
+	ctx := context.WithValue(req.Context(), UserRoleKey, "verified_email")
+	req = req.WithContext(ctx)
+	w := httptest.NewRecorder()
+	RequireVerified(testHandler()).ServeHTTP(w, req)
+
+	if w.Code != http.StatusOK {
+		t.Errorf("expected 200 for verified_email, got %d", w.Code)
+	}
+}
+
+func TestRequireVerified_Admin(t *testing.T) {
+	req := httptest.NewRequest("GET", "/", nil)
+	ctx := context.WithValue(req.Context(), UserRoleKey, "admin")
+	req = req.WithContext(ctx)
+	w := httptest.NewRecorder()
+	RequireVerified(testHandler()).ServeHTTP(w, req)
+
+	if w.Code != http.StatusOK {
+		t.Errorf("expected 200 for admin, got %d", w.Code)
+	}
+}
+
+func TestRequireVerified_Unverified(t *testing.T) {
+	req := httptest.NewRequest("GET", "/", nil)
+	ctx := context.WithValue(req.Context(), UserRoleKey, "unverified_email")
+	req = req.WithContext(ctx)
+	w := httptest.NewRecorder()
+	RequireVerified(testHandler()).ServeHTTP(w, req)
+
+	if w.Code != http.StatusForbidden {
+		t.Errorf("expected 403 for unverified, got %d", w.Code)
+	}
+}
+
+// =============================================================================
+// RequireAdmin (RequireRole("admin") + UserIDKey guard)
+// =============================================================================
+
+func TestRequireAdmin_AdminRole(t *testing.T) {
+	req := httptest.NewRequest("GET", "/", nil)
+	ctx := context.WithValue(req.Context(), UserRoleKey, "admin")
+	ctx = context.WithValue(ctx, UserIDKey, "adminuser001")
+	req = req.WithContext(ctx)
+	w := httptest.NewRecorder()
+	RequireAdmin(testHandler()).ServeHTTP(w, req)
+
+	if w.Code != http.StatusOK {
+		t.Errorf("expected 200 for admin, got %d", w.Code)
+	}
+}
+
+func TestRequireAdmin_NonAdmin(t *testing.T) {
+	req := httptest.NewRequest("GET", "/", nil)
+	ctx := context.WithValue(req.Context(), UserRoleKey, "verified_email")
+	ctx = context.WithValue(ctx, UserIDKey, "someuser001")
+	req = req.WithContext(ctx)
+	w := httptest.NewRecorder()
+	RequireAdmin(testHandler()).ServeHTTP(w, req)
+
+	if w.Code != http.StatusForbidden {
+		t.Errorf("expected 403 for non-admin, got %d", w.Code)
+	}
+}
+
+func TestRequireAdmin_NoUserIDInContext(t *testing.T) {
+	req := httptest.NewRequest("GET", "/", nil)
+	ctx := context.WithValue(req.Context(), UserRoleKey, "admin")
+	// No UserIDKey set
+	req = req.WithContext(ctx)
+	w := httptest.NewRecorder()
+	RequireAdmin(testHandler()).ServeHTTP(w, req)
+
+	if w.Code != http.StatusUnauthorized {
+		t.Errorf("expected 401 when UserID missing from context, got %d", w.Code)
+	}
+}
+
+func TestRequireAdmin_EmptyUserID(t *testing.T) {
+	req := httptest.NewRequest("GET", "/", nil)
+	ctx := context.WithValue(req.Context(), UserRoleKey, "admin")
+	ctx = context.WithValue(ctx, UserIDKey, "")
+	req = req.WithContext(ctx)
+	w := httptest.NewRecorder()
+	RequireAdmin(testHandler()).ServeHTTP(w, req)
+
+	if w.Code != http.StatusUnauthorized {
+		t.Errorf("expected 401 for empty UserID, got %d", w.Code)
+	}
+}
+
+// =============================================================================
+// Context helpers — GetUserID, GetUserRole, GetJTI
+// =============================================================================
+
+func TestGetUserID_Present(t *testing.T) {
+	ctx := context.WithValue(context.Background(), UserIDKey, "abc123")
+	id, ok := GetUserID(ctx)
+	if !ok {
+		t.Error("expected ok=true")
+	}
+	if id != "abc123" {
+		t.Errorf("expected 'abc123', got %q", id)
+	}
+}
+
+func TestGetUserID_Missing(t *testing.T) {
+	_, ok := GetUserID(context.Background())
+	if ok {
+		t.Error("expected ok=false")
+	}
+}
+
+func TestGetUserRole_Present(t *testing.T) {
+	ctx := context.WithValue(context.Background(), UserRoleKey, "admin")
+	role, ok := GetUserRole(ctx)
+	if !ok {
+		t.Error("expected ok=true")
+	}
+	if role != "admin" {
+		t.Errorf("expected 'admin', got %q", role)
+	}
+}
+
+func TestGetUserRole_Missing(t *testing.T) {
+	_, ok := GetUserRole(context.Background())
+	if ok {
+		t.Error("expected ok=false")
+	}
+}
+
+func TestGetJTI_Present(t *testing.T) {
+	ctx := context.WithValue(context.Background(), JTIKey, "jti_test_123")
+	jti, ok := GetJTI(ctx)
+	if !ok {
+		t.Error("expected ok=true")
+	}
+	if jti != "jti_test_123" {
+		t.Errorf("expected 'jti_test_123', got %q", jti)
+	}
+}
+
+func TestGetJTI_Missing(t *testing.T) {
+	_, ok := GetJTI(context.Background())
+	if ok {
+		t.Error("expected ok=false")
+	}
+}
+
+// =============================================================================
+// Full middleware chain — RequireAuth → RequireAdmin
+// =============================================================================
+
+func TestAuthChain_AdminTokenFullFlow(t *testing.T) {
+	token := jwt.GenerateAdminToken()
+
+	handler := RequireAuth(RequireAdmin(testHandler()))
+
+	req := httptest.NewRequest("GET", "/admin/dashboard", nil)
+	req.Header.Set("Authorization", "Bearer "+token)
+	w := httptest.NewRecorder()
+	handler.ServeHTTP(w, req)
+
+	if w.Code != http.StatusOK {
+		t.Errorf("expected 200 for admin token through full chain, got %d: %s", w.Code, w.Body.String())
+	}
+}
+
+func TestAuthChain_VerifiedTokenBlockedByRequireAdmin(t *testing.T) {
+	token := jwt.GenerateTestToken("user123abc", "verified_email")
+
+	handler := RequireAuth(RequireAdmin(testHandler()))
+
+	req := httptest.NewRequest("GET", "/admin/dashboard", nil)
+	req.Header.Set("Authorization", "Bearer "+token)
+	w := httptest.NewRecorder()
+	handler.ServeHTTP(w, req)
+
+	if w.Code != http.StatusForbidden {
+		t.Errorf("expected 403 for verified_email through RequireAdmin, got %d", w.Code)
+	}
+}
+
+func TestAuthChain_NoTokenBlockedByRequireAuth(t *testing.T) {
+	handler := RequireAuth(RequireAdmin(testHandler()))
+
+	req := httptest.NewRequest("GET", "/admin/dashboard", nil)
+	w := httptest.NewRecorder()
+	handler.ServeHTTP(w, req)
+
+	if w.Code != http.StatusUnauthorized {
+		t.Errorf("expected 401 with no token, got %d", w.Code)
+	}
+}