chore: improve the validations of messages (#484)

* chore: improve the validations of messages

* add runtime check

* add runtime check

* add upgrade name

* fix review comments

* refine codes

* refine codes

app/upgrade.go

@@ -90,6 +90,7 @@ func (app *App) registerPampasUpgradeHandler() {
 	app.UpgradeKeeper.SetUpgradeInitializer(upgradetypes.Pampas,
 		func() error {
 			app.Logger().Info("Init Pampas upgrade")
+
 			// enable chain id for opbnb
 			app.CrossChainKeeper.SetDestOpChainID(sdk.ChainID(app.appConfig.CrossChain.DestOpChainId))
 			return nil

e2e/tests/permission_test.go

@@ -251,14 +251,18 @@ func (s *StorageTestSuite) TestCreateObjectByOthers() {
 	s.Require().Equal(verifyPermResp.Effect, types.EFFECT_DENY)
 	s.T().Logf("resp: %s, rep %s", verifyPermReq.String(), verifyPermResp.String())
 
-	// Put bucket policy
-	statement := &types.Statement{
+	// Put object policy
+	statement1 := &types.Statement{
 		Actions: []types.ActionType{types.ACTION_CREATE_OBJECT},
 		Effect:  types.EFFECT_ALLOW,
 	}
+	statement2 := &types.Statement{
+		Actions: []types.ActionType{types.ACTION_UPDATE_OBJECT_INFO},
+		Effect:  types.EFFECT_ALLOW,
+	}
 	principal := types.NewPrincipalWithAccount(user[1].GetAddr())
 	msgPutPolicy := storagetypes.NewMsgPutPolicy(user[0].GetAddr(), types2.NewBucketGRN(bucketName).String(),
-		principal, []*types.Statement{statement}, nil)
+		principal, []*types.Statement{statement1, statement2}, nil)
 	s.SendTxBlock(user[0], msgPutPolicy)
 
 	// verify permission

go.mod

@@ -178,7 +178,7 @@ replace (
 	github.com/cometbft/cometbft => github.com/bnb-chain/greenfield-cometbft v1.0.0
 	github.com/cometbft/cometbft-db => github.com/bnb-chain/greenfield-cometbft-db v0.8.1-alpha.1
 	github.com/confio/ics23/go => github.com/cosmos/cosmos-sdk/ics23/go v0.8.0
-	github.com/cosmos/cosmos-sdk => github.com/bnb-chain/greenfield-cosmos-sdk v0.2.3-alpha.3.0.20231017121821-7b897e5a86e1
+	github.com/cosmos/cosmos-sdk => github.com/forcodedancing/greenfield-cosmos-sdk v0.2.1-0.20231016120649-fcdced9e012e
 	github.com/cosmos/iavl => github.com/bnb-chain/greenfield-iavl v0.20.1
 	github.com/syndtr/goleveldb => github.com/syndtr/goleveldb v1.0.1-0.20210819022825-2ae1ddf74ef7
 	github.com/wercker/journalhook => github.com/wercker/journalhook v0.0.0-20230927020745-64542ffa4117

go.sum

@@ -163,8 +163,6 @@ github.com/bnb-chain/greenfield-cometbft v1.0.0 h1:0r6hOJWD/+es0gxP/exKuN/krgXAr
 github.com/bnb-chain/greenfield-cometbft v1.0.0/go.mod h1:f35mk/r5ab6yvzlqEWZt68LfUje68sYgMpVlt2CUYMk=
 github.com/bnb-chain/greenfield-cometbft-db v0.8.1-alpha.1 h1:XcWulGacHVRiSCx90Q8Y//ajOrLNBQWR/KDB89dy3cU=
 github.com/bnb-chain/greenfield-cometbft-db v0.8.1-alpha.1/go.mod h1:ey1CiK4bYo1RBNJLRiVbYr5CMdSxci9S/AZRINLtppI=
-github.com/bnb-chain/greenfield-cosmos-sdk v0.2.3-alpha.3.0.20231017121821-7b897e5a86e1 h1:Wef+0FD76aE9l3DjE4tGqeRKvEw98z1YkjJ5rHPL6G4=
-github.com/bnb-chain/greenfield-cosmos-sdk v0.2.3-alpha.3.0.20231017121821-7b897e5a86e1/go.mod h1:BGVMW9gRFKGzCwK/8CmDGe3sK9r9QujL1Uz2FMMM+/s=
 github.com/bnb-chain/greenfield-cosmos-sdk/api v0.0.0-20230816082903-b48770f5e210 h1:GHPbV2bC+gmuO6/sG0Tm8oGal3KKSRlyE+zPscDjlA8=
 github.com/bnb-chain/greenfield-cosmos-sdk/api v0.0.0-20230816082903-b48770f5e210/go.mod h1:vhsZxXE9tYJeYB5JR4hPhd6Pc/uPf7j1T8IJ7p9FdeM=
 github.com/bnb-chain/greenfield-cosmos-sdk/math v0.0.0-20230816082903-b48770f5e210 h1:FLVOn4+OVbsKi2+YJX5kmD27/4dRu4FW7xCXFhzDO5s=
@@ -370,6 +368,8 @@ github.com/fjl/memsize v0.0.0-20190710130421-bcb5799ab5e5/go.mod h1:VvhXpOYNQvB+
 github.com/flynn/go-shlex v0.0.0-20150515145356-3f9db97f8568/go.mod h1:xEzjJPgXI435gkrCt3MPfRiAkVrwSbHsst4LCFVfpJc=
 github.com/flynn/noise v1.0.0/go.mod h1:xbMo+0i6+IGbYdJhF31t2eR1BIU0CYc12+BNAKwUTag=
 github.com/fogleman/gg v1.2.1-0.20190220221249-0403632d5b90/go.mod h1:R/bRT+9gY/C5z7JzPU0zXsXHKM4/ayA+zqcVNZzPa1k=
+github.com/forcodedancing/greenfield-cosmos-sdk v0.2.1-0.20231016120649-fcdced9e012e h1:nHk7ex6a2iwYl/L5ZpffJSlWC81+2IVyw5q9S1dsnKU=
+github.com/forcodedancing/greenfield-cosmos-sdk v0.2.1-0.20231016120649-fcdced9e012e/go.mod h1:BGVMW9gRFKGzCwK/8CmDGe3sK9r9QujL1Uz2FMMM+/s=
 github.com/fortytw2/leaktest v1.3.0 h1:u8491cBMTQ8ft8aeV+adlcytMZylmA5nnwwkRZjI8vw=
 github.com/fortytw2/leaktest v1.3.0/go.mod h1:jDsjWgpAGjm2CA7WthBh/CdZYEPF31XHquHwclZch5g=
 github.com/francoispqt/gojay v1.2.13/go.mod h1:ehT5mTG4ua4581f1++1WLG0vPdaA9HaiDsoyrBGkyDY=

x/permission/types/types.go

@@ -5,6 +5,8 @@ import (
 	"time"
 
 	"cosmossdk.io/math"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
 
 	gnfd "github.com/bnb-chain/greenfield/types"
 	"github.com/bnb-chain/greenfield/types/common"
@@ -35,6 +37,20 @@ var (
 
 		ACTION_TYPE_ALL: true,
 	}
+	BucketAllowedActionsAfterPampas = map[ActionType]bool{
+		ACTION_UPDATE_BUCKET_INFO: true,
+		ACTION_DELETE_BUCKET:      true,
+
+		ACTION_CREATE_OBJECT:      true,
+		ACTION_DELETE_OBJECT:      true,
+		ACTION_GET_OBJECT:         true,
+		ACTION_COPY_OBJECT:        true,
+		ACTION_EXECUTE_OBJECT:     true,
+		ACTION_LIST_OBJECT:        true,
+		ACTION_UPDATE_OBJECT_INFO: true,
+
+		ACTION_TYPE_ALL: true,
+	}
 	ObjectAllowedActions = map[ActionType]bool{
 		ACTION_UPDATE_OBJECT_INFO: true,
 		ACTION_CREATE_OBJECT:      true,
@@ -167,26 +183,13 @@ func (s *Statement) ValidateBasic(resType resource.ResourceType) error {
 	case resource.RESOURCE_TYPE_UNSPECIFIED:
 		return ErrInvalidStatement.Wrap("Please specify the ResourceType explicitly. Not allowed set RESOURCE_TYPE_UNSPECIFIED")
 	case resource.RESOURCE_TYPE_BUCKET:
-		containsCreateObject := false
-		for _, a := range s.Actions {
-			if !BucketAllowedActions[a] {
-				return ErrInvalidStatement.Wrapf("%s not allowed to be used on bucket.", a.String())
-			}
-			if a == ACTION_CREATE_OBJECT {
-				containsCreateObject = true
-			}
-		}
 		for _, r := range s.Resources {
 			var grn gnfd.GRN
 			err := grn.ParseFromString(r, true)
 			if err != nil {
 				return ErrInvalidStatement.Wrapf("GRN parse from string failed, err: %s", err)
 			}
 		}
-
-		if !containsCreateObject && s.LimitSize != nil {
-			return ErrInvalidStatement.Wrap("The LimitSize option can only be used with CreateObject actions at the bucket level. .")
-		}
 	case resource.RESOURCE_TYPE_OBJECT:
 		for _, a := range s.Actions {
 			if !ObjectAllowedActions[a] {
@@ -211,30 +214,48 @@ func (s *Statement) ValidateBasic(resType resource.ResourceType) error {
 	return nil
 }
 
-func (s *Statement) ValidateAfterNagqu(resType resource.ResourceType) error {
-	if s.Effect == EFFECT_UNSPECIFIED {
-		return ErrInvalidStatement.Wrap("Please specify the Effect explicitly. Not allowed set EFFECT_UNSPECIFIED")
-	}
-	switch resType {
-	case resource.RESOURCE_TYPE_UNSPECIFIED:
-		return ErrInvalidStatement.Wrap("Please specify the ResourceType explicitly. Not allowed set RESOURCE_TYPE_UNSPECIFIED")
-	case resource.RESOURCE_TYPE_BUCKET:
-		for _, r := range s.Resources {
-			_, err := regexp.Compile(r)
-			if err != nil {
-				return ErrInvalidStatement.Wrapf("The Resources regexp compile failed, err: %s", err)
+func (s *Statement) ValidateRuntime(ctx sdk.Context, resType resource.ResourceType) error {
+	if ctx.IsUpgraded(upgradetypes.Nagqu) {
+		switch resType {
+		case resource.RESOURCE_TYPE_BUCKET:
+			for _, r := range s.Resources {
+				_, err := regexp.Compile(r)
+				if err != nil {
+					return ErrInvalidStatement.Wrapf("The Resources regexp compile failed, err: %s", err)
+				}
+			}
+		case resource.RESOURCE_TYPE_OBJECT:
+			if s.Resources != nil {
+				return ErrInvalidStatement.Wrap("The Resources option can only be used at the bucket level. ")
+			}
+		case resource.RESOURCE_TYPE_GROUP:
+			if s.Resources != nil {
+				return ErrInvalidStatement.Wrap("The Resources option can only be used at the bucket level. ")
 			}
+		default:
+			return ErrInvalidStatement.Wrap("unknown resource type.")
 		}
-	case resource.RESOURCE_TYPE_OBJECT:
-		if s.Resources != nil {
-			return ErrInvalidStatement.Wrap("The Resources option can only be used at the bucket level. ")
+	}
+
+	var bucketAllowedActions map[ActionType]bool
+	if ctx.IsUpgraded(upgradetypes.Pampas) {
+		bucketAllowedActions = BucketAllowedActionsAfterPampas
+	} else {
+		bucketAllowedActions = BucketAllowedActions
+	}
+	if resType == resource.RESOURCE_TYPE_BUCKET {
+		containsCreateObject := false
+		for _, a := range s.Actions {
+			if !bucketAllowedActions[a] {
+				return ErrInvalidStatement.Wrapf("%s not allowed to be used on bucket.", a.String())
+			}
+			if a == ACTION_CREATE_OBJECT {
+				containsCreateObject = true
+			}
 		}
-	case resource.RESOURCE_TYPE_GROUP:
-		if s.Resources != nil {
-			return ErrInvalidStatement.Wrap("The Resources option can only be used at the bucket level. ")
+		if !containsCreateObject && s.LimitSize != nil {
+			return ErrInvalidStatement.Wrap("The LimitSize option can only be used with CreateObject actions at the bucket level. .")
 		}
-	default:
-		return ErrInvalidStatement.Wrap("unknown resource type.")
 	}
 	return nil
 }

x/sp/types/message.go

@@ -105,20 +105,20 @@ func (msg *MsgCreateStorageProvider) ValidateBasic() error {
 	if _, err := sdk.AccAddressFromHexUnsafe(msg.GcAddress); err != nil {
 		return errors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid gc address (%s)", err)
 	}
+	//MaintenanceAddress is validated in msg server
 	if !msg.Deposit.IsValid() || !msg.Deposit.Amount.IsPositive() {
-		return errors.Wrap(sdkerrors.ErrInvalidRequest, "invalid deposit amount")
+		return errors.Wrap(sdkerrors.ErrInvalidCoins, "invalid deposit amount")
 	}
 	if msg.Description == (Description{}) {
 		return errors.Wrap(sdkerrors.ErrInvalidRequest, "empty description")
 	}
 	if err := validateBlsKeyAndProof(msg.BlsKey, msg.BlsProof); err != nil {
 		return err
 	}
-	err := IsValidEndpointURL(msg.Endpoint)
-	if err != nil {
+	if err := ValidateEndpointURL(msg.Endpoint); err != nil {
 		return errors.Wrapf(sdkerrors.ErrInvalidRequest, "invalid endpoint (%s)", err)
 	}
-	if msg.ReadPrice.IsNegative() || msg.StorePrice.IsNegative() {
+	if msg.ReadPrice.IsNil() || msg.ReadPrice.IsNegative() || msg.StorePrice.IsNil() || msg.StorePrice.IsNegative() {
 		return errors.Wrap(sdkerrors.ErrInvalidRequest, "invalid price")
 	}
 	return nil
@@ -177,7 +177,7 @@ func (msg *MsgEditStorageProvider) ValidateBasic() error {
 	}
 
 	if len(msg.Endpoint) != 0 {
-		err = IsValidEndpointURL(msg.Endpoint)
+		err = ValidateEndpointURL(msg.Endpoint)
 		if err != nil {
 			return errors.Wrapf(sdkerrors.ErrInvalidRequest, "invalid endpoint (%s)", err)
 		}
@@ -369,7 +369,7 @@ func (msg *MsgUpdateStorageProviderStatus) ValidateBasic() error {
 		return errors.Wrapf(sdkerrors.ErrInvalidRequest, "not allowed to update to status %s", msg.Status)
 	}
 	if msg.Status == STATUS_IN_MAINTENANCE && msg.Duration <= 0 {
-		return errors.Wrapf(sdkerrors.ErrInvalidRequest, "maintenanceDuration need to be set for %s", msg.Status)
+		return errors.Wrapf(sdkerrors.ErrInvalidRequest, "maintenance duration need to be set for %s", msg.Status)
 	}
 	return nil
 }

x/sp/types/message_test.go

@@ -30,7 +30,7 @@ func TestMsgCreateStorageProvider_ValidateBasic(t *testing.T) {
 	}{
 		{"basic", "a", "b", "c", "d", spAddr, spAddr, spAddr, spAddr, spAddr, spAddr, spAddr, blsPubKey, blsProof, coinPos, nil},
 		{"basic_empty", "a", "b", "c", "d", sdk.AccAddress{}, spAddr, spAddr, spAddr, spAddr, spAddr, spAddr, blsPubKey, blsProof, coinPos, sdkerrors.ErrInvalidAddress},
-		{"zero deposit", "a", "b", "c", "d", spAddr, spAddr, spAddr, spAddr, spAddr, spAddr, spAddr, blsPubKey, blsProof, coinZero, nil},
+		{"zero deposit", "a", "b", "c", "d", spAddr, spAddr, spAddr, spAddr, spAddr, spAddr, spAddr, blsPubKey, blsProof, coinZero, sdkerrors.ErrInvalidCoins},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -48,7 +48,7 @@ func TestMsgCreateStorageProvider_ValidateBasic(t *testing.T) {
 				Endpoint:           "http://127.0.0.1:9033",
 				StorePrice:         sdk.ZeroDec(),
 				ReadPrice:          sdk.ZeroDec(),
-				Deposit:            coinPos,
+				Deposit:            tt.deposit,
 			}
 			err := msg.ValidateBasic()
 			if tt.err != nil {

x/sp/types/util.go

@@ -7,7 +7,7 @@ import (
 )
 
 // Verify if input endpoint URL is valid.
-func IsValidEndpointURL(endpointURL string) error {
+func ValidateEndpointURL(endpointURL string) error {
 	if endpointURL == "" {
 		return errors.Wrap(ErrInvalidEndpointURL, "Endpoint url cannot be empty.")
 	}

x/storage/keeper/msg_server.go

@@ -370,12 +370,6 @@ func (k msgServer) PutPolicy(goCtx context.Context, msg *types.MsgPutPolicy) (*t
 		if s.ExpirationTime != nil && s.ExpirationTime.Before(ctx.BlockTime()) {
 			return nil, permtypes.ErrPermissionExpired.Wrapf("The specified statement expiration time is less than the current block time, block time: %s", ctx.BlockTime().String())
 		}
-		if ctx.IsUpgraded(upgradetypes.Nagqu) {
-			err := s.ValidateAfterNagqu(grn.ResourceType())
-			if err != nil {
-				return nil, err
-			}
-		}
 	}
 
 	policy := &permtypes.Policy{

x/storage/types/message.go

@@ -8,6 +8,7 @@ import (
 	"cosmossdk.io/errors"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
+	upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
 	"github.com/cosmos/gogoproto/proto"
 
 	grn2 "github.com/bnb-chain/greenfield/types"
@@ -970,6 +971,11 @@ func (msg *MsgLeaveGroup) ValidateBasic() error {
 		return errors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid creator address (%s)", err)
 	}
 
+	_, err = sdk.AccAddressFromHexUnsafe(msg.GroupOwner)
+	if err != nil {
+		return errors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid group owner (%s)", err)
+	}
+
 	err = s3util.CheckValidGroupName(msg.GroupName)
 	if err != nil {
 		return err
@@ -1158,6 +1164,10 @@ func (msg *MsgPutPolicy) ValidateBasic() error {
 		return errors.Wrapf(gnfderrors.ErrInvalidGRN, "invalid greenfield resource name (%s)", err)
 	}
 
+	if msg.Principal == nil {
+		return gnfderrors.ErrInvalidPrincipal.Wrapf("principal cannot be empty")
+	}
+
 	if msg.Principal.Type == permtypes.PRINCIPAL_TYPE_GNFD_GROUP && grn.ResourceType() == resource.RESOURCE_TYPE_GROUP {
 		return gnfderrors.ErrInvalidPrincipal.Wrapf("Not allow grant group's permission to another group")
 	}
@@ -1177,6 +1187,18 @@ func (msg *MsgPutPolicy) ValidateBasic() error {
 	return nil
 }
 
+func (msg *MsgPutPolicy) ValidateRuntime(ctx sdk.Context) error {
+	var grn grn2.GRN
+	_ = grn.ParseFromString(msg.Resource, true) // no error after ValidateBasic
+	for _, s := range msg.Statements {
+		err := s.ValidateRuntime(ctx, grn.ResourceType())
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 func NewMsgDeletePolicy(operator sdk.AccAddress, resource string, principal *permtypes.Principal) *MsgDeletePolicy {
 	return &MsgDeletePolicy{
 		Operator:  operator.String(),
@@ -1218,9 +1240,23 @@ func (msg *MsgDeletePolicy) ValidateBasic() error {
 		return errors.Wrapf(gnfderrors.ErrInvalidGRN, "invalid greenfield resource name (%s)", err)
 	}
 
+	if msg.Principal == nil {
+		return gnfderrors.ErrInvalidPrincipal.Wrapf("principal cannot be empty")
+	}
+
 	if msg.Principal.Type == permtypes.PRINCIPAL_TYPE_GNFD_GROUP && grn.ResourceType() == resource.RESOURCE_TYPE_GROUP {
 		return gnfderrors.ErrInvalidPrincipal.Wrapf("Not allow grant group's permission to another group")
 	}
+
+	return nil
+}
+
+func (msg *MsgDeletePolicy) ValidateRuntime(ctx sdk.Context) error {
+	if ctx.IsUpgraded(upgradetypes.Pampas) {
+		if err := msg.Principal.ValidateBasic(); err != nil {
+			return err
+		}
+	}
 	return nil
 }
 
@@ -1266,7 +1302,7 @@ func (msg *MsgMirrorBucket) ValidateBasic() error {
 		return errors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid creator address (%s)", err)
 	}
 
-	if msg.Id.GT(sdk.NewUint(0)) {
+	if !msg.Id.IsNil() && msg.Id.GT(sdk.NewUint(0)) {
 		if msg.BucketName != "" {
 			return errors.Wrap(gnfderrors.ErrInvalidBucketName, "Bucket name should be empty")
 		}
@@ -1324,7 +1360,7 @@ func (msg *MsgMirrorObject) ValidateBasic() error {
 		return errors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid creator address (%s)", err)
 	}
 
-	if msg.Id.GT(sdk.NewUint(0)) {
+	if !msg.Id.IsNil() && msg.Id.GT(sdk.NewUint(0)) {
 		if msg.BucketName != "" {
 			return errors.Wrap(gnfderrors.ErrInvalidBucketName, "Bucket name should be empty")
 		}
@@ -1389,7 +1425,7 @@ func (msg *MsgMirrorGroup) ValidateBasic() error {
 		return errors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid creator address (%s)", err)
 	}
 
-	if msg.Id.GT(sdk.NewUint(0)) {
+	if !msg.Id.IsNil() && msg.Id.GT(sdk.NewUint(0)) {
 		if msg.GroupName != "" {
 			return errors.Wrap(gnfderrors.ErrInvalidGroupName, "Group name should be empty")
 		}

x/virtualgroup/types/message.go

@@ -192,7 +192,7 @@ func (msg *MsgWithdraw) ValidateBasic() error {
 	}
 
 	if !msg.Withdraw.IsValid() || !msg.Withdraw.Amount.IsPositive() {
-		return errors.Wrap(sdkerrors.ErrInvalidRequest, "invalid or non-positive deposit amount")
+		return errors.Wrap(sdkerrors.ErrInvalidRequest, "invalid or non-positive withdraw amount")
 	}
 	return nil
 }