fix: Changed bucket policy document validation to handle object conta…

…ining 'AWS' prop in principal field
versity · May 21, 2024 · 8eac24c · 8eac24c
1 parent 3d85274
commit 8eac24c
Show file tree

Hide file tree

Showing 3 changed files with 75 additions and 4 deletions.
diff --git a/auth/bucket_policy_principals.go b/auth/bucket_policy_principals.go
@@ -28,7 +28,13 @@ func (p Principals) Add(key string) {
 // Override UnmarshalJSON method to decode both []string and string properties
 func (p *Principals) UnmarshalJSON(data []byte) error {
 	ss := []string{}
+	var s string
+	var k struct {
+		AWS string
+	}
+
 	var err error
+
 	if err = json.Unmarshal(data, &ss); err == nil {
 		if len(ss) == 0 {
 			return fmt.Errorf("principals can't be empty")
@@ -37,14 +43,35 @@ func (p *Principals) UnmarshalJSON(data []byte) error {
 		for _, s := range ss {
 			p.Add(s)
 		}
+		return nil
+	} else if err = json.Unmarshal(data, &s); err == nil {
+		if s == "" {
+			return fmt.Errorf("principals can't be empty")
+		}
+		*p = make(Principals)
+		p.Add(s)
+
+		return nil
+	} else if err = json.Unmarshal(data, &k); err == nil {
+		if k.AWS == "" {
+			return fmt.Errorf("principals can't be empty")
+		}
+		*p = make(Principals)
+		p.Add(k.AWS)
+
+		return nil
 	} else {
-		var s string
-		if err = json.Unmarshal(data, &s); err == nil {
-			if s == "" {
+		var sk struct {
+			AWS []string
+		}
+		if err = json.Unmarshal(data, &sk); err == nil {
+			if len(sk.AWS) == 0 {
 				return fmt.Errorf("principals can't be empty")
 			}
 			*p = make(Principals)
-			p.Add(s)
+			for _, s := range sk.AWS {
+				p.Add(s)
+			}
 		}
 	}
 

diff --git a/tests/integration/group-tests.go b/tests/integration/group-tests.go
@@ -284,6 +284,8 @@ func TestPutBucketPolicy(s *S3Conf) {
 	PutBucketPolicy_incorrect_action_wildcard_usage(s)
 	PutBucketPolicy_empty_principals_string(s)
 	PutBucketPolicy_empty_principals_array(s)
+	PutBucketPolicy_principals_aws_struct_empty_string(s)
+	PutBucketPolicy_principals_aws_struct_empty_string_slice(s)
 	PutBucketPolicy_principals_incorrect_wildcard_usage(s)
 	PutBucketPolicy_non_existing_principals(s)
 	PutBucketPolicy_empty_resources_string(s)
@@ -619,6 +621,8 @@ func GetIntTests() IntTests {
 		"PutBucketPolicy_incorrect_action_wildcard_usage":                    PutBucketPolicy_incorrect_action_wildcard_usage,
 		"PutBucketPolicy_empty_principals_string":                            PutBucketPolicy_empty_principals_string,
 		"PutBucketPolicy_empty_principals_array":                             PutBucketPolicy_empty_principals_array,
+		"PutBucketPolicy_principals_aws_struct_empty_string":                 PutBucketPolicy_principals_aws_struct_empty_string,
+		"PutBucketPolicy_principals_aws_struct_empty_string_slice":           PutBucketPolicy_principals_aws_struct_empty_string_slice,
 		"PutBucketPolicy_principals_incorrect_wildcard_usage":                PutBucketPolicy_principals_incorrect_wildcard_usage,
 		"PutBucketPolicy_non_existing_principals":                            PutBucketPolicy_non_existing_principals,
 		"PutBucketPolicy_empty_resources_string":                             PutBucketPolicy_empty_resources_string,

diff --git a/tests/integration/tests.go b/tests/integration/tests.go
@@ -6018,6 +6018,44 @@ func PutBucketPolicy_empty_principals_array(s *S3Conf) error {
 	})
 }
 
+func PutBucketPolicy_principals_aws_struct_empty_string(s *S3Conf) error {
+	testName := "PutBucketPolicy_principals_aws_struct_empty_string"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		doc := genPolicyDoc("Allow", `{"AWS": ""}`, `"s3:*"`, `"arn:aws:s3:::*"`)
+
+		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+		_, err := s3client.PutBucketPolicy(ctx, &s3.PutBucketPolicyInput{
+			Bucket: &bucket,
+			Policy: &doc,
+		})
+		cancel()
+
+		if err := checkApiErr(err, getMalformedPolicyError("principals can't be empty")); err != nil {
+			return err
+		}
+		return nil
+	})
+}
+
+func PutBucketPolicy_principals_aws_struct_empty_string_slice(s *S3Conf) error {
+	testName := "PutBucketPolicy_principals_aws_struct_empty_string_slice"
+	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
+		doc := genPolicyDoc("Allow", `{"AWS": []}`, `"s3:*"`, `"arn:aws:s3:::*"`)
+
+		ctx, cancel := context.WithTimeout(context.Background(), shortTimeout)
+		_, err := s3client.PutBucketPolicy(ctx, &s3.PutBucketPolicyInput{
+			Bucket: &bucket,
+			Policy: &doc,
+		})
+		cancel()
+
+		if err := checkApiErr(err, getMalformedPolicyError("principals can't be empty")); err != nil {
+			return err
+		}
+		return nil
+	})
+}
+
 func PutBucketPolicy_principals_incorrect_wildcard_usage(s *S3Conf) error {
 	testName := "PutBucketPolicy_principals_incorrect_wildcard_usage"
 	return actionHandler(s, testName, func(s3client *s3.Client, bucket string) error {
@@ -6237,7 +6275,9 @@ func PutBucketPolicy_success(s *S3Conf) error {
 
 		for _, doc := range []string{
 			genPolicyDoc("Allow", `["grt1", "grt2"]`, `["s3:DeleteBucket", "s3:GetBucketAcl"]`, bucketResource),
+			genPolicyDoc("Allow", `{"AWS": ["grt1", "grt2"]}`, `["s3:DeleteBucket", "s3:GetBucketAcl"]`, bucketResource),
 			genPolicyDoc("Deny", `"*"`, `"s3:DeleteBucket"`, fmt.Sprintf(`"arn:aws:s3:::%v"`, bucket)),
+			genPolicyDoc("Deny", `{"AWS": "*"}`, `"s3:DeleteBucket"`, fmt.Sprintf(`"arn:aws:s3:::%v"`, bucket)),
 			genPolicyDoc("Allow", `"grt1"`, `["s3:PutBucketVersioning", "s3:ListMultipartUploadParts", "s3:ListBucket"]`, fmt.Sprintf(`[%v, %v]`, bucketResource, objectResource)),
 			genPolicyDoc("Allow", `"*"`, `"s3:*"`, fmt.Sprintf(`[%v, %v]`, bucketResource, objectResource)),
 			genPolicyDoc("Allow", `"*"`, `"s3:Get*"`, objectResource),