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 }