From ffc2ee7612d7f425ea1523638047e6eed48ccf4d Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Sat, 8 Apr 2023 00:14:20 +0200 Subject: [PATCH 1/6] refactor(authz): move `ValidateBasic` logic to `msgServer` --- x/authz/keeper/msg_server.go | 65 +++++++++++++--- x/authz/msgs.go | 67 ----------------- x/authz/msgs_test.go | 126 -------------------------------- x/feegrant/keeper/msg_server.go | 3 +- 4 files changed, 55 insertions(+), 206 deletions(-) diff --git a/x/authz/keeper/msg_server.go b/x/authz/keeper/msg_server.go index b8bc5b555267..f48cea7575ec 100644 --- a/x/authz/keeper/msg_server.go +++ b/x/authz/keeper/msg_server.go @@ -3,6 +3,7 @@ package keeper import ( "context" "errors" + "strings" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" @@ -13,24 +14,32 @@ var _ authz.MsgServer = Keeper{} // Grant implements the MsgServer.Grant method to create a new grant. func (k Keeper) Grant(goCtx context.Context, msg *authz.MsgGrant) (*authz.MsgGrantResponse, error) { - ctx := sdk.UnwrapSDKContext(goCtx) + if strings.EqualFold(msg.Grantee, msg.Granter) { + return nil, authz.ErrGranteeIsGranter + } + grantee, err := k.authKeeper.StringToBytes(msg.Grantee) if err != nil { + return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid grantee address: %s", err) + } + + granter, err := k.authKeeper.StringToBytes(msg.Granter) + if err != nil { + return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid grantee address: %s", err) + } + + if err := msg.Grant.ValidateBasic(); err != nil { return nil, err } // create the account if it is not in account state + ctx := sdk.UnwrapSDKContext(goCtx) granteeAcc := k.authKeeper.GetAccount(ctx, grantee) if granteeAcc == nil { granteeAcc = k.authKeeper.NewAccountWithAddress(ctx, grantee) k.authKeeper.SetAccount(ctx, granteeAcc) } - granter, err := k.authKeeper.StringToBytes(msg.Granter) - if err != nil { - return nil, err - } - authorization, err := msg.GetAuthorization() if err != nil { return nil, err @@ -51,18 +60,26 @@ func (k Keeper) Grant(goCtx context.Context, msg *authz.MsgGrant) (*authz.MsgGra // Revoke implements the MsgServer.Revoke method. func (k Keeper) Revoke(goCtx context.Context, msg *authz.MsgRevoke) (*authz.MsgRevokeResponse, error) { - ctx := sdk.UnwrapSDKContext(goCtx) + if strings.EqualFold(msg.Grantee, msg.Granter) { + return nil, authz.ErrGranteeIsGranter + } + grantee, err := k.authKeeper.StringToBytes(msg.Grantee) if err != nil { - return nil, err + return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid grantee address: %s", err) } + granter, err := k.authKeeper.StringToBytes(msg.Granter) if err != nil { - return nil, err + return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid granter address: %s", err) } - err = k.DeleteGrant(ctx, grantee, granter, msg.MsgTypeUrl) - if err != nil { + if msg.MsgTypeUrl == "" { + return nil, sdkerrors.ErrInvalidRequest.Wrap("missing method name") + } + + ctx := sdk.UnwrapSDKContext(goCtx) + if err = k.DeleteGrant(ctx, grantee, granter, msg.MsgTypeUrl); err != nil { return nil, err } @@ -75,9 +92,14 @@ func (k Keeper) Exec(goCtx context.Context, msg *authz.MsgExec) (*authz.MsgExecR if msg.Grantee == "" { return nil, errors.New("empty address string is not allowed") } + grantee, err := k.authKeeper.StringToBytes(msg.Grantee) if err != nil { - return nil, err + return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid grantee address: %s", err) + } + + if len(msg.Msgs) == 0 { + return nil, sdkerrors.ErrInvalidRequest.Wrapf("messages cannot be empty") } msgs, err := msg.GetMessages() @@ -85,6 +107,10 @@ func (k Keeper) Exec(goCtx context.Context, msg *authz.MsgExec) (*authz.MsgExecR return nil, err } + if err := validateMsgs(msgs); err != nil { + return nil, err + } + results, err := k.DispatchActions(ctx, grantee, msgs) if err != nil { return nil, err @@ -92,3 +118,18 @@ func (k Keeper) Exec(goCtx context.Context, msg *authz.MsgExec) (*authz.MsgExecR return &authz.MsgExecResponse{Results: results}, nil } + +func validateMsgs(msgs []sdk.Msg) error { + for _, msg := range msgs { + m, ok := msg.(sdk.HasValidateBasic) + if !ok { + continue + } + + if err := m.ValidateBasic(); err != nil { + return err + } + } + + return nil +} diff --git a/x/authz/msgs.go b/x/authz/msgs.go index ecd5865dcab1..306a1762a04a 100644 --- a/x/authz/msgs.go +++ b/x/authz/msgs.go @@ -47,23 +47,6 @@ func (msg MsgGrant) GetSigners() []sdk.AccAddress { return []sdk.AccAddress{granter} } -// ValidateBasic implements Msg -func (msg MsgGrant) ValidateBasic() error { - granter, err := sdk.AccAddressFromBech32(msg.Granter) - if err != nil { - return sdkerrors.ErrInvalidAddress.Wrapf("invalid granter address: %s", err) - } - grantee, err := sdk.AccAddressFromBech32(msg.Grantee) - if err != nil { - return sdkerrors.ErrInvalidAddress.Wrapf("invalid grantee address: %s", err) - } - - if granter.Equals(grantee) { - return ErrGranteeIsGranter - } - return msg.Grant.ValidateBasic() -} - // GetSignBytes implements the LegacyMsg.GetSignBytes method. func (msg MsgGrant) GetSignBytes() []byte { return sdk.MustSortJSON(authzcodec.Amino.MustMarshalJSON(&msg)) @@ -121,28 +104,6 @@ func (msg MsgRevoke) GetSigners() []sdk.AccAddress { return []sdk.AccAddress{granter} } -// ValidateBasic implements MsgRequest.ValidateBasic -func (msg MsgRevoke) ValidateBasic() error { - granter, err := sdk.AccAddressFromBech32(msg.Granter) - if err != nil { - return sdkerrors.ErrInvalidAddress.Wrapf("invalid granter address: %s", err) - } - grantee, err := sdk.AccAddressFromBech32(msg.Grantee) - if err != nil { - return sdkerrors.ErrInvalidAddress.Wrapf("invalid grantee address: %s", err) - } - - if granter.Equals(grantee) { - return ErrGranteeIsGranter - } - - if msg.MsgTypeUrl == "" { - return sdkerrors.ErrInvalidRequest.Wrap("missing method name") - } - - return nil -} - // GetSignBytes implements the LegacyMsg.GetSignBytes method. func (msg MsgRevoke) GetSignBytes() []byte { return sdk.MustSortJSON(authzcodec.Amino.MustMarshalJSON(&msg)) @@ -186,34 +147,6 @@ func (msg MsgExec) GetSigners() []sdk.AccAddress { return []sdk.AccAddress{grantee} } -// ValidateBasic implements Msg -func (msg MsgExec) ValidateBasic() error { - if _, err := sdk.AccAddressFromBech32(msg.Grantee); err != nil { - return sdkerrors.ErrInvalidAddress.Wrapf("invalid grantee address: %s", err) - } - - if len(msg.Msgs) == 0 { - return sdkerrors.ErrInvalidRequest.Wrapf("messages cannot be empty") - } - - msgs, err := msg.GetMessages() - if err != nil { - return err - } - for _, msg := range msgs { - m, ok := msg.(sdk.HasValidateBasic) - if !ok { - continue - } - - if err = m.ValidateBasic(); err != nil { - return err - } - } - - return nil -} - // GetSignBytes implements the LegacyMsg.GetSignBytes method. func (msg MsgExec) GetSignBytes() []byte { return sdk.MustSortJSON(authzcodec.Amino.MustMarshalJSON(&msg)) diff --git a/x/authz/msgs_test.go b/x/authz/msgs_test.go index 2907098a9a87..6514dad55d3e 100644 --- a/x/authz/msgs_test.go +++ b/x/authz/msgs_test.go @@ -16,132 +16,6 @@ import ( banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" ) -var ( - coinsPos = sdk.NewCoins(sdk.NewInt64Coin("steak", 100)) - granter = sdk.AccAddress("_______granter______") - grantee = sdk.AccAddress("_______grantee______") -) - -func TestMsgExecAuthorized(t *testing.T) { - tests := []struct { - title string - grantee sdk.AccAddress - msgs []sdk.Msg - expectPass bool - }{ - {"nil grantee address", nil, []sdk.Msg{}, false}, - {"zero-messages test: should fail", grantee, []sdk.Msg{}, false}, - {"invalid nested msg", grantee, []sdk.Msg{ - &banktypes.MsgSend{ - Amount: sdk.NewCoins(sdk.NewInt64Coin("steak", 2)), - FromAddress: "invalid_from_address", - ToAddress: grantee.String(), - }, - }, false}, - {"valid test: msg type", grantee, []sdk.Msg{ - &banktypes.MsgSend{ - Amount: sdk.NewCoins(sdk.NewInt64Coin("steak", 2)), - FromAddress: granter.String(), - ToAddress: grantee.String(), - }, - }, true}, - } - for i, tc := range tests { - msg := authz.NewMsgExec(tc.grantee, tc.msgs) - if tc.expectPass { - require.NoError(t, msg.ValidateBasic(), "test: %v", i) - } else { - require.Error(t, msg.ValidateBasic(), "test: %v", i) - } - } -} - -func TestMsgRevokeAuthorization(t *testing.T) { - tests := []struct { - title string - granter, grantee sdk.AccAddress - msgType string - expectPass bool - }{ - {"nil Granter address", nil, grantee, "hello", false}, - {"nil Grantee address", granter, nil, "hello", false}, - {"nil Granter and Grantee address", nil, nil, "hello", false}, - {"valid test case", granter, grantee, "hello", true}, - } - for i, tc := range tests { - msg := authz.NewMsgRevoke(tc.granter, tc.grantee, tc.msgType) - if tc.expectPass { - require.NoError(t, msg.ValidateBasic(), "test: %v", i) - } else { - require.Error(t, msg.ValidateBasic(), "test: %v", i) - } - } -} - -// add time interval to a time object and returns a pointer -func addDatePtr(t *time.Time, months, days int) *time.Time { - t2 := t.AddDate(0, months, days) - return &t2 -} - -func TestMsgGrantAuthorization(t *testing.T) { - now := time.Now() - tests := []struct { - name string - granter, grantee sdk.AccAddress - authorization authz.Authorization - expiration *time.Time - expectErr bool - valBasic bool - }{ - { - "nil granter address", - nil, grantee, &banktypes.SendAuthorization{SpendLimit: coinsPos}, &now, false, false, - }, - { - "nil grantee address", - granter, nil, &banktypes.SendAuthorization{SpendLimit: coinsPos}, &now, false, false, - }, - { - "nil granter and grantee address", - nil, nil, &banktypes.SendAuthorization{SpendLimit: coinsPos}, &now, false, false, - }, - { - "nil authorization should fail", - granter, grantee, nil, &now, true, false, - }, - { - "valid test case", - granter, grantee, &banktypes.SendAuthorization{SpendLimit: coinsPos}, addDatePtr(&now, 1, 0), false, true, - }, - { - "valid test case with nil expire time", - granter, grantee, &banktypes.SendAuthorization{SpendLimit: coinsPos}, nil, false, true, - }, - // we don't access the block time / nor time.Now, so we don't know if it's in the past at this level. - { - "past expire time should not fail", - granter, grantee, &banktypes.SendAuthorization{SpendLimit: coinsPos}, addDatePtr(&now, 0, -1), false, true, - }, - } - for _, tc := range tests { - msg, err := authz.NewMsgGrant( - tc.granter, tc.grantee, tc.authorization, tc.expiration, - ) - if !tc.expectErr { - require.NoError(t, err, "test: %v", tc.name) - } else { - require.Error(t, err, "test: %v", tc.name) - continue - } - if tc.valBasic { - require.NoError(t, msg.ValidateBasic(), "test: %v", tc.name) - } else { - require.Error(t, msg.ValidateBasic(), "test: %v", tc.name) - } - } -} - func TestMsgGrantGetAuthorization(t *testing.T) { require := require.New(t) diff --git a/x/feegrant/keeper/msg_server.go b/x/feegrant/keeper/msg_server.go index e6d735602e11..49428d33ecfb 100644 --- a/x/feegrant/keeper/msg_server.go +++ b/x/feegrant/keeper/msg_server.go @@ -2,6 +2,7 @@ package keeper import ( "context" + "strings" errorsmod "cosmossdk.io/errors" sdk "github.com/cosmos/cosmos-sdk/types" @@ -26,7 +27,7 @@ var _ feegrant.MsgServer = msgServer{} // GrantAllowance grants an allowance from the granter's funds to be used by the grantee. func (k msgServer) GrantAllowance(goCtx context.Context, msg *feegrant.MsgGrantAllowance) (*feegrant.MsgGrantAllowanceResponse, error) { - if msg.Grantee == msg.Granter { + if strings.EqualFold(msg.Granter, msg.Granter) { return nil, errorsmod.Wrap(sdkerrors.ErrInvalidAddress, "cannot self-grant fee authorization") } From 3c42e333e1649f7b881b8462d58f34344add50c8 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Sat, 8 Apr 2023 00:15:37 +0200 Subject: [PATCH 2/6] typo --- x/feegrant/keeper/msg_server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/feegrant/keeper/msg_server.go b/x/feegrant/keeper/msg_server.go index 49428d33ecfb..0c6023a1b303 100644 --- a/x/feegrant/keeper/msg_server.go +++ b/x/feegrant/keeper/msg_server.go @@ -27,7 +27,7 @@ var _ feegrant.MsgServer = msgServer{} // GrantAllowance grants an allowance from the granter's funds to be used by the grantee. func (k msgServer) GrantAllowance(goCtx context.Context, msg *feegrant.MsgGrantAllowance) (*feegrant.MsgGrantAllowanceResponse, error) { - if strings.EqualFold(msg.Granter, msg.Granter) { + if strings.EqualFold(msg.Grantee, msg.Granter) { return nil, errorsmod.Wrap(sdkerrors.ErrInvalidAddress, "cannot self-grant fee authorization") } From d322ba322ba98f7507de97ae8f79d159c8bd7d21 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Sat, 8 Apr 2023 00:49:23 +0200 Subject: [PATCH 3/6] test parity --- x/authz/authorization_grant.go | 4 ++ x/authz/keeper/msg_server.go | 2 +- x/authz/keeper/msg_server_test.go | 105 +++++++++++++++++++++++++++++- 3 files changed, 109 insertions(+), 2 deletions(-) diff --git a/x/authz/authorization_grant.go b/x/authz/authorization_grant.go index edf74f0f6c42..bc331885c930 100644 --- a/x/authz/authorization_grant.go +++ b/x/authz/authorization_grant.go @@ -53,6 +53,10 @@ func (g Grant) GetAuthorization() (Authorization, error) { } func (g Grant) ValidateBasic() error { + if g.Authorization == nil { + return sdkerrors.ErrInvalidType.Wrap("authorization is nil") + } + av := g.Authorization.GetCachedValue() a, ok := av.(Authorization) if !ok { diff --git a/x/authz/keeper/msg_server.go b/x/authz/keeper/msg_server.go index f48cea7575ec..6834d5d8b557 100644 --- a/x/authz/keeper/msg_server.go +++ b/x/authz/keeper/msg_server.go @@ -75,7 +75,7 @@ func (k Keeper) Revoke(goCtx context.Context, msg *authz.MsgRevoke) (*authz.MsgR } if msg.MsgTypeUrl == "" { - return nil, sdkerrors.ErrInvalidRequest.Wrap("missing method name") + return nil, sdkerrors.ErrInvalidRequest.Wrap("missing msg method name") } ctx := sdk.UnwrapSDKContext(goCtx) diff --git a/x/authz/keeper/msg_server_test.go b/x/authz/keeper/msg_server_test.go index 9030aacabf40..aa7c0632f5a0 100644 --- a/x/authz/keeper/msg_server_test.go +++ b/x/authz/keeper/msg_server_test.go @@ -41,6 +41,20 @@ func (suite *TestSuite) TestGrant() { expErr bool errMsg string }{ + { + name: "indentical grantee and granter", + malleate: func() *authz.MsgGrant { + grant, err := authz.NewGrant(curBlockTime, banktypes.NewSendAuthorization(coins, nil), &oneYear) + suite.Require().NoError(err) + return &authz.MsgGrant{ + Granter: grantee.String(), + Grantee: grantee.String(), + Grant: grant, + } + }, + expErr: true, + errMsg: "grantee and granter should be different", + }, { name: "invalid granter", malleate: func() *authz.MsgGrant { @@ -69,6 +83,38 @@ func (suite *TestSuite) TestGrant() { expErr: true, errMsg: "invalid bech32 string", }, + { + name: "invalid grant", + malleate: func() *authz.MsgGrant { + return &authz.MsgGrant{ + Granter: granter.String(), + Grantee: grantee.String(), + Grant: authz.Grant{ + Expiration: &oneYear, + }, + } + }, + expErr: true, + errMsg: "authorization is nil: invalid type", + }, + { + name: "invalid grant, past time", + malleate: func() *authz.MsgGrant { + pastTime := curBlockTime.Add(-time.Hour) + grant, err := authz.NewGrant(curBlockTime, banktypes.NewSendAuthorization(coins, nil), &oneHour) // we only need the authorization + suite.Require().NoError(err) + return &authz.MsgGrant{ + Granter: granter.String(), + Grantee: grantee.String(), + Grant: authz.Grant{ + Authorization: grant.Authorization, + Expiration: &pastTime, + }, + } + }, + expErr: true, + errMsg: "expiration must be after the current block time", + }, { name: "grantee account does not exist on chain: valid grant", malleate: func() *authz.MsgGrant { @@ -132,6 +178,18 @@ func (suite *TestSuite) TestGrant() { } }, }, + { + name: "valid grant with nil expiration time", + malleate: func() *authz.MsgGrant { + grant, err := authz.NewGrant(curBlockTime, banktypes.NewSendAuthorization(coins, nil), nil) + suite.Require().NoError(err) + return &authz.MsgGrant{ + Granter: granter.String(), + Grantee: grantee.String(), + Grant: grant, + } + }, + }, } for _, tc := range testCases { @@ -158,6 +216,18 @@ func (suite *TestSuite) TestRevoke() { expErr bool errMsg string }{ + { + name: "indentical grantee and granter", + malleate: func() *authz.MsgRevoke { + return &authz.MsgRevoke{ + Granter: grantee.String(), + Grantee: grantee.String(), + MsgTypeUrl: bankSendAuthMsgType, + } + }, + expErr: true, + errMsg: "grantee and granter should be different", + }, { name: "invalid granter", malleate: func() *authz.MsgRevoke { @@ -182,6 +252,18 @@ func (suite *TestSuite) TestRevoke() { expErr: true, errMsg: "invalid bech32 string", }, + { + name: "no msg given", + malleate: func() *authz.MsgRevoke { + return &authz.MsgRevoke{ + Granter: granter.String(), + Grantee: grantee.String(), + MsgTypeUrl: "", + } + }, + expErr: true, + errMsg: "missing msg method name", + }, { name: "valid grant", malleate: func() *authz.MsgRevoke { @@ -240,7 +322,7 @@ func (suite *TestSuite) TestExec() { errMsg string }{ { - name: "invalid grantee", + name: "invalid grantee (empty)", malleate: func() authz.MsgExec { return authz.NewMsgExec(sdk.AccAddress{}, []sdk.Msg{msg}) }, @@ -255,6 +337,14 @@ func (suite *TestSuite) TestExec() { expErr: true, errMsg: "authorization not found", }, + { + name: "no message case", + malleate: func() authz.MsgExec { + return authz.NewMsgExec(grantee, []sdk.Msg{}) + }, + expErr: true, + errMsg: "messages cannot be empty", + }, { name: "valid case", malleate: func() authz.MsgExec { @@ -262,6 +352,19 @@ func (suite *TestSuite) TestExec() { return authz.NewMsgExec(grantee, []sdk.Msg{msg}) }, }, + { + name: "invalid nested msg", + malleate: func() authz.MsgExec { + return authz.NewMsgExec(grantee, []sdk.Msg{ + &banktypes.MsgSend{ + Amount: sdk.NewCoins(sdk.NewInt64Coin("steak", 2)), + FromAddress: "invalid_from_address", + ToAddress: grantee.String(), + }}) + }, + expErr: true, + errMsg: "invalid from address", + }, } for _, tc := range testCases { From 62108248904f02aeef5f0ac2725fc05e5908e69f Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Sat, 8 Apr 2023 01:17:41 +0200 Subject: [PATCH 4/6] remove duplicate tests and add checks in CLI --- tests/e2e/authz/tx.go | 360 --------------------------------------- x/authz/client/cli/tx.go | 13 ++ 2 files changed, 13 insertions(+), 360 deletions(-) diff --git a/tests/e2e/authz/tx.go b/tests/e2e/authz/tx.go index c346eefdb442..cd39ce0809d4 100644 --- a/tests/e2e/authz/tx.go +++ b/tests/e2e/authz/tx.go @@ -167,366 +167,6 @@ var ( typeMsgSubmitProposal = sdk.MsgTypeURL(&govv1.MsgSubmitProposal{}) ) -func (s *E2ETestSuite) TestCLITxGrantAuthorization() { - val := s.network.Validators[0] - grantee := s.grantee[0] - - twoHours := time.Now().Add(time.Minute * 120).Unix() - pastHour := time.Now().Add(-time.Minute * 60).Unix() - - testCases := []struct { - name string - args []string - expectedCode uint32 - expectErr bool - expErrMsg string - }{ - { - "Invalid granter Address", - []string{ - "grantee_addr", - "send", - fmt.Sprintf("--%s=100stake", cli.FlagSpendLimit), - fmt.Sprintf("--%s=%s", flags.FlagFrom, "granter"), - fmt.Sprintf("--%s=true", flags.FlagGenerateOnly), - fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), - }, - 0, - true, - "key not found", - }, - { - "Invalid grantee Address", - []string{ - "grantee_addr", - "send", - fmt.Sprintf("--%s=100stake", cli.FlagSpendLimit), - fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), - fmt.Sprintf("--%s=true", flags.FlagGenerateOnly), - fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), - }, - 0, - true, - "invalid separator index", - }, - { - "Invalid expiration time", - []string{ - grantee.String(), - "send", - fmt.Sprintf("--%s=100stake", cli.FlagSpendLimit), - fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), - fmt.Sprintf("--%s=true", flags.FlagBroadcastMode), - fmt.Sprintf("--%s=%d", cli.FlagExpiration, pastHour), - }, - 0, - true, - "", - }, - { - "fail with error invalid msg-type", - []string{ - grantee.String(), - "generic", - fmt.Sprintf("--%s=invalid-msg-type", cli.FlagMsgType), - fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), - fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync), - fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), - fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), - fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), - }, - 0x1d, - false, - "", - }, - { - "failed with error both validators not allowed", - []string{ - grantee.String(), - "delegate", - fmt.Sprintf("--%s=100stake", cli.FlagSpendLimit), - fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), - fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), - fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync), - fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), - fmt.Sprintf("--%s=%s", cli.FlagAllowedValidators, val.ValAddress.String()), - fmt.Sprintf("--%s=%s", cli.FlagDenyValidators, val.ValAddress.String()), - fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), - }, - 0, - true, - "cannot set both allowed & deny list", - }, - { - "invalid bond denom for tx delegate authorization allowed validators", - []string{ - grantee.String(), - "delegate", - fmt.Sprintf("--%s=100xyz", cli.FlagSpendLimit), - fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), - fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), - fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync), - fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), - fmt.Sprintf("--%s=%s", cli.FlagAllowedValidators, val.ValAddress.String()), - fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), - }, - 0, - true, - "invalid denom", - }, - { - "invalid bond denom for tx delegate authorization deny validators", - []string{ - grantee.String(), - "delegate", - fmt.Sprintf("--%s=100xyz", cli.FlagSpendLimit), - fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), - fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), - fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync), - fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), - fmt.Sprintf("--%s=%s", cli.FlagDenyValidators, val.ValAddress.String()), - fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), - }, - 0, - true, - "invalid denom", - }, - { - "invalid bond denom for tx undelegate authorization", - []string{ - grantee.String(), - "unbond", - fmt.Sprintf("--%s=100xyz", cli.FlagSpendLimit), - fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), - fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), - fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync), - fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), - fmt.Sprintf("--%s=%s", cli.FlagAllowedValidators, val.ValAddress.String()), - fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), - }, - 0, - true, - "invalid denom", - }, - { - "invalid bond denon for tx redelegate authorization", - []string{ - grantee.String(), - "redelegate", - fmt.Sprintf("--%s=100xyz", cli.FlagSpendLimit), - fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), - fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), - fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync), - fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), - fmt.Sprintf("--%s=%s", cli.FlagAllowedValidators, val.ValAddress.String()), - fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), - }, - 0, - true, - "invalid denom", - }, - { - "invalid decimal coin expression with more than single coin", - []string{ - grantee.String(), - "delegate", - fmt.Sprintf("--%s=100stake,20xyz", cli.FlagSpendLimit), - fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), - fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), - fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync), - fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), - fmt.Sprintf("--%s=%s", cli.FlagAllowedValidators, val.ValAddress.String()), - fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), - }, - 0, - true, - "invalid decimal coin expression", - }, - { - "valid tx delegate authorization allowed validators", - []string{ - grantee.String(), - "delegate", - fmt.Sprintf("--%s=100stake", cli.FlagSpendLimit), - fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), - fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), - fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync), - fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), - fmt.Sprintf("--%s=%s", cli.FlagAllowedValidators, val.ValAddress.String()), - fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), - }, - 0, - false, - "", - }, - { - "valid tx delegate authorization deny validators", - []string{ - grantee.String(), - "delegate", - fmt.Sprintf("--%s=100stake", cli.FlagSpendLimit), - fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), - fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), - fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync), - fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), - fmt.Sprintf("--%s=%s", cli.FlagDenyValidators, val.ValAddress.String()), - fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), - }, - 0, - false, - "", - }, - { - "valid tx undelegate authorization", - []string{ - grantee.String(), - "unbond", - fmt.Sprintf("--%s=100stake", cli.FlagSpendLimit), - fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), - fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), - fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync), - fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), - fmt.Sprintf("--%s=%s", cli.FlagAllowedValidators, val.ValAddress.String()), - fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), - }, - 0, - false, - "", - }, - { - "valid tx redelegate authorization", - []string{ - grantee.String(), - "redelegate", - fmt.Sprintf("--%s=100stake", cli.FlagSpendLimit), - fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), - fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), - fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync), - fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), - fmt.Sprintf("--%s=%s", cli.FlagAllowedValidators, val.ValAddress.String()), - fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), - }, - 0, - false, - "", - }, - { - "Valid tx send authorization", - []string{ - grantee.String(), - "send", - fmt.Sprintf("--%s=100stake", cli.FlagSpendLimit), - fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), - fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync), - fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), - fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), - fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), - }, - 0, - false, - "", - }, - { - "Valid tx send authorization with allow list", - []string{ - grantee.String(), - "send", - fmt.Sprintf("--%s=100stake", cli.FlagSpendLimit), - fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), - fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync), - fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), - fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), - fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), - fmt.Sprintf("--%s=%s", cli.FlagAllowList, s.grantee[1]), - }, - 0, - false, - "", - }, - { - "Invalid tx send authorization with duplicate allow list", - []string{ - grantee.String(), - "send", - fmt.Sprintf("--%s=100stake", cli.FlagSpendLimit), - fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), - fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync), - fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), - fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), - fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), - fmt.Sprintf("--%s=%s", cli.FlagAllowList, fmt.Sprintf("%s,%s", s.grantee[1], s.grantee[1])), - }, - 0, - true, - "duplicate entry", - }, - { - "Valid tx generic authorization", - []string{ - grantee.String(), - "generic", - fmt.Sprintf("--%s=%s", cli.FlagMsgType, typeMsgVote), - fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), - fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync), - fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), - fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), - fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), - }, - 0, - false, - "", - }, - { - "fail when granter = grantee", - []string{ - grantee.String(), - "generic", - fmt.Sprintf("--%s=%s", cli.FlagMsgType, typeMsgVote), - fmt.Sprintf("--%s=%s", flags.FlagFrom, grantee.String()), - fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync), - fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), - fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), - fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), - }, - 0, - true, - "grantee and granter should be different", - }, - { - "Valid tx with amino", - []string{ - grantee.String(), - "generic", - fmt.Sprintf("--%s=%s", cli.FlagMsgType, typeMsgVote), - fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()), - fmt.Sprintf("--%s=%s", flags.FlagBroadcastMode, flags.BroadcastSync), - fmt.Sprintf("--%s=%d", cli.FlagExpiration, twoHours), - fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), - fmt.Sprintf("--%s=%s", flags.FlagFees, sdk.NewCoins(sdk.NewCoin(s.cfg.BondDenom, sdk.NewInt(10))).String()), - fmt.Sprintf("--%s=%s", flags.FlagSignMode, flags.SignModeLegacyAminoJSON), - }, - 0, - false, - "", - }, - } - - for _, tc := range testCases { - s.Run(tc.name, func() { - out, err := authzclitestutil.CreateGrant(val.ClientCtx, tc.args) - if tc.expectErr { - s.Require().Error(err, out) - s.Require().Contains(err.Error(), tc.expErrMsg) - } else { - var txResp sdk.TxResponse - s.Require().NoError(err) - s.Require().NoError(val.ClientCtx.Codec.UnmarshalJSON(out.Bytes(), &txResp), out.String()) - s.Require().NoError(clitestutil.CheckTxCode(s.network, val.ClientCtx, txResp.TxHash, tc.expectedCode)) - } - }) - } -} - func execDelegate(val *network.Validator, args []string) (testutil.BufferWriter, error) { cmd := stakingcli.NewDelegateCmd() clientCtx := val.ClientCtx diff --git a/x/authz/client/cli/tx.go b/x/authz/client/cli/tx.go index d017d6053016..5f3e5a491aeb 100644 --- a/x/authz/client/cli/tx.go +++ b/x/authz/client/cli/tx.go @@ -73,6 +73,10 @@ Examples: return err } + if strings.EqualFold(args[0], clientCtx.GetFromAddress().String()) { + return errors.New("grantee and granter should be different") + } + grantee, err := ac.StringToBytes(args[0]) if err != nil { return err @@ -100,6 +104,15 @@ Examples: return err } + // check for duplicates + for i := 0; i < len(allowList); i++ { + for j := i + 1; j < len(allowList); j++ { + if allowList[i] == allowList[j] { + return fmt.Errorf("duplicate address %s in allow-list", allowList[i]) + } + } + } + allowed, err := bech32toAccAddresses(allowList, ac) if err != nil { return err From de07033493127706d292a6f4c0bc0cd99c0583f5 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Sat, 8 Apr 2023 01:18:28 +0200 Subject: [PATCH 5/6] updates --- x/authz/client/cli/tx_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/authz/client/cli/tx_test.go b/x/authz/client/cli/tx_test.go index fb738a78fe8c..1d4c5eff1a6d 100644 --- a/x/authz/client/cli/tx_test.go +++ b/x/authz/client/cli/tx_test.go @@ -375,7 +375,7 @@ func (s *CLITestSuite) TestCLITxGrantAuthorization() { fmt.Sprintf("--%s=%s", cli.FlagAllowList, fmt.Sprintf("%s,%s", s.grantee[1], s.grantee[1])), }, true, - "duplicate entry", + "duplicate address", }, { "Valid tx generic authorization", From b965c9d5f90c96afa107eb249d4e0608ed1cf465 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Sat, 8 Apr 2023 13:13:25 +0200 Subject: [PATCH 6/6] fix e2e tests (as we've deleted tests, there is less now) --- tests/e2e/authz/grpc.go | 2 +- tests/e2e/authz/query.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/e2e/authz/grpc.go b/tests/e2e/authz/grpc.go index 5aa21e6b68cc..72de28813cba 100644 --- a/tests/e2e/authz/grpc.go +++ b/tests/e2e/authz/grpc.go @@ -197,7 +197,7 @@ func (s *E2ETestSuite) TestQueryGranterGrantsGRPC() { fmt.Sprintf("%s/cosmos/authz/v1beta1/grants/granter/%s", val.APIAddress, val.Address.String()), false, "", - 8, + 7, }, } for _, tc := range testCases { diff --git a/tests/e2e/authz/query.go b/tests/e2e/authz/query.go index b1275939fa20..e4e0f94073b8 100644 --- a/tests/e2e/authz/query.go +++ b/tests/e2e/authz/query.go @@ -234,7 +234,7 @@ func (s *E2ETestSuite) TestQueryGranterGrants() { }, false, "", - 8, + 7, }, { "valid case with pagination",