From 2843cdbd452c5cc1c72144980d775d86d32ac289 Mon Sep 17 00:00:00 2001 From: jonaustin09 Date: Thu, 11 Jul 2024 13:45:01 -0400 Subject: [PATCH] fix: Fixed ChangeBucketOwnership action implementation to update the bucket acl --- backend/azure/azure.go | 30 ++--------------------- backend/backend.go | 4 +-- backend/posix/posix.go | 35 ++------------------------- backend/s3proxy/s3.go | 8 ++++-- s3api/controllers/admin.go | 18 +++++++++++++- s3api/controllers/admin_test.go | 2 +- s3api/controllers/backend_moq_test.go | 20 +++++++-------- tests/integration/group-tests.go | 9 +++++++ tests/integration/tests.go | 32 ++++++++++++++++++++++++ 9 files changed, 81 insertions(+), 77 deletions(-) diff --git a/backend/azure/azure.go b/backend/azure/azure.go index 0d3fe230..89d1d787 100644 --- 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 index 2951887d..7e7238d8 100644 --- 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 index 70b3d7f2..9b5ab5db 100644 --- 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 index 7d7c6fbd..f2e2cdf7 100644 --- 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 index 23059992..1deaa039 100644 --- 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 index aa871c4c..7ef1b78a 100644 --- 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 index f5e59575..b5e9d5ce 100644 --- a/s3api/controllers/backend_moq_test.go +++ b/s3api/controllers/backend_moq_test.go @@ -26,7 +26,7 @@ var _ backend.Backend = &BackendMock{} // AbortMultipartUploadFunc: func(contextMoqParam context.Context, abortMultipartUploadInput *s3.AbortMultipartUploadInput) error { // panic("mock out the AbortMultipartUpload method") // }, -// ChangeBucketOwnerFunc: func(contextMoqParam context.Context, bucket string, newOwner string) error { +// ChangeBucketOwnerFunc: func(contextMoqParam context.Context, bucket string, acl []byte) error { // panic("mock out the ChangeBucketOwner method") // }, // CompleteMultipartUploadFunc: func(contextMoqParam context.Context, completeMultipartUploadInput *s3.CompleteMultipartUploadInput) (*s3.CompleteMultipartUploadOutput, error) { @@ -187,7 +187,7 @@ type BackendMock struct { AbortMultipartUploadFunc func(contextMoqParam context.Context, abortMultipartUploadInput *s3.AbortMultipartUploadInput) error // ChangeBucketOwnerFunc mocks the ChangeBucketOwner method. - ChangeBucketOwnerFunc func(contextMoqParam context.Context, bucket string, newOwner string) error + ChangeBucketOwnerFunc func(contextMoqParam context.Context, bucket string, acl []byte) error // CompleteMultipartUploadFunc mocks the CompleteMultipartUpload method. CompleteMultipartUploadFunc func(contextMoqParam context.Context, completeMultipartUploadInput *s3.CompleteMultipartUploadInput) (*s3.CompleteMultipartUploadOutput, error) @@ -351,8 +351,8 @@ type BackendMock struct { ContextMoqParam context.Context // Bucket is the bucket argument value. Bucket string - // NewOwner is the newOwner argument value. - NewOwner string + // ACL is the acl argument value. + ACL []byte } // CompleteMultipartUpload holds details about calls to the CompleteMultipartUpload method. CompleteMultipartUpload []struct { @@ -822,23 +822,23 @@ func (mock *BackendMock) AbortMultipartUploadCalls() []struct { } // ChangeBucketOwner calls ChangeBucketOwnerFunc. -func (mock *BackendMock) ChangeBucketOwner(contextMoqParam context.Context, bucket string, newOwner string) error { +func (mock *BackendMock) ChangeBucketOwner(contextMoqParam context.Context, bucket string, acl []byte) error { if mock.ChangeBucketOwnerFunc == nil { panic("BackendMock.ChangeBucketOwnerFunc: method is nil but Backend.ChangeBucketOwner was just called") } callInfo := struct { ContextMoqParam context.Context Bucket string - NewOwner string + ACL []byte }{ ContextMoqParam: contextMoqParam, Bucket: bucket, - NewOwner: newOwner, + ACL: acl, } mock.lockChangeBucketOwner.Lock() mock.calls.ChangeBucketOwner = append(mock.calls.ChangeBucketOwner, callInfo) mock.lockChangeBucketOwner.Unlock() - return mock.ChangeBucketOwnerFunc(contextMoqParam, bucket, newOwner) + return mock.ChangeBucketOwnerFunc(contextMoqParam, bucket, acl) } // ChangeBucketOwnerCalls gets all the calls that were made to ChangeBucketOwner. @@ -848,12 +848,12 @@ func (mock *BackendMock) ChangeBucketOwner(contextMoqParam context.Context, buck func (mock *BackendMock) ChangeBucketOwnerCalls() []struct { ContextMoqParam context.Context Bucket string - NewOwner string + ACL []byte } { var calls []struct { ContextMoqParam context.Context Bucket string - NewOwner string + ACL []byte } mock.lockChangeBucketOwner.RLock() calls = mock.calls.ChangeBucketOwner diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index f5166af0..73ab85ec 100644 --- 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 index 5c92570e..b7b22217 100644 --- 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 {