From 0ee05ce054f944538e7f29001a02d53c93a35d82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Andr=C3=A9s=20V=C3=A9lez=20Vidal?= Date: Thu, 28 Jul 2022 15:07:54 +0200 Subject: [PATCH] MM-45713 - change 500 error to json object (#20682) * MM-45713 - change 500 error to json object * validate possible encoding errors and follow standards * replace normal debugging string with true string * use bool type instead of string Co-authored-by: Pablo Velez Vidal Co-authored-by: Mattermod --- api4/cloud.go | 31 +++++++++++++++++------ api4/cloud_test.go | 62 ++++++++++++++++++++++++++++++++++++++++------ model/cloud.go | 4 +++ 3 files changed, 82 insertions(+), 15 deletions(-) diff --git a/api4/cloud.go b/api4/cloud.go index 0f833c8bdc..7ba3fb90f6 100644 --- a/api4/cloud.go +++ b/api4/cloud.go @@ -14,6 +14,7 @@ import ( "github.com/mattermost/mattermost-server/v6/audit" "github.com/mattermost/mattermost-server/v6/model" "github.com/mattermost/mattermost-server/v6/plugin" + "github.com/mattermost/mattermost-server/v6/shared/mlog" ) func (api *API) InitCloud() { @@ -232,12 +233,19 @@ func validateBusinessEmail(c *Context, w http.ResponseWriter, r *http.Request) { return } - errValidatingEmail := c.App.Cloud().ValidateBusinessEmail(user.Id, emailToValidate.Email) - if errValidatingEmail != nil { - c.Err = model.NewAppError("Api4.valiateBusinessEmail", "api.cloud.request_error", nil, errValidatingEmail.Error(), http.StatusInternalServerError) + emailErr := c.App.Cloud().ValidateBusinessEmail(user.Id, emailToValidate.Email) + if emailErr != nil { + c.Err = model.NewAppError("Api4.validateBusinessEmail", "api.cloud.request_error", nil, emailErr.Error(), http.StatusForbidden) + emailResp := model.ValidateBusinessEmailResponse{IsValid: false} + if err := json.NewEncoder(w).Encode(emailResp); err != nil { + mlog.Warn("Error while writing response", mlog.Err(err)) + } return } - ReturnStatusOK(w) + emailResp := model.ValidateBusinessEmailResponse{IsValid: true} + if err := json.NewEncoder(w).Encode(emailResp); err != nil { + mlog.Warn("Error while writing response", mlog.Err(err)) + } } func validateWorkspaceBusinessEmail(c *Context, w http.ResponseWriter, r *http.Request) { @@ -263,20 +271,27 @@ func validateWorkspaceBusinessEmail(c *Context, w http.ResponseWriter, r *http.R c.Err = model.NewAppError("Api4.validateWorkspaceBusinessEmail", "api.cloud.request_error", nil, err.Error(), http.StatusInternalServerError) return } - errValidatingSystemEmail := c.App.Cloud().ValidateBusinessEmail(user.Id, cloudCustomer.Email) + emailErr := c.App.Cloud().ValidateBusinessEmail(user.Id, cloudCustomer.Email) // if the current workspace email is not a valid business email - if errValidatingSystemEmail != nil { + if emailErr != nil { // grab the current admin email and validate it errValidatingAdminEmail := c.App.Cloud().ValidateBusinessEmail(user.Id, user.Email) if errValidatingAdminEmail != nil { - c.Err = model.NewAppError("Api4.validateWorkspaceBusinessEmail", "api.cloud.request_error", nil, errValidatingAdminEmail.Error(), http.StatusInternalServerError) + c.Err = model.NewAppError("Api4.validateWorkspaceBusinessEmail", "api.cloud.request_error", nil, errValidatingAdminEmail.Error(), http.StatusForbidden) + emailResp := model.ValidateBusinessEmailResponse{IsValid: false} + if err := json.NewEncoder(w).Encode(emailResp); err != nil { + mlog.Warn("Error while writing response", mlog.Err(err)) + } return } } // if any of the emails is valid, return ok - ReturnStatusOK(w) + emailResp := model.ValidateBusinessEmailResponse{IsValid: true} + if err := json.NewEncoder(w).Encode(emailResp); err != nil { + mlog.Warn("Error while writing response", mlog.Err(err)) + } } func getCloudProducts(c *Context, w http.ResponseWriter, r *http.Request) { diff --git a/api4/cloud_test.go b/api4/cloud_test.go index b959998f32..a689c68a8e 100644 --- a/api4/cloud_test.go +++ b/api4/cloud_test.go @@ -7,7 +7,6 @@ import ( "errors" "fmt" "net/http" - "net/http/httptest" "os" "testing" "time" @@ -401,21 +400,19 @@ func TestNotifyAdminToUpgrade(t *testing.T) { }) } func Test_validateBusinessEmail(t *testing.T) { - t.Run("Initial request has invalid email", func(t *testing.T) { + t.Run("Returns forbidden for non admin executors", func(t *testing.T) { th := Setup(t).InitBasic() defer th.TearDown() th.Client.Login(th.BasicUser.Email, th.BasicUser.Password) - validateBusinessEmail := model.ValidateBusinessEmailRequest{Email: ""} + invalidEmail := model.ValidateBusinessEmailRequest{Email: "invalid@gmail.com"} th.App.Srv().SetLicense(model.NewTestLicense("cloud")) cloud := mocks.CloudInterface{} - resp := httptest.NewRecorder() - - cloud.Mock.On("ValidateBusinessEmail", mock.Anything).Return(resp, nil) + cloud.Mock.On("ValidateBusinessEmail", th.SystemAdminUser.Id, invalidEmail.Email).Return(errors.New("invalid email")) cloudImpl := th.App.Srv().Cloud defer func() { @@ -423,8 +420,59 @@ func Test_validateBusinessEmail(t *testing.T) { }() th.App.Srv().Cloud = &cloud - _, err := th.Client.ValidateBusinessEmail(&validateBusinessEmail) + res, err := th.Client.ValidateBusinessEmail(&invalidEmail) require.Error(t, err) + require.Equal(t, http.StatusForbidden, res.StatusCode, "403") + }) + + t.Run("Returns forbidden for invalid business email", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + th.Client.Login(th.BasicUser.Email, th.BasicUser.Password) + + validBusinessEmail := model.ValidateBusinessEmailRequest{Email: "invalid@slacker.com"} + + th.App.Srv().SetLicense(model.NewTestLicense("cloud")) + + cloud := mocks.CloudInterface{} + + cloud.Mock.On("ValidateBusinessEmail", th.SystemAdminUser.Id, validBusinessEmail.Email).Return(errors.New("invalid email")) + + cloudImpl := th.App.Srv().Cloud + defer func() { + th.App.Srv().Cloud = cloudImpl + }() + th.App.Srv().Cloud = &cloud + + res, err := th.SystemAdminClient.ValidateBusinessEmail(&validBusinessEmail) + require.Error(t, err) + require.Equal(t, http.StatusForbidden, res.StatusCode, "403") + }) + + t.Run("Validate business email for admin", func(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + th.Client.Login(th.BasicUser.Email, th.BasicUser.Password) + + validBusinessEmail := model.ValidateBusinessEmailRequest{Email: "valid@mattermost.com"} + + th.App.Srv().SetLicense(model.NewTestLicense("cloud")) + + cloud := mocks.CloudInterface{} + + cloud.Mock.On("ValidateBusinessEmail", th.SystemAdminUser.Id, validBusinessEmail.Email).Return(nil) + + cloudImpl := th.App.Srv().Cloud + defer func() { + th.App.Srv().Cloud = cloudImpl + }() + th.App.Srv().Cloud = &cloud + + res, err := th.SystemAdminClient.ValidateBusinessEmail(&validBusinessEmail) + require.NoError(t, err) + require.Equal(t, http.StatusOK, res.StatusCode, "200") }) } diff --git a/model/cloud.go b/model/cloud.go index 46d7b7baad..d000c2a73b 100644 --- a/model/cloud.go +++ b/model/cloud.go @@ -105,6 +105,10 @@ type ValidateBusinessEmailRequest struct { Email string `json:"email"` } +type ValidateBusinessEmailResponse struct { + IsValid bool `json:"is_valid"` +} + // CloudCustomerInfo represents editable info of a customer. type CloudCustomerInfo struct { Name string `json:"name"`