diff --git a/plugins/manifest.go b/plugins/manifest.go index 7484718e3..bd04e6e52 100644 --- a/plugins/manifest.go +++ b/plugins/manifest.go @@ -65,6 +65,13 @@ func ValidateWithCapabilities(m *Manifest, capabilities []Capability) error { } } + // PlaylistProvider capability requires users permission + if hasCapability(capabilities, CapabilityPlaylistProvider) { + if m.Permissions == nil || m.Permissions.Users == nil { + return fmt.Errorf("playlist provider capability requires 'users' permission to be declared in manifest") + } + } + // Scheduler permission requires SchedulerCallback capability if m.Permissions != nil && m.Permissions.Scheduler != nil { if !hasCapability(capabilities, CapabilityScheduler) { diff --git a/plugins/manifest_test.go b/plugins/manifest_test.go index c45a480eb..723749cb3 100644 --- a/plugins/manifest_test.go +++ b/plugins/manifest_test.go @@ -463,5 +463,48 @@ var _ = Describe("Manifest", func() { err := ValidateWithCapabilities(m, []Capability{}) Expect(err).ToNot(HaveOccurred()) }) + + It("validates playlist provider capability with users permission", func() { + m := &Manifest{ + Name: "Test", + Author: "Author", + Version: "1.0.0", + Permissions: &Permissions{ + Users: &UsersPermission{}, + }, + } + + err := ValidateWithCapabilities(m, []Capability{CapabilityPlaylistProvider}) + Expect(err).ToNot(HaveOccurred()) + }) + + It("returns error when playlist provider capability without users permission", func() { + m := &Manifest{ + Name: "Test", + Author: "Author", + Version: "1.0.0", + } + + err := ValidateWithCapabilities(m, []Capability{CapabilityPlaylistProvider}) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("playlist provider")) + Expect(err.Error()).To(ContainSubstring("users")) + }) + + It("returns error when playlist provider has permissions but no users", func() { + m := &Manifest{ + Name: "Test", + Author: "Author", + Version: "1.0.0", + Permissions: &Permissions{ + Http: &HTTPPermission{}, + }, + } + + err := ValidateWithCapabilities(m, []Capability{CapabilityPlaylistProvider}) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("playlist provider")) + Expect(err.Error()).To(ContainSubstring("users")) + }) }) }) diff --git a/plugins/playlist_provider.go b/plugins/playlist_provider.go index c1cb326aa..b26fc0629 100644 --- a/plugins/playlist_provider.go +++ b/plugins/playlist_provider.go @@ -3,6 +3,7 @@ package plugins import ( "context" "fmt" + "slices" "strings" "sync/atomic" "time" @@ -133,6 +134,14 @@ func (o *playlistSyncer) discoverAndSync() { continue } ownerID := user.ID + + // Validate that the plugin is permitted to create playlists for this user + if !o.plugin.allUsers && !slices.Contains(o.plugin.allowedUserIDs, ownerID) { + log.Error(ctx, "Plugin not permitted to create playlists for user", "plugin", o.pluginName, + "playlistID", info.ID, "username", info.OwnerUsername) + continue + } + dbID := id.NewHash(o.pluginName, info.ID, ownerID) o.syncPlaylist(info, dbID, ownerID) } diff --git a/plugins/playlist_provider_test.go b/plugins/playlist_provider_test.go index 7d18e04e6..a72ef357f 100644 --- a/plugins/playlist_provider_test.go +++ b/plugins/playlist_provider_test.go @@ -173,6 +173,42 @@ var _ = Describe("PlaylistProvider", Ordered, func() { }) }) + Describe("user permission validation", func() { + It("skips playlists for unauthorized users when AllUsers is false", func() { + // Create manager with restricted users — only "other-user" is allowed, + // but the plugin returns playlists for "admin" which resolves to "user-1" + restrictedManager, _ := createTestManagerWithPluginOverrides(nil, + map[string]pluginOverride{ + "test-playlist-provider": {AllUsers: false, Users: `["other-user"]`}, + }, + "test-playlist-provider"+PackageExtension, + ) + + // No playlists should be created because "user-1" is not in allowed users + restrictedPlsRepo := restrictedManager.ds.(*tests.MockDataStore).MockedPlaylist.(*tests.MockPlaylistRepo) + Consistently(func() int { + return restrictedPlsRepo.Len() + }, "500ms").Should(Equal(0)) + }) + + It("creates playlists for authorized users when AllUsers is false", func() { + // Create manager with restricted users — "user-1" is allowed, + // and the plugin returns playlists for "admin" which resolves to "user-1" + allowedManager, _ := createTestManagerWithPluginOverrides(nil, + map[string]pluginOverride{ + "test-playlist-provider": {AllUsers: false, Users: `["user-1"]`}, + }, + "test-playlist-provider"+PackageExtension, + ) + + // Playlists should be created because "user-1" is in allowed users + allowedPlsRepo := allowedManager.ds.(*tests.MockDataStore).MockedPlaylist.(*tests.MockPlaylistRepo) + Eventually(func() int { + return allowedPlsRepo.Len() + }).Should(BeNumerically(">=", 2)) + }) + }) + Describe("stop", func() { It("stops the syncer when the manager stops", func() { stopManager, _ := createTestManagerWithPlugins(nil, diff --git a/plugins/plugins_suite_test.go b/plugins/plugins_suite_test.go index d284577d5..7f7d95f05 100644 --- a/plugins/plugins_suite_test.go +++ b/plugins/plugins_suite_test.go @@ -82,10 +82,26 @@ func createTestManagerWithPlugins(pluginConfig map[string]map[string]string, plu return createTestManagerWithPluginsAndMetrics(pluginConfig, noopMetricsRecorder{}, plugins...) } +// pluginOverride allows tests to override model.Plugin fields for specific plugins. +type pluginOverride struct { + AllUsers bool + Users string // JSON array of user IDs, e.g. `["user-1"]` +} + +// createTestManagerWithPluginOverrides creates a new plugin Manager with the given plugin config, +// per-plugin overrides, and specified plugins. +func createTestManagerWithPluginOverrides(pluginConfig map[string]map[string]string, overrides map[string]pluginOverride, plugins ...string) (*Manager, string) { + return createTestManagerFull(pluginConfig, overrides, noopMetricsRecorder{}, plugins...) +} + // createTestManagerWithPluginsAndMetrics creates a new plugin Manager with the given plugin config, // metrics recorder, and specified plugins. It creates a temp directory, copies the specified plugins, and starts the manager. // Returns the manager and temp directory path. func createTestManagerWithPluginsAndMetrics(pluginConfig map[string]map[string]string, metrics PluginMetricsRecorder, plugins ...string) (*Manager, string) { + return createTestManagerFull(pluginConfig, nil, metrics, plugins...) +} + +func createTestManagerFull(pluginConfig map[string]map[string]string, overrides map[string]pluginOverride, metrics PluginMetricsRecorder, plugins ...string) (*Manager, string) { // Create temp directory tmpDir, err := os.MkdirTemp("", "plugins-test-*") Expect(err).ToNot(HaveOccurred()) @@ -114,14 +130,21 @@ func createTestManagerWithPluginsAndMetrics(pluginConfig map[string]map[string]s configJSON = string(configBytes) } - enabledPlugins = append(enabledPlugins, model.Plugin{ + p := model.Plugin{ ID: pluginName, Path: destPath, SHA256: hashHex, Enabled: true, Config: configJSON, AllUsers: true, // Allow all users by default in tests - }) + } + if overrides != nil { + if o, ok := overrides[pluginName]; ok { + p.AllUsers = o.AllUsers + p.Users = o.Users + } + } + enabledPlugins = append(enabledPlugins, p) } // Setup config diff --git a/plugins/testdata/test-playlist-provider/manifest.json b/plugins/testdata/test-playlist-provider/manifest.json index f429c71d6..7efec1b2a 100644 --- a/plugins/testdata/test-playlist-provider/manifest.json +++ b/plugins/testdata/test-playlist-provider/manifest.json @@ -2,5 +2,10 @@ "name": "Test Playlist Provider", "author": "Navidrome Test", "version": "1.0.0", - "description": "A test playlist provider plugin for integration testing" + "description": "A test playlist provider plugin for integration testing", + "permissions": { + "users": { + "reason": "Required for playlist ownership" + } + } }