From a5fd18dc67517046d8e7c102f6c11156391b7829 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 18:45:45 -0500 Subject: [PATCH] feat: treat plugin playlists as read-only everywhere Enforce read-only restrictions for plugin playlists (PluginID != "") consistently across backend and UI, matching the existing smart playlist behavior. The Subsonic buildOSPlaylist now uses IsReadOnly() to mark both smart and plugin playlists as readonly. Backend Update and REST adapter block name/comment changes on plugin playlists while still allowing public toggle and cover art. The UI adds isPluginPlaylist helper to canChangeTracks, disables name/comment inputs in PlaylistEdit, disables auto-import toggle in PlaylistList, and adds a server-side readonly filter to exclude both smart and plugin playlists from the "add to playlist" dialog. --- core/playlists/playlists.go | 4 +++ core/playlists/playlists_test.go | 21 ++++++++++++ core/playlists/rest_adapter.go | 4 +++ core/playlists/rest_adapter_test.go | 36 +++++++++++++++++++++ model/playlist.go | 4 +++ persistence/playlist_repository.go | 12 +++++-- server/subsonic/playlists.go | 5 +-- server/subsonic/playlists_test.go | 35 ++++++++++++++++++++ ui/src/common/playlistUtils.js | 4 ++- ui/src/common/playlistUtils.test.js | 19 +++++++++++ ui/src/dialogs/AddToPlaylistDialog.test.jsx | 2 +- ui/src/dialogs/SelectPlaylistInput.jsx | 2 +- ui/src/dialogs/SelectPlaylistInput.test.jsx | 2 +- ui/src/playlist/PlaylistEdit.jsx | 9 ++++-- ui/src/playlist/PlaylistList.jsx | 3 +- 15 files changed, 151 insertions(+), 11 deletions(-) diff --git a/core/playlists/playlists.go b/core/playlists/playlists.go index ca1eb60a7..4ba6e76fd 100644 --- a/core/playlists/playlists.go +++ b/core/playlists/playlists.go @@ -162,6 +162,10 @@ func (s *playlists) Update(ctx context.Context, playlistID string, if err != nil { return err } + // Plugin playlists allow public toggle and cover art, but block name/comment changes + if pls.IsPluginPlaylist() && (name != nil || comment != nil) { + return model.ErrNotAuthorized + } return s.ds.WithTxImmediate(func(tx model.DataStore) error { repo := tx.Playlist(ctx) diff --git a/core/playlists/playlists_test.go b/core/playlists/playlists_test.go index 12ff44010..86cac3c11 100644 --- a/core/playlists/playlists_test.go +++ b/core/playlists/playlists_test.go @@ -210,6 +210,27 @@ var _ = Describe("Playlists", func() { err := ps.Update(ctx, "pls-smart", &newName, nil, nil, nil, nil) Expect(err).ToNot(HaveOccurred()) }) + + It("denies name update on a plugin playlist", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + newName := "New Name" + err := ps.Update(ctx, "pls-plugin", &newName, nil, nil, nil, nil) + Expect(err).To(MatchError(model.ErrNotAuthorized)) + }) + + It("denies comment update on a plugin playlist", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + newComment := "New Comment" + err := ps.Update(ctx, "pls-plugin", nil, &newComment, nil, nil, nil) + Expect(err).To(MatchError(model.ErrNotAuthorized)) + }) + + It("allows public toggle on a plugin playlist", func() { + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + public := true + err := ps.Update(ctx, "pls-plugin", nil, nil, &public, nil, nil) + Expect(err).ToNot(HaveOccurred()) + }) }) Describe("AddTracks", func() { diff --git a/core/playlists/rest_adapter.go b/core/playlists/rest_adapter.go index 3fecda0d5..33073e7da 100644 --- a/core/playlists/rest_adapter.go +++ b/core/playlists/rest_adapter.go @@ -93,6 +93,10 @@ func (s *playlists) updatePlaylistEntity(ctx context.Context, id string, entity if !usr.IsAdmin && entity.OwnerID != "" && entity.OwnerID != current.OwnerID { return rest.ErrPermissionDenied } + // Plugin playlists allow public and ownership changes, but block name/comment + if current.IsPluginPlaylist() && (entity.Name != current.Name || entity.Comment != current.Comment) { + return rest.ErrPermissionDenied + } // Apply ownership change (admin only) if entity.OwnerID != "" { current.OwnerID = entity.OwnerID diff --git a/core/playlists/rest_adapter_test.go b/core/playlists/rest_adapter_test.go index 097bc6310..05f4b3be2 100644 --- a/core/playlists/rest_adapter_test.go +++ b/core/playlists/rest_adapter_test.go @@ -102,6 +102,42 @@ var _ = Describe("REST Adapter", func() { Expect(err).ToNot(HaveOccurred()) }) + It("denies name change on plugin playlist", func() { + mockPlsRepo.Data["pls-plugin"] = &model.Playlist{ + ID: "pls-plugin", Name: "Plugin PL", OwnerID: "user-1", + PluginID: "test-plugin", PluginPlaylistID: "daily-mix", + } + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + repo = ps.NewRepository(ctx).(rest.Persistable) + pls := &model.Playlist{Name: "Changed Name", Comment: ""} + err := repo.Update("pls-plugin", pls) + Expect(err).To(Equal(rest.ErrPermissionDenied)) + }) + + It("denies comment change on plugin playlist", func() { + mockPlsRepo.Data["pls-plugin"] = &model.Playlist{ + ID: "pls-plugin", Name: "Plugin PL", Comment: "", OwnerID: "user-1", + PluginID: "test-plugin", PluginPlaylistID: "daily-mix", + } + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + repo = ps.NewRepository(ctx).(rest.Persistable) + pls := &model.Playlist{Name: "Plugin PL", Comment: "new comment"} + err := repo.Update("pls-plugin", pls) + Expect(err).To(Equal(rest.ErrPermissionDenied)) + }) + + It("allows public toggle on plugin playlist", func() { + mockPlsRepo.Data["pls-plugin"] = &model.Playlist{ + ID: "pls-plugin", Name: "Plugin PL", OwnerID: "user-1", + PluginID: "test-plugin", PluginPlaylistID: "daily-mix", + } + ctx = request.WithUser(ctx, model.User{ID: "user-1", IsAdmin: false}) + repo = ps.NewRepository(ctx).(rest.Persistable) + pls := &model.Playlist{Name: "Plugin PL", Public: true} + err := repo.Update("pls-plugin", pls) + Expect(err).ToNot(HaveOccurred()) + }) + It("allows admin to update any playlist", func() { ctx = request.WithUser(ctx, model.User{ID: "admin-1", IsAdmin: true}) repo = ps.NewRepository(ctx).(rest.Persistable) diff --git a/model/playlist.go b/model/playlist.go index 0bcf20ad0..d9db3f4d2 100644 --- a/model/playlist.go +++ b/model/playlist.go @@ -36,6 +36,10 @@ type Playlist struct { PluginPlaylistID string `structs:"plugin_playlist_id" json:"pluginPlaylistId,omitempty"` } +func (pls Playlist) IsReadOnly() bool { + return pls.IsSmartPlaylist() || pls.IsPluginPlaylist() +} + func (pls Playlist) IsSmartPlaylist() bool { return pls.Rules != nil && pls.Rules.Expression != nil } diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index 8d1bbe0f8..de50d5c3d 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -52,8 +52,9 @@ func NewPlaylistRepository(ctx context.Context, db dbx.Builder) model.PlaylistRe r.ctx = ctx r.db = db r.registerModel(&model.Playlist{}, map[string]filterFunc{ - "q": playlistFilter, - "smart": smartPlaylistFilter, + "q": playlistFilter, + "smart": smartPlaylistFilter, + "readonly": readonlyPlaylistFilter, }) r.setSortMappings(map[string]string{ "owner_name": "owner_name", @@ -75,6 +76,13 @@ func smartPlaylistFilter(string, any) Sqlizer { } } +func readonlyPlaylistFilter(string, any) Sqlizer { + return And{ + smartPlaylistFilter("", nil), + Or{Eq{"plugin_id": ""}, Eq{"plugin_id": nil}}, + } +} + func (r *playlistRepository) userFilter() Sqlizer { user := loggedUser(r.ctx) if user.IsAdmin { diff --git a/server/subsonic/playlists.go b/server/subsonic/playlists.go index a8c3da68c..fc0b1cd91 100644 --- a/server/subsonic/playlists.go +++ b/server/subsonic/playlists.go @@ -165,10 +165,11 @@ func buildOSPlaylist(ctx context.Context, p model.Playlist) *responses.OpenSubso } pls := responses.OpenSubsonicPlaylist{} - if p.IsSmartPlaylist() { + if p.IsReadOnly() { pls.Readonly = true - if p.EvaluatedAt != nil { + // ValidUntil only applies to smart playlists + if p.IsSmartPlaylist() && p.EvaluatedAt != nil { pls.ValidUntil = P(p.EvaluatedAt.Add(conf.Server.SmartPlaylistRefreshDelay)) } } else { diff --git a/server/subsonic/playlists_test.go b/server/subsonic/playlists_test.go index 3f2a2068e..294f0f816 100644 --- a/server/subsonic/playlists_test.go +++ b/server/subsonic/playlists_test.go @@ -156,6 +156,41 @@ var _ = Describe("buildPlaylist", func() { }) }) + Describe("plugin playlist", func() { + BeforeEach(func() { + createdAt := time.Date(2023, 1, 15, 10, 30, 0, 0, time.UTC) + updatedAt := time.Date(2023, 2, 20, 14, 45, 0, 0, time.UTC) + + playlist = model.Playlist{ + ID: "pls-plugin", + Name: "Daily Mix", + Comment: "Generated by plugin", + OwnerName: "admin", + OwnerID: "1234", + Public: false, + SongCount: 5, + Duration: 300, + CreatedAt: createdAt, + UpdatedAt: updatedAt, + PluginID: "test-plugin", + PluginPlaylistID: "daily-mix", + } + }) + + It("marks plugin playlist as readonly", func() { + ctx = request.WithUser(ctx, model.User{ID: "1234", UserName: "admin"}) + result := router.buildPlaylist(ctx, playlist) + Expect(result.Readonly).To(BeTrue()) + Expect(result.ValidUntil).To(BeNil()) + }) + + It("marks plugin playlist as readonly even for non-owner", func() { + ctx = request.WithUser(ctx, model.User{ID: "other-user", UserName: "other"}) + result := router.buildPlaylist(ctx, playlist) + Expect(result.Readonly).To(BeTrue()) + }) + }) + Describe("smart playlist", func() { evaluatedAt := time.Date(2023, 2, 20, 15, 45, 0, 0, time.UTC) validUntil := evaluatedAt.Add(5 * time.Second) diff --git a/ui/src/common/playlistUtils.js b/ui/src/common/playlistUtils.js index 74a01d47a..eef7c680c 100644 --- a/ui/src/common/playlistUtils.js +++ b/ui/src/common/playlistUtils.js @@ -11,5 +11,7 @@ export const isReadOnly = (ownerId) => { export const isSmartPlaylist = (pls) => !!pls.rules +export const isPluginPlaylist = (pls) => !!pls.pluginId + export const canChangeTracks = (pls) => - isWritable(pls.ownerId) && !isSmartPlaylist(pls) + isWritable(pls.ownerId) && !isSmartPlaylist(pls) && !isPluginPlaylist(pls) diff --git a/ui/src/common/playlistUtils.test.js b/ui/src/common/playlistUtils.test.js index 2c671ecf5..f40bfaea7 100644 --- a/ui/src/common/playlistUtils.test.js +++ b/ui/src/common/playlistUtils.test.js @@ -2,6 +2,7 @@ import { isWritable, isReadOnly, isSmartPlaylist, + isPluginPlaylist, canChangeTracks, } from './playlistUtils' @@ -56,6 +57,18 @@ describe('playlistUtils', () => { }) }) + describe('isPluginPlaylist', () => { + it('returns true if playlist has pluginId', () => { + const playlist = { pluginId: 'test-plugin' } + expect(isPluginPlaylist(playlist)).toBe(true) + }) + + it('returns false if playlist does not have pluginId', () => { + const playlist = {} + expect(isPluginPlaylist(playlist)).toBe(false) + }) + }) + describe('canChangeTracks', () => { it('returns true if user is the owner and playlist is not smart', () => { localStorage.setItem('userId', 'user1') @@ -74,5 +87,11 @@ describe('playlistUtils', () => { const playlist = { ownerId: 'user1', rules: [] } expect(canChangeTracks(playlist)).toBe(false) }) + + it('returns false if playlist is a plugin playlist', () => { + localStorage.setItem('userId', 'user1') + const playlist = { ownerId: 'user1', pluginId: 'test-plugin' } + expect(canChangeTracks(playlist)).toBe(false) + }) }) }) diff --git a/ui/src/dialogs/AddToPlaylistDialog.test.jsx b/ui/src/dialogs/AddToPlaylistDialog.test.jsx index 60d3cca0d..8735ad885 100644 --- a/ui/src/dialogs/AddToPlaylistDialog.test.jsx +++ b/ui/src/dialogs/AddToPlaylistDialog.test.jsx @@ -46,7 +46,7 @@ const createTestUtils = (mockDataProvider) => data: mockIndexedData, list: { cachedRequests: { - '{"pagination":{"page":1,"perPage":-1},"sort":{"field":"name","order":"ASC"},"filter":{"smart":false}}': + '{"pagination":{"page":1,"perPage":-1},"sort":{"field":"name","order":"ASC"},"filter":{"readonly":false}}': { ids: ['sample-id1', 'sample-id2'], total: 2, diff --git a/ui/src/dialogs/SelectPlaylistInput.jsx b/ui/src/dialogs/SelectPlaylistInput.jsx index 847107523..bc07ec0e4 100644 --- a/ui/src/dialogs/SelectPlaylistInput.jsx +++ b/ui/src/dialogs/SelectPlaylistInput.jsx @@ -264,7 +264,7 @@ export const SelectPlaylistInput = ({ onChange }) => { 'playlist', { page: 1, perPage: -1 }, { field: 'name', order: 'ASC' }, - { smart: false }, + { readonly: false }, ) const options = diff --git a/ui/src/dialogs/SelectPlaylistInput.test.jsx b/ui/src/dialogs/SelectPlaylistInput.test.jsx index 4ffcdf0b6..44bb803ed 100644 --- a/ui/src/dialogs/SelectPlaylistInput.test.jsx +++ b/ui/src/dialogs/SelectPlaylistInput.test.jsx @@ -53,7 +53,7 @@ const createTestComponent = ( data: indexedData, list: { cachedRequests: { - '{"pagination":{"page":1,"perPage":-1},"sort":{"field":"name","order":"ASC"},"filter":{"smart":false}}': + '{"pagination":{"page":1,"perPage":-1},"sort":{"field":"name","order":"ASC"},"filter":{"readonly":false}}': { ids: Object.keys(indexedData), total: Object.keys(indexedData).length, diff --git a/ui/src/playlist/PlaylistEdit.jsx b/ui/src/playlist/PlaylistEdit.jsx index f6882e366..181d23114 100644 --- a/ui/src/playlist/PlaylistEdit.jsx +++ b/ui/src/playlist/PlaylistEdit.jsx @@ -11,7 +11,7 @@ import { ReferenceInput, SelectInput, } from 'react-admin' -import { isWritable, Title } from '../common' +import { isWritable, isPluginPlaylist, Title } from '../common' const SyncFragment = ({ formData, variant, ...rest }) => { return ( @@ -33,12 +33,17 @@ const PlaylistEditForm = (props) => { const { permissions } = usePermissions() return ( - + { ) : null }