From 9ed29fbfd15447f952d06a92590c6c440526857a Mon Sep 17 00:00:00 2001 From: Julien Tant <785518+JulienTant@users.noreply.github.com> Date: Thu, 11 Apr 2024 21:02:07 -0700 Subject: [PATCH] Set EmailVerified to true for all remote users during migration (#594) (#596) * set emailverified to true for all remote users during migration * change tests accordingly * prevent panic if remoteid is nil * add EmailVerified true to synthetic user created from getters --- server/command.go | 1 - server/command_test.go | 8 ++++---- server/handlers/getters.go | 11 ++++++----- server/handlers/getters_test.go | 3 +++ server/message_hooks.go | 1 - server/message_hooks_test.go | 29 +++++------------------------ server/plugin.go | 5 ----- server/plugin_test.go | 3 +-- server/store/sqlstore/migrations.go | 12 ++++++++++++ server/store/sqlstore/store.go | 4 ++++ 10 files changed, 35 insertions(+), 42 deletions(-) diff --git a/server/command.go b/server/command.go index b201820e3..81371d188 100644 --- a/server/command.go +++ b/server/command.go @@ -597,7 +597,6 @@ func (p *Plugin) executePromoteUserCommand(args *model.CommandArgs, parameters [ user.RemoteId = nil user.Username = newUsername - user.EmailVerified = true _, appErr = p.API.UpdateUser(user) if appErr != nil { p.sendBotEphemeralPost(args.UserId, args.ChannelId, "Error: Unable to promote account "+username) diff --git a/server/command_test.go b/server/command_test.go index 0f41b82bc..e42fcc6d1 100644 --- a/server/command_test.go +++ b/server/command_test.go @@ -1382,7 +1382,7 @@ func TestExecutePromoteCommand(t *testing.T) { api.On("HasPermissionTo", testutils.GetUserID(), model.PermissionManageSystem).Return(true).Times(1) api.On("GetUserByUsername", "valid-user").Return(&model.User{Id: "test", Username: "valid-user", RemoteId: model.NewString("remote-id")}, nil).Once() api.On("GetUserByUsername", "new-user").Return(nil, &model.AppError{}).Once() - api.On("UpdateUser", &model.User{Id: "test", Username: "new-user", RemoteId: nil, EmailVerified: true}).Return(&model.User{Id: "test", Username: "new-user", RemoteId: nil, EmailVerified: true}, nil).Once() + api.On("UpdateUser", &model.User{Id: "test", Username: "new-user", RemoteId: nil}).Return(&model.User{Id: "test", Username: "new-user", RemoteId: nil}, nil).Once() api.On("SendEphemeralPost", testutils.GetUserID(), testutils.GetEphemeralPost(p.userID, testutils.GetChannelID(), "Account valid-user has been promoted and updated the username to new-user")).Return(testutils.GetPost(testutils.GetChannelID(), testutils.GetUserID(), time.Now().UnixMicro())).Once() }, setupStore: func(s *mockStore.Store) { @@ -1396,7 +1396,7 @@ func TestExecutePromoteCommand(t *testing.T) { api.On("HasPermissionTo", testutils.GetUserID(), model.PermissionManageSystem).Return(true).Times(1) api.On("GetUserByUsername", "valid-user").Return(&model.User{Id: "test", Username: "valid-user", RemoteId: model.NewString("remote-id")}, nil).Once() api.On("GetUserByUsername", "new-user").Return(nil, &model.AppError{}).Once() - api.On("UpdateUser", &model.User{Id: "test", Username: "new-user", RemoteId: nil, EmailVerified: true}).Return(&model.User{Id: "test", Username: "new-user", RemoteId: nil}, nil).Once() + api.On("UpdateUser", &model.User{Id: "test", Username: "new-user", RemoteId: nil}).Return(&model.User{Id: "test", Username: "new-user", RemoteId: nil}, nil).Once() api.On("SendEphemeralPost", testutils.GetUserID(), testutils.GetEphemeralPost(p.userID, testutils.GetChannelID(), "Account valid-user has been promoted and updated the username to new-user")).Return(testutils.GetPost(testutils.GetChannelID(), testutils.GetUserID(), time.Now().UnixMicro())).Once() }, setupStore: func(s *mockStore.Store) { @@ -1409,7 +1409,7 @@ func TestExecutePromoteCommand(t *testing.T) { setupAPI: func(api *plugintest.API) { api.On("HasPermissionTo", testutils.GetUserID(), model.PermissionManageSystem).Return(true).Times(1) api.On("GetUserByUsername", "valid-user").Return(&model.User{Id: "test", Username: "valid-user", RemoteId: model.NewString("remote-id")}, nil).Times(2) - api.On("UpdateUser", &model.User{Id: "test", Username: "valid-user", RemoteId: nil, EmailVerified: true}).Return(&model.User{Id: "test", Username: "valid-user", RemoteId: nil, EmailVerified: true}, nil).Times(1) + api.On("UpdateUser", &model.User{Id: "test", Username: "valid-user", RemoteId: nil}).Return(&model.User{Id: "test", Username: "valid-user", RemoteId: nil}, nil).Times(1) api.On("SendEphemeralPost", testutils.GetUserID(), testutils.GetEphemeralPost(p.userID, testutils.GetChannelID(), "Account valid-user has been promoted and updated the username to valid-user")).Return(testutils.GetPost(testutils.GetChannelID(), testutils.GetUserID(), time.Now().UnixMicro())).Once() }, setupStore: func(s *mockStore.Store) { @@ -1422,7 +1422,7 @@ func TestExecutePromoteCommand(t *testing.T) { setupAPI: func(api *plugintest.API) { api.On("HasPermissionTo", testutils.GetUserID(), model.PermissionManageSystem).Return(true).Times(1) api.On("GetUserByUsername", "valid-user").Return(&model.User{Id: "test", Username: "valid-user", RemoteId: model.NewString("remote-id")}, nil).Times(2) - api.On("UpdateUser", &model.User{Id: "test", Username: "valid-user", RemoteId: nil, EmailVerified: true}).Return(&model.User{Id: "test", Username: "valid-user", RemoteId: nil}, nil).Times(1) + api.On("UpdateUser", &model.User{Id: "test", Username: "valid-user", RemoteId: nil}).Return(&model.User{Id: "test", Username: "valid-user", RemoteId: nil}, nil).Times(1) api.On("SendEphemeralPost", testutils.GetUserID(), testutils.GetEphemeralPost(p.userID, testutils.GetChannelID(), "Account valid-user has been promoted and updated the username to valid-user")).Return(testutils.GetPost(testutils.GetChannelID(), testutils.GetUserID(), time.Now().UnixMicro())).Once() }, setupStore: func(s *mockStore.Store) { diff --git a/server/handlers/getters.go b/server/handlers/getters.go index c178af307..ac7cf9877 100644 --- a/server/handlers/getters.go +++ b/server/handlers/getters.go @@ -115,11 +115,12 @@ func (ah *ActivityHandler) getOrCreateSyntheticUser(user *clientmodels.User, cre username := "msteams_" + slug.Make(userDisplayName) newMMUser := &model.User{ - Username: username, - FirstName: userDisplayName, - Email: user.Mail, - Password: ah.plugin.GenerateRandomPassword(), - RemoteId: &shortUserID, + Username: username, + FirstName: userDisplayName, + Email: user.Mail, + Password: ah.plugin.GenerateRandomPassword(), + RemoteId: &shortUserID, + EmailVerified: true, } newMMUser.SetDefaultNotifications() newMMUser.NotifyProps[model.EmailNotifyProp] = "false" diff --git a/server/handlers/getters_test.go b/server/handlers/getters_test.go index 2006fb768..9cf081fd1 100644 --- a/server/handlers/getters_test.go +++ b/server/handlers/getters_test.go @@ -120,6 +120,9 @@ func TestGetOrCreateSyntheticUser(t *testing.T) { if user.Email != testutils.GetTestEmail() { return false } + if !user.EmailVerified { + return false + } return true })).Return(&model.User{Id: "new-user-id"}, nil).Times(1) api.On("UpdatePreferencesForUser", "new-user-id", mock.MatchedBy(func(preferences model.Preferences) bool { diff --git a/server/message_hooks.go b/server/message_hooks.go index 190e534a8..c2e84b449 100644 --- a/server/message_hooks.go +++ b/server/message_hooks.go @@ -26,7 +26,6 @@ import ( func (p *Plugin) UserWillLogIn(_ *plugin.Context, user *model.User) string { if p.IsRemoteUser(user) && p.getConfiguration().AutomaticallyPromoteSyntheticUsers { *user.RemoteId = "" - user.EmailVerified = true if _, appErr := p.API.UpdateUser(user); appErr != nil { p.API.LogWarn("Unable to promote synthetic user", "user_id", user.Id, "error", appErr.Error()) return "Unable to promote synthetic user" diff --git a/server/message_hooks_test.go b/server/message_hooks_test.go index a8b1830e2..0c97928c2 100644 --- a/server/message_hooks_test.go +++ b/server/message_hooks_test.go @@ -2819,22 +2819,13 @@ func TestUserWillLogin(t *testing.T) { { Name: "Autopromotion works", User: &model.User{ - Id: testutils.GetID(), - EmailVerified: false, + Id: testutils.GetID(), }, UserIsRemote: true, AutomaticallyPromoteSyntheticUsers: true, SetupAPI: func(api *plugintest.API) { api.On("UpdateUser", mock.MatchedBy(func(user *model.User) bool { - if user.EmailVerified == false { - return false - } - - if *user.RemoteId != "" { - return false - } - - return true + return !user.IsRemote() })).Once().Return(nil, nil) api.On("LogInfo", "Promoted synthetic user", "user_id", testutils.GetID()).Once() }, @@ -2847,22 +2838,13 @@ func TestUserWillLogin(t *testing.T) { { Name: "UpdateUser failed during autopromotion", User: &model.User{ - Id: testutils.GetID(), - EmailVerified: false, + Id: testutils.GetID(), }, UserIsRemote: true, AutomaticallyPromoteSyntheticUsers: true, SetupAPI: func(api *plugintest.API) { api.On("UpdateUser", mock.MatchedBy(func(user *model.User) bool { - if user.EmailVerified == false { - return false - } - - if *user.RemoteId != "" { - return false - } - - return true + return !user.IsRemote() })).Once().Return(nil, model.NewAppError("UpdateUser", "err from test", nil, "", 500)) api.On("LogWarn", "Failed to promote synthetic user", "user_id", testutils.GetID(), "err", "err from test").Once() }, @@ -2873,8 +2855,7 @@ func TestUserWillLogin(t *testing.T) { { Name: "No autopromotion", User: &model.User{ - Id: testutils.GetID(), - EmailVerified: false, + Id: testutils.GetID(), }, UserIsRemote: true, AutomaticallyPromoteSyntheticUsers: false, diff --git a/server/plugin.go b/server/plugin.go index 603315a48..6085cb647 100644 --- a/server/plugin.go +++ b/server/plugin.go @@ -737,11 +737,6 @@ func (p *Plugin) syncUsers() { shouldUpdate = true } - if !mmUser.EmailVerified { - mmUser.EmailVerified = true - shouldUpdate = true - } - if shouldUpdate { for { p.API.LogInfo("Updating user profile", "user_id", mmUser.Id, "teams_user_id", msUser.ID) diff --git a/server/plugin_test.go b/server/plugin_test.go index 5dabba5e2..619f45686 100644 --- a/server/plugin_test.go +++ b/server/plugin_test.go @@ -372,8 +372,7 @@ func TestSyncUsers(t *testing.T) { }, nil).Times(1) api.On("GetUser", testutils.GetUserID()).Return(testutils.GetRemoteUser(model.SystemAdminRoleId, "test@test.com", "remote-id"), nil).Once() api.On("UpdateUser", mock.MatchedBy(func(u *model.User) bool { - return u.EmailVerified == true && - u.FirstName == "mockDisplayName" && + return u.FirstName == "mockDisplayName" && u.Username == "msteams_mockdisplayname" })).Return(&model.User{ Id: testutils.GetID(), diff --git a/server/store/sqlstore/migrations.go b/server/store/sqlstore/migrations.go index aeea5d500..63010eec4 100644 --- a/server/store/sqlstore/migrations.go +++ b/server/store/sqlstore/migrations.go @@ -13,3 +13,15 @@ func (s *SQLStore) runMigrationRemoteID(remoteID string) error { }).Exec() return err } + +func (s *SQLStore) runSetEmailVerifiedToTrueForRemoteUsers(remoteID string) error { + _, err := s.getQueryBuilder(). + Update("Users"). + Set("EmailVerified", true). + Where(sq.And{ + sq.Eq{"RemoteID": remoteID}, + sq.Eq{"EmailVerified": false}, + }).Exec() + + return err +} diff --git a/server/store/sqlstore/store.go b/server/store/sqlstore/store.go index c550488bb..1e8a7cfc0 100644 --- a/server/store/sqlstore/store.go +++ b/server/store/sqlstore/store.go @@ -165,6 +165,10 @@ func (s *SQLStore) Init(remoteID string) error { if err := s.runMigrationRemoteID(remoteID); err != nil { return err } + + if err := s.runSetEmailVerifiedToTrueForRemoteUsers(remoteID); err != nil { + return err + } } return nil }