diff --git a/docs/web-client-gap-analysis.md b/docs/web-client-gap-analysis.md index 210bc1b..aff9a69 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 744b8eb..666ef6b 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 771b7e7..30dba18 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 ec12864..3b046d3 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 9ac1739..3f62e64 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 b1459d3..efb6600 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 367df3a..50047ed 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 f4ec1eb..35804b7 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 f8d1492..4ccf41a 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 @@ -1338,6 +1432,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 +1543,11 @@ 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); // And a manager may, which is the half of the rule the refusal above cannot diff --git a/webapp/e2e/studio-nocturne.spec.ts b/webapp/e2e/studio-nocturne.spec.ts index dd49161..f424e80 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,29 @@ 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: [ + { + 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" } }); @@ -271,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; @@ -772,3 +797,83 @@ 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); + }); + + // 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" }) + .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"]); + + // 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"); +}); + +/** + * 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/api.ts b/webapp/src/api.ts index 5cad691..92c5c86 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 0b7f663..da0b8de 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 d854921..b2b5673 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, @@ -34,6 +35,7 @@ import { getLyrics, getTrack, isAllowedRedirect, + type LibraryMember, type LyricsLine, type LyricsList, listAlbums, @@ -45,6 +47,7 @@ import { listGenres, listHistory, listLibraries, + listLibraryMembers, listNowPlaying, listPlaylists, listRandomSongs, @@ -53,6 +56,7 @@ import { login, type NowPlaying, type Playlist, + removeLibraryMember, revokeApiToken, type ScanJob, type SearchResult, @@ -62,6 +66,7 @@ import { search, setBookmark, setFavorite, + setLibraryMember, setRating, setSubsonicCredential, setUserDisabled, @@ -1538,6 +1543,153 @@ 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); + } + } + + // `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 ( +
+

+ +

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

{t("admin.memberError")}

: null} + {error ? ( +

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

+ ) : !members ? ( + + ) : ( +
    + {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(); @@ -1654,19 +1806,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} -
    - -
    • + +
  • +
    + {library.name} + {library.visibility} +
    + +
    • +
  • + +
    • +     ))}
diff --git a/webapp/src/styles.css b/webapp/src/styles.css index 3695621..f8d70af 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; +}