From a18f96912df8f809efa8dbc877272284d3912115 Mon Sep 17 00:00:00 2001 From: Huxley Date: Thu, 16 Jul 2026 12:24:36 +0800 Subject: [PATCH] feat(repository): refactor query/mutation ports with capability-safe interfaces - refactor: split UserRepository into AuthUserRepository and AdminUserRepository capabilities - refactor: split SessionRepository into AuthSessionRepository and repository-owned CLI/admin methods - refactor: replace generic Repository Update/Delete with operation-specific params and ownership predicates - refactor: replace CredentialRepository generic CRUD with passkey-specific methods - refactor: replace FileRepository generic Create/Update/Delete with UploadedFileParams, DirectoryParams, and owned soft-delete - refactor: remove repository fields from WebApp struct; repositories are now composition-time wiring only - feat: add domain error kinds ErrParentNotFound, ErrParentNotDir, ErrDirectoryNotEmpty, ErrInvalidMove - feat: add CredentialTypeAppPasskey constant - feat: add testutil.SetUserAdmin for test fixture setup that bypasses production service ports - test: add architecture test banning GORM Save in repository package - test: add capability interface contract tests ensuring each service receives the minimal interface - test: add blockingStorage helper for concurrent promotion tests - test: add preserved DSN parameter test for sqliteImmediateDSN - docs: update architecture decisions with repository write rules and capability separation - docs: update roadmap to clarify atomic single-use refresh sessions - docs: add -race test target to development docs --- docs/server/architecture.md | 16 ++ docs/server/decisions.md | 25 ++- docs/server/development.md | 7 + docs/server/roadmap.md | 6 +- internal/app/webapp.go | 54 ++--- internal/app/webapp_test.go | 4 +- internal/architecture_test.go | 19 ++ internal/handler/admin_test.go | 43 ++-- internal/model/credential.go | 3 + internal/model/errors.go | 14 +- internal/repository/capability_test.go | 40 ++++ internal/repository/credential.go | 67 +++--- internal/repository/credential_test.go | 79 ++++--- internal/repository/db.go | 18 +- internal/repository/db_test.go | 20 ++ internal/repository/file.go | 253 ++++++++++++++++++--- internal/repository/file_test.go | 275 ++++++++++++++++++++--- internal/repository/session.go | 71 ++++-- internal/repository/session_test.go | 92 ++++++-- internal/repository/user.go | 63 ++++-- internal/repository/user_test.go | 64 +++--- internal/server/file_integration_test.go | 4 - internal/server/server_test.go | 15 +- internal/service/admin.go | 6 +- internal/service/admin_test.go | 6 +- internal/service/auth.go | 75 +++---- internal/service/auth_test.go | 106 +++++++-- internal/service/file.go | 95 ++++---- internal/service/file_test.go | 90 ++++++++ internal/testutil/user.go | 23 ++ 30 files changed, 1259 insertions(+), 394 deletions(-) create mode 100644 internal/repository/capability_test.go create mode 100644 internal/testutil/user.go diff --git a/docs/server/architecture.md b/docs/server/architecture.md index 6d2a1b7..729569f 100644 --- a/docs/server/architecture.md +++ b/docs/server/architecture.md @@ -20,6 +20,9 @@ Rules: - JSON response DTOs live in HTTP packages. Domain and service result structs are not the public REST schema. - Domain models may use `json:"-"` for defensive redaction of sensitive fields; this is not a REST response contract. - Domain errors carry a protocol-neutral kind, a safe message, and an optional cause. HTTP status mapping happens only at the API boundary. +- Repository query ports remain entity-oriented, while mutation ports expose business commands with operation-specific parameter types. Services never pass a mutable domain entity to a generic update or delete method. +- Service constructors using split repository capabilities accept the smallest capability needed by that service. Their composition-time unions do not cross into business services. +- Repository mutations use explicit columns and lifecycle/ownership predicates. Full-entity `Save` is prohibited so a write cannot accidentally change protected fields or revive a soft-deleted record. - Repository abstracts the database; Storage abstracts where bytes live, including staged upload promotion. - `internal/server` is the composition root — wires all dependencies together. @@ -42,6 +45,19 @@ Rules: | | `internal/storage` | Storage backend interface + local disk impl, with staging and promotion | 🛠 WIP | | **Util** | `internal/auth` | JWT sign/verify (HS256), token type discrimination (access/refresh), password hashing (bcrypt), app passkey tokens | ✅ | | | `internal/api` | Unified JSON error response helpers and REST error-kind mapping | ✅ | +| | `internal/testutil` | Test-only fixture helpers for privileged setup that production service ports intentionally do not expose | ✅ | + +## Persistence Write Boundaries + +- User registration accepts `RegisteredUserParams`; the repository fixes new users to non-admin, active state. Administrative deletion is a separate capability. +- File creation, directory creation, metadata changes, and soft deletion use distinct commands. Metadata updates can only write `name`, `parent_id`, and GORM-maintained `updated_at`. +- Refresh sessions are created, atomically consumed, idempotently revoked, or removed by explicit maintenance operations. Refresh token rotation consumes a session at most once. +- Passkey creation fixes the credential type to the app-passkey type. Usage recording and revocation include credential-type and ownership conditions where applicable. +- `app.WebApp` contains runtime services, storage, database lifecycle, configuration, and metadata. Repository implementations remain local to bootstrap and are injected directly into the services that require them. + +## File Hierarchy Transactions + +File child creation, movement, and directory deletion share one parent-locking protocol. PostgreSQL locks active owned source and parent rows in sorted ID order with `SELECT ... FOR UPDATE`; directory deletion checks active children while holding the directory lock. SQLite connections add `_txlock=immediate` to the configured DSN so the transaction acquires its write reservation before reading hierarchy state. These rules prevent a concurrent mutation from creating an active child below a deleted directory. ## API Routes (v0) diff --git a/docs/server/decisions.md b/docs/server/decisions.md index c3f756c..c97b590 100644 --- a/docs/server/decisions.md +++ b/docs/server/decisions.md @@ -46,7 +46,7 @@ **Consequences**: - Version is build metadata from `internal/app/version.go`, not a config-file field. -- `app.WebApp` is the place to add future services, repositories, storage, and app metadata incrementally. +- `app.WebApp` is the place to add future runtime services, storage, and app metadata incrementally; repository implementations stay inside composition setup. - Request ID middleware is not part of the current foundation; add it only with a logging/tracing/error-correlation design. ## 2026-04-29: Auth Refinements @@ -123,3 +123,26 @@ - Authenticated file operations cannot use status codes or messages to determine whether another user's resource exists. - A successful first delete returns `204 No Content`; repeated deletes return `404 Not Found` while remaining idempotent in effect. - Unauthenticated requests remain `401 Unauthorized`, non-empty directory deletion remains `409 Conflict`, and permission-denied behavior outside the file boundary remains unchanged. + +## 2026-07-16: Command-Scoped Persistence Mutations + +**Context**: Generic `Repository.Update(*Entity)`, full-entity GORM `Save`, and broad delete methods exposed fields and lifecycle transitions that individual services were not authorized to perform. They also allowed a stale file entity to revive a soft-deleted row, left file parent checks vulnerable to concurrent deletion, and allowed two refresh requests to consume the same session. + +**Decisions**: + +| Decision | Guidance | +|----------|----------| +| Command-scoped writes | Every mutation has a use-case-specific method and parameter type. Mutable domain entities do not cross a service-to-repository write boundary. | +| Capability interfaces | Auth and admin services accept only the query and command capabilities they require. Wider user and session interfaces exist only for construction and repository-focused consumers. | +| Explicit mutation predicates | Updates name concrete columns and include ownership, credential type, and/or active-state predicates appropriate to the command. A zero-row guarded mutation maps to a domain not-found result. | +| Transactional file hierarchy | Child creation, movement, and directory deletion revalidate hierarchy state in the same transaction. PostgreSQL locks relevant rows in sorted ID order; SQLite DSNs force `_txlock=immediate`. | +| Atomic refresh consumption | Refresh rotates through one transaction that locks, returns, and deletes the session. A concurrent replay cannot issue a second token pair. Logout remains an idempotent revoke-by-hash command. | +| Test-only privilege setup | Tests elevate users through `internal/testutil`; no production role mutation is added merely to prepare fixtures. | + +**Consequences**: + +- Protected user, file, session, and credential fields are absent from ordinary mutation inputs, reducing illegal write paths at compile time. +- Deleted file rows cannot be revived by stale metadata updates, and successful hierarchy races cannot leave active orphan children. +- PostgreSQL relies on row-level locking while SQLite serializes these hierarchy and session write transactions from their first database operation. This favors correctness over concurrent SQLite writers. +- Adding a future profile, password, role, restore, or other mutation requires a named command and an explicit capability rather than extending a universal update method. +- REST routes, payloads, status codes, database schema, and dependency set remain unchanged. diff --git a/docs/server/development.md b/docs/server/development.md index 48b1646..287cda0 100644 --- a/docs/server/development.md +++ b/docs/server/development.md @@ -17,6 +17,7 @@ go build -o mygo . ```bash go test ./... go test -v -run TestName ./internal/... +go test -race ./internal/repository ./internal/service ./internal/server ``` ## Lint & Format @@ -52,6 +53,12 @@ go mod tidy # after adding/removing imports Server config is loaded via viper from `config.yaml` (defaults in `internal/config/load.go`). +For SQLite, the repository preserves configured DSN parameters and adds +`_txlock=immediate`. Write transactions therefore reserve the database before +performing hierarchy or refresh-session reads, matching the locking assumptions +used by the repository commands. Keep this behavior when supplying a SQLite URI +such as `file:mygo.db?cache=shared`. + ```yaml server: host: 0.0.0.0 diff --git a/docs/server/roadmap.md b/docs/server/roadmap.md index 91fa845..5c56c54 100644 --- a/docs/server/roadmap.md +++ b/docs/server/roadmap.md @@ -5,7 +5,7 @@ | Feature | Status | Notes | |---------|--------|-------| | CLI config management | ✅ | Viper YAML + env + flags, typed Duration config | -| JWT authentication | ✅ | access + refresh tokens, refresh token in DB, app passkey support | +| JWT authentication | ✅ | access + refresh tokens, atomic single-use refresh sessions, app passkey support | | Web API foundation | ✅ | WebApp composition, Gin router, graceful shutdown, `GET /api/v1/version` | | File upload/download/manage APIs | 🛠 WIP | REST API via Gin | | Admin endpoints | 🛠 WIP | user service boundary in place for superusers | @@ -21,13 +21,13 @@ Package-level implementation order (each task includes unit tests): 4. `internal/api` — protocol-neutral error kind to REST response mapping ✅ 5. `internal/auth` — JWT utils ✅ 6. `internal/storage` — backend interface + local fs with staged upload promotion -7. `internal/repository` — interfaces + GORM/SQLite impl ✅ +7. `internal/repository` — command-scoped mutation capabilities + GORM/SQLite transaction protocol ✅ 8. `internal/service` — auth, file, admin services ✅ 9. `internal/middleware` — logger, cors, auth ✅ (auth done; principal boundary enforced) 10. `internal/handler` — auth, account, file, admin handlers 🛠 (HTTP DTO mapping in place) 11. `internal/server` — Gin router, route registration, graceful shutdown ✅ 12. `cmd/serve.go`, `cmd/config.go`, `cmd/status.go` ✅ (serve done) -13. Integration tests 🛠 (authenticated register → login → upload → list → download route flow covered with a small file; authentication bypass, invalid token, concealed cross-user resources, and strict delete boundaries covered) +13. Integration tests 🛠 (authenticated register → login → upload → list → download route flow covered with a small file; authentication bypass, invalid token, concealed cross-user resources, strict delete, atomic refresh consumption, and file hierarchy race boundaries covered) 14. Architecture boundary tests ✅ ## Future diff --git a/internal/app/webapp.go b/internal/app/webapp.go index 5659e75..9d9edb8 100644 --- a/internal/app/webapp.go +++ b/internal/app/webapp.go @@ -17,15 +17,11 @@ type WebApp struct { Config *config.Config Version string - DB *gorm.DB - UserRepo repository.UserRepository - SessionRepo repository.SessionRepository - FileRepo repository.FileRepository - CredentialRepo repository.CredentialRepository - AuthService *service.AuthService - AdminService *service.AdminService - FileService *service.FileService - Storage storage.StorageBackend + DB *gorm.DB + AuthService *service.AuthService + AdminService *service.AdminService + FileService *service.FileService + Storage storage.StorageBackend } // Bootstrap creates a fully initialized WebApp from config. @@ -62,43 +58,31 @@ func Bootstrap(cfg *config.Config) (*WebApp, error) { adminService := service.NewAdminService(userRepo) return &WebApp{ - Config: cfg, - Version: AppVersion, - DB: db, - UserRepo: userRepo, - SessionRepo: sessionRepo, - FileRepo: fileRepo, - CredentialRepo: credentialRepo, - AuthService: authService, - AdminService: adminService, - FileService: fileService, - Storage: store, + Config: cfg, + Version: AppVersion, + DB: db, + AuthService: authService, + AdminService: adminService, + FileService: fileService, + Storage: store, }, nil } // NewWebApp creates a WebApp with pre-built dependencies (useful for testing). func NewWebApp(cfg *config.Config, db *gorm.DB, - userRepo repository.UserRepository, - sessionRepo repository.SessionRepository, - fileRepo repository.FileRepository, - credentialRepo repository.CredentialRepository, authService *service.AuthService, adminService *service.AdminService, fileService *service.FileService, store storage.StorageBackend, ) *WebApp { return &WebApp{ - Config: cfg, - Version: AppVersion, - DB: db, - UserRepo: userRepo, - SessionRepo: sessionRepo, - FileRepo: fileRepo, - CredentialRepo: credentialRepo, - AuthService: authService, - AdminService: adminService, - FileService: fileService, - Storage: store, + Config: cfg, + Version: AppVersion, + DB: db, + AuthService: authService, + AdminService: adminService, + FileService: fileService, + Storage: store, } } diff --git a/internal/app/webapp_test.go b/internal/app/webapp_test.go index a52c72a..2a8f3ef 100644 --- a/internal/app/webapp_test.go +++ b/internal/app/webapp_test.go @@ -9,7 +9,7 @@ import ( func TestNewWebApp(t *testing.T) { cfg := &config.Config{} - webApp := NewWebApp(cfg, nil, nil, nil, nil, nil, nil, nil, nil, nil) + webApp := NewWebApp(cfg, nil, nil, nil, nil, nil) if webApp.Config != cfg { t.Fatal("Config was not assigned") @@ -20,7 +20,7 @@ func TestNewWebApp(t *testing.T) { } func TestCloseNilDB(t *testing.T) { - webApp := NewWebApp(&config.Config{}, nil, nil, nil, nil, nil, nil, nil, nil, nil) + webApp := NewWebApp(&config.Config{}, nil, nil, nil, nil, nil) if err := webApp.Close(); err != nil { t.Errorf("Close with nil DB should not error: %v", err) } diff --git a/internal/architecture_test.go b/internal/architecture_test.go index 7e9528a..8f900a8 100644 --- a/internal/architecture_test.go +++ b/internal/architecture_test.go @@ -34,6 +34,25 @@ func TestArchitecture_HTTPDoesNotImportRepositories(t *testing.T) { } } +func TestArchitecture_RepositoriesDoNotUseFullEntitySave(t *testing.T) { + pkgs := parsePackage(t, "repository") + for _, pkg := range pkgs { + for filename, file := range pkg.Files { + ast.Inspect(file, func(node ast.Node) bool { + call, ok := node.(*ast.CallExpr) + if !ok { + return true + } + selector, ok := call.Fun.(*ast.SelectorExpr) + if ok && selector.Sel.Name == "Save" { + t.Fatalf("%s uses GORM Save; repository writes must use operation-specific columns", filename) + } + return true + }) + } + } +} + func packageImports(t *testing.T, dir string) map[string]bool { t.Helper() diff --git a/internal/handler/admin_test.go b/internal/handler/admin_test.go index 9c5dbac..0fcf0a9 100644 --- a/internal/handler/admin_test.go +++ b/internal/handler/admin_test.go @@ -17,9 +17,10 @@ import ( "github.com/dhao2001/mygo/internal/model" "github.com/dhao2001/mygo/internal/repository" "github.com/dhao2001/mygo/internal/service" + "github.com/dhao2001/mygo/internal/testutil" ) -func setupAdminRouter(t *testing.T) (*gin.Engine, repository.UserRepository) { +func setupAdminRouter(t *testing.T) (*gin.Engine, repository.UserRepository, *gorm.DB) { t.Helper() db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) @@ -63,7 +64,7 @@ func setupAdminRouter(t *testing.T) (*gin.Engine, repository.UserRepository) { admin.DELETE("/users/:id", adminHandler.DeleteUser) } - return r, userRepo + return r, userRepo, db } // registerHelper creates a user via the HTTP endpoint and returns the user ID. @@ -113,24 +114,16 @@ func loginHelper(t *testing.T, r *gin.Engine, email, password string) string { } // makeAdminHelper promotes a user to admin in the database. -func makeAdminHelper(t *testing.T, userRepo repository.UserRepository, userID string) { +func makeAdminHelper(t *testing.T, db *gorm.DB, userID string) { t.Helper() - - user, err := userRepo.FindByID(context.Background(), userID) - if err != nil { - t.Fatalf("find user %s: %v", userID, err) - } - user.IsAdmin = true - if err := userRepo.Update(context.Background(), user); err != nil { - t.Fatalf("update user: %v", err) - } + testutil.SetUserAdmin(t, db, userID, true) } func TestAdminDeleteUser(t *testing.T) { - r, userRepo := setupAdminRouter(t) + r, _, db := setupAdminRouter(t) adminID := registerHelper(t, r, "admin", "admin@example.com", "adminpass123") - makeAdminHelper(t, userRepo, adminID) + makeAdminHelper(t, db, adminID) userID := registerHelper(t, r, "bob", "bob@example.com", "bobpass123") @@ -159,10 +152,10 @@ func TestAdminDeleteUser(t *testing.T) { } func TestAdminDeleteUserForbidden(t *testing.T) { - r, userRepo := setupAdminRouter(t) + r, _, db := setupAdminRouter(t) adminID := registerHelper(t, r, "admin", "admin@example.com", "adminpass123") - makeAdminHelper(t, userRepo, adminID) + makeAdminHelper(t, db, adminID) userID := registerHelper(t, r, "bob", "bob@example.com", "bobpass123") @@ -181,7 +174,7 @@ func TestAdminDeleteUserForbidden(t *testing.T) { } func TestAdminDeleteUserUnauthorized(t *testing.T) { - r, _ := setupAdminRouter(t) + r, _, _ := setupAdminRouter(t) req := httptest.NewRequest(http.MethodDelete, "/api/v1/admin/users/some-id", nil) rec := httptest.NewRecorder() @@ -193,10 +186,10 @@ func TestAdminDeleteUserUnauthorized(t *testing.T) { } func TestAdminGetUser(t *testing.T) { - r, userRepo := setupAdminRouter(t) + r, _, db := setupAdminRouter(t) adminID := registerHelper(t, r, "admin", "admin@example.com", "adminpass123") - makeAdminHelper(t, userRepo, adminID) + makeAdminHelper(t, db, adminID) userID := registerHelper(t, r, "bob", "bob@example.com", "bobpass123") @@ -231,15 +224,15 @@ func TestAdminGetUser(t *testing.T) { } func TestAdminGetDeletedUser(t *testing.T) { - r, userRepo := setupAdminRouter(t) + r, userRepo, db := setupAdminRouter(t) adminID := registerHelper(t, r, "admin", "admin@example.com", "adminpass123") - makeAdminHelper(t, userRepo, adminID) + makeAdminHelper(t, db, adminID) userID := registerHelper(t, r, "bob", "bob@example.com", "bobpass123") // Soft-delete regular user directly via repo - if err := userRepo.Delete(context.Background(), userID); err != nil { + if err := userRepo.MarkAdminDeleted(context.Background(), userID); err != nil { t.Fatalf("soft-delete user: %v", err) } @@ -267,17 +260,17 @@ func TestAdminGetDeletedUser(t *testing.T) { } func TestAdminListIncludesDeleted(t *testing.T) { - r, userRepo := setupAdminRouter(t) + r, userRepo, db := setupAdminRouter(t) adminID := registerHelper(t, r, "admin", "admin@example.com", "adminpass123") - makeAdminHelper(t, userRepo, adminID) + makeAdminHelper(t, db, adminID) // Create 2 regular users _ = registerHelper(t, r, "alice", "alice@example.com", "alicepass123") bobID := registerHelper(t, r, "bob", "bob@example.com", "bobpass123") // Soft-delete bob - if err := userRepo.Delete(context.Background(), bobID); err != nil { + if err := userRepo.MarkAdminDeleted(context.Background(), bobID); err != nil { t.Fatalf("soft-delete user: %v", err) } diff --git a/internal/model/credential.go b/internal/model/credential.go index f75556c..53808a1 100644 --- a/internal/model/credential.go +++ b/internal/model/credential.go @@ -4,6 +4,9 @@ import ( "time" ) +// CredentialTypeAppPasskey identifies an application passkey credential. +const CredentialTypeAppPasskey = "app_passkey" + // Credential represents an alternative authentication credential for a user. // The primary password is stored on the User model; additional credentials // (app passkeys, WebAuthn, OAuth) are stored here with a type discriminator. diff --git a/internal/model/errors.go b/internal/model/errors.go index e8a0777..249af43 100644 --- a/internal/model/errors.go +++ b/internal/model/errors.go @@ -6,11 +6,15 @@ import ( ) var ( - ErrNotFound = errors.New("resource not found") - ErrDuplicate = errors.New("resource already exists") - ErrUnauthorized = errors.New("unauthorized") - ErrForbidden = errors.New("forbidden") - ErrUploadTooLarge = errors.New("upload exceeds maximum size") + ErrNotFound = errors.New("resource not found") + ErrDuplicate = errors.New("resource already exists") + ErrUnauthorized = errors.New("unauthorized") + ErrForbidden = errors.New("forbidden") + ErrUploadTooLarge = errors.New("upload exceeds maximum size") + ErrParentNotFound = errors.New("parent resource not found") + ErrParentNotDir = errors.New("parent resource is not a directory") + ErrDirectoryNotEmpty = errors.New("directory is not empty") + ErrInvalidMove = errors.New("invalid file move") ) // ErrorKind classifies domain errors without tying them to a transport. diff --git a/internal/repository/capability_test.go b/internal/repository/capability_test.go new file mode 100644 index 0000000..c6f5f2c --- /dev/null +++ b/internal/repository/capability_test.go @@ -0,0 +1,40 @@ +package repository + +import ( + "reflect" + "testing" +) + +func TestRepositoryCapabilitiesExcludeUnneededOperations(t *testing.T) { + tests := []struct { + name string + capability reflect.Type + forbidden []string + }{ + { + name: "auth user", + capability: reflect.TypeFor[AuthUserRepository](), + forbidden: []string{"MarkAdminDeleted", "ListIncludeDeleted"}, + }, + { + name: "admin user", + capability: reflect.TypeFor[AdminUserRepository](), + forbidden: []string{"CreateRegisteredUser", "FindByEmail"}, + }, + { + name: "auth session", + capability: reflect.TypeFor[AuthSessionRepository](), + forbidden: []string{"DeleteSessionsByUserID", "DeleteExpiredSessions"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + for _, method := range tt.forbidden { + if _, ok := tt.capability.MethodByName(method); ok { + t.Fatalf("capability unexpectedly exposes %s", method) + } + } + }) + } +} diff --git a/internal/repository/credential.go b/internal/repository/credential.go index 9ef6b6c..481bb5a 100644 --- a/internal/repository/credential.go +++ b/internal/repository/credential.go @@ -10,15 +10,22 @@ import ( "github.com/dhao2001/mygo/internal/model" ) +// PasskeyParams contains the fields accepted when creating an app passkey. +type PasskeyParams struct { + ID string + UserID string + Label string + SecretHash string +} + // CredentialRepository provides access to alternative credential records. type CredentialRepository interface { - Create(ctx context.Context, cred *model.Credential) error - FindByID(ctx context.Context, id string) (*model.Credential, error) - FindByUserID(ctx context.Context, userID string) ([]model.Credential, error) - FindByUserIDAndType(ctx context.Context, userID, credType string) ([]model.Credential, error) - FindByHash(ctx context.Context, hash string) (*model.Credential, error) - UpdateLastUsed(ctx context.Context, id string) error - Delete(ctx context.Context, id string) error + CreatePasskey(ctx context.Context, params PasskeyParams) error + FindPasskeyByID(ctx context.Context, id string) (*model.Credential, error) + ListPasskeys(ctx context.Context, userID string) ([]model.Credential, error) + FindPasskeyByHash(ctx context.Context, hash string) (*model.Credential, error) + RecordPasskeyUsed(ctx context.Context, id string) error + RevokeOwnedPasskey(ctx context.Context, userID, id string) error } type credentialRepository struct { @@ -30,7 +37,14 @@ func NewCredentialRepository(db *gorm.DB) CredentialRepository { return &credentialRepository{db: db} } -func (r *credentialRepository) Create(ctx context.Context, cred *model.Credential) error { +func (r *credentialRepository) CreatePasskey(ctx context.Context, params PasskeyParams) error { + cred := &model.Credential{ + ID: params.ID, + UserID: params.UserID, + Type: model.CredentialTypeAppPasskey, + Label: params.Label, + SecretHash: params.SecretHash, + } result := r.db.WithContext(ctx).Create(cred) if result.Error != nil { if isDuplicateKeyError(result.Error) { @@ -41,9 +55,9 @@ func (r *credentialRepository) Create(ctx context.Context, cred *model.Credentia return nil } -func (r *credentialRepository) FindByID(ctx context.Context, id string) (*model.Credential, error) { +func (r *credentialRepository) FindPasskeyByID(ctx context.Context, id string) (*model.Credential, error) { var cred model.Credential - result := r.db.WithContext(ctx).First(&cred, "id = ?", id) + result := r.db.WithContext(ctx).First(&cred, "id = ? AND type = ?", id, model.CredentialTypeAppPasskey) if errors.Is(result.Error, gorm.ErrRecordNotFound) { return nil, model.ErrNotFound } @@ -53,27 +67,18 @@ func (r *credentialRepository) FindByID(ctx context.Context, id string) (*model. return &cred, nil } -func (r *credentialRepository) FindByUserID(ctx context.Context, userID string) ([]model.Credential, error) { +func (r *credentialRepository) ListPasskeys(ctx context.Context, userID string) ([]model.Credential, error) { var creds []model.Credential - result := r.db.WithContext(ctx).Where("user_id = ?", userID).Find(&creds) + result := r.db.WithContext(ctx).Where("user_id = ? AND type = ?", userID, model.CredentialTypeAppPasskey).Find(&creds) if result.Error != nil { return nil, result.Error } return creds, nil } -func (r *credentialRepository) FindByUserIDAndType(ctx context.Context, userID, credType string) ([]model.Credential, error) { - var creds []model.Credential - result := r.db.WithContext(ctx).Where("user_id = ? AND type = ?", userID, credType).Find(&creds) - if result.Error != nil { - return nil, result.Error - } - return creds, nil -} - -func (r *credentialRepository) FindByHash(ctx context.Context, hash string) (*model.Credential, error) { +func (r *credentialRepository) FindPasskeyByHash(ctx context.Context, hash string) (*model.Credential, error) { var cred model.Credential - result := r.db.WithContext(ctx).First(&cred, "secret_hash = ?", hash) + result := r.db.WithContext(ctx).First(&cred, "secret_hash = ? AND type = ?", hash, model.CredentialTypeAppPasskey) if errors.Is(result.Error, gorm.ErrRecordNotFound) { return nil, model.ErrNotFound } @@ -83,19 +88,27 @@ func (r *credentialRepository) FindByHash(ctx context.Context, hash string) (*mo return &cred, nil } -func (r *credentialRepository) UpdateLastUsed(ctx context.Context, id string) error { +func (r *credentialRepository) RecordPasskeyUsed(ctx context.Context, id string) error { now := time.Now() - result := r.db.WithContext(ctx).Model(&model.Credential{}).Where("id = ?", id).Update("last_used_at", now) + result := r.db.WithContext(ctx).Model(&model.Credential{}). + Where("id = ? AND type = ?", id, model.CredentialTypeAppPasskey). + Update("last_used_at", now) if result.Error != nil { return result.Error } + if result.RowsAffected == 0 { + return model.ErrNotFound + } return nil } -func (r *credentialRepository) Delete(ctx context.Context, id string) error { - result := r.db.WithContext(ctx).Delete(&model.Credential{}, "id = ?", id) +func (r *credentialRepository) RevokeOwnedPasskey(ctx context.Context, userID, id string) error { + result := r.db.WithContext(ctx).Delete(&model.Credential{}, "id = ? AND user_id = ? AND type = ?", id, userID, model.CredentialTypeAppPasskey) if result.Error != nil { return result.Error } + if result.RowsAffected == 0 { + return model.ErrNotFound + } return nil } diff --git a/internal/repository/credential_test.go b/internal/repository/credential_test.go index e7354f5..29efe94 100644 --- a/internal/repository/credential_test.go +++ b/internal/repository/credential_test.go @@ -24,6 +24,12 @@ func setupCredentialRepo(t *testing.T) CredentialRepository { return NewCredentialRepository(db) } +func createPasskeyRecord(ctx context.Context, repo CredentialRepository, cred *model.Credential) error { + return repo.CreatePasskey(ctx, PasskeyParams{ + ID: cred.ID, UserID: cred.UserID, Label: cred.Label, SecretHash: cred.SecretHash, + }) +} + func TestCredentialRepository_Create(t *testing.T) { repo := setupCredentialRepo(t) ctx := context.Background() @@ -36,7 +42,7 @@ func TestCredentialRepository_Create(t *testing.T) { SecretHash: "hash-abc", } - if err := repo.Create(ctx, cred); err != nil { + if err := createPasskeyRecord(ctx, repo, cred); err != nil { t.Fatalf("Create = %v", err) } } @@ -48,11 +54,11 @@ func TestCredentialRepository_CreateDuplicateHash(t *testing.T) { c1 := &model.Credential{ID: "cred-1", UserID: "user-1", Type: "app_passkey", Label: "A", SecretHash: "hash-abc"} c2 := &model.Credential{ID: "cred-2", UserID: "user-1", Type: "app_passkey", Label: "B", SecretHash: "hash-abc"} - if err := repo.Create(ctx, c1); err != nil { + if err := createPasskeyRecord(ctx, repo, c1); err != nil { t.Fatalf("Create = %v", err) } - err := repo.Create(ctx, c2) + err := createPasskeyRecord(ctx, repo, c2) if err != model.ErrDuplicate { t.Fatalf("expected ErrDuplicate, got %v", err) } @@ -63,11 +69,11 @@ func TestCredentialRepository_FindByID(t *testing.T) { ctx := context.Background() cred := &model.Credential{ID: "cred-1", UserID: "user-1", Type: "app_passkey", Label: "Phone", SecretHash: "h1"} - if err := repo.Create(ctx, cred); err != nil { + if err := createPasskeyRecord(ctx, repo, cred); err != nil { t.Fatalf("Create = %v", err) } - found, err := repo.FindByID(ctx, "cred-1") + found, err := repo.FindPasskeyByID(ctx, "cred-1") if err != nil { t.Fatalf("FindByID = %v", err) } @@ -80,13 +86,13 @@ func TestCredentialRepository_FindByIDNotFound(t *testing.T) { repo := setupCredentialRepo(t) ctx := context.Background() - _, err := repo.FindByID(ctx, "nonexistent") + _, err := repo.FindPasskeyByID(ctx, "nonexistent") if err != model.ErrNotFound { t.Fatalf("expected ErrNotFound, got %v", err) } } -func TestCredentialRepository_FindByUserID(t *testing.T) { +func TestCredentialRepository_ListPasskeys(t *testing.T) { repo := setupCredentialRepo(t) ctx := context.Background() @@ -95,12 +101,12 @@ func TestCredentialRepository_FindByUserID(t *testing.T) { c3 := &model.Credential{ID: "c-3", UserID: "user-2", Type: "app_passkey", Label: "C", SecretHash: "h3"} for _, c := range []*model.Credential{c1, c2, c3} { - if err := repo.Create(ctx, c); err != nil { + if err := createPasskeyRecord(ctx, repo, c); err != nil { t.Fatalf("Create = %v", err) } } - creds, err := repo.FindByUserID(ctx, "user-1") + creds, err := repo.ListPasskeys(ctx, "user-1") if err != nil { t.Fatalf("FindByUserID = %v", err) } @@ -109,28 +115,24 @@ func TestCredentialRepository_FindByUserID(t *testing.T) { } } -func TestCredentialRepository_FindByUserIDAndType(t *testing.T) { +func TestCredentialRepository_CreatePasskeyControlsType(t *testing.T) { repo := setupCredentialRepo(t) ctx := context.Background() - c1 := &model.Credential{ID: "c-1", UserID: "user-1", Type: "app_passkey", Label: "A", SecretHash: "h1"} - c2 := &model.Credential{ID: "c-2", UserID: "user-1", Type: "oauth", Label: "Github", SecretHash: "h2"} - - for _, c := range []*model.Credential{c1, c2} { - if err := repo.Create(ctx, c); err != nil { - t.Fatalf("Create = %v", err) - } + cred := &model.Credential{ID: "c-1", UserID: "user-1", Label: "A", SecretHash: "h1"} + if err := createPasskeyRecord(ctx, repo, cred); err != nil { + t.Fatalf("Create = %v", err) } - passkeys, err := repo.FindByUserIDAndType(ctx, "user-1", "app_passkey") + passkeys, err := repo.ListPasskeys(ctx, "user-1") if err != nil { - t.Fatalf("FindByUserIDAndType = %v", err) + t.Fatalf("ListPasskeys = %v", err) } if len(passkeys) != 1 { t.Errorf("len(passkeys) = %d, want 1", len(passkeys)) } - if passkeys[0].Type != "app_passkey" { - t.Errorf("type = %q, want %q", passkeys[0].Type, "app_passkey") + if passkeys[0].Type != model.CredentialTypeAppPasskey { + t.Errorf("type = %q, want %q", passkeys[0].Type, model.CredentialTypeAppPasskey) } } @@ -139,11 +141,11 @@ func TestCredentialRepository_FindByHash(t *testing.T) { ctx := context.Background() cred := &model.Credential{ID: "c-1", UserID: "user-1", Type: "app_passkey", Label: "Phone", SecretHash: "hash-find"} - if err := repo.Create(ctx, cred); err != nil { + if err := createPasskeyRecord(ctx, repo, cred); err != nil { t.Fatalf("Create = %v", err) } - found, err := repo.FindByHash(ctx, "hash-find") + found, err := repo.FindPasskeyByHash(ctx, "hash-find") if err != nil { t.Fatalf("FindByHash = %v", err) } @@ -157,15 +159,15 @@ func TestCredentialRepository_UpdateLastUsed(t *testing.T) { ctx := context.Background() cred := &model.Credential{ID: "c-1", UserID: "user-1", Type: "app_passkey", Label: "Phone", SecretHash: "h1"} - if err := repo.Create(ctx, cred); err != nil { + if err := createPasskeyRecord(ctx, repo, cred); err != nil { t.Fatalf("Create = %v", err) } - if err := repo.UpdateLastUsed(ctx, "c-1"); err != nil { + if err := repo.RecordPasskeyUsed(ctx, "c-1"); err != nil { t.Fatalf("UpdateLastUsed = %v", err) } - found, err := repo.FindByID(ctx, "c-1") + found, err := repo.FindPasskeyByID(ctx, "c-1") if err != nil { t.Fatalf("FindByID = %v", err) } @@ -179,16 +181,35 @@ func TestCredentialRepository_Delete(t *testing.T) { ctx := context.Background() cred := &model.Credential{ID: "c-1", UserID: "user-1", Type: "app_passkey", Label: "Phone", SecretHash: "h1"} - if err := repo.Create(ctx, cred); err != nil { + if err := createPasskeyRecord(ctx, repo, cred); err != nil { t.Fatalf("Create = %v", err) } - if err := repo.Delete(ctx, "c-1"); err != nil { + if err := repo.RevokeOwnedPasskey(ctx, "user-1", "c-1"); err != nil { t.Fatalf("Delete = %v", err) } - _, err := repo.FindByID(ctx, "c-1") + _, err := repo.FindPasskeyByID(ctx, "c-1") if err != model.ErrNotFound { t.Fatalf("expected ErrNotFound after delete, got %v", err) } } + +func TestCredentialRepository_RevokeOwnedPasskeyEnforcesOwner(t *testing.T) { + repo := setupCredentialRepo(t) + ctx := context.Background() + cred := &model.Credential{ID: "c-1", UserID: "user-1", Label: "Phone", SecretHash: "h1"} + if err := createPasskeyRecord(ctx, repo, cred); err != nil { + t.Fatalf("Create = %v", err) + } + + if err := repo.RevokeOwnedPasskey(ctx, "user-2", cred.ID); err != model.ErrNotFound { + t.Fatalf("cross-owner revoke = %v, want ErrNotFound", err) + } + if _, err := repo.FindPasskeyByID(ctx, cred.ID); err != nil { + t.Fatalf("cross-owner revoke removed passkey: %v", err) + } + if err := repo.RevokeOwnedPasskey(ctx, cred.UserID, cred.ID); err != nil { + t.Fatalf("owner revoke = %v", err) + } +} diff --git a/internal/repository/db.go b/internal/repository/db.go index b7cfd12..83a2b54 100644 --- a/internal/repository/db.go +++ b/internal/repository/db.go @@ -2,8 +2,10 @@ package repository import ( "fmt" + "net/url" "os" "path/filepath" + "strings" "gorm.io/driver/postgres" "gorm.io/driver/sqlite" @@ -23,7 +25,7 @@ func Open(cfg config.DatabaseConfig) (*gorm.DB, error) { if err := os.MkdirAll(dir, 0755); err != nil { return nil, fmt.Errorf("create db directory: %w", err) } - dialector = sqlite.Open(cfg.SQLite.Path) + dialector = sqlite.Open(sqliteImmediateDSN(cfg.SQLite.Path)) case "postgres": dsn := fmt.Sprintf( "host=%s user=%s password=%s dbname=%s port=%d sslmode=%s", @@ -47,6 +49,20 @@ func Open(cfg config.DatabaseConfig) (*gorm.DB, error) { return db, nil } +func sqliteImmediateDSN(path string) string { + base, rawQuery, found := strings.Cut(path, "?") + query, err := url.ParseQuery(rawQuery) + if err != nil { + separator := "?" + if found { + separator = "&" + } + return path + separator + "_txlock=immediate" + } + query.Set("_txlock", "immediate") + return base + "?" + query.Encode() +} + // AutoMigrate runs schema migration for all domain models. func AutoMigrate(db *gorm.DB) error { return db.AutoMigrate( diff --git a/internal/repository/db_test.go b/internal/repository/db_test.go index b5cbc30..8dbe9cc 100644 --- a/internal/repository/db_test.go +++ b/internal/repository/db_test.go @@ -1,11 +1,31 @@ package repository import ( + "net/url" + "strings" "testing" "github.com/dhao2001/mygo/internal/config" ) +func TestSQLiteImmediateDSNPreservesParameters(t *testing.T) { + dsn := sqliteImmediateDSN("file:test.db?cache=shared&_busy_timeout=9000&_txlock=deferred") + base, rawQuery, found := strings.Cut(dsn, "?") + if !found || base != "file:test.db" { + t.Fatalf("DSN base = %q, want file:test.db", base) + } + query, err := url.ParseQuery(rawQuery) + if err != nil { + t.Fatalf("parse DSN query: %v", err) + } + if query.Get("cache") != "shared" || query.Get("_busy_timeout") != "9000" { + t.Fatalf("DSN did not preserve parameters: %q", dsn) + } + if query.Get("_txlock") != "immediate" { + t.Fatalf("_txlock = %q, want immediate", query.Get("_txlock")) + } +} + func TestOpenSQLite(t *testing.T) { cfg := config.DatabaseConfig{ Driver: "sqlite3", diff --git a/internal/repository/file.go b/internal/repository/file.go index fb916d2..1f532c4 100644 --- a/internal/repository/file.go +++ b/internal/repository/file.go @@ -3,22 +3,54 @@ package repository import ( "context" "errors" + "sort" "gorm.io/gorm" + "gorm.io/gorm/clause" "github.com/dhao2001/mygo/internal/model" ) +// UploadedFileParams contains persisted metadata for a completed upload. +type UploadedFileParams struct { + ID string + UserID string + ParentID *string + Name string + Size int64 + MimeType string + StoragePath string + Hash string +} + +// DirectoryParams contains the fields accepted when creating a directory. +type DirectoryParams struct { + ID string + UserID string + ParentID *string + Name string +} + +// FileMetadataUpdate contains the user-editable metadata fields. +// Nil fields are left unchanged. +type FileMetadataUpdate struct { + FileID string + UserID string + NewName *string + NewParentID *string +} + // FileRepository provides access to file records. type FileRepository interface { - Create(ctx context.Context, file *model.File) error + CreateUploadedFile(ctx context.Context, params UploadedFileParams) (*model.File, error) + CreateDirectory(ctx context.Context, params DirectoryParams) (*model.File, error) FindByID(ctx context.Context, id string) (*model.File, error) FindByUserID(ctx context.Context, userID string, offset, limit int) ([]model.File, int64, error) FindByParentID(ctx context.Context, userID string, parentID *string) ([]model.File, error) FindByParentIDPaginated(ctx context.Context, userID string, parentID *string, offset, limit int) ([]model.File, int64, error) FindByNameAndParent(ctx context.Context, userID string, parentID *string, name string) (*model.File, error) - Update(ctx context.Context, file *model.File) error - Delete(ctx context.Context, id string) error + UpdateOwnedMetadata(ctx context.Context, params FileMetadataUpdate) (*model.File, error) + SoftDeleteOwned(ctx context.Context, userID, fileID string) error } type fileRepository struct { @@ -30,15 +62,65 @@ func NewFileRepository(db *gorm.DB) FileRepository { return &fileRepository{db: db} } -func (r *fileRepository) Create(ctx context.Context, file *model.File) error { - result := r.db.WithContext(ctx).Create(file) - if result.Error != nil { - if isDuplicateKeyError(result.Error) { - return model.ErrDuplicate - } - return result.Error +func (r *fileRepository) CreateUploadedFile(ctx context.Context, params UploadedFileParams) (*model.File, error) { + file := &model.File{ + ID: params.ID, + UserID: params.UserID, + ParentID: params.ParentID, + Name: params.Name, + Size: params.Size, + MimeType: params.MimeType, + StoragePath: params.StoragePath, + Hash: params.Hash, + Status: model.StatusActive, + IsDir: false, } - return nil + if err := r.createEntry(ctx, file); err != nil { + return nil, err + } + return file, nil +} + +func (r *fileRepository) CreateDirectory(ctx context.Context, params DirectoryParams) (*model.File, error) { + dir := &model.File{ + ID: params.ID, + UserID: params.UserID, + ParentID: params.ParentID, + Name: params.Name, + Status: model.StatusActive, + IsDir: true, + } + if err := r.createEntry(ctx, dir); err != nil { + return nil, err + } + return dir, nil +} + +func (r *fileRepository) createEntry(ctx context.Context, file *model.File) error { + return r.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error { + if file.ParentID != nil { + locked, err := lockActiveOwnedFiles(tx, file.UserID, []string{*file.ParentID}) + if err != nil { + return err + } + parent, ok := locked[*file.ParentID] + if !ok { + return model.ErrParentNotFound + } + if !parent.IsDir { + return model.ErrParentNotDir + } + } + + result := tx.Create(file) + if result.Error != nil { + if isDuplicateKeyError(result.Error) { + return model.ErrDuplicate + } + return result.Error + } + return nil + }) } func (r *fileRepository) FindByID(ctx context.Context, id string) (*model.File, error) { @@ -106,24 +188,141 @@ func (r *fileRepository) FindByNameAndParent(ctx context.Context, userID string, return &file, nil } -func (r *fileRepository) Update(ctx context.Context, file *model.File) error { - result := r.db.WithContext(ctx).Save(file) - if result.Error != nil { +func (r *fileRepository) UpdateOwnedMetadata(ctx context.Context, params FileMetadataUpdate) (*model.File, error) { + var updated model.File + err := r.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error { + ids := []string{params.FileID} + if params.NewParentID != nil { + if *params.NewParentID == params.FileID { + return model.ErrInvalidMove + } + ids = append(ids, *params.NewParentID) + } + + locked, err := lockActiveOwnedFiles(tx, params.UserID, ids) + if err != nil { + return err + } + source, ok := locked[params.FileID] + if !ok { + return model.ErrNotFound + } + + updates := make(map[string]any, 2) + finalName := source.Name + finalParentID := source.ParentID + if params.NewName != nil { + updates["name"] = *params.NewName + finalName = *params.NewName + } + if params.NewParentID != nil { + parent, ok := locked[*params.NewParentID] + if !ok { + return model.ErrParentNotFound + } + if !parent.IsDir { + return model.ErrParentNotDir + } + updates["parent_id"] = *params.NewParentID + finalParentID = params.NewParentID + } + + if len(updates) > 0 { + var conflictCount int64 + if err := tx.Model(&model.File{}). + Where("user_id = ? AND parent_id IS ? AND name = ? AND status = ? AND id <> ?", params.UserID, finalParentID, finalName, model.StatusActive, params.FileID). + Count(&conflictCount).Error; err != nil { + return err + } + if conflictCount > 0 { + return model.ErrDuplicate + } + + result := tx.Model(&model.File{}). + Where("id = ? AND user_id = ? AND status = ?", params.FileID, params.UserID, model.StatusActive). + Updates(updates) + if result.Error != nil { + if isDuplicateKeyError(result.Error) { + return model.ErrDuplicate + } + return result.Error + } + if result.RowsAffected == 0 { + return model.ErrNotFound + } + } + + result := tx.First(&updated, "id = ? AND user_id = ? AND status = ?", source.ID, params.UserID, model.StatusActive) + if errors.Is(result.Error, gorm.ErrRecordNotFound) { + return model.ErrNotFound + } return result.Error + }) + if err != nil { + return nil, err } - return nil + return &updated, nil } -func (r *fileRepository) Delete(ctx context.Context, id string) error { - result := r.db.WithContext(ctx). - Model(&model.File{}). - Where("id = ? AND status = ?", id, model.StatusActive). - Update("status", model.StatusUserDeleted) - if result.Error != nil { - return result.Error - } - if result.RowsAffected == 0 { - return model.ErrNotFound - } - return nil +func (r *fileRepository) SoftDeleteOwned(ctx context.Context, userID, fileID string) error { + return r.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error { + locked, err := lockActiveOwnedFiles(tx, userID, []string{fileID}) + if err != nil { + return err + } + file, ok := locked[fileID] + if !ok { + return model.ErrNotFound + } + + if file.IsDir { + var count int64 + if err := tx.Model(&model.File{}). + Where("parent_id = ? AND status = ?", fileID, model.StatusActive). + Count(&count).Error; err != nil { + return err + } + if count > 0 { + return model.ErrDirectoryNotEmpty + } + } + + result := tx.Model(&model.File{}). + Where("id = ? AND user_id = ? AND status = ?", fileID, userID, model.StatusActive). + Update("status", model.StatusUserDeleted) + if result.Error != nil { + return result.Error + } + if result.RowsAffected == 0 { + return model.ErrNotFound + } + return nil + }) +} + +func lockActiveOwnedFiles(tx *gorm.DB, userID string, ids []string) (map[string]model.File, error) { + unique := make(map[string]struct{}, len(ids)) + for _, id := range ids { + unique[id] = struct{}{} + } + ordered := make([]string, 0, len(unique)) + for id := range unique { + ordered = append(ordered, id) + } + sort.Strings(ordered) + + var files []model.File + result := tx.Clauses(clause.Locking{Strength: clause.LockingStrengthUpdate}). + Where("id IN ? AND user_id = ? AND status = ?", ordered, userID, model.StatusActive). + Order("id ASC"). + Find(&files) + if result.Error != nil { + return nil, result.Error + } + + locked := make(map[string]model.File, len(files)) + for i := range files { + locked[files[i].ID] = files[i] + } + return locked, nil } diff --git a/internal/repository/file_test.go b/internal/repository/file_test.go index 581245e..62e414e 100644 --- a/internal/repository/file_test.go +++ b/internal/repository/file_test.go @@ -3,11 +3,14 @@ package repository import ( "context" "errors" + "path/filepath" + "sync" "testing" "gorm.io/driver/sqlite" "gorm.io/gorm" + "github.com/dhao2001/mygo/internal/config" "github.com/dhao2001/mygo/internal/model" ) @@ -25,6 +28,48 @@ func setupFileRepo(t *testing.T) FileRepository { return NewFileRepository(db) } +func createFileRecord(ctx context.Context, repo FileRepository, file *model.File) error { + var err error + if file.IsDir { + _, err = repo.CreateDirectory(ctx, DirectoryParams{ + ID: file.ID, UserID: file.UserID, ParentID: file.ParentID, Name: file.Name, + }) + } else { + _, err = repo.CreateUploadedFile(ctx, UploadedFileParams{ + ID: file.ID, UserID: file.UserID, ParentID: file.ParentID, Name: file.Name, + Size: file.Size, MimeType: file.MimeType, StoragePath: file.StoragePath, Hash: file.Hash, + }) + } + if err != nil { + return err + } + if file.Status == model.StatusUserDeleted { + return repo.SoftDeleteOwned(ctx, file.UserID, file.ID) + } + return nil +} + +func setupConcurrentFileRepo(t *testing.T) FileRepository { + t.Helper() + db, err := Open(config.DatabaseConfig{ + Driver: "sqlite3", + SQLite: config.SQLiteConfig{Path: filepath.Join(t.TempDir(), "concurrency.db")}, + }) + if err != nil { + t.Fatalf("open db: %v", err) + } + if err := AutoMigrate(db); err != nil { + t.Fatalf("migrate: %v", err) + } + t.Cleanup(func() { + sqlDB, err := db.DB() + if err == nil { + _ = sqlDB.Close() + } + }) + return NewFileRepository(db) +} + func TestFileRepository_Create(t *testing.T) { repo := setupFileRepo(t) ctx := context.Background() @@ -37,7 +82,7 @@ func TestFileRepository_Create(t *testing.T) { Status: model.StatusActive, } - if err := repo.Create(ctx, file); err != nil { + if err := createFileRecord(ctx, repo, file); err != nil { t.Fatalf("Create = %v", err) } } @@ -52,7 +97,7 @@ func TestFileRepository_FindByID(t *testing.T) { Name: "test.txt", Status: model.StatusActive, } - if err := repo.Create(ctx, file); err != nil { + if err := createFileRecord(ctx, repo, file); err != nil { t.Fatalf("Create = %v", err) } @@ -85,7 +130,7 @@ func TestFileRepository_FindByUserID(t *testing.T) { {ID: "f-3", UserID: "user-2", Name: "c.txt", Status: model.StatusActive}, } for _, f := range files { - if err := repo.Create(ctx, f); err != nil { + if err := createFileRecord(ctx, repo, f); err != nil { t.Fatalf("Create = %v", err) } } @@ -107,13 +152,16 @@ func TestFileRepository_FindByParentID(t *testing.T) { ctx := context.Background() parentID := "dir-1" + if err := createFileRecord(ctx, repo, &model.File{ID: parentID, UserID: "user-1", Name: "dir", Status: model.StatusActive, IsDir: true}); err != nil { + t.Fatalf("Create parent = %v", err) + } files := []*model.File{ {ID: "f-1", UserID: "user-1", ParentID: &parentID, Name: "a.txt", Status: model.StatusActive}, {ID: "f-2", UserID: "user-1", ParentID: &parentID, Name: "b.txt", Status: model.StatusActive}, {ID: "f-3", UserID: "user-1", Name: "c.txt", Status: model.StatusActive}, } for _, f := range files { - if err := repo.Create(ctx, f); err != nil { + if err := createFileRecord(ctx, repo, f); err != nil { t.Fatalf("Create = %v", err) } } @@ -132,12 +180,15 @@ func TestFileRepository_FindByParentIDNull(t *testing.T) { ctx := context.Background() parentID := "dir-1" + if err := createFileRecord(ctx, repo, &model.File{ID: parentID, UserID: "user-1", Name: "dir", Status: model.StatusActive, IsDir: true}); err != nil { + t.Fatalf("Create parent = %v", err) + } files := []*model.File{ {ID: "f-1", UserID: "user-1", ParentID: &parentID, Name: "a.txt", Status: model.StatusActive}, {ID: "f-2", UserID: "user-1", Name: "root.txt", Status: model.StatusActive}, } for _, f := range files { - if err := repo.Create(ctx, f); err != nil { + if err := createFileRecord(ctx, repo, f); err != nil { t.Fatalf("Create = %v", err) } } @@ -146,8 +197,8 @@ func TestFileRepository_FindByParentIDNull(t *testing.T) { if err != nil { t.Fatalf("FindByParentID(nil) = %v", err) } - if len(children) != 1 { - t.Errorf("len(children) = %d, want 1", len(children)) + if len(children) != 2 { + t.Errorf("len(children) = %d, want 2", len(children)) } } @@ -155,14 +206,26 @@ func TestFileRepository_Update(t *testing.T) { repo := setupFileRepo(t) ctx := context.Background() - file := &model.File{ID: "file-1", UserID: "user-1", Name: "original.txt", Status: model.StatusActive} - if err := repo.Create(ctx, file); err != nil { + file := &model.File{ + ID: "file-1", + UserID: "user-1", + Name: "original.txt", + Size: 1024, + MimeType: "text/plain", + StoragePath: "data/user-1/file-1", + Hash: "original-hash", + Status: model.StatusActive, + } + if err := createFileRecord(ctx, repo, file); err != nil { t.Fatalf("Create = %v", err) } + before, err := repo.FindByID(ctx, file.ID) + if err != nil { + t.Fatalf("FindByID before update = %v", err) + } - file.Name = "renamed.txt" - file.Size = 2048 - if err := repo.Update(ctx, file); err != nil { + newName := "renamed.txt" + if _, err := repo.UpdateOwnedMetadata(ctx, FileMetadataUpdate{FileID: file.ID, UserID: file.UserID, NewName: &newName}); err != nil { t.Fatalf("Update = %v", err) } @@ -173,8 +236,11 @@ func TestFileRepository_Update(t *testing.T) { if found.Name != "renamed.txt" { t.Errorf("name = %q, want %q", found.Name, "renamed.txt") } - if found.Size != 2048 { - t.Errorf("size = %d, want %d", found.Size, 2048) + if found.UserID != before.UserID || found.Status != before.Status || found.IsDir != before.IsDir || + found.Size != before.Size || found.MimeType != before.MimeType || + found.StoragePath != before.StoragePath || found.Hash != before.Hash || + !found.CreatedAt.Equal(before.CreatedAt) { + t.Fatalf("metadata update changed protected fields: before=%+v after=%+v", before, found) } } @@ -183,11 +249,11 @@ func TestFileRepository_Delete(t *testing.T) { ctx := context.Background() file := &model.File{ID: "file-1", UserID: "user-1", Name: "test.txt", Status: model.StatusActive} - if err := repo.Create(ctx, file); err != nil { + if err := createFileRecord(ctx, repo, file); err != nil { t.Fatalf("Create = %v", err) } - if err := repo.Delete(ctx, "file-1"); err != nil { + if err := repo.SoftDeleteOwned(ctx, "user-1", "file-1"); err != nil { t.Fatalf("Delete = %v", err) } @@ -207,11 +273,11 @@ func TestFileRepository_SoftDelete(t *testing.T) { Name: "test.txt", Status: model.StatusActive, } - if err := repo.Create(ctx, file); err != nil { + if err := createFileRecord(ctx, repo, file); err != nil { t.Fatalf("Create = %v", err) } - if err := repo.Delete(ctx, "file-1"); err != nil { + if err := repo.SoftDeleteOwned(ctx, "user-1", "file-1"); err != nil { t.Fatalf("Delete = %v", err) } @@ -238,10 +304,10 @@ func TestFileRepository_StatusFilter(t *testing.T) { Status: model.StatusUserDeleted, } - if err := repo.Create(ctx, activeFile); err != nil { + if err := createFileRecord(ctx, repo, activeFile); err != nil { t.Fatalf("Create active = %v", err) } - if err := repo.Create(ctx, deletedFile); err != nil { + if err := createFileRecord(ctx, repo, deletedFile); err != nil { t.Fatalf("Create deleted = %v", err) } @@ -270,17 +336,17 @@ func TestFileRepository_DeleteReturnsNotFoundAfterDeletion(t *testing.T) { Name: "test.txt", Status: model.StatusActive, } - if err := repo.Create(ctx, file); err != nil { + if err := createFileRecord(ctx, repo, file); err != nil { t.Fatalf("Create = %v", err) } - if err := repo.Delete(ctx, "file-1"); err != nil { + if err := repo.SoftDeleteOwned(ctx, "user-1", "file-1"); err != nil { t.Fatalf("first Delete = %v", err) } - if err := repo.Delete(ctx, "file-1"); !errors.Is(err, model.ErrNotFound) { + if err := repo.SoftDeleteOwned(ctx, "user-1", "file-1"); !errors.Is(err, model.ErrNotFound) { t.Fatalf("second Delete = %v, want ErrNotFound", err) } - if err := repo.Delete(ctx, "missing-file"); !errors.Is(err, model.ErrNotFound) { + if err := repo.SoftDeleteOwned(ctx, "user-1", "missing-file"); !errors.Is(err, model.ErrNotFound) { t.Fatalf("missing Delete = %v, want ErrNotFound", err) } } @@ -302,10 +368,10 @@ func TestFileRepository_StatusFilterCount(t *testing.T) { Status: model.StatusUserDeleted, } - if err := repo.Create(ctx, activeFile); err != nil { + if err := createFileRecord(ctx, repo, activeFile); err != nil { t.Fatalf("Create active = %v", err) } - if err := repo.Create(ctx, deletedFile); err != nil { + if err := createFileRecord(ctx, repo, deletedFile); err != nil { t.Fatalf("Create deleted = %v", err) } @@ -317,3 +383,160 @@ func TestFileRepository_StatusFilterCount(t *testing.T) { t.Errorf("total = %d, want 1 (soft-deleted excluded from count)", total) } } + +func TestFileRepository_UpdateCannotReviveDeletedFile(t *testing.T) { + repo := setupFileRepo(t) + ctx := context.Background() + file := &model.File{ID: "file-1", UserID: "user-1", Name: "before.txt", Status: model.StatusActive} + if err := createFileRecord(ctx, repo, file); err != nil { + t.Fatalf("Create = %v", err) + } + if err := repo.SoftDeleteOwned(ctx, file.UserID, file.ID); err != nil { + t.Fatalf("SoftDeleteOwned = %v", err) + } + + newName := "after.txt" + _, err := repo.UpdateOwnedMetadata(ctx, FileMetadataUpdate{FileID: file.ID, UserID: file.UserID, NewName: &newName}) + if !errors.Is(err, model.ErrNotFound) { + t.Fatalf("UpdateOwnedMetadata = %v, want ErrNotFound", err) + } + if _, err := repo.FindByID(ctx, file.ID); !errors.Is(err, model.ErrNotFound) { + t.Fatalf("deleted file became active again: %v", err) + } +} + +func TestFileRepository_ConcurrentUpdateAndDeleteCannotReviveFile(t *testing.T) { + repo := setupConcurrentFileRepo(t) + ctx := context.Background() + if _, err := repo.CreateUploadedFile(ctx, UploadedFileParams{ + ID: "file-1", UserID: "user-1", Name: "before.txt", StoragePath: "data/user-1/file-1", + }); err != nil { + t.Fatalf("CreateUploadedFile = %v", err) + } + + newName := "after.txt" + start := make(chan struct{}) + var wg sync.WaitGroup + wg.Add(2) + var updateErr, deleteErr error + go func() { + defer wg.Done() + <-start + _, updateErr = repo.UpdateOwnedMetadata(ctx, FileMetadataUpdate{ + FileID: "file-1", UserID: "user-1", NewName: &newName, + }) + }() + go func() { + defer wg.Done() + <-start + deleteErr = repo.SoftDeleteOwned(ctx, "user-1", "file-1") + }() + close(start) + wg.Wait() + + if deleteErr != nil { + t.Fatalf("SoftDeleteOwned = %v, want success", deleteErr) + } + if updateErr != nil && !errors.Is(updateErr, model.ErrNotFound) { + t.Fatalf("UpdateOwnedMetadata = %v, want success or ErrNotFound", updateErr) + } + if _, err := repo.FindByID(ctx, "file-1"); !errors.Is(err, model.ErrNotFound) { + t.Fatalf("concurrent update revived deleted file: %v", err) + } +} + +func TestFileRepository_CreateAndParentDeleteAreSerialized(t *testing.T) { + repo := setupConcurrentFileRepo(t) + ctx := context.Background() + dir, err := repo.CreateDirectory(ctx, DirectoryParams{ID: "dir-1", UserID: "user-1", Name: "dir"}) + if err != nil { + t.Fatalf("CreateDirectory = %v", err) + } + + start := make(chan struct{}) + var wg sync.WaitGroup + wg.Add(2) + var createErr, deleteErr error + go func() { + defer wg.Done() + <-start + _, createErr = repo.CreateUploadedFile(ctx, UploadedFileParams{ + ID: "child-1", UserID: "user-1", ParentID: &dir.ID, Name: "child.txt", StoragePath: "data/user-1/child-1", + }) + }() + go func() { + defer wg.Done() + <-start + deleteErr = repo.SoftDeleteOwned(ctx, "user-1", dir.ID) + }() + close(start) + wg.Wait() + + switch { + case createErr == nil: + if !errors.Is(deleteErr, model.ErrDirectoryNotEmpty) { + t.Fatalf("create succeeded but delete error = %v, want ErrDirectoryNotEmpty", deleteErr) + } + if _, err := repo.FindByID(ctx, dir.ID); err != nil { + t.Fatalf("parent missing after child creation: %v", err) + } + case deleteErr == nil: + if !errors.Is(createErr, model.ErrParentNotFound) { + t.Fatalf("delete succeeded but create error = %v, want ErrParentNotFound", createErr) + } + if _, err := repo.FindByID(ctx, "child-1"); !errors.Is(err, model.ErrNotFound) { + t.Fatalf("active orphan child exists: %v", err) + } + default: + t.Fatalf("neither operation succeeded: create=%v delete=%v", createErr, deleteErr) + } +} + +func TestFileRepository_MoveAndTargetDeleteAreSerialized(t *testing.T) { + repo := setupConcurrentFileRepo(t) + ctx := context.Background() + target, err := repo.CreateDirectory(ctx, DirectoryParams{ID: "target-1", UserID: "user-1", Name: "target"}) + if err != nil { + t.Fatalf("CreateDirectory = %v", err) + } + if _, err := repo.CreateUploadedFile(ctx, UploadedFileParams{ID: "file-1", UserID: "user-1", Name: "file.txt", StoragePath: "data/user-1/file-1"}); err != nil { + t.Fatalf("CreateUploadedFile = %v", err) + } + + start := make(chan struct{}) + var wg sync.WaitGroup + wg.Add(2) + var moveErr, deleteErr error + go func() { + defer wg.Done() + <-start + _, moveErr = repo.UpdateOwnedMetadata(ctx, FileMetadataUpdate{FileID: "file-1", UserID: "user-1", NewParentID: &target.ID}) + }() + go func() { + defer wg.Done() + <-start + deleteErr = repo.SoftDeleteOwned(ctx, "user-1", target.ID) + }() + close(start) + wg.Wait() + + switch { + case moveErr == nil: + if !errors.Is(deleteErr, model.ErrDirectoryNotEmpty) { + t.Fatalf("move succeeded but delete error = %v, want ErrDirectoryNotEmpty", deleteErr) + } + case deleteErr == nil: + if !errors.Is(moveErr, model.ErrParentNotFound) { + t.Fatalf("delete succeeded but move error = %v, want ErrParentNotFound", moveErr) + } + file, err := repo.FindByID(ctx, "file-1") + if err != nil { + t.Fatalf("find source file: %v", err) + } + if file.ParentID != nil { + t.Fatalf("failed move changed source parent to %v", *file.ParentID) + } + default: + t.Fatalf("neither operation succeeded: move=%v delete=%v", moveErr, deleteErr) + } +} diff --git a/internal/repository/session.go b/internal/repository/session.go index 4346984..00a00e3 100644 --- a/internal/repository/session.go +++ b/internal/repository/session.go @@ -6,18 +6,33 @@ import ( "time" "gorm.io/gorm" + "gorm.io/gorm/clause" "github.com/dhao2001/mygo/internal/model" ) -// SessionRepository provides access to refresh token sessions. +// RefreshSessionParams contains the fields persisted for a refresh session. +type RefreshSessionParams struct { + ID string + UserID string + TokenHash string + ExpiresAt time.Time +} + +// AuthSessionRepository exposes only session operations required by AuthService. +type AuthSessionRepository interface { + CreateRefreshSession(ctx context.Context, params RefreshSessionParams) error + ConsumeRefreshSession(ctx context.Context, tokenHash string) (*model.Session, error) + RevokeRefreshSession(ctx context.Context, tokenHash string) error +} + +// SessionRepository is the composition-time union of session capabilities. type SessionRepository interface { - Create(ctx context.Context, session *model.Session) error + AuthSessionRepository FindByID(ctx context.Context, id string) (*model.Session, error) FindByTokenHash(ctx context.Context, tokenHash string) (*model.Session, error) - Delete(ctx context.Context, id string) error - DeleteByUserID(ctx context.Context, userID string) error - DeleteExpired(ctx context.Context) (int64, error) + DeleteSessionsByUserID(ctx context.Context, userID string) error + DeleteExpiredSessions(ctx context.Context) (int64, error) } type sessionRepository struct { @@ -29,7 +44,13 @@ func NewSessionRepository(db *gorm.DB) SessionRepository { return &sessionRepository{db: db} } -func (r *sessionRepository) Create(ctx context.Context, session *model.Session) error { +func (r *sessionRepository) CreateRefreshSession(ctx context.Context, params RefreshSessionParams) error { + session := &model.Session{ + ID: params.ID, + UserID: params.UserID, + TokenHash: params.TokenHash, + ExpiresAt: params.ExpiresAt, + } result := r.db.WithContext(ctx).Create(session) if result.Error != nil { if isDuplicateKeyError(result.Error) { @@ -64,15 +85,39 @@ func (r *sessionRepository) FindByTokenHash(ctx context.Context, tokenHash strin return &session, nil } -func (r *sessionRepository) Delete(ctx context.Context, id string) error { - result := r.db.WithContext(ctx).Delete(&model.Session{}, "id = ?", id) - if result.Error != nil { - return result.Error +func (r *sessionRepository) ConsumeRefreshSession(ctx context.Context, tokenHash string) (*model.Session, error) { + var consumed model.Session + err := r.db.WithContext(ctx).Transaction(func(tx *gorm.DB) error { + result := tx.Clauses(clause.Locking{Strength: clause.LockingStrengthUpdate}). + First(&consumed, "token_hash = ?", tokenHash) + if errors.Is(result.Error, gorm.ErrRecordNotFound) { + return model.ErrNotFound + } + if result.Error != nil { + return result.Error + } + + result = tx.Delete(&model.Session{}, "id = ?", consumed.ID) + if result.Error != nil { + return result.Error + } + if result.RowsAffected == 0 { + return model.ErrNotFound + } + return nil + }) + if err != nil { + return nil, err } - return nil + return &consumed, nil } -func (r *sessionRepository) DeleteByUserID(ctx context.Context, userID string) error { +func (r *sessionRepository) RevokeRefreshSession(ctx context.Context, tokenHash string) error { + result := r.db.WithContext(ctx).Delete(&model.Session{}, "token_hash = ?", tokenHash) + return result.Error +} + +func (r *sessionRepository) DeleteSessionsByUserID(ctx context.Context, userID string) error { result := r.db.WithContext(ctx).Delete(&model.Session{}, "user_id = ?", userID) if result.Error != nil { return result.Error @@ -80,7 +125,7 @@ func (r *sessionRepository) DeleteByUserID(ctx context.Context, userID string) e return nil } -func (r *sessionRepository) DeleteExpired(ctx context.Context) (int64, error) { +func (r *sessionRepository) DeleteExpiredSessions(ctx context.Context) (int64, error) { result := r.db.WithContext(ctx).Delete(&model.Session{}, "expires_at < ?", time.Now()) if result.Error != nil { return 0, result.Error diff --git a/internal/repository/session_test.go b/internal/repository/session_test.go index 7556778..22a7695 100644 --- a/internal/repository/session_test.go +++ b/internal/repository/session_test.go @@ -2,12 +2,16 @@ package repository import ( "context" + "errors" + "path/filepath" + "sync" "testing" "time" "gorm.io/driver/sqlite" "gorm.io/gorm" + "github.com/dhao2001/mygo/internal/config" "github.com/dhao2001/mygo/internal/model" ) @@ -25,6 +29,12 @@ func setupSessionRepo(t *testing.T) SessionRepository { return NewSessionRepository(db) } +func createSessionRecord(ctx context.Context, repo SessionRepository, session *model.Session) error { + return repo.CreateRefreshSession(ctx, RefreshSessionParams{ + ID: session.ID, UserID: session.UserID, TokenHash: session.TokenHash, ExpiresAt: session.ExpiresAt, + }) +} + func TestSessionRepository_Create(t *testing.T) { repo := setupSessionRepo(t) ctx := context.Background() @@ -36,7 +46,7 @@ func TestSessionRepository_Create(t *testing.T) { ExpiresAt: time.Now().Add(24 * time.Hour), } - if err := repo.Create(ctx, session); err != nil { + if err := createSessionRecord(ctx, repo, session); err != nil { t.Fatalf("Create = %v", err) } } @@ -48,11 +58,11 @@ func TestSessionRepository_CreateDuplicateHash(t *testing.T) { s1 := &model.Session{ID: "session-1", UserID: "user-1", TokenHash: "hash-abc", ExpiresAt: time.Now().Add(24 * time.Hour)} s2 := &model.Session{ID: "session-2", UserID: "user-2", TokenHash: "hash-abc", ExpiresAt: time.Now().Add(24 * time.Hour)} - if err := repo.Create(ctx, s1); err != nil { + if err := createSessionRecord(ctx, repo, s1); err != nil { t.Fatalf("Create = %v", err) } - err := repo.Create(ctx, s2) + err := createSessionRecord(ctx, repo, s2) if err != model.ErrDuplicate { t.Fatalf("expected ErrDuplicate, got %v", err) } @@ -63,7 +73,7 @@ func TestSessionRepository_FindByID(t *testing.T) { ctx := context.Background() session := &model.Session{ID: "session-1", UserID: "user-1", TokenHash: "hash-abc", ExpiresAt: time.Now().Add(24 * time.Hour)} - if err := repo.Create(ctx, session); err != nil { + if err := createSessionRecord(ctx, repo, session); err != nil { t.Fatalf("Create = %v", err) } @@ -81,7 +91,7 @@ func TestSessionRepository_FindByTokenHash(t *testing.T) { ctx := context.Background() session := &model.Session{ID: "session-1", UserID: "user-1", TokenHash: "hash-abc", ExpiresAt: time.Now().Add(24 * time.Hour)} - if err := repo.Create(ctx, session); err != nil { + if err := createSessionRecord(ctx, repo, session); err != nil { t.Fatalf("Create = %v", err) } @@ -109,15 +119,19 @@ func TestSessionRepository_Delete(t *testing.T) { ctx := context.Background() session := &model.Session{ID: "session-1", UserID: "user-1", TokenHash: "hash-abc", ExpiresAt: time.Now().Add(24 * time.Hour)} - if err := repo.Create(ctx, session); err != nil { + if err := createSessionRecord(ctx, repo, session); err != nil { t.Fatalf("Create = %v", err) } - if err := repo.Delete(ctx, "session-1"); err != nil { - t.Fatalf("Delete = %v", err) + consumed, err := repo.ConsumeRefreshSession(ctx, "hash-abc") + if err != nil { + t.Fatalf("ConsumeRefreshSession = %v", err) + } + if consumed.ID != "session-1" { + t.Fatalf("consumed ID = %q, want session-1", consumed.ID) } - _, err := repo.FindByID(ctx, "session-1") + _, err = repo.FindByID(ctx, "session-1") if err != model.ErrNotFound { t.Fatalf("expected ErrNotFound after delete, got %v", err) } @@ -132,12 +146,12 @@ func TestSessionRepository_DeleteByUserID(t *testing.T) { s3 := &model.Session{ID: "session-3", UserID: "user-2", TokenHash: "hash-3", ExpiresAt: time.Now().Add(24 * time.Hour)} for _, s := range []*model.Session{s1, s2, s3} { - if err := repo.Create(ctx, s); err != nil { + if err := createSessionRecord(ctx, repo, s); err != nil { t.Fatalf("Create = %v", err) } } - if err := repo.DeleteByUserID(ctx, "user-1"); err != nil { + if err := repo.DeleteSessionsByUserID(ctx, "user-1"); err != nil { t.Fatalf("DeleteByUserID = %v", err) } @@ -168,12 +182,12 @@ func TestSessionRepository_DeleteExpired(t *testing.T) { } for _, s := range []*model.Session{expired, valid} { - if err := repo.Create(ctx, s); err != nil { + if err := createSessionRecord(ctx, repo, s); err != nil { t.Fatalf("Create = %v", err) } } - count, err := repo.DeleteExpired(ctx) + count, err := repo.DeleteExpiredSessions(ctx) if err != nil { t.Fatalf("DeleteExpired = %v", err) } @@ -188,3 +202,55 @@ func TestSessionRepository_DeleteExpired(t *testing.T) { t.Fatalf("valid session should still exist: %v", err) } } + +func TestSessionRepository_ConsumeRefreshSessionIsAtomic(t *testing.T) { + db, err := Open(config.DatabaseConfig{ + Driver: "sqlite3", + SQLite: config.SQLiteConfig{Path: filepath.Join(t.TempDir(), "sessions.db")}, + }) + if err != nil { + t.Fatalf("open db: %v", err) + } + if err := AutoMigrate(db); err != nil { + t.Fatalf("migrate: %v", err) + } + repo := NewSessionRepository(db) + ctx := context.Background() + if err := repo.CreateRefreshSession(ctx, RefreshSessionParams{ + ID: "session-1", UserID: "user-1", TokenHash: "single-use", ExpiresAt: time.Now().Add(time.Hour), + }); err != nil { + t.Fatalf("CreateRefreshSession = %v", err) + } + + start := make(chan struct{}) + results := make(chan error, 2) + var wg sync.WaitGroup + wg.Add(2) + for range 2 { + go func() { + defer wg.Done() + <-start + _, err := repo.ConsumeRefreshSession(ctx, "single-use") + results <- err + }() + } + close(start) + wg.Wait() + close(results) + + succeeded := 0 + notFound := 0 + for err := range results { + switch { + case err == nil: + succeeded++ + case errors.Is(err, model.ErrNotFound): + notFound++ + default: + t.Fatalf("ConsumeRefreshSession unexpected error: %v", err) + } + } + if succeeded != 1 || notFound != 1 { + t.Fatalf("consume results: success=%d not_found=%d, want 1 each", succeeded, notFound) + } +} diff --git a/internal/repository/user.go b/internal/repository/user.go index 7be09f8..8332388 100644 --- a/internal/repository/user.go +++ b/internal/repository/user.go @@ -15,19 +15,37 @@ func isDuplicateKeyError(err error) bool { return errors.Is(err, gorm.ErrDuplicatedKey) || strings.Contains(strings.ToLower(err.Error()), "unique constraint failed") } -// UserRepository provides access to user records. -type UserRepository interface { - Create(ctx context.Context, user *model.User) error +// RegisteredUserParams contains the fields accepted when registering a user. +// Administrative and lifecycle fields are intentionally excluded. +type RegisteredUserParams struct { + ID string + Username string + Email string + PasswordHash string +} + +// AuthUserRepository exposes only user operations required by authentication. +type AuthUserRepository interface { + CreateRegisteredUser(ctx context.Context, params RegisteredUserParams) (*model.User, error) FindByID(ctx context.Context, id string) (*model.User, error) - FindByIDIncludeDeleted(ctx context.Context, id string) (*model.User, error) FindByEmail(ctx context.Context, email string) (*model.User, error) - FindByUsername(ctx context.Context, username string) (*model.User, error) - Update(ctx context.Context, user *model.User) error - Delete(ctx context.Context, id string) error - List(ctx context.Context, offset, limit int) ([]model.User, int64, error) +} + +// AdminUserRepository exposes only user operations required by administration. +type AdminUserRepository interface { + FindByIDIncludeDeleted(ctx context.Context, id string) (*model.User, error) + MarkAdminDeleted(ctx context.Context, id string) error ListIncludeDeleted(ctx context.Context, offset, limit int) ([]model.User, int64, error) } +// UserRepository is the composition-time union of user capabilities. +type UserRepository interface { + AuthUserRepository + AdminUserRepository + FindByUsername(ctx context.Context, username string) (*model.User, error) + List(ctx context.Context, offset, limit int) ([]model.User, int64, error) +} + type userRepository struct { db *gorm.DB } @@ -37,15 +55,23 @@ func NewUserRepository(db *gorm.DB) UserRepository { return &userRepository{db: db} } -func (r *userRepository) Create(ctx context.Context, user *model.User) error { +func (r *userRepository) CreateRegisteredUser(ctx context.Context, params RegisteredUserParams) (*model.User, error) { + user := &model.User{ + ID: params.ID, + Username: params.Username, + Email: params.Email, + PasswordHash: params.PasswordHash, + IsAdmin: false, + Status: model.StatusActive, + } result := r.db.WithContext(ctx).Create(user) if result.Error != nil { if isDuplicateKeyError(result.Error) { - return model.ErrDuplicate + return nil, model.ErrDuplicate } - return result.Error + return nil, result.Error } - return nil + return user, nil } func (r *userRepository) FindByID(ctx context.Context, id string) (*model.User, error) { @@ -97,18 +123,7 @@ func (r *userRepository) FindByUsername(ctx context.Context, username string) (* return &user, nil } -func (r *userRepository) Update(ctx context.Context, user *model.User) error { - result := r.db.WithContext(ctx).Save(user) - if result.Error != nil { - if isDuplicateKeyError(result.Error) { - return model.ErrDuplicate - } - return result.Error - } - return nil -} - -func (r *userRepository) Delete(ctx context.Context, id string) error { +func (r *userRepository) MarkAdminDeleted(ctx context.Context, id string) error { result := r.db.WithContext(ctx).Model(&model.User{}).Where("id = ?", id).Update("status", model.StatusAdminDeleted) if result.Error != nil { return result.Error diff --git a/internal/repository/user_test.go b/internal/repository/user_test.go index 2bbe34d..12a4c69 100644 --- a/internal/repository/user_test.go +++ b/internal/repository/user_test.go @@ -24,6 +24,16 @@ func setupUserRepo(t *testing.T) UserRepository { return NewUserRepository(db) } +func createUserRecord(ctx context.Context, repo UserRepository, user *model.User) error { + _, err := repo.CreateRegisteredUser(ctx, RegisteredUserParams{ + ID: user.ID, + Username: user.Username, + Email: user.Email, + PasswordHash: user.PasswordHash, + }) + return err +} + func TestUserRepository_Create(t *testing.T) { repo := setupUserRepo(t) ctx := context.Background() @@ -36,7 +46,7 @@ func TestUserRepository_Create(t *testing.T) { Status: model.StatusActive, } - if err := repo.Create(ctx, user); err != nil { + if err := createUserRecord(ctx, repo, user); err != nil { t.Fatalf("Create = %v", err) } } @@ -48,11 +58,11 @@ func TestUserRepository_CreateDuplicateUsername(t *testing.T) { u1 := &model.User{ID: "user-1", Username: "alice", Email: "alice@example.com", PasswordHash: "hash", Status: model.StatusActive} u2 := &model.User{ID: "user-2", Username: "alice", Email: "alice2@example.com", PasswordHash: "hash", Status: model.StatusActive} - if err := repo.Create(ctx, u1); err != nil { + if err := createUserRecord(ctx, repo, u1); err != nil { t.Fatalf("Create = %v", err) } - err := repo.Create(ctx, u2) + err := createUserRecord(ctx, repo, u2) if err != model.ErrDuplicate { t.Fatalf("expected ErrDuplicate, got %v", err) } @@ -63,7 +73,7 @@ func TestUserRepository_FindByID(t *testing.T) { ctx := context.Background() user := &model.User{ID: "user-1", Username: "alice", Email: "alice@example.com", PasswordHash: "hash", Status: model.StatusActive} - if err := repo.Create(ctx, user); err != nil { + if err := createUserRecord(ctx, repo, user); err != nil { t.Fatalf("Create = %v", err) } @@ -91,7 +101,7 @@ func TestUserRepository_FindByEmail(t *testing.T) { ctx := context.Background() user := &model.User{ID: "user-1", Username: "alice", Email: "alice@example.com", PasswordHash: "hash", Status: model.StatusActive} - if err := repo.Create(ctx, user); err != nil { + if err := createUserRecord(ctx, repo, user); err != nil { t.Fatalf("Create = %v", err) } @@ -109,7 +119,7 @@ func TestUserRepository_FindByUsername(t *testing.T) { ctx := context.Background() user := &model.User{ID: "user-1", Username: "alice", Email: "alice@example.com", PasswordHash: "hash", Status: model.StatusActive} - if err := repo.Create(ctx, user); err != nil { + if err := createUserRecord(ctx, repo, user); err != nil { t.Fatalf("Create = %v", err) } @@ -122,26 +132,24 @@ func TestUserRepository_FindByUsername(t *testing.T) { } } -func TestUserRepository_Update(t *testing.T) { +func TestUserRepository_CreateRegisteredUserControlsPrivilegedFields(t *testing.T) { repo := setupUserRepo(t) ctx := context.Background() user := &model.User{ID: "user-1", Username: "alice", Email: "alice@example.com", PasswordHash: "hash", Status: model.StatusActive} - if err := repo.Create(ctx, user); err != nil { + if err := createUserRecord(ctx, repo, user); err != nil { t.Fatalf("Create = %v", err) } - user.Username = "alice2" - if err := repo.Update(ctx, user); err != nil { - t.Fatalf("Update = %v", err) - } - found, err := repo.FindByID(ctx, "user-1") if err != nil { t.Fatalf("FindByID = %v", err) } - if found.Username != "alice2" { - t.Errorf("username = %q, want %q", found.Username, "alice2") + if found.IsAdmin { + t.Error("registered user must not be an administrator") + } + if found.Status != model.StatusActive { + t.Errorf("status = %q, want %q", found.Status, model.StatusActive) } } @@ -150,11 +158,11 @@ func TestUserRepository_Delete(t *testing.T) { ctx := context.Background() user := &model.User{ID: "user-1", Username: "alice", Email: "alice@example.com", PasswordHash: "hash", Status: model.StatusActive} - if err := repo.Create(ctx, user); err != nil { + if err := createUserRecord(ctx, repo, user); err != nil { t.Fatalf("Create = %v", err) } - if err := repo.Delete(ctx, "user-1"); err != nil { + if err := repo.MarkAdminDeleted(ctx, "user-1"); err != nil { t.Fatalf("Delete = %v", err) } @@ -176,7 +184,7 @@ func TestUserRepository_List(t *testing.T) { PasswordHash: "hash", Status: model.StatusActive, } - if err := repo.Create(ctx, user); err != nil { + if err := createUserRecord(ctx, repo, user); err != nil { t.Fatalf("Create = %v", err) } } @@ -204,11 +212,11 @@ func TestUserRepository_SoftDelete(t *testing.T) { PasswordHash: "hash", Status: model.StatusActive, } - if err := repo.Create(ctx, user); err != nil { + if err := createUserRecord(ctx, repo, user); err != nil { t.Fatalf("Create = %v", err) } - if err := repo.Delete(ctx, "user-1"); err != nil { + if err := repo.MarkAdminDeleted(ctx, "user-1"); err != nil { t.Fatalf("Delete = %v", err) } @@ -231,11 +239,11 @@ func TestUserRepository_DisabledLogin(t *testing.T) { PasswordHash: "hash", Status: model.StatusActive, } - if err := repo.Create(ctx, user); err != nil { + if err := createUserRecord(ctx, repo, user); err != nil { t.Fatalf("Create = %v", err) } - if err := repo.Delete(ctx, "user-1"); err != nil { + if err := repo.MarkAdminDeleted(ctx, "user-1"); err != nil { t.Fatalf("Delete = %v", err) } @@ -258,7 +266,7 @@ func TestUserRepository_StatusFilterList(t *testing.T) { PasswordHash: "hash", Status: model.StatusActive, } - if err := repo.Create(ctx, u1); err != nil { + if err := createUserRecord(ctx, repo, u1); err != nil { t.Fatalf("Create u1 = %v", err) } @@ -269,12 +277,12 @@ func TestUserRepository_StatusFilterList(t *testing.T) { PasswordHash: "hash", Status: model.StatusActive, } - if err := repo.Create(ctx, u2); err != nil { + if err := createUserRecord(ctx, repo, u2); err != nil { t.Fatalf("Create u2 = %v", err) } // Soft-delete bob - if err := repo.Delete(ctx, "user-2"); err != nil { + if err := repo.MarkAdminDeleted(ctx, "user-2"); err != nil { t.Fatalf("Delete u2 = %v", err) } @@ -305,15 +313,15 @@ func TestUserRepository_DeleteIdempotent(t *testing.T) { PasswordHash: "hash", Status: model.StatusActive, } - if err := repo.Create(ctx, user); err != nil { + if err := createUserRecord(ctx, repo, user); err != nil { t.Fatalf("Create = %v", err) } // Soft-delete the same user twice should not error. - if err := repo.Delete(ctx, "user-1"); err != nil { + if err := repo.MarkAdminDeleted(ctx, "user-1"); err != nil { t.Fatalf("first Delete = %v", err) } - if err := repo.Delete(ctx, "user-1"); err != nil { + if err := repo.MarkAdminDeleted(ctx, "user-1"); err != nil { t.Fatalf("second Delete = %v", err) } } diff --git a/internal/server/file_integration_test.go b/internal/server/file_integration_test.go index bb97e0b..3dfaff8 100644 --- a/internal/server/file_integration_test.go +++ b/internal/server/file_integration_test.go @@ -116,10 +116,6 @@ func newFileIntegrationFixture(t *testing.T) *fileIntegrationFixture { webApp := app.NewWebApp( cfg, db, - userRepo, - sessionRepo, - fileRepo, - credentialRepo, authService, service.NewAdminService(userRepo), fileService, diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 12dd639..214bf6a 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -2,7 +2,6 @@ package server import ( "bytes" - "context" "encoding/json" "net/http" "net/http/httptest" @@ -14,6 +13,7 @@ import ( "github.com/dhao2001/mygo/internal/config" "github.com/dhao2001/mygo/internal/repository" "github.com/dhao2001/mygo/internal/service" + "github.com/dhao2001/mygo/internal/testutil" ) func TestVersionRoute(t *testing.T) { @@ -25,7 +25,7 @@ func TestVersionRoute(t *testing.T) { }, } authService := service.NewAuthService(nil, nil, nil, nil, 15*time.Minute, 7*24*time.Hour) - webApp := app.NewWebApp(cfg, nil, nil, nil, nil, nil, authService, nil, nil, nil) + webApp := app.NewWebApp(cfg, nil, authService, nil, nil, nil) router := NewRouter(webApp) req := httptest.NewRequest(http.MethodGet, "/api/v1/version", nil) @@ -94,7 +94,7 @@ func TestAdminRoutes(t *testing.T) { }, } - webApp := app.NewWebApp(cfg, db, userRepo, sessionRepo, nil, credentialRepo, authService, adminService, nil, nil) + webApp := app.NewWebApp(cfg, db, authService, adminService, nil, nil) router := NewRouter(webApp) // Helper: register a user via POST /api/v1/auth/register and return the user ID. @@ -123,14 +123,7 @@ func TestAdminRoutes(t *testing.T) { // Register admin user and promote to admin. adminID := register("admin", "admin@test.local", "adminpass1") - adminUser, err := userRepo.FindByID(context.Background(), adminID) - if err != nil { - t.Fatalf("find admin user: %v", err) - } - adminUser.IsAdmin = true - if err := userRepo.Update(context.Background(), adminUser); err != nil { - t.Fatalf("promote to admin: %v", err) - } + testutil.SetUserAdmin(t, db, adminID, true) // Generate admin JWT. adminToken, err := auth.GenerateAccessToken(adminID, jwtSecret, accessTTL) diff --git a/internal/service/admin.go b/internal/service/admin.go index 0adf922..417f063 100644 --- a/internal/service/admin.go +++ b/internal/service/admin.go @@ -10,11 +10,11 @@ import ( // AdminService handles administrator user-management operations. type AdminService struct { - userRepo repository.UserRepository + userRepo repository.AdminUserRepository } // NewAdminService creates an AdminService. -func NewAdminService(userRepo repository.UserRepository) *AdminService { +func NewAdminService(userRepo repository.AdminUserRepository) *AdminService { return &AdminService{userRepo: userRepo} } @@ -44,7 +44,7 @@ func (s *AdminService) DeleteUser(ctx context.Context, id string) error { if _, err := s.GetUser(ctx, id); err != nil { return err } - if err := s.userRepo.Delete(ctx, id); err != nil { + if err := s.userRepo.MarkAdminDeleted(ctx, id); err != nil { return model.NewInternalError("delete admin user", err) } return nil diff --git a/internal/service/admin_test.go b/internal/service/admin_test.go index 796d9b4..3739981 100644 --- a/internal/service/admin_test.go +++ b/internal/service/admin_test.go @@ -46,13 +46,13 @@ func TestAdminServiceListIncludesDeletedUsers(t *testing.T) { active := &model.User{ID: "active-1", Username: "active", Email: "active@example.com", Status: model.StatusActive} deleted := &model.User{ID: "deleted-1", Username: "deleted", Email: "deleted@example.com", Status: model.StatusActive} - if err := repo.Create(ctx, active); err != nil { + if _, err := repo.CreateRegisteredUser(ctx, repository.RegisteredUserParams{ID: active.ID, Username: active.Username, Email: active.Email, PasswordHash: "hash"}); err != nil { t.Fatalf("create active: %v", err) } - if err := repo.Create(ctx, deleted); err != nil { + if _, err := repo.CreateRegisteredUser(ctx, repository.RegisteredUserParams{ID: deleted.ID, Username: deleted.Username, Email: deleted.Email, PasswordHash: "hash"}); err != nil { t.Fatalf("create deleted: %v", err) } - if err := repo.Delete(ctx, deleted.ID); err != nil { + if err := repo.MarkAdminDeleted(ctx, deleted.ID); err != nil { t.Fatalf("delete user: %v", err) } diff --git a/internal/service/auth.go b/internal/service/auth.go index 6ab7d3c..da0f66c 100644 --- a/internal/service/auth.go +++ b/internal/service/auth.go @@ -34,8 +34,8 @@ type Principal struct { // AuthService handles user authentication and session management. type AuthService struct { - userRepo repository.UserRepository - sessionRepo repository.SessionRepository + userRepo repository.AuthUserRepository + sessionRepo repository.AuthSessionRepository credentialRepo repository.CredentialRepository jwtSecret []byte accessTTL time.Duration @@ -44,8 +44,8 @@ type AuthService struct { // NewAuthService creates an AuthService. func NewAuthService( - userRepo repository.UserRepository, - sessionRepo repository.SessionRepository, + userRepo repository.AuthUserRepository, + sessionRepo repository.AuthSessionRepository, credentialRepo repository.CredentialRepository, jwtSecret []byte, accessTTL time.Duration, @@ -72,15 +72,13 @@ func (s *AuthService) Register(ctx context.Context, username, email, password st return nil, model.NewInternalError("hash password", err) } - user := &model.User{ + user, err := s.userRepo.CreateRegisteredUser(ctx, repository.RegisteredUserParams{ ID: uuid.NewString(), Username: username, Email: email, PasswordHash: passwordHash, - Status: model.StatusActive, - } - - if err := s.userRepo.Create(ctx, user); err != nil { + }) + if err != nil { if errors.Is(err, model.ErrDuplicate) { return nil, model.NewConflictError("username or email already exists") } @@ -124,7 +122,7 @@ func (s *AuthService) Refresh(ctx context.Context, refreshTokenStr string) (*Tok } tokenHash := auth.HashToken(refreshTokenStr) - session, err := s.sessionRepo.FindByTokenHash(ctx, tokenHash) + session, err := s.sessionRepo.ConsumeRefreshSession(ctx, tokenHash) if err != nil { if errors.Is(err, model.ErrNotFound) { return nil, model.NewUnauthenticatedError("invalid token") @@ -144,25 +142,13 @@ func (s *AuthService) Refresh(ctx context.Context, refreshTokenStr string) (*Tok return nil, model.NewUnauthenticatedError("invalid token") } - if err := s.sessionRepo.Delete(ctx, session.ID); err != nil { - return nil, model.NewInternalError("delete old session", err) - } - return s.issueTokens(ctx, claims.UserID) } // Logout invalidates a refresh token by deleting its session. func (s *AuthService) Logout(ctx context.Context, refreshTokenStr string) error { tokenHash := auth.HashToken(refreshTokenStr) - session, err := s.sessionRepo.FindByTokenHash(ctx, tokenHash) - if err != nil { - if errors.Is(err, model.ErrNotFound) { - return nil - } - return model.NewInternalError("find session", err) - } - - if err := s.sessionRepo.Delete(ctx, session.ID); err != nil { + if err := s.sessionRepo.RevokeRefreshSession(ctx, tokenHash); err != nil { return model.NewInternalError("delete session", err) } return nil @@ -175,20 +161,18 @@ func (s *AuthService) CreatePasskey(ctx context.Context, userID, label string) ( return nil, model.NewInternalError("generate token", err) } - cred := &model.Credential{ - ID: uuid.NewString(), + credID := uuid.NewString() + if err := s.credentialRepo.CreatePasskey(ctx, repository.PasskeyParams{ + ID: credID, UserID: userID, - Type: "app_passkey", Label: label, SecretHash: hash, - } - - if err := s.credentialRepo.Create(ctx, cred); err != nil { + }); err != nil { return nil, model.NewInternalError("create credential", err) } return &CreatedPasskey{ - ID: cred.ID, + ID: credID, Raw: raw, Label: label, }, nil @@ -201,7 +185,7 @@ func (s *AuthService) LoginWithPasskey(ctx context.Context, tokenStr string) (*T } tokenHash := auth.HashToken(tokenStr) - cred, err := s.credentialRepo.FindByHash(ctx, tokenHash) + cred, err := s.credentialRepo.FindPasskeyByHash(ctx, tokenHash) if err != nil { if errors.Is(err, model.ErrNotFound) { return nil, model.NewUnauthenticatedError("invalid passkey") @@ -209,19 +193,21 @@ func (s *AuthService) LoginWithPasskey(ctx context.Context, tokenStr string) (*T return nil, model.NewInternalError("find credential", err) } - user, err := s.userRepo.FindByIDIncludeDeleted(ctx, cred.UserID) + user, err := s.userRepo.FindByID(ctx, cred.UserID) if err != nil { + if errors.Is(err, model.ErrNotFound) { + return nil, model.NewUnauthenticatedError("invalid passkey") + } return nil, model.NewInternalError("find user", err) } if user.Status != model.StatusActive { return nil, model.NewUnauthenticatedError("invalid passkey") } - if cred.Type != "app_passkey" { - return nil, model.NewUnauthenticatedError("invalid credential type") - } - - if err := s.credentialRepo.UpdateLastUsed(ctx, cred.ID); err != nil { + if err := s.credentialRepo.RecordPasskeyUsed(ctx, cred.ID); err != nil { + if errors.Is(err, model.ErrNotFound) { + return nil, model.NewUnauthenticatedError("invalid passkey") + } return nil, model.NewInternalError("update last used", err) } @@ -230,7 +216,7 @@ func (s *AuthService) LoginWithPasskey(ctx context.Context, tokenStr string) (*T // ListPasskeys returns all app passkeys for a user. func (s *AuthService) ListPasskeys(ctx context.Context, userID string) ([]model.Credential, error) { - creds, err := s.credentialRepo.FindByUserIDAndType(ctx, userID, "app_passkey") + creds, err := s.credentialRepo.ListPasskeys(ctx, userID) if err != nil { return nil, model.NewInternalError("list passkeys", err) } @@ -239,7 +225,7 @@ func (s *AuthService) ListPasskeys(ctx context.Context, userID string) ([]model. // RevokePasskey deletes an app passkey owned by the user. func (s *AuthService) RevokePasskey(ctx context.Context, userID, credID string) error { - cred, err := s.credentialRepo.FindByID(ctx, credID) + cred, err := s.credentialRepo.FindPasskeyByID(ctx, credID) if err != nil { if errors.Is(err, model.ErrNotFound) { return model.NewNotFoundError("passkey not found", model.ErrNotFound) @@ -251,7 +237,10 @@ func (s *AuthService) RevokePasskey(ctx context.Context, userID, credID string) return model.NewPermissionDeniedError("access denied", model.ErrForbidden) } - if err := s.credentialRepo.Delete(ctx, credID); err != nil { + if err := s.credentialRepo.RevokeOwnedPasskey(ctx, userID, credID); err != nil { + if errors.Is(err, model.ErrNotFound) { + return model.NewNotFoundError("passkey not found", model.ErrNotFound) + } return model.NewInternalError("delete credential", err) } return nil @@ -296,14 +285,12 @@ func (s *AuthService) issueTokens(ctx context.Context, userID string) (*TokenPai return nil, model.NewInternalError("generate refresh token", err) } - session := &model.Session{ + if err := s.sessionRepo.CreateRefreshSession(ctx, repository.RefreshSessionParams{ ID: uuid.NewString(), UserID: userID, TokenHash: auth.HashToken(refreshToken), ExpiresAt: time.Now().Add(s.refreshTTL), - } - - if err := s.sessionRepo.Create(ctx, session); err != nil { + }); err != nil { return nil, model.NewInternalError("create session", err) } diff --git a/internal/service/auth_test.go b/internal/service/auth_test.go index cb126f1..01d4339 100644 --- a/internal/service/auth_test.go +++ b/internal/service/auth_test.go @@ -3,6 +3,8 @@ package service import ( "context" "errors" + "path/filepath" + "sync" "testing" "time" @@ -10,18 +12,20 @@ import ( "gorm.io/gorm" "github.com/dhao2001/mygo/internal/auth" + "github.com/dhao2001/mygo/internal/config" "github.com/dhao2001/mygo/internal/model" "github.com/dhao2001/mygo/internal/repository" + "github.com/dhao2001/mygo/internal/testutil" ) func setupAuthService(t *testing.T) *AuthService { - svc, _ := setupAuthServiceWithRepos(t) + svc, _, _ := setupAuthServiceWithRepos(t) return svc } // setupAuthServiceWithRepos creates an AuthService and returns the UserRepository // for tests that need direct repo access (e.g., soft-deleting users). -func setupAuthServiceWithRepos(t *testing.T) (*AuthService, repository.UserRepository) { +func setupAuthServiceWithRepos(t *testing.T) (*AuthService, repository.UserRepository, *gorm.DB) { t.Helper() db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) @@ -42,7 +46,7 @@ func setupAuthServiceWithRepos(t *testing.T) (*AuthService, repository.UserRepos 15*time.Minute, 7*24*time.Hour, ) - return svc, userRepo + return svc, userRepo, db } func TestAuthService_Register(t *testing.T) { @@ -413,7 +417,7 @@ func TestAuthService_RefreshWithAccessToken(t *testing.T) { } func TestAuthService_LoginDisabledUser(t *testing.T) { - svc, userRepo := setupAuthServiceWithRepos(t) + svc, userRepo, _ := setupAuthServiceWithRepos(t) ctx := context.Background() user, err := svc.Register(ctx, "alice", "alice@example.com", "password123") @@ -421,7 +425,7 @@ func TestAuthService_LoginDisabledUser(t *testing.T) { t.Fatalf("Register = %v", err) } - if err := userRepo.Delete(ctx, user.ID); err != nil { + if err := userRepo.MarkAdminDeleted(ctx, user.ID); err != nil { t.Fatalf("Delete = %v", err) } @@ -442,7 +446,7 @@ func TestAuthService_LoginDisabledUser(t *testing.T) { } func TestAuthService_LoginDisabledUserSameMessage(t *testing.T) { - svc, userRepo := setupAuthServiceWithRepos(t) + svc, userRepo, _ := setupAuthServiceWithRepos(t) ctx := context.Background() user, err := svc.Register(ctx, "alice", "alice@example.com", "password123") @@ -454,7 +458,7 @@ func TestAuthService_LoginDisabledUserSameMessage(t *testing.T) { _, wrongPwErr := svc.Login(ctx, "alice@example.com", "wrongpassword") // Soft-delete user, then try to login - if err := userRepo.Delete(ctx, user.ID); err != nil { + if err := userRepo.MarkAdminDeleted(ctx, user.ID); err != nil { t.Fatalf("Delete = %v", err) } _, disabledErr := svc.Login(ctx, "alice@example.com", "password123") @@ -482,7 +486,7 @@ func TestAuthService_LoginDisabledUserSameMessage(t *testing.T) { } func TestAuthService_LoginWithPasskeyDisabledUser(t *testing.T) { - svc, userRepo := setupAuthServiceWithRepos(t) + svc, userRepo, _ := setupAuthServiceWithRepos(t) ctx := context.Background() user, err := svc.Register(ctx, "alice", "alice@example.com", "password123") @@ -506,7 +510,7 @@ func TestAuthService_LoginWithPasskeyDisabledUser(t *testing.T) { } // Soft-delete the user - if err := userRepo.Delete(ctx, user.ID); err != nil { + if err := userRepo.MarkAdminDeleted(ctx, user.ID); err != nil { t.Fatalf("Delete = %v", err) } @@ -527,7 +531,7 @@ func TestAuthService_LoginWithPasskeyDisabledUser(t *testing.T) { } func TestAuthService_RefreshDisabledUser(t *testing.T) { - svc, userRepo := setupAuthServiceWithRepos(t) + svc, userRepo, _ := setupAuthServiceWithRepos(t) ctx := context.Background() user, err := svc.Register(ctx, "alice", "alice@example.com", "password123") @@ -541,7 +545,7 @@ func TestAuthService_RefreshDisabledUser(t *testing.T) { } // Soft-delete the user - if err := userRepo.Delete(ctx, user.ID); err != nil { + if err := userRepo.MarkAdminDeleted(ctx, user.ID); err != nil { t.Fatalf("Delete = %v", err) } @@ -562,17 +566,14 @@ func TestAuthService_RefreshDisabledUser(t *testing.T) { } func TestAuthService_AuthenticateAccessToken(t *testing.T) { - svc, userRepo := setupAuthServiceWithRepos(t) + svc, _, db := setupAuthServiceWithRepos(t) ctx := context.Background() user, err := svc.Register(ctx, "alice", "alice@example.com", "password123") if err != nil { t.Fatalf("Register = %v", err) } - user.IsAdmin = true - if err := userRepo.Update(ctx, user); err != nil { - t.Fatalf("promote user: %v", err) - } + testutil.SetUserAdmin(t, db, user.ID, true) pair, err := svc.Login(ctx, "alice@example.com", "password123") if err != nil { @@ -592,7 +593,7 @@ func TestAuthService_AuthenticateAccessToken(t *testing.T) { } func TestAuthService_AuthenticateAccessTokenRejectsDeletedUser(t *testing.T) { - svc, userRepo := setupAuthServiceWithRepos(t) + svc, userRepo, _ := setupAuthServiceWithRepos(t) ctx := context.Background() user, err := svc.Register(ctx, "alice", "alice@example.com", "password123") @@ -605,7 +606,7 @@ func TestAuthService_AuthenticateAccessTokenRejectsDeletedUser(t *testing.T) { t.Fatalf("Login = %v", err) } - if err := userRepo.Delete(ctx, user.ID); err != nil { + if err := userRepo.MarkAdminDeleted(ctx, user.ID); err != nil { t.Fatalf("Delete = %v", err) } @@ -618,3 +619,72 @@ func TestAuthService_AuthenticateAccessTokenRejectsDeletedUser(t *testing.T) { t.Fatalf("kind = %q, want %q", ae.Kind, model.KindUnauthenticated) } } + +func TestAuthService_RefreshTokenConcurrentUseAllowsOneSuccess(t *testing.T) { + db, err := repository.Open(config.DatabaseConfig{ + Driver: "sqlite3", + SQLite: config.SQLiteConfig{Path: filepath.Join(t.TempDir(), "auth.db")}, + }) + if err != nil { + t.Fatalf("open db: %v", err) + } + if err := repository.AutoMigrate(db); err != nil { + t.Fatalf("migrate: %v", err) + } + svc := NewAuthService( + repository.NewUserRepository(db), + repository.NewSessionRepository(db), + repository.NewCredentialRepository(db), + []byte("test-secret"), + 15*time.Minute, + 7*24*time.Hour, + ) + ctx := context.Background() + if _, err := svc.Register(ctx, "alice", "alice@example.com", "password123"); err != nil { + t.Fatalf("Register = %v", err) + } + pair, err := svc.Login(ctx, "alice@example.com", "password123") + if err != nil { + t.Fatalf("Login = %v", err) + } + + type refreshResult struct { + pair *TokenPair + err error + } + start := make(chan struct{}) + results := make(chan refreshResult, 2) + var wg sync.WaitGroup + wg.Add(2) + for range 2 { + go func() { + defer wg.Done() + <-start + refreshed, err := svc.Refresh(ctx, pair.RefreshToken) + results <- refreshResult{pair: refreshed, err: err} + }() + } + close(start) + wg.Wait() + close(results) + + succeeded := 0 + rejected := 0 + for result := range results { + if result.err == nil { + succeeded++ + if result.pair == nil || result.pair.AccessToken == "" || result.pair.RefreshToken == "" { + t.Fatal("successful refresh returned an empty token pair") + } + continue + } + var appErr *model.AppError + if !errors.As(result.err, &appErr) || appErr.Kind != model.KindUnauthenticated { + t.Fatalf("concurrent refresh error = %v, want unauthenticated", result.err) + } + rejected++ + } + if succeeded != 1 || rejected != 1 { + t.Fatalf("refresh results: success=%d rejected=%d, want 1 each", succeeded, rejected) + } +} diff --git a/internal/service/file.go b/internal/service/file.go index 3f65ee3..efe161e 100644 --- a/internal/service/file.go +++ b/internal/service/file.go @@ -136,7 +136,7 @@ func (s *FileService) Upload(ctx context.Context, userID string, parentID *strin return nil, model.NewInternalError("promote staged file", err) } - file := &model.File{ + file, err := s.fileRepo.CreateUploadedFile(ctx, repository.UploadedFileParams{ ID: fileID, UserID: userID, ParentID: parentID, @@ -145,16 +145,19 @@ func (s *FileService) Upload(ctx context.Context, userID string, parentID *strin MimeType: mimeType, StoragePath: storagePath, Hash: hex.EncodeToString(hasher.Sum(nil)), - Status: model.StatusActive, - IsDir: false, - } - - if err := s.fileRepo.Create(ctx, file); err != nil { + }) + if err != nil { // Compensate: remove the promoted file. _ = s.storage.Delete(ctx, storagePath) if errors.Is(err, model.ErrDuplicate) { return nil, model.NewConflictError("a file with this name already exists in this directory") } + if errors.Is(err, model.ErrParentNotFound) { + return nil, model.NewNotFoundError("parent directory not found", model.ErrParentNotFound) + } + if errors.Is(err, model.ErrParentNotDir) { + return nil, model.NewBadRequestError("parent is not a directory") + } return nil, model.NewInternalError("create file record", err) } @@ -256,44 +259,42 @@ func (s *FileService) List(ctx context.Context, userID string, parentID *string, // Update renames or moves a file or directory. func (s *FileService) Update(ctx context.Context, userID, fileID string, newName string, newParentID *string) (*FileInfo, error) { - file, err := s.getOwnedFile(ctx, userID, fileID) - if err != nil { - return nil, err - } - - // Validate new name if provided. + var newNamePtr *string if newName != "" { if err := validateFileName(newName); err != nil { return nil, err } - file.Name = newName + newNamePtr = &newName } - // If parent is changing, verify the new parent. if newParentID != nil { if *newParentID == fileID { return nil, model.NewBadRequestError("cannot move a directory into itself") } - if err := s.verifyParent(ctx, userID, *newParentID); err != nil { - return nil, err - } - file.ParentID = newParentID } - // Check for name conflicts in the target directory. - if newName != "" || newParentID != nil { - conflict, err := s.fileRepo.FindByNameAndParent(ctx, userID, file.ParentID, file.Name) - if err == nil && conflict.ID != fileID { - return nil, model.NewConflictError("a file with this name already exists in the target directory") - } else if err != nil && !errors.Is(err, model.ErrNotFound) { - return nil, model.NewInternalError("check name conflict", err) - } - } - - if err := s.fileRepo.Update(ctx, file); err != nil { + file, err := s.fileRepo.UpdateOwnedMetadata(ctx, repository.FileMetadataUpdate{ + FileID: fileID, + UserID: userID, + NewName: newNamePtr, + NewParentID: newParentID, + }) + if err != nil { if errors.Is(err, model.ErrDuplicate) { return nil, model.NewConflictError("a file with this name already exists in the target directory") } + if errors.Is(err, model.ErrNotFound) { + return nil, model.NewNotFoundError("file not found", model.ErrNotFound) + } + if errors.Is(err, model.ErrParentNotFound) { + return nil, model.NewNotFoundError("parent directory not found", model.ErrParentNotFound) + } + if errors.Is(err, model.ErrParentNotDir) { + return nil, model.NewBadRequestError("parent is not a directory") + } + if errors.Is(err, model.ErrInvalidMove) { + return nil, model.NewBadRequestError("cannot move a directory into itself") + } return nil, model.NewInternalError("update file", err) } @@ -302,26 +303,13 @@ func (s *FileService) Update(ctx context.Context, userID, fileID string, newName // Delete soft-deletes a file or (empty) directory. Storage content is preserved. func (s *FileService) Delete(ctx context.Context, userID, fileID string) error { - file, err := s.getOwnedFile(ctx, userID, fileID) - if err != nil { - return err - } - - // Directories must be empty before deletion. - if file.IsDir { - _, total, err := s.fileRepo.FindByParentIDPaginated(ctx, userID, &fileID, 0, 1) - if err != nil { - return model.NewInternalError("check directory contents", err) - } - if total > 0 { - return model.NewConflictError("directory is not empty") - } - } - - if err := s.fileRepo.Delete(ctx, fileID); err != nil { + if err := s.fileRepo.SoftDeleteOwned(ctx, userID, fileID); err != nil { if errors.Is(err, model.ErrNotFound) { return model.NewNotFoundError("file not found", model.ErrNotFound) } + if errors.Is(err, model.ErrDirectoryNotEmpty) { + return model.NewConflictError("directory is not empty") + } return model.NewInternalError("delete file record", err) } @@ -347,19 +335,22 @@ func (s *FileService) CreateDir(ctx context.Context, userID string, parentID *st return nil, model.NewInternalError("check name conflict", err) } - dir := &model.File{ + dir, err := s.fileRepo.CreateDirectory(ctx, repository.DirectoryParams{ ID: uuid.NewString(), UserID: userID, ParentID: parentID, Name: dirName, - Status: model.StatusActive, - IsDir: true, - } - - if err := s.fileRepo.Create(ctx, dir); err != nil { + }) + if err != nil { if errors.Is(err, model.ErrDuplicate) { return nil, model.NewConflictError("a file with this name already exists in this directory") } + if errors.Is(err, model.ErrParentNotFound) { + return nil, model.NewNotFoundError("parent directory not found", model.ErrParentNotFound) + } + if errors.Is(err, model.ErrParentNotDir) { + return nil, model.NewBadRequestError("parent is not a directory") + } return nil, model.NewInternalError("create directory record", err) } diff --git a/internal/service/file_test.go b/internal/service/file_test.go index d14ba07..b828ad0 100644 --- a/internal/service/file_test.go +++ b/internal/service/file_test.go @@ -50,6 +50,33 @@ type memStorage struct { files map[string][]byte } +type blockingPromoteStorage struct { + *memStorage + promoted chan struct{} + release chan struct{} +} + +func newBlockingPromoteStorage() *blockingPromoteStorage { + return &blockingPromoteStorage{ + memStorage: newMemStorage(), + promoted: make(chan struct{}), + release: make(chan struct{}), + } +} + +func (s *blockingPromoteStorage) PromoteStaged(ctx context.Context, stagedPath, finalPath string) error { + if err := s.memStorage.PromoteStaged(ctx, stagedPath, finalPath); err != nil { + return err + } + close(s.promoted) + select { + case <-s.release: + return nil + case <-ctx.Done(): + return ctx.Err() + } +} + func newMemStorage() *memStorage { return &memStorage{files: make(map[string][]byte)} } @@ -407,6 +434,30 @@ func TestFileService_Update(t *testing.T) { } } +func TestFileService_UpdateRejectsDuplicateNameInRoot(t *testing.T) { + svc := setupFileService(t) + ctx := context.Background() + + if _, err := svc.Upload(ctx, "user1", nil, "existing.txt", strings.NewReader("first")); err != nil { + t.Fatalf("Upload existing = %v", err) + } + file, err := svc.Upload(ctx, "user1", nil, "rename-me.txt", strings.NewReader("second")) + if err != nil { + t.Fatalf("Upload source = %v", err) + } + + _, err = svc.Update(ctx, "user1", file.ID, "existing.txt", nil) + assertAppError(t, err, model.KindConflict) + + unchanged, err := svc.Get(ctx, "user1", file.ID) + if err != nil { + t.Fatalf("Get source after rejected rename = %v", err) + } + if unchanged.Name != "rename-me.txt" { + t.Fatalf("source name = %q, want rename-me.txt", unchanged.Name) + } +} + func TestFileService_UpdateReturnsNotFoundForOtherUser(t *testing.T) { svc := setupFileService(t) ctx := context.Background() @@ -663,3 +714,42 @@ func TestFileService_SoftDeleteReturnsNotFoundForOtherUser(t *testing.T) { err = svc.Delete(ctx, "user2", info.ID) assertAppErrorMessage(t, err, model.KindNotFound, "file not found") } + +func TestFileService_UploadRechecksParentAfterStoragePromotion(t *testing.T) { + db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) + if err != nil { + t.Fatalf("open db: %v", err) + } + if err := db.AutoMigrate(&model.File{}); err != nil { + t.Fatalf("migrate: %v", err) + } + + store := newBlockingPromoteStorage() + svc := NewFileService(repository.NewFileRepository(db), store, 0, nil) + ctx := context.Background() + dir, err := svc.CreateDir(ctx, "user1", nil, "parent") + if err != nil { + t.Fatalf("CreateDir = %v", err) + } + + uploadResult := make(chan error, 1) + go func() { + _, err := svc.Upload(ctx, "user1", &dir.ID, "child.txt", strings.NewReader("content")) + uploadResult <- err + }() + + <-store.promoted + if err := svc.Delete(ctx, "user1", dir.ID); err != nil { + t.Fatalf("Delete parent while upload is paused = %v", err) + } + close(store.release) + + err = <-uploadResult + assertAppErrorMessage(t, err, model.KindNotFound, "parent directory not found") + store.mu.RLock() + remainingObjects := len(store.files) + store.mu.RUnlock() + if remainingObjects != 0 { + t.Fatalf("storage objects after rejected upload = %d, want 0", remainingObjects) + } +} diff --git a/internal/testutil/user.go b/internal/testutil/user.go new file mode 100644 index 0000000..ce830ec --- /dev/null +++ b/internal/testutil/user.go @@ -0,0 +1,23 @@ +// Package testutil contains fixtures that intentionally bypass production +// service ports. Production packages must not import it. +package testutil + +import ( + "testing" + + "gorm.io/gorm" + + "github.com/dhao2001/mygo/internal/model" +) + +// SetUserAdmin changes a user's administrative flag for test fixture setup. +func SetUserAdmin(t testing.TB, db *gorm.DB, userID string, isAdmin bool) { + t.Helper() + result := db.Model(&model.User{}).Where("id = ?", userID).Update("is_admin", isAdmin) + if result.Error != nil { + t.Fatalf("set test user admin flag: %v", result.Error) + } + if result.RowsAffected == 0 { + t.Fatalf("set test user admin flag: user %q not found", userID) + } +}