From 7b8a2e89d482653641034a02f474df96ac47d329 Mon Sep 17 00:00:00 2001 From: Ibrahim Serdar Acikgoz Date: Mon, 4 Apr 2022 15:03:31 +0300 Subject: [PATCH] api4/system: add check if model.FileSettings is empty (#19876) * api4/system: add config validation for model.FileSettings * reflect review comments * add unmarshal --- api4/system.go | 45 ++++++++++++++++--- api4/system_test.go | 105 ++++++++++++++++++++++++++++++++++---------- i18n/en.json | 8 ++++ 3 files changed, 128 insertions(+), 30 deletions(-) diff --git a/api4/system.go b/api4/system.go index 6bc62438c2..d1d2d0aade 100644 --- a/api4/system.go +++ b/api4/system.go @@ -11,6 +11,7 @@ import ( "io/ioutil" "net/http" "path" + "reflect" "runtime" "strconv" "time" @@ -195,9 +196,16 @@ func getSystemPing(c *Context, w http.ResponseWriter, r *http.Request) { } func testEmail(c *Context, w http.ResponseWriter, r *http.Request) { - cfg := model.ConfigFromJSON(r.Body) - if cfg == nil { - cfg = c.App.Config() + var cfg *model.Config + jsonErr := json.NewDecoder(r.Body).Decode(&cfg) + if jsonErr != nil { + c.Err = model.NewAppError("testEmail", "api.unmarshal_error", nil, jsonErr.Error(), http.StatusBadRequest) + return + } + + if checkHasNilFields(&cfg.EmailSettings) { + c.Err = model.NewAppError("testEmail", "api.file.test_connection_email_settings_nil.app_error", nil, "", http.StatusBadRequest) + return } if !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionTestEmail) { @@ -446,9 +454,16 @@ func getSupportedTimezones(c *Context, w http.ResponseWriter, r *http.Request) { } func testS3(c *Context, w http.ResponseWriter, r *http.Request) { - cfg := model.ConfigFromJSON(r.Body) - if cfg == nil { - cfg = c.App.Config() + var cfg *model.Config + jsonErr := json.NewDecoder(r.Body).Decode(&cfg) + if jsonErr != nil { + c.Err = model.NewAppError("testS3", "api.unmarshal_error", nil, jsonErr.Error(), http.StatusBadRequest) + return + } + + if checkHasNilFields(&cfg.FileSettings) { + c.Err = model.NewAppError("testS3", "api.file.test_connection_s3_settings_nil.app_error", nil, "", http.StatusBadRequest) + return } if !c.App.SessionHasPermissionTo(*c.AppContext.Session(), model.PermissionTestS3) { @@ -960,3 +975,21 @@ func getAppliedSchemaMigrations(c *Context, w http.ResponseWriter, r *http.Reque w.Write(js) auditRec.Success() } + +// returns true if the data has nil fields +// this is being used for testS3 and testEmail methods +func checkHasNilFields(value interface{}) bool { + v := reflect.Indirect(reflect.ValueOf(value)) + if v.Kind() != reflect.Struct { + return false + } + + for i := 0; i < v.NumField(); i++ { + field := v.Field(i) + if field.Kind() == reflect.Ptr && field.IsNil() { + return true + } + } + + return false +} diff --git a/api4/system_test.go b/api4/system_test.go index 65bcbebde0..5058e89acd 100644 --- a/api4/system_test.go +++ b/api4/system_test.go @@ -134,24 +134,27 @@ func TestEmailTest(t *testing.T) { require.NoError(t, err) defer os.RemoveAll(dir) + es := model.EmailSettings{} + es.SetDefaults(false) + + es.SMTPServer = model.NewString("") + es.SMTPPort = model.NewString("") + es.SMTPPassword = model.NewString("") + es.FeedbackName = model.NewString("") + es.FeedbackEmail = model.NewString("some-addr@test.com") + es.ReplyToAddress = model.NewString("some-addr@test.com") + es.ConnectionSecurity = model.NewString("") + es.SMTPUsername = model.NewString("") + es.EnableSMTPAuth = model.NewBool(false) + es.SkipServerCertificateVerification = model.NewBool(true) + es.SendEmailNotifications = model.NewBool(false) + es.SMTPServerTimeout = model.NewInt(15) + config := model.Config{ ServiceSettings: model.ServiceSettings{ SiteURL: model.NewString(""), }, - EmailSettings: model.EmailSettings{ - SMTPServer: model.NewString(""), - SMTPPort: model.NewString(""), - SMTPPassword: model.NewString(""), - FeedbackName: model.NewString(""), - FeedbackEmail: model.NewString("some-addr@test.com"), - ReplyToAddress: model.NewString("some-addr@test.com"), - ConnectionSecurity: model.NewString(""), - SMTPUsername: model.NewString(""), - EnableSMTPAuth: model.NewBool(false), - SkipServerCertificateVerification: model.NewBool(true), - SendEmailNotifications: model.NewBool(false), - SMTPServerTimeout: model.NewInt(15), - }, + EmailSettings: es, FileSettings: model.FileSettings{ DriverName: model.NewString(model.ImageDriverLocal), Directory: model.NewString(dir), @@ -193,6 +196,14 @@ func TestEmailTest(t *testing.T) { require.Error(t, err) CheckForbiddenStatus(t, resp) }) + + t.Run("empty email settings", func(t *testing.T) { + config.EmailSettings = model.EmailSettings{} + resp, err := th.SystemAdminClient.TestEmail(&config) + require.Error(t, err) + CheckErrorID(t, err, "api.file.test_connection_email_settings_nil.app_error") + CheckBadRequestStatus(t, resp) + }) } func TestGenerateSupportPacket(t *testing.T) { @@ -514,17 +525,21 @@ func TestS3TestConnection(t *testing.T) { } s3Endpoint := fmt.Sprintf("%s:%s", s3Host, s3Port) + + fs := model.FileSettings{} + fs.SetDefaults(false) + + fs.DriverName = model.NewString(model.ImageDriverS3) + fs.AmazonS3AccessKeyId = model.NewString(model.MinioAccessKey) + fs.AmazonS3SecretAccessKey = model.NewString(model.MinioSecretKey) + fs.AmazonS3Bucket = model.NewString("") + fs.AmazonS3Endpoint = model.NewString(s3Endpoint) + fs.AmazonS3Region = model.NewString("") + fs.AmazonS3PathPrefix = model.NewString("") + fs.AmazonS3SSL = model.NewBool(false) + config := model.Config{ - FileSettings: model.FileSettings{ - DriverName: model.NewString(model.ImageDriverS3), - AmazonS3AccessKeyId: model.NewString(model.MinioAccessKey), - AmazonS3SecretAccessKey: model.NewString(model.MinioSecretKey), - AmazonS3Bucket: model.NewString(""), - AmazonS3Endpoint: model.NewString(s3Endpoint), - AmazonS3Region: model.NewString(""), - AmazonS3PathPrefix: model.NewString(""), - AmazonS3SSL: model.NewBool(false), - }, + FileSettings: fs, } t.Run("as system user", func(t *testing.T) { @@ -572,11 +587,20 @@ func TestS3TestConnection(t *testing.T) { t.Run("as restricted system admin", func(t *testing.T) { th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ExperimentalSettings.RestrictSystemAdmin = true }) + defer th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ExperimentalSettings.RestrictSystemAdmin = false }) resp, err := th.SystemAdminClient.TestS3Connection(&config) require.Error(t, err) CheckForbiddenStatus(t, resp) }) + + t.Run("empty file settings", func(t *testing.T) { + config.FileSettings = model.FileSettings{} + resp, err := th.SystemAdminClient.TestS3Connection(&config) + require.Error(t, err) + CheckErrorID(t, err, "api.file.test_connection_s3_settings_nil.app_error") + CheckBadRequestStatus(t, resp) + }) } func TestSupportedTimezones(t *testing.T) { @@ -958,3 +982,36 @@ func TestGetAppliedSchemaMigrations(t *testing.T) { CheckOKStatus(t, resp) }) } + +func TestCheckHasNilFields(t *testing.T) { + t.Run("check if the empty struct has nil fields", func(t *testing.T) { + var s model.FileSettings + res := checkHasNilFields(&s) + require.True(t, res) + }) + + t.Run("check if the struct has any nil fields", func(t *testing.T) { + s := model.FileSettings{ + DriverName: model.NewString(model.ImageDriverLocal), + } + res := checkHasNilFields(&s) + require.True(t, res) + }) + + t.Run("struct has all fields set", func(t *testing.T) { + var s model.FileSettings + s.SetDefaults(false) + res := checkHasNilFields(&s) + require.False(t, res) + }) + + t.Run("embedded struct, with nil fields", func(t *testing.T) { + type myStr struct { + Name string + Surname *string + } + s := myStr{} + res := checkHasNilFields(&s) + require.True(t, res) + }) +} diff --git a/i18n/en.json b/i18n/en.json index 1790c7c95f..f30a4f4287 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -1801,6 +1801,10 @@ "id": "api.file.test_connection.app_error", "translation": "Unable to access the file storage." }, + { + "id": "api.file.test_connection_email_settings_nil.app_error", + "translation": "Email settings has unset values." + }, { "id": "api.file.test_connection_s3_auth.app_error", "translation": "Unable to connect to S3. Verify your Amazon S3 connection authorization parameters and authentication settings." @@ -1809,6 +1813,10 @@ "id": "api.file.test_connection_s3_bucket_does_not_exist.app_error", "translation": "Ensure your Amazon S3 bucket is available, and verify your bucket permissions." }, + { + "id": "api.file.test_connection_s3_settings_nil.app_error", + "translation": "File storage settings has unset values." + }, { "id": "api.file.upload_file.incorrect_channelId.app_error", "translation": "Unable to upload the file. Incorrect channel ID: {{.channelId}}"