mirror of
https://github.com/navidrome/navidrome.git
synced 2026-10-09 02:47:29 +02:00
4 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Deluan Quintão
|
b293b96256 |
refactor(persistence): stateless repositories with per-call context (#6149)
* refactor(persistence): adopt generic deluan/rest repository API Pin deluan/rest to the refactor branch. REST-facing repository methods take a context and return typed values. Drop DataStore.Resource and ResourceRepository; the native API names typed repositories directly through a per-request adapter that later commits remove. * refactor(persistence): base repository helpers take a context * refactor(persistence): LibraryRepository takes a context per call * refactor(persistence): PropertyRepository takes a context per call * refactor(persistence): UserPropsRepository takes a context per call * refactor(persistence): TranscodingRepository takes a context per call * refactor(persistence): ShareRepository takes a context per call * refactor(persistence): PlayerRepository takes a context per call * refactor(persistence): RadioRepository takes a context per call * refactor(persistence): PlayQueueRepository takes a context per call * refactor(persistence): Tag and Genre repositories take a context per call * refactor(persistence): PluginRepository takes a context per call * refactor(persistence): Scrobble repositories take a context per call * refactor(persistence): FolderRepository takes a context per call * refactor(persistence): Artwork repositories take a context per call * refactor(persistence): UserRepository takes a context per call * refactor(persistence): ArtistRepository takes a context per call ReadAll no longer rewrites the shared sort mappings for the role filter; it works on a per-call copy. * test(persistence): assert artist role sort sanitization in ReadAll * refactor(persistence): AlbumRepository takes a context per call * test(persistence): pass the test context to album repository helpers * refactor(persistence): MediaFileRepository takes a context per call * refactor(persistence): Playlist repositories take a context per call * refactor(persistence): build all repositories once per store * refactor(core): REST repository wrappers are built once * refactor(persistence): repositories are stateless Remove the context field from the base repository and the per-request REST adapter. Enable the containedctx linter so no repository can hold a request context again. * chore(lint): skip containedctx in test files * refactor: share simplifications from the stateless repositories sweep Add deleteOwnedAll on sqlRepository and use it in player/share Delete to remove the duplicated bulk-delete loop; have Share.Repository() return model.ShareRepository so subsonic sharing.go drops its repeated type assertions. * chore(core): assert REST wrappers implement Persistable * chore: reformat imports * perf(persistence): build repositories on first use Each transaction store used to construct all 21 repositories up front, paying for filter and sort mapping setup the block never touched. Fields are now sync.OnceValue thunks, so a store only builds what it uses. * fix(persistence): clean plugin references per deleted user A bulk user delete that fails on a later id had already removed the earlier rows but skipped their plugin cleanup. Cleanup now runs right after each successful delete. * fix(core): unload disabled plugins even when a user delete fails A bulk delete can fail on a later id after earlier users were removed and their plugins auto-disabled. The wrapper returned before unloading, leaving those plugins running until the next successful delete or a restart. * chore(deps): pin deluan/rest to v1.0.1 Replaces the pseudo-version of the refactor branch with the tagged release. REST error messages now name the bare type (Artist, not model.Artist). * test: use the spec context instead of context.Background() Replace the context.Background()/context.TODO() calls this branch added to tests with the spec's ctx, GinkgoT().Context(), or t/b.Context(), so repository calls are bound to the running spec's lifetime. * test: declare the spec context once per Describe Set ctx from GinkgoT().Context() first in each top-level BeforeEach and reuse it, building user contexts on top of it instead of repeating inline calls. |
||
|
Deluan Quintão
|
3f4b6a642c |
fix(server): return 404 instead of 500 for missing native API resources (#6131)
* fix: return 404 instead of 500 for missing native API resources The deluan/rest controller only maps rest.ErrNotFound to 404, comparing with ==. Most repositories return model.ErrNotFound, which had the same message but was a different value, so requesting a missing playlist, album, artist, song, radio, player, transcoding or library returned 500. This also applied to other users' private playlists. Make model.ErrNotFound the same value as rest.ErrNotFound. This fixes every REST route at once, with no per-route wrapping. errors.Is checks against either error keep working, and nothing wraps model.ErrNotFound before it reaches the controller. Fixes #6130 * fix(radio): return not found when deleting a missing radio station radioRepository.Delete used the shared delete helper, which never reports a missing row because SQL DELETE on zero rows is not an error. Deleting an unknown id silently succeeded: DELETE /api/radio/{id} returned 200, and the Subsonic deleteInternetRadioStation endpoint returned ok. Delete now checks the affected row count and returns model.ErrNotFound when nothing was deleted. The native API returns 404, and deleteInternetRadioStation returns error 70 (data not found). This matches Subsonic 6.1.6, gonic (both verified live) and Ampache (verified in source). Airsonic-Advanced does not implement this endpoint. The shared delete helper is unchanged, as several callers rely on deletes of absent rows succeeding. * fix(ui): return 404 for missing files that are not missing or do not exist missingRepository.Read filtered media files by bare "id" and "missing" columns. The media file query joins the library table, so SQLite rejected the query as ambiguous and GET /api/missing/{id} returned 500. Read now loads the file with MediaFileRepository.Get, which qualifies the column, and returns not found when the file does not exist or is not marked missing. * fix: report missing rows on single-item deletes and adopt deluan/rest errors.Is Bump github.com/deluan/rest to the version whose controller matches errors with errors.Is and errors.As. model.ErrNotFound stays the same value as rest.ErrNotFound, so the many hand-written conversions from model.ErrNotFound to rest.ErrNotFound in repositories, core services and test mocks did nothing. Remove them, along with the duplicate rest.ErrNotFound check in the Subsonic error mapper. Mappings from model.ErrNotAuthorized stay, as those are different errors. User and transcoding deletes had the same silent success as radio: the shared delete helper never reports a missing row, so their not-found checks never fired and DELETE /api/user/{id} and /api/transcoding/{id} returned 200 for unknown ids. Add deleteByID, which returns model.ErrNotFound when no row matched, and use it for radio, user and transcoding. Also drop the dead sql.ErrNoRows branch from delete, since a DELETE never returns it. The Subsonic deleteUser endpoint is not implemented (501), so this does not change the Subsonic API. Plugin deletes keep the silent helper: there is no REST route for them, and the plugin manager only deletes rows it just read. * chore: drop ErrNotFound comment and its identity test The alias to rest.ErrNotFound is self-explanatory, and the identity test only restated the declaration. * refactor: alias model.ErrNotAuthorized to rest.ErrPermissionDenied Like ErrNotFound, make model.ErrNotAuthorized the same value as the rest library's error, so REST endpoints map it to 403 directly. This removes the ErrNotAuthorized to rest.ErrPermissionDenied mappings in the library and playlist REST adapters and the duplicate check in the Subsonic error mapper. Handlers that check model.ErrNotAuthorized now also recognize rest.ErrPermissionDenied returned by repositories, so writePlaylistError, the image upload handlers and the public share handler return 403 for it instead of their fallback status. The error message changes from "not authorized" to "permission denied". |
||
|
Deluan Quintão
|
11640f2e4d |
fix: restrict transcoding config reads to admins (#5564)
* fix(security): restrict transcoding config reads to admins
Authenticated non-admin users could read transcoding configs through
the native API (GET /api/transcoding and /api/transcoding/{id}) when
EnableTranscodingConfig was enabled. The responses included the full
command templates, disclosing admin-configured ffmpeg invocations and
local command paths. Write operations were already admin-only.
The /transcoding route was registered in the general authenticated
group, and only the repository's write methods checked IsAdmin. This
applies the boundary at two layers:
- Move the route under adminOnlyMiddleware, alongside the other
admin-only resources (/library, /config, /inspect).
- Add an IsAdmin guard to the repository's rest.Repository read
methods (Read, ReadAll, Count) as defense-in-depth.
The guard is scoped to the REST methods only. The streaming pipeline
resolves profiles via Get/FindByFormat (model.TranscodingRepository),
which stay open so transcoding keeps working for non-admin users.
Adds regression tests covering non-admin read denial and confirming
non-admin streaming lookups (Get/FindByFormat) still succeed.
* fix(security): redact transcoding Command for non-admins instead of blocking reads
Reworks the previous approach after review (Codex P2): moving /transcoding
under adminOnlyMiddleware and denying non-admin reads broke legitimate
non-admin UI flows. The web UI reads the transcoding resource as a regular
user in several places that need only the profile name and target format:
the player edit dropdown (ReferenceInput), the player list (ReferenceField),
and the share/download format pickers (useGetList -> {targetFormat, name}).
The only sensitive field is Command (the admin-owned ffmpeg template). So:
- Revert the route move; /transcoding stays in the authenticated group.
- Read/ReadAll now return the profiles to any authenticated user but blank
the Command field for non-admins (mirrors user_repository's field-level
redaction). Count is no longer denied (the UI needs list pagination).
- Writes remain admin-only (Save/Update/Delete/Put).
- Streaming is unaffected: it resolves profiles via Get/FindByFormat, which
are not redacted, so on-the-fly transcoding keeps working for non-admins.
Tests updated: non-admin reads succeed with Command blank, admin reads keep
Command, non-admin Get/FindByFormat keep Command, writes still denied.
|
||
|
Deluan Quintão
|
e5438552c6 |
fix(transcoding): restrict transcoding operations to admin users (#4096)
Signed-off-by: Deluan <deluan@navidrome.org> |