policy: Allow duplicate statements with different effects (#8775)

This allows "Allow" and "Deny" conflicting statements,
where we evaluate to implicit "Deny".

pkg/iam/policy/policy.go

@@ -106,6 +106,10 @@ func (iamp Policy) isValid() error {
 
 	for i := range iamp.Statements {
 		for _, statement := range iamp.Statements[i+1:] {
+			if iamp.Statements[i].Effect != statement.Effect {
+				continue
+			}
+
 			actions := iamp.Statements[i].Actions.Intersection(statement.Actions)
 			if len(actions) == 0 {
 				continue

pkg/iam/policy/policy_test.go

@@ -338,6 +338,24 @@ func TestPolicyIsValid(t *testing.T) {
 		},
 	}
 
+	case8Policy := Policy{
+		Version: DefaultVersion,
+		Statements: []Statement{
+			NewStatement(
+				policy.Allow,
+				NewActionSet(PutObjectAction),
+				NewResourceSet(NewResource("mybucket", "/myobject*")),
+				condition.NewFunctions(),
+			),
+			NewStatement(
+				policy.Allow,
+				NewActionSet(PutObjectAction),
+				NewResourceSet(NewResource("mybucket", "/myobject*")),
+				condition.NewFunctions(),
+			),
+		},
+	}
+
 	testCases := []struct {
 		policy    Policy
 		expectErr bool
@@ -353,8 +371,10 @@ func TestPolicyIsValid(t *testing.T) {
 		{case5Policy, true},
 		// Invalid statement error.
 		{case6Policy, true},
-		// Duplicate statement error.
-		{case7Policy, true},
+		// Duplicate statement different Effects.
+		{case7Policy, false},
+		// Duplicate statement same Effects.
+		{case8Policy, true},
 	}
 
 	for i, testCase := range testCases {
@@ -921,6 +941,25 @@ func TestPolicyUnmarshalJSON(t *testing.T) {
     ]
 }`)
 
+	case11Policy := Policy{
+		ID:      "MyPolicyForMyBucket1",
+		Version: DefaultVersion,
+		Statements: []Statement{
+			NewStatement(
+				policy.Allow,
+				NewActionSet(PutObjectAction),
+				NewResourceSet(NewResource("mybucket", "myobject*")),
+				condition.NewFunctions(),
+			),
+			NewStatement(
+				policy.Deny,
+				NewActionSet(PutObjectAction),
+				NewResourceSet(NewResource("mybucket", "myobject*")),
+				condition.NewFunctions(),
+			),
+		},
+	}
+
 	testCases := []struct {
 		data           []byte
 		expectedResult Policy
@@ -938,8 +977,8 @@ func TestPolicyUnmarshalJSON(t *testing.T) {
 		{case9Data, Policy{}, true},
 		// Duplicate statement error.
 		{case10Data, Policy{}, true},
-		// Duplicate statement error (Effect differs).
-		{case11Data, Policy{}, true},
+		// Duplicate statement success (Effect differs).
+		{case11Data, case11Policy, false},
 	}
 
 	for i, testCase := range testCases {

pkg/policy/policy.go

@@ -88,6 +88,10 @@ func (policy Policy) isValid() error {
 
 	for i := range policy.Statements {
 		for _, statement := range policy.Statements[i+1:] {
+			if policy.Statements[i].Effect != statement.Effect {
+				continue
+			}
+
 			principals := policy.Statements[i].Principal.Intersection(statement.Principal)
 			if principals.IsEmpty() {
 				continue

pkg/policy/policy_test.go

@@ -356,6 +356,26 @@ func TestPolicyIsValid(t *testing.T) {
 		},
 	}
 
+	case8Policy := Policy{
+		Version: DefaultVersion,
+		Statements: []Statement{
+			NewStatement(
+				Allow,
+				NewPrincipal("*"),
+				NewActionSet(PutObjectAction),
+				NewResourceSet(NewResource("mybucket", "/myobject*")),
+				condition.NewFunctions(),
+			),
+			NewStatement(
+				Allow,
+				NewPrincipal("*"),
+				NewActionSet(PutObjectAction),
+				NewResourceSet(NewResource("mybucket", "/myobject*")),
+				condition.NewFunctions(),
+			),
+		},
+	}
+
 	testCases := []struct {
 		policy    Policy
 		expectErr bool
@@ -371,8 +391,10 @@ func TestPolicyIsValid(t *testing.T) {
 		{case5Policy, true},
 		// Invalid statement error.
 		{case6Policy, true},
+		// Duplicate statement success different effects.
+		{case7Policy, false},
 		// Duplicate statement error.
-		{case7Policy, true},
+		{case8Policy, true},
 	}
 
 	for i, testCase := range testCases {
@@ -988,6 +1010,27 @@ func TestPolicyUnmarshalJSON(t *testing.T) {
     ]
 }`)
 
+	case11Policy := Policy{
+		ID:      "MyPolicyForMyBucket1",
+		Version: DefaultVersion,
+		Statements: []Statement{
+			NewStatement(
+				Allow,
+				NewPrincipal("*"),
+				NewActionSet(PutObjectAction),
+				NewResourceSet(NewResource("mybucket", "myobject*")),
+				condition.NewFunctions(),
+			),
+			NewStatement(
+				Deny,
+				NewPrincipal("*"),
+				NewActionSet(PutObjectAction),
+				NewResourceSet(NewResource("mybucket", "myobject*")),
+				condition.NewFunctions(),
+			),
+		},
+	}
+
 	testCases := []struct {
 		data           []byte
 		expectedResult Policy
@@ -1005,8 +1048,8 @@ func TestPolicyUnmarshalJSON(t *testing.T) {
 		{case9Data, Policy{}, true},
 		// Duplicate statement error.
 		{case10Data, Policy{}, true},
-		// Duplicate statement error (Effect differs).
-		{case11Data, Policy{}, true},
+		// Duplicate statement success (Effect differs).
+		{case11Data, case11Policy, false},
 	}
 
 	for i, testCase := range testCases {