From f295df2217f0bc4b099c764933274832b27455ae Mon Sep 17 00:00:00 2001 From: Ben McClelland Date: Wed, 9 Jul 2025 12:19:09 -0700 Subject: [PATCH] fix: add new auth method to update ownership within acl Add helper util auth.UpdateBucketACLOwner() that sets new default ACL based on new owner and removes old bucket policy. The ChangeBucketOwner() remains in the backend.Backend interface in case there is ever a backend that needs to manage ownership in some other way than with bucket ACLs. The arguments are changing to clarify the updated owner. This will break any plugins implementing the old interface. They should use the new auth.UpdateBucketACLOwner() or implement the corresponding change specific for the backend. --- auth/acl.go | 27 +++++++++++++++++++++++++++ backend/azure/azure.go | 4 ++-- backend/backend.go | 4 ++-- backend/posix/posix.go | 4 ++-- backend/s3proxy/s3.go | 4 ++-- s3api/controllers/admin.go | 25 +------------------------ s3api/controllers/admin_test.go | 2 +- s3api/controllers/backend_moq_test.go | 20 ++++++++++---------- 8 files changed, 47 insertions(+), 43 deletions(-) diff --git a/auth/acl.go b/auth/acl.go index 3b32cda..e900324 100644 --- a/auth/acl.go +++ b/auth/acl.go @@ -466,3 +466,30 @@ func VerifyPublicBucketACL(ctx context.Context, be backend.Backend, bucket strin return nil } + +// UpdateBucketACLOwner sets default ACL with new owner and removes +// any previous bucket policy that was in place +func UpdateBucketACLOwner(ctx context.Context, be backend.Backend, bucket, newOwner string) error { + acl := ACL{ + Owner: newOwner, + Grantees: []Grantee{ + { + Permission: PermissionFullControl, + Access: newOwner, + Type: types.TypeCanonicalUser, + }, + }, + } + + result, err := json.Marshal(acl) + if err != nil { + return fmt.Errorf("marshal ACL: %w", err) + } + + err = be.PutBucketAcl(ctx, bucket, result) + if err != nil { + return err + } + + return be.DeleteBucketPolicy(ctx, bucket) +} diff --git a/backend/azure/azure.go b/backend/azure/azure.go index 5cfa638..9934a5b 100644 --- a/backend/azure/azure.go +++ b/backend/azure/azure.go @@ -1683,8 +1683,8 @@ func (az *Azure) GetObjectLegalHold(ctx context.Context, bucket, object, version return &status, nil } -func (az *Azure) ChangeBucketOwner(ctx context.Context, bucket string, acl []byte) error { - return az.PutBucketAcl(ctx, bucket, acl) +func (az *Azure) ChangeBucketOwner(ctx context.Context, bucket, owner string) error { + return auth.UpdateBucketACLOwner(ctx, az, bucket, owner) } // 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 de9ea2b..946ee73 100644 --- a/backend/backend.go +++ b/backend/backend.go @@ -96,7 +96,7 @@ type Backend interface { GetObjectLegalHold(_ context.Context, bucket, object, versionId string) (*bool, error) // non AWS actions - ChangeBucketOwner(_ context.Context, bucket string, acl []byte) error + ChangeBucketOwner(_ context.Context, bucket, owner string) error ListBucketsAndOwners(context.Context) ([]s3response.Bucket, error) } @@ -280,7 +280,7 @@ func (BackendUnsupported) GetObjectLegalHold(_ context.Context, bucket, object, return nil, s3err.GetAPIError(s3err.ErrNotImplemented) } -func (BackendUnsupported) ChangeBucketOwner(_ context.Context, bucket string, acl []byte) error { +func (BackendUnsupported) ChangeBucketOwner(_ context.Context, bucket, owner string) 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 bb99ebe..3c96989 100644 --- a/backend/posix/posix.go +++ b/backend/posix/posix.go @@ -4888,8 +4888,8 @@ func (p *Posix) GetObjectRetention(_ context.Context, bucket, object, versionId return data, nil } -func (p *Posix) ChangeBucketOwner(ctx context.Context, bucket string, acl []byte) error { - return p.PutBucketAcl(ctx, bucket, acl) +func (p *Posix) ChangeBucketOwner(ctx context.Context, bucket, owner string) error { + return auth.UpdateBucketACLOwner(ctx, p, bucket, owner) } func listBucketFileInfos(bucketlinks bool) ([]fs.FileInfo, error) { diff --git a/backend/s3proxy/s3.go b/backend/s3proxy/s3.go index 2620ddf..d29db3d 100644 --- a/backend/s3proxy/s3.go +++ b/backend/s3proxy/s3.go @@ -1547,8 +1547,8 @@ func (s *S3Proxy) GetObjectLegalHold(ctx context.Context, bucket, object, versio return nil, s3err.GetAPIError(s3err.ErrNotImplemented) } -func (s *S3Proxy) ChangeBucketOwner(ctx context.Context, bucket string, acl []byte) error { - return s.PutBucketAcl(ctx, bucket, acl) +func (s *S3Proxy) ChangeBucketOwner(ctx context.Context, bucket, owner string) error { + return auth.UpdateBucketACLOwner(ctx, s, bucket, owner) } func (s *S3Proxy) ListBucketsAndOwners(ctx context.Context) ([]s3response.Bucket, error) { diff --git a/s3api/controllers/admin.go b/s3api/controllers/admin.go index 9aada79..d10dae1 100644 --- a/s3api/controllers/admin.go +++ b/s3api/controllers/admin.go @@ -15,13 +15,10 @@ package controllers import ( - "encoding/json" "encoding/xml" - "fmt" "net/http" "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" @@ -172,27 +169,7 @@ func (c AdminController) ChangeBucketOwner(ctx *fiber.Ctx) error { }) } - acl := auth.ACL{ - Owner: owner, - Grantees: []auth.Grantee{ - { - Permission: auth.PermissionFullControl, - Access: owner, - Type: types.TypeCanonicalUser, - }, - }, - } - - aclParsed, err := json.Marshal(acl) - if err != nil { - return SendResponse(ctx, fmt.Errorf("failed to marshal the bucket acl: %w", err), - &MetaOpts{ - Logger: c.l, - Action: metrics.ActionAdminChangeBucketOwner, - }) - } - - err = c.be.ChangeBucketOwner(ctx.Context(), bucket, aclParsed) + err = c.be.ChangeBucketOwner(ctx.Context(), bucket, owner) return SendResponse(ctx, err, &MetaOpts{ Logger: c.l, diff --git a/s3api/controllers/admin_test.go b/s3api/controllers/admin_test.go index 3382461..aaa82a0 100644 --- a/s3api/controllers/admin_test.go +++ b/s3api/controllers/admin_test.go @@ -324,7 +324,7 @@ func TestAdminController_ChangeBucketOwner(t *testing.T) { } adminController := AdminController{ be: &BackendMock{ - ChangeBucketOwnerFunc: func(contextMoqParam context.Context, bucket string, acl []byte) error { + ChangeBucketOwnerFunc: func(contextMoqParam context.Context, bucket, owner string) error { return nil }, }, diff --git a/s3api/controllers/backend_moq_test.go b/s3api/controllers/backend_moq_test.go index 5d2e9a2..d0ef8d7 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, acl []byte) error { +// ChangeBucketOwnerFunc: func(contextMoqParam context.Context, bucket string, owner string) error { // panic("mock out the ChangeBucketOwner method") // }, // CompleteMultipartUploadFunc: func(contextMoqParam context.Context, completeMultipartUploadInput *s3.CompleteMultipartUploadInput) (s3response.CompleteMultipartUploadResult, string, error) { @@ -196,7 +196,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, acl []byte) error + ChangeBucketOwnerFunc func(contextMoqParam context.Context, bucket string, owner string) error // CompleteMultipartUploadFunc mocks the CompleteMultipartUpload method. CompleteMultipartUploadFunc func(contextMoqParam context.Context, completeMultipartUploadInput *s3.CompleteMultipartUploadInput) (s3response.CompleteMultipartUploadResult, string, error) @@ -369,8 +369,8 @@ type BackendMock struct { ContextMoqParam context.Context // Bucket is the bucket argument value. Bucket string - // ACL is the acl argument value. - ACL []byte + // Owner is the owner argument value. + Owner string } // CompleteMultipartUpload holds details about calls to the CompleteMultipartUpload method. CompleteMultipartUpload []struct { @@ -864,23 +864,23 @@ func (mock *BackendMock) AbortMultipartUploadCalls() []struct { } // ChangeBucketOwner calls ChangeBucketOwnerFunc. -func (mock *BackendMock) ChangeBucketOwner(contextMoqParam context.Context, bucket string, acl []byte) error { +func (mock *BackendMock) ChangeBucketOwner(contextMoqParam context.Context, bucket string, owner string) error { if mock.ChangeBucketOwnerFunc == nil { panic("BackendMock.ChangeBucketOwnerFunc: method is nil but Backend.ChangeBucketOwner was just called") } callInfo := struct { ContextMoqParam context.Context Bucket string - ACL []byte + Owner string }{ ContextMoqParam: contextMoqParam, Bucket: bucket, - ACL: acl, + Owner: owner, } mock.lockChangeBucketOwner.Lock() mock.calls.ChangeBucketOwner = append(mock.calls.ChangeBucketOwner, callInfo) mock.lockChangeBucketOwner.Unlock() - return mock.ChangeBucketOwnerFunc(contextMoqParam, bucket, acl) + return mock.ChangeBucketOwnerFunc(contextMoqParam, bucket, owner) } // ChangeBucketOwnerCalls gets all the calls that were made to ChangeBucketOwner. @@ -890,12 +890,12 @@ func (mock *BackendMock) ChangeBucketOwner(contextMoqParam context.Context, buck func (mock *BackendMock) ChangeBucketOwnerCalls() []struct { ContextMoqParam context.Context Bucket string - ACL []byte + Owner string } { var calls []struct { ContextMoqParam context.Context Bucket string - ACL []byte + Owner string } mock.lockChangeBucketOwner.RLock() calls = mock.calls.ChangeBucketOwner