From e15308e84074395bb486cb58f70a722d78875238 Mon Sep 17 00:00:00 2001
From: Agniva De Sarker <agnivade@yahoo.co.in>
Date: Fri, 2 Oct 2020 00:10:28 +0530
Subject: [PATCH] MM-29033: Modify Test S3 connection to handle path prefix
 (#15659)

* MM-29033: Modify Test S3 connection to handle path prefix

When a IAM user is just allowed access to a specific path prefix,
the bucket exists test will fail because that is applicable to the entire bucket.

We modify that check to just list objects under that path prefix and return
on the first non-nil error.

Another consideration was to do a StatObject, but it is only applicable to actual
objects and not at a directory level. Path prefixes are entirely logical and depends
on the underlying implementation. Therefore, ListObjects is a better choice.

https://mattermost.atlassian.net/browse/MM-29033

* Improve logic

* tmp

* Only receive
---
 i18n/en.json                   |  6 +++++-
 services/filesstore/s3store.go | 35 +++++++++++++++++++++++++++-------
 2 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/i18n/en.json b/i18n/en.json
index b475c409b0..6a15967ce6 100644
--- a/i18n/en.json
+++ b/i18n/en.json
@@ -1409,7 +1409,7 @@
     "translation": "Don't have permissions to write to local path specified or other error."
   },
   {
-    "id": "api.file.test_connection.s3.bucked_create.app_error",
+    "id": "api.file.test_connection.s3.bucket_create.app_error",
     "translation": "Unable to create bucket."
   },
   {
@@ -1420,6 +1420,10 @@
     "id": "api.file.test_connection.s3.connection.app_error",
     "translation": "Bad connection to S3 or minio."
   },
+  {
+    "id": "api.file.test_connection.s3.list_objects.app_error",
+    "translation": "Error trying to list objects."
+  },
   {
     "id": "api.file.upload_file.incorrect_channelId.app_error",
     "translation": "Unable to upload the file. Incorrect channel ID: {{.channelId}}"
diff --git a/services/filesstore/s3store.go b/services/filesstore/s3store.go
index 7750dd4a8b..465973adfe 100644
--- a/services/filesstore/s3store.go
+++ b/services/filesstore/s3store.go
@@ -34,6 +34,11 @@ type S3FileBackend struct {
 	trace      bool
 }
 
+const (
+	// This is not exported by minio. See: https://github.com/minio/minio-go/issues/1339
+	bucketNotFound = "NoSuchBucket"
+)
+
 // Similar to s3.New() but allows initialization of signature v2 or signature v4 client.
 // If signV2 input is false, function always returns signature v4.
 //
@@ -73,20 +78,36 @@ func (b *S3FileBackend) TestConnection() *model.AppError {
 		return model.NewAppError("TestFileConnection", "api.file.test_connection.s3.connection.app_error", nil, err.Error(), http.StatusInternalServerError)
 	}
 
-	exists, err := s3Clnt.BucketExists(context.Background(), b.bucket)
-	if err != nil {
-		return model.NewAppError("TestFileConnection", "api.file.test_connection.s3.bucket_exists.app_error", nil, err.Error(), http.StatusInternalServerError)
+	exists := true
+	// If a path prefix is present, we attempt to test the bucket by listing objects under the path
+	// and just checking the first response. This is because the BucketExists call is only at a bucket level
+	// and sometimes the user might only be allowed access to the specified path prefix.
+	if b.pathPrefix != "" {
+		obj := <-s3Clnt.ListObjects(context.Background(), b.bucket, s3.ListObjectsOptions{Prefix: b.pathPrefix})
+		if obj.Err != nil {
+			typedErr := s3.ToErrorResponse(obj.Err)
+			if typedErr.Code != bucketNotFound {
+				return model.NewAppError("TestFileConnection", "api.file.test_connection.s3.list_objects.app_error", nil, obj.Err.Error(), http.StatusInternalServerError)
+			}
+			exists = false
+		}
+	} else {
+		exists, err = s3Clnt.BucketExists(context.Background(), b.bucket)
+		if err != nil {
+			return model.NewAppError("TestFileConnection", "api.file.test_connection.s3.bucket_exists.app_error", nil, err.Error(), http.StatusInternalServerError)
+		}
 	}
 
-	if !exists {
+	if exists {
+		mlog.Debug("Connection to S3 or minio is good. Bucket exists.")
+	} else {
 		mlog.Warn("Bucket specified does not exist. Attempting to create...")
 		err := s3Clnt.MakeBucket(context.Background(), b.bucket, s3.MakeBucketOptions{Region: b.region})
 		if err != nil {
-			mlog.Error("Unable to create bucket.")
-			return model.NewAppError("TestFileConnection", "api.file.test_connection.s3.bucked_create.app_error", nil, err.Error(), http.StatusInternalServerError)
+			return model.NewAppError("TestFileConnection", "api.file.test_connection.s3.bucket_create.app_error", nil, err.Error(), http.StatusInternalServerError)
 		}
 	}
-	mlog.Debug("Connection to S3 or minio is good. Bucket exists.")
+
 	return nil
 }