From a36d974942d00d8a4452adac1c50c785d70d9046 Mon Sep 17 00:00:00 2001 From: Ben McClelland Date: Tue, 13 Aug 2024 10:52:47 -0700 Subject: [PATCH] fix: copy-object with replace metadata-directive In copy-object, if the source and destination are the same then X-Amz-Metadata-Directive must be set to "REPLACE" in order to use this api call to update the metadata of the object in place. The default X-Amz-Metadata-Directive is "COPY" if not specified. "COPY" is only valid if source and destination are not the same object. When "REPLACE" selected, metadata does not have to differ for the call to be successful. The "REPLACE" always sets the incoming metadata (even if empty or the same as the source). Fixes #734 --- backend/posix/posix.go | 75 ++++++++++++++------------------ s3api/controllers/base.go | 18 ++++++++ s3err/s3err.go | 6 +++ tests/integration/group-tests.go | 2 + tests/integration/tests.go | 24 ++++++++++ 5 files changed, 83 insertions(+), 42 deletions(-) diff --git a/backend/posix/posix.go b/backend/posix/posix.go index 2143506..25c30b7 100644 --- a/backend/posix/posix.go +++ b/backend/posix/posix.go @@ -805,20 +805,6 @@ func (p *Posix) loadUserMetaData(bucket, object string, m map[string]string) (st return contentType, contentEncoding } -func compareUserMetadata(meta1, meta2 map[string]string) bool { - if len(meta1) != len(meta2) { - return false - } - - for key, val := range meta1 { - if meta2[key] != val { - return false - } - } - - return true -} - func isValidMeta(val string) bool { if strings.HasPrefix(val, metaHdr) { return true @@ -2012,42 +1998,47 @@ func (p *Posix) CopyObject(ctx context.Context, input *s3.CopyObjectInput) (*s3. return nil, s3err.GetAPIError(s3err.ErrNoSuchKey) } - meta := make(map[string]string) - p.loadUserMetaData(srcBucket, srcObject, meta) + mdmap := make(map[string]string) + p.loadUserMetaData(srcBucket, srcObject, mdmap) + + var etag string dstObjdPath := filepath.Join(dstBucket, dstObject) if dstObjdPath == objPath { - if compareUserMetadata(meta, input.Metadata) { + if input.MetadataDirective == types.MetadataDirectiveCopy { return &s3.CopyObjectOutput{}, s3err.GetAPIError(s3err.ErrInvalidCopyDest) - } else { - for key := range meta { - err := p.meta.DeleteAttribute(dstBucket, dstObject, key) - if err != nil { - return nil, fmt.Errorf("delete user metadata: %w", err) - } - } - for k, v := range input.Metadata { - err := p.meta.StoreAttribute(dstBucket, dstObject, - fmt.Sprintf("%v.%v", metaHdr, k), []byte(v)) - if err != nil { - return nil, fmt.Errorf("set user attr %q: %w", k, err) - } + } + + for key := range mdmap { + err := p.meta.DeleteAttribute(dstBucket, dstObject, key) + if err != nil && !errors.Is(err, meta.ErrNoSuchKey) { + return nil, fmt.Errorf("delete user metadata: %w", err) + } + } + for k, v := range input.Metadata { + err := p.meta.StoreAttribute(dstBucket, dstObject, + fmt.Sprintf("%v.%v", metaHdr, k), []byte(v)) + if err != nil { + return nil, fmt.Errorf("set user attr %q: %w", k, err) } } - } - contentLength := fi.Size() + b, _ := p.meta.RetrieveAttribute(dstBucket, dstObject, etagkey) + etag = string(b) + } else { + contentLength := fi.Size() - etag, err := p.PutObject(ctx, - &s3.PutObjectInput{ - Bucket: &dstBucket, - Key: &dstObject, - Body: f, - ContentLength: &contentLength, - Metadata: meta, - }) - if err != nil { - return nil, err + etag, err = p.PutObject(ctx, + &s3.PutObjectInput{ + Bucket: &dstBucket, + Key: &dstObject, + Body: f, + ContentLength: &contentLength, + Metadata: input.Metadata, + }) + if err != nil { + return nil, err + } } fi, err = os.Stat(dstObjdPath) diff --git a/s3api/controllers/base.go b/s3api/controllers/base.go index b90f2a7..a08f043 100644 --- a/s3api/controllers/base.go +++ b/s3api/controllers/base.go @@ -1533,6 +1533,7 @@ func (c S3ApiController) PutActions(ctx *fiber.Ctx) error { copySrcModifSince := ctx.Get("X-Amz-Copy-Source-If-Modified-Since") copySrcUnmodifSince := ctx.Get("X-Amz-Copy-Source-If-Unmodified-Since") copySrcRange := ctx.Get("X-Amz-Copy-Source-Range") + directive := ctx.Get("X-Amz-Metadata-Directive") // Permission headers acl := ctx.Get("X-Amz-Acl") @@ -2054,6 +2055,22 @@ func (c S3ApiController) PutActions(ctx *fiber.Ctx) error { metadata := utils.GetUserMetaData(&ctx.Request().Header) + if directive != "" && directive != "COPY" && directive != "REPLACE" { + return SendXMLResponse(ctx, nil, + s3err.GetAPIError(s3err.ErrInvalidMetadataDirective), + &MetaOpts{ + Logger: c.logger, + MetricsMng: c.mm, + Action: metrics.ActionCopyObject, + BucketOwner: parsedAcl.Owner, + }) + } + + metaDirective := types.MetadataDirectiveCopy + if directive == "REPLACE" { + metaDirective = types.MetadataDirectiveReplace + } + res, err := c.be.CopyObject(ctx.Context(), &s3.CopyObjectInput{ Bucket: &bucket, @@ -2065,6 +2082,7 @@ func (c S3ApiController) PutActions(ctx *fiber.Ctx) error { CopySourceIfUnmodifiedSince: umtime, ExpectedBucketOwner: &acct.Access, Metadata: metadata, + MetadataDirective: metaDirective, StorageClass: types.StorageClass(storageClass), }) if err == nil { diff --git a/s3err/s3err.go b/s3err/s3err.go index 9b4b262..8433ca9 100644 --- a/s3err/s3err.go +++ b/s3err/s3err.go @@ -130,6 +130,7 @@ const ( ErrMalformedACL ErrUnexpectedContent ErrMissingSecurityHeader + ErrInvalidMetadataDirective // Non-AWS errors ErrExistingObjectIsDirectory @@ -514,6 +515,11 @@ var errorCodeResponse = map[ErrorCode]APIError{ Description: "Your request was missing a required header", HTTPStatusCode: http.StatusNotFound, }, + ErrInvalidMetadataDirective: { + Code: "InvalidArgument", + Description: "Unknown metadata directive.", + HTTPStatusCode: http.StatusBadRequest, + }, // non aws errors ErrExistingObjectIsDirectory: { diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go index f0692d4..f8ee085 100644 --- a/tests/integration/group-tests.go +++ b/tests/integration/group-tests.go @@ -199,6 +199,7 @@ func TestCopyObject(s *S3Conf) { CopyObject_non_existing_dst_bucket(s) CopyObject_not_owned_source_bucket(s) CopyObject_copy_to_itself(s) + CopyObject_copy_to_itself_invalid_directive(s) CopyObject_to_itself_with_new_metadata(s) CopyObject_CopySource_starting_with_slash(s) CopyObject_non_existing_dir_object(s) @@ -620,6 +621,7 @@ func GetIntTests() IntTests { "CopyObject_non_existing_dst_bucket": CopyObject_non_existing_dst_bucket, "CopyObject_not_owned_source_bucket": CopyObject_not_owned_source_bucket, "CopyObject_copy_to_itself": CopyObject_copy_to_itself, + "CopyObject_copy_to_itself_invalid_directive": CopyObject_copy_to_itself_invalid_directive, "CopyObject_to_itself_with_new_metadata": CopyObject_to_itself_with_new_metadata, "CopyObject_CopySource_starting_with_slash": CopyObject_CopySource_starting_with_slash, "CopyObject_non_existing_dir_object": CopyObject_non_existing_dir_object, diff --git a/tests/integration/tests.go b/tests/integration/tests.go index a4c1ab9..0f5d761 100644 --- a/tests/integration/tests.go +++ b/tests/integration/tests.go @@ -4268,6 +4268,29 @@ func CopyObject_copy_to_itself(s *S3Conf) error { }) } +func CopyObject_copy_to_itself_invalid_directive(s *S3Conf) error { + testName := "CopyObject_copy_to_itself_invalid_directive" + return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { + obj := "my-obj" + err := putObjects(s3client, []string{obj}, bucket) + if err != nil { + return err + } + ctx, cancel := context.WithTimeout(context.Background(), shortTimeout) + _, err = s3client.CopyObject(ctx, &s3.CopyObjectInput{ + Bucket: &bucket, + Key: &obj, + CopySource: getPtr(fmt.Sprintf("%v/%v", bucket, obj)), + MetadataDirective: types.MetadataDirective("invalid"), + }) + cancel() + if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrInvalidMetadataDirective)); err != nil { + return err + } + return nil + }) +} + func CopyObject_to_itself_with_new_metadata(s *S3Conf) error { testName := "CopyObject_to_itself_with_new_metadata" return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error { @@ -4284,6 +4307,7 @@ func CopyObject_to_itself_with_new_metadata(s *S3Conf) error { Metadata: map[string]string{ "Hello": "World", }, + MetadataDirective: types.MetadataDirectiveReplace, }) cancel() if err != nil {