From 75107d7698b3f010603ff2cd65d1e6f293095be6 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Fri, 17 Apr 2020 21:26:42 -0700 Subject: [PATCH] fix: remove any duplicate statements in policy input (#9385) Add support for removing duplicate statements automatically --- pkg/bucket/policy/policy.go | 40 +++++++++++++++++--------------- pkg/bucket/policy/policy_test.go | 26 ++++++++++++++++----- pkg/iam/policy/policy.go | 38 ++++++++++++++++-------------- pkg/iam/policy/policy_test.go | 24 ++++++++++++++----- 4 files changed, 80 insertions(+), 48 deletions(-) diff --git a/pkg/bucket/policy/policy.go b/pkg/bucket/policy/policy.go index cbdbbc081..59bdcf999 100644 --- a/pkg/bucket/policy/policy.go +++ b/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 } - - return Errorf("duplicate principal %v, actions %v, resouces %v found in statements %v, %v", - statement.Principal, statement.Actions, - statement.Resources, policy.Statements[i], - statement) + policy.Statements = append(policy.Statements[:j], policy.Statements[j+1:]...) + goto redo } } - - 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 diff --git a/pkg/bucket/policy/policy_test.go b/pkg/bucket/policy/policy_test.go index 6f4e3dd2b..039dd6126 100644 --- a/pkg/bucket/policy/policy_test.go +++ b/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. - {case8Policy, true}, + // Duplicate statement success, duplicate statement dropped. + {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. - {case10Data, Policy{}, true}, + // Duplicate statement success, duplicate statement removed. + {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) } } } diff --git a/pkg/iam/policy/policy.go b/pkg/iam/policy/policy.go index ddb700912..e150ca200 100644 --- a/pkg/iam/policy/policy.go +++ b/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 } - - return Errorf("duplicate actions %v, resources %v found in statements %v, %v", - statement.Actions, statement.Resources, iamp.Statements[i], statement) + iamp.Statements = append(iamp.Statements[:j], iamp.Statements[j+1:]...) + goto redo } } - - 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 diff --git a/pkg/iam/policy/policy_test.go b/pkg/iam/policy/policy_test.go index 53afa5163..a981e5186 100644 --- a/pkg/iam/policy/policy_test.go +++ b/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. - {case8Policy, true}, + // Duplicate statement same Effects, duplicate effect will be removed. + {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. - {case10Data, Policy{}, true}, + // Duplicate statement success, duplicate statement is removed. + {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) } } }