From 11513a8d0d9e08ad1f4c1a7234db468c7f281e48 Mon Sep 17 00:00:00 2001 From: Agniva De Sarker Date: Thu, 13 Aug 2020 22:19:05 +0530 Subject: [PATCH] MM-27507: Propagate rate limit errors to client (#15230) * MM-27507: Propagate rate limit errors to client We return an error from SendInviteEmails instead of just logging it to let the client know that a rate limit error has happened. The status code is chosen as 413 (entity too large) instead of 429 (too many requests) because it's not the request which is rate limited, but the payload inside it which is. Ideally, the email sending should have been implemented by a queue which would just return an error to the client when full. That is also why we are not returning an X-Retry-After and X-Reset-After in the headers because that would mix with the actual rate limiting. A separate header X-Email-Invite-Reset-After might do the job, but it comes at an extra cost of additional API surface and a clunky API. Instead, that information is contained in the error response. The web client needs to just surface the error. An API client will have to do a bit more work to parse the error if it needs to automatically know when to retry. Given that an email sending client is not a very common use case, we decide to keep the API clean. This decision can be revisited if it becomes problematic in the future. https://mattermost.atlassian.net/browse/MM-27507 * Fixing translations * Added retry_after and reset_after in API response. --- api4/team_local.go | 12 +++++++-- api4/team_test.go | 45 +++++++++++++++++++++++++++++++++ app/email.go | 40 +++++++++++++---------------- app/email_test.go | 31 +++++++++++++++++++++++ app/team.go | 23 ++++++++++++----- cmd/mattermost/commands/user.go | 5 +++- i18n/en.json | 12 +++++++++ 7 files changed, 137 insertions(+), 31 deletions(-) diff --git a/api4/team_local.go b/api4/team_local.go index b89c3a16e0..9165c74aa0 100644 --- a/api4/team_local.go +++ b/api4/team_local.go @@ -115,7 +115,11 @@ func localInviteUsersToTeam(c *Context, w http.ResponseWriter, r *http.Request) } auditRec.AddMeta("errors", errList) if len(goodEmails) > 0 { - c.App.Srv().EmailService.SendInviteEmails(team, "Administrator", "mmctl "+model.NewId(), goodEmails, *c.App.Config().ServiceSettings.SiteURL) + err = c.App.Srv().EmailService.SendInviteEmails(team, "Administrator", "mmctl "+model.NewId(), goodEmails, *c.App.Config().ServiceSettings.SiteURL) + if err != nil { + c.Err = err + return + } } // in graceful mode we return both the successful ones and the failed ones w.Write([]byte(model.EmailInviteWithErrorToJson(invitesWithErrors))) @@ -132,7 +136,11 @@ func localInviteUsersToTeam(c *Context, w http.ResponseWriter, r *http.Request) c.Err = model.NewAppError("localInviteUsersToTeam", "api.team.invite_members.invalid_email.app_error", map[string]interface{}{"Addresses": s}, "", http.StatusBadRequest) return } - c.App.Srv().EmailService.SendInviteEmails(team, "Administrator", "mmctl "+model.NewId(), emailList, *c.App.Config().ServiceSettings.SiteURL) + err = c.App.Srv().EmailService.SendInviteEmails(team, "Administrator", "mmctl "+model.NewId(), emailList, *c.App.Config().ServiceSettings.SiteURL) + if err != nil { + c.Err = err + return + } ReturnStatusOK(w) } auditRec.Success() diff --git a/api4/team_test.go b/api4/team_test.go index bdcf0cc8ab..e3143b3977 100644 --- a/api4/team_test.go +++ b/api4/team_test.go @@ -2833,6 +2833,25 @@ func TestInviteUsersToTeam(t *testing.T) { require.NotNil(t, invitesWithErrors[0].Error) require.Nil(t, invitesWithErrors[1].Error) }, "override restricted domains") + + th.TestForAllClients(t, func(t *testing.T, client *model.Client4) { + th.BasicTeam.AllowedDomains = "common.com" + _, err := th.App.UpdateTeam(th.BasicTeam) + require.Nilf(t, err, "%v, Should update the team", err) + + emailList := make([]string, 22) + for i := 0; i < 22; i++ { + emailList[i] = "test-" + strconv.Itoa(i) + "@common.com" + } + okMsg, resp := client.InviteUsersToTeam(th.BasicTeam.Id, emailList) + require.False(t, okMsg, "should return false") + CheckRequestEntityTooLargeStatus(t, resp) + CheckErrorMessage(t, resp, "app.email.rate_limit_exceeded.app_error") + + _, resp = client.InviteUsersToTeamGracefully(th.BasicTeam.Id, emailList) + CheckRequestEntityTooLargeStatus(t, resp) + CheckErrorMessage(t, resp, "app.email.rate_limit_exceeded.app_error") + }, "rate limits") } func TestInviteGuestsToTeam(t *testing.T) { @@ -2942,6 +2961,32 @@ func TestInviteGuestsToTeam(t *testing.T) { err := th.App.InviteNewUsersToTeam([]string{"user@global.com"}, th.BasicTeam.Id, th.BasicUser.Id) require.Nil(t, err, "non guest user invites should not be affected by the guest domain restrictions") }) + + t.Run("rate limit", func(t *testing.T) { + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.GuestAccountsSettings.RestrictCreationToDomains = "@guest.com" }) + + _, err := th.App.UpdateTeam(th.BasicTeam) + require.Nilf(t, err, "%v, Should update the team", err) + + emailList := make([]string, 22) + for i := 0; i < 22; i++ { + emailList[i] = "test-" + strconv.Itoa(i) + "@guest.com" + } + invite := &model.GuestsInvite{ + Emails: emailList, + Channels: []string{th.BasicChannel.Id}, + Message: "test message", + } + err = th.App.InviteGuestsToChannels(th.BasicTeam.Id, invite, th.BasicUser.Id) + require.NotNil(t, err) + assert.Equal(t, "app.email.rate_limit_exceeded.app_error", err.Id) + assert.Equal(t, http.StatusRequestEntityTooLarge, err.StatusCode) + + _, err = th.App.InviteGuestsToChannelsGracefully(th.BasicTeam.Id, invite, th.BasicUser.Id) + require.NotNil(t, err) + assert.Equal(t, "app.email.rate_limit_exceeded.app_error", err.Id) + assert.Equal(t, http.StatusRequestEntityTooLarge, err.StatusCode) + }) } func TestGetTeamInviteInfo(t *testing.T) { diff --git a/app/email.go b/app/email.go index f4413f187e..e6cadc77a4 100644 --- a/app/email.go +++ b/app/email.go @@ -323,24 +323,21 @@ func (es *EmailService) sendMfaChangeEmail(email string, activated bool, locale, return nil } -func (es *EmailService) SendInviteEmails(team *model.Team, senderName string, senderUserId string, invites []string, siteURL string) { +func (es *EmailService) SendInviteEmails(team *model.Team, senderName string, senderUserId string, invites []string, siteURL string) *model.AppError { if es.EmailRateLimiter == nil { - es.srv.Log.Error("Email invite not sent, rate limiting could not be setup.", mlog.String("user_id", senderUserId), mlog.String("team_id", team.Id)) - return + return model.NewAppError("SendInviteEmails", "app.email.no_rate_limiter.app_error", nil, fmt.Sprintf("user_id=%s, team_id=%s", senderUserId, team.Id), http.StatusInternalServerError) } rateLimited, result, err := es.EmailRateLimiter.RateLimit(senderUserId, len(invites)) if err != nil { - es.srv.Log.Error("Error rate limiting invite email.", mlog.String("user_id", senderUserId), mlog.String("team_id", team.Id), mlog.Err(err)) - return + return model.NewAppError("SendInviteEmails", "app.email.setup_rate_limiter.app_error", nil, fmt.Sprintf("user_id=%s, team_id=%s, error=%v", senderUserId, team.Id, err), http.StatusInternalServerError) } if rateLimited { - es.srv.Log.Error("Invite emails rate limited.", - mlog.String("user_id", senderUserId), - mlog.String("team_id", team.Id), - mlog.String("retry_after", result.RetryAfter.String()), - mlog.Err(err)) - return + return model.NewAppError("SendInviteEmails", + "app.email.rate_limit_exceeded.app_error", map[string]interface{}{"RetryAfter": result.RetryAfter.String(), "ResetAfter": result.ResetAfter.String()}, + fmt.Sprintf("user_id=%s, team_id=%s, retry_after_secs=%f, reset_after_secs=%f", + senderUserId, team.Id, result.RetryAfter.Seconds(), result.ResetAfter.Seconds()), + http.StatusRequestEntityTooLarge) } for _, invite := range invites { @@ -382,26 +379,24 @@ func (es *EmailService) SendInviteEmails(team *model.Team, senderName string, se } } } + return nil } -func (es *EmailService) sendGuestInviteEmails(team *model.Team, channels []*model.Channel, senderName string, senderUserId string, senderProfileImage []byte, invites []string, siteURL string, message string) { +func (es *EmailService) sendGuestInviteEmails(team *model.Team, channels []*model.Channel, senderName string, senderUserId string, senderProfileImage []byte, invites []string, siteURL string, message string) *model.AppError { if es.EmailRateLimiter == nil { - es.srv.Log.Error("Email invite not sent, rate limiting could not be setup.", mlog.String("user_id", senderUserId), mlog.String("team_id", team.Id)) - return + return model.NewAppError("SendInviteEmails", "app.email.no_rate_limiter.app_error", nil, fmt.Sprintf("user_id=%s, team_id=%s", senderUserId, team.Id), http.StatusInternalServerError) } rateLimited, result, err := es.EmailRateLimiter.RateLimit(senderUserId, len(invites)) if err != nil { - es.srv.Log.Error("Error rate limiting invite email.", mlog.String("user_id", senderUserId), mlog.String("team_id", team.Id), mlog.Err(err)) - return + return model.NewAppError("SendInviteEmails", "app.email.setup_rate_limiter.app_error", nil, fmt.Sprintf("user_id=%s, team_id=%s, error=%v", senderUserId, team.Id, err), http.StatusInternalServerError) } if rateLimited { - es.srv.Log.Error("Invite emails rate limited.", - mlog.String("user_id", senderUserId), - mlog.String("team_id", team.Id), - mlog.String("retry_after", result.RetryAfter.String()), - mlog.Err(err)) - return + return model.NewAppError("SendInviteEmails", + "app.email.rate_limit_exceeded.app_error", map[string]interface{}{"RetryAfter": result.RetryAfter.String(), "ResetAfter": result.ResetAfter.String()}, + fmt.Sprintf("user_id=%s, team_id=%s, retry_after_secs=%f, reset_after_secs=%f", + senderUserId, team.Id, result.RetryAfter.Seconds(), result.ResetAfter.Seconds()), + http.StatusRequestEntityTooLarge) } for _, invite := range invites { @@ -472,6 +467,7 @@ func (es *EmailService) sendGuestInviteEmails(team *model.Team, channels []*mode } } } + return nil } func (es *EmailService) newEmailTemplate(name, locale string) *utils.HTMLTemplate { diff --git a/app/email_test.go b/app/email_test.go index 0bec302581..f38a90f0c4 100644 --- a/app/email_test.go +++ b/app/email_test.go @@ -4,8 +4,12 @@ package app import ( + "net/http" + "strconv" "testing" + "github.com/mattermost/mattermost-server/v5/model" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -29,3 +33,30 @@ func TestCondenseSiteURL(t *testing.T) { require.Equal(t, "chat.mattermost.com:8080/subpath", condenseSiteURL("http://chat.mattermost.com:8080/subpath")) require.Equal(t, "chat.mattermost.com:8080/subpath", condenseSiteURL("http://chat.mattermost.com:8080/subpath/")) } + +func TestSendInviteEmailRateLimits(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + th.BasicTeam.AllowedDomains = "common.com" + _, err := th.App.UpdateTeam(th.BasicTeam) + require.Nilf(t, err, "%v, Should update the team", err) + + th.App.UpdateConfig(func(cfg *model.Config) { + *cfg.ServiceSettings.EnableEmailInvitations = true + }) + + emailList := make([]string, 22) + for i := 0; i < 22; i++ { + emailList[i] = "test-" + strconv.Itoa(i) + "@common.com" + } + err = th.App.InviteNewUsersToTeam(emailList, th.BasicTeam.Id, th.BasicUser.Id) + require.NotNil(t, err) + assert.Equal(t, "app.email.rate_limit_exceeded.app_error", err.Id) + assert.Equal(t, http.StatusRequestEntityTooLarge, err.StatusCode) + + _, err = th.App.InviteNewUsersToTeamGracefully(emailList, th.BasicTeam.Id, th.BasicUser.Id) + require.NotNil(t, err) + assert.Equal(t, "app.email.rate_limit_exceeded.app_error", err.Id) + assert.Equal(t, http.StatusRequestEntityTooLarge, err.StatusCode) +} diff --git a/app/team.go b/app/team.go index 0fedc200c2..46a71d2b03 100644 --- a/app/team.go +++ b/app/team.go @@ -1159,7 +1159,10 @@ func (a *App) InviteNewUsersToTeamGracefully(emailList []string, teamId, senderI if len(goodEmails) > 0 { nameFormat := *a.Config().TeamSettings.TeammateNameDisplay - a.Srv().EmailService.SendInviteEmails(team, user.GetDisplayName(nameFormat), user.Id, goodEmails, a.GetSiteURL()) + err = a.Srv().EmailService.SendInviteEmails(team, user.GetDisplayName(nameFormat), user.Id, goodEmails, a.GetSiteURL()) + if err != nil { + return nil, err + } } return inviteListWithErrors, nil @@ -1246,7 +1249,10 @@ func (a *App) InviteGuestsToChannelsGracefully(teamId string, guestsInvite *mode if err != nil { a.Log().Warn("Unable to get the sender user profile image.", mlog.String("user_id", user.Id), mlog.String("team_id", team.Id), mlog.Err(err)) } - a.Srv().EmailService.sendGuestInviteEmails(team, channels, user.GetDisplayName(nameFormat), user.Id, senderProfileImage, goodEmails, a.GetSiteURL(), guestsInvite.Message) + err = a.Srv().EmailService.sendGuestInviteEmails(team, channels, user.GetDisplayName(nameFormat), user.Id, senderProfileImage, goodEmails, a.GetSiteURL(), guestsInvite.Message) + if err != nil { + return nil, err + } } return inviteListWithErrors, nil @@ -1278,12 +1284,14 @@ func (a *App) InviteNewUsersToTeam(emailList []string, teamId, senderId string) if len(invalidEmailList) > 0 { s := strings.Join(invalidEmailList, ", ") - err := model.NewAppError("InviteNewUsersToTeam", "api.team.invite_members.invalid_email.app_error", map[string]interface{}{"Addresses": s}, "", http.StatusBadRequest) - return err + return model.NewAppError("InviteNewUsersToTeam", "api.team.invite_members.invalid_email.app_error", map[string]interface{}{"Addresses": s}, "", http.StatusBadRequest) } nameFormat := *a.Config().TeamSettings.TeammateNameDisplay - a.Srv().EmailService.SendInviteEmails(team, user.GetDisplayName(nameFormat), user.Id, emailList, a.GetSiteURL()) + err = a.Srv().EmailService.SendInviteEmails(team, user.GetDisplayName(nameFormat), user.Id, emailList, a.GetSiteURL()) + if err != nil { + return err + } return nil } @@ -1315,7 +1323,10 @@ func (a *App) InviteGuestsToChannels(teamId string, guestsInvite *model.GuestsIn if err != nil { a.Log().Warn("Unable to get the sender user profile image.", mlog.String("user_id", user.Id), mlog.String("team_id", team.Id), mlog.Err(err)) } - a.Srv().EmailService.sendGuestInviteEmails(team, channels, user.GetDisplayName(nameFormat), user.Id, senderProfileImage, guestsInvite.Emails, a.GetSiteURL(), guestsInvite.Message) + err = a.Srv().EmailService.sendGuestInviteEmails(team, channels, user.GetDisplayName(nameFormat), user.Id, senderProfileImage, guestsInvite.Emails, a.GetSiteURL(), guestsInvite.Message) + if err != nil { + return err + } return nil } diff --git a/cmd/mattermost/commands/user.go b/cmd/mattermost/commands/user.go index 58cd1a92ac..d69ec602d3 100644 --- a/cmd/mattermost/commands/user.go +++ b/cmd/mattermost/commands/user.go @@ -623,7 +623,10 @@ func inviteUser(a *app.App, email string, team *model.Team, teamArg string) erro return fmt.Errorf("Email invites are disabled.") } - a.Srv().EmailService.SendInviteEmails(team, "Administrator", "Mattermost CLI "+model.NewId(), invites, *a.Config().ServiceSettings.SiteURL) + err := a.Srv().EmailService.SendInviteEmails(team, "Administrator", "Mattermost CLI "+model.NewId(), invites, *a.Config().ServiceSettings.SiteURL) + if err != nil { + return err + } CommandPrettyPrintln("Invites may or may not have been sent.") auditRec := a.MakeAuditRecord("inviteUser", audit.Success) diff --git a/i18n/en.json b/i18n/en.json index add982db05..f9c590c0f2 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -3378,6 +3378,18 @@ "id": "app.create_basic_user.save_member.max_accounts.app_error", "translation": "Unable to create default team membership because no more members are allowed in that team" }, + { + "id": "app.email.no_rate_limiter.app_error", + "translation": "Rate limiter is not set up." + }, + { + "id": "app.email.rate_limit_exceeded.app_error", + "translation": "Invite emails rate limit exceeded. Timer will be reset after {{.ResetAfter}} seconds. Please retry after {{.RetryAfter}} seconds." + }, + { + "id": "app.email.setup_rate_limiter.app_error", + "translation": "Error occurred in the rate limiter." + }, { "id": "app.emoji.create.internal_error", "translation": "Unable to save emoji."