Acl integration test (#115)

* feat: Added test an integration test case for acl actions(get, put), fixed PutBucketAcl actions bugs, fixed iam bugs on getting and creating user accounts

* fix: Fixed acl unit tests

* fix: Fixed cli path in exec command in acl integration test

* fix: fixed account creation bug

auth/acl.go

@@ -42,7 +42,10 @@ type GetBucketAclOutput struct {
 }
 
 type AccessControlList struct {
-	Grants []types.Grant
+	Grants []types.Grant `xml:"Grant"`
+}
+type AccessControlPolicy struct {
+	AccessControlList AccessControlList `xml:"AccessControlList"`
 }
 
 func ParseACL(data []byte) (ACL, error) {
@@ -80,69 +83,88 @@ func ParseACLOutput(data []byte) (GetBucketAclOutput, error) {
 	}, nil
 }
 
-func UpdateACL(input *s3.PutBucketAclInput, acl ACL, iam IAMService) error {
+func UpdateACL(input *s3.PutBucketAclInput, acl ACL, iam IAMService) ([]byte, error) {
+	if input == nil {
+		return nil, s3err.GetAPIError(s3err.ErrInvalidRequest)
+	}
 	if acl.Owner != *input.AccessControlPolicy.Owner.ID {
-		return s3err.GetAPIError(s3err.ErrAccessDenied)
+		return nil, s3err.GetAPIError(s3err.ErrAccessDenied)
 	}
 
 	// if the ACL is specified, set the ACL, else replace the grantees
 	if input.ACL != "" {
 		acl.ACL = input.ACL
 		acl.Grantees = []Grantee{}
-		return nil
+	} else {
+		grantees := []Grantee{}
+		accs := []string{}
+
+		if input.GrantRead != nil {
+			fullControlList, readList, readACPList, writeList, writeACPList := []string{}, []string{}, []string{}, []string{}, []string{}
+
+			if *input.GrantFullControl != "" {
+				fullControlList = splitUnique(*input.GrantFullControl, ",")
+				fmt.Println(fullControlList)
+				for _, str := range fullControlList {
+					grantees = append(grantees, Grantee{Access: str, Permission: "FULL_CONTROL"})
+				}
+			}
+			if *input.GrantRead != "" {
+				readList = splitUnique(*input.GrantRead, ",")
+				for _, str := range readList {
+					grantees = append(grantees, Grantee{Access: str, Permission: "READ"})
+				}
+			}
+			if *input.GrantReadACP != "" {
+				readACPList = splitUnique(*input.GrantReadACP, ",")
+				for _, str := range readACPList {
+					grantees = append(grantees, Grantee{Access: str, Permission: "READ_ACP"})
+				}
+			}
+			if *input.GrantWrite != "" {
+				writeList = splitUnique(*input.GrantWrite, ",")
+				for _, str := range writeList {
+					grantees = append(grantees, Grantee{Access: str, Permission: "WRITE"})
+				}
+			}
+			if *input.GrantWriteACP != "" {
+				writeACPList = splitUnique(*input.GrantWriteACP, ",")
+				for _, str := range writeACPList {
+					grantees = append(grantees, Grantee{Access: str, Permission: "WRITE_ACP"})
+				}
+			}
+
+			accs = append(append(append(append(fullControlList, readList...), writeACPList...), readACPList...), writeList...)
+		} else {
+			cache := make(map[string]bool)
+			for _, grt := range input.AccessControlPolicy.Grants {
+				grantees = append(grantees, Grantee{Access: *grt.Grantee.ID, Permission: grt.Permission})
+				if _, ok := cache[*grt.Grantee.ID]; !ok {
+					cache[*grt.Grantee.ID] = true
+					accs = append(accs, *grt.Grantee.ID)
+				}
+			}
+		}
+
+		// Check if the specified accounts exist
+		accList, err := checkIfAccountsExist(accs, iam)
+		if err != nil {
+			return nil, err
+		}
+		if len(accList) > 0 {
+			return nil, fmt.Errorf("accounts does not exist: %s", strings.Join(accList, ", "))
+		}
+
+		acl.Grantees = grantees
+		acl.ACL = ""
 	}
 
-	grantees := []Grantee{}
-
-	fullControlList, readList, readACPList, writeList, writeACPList := []string{}, []string{}, []string{}, []string{}, []string{}
-
-	if *input.GrantFullControl != "" {
-		fullControlList = splitUnique(*input.GrantFullControl, ",")
-		fmt.Println(fullControlList)
-		for _, str := range fullControlList {
-			grantees = append(grantees, Grantee{Access: str, Permission: "FULL_CONTROL"})
-		}
-	}
-	if *input.GrantRead != "" {
-		readList = splitUnique(*input.GrantRead, ",")
-		for _, str := range readList {
-			grantees = append(grantees, Grantee{Access: str, Permission: "READ"})
-		}
-	}
-	if *input.GrantReadACP != "" {
-		readACPList = splitUnique(*input.GrantReadACP, ",")
-		for _, str := range readACPList {
-			grantees = append(grantees, Grantee{Access: str, Permission: "READ_ACP"})
-		}
-	}
-	if *input.GrantWrite != "" {
-		writeList = splitUnique(*input.GrantWrite, ",")
-		for _, str := range writeList {
-			grantees = append(grantees, Grantee{Access: str, Permission: "WRITE"})
-		}
-	}
-	if *input.GrantWriteACP != "" {
-		writeACPList = splitUnique(*input.GrantWriteACP, ",")
-		for _, str := range writeACPList {
-			grantees = append(grantees, Grantee{Access: str, Permission: "WRITE_ACP"})
-		}
-	}
-
-	accs := append(append(append(append(fullControlList, readList...), writeACPList...), readACPList...), writeList...)
-
-	// Check if the specified accounts exist
-	accList, err := checkIfAccountsExist(accs, iam)
+	result, err := json.Marshal(acl)
 	if err != nil {
-		return err
-	}
-	if len(accList) > 0 {
-		return fmt.Errorf("accounts does not exist: %s", strings.Join(accList, ", "))
+		return nil, err
 	}
 
-	acl.Grantees = grantees
-	acl.ACL = ""
-
-	return nil
+	return result, nil
 }
 
 func checkIfAccountsExist(accs []string, iam IAMService) ([]string, error) {
@@ -153,7 +175,7 @@ func checkIfAccountsExist(accs []string, iam IAMService) ([]string, error) {
 		if err != nil && err != ErrNoSuchUser {
 			return nil, fmt.Errorf("check user account: %w", err)
 		}
-		if err == nil {
+		if err == ErrNoSuchUser {
 			result = append(result, acc)
 		}
 	}

auth/iam_internal.go

@@ -76,7 +76,7 @@ func (s *IAMServiceInternal) CreateAccount(access string, account Account) error
 				return nil, fmt.Errorf("failed to parse iam: %w", err)
 			}
 		} else {
-			conf.AccessAccounts = make(map[string]Account)
+			conf = IAMConfig{AccessAccounts: map[string]Account{}}
 		}
 
 		_, ok := conf.AccessAccounts[access]
@@ -85,10 +85,11 @@ func (s *IAMServiceInternal) CreateAccount(access string, account Account) error
 		}
 		conf.AccessAccounts[access] = account
 
-		b, err := json.Marshal(s.accts)
+		b, err := json.Marshal(conf)
 		if err != nil {
 			return nil, fmt.Errorf("failed to serialize iam: %w", err)
 		}
+		s.accts = conf
 
 		return b, nil
 	})

backend/posix/posix.go

@@ -1257,7 +1257,7 @@ func (p *Posix) InitIAM() error {
 
 	_, err := os.ReadFile(iamFile)
 	if errors.Is(err, fs.ErrNotExist) {
-		b, err := json.Marshal(auth.IAMConfig{})
+		b, err := json.Marshal(auth.IAMConfig{AccessAccounts: map[string]auth.Account{}})
 		if err != nil {
 			return fmt.Errorf("marshal default iam: %w", err)
 		}

cmd/versitygw/admin.go

@@ -65,12 +65,6 @@ func adminCommand() *cli.Command {
 						Required: true,
 						Aliases:  []string{"r"},
 					},
-					&cli.StringFlag{
-						Name:    "region",
-						Usage:   "s3 region string for the user",
-						Value:   "us-east-1",
-						Aliases: []string{"rg"},
-					},
 				},
 			},
 			{
@@ -115,15 +109,15 @@ func adminCommand() *cli.Command {
 }
 
 func createUser(ctx *cli.Context) error {
-	access, secret, role, region := ctx.String("access"), ctx.String("secret"), ctx.String("role"), ctx.String("region")
-	if access == "" || secret == "" || region == "" {
+	access, secret, role := ctx.String("access"), ctx.String("secret"), ctx.String("role")
+	if access == "" || secret == "" {
 		return fmt.Errorf("invalid input parameters for the new user")
 	}
 	if role != "admin" && role != "user" {
 		return fmt.Errorf("invalid input parameter for role")
 	}
 
-	req, err := http.NewRequest(http.MethodPost, fmt.Sprintf("http://localhost:7070/create-user?access=%v&secret=%v&role=%v&region=%v", access, secret, role, region), nil)
+	req, err := http.NewRequest(http.MethodPost, fmt.Sprintf("http://localhost:7070/create-user?access=%v&secret=%v&role=%v", access, secret, role), nil)
 	if err != nil {
 		return fmt.Errorf("failed to send the request: %w", err)
 	}

cmd/versitygw/test.go

@@ -165,6 +165,13 @@ func initTestCommands() []*cli.Command {
 			gets the tags again, checks it to be empty, then removes both the object and bucket`,
 			Action: getAction(integration.TestPutGetRemoveTags),
 		},
+		{
+			Name:  "bucket-acl-actions",
+			Usage: "Tests put/get bucket actions.",
+			Description: `Creates a bucket with s3 gateway action, puts some bucket acls
+			gets the acl, verifies it, then removes the bucket`,
+			Action: getAction(integration.TestAclActions),
+		},
 		{
 			Name:        "full-flow",
 			Usage:       "Tests the full flow of gateway.",

integration/tests.go

@@ -9,6 +9,7 @@ import (
 	"io"
 	"math"
 	"os"
+	"strings"
 	"sync"
 	"time"
 
@@ -1186,6 +1187,121 @@ func TestPutGetRemoveTags(s *S3Conf) {
 	passF(testname)
 }
 
+func TestAclActions(s *S3Conf) {
+	testname := "test put/get acl"
+	runF(testname)
+
+	bucket := "testbucket1"
+
+	err := setup(s, bucket)
+	if err != nil {
+		failF("%v: %v", testname, err)
+		return
+	}
+
+	s3client := s3.NewFromConfig(s.Config())
+
+	rootAccess := s.awsID
+	rootSecret := s.awsSecret
+
+	s.awsID = "grt1"
+	s.awsSecret = "grt1secret"
+
+	userS3Client := s3.NewFromConfig(s.Config())
+
+	s.awsID = rootAccess
+	s.awsSecret = rootSecret
+
+	grt1 := "grt1"
+
+	grants := []types.Grant{
+		{
+			Permission: "READ",
+			Grantee: &types.Grantee{
+				ID:   &grt1,
+				Type: "CanonicalUser",
+			},
+		},
+	}
+
+	succUsrCrt := "The user has been created successfully"
+	failUsrCrt := "failed to create a user: update iam data: account already exists"
+
+	out, err := execCommand("admin", "-aa", s.awsID, "-as", s.awsSecret, "create-user", "--access", grt1, "--secret", "grt1secret", "--role", "user")
+	if err != nil {
+		failF("%v: %v", err)
+		return
+	}
+	if !strings.Contains(string(out), succUsrCrt) && !strings.Contains(string(out), failUsrCrt) {
+		failF("%v: failed to create user accounts", testname)
+		return
+	}
+
+	// Validation error case
+	ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+	_, err = s3client.PutBucketAcl(ctx, &s3.PutBucketAclInput{
+		Bucket: &bucket,
+		AccessControlPolicy: &types.AccessControlPolicy{
+			Grants: grants,
+		},
+		ACL: "private",
+	})
+	cancel()
+	if err == nil {
+		failF("%v: expected validation error", testname)
+		return
+	}
+
+	ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+	_, err = s3client.PutBucketAcl(ctx, &s3.PutBucketAclInput{
+		Bucket: &bucket,
+		AccessControlPolicy: &types.AccessControlPolicy{
+			Grants: grants,
+		},
+	})
+	cancel()
+	if err != nil {
+		failF("%v: %v", testname, err)
+		return
+	}
+
+	ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+	acl, err := s3client.GetBucketAcl(ctx, &s3.GetBucketAclInput{
+		Bucket: &bucket,
+	})
+	cancel()
+	if err != nil {
+		failF("%v: %v", testname, err)
+		return
+	}
+
+	if *acl.Owner.ID != s.awsID {
+		failF("%v: expected bucket owner: %v, instead got: %v", testname, s.awsID, *acl.Owner.ID)
+		return
+	}
+	if !checkGrants(acl.Grants, grants) {
+		failF("%v: expected %v, instead got %v", testname, grants, acl.Grants)
+		return
+	}
+
+	ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+	_, err = userS3Client.PutBucketAcl(ctx, &s3.PutBucketAclInput{
+		Bucket: &bucket,
+	})
+	cancel()
+	if err == nil {
+		failF("%v: expected acl access denied error", testname)
+		return
+	}
+
+	err = teardown(s, bucket)
+	if err != nil {
+		failF("%v: %v", testname, err)
+		return
+	}
+	passF(testname)
+}
+
 // Full flow test
 func TestFullFlow(s *S3Conf) {
 	// TODO: add more test cases to get 100% coverage
@@ -1202,4 +1318,5 @@ func TestFullFlow(s *S3Conf) {
 	TestRangeGet(s)
 	TestInvalidMultiParts(s)
 	TestPutGetRemoveTags(s)
+	TestAclActions(s)
 }

integration/utils.go

@@ -3,6 +3,7 @@ package integration
 import (
 	"context"
 	"fmt"
+	"os/exec"
 	"strings"
 
 	"github.com/aws/aws-sdk-go-v2/service/s3"
@@ -136,3 +137,25 @@ func areTagsSame(tags1, tags2 []types.Tag) bool {
 	}
 	return true
 }
+
+func checkGrants(grts1, grts2 []types.Grant) bool {
+	if len(grts1) != len(grts2) {
+		return false
+	}
+
+	for i, grt := range grts1 {
+		if grt.Permission != grts2[i].Permission {
+			return false
+		}
+		if *grt.Grantee.ID != *grts2[i].Grantee.ID {
+			return false
+		}
+	}
+	return true
+}
+
+func execCommand(args ...string) ([]byte, error) {
+	cmd := exec.Command("./versitygw", args...)
+
+	return cmd.CombinedOutput()
+}

s3api/controllers/base.go

@@ -41,8 +41,8 @@ type S3ApiController struct {
 	iam auth.IAMService
 }
 
-func New(be backend.Backend) S3ApiController {
-	return S3ApiController{be: be}
+func New(be backend.Backend, iam auth.IAMService) S3ApiController {
+	return S3ApiController{be: be, iam: iam}
 }
 
 func (c S3ApiController) ListBuckets(ctx *fiber.Ctx) error {
@@ -248,13 +248,54 @@ func (c S3ApiController) PutBucketActions(ctx *fiber.Ctx) error {
 
 	grants := grantFullControl + grantRead + grantReadACP + granWrite + grantWriteACP
 
-	if grants != "" || acl != "" {
-		if grants != "" && acl != "" {
-			return errors.New("wrong api call")
-		}
+	if ctx.Request().URI().QueryArgs().Has("acl") {
+		var input *s3.PutBucketAclInput
 
-		if acl != "" && acl != "private" && acl != "public-read" && acl != "public-read-write" {
-			return errors.New("wrong api call")
+		if len(ctx.Body()) > 0 {
+			if grants+acl != "" {
+				return s3err.GetAPIError(s3err.ErrInvalidRequest)
+			}
+
+			var accessControlPolicy auth.AccessControlPolicy
+			err := xml.Unmarshal(ctx.Body(), &accessControlPolicy)
+			if err != nil {
+				return s3err.GetAPIError(s3err.ErrInvalidRequest)
+			}
+
+			input = &s3.PutBucketAclInput{
+				Bucket:              &bucket,
+				ACL:                 "",
+				AccessControlPolicy: &types.AccessControlPolicy{Owner: &types.Owner{ID: &access}, Grants: accessControlPolicy.AccessControlList.Grants},
+			}
+		}
+		if acl != "" {
+			if acl != "private" && acl != "public-read" && acl != "public-read-write" {
+				return s3err.GetAPIError(s3err.ErrInvalidRequest)
+			}
+			if len(ctx.Body()) > 0 || grants != "" {
+				return s3err.GetAPIError(s3err.ErrInvalidRequest)
+			}
+
+			input = &s3.PutBucketAclInput{
+				Bucket:              &bucket,
+				ACL:                 types.BucketCannedACL(acl),
+				AccessControlPolicy: &types.AccessControlPolicy{Owner: &types.Owner{ID: &access}},
+			}
+		}
+		if grants != "" {
+			if acl != "" || len(ctx.Body()) > 0 {
+				return s3err.GetAPIError(s3err.ErrInvalidRequest)
+			}
+
+			input = &s3.PutBucketAclInput{
+				Bucket:              &bucket,
+				GrantFullControl:    &grantFullControl,
+				GrantRead:           &grantRead,
+				GrantReadACP:        &grantReadACP,
+				GrantWrite:          &granWrite,
+				GrantWriteACP:       &grantWriteACP,
+				AccessControlPolicy: &types.AccessControlPolicy{Owner: &types.Owner{ID: &access}},
+			}
 		}
 
 		data, err := c.be.GetBucketAcl(bucket)
@@ -271,18 +312,12 @@ func (c S3ApiController) PutBucketActions(ctx *fiber.Ctx) error {
 			return SendResponse(ctx, err)
 		}
 
-		input := &s3.PutBucketAclInput{
-			Bucket:              &bucket,
-			ACL:                 types.BucketCannedACL(acl),
-			GrantFullControl:    &grantFullControl,
-			GrantRead:           &grantRead,
-			GrantReadACP:        &grantReadACP,
-			GrantWrite:          &granWrite,
-			GrantWriteACP:       &grantWriteACP,
-			AccessControlPolicy: &types.AccessControlPolicy{Owner: &types.Owner{ID: &access}},
+		updAcl, err := auth.UpdateACL(input, parsedAcl, c.iam)
+		if err != nil {
+			return SendResponse(ctx, err)
 		}
 
-		err = auth.UpdateACL(input, parsedAcl, c.iam)
+		err = c.be.PutBucketAcl(bucket, updAcl)
 		return SendResponse(ctx, err)
 	}
 

s3api/controllers/base_test.go

@@ -48,7 +48,8 @@ func init() {
 
 func TestNew(t *testing.T) {
 	type args struct {
-		be backend.Backend
+		be  backend.Backend
+		iam auth.IAMService
 	}
 
 	be := backend.BackendUnsupported{}
@@ -61,16 +62,18 @@ func TestNew(t *testing.T) {
 		{
 			name: "Initialize S3 api controller",
 			args: args{
-				be: be,
+				be:  be,
+				iam: &auth.IAMServiceInternal{},
 			},
 			want: S3ApiController{
-				be: be,
+				be:  be,
+				iam: &auth.IAMServiceInternal{},
 			},
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := New(tt.args.be); !reflect.DeepEqual(got, tt.want) {
+			if got := New(tt.args.be, tt.args.iam); !reflect.DeepEqual(got, tt.want) {
 				t.Errorf("New() = %v, want %v", got, tt.want)
 			}
 		})
@@ -403,6 +406,12 @@ func TestS3ApiController_PutBucketActions(t *testing.T) {
 	}
 
 	app := fiber.New()
+	acl := auth.ACL{Owner: "valid access", ACL: "public-read-write"}
+	acldata, err := json.Marshal(acl)
+	if err != nil {
+		t.Errorf("Failed to parse the params: %v", err.Error())
+		return
+	}
 	s3ApiController := S3ApiController{
 		be: &BackendMock{
 			GetBucketAclFunc: func(bucket string) ([]byte, error) {
@@ -426,13 +435,13 @@ func TestS3ApiController_PutBucketActions(t *testing.T) {
 	app.Put("/:bucket", s3ApiController.PutBucketActions)
 
 	// Error case
-	errorReq := httptest.NewRequest(http.MethodPut, "/my-bucket", nil)
-	errorReq.Header.Set("X-Amz-Acl", "restricted")
+	errorReq := httptest.NewRequest(http.MethodPut, "/my-bucket?acl", nil)
+	errorReq.Header.Set("X-Amz-Acl", "private")
 	errorReq.Header.Set("X-Amz-Grant-Read", "read")
 
 	// PutBucketAcl success
-	aclReq := httptest.NewRequest(http.MethodPut, "/my-bucket", nil)
-	errorReq.Header.Set("X-Amz-Acl", "full")
+	aclReq := httptest.NewRequest(http.MethodPut, "/my-bucket?acl", nil)
+	aclReq.Header.Set("X-Amz-Acl", "private")
 
 	tests := []struct {
 		name       string
@@ -473,11 +482,11 @@ func TestS3ApiController_PutBucketActions(t *testing.T) {
 		resp, err := tt.app.Test(tt.args.req)
 
 		if (err != nil) != tt.wantErr {
-			t.Errorf("S3ApiController.GetActions() error = %v, wantErr %v", err, tt.wantErr)
+			t.Errorf("S3ApiController.PutBucketActions() error = %v, wantErr %v", err, tt.wantErr)
 		}
 
 		if resp.StatusCode != tt.statusCode {
-			t.Errorf("S3ApiController.GetActions() statusCode = %v, wantStatusCode = %v", resp.StatusCode, tt.statusCode)
+			t.Errorf("S3ApiController.PutBucketActions() statusCode = %v, wantStatusCode = %v", resp.StatusCode, tt.statusCode)
 		}
 	}
 }

s3api/router.go

@@ -24,7 +24,7 @@ import (
 type S3ApiRouter struct{}
 
 func (sa *S3ApiRouter) Init(app *fiber.App, be backend.Backend, iam auth.IAMService) {
-	s3ApiController := controllers.New(be)
+	s3ApiController := controllers.New(be, iam)
 	adminController := controllers.AdminController{IAMService: iam}
 
 	// TODO: think of better routing system