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

@@ -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)
 }

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()
+	t.Run("basic tests", func(t *testing.T) {
-	defer th.TearDown()
+		th := Setup().InitBasic().InitSystemAdmin()
+		defer th.TearDown()
 
-	Client := th.Client
+		Client := th.Client
-	SystemAdminClient := th.SystemAdminClient
+		SystemAdminClient := th.SystemAdminClient
-	user := th.BasicUser
+		user := th.BasicUser
 
-	EnableUserDeactivation := th.App.Config().TeamSettings.EnableUserDeactivation
+		EnableUserDeactivation := th.App.Config().TeamSettings.EnableUserDeactivation
-	defer func() {
+		defer func() {
-		th.App.UpdateConfig(func(cfg *model.Config) { cfg.TeamSettings.EnableUserDeactivation = EnableUserDeactivation })
+			th.App.UpdateConfig(func(cfg *model.Config) { cfg.TeamSettings.EnableUserDeactivation = EnableUserDeactivation })
-	}()
+		}()
 
-	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.TeamSettings.EnableUserDeactivation = true })
+		th.App.UpdateConfig(func(cfg *model.Config) { *cfg.TeamSettings.EnableUserDeactivation = true })
-	pass, resp := Client.UpdateUserActive(user.Id, false)
+		pass, resp := Client.UpdateUserActive(user.Id, false)
-	CheckNoError(t, resp)
+		CheckNoError(t, resp)
 
-	if !pass {
+		if !pass {
-		t.Fatal("should have returned true")
+			t.Fatal("should have returned true")
-	}
+		}
 
-	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.TeamSettings.EnableUserDeactivation = false })
+		th.App.UpdateConfig(func(cfg *model.Config) { *cfg.TeamSettings.EnableUserDeactivation = false })
-	pass, resp = Client.UpdateUserActive(user.Id, false)
+		pass, resp = Client.UpdateUserActive(user.Id, false)
-	CheckUnauthorizedStatus(t, resp)
+		CheckUnauthorizedStatus(t, resp)
 
-	if pass {
+		if pass {
-		t.Fatal("should have returned false")
+			t.Fatal("should have returned false")
-	}
+		}
 
-	th.App.UpdateConfig(func(cfg *model.Config) { *cfg.TeamSettings.EnableUserDeactivation = true })
+		th.App.UpdateConfig(func(cfg *model.Config) { *cfg.TeamSettings.EnableUserDeactivation = true })
-	pass, resp = Client.UpdateUserActive(user.Id, false)
+		pass, resp = Client.UpdateUserActive(user.Id, false)
-	CheckUnauthorizedStatus(t, resp)
+		CheckUnauthorizedStatus(t, resp)
 
-	if pass {
+		if pass {
-		t.Fatal("should have returned false")
+			t.Fatal("should have returned false")
-	}
+		}
 
-	th.LoginBasic2()
+		th.LoginBasic2()
 
-	_, resp = Client.UpdateUserActive(user.Id, true)
+		_, resp = Client.UpdateUserActive(user.Id, true)
-	CheckForbiddenStatus(t, resp)
+		CheckForbiddenStatus(t, resp)
 
-	_, resp = Client.UpdateUserActive(GenerateTestId(), true)
+		_, resp = Client.UpdateUserActive(GenerateTestId(), true)
-	CheckForbiddenStatus(t, resp)
+		CheckForbiddenStatus(t, resp)
 
-	_, resp = Client.UpdateUserActive("junk", true)
+		_, resp = Client.UpdateUserActive("junk", true)
-	CheckBadRequestStatus(t, resp)
+		CheckBadRequestStatus(t, resp)
 
-	Client.Logout()
+		Client.Logout()
 
-	_, resp = Client.UpdateUserActive(user.Id, true)
+		_, resp = Client.UpdateUserActive(user.Id, true)
-	CheckUnauthorizedStatus(t, resp)
+		CheckUnauthorizedStatus(t, resp)
 
-	_, resp = SystemAdminClient.UpdateUserActive(user.Id, true)
+		_, resp = SystemAdminClient.UpdateUserActive(user.Id, true)
-	CheckNoError(t, resp)
+		CheckNoError(t, resp)
 
-	_, resp = SystemAdminClient.UpdateUserActive(user.Id, false)
+		_, resp = SystemAdminClient.UpdateUserActive(user.Id, false)
-	CheckNoError(t, resp)
+		CheckNoError(t, resp)
 
-	authData := model.NewId()
+		authData := model.NewId()
-	result := <-th.App.Srv.Store.User().UpdateAuthData(user.Id, "random", &authData, "", true)
+		result := <-th.App.Srv.Store.User().UpdateAuthData(user.Id, "random", &authData, "", true)
-	require.Nil(t, result.Err)
+		require.Nil(t, result.Err)
 
-	_, resp = SystemAdminClient.UpdateUserActive(user.Id, false)
+		_, resp = SystemAdminClient.UpdateUserActive(user.Id, false)
-	CheckNoError(t, resp)
+		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) {

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)

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 {