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

@@ -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)
+	var cfg *model.Config
-	if cfg == nil {
+	jsonErr := json.NewDecoder(r.Body).Decode(&cfg)
-		cfg = c.App.Config()
+	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)
+	var cfg *model.Config
-	if cfg == nil {
+	jsonErr := json.NewDecoder(r.Body).Decode(&cfg)
-		cfg = c.App.Config()
+	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
+}

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{
+		EmailSettings: es,
-			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),
-		},
 		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{
+		FileSettings: fs,
-			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),
-		},
 	}
 
 	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)
+	})
+}

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}}"