diff --git a/core/playlists/playlists.go b/core/playlists/playlists.go index 656bde05e..9ae8f09cb 100644 --- a/core/playlists/playlists.go +++ b/core/playlists/playlists.go @@ -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 } diff --git a/core/playlists/playlists_test.go b/core/playlists/playlists_test.go index e8134b2ef..dd182b3f1 100644 --- a/core/playlists/playlists_test.go +++ b/core/playlists/playlists_test.go @@ -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)) }) }) diff --git a/model/errors.go b/model/errors.go index 41029d316..0e2543378 100644 --- a/model/errors.go +++ b/model/errors.go @@ -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") ) diff --git a/model/playlist.go b/model/playlist.go index 55b94a640..306401271 100644 --- a/model/playlist.go +++ b/model/playlist.go @@ -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 { diff --git a/model/playlist_test.go b/model/playlist_test.go index d936129ce..d98c85716 100644 --- a/model/playlist_test.go +++ b/model/playlist_test.go @@ -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()) + }) + }) }) diff --git a/server/jellyfin/playlists.go b/server/jellyfin/playlists.go index 805b369c8..ac790a821 100644 --- a/server/jellyfin/playlists.go +++ b/server/jellyfin/playlists.go @@ -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()}) } diff --git a/server/jellyfin/playlists_test.go b/server/jellyfin/playlists_test.go index 3599cc784..270f5fe08 100644 --- a/server/jellyfin/playlists_test.go +++ b/server/jellyfin/playlists_test.go @@ -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()) + }) }) }) diff --git a/server/nativeapi/playlists.go b/server/nativeapi/playlists.go index 90b2f9e94..d215eb9dd 100644 --- a/server/nativeapi/playlists.go +++ b/server/nativeapi/playlists.go @@ -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 } diff --git a/server/nativeapi/playlists_test.go b/server/nativeapi/playlists_test.go index 9bf502687..74ef58cab 100644 --- a/server/nativeapi/playlists_test.go +++ b/server/nativeapi/playlists_test.go @@ -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 diff --git a/server/subsonic/api.go b/server/subsonic/api.go index 82e404228..029046c39 100644 --- a/server/subsonic/api.go +++ b/server/subsonic/api.go @@ -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") diff --git a/server/subsonic/playlists.go b/server/subsonic/playlists.go index bd53528d9..774a9c430 100644 --- a/server/subsonic/playlists.go +++ b/server/subsonic/playlists.go @@ -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 diff --git a/server/subsonic/playlists_test.go b/server/subsonic/playlists_test.go index a7c9e2ec7..f18f33b47 100644 --- a/server/subsonic/playlists_test.go +++ b/server/subsonic/playlists_test.go @@ -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() { diff --git a/ui/src/common/playlistUtils.js b/ui/src/common/playlistUtils.js index 74a01d47a..2d0c745ff 100644 --- a/ui/src/common/playlistUtils.js +++ b/ui/src/common/playlistUtils.js @@ -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 diff --git a/ui/src/common/playlistUtils.test.js b/ui/src/common/playlistUtils.test.js index 2c671ecf5..345b6045d 100644 --- a/ui/src/common/playlistUtils.test.js +++ b/ui/src/common/playlistUtils.test.js @@ -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) + }) }) }) diff --git a/ui/src/dialogs/SelectPlaylistInput.jsx b/ui/src/dialogs/SelectPlaylistInput.jsx index 847107523..2f040a7f7 100644 --- a/ui/src/dialogs/SelectPlaylistInput.jsx +++ b/ui/src/dialogs/SelectPlaylistInput.jsx @@ -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 = diff --git a/ui/src/dialogs/SelectPlaylistInput.test.jsx b/ui/src/dialogs/SelectPlaylistInput.test.jsx index 4ffcdf0b6..753d12c03 100644 --- a/ui/src/dialogs/SelectPlaylistInput.test.jsx +++ b/ui/src/dialogs/SelectPlaylistInput.test.jsx @@ -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 () => {