From 8a2c93fb7d6ebc263967c1a02be463a47c304373 Mon Sep 17 00:00:00 2001 From: atheeshp <59333759+atheeshp@users.noreply.github.com> Date: Tue, 6 Dec 2022 18:33:04 +0530 Subject: [PATCH] chore: x/authz audit changes (#14120) * chore: x/authz audit changes * nits * tests * Update x/authz/keeper/msg_server_test.go Co-authored-by: Likhita Polavarapu <78951027+likhita-809@users.noreply.github.com> Co-authored-by: Likhita Polavarapu <78951027+likhita-809@users.noreply.github.com> Co-authored-by: Marko --- x/authz/keeper/grpc_query.go | 3 +- x/authz/keeper/grpc_query_test.go | 4 +- x/authz/keeper/keeper.go | 2 + x/authz/keeper/keeper_test.go | 21 ++- x/authz/keeper/msg_server.go | 4 +- x/authz/keeper/msg_server_test.go | 263 ++++++++++++++++++++++++++++++ 6 files changed, 281 insertions(+), 16 deletions(-) create mode 100644 x/authz/keeper/msg_server_test.go diff --git a/x/authz/keeper/grpc_query.go b/x/authz/keeper/grpc_query.go index cfca0be70f35..c4cb72b51485 100644 --- a/x/authz/keeper/grpc_query.go +++ b/x/authz/keeper/grpc_query.go @@ -16,7 +16,7 @@ import ( var _ authz.QueryServer = Keeper{} -// Authorizations implements the Query/Grants gRPC method. +// Grants implements the Query/Grants gRPC method. // It returns grants for a granter-grantee pair. If msg type URL is set, it returns grants only for that msg type. func (k Keeper) Grants(c context.Context, req *authz.QueryGrantsRequest) (*authz.QueryGrantsResponse, error) { if req == nil { @@ -173,6 +173,7 @@ func (k Keeper) GranteeGrants(c context.Context, req *authz.QueryGranteeGrantsRe }, func() *authz.Grant { return &authz.Grant{} }) + if err != nil { return nil, err } diff --git a/x/authz/keeper/grpc_query_test.go b/x/authz/keeper/grpc_query_test.go index fc33b5f6e7d4..426c74be8707 100644 --- a/x/authz/keeper/grpc_query_test.go +++ b/x/authz/keeper/grpc_query_test.go @@ -302,11 +302,11 @@ func (suite *TestSuite) TestGRPCQueryGranteeGrants() { } } -func (suite *TestSuite) createSendAuthorization(a1, a2 sdk.AccAddress) authz.Authorization { +func (suite *TestSuite) createSendAuthorization(grantee, granter sdk.AccAddress) authz.Authorization { exp := suite.ctx.BlockHeader().Time.Add(time.Hour) newCoins := sdk.NewCoins(sdk.NewInt64Coin("steak", 100)) authorization := &banktypes.SendAuthorization{SpendLimit: newCoins} - err := suite.authzKeeper.SaveGrant(suite.ctx, a1, a2, authorization, &exp) + err := suite.authzKeeper.SaveGrant(suite.ctx, grantee, granter, authorization, &exp) suite.Require().NoError(err) return authorization } diff --git a/x/authz/keeper/keeper.go b/x/authz/keeper/keeper.go index b9a9ae6bc502..22edf8f2218f 100644 --- a/x/authz/keeper/keeper.go +++ b/x/authz/keeper/keeper.go @@ -177,11 +177,13 @@ func (k Keeper) SaveGrant(ctx sdk.Context, grantee, granter sdk.AccAddress, auth if oldGrant, found := k.getGrant(ctx, skey); found { oldExp = oldGrant.Expiration } + if oldExp != nil && (expiration == nil || !oldExp.Equal(*expiration)) { if err = k.removeFromGrantQueue(ctx, skey, granter, grantee, *oldExp); err != nil { return err } } + // If the expiration didn't change, then we don't remove it and we should not insert again if expiration != nil && (oldExp == nil || !oldExp.Equal(*expiration)) { if err = k.insertIntoGrantQueue(ctx, granter, grantee, msgType, *expiration); err != nil { diff --git a/x/authz/keeper/keeper_test.go b/x/authz/keeper/keeper_test.go index 3108d3998931..9925871df279 100644 --- a/x/authz/keeper/keeper_test.go +++ b/x/authz/keeper/keeper_test.go @@ -10,7 +10,6 @@ import ( tmtime "github.com/tendermint/tendermint/types/time" "github.com/cosmos/cosmos-sdk/baseapp" - codectypes "github.com/cosmos/cosmos-sdk/codec/types" "github.com/cosmos/cosmos-sdk/testutil" simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims" sdk "github.com/cosmos/cosmos-sdk/types" @@ -33,15 +32,15 @@ var ( type TestSuite struct { suite.Suite - ctx sdk.Context - addrs []sdk.AccAddress - authzKeeper authzkeeper.Keeper - accountKeeper *authztestutil.MockAccountKeeper - bankKeeper *authztestutil.MockBankKeeper - interfaceRegistry codectypes.InterfaceRegistry - baseApp *baseapp.BaseApp - encCfg moduletestutil.TestEncodingConfig - queryClient authz.QueryClient + ctx sdk.Context + addrs []sdk.AccAddress + authzKeeper authzkeeper.Keeper + accountKeeper *authztestutil.MockAccountKeeper + bankKeeper *authztestutil.MockBankKeeper + baseApp *baseapp.BaseApp + encCfg moduletestutil.TestEncodingConfig + queryClient authz.QueryClient + msgSrvr authz.MsgServer } func (s *TestSuite) SetupTest() { @@ -75,7 +74,7 @@ func (s *TestSuite) SetupTest() { queryClient := authz.NewQueryClient(queryHelper) s.queryClient = queryClient - s.queryClient = queryClient + s.msgSrvr = s.authzKeeper } func (s *TestSuite) TestKeeper() { diff --git a/x/authz/keeper/msg_server.go b/x/authz/keeper/msg_server.go index 7f3ff113d10e..358146124d8f 100644 --- a/x/authz/keeper/msg_server.go +++ b/x/authz/keeper/msg_server.go @@ -10,7 +10,7 @@ import ( var _ authz.MsgServer = Keeper{} -// GrantAuthorization implements the MsgServer.Grant method to create a new grant. +// 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) grantee, err := sdk.AccAddressFromBech32(msg.Grantee) @@ -48,7 +48,7 @@ func (k Keeper) Grant(goCtx context.Context, msg *authz.MsgGrant) (*authz.MsgGra return &authz.MsgGrantResponse{}, nil } -// RevokeAuthorization implements the MsgServer.Revoke method. +// Revoke implements the MsgServer.Revoke method. func (k Keeper) Revoke(goCtx context.Context, msg *authz.MsgRevoke) (*authz.MsgRevokeResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) grantee, err := sdk.AccAddressFromBech32(msg.Grantee) diff --git a/x/authz/keeper/msg_server_test.go b/x/authz/keeper/msg_server_test.go new file mode 100644 index 000000000000..50f95f1b04e4 --- /dev/null +++ b/x/authz/keeper/msg_server_test.go @@ -0,0 +1,263 @@ +package keeper_test + +import ( + "time" + + simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims" + sdk "github.com/cosmos/cosmos-sdk/types" + authtypes "github.com/cosmos/cosmos-sdk/x/auth/types" + "github.com/cosmos/cosmos-sdk/x/authz" + banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" + + "github.com/golang/mock/gomock" +) + +func (suite *TestSuite) createAccounts(accs int) []sdk.AccAddress { + addrs := simtestutil.CreateIncrementalAccounts(2) + suite.accountKeeper.EXPECT().GetAccount(gomock.Any(), suite.addrs[0]).Return(authtypes.NewBaseAccountWithAddress(suite.addrs[0])).AnyTimes() + suite.accountKeeper.EXPECT().GetAccount(gomock.Any(), suite.addrs[1]).Return(authtypes.NewBaseAccountWithAddress(suite.addrs[1])).AnyTimes() + return addrs +} + +func (suite *TestSuite) TestGrant() { + ctx := suite.ctx.WithBlockTime(time.Now()) + addrs := suite.createAccounts(2) + curBlockTime := ctx.BlockTime() + + oneHour := curBlockTime.Add(time.Hour) + oneYear := curBlockTime.AddDate(1, 0, 0) + + coins := sdk.NewCoins(sdk.NewCoin("steak", sdk.NewInt(10))) + + grantee, granter := addrs[0], addrs[1] + + testCases := []struct { + name string + malleate func() *authz.MsgGrant + expErr bool + errMsg string + }{ + { + name: "invalid granter", + malleate: func() *authz.MsgGrant { + grant, err := authz.NewGrant(curBlockTime, banktypes.NewSendAuthorization(coins, nil), &oneYear) + suite.Require().NoError(err) + return &authz.MsgGrant{ + Granter: "invalid", + Grantee: grantee.String(), + Grant: grant, + } + }, + expErr: true, + errMsg: "invalid bech32 string", + }, + { + name: "invalid grantee", + malleate: func() *authz.MsgGrant { + grant, err := authz.NewGrant(curBlockTime, banktypes.NewSendAuthorization(coins, nil), &oneYear) + suite.Require().NoError(err) + return &authz.MsgGrant{ + Granter: granter.String(), + Grantee: "invalid", + Grant: grant, + } + }, + expErr: true, + errMsg: "invalid bech32 string", + }, + { + name: "grantee account does not exist on chain: valid grant", + malleate: func() *authz.MsgGrant { + newAcc := sdk.AccAddress("valid") + suite.accountKeeper.EXPECT().GetAccount(gomock.Any(), newAcc).Return(nil).AnyTimes() + acc := authtypes.NewBaseAccountWithAddress(newAcc) + suite.accountKeeper.EXPECT().NewAccountWithAddress(gomock.Any(), newAcc).Return(acc).AnyTimes() + suite.accountKeeper.EXPECT().SetAccount(gomock.Any(), acc).Return() + + grant, err := authz.NewGrant(curBlockTime, banktypes.NewSendAuthorization(coins, nil), &oneYear) + suite.Require().NoError(err) + return &authz.MsgGrant{ + Granter: granter.String(), + Grantee: newAcc.String(), + Grant: grant, + } + }, + }, + { + name: "valid grant", + malleate: func() *authz.MsgGrant { + grant, err := authz.NewGrant(curBlockTime, banktypes.NewSendAuthorization(coins, nil), &oneYear) + suite.Require().NoError(err) + return &authz.MsgGrant{ + Granter: granter.String(), + Grantee: grantee.String(), + Grant: grant, + } + }, + }, + { + name: "valid grant, same grantee, granter pair but different msgType", + malleate: func() *authz.MsgGrant { + g, err := authz.NewGrant(curBlockTime, banktypes.NewSendAuthorization(coins, nil), &oneHour) + suite.Require().NoError(err) + _, err = suite.msgSrvr.Grant(suite.ctx, &authz.MsgGrant{ + Granter: granter.String(), + Grantee: grantee.String(), + Grant: g, + }) + suite.Require().NoError(err) + + grant, err := authz.NewGrant(curBlockTime, authz.NewGenericAuthorization("/cosmos.bank.v1beta1.MsgUpdateParams"), &oneHour) + suite.Require().NoError(err) + return &authz.MsgGrant{ + Granter: granter.String(), + Grantee: grantee.String(), + Grant: grant, + } + }, + }, + } + + for _, tc := range testCases { + suite.Run(tc.name, func() { + _, err := suite.msgSrvr.Grant(suite.ctx, tc.malleate()) + if tc.expErr { + suite.Require().Error(err) + suite.Require().Contains(err.Error(), tc.errMsg) + } else { + suite.Require().NoError(err) + } + }) + } +} + +func (suite *TestSuite) TestRevoke() { + addrs := suite.createAccounts(2) + + grantee, granter := addrs[0], addrs[1] + + testCases := []struct { + name string + malleate func() *authz.MsgRevoke + expErr bool + errMsg string + }{ + { + name: "invalid granter", + malleate: func() *authz.MsgRevoke { + return &authz.MsgRevoke{ + Granter: "invalid", + Grantee: grantee.String(), + MsgTypeUrl: bankSendAuthMsgType, + } + }, + expErr: true, + errMsg: "invalid bech32 string", + }, + { + name: "invalid grantee", + malleate: func() *authz.MsgRevoke { + return &authz.MsgRevoke{ + Granter: granter.String(), + Grantee: "invalid", + MsgTypeUrl: bankSendAuthMsgType, + } + }, + expErr: true, + errMsg: "invalid bech32 string", + }, + { + name: "valid grant", + malleate: func() *authz.MsgRevoke { + suite.createSendAuthorization(grantee, granter) + + return &authz.MsgRevoke{ + Granter: granter.String(), + Grantee: grantee.String(), + MsgTypeUrl: bankSendAuthMsgType, + } + }, + }, + { + name: "no existing grant to revoke", + malleate: func() *authz.MsgRevoke { + return &authz.MsgRevoke{ + Granter: granter.String(), + Grantee: grantee.String(), + MsgTypeUrl: bankSendAuthMsgType, + } + }, + expErr: true, + errMsg: "authorization not found", + }, + } + + for _, tc := range testCases { + suite.Run(tc.name, func() { + _, err := suite.msgSrvr.Revoke(suite.ctx, tc.malleate()) + if tc.expErr { + suite.Require().Error(err) + suite.Require().Contains(err.Error(), tc.errMsg) + } else { + suite.Require().NoError(err) + } + }) + } +} + +func (suite *TestSuite) TestExec() { + addrs := suite.createAccounts(2) + + grantee, granter := addrs[0], addrs[1] + coins := sdk.NewCoins(sdk.NewCoin("steak", sdk.NewInt(10))) + + msg := &banktypes.MsgSend{ + FromAddress: granter.String(), + ToAddress: grantee.String(), + Amount: coins, + } + + testCases := []struct { + name string + malleate func() authz.MsgExec + expErr bool + errMsg string + }{ + { + name: "invalid grantee", + malleate: func() authz.MsgExec { + return authz.NewMsgExec(sdk.AccAddress{}, []sdk.Msg{msg}) + }, + expErr: true, + errMsg: "empty address string is not allowed", + }, + { + name: "no existing grant", + malleate: func() authz.MsgExec { + return authz.NewMsgExec(grantee, []sdk.Msg{msg}) + }, + expErr: true, + errMsg: "authorization not found", + }, + { + name: "valid case", + malleate: func() authz.MsgExec { + suite.createSendAuthorization(grantee, granter) + return authz.NewMsgExec(grantee, []sdk.Msg{msg}) + }, + }, + } + + for _, tc := range testCases { + suite.Run(tc.name, func() { + req := tc.malleate() + _, err := suite.msgSrvr.Exec(suite.ctx, &req) + if tc.expErr { + suite.Require().Error(err) + suite.Require().Contains(err.Error(), tc.errMsg) + } else { + suite.Require().NoError(err) + } + }) + } +}