diff --git a/pkg/bucket/policy/effect.go b/pkg/bucket/policy/effect.go index 67a6e2092..31167587a 100644 --- a/pkg/bucket/policy/effect.go +++ b/pkg/bucket/policy/effect.go @@ -16,10 +16,6 @@ package policy -import ( - "encoding/json" -) - // Effect - policy statement effect Allow or Deny. type Effect string @@ -49,29 +45,3 @@ func (effect Effect) IsValid() bool { return false } - -// MarshalJSON - encodes Effect to JSON data. -func (effect Effect) MarshalJSON() ([]byte, error) { - if !effect.IsValid() { - return nil, Errorf("invalid effect '%v'", effect) - } - - return json.Marshal(string(effect)) -} - -// UnmarshalJSON - decodes JSON data to Effect. -func (effect *Effect) UnmarshalJSON(data []byte) error { - var s string - if err := json.Unmarshal(data, &s); err != nil { - return err - } - - e := Effect(s) - if !e.IsValid() { - return Errorf("invalid effect '%v'", s) - } - - *effect = e - - return nil -} diff --git a/pkg/bucket/policy/effect_test.go b/pkg/bucket/policy/effect_test.go index d01b3e76e..5797aaa15 100644 --- a/pkg/bucket/policy/effect_test.go +++ b/pkg/bucket/policy/effect_test.go @@ -17,8 +17,6 @@ package policy import ( - "encoding/json" - "reflect" "testing" ) @@ -63,60 +61,3 @@ func TestEffectIsValid(t *testing.T) { } } } - -func TestEffectMarshalJSON(t *testing.T) { - testCases := []struct { - effect Effect - expectedResult []byte - expectErr bool - }{ - {Allow, []byte(`"Allow"`), false}, - {Deny, []byte(`"Deny"`), false}, - {Effect(""), nil, true}, - {Effect("foo"), nil, true}, - } - - for i, testCase := range testCases { - result, err := json.Marshal(testCase.effect) - expectErr := (err != nil) - - if expectErr != testCase.expectErr { - t.Fatalf("case %v: error: expected: %v, got: %v\n", i+1, testCase.expectErr, expectErr) - } - - if !testCase.expectErr { - if !reflect.DeepEqual(result, testCase.expectedResult) { - t.Fatalf("case %v: result: expected: %v, got: %v\n", i+1, string(testCase.expectedResult), string(result)) - } - } - } -} - -func TestEffectUnmarshalJSON(t *testing.T) { - testCases := []struct { - data []byte - expectedResult Effect - expectErr bool - }{ - {[]byte(`"Allow"`), Allow, false}, - {[]byte(`"Deny"`), Deny, false}, - {[]byte(`""`), Effect(""), true}, - {[]byte(`"foo"`), Effect(""), true}, - } - - for i, testCase := range testCases { - var result Effect - err := json.Unmarshal(testCase.data, &result) - expectErr := (err != nil) - - if expectErr != testCase.expectErr { - t.Fatalf("case %v: error: expected: %v, got: %v\n", i+1, testCase.expectErr, expectErr) - } - - if !testCase.expectErr { - if result != testCase.expectedResult { - t.Fatalf("case %v: result: expected: %v, got: %v\n", i+1, testCase.expectedResult, result) - } - } - } -} diff --git a/pkg/bucket/policy/id.go b/pkg/bucket/policy/id.go index 4bae2863e..62e48e6c2 100644 --- a/pkg/bucket/policy/id.go +++ b/pkg/bucket/policy/id.go @@ -17,7 +17,6 @@ package policy import ( - "encoding/json" "unicode/utf8" ) @@ -28,29 +27,3 @@ type ID string func (id ID) IsValid() bool { return utf8.ValidString(string(id)) } - -// MarshalJSON - encodes ID to JSON data. -func (id ID) MarshalJSON() ([]byte, error) { - if !id.IsValid() { - return nil, Errorf("invalid ID %v", id) - } - - return json.Marshal(string(id)) -} - -// UnmarshalJSON - decodes JSON data to ID. -func (id *ID) UnmarshalJSON(data []byte) error { - var s string - if err := json.Unmarshal(data, &s); err != nil { - return err - } - - i := ID(s) - if !i.IsValid() { - return Errorf("invalid ID %v", s) - } - - *id = i - - return nil -} diff --git a/pkg/bucket/policy/id_test.go b/pkg/bucket/policy/id_test.go index e84885ef5..b1f7d5dfd 100644 --- a/pkg/bucket/policy/id_test.go +++ b/pkg/bucket/policy/id_test.go @@ -17,8 +17,6 @@ package policy import ( - "encoding/json" - "reflect" "testing" ) @@ -40,58 +38,3 @@ func TestIDIsValid(t *testing.T) { } } } - -func TestIDMarshalJSON(t *testing.T) { - testCases := []struct { - id ID - expectedResult []byte - expectErr bool - }{ - {ID("DenyEncryptionSt1"), []byte(`"DenyEncryptionSt1"`), false}, - {ID(""), []byte(`""`), false}, - {ID("aa\xe2"), nil, true}, // invalid utf-8 - } - - for i, testCase := range testCases { - result, err := json.Marshal(testCase.id) - expectErr := (err != nil) - - if expectErr != testCase.expectErr { - t.Fatalf("case %v: error: expected: %v, got: %v\n", i+1, testCase.expectErr, expectErr) - } - - if !testCase.expectErr { - if !reflect.DeepEqual(result, testCase.expectedResult) { - t.Fatalf("case %v: result: expected: %v, got: %v\n", i+1, string(testCase.expectedResult), string(result)) - } - } - } -} - -func TestIDUnmarshalJSON(t *testing.T) { - testCases := []struct { - data []byte - expectedResult ID - expectErr bool - }{ - {[]byte(`"DenyEncryptionSt1"`), ID("DenyEncryptionSt1"), false}, - {[]byte(`""`), ID(""), false}, - {[]byte(`"aa\xe2"`), ID(""), true}, - } - - for i, testCase := range testCases { - var result ID - err := json.Unmarshal(testCase.data, &result) - expectErr := (err != nil) - - if expectErr != testCase.expectErr { - t.Fatalf("case %v: error: expected: %v, got: %v\n", i+1, testCase.expectErr, expectErr) - } - - if !testCase.expectErr { - if result != testCase.expectedResult { - t.Fatalf("case %v: result: expected: %v, got: %v\n", i+1, testCase.expectedResult, result) - } - } - } -} diff --git a/pkg/iam/policy/action.go b/pkg/iam/policy/action.go index cda186c73..bded00c77 100644 --- a/pkg/iam/policy/action.go +++ b/pkg/iam/policy/action.go @@ -17,8 +17,6 @@ package iampolicy import ( - "encoding/json" - "github.com/minio/minio/pkg/bucket/policy/condition" "github.com/minio/minio/pkg/wildcard" ) @@ -215,47 +213,6 @@ func (action Action) IsValid() bool { return ok } -// MarshalJSON - encodes Action to JSON data. -func (action Action) MarshalJSON() ([]byte, error) { - if action.IsValid() || AdminAction(action).IsValid() { - return json.Marshal(string(action)) - } - - return nil, Errorf("invalid action '%v'", action) -} - -// UnmarshalJSON - decodes JSON data to Action. -func (action *Action) UnmarshalJSON(data []byte) error { - var s string - - if err := json.Unmarshal(data, &s); err != nil { - return err - } - - a := Action(s) - if !a.IsValid() { - return Errorf("invalid action '%v'", s) - } - - *action = a - - return nil -} - -func parseAction(s string) (Action, error) { - adminAction, err := parseAdminAction(s) - if err == nil { - return Action(adminAction), nil - } - action := Action(s) - - if action.IsValid() { - return action, nil - } - - return action, Errorf("unsupported action '%v'", s) -} - // actionConditionKeyMap - holds mapping of supported condition key for an action. var actionConditionKeyMap = map[Action]condition.KeySet{ AllActions: condition.NewKeySet(condition.AllSupportedKeys...), diff --git a/pkg/iam/policy/action_test.go b/pkg/iam/policy/action_test.go index 94f0f3fd3..653ee4a57 100644 --- a/pkg/iam/policy/action_test.go +++ b/pkg/iam/policy/action_test.go @@ -17,8 +17,6 @@ package iampolicy import ( - "encoding/json" - "reflect" "testing" ) @@ -49,6 +47,7 @@ func TestActionIsValid(t *testing.T) { action Action expectedResult bool }{ + {PutObjectAction, true}, {AbortMultipartUploadAction, true}, {Action("foo"), false}, } @@ -61,56 +60,3 @@ func TestActionIsValid(t *testing.T) { } } } - -func TestActionMarshalJSON(t *testing.T) { - testCases := []struct { - action Action - expectedResult []byte - expectErr bool - }{ - {PutObjectAction, []byte(`"s3:PutObject"`), false}, - {Action("foo"), nil, true}, - } - - for i, testCase := range testCases { - result, err := json.Marshal(testCase.action) - expectErr := (err != nil) - - if testCase.expectErr != expectErr { - t.Fatalf("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) - } - } - } -} - -func TestActionUnmarshalJSON(t *testing.T) { - testCases := []struct { - data []byte - expectedResult Action - expectErr bool - }{ - {[]byte(`"s3:PutObject"`), PutObjectAction, false}, - {[]byte(`"foo"`), Action(""), true}, - } - - for i, testCase := range testCases { - var result Action - err := json.Unmarshal(testCase.data, &result) - expectErr := (err != nil) - - if testCase.expectErr != expectErr { - t.Fatalf("case %v: error: expected: %v, got: %v", i+1, testCase.expectErr, expectErr) - } - - if !testCase.expectErr { - if testCase.expectedResult != result { - t.Fatalf("case %v: result: expected: %v, got: %v", i+1, testCase.expectedResult, result) - } - } - } -} diff --git a/pkg/iam/policy/actionset.go b/pkg/iam/policy/actionset.go index e670c7d6c..ef6ad588a 100644 --- a/pkg/iam/policy/actionset.go +++ b/pkg/iam/policy/actionset.go @@ -107,6 +107,16 @@ func (actionSet ActionSet) ToSlice() []Action { return actions } +// ToAdminSlice - returns slice of admin actions from the action set. +func (actionSet ActionSet) ToAdminSlice() []AdminAction { + actions := []AdminAction{} + for action := range actionSet { + actions = append(actions, AdminAction(action)) + } + + return actions +} + // UnmarshalJSON - decodes JSON data to ActionSet. func (actionSet *ActionSet) UnmarshalJSON(data []byte) error { var sset set.StringSet @@ -120,14 +130,29 @@ func (actionSet *ActionSet) UnmarshalJSON(data []byte) error { *actionSet = make(ActionSet) for _, s := range sset.ToSlice() { - action, err := parseAction(s) - if err != nil { - return err - } + actionSet.Add(Action(s)) + } - actionSet.Add(action) + return nil +} + +// ValidateAdmin checks if all actions are valid Admin actions +func (actionSet ActionSet) ValidateAdmin() error { + for _, action := range actionSet.ToAdminSlice() { + if !action.IsValid() { + return Errorf("unsupported admin action '%v'", action) + } } + return nil +} +// Validate checks if all actions are valid +func (actionSet ActionSet) Validate() error { + for _, action := range actionSet.ToSlice() { + if !action.IsValid() { + return Errorf("unsupported action '%v'", action) + } + } return nil } diff --git a/pkg/iam/policy/actionset_test.go b/pkg/iam/policy/actionset_test.go index 37dc26fde..9cb5c6116 100644 --- a/pkg/iam/policy/actionset_test.go +++ b/pkg/iam/policy/actionset_test.go @@ -128,17 +128,18 @@ func TestActionSetToSlice(t *testing.T) { func TestActionSetUnmarshalJSON(t *testing.T) { testCases := []struct { - data []byte - expectedResult ActionSet - expectErr bool + data []byte + expectedResult ActionSet + expectUnmarshalErr bool + expectValidateErr bool }{ - {[]byte(`"s3:PutObject"`), NewActionSet(PutObjectAction), false}, - {[]byte(`["s3:PutObject"]`), NewActionSet(PutObjectAction), false}, - {[]byte(`["s3:PutObject", "s3:GetObject"]`), NewActionSet(PutObjectAction, GetObjectAction), false}, - {[]byte(`["s3:PutObject", "s3:GetObject", "s3:PutObject"]`), NewActionSet(PutObjectAction, GetObjectAction), false}, - {[]byte(`[]`), NewActionSet(), true}, // Empty array. - {[]byte(`"foo"`), nil, true}, // Invalid action. - {[]byte(`["s3:PutObject", "foo"]`), nil, true}, // Invalid action. + {[]byte(`"s3:PutObject"`), NewActionSet(PutObjectAction), false, false}, + {[]byte(`["s3:PutObject"]`), NewActionSet(PutObjectAction), false, false}, + {[]byte(`["s3:PutObject", "s3:GetObject"]`), NewActionSet(PutObjectAction, GetObjectAction), false, false}, + {[]byte(`["s3:PutObject", "s3:GetObject", "s3:PutObject"]`), NewActionSet(PutObjectAction, GetObjectAction), false, false}, + {[]byte(`[]`), NewActionSet(), true, false}, // Empty array. + {[]byte(`"foo"`), nil, false, true}, // Invalid action. + {[]byte(`["s3:PutObject", "foo"]`), nil, false, true}, // Invalid action. } for i, testCase := range testCases { @@ -146,11 +147,17 @@ func TestActionSetUnmarshalJSON(t *testing.T) { err := json.Unmarshal(testCase.data, &result) expectErr := (err != nil) - if expectErr != testCase.expectErr { - t.Fatalf("case %v: error: expected: %v, got: %v\n", i+1, testCase.expectErr, expectErr) + if expectErr != testCase.expectUnmarshalErr { + t.Fatalf("case %v: error during unmarshal: expected: %v, got: %v\n", i+1, testCase.expectUnmarshalErr, expectErr) } - if !testCase.expectErr { + err = result.Validate() + expectErr = (err != nil) + if expectErr != testCase.expectValidateErr { + t.Fatalf("case %v: error during validation: expected: %v, got: %v\n", i+1, testCase.expectValidateErr, expectErr) + } + + if !testCase.expectUnmarshalErr && !testCase.expectValidateErr { if !reflect.DeepEqual(result, testCase.expectedResult) { t.Fatalf("case %v: result: expected: %v, got: %v\n", i+1, testCase.expectedResult, result) } diff --git a/pkg/iam/policy/policy.go b/pkg/iam/policy/policy.go index e150ca200..4b874ef82 100644 --- a/pkg/iam/policy/policy.go +++ b/pkg/iam/policy/policy.go @@ -103,21 +103,9 @@ func (iamp Policy) isValid() error { return err } } - 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 { @@ -153,30 +141,14 @@ func (iamp *Policy) UnmarshalJSON(data []byte) error { } p := Policy(sp) - if err := p.isValid(); err != nil { - return err - } - p.dropDuplicateStatements() - *iamp = p - return nil } // Validate - validates all statements are for given bucket or not. func (iamp Policy) Validate() error { - if err := iamp.isValid(); err != nil { - return err - } - - for _, statement := range iamp.Statements { - if err := statement.Validate(); err != nil { - return err - } - } - - return nil + return iamp.isValid() } // ParseConfig - parses data in given reader to Iamp. diff --git a/pkg/iam/policy/policy_test.go b/pkg/iam/policy/policy_test.go index a981e5186..b082f0972 100644 --- a/pkg/iam/policy/policy_test.go +++ b/pkg/iam/policy/policy_test.go @@ -387,226 +387,7 @@ func TestPolicyIsValid(t *testing.T) { } } -func TestPolicyMarshalJSON(t *testing.T) { - case1Policy := Policy{ - ID: "MyPolicyForMyBucket1", - Version: DefaultVersion, - Statements: []Statement{ - NewStatement( - policy.Allow, - NewActionSet(PutObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(), - ), - }, - } - case1Policy.Statements[0].SID = "SomeId1" - case1Data := []byte(`{"ID":"MyPolicyForMyBucket1","Version":"2012-10-17","Statement":[{"Sid":"SomeId1","Effect":"Allow","Action":["s3:PutObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"]}]}`) - - _, IPNet1, err := net.ParseCIDR("192.168.1.0/24") - if err != nil { - t.Fatalf("unexpected error. %v\n", err) - } - func1, err := condition.NewIPAddressFunc( - condition.AWSSourceIP, - IPNet1, - ) - if err != nil { - t.Fatalf("unexpected error. %v\n", err) - } - - case2Policy := Policy{ - Version: DefaultVersion, - Statements: []Statement{ - NewStatement( - policy.Allow, - NewActionSet(PutObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(), - ), - NewStatement( - policy.Deny, - NewActionSet(GetObjectAction), - NewResourceSet(NewResource("mybucket", "/yourobject*")), - condition.NewFunctions(func1), - ), - }, - } - case2Data := []byte(`{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:PutObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"]},{"Effect":"Deny","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::mybucket/yourobject*"],"Condition":{"IpAddress":{"aws:SourceIp":["192.168.1.0/24"]}}}]}`) - - case3Policy := Policy{ - ID: "MyPolicyForMyBucket1", - Version: DefaultVersion, - Statements: []Statement{ - NewStatement( - policy.Allow, - NewActionSet(GetObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(), - ), - NewStatement( - policy.Allow, - NewActionSet(PutObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(), - ), - }, - } - case3Data := []byte(`{"ID":"MyPolicyForMyBucket1","Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"]},{"Effect":"Allow","Action":["s3:PutObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"]}]}`) - - case4Policy := Policy{ - ID: "MyPolicyForMyBucket1", - Version: DefaultVersion, - Statements: []Statement{ - NewStatement( - policy.Allow, - NewActionSet(PutObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(), - ), - NewStatement( - policy.Allow, - NewActionSet(GetObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(), - ), - }, - } - case4Data := []byte(`{"ID":"MyPolicyForMyBucket1","Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:PutObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"]},{"Effect":"Allow","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"]}]}`) - - case5Policy := Policy{ - ID: "MyPolicyForMyBucket1", - Version: DefaultVersion, - Statements: []Statement{ - NewStatement( - policy.Allow, - NewActionSet(PutObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(), - ), - NewStatement( - policy.Allow, - NewActionSet(PutObjectAction), - NewResourceSet(NewResource("mybucket", "/yourobject*")), - condition.NewFunctions(), - ), - }, - } - case5Data := []byte(`{"ID":"MyPolicyForMyBucket1","Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:PutObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"]},{"Effect":"Allow","Action":["s3:PutObject"],"Resource":["arn:aws:s3:::mybucket/yourobject*"]}]}`) - - _, IPNet2, err := net.ParseCIDR("192.168.2.0/24") - if err != nil { - t.Fatalf("unexpected error. %v\n", err) - } - func2, err := condition.NewIPAddressFunc( - condition.AWSSourceIP, - IPNet2, - ) - if err != nil { - t.Fatalf("unexpected error. %v\n", err) - } - - case6Policy := Policy{ - ID: "MyPolicyForMyBucket1", - Version: DefaultVersion, - Statements: []Statement{ - NewStatement( - policy.Allow, - NewActionSet(PutObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(func1), - ), - NewStatement( - policy.Allow, - NewActionSet(PutObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(func2), - ), - }, - } - case6Data := []byte(`{"ID":"MyPolicyForMyBucket1","Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:PutObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"],"Condition":{"IpAddress":{"aws:SourceIp":["192.168.1.0/24"]}}},{"Effect":"Allow","Action":["s3:PutObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"],"Condition":{"IpAddress":{"aws:SourceIp":["192.168.2.0/24"]}}}]}`) - - case7Policy := Policy{ - ID: "MyPolicyForMyBucket1", - Version: DefaultVersion, - Statements: []Statement{ - NewStatement( - policy.Allow, - NewActionSet(GetBucketLocationAction), - NewResourceSet(NewResource("mybucket", "")), - condition.NewFunctions(), - ), - }, - } - case7Data := []byte(`{"ID":"MyPolicyForMyBucket1","Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:GetBucketLocation"],"Resource":["arn:aws:s3:::mybucket"]}]}`) - - case8Policy := Policy{ - ID: "MyPolicyForMyBucket1", - Version: DefaultVersion, - Statements: []Statement{ - NewStatement( - policy.Allow, - NewActionSet(GetBucketLocationAction), - NewResourceSet(NewResource("*", "")), - condition.NewFunctions(), - ), - }, - } - case8Data := []byte(`{"ID":"MyPolicyForMyBucket1","Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":["s3:GetBucketLocation"],"Resource":["arn:aws:s3:::*"]}]}`) - - func3, err := condition.NewNullFunc( - condition.S3XAmzCopySource, - true, - ) - if err != nil { - t.Fatalf("unexpected error. %v\n", err) - } - case9Policy := Policy{ - ID: "MyPolicyForMyBucket1", - Version: DefaultVersion, - Statements: []Statement{ - NewStatement( - policy.Allow, - NewActionSet(GetObjectAction, PutObjectAction), - NewResourceSet(NewResource("mybucket", "myobject*")), - condition.NewFunctions(func1, func2, func3), - ), - }, - } - - testCases := []struct { - policy Policy - expectedResult []byte - expectErr bool - }{ - {case1Policy, case1Data, false}, - {case2Policy, case2Data, false}, - {case3Policy, case3Data, false}, - {case4Policy, case4Data, false}, - {case5Policy, case5Data, false}, - {case6Policy, case6Data, false}, - {case7Policy, case7Data, false}, - {case8Policy, case8Data, false}, - {case9Policy, nil, true}, - } - - for i, testCase := range testCases { - result, err := json.Marshal(testCase.policy) - expectErr := (err != nil) - - if expectErr != testCase.expectErr { - t.Fatalf("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, string(testCase.expectedResult), string(result)) - } - } - } -} - -func TestPolicyUnmarshalJSON(t *testing.T) { +func TestPolicyUnmarshalJSONAndValidate(t *testing.T) { case1Data := []byte(`{ "ID": "MyPolicyForMyBucket1", "Version": "2012-10-17", @@ -973,24 +754,25 @@ func TestPolicyUnmarshalJSON(t *testing.T) { } testCases := []struct { - data []byte - expectedResult Policy - expectErr bool + data []byte + expectedResult Policy + expectUnmarshalErr bool + expectValidationErr bool }{ - {case1Data, case1Policy, false}, - {case2Data, case2Policy, false}, - {case3Data, case3Policy, false}, - {case4Data, case4Policy, false}, - {case5Data, case5Policy, false}, - {case6Data, case6Policy, false}, - {case7Data, case7Policy, false}, - {case8Data, case8Policy, false}, + {case1Data, case1Policy, false, false}, + {case2Data, case2Policy, false, false}, + {case3Data, case3Policy, false, false}, + {case4Data, case4Policy, false, false}, + {case5Data, case5Policy, false, false}, + {case6Data, case6Policy, false, false}, + {case7Data, case7Policy, false, false}, + {case8Data, case8Policy, false, false}, // Invalid version error. - {case9Data, Policy{}, true}, + {case9Data, Policy{}, false, true}, // Duplicate statement success, duplicate statement is removed. - {case10Data, case10Policy, false}, + {case10Data, case10Policy, false, false}, // Duplicate statement success (Effect differs). - {case11Data, case11Policy, false}, + {case11Data, case11Policy, false, false}, } for i, testCase := range testCases { @@ -998,11 +780,18 @@ func TestPolicyUnmarshalJSON(t *testing.T) { err := json.Unmarshal(testCase.data, &result) expectErr := (err != nil) - if expectErr != testCase.expectErr { - t.Errorf("case %v: error: expected: %v, got: %v", i+1, testCase.expectErr, expectErr) + if expectErr != testCase.expectUnmarshalErr { + t.Errorf("case %v: error during unmarshal: expected: %v, got: %v", i+1, testCase.expectUnmarshalErr, expectErr) + } + + err = result.Validate() + expectErr = (err != nil) + + if expectErr != testCase.expectValidationErr { + t.Errorf("case %v: error during validation: expected: %v, got: %v", i+1, testCase.expectValidationErr, expectErr) } - if !testCase.expectErr { + if !testCase.expectUnmarshalErr && !testCase.expectValidationErr { if !reflect.DeepEqual(result, testCase.expectedResult) { t.Errorf("case %v: result: expected: %v, got: %v", i+1, testCase.expectedResult, result) } diff --git a/pkg/iam/policy/statement.go b/pkg/iam/policy/statement.go index 2ca0bc3d4..74f8874f3 100644 --- a/pkg/iam/policy/statement.go +++ b/pkg/iam/policy/statement.go @@ -17,7 +17,6 @@ package iampolicy import ( - "encoding/json" "strings" "github.com/minio/minio/pkg/bucket/policy" @@ -63,11 +62,11 @@ func (statement Statement) IsAllowed(args Args) bool { } func (statement Statement) isAdmin() bool { for action := range statement.Actions { - if !AdminAction(action).IsValid() { - return false + if AdminAction(action).IsValid() { + return true } } - return true + return false } // isValid - checks whether statement is valid or not. @@ -81,6 +80,9 @@ func (statement Statement) isValid() error { } if statement.isAdmin() { + if err := statement.Actions.ValidateAdmin(); err != nil { + return err + } for action := range statement.Actions { keys := statement.Conditions.Keys() keyDiff := keys.Difference(adminActionConditionKeyMap[action]) @@ -91,6 +93,10 @@ func (statement Statement) isValid() error { return nil } + if !statement.SID.IsValid() { + return Errorf("invalid SID %v", statement.SID) + } + if len(statement.Resources) == 0 { return Errorf("Resource must not be empty") } @@ -99,6 +105,10 @@ func (statement Statement) isValid() error { return err } + if err := statement.Actions.Validate(); err != nil { + return err + } + for action := range statement.Actions { if !statement.Resources.objectResourceExists() && !statement.Resources.bucketResourceExists() { return Errorf("unsupported Resource found %v for action %v", statement.Resources, action) @@ -114,38 +124,6 @@ func (statement Statement) isValid() error { return nil } -// MarshalJSON - encodes JSON data to Statement. -func (statement Statement) MarshalJSON() ([]byte, error) { - if err := statement.isValid(); err != nil { - return nil, err - } - - // subtype to avoid recursive call to MarshalJSON() - type subStatement Statement - ss := subStatement(statement) - return json.Marshal(ss) -} - -// UnmarshalJSON - decodes JSON data to Statement. -func (statement *Statement) UnmarshalJSON(data []byte) error { - // subtype to avoid recursive call to UnmarshalJSON() - type subStatement Statement - var ss subStatement - - if err := json.Unmarshal(data, &ss); err != nil { - return err - } - - s := Statement(ss) - if err := s.isValid(); err != nil { - return err - } - - *statement = s - - return nil -} - // Validate - validates Statement is for given bucket or not. func (statement Statement) Validate() error { return statement.isValid() diff --git a/pkg/iam/policy/statement_test.go b/pkg/iam/policy/statement_test.go index 71b19eab1..3b290ff73 100644 --- a/pkg/iam/policy/statement_test.go +++ b/pkg/iam/policy/statement_test.go @@ -265,82 +265,7 @@ func TestStatementIsValid(t *testing.T) { } } -func TestStatementMarshalJSON(t *testing.T) { - case1Statement := NewStatement( - policy.Allow, - NewActionSet(PutObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(), - ) - case1Statement.SID = "SomeId1" - case1Data := []byte(`{"Sid":"SomeId1","Effect":"Allow","Action":["s3:PutObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"]}`) - - func1, err := condition.NewNullFunc( - condition.S3XAmzCopySource, - true, - ) - if err != nil { - t.Fatalf("unexpected error. %v\n", err) - } - case2Statement := NewStatement( - policy.Allow, - NewActionSet(PutObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(func1), - ) - case2Data := []byte(`{"Effect":"Allow","Action":["s3:PutObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"],"Condition":{"Null":{"s3:x-amz-copy-source":[true]}}}`) - - func2, err := condition.NewNullFunc( - condition.S3XAmzServerSideEncryption, - false, - ) - if err != nil { - t.Fatalf("unexpected error. %v\n", err) - } - case3Statement := NewStatement( - policy.Deny, - NewActionSet(GetObjectAction), - NewResourceSet(NewResource("mybucket", "/myobject*")), - condition.NewFunctions(func2), - ) - case3Data := []byte(`{"Effect":"Deny","Action":["s3:GetObject"],"Resource":["arn:aws:s3:::mybucket/myobject*"],"Condition":{"Null":{"s3:x-amz-server-side-encryption":[false]}}}`) - - case4Statement := NewStatement( - policy.Allow, - NewActionSet(GetObjectAction, PutObjectAction), - NewResourceSet(NewResource("mybucket", "myobject*")), - condition.NewFunctions(func1, func2), - ) - - testCases := []struct { - statement Statement - expectedResult []byte - expectErr bool - }{ - {case1Statement, case1Data, false}, - {case2Statement, case2Data, false}, - {case3Statement, case3Data, false}, - // Invalid statement error. - {case4Statement, nil, true}, - } - - for i, testCase := range testCases { - result, err := json.Marshal(testCase.statement) - expectErr := (err != nil) - - if expectErr != testCase.expectErr { - t.Fatalf("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, string(testCase.expectedResult), string(result)) - } - } - } -} - -func TestStatementUnmarshalJSON(t *testing.T) { +func TestStatementUnmarshalJSONAndValidate(t *testing.T) { case1Data := []byte(`{ "Sid": "SomeId1", "Effect": "Allow", @@ -408,7 +333,7 @@ func TestStatementUnmarshalJSON(t *testing.T) { case4Data := []byte(`{ "Effect": "Allow", - "Action": "s3:PutObjec", + "Action": "s3:PutObjec, "Resource": "arn:aws:s3:::mybucket/myobject*" }`) @@ -450,36 +375,42 @@ func TestStatementUnmarshalJSON(t *testing.T) { }`) testCases := []struct { - data []byte - expectedResult Statement - expectErr bool + data []byte + expectedResult Statement + expectUnmarshalErr bool + expectValidationErr bool }{ - {case1Data, case1Statement, false}, - {case2Data, case2Statement, false}, - {case3Data, case3Statement, false}, + {case1Data, case1Statement, false, false}, + {case2Data, case2Statement, false, false}, + {case3Data, case3Statement, false, false}, // JSON unmarshaling error. - {case4Data, Statement{}, true}, + {case4Data, Statement{}, true, true}, // Invalid effect error. - {case5Data, Statement{}, true}, + {case5Data, Statement{}, false, true}, // Empty action error. - {case7Data, Statement{}, true}, + {case7Data, Statement{}, false, true}, // Empty resource error. - {case8Data, Statement{}, true}, + {case8Data, Statement{}, false, true}, // Empty condition error. - {case9Data, Statement{}, true}, + {case9Data, Statement{}, true, false}, // Unsupported condition key error. - {case10Data, Statement{}, true}, + {case10Data, Statement{}, false, true}, } for i, testCase := range testCases { var result Statement expectErr := (json.Unmarshal(testCase.data, &result) != nil) - if expectErr != testCase.expectErr { - t.Fatalf("case %v: error: expected: %v, got: %v", i+1, testCase.expectErr, expectErr) + if expectErr != testCase.expectUnmarshalErr { + t.Fatalf("case %v: error during unmarshal: expected: %v, got: %v", i+1, testCase.expectUnmarshalErr, expectErr) + } + + expectErr = (result.Validate() != nil) + if expectErr != testCase.expectValidationErr { + t.Fatalf("case %v: error during validation: expected: %v, got: %v", i+1, testCase.expectValidationErr, expectErr) } - if !testCase.expectErr { + if !testCase.expectUnmarshalErr && !testCase.expectValidationErr { if !reflect.DeepEqual(result, testCase.expectedResult) { t.Fatalf("case %v: result: expected: %v, got: %v", i+1, testCase.expectedResult, result) }