Mm 29605 read permission s3 bucket (#16977)

Automatic Merge

.gitignore

@@ -10,6 +10,7 @@ webapp/yarn-error.log
 mattermost.mattermost-license
 config/mattermost.mattermost-license
 config/config.json
+config/*.crt
 
 web/static/js/bundle*.js
 web/static/js/bundle*.js.map

api4/system_test.go

@@ -509,9 +509,10 @@ func TestS3TestConnection(t *testing.T) {
 		CheckInternalErrorStatus(t, resp)
 		assert.Equal(t, "api.file.test_connection.app_error", resp.Error.Id)
 
-		*config.FileSettings.AmazonS3Bucket = "shouldcreatenewbucket"
+		*config.FileSettings.AmazonS3Bucket = "shouldnotcreatenewbucket"
 		_, resp = th.SystemAdminClient.TestS3Connection(&config)
-		CheckOKStatus(t, resp)
+		CheckInternalErrorStatus(t, resp)
+		assert.Equal(t, "api.file.test_connection.app_error", resp.Error.Id)
 	})
 
 	t.Run("as restricted system admin", func(t *testing.T) {

app/server.go

@@ -529,8 +529,14 @@ func NewServer(options ...Option) (*Server, error) {
 	if appErr != nil {
 		mlog.Error("Problem with file storage settings", mlog.Err(appErr))
 	} else {
-		if nErr := backend.TestConnection(); nErr != nil {
+		nErr := backend.TestConnection()
-			mlog.Error("Problem with file storage settings", mlog.Err(nErr))
+		if nErr != nil {
+			if errors.Is(nErr, filestore.ErrNoS3Bucket) {
+				nErr = backend.(*filestore.S3FileBackend).MakeBucket()
+			}
+			if nErr != nil {
+				mlog.Error("Problem with file storage settings", mlog.Err(nErr))
+			}
 		}
 	}
 

app/server_test.go

@@ -26,6 +26,7 @@ import (
 
 	"github.com/mattermost/mattermost-server/v5/config"
 	"github.com/mattermost/mattermost-server/v5/model"
+	"github.com/mattermost/mattermost-server/v5/shared/filestore"
 	"github.com/mattermost/mattermost-server/v5/shared/mlog"
 	"github.com/mattermost/mattermost-server/v5/store/storetest"
 	"github.com/mattermost/mattermost-server/v5/utils/fileutils"
@@ -138,6 +139,46 @@ func TestStartServerPortUnavailable(t *testing.T) {
 	require.Error(t, serverErr)
 }
 
+func TestStartServerNoS3Bucket(t *testing.T) {
+	s3Host := os.Getenv("CI_MINIO_HOST")
+	if s3Host == "" {
+		s3Host = "localhost"
+	}
+
+	s3Port := os.Getenv("CI_MINIO_PORT")
+	if s3Port == "" {
+		s3Port = "9000"
+	}
+
+	s3Endpoint := fmt.Sprintf("%s:%s", s3Host, s3Port)
+
+	s, err := NewServer(func(server *Server) error {
+		configStore, _ := config.NewFileStore("config.json", true)
+		store, _ := config.NewStoreFromBacking(configStore, nil, false)
+		server.configStore = store
+		server.UpdateConfig(func(cfg *model.Config) {
+			cfg.FileSettings = model.FileSettings{
+				DriverName:              model.NewString(model.IMAGE_DRIVER_S3),
+				AmazonS3AccessKeyId:     model.NewString(model.MINIO_ACCESS_KEY),
+				AmazonS3SecretAccessKey: model.NewString(model.MINIO_SECRET_KEY),
+				AmazonS3Bucket:          model.NewString("nosuchbucket"),
+				AmazonS3Endpoint:        model.NewString(s3Endpoint),
+				AmazonS3Region:          model.NewString(""),
+				AmazonS3PathPrefix:      model.NewString(""),
+				AmazonS3SSL:             model.NewBool(false),
+			}
+		})
+		return nil
+	})
+	require.NoError(t, err)
+
+	// ensure that a new bucket was created
+	backend, err := s.FileBackend()
+	require.Nil(t, err)
+	err = backend.(*filestore.S3FileBackend).TestConnection()
+	require.Nil(t, err)
+}
+
 func TestStartServerTLSSuccess(t *testing.T) {
 	s, err := NewServer()
 	require.NoError(t, err)

shared/filestore/s3store.go

@@ -41,6 +41,8 @@ const (
 	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"}
@@ -157,16 +159,18 @@ func (b *S3FileBackend) TestConnection() error {
 		}
 	}
 
-	if exists {
+	if !exists {
-		mlog.Debug("Connection to S3 or minio is good. Bucket exists.")
+		return ErrNoS3Bucket
-	} else {
-		mlog.Warn("Bucket specified does not exist. Attempting to create...")
-		err := b.client.MakeBucket(context.Background(), b.bucket, s3.MakeBucketOptions{Region: b.region})
-		if err != nil {
-			return errors.Wrap(err, "unable to create the s3 bucket")
-		}
 	}
+	mlog.Debug("Connection to S3 or minio is good. Bucket exists.")
+	return nil
+}
 
+func (b *S3FileBackend) MakeBucket() error {
+	err := b.client.MakeBucket(context.Background(), b.bucket, s3.MakeBucketOptions{Region: b.region})
+	if err != nil {
+		return errors.Wrap(err, "unable to create the s3 bucket")
+	}
 	return nil
 }
 

shared/filestore/s3store_test.go

@@ -4,11 +4,23 @@
 package filestore
 
 import (
+	"crypto/rand"
+	"encoding/base64"
+	"fmt"
+	"os"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 )
 
+// Copied from model/config.go to avoid an import cycle
+const (
+	MinioAccessKey = "minioaccesskey"
+	MinioSecretKey = "miniosecretkey"
+	ImageDriverS3  = "amazons3"
+)
+
 func TestCheckMandatoryS3Fields(t *testing.T) {
 	cfg := FileBackendSettings{}
 
@@ -26,3 +38,42 @@ func TestCheckMandatoryS3Fields(t *testing.T) {
 
 	require.Equal(t, "s3.amazonaws.com", cfg.AmazonS3Endpoint, "should've set the endpoint to the default")
 }
+
+func TestMakeBucket(t *testing.T) {
+	s3Host := os.Getenv("CI_MINIO_HOST")
+	if s3Host == "" {
+		s3Host = "localhost"
+	}
+
+	s3Port := os.Getenv("CI_MINIO_PORT")
+	if s3Port == "" {
+		s3Port = "9000"
+	}
+
+	s3Endpoint := fmt.Sprintf("%s:%s", s3Host, s3Port)
+
+	// Generate a random bucket name
+	b := make([]byte, 30)
+	rand.Read(b)
+	bucketName := base64.StdEncoding.EncodeToString(b)
+	bucketName = strings.ToLower(bucketName)
+	bucketName = strings.Replace(bucketName, "+", "", -1)
+	bucketName = strings.Replace(bucketName, "/", "", -1)
+
+	cfg := FileBackendSettings{
+		DriverName:              ImageDriverS3,
+		AmazonS3AccessKeyId:     MinioAccessKey,
+		AmazonS3SecretAccessKey: MinioSecretKey,
+		AmazonS3Bucket:          bucketName,
+		AmazonS3Endpoint:        s3Endpoint,
+		AmazonS3Region:          "",
+		AmazonS3PathPrefix:      "",
+		AmazonS3SSL:             false,
+	}
+
+	fileBackend, err := NewS3FileBackend(cfg)
+	require.NoError(t, err)
+
+	err = fileBackend.MakeBucket()
+	require.NoError(t, err)
+}