policy: Do not return an error for invalid value during parsing (#9442)

s3:HardwareInfo was removed recently. Users having that admin action
stored in the backend will have an issue starting the server.

To fix this, we need to avoid returning an error in Marshal/Unmarshal
when they encounter an invalid action and validate only in specific
location.

Currently the validation is done and in ParseConfig().

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
-}

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)
-			}
-		}
-	}
-}

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
-}

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)
-			}
-		}
-	}
-}

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...),

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)
-			}
-		}
-	}
-}

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)
+		actionSet.Add(Action(s))
-		if err != nil {
+	}
-			return err
-		}
 
-		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
 }
 

pkg/iam/policy/actionset_test.go

@@ -128,17 +128,18 @@ func TestActionSetToSlice(t *testing.T) {
 
 func TestActionSetUnmarshalJSON(t *testing.T) {
 	testCases := []struct {
-		data           []byte
+		data               []byte
-		expectedResult ActionSet
+		expectedResult     ActionSet
-		expectErr      bool
+		expectUnmarshalErr bool
+		expectValidateErr  bool
 	}{
-		{[]byte(`"s3:PutObject"`), NewActionSet(PutObjectAction), false},
+		{[]byte(`"s3:PutObject"`), NewActionSet(PutObjectAction), false, false},
-		{[]byte(`["s3:PutObject"]`), NewActionSet(PutObjectAction), false},
+		{[]byte(`["s3:PutObject"]`), NewActionSet(PutObjectAction), false, false},
-		{[]byte(`["s3:PutObject", "s3:GetObject"]`), NewActionSet(PutObjectAction, GetObjectAction), false},
+		{[]byte(`["s3:PutObject", "s3:GetObject"]`), NewActionSet(PutObjectAction, GetObjectAction), false, false},
-		{[]byte(`["s3:PutObject", "s3:GetObject", "s3:PutObject"]`), NewActionSet(PutObjectAction, GetObjectAction), false},
+		{[]byte(`["s3:PutObject", "s3:GetObject", "s3:PutObject"]`), NewActionSet(PutObjectAction, GetObjectAction), false, false},
-		{[]byte(`[]`), NewActionSet(), true},           // Empty array.
+		{[]byte(`[]`), NewActionSet(), true, false},           // Empty array.
-		{[]byte(`"foo"`), nil, true},                   // Invalid action.
+		{[]byte(`"foo"`), nil, false, true},                   // Invalid action.
-		{[]byte(`["s3:PutObject", "foo"]`), nil, 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 {
+		if expectErr != testCase.expectUnmarshalErr {
-			t.Fatalf("case %v: error: expected: %v, got: %v\n", i+1, testCase.expectErr, expectErr)
+			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)
 			}

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 iamp.isValid()
-		return err
-	}
-
-	for _, statement := range iamp.Statements {
-		if err := statement.Validate(); err != nil {
-			return err
-		}
-	}
-
-	return nil
 }
 
 // ParseConfig - parses data in given reader to Iamp.

pkg/iam/policy/policy_test.go

@@ -387,226 +387,7 @@ func TestPolicyIsValid(t *testing.T) {
 	}
 }
 
-func TestPolicyMarshalJSON(t *testing.T) {
+func TestPolicyUnmarshalJSONAndValidate(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) {
 	case1Data := []byte(`{
     "ID": "MyPolicyForMyBucket1",
     "Version": "2012-10-17",
@@ -973,24 +754,25 @@ func TestPolicyUnmarshalJSON(t *testing.T) {
 	}
 
 	testCases := []struct {
-		data           []byte
+		data                []byte
-		expectedResult Policy
+		expectedResult      Policy
-		expectErr      bool
+		expectUnmarshalErr  bool
+		expectValidationErr bool
 	}{
-		{case1Data, case1Policy, false},
+		{case1Data, case1Policy, false, false},
-		{case2Data, case2Policy, false},
+		{case2Data, case2Policy, false, false},
-		{case3Data, case3Policy, false},
+		{case3Data, case3Policy, false, false},
-		{case4Data, case4Policy, false},
+		{case4Data, case4Policy, false, false},
-		{case5Data, case5Policy, false},
+		{case5Data, case5Policy, false, false},
-		{case6Data, case6Policy, false},
+		{case6Data, case6Policy, false, false},
-		{case7Data, case7Policy, false},
+		{case7Data, case7Policy, false, false},
-		{case8Data, case8Policy, 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 {
+		if expectErr != testCase.expectUnmarshalErr {
-			t.Errorf("case %v: error: expected: %v, got: %v", i+1, testCase.expectErr, expectErr)
+			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)
 			}

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() {
+		if AdminAction(action).IsValid() {
-			return false
+			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()

pkg/iam/policy/statement_test.go

@@ -265,82 +265,7 @@ func TestStatementIsValid(t *testing.T) {
 	}
 }
 
-func TestStatementMarshalJSON(t *testing.T) {
+func TestStatementUnmarshalJSONAndValidate(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) {
 	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
+		data                []byte
-		expectedResult Statement
+		expectedResult      Statement
-		expectErr      bool
+		expectUnmarshalErr  bool
+		expectValidationErr bool
 	}{
-		{case1Data, case1Statement, false},
+		{case1Data, case1Statement, false, false},
-		{case2Data, case2Statement, false},
+		{case2Data, case2Statement, false, false},
-		{case3Data, case3Statement, 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 {
+		if expectErr != testCase.expectUnmarshalErr {
-			t.Fatalf("case %v: error: expected: %v, got: %v", i+1, testCase.expectErr, expectErr)
+			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)
 			}