From b82a891c508957aaed0367468ffddd2181276b48 Mon Sep 17 00:00:00 2001 From: XingfenD Date: Mon, 14 Sep 2026 19:42:11 +0800 Subject: [PATCH] fix: upload part tmp+rename (B4), transactional last-admin DeleteUser (B5), RowsAffected order (B13) --- backend/cmd/webui/handlers/uploads.go | 12 +++++- backend/cmd/webui/handlers/users.go | 21 +++-------- backend/internal/store/store.go | 54 ++++++++++++++++++++++----- backend/internal/store/store_test.go | 48 ++++++++++++++++++++++++ 4 files changed, 108 insertions(+), 27 deletions(-) diff --git a/backend/cmd/webui/handlers/uploads.go b/backend/cmd/webui/handlers/uploads.go index e0506c3..fbc9cb1 100644 --- a/backend/cmd/webui/handlers/uploads.go +++ b/backend/cmd/webui/handlers/uploads.go @@ -198,8 +198,11 @@ func (h *H) UploadPart(c *gin.Context) { } lo, hi := chunkRange(m, idx) c.Request.Body = http.MaxBytesReader(c.Writer, c.Request.Body, hi-lo) + // B4: write to .tmp then rename — prevents truncated parts from being + // reported as "received" by UploadStatus if the process crashes mid-write. p := filepath.Join(dir, "parts", strconv.FormatInt(idx, 10)) - f, e := os.OpenFile(p, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o644) + tmp := p + ".tmp" + f, e := os.OpenFile(tmp, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o644) if e != nil { err(c, http.StatusInternalServerError, "internal", "create part") return @@ -207,7 +210,7 @@ func (h *H) UploadPart(c *gin.Context) { n, e := io.Copy(f, c.Request.Body) f.Close() if e != nil || n != hi-lo { - os.Remove(p) + os.Remove(tmp) var mbe *http.MaxBytesError code, msg := "too_large", "part size mismatch" if errors.As(e, &mbe) { @@ -216,6 +219,11 @@ func (h *H) UploadPart(c *gin.Context) { err(c, http.StatusRequestEntityTooLarge, code, msg) return } + if e := os.Rename(tmp, p); e != nil { + os.Remove(tmp) + err(c, http.StatusInternalServerError, "internal", "rename part") + return + } c.JSON(http.StatusAccepted, gin.H{"accepted": true}) } diff --git a/backend/cmd/webui/handlers/users.go b/backend/cmd/webui/handlers/users.go index 821f7f5..5d62377 100644 --- a/backend/cmd/webui/handlers/users.go +++ b/backend/cmd/webui/handlers/users.go @@ -8,15 +8,13 @@ import ( "github.com/gin-gonic/gin" "github.com/jackc/pgx/v5" - "github.com/jackc/pgx/v5/pgconn" "booklib/internal/auth" + "booklib/internal/store" ) -func isUnique(e error) bool { - var pgErr *pgconn.PgError - return errors.As(e, &pgErr) && pgErr.Code == "23505" -} +// isUnique is a convenience alias for store.IsUniqueViolation. +func isUnique(e error) bool { return store.IsUniqueViolation(e) } func (h *H) ListUsers(c *gin.Context) { users, e := h.st.ListUsers(c) @@ -73,23 +71,16 @@ func (h *H) DeleteUser(c *gin.Context) { err(c, http.StatusBadRequest, "bad_request", "cannot delete yourself") return } - target, e := h.st.GetUserByID(c, id) - if e != nil { + // B5: transactional last-admin check eliminates TOCTOU race. + if e := h.st.DeleteUser(c, id); e != nil { if errors.Is(e, pgx.ErrNoRows) { err(c, http.StatusNotFound, "not_found", "no such user") return } - dbErr(c, e) - return - } - if target.Role == "admin" { - n, _ := h.st.CountAdmins(c) // 防删光最后一个 admin - if n <= 1 { + if errors.Is(e, store.ErrLastAdmin) { err(c, http.StatusBadRequest, "bad_request", "cannot delete the last admin") return } - } - if e := h.st.DeleteUser(c, id); e != nil { dbErr(c, e) return } diff --git a/backend/internal/store/store.go b/backend/internal/store/store.go index f365f97..eb0effc 100644 --- a/backend/internal/store/store.go +++ b/backend/internal/store/store.go @@ -2,12 +2,23 @@ package store import ( "context" + "errors" "time" "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgconn" "github.com/jackc/pgx/v5/pgxpool" ) +// ErrLastAdmin is returned by DeleteUser when attempting to delete the last admin. +var ErrLastAdmin = errors.New("cannot delete the last admin") + +// IsUniqueViolation reports whether err is a PostgreSQL unique constraint violation (23505). +func IsUniqueViolation(err error) bool { + var pgErr *pgconn.PgError + return errors.As(err, &pgErr) && pgErr.Code == "23505" +} + type Store struct{ P *pgxpool.Pool } func New(p *pgxpool.Pool) *Store { return &Store{P: p} } @@ -103,9 +114,31 @@ func (s *Store) ListUsers(ctx context.Context) ([]User, error) { return out, rows.Err() } +// DeleteUser deletes a user with a transactional last-admin check (B5). +// Returns ErrLastAdmin if the target is the last admin. func (s *Store) DeleteUser(ctx context.Context, id int64) error { - _, err := s.P.Exec(ctx, "DELETE FROM users WHERE id=$1", id) - return err + tx, err := s.P.Begin(ctx) + if err != nil { + return err + } + defer tx.Rollback(ctx) + var role string + if err := tx.QueryRow(ctx, "SELECT role FROM users WHERE id=$1 FOR UPDATE", id).Scan(&role); err != nil { + return err // includes pgx.ErrNoRows + } + if role == "admin" { + var n int + if err := tx.QueryRow(ctx, "SELECT count(*) FROM users WHERE role='admin'").Scan(&n); err != nil { + return err + } + if n <= 1 { + return ErrLastAdmin + } + } + if _, err := tx.Exec(ctx, "DELETE FROM users WHERE id=$1", id); err != nil { + return err + } + return tx.Commit(ctx) } func scanUser(row pgx.Row) (User, error) { @@ -114,12 +147,7 @@ func scanUser(row pgx.Row) (User, error) { return u, err } -// CountAdmins 供 Task 5 的"最后一个 admin 不可删"保护 -func (s *Store) CountAdmins(ctx context.Context) (int, error) { - var n int - err := s.P.QueryRow(ctx, "SELECT count(*) FROM users WHERE role='admin'").Scan(&n) - return n, err -} + // ---------- libraries ---------- @@ -357,12 +385,18 @@ func (s *Store) ListBookmarks(ctx context.Context, userID, libID int64, bookPath func (s *Store) UpdateBookmarkNote(ctx context.Context, userID, id int64, note string) (bool, error) { res, err := s.P.Exec(ctx, `UPDATE bookmarks SET note=$3 WHERE id=$1 AND user_id=$2`, id, userID, note) - return res.RowsAffected() > 0, err + if err != nil { + return false, err + } + return res.RowsAffected() > 0, nil } func (s *Store) DeleteBookmark(ctx context.Context, userID, id int64) (bool, error) { res, err := s.P.Exec(ctx, `DELETE FROM bookmarks WHERE id=$1 AND user_id=$2`, id, userID) - return res.RowsAffected() > 0, err + if err != nil { + return false, err + } + return res.RowsAffected() > 0, nil } func (s *Store) GetProgress(ctx context.Context, userID, libID int64, bookPath string) (Progress, error) { diff --git a/backend/internal/store/store_test.go b/backend/internal/store/store_test.go index b91dda5..0eb8921 100644 --- a/backend/internal/store/store_test.go +++ b/backend/internal/store/store_test.go @@ -2,6 +2,7 @@ package store import ( "context" + "errors" "os" "testing" @@ -129,3 +130,50 @@ func TestProgressUpsertAndJoin(t *testing.T) { t.Fatalf("views %+v", views) } } + +func TestDeleteUserLastAdmin(t *testing.T) { + s := setup(t) + ctx := context.Background() + id, err := s.CreateUser(ctx, "onlyadmin", "hash", "admin") + if err != nil { + t.Fatal(err) + } + err = s.DeleteUser(ctx, id) + if !errors.Is(err, ErrLastAdmin) { + t.Fatalf("expected ErrLastAdmin, got %v", err) + } + // Verify user still exists. + _, err = s.GetUserByID(ctx, id) + if err != nil { + t.Fatal("admin should still exist after blocked delete") + } +} + +func TestDeleteUserNonLastAdmin(t *testing.T) { + s := setup(t) + ctx := context.Background() + id1, _ := s.CreateUser(ctx, "admin1", "h", "admin") + _, _ = s.CreateUser(ctx, "admin2", "h", "admin") + if err := s.DeleteUser(ctx, id1); err != nil { + t.Fatalf("should allow deleting non-last admin: %v", err) + } +} + +func TestDeleteUserMember(t *testing.T) { + s := setup(t) + ctx := context.Background() + id, _ := s.CreateUser(ctx, "member", "h", "member") + if err := s.DeleteUser(ctx, id); err != nil { + t.Fatalf("member delete should succeed: %v", err) + } +} + +func TestIsUniqueViolation(t *testing.T) { + s := setup(t) + ctx := context.Background() + _, _ = s.CreateUser(ctx, "dup", "h", "member") + _, err := s.CreateUser(ctx, "dup", "h", "member") + if !IsUniqueViolation(err) { + t.Fatalf("expected unique violation, got %v", err) + } +}