diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go index 82f3ca9bf..6f55f061b 100644 --- a/cmd/wire_gen.go +++ b/cmd/wire_gen.go @@ -69,9 +69,10 @@ func CreateNativeAPIRouter(ctx context.Context) *nativeapi.Router { cacheWarmer := artwork.NewCacheWarmer(artworkArtwork, fileCache) modelScanner := scanner.New(ctx, dataStore, cacheWarmer, broker, playlists, metricsMetrics) watcher := scanner.GetWatcher(dataStore, modelScanner) - library := core.NewLibrary(dataStore, modelScanner, watcher, broker) + library := core.NewLibrary(dataStore, modelScanner, watcher, broker, manager) + user := core.NewUser(dataStore, manager) maintenance := core.NewMaintenance(dataStore) - router := nativeapi.New(dataStore, share, playlists, insights, library, maintenance, manager) + router := nativeapi.New(dataStore, share, playlists, insights, library, user, maintenance, manager) return router } @@ -200,7 +201,7 @@ func getPluginManager() *plugins.Manager { // wire_injectors.go: -var allProviders = wire.NewSet(core.Set, artwork.Set, server.New, subsonic.New, nativeapi.New, public.New, persistence.New, lastfm.NewRouter, listenbrainz.NewRouter, events.GetBroker, scanner.New, scanner.GetWatcher, metrics.GetPrometheusInstance, db.Db, plugins.GetManager, wire.Bind(new(agents.PluginLoader), new(*plugins.Manager)), wire.Bind(new(scrobbler.PluginLoader), new(*plugins.Manager)), wire.Bind(new(nativeapi.PluginManager), new(*plugins.Manager)), wire.Bind(new(plugins.PluginMetricsRecorder), new(metrics.Metrics)), wire.Bind(new(core.Watcher), new(scanner.Watcher))) +var allProviders = wire.NewSet(core.Set, artwork.Set, server.New, subsonic.New, nativeapi.New, public.New, persistence.New, lastfm.NewRouter, listenbrainz.NewRouter, events.GetBroker, scanner.New, scanner.GetWatcher, metrics.GetPrometheusInstance, db.Db, plugins.GetManager, wire.Bind(new(agents.PluginLoader), new(*plugins.Manager)), wire.Bind(new(scrobbler.PluginLoader), new(*plugins.Manager)), wire.Bind(new(nativeapi.PluginManager), new(*plugins.Manager)), wire.Bind(new(core.PluginUnloader), new(*plugins.Manager)), wire.Bind(new(plugins.PluginMetricsRecorder), new(metrics.Metrics)), wire.Bind(new(core.Watcher), new(scanner.Watcher))) func GetPluginManager(ctx context.Context) *plugins.Manager { manager := getPluginManager() diff --git a/cmd/wire_injectors.go b/cmd/wire_injectors.go index 6107fdf9b..c6df60654 100644 --- a/cmd/wire_injectors.go +++ b/cmd/wire_injectors.go @@ -45,6 +45,7 @@ var allProviders = wire.NewSet( wire.Bind(new(agents.PluginLoader), new(*plugins.Manager)), wire.Bind(new(scrobbler.PluginLoader), new(*plugins.Manager)), wire.Bind(new(nativeapi.PluginManager), new(*plugins.Manager)), + wire.Bind(new(core.PluginUnloader), new(*plugins.Manager)), wire.Bind(new(plugins.PluginMetricsRecorder), new(metrics.Metrics)), wire.Bind(new(core.Watcher), new(scanner.Watcher)), ) diff --git a/core/library.go b/core/library.go index f4f55ec5a..bb81a0a81 100644 --- a/core/library.go +++ b/core/library.go @@ -37,19 +37,21 @@ type Library interface { } type libraryService struct { - ds model.DataStore - scanner model.Scanner - watcher Watcher - broker events.Broker + ds model.DataStore + scanner model.Scanner + watcher Watcher + broker events.Broker + pluginManager PluginUnloader } // NewLibrary creates a new Library service -func NewLibrary(ds model.DataStore, scanner model.Scanner, watcher Watcher, broker events.Broker) Library { +func NewLibrary(ds model.DataStore, scanner model.Scanner, watcher Watcher, broker events.Broker, pluginManager PluginUnloader) Library { return &libraryService{ - ds: ds, - scanner: scanner, - watcher: watcher, - broker: broker, + ds: ds, + scanner: scanner, + watcher: watcher, + broker: broker, + pluginManager: pluginManager, } } @@ -141,6 +143,7 @@ func (s *libraryService) NewRepository(ctx context.Context) rest.Repository { scanner: s.scanner, watcher: s.watcher, broker: s.broker, + pluginManager: s.pluginManager, } return wrapper } @@ -148,11 +151,12 @@ func (s *libraryService) NewRepository(ctx context.Context) rest.Repository { type libraryRepositoryWrapper struct { rest.Repository model.LibraryRepository - ctx context.Context - ds model.DataStore - scanner model.Scanner - watcher Watcher - broker events.Broker + ctx context.Context + ds model.DataStore + scanner model.Scanner + watcher Watcher + broker events.Broker + pluginManager PluginUnloader } func (r *libraryRepositoryWrapper) Save(entity interface{}) (string, error) { @@ -272,6 +276,10 @@ func (r *libraryRepositoryWrapper) Delete(id string) error { log.Debug(r.ctx, "Library deleted - sent refresh event", "libraryID", libID, "name", lib.Name) } + // After successful deletion, check if any plugins were auto-disabled + // and need to be unloaded from memory + r.pluginManager.UnloadDisabledPlugins(r.ctx) + return nil } diff --git a/core/library_test.go b/core/library_test.go index bf73a62b7..16972da75 100644 --- a/core/library_test.go +++ b/core/library_test.go @@ -32,6 +32,7 @@ var _ = Describe("Library Service", func() { var scanner *tests.MockScanner var watcherManager *mockWatcherManager var broker *mockEventBroker + var pluginManager *mockPluginUnloader BeforeEach(func() { DeferCleanup(configtest.SetupConfig()) @@ -50,7 +51,9 @@ var _ = Describe("Library Service", func() { } // Create a mock event broker broker = &mockEventBroker{} - service = core.NewLibrary(ds, scanner, watcherManager, broker) + // Create a mock plugin unloader + pluginManager = &mockPluginUnloader{} + service = core.NewLibrary(ds, scanner, watcherManager, broker, pluginManager) ctx = context.Background() // Create a temporary directory for testing valid paths @@ -869,8 +872,45 @@ var _ = Describe("Library Service", func() { Expect(broker.Events).To(HaveLen(1)) }) }) + + Describe("Plugin Manager Integration", func() { + var repo rest.Persistable + + BeforeEach(func() { + // Reset the call count for each test + pluginManager.unloadCalls = 0 + r := service.NewRepository(ctx) + repo = r.(rest.Persistable) + }) + + It("calls UnloadDisabledPlugins after successful library deletion", func() { + libraryRepo.SetData(model.Libraries{ + {ID: 2, Name: "Library to Delete", Path: tempDir}, + }) + + err := repo.Delete("2") + Expect(err).NotTo(HaveOccurred()) + Expect(pluginManager.unloadCalls).To(Equal(1)) + }) + + It("does not call UnloadDisabledPlugins when library deletion fails", func() { + // Try to delete non-existent library + err := repo.Delete("999") + Expect(err).To(HaveOccurred()) + Expect(pluginManager.unloadCalls).To(Equal(0)) + }) + }) }) +// mockPluginUnloader is a simple mock for testing UnloadDisabledPlugins calls +type mockPluginUnloader struct { + unloadCalls int +} + +func (m *mockPluginUnloader) UnloadDisabledPlugins(ctx context.Context) { + m.unloadCalls++ +} + // mockWatcherManager provides a simple mock implementation of core.Watcher for testing type mockWatcherManager struct { StartedWatchers []model.Library diff --git a/core/mock_user_service.go b/core/mock_user_service.go new file mode 100644 index 000000000..132eaff6a --- /dev/null +++ b/core/mock_user_service.go @@ -0,0 +1,55 @@ +package core + +import ( + "context" + + "github.com/deluan/rest" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/tests" +) + +// MockUserWrapper provides a simple wrapper around MockedUserRepo +// that implements the core.User interface for testing +type MockUserWrapper struct { + *tests.MockedUserRepo +} + +// MockUserRestAdapter adapts MockedUserRepo to rest.Repository interface +type MockUserRestAdapter struct { + *tests.MockedUserRepo +} + +// NewMockUserService creates a new mock user service for testing +func NewMockUserService() User { + repo := tests.CreateMockUserRepo() + return &MockUserWrapper{MockedUserRepo: repo} +} + +func (m *MockUserWrapper) NewRepository(ctx context.Context) rest.Repository { + return &MockUserRestAdapter{MockedUserRepo: m.MockedUserRepo} +} + +// rest.Repository interface implementation + +func (a *MockUserRestAdapter) Count(options ...rest.QueryOptions) (int64, error) { + return a.CountAll() +} + +func (a *MockUserRestAdapter) Read(id string) (interface{}, error) { + return a.Get(id) +} + +func (a *MockUserRestAdapter) ReadAll(options ...rest.QueryOptions) (interface{}, error) { + return a.GetAll() +} + +func (a *MockUserRestAdapter) EntityName() string { + return "user" +} + +func (a *MockUserRestAdapter) NewInstance() interface{} { + return &model.User{} +} + +var _ User = (*MockUserWrapper)(nil) +var _ rest.Repository = (*MockUserRestAdapter)(nil) diff --git a/core/user.go b/core/user.go new file mode 100644 index 000000000..a0a5f5377 --- /dev/null +++ b/core/user.go @@ -0,0 +1,76 @@ +package core + +import ( + "context" + + "github.com/deluan/rest" + "github.com/navidrome/navidrome/model" +) + +// PluginUnloader defines the interface for unloading disabled plugins. +// This is satisfied by plugins.Manager but defined here to avoid import cycles. +type PluginUnloader interface { + UnloadDisabledPlugins(ctx context.Context) +} + +// User provides business logic for user management with plugin coordination. +type User interface { + NewRepository(ctx context.Context) rest.Repository +} + +type userService struct { + ds model.DataStore + pluginManager PluginUnloader +} + +// NewUser creates a new User service +func NewUser(ds model.DataStore, pluginManager PluginUnloader) User { + return &userService{ + ds: ds, + pluginManager: pluginManager, + } +} + +// NewRepository returns a REST repository wrapper for user operations. +// The wrapper intercepts Delete operations to coordinate plugin unloading. +func (s *userService) NewRepository(ctx context.Context) rest.Repository { + repo := s.ds.User(ctx) + wrapper := &userRepositoryWrapper{ + ctx: ctx, + UserRepository: repo, + pluginManager: s.pluginManager, + } + return wrapper +} + +type userRepositoryWrapper struct { + model.UserRepository + ctx context.Context + pluginManager PluginUnloader +} + +// Save implements rest.Persistable by delegating to the underlying repository. +func (r *userRepositoryWrapper) Save(entity interface{}) (string, error) { + return r.UserRepository.(rest.Persistable).Save(entity) +} + +// Update implements rest.Persistable by delegating to the underlying repository. +func (r *userRepositoryWrapper) Update(id string, entity interface{}, cols ...string) error { + return r.UserRepository.(rest.Persistable).Update(id, entity, cols...) +} + +// Delete implements rest.Persistable and coordinates plugin unloading. +func (r *userRepositoryWrapper) Delete(id string) error { + // The underlying repository Delete handles the database cleanup + // including calling cleanupPluginUserReferences + err := r.UserRepository.(rest.Persistable).Delete(id) + if err != nil { + return err + } + + // After successful deletion, check if any plugins were auto-disabled + // and need to be unloaded from memory + r.pluginManager.UnloadDisabledPlugins(r.ctx) + + return nil +} diff --git a/core/user_test.go b/core/user_test.go new file mode 100644 index 000000000..b2d3117f8 --- /dev/null +++ b/core/user_test.go @@ -0,0 +1,86 @@ +package core_test + +import ( + "context" + "errors" + + "github.com/deluan/rest" + "github.com/navidrome/navidrome/core" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("User Service", func() { + var service core.User + var ds *tests.MockDataStore + var userRepo *tests.MockedUserRepo + var pluginManager *mockPluginUnloader + var ctx context.Context + + BeforeEach(func() { + ds = &tests.MockDataStore{} + userRepo = tests.CreateMockUserRepo() + ds.MockedUser = userRepo + pluginManager = &mockPluginUnloader{} + service = core.NewUser(ds, pluginManager) + ctx = GinkgoT().Context() + }) + + Describe("NewRepository", func() { + It("returns a rest.Persistable", func() { + repo := service.NewRepository(ctx) + _, ok := repo.(rest.Persistable) + Expect(ok).To(BeTrue()) + }) + }) + + Describe("Delete", func() { + var repo rest.Persistable + + BeforeEach(func() { + r := service.NewRepository(ctx) + repo = r.(rest.Persistable) + + // Add a test user + user := &model.User{ + ID: "user-123", + UserName: "testuser", + IsAdmin: false, + } + user.NewPassword = "password" + Expect(userRepo.Put(user)).To(Succeed()) + }) + + It("deletes the user successfully", func() { + err := repo.Delete("user-123") + Expect(err).NotTo(HaveOccurred()) + + // Verify user is deleted + _, err = userRepo.Get("user-123") + Expect(err).To(Equal(model.ErrNotFound)) + }) + + It("calls UnloadDisabledPlugins after successful deletion", func() { + err := repo.Delete("user-123") + Expect(err).NotTo(HaveOccurred()) + Expect(pluginManager.unloadCalls).To(Equal(1)) + }) + + It("does not call UnloadDisabledPlugins when deletion fails", func() { + // Try to delete non-existent user + err := repo.Delete("non-existent") + Expect(err).To(HaveOccurred()) + Expect(pluginManager.unloadCalls).To(Equal(0)) + }) + + It("returns error when repository fails", func() { + userRepo.Error = errors.New("database error") + err := repo.Delete("user-123") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("database error")) + Expect(pluginManager.unloadCalls).To(Equal(0)) + }) + }) +}) diff --git a/core/wire_providers.go b/core/wire_providers.go index 16335645c..a8b1fde03 100644 --- a/core/wire_providers.go +++ b/core/wire_providers.go @@ -18,6 +18,7 @@ var Set = wire.NewSet( NewShare, NewPlaylists, NewLibrary, + NewUser, NewMaintenance, agents.GetAgents, external.NewProvider, diff --git a/persistence/library_repository.go b/persistence/library_repository.go index 9349f3c4c..f9ea65001 100644 --- a/persistence/library_repository.go +++ b/persistence/library_repository.go @@ -266,6 +266,10 @@ func (r *libraryRepository) Delete(id int) error { defer libLock.Unlock() delete(libCache, id) + // Clean up orphaned plugin references for the deleted library + if err := cleanupPluginLibraryReferences(r.db, id); err != nil { + log.Error(r.ctx, "Failed to cleanup plugin library references", "libraryID", id, err) + } return nil } diff --git a/persistence/plugin_cleanup.go b/persistence/plugin_cleanup.go new file mode 100644 index 000000000..0202726e4 --- /dev/null +++ b/persistence/plugin_cleanup.go @@ -0,0 +1,86 @@ +package persistence + +import ( + "github.com/pocketbase/dbx" +) + +// cleanupPluginUserReferences removes a user ID from all plugins' users JSON arrays +// and auto-disables plugins that lose their only permitted user (when users permission is required). +// This is called from userRepository.Delete() to maintain referential integrity. +func cleanupPluginUserReferences(db dbx.Builder, userID string) error { + // SQLite JSON function: json_remove removes the element at the path where user matches. + // We use a subquery with json_each to find and remove the user ID from the array. + // This updates all plugins where the users array contains the given user ID. + _, err := db.NewQuery(` + UPDATE plugin + SET users = ( + SELECT json_group_array(value) + FROM json_each(plugin.users) + WHERE value != {:userID} + ), + updated_at = CURRENT_TIMESTAMP + WHERE users IS NOT NULL + AND users != '' + AND EXISTS (SELECT 1 FROM json_each(plugin.users) WHERE value = {:userID}) + `).Bind(dbx.Params{"userID": userID}).Execute() + if err != nil { + return err + } + + // Auto-disable plugins that: + // 1. Are currently enabled + // 2. Require users permission (manifest has permissions.users) + // 3. Don't have allUsers enabled + // 4. Now have an empty users array after cleanup + // + // The manifest check uses JSON path to see if permissions.users exists. + _, err = db.NewQuery(` + UPDATE plugin + SET enabled = false, + updated_at = CURRENT_TIMESTAMP + WHERE enabled = true + AND all_users = false + AND json_extract(manifest, '$.permissions.users') IS NOT NULL + AND (users IS NULL OR users = '' OR users = '[]' OR json_array_length(users) = 0) + `).Execute() + return err +} + +// cleanupPluginLibraryReferences removes a library ID from all plugins' libraries JSON arrays +// and auto-disables plugins that lose their only permitted library (when library permission is required). +// This is called from libraryRepository.Delete() to maintain referential integrity. +func cleanupPluginLibraryReferences(db dbx.Builder, libraryID int) error { + // SQLite JSON function: we filter out the library ID from the array. + // Libraries are stored as integers in the JSON array. + _, err := db.NewQuery(` + UPDATE plugin + SET libraries = ( + SELECT json_group_array(value) + FROM json_each(plugin.libraries) + WHERE CAST(value AS INTEGER) != {:libraryID} + ), + updated_at = CURRENT_TIMESTAMP + WHERE libraries IS NOT NULL + AND libraries != '' + AND EXISTS (SELECT 1 FROM json_each(plugin.libraries) WHERE CAST(value AS INTEGER) = {:libraryID}) + `).Bind(dbx.Params{"libraryID": libraryID}).Execute() + if err != nil { + return err + } + + // Auto-disable plugins that: + // 1. Are currently enabled + // 2. Require library permission (manifest has permissions.library) + // 3. Don't have allLibraries enabled + // 4. Now have an empty libraries array after cleanup + _, err = db.NewQuery(` + UPDATE plugin + SET enabled = false, + updated_at = CURRENT_TIMESTAMP + WHERE enabled = true + AND all_libraries = false + AND json_extract(manifest, '$.permissions.library') IS NOT NULL + AND (libraries IS NULL OR libraries = '' OR libraries = '[]' OR json_array_length(libraries) = 0) + `).Execute() + return err +} diff --git a/persistence/plugin_cleanup_test.go b/persistence/plugin_cleanup_test.go new file mode 100644 index 000000000..bfe6d60ca --- /dev/null +++ b/persistence/plugin_cleanup_test.go @@ -0,0 +1,263 @@ +package persistence + +import ( + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/model/request" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Plugin Cleanup", func() { + var pluginRepo model.PluginRepository + var userRepo model.UserRepository + var libraryRepo model.LibraryRepository + + BeforeEach(func() { + ctx := GinkgoT().Context() + ctx = request.WithUser(ctx, model.User{ID: "admin", UserName: "admin", IsAdmin: true}) + db := GetDBXBuilder() + pluginRepo = NewPluginRepository(ctx, db) + userRepo = NewUserRepository(ctx, db) + libraryRepo = NewLibraryRepository(ctx, db) + + // Clean up any existing plugins + all, _ := pluginRepo.GetAll() + for _, p := range all { + _ = pluginRepo.Delete(p.ID) + } + }) + + AfterEach(func() { + // Clean up after tests + all, _ := pluginRepo.GetAll() + for _, p := range all { + _ = pluginRepo.Delete(p.ID) + } + }) + + Describe("cleanupPluginUserReferences", func() { + It("removes user ID from plugin users array", func() { + // Create a plugin with multiple users + plugin := &model.Plugin{ + ID: "test-plugin", + Path: "/plugins/test.wasm", + Manifest: `{"name":"test"}`, + SHA256: "abc123", + Users: `["user1","user2","user3"]`, + Enabled: true, + } + Expect(pluginRepo.Put(plugin)).To(Succeed()) + + // Clean up user2 reference + db := GetDBXBuilder() + Expect(cleanupPluginUserReferences(db, "user2")).To(Succeed()) + + // Verify user2 was removed + updated, err := pluginRepo.Get("test-plugin") + Expect(err).ToNot(HaveOccurred()) + Expect(updated.Users).To(Equal(`["user1","user3"]`)) + Expect(updated.Enabled).To(BeTrue()) // Still has users, should remain enabled + }) + + It("auto-disables plugin when last permitted user is removed", func() { + // Create a plugin that requires users permission with only one user + plugin := &model.Plugin{ + ID: "user-plugin", + Path: "/plugins/user.wasm", + Manifest: `{"name":"user-plugin","permissions":{"users":{}}}`, + SHA256: "def456", + Users: `["only-user"]`, + AllUsers: false, + Enabled: true, + } + Expect(pluginRepo.Put(plugin)).To(Succeed()) + + // Remove the only user + db := GetDBXBuilder() + Expect(cleanupPluginUserReferences(db, "only-user")).To(Succeed()) + + // Verify plugin was auto-disabled + updated, err := pluginRepo.Get("user-plugin") + Expect(err).ToNot(HaveOccurred()) + Expect(updated.Users).To(Equal(`[]`)) + Expect(updated.Enabled).To(BeFalse()) + }) + + It("does not disable plugin when allUsers is true", func() { + plugin := &model.Plugin{ + ID: "all-users-plugin", + Path: "/plugins/all.wasm", + Manifest: `{"name":"all-users","permissions":{"users":{}}}`, + SHA256: "ghi789", + Users: `["user1"]`, + AllUsers: true, + Enabled: true, + } + Expect(pluginRepo.Put(plugin)).To(Succeed()) + + // Remove the user (but allUsers is true) + db := GetDBXBuilder() + Expect(cleanupPluginUserReferences(db, "user1")).To(Succeed()) + + // Plugin should still be enabled because allUsers is true + updated, err := pluginRepo.Get("all-users-plugin") + Expect(err).ToNot(HaveOccurred()) + Expect(updated.Enabled).To(BeTrue()) + }) + + It("does not affect plugins without users permission requirement", func() { + plugin := &model.Plugin{ + ID: "no-users-perm", + Path: "/plugins/noperm.wasm", + Manifest: `{"name":"no-perm"}`, // No permissions.users in manifest + SHA256: "jkl012", + Users: `["user1"]`, + Enabled: true, + } + Expect(pluginRepo.Put(plugin)).To(Succeed()) + + // Remove the user + db := GetDBXBuilder() + Expect(cleanupPluginUserReferences(db, "user1")).To(Succeed()) + + // Plugin should still be enabled (no users permission requirement) + updated, err := pluginRepo.Get("no-users-perm") + Expect(err).ToNot(HaveOccurred()) + Expect(updated.Users).To(Equal(`[]`)) + Expect(updated.Enabled).To(BeTrue()) + }) + }) + + Describe("cleanupPluginLibraryReferences", func() { + It("removes library ID from plugin libraries array", func() { + // Create a plugin with multiple libraries + plugin := &model.Plugin{ + ID: "lib-plugin", + Path: "/plugins/lib.wasm", + Manifest: `{"name":"lib-plugin"}`, + SHA256: "mno345", + Libraries: `[1,2,3]`, + Enabled: true, + } + Expect(pluginRepo.Put(plugin)).To(Succeed()) + + // Clean up library 2 reference + db := GetDBXBuilder() + Expect(cleanupPluginLibraryReferences(db, 2)).To(Succeed()) + + // Verify library 2 was removed + updated, err := pluginRepo.Get("lib-plugin") + Expect(err).ToNot(HaveOccurred()) + Expect(updated.Libraries).To(Equal(`[1,3]`)) + }) + + It("auto-disables plugin when last permitted library is removed", func() { + // Create a plugin that requires library permission with only one library + plugin := &model.Plugin{ + ID: "lib-only-plugin", + Path: "/plugins/libonly.wasm", + Manifest: `{"name":"lib-only","permissions":{"library":{}}}`, + SHA256: "pqr678", + Libraries: `[99]`, + AllLibraries: false, + Enabled: true, + } + Expect(pluginRepo.Put(plugin)).To(Succeed()) + + // Remove the only library + db := GetDBXBuilder() + Expect(cleanupPluginLibraryReferences(db, 99)).To(Succeed()) + + // Verify plugin was auto-disabled + updated, err := pluginRepo.Get("lib-only-plugin") + Expect(err).ToNot(HaveOccurred()) + Expect(updated.Libraries).To(Equal(`[]`)) + Expect(updated.Enabled).To(BeFalse()) + }) + + It("does not disable plugin when allLibraries is true", func() { + plugin := &model.Plugin{ + ID: "all-libs-plugin", + Path: "/plugins/alllibs.wasm", + Manifest: `{"name":"all-libs","permissions":{"library":{}}}`, + SHA256: "stu901", + Libraries: `[1]`, + AllLibraries: true, + Enabled: true, + } + Expect(pluginRepo.Put(plugin)).To(Succeed()) + + // Remove the library (but allLibraries is true) + db := GetDBXBuilder() + Expect(cleanupPluginLibraryReferences(db, 1)).To(Succeed()) + + // Plugin should still be enabled + updated, err := pluginRepo.Get("all-libs-plugin") + Expect(err).ToNot(HaveOccurred()) + Expect(updated.Enabled).To(BeTrue()) + }) + }) + + Describe("User Delete integration", func() { + It("cleans up plugin references when user is deleted", func() { + // Create a test user + user := &model.User{ + ID: "test-delete-user", + UserName: "plugin-cleanup-test-user", + IsAdmin: false, + } + user.NewPassword = "password123" + Expect(userRepo.Put(user)).To(Succeed()) + + // Create a plugin referencing this user + plugin := &model.Plugin{ + ID: "user-ref-plugin", + Path: "/plugins/userref.wasm", + Manifest: `{"name":"user-ref"}`, + SHA256: "xyz123", + Users: `["test-delete-user","other-user"]`, + Enabled: true, + } + Expect(pluginRepo.Put(plugin)).To(Succeed()) + + // Delete the user + Expect(userRepo.Delete("test-delete-user")).To(Succeed()) + + // Verify user was removed from plugin + updated, err := pluginRepo.Get("user-ref-plugin") + Expect(err).ToNot(HaveOccurred()) + Expect(updated.Users).To(Equal(`["other-user"]`)) + }) + }) + + Describe("Library Delete integration", func() { + It("cleans up plugin references when library is deleted", func() { + // Create a test library (ID > 1 since ID 1 cannot be deleted) + library := &model.Library{ + ID: 99, + Name: "Test Library", + Path: "/tmp/test-lib", + } + Expect(libraryRepo.Put(library)).To(Succeed()) + + // Create a plugin referencing this library + plugin := &model.Plugin{ + ID: "lib-ref-plugin", + Path: "/plugins/libref.wasm", + Manifest: `{"name":"lib-ref"}`, + SHA256: "abc789", + Libraries: `[99,1]`, + Enabled: true, + } + Expect(pluginRepo.Put(plugin)).To(Succeed()) + + // Delete the library + Expect(libraryRepo.Delete(99)).To(Succeed()) + + // Verify library was removed from plugin + updated, err := pluginRepo.Get("lib-ref-plugin") + Expect(err).ToNot(HaveOccurred()) + Expect(updated.Libraries).To(Equal(`[1]`)) + }) + }) +}) diff --git a/persistence/user_repository.go b/persistence/user_repository.go index 7baa8f6a8..dc149e8ba 100644 --- a/persistence/user_repository.go +++ b/persistence/user_repository.go @@ -340,7 +340,15 @@ func (r *userRepository) Delete(id string) error { if errors.Is(err, model.ErrNotFound) { return rest.ErrNotFound } - return err + if err != nil { + return err + } + + // Clean up orphaned plugin references for the deleted user + if err := cleanupPluginUserReferences(r.db, id); err != nil { + log.Error(r.ctx, "Failed to cleanup plugin user references", "userID", id, err) + } + return nil } func keyTo32Bytes(input string) []byte { diff --git a/plugins/manager.go b/plugins/manager.go index 370a0e514..464c50e4e 100644 --- a/plugins/manager.go +++ b/plugins/manager.go @@ -12,6 +12,7 @@ import ( "sync/atomic" "time" + "github.com/Masterminds/squirrel" extism "github.com/extism/go-sdk" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/core/agents" @@ -532,6 +533,50 @@ func (m *Manager) unloadPlugin(name string) error { return nil } +// UnloadDisabledPlugins checks for plugins that are disabled in the database +// but still loaded in memory, and unloads them. This is called after user or +// library deletion to clean up plugins that were auto-disabled due to +// permission loss. +func (m *Manager) UnloadDisabledPlugins(ctx context.Context) { + if m.ds == nil { + return + } + + adminCtx := adminContext(ctx) + repo := m.ds.Plugin(adminCtx) + + // Get all disabled plugins from the database + plugins, err := repo.GetAll(model.QueryOptions{ + Filters: squirrel.Eq{"enabled": false}, + }) + if err != nil { + log.Error(ctx, "Failed to get disabled plugins", err) + return + } + + // Check each disabled plugin and unload if still in memory + var unloaded []string + for _, p := range plugins { + m.mu.RLock() + _, loaded := m.plugins[p.ID] + m.mu.RUnlock() + + if loaded { + if err := m.unloadPlugin(p.ID); err != nil { + log.Warn(ctx, "Failed to unload disabled plugin", "plugin", p.ID, err) + } else { + unloaded = append(unloaded, p.ID) + log.Info(ctx, "Unloaded disabled plugin", "plugin", p.ID) + } + } + } + + // Send refresh events for unloaded plugins + if len(unloaded) > 0 { + m.sendPluginRefreshEvent(ctx, unloaded...) + } +} + // checkPermissionGates validates that all permission-based requirements are met // before a plugin can be enabled. Returns an error if any gate condition fails. func (m *Manager) checkPermissionGates(p *model.Plugin) error { diff --git a/server/nativeapi/config_test.go b/server/nativeapi/config_test.go index 7edda34d2..20002cba5 100644 --- a/server/nativeapi/config_test.go +++ b/server/nativeapi/config_test.go @@ -29,7 +29,7 @@ var _ = Describe("Config API", func() { conf.Server.DevUIShowConfig = true // Enable config endpoint for tests ds = &tests.MockDataStore{} auth.Init(ds) - nativeRouter := New(ds, nil, nil, nil, core.NewMockLibraryService(), nil, nil) + nativeRouter := New(ds, nil, nil, nil, core.NewMockLibraryService(), core.NewMockUserService(), nil, nil) router = server.JWTVerifier(nativeRouter) // Create test users diff --git a/server/nativeapi/library_test.go b/server/nativeapi/library_test.go index df5e7b16e..791718115 100644 --- a/server/nativeapi/library_test.go +++ b/server/nativeapi/library_test.go @@ -30,7 +30,7 @@ var _ = Describe("Library API", func() { DeferCleanup(configtest.SetupConfig()) ds = &tests.MockDataStore{} auth.Init(ds) - nativeRouter := New(ds, nil, nil, nil, core.NewMockLibraryService(), nil, nil) + nativeRouter := New(ds, nil, nil, nil, core.NewMockLibraryService(), core.NewMockUserService(), nil, nil) router = server.JWTVerifier(nativeRouter) // Create test users diff --git a/server/nativeapi/native_api.go b/server/nativeapi/native_api.go index 3db55a8ad..bdea69c70 100644 --- a/server/nativeapi/native_api.go +++ b/server/nativeapi/native_api.go @@ -29,6 +29,7 @@ type PluginManager interface { UpdatePluginUsers(ctx context.Context, id, usersJSON string, allUsers bool) error UpdatePluginLibraries(ctx context.Context, id, librariesJSON string, allLibraries bool) error RescanPlugins(ctx context.Context) error + UnloadDisabledPlugins(ctx context.Context) } type Router struct { @@ -38,12 +39,13 @@ type Router struct { playlists core.Playlists insights metrics.Insights libs core.Library + users core.User maintenance core.Maintenance pluginManager PluginManager } -func New(ds model.DataStore, share core.Share, playlists core.Playlists, insights metrics.Insights, libraryService core.Library, maintenance core.Maintenance, pluginManager PluginManager) *Router { - r := &Router{ds: ds, share: share, playlists: playlists, insights: insights, libs: libraryService, maintenance: maintenance, pluginManager: pluginManager} +func New(ds model.DataStore, share core.Share, playlists core.Playlists, insights metrics.Insights, libraryService core.Library, userService core.User, maintenance core.Maintenance, pluginManager PluginManager) *Router { + r := &Router{ds: ds, share: share, playlists: playlists, insights: insights, libs: libraryService, users: userService, maintenance: maintenance, pluginManager: pluginManager} r.Handler = r.routes() return r } @@ -59,7 +61,7 @@ func (api *Router) routes() http.Handler { r.Use(server.Authenticator(api.ds)) r.Use(server.JWTRefresher) r.Use(server.UpdateLastAccessMiddleware(api.ds)) - api.R(r, "/user", model.User{}, true) + api.RX(r, "/user", api.users.NewRepository, true) api.R(r, "/song", model.MediaFile{}, false) api.R(r, "/album", model.Album{}, false) api.R(r, "/artist", model.Artist{}, false) diff --git a/server/nativeapi/native_api_song_test.go b/server/nativeapi/native_api_song_test.go index e90379f68..254777858 100644 --- a/server/nativeapi/native_api_song_test.go +++ b/server/nativeapi/native_api_song_test.go @@ -95,7 +95,7 @@ var _ = Describe("Song Endpoints", func() { mfRepo.SetData(testSongs) // Create the native API router and wrap it with the JWTVerifier middleware - nativeRouter := New(ds, nil, nil, nil, core.NewMockLibraryService(), nil, nil) + nativeRouter := New(ds, nil, nil, nil, core.NewMockLibraryService(), core.NewMockUserService(), nil, nil) router = server.JWTVerifier(nativeRouter) w = httptest.NewRecorder() }) diff --git a/server/nativeapi/plugin_test.go b/server/nativeapi/plugin_test.go index 2c0e7df5b..067c10e34 100644 --- a/server/nativeapi/plugin_test.go +++ b/server/nativeapi/plugin_test.go @@ -34,7 +34,7 @@ var _ = Describe("Plugin API", func() { ds = &tests.MockDataStore{} mockManager = &tests.MockPluginManager{} auth.Init(ds) - nativeRouter := New(ds, nil, nil, nil, core.NewMockLibraryService(), nil, mockManager) + nativeRouter := New(ds, nil, nil, nil, core.NewMockLibraryService(), core.NewMockUserService(), nil, mockManager) router = server.JWTVerifier(nativeRouter) // Create test users diff --git a/tests/mock_plugin_manager.go b/tests/mock_plugin_manager.go index 871f68b4b..44f20088d 100644 --- a/tests/mock_plugin_manager.go +++ b/tests/mock_plugin_manager.go @@ -106,3 +106,7 @@ func (m *MockPluginManager) RescanPlugins(ctx context.Context) error { } return m.RescanError } + +func (m *MockPluginManager) UnloadDisabledPlugins(ctx context.Context) { + // No-op for mock - plugins are not actually loaded in tests +} diff --git a/tests/mock_user_repo.go b/tests/mock_user_repo.go index 46f528ba5..b74ae74d0 100644 --- a/tests/mock_user_repo.go +++ b/tests/mock_user_repo.go @@ -134,3 +134,34 @@ func (u *MockedUserRepo) SetUserLibraries(userID string, libraryIDs []int) error u.UserLibraries[userID] = libraryIDs return nil } + +func (u *MockedUserRepo) Delete(id string) error { + if u.Error != nil { + return u.Error + } + for key, usr := range u.Data { + if usr.ID == id { + delete(u.Data, key) + delete(u.UserLibraries, id) + return nil + } + } + return model.ErrNotFound +} + +func (u *MockedUserRepo) Save(entity interface{}) (string, error) { + usr := entity.(*model.User) + if err := u.Put(usr); err != nil { + return "", err + } + return usr.ID, nil +} + +func (u *MockedUserRepo) Update(id string, entity interface{}, cols ...string) error { + if u.Error != nil { + return u.Error + } + usr := entity.(*model.User) + usr.ID = id + return u.Put(usr) +} diff --git a/ui/src/plugin/PluginList.test.jsx b/ui/src/plugin/PluginList.test.jsx index 3688c68fc..2fab2a3e0 100644 --- a/ui/src/plugin/PluginList.test.jsx +++ b/ui/src/plugin/PluginList.test.jsx @@ -134,7 +134,9 @@ describe('PluginList', () => { fireEvent.click(rescanButton) await waitFor(() => { - expect(mockNotify).toHaveBeenCalledWith('Network error', { type: 'warning' }) + expect(mockNotify).toHaveBeenCalledWith('Network error', { + type: 'warning', + }) }) }) })