fix: remove any duplicate statements in policy input (#9385)

Add support for removing duplicate statements automatically

pkg/bucket/policy/policy.go

@@ -86,8 +86,24 @@ func (policy Policy) isValid() error {
 		}
 	}
 
+	return nil
+}
+
+// MarshalJSON - encodes Policy to JSON data.
+func (policy Policy) MarshalJSON() ([]byte, error) {
+	if err := policy.isValid(); err != nil {
+		return nil, err
+	}
+
+	// subtype to avoid recursive call to MarshalJSON()
+	type subPolicy Policy
+	return json.Marshal(subPolicy(policy))
+}
+
+func (policy *Policy) dropDuplicateStatements() {
+redo:
 	for i := range policy.Statements {
-		for _, statement := range policy.Statements[i+1:] {
+		for j, statement := range policy.Statements[i+1:] {
 			if policy.Statements[i].Effect != statement.Effect {
 				continue
 			}
@@ -107,26 +123,10 @@ func (policy Policy) isValid() error {
 			if policy.Statements[i].Conditions.String() != statement.Conditions.String() {
 				continue
 			}
-
+			policy.Statements = append(policy.Statements[:j], policy.Statements[j+1:]...)
-			return Errorf("duplicate principal %v, actions %v, resouces %v found in statements %v, %v",
+			goto redo
-				statement.Principal, statement.Actions,
-				statement.Resources, policy.Statements[i],
-				statement)
 		}
 	}
-
-	return nil
-}
-
-// MarshalJSON - encodes Policy to JSON data.
-func (policy Policy) MarshalJSON() ([]byte, error) {
-	if err := policy.isValid(); err != nil {
-		return nil, err
-	}
-
-	// subtype to avoid recursive call to MarshalJSON()
-	type subPolicy Policy
-	return json.Marshal(subPolicy(policy))
 }
 
 // UnmarshalJSON - decodes JSON data to Policy.
@@ -143,6 +143,8 @@ func (policy *Policy) UnmarshalJSON(data []byte) error {
 		return err
 	}
 
+	p.dropDuplicateStatements()
+
 	*policy = p
 
 	return nil

pkg/bucket/policy/policy_test.go

@@ -393,8 +393,8 @@ func TestPolicyIsValid(t *testing.T) {
 		{case6Policy, true},
 		// Duplicate statement success different effects.
 		{case7Policy, false},
-		// Duplicate statement error.
+		// Duplicate statement success, duplicate statement dropped.
-		{case8Policy, true},
+		{case8Policy, false},
 	}
 
 	for i, testCase := range testCases {
@@ -991,6 +991,20 @@ func TestPolicyUnmarshalJSON(t *testing.T) {
     ]
 }`)
 
+	case10Policy := Policy{
+		ID:      "MyPolicyForMyBucket1",
+		Version: DefaultVersion,
+		Statements: []Statement{
+			NewStatement(
+				Allow,
+				NewPrincipal("*"),
+				NewActionSet(PutObjectAction),
+				NewResourceSet(NewResource("mybucket", "myobject*")),
+				condition.NewFunctions(),
+			),
+		},
+	}
+
 	case11Data := []byte(`{
     "ID": "MyPolicyForMyBucket1",
     "Version": "2012-10-17",
@@ -1046,8 +1060,8 @@ func TestPolicyUnmarshalJSON(t *testing.T) {
 		{case8Data, case8Policy, false},
 		// Invalid version error.
 		{case9Data, Policy{}, true},
-		// Duplicate statement error.
+		// Duplicate statement success, duplicate statement removed.
-		{case10Data, Policy{}, true},
+		{case10Data, case10Policy, false},
 		// Duplicate statement success (Effect differs).
 		{case11Data, case11Policy, false},
 	}
@@ -1058,12 +1072,12 @@ func TestPolicyUnmarshalJSON(t *testing.T) {
 		expectErr := (err != nil)
 
 		if expectErr != testCase.expectErr {
-			t.Fatalf("case %v: error: expected: %v, got: %v", i+1, testCase.expectErr, expectErr)
+			t.Errorf("case %v: error: expected: %v, got: %v", i+1, testCase.expectErr, expectErr)
 		}
 
 		if !testCase.expectErr {
 			if !reflect.DeepEqual(result, testCase.expectedResult) {
-				t.Fatalf("case %v: result: expected: %v, got: %v", i+1, testCase.expectedResult, result)
+				t.Errorf("case %v: result: expected: %v, got: %v", i+1, testCase.expectedResult, result)
 			}
 		}
 	}

pkg/iam/policy/policy.go

@@ -104,8 +104,24 @@ func (iamp Policy) isValid() error {
 		}
 	}
 
+	return nil
+}
+
+// MarshalJSON - encodes Policy to JSON data.
+func (iamp Policy) MarshalJSON() ([]byte, error) {
+	if err := iamp.isValid(); err != nil {
+		return nil, err
+	}
+
+	// subtype to avoid recursive call to MarshalJSON()
+	type subPolicy Policy
+	return json.Marshal(subPolicy(iamp))
+}
+
+func (iamp *Policy) dropDuplicateStatements() {
+redo:
 	for i := range iamp.Statements {
-		for _, statement := range iamp.Statements[i+1:] {
+		for j, statement := range iamp.Statements[i+1:] {
 			if iamp.Statements[i].Effect != statement.Effect {
 				continue
 			}
@@ -121,24 +137,10 @@ func (iamp Policy) isValid() error {
 			if iamp.Statements[i].Conditions.String() != statement.Conditions.String() {
 				continue
 			}
-
+			iamp.Statements = append(iamp.Statements[:j], iamp.Statements[j+1:]...)
-			return Errorf("duplicate actions %v, resources %v found in statements %v, %v",
+			goto redo
-				statement.Actions, statement.Resources, iamp.Statements[i], statement)
 		}
 	}
-
-	return nil
-}
-
-// MarshalJSON - encodes Policy to JSON data.
-func (iamp Policy) MarshalJSON() ([]byte, error) {
-	if err := iamp.isValid(); err != nil {
-		return nil, err
-	}
-
-	// subtype to avoid recursive call to MarshalJSON()
-	type subPolicy Policy
-	return json.Marshal(subPolicy(iamp))
 }
 
 // UnmarshalJSON - decodes JSON data to Iamp.
@@ -155,6 +157,8 @@ func (iamp *Policy) UnmarshalJSON(data []byte) error {
 		return err
 	}
 
+	p.dropDuplicateStatements()
+
 	*iamp = p
 
 	return nil

pkg/iam/policy/policy_test.go

@@ -373,8 +373,8 @@ func TestPolicyIsValid(t *testing.T) {
 		{case6Policy, true},
 		// Duplicate statement different Effects.
 		{case7Policy, false},
-		// Duplicate statement same Effects.
+		// Duplicate statement same Effects, duplicate effect will be removed.
-		{case8Policy, true},
+		{case8Policy, false},
 	}
 
 	for i, testCase := range testCases {
@@ -923,6 +923,18 @@ func TestPolicyUnmarshalJSON(t *testing.T) {
         }
     ]
 }`)
+	case10Policy := Policy{
+		ID:      "MyPolicyForMyBucket1",
+		Version: DefaultVersion,
+		Statements: []Statement{
+			NewStatement(
+				policy.Allow,
+				NewActionSet(PutObjectAction),
+				NewResourceSet(NewResource("mybucket", "myobject*")),
+				condition.NewFunctions(),
+			),
+		},
+	}
 
 	case11Data := []byte(`{
     "ID": "MyPolicyForMyBucket1",
@@ -975,8 +987,8 @@ func TestPolicyUnmarshalJSON(t *testing.T) {
 		{case8Data, case8Policy, false},
 		// Invalid version error.
 		{case9Data, Policy{}, true},
-		// Duplicate statement error.
+		// Duplicate statement success, duplicate statement is removed.
-		{case10Data, Policy{}, true},
+		{case10Data, case10Policy, false},
 		// Duplicate statement success (Effect differs).
 		{case11Data, case11Policy, false},
 	}
@@ -987,12 +999,12 @@ func TestPolicyUnmarshalJSON(t *testing.T) {
 		expectErr := (err != nil)
 
 		if expectErr != testCase.expectErr {
-			t.Fatalf("case %v: error: expected: %v, got: %v", i+1, testCase.expectErr, expectErr)
+			t.Errorf("case %v: error: expected: %v, got: %v", i+1, testCase.expectErr, expectErr)
 		}
 
 		if !testCase.expectErr {
 			if !reflect.DeepEqual(result, testCase.expectedResult) {
-				t.Fatalf("case %v: result: expected: %v, got: %v", i+1, testCase.expectedResult, result)
+				t.Errorf("case %v: result: expected: %v, got: %v", i+1, testCase.expectedResult, result)
 			}
 		}
 	}