From 9053a4ffe946073ee151c56aa78fb7c11176bbab Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 5 Mar 2026 17:37:26 -0500 Subject: [PATCH] feat(plugins): require users permission for PlaylistProvider and validate owner PlaylistProvider capability now requires 'users' permission in the manifest (matching existing Scrobbler behavior) and validates that the resolved owner user ID is in the plugin's allowed users list before creating playlists. --- plugins/manifest.go | 7 +++ plugins/manifest_test.go | 43 +++++++++++++++++++ plugins/playlist_provider.go | 9 ++++ plugins/playlist_provider_test.go | 36 ++++++++++++++++ plugins/plugins_suite_test.go | 27 +++++++++++- .../test-playlist-provider/manifest.json | 7 ++- 6 files changed, 126 insertions(+), 3 deletions(-) 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" + } + } }