[MM-33826] add more detailed S3 error messages (#17182)

Automatic Merge

api4/system_test.go

@@ -507,12 +507,20 @@ func TestS3TestConnection(t *testing.T) {
 		config.FileSettings.AmazonS3Bucket = model.NewString("Wrong_bucket")
 		_, resp = th.SystemAdminClient.TestS3Connection(&config)
 		CheckInternalErrorStatus(t, resp)
-		assert.Equal(t, "api.file.test_connection.app_error", resp.Error.Id)
+		assert.Equal(t, "api.file.test_connection_s3_bucket_does_not_exist.app_error", resp.Error.Id)
 
 		*config.FileSettings.AmazonS3Bucket = "shouldnotcreatenewbucket"
 		_, resp = th.SystemAdminClient.TestS3Connection(&config)
 		CheckInternalErrorStatus(t, resp)
-		assert.Equal(t, "api.file.test_connection.app_error", resp.Error.Id)
+		assert.Equal(t, "api.file.test_connection_s3_bucket_does_not_exist.app_error", resp.Error.Id)
+	})
+
+	t.Run("with incorrect credentials", func(t *testing.T) {
+		configCopy := config
+		*configCopy.FileSettings.AmazonS3AccessKeyId = "invalidaccesskey"
+		_, resp := th.SystemAdminClient.TestS3Connection(&configCopy)
+		CheckInternalErrorStatus(t, resp)
+		assert.Equal(t, "api.file.test_connection_s3_auth.app_error", resp.Error.Id)
 	})
 
 	t.Run("as restricted system admin", func(t *testing.T) {
@@ -521,7 +529,6 @@ func TestS3TestConnection(t *testing.T) {
 		_, resp := th.SystemAdminClient.TestS3Connection(&config)
 		CheckForbiddenStatus(t, resp)
 	})
-
 }
 
 func TestSupportedTimezones(t *testing.T) {

app/file.go

@@ -91,6 +91,17 @@ func (a *App) CheckMandatoryS3Fields(settings *model.FileSettings) *model.AppErr
 	return nil
 }
 
+func connectionTestErrorToAppError(connTestErr error) *model.AppError {
+	switch err := connTestErr.(type) {
+	case *filestore.S3FileBackendAuthError:
+		return model.NewAppError("TestConnection", "api.file.test_connection_s3_auth.app_error", nil, err.Error(), http.StatusInternalServerError)
+	case *filestore.S3FileBackendNoBucketError:
+		return model.NewAppError("TestConnection", "api.file.test_connection_s3_bucket_does_not_exist.app_error", nil, err.Error(), http.StatusInternalServerError)
+	default:
+		return model.NewAppError("TestConnection", "api.file.test_connection.app_error", nil, connTestErr.Error(), http.StatusInternalServerError)
+	}
+}
+
 func (a *App) TestFileStoreConnection() *model.AppError {
 	backend, err := a.FileBackend()
 	if err != nil {
@@ -98,7 +109,7 @@ func (a *App) TestFileStoreConnection() *model.AppError {
 	}
 	nErr := backend.TestConnection()
 	if nErr != nil {
-		return model.NewAppError("TestConnection", "api.file.test_connection.app_error", nil, nErr.Error(), http.StatusInternalServerError)
+		return connectionTestErrorToAppError(nErr)
 	}
 	return nil
 }
@@ -111,7 +122,7 @@ func (a *App) TestFileStoreConnectionWithConfig(cfg *model.FileSettings) *model.
 	}
 	nErr := backend.TestConnection()
 	if nErr != nil {
-		return model.NewAppError("TestConnection", "api.file.test_connection.app_error", nil, nErr.Error(), http.StatusInternalServerError)
+		return connectionTestErrorToAppError(nErr)
 	}
 	return nil
 }

app/server.go

@@ -522,7 +522,7 @@ func NewServer(options ...Option) (*Server, error) {
 	} else {
 		nErr := backend.TestConnection()
 		if nErr != nil {
-			if errors.Is(nErr, filestore.ErrNoS3Bucket) {
+			if _, ok := nErr.(*filestore.S3FileBackendNoBucketError); ok {
 				nErr = backend.(*filestore.S3FileBackend).MakeBucket()
 			}
 			if nErr != nil {

i18n/en.json

@@ -1832,6 +1832,14 @@
     "id": "api.file.test_connection.app_error",
     "translation": "Unable to access the file storage."
   },
+  {
+    "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."
+  },
+  {
+    "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.upload_file.incorrect_channelId.app_error",
     "translation": "Unable to upload the file. Incorrect channel ID: {{.channelId}}"

shared/filestore/filesstore_test.go

@@ -94,9 +94,9 @@ func (s *FileBackendTestSuite) SetupTest() {
 	s.backend = backend
 
 	// This is needed to create the bucket if it doesn't exist.
-	if err := s.backend.TestConnection(); err == ErrNoS3Bucket {
-		s3Backend, ok := s.backend.(*S3FileBackend)
-		s.True(ok)
+	err = s.backend.TestConnection()
+	if _, ok := err.(*S3FileBackendNoBucketError); ok {
+		s3Backend := s.backend.(*S3FileBackend)
 		s.NoError(s3Backend.MakeBucket())
 	} else {
 		s.NoError(err)

shared/filestore/s3store.go

@@ -36,13 +36,18 @@ type S3FileBackend struct {
 	client     *s3.Client
 }
 
+type S3FileBackendAuthError struct {
+	DetailedError string
+}
+
+// S3FileBackendNoBucketError is returned when testing a connection and no S3 bucket is found
+type S3FileBackendNoBucketError struct{}
+
 const (
 	// This is not exported by minio. See: https://github.com/minio/minio-go/issues/1339
 	bucketNotFound = "NoSuchBucket"
 )
 
-// ErrNoS3Bucket is returned when testing a connection and no S3 bucket is found
-var ErrNoS3Bucket = errors.New("no such bucket")
 var (
 	imageExtensions = map[string]bool{".jpg": true, ".jpeg": true, ".gif": true, ".bmp": true, ".png": true, ".tiff": true, "tif": true}
 	imageMimeTypes  = map[string]string{".jpg": "image/jpeg", ".jpeg": "image/jpeg", ".gif": "image/gif", ".bmp": "image/bmp", ".png": "image/png", ".tiff": "image/tiff", ".tif": "image/tif"}
@@ -61,6 +66,14 @@ func getImageMimeType(ext string) string {
 	return imageMimeTypes[ext]
 }
 
+func (s *S3FileBackendAuthError) Error() string {
+	return s.DetailedError
+}
+
+func (s *S3FileBackendNoBucketError) Error() string {
+	return "no such bucket"
+}
+
 // NewS3FileBackend returns an instance of an S3FileBackend.
 func NewS3FileBackend(settings FileBackendSettings) (*S3FileBackend, error) {
 	backend := &S3FileBackend{
@@ -148,19 +161,19 @@ func (b *S3FileBackend) TestConnection() error {
 		if obj.Err != nil {
 			typedErr := s3.ToErrorResponse(obj.Err)
 			if typedErr.Code != bucketNotFound {
-				return errors.Wrap(err, "unable to list objects in the s3 bucket")
+				return &S3FileBackendAuthError{DetailedError: "unable to list objects in the S3 bucket"}
 			}
 			exists = false
 		}
 	} else {
 		exists, err = b.client.BucketExists(context.Background(), b.bucket)
 		if err != nil {
-			return errors.Wrap(err, "unable to check if the s3 bucket exists")
+			return &S3FileBackendAuthError{DetailedError: "unable to check if the S3 bucket exists"}
 		}
 	}
 
 	if !exists {
-		return ErrNoS3Bucket
+		return &S3FileBackendNoBucketError{}
 	}
 	mlog.Debug("Connection to S3 or minio is good. Bucket exists.")
 	return nil