From ac1bd04df3ff2ee05fe4f57d7e8b89d52e19711d Mon Sep 17 00:00:00 2001 From: mithral Date: Sun, 11 Oct 2026 15:47:18 -0400 Subject: [PATCH] Phase 1: one item-access seam, and architecture rules that fail the build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Motivation, from the evidence rather than from taste. Five packages each implemented "load an item, then check the caller may read it": media, comics, games, jellyfin (with its own dbItem type and its own SQL), and subsonic. Five copies of one authorization rule means four can drift, and the failure mode is not a crash — it is silently serving somebody a library they were never granted. One implementation is one thing to review. internal/catalog now owns item and file access, and the access check. LoadUnchecked(ctx, db, id) -- no access check; jobs and tests only LoadFor(ctx, db, u, id) -- load AND enforce the grant, in one call Files / FilesFor / FileByID / FirstFile / FileCount The unchecked variant is deliberately named so that reaching for it by accident is hard: it is the dangerous call, so it should not have the friendlier name. This came from the interface-design rule that a good interface makes the right thing easy and the wrong thing hard. The security property is tested, not asserted: a forbidden item and an absent item return byte-identical errors, so no handler can leak the existence of another user's library through a 403. That is now enforced in one place. Migrated all five surfaces onto it. media, comics, games and ebooks went from four duplicate loaders to zero. jellyfin's dbItem and loadDBItem are gone. subsonic's per-item authorization moved while its album/artist aggregate projections stayed put, because grouping items into albums is a presentation concern, not a data-access one. The remaining raw SQL everywhere is browse and aggregate queries; no per-item load decides access any more. internal/arch enforces the layering instead of documenting it. Five rules, each proven able to fail by deliberately breaking it: - no upward imports across six explicit layers - internal/core may depend only on migrations - internal/catalog must not know about HTTP, or it stops being reusable - modules must not import each other - every package must be placed in a layer, so the map cannot go stale A test that only ever passes is decoration, so each rule was verified by introducing the violation and confirming the failure. Verified: gofmt clean, go build ./... exit 0, go vet exit 0, 20 packages pass, svelte-check 0/0. End to end against a running server: the vellum API serves item detail with codec facts, an unknown item 404s, Jellyfin AuthenticateByName mints a token and /Items browses, and Subsonic ping returns status ok. The subsonic migration was additionally A/B'd byte-identical against the previous implementation across ~15 endpoints, and Jellyfin/Subsonic both still return the same error for a forbidden item as for an absent one. Known follow-ups, recorded rather than silently skipped: - internal/jellyfin has no test files at all. Its wire contract is the most client-visible surface in the tree and is now verified only by hand. - catalog is item-centric, but Subsonic addresses songs file-first (mf-), so it resolves file to item through its own projection. A catalog lookup that takes a file id would close that. - subsonic loadAlbum now scopes the projection to the item's own library rather than all visible audio libraries. Equivalent by construction, since an item's kind tracks its library's kind, but a hand-edited mismatch would differ. --- internal/arch/arch.go | 7 + internal/arch/arch_test.go | 234 ++++++++++++++++++++++++++++ internal/catalog/catalog.go | 229 +++++++++++++++++++++++++++ internal/catalog/catalog_test.go | 221 ++++++++++++++++++++++++++ internal/jellyfin/dto.go | 126 ++++++++------- internal/jellyfin/items.go | 39 +++-- internal/jellyfin/playback.go | 26 ++-- internal/jellyfin/query.go | 172 +++++++++++++------- internal/jellyfin/sessions.go | 7 +- internal/media/handlers.go | 73 ++------- internal/media/media.go | 99 +++--------- internal/metadata/enrich.go | 16 +- internal/metadata/enrich_test.go | 2 +- internal/metadata/handlers.go | 21 ++- internal/modules/comics/handlers.go | 13 +- internal/modules/ebooks/handlers.go | 11 +- internal/modules/games/handlers.go | 14 +- internal/playback/handlers.go | 40 ++--- internal/subsonic/store.go | 31 +++- 19 files changed, 1019 insertions(+), 362 deletions(-) create mode 100644 internal/arch/arch.go create mode 100644 internal/arch/arch_test.go create mode 100644 internal/catalog/catalog.go create mode 100644 internal/catalog/catalog_test.go diff --git a/internal/arch/arch.go b/internal/arch/arch.go new file mode 100644 index 0000000..a889478 --- /dev/null +++ b/internal/arch/arch.go @@ -0,0 +1,7 @@ +// Package arch holds no code. It exists so the import rules this project +// depends on are enforced by the test suite rather than by good intentions. +// +// See arch_test.go: it parses every package's imports and fails if a layer is +// violated, if core learns about a feature, if a module imports a sibling, or +// if a new package is added without being placed in a layer. +package arch diff --git a/internal/arch/arch_test.go b/internal/arch/arch_test.go new file mode 100644 index 0000000..3c0e564 --- /dev/null +++ b/internal/arch/arch_test.go @@ -0,0 +1,234 @@ +// Package arch holds no code. It exists so that the import rules this project +// depends on are enforced by the test suite instead of by good intentions. +// +// A layering rule that is only written down in a comment is a rule that will be +// broken within a month, and the breakage is invisible: an upward import does +// not fail to compile, it just quietly welds two layers together until nothing +// can be moved. These tests fail loudly instead. +package arch + +import ( + "go/parser" + "go/token" + "os" + "path/filepath" + "sort" + "strings" + "testing" +) + +const modulePath = "github.com/mithral/vellum/" + +// Layers, lowest first. A package may import from a LOWER layer or its own +// layer, never from a higher one. Peers are allowed because they genuinely +// collaborate (metadata enriches items that media serves); upward imports are +// not, because they invert the dependency and make the lower layer unusable on +// its own. +var layers = map[string]int{ + // 0 — foundation. No knowledge of the application. + "internal/core": 0, + "internal/httpx": 0, + "internal/natsort": 0, + "internal/arch": 0, + "migrations": 0, + + // 1 — primitives the application is built from. + "internal/auth": 1, + "internal/jobs": 1, + "internal/stream": 1, + "internal/progress": 1, + "internal/probe": 1, + "internal/engine": 1, + "internal/apitoken": 1, + + // 2 — the data seam: items and access. + "internal/catalog": 2, + + // 3 — the application. + "internal/library": 3, + "internal/media": 3, + "internal/search": 3, + "internal/metadata": 3, + "internal/playback": 3, + "internal/modules/comics": 3, + "internal/modules/ebooks": 3, + "internal/modules/games": 3, + "internal/modules/photos": 3, + + // 4 — adapters: HTTP surfaces and foreign protocols. + "internal/httpapi": 4, + "internal/jellyfin": 4, + "internal/subsonic": 4, + "internal/cli": 4, + + // 5 — the only place everything is wired together. + "cmd/vellum": 5, +} + +// repoRoot walks up until it finds go.mod, so the test works regardless of +// where `go test` is invoked from. +func repoRoot(t *testing.T) string { + t.Helper() + dir, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + for { + if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil { + return dir + } + parent := filepath.Dir(dir) + if parent == dir { + t.Fatal("could not find go.mod above the test directory") + } + dir = parent + } +} + +// importsOf parses a directory and returns the internal packages it imports. +func importsOf(t *testing.T, dir string) []string { + t.Helper() + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("read %s: %v", dir, err) + } + seen := map[string]bool{} + for _, e := range entries { + if e.IsDir() || !strings.HasSuffix(e.Name(), ".go") || strings.HasSuffix(e.Name(), "_test.go") { + continue + } + fset := token.NewFileSet() + f, err := parser.ParseFile(fset, filepath.Join(dir, e.Name()), nil, parser.ImportsOnly) + if err != nil { + t.Fatalf("parse %s: %v", e.Name(), err) + } + for _, imp := range f.Imports { + p := strings.Trim(imp.Path.Value, `"`) + if strings.HasPrefix(p, modulePath) { + seen[strings.TrimPrefix(p, modulePath)] = true + } + } + } + out := make([]string, 0, len(seen)) + for p := range seen { + out = append(out, p) + } + sort.Strings(out) + return out +} + +// TestNoUpwardImports is the rule that keeps the layers separable. +func TestNoUpwardImports(t *testing.T) { + root := repoRoot(t) + for pkg, layer := range layers { + dir := filepath.Join(root, filepath.FromSlash(pkg)) + if _, err := os.Stat(dir); err != nil { + continue // package not present (e.g. a module was removed) + } + for _, dep := range importsOf(t, dir) { + depLayer, known := layers[dep] + if !known { + continue // a package outside the map; TestEveryPackageIsLayered catches it + } + if depLayer > layer { + t.Errorf("%s (layer %d) imports %s (layer %d): upward dependency", + pkg, layer, dep, depLayer) + } + } + } +} + +// TestCoreStaysPure is the module rule: core may not know about any feature, or +// it stops being the thing everything else can depend on. +func TestCoreStaysPure(t *testing.T) { + root := repoRoot(t) + for _, dep := range importsOf(t, filepath.Join(root, "internal", "core")) { + if dep != "migrations" { + t.Errorf("internal/core imports %s; core may depend only on migrations", dep) + } + } +} + +// TestCatalogHasNoHTTP keeps the data seam a data seam. If it grows an +// http.ResponseWriter it can no longer be reused by a job, a CLI or a test +// without dragging a request in. +func TestCatalogHasNoHTTP(t *testing.T) { + root := repoRoot(t) + for _, dep := range importsOf(t, filepath.Join(root, "internal", "catalog")) { + if dep == "internal/httpx" || dep == "internal/httpapi" { + t.Errorf("internal/catalog imports %s; the data seam must not know about HTTP", dep) + } + } +} + +// TestModulesDoNotImportEachOther keeps modules independently removable. A +// module that imports a sibling cannot be dropped without dropping both. +func TestModulesDoNotImportEachOther(t *testing.T) { + root := repoRoot(t) + modsDir := filepath.Join(root, "internal", "modules") + mods, err := os.ReadDir(modsDir) + if err != nil { + t.Skip("no modules directory") + } + for _, m := range mods { + if !m.IsDir() { + continue + } + self := "internal/modules/" + m.Name() + for _, dep := range importsOf(t, filepath.Join(modsDir, m.Name())) { + if strings.HasPrefix(dep, "internal/modules/") && dep != self { + t.Errorf("%s imports %s; modules must not depend on each other", self, dep) + } + } + } +} + +// TestEveryPackageIsLayered stops the map above from silently going stale. A +// new package that nobody placed in a layer is a package with no rules, which +// is how the layering erodes. +func TestEveryPackageIsLayered(t *testing.T) { + root := repoRoot(t) + var found []string + + var walk func(base string) + walk = func(base string) { + entries, err := os.ReadDir(base) + if err != nil { + return + } + for _, e := range entries { + if !e.IsDir() { + continue + } + full := filepath.Join(base, e.Name()) + rel, _ := filepath.Rel(root, full) + rel = filepath.ToSlash(rel) + // Only directories that actually contain Go files are packages. + if hasGoFiles(full) { + found = append(found, rel) + } + walk(full) + } + } + walk(filepath.Join(root, "internal")) + found = append(found, "cmd/vellum") + + for _, pkg := range found { + if _, ok := layers[pkg]; !ok { + t.Errorf("%s is not in the layers map; add it so the import rules apply to it", pkg) + } + } +} + +func hasGoFiles(dir string) bool { + entries, err := os.ReadDir(dir) + if err != nil { + return false + } + for _, e := range entries { + if !e.IsDir() && strings.HasSuffix(e.Name(), ".go") { + return true + } + } + return false +} diff --git a/internal/catalog/catalog.go b/internal/catalog/catalog.go new file mode 100644 index 0000000..6bd9208 --- /dev/null +++ b/internal/catalog/catalog.go @@ -0,0 +1,229 @@ +// Package catalog is the one place that reads items and files out of the +// database, and the one place that enforces "may this user read this item". +// +// It exists because that check had been reimplemented five times — in media, +// comics, games, jellyfin and subsonic — each with its own SQL and its own idea +// of what to do when access is denied. Five copies of one authorization rule +// means four of them can drift, and the failure mode is not a crash: it is +// silently serving somebody a library they were not granted. One implementation +// is one thing to review. +// +// Consumers depend on this package rather than on *sql.DB, which is also what +// makes the storage engine swappable: a Postgres implementation would satisfy +// this package's surface instead of editing 186 statements scattered across the +// tree. +// +// This package holds no HTTP knowledge. It returns typed errors and lets the +// caller choose a status code. +package catalog + +import ( + "context" + "database/sql" + "errors" + "fmt" + "path/filepath" + "time" + + "github.com/mithral/vellum/internal/auth" +) + +// ErrNotFound means the item does not exist, OR exists but the caller may not +// read it. +// +// The two are deliberately indistinguishable. A caller that could tell them +// apart would leak the existence of other people's libraries through a 403, so +// every handler turns this into a plain 404. +var ErrNotFound = errors.New("catalog: item not found") + +// Item is one media item plus the root of its library. +type Item struct { + ID int64 + LibraryID int64 + Kind string + Path string // path relative to the library root + Title string + Year int + Missing bool + + // RootPath is the absolute library root, so AbsPath can be computed. + RootPath string + + // ModTime is the backing file's modification time, used for conditional + // GETs. Zero when unknown. + ModTime time.Time +} + +// AbsPath returns the absolute filesystem path of the item's file. +// +// item.path is stored relative to the library root and the root is stored +// resolved, so this join cannot escape the library. +func (i Item) AbsPath() string { + return filepath.Join(i.RootPath, filepath.FromSlash(i.Path)) +} + +// File is one physical file backing an item, including the ffprobe facts. +// +// The JSON tags are the frozen MediaFile contract in web/src/lib/api.ts. The +// codec fields are what let a CLIENT decide whether it can direct-play, since +// Vellum never transcodes and playability depends on the client, not the file. +type File struct { + ID int64 `json:"id"` + Path string `json:"path"` + Size int64 `json:"size"` + Container string `json:"container"` + Missing bool `json:"missing"` + DurationMS int64 `json:"duration_ms"` + VCodec string `json:"vcodec"` + ACodec string `json:"acodec"` + Width int `json:"width"` + Height int `json:"height"` +} + +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), '') + FROM media_items i + 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) { + var ( + it Item + mtime string + ) + err := 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) { + return Item{}, ErrNotFound + } + if err != nil { + return Item{}, fmt.Errorf("catalog: load item: %w", err) + } + if nanos := parseUnixNano(mtime); nanos > 0 { + it.ModTime = time.Unix(0, nanos).UTC() + } + return it, nil +} + +// LoadFor 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) + if err != nil { + return Item{}, err + } + allowed, err := auth.CanAccessLibrary(ctx, db, u, it.LibraryID) + if err != nil { + return Item{}, fmt.Errorf("catalog: check access: %w", err) + } + if !allowed { + // Deliberately the same error as a missing row: see ErrNotFound. + return Item{}, ErrNotFound + } + return it, nil +} + +// 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, ` + SELECT id, path, size, container, missing, + duration_ms, vcodec, acodec, width, height + FROM media_files WHERE item_id = ? ORDER BY id`, itemID) + if err != nil { + return nil, fmt.Errorf("catalog: list files: %w", err) + } + defer rows.Close() + + out := []File{} + for rows.Next() { + var f File + if err := rows.Scan(&f.ID, &f.Path, &f.Size, &f.Container, &f.Missing, + &f.DurationMS, &f.VCodec, &f.ACodec, &f.Width, &f.Height); err != nil { + return nil, fmt.Errorf("catalog: scan file: %w", err) + } + out = append(out, f) + } + return out, rows.Err() +} + +// 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 { + return nil, err + } + return Files(ctx, db, 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) { + var f File + err := 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( + &f.ID, &f.Path, &f.Size, &f.Container, &f.Missing, + &f.DurationMS, &f.VCodec, &f.ACodec, &f.Width, &f.Height) + if errors.Is(err, sql.ErrNoRows) { + return File{}, ErrNotFound + } + if err != nil { + return File{}, fmt.Errorf("catalog: load file: %w", err) + } + return f, nil +} + +// 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) { + var f File + err := 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( + &f.ID, &f.Path, &f.Size, &f.Container, &f.Missing, + &f.DurationMS, &f.VCodec, &f.ACodec, &f.Width, &f.Height) + if errors.Is(err, sql.ErrNoRows) { + return File{}, ErrNotFound + } + if err != nil { + return File{}, fmt.Errorf("catalog: first file: %w", err) + } + return f, nil +} + +// FileCount reports how many files back an item. +func FileCount(ctx context.Context, db *sql.DB, itemID int64) (int, error) { + var n int + err := 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) + } + return n, nil +} + +// parseUnixNano parses a base-10 nanosecond timestamp, returning 0 on anything +// that is not all digits. +func parseUnixNano(s string) int64 { + if s == "" { + return 0 + } + var n int64 + for i := 0; i < len(s); i++ { + if s[i] < '0' || s[i] > '9' { + return 0 + } + n = n*10 + int64(s[i]-'0') + } + return n +} diff --git a/internal/catalog/catalog_test.go b/internal/catalog/catalog_test.go new file mode 100644 index 0000000..c05350a --- /dev/null +++ b/internal/catalog/catalog_test.go @@ -0,0 +1,221 @@ +package catalog + +import ( + "context" + "database/sql" + "errors" + "io" + "log/slog" + "path/filepath" + "testing" + + "github.com/mithral/vellum/internal/auth" + "github.com/mithral/vellum/internal/core" + "github.com/mithral/vellum/migrations" +) + +// testDB builds a migrated database in a temp directory. +func testDB(t *testing.T) *sql.DB { + t.Helper() + db, err := core.OpenDB(filepath.Join(t.TempDir(), "catalog.db")) + if err != nil { + t.Fatalf("open: %v", err) + } + t.Cleanup(func() { _ = db.Close() }) + log := slog.New(slog.NewTextHandler(io.Discard, nil)) + if err := core.Migrate(context.Background(), db, migrations.FS, log); err != nil { + t.Fatalf("migrate: %v", err) + } + return db +} + +// newLibrary inserts a library and returns its id. +func newLibrary(t *testing.T, db *sql.DB, name string) int64 { + t.Helper() + res, err := db.Exec( + `INSERT INTO libraries (kind, name, root_path) VALUES ('ebooks', ?, ?)`, + name, "/media/"+name) + if err != nil { + t.Fatalf("insert library: %v", err) + } + id, _ := res.LastInsertId() + return id +} + +// newUser inserts a user. isAdmin decides whether grants matter. +func newUser(t *testing.T, db *sql.DB, name string, admin bool) auth.User { + t.Helper() + adminFlag := 0 + if admin { + adminFlag = 1 + } + res, err := db.Exec( + `INSERT INTO users (username, display_name, password_hash, is_admin) VALUES (?, ?, 'x', ?)`, + name, name, adminFlag) + if err != nil { + t.Fatalf("insert user: %v", err) + } + id, _ := res.LastInsertId() + return auth.User{ID: id, Username: name, IsAdmin: admin} +} + +// newItem inserts an item with one file, including codec facts. +func newItem(t *testing.T, db *sql.DB, libID int64, title string) int64 { + t.Helper() + res, err := db.Exec(` + INSERT INTO media_items (library_id, kind, path, title, sort_title, year) + VALUES (?, 'ebooks', ?, ?, ?, 1999)`, libID, title+".epub", title, title) + if err != nil { + t.Fatalf("insert item: %v", err) + } + itemID, _ := res.LastInsertId() + if _, err := db.Exec(` + INSERT INTO media_files (item_id, path, size, mtime, container, duration_ms, + vcodec, acodec, width, height) + VALUES (?, ?, 100, '1700000000000000000', 'mp4', 5000, 'h264', 'aac', 640, 360)`, + itemID, title+".epub"); err != nil { + t.Fatalf("insert file: %v", err) + } + return itemID +} + +func grant(t *testing.T, db *sql.DB, u auth.User, libID int64) { + t.Helper() + if _, err := db.Exec( + `INSERT INTO library_grants (user_id, library_id, can_read) VALUES (?, ?, 1)`, + u.ID, libID); err != nil { + t.Fatalf("grant: %v", err) + } +} + +func TestLoadReturnsCodecFacts(t *testing.T) { + db := testDB(t) + lib := newLibrary(t, db, "A") + itemID := newItem(t, db, lib, "Thing") + + it, err := LoadUnchecked(context.Background(), db, itemID) + if err != nil { + t.Fatal(err) + } + if it.Title != "Thing" || it.LibraryID != lib || it.Kind != "ebooks" { + t.Errorf("unexpected item: %+v", it) + } + if it.RootPath != "/media/A" { + t.Errorf("RootPath = %q", it.RootPath) + } + if it.AbsPath() != "/media/A/Thing.epub" { + t.Errorf("AbsPath = %q", it.AbsPath()) + } + if it.ModTime.IsZero() { + t.Error("ModTime not parsed from the file's mtime") + } + + files, err := Files(context.Background(), db, itemID) + if err != nil { + t.Fatal(err) + } + if len(files) != 1 { + t.Fatalf("files = %d, want 1", len(files)) + } + f := files[0] + if f.VCodec != "h264" || f.ACodec != "aac" || f.Width != 640 || f.DurationMS != 5000 { + t.Errorf("codec facts lost: %+v", f) + } + if f.Missing { + t.Error("a present file reported as missing") + } +} + +// The security property: a missing item and a forbidden item must be +// INDISTINGUISHABLE, or a 403 would leak the existence of another user's +// library. +func TestLoadForForbiddenIsIndistinguishableFromAbsent(t *testing.T) { + db := testDB(t) + ctx := context.Background() + lib := newLibrary(t, db, "Secret") + itemID := newItem(t, db, lib, "Hidden") + + outsider := newUser(t, db, "outsider", false) + granted := newUser(t, db, "granted", false) + grant(t, db, granted, lib) + + // Forbidden: the item exists but this user has no grant. + _, errForbidden := LoadFor(ctx, db, outsider, itemID) + // Absent: no such row at all. + _, errAbsent := LoadFor(ctx, db, outsider, itemID+9999) + + if !errors.Is(errForbidden, ErrNotFound) { + t.Errorf("forbidden error = %v, want ErrNotFound", errForbidden) + } + if !errors.Is(errAbsent, ErrNotFound) { + t.Errorf("absent error = %v, want ErrNotFound", errAbsent) + } + if errForbidden.Error() != errAbsent.Error() { + t.Errorf("errors differ and would leak existence: %q vs %q", + errForbidden, errAbsent) + } + + // Positive control: a granted user does get it. + if _, err := LoadFor(ctx, db, granted, itemID); err != nil { + t.Errorf("granted user denied: %v", err) + } +} + +func TestAdminBypassesGrants(t *testing.T) { + db := testDB(t) + ctx := context.Background() + lib := newLibrary(t, db, "Any") + itemID := newItem(t, db, lib, "Thing") + + admin := newUser(t, db, "root", true) + if _, err := LoadFor(ctx, db, admin, itemID); err != nil { + t.Errorf("admin denied: %v", err) + } +} + +func TestFileByIDIsScopedToItsItem(t *testing.T) { + db := testDB(t) + ctx := context.Background() + lib := newLibrary(t, db, "A") + first := newItem(t, db, lib, "One") + second := newItem(t, db, lib, "Two") + + other, err := FirstFile(ctx, db, second) + if err != nil { + t.Fatal(err) + } + + // Asking for item Two's file through item One must not resolve. + if _, err := FileByID(ctx, db, first, other.ID); !errors.Is(err, ErrNotFound) { + t.Errorf("file id leaked across items: err = %v, want ErrNotFound", err) + } + if _, err := FileByID(ctx, db, second, other.ID); err != nil { + t.Errorf("legitimate lookup failed: %v", err) + } +} + +func TestFilesForRespectsGrants(t *testing.T) { + db := testDB(t) + ctx := context.Background() + lib := newLibrary(t, db, "A") + itemID := newItem(t, db, lib, "Thing") + + outsider := newUser(t, db, "outsider", false) + if _, err := FilesFor(ctx, db, outsider, itemID); !errors.Is(err, ErrNotFound) { + t.Errorf("FilesFor leaked to an ungranted user: %v", err) + } +} + +func TestFileCount(t *testing.T) { + db := testDB(t) + lib := newLibrary(t, db, "A") + itemID := newItem(t, db, lib, "Thing") + + n, err := FileCount(context.Background(), db, itemID) + if err != nil { + t.Fatal(err) + } + if n != 1 { + t.Errorf("FileCount = %d, want 1", n) + } +} diff --git a/internal/jellyfin/dto.go b/internal/jellyfin/dto.go index efc6e09..7f64b6b 100644 --- a/internal/jellyfin/dto.go +++ b/internal/jellyfin/dto.go @@ -1,6 +1,10 @@ package jellyfin -import "database/sql" +import ( + "database/sql" + + "github.com/mithral/vellum/internal/catalog" +) // This file defines a documented SUBSET of the Jellyfin DTOs. Jellyfin's // BaseItemDto has roughly a hundred fields; real clients tolerate the absence @@ -115,33 +119,33 @@ type itemQueryResult struct { StartIndex int `json:"StartIndex"` } -// dbItem is one media_items row plus the file facts and per-user state a DTO -// needs. It is the single shape both the list and single-item paths build. -type dbItem struct { - ID int64 - LibraryID int64 - Kind string - Title string - SortTitle string - Year int - ParentID sql.NullInt64 - Container string - DurationMS int64 - VCodec string - ACodec string - Width int - Height int - Size int64 - Path string - HasCover bool +// itemView is one browsable item assembled for a Jellyfin DTO. +// +// It composes the two types internal/catalog owns — the item row and its first +// file — with the per-user overlay (progress, view count, favourite) and the +// two browse-only columns catalog.Item does not carry (sort title and parent). +// Nothing here reads the database or checks a grant: catalog.LoadFor loads the +// item and enforces the grant, and catalog.FirstFile reads the file, so there +// is exactly one implementation of "may this user read this item" and exactly +// one place item and file rows are read. +type itemView struct { + Item catalog.Item + File catalog.File + + // SortTitle and ParentID are browse-only columns; the DTO maps them to + // SortName and ParentId. + SortTitle string + ParentID sql.NullInt64 + + // HasCover is true when the scanner extracted a primary image. + HasCover bool // Per-user state, zero when the user has never touched the item. - PositionMS int64 - Percent float64 - Finished bool - HasProgress bool - PlayCount int - Favorite bool + PositionMS int64 + Percent float64 + Finished bool + PlayCount int + Favorite bool } // itemType maps a Vellum media kind to the Jellyfin item type. The four types @@ -199,52 +203,54 @@ func kindsForTypes(types []string) []string { return out } -// toBaseItemDto renders one dbItem. includeMediaSources is set from the Fields -// query parameter; lists normally omit sources to keep responses small. -func toBaseItemDto(it dbItem, includeMediaSources bool, apiKey string) BaseItemDto { +// toBaseItemDto renders one itemView. includeMediaSources is set from the +// Fields query parameter; lists normally omit sources to keep responses small. +func toBaseItemDto(v itemView, includeMediaSources bool, apiKey string) BaseItemDto { dto := BaseItemDto{ - Id: itemID(it.ID), - Name: it.Title, - Type: itemType(it.Kind), - MediaType: mediaType(it.Kind), - Container: it.Container, - RunTimeTicks: msToTicks(it.DurationMS), - ProductionYear: it.Year, + Id: itemID(v.Item.ID), + Name: v.Item.Title, + Type: itemType(v.Item.Kind), + MediaType: mediaType(v.Item.Kind), + Container: v.File.Container, + RunTimeTicks: msToTicks(v.File.DurationMS), + ProductionYear: v.Item.Year, IsFolder: false, - SortName: it.SortTitle, - Path: it.Path, + SortName: v.SortTitle, + Path: v.File.Path, } - if it.ParentID.Valid { - dto.ParentId = itemID(it.ParentID.Int64) + if v.ParentID.Valid { + dto.ParentId = itemID(v.ParentID.Int64) } - if it.HasCover { + if v.HasCover { // The tag is the item id: stable, and it changes when the item changes. - dto.ImageTags = map[string]string{"Primary": itemID(it.ID)} + dto.ImageTags = map[string]string{"Primary": itemID(v.Item.ID)} } dto.UserData = &UserItemDataDto{ - PlaybackPositionTicks: msToTicks(it.PositionMS), - PlayedPercentage: it.Percent * 100, - PlayCount: it.PlayCount, - IsFavorite: it.Favorite, - Played: it.Finished, + PlaybackPositionTicks: msToTicks(v.PositionMS), + PlayedPercentage: v.Percent * 100, + PlayCount: v.PlayCount, + IsFavorite: v.Favorite, + Played: v.Finished, } if includeMediaSources { - dto.MediaSources = []MediaSourceInfo{buildMediaSource(it, apiKey)} + dto.MediaSources = []MediaSourceInfo{buildMediaSource(v.Item, v.File, apiKey)} } return dto } // buildMediaSource renders the single direct-play source for an item. It is // shared by PlaybackInfo and by list responses that requested the -// MediaSources field. -func buildMediaSource(it dbItem, apiKey string) MediaSourceInfo { +// MediaSources field. Both inputs come from internal/catalog; a zero File is +// an item with no file, and produces the same defaults a missing file always +// did. +func buildMediaSource(it catalog.Item, f catalog.File, apiKey string) MediaSourceInfo { ms := MediaSourceInfo{ Id: itemID(it.ID), Name: it.Title, - Path: it.Path, - Container: it.Container, - RunTimeTicks: msToTicks(it.DurationMS), - Size: it.Size, + Path: f.Path, + Container: f.Container, + RunTimeTicks: msToTicks(f.DurationMS), + Size: f.Size, SupportsDirectPlay: true, SupportsDirectStream: true, // The hard constraint: no transcoding, ever. @@ -255,15 +261,15 @@ func buildMediaSource(it dbItem, apiKey string) MediaSourceInfo { } ms.DirectStreamUrl = directStreamURL(it, apiKey) - if it.VCodec != "" { + if f.VCodec != "" { ms.MediaStreams = append(ms.MediaStreams, MediaStream{ - Type: "Video", Codec: it.VCodec, Index: 0, IsDefault: true, - Width: it.Width, Height: it.Height, + Type: "Video", Codec: f.VCodec, Index: 0, IsDefault: true, + Width: f.Width, Height: f.Height, }) } - if it.ACodec != "" { + if f.ACodec != "" { ms.MediaStreams = append(ms.MediaStreams, MediaStream{ - Type: "Audio", Codec: it.ACodec, Index: 1, IsDefault: true, + Type: "Audio", Codec: f.ACodec, Index: 1, IsDefault: true, }) } return ms @@ -273,7 +279,7 @@ func buildMediaSource(it dbItem, apiKey string) MediaSourceInfo { // /Videos, everything else (audio, books, photos) from /Audio, which is the // same file-serving path either way. The api_key is embedded because media // elements cannot set headers. -func directStreamURL(it dbItem, apiKey string) string { +func directStreamURL(it catalog.Item, apiKey string) string { base := "/Audio/" + itemID(it.ID) + "/stream" if it.Kind == "video" { base = "/Videos/" + itemID(it.ID) + "/stream" diff --git a/internal/jellyfin/items.go b/internal/jellyfin/items.go index fa5d99f..f7867c8 100644 --- a/internal/jellyfin/items.go +++ b/internal/jellyfin/items.go @@ -8,6 +8,7 @@ import ( "strings" "github.com/mithral/vellum/internal/auth" + "github.com/mithral/vellum/internal/catalog" "github.com/mithral/vellum/internal/httpx" "github.com/mithral/vellum/internal/library" "github.com/mithral/vellum/internal/stream" @@ -159,7 +160,20 @@ func (h *Handlers) handleItem(w http.ResponseWriter, r *http.Request, u auth.Use if !ok { return } - httpx.WriteJSON(w, http.StatusOK, toBaseItemDto(it, true, tokenFromRequest(r))) + v := itemView{Item: it} + if err := h.attachFile(r.Context(), &v); err != nil { + h.fail(w, "load file", err) + return + } + if err := h.loadOverlay(r.Context(), u.ID, it.ID, &v); err != nil { + if errors.Is(err, sql.ErrNoRows) { + httpx.WriteError(w, http.StatusNotFound, "item not found") + return + } + h.fail(w, "load item state", err) + return + } + httpx.WriteJSON(w, http.StatusOK, toBaseItemDto(v, true, tokenFromRequest(r))) } // handleImagePrimary streams the cover bytes the scanner/web already extracted. @@ -197,25 +211,18 @@ func (h *Handlers) handleImagePrimary(w http.ResponseWriter, r *http.Request, u } // authorizedItem loads an item and confirms access, writing a 404 (never a 403, -// so existence is not disclosed) on failure. -func (h *Handlers) authorizedItem(w http.ResponseWriter, r *http.Request, u auth.User, id int64) (dbItem, bool) { - it, err := h.loadDBItem(r.Context(), u.ID, id) - if errors.Is(err, sql.ErrNoRows) { +// so existence is not disclosed) on failure. Both the load and the grant check +// live in catalog.LoadFor, which returns the same ErrNotFound for a missing +// item and an unreadable one. +func (h *Handlers) authorizedItem(w http.ResponseWriter, r *http.Request, u auth.User, id int64) (catalog.Item, bool) { + it, err := catalog.LoadFor(r.Context(), h.db, u, id) + if errors.Is(err, catalog.ErrNotFound) { httpx.WriteError(w, http.StatusNotFound, "item not found") - return dbItem{}, false + return catalog.Item{}, false } if err != nil { h.fail(w, "load item", err) - return dbItem{}, false - } - allowed, err := auth.CanAccessLibrary(r.Context(), h.db, u, it.LibraryID) - if err != nil { - h.fail(w, "check access", err) - return dbItem{}, false - } - if !allowed { - httpx.WriteError(w, http.StatusNotFound, "item not found") - return dbItem{}, false + return catalog.Item{}, false } return it, true } diff --git a/internal/jellyfin/playback.go b/internal/jellyfin/playback.go index 7dce81c..b65e72e 100644 --- a/internal/jellyfin/playback.go +++ b/internal/jellyfin/playback.go @@ -1,7 +1,6 @@ package jellyfin import ( - "database/sql" "errors" "net/http" "os" @@ -10,6 +9,7 @@ import ( "strings" "github.com/mithral/vellum/internal/auth" + "github.com/mithral/vellum/internal/catalog" "github.com/mithral/vellum/internal/httpx" "github.com/mithral/vellum/internal/stream" ) @@ -31,12 +31,17 @@ func (h *Handlers) handlePlaybackInfo(w http.ResponseWriter, r *http.Request, u if !ok { return } + f, err := h.firstFile(r.Context(), id) + if err != nil { + h.fail(w, "load file facts", err) + return + } q := r.URL.Query() directPlay := queryBoolDefault(q.Get("EnableDirectPlay"), true) directStream := queryBoolDefault(q.Get("EnableDirectStream"), true) - src := buildMediaSource(it, tokenFromRequest(r)) + src := buildMediaSource(it, f, tokenFromRequest(r)) resp := PlaybackInfoResponse{MediaSources: []MediaSourceInfo{src}} if !directPlay && !directStream { resp.ErrorCode = "NoCompatibleStream" @@ -75,19 +80,13 @@ func (h *Handlers) serveStream(w http.ResponseWriter, r *http.Request, u auth.Us httpx.WriteError(w, http.StatusNotFound, "item not found") return } - if _, ok := h.authorizedItem(w, r, u, id); !ok { + it, ok := h.authorizedItem(w, r, u, id) + if !ok { return } - var root, rel string - err := h.db.QueryRowContext(r.Context(), ` - SELECT l.root_path, f.path - FROM media_items i - JOIN libraries l ON l.id = i.library_id - JOIN media_files f ON f.item_id = i.id - WHERE i.id = ? - ORDER BY f.id LIMIT 1`, id).Scan(&root, &rel) - if errors.Is(err, sql.ErrNoRows) { + f, err := catalog.FirstFile(r.Context(), h.db, id) + if errors.Is(err, catalog.ErrNotFound) { httpx.WriteError(w, http.StatusNotFound, "item has no file") return } @@ -97,7 +96,8 @@ func (h *Handlers) serveStream(w http.ResponseWriter, r *http.Request, u auth.Us } w.Header().Set("Content-Disposition", "inline") - if err := stream.ServeFile(w, r, filepath.Join(root, filepath.FromSlash(rel))); err != nil { + abs := filepath.Join(it.RootPath, filepath.FromSlash(f.Path)) + if err := stream.ServeFile(w, r, abs); err != nil { if errors.Is(err, os.ErrNotExist) { httpx.WriteError(w, http.StatusNotFound, "file is not available") return diff --git a/internal/jellyfin/query.go b/internal/jellyfin/query.go index f3c9857..705470d 100644 --- a/internal/jellyfin/query.go +++ b/internal/jellyfin/query.go @@ -3,11 +3,13 @@ package jellyfin import ( "context" "database/sql" + "errors" "sort" "strconv" "strings" "github.com/mithral/vellum/internal/auth" + "github.com/mithral/vellum/internal/catalog" ) // defaultLimit caps an unbounded browse so a client that omits Limit cannot @@ -17,29 +19,33 @@ const ( maxLimit = 1000 ) -// itemColumns / itemFrom are shared by the list and single-item queries so the -// two paths can never disagree about what a dbItem contains. +// overlayColumns / overlayFrom carry the per-user state and the two browse-only +// columns (sort title, parent) that catalog.Item does not expose. They never +// read media_files and never check a grant: catalog.LoadFor loads the item and +// enforces the grant, and catalog.FirstFile reads the file, so the one +// authorization rule lives in one place instead of a sixth copy here. // -// File facts come from the item's first media_files row (multi-file items such -// as audiobook chapters are still one browsable item). Per-user state comes -// from a windowed progress join that picks the most recently updated row per -// item, plus item_views and favorites. -const itemColumns = `i.id, i.library_id, i.kind, i.title, i.sort_title, COALESCE(i.year,0), - i.parent_id, - COALESCE(f.container,''), COALESCE(f.duration_ms,0), COALESCE(f.vcodec,''), COALESCE(f.acodec,''), - COALESCE(f.width,0), COALESCE(f.height,0), COALESCE(f.size,0), COALESCE(f.path,''), - p.item_id, COALESCE(p.position,0), COALESCE(p.percent,0), COALESCE(p.finished,0), +// Per-user state comes from a windowed progress join that picks the most +// recently updated row per item, plus item_views and favorites. +const overlayColumns = `i.sort_title, i.parent_id, + COALESCE(p.position,0), COALESCE(p.percent,0), COALESCE(p.finished,0), COALESCE(v.views,0), COALESCE(fa.user_id,0), COALESCE((SELECT 1 FROM assets a WHERE a.item_id=i.id AND a.role='cover' LIMIT 1),0)` -const itemFrom = `FROM media_items i - LEFT JOIN media_files f ON f.id = (SELECT id FROM media_files WHERE item_id=i.id ORDER BY id LIMIT 1) +const overlayFrom = `FROM media_items i LEFT JOIN (SELECT item_id, position, percent, finished, ROW_NUMBER() OVER (PARTITION BY item_id ORDER BY updated_at DESC) AS rn FROM progress WHERE user_id = ?) p ON p.item_id = i.id AND p.rn = 1 LEFT JOIN item_views v ON v.user_id = ? AND v.item_id = i.id LEFT JOIN favorites fa ON fa.user_id = ? AND fa.item_id = i.id` +// itemColumns is the browse projection: the catalog.Item identity columns plus +// the per-user overlay. The list and single-item paths share it so the two can +// never disagree about what a browse row contains. File facts are attached +// separately via catalog.FirstFile. +const itemColumns = `i.id, i.library_id, i.kind, i.path, i.title, COALESCE(i.year,0), i.missing, ` + + overlayColumns + // listQuery is everything the browse query needs. type listQuery struct { userID int64 @@ -56,7 +62,11 @@ type listQuery struct { } // queryItems runs the browse query and its matching count. -func (h *Handlers) queryItems(ctx context.Context, q listQuery) ([]dbItem, int, error) { +// +// The browse rows are drained before the per-item file lookups run: catalog +// reads one file at a time, and issuing those queries against a still-open +// result set is the classic nested-query footgun. +func (h *Handlers) queryItems(ctx context.Context, q listQuery) ([]itemView, int, error) { where, whereArgs := buildWhere(q) var total int @@ -71,7 +81,7 @@ func (h *Handlers) queryItems(ctx context.Context, q listQuery) ([]dbItem, int, } else { order += " ASC" } - sqlText := `SELECT ` + itemColumns + ` ` + itemFrom + + sqlText := `SELECT ` + itemColumns + ` ` + overlayFrom + ` WHERE ` + where + ` ORDER BY ` + order + ` LIMIT ? OFFSET ?` args := []any{q.userID, q.userID, q.userID} @@ -84,67 +94,115 @@ func (h *Handlers) queryItems(ctx context.Context, q listQuery) ([]dbItem, int, } defer rows.Close() - out := []dbItem{} + out := []itemView{} for rows.Next() { - it, err := scanItem(rows) + v, err := scanItem(rows) if err != nil { return nil, 0, err } - out = append(out, it) + out = append(out, v) } - return out, total, rows.Err() + if err := rows.Err(); err != nil { + return nil, 0, err + } + rows.Close() + + for i := range out { + if err := h.attachFile(ctx, &out[i]); err != nil { + return nil, 0, err + } + } + return out, total, nil } -// loadDBItem loads one item with the same columns as the browse query. -func (h *Handlers) loadDBItem(ctx context.Context, userID, id int64) (dbItem, error) { - row := h.db.QueryRowContext(ctx, - `SELECT `+itemColumns+` `+itemFrom+` WHERE i.id = ?`, - userID, userID, userID, id) - return scanItem(row) -} - -// itemVisible reports whether the caller may read the item's library. A -// missing item is false, not an error. -func (h *Handlers) itemVisible(ctx context.Context, u auth.User, id int64) (bool, error) { - var libID int64 - err := h.db.QueryRowContext(ctx, - `SELECT library_id FROM media_items WHERE id = ?`, id).Scan(&libID) - if err == sql.ErrNoRows { - return false, nil - } +// attachFile fills the first-file facts from internal/catalog, the one place +// that reads media_files. An item with no file keeps a zero File, which is +// exactly the defaults the old media_files LEFT JOIN produced. +func (h *Handlers) attachFile(ctx context.Context, v *itemView) error { + f, err := h.firstFile(ctx, v.Item.ID) if err != nil { + return err + } + v.File = f + return nil +} + +// firstFile returns the item's first file, or a zero File when the item has +// none. It is catalog.FirstFile with the "no file" case flattened, for callers +// that only need the facts rather than a 404. +func (h *Handlers) firstFile(ctx context.Context, itemID int64) (catalog.File, error) { + f, err := catalog.FirstFile(ctx, h.db, itemID) + if errors.Is(err, catalog.ErrNotFound) { + return catalog.File{}, nil + } + return f, err +} + +// loadOverlay fills the browse-only and per-user fields of an already-loaded +// item. It reads no file and performs no access check: catalog.LoadFor already +// did both. +func (h *Handlers) loadOverlay(ctx context.Context, userID, itemID int64, v *itemView) error { + row := h.db.QueryRowContext(ctx, + `SELECT `+overlayColumns+` `+overlayFrom+` WHERE i.id = ?`, + userID, userID, userID, itemID) + var ( + parentID sql.NullInt64 + finished int + favUser int64 + hasCover int + ) + if err := row.Scan( + &v.SortTitle, &parentID, + &v.PositionMS, &v.Percent, &finished, + &v.PlayCount, &favUser, &hasCover, + ); err != nil { + return err + } + v.ParentID = parentID + v.Finished = finished == 1 + v.Favorite = favUser != 0 + v.HasCover = hasCover == 1 + return nil +} + +// itemVisible reports whether the caller may read the item's library. A missing +// item is false, not an error. It delegates to catalog.LoadFor, the same +// chokepoint every other surface uses, rather than reimplementing the grant +// rule here. +func (h *Handlers) itemVisible(ctx context.Context, u auth.User, id int64) (bool, error) { + if _, err := catalog.LoadFor(ctx, h.db, u, id); err != nil { + if errors.Is(err, catalog.ErrNotFound) { + return false, nil + } return false, err } - return auth.CanAccessLibrary(ctx, h.db, u, libID) + return true, nil } // scanItem reads one row of the shared item column list. -func scanItem(row interface{ Scan(...any) error }) (dbItem, error) { +func scanItem(row interface{ Scan(...any) error }) (itemView, error) { var ( - it dbItem - parentID sql.NullInt64 - hasProgress sql.NullInt64 - finished int - favUser int64 - hasCover int + v itemView + parentID sql.NullInt64 + finished int + favUser int64 + hasCover int ) err := row.Scan( - &it.ID, &it.LibraryID, &it.Kind, &it.Title, &it.SortTitle, &it.Year, - &parentID, - &it.Container, &it.DurationMS, &it.VCodec, &it.ACodec, - &it.Width, &it.Height, &it.Size, &it.Path, - &hasProgress, &it.PositionMS, &it.Percent, &finished, - &it.PlayCount, &favUser, &hasCover, + &v.Item.ID, &v.Item.LibraryID, &v.Item.Kind, &v.Item.Path, &v.Item.Title, + &v.Item.Year, &v.Item.Missing, + &v.SortTitle, &parentID, + &v.PositionMS, &v.Percent, &finished, + &v.PlayCount, &favUser, &hasCover, ) if err != nil { - return dbItem{}, err + return itemView{}, err } - it.ParentID = parentID - it.HasProgress = hasProgress.Valid - it.Finished = finished == 1 - it.Favorite = favUser != 0 - it.HasCover = hasCover == 1 - return it, nil + v.ParentID = parentID + v.Finished = finished == 1 + v.Favorite = favUser != 0 + v.HasCover = hasCover == 1 + return v, nil } // buildWhere assembles the WHERE clause and its arguments. An empty library set diff --git a/internal/jellyfin/sessions.go b/internal/jellyfin/sessions.go index 6bb0953..5267bf8 100644 --- a/internal/jellyfin/sessions.go +++ b/internal/jellyfin/sessions.go @@ -67,13 +67,12 @@ func (h *Handlers) writePlaybackProgress(w http.ResponseWriter, r *http.Request, // Duration comes from the item's first file, the same source the browse // DTOs use, so PlayedPercentage is consistent across endpoints. - var durationMS int64 - if err := h.db.QueryRowContext(r.Context(), - `SELECT COALESCE((SELECT duration_ms FROM media_files WHERE item_id = ? ORDER BY id LIMIT 1), 0)`, - id).Scan(&durationMS); err != nil { + f, err := h.firstFile(r.Context(), id) + if err != nil { h.fail(w, "load duration", err) return } + durationMS := f.DurationMS posMS := ticksToMS(rep.PositionTicks) if posMS < 0 { diff --git a/internal/media/handlers.go b/internal/media/handlers.go index 512c1e0..d2c22f2 100644 --- a/internal/media/handlers.go +++ b/internal/media/handlers.go @@ -10,6 +10,7 @@ import ( "strconv" "github.com/mithral/vellum/internal/auth" + "github.com/mithral/vellum/internal/catalog" "github.com/mithral/vellum/internal/httpx" "github.com/mithral/vellum/internal/progress" "github.com/mithral/vellum/internal/stream" @@ -158,10 +159,8 @@ func (h *Handlers) handleFileByID(w http.ResponseWriter, r *http.Request, u auth // Confirm the file belongs to this item, so an item id cannot be paired // with another item's file id. - var relPath string - err = h.db.QueryRowContext(r.Context(), - `SELECT path FROM media_files WHERE id = ? AND item_id = ?`, fileID, it.ID).Scan(&relPath) - if errors.Is(err, sql.ErrNoRows) { + f, err := catalog.FileByID(r.Context(), h.db, it.ID, fileID) + if errors.Is(err, catalog.ErrNotFound) { httpx.WriteError(w, http.StatusNotFound, "file not found") return } @@ -170,7 +169,7 @@ func (h *Handlers) handleFileByID(w http.ResponseWriter, r *http.Request, u auth return } - abs := filepath.Join(it.RootPath, filepath.FromSlash(relPath)) + abs := filepath.Join(it.RootPath, filepath.FromSlash(f.Path)) w.Header().Set("Content-Disposition", "inline") if err := stream.ServeFile(w, r, abs); err != nil { if errors.Is(err, os.ErrNotExist) { @@ -276,6 +275,9 @@ func (h *Handlers) handleRecent(w http.ResponseWriter, r *http.Request, u auth.U // authorized loads an item and confirms the caller may read its library. // On failure it has already written the response. +// +// The load and the access check both live in catalog.LoadFor, so this package +// no longer carries its own copy of the "may this user read this item" rule. func (h *Handlers) authorized(w http.ResponseWriter, r *http.Request, u auth.User) (Item, bool) { id, err := strconv.ParseInt(r.PathValue("id"), 10, 64) if err != nil || id <= 0 { @@ -283,8 +285,8 @@ func (h *Handlers) authorized(w http.ResponseWriter, r *http.Request, u auth.Use return Item{}, false } - it, err := LoadItem(r.Context(), h.db, id) - if errors.Is(err, ErrItemNotFound) { + it, err := catalog.LoadFor(r.Context(), h.db, u, id) + if errors.Is(err, catalog.ErrNotFound) { httpx.WriteError(w, http.StatusNotFound, "item not found") return Item{}, false } @@ -292,59 +294,16 @@ func (h *Handlers) authorized(w http.ResponseWriter, r *http.Request, u auth.Use h.fail(w, "load item", err) return Item{}, false } - - allowed, err := auth.CanAccessLibrary(r.Context(), h.db, u, it.LibraryID) - if err != nil { - h.fail(w, "check access", err) - return Item{}, false - } - if !allowed { - httpx.WriteError(w, http.StatusNotFound, "item not found") - return Item{}, false - } return it, true } -// fileDTO is one physical file in an item's detail response. It carries the -// ffprobe codec facts alongside the container, because direct play is a -// decision each client makes for itself: the server reports what is in the file -// and never transcodes. The wire shape is frozen by MediaFile in -// web/src/lib/api.ts. -type fileDTO struct { - ID int64 `json:"id"` - Path string `json:"path"` - Size int64 `json:"size"` - Container string `json:"container"` - Missing bool `json:"missing"` - DurationMS int64 `json:"duration_ms"` - VCodec string `json:"vcodec"` - ACodec string `json:"acodec"` - Width int `json:"width"` - Height int `json:"height"` -} - -func (h *Handlers) files(r *http.Request, itemID int64) ([]fileDTO, error) { - rows, err := h.db.QueryContext(r.Context(), ` - SELECT id, path, size, container, missing, - duration_ms, vcodec, acodec, width, height - FROM media_files WHERE item_id = ? ORDER BY id`, itemID) - if err != nil { - return nil, err - } - defer rows.Close() - - out := []fileDTO{} - for rows.Next() { - var f fileDTO - var missing int - if err := rows.Scan(&f.ID, &f.Path, &f.Size, &f.Container, &missing, - &f.DurationMS, &f.VCodec, &f.ACodec, &f.Width, &f.Height); err != nil { - return nil, err - } - f.Missing = missing == 1 - out = append(out, f) - } - return out, rows.Err() +// files returns an item's physical files with their ffprobe codec facts. +// +// The shape is catalog.File, whose JSON tags are frozen by MediaFile in +// web/src/lib/api.ts. The server reports what is IN the file and never +// transcodes; each client decides whether it can direct-play it. +func (h *Handlers) files(r *http.Request, itemID int64) ([]catalog.File, error) { + return catalog.Files(r.Context(), h.db, itemID) } func (h *Handlers) hasCover(r *http.Request, it Item) bool { diff --git a/internal/media/media.go b/internal/media/media.go index 0358bb7..fda97f0 100644 --- a/internal/media/media.go +++ b/internal/media/media.go @@ -1,97 +1,34 @@ // Package media serves generic item endpoints: detail, the raw file, cover // art, and progress. It knows nothing about any specific media format; modules // register cover extractors and expose their own metadata endpoints. +// +// Item and file ACCESS does not live here any more — it lives in +// internal/catalog, so that the web app, the modules, Jellyfin and Subsonic all +// share one implementation of "may this user read this item" instead of five. +// The names below are kept as aliases so existing callers do not have to move +// in the same step; new code should import catalog directly. package media import ( "context" "database/sql" - "errors" - "fmt" - "path/filepath" - "time" + + "github.com/mithral/vellum/internal/catalog" ) -// ErrItemNotFound means no item matched. -var ErrItemNotFound = errors.New("media: item not found") +// Item is one media item plus the root of its library. See catalog.Item. +type Item = catalog.Item -// Item is one media item plus the root of its library. -type Item struct { - ID int64 - LibraryID int64 - Kind string - Path string // path relative to the library root - Title string - Year int - Missing bool +// FileEntry is one physical file backing an item, with its codec facts. +// See catalog.File. +type FileEntry = catalog.File - // RootPath is the absolute library root, so AbsPath can be computed. - RootPath string - - // ModTime is the backing file's modification time, used for conditional - // GETs. Zero when unknown. - ModTime time.Time -} - -// AbsPath returns the absolute filesystem path of the item's file. -// -// item.path is stored relative to the library root and the root is stored -// resolved, so this join cannot escape the library. -func (i Item) AbsPath() string { - return filepath.Join(i.RootPath, filepath.FromSlash(i.Path)) -} - -// LoadItem loads an item and its library root. -func LoadItem(ctx context.Context, db *sql.DB, id int64) (Item, error) { - var it Item - var mtime string - err := db.QueryRowContext(ctx, ` - 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), '') - FROM media_items i - JOIN libraries l ON l.id = i.library_id - WHERE i.id = ?`, 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) { - return Item{}, ErrItemNotFound - } - if err != nil { - return Item{}, fmt.Errorf("media: load item: %w", err) - } - if nanos := parseUnixNano(mtime); nanos > 0 { - it.ModTime = time.Unix(0, nanos).UTC() - } - return it, nil -} - -// parseUnixNano parses a base-10 nanosecond timestamp, returning 0 on anything -// that is not all digits. -func parseUnixNano(s string) int64 { - if s == "" { - return 0 - } - var n int64 - for i := 0; i < len(s); i++ { - if s[i] < '0' || s[i] > '9' { - return 0 - } - n = n*10 + int64(s[i]-'0') - } - return n -} - -// FileEntry is one physical file backing an item. -type FileEntry struct { - ID int64 `json:"id"` - Path string `json:"path"` - Size int64 `json:"size"` - Container string `json:"container"` - Missing bool `json:"missing"` -} +// ErrItemNotFound means no item matched, or the caller may not read it. The two +// are deliberately indistinguishable so a handler cannot leak the existence of +// another user's library. See catalog.ErrNotFound. +var ErrItemNotFound = catalog.ErrNotFound // FileCount returns how many files back an item. func FileCount(ctx context.Context, db *sql.DB, itemID int64) (int, error) { - var n int - err := db.QueryRowContext(ctx, `SELECT count(*) FROM media_files WHERE item_id = ?`, itemID).Scan(&n) - return n, err + return catalog.FileCount(ctx, db, itemID) } diff --git a/internal/metadata/enrich.go b/internal/metadata/enrich.go index 9a314d1..6b65754 100644 --- a/internal/metadata/enrich.go +++ b/internal/metadata/enrich.go @@ -15,6 +15,7 @@ import ( "strings" "time" + "github.com/mithral/vellum/internal/catalog" "github.com/mithral/vellum/internal/core" "github.com/mithral/vellum/internal/library" "github.com/mithral/vellum/internal/media" @@ -76,9 +77,14 @@ func (e *Enricher) Handle(ctx context.Context, payload json.RawMessage) error { // returns an error only for a database failure, because a missing match is a // normal outcome and must not fail the job. func (e *Enricher) EnrichItem(ctx context.Context, itemID int64) error { - it, err := media.LoadItem(ctx, e.db, itemID) + // This runs as a background job, not a request, so there is no user and no + // grant to check. catalog.Load is the unchecked read, which is correct + // here: the job acts on an item the scanner already admitted to the + // catalogue, and a job that silently skipped its work because of a grant + // check it cannot perform would be a bug. + it, err := catalog.LoadUnchecked(ctx, e.db, itemID) if err != nil { - if errors.Is(err, media.ErrItemNotFound) { + if errors.Is(err, catalog.ErrNotFound) { return nil // deleted since the job was queued } return err @@ -150,7 +156,7 @@ func (e *Enricher) EnrichItem(ctx context.Context, itemID int64) error { // so the parent directory is a strong signal that the provider search should // use to disambiguate. Without it, "Random Access Memories" matches a release // by an unrelated artist. -func artistHint(it media.Item) string { +func artistHint(it catalog.Item) string { if library.GroupModeFor(it.Kind) != library.GroupByDirectory { return "" } @@ -237,7 +243,7 @@ func normalizeTitle(s string) string { } // apply writes a chosen candidate back to the database. -func (e *Enricher) apply(ctx context.Context, it media.Item, c Candidate) error { +func (e *Enricher) apply(ctx context.Context, it catalog.Item, c Candidate) error { tx, err := e.db.BeginTx(ctx, nil) if err != nil { return err @@ -301,7 +307,7 @@ func (e *Enricher) apply(ctx context.Context, it media.Item, c Candidate) error } // downloadCover fetches a cover into the cache and records it as an asset. -func (e *Enricher) downloadCover(ctx context.Context, it media.Item, c Candidate) error { +func (e *Enricher) downloadCover(ctx context.Context, it catalog.Item, c Candidate) error { var existing int if err := e.db.QueryRowContext(ctx, `SELECT count(*) FROM assets WHERE item_id = ? AND role = 'cover'`, it.ID).Scan(&existing); err != nil { diff --git a/internal/metadata/enrich_test.go b/internal/metadata/enrich_test.go index 66e52e7..f9bb8c9 100644 --- a/internal/metadata/enrich_test.go +++ b/internal/metadata/enrich_test.go @@ -44,7 +44,7 @@ func seedItem(t *testing.T, db *sql.DB, title string) int64 { libID, title, title).Scan(&itemID); err != nil { t.Fatal(err) } - // A file row so media.LoadItem can resolve a modtime. + // A file row so catalog.Load can resolve a modtime. if _, err := db.ExecContext(ctx, `INSERT INTO media_files (item_id, path, size, mtime) VALUES (?, 'book.epub', 1, '1')`, itemID); err != nil { t.Fatal(err) diff --git a/internal/metadata/handlers.go b/internal/metadata/handlers.go index e7ba5fb..361f7fb 100644 --- a/internal/metadata/handlers.go +++ b/internal/metadata/handlers.go @@ -8,9 +8,9 @@ import ( "strconv" "github.com/mithral/vellum/internal/auth" + "github.com/mithral/vellum/internal/catalog" "github.com/mithral/vellum/internal/httpx" "github.com/mithral/vellum/internal/jobs" - "github.com/mithral/vellum/internal/media" ) // Handlers serves metadata endpoints. @@ -97,20 +97,17 @@ func (h *Handlers) handleItemMetadata(w http.ResponseWriter, r *http.Request, u return } - it, err := media.LoadItem(r.Context(), h.db, id) - if errors.Is(err, media.ErrItemNotFound) { - httpx.WriteError(w, http.StatusNotFound, "item not found") - return - } - if err != nil { + // catalog.LoadFor is the authorization chokepoint: it returns the same + // ErrNotFound for a missing item and an unreadable one, so this stays a + // 404 either way and never leaks the existence of another user's library. + if _, err := catalog.LoadFor(r.Context(), h.db, u, id); err != nil { + if errors.Is(err, catalog.ErrNotFound) { + httpx.WriteError(w, http.StatusNotFound, "item not found") + return + } h.fail(w, "load item", err) return } - allowed, err := auth.CanAccessLibrary(r.Context(), h.db, u, it.LibraryID) - if err != nil || !allowed { - httpx.WriteError(w, http.StatusNotFound, "item not found") - return - } type providerID struct { Provider string `json:"provider"` diff --git a/internal/modules/comics/handlers.go b/internal/modules/comics/handlers.go index 94a6149..a7bc94a 100644 --- a/internal/modules/comics/handlers.go +++ b/internal/modules/comics/handlers.go @@ -9,6 +9,7 @@ import ( "strconv" "github.com/mithral/vellum/internal/auth" + "github.com/mithral/vellum/internal/catalog" "github.com/mithral/vellum/internal/httpx" "github.com/mithral/vellum/internal/media" "github.com/mithral/vellum/internal/stream" @@ -84,6 +85,9 @@ func (h *Handlers) handlePage(w http.ResponseWriter, r *http.Request, u auth.Use } // authorized loads an item, confirms access, and checks it is a comic. +// +// The load and the access check both live in catalog.LoadFor; this package no +// longer reimplements "may this user read this item". func (h *Handlers) authorized(w http.ResponseWriter, r *http.Request, u auth.User) (media.Item, bool) { id, err := strconv.ParseInt(r.PathValue("id"), 10, 64) if err != nil || id <= 0 { @@ -91,8 +95,8 @@ func (h *Handlers) authorized(w http.ResponseWriter, r *http.Request, u auth.Use return media.Item{}, false } - it, err := media.LoadItem(r.Context(), h.db, id) - if errors.Is(err, media.ErrItemNotFound) { + it, err := catalog.LoadFor(r.Context(), h.db, u, id) + if errors.Is(err, catalog.ErrNotFound) { httpx.WriteError(w, http.StatusNotFound, "item not found") return media.Item{}, false } @@ -102,11 +106,6 @@ func (h *Handlers) authorized(w http.ResponseWriter, r *http.Request, u auth.Use return media.Item{}, false } - allowed, err := auth.CanAccessLibrary(r.Context(), h.db, u, it.LibraryID) - if err != nil || !allowed { - httpx.WriteError(w, http.StatusNotFound, "item not found") - return media.Item{}, false - } if it.Kind != Kind { httpx.WriteError(w, http.StatusBadRequest, "item is not a comic") return media.Item{}, false diff --git a/internal/modules/ebooks/handlers.go b/internal/modules/ebooks/handlers.go index 41deab6..0de65b5 100644 --- a/internal/modules/ebooks/handlers.go +++ b/internal/modules/ebooks/handlers.go @@ -8,8 +8,8 @@ import ( "strconv" "github.com/mithral/vellum/internal/auth" + "github.com/mithral/vellum/internal/catalog" "github.com/mithral/vellum/internal/httpx" - "github.com/mithral/vellum/internal/media" ) // Handlers serves ebook-specific endpoints. @@ -40,8 +40,8 @@ func (h *Handlers) handleMetadata(w http.ResponseWriter, r *http.Request, u auth return } - it, err := media.LoadItem(r.Context(), h.db, id) - if errors.Is(err, media.ErrItemNotFound) { + it, err := catalog.LoadFor(r.Context(), h.db, u, id) + if errors.Is(err, catalog.ErrNotFound) { httpx.WriteError(w, http.StatusNotFound, "item not found") return } @@ -51,11 +51,6 @@ func (h *Handlers) handleMetadata(w http.ResponseWriter, r *http.Request, u auth return } - allowed, err := auth.CanAccessLibrary(r.Context(), h.db, u, it.LibraryID) - if err != nil || !allowed { - httpx.WriteError(w, http.StatusNotFound, "item not found") - return - } if it.Kind != Kind { httpx.WriteError(w, http.StatusBadRequest, "item is not an ebook") return diff --git a/internal/modules/games/handlers.go b/internal/modules/games/handlers.go index 31ff5fc..9eba0a3 100644 --- a/internal/modules/games/handlers.go +++ b/internal/modules/games/handlers.go @@ -10,6 +10,7 @@ import ( "strings" "github.com/mithral/vellum/internal/auth" + "github.com/mithral/vellum/internal/catalog" "github.com/mithral/vellum/internal/httpx" "github.com/mithral/vellum/internal/media" ) @@ -174,6 +175,10 @@ func slotName(w http.ResponseWriter, r *http.Request) (string, bool) { return slot, true } +// authorized loads an item, confirms access, and checks it is a game. +// +// The load and the access check both live in catalog.LoadFor; this package no +// longer reimplements "may this user read this item". func (h *Handlers) authorized(w http.ResponseWriter, r *http.Request, u auth.User) (media.Item, bool) { id, err := strconv.ParseInt(r.PathValue("id"), 10, 64) if err != nil || id <= 0 { @@ -181,8 +186,8 @@ func (h *Handlers) authorized(w http.ResponseWriter, r *http.Request, u auth.Use return media.Item{}, false } - it, err := media.LoadItem(r.Context(), h.db, id) - if errors.Is(err, media.ErrItemNotFound) { + it, err := catalog.LoadFor(r.Context(), h.db, u, id) + if errors.Is(err, catalog.ErrNotFound) { httpx.WriteError(w, http.StatusNotFound, "item not found") return media.Item{}, false } @@ -191,11 +196,6 @@ func (h *Handlers) authorized(w http.ResponseWriter, r *http.Request, u auth.Use return media.Item{}, false } - allowed, err := auth.CanAccessLibrary(r.Context(), h.db, u, it.LibraryID) - if err != nil || !allowed { - httpx.WriteError(w, http.StatusNotFound, "item not found") - return media.Item{}, false - } if it.Kind != Kind { httpx.WriteError(w, http.StatusBadRequest, "item is not a game") return media.Item{}, false diff --git a/internal/playback/handlers.go b/internal/playback/handlers.go index 8e62f20..826b70b 100644 --- a/internal/playback/handlers.go +++ b/internal/playback/handlers.go @@ -15,6 +15,7 @@ import ( "net/http" "github.com/mithral/vellum/internal/auth" + "github.com/mithral/vellum/internal/catalog" "github.com/mithral/vellum/internal/engine" "github.com/mithral/vellum/internal/httpx" ) @@ -63,39 +64,24 @@ func (h *Handlers) handleResolve(w http.ResponseWriter, r *http.Request, u auth. return } - // The same access-control chokepoint the media handlers use: load the item's - // library, then ask CanAccessLibrary. An item the user cannot read is a 404, - // not a 403, so its existence is not disclosed. - var libraryID int64 - err := h.db.QueryRowContext(r.Context(), - `SELECT library_id FROM media_items WHERE id = ?`, req.ItemID).Scan(&libraryID) - if errors.Is(err, sql.ErrNoRows) { - httpx.WriteError(w, http.StatusNotFound, "item not found") - return - } - if err != nil { + // catalog.LoadFor is the shared access-control chokepoint: it reads the + // 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 errors.Is(err, catalog.ErrNotFound) { + httpx.WriteError(w, http.StatusNotFound, "item not found") + return + } h.fail(w, "load item", err) return } - allowed, err := auth.CanAccessLibrary(r.Context(), h.db, u, libraryID) - if err != nil { - h.fail(w, "check access", err) - return - } - if !allowed { - httpx.WriteError(w, http.StatusNotFound, "item not found") - return - } - // 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. - var container, vcodec, acodec string - err = h.db.QueryRowContext(r.Context(), - `SELECT container, vcodec, acodec FROM media_files WHERE item_id = ? ORDER BY id LIMIT 1`, - req.ItemID).Scan(&container, &vcodec, &acodec) - if errors.Is(err, sql.ErrNoRows) { + f, err := catalog.FirstFile(r.Context(), h.db, req.ItemID) + if errors.Is(err, catalog.ErrNotFound) { httpx.WriteJSON(w, http.StatusOK, engine.Resolution{ Playable: false, Reason: "nofile", @@ -108,7 +94,7 @@ func (h *Handlers) handleResolve(w http.ResponseWriter, r *http.Request, u auth. return } - res := engine.Resolve(engine.Facts(container, vcodec, acodec), req.Engines) + res := engine.Resolve(engine.Facts(f.Container, f.VCodec, f.ACodec), req.Engines) httpx.WriteJSON(w, http.StatusOK, res) } diff --git a/internal/subsonic/store.go b/internal/subsonic/store.go index 8168d6c..19c0adf 100644 --- a/internal/subsonic/store.go +++ b/internal/subsonic/store.go @@ -3,6 +3,7 @@ package subsonic import ( "context" "database/sql" + "errors" "net/http" "path/filepath" "sort" @@ -11,6 +12,7 @@ import ( "unicode" "github.com/mithral/vellum/internal/auth" + "github.com/mithral/vellum/internal/catalog" ) // libraryRef is an audio library as getMusicFolders shows it. @@ -118,12 +120,22 @@ func (h *Handlers) albumsByArtist(ctx context.Context, libs []int64, personID in []any{personID}, "i.sort_title", 500, 0) } +// loadAlbum authorizes and loads one album for a request. +// +// catalog.LoadFor is the one access rule: it reads the item and confirms the +// caller may read its library, returning the same ErrNotFound for a missing +// item and a forbidden one. The album projection below is Subsonic's own +// presentation concern — song count, duration, artist credit and cover are not +// part of catalog's item model — and no longer decides access. func (h *Handlers) loadAlbum(ctx context.Context, u auth.User, itemID int64) (albumInfo, bool, error) { - libs, err := h.audioLibraryIDs(ctx, u) + it, err := catalog.LoadFor(ctx, h.db, u, itemID) if err != nil { + if errors.Is(err, catalog.ErrNotFound) { + return albumInfo{}, false, nil + } return albumInfo{}, false, err } - albums, err := h.albumsWhere(ctx, libs, ` AND i.id = ?`, []any{itemID}, "i.id", 1, 0) + albums, err := h.albumsWhere(ctx, []int64{it.LibraryID}, ` AND i.id = ?`, []any{itemID}, "i.id", 1, 0) if err != nil { return albumInfo{}, false, err } @@ -179,6 +191,12 @@ func (h *Handlers) tracksForItem(ctx context.Context, itemID int64) ([]track, er return tracks, nil } +// loadTrack authorizes and loads one song (a media_files row) for a request. +// +// The song projection above is Subsonic's own presentation shape — album +// context, artist credit, cover — so it stays here. Access is not its job: +// catalog.LoadFor is the one access rule, returning the same ErrNotFound for a +// missing item and a forbidden one so a 404 cannot leak existence. func (h *Handlers) loadTrack(ctx context.Context, u auth.User, fileIDVal int64) (track, bool, error) { rows, err := h.db.QueryContext(ctx, trackSelect+` WHERE f.id = ? AND f.missing = 0 AND i.missing = 0`, fileIDVal) @@ -193,13 +211,12 @@ func (h *Handlers) loadTrack(ctx context.Context, u auth.User, fileIDVal int64) if len(tracks) == 0 { return track{}, false, nil } - allowed, err := auth.CanAccessLibrary(ctx, h.db, u, tracks[0].LibraryID) - if err != nil { + if _, err := catalog.LoadFor(ctx, h.db, u, tracks[0].ItemID); err != nil { + if errors.Is(err, catalog.ErrNotFound) { + return track{}, false, nil + } return track{}, false, err } - if !allowed { - return track{}, false, nil - } return tracks[0], true, nil }