From 7d760e7cb778dc000354ac96707992594e09b71a Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Thu, 10 Sep 2026 15:10:02 +0200 Subject: [PATCH 1/4] feat(api): read a track's corrections, and a library's members MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two routes that were missing and blocked nothing. Both are reads: they have no consumer to migrate and no semantics to change, which is what separates them from the work that is actually waiting on an audit. **`GET /api/v2/tracks/{track_id}/overrides`.** The catalogue answers the effective value and says nothing about where it came from, so an editor cannot tell a corrected field from a scanned one, nor offer to restore what it cannot read. Separate from `GET /tracks/{track_id}`, which should keep answering `SongItem`: provenance is of no use to a listing, and putting it there would weigh down a type the frozen Subsonic façade also builds from. Writing it corrected the contract this repository recorded three days ago. `source = override ?? file` holds for **seven** of the nine fields, not nine. Artists and genres are not columns: correcting either is *materialised* — `apply_track_override_lists` deletes from `track_participant` and reinserts, and overwrites `artist_display` — because those rows feed every projection and the search index. Once a correction exists the file's own credits are no longer in the database, only in the file. The seven scalars survive because the projection merges them with `COALESCE` instead. So `TrackSourceTags` carries seven fields and says why it will never carry nine, and an editor offers to restore a list without previewing what it will restore to. **`GET /api/v2/libraries/{library_id}/members`.** `PUT` and `DELETE` on that pair have existed since M4 with nothing to read them back, so an interface could grant and revoke without showing who already had access — and no client can work it out, since an account's own membership says nothing about anyone else's. Tenancy is in the join, and a library the caller is not in answers 404 exactly as one that does not exist: removing the guard makes the test fail with 200 where it wants 404. Both are asserted inside tests that already had the accounts and the library they needed, rather than beside them: this suite's `hash_password` fixture raises a fresh critical CodeQL alert for every new test that creates an account. Claude-Session: https://claude.ai/code/session_01TyKunaKXS16hyFDBHwc5KK Signed-off-by: InstaZDLL --- docs/web-client-gap-analysis.md | 24 ++++++++- src/api/libraries.rs | 20 ++++++++ src/api/mod.rs | 8 +++ src/api/tracks.rs | 26 ++++++++++ src/lib.rs | 6 +++ src/services/admin.rs | 48 ++++++++++++++++++ src/services/mod.rs | 65 +++++++++++++++++++++++++ src/services/track_metadata.rs | 59 ++++++++++++++++++++++ tests/native_api.rs | 86 ++++++++++++++++++++++++++++++++- 9 files changed, 339 insertions(+), 3 deletions(-) diff --git a/docs/web-client-gap-analysis.md b/docs/web-client-gap-analysis.md index 210bc1be..aff9a69a 100644 --- a/docs/web-client-gap-analysis.md +++ b/docs/web-client-gap-analysis.md @@ -350,6 +350,22 @@ override = la correction WaveFlow, ou absente (ligne de `track_override`) effective = override ?? source ``` +> **Rectifié le 2026-09-10, en écrivant la route de lecture.** Ce schéma vaut +> pour **sept** des neuf champs, pas pour les neuf. Les artistes et les genres +> ne sont pas des colonnes : une correction sur l'un ou l'autre est +> **matérialisée** dans `track_participant`, `track_genre` et les chaînes +> `*_display` — `apply_track_override_lists` fait `DELETE FROM +> track_participant` puis réinsère —, parce que ces lignes alimentent toutes +> les projections et l'index de recherche. **Une fois la correction posée, la +> valeur du fichier n'est donc plus en base**, seulement dans le fichier. Les +> sept champs scalaires survivent parce que la projection les fusionne par +> `COALESCE` au lieu de les écraser. +> +> Conséquence pour l'éditeur : il montre la provenance des sept, et propose sur +> les deux autres un rétablissement **sans aperçu**. Rétablir relit le fichier, +> ce que `set_track_metadata` fait déjà ; ce qui n'est pas faisable à bon +> marché, c'est de l'afficher d'avance. + Le serveur travaille déjà ainsi et le dit : la migration `20260826010000_track_override.sql` écrit « *every column is nullable and NULL means "no correction here, use what the file said"* », et `song_select!` fait @@ -381,10 +397,14 @@ Une route de lecture nouvelle, et la route d'écriture existante corrigée. ``` GET /api/v2/tracks/{id} → SongItem, valeur effective, inchangé -GET /api/v2/tracks/{id}/overrides → { source: {…}, overrides: {…} } -PATCH /api/v2/tracks/{id} → patch partiel à trois états +GET /api/v2/tracks/{id}/overrides → { source: {…}, overrides: {…} } ✅ écrite +PATCH /api/v2/tracks/{id} → patch partiel à trois états ⏳ bloquée ``` +La route de lecture est écrite : elle n'a aucun consommateur à ménager et le +prérequis d'audit ne portait que sur le **changement de sémantique**. Le +`PATCH` attend toujours l'audit du client desktop. + `overrides` doit être la **ligne réelle**, pas une seconde projection `COALESCE` : c'est précisément la différence que l'éditeur affiche. `effective` n'a pas à y figurer, le client tient déjà le `SongItem`. diff --git a/src/api/libraries.rs b/src/api/libraries.rs index 744b8eb5..666ef6bb 100644 --- a/src/api/libraries.rs +++ b/src/api/libraries.rs @@ -262,6 +262,26 @@ pub async fn library_events_ack( Ok(StatusCode::NO_CONTENT) } +/// Who may see a library, and in what standing. +/// +/// `PUT` and `DELETE` on `/libraries/{library_id}/members/{user_id}` have +/// existed since M4 with nothing to read them back, so an interface could grant +/// and revoke without showing who already had access. +#[utoipa::path(get, path = "/api/v2/libraries/{library_id}/members", tag = "administration", params(("library_id" = Uuid, Path)), responses((status = 200, body = [crate::services::LibraryMember]), (status = 401, body = ErrorResponse), (status = 404, body = ErrorResponse)))] +pub async fn list_library_members( + State(state): State, + Path(library_id): Path, + headers: HeaderMap, +) -> Result>, ApiError> { + let user = authenticated(&state, &headers, Access::Read).await?; + state + .services + .library_members(user.id, library_id) + .await + .map(Json) + .map_err(service_error) +} + /// What a device says it has read. #[derive(Debug, serde::Deserialize, utoipa::ToSchema)] pub struct LibraryEventAckRequest { diff --git a/src/api/mod.rs b/src/api/mod.rs index 771b7e71..30dba18f 100644 --- a/src/api/mod.rs +++ b/src/api/mod.rs @@ -95,6 +95,10 @@ pub fn router(state: AppState) -> Router { "/api/v2/libraries", get(list_libraries).post(create_library), ) + .route( + "/api/v2/libraries/{library_id}/members", + get(list_library_members), + ) .route( "/api/v2/libraries/{library_id}/members/{user_id}", put(set_library_member).delete(remove_library_member), @@ -127,6 +131,10 @@ pub fn router(state: AppState) -> Router { get(get_track).patch(update_track), ) .route("/api/v2/tracks/{track_id}/lyrics", get(get_track_lyrics)) + .route( + "/api/v2/tracks/{track_id}/overrides", + get(get_track_overrides), + ) .route("/api/v2/albums", get(list_albums)) .route("/api/v2/genres", get(list_genres)) .route("/api/v2/albums/{album_id}", get(get_album)) diff --git a/src/api/tracks.rs b/src/api/tracks.rs index ec128640..3b046d3a 100644 --- a/src/api/tracks.rs +++ b/src/api/tracks.rs @@ -42,6 +42,32 @@ pub async fn list_tracks( Ok(Json(tracks)) } +/// What the file says and what a correction says instead. +/// +/// Separate from `GET /api/v2/tracks/{track_id}`, which answers the effective +/// value and should keep doing so: the catalogue has no use for provenance, +/// and putting it on `SongItem` would weigh down the type every listing +/// returns — a type the frozen Subsonic façade also builds from. +/// +/// It is the read half of correcting a tag. The write half, `PATCH +/// /api/v2/tracks/{track_id}`, still replaces the whole set, so a client that +/// means to change one field has to send back the others; until that is fixed +/// this route is what lets it know what they are. +#[utoipa::path(get, path = "/api/v2/tracks/{track_id}/overrides", tag = "catalog", params(("track_id" = Uuid, Path)), responses((status = 200, body = crate::services::TrackOverrides), (status = 401, body = ErrorResponse), (status = 404, body = ErrorResponse)))] +pub async fn get_track_overrides( + State(state): State, + Path(track_id): Path, + headers: HeaderMap, +) -> Result, ApiError> { + let user = authenticated(&state, &headers, Access::Read).await?; + state + .services + .track_overrides(user.id, track_id) + .await + .map(Json) + .map_err(service_error) +} + #[utoipa::path(get, path = "/api/v2/tracks/{track_id}", tag = "catalog", params(("track_id" = Uuid, Path)), responses((status = 200, body = crate::services::SongItem), (status = 401, body = ErrorResponse), (status = 404, body = ErrorResponse)))] pub async fn get_track( State(state): State, diff --git a/src/lib.rs b/src/lib.rs index 9ac1739e..3f62e642 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -122,6 +122,7 @@ fn chunk_body_limit(limits: &config::UploadLimits) -> usize { api::start_scan, api::list_libraries, api::create_library, + api::list_library_members, api::set_library_member, api::remove_library_member, api::scan_status, @@ -136,6 +137,7 @@ fn chunk_body_limit(limits: &config::UploadLimits) -> usize { api::get_track, api::update_track, api::get_track_lyrics, + api::get_track_overrides, api::list_albums, api::list_genres, api::get_album, @@ -211,7 +213,11 @@ fn chunk_body_limit(limits: &config::UploadLimits) -> usize { services::AlbumItem, services::ArtistItem, services::GenreItem, + services::LibraryMember, services::SongItem, + services::TrackOverrides, + services::TrackOverrideValues, + services::TrackSourceTags, lyrics::LyricsList, lyrics::StructuredLyrics, lyrics::LyricsLine, diff --git a/src/services/admin.rs b/src/services/admin.rs index b1459d3c..efb6600b 100644 --- a/src/services/admin.rs +++ b/src/services/admin.rs @@ -5,6 +5,54 @@ use super::*; impl DomainServices { + /// Who may see a library, and in what standing. + /// + /// The write side has existed since M4 — `PUT` and `DELETE` on + /// `/libraries/{id}/members/{user}` — with nothing to read it back, so a + /// screen could grant and revoke without ever showing who already had + /// access. That is not a listing anyone can build client-side: an account's + /// own membership tells it nothing about anyone else's. + /// + /// Restricted to members, and by membership in the join rather than by a + /// check ahead of it: a library the caller is not in is missing, and never + /// confirmed to exist by a different answer. + pub async fn library_members( + &self, + user_id: Uuid, + library_id: Uuid, + ) -> Result, ServiceError> { + let rows = sqlx::query( + "SELECT m.user_id, a.username, m.role, m.created_at \ + FROM library_member m \ + JOIN account a ON a.id=m.user_id \ + WHERE m.library_id=? AND EXISTS ( \ + SELECT 1 FROM library_member self \ + WHERE self.library_id=m.library_id AND self.user_id=?) \ + ORDER BY a.username COLLATE NOCASE", + ) + .bind(library_id.to_string()) + .bind(user_id.to_string()) + .fetch_all(self.db.pool()) + .await?; + // An empty answer would be indistinguishable from a library the caller + // cannot see, and every library has at least its owner — so nothing + // here means nothing to see. + if rows.is_empty() { + return Err(ServiceError::NotFound); + } + rows.into_iter() + .map(|row| { + Ok(LibraryMember { + user_id: parse_uuid(row.try_get("user_id")?)?, + username: row.try_get("username")?, + role: row.try_get("role")?, + created_at: row.try_get("created_at")?, + }) + }) + .collect::, sqlx::Error>>() + .map_err(ServiceError::from) + } + pub async fn users(&self, actor_id: Uuid) -> Result, ServiceError> { self.require_admin(actor_id).await?; let mut users = sqlx::query("SELECT a.id, a.username, a.role, a.disabled, c.user_id IS NOT NULL AS has_credential FROM account a LEFT JOIN subsonic_credential c ON c.user_id=a.id ORDER BY a.username COLLATE NOCASE") diff --git a/src/services/mod.rs b/src/services/mod.rs index 367df3ab..50047ed1 100644 --- a/src/services/mod.rs +++ b/src/services/mod.rs @@ -535,6 +535,71 @@ pub struct LibraryEventPage { pub purged_through: i64, } +/// What a track's tags say, and what a correction says instead. +/// +/// The editor's view of [`TrackMetadataPatch`]: every field it can write, with +/// the file's own value beside the correction standing over it. The catalogue +/// never shows this — it answers `effective`, which is `override ?? source` — +/// and nothing else needs it. It exists because an editor cannot offer to +/// restore a value it cannot read, and because a client that means to change +/// one correction has to know the others in order not to drop them. +#[derive(Debug, Clone, Serialize, ToSchema)] +pub struct TrackOverrides { + /// What the file said, for the fields where the database still knows. + pub source: TrackSourceTags, + /// The correction, field by field. `null` means no correction on that + /// field, which is what the row's nullable columns mean. + pub overrides: TrackOverrideValues, +} + +/// The file's own values, as the last scan read them. +/// +/// **Artists and genres are absent, and cannot be added here.** They are more +/// than columns: a correction to either is *materialised* into +/// `track_participant`, `track_genre` and the `*_display` strings, because +/// those rows feed every projection and the search index. Applying it +/// overwrites what the scan read, so once a correction exists the file's own +/// credits are no longer in the database — only in the file. The seven fields +/// below survive because the projection merges them with `COALESCE` instead, +/// leaving the scanned column untouched. +/// +/// Restoring a corrected list therefore re-reads the file, which +/// `set_track_metadata` already does. What cannot be done cheaply is *showing* +/// it beforehand, so an editor offers the reset without previewing its result. +#[derive(Debug, Clone, Serialize, ToSchema)] +pub struct TrackSourceTags { + pub title: String, + pub sort_title: Option, + pub year: Option, + pub track_number: Option, + pub disc_number: Option, + pub musicbrainz_recording_id: Option, + pub comment: Option, +} + +/// The `track_override` row as it stands, not a projection of it. +#[derive(Debug, Clone, Default, Serialize, ToSchema)] +pub struct TrackOverrideValues { + pub title: Option, + pub sort_title: Option, + pub year: Option, + pub track_number: Option, + pub disc_number: Option, + pub musicbrainz_recording_id: Option, + pub comment: Option, + pub artists: Option>, + pub genres: Option>, +} + +/// One account's standing in a library. +#[derive(Debug, Clone, Serialize, ToSchema)] +pub struct LibraryMember { + pub user_id: Uuid, + pub username: String, + pub role: String, + pub created_at: i64, +} + /// Everything one account has starred, across the three entity kinds. #[derive(Debug, Clone)] pub struct StarredCatalog { diff --git a/src/services/track_metadata.rs b/src/services/track_metadata.rs index f4ec1ebc..35804b75 100644 --- a/src/services/track_metadata.rs +++ b/src/services/track_metadata.rs @@ -341,4 +341,63 @@ impl DomainServices { .pop() .ok_or(ServiceError::NotFound) } + + /// What the file says and what the correction says, side by side. + /// + /// Read in one snapshot: a page that showed a source from before a write + /// and an override from after it would describe a track that never + /// existed. Tenancy is in the join, so a track in a library the caller is + /// not a member of is missing rather than forbidden. + /// + /// `LEFT JOIN`, because most tracks carry no correction at all and an + /// absent row is an answer — every field `null` — rather than a 404. + pub async fn track_overrides( + &self, + user_id: Uuid, + track_id: Uuid, + ) -> Result { + let row = sqlx::query( + "SELECT t.title, t.sort_title, t.year, t.track_number, t.disc_number, t.musicbrainz_recording_id, t.comment, ovr.title AS o_title, ovr.sort_title AS o_sort_title, ovr.year AS o_year, ovr.track_number AS o_track_number, ovr.disc_number AS o_disc_number, ovr.musicbrainz_recording_id AS o_musicbrainz_recording_id, ovr.comment AS o_comment, ovr.artists AS o_artists, ovr.genres AS o_genres FROM track t JOIN library_member m ON m.library_id=t.library_id LEFT JOIN track_override ovr ON ovr.track_id=t.id WHERE t.id=? AND m.user_id=?", + ) + .bind(track_id.to_string()) + .bind(user_id.to_string()) + .fetch_optional(self.db.pool()) + .await? + .ok_or(ServiceError::NotFound)?; + + // Stored as JSON rather than the `;`-joined form the tag columns use, + // because an override is a list someone typed on purpose. A row that + // will not parse is reported as no correction rather than taking the + // request down: the editor then shows the file's credits, which is the + // safe reading of a value nobody can interpret. + let list = |column: &str| -> Option> { + row.try_get::, _>(column) + .ok() + .flatten() + .and_then(|raw| serde_json::from_str::>(&raw).ok()) + }; + + Ok(TrackOverrides { + source: TrackSourceTags { + title: row.try_get("title")?, + sort_title: row.try_get("sort_title")?, + year: row.try_get("year")?, + track_number: row.try_get("track_number")?, + disc_number: row.try_get("disc_number")?, + musicbrainz_recording_id: row.try_get("musicbrainz_recording_id")?, + comment: row.try_get("comment")?, + }, + overrides: TrackOverrideValues { + title: row.try_get("o_title")?, + sort_title: row.try_get("o_sort_title")?, + year: row.try_get("o_year")?, + track_number: row.try_get("o_track_number")?, + disc_number: row.try_get("o_disc_number")?, + musicbrainz_recording_id: row.try_get("o_musicbrainz_recording_id")?, + comment: row.try_get("o_comment")?, + artists: list("o_artists"), + genres: list("o_genres"), + }, + }) + } } diff --git a/tests/native_api.rs b/tests/native_api.rs index f8d1492d..69a4f4b1 100644 --- a/tests/native_api.rs +++ b/tests/native_api.rs @@ -1338,6 +1338,37 @@ async fn a_track_correction_survives_the_scan_that_would_have_erased_it() { // across an edit. assert_eq!(corrected["full_hash"], scanned_hash); + // The track answers the effective value and says nothing about where it + // came from, so an editor cannot tell a corrected field from a scanned one + // — nor offer to restore what it cannot read. That is what the overrides + // route is for, and it is the read half of correcting a tag. + let overrides = { + let response = router + .clone() + .oneshot( + Request::get(format!("/api/v2/tracks/{track_id}/overrides")) + .header("authorization", format!("Bearer {token}")) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(response.status(), StatusCode::OK); + json_body(response).await + }; + // The scanned column is untouched by the correction: the projection merges + // the two with COALESCE rather than writing over the file's value. + assert_eq!(overrides["source"]["title"], "Mispelled Titel"); + assert_eq!(overrides["overrides"]["title"], "Misspelled Title"); + assert_eq!(overrides["overrides"]["year"], 1998); + // A field nobody corrected is null, which is what its column holds. + assert!(overrides["overrides"]["comment"].is_null()); + assert!(overrides["overrides"]["artists"].is_null()); + // Artists and genres are deliberately absent from `source`: correcting + // either rewrites `track_participant` and the display string, so once a + // correction exists the file's own credits are no longer in the database. + assert!(overrides["source"].get("artists").is_none()); + // The headline. A scan applies `title=excluded.title` over the track row, // so a correction stored there would be gone by now. // @@ -1418,7 +1449,60 @@ async fn a_track_correction_survives_the_scan_that_would_have_erased_it() { .await .unwrap(); let listener_token = login_token(&router, "tag-listener", password).await; - let (status, _) = patch(listener_token, serde_json::json!({ "title": "Nope" })).await; + let (status, _) = patch( + listener_token.clone(), + serde_json::json!({ "title": "Nope" }), + ) + .await; + assert_eq!(status, StatusCode::NOT_FOUND); + + // Who may see this library, which until now could be written and never + // read: an interface could grant and revoke without showing who already + // had access, and no client can work that out for itself — an account's own + // membership says nothing about anyone else's. + let members = |token: String, library: String| { + let router = router.clone(); + async move { + let response = router + .oneshot( + Request::get(format!("/api/v2/libraries/{library}/members")) + .header("authorization", format!("Bearer {token}")) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + (response.status(), json_body(response).await) + } + }; + let (status, listed) = members(listener_token.clone(), library_id.to_string()).await; + assert_eq!(status, StatusCode::OK); + let listed = listed.as_array().expect("a list"); + assert_eq!(listed.len(), 2, "the owner and the listener"); + // Ordered by username, so the answer does not depend on insertion order. + assert_eq!(listed[0]["username"], "tag-listener"); + assert_eq!(listed[0]["role"], "listener"); + assert_eq!(listed[1]["username"], "tagger"); + assert_eq!(listed[1]["role"], "owner"); + + // A library the caller is not in is missing, not forbidden. Answering + // anything else would confirm that it exists. + let stranger = state + .db + .create_account("tag-stranger", &hash, AccountRole::User, now_ms()) + .await + .unwrap(); + assert_ne!(stranger, listener, "a distinct account"); + let stranger_token = login_token(&router, "tag-stranger", password).await; + let (status, _) = members(stranger_token, library_id.to_string()).await; + assert_eq!(status, StatusCode::NOT_FOUND); + // And a library nobody has is the same answer, so the two cannot be told + // apart from outside. + let (status, _) = members( + listener_token, + "00000000-0000-4000-8000-000000000000".into(), + ) + .await; assert_eq!(status, StatusCode::NOT_FOUND); // And a manager may, which is the half of the rule the refusal above cannot From 64929522d31364ca129e74acee8a8345c3bab9fc Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Thu, 10 Sep 2026 15:10:12 +0200 Subject: [PATCH 2/4] feat(webapp): show who may see a library, and say what the API does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The membership screen the plan filed under lot C and this branch's route finally makes possible. It lists who has access, changes a role, revokes one, and grants a listener to an account that has none. The owner is shown and offers no control: the route refuses `owner` outright, so a select there would be offering a refusal. Behind a disclosure, for the reason the token panel is: the admin screen renders it once per library, so loading on mount would ask for every membership list every time the page opened. The end-to-end test asserts that nothing is asked until the panel opens, and that it is asked exactly once when it does. Also corrects four doc comments that described the caller rather than the function. `listAlbums`, `listArtists`, `listGenres` and `listGenreSongs` were documented as listing "in the active library", while their signatures take `libraryId?: string` and are perfectly able to list across libraries — a reader following the comment would have believed the scope was guaranteed by construction. `listRandomSongs` already said "optionally scoped", and is left alone. Claude-Session: https://claude.ai/code/session_01TyKunaKXS16hyFDBHwc5KK Signed-off-by: InstaZDLL --- webapp/e2e/studio-nocturne.spec.ts | 57 ++++++++++++ webapp/src/api.ts | 40 +++++++- webapp/src/i18n.tsx | 14 +++ webapp/src/pages.tsx | 145 +++++++++++++++++++++++++++++ webapp/src/styles.css | 18 ++++ 5 files changed, 270 insertions(+), 4 deletions(-) diff --git a/webapp/e2e/studio-nocturne.spec.ts b/webapp/e2e/studio-nocturne.spec.ts index dd491617..e96c5e80 100644 --- a/webapp/e2e/studio-nocturne.spec.ts +++ b/webapp/e2e/studio-nocturne.spec.ts @@ -201,6 +201,25 @@ async function mockAuthenticatedApi(page: Page) { }); return; } + if (url.pathname.endsWith("/members")) { + await route.fulfill({ + json: [ + { + user_id: "user-1", + username: "listener", + role: "owner", + created_at: 1, + }, + { + user_id: "user-2", + username: "guest", + role: "listener", + created_at: 2, + }, + ], + }); + return; + } if (url.pathname.endsWith("/tokens")) { if (tokensFail) { await route.fulfill({ status: 500, json: { error: "boom" } }); @@ -772,3 +791,41 @@ test("says so when the progress stream never connects", async ({ page }) => { "The progress stream could not be opened", ); }); + +/** + * Library membership could be written since M4 and never read, so an interface + * could grant and revoke without ever showing who already had access. The list + * is behind a disclosure for the same reason the tokens are: the panel renders + * once per library, and loading on mount would ask for every membership list + * every time the admin screen opened. + */ +test("lists who may see a library, once its panel is opened", async ({ + page, +}) => { + const asked: string[] = []; + page.on("request", (request) => { + const path = new URL(request.url()).pathname; + if (path.endsWith("/members")) asked.push(path); + }); + + await page.goto("/admin"); + const disclosure = page.getByRole("button", { + name: "Who may see this library", + }); + await expect(disclosure).toHaveAttribute("aria-expanded", "false"); + expect(asked).toEqual([]); + + await disclosure.click(); + await expect(page.getByText("guest")).toBeVisible(); + expect(asked).toEqual(["/api/v2/libraries/library-1/members"]); + + // The owner is shown and offers no role control: the route refuses `owner` + // outright, so a select here would be offering a refusal. + await expect(page.getByText("owner, and stays one")).toBeVisible(); + await expect( + page.getByRole("combobox", { name: "Role: listener" }), + ).toHaveCount(0); + await expect( + page.getByRole("combobox", { name: "Role: guest" }), + ).toHaveValue("listener"); +}); diff --git a/webapp/src/api.ts b/webapp/src/api.ts index 5cad6917..92c5c86f 100644 --- a/webapp/src/api.ts +++ b/webapp/src/api.ts @@ -342,12 +342,12 @@ function scoped( return libraryId ? { ...extra, library_id: libraryId } : extra; } -/** List albums in the active library, using the requested catalogue order. */ +/** Albums in the requested order, scoped to `libraryId` when one is given. */ export const listAlbums = (sort?: AlbumSort, libraryId?: string) => collect("/api/v2/albums", scoped(libraryId, sort ? { sort } : {})); export const getAlbum = (id: string) => call(`/api/v2/albums/${id}`); -/** List artists in the active library. */ +/** Artists, scoped to `libraryId` when one is given. */ export const listArtists = (libraryId?: string) => collect("/api/v2/artists", scoped(libraryId)); export const getArtist = (id: string) => @@ -578,6 +578,38 @@ export type ApiToken = { }; /** List every API token issued to an account. */ +export type LibraryMember = { + user_id: string; + username: string; + role: "owner" | "manager" | "listener"; + created_at: number; +}; + +/** + * Who may see a library. The write side has existed since M4 with nothing to + * read it back, so a screen could grant and revoke without showing who already + * had access — and no client can work that out for itself, since an account's + * own membership says nothing about anyone else's. + */ +export const listLibraryMembers = (libraryId: string) => + call(`/api/v2/libraries/${libraryId}/members`); + +/** `owner` cannot be granted: the route refuses it. */ +export const setLibraryMember = ( + libraryId: string, + userId: string, + role: "manager" | "listener", +) => + call(`/api/v2/libraries/${libraryId}/members/${userId}`, { + method: "PUT", + body: JSON.stringify({ role }), + }); + +export const removeLibraryMember = (libraryId: string, userId: string) => + call(`/api/v2/libraries/${libraryId}/members/${userId}`, { + method: "DELETE", + }); + export const listApiTokens = (username: string) => call( `/api/v2/admin/users/${encodeURIComponent(username)}/tokens`, @@ -702,11 +734,11 @@ export type Genre = { album_count: number; }; -/** List genre summaries in the active library. */ +/** Genre summaries, scoped to `libraryId` when one is given. */ export const listGenres = (libraryId?: string) => call(`/api/v2/genres?${new URLSearchParams(scoped(libraryId))}`); -/** List songs for a genre in the active library. */ +/** Songs of one genre, scoped to `libraryId` when one is given. */ export const listGenreSongs = (genre: string, libraryId?: string) => collect("/api/v2/songs/by-genre", scoped(libraryId, { genre })); diff --git a/webapp/src/i18n.tsx b/webapp/src/i18n.tsx index 0b7f6639..da0b8dee 100644 --- a/webapp/src/i18n.tsx +++ b/webapp/src/i18n.tsx @@ -246,6 +246,13 @@ const en = { "admin.tokenNone": "No token on this account.", "scan.lost": "The progress stream could not be opened. The scan itself is unaffected.", + "admin.members": "Who may see this library", + "admin.memberRole": "Role", + "admin.memberRemove": "Remove", + "admin.memberOwner": "owner, and stays one", + "admin.memberAdd": "Give access to", + "admin.memberChoose": "Choose an account…", + "admin.memberError": "That membership could not be changed.", } as const; export type TranslationKey = keyof typeof en; @@ -489,6 +496,13 @@ const fr: Record = { "admin.tokenNone": "Aucun jeton sur ce compte.", "scan.lost": "Le flux de progression n’a pas pu s’ouvrir. L’analyse elle-même se poursuit.", + "admin.members": "Qui peut voir cette bibliothèque", + "admin.memberRole": "Rôle", + "admin.memberRemove": "Retirer", + "admin.memberOwner": "propriétaire, et le reste", + "admin.memberAdd": "Donner accès à", + "admin.memberChoose": "Choisir un compte…", + "admin.memberError": "Cette appartenance n’a pas pu être modifiée.", }; export type Locale = "en" | "fr"; diff --git a/webapp/src/pages.tsx b/webapp/src/pages.tsx index d854921a..044b3821 100644 --- a/webapp/src/pages.tsx +++ b/webapp/src/pages.tsx @@ -34,6 +34,7 @@ import { getLyrics, getTrack, isAllowedRedirect, + type LibraryMember, type LyricsLine, type LyricsList, listAlbums, @@ -45,6 +46,7 @@ import { listGenres, listHistory, listLibraries, + listLibraryMembers, listNowPlaying, listPlaylists, listRandomSongs, @@ -53,6 +55,7 @@ import { login, type NowPlaying, type Playlist, + removeLibraryMember, revokeApiToken, type ScanJob, type SearchResult, @@ -62,6 +65,7 @@ import { search, setBookmark, setFavorite, + setLibraryMember, setRating, setSubsonicCredential, setUserDisabled, @@ -1538,6 +1542,142 @@ function ApiTokensPanel({ username }: { username: string }) { ); } +/** + * Who may see one library, and in what standing. + * + * Its own disclosure per library, for the reason the token panel has one: the + * admin screen renders it once per library, and loading on mount would ask for + * every membership list every time the page opened. + * + * The owner is shown and cannot be changed from here — the route refuses + * `owner` outright, so offering it would be offering a refusal. + */ +function LibraryMembersPanel({ + libraryId, + users, +}: { + libraryId: string; + users: User[]; +}) { + const { t } = useI18n(); + const [open, setOpen] = useState(false); + const [revision, setRevision] = useState(0); + const [failed, setFailed] = useState(false); + const { value, error } = useAsync( + () => (open ? listLibraryMembers(libraryId) : Promise.resolve(null)), + [libraryId, revision, open], + ); + + async function grant(userId: string, role: "manager" | "listener") { + setFailed(false); + try { + await setLibraryMember(libraryId, userId, role); + setRevision((n) => n + 1); + } catch { + setFailed(true); + } + } + + async function revoke(userId: string) { + setFailed(false); + try { + await removeLibraryMember(libraryId, userId); + setRevision((n) => n + 1); + } catch { + setFailed(true); + } + } + + const members = value ?? []; + const outside = users.filter( + (user) => !members.some((member) => member.user_id === user.id), + ); + return ( +
+

+ +

+ {open ? ( + <> + {failed ?

{t("admin.memberError")}

: null} + {error ? ( +

+ {t("common.loadError")} +

+ ) : ( +
    + {members.map((member) => ( +
  • +
    + {member.username} + {member.role} +
    + {member.role === "owner" ? ( + {t("admin.memberOwner")} + ) : ( +
    + + +
    + )} +
    • + ))} +
+ )} + {outside.length ? ( + + ) : null} + + ) : null} +
+ ); +} + export function AdminPage() { const signedInUser = currentUser(); const { t } = useI18n(); @@ -1668,6 +1808,11 @@ export function AdminPage() { ))} + {libraries.map((library) => ( +
  • + +
    • + ))}
    diff --git a/webapp/src/styles.css b/webapp/src/styles.css index 36956214..f8d70af4 100644 --- a/webapp/src/styles.css +++ b/webapp/src/styles.css @@ -1918,3 +1918,21 @@ button.danger { transition: none; } } + +/* The membership panel is a row of the library list rather than a column of + its own: it belongs to one library, and there is one of them per library. */ +.member-row { + display: block; + padding: 0; + border: 0; +} + +.member-row .admin-panel { + padding: 0.6rem 0; + border: 0; + background: none; +} + +.member-row .control { + min-width: 8rem; +} From dbce66cab9ccdc8ff01583c326b7994eaf2a8051 Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Thu, 10 Sep 2026 15:34:49 +0200 Subject: [PATCH 3/4] test(api): give the members route its own test, and pair each panel with its library MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings, both valid. **The membership assertions were in the tag-correction test**, which does not speak about membership. They were there to avoid another occurrence of the fixture CodeQL watches, and that is a poor reason to hide assertions in a test named for something else. Split instead. The refusal joins `catalog_and_scan_routes_blur_foreign_libraries`, whose whole subject is what a stranger gets from a library's routes — a 404 there costs nothing, since the accounts already exist. The happy path gets its own test. That one creates accounts, so it will raise the recurring critical CodeQL alert on `hash_password` and needs a dismissal; the trade is deliberate. **Two `libraries.map()` passes listed every library and then every membership panel.** With three libraries the third one's members sat four rows below it, under something that was not theirs. One pass now, with a `Fragment`. The first check of that fix proved nothing: inverting it left the test passing, because the mock held a single library and both layouts render the same rows for one. The defect only appears from the second library on, so the mock has two, and the inversion now fails. Claude-Session: https://claude.ai/code/session_01TyKunaKXS16hyFDBHwc5KK Signed-off-by: InstaZDLL --- tests/native_api.rs | 155 +++++++++++++++++++---------- webapp/e2e/studio-nocturne.spec.ts | 23 ++++- webapp/src/pages.tsx | 38 +++---- 3 files changed, 141 insertions(+), 75 deletions(-) diff --git a/tests/native_api.rs b/tests/native_api.rs index 69a4f4b1..4ccf41a9 100644 --- a/tests/native_api.rs +++ b/tests/native_api.rs @@ -1237,12 +1237,15 @@ async fn catalog_and_scan_routes_blur_foreign_libraries() { .unwrap(); assert_eq!(invalid_page.status(), StatusCode::UNPROCESSABLE_ENTITY); - for method in ["GET", "POST"] { - let uri = if method == "GET" { - format!("/api/v2/libraries/{library_id}/tracks") - } else { - format!("/api/v2/libraries/{library_id}/scans") - }; + for (method, path) in [ + ("GET", "tracks"), + ("POST", "scans"), + // Membership answers 404 for the same reason the others do: telling a + // stranger who the members are would first tell them the library is + // there. + ("GET", "members"), + ] { + let uri = format!("/api/v2/libraries/{library_id}/{path}"); let request = Request::builder() .method(method) .uri(uri) @@ -1254,6 +1257,97 @@ async fn catalog_and_scan_routes_blur_foreign_libraries() { } } +/// Who may see a library, and in what standing. +/// +/// `PUT` and `DELETE` on `/libraries/{id}/members/{user}` have existed since M4 +/// with nothing to read them back, so an interface could grant and revoke +/// without ever showing who already had access — and no client can work that +/// out for itself, since an account's own membership says nothing about anyone +/// else's. +/// +/// The refusal side lives in `catalog_and_scan_routes_blur_foreign_libraries`, +/// beside the other routes that answer 404 rather than confirm a library +/// exists. This is the half that says what the route returns when it does. +#[tokio::test] +async fn the_members_route_names_who_may_see_a_library() { + let (_temp, config, state) = test_app().await; + let password = "correct horse battery staple"; + let hash = security::hash_password(password).unwrap(); + let owner = state + .db + .create_account("member-owner", &hash, AccountRole::Admin, now_ms()) + .await + .unwrap(); + let guest = state + .db + .create_account("member-guest", &hash, AccountRole::User, now_ms()) + .await + .unwrap(); + let music = config.data_dir.join("member-music"); + std::fs::create_dir_all(&music).unwrap(); + let root = std::fs::canonicalize(&music).unwrap(); + let library_id = state + .db + .create_library( + owner, + "Member library", + &root, + LibraryVisibility::Private, + now_ms(), + ) + .await + .unwrap(); + sqlx::query( + "INSERT INTO library_member (library_id, user_id, role, created_at) \ + VALUES (?, ?, 'listener', ?)", + ) + .bind(library_id.to_string()) + .bind(guest.to_string()) + .bind(now_ms()) + .execute(state.db.pool()) + .await + .unwrap(); + + let router = waveflow_server::app(&config, state); + let token = login_token(&router, "member-guest", password).await; + let response = router + .clone() + .oneshot( + Request::get(format!("/api/v2/libraries/{library_id}/members")) + .header("authorization", format!("Bearer {token}")) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(response.status(), StatusCode::OK); + let listed = json_body(response).await; + let listed = listed.as_array().expect("a list"); + assert_eq!(listed.len(), 2, "the owner and the listener"); + // Ordered by username, so the answer does not depend on insertion order. + assert_eq!(listed[0]["username"], "member-guest"); + assert_eq!(listed[0]["role"], "listener"); + assert_eq!(listed[1]["username"], "member-owner"); + assert_eq!(listed[1]["role"], "owner"); + // A listener sees the list: the route is about who may read the library, + // and every member may ask. Restricting it to owners would leave a + // listener unable to tell whether a library is shared at all. + assert_eq!(listed[0]["user_id"], guest.to_string()); + + // A library nobody has answers the same 404 as one the caller cannot see, + // so the two cannot be told apart from outside. + let missing = router + .oneshot( + Request::get("/api/v2/libraries/00000000-0000-4000-8000-000000000000/members") + .header("authorization", format!("Bearer {token}")) + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(missing.status(), StatusCode::NOT_FOUND); +} + /// A correction outlives the scan that would have erased it. /// /// Writing tags had two obvious routes and both were wrong: rewriting the file @@ -1456,55 +1550,6 @@ async fn a_track_correction_survives_the_scan_that_would_have_erased_it() { .await; assert_eq!(status, StatusCode::NOT_FOUND); - // Who may see this library, which until now could be written and never - // read: an interface could grant and revoke without showing who already - // had access, and no client can work that out for itself — an account's own - // membership says nothing about anyone else's. - let members = |token: String, library: String| { - let router = router.clone(); - async move { - let response = router - .oneshot( - Request::get(format!("/api/v2/libraries/{library}/members")) - .header("authorization", format!("Bearer {token}")) - .body(Body::empty()) - .unwrap(), - ) - .await - .unwrap(); - (response.status(), json_body(response).await) - } - }; - let (status, listed) = members(listener_token.clone(), library_id.to_string()).await; - assert_eq!(status, StatusCode::OK); - let listed = listed.as_array().expect("a list"); - assert_eq!(listed.len(), 2, "the owner and the listener"); - // Ordered by username, so the answer does not depend on insertion order. - assert_eq!(listed[0]["username"], "tag-listener"); - assert_eq!(listed[0]["role"], "listener"); - assert_eq!(listed[1]["username"], "tagger"); - assert_eq!(listed[1]["role"], "owner"); - - // A library the caller is not in is missing, not forbidden. Answering - // anything else would confirm that it exists. - let stranger = state - .db - .create_account("tag-stranger", &hash, AccountRole::User, now_ms()) - .await - .unwrap(); - assert_ne!(stranger, listener, "a distinct account"); - let stranger_token = login_token(&router, "tag-stranger", password).await; - let (status, _) = members(stranger_token, library_id.to_string()).await; - assert_eq!(status, StatusCode::NOT_FOUND); - // And a library nobody has is the same answer, so the two cannot be told - // apart from outside. - let (status, _) = members( - listener_token, - "00000000-0000-4000-8000-000000000000".into(), - ) - .await; - assert_eq!(status, StatusCode::NOT_FOUND); - // And a manager may, which is the half of the rule the refusal above cannot // show: were it `Owner` alone, everything asserted so far would still pass. let manager = state diff --git a/webapp/e2e/studio-nocturne.spec.ts b/webapp/e2e/studio-nocturne.spec.ts index e96c5e80..46ea7459 100644 --- a/webapp/e2e/studio-nocturne.spec.ts +++ b/webapp/e2e/studio-nocturne.spec.ts @@ -808,13 +808,30 @@ test("lists who may see a library, once its panel is opened", async ({ if (path.endsWith("/members")) asked.push(path); }); + // Two libraries, because one cannot tell the two layouts apart: with a single + // library, "list then panel" and "list, then every panel" produce the same + // rows. The defect only shows from the second library on. + libraries = [ + library("library-1", "Ma musique"), + library("library-2", "Les enfants"), + ]; + await page.goto("/admin"); - const disclosure = page.getByRole("button", { - name: "Who may see this library", - }); + const disclosure = page + .getByRole("button", { name: "Who may see this library" }) + .first(); await expect(disclosure).toHaveAttribute("aria-expanded", "false"); expect(asked).toEqual([]); + // Each library's membership sits directly under that library, not after the + // whole list: two render passes put the third library's members four rows + // below it, which reads as belonging to whatever is above them. + const rows = page.locator(".admin-panel .resource-list > li"); + await expect(rows.nth(0)).toContainText("Ma musique"); + await expect(rows.nth(1)).toContainText("Who may see this library"); + await expect(rows.nth(2)).toContainText("Les enfants"); + await expect(rows.nth(3)).toContainText("Who may see this library"); + await disclosure.click(); await expect(page.getByText("guest")).toBeVisible(); expect(asked).toEqual(["/api/v2/libraries/library-1/members"]); diff --git a/webapp/src/pages.tsx b/webapp/src/pages.tsx index 044b3821..dea86b0c 100644 --- a/webapp/src/pages.tsx +++ b/webapp/src/pages.tsx @@ -1,6 +1,7 @@ import { Link, useNavigate } from "@tanstack/react-router"; import { type FormEvent, + Fragment, type ReactNode, useEffect, useMemo, @@ -1794,24 +1795,27 @@ export function AdminPage() {
      + {/* One pass, so each library's membership sits under that library. + Two passes listed every library and then every panel, which put + the third library's members four rows below it. */} {libraries.map((library) => ( -
    • -
      - {library.name} - {library.visibility} -
      - -
      • - ))} - {libraries.map((library) => ( -
    • - -
      • + +
    • +
      + {library.name} + {library.visibility} +
      + +
      • +
    • + +
      • +       ))}
    From a752fc5e3bad35f101067172bdf0d3cf148d9dfd Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Thu, 10 Sep 2026 15:46:42 +0200 Subject: [PATCH 4/4] fix(webapp): a membership list that failed to load is not an empty one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `value ?? []` conflated three states, and only one of them is a list. An empty list is not even reachable: every library has at least its owner, and the service answers 404 rather than nothing. So `null` only ever means "not known yet" — in flight, or failed — and standing an empty array in for it made every account on the server look like a non-member. The panel then offered access to people who already had it, printed underneath the notice saying the list could not be read. The list is now `null` until it is one, the wait says so, and the grant control is built from the list rather than from its absence. Restoring `?? []` fails the new test on both projects — checked with the build succeeding first, so the suite ran against the inverted bundle and not a stale one. Claude-Session: https://claude.ai/code/session_01TyKunaKXS16hyFDBHwc5KK Signed-off-by: InstaZDLL --- webapp/e2e/studio-nocturne.spec.ts | 31 ++++++++++++++++++++++++++++++ webapp/src/pages.tsx | 19 ++++++++++++++---- 2 files changed, 46 insertions(+), 4 deletions(-) diff --git a/webapp/e2e/studio-nocturne.spec.ts b/webapp/e2e/studio-nocturne.spec.ts index 46ea7459..f424e80d 100644 --- a/webapp/e2e/studio-nocturne.spec.ts +++ b/webapp/e2e/studio-nocturne.spec.ts @@ -77,6 +77,7 @@ const library = (id: string, name: string) => ({ let libraries: Array> = [library("library-1", "Ma musique")]; /** Flipped by the two tests that check a failure is shown as one. */ +let membersFail = false; let tokensFail = false; let scanFails = false; /** The stream never answers at all, which is what a lost network looks like. */ @@ -201,6 +202,10 @@ async function mockAuthenticatedApi(page: Page) { }); return; } + if (url.pathname.endsWith("/members") && membersFail) { + await route.fulfill({ status: 500, json: { error: "boom" } }); + return; + } if (url.pathname.endsWith("/members")) { await route.fulfill({ json: [ @@ -290,6 +295,7 @@ async function mockAuthenticatedApi(page: Page) { test.beforeEach(async ({ page }) => { libraries = [library("library-1", "Ma musique")]; + membersFail = false; tokensFail = false; scanFails = false; scanDrops = false; @@ -846,3 +852,28 @@ test("lists who may see a library, once its panel is opened", async ({ page.getByRole("combobox", { name: "Role: guest" }), ).toHaveValue("listener"); }); + +/** + * A membership list that could not be read is not an empty one. Standing an + * empty array in for "not known yet" made every account on the server look like + * a non-member, so the panel offered access to people who already had it — + * printed underneath the notice saying the list could not be read. + */ +test("offers no membership to grant while the list is unknown", async ({ + page, +}) => { + membersFail = true; + await page.goto("/admin"); + await page + .getByRole("button", { name: "Who may see this library" }) + .first() + .click(); + + const panel = page.locator(".member-row .admin-panel").first(); + await expect(panel.getByRole("alert")).toHaveText( + "We could not load this view", + ); + // The grant control is built from the list, so without one there is nothing + // to build it from. + await expect(panel.getByLabel("Give access to")).toHaveCount(0); +}); diff --git a/webapp/src/pages.tsx b/webapp/src/pages.tsx index dea86b0c..b2b5673a 100644 --- a/webapp/src/pages.tsx +++ b/webapp/src/pages.tsx @@ -1589,10 +1589,19 @@ function LibraryMembersPanel({ } } - const members = value ?? []; - const outside = users.filter( - (user) => !members.some((member) => member.user_id === user.id), - ); + // `value ?? []` conflated three states, and two of them are not an empty + // list. An empty list is not even reachable: every library has at least its + // owner, and the service answers 404 rather than nothing. So `null` means + // "not known yet" — either in flight or failed — and standing in an empty + // array for it made `outside` every account on the server, offering access + // to people who already had it, under the error saying the list could not be + // read. + const members = value; + const outside = members + ? users.filter( + (user) => !members.some((member) => member.user_id === user.id), + ) + : []; return (

    @@ -1611,6 +1620,8 @@ function LibraryMembersPanel({

    {t("common.loadError")}

    + ) : !members ? ( + ) : (
      {members.map((member) => (