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-<id>),
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.
221 lines
6.1 KiB
Go
221 lines
6.1 KiB
Go
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)
|
|
}
|
|
}
|