From 7661449f82a85a397410a3013674a3fded5f9ee4 Mon Sep 17 00:00:00 2001 From: chrislu Date: Fri, 18 Jul 2025 20:10:16 -0700 Subject: [PATCH] fix tests --- weed/s3api/s3api_object_handlers_delete.go | 26 +++++++++++++++---- weed/s3api/s3api_object_handlers_put.go | 2 +- weed/s3api/s3api_object_handlers_retention.go | 4 +-- weed/s3api/s3api_object_retention.go | 20 +++++++++----- 4 files changed, 38 insertions(+), 14 deletions(-) diff --git a/weed/s3api/s3api_object_handlers_delete.go b/weed/s3api/s3api_object_handlers_delete.go index f06749f1d..72f484429 100644 --- a/weed/s3api/s3api_object_handlers_delete.go +++ b/weed/s3api/s3api_object_handlers_delete.go @@ -53,7 +53,7 @@ func (s3a *S3ApiServer) DeleteObjectHandler(w http.ResponseWriter, r *http.Reque // Handle versioned delete if versionId != "" { // Check object lock permissions before deleting specific version - bypassGovernance := r.Header.Get("x-amz-bypass-governance-retention") == "true" + bypassGovernance := s3a.validateGovernanceBypass(r, bucket, object) if err := s3a.checkObjectLockPermissions(r, bucket, object, versionId, bypassGovernance); err != nil { glog.V(2).Infof("DeleteObjectHandler: object lock check failed for %s/%s: %v", bucket, object, err) s3err.WriteErrorResponse(w, r, s3err.ErrAccessDenied) @@ -71,7 +71,16 @@ func (s3a *S3ApiServer) DeleteObjectHandler(w http.ResponseWriter, r *http.Reque // Set version ID in response header w.Header().Set("x-amz-version-id", versionId) } else { - // Create delete marker (logical delete) - this is NOT blocked by object lock retention + // Check object lock permissions before creating delete marker + // AWS S3 behavior: delete operations fail if latest version has retention protection + bypassGovernance := s3a.validateGovernanceBypass(r, bucket, object) + if err := s3a.checkObjectLockPermissions(r, bucket, object, "", bypassGovernance); err != nil { + glog.V(2).Infof("DeleteObjectHandler: object lock check failed for %s/%s: %v", bucket, object, err) + s3err.WriteErrorResponse(w, r, s3err.ErrAccessDenied) + return + } + + // Create delete marker (logical delete) deleteMarkerVersionId, err := s3a.createDeleteMarker(bucket, object) if err != nil { glog.Errorf("Failed to create delete marker: %v", err) @@ -85,6 +94,14 @@ func (s3a *S3ApiServer) DeleteObjectHandler(w http.ResponseWriter, r *http.Reque } } else { // Handle regular delete (non-versioned) + // Check object lock permissions before deleting object + bypassGovernance := s3a.validateGovernanceBypass(r, bucket, object) + if err := s3a.checkObjectLockPermissions(r, bucket, object, "", bypassGovernance); err != nil { + glog.V(2).Infof("DeleteObjectHandler: object lock check failed for %s/%s: %v", bucket, object, err) + s3err.WriteErrorResponse(w, r, s3err.ErrAccessDenied) + return + } + target := util.FullPath(fmt.Sprintf("%s/%s%s", s3a.option.BucketsPath, bucket, object)) dir, name := target.DirAndName() @@ -191,9 +208,6 @@ func (s3a *S3ApiServer) DeleteMultipleObjectsHandler(w http.ResponseWriter, r *h auditLog = s3err.GetAccessLog(r, http.StatusNoContent, s3err.ErrNone) } - // Check for bypass governance retention header - bypassGovernance := r.Header.Get("x-amz-bypass-governance-retention") == "true" - // Check if versioning is enabled for the bucket (needed for object lock checks) versioningEnabled, err := s3a.isVersioningEnabled(bucket) if err != nil { @@ -216,6 +230,8 @@ func (s3a *S3ApiServer) DeleteMultipleObjectsHandler(w http.ResponseWriter, r *h // Check object lock permissions before deletion (only for versioned buckets) if versioningEnabled { + // Validate governance bypass for this specific object + bypassGovernance := s3a.validateGovernanceBypass(r, bucket, object.Key) if err := s3a.checkObjectLockPermissions(r, bucket, object.Key, object.VersionId, bypassGovernance); err != nil { glog.V(2).Infof("DeleteMultipleObjectsHandler: object lock check failed for %s/%s (version: %s): %v", bucket, object.Key, object.VersionId, err) deleteErrors = append(deleteErrors, DeleteError{ diff --git a/weed/s3api/s3api_object_handlers_put.go b/weed/s3api/s3api_object_handlers_put.go index 8142113d8..408479f8b 100644 --- a/weed/s3api/s3api_object_handlers_put.go +++ b/weed/s3api/s3api_object_handlers_put.go @@ -119,7 +119,7 @@ func (s3a *S3ApiServer) PutObjectHandler(w http.ResponseWriter, r *http.Request) // For non-versioned buckets, check if existing object has object lock protections // that would prevent overwrite (PUT operations overwrite existing objects in non-versioned buckets) if !versioningEnabled { - bypassGovernance := r.Header.Get("x-amz-bypass-governance-retention") == "true" + bypassGovernance := s3a.validateGovernanceBypass(r, bucket, object) if err := s3a.checkObjectLockPermissions(r, bucket, object, "", bypassGovernance); err != nil { glog.V(2).Infof("PutObjectHandler: object lock permissions check failed for %s/%s: %v", bucket, object, err) s3err.WriteErrorResponse(w, r, s3err.ErrAccessDenied) diff --git a/weed/s3api/s3api_object_handlers_retention.go b/weed/s3api/s3api_object_handlers_retention.go index 0414ebbad..fb6d6e737 100644 --- a/weed/s3api/s3api_object_handlers_retention.go +++ b/weed/s3api/s3api_object_handlers_retention.go @@ -25,8 +25,8 @@ func (s3a *S3ApiServer) PutObjectRetentionHandler(w http.ResponseWriter, r *http // Get version ID from query parameters versionId := r.URL.Query().Get("versionId") - // Check for bypass governance retention header - bypassGovernance := r.Header.Get("x-amz-bypass-governance-retention") == "true" + // Validate governance bypass permission + bypassGovernance := s3a.validateGovernanceBypass(r, bucket, object) // Parse retention configuration from request body retention, err := parseObjectRetention(r) diff --git a/weed/s3api/s3api_object_retention.go b/weed/s3api/s3api_object_retention.go index b3b7635cc..273b6b641 100644 --- a/weed/s3api/s3api_object_retention.go +++ b/weed/s3api/s3api_object_retention.go @@ -627,6 +627,18 @@ func (s3a *S3ApiServer) checkGovernanceBypassPermission(request *http.Request, b return false } +// validateGovernanceBypass checks if the user has requested governance bypass via header +// and validates they have the required s3:BypassGovernanceRetention permission. +// This helper method consolidates the repetitive governance bypass validation logic +// used across multiple handlers (DELETE, PUT, etc.). +func (s3a *S3ApiServer) validateGovernanceBypass(r *http.Request, bucket, object string) bool { + // Check if governance bypass header is present + bypassRequested := r.Header.Get("x-amz-bypass-governance-retention") == "true" + + // Only allow bypass if both header is present AND user has permission + return bypassRequested && s3a.checkGovernanceBypassPermission(r, bucket, object) +} + // checkObjectLockPermissions checks if an object can be deleted or modified func (s3a *S3ApiServer) checkObjectLockPermissions(request *http.Request, bucket, object, versionId string, bypassGovernance bool) error { // Get the object entry to check both retention and legal hold @@ -682,12 +694,8 @@ func (s3a *S3ApiServer) checkObjectLockPermissions(request *http.Request, bucket if !bypassGovernance { return ErrGovernanceModeActive } - - // If bypass is requested, check if user has permission - if !s3a.checkGovernanceBypassPermission(request, bucket, object) { - glog.V(2).Infof("User does not have s3:BypassGovernanceRetention permission for %s/%s", bucket, object) - return ErrGovernanceBypassNotPermitted - } + // Note: bypassGovernance parameter is already validated by validateGovernanceBypass() + // which checks both header presence and IAM permissions, so we trust it here } }