From 927b11f6e247746f0f5ceeae592b9c525e3c2f76 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Thu, 7 Jun 2018 14:19:19 -0400 Subject: MM-10803: remove premature user sanitization on deactivation (#8926) * remove unused UpdateNonSSOUserActive * MM-10803: stop prematurely sanitizing users on deactivate This change was preceded by the removal of UpdateNonSSOUserActive to ensure there are no APIs relying on the sanitized return value. * MM-10803: test websocket events after UpdateUserActive --- api4/apitestlib.go | 4 ++ api4/user_test.go | 184 +++++++++++++++++++++++++++++++++++++++-------------- app/user.go | 19 ------ app/user_test.go | 18 ------ 4 files changed, 140 insertions(+), 85 deletions(-) diff --git a/api4/apitestlib.go b/api4/apitestlib.go index 22084a1d6..8293a03f7 100644 --- a/api4/apitestlib.go +++ b/api4/apitestlib.go @@ -271,6 +271,10 @@ func (me *TestHelper) CreateWebSocketClient() (*model.WebSocketClient, *model.Ap return model.NewWebSocketClient4(fmt.Sprintf("ws://localhost:%v", me.App.Srv.ListenAddr.Port), me.Client.AuthToken) } +func (me *TestHelper) CreateWebSocketSystemAdminClient() (*model.WebSocketClient, *model.AppError) { + return model.NewWebSocketClient4(fmt.Sprintf("ws://localhost:%v", me.App.Srv.ListenAddr.Port), me.SystemAdminClient.AuthToken) +} + func (me *TestHelper) CreateUser() *model.User { return me.CreateUserWithClient(me.Client) } diff --git a/api4/user_test.go b/api4/user_test.go index 593208c92..1044e6162 100644 --- a/api4/user_test.go +++ b/api4/user_test.go @@ -513,7 +513,7 @@ func TestSearchUsers(t *testing.T) { t.Fatal("should have found user") } - _, err := th.App.UpdateNonSSOUserActive(th.BasicUser2.Id, false) + _, err := th.App.UpdateActive(th.BasicUser2, false) if err != nil { t.Fatal(err) } @@ -630,7 +630,7 @@ func TestSearchUsers(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { cfg.PrivacySettings.ShowEmailAddress = false }) th.App.UpdateConfig(func(cfg *model.Config) { cfg.PrivacySettings.ShowFullName = false }) - _, err = th.App.UpdateNonSSOUserActive(th.BasicUser2.Id, true) + _, err = th.App.UpdateActive(th.BasicUser2, true) if err != nil { t.Fatal(err) } @@ -1190,71 +1190,159 @@ func TestUpdateUserRoles(t *testing.T) { CheckBadRequestStatus(t, resp) } +func assertExpectedWebsocketEvent(t *testing.T, client *model.WebSocketClient, event string, test func(*model.WebSocketEvent)) { + for { + select { + case resp, ok := <-client.EventChannel: + if !ok { + t.Fatalf("channel closed before receiving expected event %s", model.WEBSOCKET_EVENT_USER_UPDATED) + } else if resp.Event == model.WEBSOCKET_EVENT_USER_UPDATED { + test(resp) + return + } + case <-time.After(5 * time.Second): + t.Fatalf("failed to receive expected event %s", model.WEBSOCKET_EVENT_USER_UPDATED) + } + } +} + +func assertWebsocketEventUserUpdatedWithEmail(t *testing.T, client *model.WebSocketClient, email string) { + assertExpectedWebsocketEvent(t, client, model.WEBSOCKET_EVENT_USER_UPDATED, func(event *model.WebSocketEvent) { + if eventUser, ok := event.Data["user"].(map[string]interface{}); !ok { + t.Fatalf("expected user") + } else if userEmail, ok := eventUser["email"].(string); !ok { + t.Fatalf("expected email %s, but got nil", email) + } else { + assert.Equal(t, email, userEmail) + } + }) +} + func TestUpdateUserActive(t *testing.T) { - th := Setup().InitBasic().InitSystemAdmin() - defer th.TearDown() + t.Run("basic tests", func(t *testing.T) { + th := Setup().InitBasic().InitSystemAdmin() + defer th.TearDown() - Client := th.Client - SystemAdminClient := th.SystemAdminClient - user := th.BasicUser + Client := th.Client + SystemAdminClient := th.SystemAdminClient + user := th.BasicUser - EnableUserDeactivation := th.App.Config().TeamSettings.EnableUserDeactivation - defer func() { - th.App.UpdateConfig(func(cfg *model.Config) { cfg.TeamSettings.EnableUserDeactivation = EnableUserDeactivation }) - }() + EnableUserDeactivation := th.App.Config().TeamSettings.EnableUserDeactivation + defer func() { + th.App.UpdateConfig(func(cfg *model.Config) { cfg.TeamSettings.EnableUserDeactivation = EnableUserDeactivation }) + }() - th.App.UpdateConfig(func(cfg *model.Config) { *cfg.TeamSettings.EnableUserDeactivation = true }) - pass, resp := Client.UpdateUserActive(user.Id, false) - CheckNoError(t, resp) + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.TeamSettings.EnableUserDeactivation = true }) + pass, resp := Client.UpdateUserActive(user.Id, false) + CheckNoError(t, resp) - if !pass { - t.Fatal("should have returned true") - } + if !pass { + t.Fatal("should have returned true") + } - th.App.UpdateConfig(func(cfg *model.Config) { *cfg.TeamSettings.EnableUserDeactivation = false }) - pass, resp = Client.UpdateUserActive(user.Id, false) - CheckUnauthorizedStatus(t, resp) + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.TeamSettings.EnableUserDeactivation = false }) + pass, resp = Client.UpdateUserActive(user.Id, false) + CheckUnauthorizedStatus(t, resp) - if pass { - t.Fatal("should have returned false") - } + if pass { + t.Fatal("should have returned false") + } - th.App.UpdateConfig(func(cfg *model.Config) { *cfg.TeamSettings.EnableUserDeactivation = true }) - pass, resp = Client.UpdateUserActive(user.Id, false) - CheckUnauthorizedStatus(t, resp) + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.TeamSettings.EnableUserDeactivation = true }) + pass, resp = Client.UpdateUserActive(user.Id, false) + CheckUnauthorizedStatus(t, resp) - if pass { - t.Fatal("should have returned false") - } + if pass { + t.Fatal("should have returned false") + } - th.LoginBasic2() + th.LoginBasic2() - _, resp = Client.UpdateUserActive(user.Id, true) - CheckForbiddenStatus(t, resp) + _, resp = Client.UpdateUserActive(user.Id, true) + CheckForbiddenStatus(t, resp) - _, resp = Client.UpdateUserActive(GenerateTestId(), true) - CheckForbiddenStatus(t, resp) + _, resp = Client.UpdateUserActive(GenerateTestId(), true) + CheckForbiddenStatus(t, resp) - _, resp = Client.UpdateUserActive("junk", true) - CheckBadRequestStatus(t, resp) + _, resp = Client.UpdateUserActive("junk", true) + CheckBadRequestStatus(t, resp) - Client.Logout() + Client.Logout() - _, resp = Client.UpdateUserActive(user.Id, true) - CheckUnauthorizedStatus(t, resp) + _, resp = Client.UpdateUserActive(user.Id, true) + CheckUnauthorizedStatus(t, resp) - _, resp = SystemAdminClient.UpdateUserActive(user.Id, true) - CheckNoError(t, resp) + _, resp = SystemAdminClient.UpdateUserActive(user.Id, true) + CheckNoError(t, resp) - _, resp = SystemAdminClient.UpdateUserActive(user.Id, false) - CheckNoError(t, resp) + _, resp = SystemAdminClient.UpdateUserActive(user.Id, false) + CheckNoError(t, resp) - authData := model.NewId() - result := <-th.App.Srv.Store.User().UpdateAuthData(user.Id, "random", &authData, "", true) - require.Nil(t, result.Err) + authData := model.NewId() + result := <-th.App.Srv.Store.User().UpdateAuthData(user.Id, "random", &authData, "", true) + require.Nil(t, result.Err) - _, resp = SystemAdminClient.UpdateUserActive(user.Id, false) - CheckNoError(t, resp) + _, resp = SystemAdminClient.UpdateUserActive(user.Id, false) + CheckNoError(t, resp) + }) + + t.Run("websocket events", func(t *testing.T) { + th := Setup().InitBasic().InitSystemAdmin() + defer th.TearDown() + + SystemAdminClient := th.SystemAdminClient + user := th.BasicUser2 + + EnableUserDeactivation := th.App.Config().TeamSettings.EnableUserDeactivation + defer func() { + th.App.UpdateConfig(func(cfg *model.Config) { cfg.TeamSettings.EnableUserDeactivation = EnableUserDeactivation }) + }() + + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.TeamSettings.EnableUserDeactivation = true }) + + webSocketClient, err := th.CreateWebSocketClient() + assert.Nil(t, err) + defer webSocketClient.Close() + + webSocketClient.Listen() + + time.Sleep(300 * time.Millisecond) + if resp := <-webSocketClient.ResponseChannel; resp.Status != model.STATUS_OK { + t.Fatal("should have responded OK to authentication challenge") + } + + adminWebSocketClient, err := th.CreateWebSocketSystemAdminClient() + assert.Nil(t, err) + defer adminWebSocketClient.Close() + + adminWebSocketClient.Listen() + + time.Sleep(300 * time.Millisecond) + if resp := <-adminWebSocketClient.ResponseChannel; resp.Status != model.STATUS_OK { + t.Fatal("should have responded OK to authentication challenge") + } + + ShowEmailAddress := th.App.Config().PrivacySettings.ShowEmailAddress + defer func() { + th.App.UpdateConfig(func(cfg *model.Config) { cfg.PrivacySettings.ShowEmailAddress = ShowEmailAddress }) + }() + + // Verify that both admins and regular users see the email when privacy settings allow same. + th.App.UpdateConfig(func(cfg *model.Config) { cfg.PrivacySettings.ShowEmailAddress = true }) + _, resp := SystemAdminClient.UpdateUserActive(user.Id, false) + CheckNoError(t, resp) + + assertWebsocketEventUserUpdatedWithEmail(t, webSocketClient, user.Email) + assertWebsocketEventUserUpdatedWithEmail(t, adminWebSocketClient, user.Email) + + // Verify that only admins see the email when privacy settings hide emails. + th.App.UpdateConfig(func(cfg *model.Config) { cfg.PrivacySettings.ShowEmailAddress = false }) + _, resp = SystemAdminClient.UpdateUserActive(user.Id, true) + CheckNoError(t, resp) + + assertWebsocketEventUserUpdatedWithEmail(t, webSocketClient, "") + assertWebsocketEventUserUpdatedWithEmail(t, adminWebSocketClient, user.Email) + }) } func TestGetUsers(t *testing.T) { diff --git a/app/user.go b/app/user.go index 2325c6338..ccf8dd40e 100644 --- a/app/user.go +++ b/app/user.go @@ -862,22 +862,6 @@ func (a *App) UpdatePasswordAsUser(userId, currentPassword, newPassword string) return a.UpdatePasswordSendEmail(user, newPassword, T("api.user.update_password.menu")) } -func (a *App) UpdateNonSSOUserActive(userId string, active bool) (*model.User, *model.AppError) { - var user *model.User - var err *model.AppError - if user, err = a.GetUser(userId); err != nil { - return nil, err - } - - if user.IsSSOUser() { - err := model.NewAppError("UpdateActive", "api.user.update_active.no_deactivate_sso.app_error", nil, "userId="+user.Id, http.StatusBadRequest) - err.StatusCode = http.StatusBadRequest - return nil, err - } - - return a.UpdateActive(user, active) -} - func (a *App) UpdateActive(user *model.User, active bool) (*model.User, *model.AppError) { if active { user.DeleteAt = 0 @@ -895,9 +879,6 @@ func (a *App) UpdateActive(user *model.User, active bool) (*model.User, *model.A } ruser := result.Data.([2]*model.User)[0] - options := a.Config().GetSanitizeOptions() - options["passwordupdate"] = false - ruser.Sanitize(options) if !active { a.SetStatusOffline(ruser.Id, false) diff --git a/app/user_test.go b/app/user_test.go index f0e026fa9..b557d296b 100644 --- a/app/user_test.go +++ b/app/user_test.go @@ -96,24 +96,6 @@ func TestCreateOAuthUser(t *testing.T) { } } -func TestDeactivateSSOUser(t *testing.T) { - th := Setup().InitBasic() - defer th.TearDown() - - r := rand.New(rand.NewSource(time.Now().UnixNano())) - glUser := oauthgitlab.GitLabUser{Id: int64(r.Intn(1000)) + 1, Username: "o" + model.NewId(), Email: model.NewId() + "@simulator.amazonses.com", Name: "Joram Wilander"} - - json := glUser.ToJson() - user, err := th.App.CreateOAuthUser(model.USER_AUTH_SERVICE_GITLAB, strings.NewReader(json), th.BasicTeam.Id) - if err != nil { - t.Fatal(err) - } - defer th.App.PermanentDeleteUser(user) - - _, err = th.App.UpdateNonSSOUserActive(user.Id, false) - assert.Equal(t, "api.user.update_active.no_deactivate_sso.app_error", err.Id) -} - func TestCreateProfileImage(t *testing.T) { b, err := CreateProfileImage("Corey Hulen", "eo1zkdr96pdj98pjmq8zy35wba", "luximbi.ttf") if err != nil { -- cgit v1.2.3-1-g7c22