From da451cf9273a272a5a4d14114ddba3bf898ebfb6 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sun, 5 Jul 2026 21:23:55 -0400 Subject: [PATCH] fix(jellyfin): read lowercase ids/entryIds playlist params and add playlist-users endpoints --- server/jellyfin/api.go | 2 + server/jellyfin/dto/dto.go | 7 ++++ server/jellyfin/playlists.go | 29 ++++++++++++-- server/jellyfin/playlists_test.go | 66 +++++++++++++++++++++++++++---- 4 files changed, 93 insertions(+), 11 deletions(-) diff --git a/server/jellyfin/api.go b/server/jellyfin/api.go index 094930cc4..9ba33f2be 100644 --- a/server/jellyfin/api.go +++ b/server/jellyfin/api.go @@ -83,6 +83,8 @@ func (api *Router) routes() http.Handler { r.Get("/Playlists/{playlistId}/Items", api.getPlaylistItems) r.Post("/Playlists/{playlistId}/Items", api.addToPlaylist) r.Delete("/Playlists/{playlistId}/Items", api.removeFromPlaylist) + r.Get("/Playlists/{playlistId}/Users", api.getPlaylistUsers) + r.Get("/Playlists/{playlistId}/Users/{userId}", api.getPlaylistUser) r.Get("/Audio/{itemId}/stream", api.streamAudio) r.Get("/Audio/{itemId}/stream.{container}", api.streamAudio) diff --git a/server/jellyfin/dto/dto.go b/server/jellyfin/dto/dto.go index 5118b24f7..67b1710d4 100644 --- a/server/jellyfin/dto/dto.go +++ b/server/jellyfin/dto/dto.go @@ -72,6 +72,13 @@ type BaseItemDto struct { CanDownload bool `json:"CanDownload"` } +// PlaylistUserPermissions is the response shape for GET /Playlists/{id}/Users(/{userId}), which +// Finamp probes before allowing playlist edits. +type PlaylistUserPermissions struct { + UserId string `json:"UserId"` + CanEdit bool `json:"CanEdit"` +} + type QueryResult struct { Items []BaseItemDto `json:"Items"` TotalRecordCount int `json:"TotalRecordCount"` diff --git a/server/jellyfin/playlists.go b/server/jellyfin/playlists.go index 5bf533f7f..3fac0655b 100644 --- a/server/jellyfin/playlists.go +++ b/server/jellyfin/playlists.go @@ -7,6 +7,7 @@ import ( "github.com/go-chi/chi/v5" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server/jellyfin/dto" "github.com/navidrome/navidrome/utils/slice" ) @@ -69,12 +70,21 @@ func splitIds(s string) []string { return strings.Split(s, ",") } +// queryParam reads lower first, falling back to pascal. Real Jellyfin clients (e.g. Finamp) send +// lowercase query params (ids, entryIds); PascalCase is also accepted for robustness. +func queryParam(r *http.Request, lower, pascal string) string { + if v := r.URL.Query().Get(lower); v != "" { + return v + } + return r.URL.Query().Get(pascal) +} + // addToPlaylist appends songs by their own id (core/playlists.AddTracks treats ids as media file // ids). Ownership/editability is enforced by AddTracks itself; any error maps to 404. func (api *Router) addToPlaylist(w http.ResponseWriter, r *http.Request) { ctx := r.Context() id := chi.URLParam(r, "playlistId") - ids := splitIds(r.URL.Query().Get("Ids")) + ids := splitIds(queryParam(r, "ids", "Ids")) if _, err := api.playlists.AddTracks(ctx, id, ids); err != nil { http.Error(w, "Not Found", http.StatusNotFound) return @@ -82,7 +92,7 @@ func (api *Router) addToPlaylist(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNoContent) } -// removeFromPlaylist removes entries by EntryIds. Unlike addToPlaylist's Ids, these must be +// removeFromPlaylist removes entries by entryIds. Unlike addToPlaylist's ids, these must be // playlist-entry ids (model.PlaylistTrack.ID / trackToBaseItem's PlaylistItemId) — the position // of the entry within the playlist — because core/playlists.RemoveTracks deletes playlist_tracks // rows by that id, not by media file id. Ownership/editability is enforced by RemoveTracks itself; @@ -90,10 +100,23 @@ func (api *Router) addToPlaylist(w http.ResponseWriter, r *http.Request) { func (api *Router) removeFromPlaylist(w http.ResponseWriter, r *http.Request) { ctx := r.Context() id := chi.URLParam(r, "playlistId") - ids := splitIds(r.URL.Query().Get("EntryIds")) + ids := splitIds(queryParam(r, "entryIds", "EntryIds")) if err := api.playlists.RemoveTracks(ctx, id, ids); err != nil { http.Error(w, "Not Found", http.StatusNotFound) return } w.WriteHeader(http.StatusNoContent) } + +// getPlaylistUsers and getPlaylistUser are best-effort probes some clients (e.g. Finamp) make +// before allowing playlist edits. Navidrome has no per-playlist ACL model, so every user is +// reported as able to edit; ownership is still enforced by AddTracks/RemoveTracks themselves. +func (api *Router) getPlaylistUsers(w http.ResponseWriter, r *http.Request) { + u, _ := request.UserFrom(r.Context()) + api.ok(w, r, []dto.PlaylistUserPermissions{{UserId: u.ID, CanEdit: true}}) +} + +func (api *Router) getPlaylistUser(w http.ResponseWriter, r *http.Request) { + userId := chi.URLParam(r, "userId") + api.ok(w, r, dto.PlaylistUserPermissions{UserId: userId, CanEdit: true}) +} diff --git a/server/jellyfin/playlists_test.go b/server/jellyfin/playlists_test.go index a020e375b..a6b6064a3 100644 --- a/server/jellyfin/playlists_test.go +++ b/server/jellyfin/playlists_test.go @@ -8,8 +8,10 @@ import ( "net/http/httptest" "strings" + "github.com/go-chi/chi/v5" "github.com/navidrome/navidrome/core/playlists" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" "github.com/navidrome/navidrome/server/jellyfin/dto" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -157,9 +159,9 @@ var _ = Describe("Playlists", func() { }) Describe("addToPlaylist", func() { - It("adds tracks by song id and returns 204", func() { + It("adds tracks by song id from the lowercase ids param real Jellyfin clients send", func() { w := httptest.NewRecorder() - r := httptest.NewRequest("POST", "/Playlists/pl1/Items?Ids=s1,s2", nil).WithContext(context.Background()) + r := httptest.NewRequest("POST", "/Playlists/pl1/Items?ids=s1,s2", nil).WithContext(context.Background()) r = withChiURLParam(r, "playlistId", "pl1") api.addToPlaylist(w, r) Expect(w.Code).To(Equal(http.StatusNoContent)) @@ -167,16 +169,25 @@ var _ = Describe("Playlists", func() { Expect(fp.addIds).To(Equal([]string{"s1", "s2"})) }) + It("falls back to the PascalCase Ids param", func() { + w := httptest.NewRecorder() + r := httptest.NewRequest("POST", "/Playlists/pl1/Items?Ids=s1,s2", nil).WithContext(context.Background()) + r = withChiURLParam(r, "playlistId", "pl1") + api.addToPlaylist(w, r) + Expect(w.Code).To(Equal(http.StatusNoContent)) + Expect(fp.addIds).To(Equal([]string{"s1", "s2"})) + }) + It("returns 404 when the service rejects the request (not found/not owned)", func() { fp.addErr = model.ErrNotAuthorized w := httptest.NewRecorder() - r := httptest.NewRequest("POST", "/Playlists/pl1/Items?Ids=s1", nil).WithContext(context.Background()) + r := httptest.NewRequest("POST", "/Playlists/pl1/Items?ids=s1", nil).WithContext(context.Background()) r = withChiURLParam(r, "playlistId", "pl1") api.addToPlaylist(w, r) Expect(w.Code).To(Equal(http.StatusNotFound)) }) - It("passes no ids (not a spurious empty string) when the Ids param is absent", func() { + It("passes no ids (not a spurious empty string) when the ids param is absent", func() { w := httptest.NewRecorder() r := httptest.NewRequest("POST", "/Playlists/pl1/Items", nil).WithContext(context.Background()) r = withChiURLParam(r, "playlistId", "pl1") @@ -188,9 +199,9 @@ var _ = Describe("Playlists", func() { }) Describe("removeFromPlaylist", func() { - It("removes entries by EntryIds (playlist-track position ids, not song ids) and returns 204", func() { + It("removes entries by the lowercase entryIds param real Jellyfin clients send (playlist-track position ids, not song ids)", func() { w := httptest.NewRecorder() - r := httptest.NewRequest("DELETE", "/Playlists/pl1/Items?EntryIds=1,2", nil).WithContext(context.Background()) + r := httptest.NewRequest("DELETE", "/Playlists/pl1/Items?entryIds=1,2", nil).WithContext(context.Background()) r = withChiURLParam(r, "playlistId", "pl1") api.removeFromPlaylist(w, r) Expect(w.Code).To(Equal(http.StatusNoContent)) @@ -198,16 +209,25 @@ var _ = Describe("Playlists", func() { Expect(fp.removeIds).To(Equal([]string{"1", "2"})) }) + It("falls back to the PascalCase EntryIds param", func() { + w := httptest.NewRecorder() + r := httptest.NewRequest("DELETE", "/Playlists/pl1/Items?EntryIds=1,2", nil).WithContext(context.Background()) + r = withChiURLParam(r, "playlistId", "pl1") + api.removeFromPlaylist(w, r) + Expect(w.Code).To(Equal(http.StatusNoContent)) + Expect(fp.removeIds).To(Equal([]string{"1", "2"})) + }) + It("returns 404 when the service rejects the request (not found/not owned)", func() { fp.removeErr = model.ErrNotFound w := httptest.NewRecorder() - r := httptest.NewRequest("DELETE", "/Playlists/pl1/Items?EntryIds=1", nil).WithContext(context.Background()) + r := httptest.NewRequest("DELETE", "/Playlists/pl1/Items?entryIds=1", nil).WithContext(context.Background()) r = withChiURLParam(r, "playlistId", "pl1") api.removeFromPlaylist(w, r) Expect(w.Code).To(Equal(http.StatusNotFound)) }) - It("passes no ids (not a spurious empty string) when the EntryIds param is absent", func() { + It("passes no ids (not a spurious empty string) when the entryIds param is absent", func() { w := httptest.NewRecorder() r := httptest.NewRequest("DELETE", "/Playlists/pl1/Items", nil).WithContext(context.Background()) r = withChiURLParam(r, "playlistId", "pl1") @@ -217,4 +237,34 @@ var _ = Describe("Playlists", func() { Expect(fp.removeIds).To(BeEmpty()) }) }) + + Describe("getPlaylistUsers", func() { + It("returns the current user with CanEdit true", func() { + w := httptest.NewRecorder() + ctx := request.WithUser(context.Background(), model.User{ID: "u1", UserName: "alice"}) + r := httptest.NewRequest("GET", "/Playlists/pl1/Users", nil).WithContext(ctx) + r = withChiURLParam(r, "playlistId", "pl1") + api.getPlaylistUsers(w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + var res []dto.PlaylistUserPermissions + Expect(json.Unmarshal(w.Body.Bytes(), &res)).To(Succeed()) + Expect(res).To(Equal([]dto.PlaylistUserPermissions{{UserId: "u1", CanEdit: true}})) + }) + }) + + Describe("getPlaylistUser", func() { + It("returns CanEdit true for the requested user", func() { + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/Playlists/pl1/Users/u1", nil).WithContext(context.Background()) + rctx := chi.NewRouteContext() + rctx.URLParams.Add("playlistId", "pl1") + rctx.URLParams.Add("userId", "u1") + r = r.WithContext(context.WithValue(r.Context(), chi.RouteCtxKey, rctx)) + api.getPlaylistUser(w, r) + Expect(w.Code).To(Equal(http.StatusOK)) + var res dto.PlaylistUserPermissions + Expect(json.Unmarshal(w.Body.Bytes(), &res)).To(Succeed()) + Expect(res).To(Equal(dto.PlaylistUserPermissions{UserId: "u1", CanEdit: true})) + }) + }) })