From 3e198e4d5518c09836ea4c8e7ac7201c83aab67b Mon Sep 17 00:00:00 2001 From: Mattermost Build Date: Thu, 25 Jun 2026 08:05:18 +0200 Subject: [PATCH] [MM-69183] Gate expensive user/guest count queries behind admin check in getServerLimits (#36999) (#37161) Automatic Merge --- server/channels/api4/limits.go | 5 +- server/channels/app/limits.go | 17 +++++- server/channels/app/limits_test.go | 84 ++++++++++++++++++------------ server/channels/app/user.go | 4 +- 4 files changed, 73 insertions(+), 37 deletions(-) diff --git a/server/channels/api4/limits.go b/server/channels/api4/limits.go index 2306ace05fc..3338425d955 100644 --- a/server/channels/api4/limits.go +++ b/server/channels/api4/limits.go @@ -19,7 +19,10 @@ func (api *API) InitLimits() { func getServerLimits(c *Context, w http.ResponseWriter, r *http.Request) { isAdmin := c.IsSystemAdmin() && c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionSysconsoleReadUserManagementUsers) - serverLimits, err := c.App.GetServerLimits() + // Only admins receive (and need) the user/guest counts, so only compute them for + // admins. This keeps the expensive count queries off the per-login/per-refresh hot + // path that non-admin clients hit via loadMe()/loadConfigAndMe(). + serverLimits, err := c.App.GetServerLimits(isAdmin) if err != nil { c.Err = err return diff --git a/server/channels/app/limits.go b/server/channels/app/limits.go index 4c88c1f0490..45448e4b2b5 100644 --- a/server/channels/app/limits.go +++ b/server/channels/app/limits.go @@ -14,7 +14,14 @@ const ( maxUsersHardLimit = 250 ) -func (a *App) GetServerLimits() (*model.ServerLimits, *model.AppError) { +// GetServerLimits returns the server's seat/post-history limits. The license-derived +// limit fields and post-history fields are always computed (they are cheap and needed +// by all users). The active user and single-channel guest counts are only computed when +// includeUserCounts is true, because those queries are expensive and the counts are only +// consumed by admin-gated UI and internal seat-limit checks. Callers that do not need the +// counts (e.g. non-admin API requests) should pass false to keep the expensive queries off +// the hot path. +func (a *App) GetServerLimits(includeUserCounts bool) (*model.ServerLimits, *model.AppError) { limits := &model.ServerLimits{} license := a.License() @@ -47,6 +54,12 @@ func (a *App) GetServerLimits() (*model.ServerLimits, *model.AppError) { limits.LastAccessiblePostTime = lastAccessibleTime } + // The user/guest count queries are expensive (the single-channel guest count is a + // full ChannelMembers scan). Only run them when the caller actually needs the counts. + if !includeUserCounts { + return limits, nil + } + activeUserCount, appErr := a.Srv().Store().User().Count(model.UserCountOptions{}) if appErr != nil { return nil, model.NewAppError("GetServerLimits", "app.limits.get_app_limits.user_count.store_error", nil, "", http.StatusInternalServerError).Wrap(appErr) @@ -99,7 +112,7 @@ func (a *App) GetPostHistoryLimit() int64 { } func (a *App) isAtUserLimit() (bool, *model.AppError) { - userLimits, appErr := a.GetServerLimits() + userLimits, appErr := a.GetServerLimits(true) if appErr != nil { return false, appErr } diff --git a/server/channels/app/limits_test.go b/server/channels/app/limits_test.go index b9b181fa298..2127ac3df83 100644 --- a/server/channels/app/limits_test.go +++ b/server/channels/app/limits_test.go @@ -22,7 +22,7 @@ func TestGetServerLimits(t *testing.T) { th.App.Srv().SetLicense(nil) - serverLimits, appErr := th.App.GetServerLimits() + serverLimits, appErr := th.App.GetServerLimits(true) require.Nil(t, appErr) // InitBasic creates 3 users by default @@ -31,25 +31,45 @@ func TestGetServerLimits(t *testing.T) { require.Equal(t, int64(250), serverLimits.MaxUsersHardLimit) }) + t.Run("user counts are skipped when includeUserCounts is false", func(t *testing.T) { + th := Setup(t).InitBasic(t) + + th.App.Srv().SetLicense(nil) + + // With counts included we get the real active user count (InitBasic creates 3 users). + withCounts, appErr := th.App.GetServerLimits(true) + require.Nil(t, appErr) + require.Equal(t, int64(3), withCounts.ActiveUserCount) + + // Without counts the expensive count queries are skipped, so the count is zero even + // though users exist. The cheap license-derived limits are still returned. + withoutCounts, appErr := th.App.GetServerLimits(false) + require.Nil(t, appErr) + require.Equal(t, int64(0), withoutCounts.ActiveUserCount) + require.Equal(t, int64(0), withoutCounts.SingleChannelGuestCount) + require.Equal(t, int64(200), withoutCounts.MaxUsersLimit) + require.Equal(t, int64(250), withoutCounts.MaxUsersHardLimit) + }) + t.Run("user count should increase on creating new user and decrease on permanently deleting", func(t *testing.T) { th := Setup(t).InitBasic(t) th.App.Srv().SetLicense(nil) - serverLimits, appErr := th.App.GetServerLimits() + serverLimits, appErr := th.App.GetServerLimits(true) require.Nil(t, appErr) require.Equal(t, int64(3), serverLimits.ActiveUserCount) // now we create a new user newUser := th.CreateUser(t) - serverLimits, appErr = th.App.GetServerLimits() + serverLimits, appErr = th.App.GetServerLimits(true) require.Nil(t, appErr) require.Equal(t, int64(4), serverLimits.ActiveUserCount) // now we'll delete the user _ = th.App.PermanentDeleteUser(th.Context, newUser) - serverLimits, appErr = th.App.GetServerLimits() + serverLimits, appErr = th.App.GetServerLimits(true) require.Nil(t, appErr) require.Equal(t, int64(3), serverLimits.ActiveUserCount) }) @@ -59,20 +79,20 @@ func TestGetServerLimits(t *testing.T) { th.App.Srv().SetLicense(nil) - serverLimits, appErr := th.App.GetServerLimits() + serverLimits, appErr := th.App.GetServerLimits(true) require.Nil(t, appErr) require.Equal(t, int64(3), serverLimits.ActiveUserCount) // now we create a new user newGuestUser := th.CreateGuest(t) - serverLimits, appErr = th.App.GetServerLimits() + serverLimits, appErr = th.App.GetServerLimits(true) require.Nil(t, appErr) require.Equal(t, int64(4), serverLimits.ActiveUserCount) // now we'll delete the user _ = th.App.PermanentDeleteUser(th.Context, newGuestUser) - serverLimits, appErr = th.App.GetServerLimits() + serverLimits, appErr = th.App.GetServerLimits(true) require.Nil(t, appErr) require.Equal(t, int64(3), serverLimits.ActiveUserCount) }) @@ -82,21 +102,21 @@ func TestGetServerLimits(t *testing.T) { th.App.Srv().SetLicense(nil) - serverLimits, appErr := th.App.GetServerLimits() + serverLimits, appErr := th.App.GetServerLimits(true) require.Nil(t, appErr) require.Equal(t, int64(3), serverLimits.ActiveUserCount) // now we create a new user newUser := th.CreateUser(t) - serverLimits, appErr = th.App.GetServerLimits() + serverLimits, appErr = th.App.GetServerLimits(true) require.Nil(t, appErr) require.Equal(t, int64(4), serverLimits.ActiveUserCount) // now we'll delete the user _, appErr = th.App.UpdateActive(th.Context, newUser, false) require.Nil(t, appErr) - serverLimits, appErr = th.App.GetServerLimits() + serverLimits, appErr = th.App.GetServerLimits(true) require.Nil(t, appErr) require.Equal(t, int64(3), serverLimits.ActiveUserCount) }) @@ -106,21 +126,21 @@ func TestGetServerLimits(t *testing.T) { th.App.Srv().SetLicense(nil) - serverLimits, appErr := th.App.GetServerLimits() + serverLimits, appErr := th.App.GetServerLimits(true) require.Nil(t, appErr) require.Equal(t, int64(3), serverLimits.ActiveUserCount) // now we create a new user newGuestUser := th.CreateGuest(t) - serverLimits, appErr = th.App.GetServerLimits() + serverLimits, appErr = th.App.GetServerLimits(true) require.Nil(t, appErr) require.Equal(t, int64(4), serverLimits.ActiveUserCount) // now we'll delete the user _, appErr = th.App.UpdateActive(th.Context, newGuestUser, false) require.Nil(t, appErr) - serverLimits, appErr = th.App.GetServerLimits() + serverLimits, appErr = th.App.GetServerLimits(true) require.Nil(t, appErr) require.Equal(t, int64(3), serverLimits.ActiveUserCount) }) @@ -130,20 +150,20 @@ func TestGetServerLimits(t *testing.T) { th.App.Srv().SetLicense(nil) - serverLimits, appErr := th.App.GetServerLimits() + serverLimits, appErr := th.App.GetServerLimits(true) require.Nil(t, appErr) require.Equal(t, int64(3), serverLimits.ActiveUserCount) // now we create a new bot newBot := th.CreateBot(t) - serverLimits, appErr = th.App.GetServerLimits() + serverLimits, appErr = th.App.GetServerLimits(true) require.Nil(t, appErr) require.Equal(t, int64(3), serverLimits.ActiveUserCount) // now we'll delete the bot _ = th.App.PermanentDeleteBot(th.Context, newBot.UserId) - serverLimits, appErr = th.App.GetServerLimits() + serverLimits, appErr = th.App.GetServerLimits(true) require.Nil(t, appErr) require.Equal(t, int64(3), serverLimits.ActiveUserCount) }) @@ -155,7 +175,7 @@ func TestGetServerLimits(t *testing.T) { license.IsSeatCountEnforced = false th.App.Srv().SetLicense(license) - serverLimits, appErr := th.App.GetServerLimits() + serverLimits, appErr := th.App.GetServerLimits(true) require.Nil(t, appErr) require.Greater(t, serverLimits.ActiveUserCount, int64(0)) @@ -174,7 +194,7 @@ func TestGetServerLimits(t *testing.T) { license.ExtraUsers = &extraUsers th.App.Srv().SetLicense(license) - serverLimits, appErr := th.App.GetServerLimits() + serverLimits, appErr := th.App.GetServerLimits(true) require.Nil(t, appErr) // InitBasic creates 3 users by default @@ -193,7 +213,7 @@ func TestGetServerLimits(t *testing.T) { license.ExtraUsers = nil // Not configured th.App.Srv().SetLicense(license) - serverLimits, appErr := th.App.GetServerLimits() + serverLimits, appErr := th.App.GetServerLimits(true) require.Nil(t, appErr) // InitBasic creates 3 users by default @@ -213,7 +233,7 @@ func TestGetServerLimits(t *testing.T) { license.ExtraUsers = &extraUsers th.App.Srv().SetLicense(license) - serverLimits, appErr := th.App.GetServerLimits() + serverLimits, appErr := th.App.GetServerLimits(true) require.Nil(t, appErr) // InitBasic creates 3 users by default @@ -230,7 +250,7 @@ func TestGetServerLimits(t *testing.T) { license.Features.Users = nil th.App.Srv().SetLicense(license) - serverLimits, appErr := th.App.GetServerLimits() + serverLimits, appErr := th.App.GetServerLimits(true) require.Nil(t, appErr) require.Greater(t, serverLimits.ActiveUserCount, int64(0)) @@ -247,7 +267,7 @@ func TestGetServerLimits(t *testing.T) { license.Features.Users = &userLimit th.App.Srv().SetLicense(license) - serverLimits, appErr := th.App.GetServerLimits() + serverLimits, appErr := th.App.GetServerLimits(true) require.Nil(t, appErr) require.Greater(t, serverLimits.ActiveUserCount, int64(0)) @@ -510,7 +530,7 @@ func TestExtraUsersBehavior(t *testing.T) { license.ExtraUsers = tt.extraUsers th.App.Srv().SetLicense(license) - serverLimits, appErr := th.App.GetServerLimits() + serverLimits, appErr := th.App.GetServerLimits(true) require.Nil(t, appErr) require.Equal(t, tt.expectedBaseLimit, serverLimits.MaxUsersLimit) @@ -524,7 +544,7 @@ func TestExtraUsersBehavior(t *testing.T) { th.App.Srv().SetLicense(nil) - serverLimits, appErr := th.App.GetServerLimits() + serverLimits, appErr := th.App.GetServerLimits(true) require.Nil(t, appErr) // Unlicensed servers use hard-coded limits without extra users @@ -541,7 +561,7 @@ func TestGetServerLimitsWithPostHistory(t *testing.T) { th.App.Srv().SetLicense(nil) - serverLimits, appErr := th.App.GetServerLimits() + serverLimits, appErr := th.App.GetServerLimits(true) require.Nil(t, appErr) // Unlicensed servers should have no post history limits @@ -556,7 +576,7 @@ func TestGetServerLimitsWithPostHistory(t *testing.T) { license.Limits = nil // No limits configured th.App.Srv().SetLicense(license) - serverLimits, appErr := th.App.GetServerLimits() + serverLimits, appErr := th.App.GetServerLimits(true) require.Nil(t, appErr) // Should have no post history limits when Limits is nil @@ -573,7 +593,7 @@ func TestGetServerLimitsWithPostHistory(t *testing.T) { } th.App.Srv().SetLicense(license) - serverLimits, appErr := th.App.GetServerLimits() + serverLimits, appErr := th.App.GetServerLimits(true) require.Nil(t, appErr) // Should have no post history limits when PostHistory is 0 @@ -606,7 +626,7 @@ func TestGetServerLimitsWithPostHistory(t *testing.T) { } th.App.Srv().SetLicense(license) - serverLimits, appErr := th.App.GetServerLimits() + serverLimits, appErr := th.App.GetServerLimits(true) require.Nil(t, appErr) // Should have proper post history limits set @@ -637,7 +657,7 @@ func TestGetServerLimitsWithPostHistory(t *testing.T) { } th.App.Srv().SetLicense(license) - _, appErr := th.App.GetServerLimits() + _, appErr := th.App.GetServerLimits(true) require.NotNil(t, appErr) require.Contains(t, appErr.Message, "Unable to find the system variable") }) @@ -664,7 +684,7 @@ func TestGetServerLimitsWithPostHistory(t *testing.T) { } th.App.Srv().SetLicense(license) - serverLimits, appErr := th.App.GetServerLimits() + serverLimits, appErr := th.App.GetServerLimits(true) require.Nil(t, appErr) // Should have post history limit set but LastAccessiblePostTime should be 0 (all posts accessible) @@ -745,7 +765,7 @@ func TestGetServerLimitsWithSingleChannelGuests(t *testing.T) { th.LinkUserToTeam(t, guest, th.BasicTeam) th.AddUserToChannel(t, guest, th.BasicChannel) - serverLimits, appErr := th.App.GetServerLimits() + serverLimits, appErr := th.App.GetServerLimits(true) require.Nil(t, appErr) require.Greater(t, serverLimits.SingleChannelGuestCount, int64(0)) @@ -761,7 +781,7 @@ func TestGetServerLimitsWithSingleChannelGuests(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.GuestAccountsSettings.Enable = true }) - serverLimits, appErr := th.App.GetServerLimits() + serverLimits, appErr := th.App.GetServerLimits(true) require.Nil(t, appErr) require.Equal(t, int64(0), serverLimits.SingleChannelGuestCount) diff --git a/server/channels/app/user.go b/server/channels/app/user.go index ff33087e07c..ba50698354f 100644 --- a/server/channels/app/user.go +++ b/server/channels/app/user.go @@ -408,7 +408,7 @@ func (a *App) createUserOrGuest(rctx request.CTX, user *model.User, guest bool) }, plugin.UserHasBeenCreatedID) }) - userLimits, limitErr := a.GetServerLimits() + userLimits, limitErr := a.GetServerLimits(true) if limitErr != nil { // we don't want to break the create user flow just because of this. // So, we log the error, not return @@ -1225,7 +1225,7 @@ func (a *App) UpdateActive(rctx request.CTX, user *model.User, active bool) (*mo } if active { - userLimits, appErr := a.GetServerLimits() + userLimits, appErr := a.GetServerLimits(true) if appErr != nil { rctx.Logger().Error("Error fetching user limits in UpdateActive", mlog.Err(appErr)) } else {