From 0393f816c4328b2f12aa681350686f867cfef775 Mon Sep 17 00:00:00 2001 From: Shruthi-1MN Date: Wed, 20 Nov 2019 16:40:47 +0530 Subject: [PATCH] File share name fixes --- pkg/api/controllers/fileshare.go | 29 +++++++++ pkg/api/controllers/fileshare_test.go | 89 +++++++++++++++++++++++++++ pkg/api/util/db.go | 27 +++++++- pkg/api/util/db_test.go | 54 +++++++++++++++- pkg/utils/utils.go | 22 +++++++ test/e2e/e2e_test.go | 6 +- testutils/collection/data.go | 4 +- 7 files changed, 221 insertions(+), 10 deletions(-) diff --git a/pkg/api/controllers/fileshare.go b/pkg/api/controllers/fileshare.go index f55843ef2..137d55bd2 100644 --- a/pkg/api/controllers/fileshare.go +++ b/pkg/api/controllers/fileshare.go @@ -371,6 +371,35 @@ func (f *FileSharePortal) UpdateFileShare() { return } + if fshare.Name == "" { + errMsg := fmt.Sprintf("empty fileshare name is not allowed. Please give valid name.") + log.Error(errMsg) + f.ErrorHandle(model.ErrorBadRequest, errMsg) + return + } + if len(fshare.Name) > 255 { + errMsg := fmt.Sprintf("fileshare name length should not be more than 255 characters. input name length is : %d", len(fshare.Name)) + log.Error(errMsg) + f.ErrorHandle(model.ErrorBadRequest, errMsg) + return + } + reg, err := utils.Special_Character_Regex_Match_Pattern() + if err != nil { + log.Error(err) + return + } + if reg.MatchString(fshare.Name) { + errMsg := fmt.Sprintf("invalid fileshare name because it has some special characters") + log.Error(errMsg) + f.ErrorHandle(model.ErrorBadRequest, errMsg) + return + } + if reg.MatchString(fshare.Description) { + errMsg := fmt.Sprintf("invalid fileshare description and it has some special characters") + log.Error(errMsg) + f.ErrorHandle(model.ErrorBadRequest, errMsg) + return + } fshare.Id = id result, err := db.C.UpdateFileShare(c.GetContext(f.Ctx), &fshare) if err != nil { diff --git a/pkg/api/controllers/fileshare_test.go b/pkg/api/controllers/fileshare_test.go index 03cd8fbf6..74bad2127 100644 --- a/pkg/api/controllers/fileshare_test.go +++ b/pkg/api/controllers/fileshare_test.go @@ -29,6 +29,7 @@ import ( "github.com/opensds/opensds/pkg/db" "github.com/opensds/opensds/pkg/model" pb "github.com/opensds/opensds/pkg/model/proto" + "github.com/opensds/opensds/pkg/utils" . "github.com/opensds/opensds/testutils/collection" ctrtest "github.com/opensds/opensds/testutils/controller/testing" dbtest "github.com/opensds/opensds/testutils/db/testing" @@ -208,6 +209,94 @@ func TestUpdateFileShare(t *testing.T) { assertTestResult(t, &output, &expected) }) + t.Run("Should return 400 empty fileshare name is not allowed", func(t *testing.T) { + var jsonStr = []byte(`{ + "name":"", + "description":"fake fileshare" + }`) + share := model.FileShareSnapshotSpec{BaseModel: &model.BaseModel{}} + json.NewDecoder(bytes.NewBuffer(jsonStr)).Decode(&share) + mockClient := new(dbtest.Client) + mockClient.On("UpdateFileShare", c.NewAdminContext(), share.Id, &share). + Return(&expected, nil) + db.C = mockClient + + r, _ := http.NewRequest("PUT", "/v1beta/file/shares/3769855c-a102-11e7-b772-17b880d2f537", bytes.NewBuffer(jsonStr)) + w := httptest.NewRecorder() + r.Header.Set("Content-Type", "application/JSON") + beego.InsertFilter("*", beego.BeforeExec, func(httpCtx *context.Context) { + httpCtx.Input.SetData("context", c.NewAdminContext()) + }) + beego.BeeApp.Handlers.ServeHTTP(w, r) + assertTestResult(t, w.Code, 400) + }) + + t.Run("Should return 400 fileshare name length should not be more than 255 characters", func(t *testing.T) { + var jsonStr = []byte(`{ + "description":"fake fileshare" + }`) + share := model.FileShareSnapshotSpec{BaseModel: &model.BaseModel{}} + share.Name = utils.RandomString(258) + json.NewDecoder(bytes.NewBuffer(jsonStr)).Decode(&share) + mockClient := new(dbtest.Client) + mockClient.On("UpdateFileShare", c.NewAdminContext(), share.Id, &share). + Return(&expected, nil) + db.C = mockClient + + r, _ := http.NewRequest("PUT", "/v1beta/file/shares/3769855c-a102-11e7-b772-17b880d2f537", bytes.NewBuffer(jsonStr)) + w := httptest.NewRecorder() + r.Header.Set("Content-Type", "application/JSON") + beego.InsertFilter("*", beego.BeforeExec, func(httpCtx *context.Context) { + httpCtx.Input.SetData("context", c.NewAdminContext()) + }) + beego.BeeApp.Handlers.ServeHTTP(w, r) + assertTestResult(t, w.Code, 400) + }) + + t.Run("Should return 400 invalid fileshare name because it has some special characters", func(t *testing.T) { + var jsonStr = []byte(`{ + "description":"fake fileshare" + }`) + share := model.FileShareSnapshotSpec{BaseModel: &model.BaseModel{}} + share.Name = "#Share !$!test" + json.NewDecoder(bytes.NewBuffer(jsonStr)).Decode(&share) + mockClient := new(dbtest.Client) + mockClient.On("UpdateFileShare", c.NewAdminContext(), share.Id, &share). + Return(&expected, nil) + db.C = mockClient + + r, _ := http.NewRequest("PUT", "/v1beta/file/shares/3769855c-a102-11e7-b772-17b880d2f537", bytes.NewBuffer(jsonStr)) + w := httptest.NewRecorder() + r.Header.Set("Content-Type", "application/JSON") + beego.InsertFilter("*", beego.BeforeExec, func(httpCtx *context.Context) { + httpCtx.Input.SetData("context", c.NewAdminContext()) + }) + beego.BeeApp.Handlers.ServeHTTP(w, r) + assertTestResult(t, w.Code, 400) + }) + + t.Run("Should return 400 invalid fileshare description and it has some special characters", func(t *testing.T) { + var jsonStr = []byte(`{ + "name": "sampletestfileshare01", + "description":"#Share !$!test" + }`) + share := model.FileShareSnapshotSpec{BaseModel: &model.BaseModel{}} + json.NewDecoder(bytes.NewBuffer(jsonStr)).Decode(&share) + mockClient := new(dbtest.Client) + mockClient.On("UpdateFileShare", c.NewAdminContext(), share.Id, &share). + Return(&expected, nil) + db.C = mockClient + + r, _ := http.NewRequest("PUT", "/v1beta/file/shares/3769855c-a102-11e7-b772-17b880d2f537", bytes.NewBuffer(jsonStr)) + w := httptest.NewRecorder() + r.Header.Set("Content-Type", "application/JSON") + beego.InsertFilter("*", beego.BeforeExec, func(httpCtx *context.Context) { + httpCtx.Input.SetData("context", c.NewAdminContext()) + }) + beego.BeeApp.Handlers.ServeHTTP(w, r) + assertTestResult(t, w.Code, 400) + }) + t.Run("Should return 500 if update file share with bad request", func(t *testing.T) { fileshare := model.FileShareSpec{BaseModel: &model.BaseModel{}} json.NewDecoder(bytes.NewBuffer(jsonStr)).Decode(&fileshare) diff --git a/pkg/api/util/db.go b/pkg/api/util/db.go index ddec18623..d837fb783 100644 --- a/pkg/api/util/db.go +++ b/pkg/api/util/db.go @@ -22,7 +22,6 @@ import ( "errors" "fmt" "net" - "regexp" "strings" "time" @@ -182,10 +181,32 @@ func CreateFileShareDBEntry(ctx *c.Context, in *model.FileShareSpec) (*model.Fil return nil, errors.New(errMsg) } + shares, err := db.C.ListFileShares(ctx) + if err != nil { + return nil, err + } else { + for _, fshare := range shares { + if fshare.Name == in.Name { + errMsg := fmt.Sprintf("file share name already exists") + log.Error(errMsg) + return nil, errors.New(errMsg) + } + if fshare.Id == in.Id { + errMsg := fmt.Sprintf("file share id already exists") + log.Error(errMsg) + return nil, errors.New(errMsg) + } + } + } + // validate the description - reg, err := regexp.Compile("[^a-zA-Z0-9 ]+") + reg, err := utils.Special_Character_Regex_Match_Pattern() if err != nil { - errMsg := fmt.Sprintf("regex compilation for file share description validation failed") + log.Error(err) + return nil, err + } + if reg.MatchString(in.Name) { + errMsg := fmt.Sprintf("invalid fileshare name because it has some special characters") log.Error(errMsg) return nil, errors.New(errMsg) } diff --git a/pkg/api/util/db_test.go b/pkg/api/util/db_test.go index 22ac5a054..6cf187f44 100644 --- a/pkg/api/util/db_test.go +++ b/pkg/api/util/db_test.go @@ -380,7 +380,7 @@ func TestCreateFileShareSnapshotDBEntry(t *testing.T) { func TestCreateFileShareDBEntry(t *testing.T) { var in = &model.FileShareSpec{ BaseModel: &model.BaseModel{}, - Name: "sample-fileshare-01", + Name: "samplefileshare01", Description: "This is a sample fileshare for testing", Size: int64(1), ProfileId: "b3585ebe-c42c-120g-b28e-f373746a71ca", @@ -390,6 +390,7 @@ func TestCreateFileShareDBEntry(t *testing.T) { t.Run("Everything should work well", func(t *testing.T) { mockClient := new(dbtest.Client) mockClient.On("CreateFileShare", context.NewAdminContext(), in).Return(&SampleFileShares[0], nil) + mockClient.On("ListFileShares", context.NewAdminContext()).Return(nil, nil) db.C = mockClient var expected = &SampleFileShares[0] @@ -404,6 +405,7 @@ func TestCreateFileShareDBEntry(t *testing.T) { in.Size = int64(-2) mockClient := new(dbtest.Client) mockClient.On("CreateFileShare", context.NewAdminContext(), in).Return(&SampleFileShares[0], nil) + mockClient.On("ListFileShares", context.NewAdminContext()).Return(nil, nil) db.C = mockClient _, err := CreateFileShareDBEntry(context.NewAdminContext(), in) @@ -415,6 +417,7 @@ func TestCreateFileShareDBEntry(t *testing.T) { in.ProfileId = "" mockClient := new(dbtest.Client) mockClient.On("CreateFileShare", context.NewAdminContext(), in).Return(&SampleFileShares[0], nil) + mockClient.On("ListFileShares", context.NewAdminContext()).Return(nil, nil) db.C = mockClient _, err := CreateFileShareDBEntry(context.NewAdminContext(), in) @@ -426,6 +429,7 @@ func TestCreateFileShareDBEntry(t *testing.T) { in.Size, in.Name, in.ProfileId = int64(1), "", "b3585ebe-c42c-120g-b28e-f373746a71ca" mockClient := new(dbtest.Client) mockClient.On("CreateFileShare", context.NewAdminContext(), in).Return(&SampleFileShares[0], nil) + mockClient.On("ListFileShares", context.NewAdminContext()).Return(nil, nil) db.C = mockClient _, err := CreateFileShareDBEntry(context.NewAdminContext(), in) @@ -438,6 +442,7 @@ func TestCreateFileShareDBEntry(t *testing.T) { in.Size, in.ProfileId = int64(1), "b3585ebe-c42c-120g-b28e-f373746a71ca" mockClient := new(dbtest.Client) mockClient.On("CreateFileShare", context.NewAdminContext(), in).Return(&SampleFileShares[0], nil) + mockClient.On("ListFileShares", context.NewAdminContext()).Return(nil, nil) db.C = mockClient _, err := CreateFileShareDBEntry(context.NewAdminContext(), in) @@ -450,6 +455,7 @@ func TestCreateFileShareDBEntry(t *testing.T) { in.Size, in.ProfileId = int64(1), "b3585ebe-c42c-120g-b28e-f373746a71ca" mockClient := new(dbtest.Client) mockClient.On("CreateFileShare", context.NewAdminContext(), in).Return(&SampleFileShares[0], nil) + mockClient.On("ListFileShares", context.NewAdminContext()).Return(nil, nil) db.C = mockClient var expected = &SampleFileShares[0] @@ -465,6 +471,7 @@ func TestCreateFileShareDBEntry(t *testing.T) { in.Size, in.ProfileId = int64(1), "b3585ebe-c42c-120g-b28e-f373746a71ca" mockClient := new(dbtest.Client) mockClient.On("CreateFileShare", context.NewAdminContext(), in).Return(&SampleFileShares[0], nil) + mockClient.On("ListFileShares", context.NewAdminContext()).Return(nil, nil) db.C = mockClient var expected = &SampleFileShares[0] @@ -480,6 +487,7 @@ func TestCreateFileShareDBEntry(t *testing.T) { in.Size, in.ProfileId = int64(1), "b3585ebe-c42c-120g-b28e-f373746a71ca" mockClient := new(dbtest.Client) mockClient.On("CreateFileShare", context.NewAdminContext(), in).Return(&SampleFileShares[0], nil) + mockClient.On("ListFileShares", context.NewAdminContext()).Return(nil, nil) db.C = mockClient var expected = &SampleFileShares[0] @@ -495,6 +503,7 @@ func TestCreateFileShareDBEntry(t *testing.T) { in.Size, in.ProfileId = int64(1), "b3585ebe-c42c-120g-b28e-f373746a71ca" mockClient := new(dbtest.Client) mockClient.On("CreateFileShare", context.NewAdminContext(), in).Return(&SampleFileShares[0], nil) + mockClient.On("ListFileShares", context.NewAdminContext()).Return(nil, nil) db.C = mockClient var expected = &SampleFileShares[0] @@ -510,6 +519,7 @@ func TestCreateFileShareDBEntry(t *testing.T) { in.Size, in.ProfileId = int64(1), "b3585ebe-c42c-120g-b28e-f373746a71ca" mockClient := new(dbtest.Client) mockClient.On("CreateFileShare", context.NewAdminContext(), in).Return(&SampleFileShares[0], nil) + mockClient.On("ListFileShares", context.NewAdminContext()).Return(nil, nil) db.C = mockClient _, err := CreateFileShareDBEntry(context.NewAdminContext(), in) @@ -522,6 +532,19 @@ func TestCreateFileShareDBEntry(t *testing.T) { in.Size, in.ProfileId = int64(1), "b3585ebe-c42c-120g-b28e-f373746a71ca" mockClient := new(dbtest.Client) mockClient.On("CreateFileShare", context.NewAdminContext(), in).Return(&SampleFileShares[0], nil) + mockClient.On("ListFileShares", context.NewAdminContext()).Return(nil, nil) + db.C = mockClient + + _, err := CreateFileShareDBEntry(context.NewAdminContext(), in) + expectedError := "fileshare name length should not be more than 255 characters. input name length is : " + strconv.Itoa(len(in.Name)) + assertTestResult(t, err.Error(), expectedError) + }) + + t.Run("Special characters in file share name are not allowed", func(t *testing.T) { + in.Size, in.Name, in.ProfileId, in.Description = int64(1), "#Share !$!test", "b3585ebe-c42c-120g-b28e-f373746a71ca", "File share test" + mockClient := new(dbtest.Client) + mockClient.On("CreateFileShare", context.NewAdminContext(), in).Return(&SampleFileShares[0], nil) + mockClient.On("ListFileShares", context.NewAdminContext()).Return(nil, nil) db.C = mockClient _, err := CreateFileShareDBEntry(context.NewAdminContext(), in) @@ -530,15 +553,42 @@ func TestCreateFileShareDBEntry(t *testing.T) { }) t.Run("Special characters in file share description are not allowed", func(t *testing.T) { - in.Size, in.Name, in.ProfileId, in.Description = int64(1), "sample-fileshare-01", "b3585ebe-c42c-120g-b28e-f373746a71ca", "#FileShare Code!$! test" + in.Size, in.Name, in.ProfileId, in.Description = int64(1), "test tmp", "b3585ebe-c42c-120g-b28e-f373746a71ca", "#Share !$!test" mockClient := new(dbtest.Client) mockClient.On("CreateFileShare", context.NewAdminContext(), in).Return(&SampleFileShares[0], nil) + mockClient.On("ListFileShares", context.NewAdminContext()).Return(nil, nil) db.C = mockClient _, err := CreateFileShareDBEntry(context.NewAdminContext(), in) expectedError := "invalid fileshare description and it has some special characters" assertTestResult(t, err.Error(), expectedError) }) + + t.Run("Duplicate file share name are not allowed", func(t *testing.T) { + var sampleshares = []*model.FileShareSpec{&SampleFileShares[0]} + in.Size, in.Name, in.ProfileId, in.Description = int64(1), "samplefileshare01", "b3585ebe-c42c-120g-b28e-f373746a71ca", "File share test" + mockClient := new(dbtest.Client) + mockClient.On("CreateFileShare", context.NewAdminContext(), in).Return(&SampleFileShares[0], nil) + mockClient.On("ListFileShares", context.NewAdminContext()).Return(sampleshares, nil) + db.C = mockClient + + _, err := CreateFileShareDBEntry(context.NewAdminContext(), in) + expectedError := "file share name already exists" + assertTestResult(t, err.Error(), expectedError) + }) + + t.Run("Overwritting existing file share resource are not allowed", func(t *testing.T) { + var sampleshares = []*model.FileShareSpec{&SampleFileShares[0]} + in.Size, in.Name, in.ProfileId, in.Description, in.Id = int64(1), "samplefileshare05", "b3585ebe-c42c-120g-b28e-f373746a71ca", "File share test", "d2975ebe-d82c-430f-b28e-f373746a71ca" + mockClient := new(dbtest.Client) + mockClient.On("CreateFileShare", context.NewAdminContext(), in).Return(&SampleFileShares[0], nil) + mockClient.On("ListFileShares", context.NewAdminContext()).Return(sampleshares, nil) + db.C = mockClient + + _, err := CreateFileShareDBEntry(context.NewAdminContext(), in) + expectedError := "file share id already exists" + assertTestResult(t, err.Error(), expectedError) + }) } func TestDeleteFileShareDBEntry(t *testing.T) { diff --git a/pkg/utils/utils.go b/pkg/utils/utils.go index 970bfe443..d0b5c71c4 100755 --- a/pkg/utils/utils.go +++ b/pkg/utils/utils.go @@ -23,6 +23,7 @@ import ( "reflect" "sort" "strings" + "regexp" "time" "github.com/opensds/opensds/pkg/utils/constants" @@ -327,3 +328,24 @@ func WaitForCondition(f func() (bool, error), interval, timeout time.Duration) e } return fmt.Errorf("wait for condition timeout") } + +func RandomString(n int) string { + var letter = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789") + + b := make([]rune, n) + for i := range b { + b[i] = letter[rand.Intn(len(letter))] + } + return string(b) +} + +func Special_Character_Regex_Match_Pattern() (*regexp.Regexp, error) { + reg, err := regexp.Compile(`[^a-zA-Z0-9 ]+`) + if err != nil { + errMsg := fmt.Sprintf("regex compilation validation failed") + log.Error(errMsg) + return nil, errors.New(errMsg) + } else { + return reg, nil + } +} \ No newline at end of file diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 7dc2ed715..0d1f2221b 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -714,7 +714,7 @@ func TestUpdateVolumeGroup(t *testing.T) { func TestCreateFileShare(t *testing.T) { t.Log("Start creating file share...") var body = &model.FileShareSpec{ - Name: "share_test", + Name: "sharetest", Description: "This is a test", Size: int64(1), } @@ -743,7 +743,7 @@ func prepareGetFileShare(t *testing.T) (string, error) { } for _, item := range filesharelst { - if item.Name == "share_test" { + if item.Name == "sharetest" { fileshare_id = item.Id } } @@ -792,7 +792,7 @@ func prepareGetShare(t *testing.T) (string, error) { } for _, item := range filesharelst { - if item.Name == "share_test" { + if item.Name == "sharetest" { fileshare_id = item.Id } } diff --git a/testutils/collection/data.go b/testutils/collection/data.go index 89aa998f2..0ed1c4205 100644 --- a/testutils/collection/data.go +++ b/testutils/collection/data.go @@ -228,7 +228,7 @@ var ( BaseModel: &model.BaseModel{ Id: "d2975ebe-d82c-430f-b28e-f373746a71ca", }, - Name: "sample-fileshare-01", + Name: "samplefileshare01", Description: "This is first sample fileshare for testing", Size: int64(1), Status: "available", @@ -242,7 +242,7 @@ var ( BaseModel: &model.BaseModel{ Id: "1e643aca-4922-4b1a-bb98-4245054aeff4", }, - Name: "sample-fileshare-2", + Name: "samplefileshare2", Description: "This is second sample fileshare for testing", Size: int64(1), Status: "available",