fix: Fixed PutBucketAcl action error handling, removed the bucket owner check for all the acl options

auth/acl.go

@@ -56,7 +56,7 @@ type PutBucketAclInput struct {
 
 type AccessControlPolicy struct {
 	AccessControlList AccessControlList `xml:"AccessControlList"`
-	Owner             types.Owner
+	Owner             *types.Owner
 }
 
 type AccessControlList struct {
@@ -122,9 +122,6 @@ func UpdateACL(input *PutBucketAclInput, acl ACL, iam IAMService, isAdmin bool)
 	if input == nil {
 		return nil, s3err.GetAPIError(s3err.ErrInvalidRequest)
 	}
-	if !isAdmin && acl.Owner != *input.AccessControlPolicy.Owner.ID {
-		return nil, s3err.GetAPIError(s3err.ErrAccessDenied)
-	}
 
 	defaultGrantees := []Grantee{
 		{

s3api/controllers/base.go

@@ -1286,7 +1286,37 @@ func (c S3ApiController) PutBucketActions(ctx *fiber.Ctx) error {
 				if c.debug {
 					log.Printf("error unmarshalling access control policy: %v", err)
 				}
-				return SendResponse(ctx, s3err.GetAPIError(s3err.ErrMalformedXML),
+				return SendResponse(ctx, s3err.GetAPIError(s3err.ErrMalformedACL),
+					&MetaOpts{
+						Logger:      c.logger,
+						Action:      metrics.ActionPutBucketAcl,
+						BucketOwner: parsedAcl.Owner,
+					})
+			}
+
+			if accessControlPolicy.Owner == nil ||
+				accessControlPolicy.Owner.ID == nil ||
+				*accessControlPolicy.Owner.ID == "" {
+				if c.debug {
+					log.Println("empty access control policy owner")
+				}
+				return SendResponse(ctx, s3err.GetAPIError(s3err.ErrMalformedACL),
+					&MetaOpts{
+						Logger:      c.logger,
+						Action:      metrics.ActionPutBucketAcl,
+						BucketOwner: parsedAcl.Owner,
+					})
+			}
+
+			if *accessControlPolicy.Owner.ID != parsedAcl.Owner {
+				if c.debug {
+					log.Printf("invalid access control policy owner id: %v, expected %v", *accessControlPolicy.Owner.ID, parsedAcl.Owner)
+				}
+				return SendResponse(ctx, s3err.APIError{
+					Code:           "InvalidArgument",
+					Description:    "Invalid id",
+					HTTPStatusCode: http.StatusBadRequest,
+				},
 					&MetaOpts{
 						Logger:      c.logger,
 						Action:      metrics.ActionPutBucketAcl,
@@ -1300,7 +1330,7 @@ func (c S3ApiController) PutBucketActions(ctx *fiber.Ctx) error {
 						grants, acl)
 				}
 				return SendResponse(ctx,
-					s3err.GetAPIError(s3err.ErrInvalidRequest),
+					s3err.GetAPIError(s3err.ErrUnexpectedContent),
 					&MetaOpts{
 						Logger:      c.logger,
 						MetricsMng:  c.mm,
@@ -1311,11 +1341,9 @@ func (c S3ApiController) PutBucketActions(ctx *fiber.Ctx) error {
 
 			input = &auth.PutBucketAclInput{
 				Bucket:              &bucket,
-				ACL:                 "",
 				AccessControlPolicy: &accessControlPolicy,
 			}
-		}
-		if acl != "" {
+		} else if acl != "" {
 			if acl != "private" && acl != "public-read" && acl != "public-read-write" {
 				if c.debug {
 					log.Printf("invalid acl: %q", acl)
@@ -1329,13 +1357,13 @@ func (c S3ApiController) PutBucketActions(ctx *fiber.Ctx) error {
 						BucketOwner: parsedAcl.Owner,
 					})
 			}
-			if len(ctx.Body()) > 0 || grants != "" {
+			if grants != "" {
 				if c.debug {
 					log.Printf("invalid request: %q (grants) %q (acl)",
 						grants, acl)
 				}
 				return SendResponse(ctx,
-					s3err.GetAPIError(s3err.ErrInvalidRequest),
+					s3err.GetAPIError(s3err.ErrBothCannedAndHeaderGrants),
 					&MetaOpts{
 						Logger:      c.logger,
 						MetricsMng:  c.mm,
@@ -1347,14 +1375,8 @@ func (c S3ApiController) PutBucketActions(ctx *fiber.Ctx) error {
 			input = &auth.PutBucketAclInput{
 				Bucket: &bucket,
 				ACL:    types.BucketCannedACL(acl),
-				AccessControlPolicy: &auth.AccessControlPolicy{
-					Owner: types.Owner{
-						ID: &acct.Access,
-					},
-				},
 			}
-		}
-		if grants != "" {
+		} else if grants != "" {
 			input = &auth.PutBucketAclInput{
 				Bucket:           &bucket,
 				GrantFullControl: &grantFullControl,
@@ -1362,13 +1384,19 @@ func (c S3ApiController) PutBucketActions(ctx *fiber.Ctx) error {
 				GrantReadACP:     &grantReadACP,
 				GrantWrite:       &granWrite,
 				GrantWriteACP:    &grantWriteACP,
-				AccessControlPolicy: &auth.AccessControlPolicy{
-					Owner: types.Owner{
-						ID: &acct.Access,
-					},
-				},
-				ACL: "",
 			}
+		} else {
+			if c.debug {
+				log.Println("none of the bucket acl options has been specified: canned, req headers, req body")
+			}
+			return SendResponse(ctx,
+				s3err.GetAPIError(s3err.ErrMissingSecurityHeader),
+				&MetaOpts{
+					Logger:      c.logger,
+					MetricsMng:  c.mm,
+					Action:      metrics.ActionPutBucketAcl,
+					BucketOwner: parsedAcl.Owner,
+				})
 		}
 
 		updAcl, err := auth.UpdateACL(input, parsedAcl, c.iam, acct.Role == auth.RoleAdmin)
@@ -1454,7 +1482,7 @@ func (c S3ApiController) PutBucketActions(ctx *fiber.Ctx) error {
 		GrantWrite:       &granWrite,
 		GrantWriteACP:    &grantWriteACP,
 		AccessControlPolicy: &auth.AccessControlPolicy{
-			Owner: types.Owner{
+			Owner: &types.Owner{
 				ID: &acct.Access,
 			}},
 		ACL: types.BucketCannedACL(acl),
@@ -1893,7 +1921,7 @@ func (c S3ApiController) PutActions(ctx *fiber.Ctx) error {
 				Key:    &keyStart,
 				ACL:    "",
 				AccessControlPolicy: &types.AccessControlPolicy{
-					Owner:  &accessControlPolicy.Owner,
+					Owner:  accessControlPolicy.Owner,
 					Grants: grants,
 				},
 			}

s3api/controllers/base_test.go

@@ -858,7 +858,7 @@ func TestS3ApiController_PutBucketActions(t *testing.T) {
 				req: incorrectBucketOwner,
 			},
 			wantErr:    false,
-			statusCode: 403,
+			statusCode: 400,
 		},
 		{
 			name: "Put-bucket-acl-success",

s3err/s3err.go

@@ -127,6 +127,9 @@ const (
 	ErrBothCannedAndHeaderGrants
 	ErrOwnershipControlsNotFound
 	ErrAclNotSupported
+	ErrMalformedACL
+	ErrUnexpectedContent
+	ErrMissingSecurityHeader
 
 	// Non-AWS errors
 	ErrExistingObjectIsDirectory
@@ -496,6 +499,21 @@ var errorCodeResponse = map[ErrorCode]APIError{
 		Description:    "The bucket does not allow ACLs",
 		HTTPStatusCode: http.StatusBadRequest,
 	},
+	ErrMalformedACL: {
+		Code:           "MalformedACLError",
+		Description:    "The XML you provided was not well-formed or did not validate against our published schema",
+		HTTPStatusCode: http.StatusBadRequest,
+	},
+	ErrUnexpectedContent: {
+		Code:           "UnexpectedContent",
+		Description:    "This request does not support content",
+		HTTPStatusCode: http.StatusBadRequest,
+	},
+	ErrMissingSecurityHeader: {
+		Code:           "MissingSecurityHeader",
+		Description:    "Your request was missing a required header",
+		HTTPStatusCode: http.StatusNotFound,
+	},
 
 	// non aws errors
 	ErrExistingObjectIsDirectory: {

tests/integration/group-tests.go

@@ -293,6 +293,7 @@ func TestCompleteMultipartUpload(s *S3Conf) {
 func TestPutBucketAcl(s *S3Conf) {
 	PutBucketAcl_non_existing_bucket(s)
 	PutBucketAcl_disabled(s)
+	PutBucketAcl_none_of_the_options_specified(s)
 	PutBucketAcl_invalid_acl_canned_and_acp(s)
 	PutBucketAcl_invalid_acl_canned_and_grants(s)
 	PutBucketAcl_invalid_acl_acp_and_grants(s)
@@ -489,6 +490,7 @@ func TestAccessControl(s *S3Conf) {
 	AccessControl_multi_statement_policy(s)
 	AccessControl_bucket_ownership_to_user(s)
 	AccessControl_root_PutBucketAcl(s)
+	AccessControl_user_PutBucketAcl_with_policy_access(s)
 }
 
 type IntTests map[string]func(s *S3Conf) error
@@ -678,6 +680,8 @@ func GetIntTests() IntTests {
 		"CompleteMultipartUpload_invalid_ETag":                                CompleteMultipartUpload_invalid_ETag,
 		"CompleteMultipartUpload_success":                                     CompleteMultipartUpload_success,
 		"PutBucketAcl_non_existing_bucket":                                    PutBucketAcl_non_existing_bucket,
+		"PutBucketAcl_disabled":                                               PutBucketAcl_disabled,
+		"PutBucketAcl_none_of_the_options_specified":                          PutBucketAcl_none_of_the_options_specified,
 		"PutBucketAcl_invalid_acl_canned_and_acp":                             PutBucketAcl_invalid_acl_canned_and_acp,
 		"PutBucketAcl_invalid_acl_canned_and_grants":                          PutBucketAcl_invalid_acl_canned_and_grants,
 		"PutBucketAcl_invalid_acl_acp_and_grants":                             PutBucketAcl_invalid_acl_acp_and_grants,
@@ -787,5 +791,6 @@ func GetIntTests() IntTests {
 		"AccessControl_multi_statement_policy":                                AccessControl_multi_statement_policy,
 		"AccessControl_bucket_ownership_to_user":                              AccessControl_bucket_ownership_to_user,
 		"AccessControl_root_PutBucketAcl":                                     AccessControl_root_PutBucketAcl,
+		"AccessControl_user_PutBucketAcl_with_policy_access":                  AccessControl_user_PutBucketAcl_with_policy_access,
 	}
 }

tests/integration/tests.go

@@ -6348,6 +6348,21 @@ func PutBucketAcl_disabled(s *S3Conf) error {
 	})
 }
 
+func PutBucketAcl_none_of_the_options_specified(s *S3Conf) error {
+	testName := "PutBucketAcl_none_of_the_options_specified"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+		_, err := s3client.PutBucketAcl(ctx, &s3.PutBucketAclInput{
+			Bucket: &bucket,
+		})
+		cancel()
+		if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrMissingSecurityHeader)); err != nil {
+			return err
+		}
+		return nil
+	}, withOwnership(types.ObjectOwnershipBucketOwnerPreferred))
+}
+
 func PutBucketAcl_invalid_acl_canned_and_acp(s *S3Conf) error {
 	testName := "PutBucketAcl_invalid_acl_canned_and_acp"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
@@ -6358,7 +6373,7 @@ func PutBucketAcl_invalid_acl_canned_and_acp(s *S3Conf) error {
 			GrantRead: getPtr("user1"),
 		})
 		cancel()
-		if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrInvalidRequest)); err != nil {
+		if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrBothCannedAndHeaderGrants)); err != nil {
 			return err
 		}
 
@@ -6388,7 +6403,7 @@ func PutBucketAcl_invalid_acl_canned_and_grants(s *S3Conf) error {
 			},
 		})
 		cancel()
-		if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrInvalidRequest)); err != nil {
+		if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrUnexpectedContent)); err != nil {
 			return err
 		}
 
@@ -6418,7 +6433,7 @@ func PutBucketAcl_invalid_acl_acp_and_grants(s *S3Conf) error {
 			},
 		})
 		cancel()
-		if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrInvalidRequest)); err != nil {
+		if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrUnexpectedContent)); err != nil {
 			return err
 		}
 
@@ -6463,7 +6478,11 @@ func PutBucketAcl_invalid_owner(s *S3Conf) error {
 			},
 		})
 		cancel()
-		if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)); err != nil {
+		if err := checkApiErr(err, s3err.APIError{
+			Code:           "InvalidArgument",
+			Description:    "Invalid id",
+			HTTPStatusCode: http.StatusBadRequest,
+		}); err != nil {
 			return err
 		}
 
@@ -6474,22 +6493,23 @@ func PutBucketAcl_invalid_owner(s *S3Conf) error {
 func PutBucketAcl_invalid_owner_not_in_body(s *S3Conf) error {
 	testName := "PutBucketAcl_invalid_owner_not_in_body"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
-		if err := createUsers(s, []user{{"grt1", "grt1secret", "user"}}); err != nil {
-			return err
-		}
-
-		newConf := *s
-		newConf.awsID = "grt1"
-		newConf.awsSecret = "grt1secret"
-		userClient := s3.NewFromConfig(newConf.Config())
-
 		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
-		_, err := userClient.PutBucketAcl(ctx, &s3.PutBucketAclInput{
+		_, err := s3client.PutBucketAcl(ctx, &s3.PutBucketAclInput{
 			Bucket: &bucket,
-			ACL:    types.BucketCannedACLPublicRead,
+			AccessControlPolicy: &types.AccessControlPolicy{
+				Grants: []types.Grant{
+					{
+						Grantee: &types.Grantee{
+							Type: types.TypeCanonicalUser,
+							ID:   getPtr("grt1"),
+						},
+						Permission: types.PermissionRead,
+					},
+				},
+			},
 		})
 		cancel()
-		if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrAccessDenied)); err != nil {
+		if err := checkApiErr(err, s3err.GetAPIError(s3err.ErrMalformedACL)); err != nil {
 			return err
 		}
 
@@ -9475,6 +9495,76 @@ func AccessControl_root_PutBucketAcl(s *S3Conf) error {
 	}, withOwnership(types.ObjectOwnershipBucketOwnerPreferred))
 }
 
+func AccessControl_user_PutBucketAcl_with_policy_access(s *S3Conf) error {
+	testName := "AccessControl_user_PutBucketAcl_with_policy_access"
+	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
+		}
+
+		policy := genPolicyDoc("Allow", fmt.Sprintf(`"%v"`, usr.access), `"s3:PutBucketAcl"`, fmt.Sprintf(`"arn:aws:s3:::%v"`, bucket))
+
+		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+		_, err := s3client.PutBucketPolicy(ctx, &s3.PutBucketPolicyInput{
+			Bucket: &bucket,
+			Policy: &policy,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		userClient := getUserS3Client(usr, s)
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		_, err = userClient.PutBucketAcl(ctx, &s3.PutBucketAclInput{
+			Bucket: &bucket,
+			ACL:    types.BucketCannedACLPublicRead,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		ctx, cancel = context.WithTimeout(context.Background(), shortTimeout)
+		res, err := s3client.GetBucketAcl(ctx, &s3.GetBucketAclInput{
+			Bucket: &bucket,
+		})
+		cancel()
+		if err != nil {
+			return err
+		}
+
+		expectedGrants := []types.Grant{
+			{
+				Grantee: &types.Grantee{
+					ID:   &s.awsID,
+					Type: types.TypeCanonicalUser,
+				},
+				Permission: types.PermissionFullControl,
+			},
+			{
+				Grantee: &types.Grantee{
+					ID:   getPtr("all-users"),
+					Type: types.TypeGroup,
+				},
+				Permission: types.PermissionRead,
+			},
+		}
+
+		if !compareGrants(res.Grants, expectedGrants) {
+			return fmt.Errorf("expected the resulting grants to be %v, instead got %v", expectedGrants, res.Grants)
+		}
+
+		return nil
+	}, withOwnership(types.ObjectOwnershipBucketOwnerPreferred))
+}
+
 // IAM related tests
 // multi-user iam tests
 func IAM_user_access_denied(s *S3Conf) error {