From b02d634916b2e487a0ad8c0f8fda5c14985fc4c2 Mon Sep 17 00:00:00 2001 From: Harshil Sharma <18575143+harshilsharma63@users.noreply.github.com> Date: Thu, 21 Mar 2024 19:41:53 +0530 Subject: [PATCH] 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 --- server/channels/api4/user.go | 1 + server/channels/app/limits.go | 22 ++- server/channels/app/plugin_hooks_test.go | 14 +- server/channels/app/user.go | 26 ++- server/channels/app/user_test.go | 157 ++++++++++++++++++ server/i18n/en.json | 8 + server/public/model/limits.go | 5 +- .../system_users/system_users.scss | 5 + 8 files changed, 228 insertions(+), 10 deletions(-) diff --git a/server/channels/api4/user.go b/server/channels/api4/user.go index 472d0c0100..0edaa7cbd0 100644 --- 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() diff --git a/server/channels/app/limits.go b/server/channels/app/limits.go index 09eb7d54d0..42e26d7190 100644 --- 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 +} diff --git a/server/channels/app/plugin_hooks_test.go b/server/channels/app/plugin_hooks_test.go index 8bb3947ac0..0a4017e7ec 100644 --- 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) }) diff --git a/server/channels/app/user.go b/server/channels/app/user.go index 3c8e6bc834..f20780ddd7 100644 --- 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 diff --git a/server/channels/app/user_test.go b/server/channels/app/user_test.go index e6cdbf5b1e..71867e4df2 100644 --- 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) +} diff --git a/server/i18n/en.json b/server/i18n/en.json index ab7f3ce556..62bdd46795 100644 --- 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." diff --git a/server/public/model/limits.go b/server/public/model/limits.go index b8d55c2a4e..97ba516142 100644 --- 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 } diff --git a/webapp/channels/src/components/admin_console/system_users/system_users.scss b/webapp/channels/src/components/admin_console/system_users/system_users.scss index 573084baf3..f6fe780b21 100644 --- a/webapp/channels/src/components/admin_console/system_users/system_users.scss +++ b/webapp/channels/src/components/admin_console/system_users/system_users.scss @@ -60,10 +60,15 @@ table.systemUsersTable { } .error { + max-width: 300px; align-self: start; grid-area: error; @include textElipsis; + + // this need to be here, after textElipsis because + // we need to override white-space property coming from the mixing + white-space: normal; } }