From c2cde6beb5fabf76afe3089e68b1690b1a6eba0d Mon Sep 17 00:00:00 2001
From: Harshavardhana <harsha@minio.io>
Date: Wed, 8 Jan 2020 23:00:54 -0800
Subject: [PATCH] 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      |  4 +++
 pkg/iam/policy/policy_test.go | 47 ++++++++++++++++++++++++++++++---
 pkg/policy/policy.go          |  4 +++
 pkg/policy/policy_test.go     | 49 ++++++++++++++++++++++++++++++++---
 4 files changed, 97 insertions(+), 7 deletions(-)

diff --git a/pkg/iam/policy/policy.go b/pkg/iam/policy/policy.go
index 38585cd18..a4ec9faea 100644
--- a/pkg/iam/policy/policy.go
+++ b/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
diff --git a/pkg/iam/policy/policy_test.go b/pkg/iam/policy/policy_test.go
index cf0e87089..806dbc6ef 100644
--- a/pkg/iam/policy/policy_test.go
+++ b/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 {
diff --git a/pkg/policy/policy.go b/pkg/policy/policy.go
index 38d374e27..13931c299 100644
--- a/pkg/policy/policy.go
+++ b/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
diff --git a/pkg/policy/policy_test.go b/pkg/policy/policy_test.go
index d2200e2cf..e5312f7ad 100644
--- a/pkg/policy/policy_test.go
+++ b/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 {