From 5ce5ea93f4f6ec6da086673af1dbb31f25fc46d6 Mon Sep 17 00:00:00 2001 From: Allan Guwatudde Date: Mon, 7 Jun 2021 17:38:33 +0300 Subject: [PATCH] [MM-36198] - Activating a user in System Console does not respect cloud user limits (#17719) * [MM-36198] - Activating a user in System Console does not respect cloud user limits * feedback impl * improve error check --- api4/cloud.go | 24 ++------------- api4/user.go | 12 ++++++++ api4/user_test.go | 42 +++++++++++++++++++++++++ app/app_iface.go | 2 ++ app/cloud.go | 46 ++++++++++++++++++++++++++++ app/opentracing/opentracing_layer.go | 44 ++++++++++++++++++++++++++ i18n/en.json | 8 +++++ 7 files changed, 157 insertions(+), 21 deletions(-) diff --git a/api4/cloud.go b/api4/cloud.go index 133c38ae04..dbf3237b0c 100644 --- a/api4/cloud.go +++ b/api4/cloud.go @@ -113,31 +113,13 @@ func changeSubscription(c *Context, w http.ResponseWriter, r *http.Request) { } func getSubscriptionStats(c *Context, w http.ResponseWriter, r *http.Request) { - if c.App.Srv().License() == nil || !*c.App.Srv().License().Features.Cloud { - c.Err = model.NewAppError("Api4.getSubscriptionStats", "api.cloud.license_error", nil, "", http.StatusInternalServerError) - return - } - - subscription, appErr := c.App.Cloud().GetSubscription("") - - if appErr != nil { - c.Err = model.NewAppError("Api4.getSubscriptionStats", "api.cloud.request_error", nil, appErr.Error(), http.StatusInternalServerError) - return - } - - count, err := c.App.Srv().Store.User().Count(model.UserCountOptions{}) + s, err := c.App.GetSubscriptionStats() if err != nil { - c.Err = model.NewAppError("Api4.getSubscriptionStats", "app.user.get_total_users_count.app_error", nil, err.Error(), http.StatusInternalServerError) + c.Err = err return } - cloudUserLimit := *c.App.Config().ExperimentalSettings.CloudUserLimit - s := cloudUserLimit - count - - stats, _ := json.Marshal(model.SubscriptionStats{ - RemainingSeats: int(s), - IsPaidTier: subscription.IsPaidTier, - }) + stats, _ := json.Marshal(s) w.Write([]byte(string(stats))) } diff --git a/api4/user.go b/api4/user.go index bd97a5b7dc..c04f9ddba4 100644 --- a/api4/user.go +++ b/api4/user.go @@ -1377,6 +1377,18 @@ func updateUserActive(c *Context, w http.ResponseWriter, r *http.Request) { return } + // if non cloud instances, isOverLimit is false and no error + isAtLimit, err := c.App.CheckCloudAccountAtLimit() + if err != nil { + c.Err = model.NewAppError("updateUserActive", "api.user.update_active.cloud_at_limit_check_error", nil, "userId="+c.Params.UserId, http.StatusInternalServerError) + return + } + + if active && isAtLimit { + c.Err = model.NewAppError("updateUserActive", "api.user.update_active.cloud_at_or_over_limit_check_overcapacity", nil, "userId="+c.Params.UserId, http.StatusBadRequest) + return + } + if _, err = c.App.UpdateActive(c.AppContext, user, active); err != nil { c.Err = err } diff --git a/api4/user_test.go b/api4/user_test.go index 2dd087d4db..02ab78f140 100644 --- a/api4/user_test.go +++ b/api4/user_test.go @@ -2104,6 +2104,48 @@ func assertWebsocketEventUserUpdatedWithEmail(t *testing.T, client *model.WebSoc } func TestUpdateUserActive(t *testing.T) { + t.Run("not activating more users when cloud license users at limit", func(t *testing.T) { + // create 5 active users + th := Setup(t).InitBasic() + defer th.TearDown() + + cloudMock := &mocks.CloudInterface{} + cloudMock.Mock.On( + "GetSubscription", mock.Anything, + ).Return(&model.Subscription{ + ID: "MySubscriptionID", + CustomerID: "MyCustomer", + ProductID: "SomeProductId", + AddOns: []string{}, + StartAt: 1000000000, + EndAt: 2000000000, + CreateAt: 1000000000, + Seats: 100, + DNS: "some.dns.server", + IsPaidTier: "false", + }, nil) + + th.App.Srv().SetLicense(model.NewTestLicense("cloud")) + th.App.Srv().Cloud = cloudMock + + user := th.BasicUser + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.TeamSettings.EnableUserDeactivation = true + *cfg.ExperimentalSettings.CloudUserLimit = 4 + }) + + // deactivate 5th user, now we have 4 active users and are at limit + pass, resp := th.SystemAdminClient.UpdateUserActive(user.Id, false) + CheckNoError(t, resp) + require.True(t, pass) + + // try and reactivate 5th user, not allowed because it exceeds the set cloud user limit + pass, resp = th.SystemAdminClient.UpdateUserActive(user.Id, true) + CheckBadRequestStatus(t, resp) + require.False(t, pass) + require.Equal(t, resp.Error.Message, "Unable to activate more users as the cloud account is over capacity.") + }) t.Run("basic tests", func(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() diff --git a/app/app_iface.go b/app/app_iface.go index 8f271a2ab5..c5f2d953c0 100644 --- a/app/app_iface.go +++ b/app/app_iface.go @@ -428,6 +428,7 @@ type AppIface interface { ChannelMembersToRemove(teamID *string) ([]*model.ChannelMember, *model.AppError) CheckAndSendUserLimitWarningEmails(c *request.Context) *model.AppError CheckCanInviteToSharedChannel(channelId string) error + CheckCloudAccountAtLimit() (bool, *model.AppError) CheckForClientSideCert(r *http.Request) (string, string, string) CheckIntegrity() <-chan model.IntegrityCheckResult CheckMandatoryS3Fields(settings *model.FileSettings) *model.AppError @@ -734,6 +735,7 @@ type AppIface interface { GetStatus(userID string) (*model.Status, *model.AppError) GetStatusFromCache(userID string) *model.Status GetStatusesByIds(userIDs []string) (map[string]interface{}, *model.AppError) + GetSubscriptionStats() (*model.SubscriptionStats, *model.AppError) GetTeam(teamID string) (*model.Team, *model.AppError) GetTeamByInviteId(inviteId string) (*model.Team, *model.AppError) GetTeamByName(name string) (*model.Team, *model.AppError) diff --git a/app/cloud.go b/app/cloud.go index fca316b5f7..37bcf67f7e 100644 --- a/app/cloud.go +++ b/app/cloud.go @@ -79,6 +79,52 @@ func (a *App) SendAdminUpgradeRequestEmail(username string, subscription *model. return nil } +func (a *App) GetSubscriptionStats() (*model.SubscriptionStats, *model.AppError) { + if a.Srv().License() == nil || !*a.Srv().License().Features.Cloud { + return nil, model.NewAppError("app.GetSubscriptionStats", "api.cloud.license_error", nil, "", http.StatusInternalServerError) + } + + subscription, appErr := a.Cloud().GetSubscription("") + if appErr != nil { + return nil, model.NewAppError("app.GetSubscriptionStats", "api.cloud.request_error", nil, appErr.Error(), http.StatusInternalServerError) + } + + count, err := a.Srv().Store.User().Count(model.UserCountOptions{}) + if err != nil { + return nil, model.NewAppError("app.GetSubscriptionStats", "app.user.get_total_users_count.app_error", nil, err.Error(), http.StatusInternalServerError) + } + cloudUserLimit := *a.Config().ExperimentalSettings.CloudUserLimit + + s := cloudUserLimit - count + + return &model.SubscriptionStats{ + RemainingSeats: int(s), + IsPaidTier: subscription.IsPaidTier, + }, nil +} + +func (a *App) CheckCloudAccountAtLimit() (bool, *model.AppError) { + if a.Srv().License() == nil || (a.Srv().License() != nil && !*a.Srv().License().Features.Cloud) { + // Not cloud instance, so no at limit checks + return false, nil + } + + stats, err := a.GetSubscriptionStats() + if err != nil { + return false, err + } + + if stats.IsPaidTier == "true" { + return false, nil + } + + if stats.RemainingSeats < 1 { + return true, nil + } + + return false, nil +} + func (a *App) CheckAndSendUserLimitWarningEmails(c *request.Context) *model.AppError { if a.Srv().License() == nil || (a.Srv().License() != nil && !*a.Srv().License().Features.Cloud) { // Not cloud instance, do nothing diff --git a/app/opentracing/opentracing_layer.go b/app/opentracing/opentracing_layer.go index 94b7fa68d6..0312805f39 100644 --- a/app/opentracing/opentracing_layer.go +++ b/app/opentracing/opentracing_layer.go @@ -1134,6 +1134,28 @@ func (a *OpenTracingAppLayer) CheckCanInviteToSharedChannel(channelId string) er return resultVar0 } +func (a *OpenTracingAppLayer) CheckCloudAccountAtLimit() (bool, *model.AppError) { + origCtx := a.ctx + span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.CheckCloudAccountAtLimit") + + a.ctx = newCtx + a.app.Srv().Store.SetContext(newCtx) + defer func() { + a.app.Srv().Store.SetContext(origCtx) + a.ctx = origCtx + }() + + defer span.Finish() + resultVar0, resultVar1 := a.app.CheckCloudAccountAtLimit() + + if resultVar1 != nil { + span.LogFields(spanlog.Error(resultVar1)) + ext.Error.Set(span, true) + } + + return resultVar0, resultVar1 +} + func (a *OpenTracingAppLayer) CheckForClientSideCert(r *http.Request) (string, string, string) { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.CheckForClientSideCert") @@ -8748,6 +8770,28 @@ func (a *OpenTracingAppLayer) GetStatusesByIds(userIDs []string) (map[string]int return resultVar0, resultVar1 } +func (a *OpenTracingAppLayer) GetSubscriptionStats() (*model.SubscriptionStats, *model.AppError) { + origCtx := a.ctx + span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.GetSubscriptionStats") + + a.ctx = newCtx + a.app.Srv().Store.SetContext(newCtx) + defer func() { + a.app.Srv().Store.SetContext(origCtx) + a.ctx = origCtx + }() + + defer span.Finish() + resultVar0, resultVar1 := a.app.GetSubscriptionStats() + + if resultVar1 != nil { + span.LogFields(spanlog.Error(resultVar1)) + ext.Error.Set(span, true) + } + + return resultVar0, resultVar1 +} + func (a *OpenTracingAppLayer) GetSuggestions(c *request.Context, commandArgs *model.CommandArgs, commands []*model.Command, roleID string) []model.AutocompleteSuggestion { origCtx := a.ctx span, newCtx := tracing.StartSpanWithParentByContext(a.ctx, "app.GetSuggestions") diff --git a/i18n/en.json b/i18n/en.json index 809a666214..6b1cc7f6cf 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -4134,6 +4134,14 @@ "id": "api.user.update_active.cannot_enable_guest_when_guest_feature_is_disabled.app_error", "translation": "You cannot activate a guest account because Guest Access feature is not enabled." }, + { + "id": "api.user.update_active.cloud_at_limit_check_error", + "translation": "Unable to make cloud check for at or over the limit." + }, + { + "id": "api.user.update_active.cloud_at_or_over_limit_check_overcapacity", + "translation": "Unable to activate more users as the cloud account is over capacity." + }, { "id": "api.user.update_active.not_enable.app_error", "translation": "You cannot deactivate yourself because this feature is not enabled. Please contact your System Administrator."