d/aws_iam_policy_document: handle duplicated conditions consistently

.changelog/33093.txt

@@ -0,0 +1,7 @@
+```release-note:bug
+data-source/aws_iam_policy_document: Fix inconsistent handling of `condition` blocks with duplicated `test` and `variable` arguments
+```
+
+```release-note:note
+data-source/aws_iam_policy_document: In some cases, `statement.*.condition` blocks with the same `test` and `variable` arguments were incorrectly handled by the provider. Since this results in unexpected IAM Policies being submitted to AWS, we have updated the logic to merge `values` lists in this case. This may cause existing IAM Policy documents to report a difference. However, those policies are likely not what was originally intended.
+```

internal/service/iam/policy_document_data_source_test.go

@@ -74,6 +74,25 @@ func TestAccIAMPolicyDocumentDataSource_multipleConditionKeys(t *testing.T) {
 	})
 }
 
+func TestAccIAMPolicyDocumentDataSource_duplicateConditionKeys(t *testing.T) {
+	ctx := acctest.Context(t)
+	dataSourceName := "data.aws_iam_policy_document.test"
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:                 func() { acctest.PreCheck(ctx, t) },
+		ErrorCheck:               acctest.ErrorCheck(t, iam.EndpointsID),
+		ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
+		Steps: []resource.TestStep{
+			{
+				Config: testAccPolicyDocumentDataSourceConfig_duplicateConditionKeys,
+				Check: resource.ComposeTestCheckFunc(
+					acctest.CheckResourceAttrEquivalentJSON(dataSourceName, "json", testAccPolicyDocumentConfig_duplicateConditionKeys_ExpectedJSON),
+				),
+			},
+		},
+	})
+}
+
 func TestAccIAMPolicyDocumentDataSource_conditionWithBoolValue(t *testing.T) {
 	ctx := acctest.Context(t)
 	resource.ParallelTest(t, resource.TestCase{
@@ -662,6 +681,57 @@ var testAccPolicyDocumentConfig_multipleConditionKeys_ExpectedJSON = `{
 }
 `
 
+const testAccPolicyDocumentDataSourceConfig_duplicateConditionKeys = `
+data "aws_iam_policy_document" "test" {
+  statement {
+    sid = "DuplicateConditionTest"
+
+    effect = "Allow"
+
+    principals {
+      type        = "Service"
+      identifiers = ["cloudtrail.amazonaws.com"]
+    }
+
+    actions = ["s3:PutObject"]
+
+    resources = ["*"]
+
+    condition {
+      test     = "StringEquals"
+      variable = "s3:prefix"
+      values   = ["one/", "two/"]
+    }
+    condition {
+      test     = "StringEquals"
+      variable = "s3:prefix"
+      values   = ["three/"]
+    }
+  }
+}
+`
+
+const testAccPolicyDocumentConfig_duplicateConditionKeys_ExpectedJSON = `{
+  "Version": "2012-10-17",
+  "Statement": [
+    {
+      "Sid": "DuplicateConditionTest",
+      "Effect": "Allow",
+      "Action": "s3:PutObject",
+      "Resource": "*",
+      "Principal": {
+        "Service": "cloudtrail.amazonaws.com"
+      },
+      "Condition": {
+        "StringEquals": {
+          "s3:prefix": ["one/", "two/", "three/"]
+        }
+      }
+    }
+  ]
+}
+`
+
 var testAccPolicyDocumentDataSourceConfig_deprecated = `
 data "aws_partition" "current" {}
 
@@ -1409,12 +1479,8 @@ func testAccPolicyDocumentConditionWithBoolValueExpectedJSON() string {
                     "aws:ResourceTag/SpecialTag": "false"
                 },
                 "StringLike": {
-                    "aws:ResourceAccount": [
-                        "123456"
-                    ],
-                    "aws:PrincipalArn": [
-						"arn:%[1]s:iam::*:role/AWSAFTExecution"
-					]
+                    "aws:ResourceAccount": "123456",
+                    "aws:PrincipalArn": "arn:%[1]s:iam::*:role/AWSAFTExecution"
                 }
             }
         }

internal/service/iam/policy_model.go

@@ -177,20 +177,30 @@ func (cs IAMPolicyStatementConditionSet) MarshalJSON() ([]byte, error) {
 		if _, ok := raw[c.Test]; !ok {
 			raw[c.Test] = map[string]interface{}{}
 		}
+		if _, ok := raw[c.Test][c.Variable]; !ok {
+			raw[c.Test][c.Variable] = []string{}
+		}
 		switch i := c.Values.(type) {
 		case []string:
-			if _, ok := raw[c.Test][c.Variable]; !ok {
-				raw[c.Test][c.Variable] = make([]string, 0, len(i))
-			}
 			// order matters with values so not sorting here
 			raw[c.Test][c.Variable] = append(raw[c.Test][c.Variable].([]string), i...)
 		case string:
-			raw[c.Test][c.Variable] = i
+			raw[c.Test][c.Variable] = append(raw[c.Test][c.Variable].([]string), i)
 		default:
 			return nil, fmt.Errorf("Unsupported data type for IAMPolicyStatementConditionSet: %s", i)
 		}
 	}
 
+	// flatten entries with a single item to match AWS IAM syntax
+	for k1 := range raw {
+		for k2 := range raw[k1] {
+			items := raw[k1][k2].([]string)
+			if len(items) == 1 {
+				raw[k1][k2] = items[0]
+			}
+		}
+	}
+
 	return json.Marshal(&raw)
 }
 

internal/service/iam/policy_model_test.go

@@ -5,6 +5,7 @@ package iam
 
 import (
 	"encoding/json"
+	"reflect"
 	"testing"
 
 	"github.com/hashicorp/terraform-provider-aws/internal/errs"
@@ -265,3 +266,127 @@ func TestIsValidAWSPrincipal(t *testing.T) { // nosemgrep:ci.aws-in-func-name
 		})
 	}
 }
+
+func TestIAMPolicyStatementConditionSet_MarshalJSON(t *testing.T) { // nosemgrep:ci.iam-in-func-name
+	t.Parallel()
+
+	testcases := map[string]struct {
+		cs      IAMPolicyStatementConditionSet
+		want    []byte
+		wantErr bool
+	}{
+		"invalid value type": {
+			cs: IAMPolicyStatementConditionSet{
+				{Test: "StringLike", Variable: "s3:prefix", Values: 1},
+			},
+			wantErr: true,
+		},
+		"single condition single value": {
+			cs: IAMPolicyStatementConditionSet{
+				{Test: "StringLike", Variable: "s3:prefix", Values: "one/"},
+			},
+			want: []byte(`{"StringLike":{"s3:prefix":"one/"}}`),
+		},
+		"single condition multiple values": {
+			cs: IAMPolicyStatementConditionSet{
+				{Test: "StringLike", Variable: "s3:prefix", Values: []string{"one/", "two/"}},
+			},
+			want: []byte(`{"StringLike":{"s3:prefix":["one/","two/"]}}`),
+		},
+		// Multiple distinct conditions
+		"multiple condition single value": {
+			cs: IAMPolicyStatementConditionSet{
+				{Test: "ArnNotLike", Variable: "aws:PrincipalArn", Values: "1"},
+				{Test: "StringLike", Variable: "s3:prefix", Values: "one/"},
+			},
+			want: []byte(`{"ArnNotLike":{"aws:PrincipalArn":"1"},"StringLike":{"s3:prefix":"one/"}}`),
+		},
+		"multiple condition multiple values": {
+			cs: IAMPolicyStatementConditionSet{
+				{Test: "ArnNotLike", Variable: "aws:PrincipalArn", Values: []string{"1", "2"}},
+				{Test: "StringLike", Variable: "s3:prefix", Values: []string{"one/", "two/"}},
+			},
+			want: []byte(`{"ArnNotLike":{"aws:PrincipalArn":["1","2"]},"StringLike":{"s3:prefix":["one/","two/"]}}`),
+		},
+		"multiple condition mixed value lengths": {
+			cs: IAMPolicyStatementConditionSet{
+				{Test: "ArnNotLike", Variable: "aws:PrincipalArn", Values: "1"},
+				{Test: "StringLike", Variable: "s3:prefix", Values: []string{"one/", "two/"}},
+			},
+			want: []byte(`{"ArnNotLike":{"aws:PrincipalArn":"1"},"StringLike":{"s3:prefix":["one/","two/"]}}`),
+		},
+		// Multiple conditions with duplicated `test` arguments
+		"duplicate condition test single value": {
+			cs: IAMPolicyStatementConditionSet{
+				{Test: "StringLike", Variable: "s3:prefix", Values: "one/"},
+				{Test: "StringLike", Variable: "s3:versionid", Values: "abc123"},
+			},
+			want: []byte(`{"StringLike":{"s3:prefix":"one/","s3:versionid":"abc123"}}`),
+		},
+		"duplicate condition test multiple values": {
+			cs: IAMPolicyStatementConditionSet{
+				{Test: "StringLike", Variable: "s3:prefix", Values: []string{"one/", "two/"}},
+				{Test: "StringLike", Variable: "s3:versionid", Values: []string{"abc123", "def456"}},
+			},
+			want: []byte(`{"StringLike":{"s3:prefix":["one/","two/"],"s3:versionid":["abc123","def456"]}}`),
+		},
+		"duplicate condition test mixed value lengths": {
+			cs: IAMPolicyStatementConditionSet{
+				{Test: "StringLike", Variable: "s3:prefix", Values: "one/"},
+				{Test: "StringLike", Variable: "s3:versionid", Values: []string{"abc123", "def456"}},
+			},
+			want: []byte(`{"StringLike":{"s3:prefix":"one/","s3:versionid":["abc123","def456"]}}`),
+		},
+		"duplicate condition test mixed value lengths reversed": {
+			cs: IAMPolicyStatementConditionSet{
+				{Test: "StringLike", Variable: "s3:prefix", Values: []string{"one/", "two/"}},
+				{Test: "StringLike", Variable: "s3:versionid", Values: "abc123"},
+			},
+			want: []byte(`{"StringLike":{"s3:prefix":["one/","two/"],"s3:versionid":"abc123"}}`),
+		},
+		// Multiple conditions with duplicated `test` and `variable` arguments
+		"duplicate condition test and variable single value": {
+			cs: IAMPolicyStatementConditionSet{
+				{Test: "StringLike", Variable: "s3:prefix", Values: "one/"},
+				{Test: "StringLike", Variable: "s3:prefix", Values: "two/"},
+			},
+			want: []byte(`{"StringLike":{"s3:prefix":["one/","two/"]}}`),
+		},
+		"duplicate condition test and variable multiple values": {
+			cs: IAMPolicyStatementConditionSet{
+				{Test: "StringLike", Variable: "s3:prefix", Values: []string{"one/", "two/"}},
+				{Test: "StringLike", Variable: "s3:prefix", Values: []string{"three/", "four/"}},
+			},
+			want: []byte(`{"StringLike":{"s3:prefix":["one/","two/","three/","four/"]}}`),
+		},
+		"duplicate condition test and variable mixed value lengths": {
+			cs: IAMPolicyStatementConditionSet{
+				{Test: "StringLike", Variable: "s3:prefix", Values: "one/"},
+				{Test: "StringLike", Variable: "s3:prefix", Values: []string{"three/", "four/"}},
+			},
+			want: []byte(`{"StringLike":{"s3:prefix":["one/","three/","four/"]}}`),
+		},
+		"duplicate condition test and variable mixed value lengths reversed": {
+			cs: IAMPolicyStatementConditionSet{
+				{Test: "StringLike", Variable: "s3:prefix", Values: []string{"one/", "two/"}},
+				{Test: "StringLike", Variable: "s3:prefix", Values: "three/"},
+			},
+			want: []byte(`{"StringLike":{"s3:prefix":["one/","two/","three/"]}}`),
+		},
+	}
+	for name, tc := range testcases {
+		tc := tc
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+
+			got, err := tc.cs.MarshalJSON()
+			if (err != nil) != tc.wantErr {
+				t.Errorf("IAMPolicyStatementConditionSet.MarshalJSON() error = %v, wantErr %v", err, tc.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, tc.want) {
+				t.Errorf("IAMPolicyStatementConditionSet.MarshalJSON() = %v, want %v", string(got), string(tc.want))
+			}
+		})
+	}
+}