User limit enforcement (#26511)

* Added hard limits when creating user * Added check to user activation * Added missing check for licensed servers * Fix i18n * Fixed style order * Added a separate hard limit along with existing 10k user soft limit * For CI * Fixing flaky test, hopefully * Added tests
2024-03-21 19:41:53 +05:30
--- a/server/channels/api4/user.go
+++ b/server/channels/api4/user.go
@@ -1561,6 +1561,7 @@ func updateUserActive(c *Context, w http.ResponseWriter, r *http.Request) {

 	if _, err = c.App.UpdateActive(c.AppContext, user, active); err != nil {
 		c.Err = err
+		return
 	}

 	auditRec.Success()
--- a/server/channels/app/limits.go
+++ b/server/channels/app/limits.go
@@ -6,10 +6,15 @@ package app
 import (
 	"net/http"

+	"github.com/mattermost/mattermost/server/public/shared/mlog"
+
 	"github.com/mattermost/mattermost/server/public/model"
 )

-const maxUsersLimit = 10000
+const (
+	maxUsersLimit     = 10000
+	maxUsersHardLimit = 11000
+)

 func (a *App) GetUserLimits() (*model.UserLimits, *model.AppError) {
 	if !a.shouldShowUserLimits() {
@@ -18,12 +23,14 @@ func (a *App) GetUserLimits() (*model.UserLimits, *model.AppError) {

 	activeUserCount, appErr := a.Srv().Store().User().Count(model.UserCountOptions{})
 	if appErr != nil {
+		mlog.Error("Failed to get active user count from database", mlog.String("error", appErr.Error()))
 		return nil, model.NewAppError("GetUsersLimits", "app.limits.get_user_limits.user_count.store_error", nil, "", http.StatusInternalServerError).Wrap(appErr)
 	}

 	return &model.UserLimits{
-		ActiveUserCount: activeUserCount,
-		MaxUsersLimit:   maxUsersLimit,
+		ActiveUserCount:   activeUserCount,
+		MaxUsersLimit:     maxUsersLimit,
+		MaxUsersHardLimit: maxUsersHardLimit,
 	}, nil
 }

@@ -34,3 +41,12 @@ func (a *App) shouldShowUserLimits() bool {

 	return a.License() == nil
 }
+
+func (a *App) isHardUserLimitExceeded() (bool, *model.AppError) {
+	userLimits, appErr := a.GetUserLimits()
+	if appErr != nil {
+		return false, appErr
+	}
+
+	return userLimits.ActiveUserCount > userLimits.MaxUsersHardLimit, appErr
+}
--- a/server/channels/app/plugin_hooks_test.go
+++ b/server/channels/app/plugin_hooks_test.go
@@ -1985,11 +1985,21 @@ func TestUserHasJoinedChannel(t *testing.T) {
 		require.Nil(t, appErr)

 		assert.EventuallyWithT(t, func(t *assert.CollectT) {
-			posts, appErr := th.App.GetPosts(channel.Id, 0, 1)
+			posts, appErr := th.App.GetPosts(channel.Id, 0, 30)

 			require.Nil(t, appErr)
 			assert.True(t, len(posts.Order) > 0)
-			assert.Equal(t, fmt.Sprintf("Test: User %s joined %s", user2.Id, channel.Id), posts.Posts[posts.Order[0]].Message)
+
+			found := false
+			for _, post := range posts.Posts {
+				if post.Message == fmt.Sprintf("Test: User %s joined %s", user2.Id, channel.Id) {
+					found = true
+				}
+			}
+
+			if !found {
+				assert.Fail(t, "Couldn't find user joined channel hook message post")
+			}
 		}, 5*time.Second, 100*time.Millisecond)
 	})

--- a/server/channels/app/user.go
+++ b/server/channels/app/user.go
@@ -230,6 +230,15 @@ func (a *App) CreateGuest(c request.CTX, user *model.User) (*model.User, *model.
 }

 func (a *App) createUserOrGuest(c request.CTX, user *model.User, guest bool) (*model.User, *model.AppError) {
+	exceeded, limitErr := a.isHardUserLimitExceeded()
+	if limitErr != nil {
+		return nil, limitErr
+	}
+
+	if exceeded {
+		return nil, model.NewAppError("createUserOrGuest", "api.user.create_user.user_limits.exceeded", nil, "", http.StatusBadRequest)
+	}
+
 	if err := a.isUniqueToGroupNames(user.Username); err != nil {
 		err.Where = "createUserOrGuest"
 		return nil, err
@@ -324,11 +333,11 @@ func (a *App) createUserOrGuest(c request.CTX, user *model.User, guest bool) (*m
 		}(ruser.Id)
 	}

-	userLimits, appErr := a.GetUserLimits()
-	if appErr != nil {
+	userLimits, limitErr := a.GetUserLimits()
+	if limitErr != nil {
 		// we don't want to break the create user flow just because of this.
 		// So, we log the error, not return
-		mlog.Error("Error fetching user limits in createUserOrGuest", mlog.Err(appErr))
+		mlog.Error("Error fetching user limits in createUserOrGuest", mlog.Err(limitErr))
 	} else {
 		if userLimits.ActiveUserCount > userLimits.MaxUsersLimit {
 			mlog.Warn("ERROR_SAFETY_LIMITS_EXCEEDED: Created user exceeds the total activated users limit.", mlog.Int("user_limit", userLimits.MaxUsersLimit))
@@ -1003,6 +1012,17 @@ func (a *App) invalidateUserChannelMembersCaches(c request.CTX, userID string) *
 }

 func (a *App) UpdateActive(c request.CTX, user *model.User, active bool) (*model.User, *model.AppError) {
+	if active {
+		exceeded, appErr := a.isHardUserLimitExceeded()
+		if appErr != nil {
+			return nil, appErr
+		}
+
+		if exceeded {
+			return nil, model.NewAppError("UpdateActive", "app.user.update_active.user_limit.exceeded", nil, "", http.StatusBadRequest)
+		}
+	}
+
 	user.UpdateAt = model.GetMillis()
 	if active {
 		user.DeleteAt = 0
--- a/server/channels/app/user_test.go
+++ b/server/channels/app/user_test.go
@@ -2008,3 +2008,160 @@ func TestGetUsersForReporting(t *testing.T) {
 		require.NotNil(t, userReports)
 	})
 }
+
+func TestCreateUserOrGuest(t *testing.T) {
+	t.Run("base case - you can create a user", func(t *testing.T) {
+		th := Setup(t)
+		defer th.TearDown()
+
+		user := &model.User{
+			Email:         "TestCreateUserOrGuest@example.com",
+			Username:      "username_123",
+			Nickname:      "nn_username_123",
+			Password:      "Password1",
+			EmailVerified: true,
+		}
+		createdUser, appErr := th.App.createUserOrGuest(th.Context, user, false)
+		require.Nil(t, appErr)
+		require.Equal(t, "username_123", createdUser.Username)
+	})
+
+	t.Run("cannot create user when user count has exceeded the permissible limit", func(t *testing.T) {
+		th := SetupWithStoreMock(t)
+		defer th.TearDown()
+
+		mockUserStore := storemocks.UserStore{}
+		mockUserStore.On("Count", mock.Anything).Return(int64(12000), nil)
+
+		mockStore := th.App.Srv().Store().(*storemocks.Store)
+		mockStore.On("User").Return(&mockUserStore)
+
+		user := &model.User{
+			Email:         "TestCreateUserOrGuest@example.com",
+			Username:      "username_123",
+			Nickname:      "nn_username_123",
+			Password:      "Password1",
+			EmailVerified: true,
+		}
+		createdUser, appErr := th.App.createUserOrGuest(th.Context, user, false)
+		require.NotNil(t, appErr)
+		require.Nil(t, createdUser)
+	})
+
+	t.Run("can create user when server is exactly on limit", func(t *testing.T) {
+		th := SetupWithStoreMock(t)
+		defer th.TearDown()
+
+		id := NewTestId()
+		userCreationMocks(t, th, id, 11000)
+
+		user := &model.User{
+			Email:         "TestCreateUserOrGuest@example.com",
+			Username:      "username_123",
+			Nickname:      "nn_username_123",
+			Password:      "Password1",
+			EmailVerified: true,
+		}
+		createdUser, appErr := th.App.createUserOrGuest(th.Context, user, false)
+		require.Nil(t, appErr)
+		require.Equal(t, "username_123", createdUser.Username)
+	})
+
+	t.Run("licensed server can create user when server is OVER limit", func(t *testing.T) {
+		th := SetupWithStoreMock(t)
+		defer th.TearDown()
+
+		id := NewTestId()
+		userCreationMocks(t, th, id, 20000)
+
+		user := &model.User{
+			Email:         "TestCreateUserOrGuest@example.com",
+			Username:      "username_123",
+			Nickname:      "nn_username_123",
+			Password:      "Password1",
+			EmailVerified: true,
+		}
+
+		th.App.Srv().SetLicense(model.NewTestLicense(""))
+		createdUser, appErr := th.App.createUserOrGuest(th.Context, user, false)
+		require.Nil(t, appErr)
+		require.Equal(t, "username_123", createdUser.Username)
+	})
+
+	t.Run("licensed server can create user when server is UNDER limit", func(t *testing.T) {
+		th := SetupWithStoreMock(t)
+		defer th.TearDown()
+
+		id := NewTestId()
+		userCreationMocks(t, th, id, 10)
+
+		user := &model.User{
+			Email:         "TestCreateUserOrGuest@example.com",
+			Username:      "username_123",
+			Nickname:      "nn_username_123",
+			Password:      "Password1",
+			EmailVerified: true,
+		}
+
+		th.App.Srv().SetLicense(model.NewTestLicense(""))
+		createdUser, appErr := th.App.createUserOrGuest(th.Context, user, false)
+		require.Nil(t, appErr)
+		require.Equal(t, "username_123", createdUser.Username)
+	})
+}
+
+func userCreationMocks(t *testing.T, th *TestHelper, userID string, activeUserCount int64) {
+	mockUserStore := storemocks.UserStore{}
+	mockUserStore.On("Count", mock.Anything).Return(activeUserCount, nil)
+	mockUserStore.On("IsEmpty", mock.Anything).Return(false, nil)
+	mockUserStore.On("VerifyEmail", mock.Anything, "TestCreateUserOrGuest@example.com").Return("", nil)
+	mockUserStore.On("InvalidateProfilesInChannelCacheByUser", mock.Anything).Return()
+	mockUserStore.On("InvalidateProfileCacheForUser", mock.Anything).Return()
+	mockUserStore.On("Save", mock.Anything, mock.Anything).Return(&model.User{
+		Id:            userID,
+		Email:         "TestCreateUserOrGuest@example.com",
+		Username:      "username_123",
+		Nickname:      "nn_username_123",
+		Password:      "Password1",
+		EmailVerified: true,
+	}, nil)
+
+	mockUserStore.On("Get", mock.Anything, userID).Return(&model.User{
+		Id:            userID,
+		Email:         "TestCreateUserOrGuest@example.com",
+		Username:      "username_123",
+		Nickname:      "nn_username_123",
+		Password:      "Password1",
+		EmailVerified: true,
+	}, nil)
+
+	mockGroupStore := storemocks.GroupStore{}
+	mockGroupStore.On("GetByName", "username_123", mock.Anything).Return(nil, nil)
+
+	mockChannelStore := storemocks.ChannelStore{}
+	mockChannelStore.On("InvalidateAllChannelMembersForUser", mock.Anything).Return()
+
+	mockPreferencesStore := storemocks.PreferenceStore{}
+	mockPreferencesStore.On("Save", mock.Anything).Return(nil)
+
+	mockProductNoticeStore := storemocks.ProductNoticesStore{}
+	mockProductNoticeStore.On("View", userID, mock.Anything).Return(nil)
+
+	mockStore := th.App.Srv().Store().(*storemocks.Store)
+	mockStore.On("User").Return(&mockUserStore)
+	mockStore.On("Group").Return(&mockGroupStore)
+	mockStore.On("Channel").Return(&mockChannelStore)
+	mockStore.On("Preference").Return(&mockPreferencesStore)
+	mockStore.On("ProductNotices").Return(&mockProductNoticeStore)
+
+	var err error
+	th.App.ch.srv.userService, err = users.New(users.ServiceConfig{
+		UserStore:    &mockUserStore,
+		SessionStore: &storemocks.SessionStore{},
+		OAuthStore:   &storemocks.OAuthStore{},
+		ConfigFn:     th.App.ch.srv.platform.Config,
+		LicenseFn:    th.App.ch.srv.License,
+	})
+
+	require.NoError(t, err)
+}
--- a/server/i18n/en.json
+++ b/server/i18n/en.json
@@ -4178,6 +4178,10 @@
    "id": "api.user.create_user.signup_link_invalid.app_error",
    "translation": "The signup link does not appear to be valid."
  },
+  {
+    "id": "api.user.create_user.user_limits.exceeded",
+    "translation": "Can't create user. Server exceeds safe user limit. Contact your system administrator."
+  },
  {
    "id": "api.user.delete_channel.not_enabled.app_error",
    "translation": "Permanent channel deletion feature is not enabled. Please contact your System Administrator."
@@ -7178,6 +7182,10 @@
    "id": "app.user.update.finding.app_error",
    "translation": "We encountered an error finding the account."
  },
+  {
+    "id": "app.user.update_active.user_limit.exceeded",
+    "translation": "Can't activate user. Server exceeds safe user limit. Contact your system administrator. Error code: ERROR_SAFETY_LIMITS_EXCEEDED."
+  },
  {
    "id": "app.user.update_active_for_multiple_users.updating.app_error",
    "translation": "Unable to deactivate guests."
--- a/server/public/model/limits.go
+++ b/server/public/model/limits.go
@@ -4,6 +4,7 @@
 package model

 type UserLimits struct {
-	MaxUsersLimit   int64 `json:"maxUsersLimit"`   // max number of users allowed
-	ActiveUserCount int64 `json:"activeUserCount"` // actual number of active users on server. Active = non deleted
+	MaxUsersLimit     int64 `json:"maxUsersLimit"`     // soft limit for max number of users.
+	MaxUsersHardLimit int64 `json:"maxUsersHardLimit"` // hard limit for max number of active users.
+	ActiveUserCount   int64 `json:"activeUserCount"`   // actual number of active users on server. Active = non deleted
 }