nik/navidrome
1
0
Fork 0
mirror of https://github.com/navidrome/navidrome.git synced 2026-08-31 07:30:32 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(playlist): block track edits on synced playlists across all APIs (#5984)

Browse Source 
* fix(playlist): block track edits on synced playlists across all APIs

A synced playlist's tracks come from its source file, so any track edit made
through the UI or an API was silently reverted on the next scan. Track mutations
funnel through two service guards, checkTracksEditable (incremental edits) and
Create (wholesale replace, used by Subsonic createPlaylist and Jellyfin's
replace path), which each duplicated the smart-playlist check. Both now consult
a shared model.Playlist.TracksEditable() predicate, so the native, Subsonic, and
Jellyfin paths are all locked: track edits return ErrNotAuthorized (403, or
Subsonic error 50) instead of being accepted and lost. Metadata-only edits
(name, comment, public, the sync flag itself) still go through checkWritable and
are unaffected. In the UI, a synced playlist's track list becomes read-only,
mirroring how smart playlists already behave.

* fix(playlist): return 409 Conflict for non-editable playlist track edits

The previous commit rejected track edits on smart and synced playlists with
ErrNotAuthorized (403). That conflates two different things: a 403 says the
caller lacks permission, but a synced or smart playlist's tracks are immutable
for everyone, including the owner and admins. It is a property of the resource,
not the caller.

Introduce ErrPlaylistNotEditable and return it from both track-edit guards. The
Native and Jellyfin APIs now map it to 409 Conflict; Subsonic maps it to error
50, the closest code it has (it has no read-only concept). The Native track
handlers previously mapped this rejection inconsistently (400 on add, 500 on
remove, 403 on reorder) through a new shared writePlaylistError helper. Genuine
authorization failures (non-owner, non-admin) still return ErrNotAuthorized.

* fix(playlist): surface synced read-only state in picker, Jellyfin, and OpenSubsonic

Follow-up to the track-edit lock: the read-only state was enforced but not
advertised consistently, so clients still offered edits that the server rejects.

- UI: the Add to Playlist picker filtered targets by isWritable only, offering
  synced playlists that then 409 on add. It now filters with canChangeTracks.
- Jellyfin: addToPlaylist/removeFromPlaylist hard-coded every error to 404, so a
  locked playlist reported "not found" instead of 409. They now return 409 for
  ErrPlaylistNotEditable while keeping the deliberate anti-probing 404 for every
  other error (a non-owner never reaches ErrPlaylistNotEditable, so 409 leaks
  nothing).
- OpenSubsonic: buildOSPlaylist marked only smart playlists readonly; owned
  synced playlists advertised readonly=false. Readonly now also covers
  !TracksEditable(), matching the existing smart-playlist treatment.

* fix(jellyfin): report CanEdit from playlist editability in permission probes

getPlaylistUsers and getPlaylistUser returned CanEdit: true unconditionally, so
Finamp (which probes this before showing edit controls) offered track editing on
synced/smart playlists whose add/remove requests now return 409. Both handlers
now fetch the playlist and set CanEdit from TracksEditable(), keeping the
deliberate non-owner looseness (CanEdit stays true for a normal playlist a
non-owner views) and mapping any lookup error to 404 like the sibling probes.

* fix(playlist): check ownership before editability when replacing tracks

Create checked TracksEditable() before ownership, so a non-owner replacing
another user's public smart/synced playlist (Jellyfin updatePlaylist with a
non-empty Ids list) received a 409 read-only conflict instead of a 403
authorization failure. The incremental guards check ownership first via
checkWritable; Create now matches that order. Subsonic is unaffected (both errors
map to code 50). Owners of their own smart/synced playlists still get the
read-only conflict.

* fix(jellyfin): return 403 for locked playlists, matching Jellyfin

Jellyfin itself refuses edits on its file-backed playlists with Forbid() (403):
PlaylistsController gates every mutation on OwnerUserId == caller or a share with
CanEdit, and playlists imported from .m3u files satisfy neither. Its CanEdit is
an ACL field, not a read-only marker, and Jellyfin core has no server-managed
playlist type at all.

Our Jellyfin routes exist to imitate that API, so ErrPlaylistNotEditable now maps
to 403 there instead of 409. The native API keeps 409 (a resource-state conflict
is the accurate REST answer where we define the contract) and Subsonic keeps
error 50, its closest code.

* chore(playlist): trim comments added by this branch

Several comments ran to three or four lines and carried rationale that belongs in
the commit history rather than the code: what Jellyfin does with its own
file-backed playlists, and restatements of the expressions directly below them.
Each block is now one or two lines covering only the non-obvious why.
This commit is contained in:
Deluan Quintão 2026-08-19 08:47:53 -04:00 committed by GitHub
parent 7a11ca69bb
commit 1f3034f022
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
16 changed files with 232 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
core/playlists/playlists.go
View File

@ -130,12 +130,13 @@ func (s *playlists) Create(ctx context.Context, playlistId string, name string,
if err != nil {
return err
}
if pls.IsSmartPlaylist() {
return model.ErrNotAuthorized
}
// Ownership first: a non-owner must get ErrNotAuthorized, not a read-only conflict.
if !usr.IsAdmin && pls.OwnerID != usr.ID {
return model.ErrNotAuthorized
}
if !pls.TracksEditable() {
return model.ErrPlaylistNotEditable
}
} else {
pls = &model.Playlist{Name: name}
pls.OwnerID = usr.ID
@ -230,14 +231,14 @@ func (s *playlists) checkWritable(ctx context.Context, id string) (*model.Playli
return pls, nil
}
// checkTracksEditable verifies the user can modify tracks (ownership + not smart playlist).
// checkTracksEditable verifies the user owns the playlist and its tracks are editable.
func (s *playlists) checkTracksEditable(ctx context.Context, playlistID string) (*model.Playlist, error) {
pls, err := s.checkWritable(ctx, playlistID)
if err != nil {
return nil, err
}
if pls.IsSmartPlaylist() {
return nil, model.ErrNotAuthorized
if !pls.TracksEditable() {
return nil, model.ErrPlaylistNotEditable
}
return pls, nil
}

46
core/playlists/playlists_test.go
View File

@ -102,6 +102,8 @@ var _ = Describe("Playlists", func() {
"pls-2": {ID: "pls-2", Name: "Other's", OwnerID: "other-user"},
"pls-smart": {ID: "pls-smart", Name: "Smart", OwnerID: "user-1",
Rules: &criteria.Criteria{Expression: criteria.Contains{"title": "test"}}},
"pls-synced": {ID: "pls-synced", Name: "Synced", OwnerID: "user-1", Sync: true},
"pls-synced-other": {ID: "pls-synced-other", Name: "Other's Synced", OwnerID: "other-user", Sync: true, Public: true},
}
ps = playlists.NewPlaylists(ds, artwork.NewUploader(ds))
})
@ -145,6 +147,18 @@ var _ = Describe("Playlists", func() {
It("denies replacing tracks on a smart playlist", func() {
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
_, err := ps.Create(ctx, "pls-smart", "", []string{"song-1"})
Expect(err).To(MatchError(model.ErrPlaylistNotEditable))
})
It("denies replacing tracks on a synced playlist", func() {
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
_, err := ps.Create(ctx, "pls-synced", "", []string{"song-1"})
Expect(err).To(MatchError(model.ErrPlaylistNotEditable))
})
It("denies a non-owner with authorization, not a conflict, on a public synced playlist", func() {
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
_, err := ps.Create(ctx, "pls-synced-other", "", []string{"song-1"})
Expect(err).To(MatchError(model.ErrNotAuthorized))
})
})
@ -159,6 +173,7 @@ var _ = Describe("Playlists", func() {
"pls-other": {ID: "pls-other", Name: "Other's", OwnerID: "other-user"},
"pls-smart": {ID: "pls-smart", Name: "Smart", OwnerID: "user-1",
Rules: &criteria.Criteria{Expression: criteria.Contains{"title": "test"}}},
"pls-synced": {ID: "pls-synced", Name: "Synced", OwnerID: "user-1", Sync: true},
}
mockPlsRepo.TracksRepo = mockTracks
ps = playlists.NewPlaylists(ds, artwork.NewUploader(ds))
@ -191,13 +206,13 @@ var _ = Describe("Playlists", func() {
It("denies adding tracks to a smart playlist", func() {
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
err := ps.Update(ctx, "pls-smart", nil, nil, nil, []string{"song-1"}, nil)
Expect(err).To(MatchError(model.ErrNotAuthorized))
Expect(err).To(MatchError(model.ErrPlaylistNotEditable))
})
It("denies removing tracks from a smart playlist", func() {
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
err := ps.Update(ctx, "pls-smart", nil, nil, nil, nil, []int{0})
Expect(err).To(MatchError(model.ErrNotAuthorized))
Expect(err).To(MatchError(model.ErrPlaylistNotEditable))
})
It("allows metadata updates on a smart playlist", func() {
@ -205,6 +220,18 @@ var _ = Describe("Playlists", func() {
err := ps.Update(ctx, "pls-smart", new("Updated Smart"), nil, nil, nil, nil)
Expect(err).ToNot(HaveOccurred())
})
It("denies adding tracks to a synced playlist", func() {
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
err := ps.Update(ctx, "pls-synced", nil, nil, nil, []string{"song-1"}, nil)
Expect(err).To(MatchError(model.ErrPlaylistNotEditable))
})
It("allows metadata updates on a synced playlist", func() {
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
err := ps.Update(ctx, "pls-synced", new("Renamed Synced"), nil, nil, nil, nil)
Expect(err).ToNot(HaveOccurred())
})
})
Describe("AddTracks", func() {
@ -216,7 +243,8 @@ var _ = Describe("Playlists", func() {
"pls-1": {ID: "pls-1", Name: "My Playlist", OwnerID: "user-1"},
"pls-smart": {ID: "pls-smart", Name: "Smart", OwnerID: "user-1",
Rules: &criteria.Criteria{Expression: criteria.Contains{"title": "test"}}},
"pls-other": {ID: "pls-other", Name: "Other's", OwnerID: "other-user"},
"pls-other": {ID: "pls-other", Name: "Other's", OwnerID: "other-user"},
"pls-synced": {ID: "pls-synced", Name: "Synced", OwnerID: "user-1", Sync: true},
}
mockPlsRepo.TracksRepo = mockTracks
ps = playlists.NewPlaylists(ds, artwork.NewUploader(ds))
@ -246,7 +274,13 @@ var _ = Describe("Playlists", func() {
It("denies editing smart playlists", func() {
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
_, err := ps.AddTracks(ctx, "pls-smart", []string{"song-1"})
Expect(err).To(MatchError(model.ErrNotAuthorized))
Expect(err).To(MatchError(model.ErrPlaylistNotEditable))
})
It("denies editing synced playlists", func() {
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
_, err := ps.AddTracks(ctx, "pls-synced", []string{"song-1"})
Expect(err).To(MatchError(model.ErrPlaylistNotEditable))
})
It("returns error when playlist not found", func() {
@ -280,7 +314,7 @@ var _ = Describe("Playlists", func() {
It("denies on smart playlist", func() {
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
err := ps.RemoveTracks(ctx, "pls-smart", []string{"track-1"})
Expect(err).To(MatchError(model.ErrNotAuthorized))
Expect(err).To(MatchError(model.ErrPlaylistNotEditable))
})
It("denies non-owner", func() {
@ -314,7 +348,7 @@ var _ = Describe("Playlists", func() {
It("denies on smart playlist", func() {
ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false})
err := ps.ReorderTrack(ctx, "pls-smart", 1, 3)
Expect(err).To(MatchError(model.ErrNotAuthorized))
Expect(err).To(MatchError(model.ErrPlaylistNotEditable))
})
})

3
model/errors.go
View File

@ -9,4 +9,7 @@ var (
ErrExpired = errors.New("access expired")
ErrNotAvailable = errors.New("functionality not available")
ErrValidation = errors.New("validation error")
// ErrPlaylistNotEditable: tracks are server-managed, so nobody can edit them (not an ACL failure).
ErrPlaylistNotEditable = errors.New("playlist tracks are not editable")
)

5
model/playlist.go
View File

@ -42,6 +42,11 @@ func (pls Playlist) IsSmartPlaylist() bool {
return pls.Rules != nil && pls.Rules.Expression != nil
}
// TracksEditable reports whether the track list is user-owned rather than server-managed.
func (pls Playlist) TracksEditable() bool {
return !pls.IsSmartPlaylist() && !pls.Sync
}
// RefreshDelay returns the playlist's own refresh window when set, falling
// back to the global SmartPlaylistRefreshDelay.
func (pls Playlist) RefreshDelay() time.Duration {

15
model/playlist_test.go
View File

@ -73,4 +73,19 @@ var _ = Describe("Playlist", func() {
Expect(pls.RefreshDelay()).To(Equal(5 * time.Second))
})
})
Describe("TracksEditable", func() {
It("is true for a plain playlist", func() {
Expect(model.Playlist{}.TracksEditable()).To(BeTrue())
})
It("is false for a smart playlist", func() {
pls := model.Playlist{Rules: &criteria.Criteria{Expression: criteria.Is{"loved": true}}}
Expect(pls.TracksEditable()).To(BeFalse())
})
It("is false for a synced playlist", func() {
Expect(model.Playlist{Sync: true}.TracksEditable()).To(BeFalse())
})
})
})

49
server/jellyfin/playlists.go
View File

@ -29,11 +29,11 @@ func playlistsFolder() dto.BaseItemDto {
}
}
// playlistError maps core/playlists write errors to HTTP status: ownership -> 403, missing/invisible
// -> 404 (never revealing another user's private playlist), else -> 500.
// playlistError maps core/playlists write errors to HTTP status: ownership or locked -> 403,
// missing/invisible -> 404 (never revealing another user's private playlist), else -> 500.
func (api *Router) playlistError(w http.ResponseWriter, r *http.Request, err error) {
switch {
case errors.Is(err, model.ErrNotAuthorized):
case errors.Is(err, model.ErrNotAuthorized), errors.Is(err, model.ErrPlaylistNotEditable):
http.Error(w, "Forbidden", http.StatusForbidden)
case errors.Is(err, model.ErrNotFound):
http.Error(w, "Not Found", http.StatusNotFound)
@ -262,7 +262,7 @@ func (api *Router) songIDs(ctx context.Context, opts model.QueryOptions) []strin
}
// addToPlaylist appends items by id, expanding containers into tracks (see expandContainerIDs).
// AddTracks enforces ownership; any error maps to 404.
// AddTracks enforces ownership; a locked playlist maps to 403, any other error to 404.
func (api *Router) addToPlaylist(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
id, ok := itemIDParam(w, r, "playlistId")
@ -276,6 +276,10 @@ func (api *Router) addToPlaylist(w http.ResponseWriter, r *http.Request) {
}
ids := api.expandContainerIDs(ctx, decoded)
if _, err := api.playlists.AddTracks(ctx, id, ids); err != nil {
if errors.Is(err, model.ErrPlaylistNotEditable) {
http.Error(w, "Forbidden", http.StatusForbidden)
return
}
http.Error(w, "Not Found", http.StatusNotFound)
return
}
@ -284,7 +288,7 @@ func (api *Router) addToPlaylist(w http.ResponseWriter, r *http.Request) {
// removeFromPlaylist removes entries by entryIds — playlist-entry ids (PlaylistItemId), not media
// file ids, since RemoveTracks deletes playlist_tracks rows by that id. RemoveTracks enforces
// ownership; any error maps to 404.
// ownership; a locked playlist maps to 403, any other error to 404.
func (api *Router) removeFromPlaylist(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
id, ok := itemIDParam(w, r, "playlistId")
@ -302,21 +306,44 @@ func (api *Router) removeFromPlaylist(w http.ResponseWriter, r *http.Request) {
ids = append(ids, entry)
}
if err := api.playlists.RemoveTracks(ctx, id, ids); err != nil {
if errors.Is(err, model.ErrPlaylistNotEditable) {
http.Error(w, "Forbidden", http.StatusForbidden)
return
}
http.Error(w, "Not Found", http.StatusNotFound)
return
}
w.WriteHeader(http.StatusNoContent)
}
// getPlaylistUsers and getPlaylistUser answer client probes (e.g. Finamp) made before allowing
// edits. Navidrome has no per-playlist ACL, so every user is reported CanEdit; ownership is still
// enforced by AddTracks/RemoveTracks.
// Clients probe these before offering edits. Navidrome has no per-playlist ACL, so CanEdit carries
// only editability; ownership is enforced on write, and a lookup error 404s to prevent probing.
func (api *Router) getPlaylistUsers(w http.ResponseWriter, r *http.Request) {
u, _ := request.UserFrom(r.Context())
api.ok(w, r, []dto.PlaylistUserPermissions{{UserId: dto.EncodeID(u.ID), CanEdit: true}})
ctx := r.Context()
id, ok := itemIDParam(w, r, "playlistId")
if !ok {
return
}
pls, err := api.playlists.Get(ctx, id)
if err != nil {
http.Error(w, "Not Found", http.StatusNotFound)
return
}
u, _ := request.UserFrom(ctx)
api.ok(w, r, []dto.PlaylistUserPermissions{{UserId: dto.EncodeID(u.ID), CanEdit: pls.TracksEditable()}})
}
func (api *Router) getPlaylistUser(w http.ResponseWriter, r *http.Request) {
ctx := r.Context()
id, ok := itemIDParam(w, r, "playlistId")
if !ok {
return
}
pls, err := api.playlists.Get(ctx, id)
if err != nil {
http.Error(w, "Not Found", http.StatusNotFound)
return
}
userId := chi.URLParam(r, "userId")
api.ok(w, r, dto.PlaylistUserPermissions{UserId: userId, CanEdit: true})
api.ok(w, r, dto.PlaylistUserPermissions{UserId: userId, CanEdit: pls.TracksEditable()})
}

63
server/jellyfin/playlists_test.go
View File

@ -381,6 +381,15 @@ var _ = Describe("Playlists", func() {
Expect(w.Code).To(Equal(http.StatusNotFound))
})
It("returns 403 when the playlist is not editable (synced/smart), like Jellyfin", func() {
fp.addErr = model.ErrPlaylistNotEditable
w := httptest.NewRecorder()
r := httptest.NewRequest("POST", "/Playlists/"+dto.EncodeID(testID("pl1"))+"/Items?ids="+dto.EncodeID(testID("s1")), nil).WithContext(context.Background())
r = withChiURLParam(r, "playlistId", dto.EncodeID(testID("pl1")))
invoke(api.addToPlaylist, w, r)
Expect(w.Code).To(Equal(http.StatusForbidden))
})
It("passes no ids (not a spurious empty string) when the ids param is absent", func() {
w := httptest.NewRecorder()
r := httptest.NewRequest("POST", "/Playlists/"+dto.EncodeID(testID("pl1"))+"/Items", nil).WithContext(context.Background())
@ -430,6 +439,15 @@ var _ = Describe("Playlists", func() {
Expect(w.Code).To(Equal(http.StatusNotFound))
})
It("returns 403 when the playlist is not editable (synced/smart), like Jellyfin", func() {
fp.removeErr = model.ErrPlaylistNotEditable
w := httptest.NewRecorder()
r := httptest.NewRequest("DELETE", "/Playlists/"+dto.EncodeID(testID("pl1"))+"/Items?entryIds="+dto.EncodePlaylistEntryID("1"), nil).WithContext(context.Background())
r = withChiURLParam(r, "playlistId", dto.EncodeID(testID("pl1")))
invoke(api.removeFromPlaylist, w, r)
Expect(w.Code).To(Equal(http.StatusForbidden))
})
It("passes no ids (not a spurious empty string) when the entryIds param is absent", func() {
w := httptest.NewRecorder()
r := httptest.NewRequest("DELETE", "/Playlists/"+dto.EncodeID(testID("pl1"))+"/Items", nil).WithContext(context.Background())
@ -442,7 +460,8 @@ var _ = Describe("Playlists", func() {
})
Describe("getPlaylistUsers", func() {
It("returns the current user with CanEdit true", func() {
It("returns the current user with CanEdit true for an editable playlist", func() {
fp.getByIDPls = &model.Playlist{ID: testID("pl1")}
w := httptest.NewRecorder()
ctx := request.WithUser(context.Background(), model.User{ID: testID("u1"), UserName: "alice"})
r := httptest.NewRequest("GET", "/Playlists/"+testID("pl1")+"/Users", nil).WithContext(ctx)
@ -453,21 +472,59 @@ var _ = Describe("Playlists", func() {
Expect(json.Unmarshal(w.Body.Bytes(), &res)).To(Succeed())
Expect(res).To(Equal([]dto.PlaylistUserPermissions{{UserId: dto.EncodeID(testID("u1")), CanEdit: true}}))
})
It("reports CanEdit false for a synced playlist", func() {
fp.getByIDPls = &model.Playlist{ID: testID("pl1"), Sync: true}
w := httptest.NewRecorder()
ctx := request.WithUser(context.Background(), model.User{ID: testID("u1"), UserName: "alice"})
r := httptest.NewRequest("GET", "/Playlists/"+testID("pl1")+"/Users", nil).WithContext(ctx)
r = withChiURLParam(r, "playlistId", dto.EncodeID(testID("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[0].CanEdit).To(BeFalse())
})
It("returns 404 when the playlist is not visible", func() {
fp.getByIDErr = model.ErrNotFound
w := httptest.NewRecorder()
ctx := request.WithUser(context.Background(), model.User{ID: testID("u1"), UserName: "alice"})
r := httptest.NewRequest("GET", "/Playlists/"+testID("pl1")+"/Users", nil).WithContext(ctx)
r = withChiURLParam(r, "playlistId", dto.EncodeID(testID("pl1")))
api.getPlaylistUsers(w, r)
Expect(w.Code).To(Equal(http.StatusNotFound))
})
})
Describe("getPlaylistUser", func() {
It("returns CanEdit true for the requested user", func() {
requestUser := func() *httptest.ResponseRecorder {
w := httptest.NewRecorder()
r := httptest.NewRequest("GET", "/Playlists/"+testID("pl1")+"/Users/"+testID("u1"), nil).WithContext(context.Background())
rctx := chi.NewRouteContext()
rctx.URLParams.Add("playlistId", testID("pl1"))
rctx.URLParams.Add("playlistId", dto.EncodeID(testID("pl1")))
rctx.URLParams.Add("userId", testID("u1"))
r = r.WithContext(context.WithValue(r.Context(), chi.RouteCtxKey, rctx))
api.getPlaylistUser(w, r)
return w
}
It("returns CanEdit true for an editable playlist", func() {
fp.getByIDPls = &model.Playlist{ID: testID("pl1")}
w := requestUser()
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: testID("u1"), CanEdit: true}))
})
It("reports CanEdit false for a synced playlist", func() {
fp.getByIDPls = &model.Playlist{ID: testID("pl1"), Sync: true}
w := requestUser()
Expect(w.Code).To(Equal(http.StatusOK))
var res dto.PlaylistUserPermissions
Expect(json.Unmarshal(w.Body.Bytes(), &res)).To(Succeed())
Expect(res.CanEdit).To(BeFalse())
})
})
})

30
server/nativeapi/playlists.go
View File

@ -20,6 +20,20 @@ import (
type restHandler = func(rest.RepositoryConstructor, ...rest.Logger) http.HandlerFunc
// writePlaylistError maps a playlist service error to an HTTP status, or defaultStatus if unknown.
func writePlaylistError(w http.ResponseWriter, err error, defaultStatus int) {
switch {
case errors.Is(err, model.ErrNotFound):
http.Error(w, err.Error(), http.StatusNotFound)
case errors.Is(err, model.ErrNotAuthorized):
http.Error(w, err.Error(), http.StatusForbidden)
case errors.Is(err, model.ErrPlaylistNotEditable):
http.Error(w, err.Error(), http.StatusConflict)
default:
http.Error(w, err.Error(), defaultStatus)
}
}
func playlistTracksHandler(pls playlists.Playlists, handler restHandler, refreshSmartPlaylist func(*http.Request) bool) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
plsId := chi.URLParam(r, "playlistId")
@ -111,7 +125,7 @@ func deleteFromPlaylist(pls playlists.Playlists) http.HandlerFunc {
}
if err != nil {
log.Error(r.Context(), "Error deleting tracks from playlist", "playlistId", playlistId, "ids", ids, err)
http.Error(w, err.Error(), http.StatusInternalServerError)
writePlaylistError(w, err, http.StatusInternalServerError)
return
}
writeDeleteManyResponse(w, r, ids)
@ -138,22 +152,22 @@ func addToPlaylist(pls playlists.Playlists) http.HandlerFunc {
}
count, c := 0, 0
if c, err = pls.AddTracks(ctx, playlistId, payload.Ids); err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
writePlaylistError(w, err, http.StatusBadRequest)
return
}
count += c
if c, err = pls.AddAlbums(ctx, playlistId, payload.AlbumIds); err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
writePlaylistError(w, err, http.StatusBadRequest)
return
}
count += c
if c, err = pls.AddArtists(ctx, playlistId, payload.ArtistIds); err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
writePlaylistError(w, err, http.StatusBadRequest)
return
}
count += c
if c, err = pls.AddDiscs(ctx, playlistId, payload.Discs); err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
writePlaylistError(w, err, http.StatusBadRequest)
return
}
count += c
@ -192,12 +206,8 @@ func reorderItem(pls playlists.Playlists) http.HandlerFunc {
return
}
err = pls.ReorderTrack(ctx, playlistId, id, newPos)
if errors.Is(err, model.ErrNotAuthorized) {
http.Error(w, err.Error(), http.StatusForbidden)
return
}
if err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
writePlaylistError(w, err, http.StatusBadRequest)
return
}

14
server/nativeapi/playlists_test.go
View File

@ -183,6 +183,20 @@ var _ = Describe("Playlist Tracks Endpoint", func() {
})
})
var _ = Describe("writePlaylistError", func() {
DescribeTable("maps a service error to an HTTP status",
func(err error, expected int) {
w := httptest.NewRecorder()
writePlaylistError(w, err, http.StatusBadRequest)
Expect(w.Code).To(Equal(expected))
},
Entry("not found -> 404", model.ErrNotFound, http.StatusNotFound),
Entry("not authorized -> 403", model.ErrNotAuthorized, http.StatusForbidden),
Entry("not editable -> 409", model.ErrPlaylistNotEditable, http.StatusConflict),
Entry("unrecognized -> default", model.ErrValidation, http.StatusBadRequest),
)
})
type mockPlaylistTrackRepo struct {
model.PlaylistTrackRepository
tracks model.PlaylistTracks

3
server/subsonic/api.go
View File

@ -304,7 +304,8 @@ func mapToSubsonicError(err error) subError {
err = newError(responses.ErrorGeneric, err.Error())
case errors.Is(err, model.ErrNotFound), errors.Is(err, rest.ErrNotFound):
err = newError(responses.ErrorDataNotFound, "data not found")
case errors.Is(err, model.ErrNotAuthorized), errors.Is(err, rest.ErrPermissionDenied):
case errors.Is(err, model.ErrNotAuthorized), errors.Is(err, rest.ErrPermissionDenied),
errors.Is(err, model.ErrPlaylistNotEditable): // Subsonic has no code for "read-only resource"
err = newError(responses.ErrorAuthorizationFail)
case errors.Is(err, stream.ErrTooManyTranscodes):
err = newError(responses.ErrorGeneric, "too many concurrent transcodes, please retry shortly")

2
server/subsonic/playlists.go
View File

@ -172,7 +172,7 @@ func buildOSPlaylist(ctx context.Context, p model.Playlist) *responses.OpenSubso
}
} else {
user, ok := request.UserFrom(ctx)
pls.Readonly = !ok || p.OwnerID != user.ID
pls.Readonly = !ok || p.OwnerID != user.ID || !p.TracksEditable()
}
return &pls

9
server/subsonic/playlists_test.go
View File

@ -111,6 +111,15 @@ var _ = Describe("buildPlaylist", func() {
Expect(result.Public).To(BeTrue())
Expect(result.Readonly).To(BeFalse())
})
It("is read-only for a synced playlist even as owner", func() {
ctx = request.WithUser(ctx, model.User{ID: "1234", UserName: "admin"})
playlist.Sync = true
result := router.buildPlaylist(ctx, playlist)
Expect(result.Readonly).To(BeTrue())
})
})
Context("when minimal clients list is empty", func() {

2
ui/src/common/playlistUtils.js
View File

@ -12,4 +12,4 @@ export const isReadOnly = (ownerId) => {
export const isSmartPlaylist = (pls) => !!pls.rules
export const canChangeTracks = (pls) =>
isWritable(pls.ownerId) && !isSmartPlaylist(pls)
isWritable(pls.ownerId) && !isSmartPlaylist(pls) && !pls.sync

6
ui/src/common/playlistUtils.test.js
View File

@ -74,5 +74,11 @@ describe('playlistUtils', () => {
const playlist = { ownerId: 'user1', rules: [] }
expect(canChangeTracks(playlist)).toBe(false)
})
it('returns false if playlist is synced', () => {
localStorage.setItem('userId', 'user1')
const playlist = { ownerId: 'user1', sync: true }
expect(canChangeTracks(playlist)).toBe(false)
})
})
})

5
ui/src/dialogs/SelectPlaylistInput.jsx
View File

@ -16,7 +16,7 @@ import {
import AddIcon from '@material-ui/icons/Add'
import { useGetList, useTranslate } from 'react-admin'
import PropTypes from 'prop-types'
import { isWritable } from '../common'
import { canChangeTracks } from '../common'
import { makeStyles } from '@material-ui/core'
const useStyles = makeStyles((theme) => ({
@ -268,8 +268,7 @@ export const SelectPlaylistInput = ({ onChange }) => {
)
const options =
ids &&
ids.map((id) => data[id]).filter((option) => isWritable(option.ownerId))
ids && ids.map((id) => data[id]).filter((option) => canChangeTracks(option))
// Filter playlists based on search text
const filteredOptions =

9
ui/src/dialogs/SelectPlaylistInput.test.jsx
View File

@ -16,6 +16,7 @@ const mockPlaylists = [
{ id: 'playlist-2', name: 'Jazz Collection', ownerId: 'admin' },
{ id: 'playlist-3', name: 'Electronic Beats', ownerId: 'admin' },
{ id: 'playlist-4', name: 'Chill Vibes', ownerId: 'user2' }, // Not writable by admin
{ id: 'playlist-5', name: 'Synced List', ownerId: 'admin', sync: true },
]
const mockIndexedData = {
@ -27,6 +28,12 @@ const mockIndexedData = {
ownerId: 'admin',
},
'playlist-4': { id: 'playlist-4', name: 'Chill Vibes', ownerId: 'user2' },
'playlist-5': {
id: 'playlist-5',
name: 'Synced List',
ownerId: 'admin',
sync: true,
},
}
const createTestComponent = (
@ -89,6 +96,8 @@ describe('SelectPlaylistInput', () => {
// Should not show playlists not owned by admin (not writable)
expect(screen.queryByText('Chill Vibes')).not.toBeInTheDocument()
// Should not show synced playlists (their tracks are not editable)
expect(screen.queryByText('Synced List')).not.toBeInTheDocument()
})
it('should filter playlists based on search input', async () => {