Merge pull request #1753 from versity/sis/delete-locked-object

fix: skips object lock check in DeleteObject without versionId.
2026-07-26 18:12:42 +00:00 · 2026-01-13 12:05:36 -08:00
parent abbd6697d1 06f4f0ac15
commit f9e903aaf4
4 changed files with 244 additions and 4 deletions
@@ -254,6 +254,12 @@ func CheckObjectAccess(ctx context.Context, bucket, userAccess string, objects [
 		}
 	}

+	var versioningEnabled bool
+	vers, err := be.GetBucketVersioning(ctx, bucket)
+	if err == nil && vers.Status != nil {
+		versioningEnabled = *vers.Status == types.BucketVersioningStatusEnabled
+	}
+
 	for _, obj := range objects {
 		var key, versionId string
 		if obj.Key != nil {
@@ -262,6 +268,11 @@ func CheckObjectAccess(ctx context.Context, bucket, userAccess string, objects [
 		if obj.VersionId != nil {
 			versionId = *obj.VersionId
 		}
+		// if bucket versioning is enabled and versionId isn't provided
+		// no lock check is needed, as it leads to a new delete marker creation
+		if versioningEnabled && versionId == "" {
+			continue
+		}
 		checkRetention := true
 		retentionData, err := be.GetObjectRetention(ctx, bucket, key, versionId)
 		if errors.Is(err, s3err.GetAPIError(s3err.ErrNoSuchKey)) {
@@ -728,8 +728,17 @@ func (p *Posix) deleteNullVersionIdObject(bucket, key string) error {
 	return err
 }

+func isRemovableAttr(attr string) bool {
+	switch attr {
+	case objectLegalHoldKey, objectRetentionKey:
+		return true
+	default:
+		return false
+	}
+}
+
 // Creates a new copy(version) of an object in the versioning directory
-func (p *Posix) createObjVersion(bucket, key string, size int64, acc auth.Account) (versionPath string, err error) {
+func (p *Posix) createObjVersion(bucket, key string, size int64, acc auth.Account, removeAttributes bool) (versionPath string, err error) {
 	sf, err := os.Open(filepath.Join(bucket, key))
 	if err != nil {
 		return "", err
@@ -785,6 +794,14 @@ func (p *Posix) createObjVersion(bucket, key string, size int64, acc auth.Accoun
 		if err != nil {
 			return versionPath, fmt.Errorf("store %v attribute: %w", attr, err)
 		}
+
+		// remove object lock attributes in delete marker
+		if removeAttributes && isRemovableAttr(attr) {
+			err := p.meta.DeleteAttribute(bucket, key, attr)
+			if err != nil {
+				return versionPath, fmt.Errorf("remove %s attribute: %w", attr, err)
+			}
+		}
 	}

 	if err := f.link(); err != nil {
@@ -1699,7 +1716,7 @@ func (p *Posix) CompleteMultipartUploadWithCopy(ctx context.Context, input *s3.C

 	// if the versioninng is enabled first create the file object version
 	if p.versioningEnabled() && vEnabled && err == nil && !d.IsDir() {
-		_, err := p.createObjVersion(bucket, object, d.Size(), acct)
+		_, err := p.createObjVersion(bucket, object, d.Size(), acct, false)
 		if err != nil {
 			return res, "", fmt.Errorf("create object version: %w", err)
 		}
@@ -2995,7 +3012,7 @@ func (p *Posix) PutObjectWithPostFunc(ctx context.Context, po s3response.PutObje
 			isVersionIdMissing = len(vIdBytes) == 0
 		}
 		if !isVersionIdMissing {
-			_, err := p.createObjVersion(*po.Bucket, *po.Key, d.Size(), acct)
+			_, err := p.createObjVersion(*po.Bucket, *po.Key, d.Size(), acct, false)
 			if err != nil {
 				return s3response.PutObjectOutput{}, fmt.Errorf("create object version: %w", err)
 			}
@@ -3343,7 +3360,7 @@ func (p *Posix) DeleteObject(ctx context.Context, input *s3.DeleteObjectInput) (

 			// Creates a new object version in the versioning directory
 			if p.isBucketVersioningEnabled(vStatus) || string(vId) != nullVersionId {
-				_, err = p.createObjVersion(bucket, object, fi.Size(), acct)
+				_, err = p.createObjVersion(bucket, object, fi.Size(), acct, true)
 				if err != nil {
 					return nil, err
 				}
@@ -1076,6 +1076,9 @@ func TestVersioning(ts *TestState) {
 	ts.Run(Versioning_WORM_obj_version_locked_with_legal_hold)
 	ts.Run(Versioning_WORM_obj_version_locked_with_governance_retention)
 	ts.Run(Versioning_WORM_obj_version_locked_with_compliance_retention)
+	ts.Run(Versioning_WORM_delete_marker_locked_object_legal_hold)
+	ts.Run(Versioning_WORM_delete_marker_locked_object_governance_retention)
+	ts.Run(Versioning_WORM_delete_marker_locked_object_compliance_retention)
 	ts.Run(Versioning_WORM_PutObject_overwrite_locked_object)
 	ts.Run(Versioning_WORM_CopyObject_overwrite_locked_object)
 	ts.Run(Versioning_WORM_CompleteMultipartUpload_overwrite_locked_object)
@@ -1774,6 +1777,9 @@ func GetIntTests() IntTests {
 		"Versioning_WORM_obj_version_locked_with_legal_hold":                       Versioning_WORM_obj_version_locked_with_legal_hold,
 		"Versioning_WORM_obj_version_locked_with_governance_retention":             Versioning_WORM_obj_version_locked_with_governance_retention,
 		"Versioning_WORM_obj_version_locked_with_compliance_retention":             Versioning_WORM_obj_version_locked_with_compliance_retention,
+		"Versioning_WORM_delete_marker_locked_object_legal_hold":                   Versioning_WORM_delete_marker_locked_object_legal_hold,
+		"Versioning_WORM_delete_marker_locked_object_governance_retention":         Versioning_WORM_delete_marker_locked_object_governance_retention,
+		"Versioning_WORM_delete_marker_locked_object_compliance_retention":         Versioning_WORM_delete_marker_locked_object_compliance_retention,
 		"Versioning_WORM_PutObject_overwrite_locked_object":                        Versioning_WORM_PutObject_overwrite_locked_object,
 		"Versioning_WORM_CopyObject_overwrite_locked_object":                       Versioning_WORM_CopyObject_overwrite_locked_object,
 		"Versioning_WORM_CompleteMultipartUpload_overwrite_locked_object":          Versioning_WORM_CompleteMultipartUpload_overwrite_locked_object,
@@ -2445,6 +2445,212 @@ func Versioning_WORM_obj_version_locked_with_compliance_retention(s *S3Conf) err
 	}, withLock(), withVersioning(types.BucketVersioningStatusEnabled))
 }

+func Versioning_WORM_delete_marker_locked_object_legal_hold(s *S3Conf) error {
+	testName := "Versioning_WORM_delete_marker_locked_object_legal_hold"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		obj := "my-obj"
+		objVersions, err := createObjVersions(s3client, bucket, obj, 1)
+		if err != nil {
+			return err
+		}
+		version := objVersions[0]
+		objVersions[0].IsLatest = getPtr(false)
+
+		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+		_, err = s3client.PutObjectLegalHold(ctx, &s3.PutObjectLegalHoldInput{
+			Bucket: &bucket,
+			Key:    &obj,
+			LegalHold: &types.ObjectLockLegalHold{
+				Status: types.ObjectLockLegalHoldStatusOn,
+			},
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		out, err := s3client.DeleteObject(ctx, &s3.DeleteObjectInput{
+			Bucket: &bucket,
+			Key:    &obj,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		delMarkers := []types.DeleteMarkerEntry{
+			{
+				IsLatest:  getPtr(true),
+				Key:       &obj,
+				VersionId: out.VersionId,
+			},
+		}
+
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		resp, err := s3client.ListObjectVersions(ctx, &s3.ListObjectVersionsInput{
+			Bucket: &bucket,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		if !compareVersions(objVersions, resp.Versions) {
+			return fmt.Errorf("expected the object versions to be %v, instead got %v", objVersions, resp.Versions)
+		}
+		if !compareDelMarkers(delMarkers, resp.DeleteMarkers) {
+			return fmt.Errorf("expected the object delete markers to be %v, instead got %v", delMarkers, resp.DeleteMarkers)
+		}
+
+		return cleanupLockedObjects(s3client, bucket, []objToDelete{
+			{
+				key:                obj,
+				versionId:          getString(version.VersionId),
+				removeOnlyLeglHold: true,
+			},
+		})
+	}, withLock())
+}
+
+func Versioning_WORM_delete_marker_locked_object_governance_retention(s *S3Conf) error {
+	testName := "Versioning_WORM_delete_marker_locked_object_governance_retention"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		obj := "my-obj"
+		objVersions, err := createObjVersions(s3client, bucket, obj, 1)
+		if err != nil {
+			return err
+		}
+		version := objVersions[0]
+		objVersions[0].IsLatest = getPtr(false)
+
+		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+		_, err = s3client.PutObjectRetention(ctx, &s3.PutObjectRetentionInput{
+			Bucket: &bucket,
+			Key:    &obj,
+			Retention: &types.ObjectLockRetention{
+				Mode:            types.ObjectLockRetentionModeGovernance,
+				RetainUntilDate: getPtr(time.Now().AddDate(1, 0, 0)),
+			},
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		out, err := s3client.DeleteObject(ctx, &s3.DeleteObjectInput{
+			Bucket: &bucket,
+			Key:    &obj,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		delMarkers := []types.DeleteMarkerEntry{
+			{
+				IsLatest:  getPtr(true),
+				Key:       &obj,
+				VersionId: out.VersionId,
+			},
+		}
+
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		resp, err := s3client.ListObjectVersions(ctx, &s3.ListObjectVersionsInput{
+			Bucket: &bucket,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		if !compareVersions(objVersions, resp.Versions) {
+			return fmt.Errorf("expected the object versions to be %v, instead got %v", objVersions, resp.Versions)
+		}
+		if !compareDelMarkers(delMarkers, resp.DeleteMarkers) {
+			return fmt.Errorf("expected the object delete markers to be %v, instead got %v", delMarkers, resp.DeleteMarkers)
+		}
+
+		return cleanupLockedObjects(s3client, bucket, []objToDelete{
+			{
+				key:          obj,
+				versionId:    getString(version.VersionId),
+				isCompliance: false,
+			},
+		})
+	}, withLock())
+}
+
+func Versioning_WORM_delete_marker_locked_object_compliance_retention(s *S3Conf) error {
+	testName := "Versioning_WORM_delete_marker_locked_object_compliance_retention"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		obj := "my-obj"
+		objVersions, err := createObjVersions(s3client, bucket, obj, 1)
+		if err != nil {
+			return err
+		}
+		version := objVersions[0]
+		objVersions[0].IsLatest = getPtr(false)
+
+		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+		_, err = s3client.PutObjectRetention(ctx, &s3.PutObjectRetentionInput{
+			Bucket: &bucket,
+			Key:    &obj,
+			Retention: &types.ObjectLockRetention{
+				Mode:            types.ObjectLockRetentionModeCompliance,
+				RetainUntilDate: getPtr(time.Now().AddDate(1, 0, 0)),
+			},
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		out, err := s3client.DeleteObject(ctx, &s3.DeleteObjectInput{
+			Bucket: &bucket,
+			Key:    &obj,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		delMarkers := []types.DeleteMarkerEntry{
+			{
+				IsLatest:  getPtr(true),
+				Key:       &obj,
+				VersionId: out.VersionId,
+			},
+		}
+
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		resp, err := s3client.ListObjectVersions(ctx, &s3.ListObjectVersionsInput{
+			Bucket: &bucket,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		if !compareVersions(objVersions, resp.Versions) {
+			return fmt.Errorf("expected the object versions to be %v, instead got %v", objVersions, resp.Versions)
+		}
+		if !compareDelMarkers(delMarkers, resp.DeleteMarkers) {
+			return fmt.Errorf("expected the object delete markers to be %v, instead got %v", delMarkers, resp.DeleteMarkers)
+		}
+
+		return cleanupLockedObjects(s3client, bucket, []objToDelete{
+			{
+				key:          obj,
+				versionId:    getString(version.VersionId),
+				isCompliance: true,
+			},
+		})
+	}, withLock())
+}
+
 func Versioning_WORM_PutObject_overwrite_locked_object(s *S3Conf) error {
 	testName := "Versioning_WORM_PutObject_overwrite_locked_object"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {