diff --git a/cmd/vellum/main.go b/cmd/vellum/main.go index db223e2..59203cf 100644 --- a/cmd/vellum/main.go +++ b/cmd/vellum/main.go @@ -15,6 +15,7 @@ import ( "time" "github.com/mithral/vellum/internal/auth" + "github.com/mithral/vellum/internal/catalog" "github.com/mithral/vellum/internal/cli" "github.com/mithral/vellum/internal/core" "github.com/mithral/vellum/internal/home" @@ -116,7 +117,7 @@ func run(cfg core.Config, log *slog.Logger) error { // Playback engine registry + selection. Vellum never transcodes, so the // server reports what is inside a file and matches it against the engines the // CLIENT declares — a browser offers "html5", a native client can offer mpv. - playback.NewHandlers(db, log, authenticator).Register(api.Mux()) + playback.NewHandlers(catalog.New(db), log, authenticator).Register(api.Mux()) // Client compatibility layers. Neither speaks the session cookie; both // authenticate with an api_tokens credential and then go through the same diff --git a/internal/catalog/catalog.go b/internal/catalog/catalog.go index 6bd9208..cd415e6 100644 --- a/internal/catalog/catalog.go +++ b/internal/catalog/catalog.go @@ -13,6 +13,23 @@ // this package's surface instead of editing 186 statements scattered across the // tree. // +// # What the Store covers, and what it deliberately does not +// +// Store is the seam, and it holds the per-item load path only: Item, ItemFor, +// Files, FilesFor, FileByID, FirstFile, FileCount. Those are the queries whose +// correctness is a security property (ItemFor is the authorization chokepoint) +// and whose shape is identical for every media kind, so one implementation is +// worth having. +// +// Aggregate and list queries — browse lists, home shelves, Subsonic albums — +// deliberately stay OUT of it. Each is a presentation concern of the package +// that owns the surface, with its own columns, ordering and JSON contract, and +// none of them is shared. Pulling them in would mean rewriting all 186 +// statements to match one interface, which is the exact cost this seam exists to +// avoid, and the result would be an interface nobody could implement without +// writing the whole application's query layer twice. The seam buys a swappable +// per-item load; it is not a repository. +// // This package holds no HTTP knowledge. It returns typed errors and lets the // caller choose a status code. package catalog @@ -80,6 +97,22 @@ type File struct { Height int `json:"height"` } +// Store is the storage seam. Implement it to run this package's per-item load +// path against something other than SQLite. +// +// The contract is the one the SQL implementation has always had: absent item and +// forbidden item both come back as ErrNotFound, and every other failure is a +// wrapped error. A caller that can distinguish the two has a bug. +type Store interface { + Item(ctx context.Context, id int64) (Item, error) + ItemFor(ctx context.Context, u auth.User, id int64) (Item, error) + Files(ctx context.Context, itemID int64) ([]File, error) + FilesFor(ctx context.Context, u auth.User, itemID int64) ([]File, error) + FileByID(ctx context.Context, itemID, fileID int64) (File, error) + FirstFile(ctx context.Context, itemID int64) (File, error) + FileCount(ctx context.Context, itemID int64) (int, error) +} + const itemSelect = ` SELECT i.id, i.library_id, i.kind, i.path, i.title, COALESCE(i.year, 0), i.missing, l.root_path, COALESCE((SELECT f.mtime FROM media_files f WHERE f.item_id = i.id ORDER BY f.id LIMIT 1), '') @@ -87,15 +120,25 @@ const itemSelect = ` JOIN libraries l ON l.id = i.library_id WHERE i.id = ?` -// LoadUnchecked reads an item WITHOUT checking access. Prefer LoadFor in any handler -// that serves a request: this exists for the scanner, jobs and tests, which -// have already established the right to touch the row. -func LoadUnchecked(ctx context.Context, db *sql.DB, id int64) (Item, error) { +// SQL is the SQLite implementation of Store, and the only thing in this package +// that knows the query text. +type SQL struct{ db *sql.DB } + +var _ Store = (*SQL)(nil) + +// New returns a Store backed by db. The db must already be migrated; this +// package never creates schema. +func New(db *sql.DB) *SQL { return &SQL{db: db} } + +// Item reads an item WITHOUT checking access. Prefer ItemFor in any handler that +// serves a request: this exists for the scanner, jobs and tests, which have +// already established the right to touch the row. +func (s *SQL) Item(ctx context.Context, id int64) (Item, error) { var ( it Item mtime string ) - err := db.QueryRowContext(ctx, itemSelect, id).Scan( + err := s.db.QueryRowContext(ctx, itemSelect, id).Scan( &it.ID, &it.LibraryID, &it.Kind, &it.Path, &it.Title, &it.Year, &it.Missing, &it.RootPath, &mtime) if errors.Is(err, sql.ErrNoRows) { @@ -110,17 +153,17 @@ func LoadUnchecked(ctx context.Context, db *sql.DB, id int64) (Item, error) { return it, nil } -// LoadFor reads an item and confirms the user may read its library. +// ItemFor reads an item and confirms the user may read its library. // // This is the authorization chokepoint. Every HTTP surface — the web app, the // modules, Jellyfin, Subsonic — is expected to go through it, so there is one // implementation to audit rather than five to keep in sync. -func LoadFor(ctx context.Context, db *sql.DB, u auth.User, id int64) (Item, error) { - it, err := LoadUnchecked(ctx, db, id) +func (s *SQL) ItemFor(ctx context.Context, u auth.User, id int64) (Item, error) { + it, err := s.Item(ctx, id) if err != nil { return Item{}, err } - allowed, err := auth.CanAccessLibrary(ctx, db, u, it.LibraryID) + allowed, err := auth.CanAccessLibrary(ctx, s.db, u, it.LibraryID) if err != nil { return Item{}, fmt.Errorf("catalog: check access: %w", err) } @@ -132,8 +175,8 @@ func LoadFor(ctx context.Context, db *sql.DB, u auth.User, id int64) (Item, erro } // Files returns an item's files in order, with codec facts. -func Files(ctx context.Context, db *sql.DB, itemID int64) ([]File, error) { - rows, err := db.QueryContext(ctx, ` +func (s *SQL) Files(ctx context.Context, itemID int64) ([]File, error) { + rows, err := s.db.QueryContext(ctx, ` SELECT id, path, size, container, missing, duration_ms, vcodec, acodec, width, height FROM media_files WHERE item_id = ? ORDER BY id`, itemID) @@ -156,18 +199,18 @@ func Files(ctx context.Context, db *sql.DB, itemID int64) ([]File, error) { // FilesFor returns an item's files after confirming access. Callers that // already hold an authorized Item should use Files to avoid a second check. -func FilesFor(ctx context.Context, db *sql.DB, u auth.User, itemID int64) ([]File, error) { - if _, err := LoadFor(ctx, db, u, itemID); err != nil { +func (s *SQL) FilesFor(ctx context.Context, u auth.User, itemID int64) ([]File, error) { + if _, err := s.ItemFor(ctx, u, itemID); err != nil { return nil, err } - return Files(ctx, db, itemID) + return s.Files(ctx, itemID) } // FileByID returns one file of an item, scoped to that item so a file id from // another item cannot be fetched through it. -func FileByID(ctx context.Context, db *sql.DB, itemID, fileID int64) (File, error) { +func (s *SQL) FileByID(ctx context.Context, itemID, fileID int64) (File, error) { var f File - err := db.QueryRowContext(ctx, ` + err := s.db.QueryRowContext(ctx, ` SELECT id, path, size, container, missing, duration_ms, vcodec, acodec, width, height FROM media_files WHERE item_id = ? AND id = ?`, itemID, fileID).Scan( @@ -184,9 +227,9 @@ func FileByID(ctx context.Context, db *sql.DB, itemID, fileID int64) (File, erro // FirstFile returns the item's first file, which is what single-file playback // and cover extraction both want. -func FirstFile(ctx context.Context, db *sql.DB, itemID int64) (File, error) { +func (s *SQL) FirstFile(ctx context.Context, itemID int64) (File, error) { var f File - err := db.QueryRowContext(ctx, ` + err := s.db.QueryRowContext(ctx, ` SELECT id, path, size, container, missing, duration_ms, vcodec, acodec, width, height FROM media_files WHERE item_id = ? ORDER BY id LIMIT 1`, itemID).Scan( @@ -202,9 +245,9 @@ func FirstFile(ctx context.Context, db *sql.DB, itemID int64) (File, error) { } // FileCount reports how many files back an item. -func FileCount(ctx context.Context, db *sql.DB, itemID int64) (int, error) { +func (s *SQL) FileCount(ctx context.Context, itemID int64) (int, error) { var n int - err := db.QueryRowContext(ctx, + err := s.db.QueryRowContext(ctx, `SELECT count(*) FROM media_files WHERE item_id = ?`, itemID).Scan(&n) if err != nil { return 0, fmt.Errorf("catalog: file count: %w", err) @@ -212,6 +255,42 @@ func FileCount(ctx context.Context, db *sql.DB, itemID int64) (int, error) { return n, nil } +// The functions below are the original package API, kept so that existing +// callers compile and behave exactly as before. Each one is a one-line +// delegation to the SQL implementation above. +// +// NEW CODE SHOULD TAKE A Store, NOT A *sql.DB. A caller that wants to be +// swappable off SQLite takes the Store interface and receives New(db) at the +// wiring point; reaching for these functions bakes the engine back in. + +func LoadUnchecked(ctx context.Context, db *sql.DB, id int64) (Item, error) { + return New(db).Item(ctx, id) +} + +func LoadFor(ctx context.Context, db *sql.DB, u auth.User, id int64) (Item, error) { + return New(db).ItemFor(ctx, u, id) +} + +func Files(ctx context.Context, db *sql.DB, itemID int64) ([]File, error) { + return New(db).Files(ctx, itemID) +} + +func FilesFor(ctx context.Context, db *sql.DB, u auth.User, itemID int64) ([]File, error) { + return New(db).FilesFor(ctx, u, itemID) +} + +func FileByID(ctx context.Context, db *sql.DB, itemID, fileID int64) (File, error) { + return New(db).FileByID(ctx, itemID, fileID) +} + +func FirstFile(ctx context.Context, db *sql.DB, itemID int64) (File, error) { + return New(db).FirstFile(ctx, itemID) +} + +func FileCount(ctx context.Context, db *sql.DB, itemID int64) (int, error) { + return New(db).FileCount(ctx, itemID) +} + // parseUnixNano parses a base-10 nanosecond timestamp, returning 0 on anything // that is not all digits. func parseUnixNano(s string) int64 { diff --git a/internal/catalog/store_test.go b/internal/catalog/store_test.go new file mode 100644 index 0000000..45a42d1 --- /dev/null +++ b/internal/catalog/store_test.go @@ -0,0 +1,208 @@ +package catalog + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/mithral/vellum/internal/auth" +) + +// This file exists to keep the Store seam honest. An interface that no one can +// substitute is decoration, so fakeStore implements Store entirely in memory — +// no database, no SQL — and the test drives a caller through it. If a method is +// added to Store later, this file stops compiling, which is the intended alarm. +// +// What is NOT tested here: the query text. That is the SQL implementation's job +// and catalog_test.go covers it against a real database. + +// fakeStore is an in-memory Store. Its authorization model mirrors auth's: an +// admin sees everything, everyone else sees only granted libraries. +type fakeStore struct { + items map[int64]Item + files map[int64][]File + granted map[int64][]int64 // user id -> library ids + admins map[int64]bool + + // calls records which methods ran, so the test can prove the caller went + // through the interface rather than reaching for a database. + calls []string +} + +var _ Store = (*fakeStore)(nil) + +func newFakeStore() *fakeStore { + return &fakeStore{ + items: map[int64]Item{}, + files: map[int64][]File{}, + granted: map[int64][]int64{}, + admins: map[int64]bool{}, + } +} + +func (f *fakeStore) allowed(u auth.User, libID int64) bool { + if f.admins[u.ID] { + return true + } + for _, id := range f.granted[u.ID] { + if id == libID { + return true + } + } + return false +} + +func (f *fakeStore) Item(_ context.Context, id int64) (Item, error) { + f.calls = append(f.calls, "Item") + it, ok := f.items[id] + if !ok { + return Item{}, ErrNotFound + } + return it, nil +} + +func (f *fakeStore) ItemFor(_ context.Context, u auth.User, id int64) (Item, error) { + f.calls = append(f.calls, "ItemFor") + it, ok := f.items[id] + if !ok || !f.allowed(u, it.LibraryID) { + // The whole point: absent and forbidden are the same error. + return Item{}, ErrNotFound + } + return it, nil +} + +func (f *fakeStore) Files(_ context.Context, itemID int64) ([]File, error) { + f.calls = append(f.calls, "Files") + return f.files[itemID], nil +} + +func (f *fakeStore) FilesFor(ctx context.Context, u auth.User, itemID int64) ([]File, error) { + f.calls = append(f.calls, "FilesFor") + if _, err := f.ItemFor(ctx, u, itemID); err != nil { + return nil, err + } + return f.Files(ctx, itemID) +} + +func (f *fakeStore) FileByID(_ context.Context, itemID, fileID int64) (File, error) { + f.calls = append(f.calls, "FileByID") + for _, fl := range f.files[itemID] { // scoped to the item, like the SQL + if fl.ID == fileID { + return fl, nil + } + } + return File{}, ErrNotFound +} + +func (f *fakeStore) FirstFile(_ context.Context, itemID int64) (File, error) { + f.calls = append(f.calls, "FirstFile") + fs := f.files[itemID] + if len(fs) == 0 { + return File{}, ErrNotFound + } + return fs[0], nil +} + +func (f *fakeStore) FileCount(_ context.Context, itemID int64) (int, error) { + f.calls = append(f.calls, "FileCount") + return len(f.files[itemID]), nil +} + +// serveItem is a stand-in for a real caller (media.playback, jellyfin.playback) +// written against the Store interface instead of *sql.DB. It is here because +// that is what the seam is FOR: this function cannot be told it needs SQLite. +func serveItem(ctx context.Context, s Store, u auth.User, id int64) (Item, File, error) { + it, err := s.ItemFor(ctx, u, id) + if err != nil { + return Item{}, File{}, err + } + f, err := s.FirstFile(ctx, id) + if err != nil { + return Item{}, File{}, err + } + return it, f, nil +} + +func fixture() (*fakeStore, auth.User, auth.User, int64) { + s := newFakeStore() + const libID = 42 + s.items[7] = Item{ID: 7, LibraryID: libID, Kind: "ebooks", Path: "Thing.epub", + Title: "Thing", Year: 1999, RootPath: "/media/A", + ModTime: time.Unix(1700000000, 0).UTC()} + s.files[7] = []File{{ID: 700, Path: "Thing.epub", Size: 100, Container: "mp4"}} + s.files[8] = []File{{ID: 800, Path: "Other.epub", Size: 100}} + + granted := auth.User{ID: 1, Username: "granted"} + outsider := auth.User{ID: 2, Username: "outsider"} + s.granted[granted.ID] = []int64{libID} + return s, granted, outsider, 7 +} + +func TestStoreIsSubstitutableWithoutADatabase(t *testing.T) { + s, granted, _, itemID := fixture() + it, f, err := serveItem(context.Background(), s, granted, itemID) + if err != nil { + t.Fatalf("in-memory Store rejected a granted user: %v", err) + } + if it.Title != "Thing" || it.AbsPath() != "/media/A/Thing.epub" { + t.Errorf("item lost in translation: %+v", it) + } + if f.ID != 700 { + t.Errorf("FirstFile = %d, want 700", f.ID) + } + if want := []string{"ItemFor", "FirstFile"}; len(s.calls) != 2 || + s.calls[0] != want[0] || s.calls[1] != want[1] { + t.Errorf("calls = %v, want %v", s.calls, want) + } +} + +// The security property has to survive the seam: a caller that could tell a +// forbidden item from an absent one would leak another user's library. +func TestStoreKeepsForbiddenIndistinguishableFromAbsent(t *testing.T) { + s, _, outsider, itemID := fixture() + ctx := context.Background() + + _, _, ferr := serveItem(ctx, s, outsider, itemID) + _, _, aerr := serveItem(ctx, s, outsider, itemID+9999) + if !errors.Is(ferr, ErrNotFound) || !errors.Is(aerr, ErrNotFound) { + t.Fatalf("errors = %v / %v, want ErrNotFound for both", ferr, aerr) + } + if ferr.Error() != aerr.Error() { + t.Errorf("errors differ and would leak existence: %q vs %q", ferr, aerr) + } +} + +func TestStoreAdminSeesEverythingWithoutAGrant(t *testing.T) { + s, _, _, itemID := fixture() + admin := auth.User{ID: 3, Username: "root", IsAdmin: true} + s.admins[admin.ID] = true + + if _, err := s.ItemFor(context.Background(), admin, itemID); err != nil { + t.Errorf("admin denied: %v", err) + } + if _, err := s.FilesFor(context.Background(), admin, itemID); err != nil { + t.Errorf("admin FilesFor denied: %v", err) + } + if n, err := s.FileCount(context.Background(), itemID); err != nil || n != 1 { + t.Errorf("FileCount = %d, %v; want 1, nil", n, err) + } +} + +// FileByID must stay scoped to its item through the interface too, or the +// in-memory Store would be laxer than the SQL one it stands in for. +func TestStoreFileByIDIsScopedToItsItem(t *testing.T) { + s, _, _, itemID := fixture() + ctx := context.Background() + + other, err := s.FirstFile(ctx, itemID+1) + if err != nil { + t.Fatal(err) + } + if _, err := s.FileByID(ctx, itemID, other.ID); !errors.Is(err, ErrNotFound) { + t.Errorf("file id leaked across items: %v", err) + } + if _, err := s.FileByID(ctx, itemID+1, other.ID); err != nil { + t.Errorf("legitimate lookup failed: %v", err) + } +} diff --git a/internal/playback/handlers.go b/internal/playback/handlers.go index 826b70b..b5aff32 100644 --- a/internal/playback/handlers.go +++ b/internal/playback/handlers.go @@ -9,7 +9,6 @@ package playback import ( - "database/sql" "errors" "log/slog" "net/http" @@ -22,14 +21,17 @@ import ( // Handlers serves the playback selection endpoints. type Handlers struct { - db *sql.DB - log *slog.Logger - auth *auth.Authenticator + // items is the catalog seam. This package talks to a Store rather than to + // *sql.DB, so the storage engine is swappable without touching the handler — + // and a test can substitute a fake with no database at all. + items catalog.Store + log *slog.Logger + auth *auth.Authenticator } // NewHandlers builds the playback HTTP layer. -func NewHandlers(db *sql.DB, log *slog.Logger, a *auth.Authenticator) *Handlers { - return &Handlers{db: db, log: log, auth: a} +func NewHandlers(items catalog.Store, log *slog.Logger, a *auth.Authenticator) *Handlers { + return &Handlers{items: items, log: log, auth: a} } // Register mounts the routes. Both require an authenticated user; the resolve @@ -68,7 +70,7 @@ func (h *Handlers) handleResolve(w http.ResponseWriter, r *http.Request, u auth. // item and confirms the caller may read its library, returning the same // ErrNotFound for a missing item and an unreadable one. That keeps an // inaccessible item a 404, never a 403, so its existence is not disclosed. - if _, err := catalog.LoadFor(r.Context(), h.db, u, req.ItemID); err != nil { + if _, err := h.items.ItemFor(r.Context(), u, req.ItemID); err != nil { if errors.Is(err, catalog.ErrNotFound) { httpx.WriteError(w, http.StatusNotFound, "item not found") return @@ -80,7 +82,7 @@ func (h *Handlers) handleResolve(w http.ResponseWriter, r *http.Request, u auth. // The first file carries the codec facts the player will try. An item with // no file cannot be played, but is still a 200 with a resolution so the UI // has one shape to render. - f, err := catalog.FirstFile(r.Context(), h.db, req.ItemID) + f, err := h.items.FirstFile(r.Context(), req.ItemID) if errors.Is(err, catalog.ErrNotFound) { httpx.WriteJSON(w, http.StatusOK, engine.Resolution{ Playable: false, diff --git a/internal/playback/handlers_test.go b/internal/playback/handlers_test.go index 640090a..040db7a 100644 --- a/internal/playback/handlers_test.go +++ b/internal/playback/handlers_test.go @@ -14,6 +14,7 @@ import ( "testing" "github.com/mithral/vellum/internal/auth" + "github.com/mithral/vellum/internal/catalog" "github.com/mithral/vellum/internal/core" "github.com/mithral/vellum/internal/engine" "github.com/mithral/vellum/migrations" @@ -53,7 +54,7 @@ func newHandlers(t *testing.T, db *sql.DB) *Handlers { t.Helper() log := slog.New(slog.NewTextHandler(io.Discard, nil)) a := auth.NewAuthenticator(db, "vellum_session", 0, false, log) - return NewHandlers(db, log, a) + return NewHandlers(catalog.New(db), log, a) } // fixture builds two libraries with one item each and a user granted only the