From fb237cc6242594dbea2c9b13b135cb52b5e7c426 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jes=C3=BAs=20Espino?= Date: Tue, 28 May 2019 09:40:29 +0200 Subject: [PATCH] Migrate User.Update method to sync by default (#10942) --- app/bot.go | 4 +- app/user.go | 46 ++++++----- model/user.go | 5 ++ store/sqlstore/user_store.go | 125 +++++++++++++++-------------- store/store.go | 2 +- store/storetest/bot_store.go | 2 +- store/storetest/channel_store.go | 6 +- store/storetest/mocks/UserStore.go | 19 +++-- store/storetest/post_store.go | 3 +- store/storetest/team_store.go | 3 +- store/storetest/user_store.go | 16 ++-- 11 files changed, 127 insertions(+), 104 deletions(-) diff --git a/app/bot.go b/app/bot.go index e0f69f211f..72db58ed7c 100644 --- a/app/bot.go +++ b/app/bot.go @@ -44,8 +44,8 @@ func (a *App) PatchBot(botUserId string, botPatch *model.BotPatch) (*model.Bot, user.Username = patchedUser.Username user.Email = patchedUser.Email user.FirstName = patchedUser.FirstName - if result := <-a.Srv.Store.User().Update(user, true); result.Err != nil { - return nil, result.Err + if _, err := a.Srv.Store.User().Update(user, true); err != nil { + return nil, err } result := <-a.Srv.Store.Bot().Update(bot) diff --git a/app/user.go b/app/user.go index 6b69e85fb9..8beecc0c60 100644 --- a/app/user.go +++ b/app/user.go @@ -989,11 +989,11 @@ func (a *App) UpdateActive(user *model.User, active bool) (*model.User, *model.A user.DeleteAt = model.GetMillis() } - result := <-a.Srv.Store.User().Update(user, true) - if result.Err != nil { - return nil, result.Err + userUpdate, err := a.Srv.Store.User().Update(user, true) + if err != nil { + return nil, err } - ruser := result.Data.([2]*model.User)[0] + ruser := userUpdate.New if !active { if err := a.userDeactivated(ruser); err != nil { @@ -1110,7 +1110,7 @@ func (a *App) UpdateUser(user *model.User, sendNotifications bool) (*model.User, if *a.Config().EmailSettings.RequireEmailVerification && prev.Email != user.Email { newEmail = user.Email - _, err := a.GetUserByEmail(newEmail) + _, err = a.GetUserByEmail(newEmail) if err == nil { return nil, model.NewAppError("UpdateUser", "store.sql_user.update.email_taken.app_error", nil, "user_id="+user.Id, http.StatusBadRequest) } @@ -1118,32 +1118,31 @@ func (a *App) UpdateUser(user *model.User, sendNotifications bool) (*model.User, user.Email = prev.Email } - result := <-a.Srv.Store.User().Update(user, false) - if result.Err != nil { - return nil, result.Err + userUpdate, err := a.Srv.Store.User().Update(user, false) + if err != nil { + return nil, err } - rusers := result.Data.([2]*model.User) if sendNotifications { - if rusers[0].Email != rusers[1].Email || newEmail != "" { + if userUpdate.New.Email != userUpdate.Old.Email || newEmail != "" { if *a.Config().EmailSettings.RequireEmailVerification { a.Srv.Go(func() { - if err := a.SendEmailVerification(rusers[0], newEmail); err != nil { + if err := a.SendEmailVerification(userUpdate.New, newEmail); err != nil { mlog.Error(err.Error()) } }) } else { a.Srv.Go(func() { - if err := a.SendEmailChangeEmail(rusers[1].Email, rusers[0].Email, rusers[0].Locale, a.GetSiteURL()); err != nil { + if err := a.SendEmailChangeEmail(userUpdate.Old.Email, userUpdate.New.Email, userUpdate.New.Locale, a.GetSiteURL()); err != nil { mlog.Error(err.Error()) } }) } } - if rusers[0].Username != rusers[1].Username { + if userUpdate.New.Username != userUpdate.Old.Username { a.Srv.Go(func() { - if err := a.SendChangeUsernameEmail(rusers[1].Username, rusers[0].Username, rusers[0].Email, rusers[0].Locale, a.GetSiteURL()); err != nil { + if err := a.SendChangeUsernameEmail(userUpdate.Old.Username, userUpdate.New.Username, userUpdate.New.Email, userUpdate.New.Locale, a.GetSiteURL()); err != nil { mlog.Error(err.Error()) } }) @@ -1161,7 +1160,7 @@ func (a *App) UpdateUser(user *model.User, sendNotifications bool) (*model.User, }) } - return rusers[0], nil + return userUpdate.New, nil } func (a *App) UpdateUserActive(userId string, active bool) *model.AppError { @@ -1375,14 +1374,19 @@ func (a *App) UpdateUserRoles(userId string, newRoles string, sendWebSocketEvent } user.Roles = newRoles - uchan := a.Srv.Store.User().Update(user, true) + uchan := make(chan store.StoreResult, 1) + go func() { + userUpdate, err := a.Srv.Store.User().Update(user, true) + uchan <- store.StoreResult{Data: userUpdate, Err: err} + close(uchan) + }() schan := a.Srv.Store.Session().UpdateRoles(user.Id, newRoles) result := <-uchan if result.Err != nil { return nil, result.Err } - ruser := result.Data.([2]*model.User)[0] + ruser := result.Data.(*model.UserUpdate).New if result := <-schan; result.Err != nil { // soft error since the user roles were still updated @@ -1886,12 +1890,12 @@ func (a *App) UpdateOAuthUserAttrs(userData io.Reader, user *model.User, provide } if userAttrsChanged { - result := <-a.Srv.Store.User().Update(user, true) - if result.Err != nil { - return result.Err + users, err := a.Srv.Store.User().Update(user, true) + if err != nil { + return err } - user = result.Data.([2]*model.User)[0] + user = users.New a.InvalidateCacheForUser(user.Id) esInterface := a.Elasticsearch diff --git a/model/user.go b/model/user.go index 4e24c0b158..5a2829aace 100644 --- a/model/user.go +++ b/model/user.go @@ -87,6 +87,11 @@ type User struct { TermsOfServiceCreateAt int64 `db:"-" json:"terms_of_service_create_at,omitempty"` } +type UserUpdate struct { + Old *User + New *User +} + type UserPatch struct { Username *string `json:"username"` Password *string `json:"password,omitempty"` diff --git a/store/sqlstore/user_store.go b/store/sqlstore/user_store.go index 979767c580..b8b55a25a7 100644 --- a/store/sqlstore/user_store.go +++ b/store/sqlstore/user_store.go @@ -142,71 +142,72 @@ func (us SqlUserStore) Save(user *model.User) store.StoreChannel { }) } -func (us SqlUserStore) Update(user *model.User, trustedUpdateData bool) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - user.PreUpdate() +func (us SqlUserStore) Update(user *model.User, trustedUpdateData bool) (*model.UserUpdate, *model.AppError) { + user.PreUpdate() - if result.Err = user.IsValid(); result.Err != nil { - return + if err := user.IsValid(); err != nil { + return nil, err + } + + oldUserResult, err := us.GetMaster().Get(model.User{}, user.Id) + if err != nil { + return nil, model.NewAppError("SqlUserStore.Update", "store.sql_user.update.finding.app_error", nil, "user_id="+user.Id+", "+err.Error(), http.StatusInternalServerError) + } + + if oldUserResult == nil { + return nil, model.NewAppError("SqlUserStore.Update", "store.sql_user.update.find.app_error", nil, "user_id="+user.Id, http.StatusBadRequest) + } + + oldUser := oldUserResult.(*model.User) + user.CreateAt = oldUser.CreateAt + user.AuthData = oldUser.AuthData + user.AuthService = oldUser.AuthService + user.Password = oldUser.Password + user.LastPasswordUpdate = oldUser.LastPasswordUpdate + user.LastPictureUpdate = oldUser.LastPictureUpdate + user.EmailVerified = oldUser.EmailVerified + user.FailedAttempts = oldUser.FailedAttempts + user.MfaSecret = oldUser.MfaSecret + user.MfaActive = oldUser.MfaActive + + if !trustedUpdateData { + user.Roles = oldUser.Roles + user.DeleteAt = oldUser.DeleteAt + } + + if user.IsOAuthUser() { + if !trustedUpdateData { + user.Email = oldUser.Email } - - if oldUserResult, err := us.GetMaster().Get(model.User{}, user.Id); err != nil { - result.Err = model.NewAppError("SqlUserStore.Update", "store.sql_user.update.finding.app_error", nil, "user_id="+user.Id+", "+err.Error(), http.StatusInternalServerError) - } else if oldUserResult == nil { - result.Err = model.NewAppError("SqlUserStore.Update", "store.sql_user.update.find.app_error", nil, "user_id="+user.Id, http.StatusBadRequest) - } else { - oldUser := oldUserResult.(*model.User) - user.CreateAt = oldUser.CreateAt - user.AuthData = oldUser.AuthData - user.AuthService = oldUser.AuthService - user.Password = oldUser.Password - user.LastPasswordUpdate = oldUser.LastPasswordUpdate - user.LastPictureUpdate = oldUser.LastPictureUpdate - user.EmailVerified = oldUser.EmailVerified - user.FailedAttempts = oldUser.FailedAttempts - user.MfaSecret = oldUser.MfaSecret - user.MfaActive = oldUser.MfaActive - - if !trustedUpdateData { - user.Roles = oldUser.Roles - user.DeleteAt = oldUser.DeleteAt - } - - if user.IsOAuthUser() { - if !trustedUpdateData { - user.Email = oldUser.Email - } - } else if user.IsLDAPUser() && !trustedUpdateData { - if user.Username != oldUser.Username || - user.Email != oldUser.Email { - result.Err = model.NewAppError("SqlUserStore.Update", "store.sql_user.update.can_not_change_ldap.app_error", nil, "user_id="+user.Id, http.StatusBadRequest) - return - } - } else if user.Email != oldUser.Email { - user.EmailVerified = false - } - - if user.Username != oldUser.Username { - user.UpdateMentionKeysFromUsername(oldUser.Username) - } - - if count, err := us.GetMaster().Update(user); err != nil { - if IsUniqueConstraintError(err, []string{"Email", "users_email_key", "idx_users_email_unique"}) { - result.Err = model.NewAppError("SqlUserStore.Update", "store.sql_user.update.email_taken.app_error", nil, "user_id="+user.Id+", "+err.Error(), http.StatusBadRequest) - } else if IsUniqueConstraintError(err, []string{"Username", "users_username_key", "idx_users_username_unique"}) { - result.Err = model.NewAppError("SqlUserStore.Update", "store.sql_user.update.username_taken.app_error", nil, "user_id="+user.Id+", "+err.Error(), http.StatusBadRequest) - } else { - result.Err = model.NewAppError("SqlUserStore.Update", "store.sql_user.update.updating.app_error", nil, "user_id="+user.Id+", "+err.Error(), http.StatusInternalServerError) - } - } else if count != 1 { - result.Err = model.NewAppError("SqlUserStore.Update", "store.sql_user.update.app_error", nil, fmt.Sprintf("user_id=%v, count=%v", user.Id, count), http.StatusInternalServerError) - } else { - user.Sanitize(map[string]bool{}) - oldUser.Sanitize(map[string]bool{}) - result.Data = [2]*model.User{user, oldUser} - } + } else if user.IsLDAPUser() && !trustedUpdateData { + if user.Username != oldUser.Username || user.Email != oldUser.Email { + return nil, model.NewAppError("SqlUserStore.Update", "store.sql_user.update.can_not_change_ldap.app_error", nil, "user_id="+user.Id, http.StatusBadRequest) } - }) + } else if user.Email != oldUser.Email { + user.EmailVerified = false + } + + if user.Username != oldUser.Username { + user.UpdateMentionKeysFromUsername(oldUser.Username) + } + + count, err := us.GetMaster().Update(user) + if err != nil { + if IsUniqueConstraintError(err, []string{"Email", "users_email_key", "idx_users_email_unique"}) { + return nil, model.NewAppError("SqlUserStore.Update", "store.sql_user.update.email_taken.app_error", nil, "user_id="+user.Id+", "+err.Error(), http.StatusBadRequest) + } else if IsUniqueConstraintError(err, []string{"Username", "users_username_key", "idx_users_username_unique"}) { + return nil, model.NewAppError("SqlUserStore.Update", "store.sql_user.update.username_taken.app_error", nil, "user_id="+user.Id+", "+err.Error(), http.StatusBadRequest) + } + return nil, model.NewAppError("SqlUserStore.Update", "store.sql_user.update.updating.app_error", nil, "user_id="+user.Id+", "+err.Error(), http.StatusInternalServerError) + } + + if count != 1 { + return nil, model.NewAppError("SqlUserStore.Update", "store.sql_user.update.app_error", nil, fmt.Sprintf("user_id=%v, count=%v", user.Id, count), http.StatusInternalServerError) + } + + user.Sanitize(map[string]bool{}) + oldUser.Sanitize(map[string]bool{}) + return &model.UserUpdate{New: user, Old: oldUser}, nil } func (us SqlUserStore) UpdateLastPictureUpdate(userId string) store.StoreChannel { diff --git a/store/store.go b/store/store.go index c39b15f208..513de3aa88 100644 --- a/store/store.go +++ b/store/store.go @@ -245,7 +245,7 @@ type PostStore interface { type UserStore interface { Save(user *model.User) StoreChannel - Update(user *model.User, allowRoleUpdate bool) StoreChannel + Update(user *model.User, allowRoleUpdate bool) (*model.UserUpdate, *model.AppError) UpdateLastPictureUpdate(userId string) StoreChannel ResetLastPictureUpdate(userId string) StoreChannel UpdateUpdateAt(userId string) StoreChannel diff --git a/store/storetest/bot_store.go b/store/storetest/bot_store.go index c4a9dd87e3..c599602f11 100644 --- a/store/storetest/bot_store.go +++ b/store/storetest/bot_store.go @@ -175,7 +175,7 @@ func testBotStoreGetAll(t *testing.T, ss store.Store) { t.Fatal("couldn't save user", err) } deletedUser.DeleteAt = model.GetMillis() - if err := (<-ss.User().Update(&deletedUser, true)).Err; err != nil { + if _, err := ss.User().Update(&deletedUser, true); err != nil { t.Fatal("couldn't delete user", err) } defer func() { require.Nil(t, ss.User().PermanentDelete(deletedUser.Id)) }() diff --git a/store/storetest/channel_store.go b/store/storetest/channel_store.go index 888c78023a..6c6bc276cf 100644 --- a/store/storetest/channel_store.go +++ b/store/storetest/channel_store.go @@ -3433,8 +3433,10 @@ func testChannelStoreRemoveAllDeactivatedMembers(t *testing.T, ss store.Store) { // Deactivate users 1 & 2. u1.DeleteAt = model.GetMillis() u2.DeleteAt = model.GetMillis() - require.Nil(t, (<-ss.User().Update(&u1, true)).Err) - require.Nil(t, (<-ss.User().Update(&u2, true)).Err) + _, err = ss.User().Update(&u1, true) + require.Nil(t, err) + _, err = ss.User().Update(&u2, true) + require.Nil(t, err) // Remove all deactivated users from the channel. assert.Nil(t, (<-ss.Channel().RemoveAllDeactivatedMembers(c1.Id)).Err) diff --git a/store/storetest/mocks/UserStore.go b/store/storetest/mocks/UserStore.go index e3a6858fa6..4626f973d7 100644 --- a/store/storetest/mocks/UserStore.go +++ b/store/storetest/mocks/UserStore.go @@ -747,19 +747,28 @@ func (_m *UserStore) SearchWithoutTeam(term string, options *model.UserSearchOpt } // Update provides a mock function with given fields: user, allowRoleUpdate -func (_m *UserStore) Update(user *model.User, allowRoleUpdate bool) store.StoreChannel { +func (_m *UserStore) Update(user *model.User, allowRoleUpdate bool) (*model.UserUpdate, *model.AppError) { ret := _m.Called(user, allowRoleUpdate) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(*model.User, bool) store.StoreChannel); ok { + var r0 *model.UserUpdate + if rf, ok := ret.Get(0).(func(*model.User, bool) *model.UserUpdate); ok { r0 = rf(user, allowRoleUpdate) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).(*model.UserUpdate) } } - return r0 + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(*model.User, bool) *model.AppError); ok { + r1 = rf(user, allowRoleUpdate) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 } // UpdateAuthData provides a mock function with given fields: userId, service, authData, email, resetMfa diff --git a/store/storetest/post_store.go b/store/storetest/post_store.go index 68ee41555d..80a3e03755 100644 --- a/store/storetest/post_store.go +++ b/store/storetest/post_store.go @@ -2167,7 +2167,8 @@ func testPostStoreGetRepliesForExport(t *testing.T, ss store.Store) { // Checking whether replies by deleted user are exported u1.DeleteAt = 1002 - store.Must(ss.User().Update(&u1, false)) + _, err = ss.User().Update(&u1, false) + require.Nil(t, err) r1 = <-ss.Post().GetRepliesForExport(p1.Id) assert.Nil(t, r1.Err) diff --git a/store/storetest/team_store.go b/store/storetest/team_store.go index 3d4a92929c..5f2689f5a1 100644 --- a/store/storetest/team_store.go +++ b/store/storetest/team_store.go @@ -1026,7 +1026,8 @@ func testSaveTeamMemberMaxMembers(t *testing.T, ss store.Store) { user2, err := ss.User().Get(userIds[1]) require.Nil(t, err) user2.DeleteAt = 1234 - store.Must(ss.User().Update(user2, true)) + _, err = ss.User().Update(user2, true) + require.Nil(t, err) newUserId2 := store.Must(ss.User().Save(&model.User{ Username: model.NewId(), diff --git a/store/storetest/user_store.go b/store/storetest/user_store.go index 1e465b528c..d828403e7a 100644 --- a/store/storetest/user_store.go +++ b/store/storetest/user_store.go @@ -152,24 +152,24 @@ func testUserStoreUpdate(t *testing.T, ss store.Store) { time.Sleep(100 * time.Millisecond) - if err := (<-ss.User().Update(u1, false)).Err; err != nil { + if _, err := ss.User().Update(u1, false); err != nil { t.Fatal(err) } missing := &model.User{} - if err := (<-ss.User().Update(missing, false)).Err; err == nil { + if _, err := ss.User().Update(missing, false); err == nil { t.Fatal("Update should have failed because of missing key") } newId := &model.User{ Id: model.NewId(), } - if err := (<-ss.User().Update(newId, false)).Err; err == nil { + if _, err := ss.User().Update(newId, false); err == nil { t.Fatal("Update should have failed because id change") } u2.Email = MakeEmail() - if err := (<-ss.User().Update(u2, false)).Err; err == nil { + if _, err := ss.User().Update(u2, false); err == nil { t.Fatal("Update should have failed because you can't modify AD/LDAP fields") } @@ -183,20 +183,20 @@ func testUserStoreUpdate(t *testing.T, ss store.Store) { store.Must(ss.Team().SaveMember(&model.TeamMember{TeamId: model.NewId(), UserId: u3.Id}, -1)) u3.Email = MakeEmail() - if result := <-ss.User().Update(u3, false); result.Err != nil { + if userUpdate, err := ss.User().Update(u3, false); err != nil { t.Fatal("Update should not have failed") } else { - newUser := result.Data.([2]*model.User)[0] + newUser := userUpdate.New if newUser.Email != oldEmail { t.Fatal("Email should not have been updated as the update is not trusted") } } u3.Email = MakeEmail() - if result := <-ss.User().Update(u3, true); result.Err != nil { + if userUpdate, err := ss.User().Update(u3, true); err != nil { t.Fatal("Update should not have failed") } else { - newUser := result.Data.([2]*model.User)[0] + newUser := userUpdate.New if newUser.Email == oldEmail { t.Fatal("Email should have been updated as the update is trusted") }