refactor(api): enforce protocol-neutral service boundaries

- refactor: move HTTP status and DTO concerns out of model and service layers into API and handler code. - feat: add admin service boundary and route auth through services instead of direct repository access. - test: add architecture and error tests covering package boundaries, redaction, and service behavior. - docs: record the protocol-neutral boundary decision and update architecture and roadmap notes.
2026-07-05 23:30:17 +08:00
parent 28e17a5b08
commit 63ede5c237
34 changed files with 1028 additions and 438 deletions
@@ -7,29 +7,47 @@ import (
 	"net/http/httptest"
 	"strings"
 	"testing"
-	"time"

 	"github.com/gin-gonic/gin"
-	"gorm.io/driver/sqlite"
-	"gorm.io/gorm"

-	"github.com/dhao2001/mygo/internal/auth"
 	"github.com/dhao2001/mygo/internal/model"
-	"github.com/dhao2001/mygo/internal/repository"
+	"github.com/dhao2001/mygo/internal/service"
 )

-func setupTestRouter(secret []byte) *gin.Engine {
+type stubAccessAuthenticator struct {
+	wantToken string
+	principal service.Principal
+	err       error
+	called    bool
+}
+
+func (s *stubAccessAuthenticator) AuthenticateAccessToken(_ context.Context, token string) (*service.Principal, error) {
+	s.called = true
+	if s.wantToken != "" && token != s.wantToken {
+		return nil, model.NewUnauthenticatedError("invalid token")
+	}
+	if s.err != nil {
+		return nil, s.err
+	}
+	return &s.principal, nil
+}
+
+func setupTestRouter(authenticator accessAuthenticator) *gin.Engine {
 	gin.SetMode(gin.TestMode)
 	r := gin.New()
-	r.Use(AuthRequired(secret))
+	r.Use(AuthRequired(authenticator))
 	r.GET("/protected", func(c *gin.Context) {
-		c.JSON(http.StatusOK, gin.H{"user_id": MustGetUserID(c)})
+		c.JSON(http.StatusOK, gin.H{
+			"user_id":  MustGetUserID(c),
+			"is_admin": MustGetPrincipal(c).IsAdmin,
+		})
 	})
 	return r
 }

 func TestAuthRequiredNoHeader(t *testing.T) {
-	r := setupTestRouter([]byte("test-secret"))
+	authenticator := &stubAccessAuthenticator{}
+	r := setupTestRouter(authenticator)

 	req := httptest.NewRequest(http.MethodGet, "/protected", nil)
 	rec := httptest.NewRecorder()
@@ -38,10 +56,14 @@ func TestAuthRequiredNoHeader(t *testing.T) {
 	if rec.Code != http.StatusUnauthorized {
 		t.Errorf("status = %d, want %d", rec.Code, http.StatusUnauthorized)
 	}
+	if authenticator.called {
+		t.Fatal("authenticator should not be called without a bearer token")
+	}
 }

 func TestAuthRequiredInvalidFormat(t *testing.T) {
-	r := setupTestRouter([]byte("test-secret"))
+	authenticator := &stubAccessAuthenticator{}
+	r := setupTestRouter(authenticator)

 	req := httptest.NewRequest(http.MethodGet, "/protected", nil)
 	req.Header.Set("Authorization", "invalid")
@@ -51,10 +73,14 @@ func TestAuthRequiredInvalidFormat(t *testing.T) {
 	if rec.Code != http.StatusUnauthorized {
 		t.Errorf("status = %d, want %d", rec.Code, http.StatusUnauthorized)
 	}
+	if authenticator.called {
+		t.Fatal("authenticator should not be called for malformed authorization header")
+	}
 }

 func TestAuthRequiredNotBearer(t *testing.T) {
-	r := setupTestRouter([]byte("test-secret"))
+	authenticator := &stubAccessAuthenticator{}
+	r := setupTestRouter(authenticator)

 	req := httptest.NewRequest(http.MethodGet, "/protected", nil)
 	req.Header.Set("Authorization", "Basic dXNlcjpwYXNz")
@@ -64,105 +90,51 @@ func TestAuthRequiredNotBearer(t *testing.T) {
 	if rec.Code != http.StatusUnauthorized {
 		t.Errorf("status = %d, want %d", rec.Code, http.StatusUnauthorized)
 	}
+	if authenticator.called {
+		t.Fatal("authenticator should not be called for non-bearer authorization")
+	}
 }

-func TestAuthRequiredExpiredToken(t *testing.T) {
-	secret := []byte("test-secret")
-	token, err := auth.GenerateAccessToken("user-1", secret, -1*time.Minute)
-	if err != nil {
-		t.Fatalf("GenerateAccessToken = %v", err)
-	}
+func TestAuthRequiredServiceRejectsToken(t *testing.T) {
+	authenticator := &stubAccessAuthenticator{err: model.NewUnauthenticatedError("invalid or expired token")}
+	r := setupTestRouter(authenticator)

-	r := setupTestRouter(secret)
 	req := httptest.NewRequest(http.MethodGet, "/protected", nil)
-	req.Header.Set("Authorization", "Bearer "+token)
+	req.Header.Set("Authorization", "Bearer expired")
 	rec := httptest.NewRecorder()
 	r.ServeHTTP(rec, req)

 	if rec.Code != http.StatusUnauthorized {
 		t.Errorf("status = %d, want %d", rec.Code, http.StatusUnauthorized)
 	}
+	if !authenticator.called {
+		t.Fatal("authenticator was not called")
+	}
 }

 func TestAuthRequiredValidToken(t *testing.T) {
-	secret := []byte("test-secret")
-	token, err := auth.GenerateAccessToken("user-1", secret, 15*time.Minute)
-	if err != nil {
-		t.Fatalf("GenerateAccessToken = %v", err)
+	authenticator := &stubAccessAuthenticator{
+		wantToken: "valid",
+		principal: service.Principal{
+			UserID:  "user-1",
+			IsAdmin: true,
+		},
 	}
+	r := setupTestRouter(authenticator)

-	r := setupTestRouter(secret)
 	req := httptest.NewRequest(http.MethodGet, "/protected", nil)
-	req.Header.Set("Authorization", "Bearer "+token)
+	req.Header.Set("Authorization", "Bearer valid")
 	rec := httptest.NewRecorder()
 	r.ServeHTTP(rec, req)

 	if rec.Code != http.StatusOK {
 		t.Errorf("status = %d, want %d", rec.Code, http.StatusOK)
 	}
-}
-
-func TestAuthRequiredRefreshTokenRejected(t *testing.T) {
-	secret := []byte("test-secret")
-	token, err := auth.GenerateRefreshToken("user-1", secret, 7*24*time.Hour)
-	if err != nil {
-		t.Fatalf("GenerateRefreshToken = %v", err)
+	if !strings.Contains(rec.Body.String(), "user-1") {
+		t.Errorf("response body %q does not contain user id", rec.Body.String())
 	}
-
-	r := setupTestRouter(secret)
-	req := httptest.NewRequest(http.MethodGet, "/protected", nil)
-	req.Header.Set("Authorization", "Bearer "+token)
-	rec := httptest.NewRecorder()
-	r.ServeHTTP(rec, req)
-
-	if rec.Code != http.StatusUnauthorized {
-		t.Errorf("status = %d, want %d (refresh token should be rejected)", rec.Code, http.StatusUnauthorized)
-	}
-}
-
-func TestGetUserID(t *testing.T) {
-	secret := []byte("test-secret")
-	token, err := auth.GenerateAccessToken("alice-42", secret, 15*time.Minute)
-	if err != nil {
-		t.Fatalf("GenerateAccessToken = %v", err)
-	}
-
-	r := setupTestRouter(secret)
-	req := httptest.NewRequest(http.MethodGet, "/protected", nil)
-	req.Header.Set("Authorization", "Bearer "+token)
-	rec := httptest.NewRecorder()
-	r.ServeHTTP(rec, req)
-
-	if rec.Code != http.StatusOK {
-		t.Fatalf("status = %d", rec.Code)
-	}
-
-	body := rec.Body.String()
-	if !strings.Contains(body, "alice-42") {
-		t.Errorf("response body %q does not contain user id", body)
-	}
-}
-
-func TestMustGetUserID(t *testing.T) {
-	secret := []byte("test-secret")
-	token, err := auth.GenerateAccessToken("bob-99", secret, 15*time.Minute)
-	if err != nil {
-		t.Fatalf("GenerateAccessToken = %v", err)
-	}
-
-	r := setupTestRouter(secret)
-	req := httptest.NewRequest(http.MethodGet, "/protected", nil)
-	req.Header.Set("Authorization", "Bearer "+token)
-	rec := httptest.NewRecorder()
-	r.ServeHTTP(rec, req)
-
-	if rec.Code != http.StatusOK {
-		t.Fatalf("status = %d", rec.Code)
-	}
-
-	body := rec.Body.String()
-	if !strings.Contains(body, "bob-99") {
-		t.Errorf("response body %q does not contain user id", body)
+	if !strings.Contains(rec.Body.String(), "true") {
+		t.Errorf("response body %q does not contain admin flag", rec.Body.String())
 	}
 }

@@ -170,7 +142,7 @@ func TestMustGetUserIDPanics(t *testing.T) {
 	gin.SetMode(gin.TestMode)
 	r := gin.New()
 	r.GET("/naked", func(c *gin.Context) {
-		MustGetUserID(c) // should panic — no AuthRequired middleware applied
+		MustGetUserID(c)
 	})

 	req := httptest.NewRequest(http.MethodGet, "/naked", nil)
@@ -184,75 +156,27 @@ func TestMustGetUserIDPanics(t *testing.T) {
 	r.ServeHTTP(rec, req)
 }

-func TestAuthRequiredWrongSecret(t *testing.T) {
-	secret := []byte("test-secret")
-	token, err := auth.GenerateAccessToken("user-1", secret, 15*time.Minute)
-	if err != nil {
-		t.Fatalf("GenerateAccessToken = %v", err)
-	}
-
-	// Use a different secret for the middleware
-	r := setupTestRouter([]byte("different-secret"))
-	req := httptest.NewRequest(http.MethodGet, "/protected", nil)
-	req.Header.Set("Authorization", "Bearer "+token)
-	rec := httptest.NewRecorder()
-	r.ServeHTTP(rec, req)
-
-	if rec.Code != http.StatusUnauthorized {
-		t.Errorf("status = %d, want %d", rec.Code, http.StatusUnauthorized)
-	}
-}
-
-// --- AdminRequired tests ---
-
-// errorBody extracts the error.message field from a JSON response body.
 type errorBody struct {
 	Error struct {
 		Message string `json:"message"`
 	} `json:"error"`
 }

-func setupAdminTestDB(t *testing.T) repository.UserRepository {
-	t.Helper()
-
-	db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{})
-	if err != nil {
-		t.Fatalf("open db: %v", err)
-	}
-	if err := db.AutoMigrate(&model.User{}); err != nil {
-		t.Fatalf("migrate: %v", err)
-	}
-
-	return repository.NewUserRepository(db)
-}
-
-// injectUserIDMiddleware simulates AuthRequired by injecting a user_id into the context.
-func injectUserIDMiddleware(userID string) gin.HandlerFunc {
+func injectPrincipalMiddleware(principal *service.Principal) gin.HandlerFunc {
 	return func(c *gin.Context) {
-		c.Set(userIDKey, userID)
+		if principal != nil {
+			c.Set(principalKey, *principal)
+			c.Set(userIDKey, principal.UserID)
+		}
 		c.Next()
 	}
 }

 func TestAdminRequired_AdminPasses(t *testing.T) {
-	repo := setupAdminTestDB(t)
-	ctx := context.Background()
-
-	user := &model.User{
-		ID:       "admin-1",
-		Username: "admin",
-		Email:    "admin@example.com",
-		IsAdmin:  true,
-		Status:   model.StatusActive,
-	}
-	if err := repo.Create(ctx, user); err != nil {
-		t.Fatalf("Create = %v", err)
-	}
-
 	gin.SetMode(gin.TestMode)
 	r := gin.New()
-	r.Use(injectUserIDMiddleware("admin-1"))
-	r.Use(AdminRequired(repo))
+	r.Use(injectPrincipalMiddleware(&service.Principal{UserID: "admin-1", IsAdmin: true}))
+	r.Use(AdminRequired())
 	r.GET("/admin", func(c *gin.Context) {
 		c.JSON(http.StatusOK, gin.H{"status": "ok"})
 	})
@@ -267,24 +191,10 @@ func TestAdminRequired_AdminPasses(t *testing.T) {
 }

 func TestAdminRequired_NonAdminForbidden(t *testing.T) {
-	repo := setupAdminTestDB(t)
-	ctx := context.Background()
-
-	user := &model.User{
-		ID:       "user-1",
-		Username: "regular",
-		Email:    "regular@example.com",
-		IsAdmin:  false,
-		Status:   model.StatusActive,
-	}
-	if err := repo.Create(ctx, user); err != nil {
-		t.Fatalf("Create = %v", err)
-	}
-
 	gin.SetMode(gin.TestMode)
 	r := gin.New()
-	r.Use(injectUserIDMiddleware("user-1"))
-	r.Use(AdminRequired(repo))
+	r.Use(injectPrincipalMiddleware(&service.Principal{UserID: "user-1", IsAdmin: false}))
+	r.Use(AdminRequired())
 	r.GET("/admin", func(c *gin.Context) {
 		c.JSON(http.StatusOK, gin.H{"status": "ok"})
 	})
@@ -306,49 +216,10 @@ func TestAdminRequired_NonAdminForbidden(t *testing.T) {
 	}
 }

-func TestAdminRequired_SoftDeletedAdmin(t *testing.T) {
-	repo := setupAdminTestDB(t)
-	ctx := context.Background()
-
-	user := &model.User{
-		ID:       "admin-2",
-		Username: "deleted_admin",
-		Email:    "deleted_admin@example.com",
-		IsAdmin:  true,
-		Status:   model.StatusActive,
-	}
-	if err := repo.Create(ctx, user); err != nil {
-		t.Fatalf("Create = %v", err)
-	}
-
-	// Soft-delete the admin user
-	if err := repo.Delete(ctx, "admin-2"); err != nil {
-		t.Fatalf("Delete = %v", err)
-	}
-
+func TestAdminRequired_NoPrincipal(t *testing.T) {
 	gin.SetMode(gin.TestMode)
 	r := gin.New()
-	r.Use(injectUserIDMiddleware("admin-2"))
-	r.Use(AdminRequired(repo))
-	r.GET("/admin", func(c *gin.Context) {
-		c.JSON(http.StatusOK, gin.H{"status": "ok"})
-	})
-
-	req := httptest.NewRequest(http.MethodGet, "/admin", nil)
-	rec := httptest.NewRecorder()
-	r.ServeHTTP(rec, req)
-
-	if rec.Code != http.StatusUnauthorized {
-		t.Errorf("status = %d, want %d", rec.Code, http.StatusUnauthorized)
-	}
-}
-
-func TestAdminRequired_NoUserID(t *testing.T) {
-	repo := setupAdminTestDB(t)
-
-	gin.SetMode(gin.TestMode)
-	r := gin.New()
-	r.Use(AdminRequired(repo))
+	r.Use(AdminRequired())
 	r.GET("/admin", func(c *gin.Context) {
 		c.JSON(http.StatusOK, gin.H{"status": "ok"})
 	})