diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ee86691c2ca..57647a04c046 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -177,6 +177,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ibc-denom. * [\#10593](https://github.com/cosmos/cosmos-sdk/pull/10593) Update swagger-ui to v4.1.0 to fix xss vulnerability. * [\#10674](https://github.com/cosmos/cosmos-sdk/pull/10674) Fix issue with `Error.Wrap` and `Error.Wrapf` usage with `errors.Is`. +* (x/authz) [\#10633](https://github.com/cosmos/cosmos-sdk/pull/10633) Fixed authorization not found error when executing message ### State Machine Breaking diff --git a/x/authz/client/testutil/tx.go b/x/authz/client/testutil/tx.go index f3f245e1f52c..686a3d2771ec 100644 --- a/x/authz/client/testutil/tx.go +++ b/x/authz/client/testutil/tx.go @@ -548,7 +548,7 @@ func (s *IntegrationTestSuite) TestExecAuthorizationWithExpiration() { fmt.Sprintf("--%s=true", flags.FlagSkipConfirmation), }) s.Require().NoError(err) - s.Require().Contains(res.String(), "authorization not found") + s.Require().Contains(res.String(), "authorization expired") } func (s *IntegrationTestSuite) TestNewExecGenericAuthorized() { diff --git a/x/authz/keeper/grpc_query.go b/x/authz/keeper/grpc_query.go index 31e7d7967518..1e4a5a605a8a 100644 --- a/x/authz/keeper/grpc_query.go +++ b/x/authz/keeper/grpc_query.go @@ -33,17 +33,19 @@ func (k Keeper) Grants(c context.Context, req *authz.QueryGrantsRequest) (*authz if err != nil { return nil, err } - ctx := sdk.UnwrapSDKContext(c) - - store := ctx.KVStore(k.storeKey) - key := grantStoreKey(grantee, granter, "") - authStore := prefix.NewStore(store, key) + ctx := sdk.UnwrapSDKContext(c) if req.MsgTypeUrl != "" { - authorization, expiration := k.GetCleanAuthorization(ctx, grantee, granter, req.MsgTypeUrl) - if authorization == nil { + grant, found := k.getGrant(ctx, grantStoreKey(grantee, granter, req.MsgTypeUrl)) + if !found { return nil, status.Errorf(codes.NotFound, "no authorization found for %s type", req.MsgTypeUrl) } + + authorization, err := getAuthorization(grant) + if err != nil { + return nil, err + } + authorizationAny, err := codectypes.NewAnyWithValue(authorization) if err != nil { return nil, status.Errorf(codes.Internal, err.Error()) @@ -51,11 +53,15 @@ func (k Keeper) Grants(c context.Context, req *authz.QueryGrantsRequest) (*authz return &authz.QueryGrantsResponse{ Grants: []*authz.Grant{{ Authorization: authorizationAny, - Expiration: expiration, + Expiration: grant.Expiration, }}, }, nil } + store := ctx.KVStore(k.storeKey) + key := grantStoreKey(grantee, granter, "") + authStore := prefix.NewStore(store, key) + var authorizations []*authz.Grant pageRes, err := query.FilteredPaginate(authStore, req.Pagination, func(key []byte, value []byte, accumulate bool) (bool, error) { auth, err := unmarshalAuthorization(k.cdc, value) diff --git a/x/authz/keeper/keeper.go b/x/authz/keeper/keeper.go index c3ca01fb633a..f87e904afff1 100644 --- a/x/authz/keeper/keeper.go +++ b/x/authz/keeper/keeper.go @@ -87,10 +87,20 @@ func (k Keeper) DispatchActions(ctx sdk.Context, grantee sdk.AccAddress, msgs [] // if granter != grantee then check authorization.Accept, otherwise we implicitly accept. if !granter.Equals(grantee) { - authorization, _ := k.GetCleanAuthorization(ctx, grantee, granter, sdk.MsgTypeURL(msg)) - if authorization == nil { + grant, found := k.getGrant(ctx, grantStoreKey(grantee, granter, sdk.MsgTypeURL(msg))) + if !found { return nil, sdkerrors.ErrUnauthorized.Wrap("authorization not found") } + + if grant.Expiration.Before(ctx.BlockHeader().Time) { + return nil, sdkerrors.ErrUnauthorized.Wrap("authorization expired") + } + + authorization, err := getAuthorization(grant) + if err != nil { + return nil, err + } + resp, err := authorization.Accept(ctx, msg) if err != nil { return nil, err @@ -183,22 +193,6 @@ func (k Keeper) GetAuthorizations(ctx sdk.Context, grantee sdk.AccAddress, grant return authorizations } -// GetCleanAuthorization returns an `Authorization` and it's expiration time for -// (grantee, granter, message name) grant. If there is no grant `nil` is returned. -// If the grant is expired, the grant is revoked, removed from the storage, and `nil` is returned. -func (k Keeper) GetCleanAuthorization(ctx sdk.Context, grantee sdk.AccAddress, granter sdk.AccAddress, msgType string) (cap authz.Authorization, expiration time.Time) { - grant, found := k.getGrant(ctx, grantStoreKey(grantee, granter, msgType)) - if !found { - return nil, time.Time{} - } - if grant.Expiration.Before(ctx.BlockHeader().Time) { - k.DeleteGrant(ctx, grantee, granter, msgType) - return nil, time.Time{} - } - - return grant.GetAuthorization(), grant.Expiration -} - // IterateGrants iterates over all authorization grants // This function should be used with caution because it can involve significant IO operations. // It should not be used in query or msg services without charging additional gas. @@ -256,3 +250,12 @@ func (k Keeper) InitGenesis(ctx sdk.Context, data *authz.GenesisState) { } } } + +func getAuthorization(g authz.Grant) (authz.Authorization, error) { + a, ok := g.Authorization.GetCachedValue().(authz.Authorization) + if !ok { + return nil, sdkerrors.ErrInvalidType.Wrapf("expected %T, got %T", (authz.Authorization)(nil), g.Authorization.GetCachedValue()) + } + + return a, nil +} diff --git a/x/authz/keeper/keeper_test.go b/x/authz/keeper/keeper_test.go index 539300130bc6..3558f74392c0 100644 --- a/x/authz/keeper/keeper_test.go +++ b/x/authz/keeper/keeper_test.go @@ -2,7 +2,6 @@ package keeper_test import ( "testing" - "time" "github.com/stretchr/testify/suite" tmtime "github.com/tendermint/tendermint/libs/time" @@ -16,7 +15,12 @@ import ( banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" ) -var bankSendAuthMsgType = banktypes.SendAuthorization{}.MsgTypeURL() +var ( + bankSendAuthMsgType = banktypes.SendAuthorization{}.MsgTypeURL() + coins10 = sdk.NewCoins(sdk.NewInt64Coin("stake", 10)) + coins100 = sdk.NewCoins(sdk.NewInt64Coin("stake", 100)) + coins1000 = sdk.NewCoins(sdk.NewInt64Coin("stake", 1000)) +) type TestSuite struct { suite.Suite @@ -45,54 +49,48 @@ func (s *TestSuite) SetupTest() { func (s *TestSuite) TestKeeper() { app, ctx, addrs := s.app, s.ctx, s.addrs + now := ctx.BlockTime() + require := s.Require() granterAddr := addrs[0] granteeAddr := addrs[1] - recipientAddr := addrs[2] s.T().Log("verify that no authorization returns nil") - authorization, expiration := app.AuthzKeeper.GetCleanAuthorization(ctx, granteeAddr, granterAddr, bankSendAuthMsgType) - s.Require().Nil(authorization) - s.Require().Equal(expiration, time.Time{}) - now := s.ctx.BlockHeader().Time - s.Require().NotNil(now) - - newCoins := sdk.NewCoins(sdk.NewInt64Coin("steak", 100)) - s.T().Log("verify if expired authorization is rejected") - x := &banktypes.SendAuthorization{SpendLimit: newCoins} - err := app.AuthzKeeper.SaveGrant(ctx, granterAddr, granteeAddr, x, now.Add(-1*time.Hour)) - s.Require().NoError(err) - authorization, _ = app.AuthzKeeper.GetCleanAuthorization(ctx, granteeAddr, granterAddr, bankSendAuthMsgType) - s.Require().Nil(authorization) - - s.T().Log("verify if authorization is accepted") - x = &banktypes.SendAuthorization{SpendLimit: newCoins} - err = app.AuthzKeeper.SaveGrant(ctx, granteeAddr, granterAddr, x, now.Add(time.Hour)) - s.Require().NoError(err) - authorization, _ = app.AuthzKeeper.GetCleanAuthorization(ctx, granteeAddr, granterAddr, bankSendAuthMsgType) - s.Require().NotNil(authorization) - s.Require().Equal(authorization.MsgTypeURL(), bankSendAuthMsgType) - - s.T().Log("verify fetching authorization with wrong msg type fails") - authorization, _ = app.AuthzKeeper.GetCleanAuthorization(ctx, granteeAddr, granterAddr, sdk.MsgTypeURL(&banktypes.MsgMultiSend{})) - s.Require().Nil(authorization) - - s.T().Log("verify fetching authorization with wrong grantee fails") - authorization, _ = app.AuthzKeeper.GetCleanAuthorization(ctx, recipientAddr, granterAddr, bankSendAuthMsgType) - s.Require().Nil(authorization) - - s.T().Log("verify revoke fails with wrong information") - err = app.AuthzKeeper.DeleteGrant(ctx, recipientAddr, granterAddr, bankSendAuthMsgType) - s.Require().Error(err) - authorization, _ = app.AuthzKeeper.GetCleanAuthorization(ctx, recipientAddr, granterAddr, bankSendAuthMsgType) - s.Require().Nil(authorization) + authorizations := app.AuthzKeeper.GetAuthorizations(ctx, granteeAddr, granterAddr) + require.Len(authorizations, 0) + + s.T().Log("verify save, get and delete") + sendAutz := &banktypes.SendAuthorization{SpendLimit: coins100} + err := app.AuthzKeeper.SaveGrant(ctx, granteeAddr, granterAddr, sendAutz, now.AddDate(1, 0, 0)) + require.NoError(err) + authorizations = app.AuthzKeeper.GetAuthorizations(ctx, granteeAddr, granterAddr) + require.Len(authorizations, 1) + + err = app.AuthzKeeper.DeleteGrant(ctx, granteeAddr, granterAddr, sendAutz.MsgTypeURL()) + require.NoError(err) - s.T().Log("verify revoke executes with correct information") - err = app.AuthzKeeper.DeleteGrant(ctx, granteeAddr, granterAddr, bankSendAuthMsgType) - s.Require().NoError(err) - authorization, _ = app.AuthzKeeper.GetCleanAuthorization(ctx, granteeAddr, granterAddr, bankSendAuthMsgType) - s.Require().Nil(authorization) + authorizations = app.AuthzKeeper.GetAuthorizations(ctx, granteeAddr, granterAddr) + require.Len(authorizations, 0) + s.T().Log("verify granting same authorization overwrite existing authorization") + err = app.AuthzKeeper.SaveGrant(ctx, granteeAddr, granterAddr, sendAutz, now.AddDate(1, 0, 0)) + require.NoError(err) + authorizations = app.AuthzKeeper.GetAuthorizations(ctx, granteeAddr, granterAddr) + require.Len(authorizations, 1) + + sendAutz = &banktypes.SendAuthorization{SpendLimit: coins1000} + err = app.AuthzKeeper.SaveGrant(ctx, granteeAddr, granterAddr, sendAutz, now.AddDate(1, 0, 0)) + require.NoError(err) + authorizations = app.AuthzKeeper.GetAuthorizations(ctx, granteeAddr, granterAddr) + require.Len(authorizations, 1) + authorization := authorizations[0] + sendAuth := authorization.(*banktypes.SendAuthorization) + require.Equal(sendAuth.SpendLimit, sendAutz.SpendLimit) + require.Equal(sendAuth.MsgTypeURL(), sendAutz.MsgTypeURL()) + + s.T().Log("verify removing non existing authorization returns error") + err = app.AuthzKeeper.DeleteGrant(ctx, granterAddr, granteeAddr, "abcd") + s.Require().Error(err) } func (s *TestSuite) TestKeeperIter() { @@ -100,101 +98,150 @@ func (s *TestSuite) TestKeeperIter() { granterAddr := addrs[0] granteeAddr := addrs[1] + granter2Addr := addrs[2] - s.T().Log("verify that no authorization returns nil") - authorization, expiration := app.AuthzKeeper.GetCleanAuthorization(ctx, granteeAddr, granterAddr, "Abcd") - s.Require().Nil(authorization) - s.Require().Equal(time.Time{}, expiration) - now := s.ctx.BlockHeader().Time - s.Require().NotNil(now) - - newCoins := sdk.NewCoins(sdk.NewInt64Coin("steak", 100)) - s.T().Log("verify if expired authorization is rejected") - x := &banktypes.SendAuthorization{SpendLimit: newCoins} - err := app.AuthzKeeper.SaveGrant(ctx, granteeAddr, granterAddr, x, now.Add(-1*time.Hour)) - s.Require().NoError(err) - authorization, _ = app.AuthzKeeper.GetCleanAuthorization(ctx, granteeAddr, granterAddr, "abcd") - s.Require().Nil(authorization) + s.app.AuthzKeeper.SaveGrant(ctx, granteeAddr, granterAddr, banktypes.NewSendAuthorization(coins100), ctx.BlockTime().AddDate(1, 0, 0)) + s.app.AuthzKeeper.SaveGrant(ctx, granteeAddr, granter2Addr, banktypes.NewSendAuthorization(coins100), ctx.BlockTime().AddDate(1, 0, 0)) app.AuthzKeeper.IterateGrants(ctx, func(granter, grantee sdk.AccAddress, grant authz.Grant) bool { - s.Require().Equal(granter, granterAddr) - s.Require().Equal(grantee, granteeAddr) + s.Require().Equal(granteeAddr, grantee) + s.Require().Contains([]sdk.AccAddress{granterAddr, granter2Addr}, granter) return true }) } -func (s *TestSuite) TestKeeperFees() { +func (s *TestSuite) TestDispatchAction() { app, addrs := s.app, s.addrs + require := s.Require() + now := s.ctx.BlockTime() granterAddr := addrs[0] granteeAddr := addrs[1] recipientAddr := addrs[2] - s.Require().NoError(testutil.FundAccount(app.BankKeeper, s.ctx, granterAddr, sdk.NewCoins(sdk.NewInt64Coin("steak", 10000)))) - now := s.ctx.BlockHeader().Time - s.Require().NotNil(now) - smallCoin := sdk.NewCoins(sdk.NewInt64Coin("steak", 20)) - someCoin := sdk.NewCoins(sdk.NewInt64Coin("steak", 123)) - - msgs := authz.NewMsgExec(granteeAddr, []sdk.Msg{ - &banktypes.MsgSend{ - Amount: sdk.NewCoins(sdk.NewInt64Coin("steak", 2)), - FromAddress: granterAddr.String(), - ToAddress: recipientAddr.String(), + require.NoError(testutil.FundAccount(app.BankKeeper, s.ctx, granterAddr, coins1000)) + + testCases := []struct { + name string + req authz.MsgExec + expectErr bool + errMsg string + preRun func() + postRun func() + }{ + { + "expect error authorization not found", + authz.NewMsgExec(granteeAddr, []sdk.Msg{ + &banktypes.MsgSend{ + Amount: coins10, + FromAddress: granterAddr.String(), + ToAddress: recipientAddr.String(), + }, + }), + true, + "authorization not found", + func() { + // remove any existing authorizations + app.AuthzKeeper.DeleteGrant(s.ctx, granteeAddr, granterAddr, bankSendAuthMsgType) + }, + func() {}, }, - }) - - s.Require().NoError(msgs.UnpackInterfaces(app.AppCodec())) - - s.T().Log("verify dispatch fails with invalid authorization") - executeMsgs, err := msgs.GetMessages() - s.Require().NoError(err) - result, err := app.AuthzKeeper.DispatchActions(s.ctx, granteeAddr, executeMsgs) - - s.Require().Nil(result) - s.Require().NotNil(err) - - s.T().Log("verify dispatch executes with correct information") - // grant authorization - err = app.AuthzKeeper.SaveGrant(s.ctx, granteeAddr, granterAddr, &banktypes.SendAuthorization{SpendLimit: smallCoin}, now) - s.Require().NoError(err) - authorization, _ := app.AuthzKeeper.GetCleanAuthorization(s.ctx, granteeAddr, granterAddr, bankSendAuthMsgType) - s.Require().NotNil(authorization) - - s.Require().Equal(authorization.MsgTypeURL(), bankSendAuthMsgType) - - executeMsgs, err = msgs.GetMessages() - s.Require().NoError(err) - - result, err = app.AuthzKeeper.DispatchActions(s.ctx, granteeAddr, executeMsgs) - s.Require().NoError(err) - s.Require().NotNil(result) - - authorization, _ = app.AuthzKeeper.GetCleanAuthorization(s.ctx, granteeAddr, granterAddr, bankSendAuthMsgType) - s.Require().NotNil(authorization) - - s.T().Log("verify dispatch fails with overlimit") - // grant authorization - - msgs = authz.NewMsgExec(granteeAddr, []sdk.Msg{ - &banktypes.MsgSend{ - Amount: someCoin, - FromAddress: granterAddr.String(), - ToAddress: recipientAddr.String(), + { + "expect error expired authorization", + authz.NewMsgExec(granteeAddr, []sdk.Msg{ + &banktypes.MsgSend{ + Amount: coins10, + FromAddress: granterAddr.String(), + ToAddress: recipientAddr.String(), + }, + }), + true, + "authorization expired", + func() { + err := app.AuthzKeeper.SaveGrant(s.ctx, granteeAddr, granterAddr, banktypes.NewSendAuthorization(coins100), now.AddDate(0, -1, 0)) + require.NoError(err) + }, + func() {}, }, - }) - - s.Require().NoError(msgs.UnpackInterfaces(app.AppCodec())) - executeMsgs, err = msgs.GetMessages() - s.Require().NoError(err) + { + "expect error over spent limit", + authz.NewMsgExec(granteeAddr, []sdk.Msg{ + &banktypes.MsgSend{ + Amount: coins1000, + FromAddress: granterAddr.String(), + ToAddress: recipientAddr.String(), + }, + }), + true, + "requested amount is more than spend limit", + func() { + err := app.AuthzKeeper.SaveGrant(s.ctx, granteeAddr, granterAddr, banktypes.NewSendAuthorization(coins100), now.AddDate(0, 1, 0)) + require.NoError(err) + }, + func() {}, + }, + { + "valid test verify amount left", + authz.NewMsgExec(granteeAddr, []sdk.Msg{ + &banktypes.MsgSend{ + Amount: coins10, + FromAddress: granterAddr.String(), + ToAddress: recipientAddr.String(), + }, + }), + false, + "", + func() { + err := app.AuthzKeeper.SaveGrant(s.ctx, granteeAddr, granterAddr, banktypes.NewSendAuthorization(coins100), now.AddDate(0, 1, 0)) + require.NoError(err) + }, + func() { + authzs := app.AuthzKeeper.GetAuthorizations(s.ctx, granteeAddr, granterAddr) + require.Len(authzs, 1) + authorization := authzs[0].(*banktypes.SendAuthorization) + require.NotNil(authorization) + require.Equal(authorization.SpendLimit, coins100.Sub(coins10)) + }, + }, + { + "valid test verify authorization is removed when it is used up", + authz.NewMsgExec(granteeAddr, []sdk.Msg{ + &banktypes.MsgSend{ + Amount: coins100, + FromAddress: granterAddr.String(), + ToAddress: recipientAddr.String(), + }, + }), + false, + "", + func() { + err := app.AuthzKeeper.SaveGrant(s.ctx, granteeAddr, granterAddr, banktypes.NewSendAuthorization(coins100), now.AddDate(0, 1, 0)) + require.NoError(err) + }, + func() { + authzs := app.AuthzKeeper.GetAuthorizations(s.ctx, granteeAddr, granterAddr) + require.Len(authzs, 0) + }, + }, + } - result, err = app.AuthzKeeper.DispatchActions(s.ctx, granteeAddr, executeMsgs) - s.Require().Nil(result) - s.Require().NotNil(err) + for _, tc := range testCases { + tc.preRun() + executeMsgs, err := tc.req.GetMessages() + s.Require().NoError(err) + result, err := app.AuthzKeeper.DispatchActions(s.ctx, granteeAddr, executeMsgs) + if tc.expectErr { + require.Error(err) + require.Nil(result) + require.Contains(err.Error(), tc.errMsg) + } else { + require.NoError(err) + require.NotNil(result) + } + tc.postRun() + } - authorization, _ = app.AuthzKeeper.GetCleanAuthorization(s.ctx, granteeAddr, granterAddr, bankSendAuthMsgType) - s.Require().NotNil(authorization) } // Tests that all msg events included in an authz MsgExec tx @@ -205,24 +252,24 @@ func (s *TestSuite) TestDispatchedEvents() { granterAddr := addrs[0] granteeAddr := addrs[1] recipientAddr := addrs[2] - require.NoError(testutil.FundAccount(app.BankKeeper, s.ctx, granterAddr, sdk.NewCoins(sdk.NewInt64Coin("steak", 10000)))) + require.NoError(testutil.FundAccount(app.BankKeeper, s.ctx, granterAddr, coins1000)) now := s.ctx.BlockHeader().Time require.NotNil(now) - smallCoin := sdk.NewCoins(sdk.NewInt64Coin("steak", 20)) msgs := authz.NewMsgExec(granteeAddr, []sdk.Msg{ &banktypes.MsgSend{ - Amount: sdk.NewCoins(sdk.NewInt64Coin("steak", 2)), + Amount: coins10, FromAddress: granterAddr.String(), ToAddress: recipientAddr.String(), }, }) // grant authorization - err := app.AuthzKeeper.SaveGrant(s.ctx, granteeAddr, granterAddr, &banktypes.SendAuthorization{SpendLimit: smallCoin}, now) + err := app.AuthzKeeper.SaveGrant(s.ctx, granteeAddr, granterAddr, &banktypes.SendAuthorization{SpendLimit: coins10}, now) require.NoError(err) - authorization, _ := app.AuthzKeeper.GetCleanAuthorization(s.ctx, granteeAddr, granterAddr, bankSendAuthMsgType) - require.NotNil(authorization) + authorizations := app.AuthzKeeper.GetAuthorizations(s.ctx, granteeAddr, granterAddr) + require.Len(authorizations, 1) + authorization := authorizations[0].(*banktypes.SendAuthorization) require.Equal(authorization.MsgTypeURL(), bankSendAuthMsgType) executeMsgs, err := msgs.GetMessages()