From 60ec4660505e3ebce8a642a541815ca6d9c1078c Mon Sep 17 00:00:00 2001 From: Yumechi Date: Wed, 2 Sep 2026 16:51:08 +0800 Subject: [PATCH] enhance: serialize user update methods --- api/oidc.go | 2 +- api/oidc_test.go | 3 +- api/plugin.go | 12 +-- api/plugin_test.go | 71 +++++++++---- api/stream/stream.go | 3 +- api/stream/stream_test.go | 2 +- api/user.go | 115 ++++++++++++--------- api/user_test.go | 32 +++--- database/database.go | 15 ++- database/user.go | 18 ++-- database/user_test.go | 13 +-- plugin/manager.go | 208 ++++++++++++++++++++++++++------------ plugin/manager_test.go | 123 ++++++++++++---------- router/router.go | 4 +- test/testdb/database.go | 7 ++ 15 files changed, 393 insertions(+), 235 deletions(-) diff --git a/api/oidc.go b/api/oidc.go index 2e27d2a20..5734155bb 100644 --- a/api/oidc.go +++ b/api/oidc.go @@ -519,7 +519,7 @@ func (a *OIDCAPI) registerUser(username, oidcID string, hasAdminGroup bool) (*mo return nil, http.StatusInternalServerError, fmt.Errorf("failed to create user: %w", err) } log.Info().Str("oidc_id", oidcID).Str("username", user.Name).Bool("admin", user.Admin).Msg("OIDC auto registration") - if err := a.UserChangeNotifier.fireUserAdded(user.ID); err != nil { + if err := a.UserChangeNotifier.fireUserAdded(a.DB, user.ID); err != nil { log.Error().Err(err).Uint("user_id", user.ID).Msg("Could not notify user change") } return user, 0, nil diff --git a/api/oidc_test.go b/api/oidc_test.go index 913fe21c8..023733518 100644 --- a/api/oidc_test.go +++ b/api/oidc_test.go @@ -13,6 +13,7 @@ import ( "github.com/gin-gonic/gin" "github.com/gotify/server/v3/auth" + "github.com/gotify/server/v3/database" "github.com/gotify/server/v3/decaymap" "github.com/gotify/server/v3/mode" "github.com/gotify/server/v3/model" @@ -49,7 +50,7 @@ func (s *OIDCSuite) BeforeTest(suiteName, testName string) { s.db = testdb.NewDB(s.T()) s.notified = false notifier := new(UserChangeNotifier) - notifier.OnUserAdded(func(uint) error { + notifier.OnUserAdded(func(tx *database.GormDatabase, uid uint) error { s.notified = true return nil }) diff --git a/api/plugin.go b/api/plugin.go index dec78a39d..b05867ee8 100644 --- a/api/plugin.go +++ b/api/plugin.go @@ -80,7 +80,7 @@ func (c *PluginAPI) GetPlugins(ctx *gin.Context) { Website: info.Website, License: info.License, Enabled: conf.Enabled, - Capabilities: inst.Supports().Strings(), + Capabilities: inst.Instance().Supports().Strings(), }) } } @@ -255,7 +255,7 @@ func (c *PluginAPI) GetDisplay(ctx *gin.Context) { ctx.AbortWithError(404, errors.New("plugin instance not found")) return } - ctx.JSON(200, instance.GetDisplay(location.Get(ctx))) + ctx.JSON(200, instance.Instance().GetDisplay(location.Get(ctx))) }) } @@ -317,7 +317,7 @@ func (c *PluginAPI) GetConfig(ctx *gin.Context) { return } - if aborted := supportOrAbort(ctx, instance, compat.Configurer); aborted { + if aborted := supportOrAbort(ctx, instance.Instance(), compat.Configurer); aborted { return } @@ -381,11 +381,11 @@ func (c *PluginAPI) UpdateConfig(ctx *gin.Context) { return } - if aborted := supportOrAbort(ctx, instance, compat.Configurer); aborted { + if aborted := supportOrAbort(ctx, instance.Instance(), compat.Configurer); aborted { return } - newConf := instance.DefaultConfig() + newConf := instance.Instance().DefaultConfig() newconfBytes, err := io.ReadAll(ctx.Request.Body) if err != nil { ctx.AbortWithError(500, err) @@ -395,7 +395,7 @@ func (c *PluginAPI) UpdateConfig(ctx *gin.Context) { ctx.AbortWithError(400, err) return } - if err := instance.ValidateAndSetConfig(newConf); err != nil { + if err := instance.Instance().ValidateAndSetConfig(newConf); err != nil { ctx.AbortWithError(400, err) return } diff --git a/api/plugin_test.go b/api/plugin_test.go index db1d6230c..127241bbe 100644 --- a/api/plugin_test.go +++ b/api/plugin_test.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "net/http/httptest" + "sync" "testing" "github.com/gin-gonic/gin" @@ -29,6 +30,7 @@ type PluginSuite struct { suite.Suite db *testdb.Database a *PluginAPI + u *UserAPI ctx *gin.Context recorder *httptest.ResponseRecorder manager *plugin.Manager @@ -39,19 +41,23 @@ func (s *PluginSuite) BeforeTest(suiteName, testName string) { mode.Set(mode.TestDev) s.db = testdb.NewDB(s.T()) s.resetRecorder() - manager, err := plugin.NewManager(s.db, "", nil, s) + manager, err := plugin.NewManager(s.db.GormDatabase, "", nil, s) assert.Nil(s.T(), err) s.manager = manager withURL(s.ctx, "http", "example.com") s.a = &PluginAPI{DB: s.db, Manager: manager, Notifier: s} + s.u = &UserAPI{DB: s.db.GormDatabase, UserChangeNotifier: &UserChangeNotifier{}} mockPluginCompat := new(mock.Plugin) assert.Nil(s.T(), s.manager.LoadPlugin(mockPluginCompat)) - s.db.User(1) - assert.Nil(s.T(), s.manager.InitializeForUserID(1)) - s.db.User(2) - assert.Nil(s.T(), s.manager.InitializeForUserID(2)) + s.db.NewUserWithNameAdmin(1, "user1", true) + wg := &sync.WaitGroup{} + assert.Nil(s.T(), s.manager.InitializeForUserID(s.db.GormDatabase, 1, wg)) + wg.Wait() + s.db.NewUserWithNameAdmin(2, "user2", true) + assert.Nil(s.T(), s.manager.InitializeForUserID(s.db.GormDatabase, 2, wg)) + wg.Wait() s.db.CreatePluginConf(&model.PluginConf{ UserID: 1, @@ -97,6 +103,31 @@ func (s *PluginSuite) Test_GetPlugins() { assert.False(s.T(), pluginConfs[0].Enabled, "Plugins should be disabled by default") } +func (s *PluginSuite) Test_DeleteUser() { + test.WithUser(s.ctx, 1) + + s.ctx.Request = httptest.NewRequest("POST", "/plugin/1/enable", nil) + s.ctx.Params = gin.Params{{Key: "id", Value: "1"}} + s.a.EnablePlugin(s.ctx) + + assert.Equal(s.T(), 200, s.recorder.Code) + + if pluginConf, err := s.db.GetPluginConfByUserAndPath(1, mock.ModulePath); assert.NoError(s.T(), err) { + assert.True(s.T(), pluginConf.Enabled) + } + s.resetRecorder() + + s.ctx.Request = httptest.NewRequest("DELETE", "/user/1", nil) + s.ctx.Params = gin.Params{{Key: "id", Value: "1"}} + s.u.DeleteUserByID(s.ctx) + + assert.Equal(s.T(), 200, s.recorder.Code) + + user, err := s.db.GetUserByID(1) + assert.NoError(s.T(), err) + assert.Nil(s.T(), user) +} + func (s *PluginSuite) Test_EnableDisablePlugin() { { test.WithUser(s.ctx, 1) @@ -161,7 +192,9 @@ func (s *PluginSuite) Test_EnableDisablePlugin() { func (s *PluginSuite) Test_EnableDisablePlugin_EnableReturnsError_expect500() { s.db.User(16) - assert.Nil(s.T(), s.manager.InitializeForUserID(16)) + wg := &sync.WaitGroup{} + assert.Nil(s.T(), s.manager.InitializeForUserID(s.db.GormDatabase, 16, wg)) + wg.Wait() mock.ReturnErrorOnEnableForUser(16, errors.New("test error")) conf, err := s.db.GetPluginConfByUserAndPath(16, mock.ModulePath) assert.NoError(s.T(), err) @@ -183,7 +216,9 @@ func (s *PluginSuite) Test_EnableDisablePlugin_EnableReturnsError_expect500() { func (s *PluginSuite) Test_EnableDisablePlugin_DisableReturnsError_expect500() { s.db.User(17) - assert.Nil(s.T(), s.manager.InitializeForUserID(17)) + wg := &sync.WaitGroup{} + assert.Nil(s.T(), s.manager.InitializeForUserID(s.db.GormDatabase, 17, wg)) + wg.Wait() mock.ReturnErrorOnDisableForUser(17, errors.New("test error")) conf, err := s.db.GetPluginConfByUserAndPath(17, mock.ModulePath) assert.NoError(s.T(), err) @@ -291,7 +326,7 @@ func (s *PluginSuite) Test_GetDisplay() { assert.NoError(s.T(), err) inst, err := s.manager.Instance(conf.ID) assert.Nil(s.T(), err) - mockInst := inst.(*mock.PluginInstance) + mockInst := inst.Instance().(*mock.PluginInstance) mockInst.DisplayString = "test string" @@ -312,7 +347,7 @@ func (s *PluginSuite) Test_GetDisplay_NotImplemented_expectEmptyString() { assert.NoError(s.T(), err) inst, err := s.manager.Instance(conf.ID) assert.Nil(s.T(), err) - mockInst := inst.(*mock.PluginInstance) + mockInst := inst.Instance().(*mock.PluginInstance) mockInst.SetCapability(compat.Displayer, false) defer mockInst.SetCapability(compat.Displayer, true) @@ -334,7 +369,7 @@ func (s *PluginSuite) Test_GetDisplay_incorrectUser_expectNotFound() { assert.NoError(s.T(), err) inst, err := s.manager.Instance(conf.ID) assert.Nil(s.T(), err) - mockInst := inst.(*mock.PluginInstance) + mockInst := inst.Instance().(*mock.PluginInstance) mockInst.DisplayString = "test string" @@ -380,7 +415,7 @@ func (s *PluginSuite) Test_GetConfig() { assert.NoError(s.T(), err) inst, err := s.manager.Instance(conf.ID) assert.Nil(s.T(), err) - mockInst := inst.(*mock.PluginInstance) + mockInst := inst.Instance().(*mock.PluginInstance) assert.Equal(s.T(), mockInst.DefaultConfig(), mockInst.Config, "Initial config should be default config") { @@ -402,7 +437,7 @@ func (s *PluginSuite) Test_GetConfg_notImplemeted_expect400() { assert.NoError(s.T(), err) inst, err := s.manager.Instance(conf.ID) assert.Nil(s.T(), err) - mockInst := inst.(*mock.PluginInstance) + mockInst := inst.Instance().(*mock.PluginInstance) mockInst.SetCapability(compat.Configurer, false) defer mockInst.SetCapability(compat.Configurer, true) @@ -464,7 +499,7 @@ func (s *PluginSuite) Test_UpdateConfig() { assert.NoError(s.T(), err) inst, err := s.manager.Instance(conf.ID) assert.Nil(s.T(), err) - mockInst := inst.(*mock.PluginInstance) + mockInst := inst.Instance().(*mock.PluginInstance) newConfig := &mock.PluginConfig{ TestKey: "test__new__config", @@ -499,7 +534,7 @@ func (s *PluginSuite) Test_UpdateConfig_invalidConfig_expect400() { assert.NoError(s.T(), err) inst, err := s.manager.Instance(conf.ID) assert.Nil(s.T(), err) - mockInst := inst.(*mock.PluginInstance) + mockInst := inst.Instance().(*mock.PluginInstance) origConfig := mockInst.Config newConfig := &mock.PluginConfig{ @@ -536,7 +571,7 @@ func (s *PluginSuite) Test_UpdateConfig_malformedYAML_expect400() { assert.NoError(s.T(), err) inst, err := s.manager.Instance(conf.ID) assert.Nil(s.T(), err) - mockInst := inst.(*mock.PluginInstance) + mockInst := inst.Instance().(*mock.PluginInstance) origConfig := mockInst.Config newConfigYAML := []byte(`--- "rg e""`) @@ -568,7 +603,7 @@ func (s *PluginSuite) Test_UpdateConfig_ioError_expect500() { assert.NoError(s.T(), err) inst, err := s.manager.Instance(conf.ID) assert.Nil(s.T(), err) - mockInst := inst.(*mock.PluginInstance) + mockInst := inst.Instance().(*mock.PluginInstance) origConfig := mockInst.Config { @@ -598,7 +633,7 @@ func (s *PluginSuite) Test_UpdateConfig_notImplemented_expect400() { assert.NoError(s.T(), err) inst, err := s.manager.Instance(conf.ID) assert.Nil(s.T(), err) - mockInst := inst.(*mock.PluginInstance) + mockInst := inst.Instance().(*mock.PluginInstance) newConfig := &mock.PluginConfig{ TestKey: "test__new__config", @@ -626,7 +661,7 @@ func (s *PluginSuite) Test_UpdateConfig_incorrectUser_expectNotFound() { assert.NoError(s.T(), err) inst, err := s.manager.Instance(conf.ID) assert.Nil(s.T(), err) - mockInst := inst.(*mock.PluginInstance) + mockInst := inst.Instance().(*mock.PluginInstance) origConfig := mockInst.Config newConfig := &mock.PluginConfig{ diff --git a/api/stream/stream.go b/api/stream/stream.go index 4cf26b485..bc6030c64 100644 --- a/api/stream/stream.go +++ b/api/stream/stream.go @@ -12,6 +12,7 @@ import ( "github.com/gorilla/websocket" "github.com/gotify/server/v3/auth" "github.com/gotify/server/v3/config" + "github.com/gotify/server/v3/database" "github.com/gotify/server/v3/model" ) @@ -51,7 +52,7 @@ func (a *API) CollectConnectedClientTokens() []string { } // NotifyDeletedUser closes existing connections for the given user. -func (a *API) NotifyDeletedUser(userID uint) error { +func (a *API) NotifyDeletedUser(tx *database.GormDatabase, userID uint) error { a.lock.Lock() defer a.lock.Unlock() if clients, ok := a.clients[userID]; ok { diff --git a/api/stream/stream_test.go b/api/stream/stream_test.go index 8ed0697b5..233280fd6 100644 --- a/api/stream/stream_test.go +++ b/api/stream/stream_test.go @@ -322,7 +322,7 @@ func TestDeleteUser(t *testing.T) { expectNoMessage(userTwo...) expectNoMessage(userThree...) - api.NotifyDeletedUser(1) + api.NotifyDeletedUser(nil, 1) api.Notify(1, &model.MessageExternal{ID: 2, Message: "there"}) expectNoMessage(userOne...) diff --git a/api/user.go b/api/user.go index bf1ac1c12..dba460cdc 100644 --- a/api/user.go +++ b/api/user.go @@ -9,48 +9,40 @@ import ( "github.com/gin-gonic/gin" "github.com/gotify/server/v3/auth" "github.com/gotify/server/v3/auth/password" + "github.com/gotify/server/v3/database" "github.com/gotify/server/v3/model" ) -// The UserDatabase interface for encapsulating database access. -type UserDatabase interface { - GetUsers() ([]*model.User, error) - GetUserByID(id uint) (*model.User, error) - GetUserByName(name string) (*model.User, error) - DeleteUserByID(id uint) error - UpdateUser(user *model.User) error - CreateUser(user *model.User) error - CountUser(condition ...any) (int64, error) -} +var errCannotDeleteLastAdmin = errors.New("cannot delete last admin") // UserChangeNotifier notifies listeners for user changes. type UserChangeNotifier struct { - userDeletedCallbacks []func(uid uint) error - userAddedCallbacks []func(uid uint) error + userDeletedCallbacks []func(tx *database.GormDatabase, uid uint) error + userAddedCallbacks []func(tx *database.GormDatabase, uid uint) error } // OnUserDeleted is called on user deletion. -func (c *UserChangeNotifier) OnUserDeleted(cb func(uid uint) error) { +func (c *UserChangeNotifier) OnUserDeleted(cb func(tx *database.GormDatabase, uid uint) error) { c.userDeletedCallbacks = append(c.userDeletedCallbacks, cb) } // OnUserAdded is called on user creation. -func (c *UserChangeNotifier) OnUserAdded(cb func(uid uint) error) { +func (c *UserChangeNotifier) OnUserAdded(cb func(tx *database.GormDatabase, uid uint) error) { c.userAddedCallbacks = append(c.userAddedCallbacks, cb) } -func (c *UserChangeNotifier) fireUserDeleted(uid uint) error { +func (c *UserChangeNotifier) fireUserDeleted(tx *database.GormDatabase, uid uint) error { for _, cb := range c.userDeletedCallbacks { - if err := cb(uid); err != nil { + if err := cb(tx, uid); err != nil { return err } } return nil } -func (c *UserChangeNotifier) fireUserAdded(uid uint) error { +func (c *UserChangeNotifier) fireUserAdded(tx *database.GormDatabase, uid uint) error { for _, cb := range c.userAddedCallbacks { - if err := cb(uid); err != nil { + if err := cb(tx, uid); err != nil { return err } } @@ -59,7 +51,7 @@ func (c *UserChangeNotifier) fireUserAdded(uid uint) error { // The UserAPI provides handlers for managing users. type UserAPI struct { - DB UserDatabase + DB *database.GormDatabase PasswordStrength int UserChangeNotifier *UserChangeNotifier Registration bool @@ -222,11 +214,14 @@ func (a *UserAPI) CreateUser(ctx *gin.Context) { } if existingUser == nil { - if success := successOrAbort(ctx, 500, a.DB.CreateUser(internal)); !success { - return - } - if err := a.UserChangeNotifier.fireUserAdded(internal.ID); err != nil { - ctx.AbortWithError(500, err) + // this should not cause conflicts, so no need to retry + err = a.DB.Txn(func(txdb *database.GormDatabase) error { + if err := txdb.CreateUser(internal); err != nil { + return err + } + return a.UserChangeNotifier.fireUserAdded(txdb, internal.ID) + }) + if success := successOrAbort(ctx, 500, err); !success { return } ctx.JSON(200, toExternalUser(internal)) @@ -332,19 +327,26 @@ func (a *UserAPI) DeleteUserByID(ctx *gin.Context) { return } if user != nil { - adminCount, err := a.DB.CountUser(&model.User{Admin: true}) - if success := successOrAbort(ctx, 500, err); !success { - return - } - if user.Admin && adminCount == 1 { - ctx.AbortWithError(400, errors.New("cannot delete last admin")) - return - } - if err := a.UserChangeNotifier.fireUserDeleted(id); err != nil { - ctx.AbortWithError(500, err) - return + for range 3 { + err = a.DB.Txn(func(txdb *database.GormDatabase) error { + if err := txdb.DeleteUserByID(id); err != nil { + return err + } + anotherAdmin, err := txdb.GetUsers(&model.User{Admin: true}) + if err != nil { + return err + } + if user.Admin && len(anotherAdmin) == 0 { + ctx.AbortWithError(400, errCannotDeleteLastAdmin) + return errCannotDeleteLastAdmin + } + return a.UserChangeNotifier.fireUserDeleted(txdb, id) + }) + if err == nil || ctx.IsAborted() { + return + } } - successOrAbort(ctx, 500, a.DB.DeleteUserByID(id)) + successOrAbort(ctx, 500, err) } else { ctx.AbortWithError(404, errors.New("user does not exist")) } @@ -459,15 +461,7 @@ func (a *UserAPI) UpdateUserByID(ctx *gin.Context) { return } if dbUser != nil { - adminCount, err := a.DB.CountUser(&model.User{Admin: true}) - if success := successOrAbort(ctx, 500, err); !success { - return - } - if !updatedUser.Admin && dbUser.Admin && adminCount == 1 { - ctx.AbortWithError(400, errors.New("cannot delete last admin")) - return - } - + dbUserWasAdmin := dbUser.Admin dbUser.Name = updatedUser.Name dbUser.Admin = updatedUser.Admin @@ -483,10 +477,35 @@ func (a *UserAPI) UpdateUserByID(ctx *gin.Context) { } dbUser.Pass = pw } - if success := successOrAbort(ctx, 500, a.DB.UpdateUser(dbUser)); !success { - return + + for range 3 { + err = a.DB.Txn(func(txdb *database.GormDatabase) error { + if err := txdb.UpdateUser(dbUser); err != nil { + return err + } + + anotherAdmin, err := txdb.GetUsers(&model.User{Admin: true}) + if err != nil { + return err + } + if !updatedUser.Admin && dbUserWasAdmin && len(anotherAdmin) == 0 { + ctx.AbortWithError(400, errCannotDeleteLastAdmin) + return errCannotDeleteLastAdmin + } + + return nil + }) + + if ctx.IsAborted() { + return + } + + if err == nil { + ctx.JSON(200, toExternalUser(dbUser)) + return + } } - ctx.JSON(200, toExternalUser(dbUser)) + ctx.AbortWithError(500, err) } else { ctx.AbortWithError(404, errors.New("user does not exist")) } diff --git a/api/user_test.go b/api/user_test.go index aa4855f4b..b55c666df 100644 --- a/api/user_test.go +++ b/api/user_test.go @@ -1,7 +1,9 @@ package api import ( + "encoding/json" "errors" + "io" "net/http/httptest" "strings" "testing" @@ -9,6 +11,7 @@ import ( "github.com/gin-gonic/gin" "github.com/gotify/server/v3/auth" "github.com/gotify/server/v3/auth/password" + "github.com/gotify/server/v3/database" "github.com/gotify/server/v3/mode" "github.com/gotify/server/v3/model" "github.com/gotify/server/v3/test" @@ -41,15 +44,15 @@ func (s *UserSuite) BeforeTest(suiteName, testName string) { s.db = testdb.NewDB(s.T()) s.notifier = new(UserChangeNotifier) - s.notifier.OnUserDeleted(func(uint) error { + s.notifier.OnUserDeleted(func(ctx *database.GormDatabase, uid uint) error { s.notifiedDelete = true return nil }) - s.notifier.OnUserAdded(func(uint) error { + s.notifier.OnUserAdded(func(ctx *database.GormDatabase, uid uint) error { s.notifiedAdd = true return nil }) - s.a = &UserAPI{DB: s.db, UserChangeNotifier: s.notifier} + s.a = &UserAPI{DB: s.db.GormDatabase, UserChangeNotifier: s.notifier} } func (s *UserSuite) AfterTest(suiteName, testName string) { @@ -153,7 +156,7 @@ func (s *UserSuite) Test_DeleteUserByID() { func (s *UserSuite) Test_DeleteUserByID_NotifyFail() { s.db.User(5) - s.notifier.OnUserDeleted(func(id uint) error { + s.notifier.OnUserDeleted(func(ctx *database.GormDatabase, id uint) error { if id == 5 { return errors.New("some error") } @@ -230,8 +233,8 @@ func (s *UserSuite) Test_CreateUser_Register_Admin_Anonymous() { func (s *UserSuite) Test_CreateUser_NotifyFail() { s.loginAdmin() - s.notifier.OnUserAdded(func(id uint) error { - user, err := s.db.GetUserByID(id) + s.notifier.OnUserAdded(func(tx *database.GormDatabase, id uint) error { + user, err := tx.GetUserByID(id) if err != nil { return err } @@ -311,17 +314,6 @@ func (s *UserSuite) Test_UpdateUserByID_InvalidID() { assert.Equal(s.T(), 400, s.recorder.Code) } -func (s *UserSuite) Test_UpdateUserByID_EmptyPassword_Expect400() { - s.loginAdmin() - - s.ctx.Params = gin.Params{{Key: "id", Value: "1"}} - - s.ctx.Request = httptest.NewRequest("POST", "/user/1", strings.NewReader(`{"name": "admin", "pass": "", "admin": false}`)) - s.ctx.Request.Header.Set("Content-Type", "application/json") - s.a.UpdateUserByID(s.ctx) - assert.Equal(s.T(), 400, s.recorder.Code) -} - func (s *UserSuite) Test_UpdateUserByID_TooLongPassword_Expect400() { s.loginAdmin() @@ -373,6 +365,12 @@ func (s *UserSuite) Test_UpdateUserByID_UpdateNotPassword() { s.a.UpdateUserByID(s.ctx) assert.Equal(s.T(), 200, s.recorder.Code) + body, err := io.ReadAll(s.recorder.Body) + require.NoError(s.T(), err) + var retUser model.UserExternal + require.NoError(s.T(), json.Unmarshal(body, &retUser)) + assert.Equal(s.T(), "tom", retUser.Name) + assert.Equal(s.T(), true, retUser.Admin) user, err := s.db.GetUserByID(2) assert.NoError(s.T(), err) assert.NotNil(s.T(), user) diff --git a/database/database.go b/database/database.go index 8da809064..7666d12f2 100644 --- a/database/database.go +++ b/database/database.go @@ -1,6 +1,7 @@ package database import ( + "context" "database/sql" "errors" "math" @@ -172,14 +173,26 @@ func createDirectoryIfSqlite(dialect, connection string) { // GormDatabase is a wrapper for the gorm framework. type GormDatabase struct { - DB *gorm.DB + DB *gorm.DB + Nested bool } // Close closes the gorm database connection. func (d *GormDatabase) Close() { + if d.Nested { + return + } sqldb, err := d.DB.DB() if err != nil { return } sqldb.Close() } + +func (d *GormDatabase) Txn(fn func(txdb *GormDatabase) error) error { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + return d.DB.WithContext(ctx).Transaction(func(tx *gorm.DB) error { + return fn(&GormDatabase{DB: tx, Nested: true}) + }, &sql.TxOptions{Isolation: sql.LevelSerializable}) +} diff --git a/database/user.go b/database/user.go index a2bdcde14..d1acfc6ef 100644 --- a/database/user.go +++ b/database/user.go @@ -44,23 +44,19 @@ func (d *GormDatabase) GetUserByID(id uint) (*model.User, error) { return nil, err } -// CountUser returns the user count which satisfies the given condition. -func (d *GormDatabase) CountUser(condition ...any) (int64, error) { - c := int64(-1) +// GetUsers returns the users which satisfy the given condition. +func (d *GormDatabase) GetUsers(condition ...any) ([]*model.User, error) { + users := make([]*model.User, 0) handle := d.DB.Model(new(model.User)) if len(condition) == 1 { handle = handle.Where(condition[0]) } else if len(condition) > 1 { handle = handle.Where(condition[0], condition[1:]...) } - err := handle.Count(&c).Error - return c, err -} - -// GetUsers returns all users. -func (d *GormDatabase) GetUsers() ([]*model.User, error) { - var users []*model.User - err := d.DB.Find(&users).Error + err := handle.Find(&users).Error + if err == gorm.ErrRecordNotFound { + return nil, nil + } return users, err } diff --git a/database/user_test.go b/database/user_test.go index c10d97a89..aefe182cb 100644 --- a/database/user_test.go +++ b/database/user_test.go @@ -19,9 +19,10 @@ func (s *DatabaseSuite) TestUser() { require.NoError(s.T(), err) assert.NotNil(s.T(), jmattheis, "on bootup the first user should be automatically created") - adminCount, err := s.db.CountUser("admin = ?", true) + admins, err := s.db.GetUsers("admin = ?", true) require.NoError(s.T(), err) - assert.Equal(s.T(), int64(1), adminCount, "there is initially one admin") + assert.Len(s.T(), admins, 1) + assert.True(s.T(), admins[0].Admin, "the admin user should be an admin") users, err := s.db.GetUsers() require.NoError(s.T(), err) @@ -31,9 +32,9 @@ func (s *DatabaseSuite) TestUser() { nicories := &model.User{Name: "nicories", Pass: []byte{1, 2, 3, 4}, Admin: false} s.db.CreateUser(nicories) assert.NotEqual(s.T(), 0, nicories.ID, "on create user a new id should be assigned") - userCount, err := s.db.CountUser() + users, err = s.db.GetUsers() require.NoError(s.T(), err) - assert.Equal(s.T(), int64(2), userCount, "two users should exist") + assert.Len(s.T(), users, 2, "two users should exist") user, err = s.db.GetUserByName("nicories") require.NoError(s.T(), err) @@ -58,9 +59,9 @@ func (s *DatabaseSuite) TestUser() { require.NoError(s.T(), err) assert.Len(s.T(), users, 2) - adminCount, err = s.db.CountUser(&model.User{Admin: true}) + admins, err = s.db.GetUsers(&model.User{Admin: true}) require.NoError(s.T(), err) - assert.Equal(s.T(), int64(2), adminCount, "two admins exist") + assert.Len(s.T(), admins, 2, "two admins exist") require.NoError(s.T(), s.db.DeleteUserByID(tom.ID)) users, err = s.db.GetUsers() diff --git a/plugin/manager.go b/plugin/manager.go index feac8efaf..aaa3a49e4 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -6,15 +6,18 @@ import ( "encoding/json" "errors" "fmt" + "maps" "os" "path/filepath" "plugin" "strconv" "strings" "sync" + "sync/atomic" "github.com/gin-gonic/gin" "github.com/gotify/server/v3/auth" + "github.com/gotify/server/v3/database" "github.com/gotify/server/v3/model" "github.com/gotify/server/v3/plugin/compat" "github.com/rs/zerolog/log" @@ -23,7 +26,7 @@ import ( // The Database interface for encapsulating database access. type Database interface { - GetUsers() ([]*model.User, error) + GetUsers(condition ...any) ([]*model.User, error) GetPluginConfByUserAndPath(userid uint, path string) (*model.PluginConf, error) CreatePluginConf(p *model.PluginConf) error GetPluginConfByApplicationID(appid uint) (*model.PluginConf, error) @@ -43,21 +46,84 @@ type Notifier interface { Notify(userID uint, message *model.MessageExternal) } +var ( + errEnableDisableAlreadyInProgress = errors.New("enable/disable already in progress") + errAlreadyEnabled = errors.New("already enabled") + errAlreadyDisabled = errors.New("already disabled") +) + +const ( + stateEnabledBit = 0 + stateEnabledMask = uint32(1) << stateEnabledBit + stateBusyBit = 1 + stateBusyMask = uint32(1) << stateBusyBit +) + +type InstanceWrapper struct { + state atomic.Uint32 + userID uint + instance compat.PluginInstance +} + +func (i *InstanceWrapper) Instance() compat.PluginInstance { + return i.instance +} + +func (i *InstanceWrapper) Enable() error { + prevState := i.state.Or(stateBusyMask) + if prevState&stateBusyMask != 0 { + return errEnableDisableAlreadyInProgress + } + defer func() { + i.state.And(^stateBusyMask) + }() + prevState = i.state.Or(stateEnabledMask) + if prevState&stateEnabledMask != 0 { + return errAlreadyEnabled + } + err := i.instance.Enable() + if err != nil { + i.state.And(^stateEnabledMask) + return err + } + return nil +} + +func (i *InstanceWrapper) Disable() error { + prevState := i.state.Or(stateBusyMask) + if prevState&stateBusyMask != 0 { + return errEnableDisableAlreadyInProgress + } + defer func() { + i.state.And(^stateBusyMask) + }() + prevState = i.state.And(^stateEnabledMask) + if prevState&stateEnabledMask == 0 { + return errAlreadyDisabled + } + err := i.instance.Disable() + if err != nil { + i.state.Or(stateEnabledMask) + return err + } + return nil +} + // Manager is an encapsulating layer for plugins and manages all plugins and its instances. type Manager struct { mutex *sync.RWMutex - instances map[uint]compat.PluginInstance + instances map[uint]*InstanceWrapper plugins map[string]compat.Plugin messages chan MessageWithUserID - db Database + db *database.GormDatabase mux *gin.RouterGroup } // NewManager created a Manager from configurations. -func NewManager(db Database, directory string, mux *gin.RouterGroup, notifier Notifier) (*Manager, error) { +func NewManager(db *database.GormDatabase, directory string, mux *gin.RouterGroup, notifier Notifier) (*Manager, error) { manager := &Manager{ mutex: &sync.RWMutex{}, - instances: map[uint]compat.PluginInstance{}, + instances: map[uint]*InstanceWrapper{}, plugins: map[string]compat.Plugin{}, messages: make(chan MessageWithUserID), db: db, @@ -92,9 +158,11 @@ func NewManager(db Database, directory string, mux *gin.RouterGroup, notifier No return nil, err } for _, user := range users { - if err := manager.initializeForUser(*user); err != nil { + wg := &sync.WaitGroup{} + if err := manager.initializeForUser(manager.db, *user, wg); err != nil { return nil, err } + wg.Wait() } return manager, nil @@ -105,7 +173,7 @@ var ErrAlreadyEnabledOrDisabled = errors.New("config is already enabled/disabled // SetPluginEnabled sets the plugins enabled state. func (m *Manager) SetPluginEnabled(pluginID uint, enabled bool) error { - instance, err := m.Instance(pluginID) + instanceWrapper, err := m.Instance(pluginID) if err != nil { return errors.New("instance not found") } @@ -122,9 +190,9 @@ func (m *Manager) SetPluginEnabled(pluginID uint, enabled bool) error { defer m.mutex.Unlock() if enabled { - err = instance.Enable() + err = instanceWrapper.Enable() } else { - err = instance.Disable() + err = instanceWrapper.Disable() } if err != nil { return err @@ -154,7 +222,7 @@ func (m *Manager) PluginInfo(modulePath string) compat.Info { } // Instance returns an instance with the given ID. -func (m *Manager) Instance(pluginID uint) (compat.PluginInstance, error) { +func (m *Manager) Instance(pluginID uint) (*InstanceWrapper, error) { m.mutex.RLock() defer m.mutex.RUnlock() @@ -171,31 +239,22 @@ func (m *Manager) HasInstance(pluginID uint) bool { } // RemoveUser disabled all plugins of a user when the user is disabled. -func (m *Manager) RemoveUser(userID uint) error { - for _, p := range m.plugins { - pluginConf, err := m.db.GetPluginConfByUserAndPath(userID, p.PluginInfo().ModulePath) - if err != nil { - return err - } - if pluginConf == nil { - continue - } - if pluginConf.Enabled { - inst, err := m.Instance(pluginConf.ID) +func (m *Manager) RemoveUser(tx *database.GormDatabase, userID uint) error { + m.mutex.Lock() + defer m.mutex.Unlock() + maps.DeleteFunc(m.instances, func(id uint, instance *InstanceWrapper) (delete bool) { + delete = instance.userID == userID + if delete && instance.state.Load()&stateEnabledMask != 0 { + // ignore errors and force delete to prevent + // leftover instances for deleted users + err := instance.Disable() if err != nil { - continue - } - m.mutex.Lock() - err = inst.Disable() - m.mutex.Unlock() - if err != nil { - return err + log.Warn().Err(err).Uint("user_id", userID).Uint("plugin_id", id).Msg("Plugin disable failed") } } - m.mutex.Lock() - delete(m.instances, pluginConf.ID) - m.mutex.Unlock() - } + return delete + }) + return nil } @@ -256,21 +315,21 @@ func (m *Manager) LoadPlugin(compatPlugin compat.Plugin) error { } // InitializeForUserID initializes all plugin instances for a given user. -func (m *Manager) InitializeForUserID(userID uint) error { +func (m *Manager) InitializeForUserID(tx *database.GormDatabase, userID uint, wg *sync.WaitGroup) error { m.mutex.Lock() defer m.mutex.Unlock() - user, err := m.db.GetUserByID(userID) + user, err := tx.GetUserByID(userID) if err != nil { return err } if user != nil { - return m.initializeForUser(*user) + return m.initializeForUser(tx, *user, wg) } return fmt.Errorf("user with id %d not found", userID) } -func (m *Manager) initializeForUser(user model.User) error { +func (m *Manager) initializeForUser(tx *database.GormDatabase, user model.User, wg *sync.WaitGroup) error { userCtx := compat.UserContext{ ID: user.ID, Name: user.Name, @@ -278,17 +337,17 @@ func (m *Manager) initializeForUser(user model.User) error { } for _, p := range m.plugins { - if err := m.initializeSingleUserPlugin(userCtx, p); err != nil { + if err := m.initializeSingleUserPlugin(tx, userCtx, p, wg); err != nil { return err } } - apps, err := m.db.GetApplicationsByUser(user.ID) + apps, err := tx.GetApplicationsByUser(user.ID) if err != nil { return err } for _, app := range apps { - conf, err := m.db.GetPluginConfByApplicationID(app.ID) + conf, err := tx.GetPluginConfByApplicationID(app.ID) if err != nil { return err } @@ -298,31 +357,36 @@ func (m *Manager) initializeForUser(user model.User) error { } else { app.Internal = false } - m.db.UpdateApplication(app) + tx.UpdateApplication(app) } return nil } -func (m *Manager) initializeSingleUserPlugin(userCtx compat.UserContext, p compat.Plugin) error { +func (m *Manager) initializeSingleUserPlugin(tx *database.GormDatabase, userCtx compat.UserContext, p compat.Plugin, wg *sync.WaitGroup) error { info := p.PluginInfo() instance := p.NewPluginInstance(userCtx) userID := userCtx.ID - pluginConf, err := m.db.GetPluginConfByUserAndPath(userID, info.ModulePath) + pluginConf, err := tx.GetPluginConfByUserAndPath(userID, info.ModulePath) if err != nil { return err } if pluginConf == nil { var err error - pluginConf, err = m.createPluginConf(instance, info, userID) + pluginConf, err = m.createPluginConf(tx, instance, info, userID) if err != nil { return err } } - m.instances[pluginConf.ID] = instance + instanceWrapper := &InstanceWrapper{ + userID: userID, + instance: instance, + } + + m.instances[pluginConf.ID] = instanceWrapper if compat.HasSupport(instance, compat.Messenger) { if pluginConf.ApplicationID == 0 { @@ -330,12 +394,12 @@ func (m *Manager) initializeSingleUserPlugin(userCtx compat.UserContext, p compa // initialized for the user, so no internal application exists yet. // Create one now, otherwise messages would be stored with // application_id = 0 and become orphaned (not shown, not deletable). - app, err := m.createInternalApplication(info, userID) + app, err := m.createInternalApplication(tx, info, userID) if err != nil { return err } pluginConf.ApplicationID = app.ID - if err := m.db.UpdatePluginConf(pluginConf); err != nil { + if err := tx.UpdatePluginConf(pluginConf); err != nil { return err } } @@ -345,36 +409,46 @@ func (m *Manager) initializeSingleUserPlugin(userCtx compat.UserContext, p compa Messages: m.messages, }) } - if compat.HasSupport(instance, compat.Storager) { - instance.SetStorageHandler(dbStorageHandler{pluginConf.ID, m.db}) - } if compat.HasSupport(instance, compat.Configurer) { - m.initializeConfigurerForSingleUserPlugin(instance, pluginConf) + m.initializeConfigurerForSingleUserPlugin(tx, instance, pluginConf) } if compat.HasSupport(instance, compat.Webhooker) { id := pluginConf.ID g := m.mux.Group(pluginConf.Token+"/", requirePluginEnabled(id, m.db)) instance.RegisterWebhook(strings.Replace(g.BasePath(), ":id", strconv.Itoa(int(id)), 1), g) } - if pluginConf.Enabled { - err := instance.Enable() - if err != nil { - // Single user plugin cannot be enabled - // Don't panic, disable for now and wait for user to update config - log.Warn().Err(err).Str("user", userCtx.Name).Msg("Plugin initialize failed, disabling now") - pluginConf.Enabled = false - m.db.UpdatePluginConf(pluginConf) - } + if wg != nil { + wg.Add(1) } + go func() { + if compat.HasSupport(instance, compat.Storager) { + instance.SetStorageHandler(dbStorageHandler{pluginConf.ID, m.db}) + } + if pluginConf.Enabled { + err := instanceWrapper.Enable() + if err != nil { + // Single user plugin cannot be enabled + // Don't panic, disable for now and wait for user to update config + log.Warn().Err(err).Str("user", userCtx.Name).Msg("Plugin initialize failed, disabling now") + pluginConf.Enabled = false + if err = tx.UpdatePluginConf(pluginConf); err != nil { + log.Warn().Err(err).Uint("plugin_id", pluginConf.ID).Msg("Plugin enable failed, disabling now") + } + } + } + if wg != nil { + wg.Done() + } + }() return nil } -func (m *Manager) initializeConfigurerForSingleUserPlugin(instance compat.PluginInstance, pluginConf *model.PluginConf) { +func (m *Manager) initializeConfigurerForSingleUserPlugin(tx *database.GormDatabase, instance compat.PluginInstance, pluginConf *model.PluginConf) { if len(pluginConf.Config) == 0 { // The Configurer is newly implemented // Use the default config pluginConf.Config, _ = yaml.Marshal(instance.DefaultConfig()) - m.db.UpdatePluginConf(pluginConf) + tx.UpdatePluginConf(pluginConf) } c := instance.DefaultConfig() if yaml.Unmarshal(pluginConf.Config, c) != nil || instance.ValidateAndSetConfig(c) != nil { @@ -400,12 +474,12 @@ func (m *Manager) initializeConfigurerForSingleUserPlugin(instance compat.Plugin pluginConf.Config = newConf.Bytes() - m.db.UpdatePluginConf(pluginConf) + tx.UpdatePluginConf(pluginConf) instance.ValidateAndSetConfig(instance.DefaultConfig()) } } -func (m *Manager) createPluginConf(instance compat.PluginInstance, info compat.Info, userID uint) (*model.PluginConf, error) { +func (m *Manager) createPluginConf(tx *database.GormDatabase, instance compat.PluginInstance, info compat.Info, userID uint) (*model.PluginConf, error) { pluginConf := &model.PluginConf{ UserID: userID, ModulePath: info.ModulePath, @@ -415,13 +489,13 @@ func (m *Manager) createPluginConf(instance compat.PluginInstance, info compat.I pluginConf.Config, _ = yaml.Marshal(instance.DefaultConfig()) } if compat.HasSupport(instance, compat.Messenger) { - app, err := m.createInternalApplication(info, userID) + app, err := m.createInternalApplication(tx, info, userID) if err != nil { return nil, err } pluginConf.ApplicationID = app.ID } - if err := m.db.CreatePluginConf(pluginConf); err != nil { + if err := tx.CreatePluginConf(pluginConf); err != nil { return nil, err } return pluginConf, nil @@ -429,7 +503,7 @@ func (m *Manager) createPluginConf(instance compat.PluginInstance, info compat.I // createInternalApplication creates the auto generated internal application a // Messenger plugin uses to publish its messages. -func (m *Manager) createInternalApplication(info compat.Info, userID uint) (*model.Application, error) { +func (m *Manager) createInternalApplication(tx *database.GormDatabase, info compat.Info, userID uint) (*model.Application, error) { tokenPublic, _ := auth.GenerateApplicationToken() app := &model.Application{ Token: tokenPublic, @@ -438,7 +512,7 @@ func (m *Manager) createInternalApplication(info compat.Info, userID uint) (*mod Internal: true, Description: fmt.Sprintf("auto generated application for %s", info.ModulePath), } - if err := m.db.CreateApplication(app); err != nil { + if err := tx.CreateApplication(app); err != nil { return nil, err } return app, nil diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 9c3edaf4a..4703992b3 100644 --- a/plugin/manager_test.go +++ b/plugin/manager_test.go @@ -9,6 +9,7 @@ import ( "os" "os/exec" "path" + "sync" "testing" "time" @@ -21,6 +22,7 @@ import ( "github.com/gotify/server/v3/test/testdb" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/suite" + "gorm.io/gorm" ) const ( @@ -70,10 +72,12 @@ func (s *ManagerSuite) SetupSuite() { p := new(mock.Plugin) assert.Nil(s.T(), manager.LoadPlugin(p)) - assert.Nil(s.T(), manager.initializeSingleUserPlugin(compat.UserContext{ + wg := &sync.WaitGroup{} + assert.Nil(s.T(), manager.initializeSingleUserPlugin(s.db.GormDatabase, compat.UserContext{ ID: 1, Admin: true, - }, p)) + }, p, wg)) + wg.Wait() s.manager = manager s.msgReceiver = make(chan MessageWithUserID) @@ -102,7 +106,10 @@ func (s *ManagerSuite) getConfForMockPlugin(uid uint) *model.PluginConf { func (s *ManagerSuite) getMockPluginInstance(uid uint) *mock.PluginInstance { pid := s.getConfForMockPlugin(uid).ID - return s.manager.instances[pid].(*mock.PluginInstance) + instance, ok := s.manager.instances[pid] + assert.True(s.T(), ok) + assert.Equal(s.T(), uid, instance.userID) + return instance.instance.(*mock.PluginInstance) } func (s *ManagerSuite) makeDanglingPluginConf(uid uint) *model.PluginConf { @@ -200,7 +207,9 @@ func (s *ManagerSuite) TestInitializePlugin_alreadyEnabledInConf_expectAutoEnabl Enabled: true, }) - assert.Nil(s.T(), s.manager.InitializeForUserID(2)) + wg := &sync.WaitGroup{} + assert.Nil(s.T(), s.manager.InitializeForUserID(s.db.GormDatabase, 2, wg)) + wg.Wait() inst := s.getMockPluginInstance(2) assert.True(s.T(), inst.Enabled) } @@ -215,7 +224,9 @@ func (s *ManagerSuite) TestInitializePlugin_alreadyEnabledInConf_failedToLoadCon Config: []byte(`invalid: """`), }) - assert.Nil(s.T(), s.manager.InitializeForUserID(3)) + wg := &sync.WaitGroup{} + assert.Nil(s.T(), s.manager.InitializeForUserID(s.db.GormDatabase, 3, wg)) + wg.Wait() inst := s.getMockPluginInstance(3) assert.False(s.T(), inst.Enabled) } @@ -230,14 +241,18 @@ func (s *ManagerSuite) TestInitializePlugin_alreadyEnabled_cannotEnable_disabled Enabled: true, }) - assert.Nil(s.T(), s.manager.InitializeForUserID(4)) + wg := &sync.WaitGroup{} + assert.Nil(s.T(), s.manager.InitializeForUserID(s.db.GormDatabase, 4, wg)) + wg.Wait() inst := s.getMockPluginInstance(4) assert.False(s.T(), inst.Enabled) assert.False(s.T(), s.getConfForMockPlugin(4).Enabled) } func (s *ManagerSuite) TestInitializePlugin_userIDNotExist_expectError() { - assert.Error(s.T(), s.manager.InitializeForUserID(99)) + wg := &sync.WaitGroup{} + assert.Error(s.T(), s.manager.InitializeForUserID(s.db.GormDatabase, 99, wg)) + wg.Wait() } func (s *ManagerSuite) TestSetPluginEnabled() { @@ -252,7 +267,9 @@ func (s *ManagerSuite) TestSetPluginEnabled_EnableReturnsError_cannotEnable() { errExpected := errors.New("test error") mock.ReturnErrorOnEnableForUser(5, errExpected) - assert.Nil(s.T(), s.manager.InitializeForUserID(5)) + wg := &sync.WaitGroup{} + assert.Nil(s.T(), s.manager.InitializeForUserID(s.db.GormDatabase, 5, wg)) + wg.Wait() pid := s.getConfForMockPlugin(5).ID assert.Error(s.T(), s.manager.SetPluginEnabled(pid, false)) @@ -266,7 +283,9 @@ func (s *ManagerSuite) TestSetPluginEnabled_DisableReturnsError_cannotDisable() errExpected := errors.New("test error") mock.ReturnErrorOnDisableForUser(6, errExpected) - assert.Nil(s.T(), s.manager.InitializeForUserID(6)) + wg := &sync.WaitGroup{} + assert.Nil(s.T(), s.manager.InitializeForUserID(s.db.GormDatabase, 6, wg)) + wg.Wait() pid := s.getConfForMockPlugin(6).ID assert.Nil(s.T(), s.manager.SetPluginEnabled(pid, true)) @@ -279,23 +298,27 @@ func (s *ManagerSuite) TestAddRemoveNewUser() { s.db.User(7) s.makeDanglingPluginConf(7) - assert.Nil(s.T(), s.manager.InitializeForUserID(7)) + wg := &sync.WaitGroup{} + assert.Nil(s.T(), s.manager.InitializeForUserID(s.db.GormDatabase, 7, wg)) + wg.Wait() pid := s.getConfForExamplePlugin(7).ID assert.True(s.T(), s.manager.HasInstance(pid)) assert.Nil(s.T(), s.manager.SetPluginEnabled(s.getConfForMockPlugin(7).ID, true)) - assert.Nil(s.T(), s.manager.RemoveUser(7)) + assert.Nil(s.T(), s.manager.RemoveUser(s.db.GormDatabase, 7)) assert.False(s.T(), s.manager.HasInstance(pid)) } -func (s *ManagerSuite) TestRemoveUser_DisableFail_cannotRemove() { - s.manager.initializeForUser(*s.db.NewUserWithName(8, "disable_fail_2")) +func (s *ManagerSuite) TestRemoveUser_DisableFail_forceRemove() { + wg := &sync.WaitGroup{} + s.manager.initializeForUser(s.db.GormDatabase, *s.db.NewUserWithName(8, "disable_fail_2"), wg) + wg.Wait() errExpected := errors.New("test error") mock.ReturnErrorOnDisableForUser(8, errExpected) s.manager.SetPluginEnabled(s.getConfForMockPlugin(8).ID, true) - assert.EqualError(s.T(), s.manager.RemoveUser(8), errExpected.Error()) + assert.Nil(s.T(), s.manager.RemoveUser(s.db.GormDatabase, 8)) } func (s *ManagerSuite) TestRemoveUser_danglingConf_expectSuccess() { @@ -313,7 +336,7 @@ func (s *ManagerSuite) TestRemoveUser_danglingConf_expectSuccess() { UserID: 9, Token: auth.GeneratePluginToken(), }) - assert.Nil(s.T(), s.manager.RemoveUser(9)) + assert.Nil(s.T(), s.manager.RemoveUser(s.db.GormDatabase, 9)) } func (s *ManagerSuite) TestTriggerMessage() { @@ -380,7 +403,7 @@ func TestNewManager_InternalApplicationManagement(t *testing.T) { if app, err := db.GetApplicationByToken("Ainternal_obsolete"); assert.NoError(t, err) { assert.True(t, app.Internal) } - _, err := NewManager(db, "", nil, nil) + _, err := NewManager(db.GormDatabase, "", nil, nil) assert.Nil(t, err) if app, err := db.GetApplicationByToken("Ainternal_obsolete"); assert.NoError(t, err) { assert.False(t, app.Internal) @@ -406,7 +429,7 @@ func TestNewManager_InternalApplicationManagement(t *testing.T) { if app, err := db.GetApplicationByToken("Ainternal_not_loaded"); assert.NoError(t, err) { assert.True(t, app.Internal) } - _, err := NewManager(db, "", nil, nil) + _, err := NewManager(db.GormDatabase, "", nil, nil) assert.Nil(t, err) if app, err := db.GetApplicationByToken("Ainternal_not_loaded"); assert.NoError(t, err) { assert.False(t, app.Internal) @@ -433,10 +456,12 @@ func TestNewManager_InternalApplicationManagement(t *testing.T) { if app, err := db.GetApplicationByToken("Ainternal_loaded"); assert.NoError(t, err) { assert.False(t, app.Internal) } - manager, err := NewManager(db, "", nil, nil) + manager, err := NewManager(db.GormDatabase, "", nil, nil) assert.Nil(t, err) assert.Nil(t, manager.LoadPlugin(new(mock.Plugin))) - assert.Nil(t, manager.InitializeForUserID(1)) + wg := &sync.WaitGroup{} + assert.Nil(t, manager.InitializeForUserID(db.GormDatabase, 1, wg)) + wg.Wait() if app, err := db.GetApplicationByToken("Ainternal_loaded"); assert.NoError(t, err) { assert.True(t, app.Internal) } @@ -456,12 +481,14 @@ func TestNewManager_MessengerAddedAfterInit_createsApplication(t *testing.T) { Token: auth.GeneratePluginToken(), })) - manager, err := NewManager(db, "", nil, nil) + manager, err := NewManager(db.GormDatabase, "", nil, nil) assert.Nil(t, err) assert.Nil(t, manager.LoadPlugin(new(mock.Plugin))) // The mock plugin supports Messenger, so re-initializing must back-fill the // missing internal application instead of leaving ApplicationID at 0. - assert.Nil(t, manager.InitializeForUserID(1)) + wg := &sync.WaitGroup{} + assert.Nil(t, manager.InitializeForUserID(db.GormDatabase, 1, wg)) + wg.Wait() conf, err := db.GetPluginConfByUserAndPath(1, mock.ModulePath) assert.NoError(t, err) @@ -477,29 +504,6 @@ func TestNewManager_MessengerAddedAfterInit_createsApplication(t *testing.T) { } } -// failingDB wraps a real test database so that individual operations can be -// forced to fail, allowing the error branches around internal-application -// creation to be exercised. -type failingDB struct { - *testdb.Database - failCreateApplication error - failUpdatePluginConf error -} - -func (d *failingDB) CreateApplication(app *model.Application) error { - if d.failCreateApplication != nil { - return d.failCreateApplication - } - return d.Database.CreateApplication(app) -} - -func (d *failingDB) UpdatePluginConf(conf *model.PluginConf) error { - if d.failUpdatePluginConf != nil { - return d.failUpdatePluginConf - } - return d.Database.UpdatePluginConf(conf) -} - func seedMessengerConfWithoutApplication(t *testing.T, db Database) { t.Helper() assert.NoError(t, db.CreatePluginConf(&model.PluginConf{ @@ -511,35 +515,42 @@ func seedMessengerConfWithoutApplication(t *testing.T, db Database) { } func TestNewManager_MessengerAddedAfterInit_createApplicationError(t *testing.T) { - db := &failingDB{ - Database: testdb.NewDBWithDefaultUser(t), - failCreateApplication: errors.New("create application failed"), - } + expectedError := errors.New("create application failed") + db := testdb.NewDBWithDefaultUser(t) seedMessengerConfWithoutApplication(t, db) + db.GormDatabase.DB.Callback().Create().Before("gorm:create").Register("failCreateApplication", func(tx *gorm.DB) { + tx.AddError(expectedError) + }) - manager, err := NewManager(db, "", nil, nil) + manager, err := NewManager(db.GormDatabase, "", nil, nil) assert.Nil(t, err) assert.Nil(t, manager.LoadPlugin(new(mock.Plugin))) // Back-filling the missing internal application must surface the database // error instead of silently continuing with ApplicationID == 0. - assert.EqualError(t, manager.InitializeForUserID(1), "create application failed") + wg := &sync.WaitGroup{} + err = manager.InitializeForUserID(db.GormDatabase, 1, wg) + wg.Wait() + assert.ErrorIs(t, err, expectedError) } func TestNewManager_MessengerAddedAfterInit_updatePluginConfError(t *testing.T) { - db := &failingDB{ - Database: testdb.NewDBWithDefaultUser(t), - failUpdatePluginConf: errors.New("update plugin conf failed"), - } + expectedError := errors.New("update plugin conf failed") + db := testdb.NewDBWithDefaultUser(t) seedMessengerConfWithoutApplication(t, db) + db.GormDatabase.DB.Callback().Update().Before("gorm:update").Register("failUpdatePluginConf", func(tx *gorm.DB) { + tx.AddError(expectedError) + }) - manager, err := NewManager(db, "", nil, nil) + manager, err := NewManager(db.GormDatabase, "", nil, nil) assert.Nil(t, err) assert.Nil(t, manager.LoadPlugin(new(mock.Plugin))) // Persisting the back-filled ApplicationID may fail; that error must be // propagated as well. - assert.EqualError(t, manager.InitializeForUserID(1), "update plugin conf failed") + wg := &sync.WaitGroup{} + assert.ErrorIs(t, manager.InitializeForUserID(db.GormDatabase, 1, wg), expectedError) + wg.Wait() } func TestPluginFileLoadError(t *testing.T) { diff --git a/router/router.go b/router/router.go index 57719f89e..1da162a57 100644 --- a/router/router.go +++ b/router/router.go @@ -118,7 +118,9 @@ func Create(db *database.GormDatabase, vInfo *model.VersionInfo, conf *config.Co userChangeNotifier.OnUserDeleted(streamHandler.NotifyDeletedUser) userChangeNotifier.OnUserDeleted(pluginManager.RemoveUser) - userChangeNotifier.OnUserAdded(pluginManager.InitializeForUserID) + userChangeNotifier.OnUserAdded(func(tx *database.GormDatabase, uid uint) error { + return pluginManager.InitializeForUserID(tx, uid, nil) + }) ui.Register(g, *vInfo, conf.Registration, conf.LocalAuthEnabled, conf.OIDC.Enabled, conf.OIDC.IDPName, conf.OIDC.AutoRedirect) diff --git a/test/testdb/database.go b/test/testdb/database.go index 26f9b0890..d20e7f9e0 100644 --- a/test/testdb/database.go +++ b/test/testdb/database.go @@ -72,6 +72,13 @@ func (d *Database) NewUserWithName(id uint, name string) *model.User { return user } +// NewUserWithNameAdmin creates a user with a name and admin status and returns the user. +func (d *Database) NewUserWithNameAdmin(id uint, name string, admin bool) *model.User { + user := &model.User{ID: id, Name: name, Admin: admin} + d.CreateUser(user) + return user +} + // App creates an application and returns a message builder. func (ab *AppClientBuilder) App(id uint) *MessageBuilder { return ab.app(id, false)