Merge pull request #671 from versity/fix/change-bucket-owner-ownership

ChangeBucketOwner acl update
versity · Jul 11, 2024 · 5d33c7b · 5d33c7b
2 parents 85dc8e7 + 2843cdb
commit 5d33c7b
Show file tree

Hide file tree

Showing 9 changed files with 81 additions and 77 deletions.
diff --git a/backend/azure/azure.go b/backend/azure/azure.go
@@ -1368,34 +1368,8 @@ func (az *Azure) GetObjectLegalHold(ctx context.Context, bucket, object, version
 	return &status, nil
 }
 
-func (az *Azure) ChangeBucketOwner(ctx context.Context, bucket, newOwner string) error {
-	client, err := az.getContainerClient(bucket)
-	if err != nil {
-		return err
-	}
-	props, err := client.GetProperties(ctx, nil)
-	if err != nil {
-		return azureErrToS3Err(err)
-	}
-
-	acl, err := getAclFromMetadata(props.Metadata, keyAclCapital)
-	if err != nil {
-		return err
-	}
-
-	acl.Owner = newOwner
-
-	newAcl, err := json.Marshal(acl)
-	if err != nil {
-		return fmt.Errorf("marshal acl: %w", err)
-	}
-
-	err = az.PutBucketAcl(ctx, bucket, newAcl)
-	if err != nil {
-		return err
-	}
-
-	return nil
+func (az *Azure) ChangeBucketOwner(ctx context.Context, bucket string, acl []byte) error {
+	return az.PutBucketAcl(ctx, bucket, acl)
 }
 
 // The action actually returns the containers owned by the user, who initialized the gateway

diff --git a/backend/backend.go b/backend/backend.go
@@ -93,7 +93,7 @@ type Backend interface {
 	GetObjectLegalHold(_ context.Context, bucket, object, versionId string) (*bool, error)
 
 	// non AWS actions
-	ChangeBucketOwner(_ context.Context, bucket, newOwner string) error
+	ChangeBucketOwner(_ context.Context, bucket string, acl []byte) error
 	ListBucketsAndOwners(context.Context) ([]s3response.Bucket, error)
 }
 
@@ -268,7 +268,7 @@ func (BackendUnsupported) GetObjectLegalHold(_ context.Context, bucket, object,
 	return nil, s3err.GetAPIError(s3err.ErrNotImplemented)
 }
 
-func (BackendUnsupported) ChangeBucketOwner(_ context.Context, bucket, newOwner string) error {
+func (BackendUnsupported) ChangeBucketOwner(_ context.Context, bucket string, acl []byte) error {
 	return s3err.GetAPIError(s3err.ErrNotImplemented)
 }
 func (BackendUnsupported) ListBucketsAndOwners(context.Context) ([]s3response.Bucket, error) {

diff --git a/backend/posix/posix.go b/backend/posix/posix.go
@@ -2619,39 +2619,8 @@ func (p *Posix) GetObjectRetention(_ context.Context, bucket, object, versionId
 	return data, nil
 }
 
-func (p *Posix) ChangeBucketOwner(ctx context.Context, bucket, newOwner string) error {
-	_, err := os.Stat(bucket)
-	if errors.Is(err, fs.ErrNotExist) {
-		return s3err.GetAPIError(s3err.ErrNoSuchBucket)
-	}
-	if err != nil {
-		return fmt.Errorf("stat bucket: %w", err)
-	}
-
-	aclTag, err := p.meta.RetrieveAttribute(bucket, "", aclkey)
-	if err != nil {
-		return fmt.Errorf("get acl: %w", err)
-	}
-
-	var acl auth.ACL
-	err = json.Unmarshal(aclTag, &acl)
-	if err != nil {
-		return fmt.Errorf("unmarshal acl: %w", err)
-	}
-
-	acl.Owner = newOwner
-
-	newAcl, err := json.Marshal(acl)
-	if err != nil {
-		return fmt.Errorf("marshal acl: %w", err)
-	}
-
-	err = p.meta.StoreAttribute(bucket, "", aclkey, newAcl)
-	if err != nil {
-		return fmt.Errorf("set acl: %w", err)
-	}
-
-	return nil
+func (p *Posix) ChangeBucketOwner(ctx context.Context, bucket string, acl []byte) error {
+	return p.PutBucketAcl(ctx, bucket, acl)
 }
 
 func (p *Posix) ListBucketsAndOwners(ctx context.Context) (buckets []s3response.Bucket, err error) {

diff --git a/backend/s3proxy/s3.go b/backend/s3proxy/s3.go
@@ -619,8 +619,12 @@ func (s *S3Proxy) GetObjectLegalHold(ctx context.Context, bucket, object, versio
 	return &status, nil
 }
 
-func (s *S3Proxy) ChangeBucketOwner(ctx context.Context, bucket, newOwner string) error {
-	req, err := http.NewRequest(http.MethodPatch, fmt.Sprintf("%v/change-bucket-owner/?bucket=%v&owner=%v", s.endpoint, bucket, newOwner), nil)
+func (s *S3Proxy) ChangeBucketOwner(ctx context.Context, bucket string, acl []byte) error {
+	var acll auth.ACL
+	if err := json.Unmarshal(acl, &acll); err != nil {
+		return fmt.Errorf("unmarshal acl: %w", err)
+	}
+	req, err := http.NewRequest(http.MethodPatch, fmt.Sprintf("%v/change-bucket-owner/?bucket=%v&owner=%v", s.endpoint, bucket, acll.Owner), nil)
 	if err != nil {
 		return fmt.Errorf("failed to send the request: %w", err)
 	}

diff --git a/s3api/controllers/admin.go b/s3api/controllers/admin.go
@@ -19,6 +19,7 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/aws/aws-sdk-go-v2/service/s3/types"
 	"github.com/gofiber/fiber/v2"
 	"github.com/versity/versitygw/auth"
 	"github.com/versity/versitygw/backend"
@@ -138,7 +139,22 @@ func (c AdminController) ChangeBucketOwner(ctx *fiber.Ctx) error {
 		return ctx.Status(fiber.StatusNotFound).SendString("user specified as the new bucket owner does not exist")
 	}
 
-	err = c.be.ChangeBucketOwner(ctx.Context(), bucket, owner)
+	acl := auth.ACL{
+		Owner: owner,
+		Grantees: []auth.Grantee{
+			{
+				Permission: types.PermissionFullControl,
+				Access:     owner,
+			},
+		},
+	}
+
+	aclParsed, err := json.Marshal(acl)
+	if err != nil {
+		return fmt.Errorf("failed to marshal the bucket acl: %w", err)
+	}
+
+	err = c.be.ChangeBucketOwner(ctx.Context(), bucket, aclParsed)
 	if err != nil {
 		return err
 	}

diff --git a/s3api/controllers/admin_test.go b/s3api/controllers/admin_test.go
@@ -407,7 +407,7 @@ func TestAdminController_ChangeBucketOwner(t *testing.T) {
 	}
 	adminController := AdminController{
 		be: &BackendMock{
-			ChangeBucketOwnerFunc: func(contextMoqParam context.Context, bucket, newOwner string) error {
+			ChangeBucketOwnerFunc: func(contextMoqParam context.Context, bucket string, acl []byte) error {
 				return nil
 			},
 		},

diff --git a/s3api/controllers/backend_moq_test.go b/s3api/controllers/backend_moq_test.go
diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go
@@ -480,6 +480,7 @@ func TestAccessControl(s *S3Conf) {
 	AccessControl_bucket_resource_all_action(s)
 	AccessControl_single_object_resource_actions(s)
 	AccessControl_multi_statement_policy(s)
+	AccessControl_bucket_ownership_to_user(s)
 }
 
 type IntTests map[string]func(s *S3Conf) error
@@ -762,5 +763,13 @@ func GetIntTests() IntTests {
 		"IAM_userplus_access_denied":                                          IAM_userplus_access_denied,
 		"IAM_userplus_CreateBucket":                                           IAM_userplus_CreateBucket,
 		"IAM_admin_ChangeBucketOwner":                                         IAM_admin_ChangeBucketOwner,
+		"AccessControl_default_ACL_user_access_denied":                        AccessControl_default_ACL_user_access_denied,
+		"AccessControl_default_ACL_userplus_access_denied":                    AccessControl_default_ACL_userplus_access_denied,
+		"AccessControl_default_ACL_admin_successful_access":                   AccessControl_default_ACL_admin_successful_access,
+		"AccessControl_bucket_resource_single_action":                         AccessControl_bucket_resource_single_action,
+		"AccessControl_bucket_resource_all_action":                            AccessControl_bucket_resource_all_action,
+		"AccessControl_single_object_resource_actions":                        AccessControl_single_object_resource_actions,
+		"AccessControl_multi_statement_policy":                                AccessControl_multi_statement_policy,
+		"AccessControl_bucket_ownership_to_user":                              AccessControl_bucket_ownership_to_user,
 	}
 }
diff --git a/tests/integration/tests.go b/tests/integration/tests.go
@@ -9151,6 +9151,38 @@ func AccessControl_multi_statement_policy(s *S3Conf) error {
 	})
 }
 
+func AccessControl_bucket_ownership_to_user(s *S3Conf) error {
+	testName := "AccessControl_bucket_ownership_to_user"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		usr := user{
+			access: "grt1",
+			secret: "grt1secret",
+			role:   "user",
+		}
+
+		if err := createUsers(s, []user{usr}); err != nil {
+			return err
+		}
+
+		if err := changeBucketsOwner(s, []string{bucket}, usr.access); err != nil {
+			return err
+		}
+
+		userClient := getUserS3Client(usr, s)
+
+		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+		_, err := userClient.HeadBucket(ctx, &s3.HeadBucketInput{
+			Bucket: &bucket,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		return nil
+	})
+}
+
 // IAM related tests
 // multi-user iam tests
 func IAM_user_access_denied(s *S3Conf) error {