From 67f04f7c9276f02b258e8592c28a8bb94fe159aa Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Tue, 18 Aug 2026 22:09:18 +0200 Subject: [PATCH 1/2] feat(auth): check token scopes at every route, not only the admin door MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous release made scopes real on the administrative routes and left them decorative everywhere else. `user.scopes` was read in exactly one place, so a `catalog:read` token could still create and delete playlists and shares, write bookmarks, save the queue, scrobble, and set favorites and ratings. The worst case was closed and the principle was not. The check moves to where the caller is resolved. `authenticated` now takes an `Access` — `Read`, `Write` or `Admin` — and every one of the fifty-two call sites states what its route needs. That is the point of a parameter rather than a second helper: a route cannot be written without answering, because the compiler asks. `require_admin` is gone; both halves of administrative authority, the role and the scope, now live in one place and cannot be applied apart. Two scope names are checked. `write` admits any mutation, `admin` admits the instance and implies `write` — a credential trusted to create accounts is not usefully barred from creating a playlist, and the surprise would run the other way. Reading needs nothing, so a token naming neither is read-only, and **a name the server does not know grants nothing**: `catalog:read` reads and does no more without anyone enumerating a vocabulary. An empty list stays unrestricted, which is what sessions, OAuth grants and existing tokens carry, so nothing that works today stops working. Issuing a token stays administrative, now deliberately rather than by inheritance from the CLI: a token carries the authority of the account it belongs to, so who may mint one is a question about the instance. Three gaps the audit listed alongside it. `GET /api/v2/songs` and `/api/v2/songs/random` are the native forms of `getSongsByGenre` and `getRandomSongs`, whose services existed with no adapter; `genre` is required on the first, because answering with an unfiltered catalogue would drop the filter in silence. `GET /api/v2/search` accepts `artist_offset`, `album_offset` and `song_offset` beside the shared `offset`, so a client that has exhausted one kind can page another — which `search3` has always allowed. And `catalog_snapshot`, which no route calls any more, says so: it survives as a test fixture, and its doc comment now warns off the shape that made one album page read a tenant's whole catalogue. Signed-off-by: InstaZDLL --- docs/api-v2-guide.md | 53 ++++- docs/rfcs/RFC-002-waveflow-server-v2.md | 2 + src/http.rs | 292 ++++++++++++++++-------- src/lib.rs | 2 + src/services.rs | 30 ++- tests/v2_foundations.rs | 136 +++++++++++ 6 files changed, 400 insertions(+), 115 deletions(-) diff --git a/docs/api-v2-guide.md b/docs/api-v2-guide.md index 92b006b..594d28a 100644 --- a/docs/api-v2-guide.md +++ b/docs/api-v2-guide.md @@ -138,7 +138,7 @@ Wildcards are rejected. Native interactive clients should use the system browser: -1. Generate a 43–128 character PKCE verifier and keep it private. +1. Generate a 43–128 character PKCE verifier and keep it private. 2. Compute `challenge = base64url_no_padding(sha256(verifier))`. 3. Generate and retain an unpredictable `state` value. 4. Open the embedded UI route below in the system browser: @@ -196,6 +196,17 @@ curl https://music.example.com/api/v2/artists/ARTIST_UUID \ -H "Authorization: Bearer ACCESS_TOKEN" ``` +`GET /api/v2/songs` takes a required `genre` and pages through it; `GET +/api/v2/songs/random` draws a selection in SQL, with optional `genre`, +`from_year` and `to_year`. Both match the genre on its canonical name like +every other genre filter, and both are the native form of a Subsonic method +(`getSongsByGenre`, `getRandomSongs`) resolving through the same service. + +`GET /api/v2/search` applies `offset` to all three kinds, and accepts +`artist_offset`, `album_offset` and `song_offset` to page one of them on its +own — which is what a client that has exhausted the artists but not the songs +needs, and what `search3` has always allowed. + Browse and search pages accept `offset >= 0` and `1 <= limit <= 500`. `GET /api/v2/albums` and `/artists` additionally accept an optional `library_id`. A `SongItem` contains stable `id`, optional `album_id` and @@ -388,10 +399,10 @@ Other user-data routes: |---|---| | Playlists | `GET/POST /api/v2/playlists`, `GET/PATCH/DELETE /api/v2/playlists/{id}` | | Favorites | `GET /api/v2/favorites`, `PUT/DELETE /api/v2/favorites/{track|album|artist}/{id}` | -| Ratings | `GET /api/v2/ratings`, `PUT /api/v2/ratings/{track|album|artist}/{id}` with `rating` 0–5 | +| Ratings | `GET /api/v2/ratings`, `PUT /api/v2/ratings/{track|album|artist}/{id}` with `rating` 0–5 | | Playback activity | `POST /api/v2/scrobbles`, `GET /api/v2/history`, `GET /api/v2/now-playing` | | Queue | `GET/PUT /api/v2/queue` (`track_ids` allows repeated tracks; maximum 400) | -| Shares | `GET/POST /api/v2/shares`, `PATCH/DELETE /api/v2/shares/{id}` (1–400 tracks) | +| Shares | `GET/POST /api/v2/shares`, `PATCH/DELETE /api/v2/shares/{id}` (1–400 tracks) | `POST /api/v2/shares` returns the bearer URL at creation and on an authenticated idempotent replay. Later list, snapshot and update responses omit it because @@ -415,7 +426,7 @@ curl "https://music.example.com/api/v2/sync/changes?after=CURSOR&limit=100" \ ``` Apply every event idempotently, advance to `next_cursor`, and continue while -`has_more` is true. Limits are 1–500. After durable local application, ACK the +`has_more` is true. Limits are 1–500. After durable local application, ACK the device cursor: ```bash @@ -508,13 +519,33 @@ SHA-256 hash is stored, so the secret appears there and never again: the listing returns names, scopes and timestamps, and a caller who loses a token issues another rather than reading it back. -**Scopes are enforced.** A token issued with a non-empty `scopes` list is -restricted to it, whatever the account behind it may do: the administrative -routes require the `admin` scope, so a `catalog:read` token belonging to an -administrator is refused with `403`. A token issued **without** scopes is -unrestricted and carries the account's full authority, which is what the CLI -has always produced and what tokens created before this release hold. Sessions -and Authorization Code grants are likewise unrestricted. `DELETE` revokes one; the token +**Scopes are enforced on every route.** A token issued with a non-empty +`scopes` list is restricted to it, whatever the account behind it may do. Two +scopes are checked: + +| Scope | Admits | +|---|---| +| `write` | any mutation: playlists, favorites, ratings, the queue, bookmarks, shares, scrobbles, scans, issuing an OAuth code | +| `admin` | the administrative routes, and everything `write` admits | + +Reading needs no scope, so a token naming neither is read-only. **A scope this +server does not know grants nothing**, which is why `catalog:read` reads and +does no more: there is no vocabulary to learn, only these two names to use. + +A token issued **without** scopes is unrestricted and carries the account's full +authority. That is what the CLI has always produced, what tokens created before +this release hold, and what sessions and Authorization Code grants carry, so +nothing that works today stops working. + +The check happens where the caller is resolved, not in each handler, so a route +cannot be added without choosing what it needs — the compiler asks. This +matters because the previous release stored scopes, returned them from the API +and printed them from the CLI while reading them nowhere. + +Issuing a token is administrative: an account cannot mint one for itself. A +token carries the authority of the account it belongs to, so who may create one +is a question about the instance rather than about the account, and the answer +is the same from the CLI and from the API. `DELETE` revokes one; the token stops authenticating immediately, and revoking it again answers `404`, because it is already not working. diff --git a/docs/rfcs/RFC-002-waveflow-server-v2.md b/docs/rfcs/RFC-002-waveflow-server-v2.md index 757cb9c..3e68046 100644 --- a/docs/rfcs/RFC-002-waveflow-server-v2.md +++ b/docs/rfcs/RFC-002-waveflow-server-v2.md @@ -70,6 +70,8 @@ Genre matching is one rule across every method. `getGenres` groups by `genre.can `getArtistInfo` and `getArtistInfo2` resolve the requested artist through tenant-scoped catalogue access and return their standard empty containers until artist biography enrichment is implemented. This preserves compatibility with clients such as DSub without fabricating biography or similar-artist metadata. `getAlbumInfo` and `getAlbumInfo2` behave the same way for albums, for the same reason: Feishin and Symfonium call them as soon as an album page opens, and an unimplemented-method error there reads to the client as a broken album rather than as absent enrichment. They carry one real value, the album's release identifier, as their `musicBrainzId` element; notes and biography images stay absent because WaveFlow queries no remote source. `AlbumInfo` predates the presence rule and its members are elements rather than attributes, so an album with no release id omits the element instead of sending it empty. +API tokens are restricted by their scopes on every route, not only on the administrative ones. `Access` names what a route needs — `Read`, `Write` or `Admin` — and is chosen at the single point where the caller is resolved, so a route cannot exist without answering the question. An empty scope list is unrestricted, which is what sessions, OAuth grants and tokens issued without scopes carry; a non-empty list grants only what it names, and a name the server does not know grants nothing, so no vocabulary has to be enumerated. `admin` implies `write`, because a credential trusted to create accounts is not usefully barred from creating a playlist. Issuing a token stays administrative on both surfaces: a token carries the authority of the account it belongs to, so who may mint one is a question about the instance. + `playlist.owner` and `share.username` carry the authenticated username. Both collections are read scoped to their owner, so no other name is reachable; the empty string previously emitted made Feishin treat every playlist as another account's and refuse to edit it. Mutation methods whose Subsonic result is empty (`updatePlaylist`, `deletePlaylist`, stars, ratings, scrobbles, queue save, share deletion and user-management writes) return only the successful protocol envelope. They do not add implementation-specific child elements. diff --git a/src/http.rs b/src/http.rs index 8c727bb..1cab0d1 100644 --- a/src/http.rs +++ b/src/http.rs @@ -111,6 +111,27 @@ pub struct GenreQuery { #[derive(Debug, Deserialize)] pub struct SearchQuery { pub q: String, + /// Applied to every kind unless the per-kind offset below overrides it. + pub offset: Option, + pub limit: Option, + pub artist_offset: Option, + pub album_offset: Option, + pub song_offset: Option, +} + +#[derive(Debug, Deserialize)] +pub struct RandomSongQuery { + pub library_id: Option, + pub genre: Option, + pub from_year: Option, + pub to_year: Option, + pub limit: Option, +} + +#[derive(Debug, Deserialize)] +pub struct GenreSongQuery { + pub genre: String, + pub library_id: Option, pub offset: Option, pub limit: Option, } @@ -373,6 +394,8 @@ pub fn router(state: AppState) -> Router { .route("/api/v2/artists", get(list_artists)) .route("/api/v2/artists/{artist_id}", get(get_artist)) .route("/api/v2/search", get(search_catalog)) + .route("/api/v2/songs", get(list_songs_by_genre)) + .route("/api/v2/songs/random", get(list_random_songs)) .route( "/api/v2/playlists", get(list_playlists).post(create_playlist), @@ -662,7 +685,7 @@ pub async fn start_scan( Path(library_id): Path, headers: HeaderMap, ) -> Result<(StatusCode, Json), ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Write).await?; let scan_id = state .services .start_library_scan(user.id, library_id) @@ -676,7 +699,7 @@ pub async fn list_libraries( State(state): State, headers: HeaderMap, ) -> Result>, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; state .db .libraries_for_user(user.id) @@ -691,8 +714,7 @@ pub async fn create_library( headers: HeaderMap, Json(request): Json, ) -> Result<(StatusCode, Json), ApiError> { - let actor = authenticated(&state, &headers).await?; - require_admin(&actor)?; + let actor = authenticated(&state, &headers, Access::Admin).await?; let path = std::path::PathBuf::from(&request.path); let metadata = tokio::fs::symlink_metadata(&path) .await @@ -746,8 +768,7 @@ pub async fn set_library_member( headers: HeaderMap, Json(request): Json, ) -> Result { - let actor = authenticated(&state, &headers).await?; - require_admin(&actor)?; + let actor = authenticated(&state, &headers, Access::Admin).await?; if request.role == crate::database::LibraryRole::Owner || state .db @@ -785,8 +806,7 @@ pub async fn remove_library_member( Path((library_id, user_id)): Path<(Uuid, Uuid)>, headers: HeaderMap, ) -> Result { - let actor = authenticated(&state, &headers).await?; - require_admin(&actor)?; + let actor = authenticated(&state, &headers, Access::Admin).await?; if state .db .remove_library_member( @@ -810,7 +830,7 @@ pub async fn scan_status( Path(scan_id): Path, headers: HeaderMap, ) -> Result, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; state .db .scan_job_for_user(user.id, scan_id) @@ -826,7 +846,7 @@ pub async fn scan_events( Path(scan_id): Path, headers: HeaderMap, ) -> Result>>, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; let initial = state .db .scan_job_for_user(user.id, scan_id) @@ -856,7 +876,7 @@ pub async fn list_tracks( Query(query): Query, headers: HeaderMap, ) -> Result>, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; if state .db .library_for_user(user.id, library_id) @@ -886,7 +906,7 @@ pub async fn get_track( Path(track_id): Path, headers: HeaderMap, ) -> Result, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; state .services .songs_by_ids(user.id, &[track_id]) @@ -904,7 +924,7 @@ pub async fn get_track_lyrics( Path(track_id): Path, headers: HeaderMap, ) -> Result, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; state .services .lyrics(user.id, track_id) @@ -919,7 +939,7 @@ pub async fn list_albums( Query(query): Query, headers: HeaderMap, ) -> Result>, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; let order = query .sort .as_deref() @@ -949,7 +969,7 @@ pub async fn list_genres( Query(query): Query, headers: HeaderMap, ) -> Result>, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; let libraries = query.library_id.into_iter().collect::>(); state .services @@ -965,7 +985,7 @@ pub async fn get_album( Path(album_id): Path, headers: HeaderMap, ) -> Result, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; state .services .album(user.id, album_id) @@ -980,7 +1000,7 @@ pub async fn list_artists( Query(query): Query, headers: HeaderMap, ) -> Result>, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; let page = crate::services::BrowsePage::new(query.offset, query.limit).map_err(service_error)?; state @@ -997,7 +1017,7 @@ pub async fn get_artist( Path(artist_id): Path, headers: HeaderMap, ) -> Result, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; state .services .artist(user.id, artist_id) @@ -1012,12 +1032,70 @@ pub async fn search_catalog( Query(query): Query, headers: HeaderMap, ) -> Result, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; + // One offset for all three kinds unless the caller names one, which is + // what `search3` has always allowed and what a client paging songs past + // the end of the artists needs. + let page = |offset: Option| { + crate::services::BrowsePage::new(offset.or(query.offset), query.limit) + .map_err(service_error) + }; + state + .services + .search( + user.id, + &query.q, + page(query.artist_offset)?, + page(query.album_offset)?, + page(query.song_offset)?, + ) + .await + .map(Json) + .map_err(service_error) +} + +/// The native form of `getRandomSongs`. +/// +/// The selection is drawn in SQL, so a request for ten reads ten. `genre` +/// matches the canonical name, like every other genre filter on either +/// surface, and a reversed year range is read as a range rather than as an +/// empty one. +#[utoipa::path(get, path = "/api/v2/songs/random", tag = "catalog", params(("library_id" = Option, Query), ("genre" = Option, Query), ("from_year" = Option, Query), ("to_year" = Option, Query), ("limit" = Option, Query)), responses((status = 200, body = [crate::services::SongItem]), (status = 401, body = ErrorResponse), (status = 422, body = ErrorResponse)))] +pub async fn list_random_songs( + State(state): State, + Query(query): Query, + headers: HeaderMap, +) -> Result>, ApiError> { + let user = authenticated(&state, &headers, Access::Read).await?; + state + .services + .random_songs( + user.id, + query.library_id.as_slice(), + query.genre.as_deref(), + query.from_year, + query.to_year, + query.limit.unwrap_or(10), + ) + .await + .map(Json) + .map_err(service_error) +} + +/// The native form of `getSongsByGenre`. `genre` is required: answering an +/// unfiltered catalogue would drop the filter in silence. +#[utoipa::path(get, path = "/api/v2/songs", tag = "catalog", params(("genre" = String, Query), ("library_id" = Option, Query), ("offset" = Option, Query), ("limit" = Option, Query)), responses((status = 200, body = [crate::services::SongItem]), (status = 401, body = ErrorResponse), (status = 422, body = ErrorResponse)))] +pub async fn list_songs_by_genre( + State(state): State, + Query(query): Query, + headers: HeaderMap, +) -> Result>, ApiError> { + let user = authenticated(&state, &headers, Access::Read).await?; let page = crate::services::BrowsePage::new(query.offset, query.limit).map_err(service_error)?; state .services - .search(user.id, &query.q, page) + .songs_by_genre(user.id, query.library_id.as_slice(), &query.genre, page) .await .map(Json) .map_err(service_error) @@ -1031,7 +1109,7 @@ pub async fn oauth_authorize( ) -> Result, ApiError> { // The browser session is the proof of identity; the consent screen is a // route of the embedded client, so this is a JSON call rather than a form. - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Write).await?; let redirect_to = state .services .authorize_native_client( @@ -1087,7 +1165,7 @@ pub async fn list_playlists( State(state): State, headers: HeaderMap, ) -> Result>, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; state .services .playlists(user.id) @@ -1102,7 +1180,7 @@ pub async fn create_playlist( headers: HeaderMap, Json(request): Json, ) -> Result<(StatusCode, Json), ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Write).await?; let context = mutation_context(&state, &headers, user.id).await?; let playlist = state .services @@ -1118,7 +1196,7 @@ pub async fn get_playlist( Path(playlist_id): Path, headers: HeaderMap, ) -> Result, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; state .services .playlist(user.id, playlist_id) @@ -1161,7 +1239,7 @@ pub async fn update_playlist( headers: HeaderMap, Json(request): Json, ) -> Result, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Write).await?; let context = mutation_context(&state, &headers, user.id).await?; state .services @@ -1187,7 +1265,7 @@ pub async fn delete_playlist( Path(playlist_id): Path, headers: HeaderMap, ) -> Result { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Write).await?; let context = mutation_context(&state, &headers, user.id).await?; state .services @@ -1202,7 +1280,7 @@ pub async fn list_favorites( State(state): State, headers: HeaderMap, ) -> Result>, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; let entries = state .services .starred_ids(user.id) @@ -1243,7 +1321,7 @@ async fn set_favorite( entity_id: Uuid, starred: bool, ) -> Result { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Write).await?; let context = mutation_context(&state, &headers, user.id).await?; state .services @@ -1260,7 +1338,7 @@ pub async fn set_rating( headers: HeaderMap, Json(request): Json, ) -> Result { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Write).await?; let context = mutation_context(&state, &headers, user.id).await?; state .services @@ -1286,7 +1364,7 @@ pub async fn list_bookmarks( State(state): State, headers: HeaderMap, ) -> Result>, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; state .services .bookmarks(user.id) @@ -1308,7 +1386,7 @@ pub async fn set_bookmark( headers: HeaderMap, Json(request): Json, ) -> Result { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Write).await?; let context = mutation_context(&state, &headers, user.id).await?; state .services @@ -1333,7 +1411,7 @@ pub async fn delete_bookmark( Path(track_id): Path, headers: HeaderMap, ) -> Result { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Write).await?; let context = mutation_context(&state, &headers, user.id).await?; state .services @@ -1368,8 +1446,7 @@ pub async fn list_api_tokens( Path(username): Path, headers: HeaderMap, ) -> Result>, ApiError> { - let actor = authenticated(&state, &headers).await?; - require_admin(&actor)?; + let actor = authenticated(&state, &headers, Access::Admin).await?; state .services .api_tokens(actor.id, &username) @@ -1391,8 +1468,7 @@ pub async fn create_api_token( headers: HeaderMap, Json(request): Json, ) -> Result<(StatusCode, Json), ApiError> { - let actor = authenticated(&state, &headers).await?; - require_admin(&actor)?; + let actor = authenticated(&state, &headers, Access::Admin).await?; let (token, secret) = state .services .create_api_token(actor.id, &username, &request.name, &request.scopes) @@ -1410,8 +1486,7 @@ pub async fn revoke_api_token( Path((username, token_id)): Path<(String, Uuid)>, headers: HeaderMap, ) -> Result { - let actor = authenticated(&state, &headers).await?; - require_admin(&actor)?; + let actor = authenticated(&state, &headers, Access::Admin).await?; state .services .revoke_api_token(actor.id, &username, token_id) @@ -1425,7 +1500,7 @@ pub async fn list_ratings( State(state): State, headers: HeaderMap, ) -> Result>, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; state .services .ratings(user.id) @@ -1440,7 +1515,7 @@ pub async fn create_scrobble( headers: HeaderMap, Json(request): Json, ) -> Result { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Write).await?; let context = mutation_context(&state, &headers, user.id).await?; state .services @@ -1462,7 +1537,7 @@ pub async fn list_history( headers: HeaderMap, Query(query): Query, ) -> Result>, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; let limit = query.limit.unwrap_or(200); if !(1..=crate::sync::MAX_SYNC_LIMIT).contains(&limit) { return Err(ApiError::Validation); @@ -1480,7 +1555,7 @@ pub async fn transcode_status( State(state): State, headers: HeaderMap, ) -> Result, ApiError> { - authenticated(&state, &headers).await?; + authenticated(&state, &headers, Access::Read).await?; Ok(Json(TranscodeStatusResponse { available: state.media.transcoding_available(), active: state.media.active_transcodes(), @@ -1492,8 +1567,7 @@ pub async fn list_users( State(state): State, headers: HeaderMap, ) -> Result>, ApiError> { - let actor = authenticated(&state, &headers).await?; - require_admin(&actor)?; + let actor = authenticated(&state, &headers, Access::Admin).await?; state .services .users(actor.id) @@ -1508,8 +1582,7 @@ pub async fn create_user( headers: HeaderMap, Json(request): Json, ) -> Result<(StatusCode, Json), ApiError> { - let actor = authenticated(&state, &headers).await?; - require_admin(&actor)?; + let actor = authenticated(&state, &headers, Access::Admin).await?; let user = state .services .create_web_user( @@ -1530,8 +1603,7 @@ pub async fn update_user( headers: HeaderMap, Json(request): Json, ) -> Result, ApiError> { - let actor = authenticated(&state, &headers).await?; - require_admin(&actor)?; + let actor = authenticated(&state, &headers, Access::Admin).await?; state .services .update_user( @@ -1558,8 +1630,7 @@ pub async fn delete_user( Path(username): Path, headers: HeaderMap, ) -> Result { - let actor = authenticated(&state, &headers).await?; - require_admin(&actor)?; + let actor = authenticated(&state, &headers, Access::Admin).await?; state .services .delete_user(actor.id, &username) @@ -1575,8 +1646,7 @@ pub async fn set_subsonic_credential( headers: HeaderMap, Json(request): Json, ) -> Result, ApiError> { - let actor = authenticated(&state, &headers).await?; - require_admin(&actor)?; + let actor = authenticated(&state, &headers, Access::Admin).await?; let api_key = state .services .set_subsonic_credential(actor.id, &username, &request.password) @@ -1591,8 +1661,7 @@ pub async fn revoke_subsonic_credential( Path(username): Path, headers: HeaderMap, ) -> Result { - let actor = authenticated(&state, &headers).await?; - require_admin(&actor)?; + let actor = authenticated(&state, &headers, Access::Admin).await?; state .services .revoke_subsonic_credential(actor.id, &username) @@ -1606,7 +1675,7 @@ pub async fn list_now_playing( State(state): State, headers: HeaderMap, ) -> Result>, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; let entries = state .services .now_playing(user.id) @@ -1627,7 +1696,7 @@ pub async fn get_queue( State(state): State, headers: HeaderMap, ) -> Result>, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; state .services .queue(user.id) @@ -1642,7 +1711,7 @@ pub async fn save_queue( headers: HeaderMap, Json(request): Json, ) -> Result { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Write).await?; let context = mutation_context(&state, &headers, user.id).await?; state .services @@ -1664,7 +1733,7 @@ pub async fn list_shares( State(state): State, headers: HeaderMap, ) -> Result>, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; let shares = state .services .shares(user.id) @@ -1682,7 +1751,7 @@ pub async fn create_share( headers: HeaderMap, Json(request): Json, ) -> Result<(StatusCode, Json), ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Write).await?; let context = mutation_context(&state, &headers, user.id).await?; let share = state .services @@ -1705,7 +1774,7 @@ pub async fn update_share( headers: HeaderMap, Json(request): Json, ) -> Result, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Write).await?; let context = mutation_context(&state, &headers, user.id).await?; let share = state .services @@ -1728,7 +1797,7 @@ pub async fn delete_share( Path(share_id): Path, headers: HeaderMap, ) -> Result { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Write).await?; let context = mutation_context(&state, &headers, user.id).await?; state .services @@ -1781,7 +1850,7 @@ pub async fn sync_changes( headers: HeaderMap, Query(query): Query, ) -> Result, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; let after = query.after.unwrap_or(0); let limit = query.limit.unwrap_or(crate::sync::DEFAULT_SYNC_LIMIT); if after < 0 || limit <= 0 || limit > crate::sync::MAX_SYNC_LIMIT { @@ -1805,7 +1874,7 @@ pub async fn sync_snapshot( State(state): State, headers: HeaderMap, ) -> Result, ApiError> { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; let snapshot = state .services .sync_snapshot(user.id, crate::sync::MAX_SYNC_LIMIT) @@ -1853,7 +1922,7 @@ pub async fn sync_ack( headers: HeaderMap, Json(request): Json, ) -> Result { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Write).await?; let acknowledged = state .sync .acknowledge(user.id, request.device_id, request.cursor) @@ -1885,7 +1954,7 @@ pub async fn sync_socket( Query(query): Query, upgrade: WebSocketUpgrade, ) -> Result { - let user = authenticated(&state, &headers).await?; + let user = authenticated(&state, &headers, Access::Read).await?; let after = query.after.unwrap_or(0); if after < 0 { return Err(ApiError::Validation); @@ -2157,42 +2226,75 @@ fn bearer_token(headers: &HeaderMap) -> Option<&str> { .filter(|token| !token.is_empty()) } -async fn authenticated( - state: &AppState, - headers: &HeaderMap, -) -> Result { - let token = bearer_token(headers).ok_or(ApiError::Unauthorized)?; - state.auth.authenticate(token).await.map_err(ApiError::from) +/// What a route needs of the credential it was called with. +/// +/// Chosen at every call of [`authenticated`], which is the only way into a +/// route, so a new route cannot be written without deciding: the compiler asks +/// the question. That is the whole reason this is a parameter rather than a +/// second helper a handler may forget to call — which is exactly what happened +/// to the scope list, stored since the foundations and read by nothing. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Access { + /// Reads the caller's own catalogue and user data. + Read, + /// Writes on the caller's behalf: playlists, favorites, ratings, the queue, + /// bookmarks, shares, scrobbles, scans, and issuing an OAuth code. + Write, + /// Acts on the instance: accounts, libraries, memberships, credentials. + Admin, } /// The scope that admits the administrative routes. -/// -/// A token issued with an explicit scope list has to carry it. Without that -/// the list was decoration: the scopes were stored, returned by the API and -/// printed by the CLI, while a `catalog:read` token on an administrator's -/// account could still create users. That is worse than having no scopes at -/// all, because the operator believes the token is limited. const ADMIN_SCOPE: &str = "admin"; +/// The scope that admits any mutation. +const WRITE_SCOPE: &str = "write"; -/// Administrative authority: an active administrator, on a credential that -/// has not been narrowed away from it. -/// -/// An empty scope list is unrestricted, which is what a session, an OAuth -/// grant and a token issued without scopes all carry. Both conditions have -/// to hold: being an administrator does not widen a token, and a token -/// cannot promote an ordinary account. +impl Access { + /// Whether a credential carrying `scopes` may do this. + /// + /// An empty list is unrestricted: a session, an OAuth grant and a token + /// issued without scopes all carry the account's full authority, so nothing + /// that works today stops working. + /// + /// A non-empty list grants only what it names, and a name this server does + /// not know grants nothing — so `catalog:read` reads and does no more, + /// without needing a vocabulary of every possible scope. `admin` implies + /// `write`: a credential trusted to create accounts is not usefully barred + /// from creating a playlist, and the surprise would be the other way round. + fn granted_by(self, scopes: &[String]) -> bool { + if scopes.is_empty() { + return true; + } + let holds = |wanted: &str| scopes.iter().any(|scope| scope == wanted); + match self { + Self::Read => true, + Self::Write => holds(WRITE_SCOPE) || holds(ADMIN_SCOPE), + Self::Admin => holds(ADMIN_SCOPE), + } + } +} + +/// Resolves the caller and checks, in one place, that the credential may do +/// what the route is about to do. /// -/// A scope list grants the union of its entries, as scope lists do -/// everywhere: `admin` beside `catalog:read` admits these routes, because a -/// token that explicitly names a permission must not be refused it. The -/// comparison is exact rather than trimmed, which it can be because -/// `DomainServices::create_api_token` normalises what it stores: the value -/// a listing shows is the value this compares. -fn require_admin(user: &crate::authentication::AuthUser) -> Result<(), ApiError> { - let is_admin = user.role == crate::database::AccountRole::Admin; - let in_scope = user.scopes.is_empty() || user.scopes.iter().any(|scope| scope == ADMIN_SCOPE); - if is_admin && in_scope { - Ok(()) +/// Both halves of administrative authority live here: an active administrator, +/// on a credential that has not been narrowed away from it. Being an +/// administrator does not widen a token, and a token cannot promote an +/// ordinary account. +async fn authenticated( + state: &AppState, + headers: &HeaderMap, + access: Access, +) -> Result { + let token = bearer_token(headers).ok_or(ApiError::Unauthorized)?; + let user = state + .auth + .authenticate(token) + .await + .map_err(ApiError::from)?; + let role_ok = access != Access::Admin || user.role == crate::database::AccountRole::Admin; + if role_ok && access.granted_by(&user.scopes) { + Ok(user) } else { Err(ApiError::Forbidden) } diff --git a/src/lib.rs b/src/lib.rs index 24554f8..69e6995 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -93,6 +93,8 @@ pub struct AppState { http::list_artists, http::get_artist, http::search_catalog, + http::list_random_songs, + http::list_songs_by_genre, http::list_playlists, http::create_playlist, http::get_playlist, diff --git a/src/services.rs b/src/services.rs index c41e3d4..cc8a38d 100644 --- a/src/services.rs +++ b/src/services.rs @@ -824,8 +824,14 @@ impl DomainServices { /// The overview plus every visible track. /// - /// Only the methods that genuinely need the whole track list should reach - /// for this. Everything with an id in hand has a targeted query. + /// **No route calls this.** Every browse method that used to now asks for + /// what it renders, and nothing should reach for this again: it is the + /// shape that made one album page read a tenant's whole catalogue. + /// + /// It survives as a fixture. The integration suite builds ids from it — + /// "give me an album of this account so I can ask for it" — which is a + /// legitimate use of a full read in a test with three tracks in it, and + /// not one in a request. pub async fn catalog_snapshot( &self, user_id: Uuid, @@ -1427,11 +1433,17 @@ impl DomainServices { /// through the FTS5 index built in M1, which folds case and diacritics, so /// "echo" finds "Écho". Albums and artists are derived from the same index /// rather than a second scan, keeping one source of truth for relevance. + /// + /// Each kind is paged independently, as `search3` has always allowed: + /// a client that has read every matching song should be able to ask for + /// the next page of songs without re-reading the artists beside them. pub async fn search( &self, user_id: Uuid, query: &str, - page: BrowsePage, + artists: BrowsePage, + albums: BrowsePage, + songs: BrowsePage, ) -> Result { // Prefix on the trailing term, like the Subsonic surface: a client // querying on each keystroke would otherwise get nothing until the word @@ -1451,8 +1463,8 @@ impl DomainServices { )) .bind(user_id.to_string()) .bind(&fts) - .bind(page.limit) - .bind(page.offset) + .bind(songs.limit) + .bind(songs.offset) .fetch_all(self.db.pool()) .await? .into_iter() @@ -1468,8 +1480,8 @@ impl DomainServices { )) .bind(user_id.to_string()) .bind(&fts) - .bind(page.limit) - .bind(page.offset) + .bind(albums.limit) + .bind(albums.offset) .fetch_all(self.db.pool()) .await? .into_iter() @@ -1484,8 +1496,8 @@ impl DomainServices { )) .bind(user_id.to_string()) .bind(&fts) - .bind(page.limit) - .bind(page.offset) + .bind(artists.limit) + .bind(artists.offset) .fetch_all(self.db.pool()) .await? .into_iter() diff --git a/tests/v2_foundations.rs b/tests/v2_foundations.rs index f6183bc..16bfaef 100644 --- a/tests/v2_foundations.rs +++ b/tests/v2_foundations.rs @@ -5176,6 +5176,68 @@ async fn genre_matching_is_canonical_on_every_surface() { 1 ); + // The native surface answers the same two questions the facade does, on + // the same services: an asymmetry the audit left open, where the query + // existed and only the HTTP adapter was missing. + let login = router + .clone() + .oneshot(json_request( + "/api/v2/auth/login", + serde_json::json!({ + "username": "genre-owner", + "password": "correct horse battery staple", + "device_name": "Integration" + }), + )) + .await + .unwrap(); + let access = json_body(login).await["access_token"] + .as_str() + .unwrap() + .to_owned(); + let native = |path: String| { + let router = router.clone(); + let access = access.clone(); + async move { + router + .oneshot( + Request::get(path) + .header("authorization", format!("Bearer {access}")) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap() + } + }; + // Any spelling reaches the same three tracks, natively too. + let by_genre = json_body(native("/api/v2/songs?genre=hip%20hop&limit=50".into()).await).await; + assert_eq!(by_genre.as_array().expect("a list").len(), 3); + let random = + json_body(native("/api/v2/songs/random?genre=HIP%20%20HOP&limit=50".into()).await).await; + assert_eq!(random.as_array().expect("a list").len(), 3); + // The year filter narrows, and a genre nobody uses is empty rather than + // an error. + let narrowed = json_body( + native("/api/v2/songs/random?genre=Hip-Hop&limit=50&from_year=2010&to_year=2020".into()) + .await, + ) + .await; + assert!(narrowed.as_array().expect("a list").is_empty()); + let unused = json_body(native("/api/v2/songs?genre=Polka".into()).await).await; + assert!(unused.as_array().expect("a list").is_empty()); + // The genre is what the request is about, so its absence is a malformed + // request and not an unfiltered catalogue. + assert_eq!( + native("/api/v2/songs".into()).await.status(), + StatusCode::BAD_REQUEST + ); + // Search pages each kind on its own offset. + let paged = + json_body(native("/api/v2/search?q=Boom&limit=10&song_offset=5".into()).await).await; + assert!(paged["songs"].as_array().expect("songs").is_empty()); + assert_eq!(paged["albums"].as_array().expect("albums").len(), 1); + // The album carries the credits and genres of its tracks, folded the same // way, which is what AlbumID3 asks for. let albums = state.services.catalog_snapshot(owner, &[]).await.unwrap(); @@ -5640,6 +5702,80 @@ async fn native_bookmarks_and_api_tokens_round_trip() { StatusCode::OK ); + // Reading is all a `catalog:read` token may do. Before the scope check + // reached the mutations it could still write playlists, shares, ratings, + // the queue and these very bookmarks: only the administrative door was + // closed, which shut the worst case and left the principle open. + let write_attempt = router + .clone() + .oneshot( + Request::builder() + .method(Method::PUT) + .uri(format!("/api/v2/bookmarks/{track}")) + .header("authorization", format!("Bearer {secret}")) + .header("content-type", "application/json") + .body(Body::from( + serde_json::json!({"position_ms": 1}).to_string(), + )) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(write_attempt.status(), StatusCode::FORBIDDEN); + // And it still reads, so the refusal is the mutation and not the token. + let read_attempt = router + .clone() + .oneshot( + Request::get("/api/v2/bookmarks") + .header("authorization", format!("Bearer {secret}")) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(read_attempt.status(), StatusCode::OK); + + // `write` admits the mutation and stops at the instance: the two levels + // are separate, and `admin` implies `write` rather than the reverse. + let writer = json_body( + json_request( + Method::POST, + "/api/v2/admin/users/token-admin/tokens".into(), + serde_json::json!({"name": "sync agent", "scopes": ["write"]}), + ) + .await, + ) + .await; + let writer = writer["secret"].as_str().unwrap().to_owned(); + let allowed = router + .clone() + .oneshot( + Request::builder() + .method(Method::PUT) + .uri(format!("/api/v2/bookmarks/{track}")) + .header("authorization", format!("Bearer {writer}")) + .header("content-type", "application/json") + .body(Body::from( + serde_json::json!({"position_ms": 1}).to_string(), + )) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(allowed.status(), StatusCode::NO_CONTENT); + let refused = router + .clone() + .oneshot( + Request::get("/api/v2/admin/users") + .header("authorization", format!("Bearer {writer}")) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(refused.status(), StatusCode::FORBIDDEN); + state.services.delete_bookmark(admin, track).await.unwrap(); + // A scope list grants the union of its entries: naming `admin` beside // another scope admits these routes, because a token that explicitly // carries a permission must not be refused it. The stored form is From 002adf4f80ed31957b645bfd7b1e7061a03fcd83 Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Tue, 18 Aug 2026 22:21:17 +0200 Subject: [PATCH 2/2] fix: repair four en-dashes, and route the media handlers through the door MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review points, one of them a defect I had reported clean. **The encoding sweep was wrong, not the file.** Four en-dashes in docs/api-v2-guide.md were double-encoded by the same scripted edit that damaged the em-dashes, and my check did not see them: it looked for `Ã` and `Â`, which is the signature of a doubled two-byte character, while a dash is three bytes and doubles to `â` followed by two `Â`. I had reported the tree clean on a check that could not have found this. The repair is generic — any `C3 A2 C2 xx C2 yy` back to `E2 xx yy` — and the sweep now covers all three signatures across src, tests, docs and migrations. `43â128` reads `43–128` again. **Three media handlers authenticated outside the door.** `artwork`, `create_stream_ticket` and `stream_track` each extracted the header and called the authenticator themselves, so they sat outside the one place scopes are checked. They are reads, so nothing they grant changes; what changes is that a mutation added there cannot quietly skip the check, which is precisely how the scope list came to be stored and never read. `Access` and `authenticated` become `pub(crate)`, the triplicated header parsing collapses into one helper, and `/api/v2/stream/{ticket}` stays out of it because its credential is the sealed ticket in the path. **`GET /api/v2/songs` documents its 400.** A missing `genre` is rejected by axum before the handler runs, and the annotation listed only 200, 401 and 422. `search_catalog` has the same omission for its required `q`; it predates this branch and is left alone. Not applied: requiring an explicit read scope on `Access::Read`. Reading needs no scope by design — a name the server does not know grants nothing, which is what lets `catalog:read` read without anyone enumerating a vocabulary. Requiring a literal `read` would strand every token already issued, including the documented example, and would make an empty list and a scoped list differ in kind rather than in breadth. A test now pins it: a `catalog:read` token still mints a stream ticket, and an unauthenticated request still cannot. Signed-off-by: InstaZDLL --- docs/api-v2-guide.md | 8 +++--- src/http.rs | 6 ++--- src/media.rs | 57 +++++++++++++++++------------------------ tests/v2_foundations.rs | 30 ++++++++++++++++++++++ 4 files changed, 61 insertions(+), 40 deletions(-) diff --git a/docs/api-v2-guide.md b/docs/api-v2-guide.md index 594d28a..9d133f9 100644 --- a/docs/api-v2-guide.md +++ b/docs/api-v2-guide.md @@ -138,7 +138,7 @@ Wildcards are rejected. Native interactive clients should use the system browser: -1. Generate a 43–128 character PKCE verifier and keep it private. +1. Generate a 43–128 character PKCE verifier and keep it private. 2. Compute `challenge = base64url_no_padding(sha256(verifier))`. 3. Generate and retain an unpredictable `state` value. 4. Open the embedded UI route below in the system browser: @@ -399,10 +399,10 @@ Other user-data routes: |---|---| | Playlists | `GET/POST /api/v2/playlists`, `GET/PATCH/DELETE /api/v2/playlists/{id}` | | Favorites | `GET /api/v2/favorites`, `PUT/DELETE /api/v2/favorites/{track|album|artist}/{id}` | -| Ratings | `GET /api/v2/ratings`, `PUT /api/v2/ratings/{track|album|artist}/{id}` with `rating` 0–5 | +| Ratings | `GET /api/v2/ratings`, `PUT /api/v2/ratings/{track|album|artist}/{id}` with `rating` 0–5 | | Playback activity | `POST /api/v2/scrobbles`, `GET /api/v2/history`, `GET /api/v2/now-playing` | | Queue | `GET/PUT /api/v2/queue` (`track_ids` allows repeated tracks; maximum 400) | -| Shares | `GET/POST /api/v2/shares`, `PATCH/DELETE /api/v2/shares/{id}` (1–400 tracks) | +| Shares | `GET/POST /api/v2/shares`, `PATCH/DELETE /api/v2/shares/{id}` (1–400 tracks) | `POST /api/v2/shares` returns the bearer URL at creation and on an authenticated idempotent replay. Later list, snapshot and update responses omit it because @@ -426,7 +426,7 @@ curl "https://music.example.com/api/v2/sync/changes?after=CURSOR&limit=100" \ ``` Apply every event idempotently, advance to `next_cursor`, and continue while -`has_more` is true. Limits are 1–500. After durable local application, ACK the +`has_more` is true. Limits are 1–500. After durable local application, ACK the device cursor: ```bash diff --git a/src/http.rs b/src/http.rs index 1cab0d1..3049540 100644 --- a/src/http.rs +++ b/src/http.rs @@ -1084,7 +1084,7 @@ pub async fn list_random_songs( /// The native form of `getSongsByGenre`. `genre` is required: answering an /// unfiltered catalogue would drop the filter in silence. -#[utoipa::path(get, path = "/api/v2/songs", tag = "catalog", params(("genre" = String, Query), ("library_id" = Option, Query), ("offset" = Option, Query), ("limit" = Option, Query)), responses((status = 200, body = [crate::services::SongItem]), (status = 401, body = ErrorResponse), (status = 422, body = ErrorResponse)))] +#[utoipa::path(get, path = "/api/v2/songs", tag = "catalog", params(("genre" = String, Query), ("library_id" = Option, Query), ("offset" = Option, Query), ("limit" = Option, Query)), responses((status = 200, body = [crate::services::SongItem]), (status = 400, description = "genre is required"), (status = 401, body = ErrorResponse), (status = 422, body = ErrorResponse)))] pub async fn list_songs_by_genre( State(state): State, Query(query): Query, @@ -2234,7 +2234,7 @@ fn bearer_token(headers: &HeaderMap) -> Option<&str> { /// second helper a handler may forget to call — which is exactly what happened /// to the scope list, stored since the foundations and read by nothing. #[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum Access { +pub(crate) enum Access { /// Reads the caller's own catalogue and user data. Read, /// Writes on the caller's behalf: playlists, favorites, ratings, the queue, @@ -2281,7 +2281,7 @@ impl Access { /// on a credential that has not been narrowed away from it. Being an /// administrator does not widen a token, and a token cannot promote an /// ordinary account. -async fn authenticated( +pub(crate) async fn authenticated( state: &AppState, headers: &HeaderMap, access: Access, diff --git a/src/media.rs b/src/media.rs index 260177b..2298534 100644 --- a/src/media.rs +++ b/src/media.rs @@ -473,6 +473,27 @@ pub struct StreamTicketResponse { pub expires_at: i64, } +/// Resolves a Bearer caller for the media routes through the same door as +/// every other route. +/// +/// These three used to extract the header and call the authenticator +/// themselves, which meant they were outside the one place scopes are +/// checked. They are reads, so today the check grants what it already +/// granted; the point is that a mutation added here cannot quietly skip it, +/// which is exactly how the scope list came to be stored and never read. +/// +/// `/api/v2/stream/{ticket}` is deliberately not routed through this: its +/// credential is the sealed ticket in the path, because `