051169a18a
Docs - docs/ARCHITECTURE.md: how the pieces fit and why. The request path, the six enforced layers, the data model, the scanner, the job queue, direct-play playback, and the three protocol surfaces over one application. - docs/decisions/: seven ADRs with the alternatives that were rejected — SQLite-with-a-seam, direct-play-only, mark-missing-never-delete, the modular monolith, the one item-access chokepoint, the table-backed queue, and fixed-width timestamps. - README: the layout section was stale (it predated catalog, home, scanner, providers, engine, probe, playback, apitoken, arch and the dto packages) and now lists what exists, plus links to the new docs. Two real bugs, both found by verifying the docs instead of trusting them 1. The CLI could not run on a fresh install. `Config.Validate` created the data and config directories, but the CLI never called it, so `vellum user add` — the recovery path when you are locked out — failed with "unable to open database file" until the server had booted at least once. Split the directory creation into `Config.EnsureDirs`, call it from the CLI, and keep the media root check out of it: an admin command must not fail because a library is unmounted. The CLI tests now point DBPath inside DataDir, exactly as LoadConfig derives it, so they exercise the fresh-install path; removing EnsureDirs fails three of them. 2. A fresh clone could not build at all. `//go:embed all:dist` fails to compile when web/dist is absent, and web/.gitignore expected a `dist/.gitkeep` that was never created. Added it, so a clone builds and the server logs "web UI not embedded; run the frontend build to include it" — the intended behaviour for someone who only wants the API. Also: the httpapi tests depended on a built frontend and FAILED rather than skipping, so the suite was red on any fresh clone. findAsset and readAsset now skip with a message naming the missing step. They still RUN when a bundle is present (verified: zero skips here), so they have not been quietly disabled. Verified from a simulated clone of tracked files only: go build exit 0, 25 packages pass, the server boots and applies all 12 migrations, and the CLI creates a user and takes a backup. gofmt clean, vet exit 0, svelte-check 0/0.
77 lines
2.9 KiB
Markdown
77 lines
2.9 KiB
Markdown
# ADR-005: One item-access chokepoint, `internal/catalog`
|
|
|
|
## Status
|
|
Accepted
|
|
|
|
## Date
|
|
2026-10-12
|
|
|
|
## Context
|
|
|
|
Five different packages needed the same thing: load a media item, then check the
|
|
caller is allowed to read it.
|
|
|
|
- `internal/media`
|
|
- `internal/modules/comics`
|
|
- `internal/modules/games`
|
|
- `internal/jellyfin` — with its own `dbItem` type and its own SQL
|
|
- `internal/subsonic` — with its own per-item queries
|
|
|
|
Each had its own implementation, and each decided for itself what to do when
|
|
access was denied.
|
|
|
|
## Decision
|
|
|
|
One package owns item access **and** the access check: `internal/catalog`.
|
|
|
|
## Alternatives Considered
|
|
|
|
### Leave the duplication
|
|
|
|
- **Pros:** no refactor; each package stays self-contained.
|
|
- **Cons:** five copies of one authorization rule means four can drift from the
|
|
fifth. The failure mode is not a crash — it is silently serving somebody a
|
|
library they were never granted. That is a data-leak bug, and it would be found
|
|
by accident, if at all.
|
|
- **Rejected:** this is the highest-value thing in the codebase to have exactly
|
|
one of.
|
|
|
|
### A shared helper that packages call with their own SQL
|
|
|
|
- **Pros:** smaller change.
|
|
- **Cons:** leaves five copies of the item-loading SQL, so a schema change still
|
|
has to be made in five places.
|
|
- **Rejected:** it halves the problem instead of removing it.
|
|
|
|
### A generic repository layer for everything
|
|
|
|
- **Pros:** one place for all SQL.
|
|
- **Cons:** pulls browse lists, home shelves and Subsonic album aggregates into a
|
|
generic layer where they do not belong. Those are each package's own
|
|
presentation concern, and moving them is a 186-statement rewrite.
|
|
- **Rejected:** the chokepoint is the security-critical, most-duplicated path —
|
|
not every query.
|
|
|
|
## The security property
|
|
|
|
`catalog.ItemFor` returns the **same** `ErrNotFound` for an item that does not
|
|
exist and an item the caller may not read.
|
|
|
|
This is deliberate. If the two were distinguishable, a 403 would confirm that
|
|
another user's library contains that item, leaking its shape. The test asserts
|
|
the two responses are **byte-identical**, not merely both non-200.
|
|
|
|
## Consequences
|
|
|
|
- Five duplicate implementations became one. `internal/jellyfin`'s `dbItem` type
|
|
and its own item SQL are gone.
|
|
- All four HTTP surfaces — web, modules, Jellyfin, Subsonic — go through the same
|
|
check, so an API token can never see more than the web app can.
|
|
- An unreadable item is **404, never 403**, everywhere.
|
|
- Aggregate and list queries deliberately stayed where they are. `catalog` is
|
|
item-centric; Subsonic is file-first (`mf-<id>`) and resolves file→item through
|
|
its own projection. A catalog lookup that takes a file id would close that gap.
|
|
- The seam is a `Store` interface with a SQLite implementation, so the storage
|
|
engine is swappable. One consumer (`internal/playback`) is threaded onto it;
|
|
the rest still use the package-level wrappers, and the remaining threading is
|
|
mechanical.
|