Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove time.now check from authz #11129

Merged
merged 17 commits into from
Feb 7, 2022
Merged
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [\#10816](https://github.com/cosmos/cosmos-sdk/pull/10816) Reuse blocked addresses from the bank module. No need to pass them to distribution.
* [\#10852](https://github.com/cosmos/cosmos-sdk/pull/10852) Move `x/gov/types` to `x/gov/types/v1beta2`.
* [\#10868](https://github.com/cosmos/cosmos-sdk/pull/10868), [\#10989](https://github.com/cosmos/cosmos-sdk/pull/10989), [\#11093](https://github.com/cosmos/cosmos-sdk/pull/11093) The Gov keeper accepts now 2 more mandatory arguments, the ServiceMsgRouter and a gov Config including the max metadata length.
* (x/authz) [\#10447](https://github.com/cosmos/cosmos-sdk/pull/10447) authz `NewGrant` takes a new argument: block time, to correctly validate expire time.

### Client Breaking Changes
* [\#11089](https://github.com/cosmos/cosmos-sdk/pull/11089]) interacting with the node through `grpc.Dial` requires clients to pass a codec refer to [doc](docs/run-node/interact-node.md).
Expand Down Expand Up @@ -184,6 +185,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [#9790](https://github.com/cosmos/cosmos-sdk/pull/10687) Fix behavior of `DecCoins.MulDecTruncate`.
* [\#10990](https://github.com/cosmos/cosmos-sdk/pull/10990) Fixes missing `iavl-cache-size` config parsing in `GetConfig` method.
* (crypto) [#11027] Remove dependency on Tendermint core for xsalsa20symmetric.
* (x/authz) [\#10447](https://github.com/cosmos/cosmos-sdk/pull/10447) Fix authz `NewGrant` expiration check.

### State Machine Breaking

Expand Down
9 changes: 4 additions & 5 deletions x/authz/authorization_grant.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@ import (
)

// NewGrant returns new Grant
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
func NewGrant(a Authorization, expiration time.Time) (Grant, error) {
func NewGrant(blockTime time.Time, a Authorization, expiration time.Time) (Grant, error) {
if !expiration.After(blockTime) {
return Grant{}, sdkerrors.ErrInvalidRequest.Wrapf("expiration must be after the current block time (%v), got %v", blockTime.Format(time.RFC3339), expiration.Format(time.RFC3339))
}
g := Grant{
Expiration: expiration,
}
Expand Down Expand Up @@ -51,10 +54,6 @@ func (g Grant) GetAuthorization() Authorization {
}

func (g Grant) ValidateBasic() error {
if g.Expiration.Unix() < time.Now().Unix() {
return sdkerrors.Wrap(ErrInvalidExpirationTime, "Time can't be in the past")
}

av := g.Authorization.GetCachedValue()
a, ok := av.(Authorization)
if !ok {
Expand Down
43 changes: 43 additions & 0 deletions x/authz/authorization_grant_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
package authz

import (
"testing"
"time"

"github.com/stretchr/testify/require"
)

// TODO: remove and use: robert/expect-error
func expecError(r *require.Assertions, expected string, received error) {
if expected == "" {
r.NoError(received)
} else {
r.Error(received)
r.Contains(received.Error(), expected)
}
}

func TestNewGrant(t *testing.T) {
a := NewGenericAuthorization("some-type")
var tcs = []struct {
title string
a Authorization
blockTime time.Time
expire time.Time
err string
}{
{"wrong expire time (1)", a, time.Unix(10, 0), time.Unix(8, 0), "expiration must be after"},
{"wrong expire time (2)", a, time.Unix(10, 0), time.Unix(10, 0), "expiration must be after"},
{"good expire time (1)", a, time.Unix(10, 0), time.Unix(10, 1), ""},
{"good expire time (2)", a, time.Unix(10, 0), time.Unix(11, 0), ""},
}

for _, tc := range tcs {
tc := tc
t.Run(tc.title, func(t *testing.T) {
_, err := NewGrant(tc.blockTime, tc.a, tc.expire)
expecError(require.New(t), tc.err, err)
})
}

}
2 changes: 1 addition & 1 deletion x/authz/client/cli/tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func NewCmdGrantAuthorization() *cobra.Command {
Use: "grant <grantee> <authorization_type=\"send\"|\"generic\"|\"delegate\"|\"unbond\"|\"redelegate\"> --from <granter>",
Short: "Grant authorization to an address",
Long: strings.TrimSpace(
fmt.Sprintf(`grant authorization to an address to execute a transaction on your behalf:
fmt.Sprintf(`create a new grant authorization to an address to execute a transaction on your behalf:

Examples:
$ %s tx %s grant cosmos1skjw.. send %s --spend-limit=1000stake --from=cosmos1skl..
Expand Down
2 changes: 1 addition & 1 deletion x/authz/client/testutil/grpc.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ func (s *IntegrationTestSuite) TestQueryGrantsGRPC() {
false,
"",
func() {
_, err := ExecGrant(val, []string{
_, err := CreateGrant(val, []string{
grantee.String(),
"generic",
fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()),
Expand Down
4 changes: 2 additions & 2 deletions x/authz/client/testutil/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ func (s *IntegrationTestSuite) TestQueryAuthorizations() {
grantee := s.grantee[0]
twoHours := time.Now().Add(time.Minute * time.Duration(120)).Unix()

_, err := ExecGrant(
_, err := CreateGrant(
val,
[]string{
grantee.String(),
Expand Down Expand Up @@ -98,7 +98,7 @@ func (s *IntegrationTestSuite) TestQueryAuthorization() {
grantee := s.grantee[0]
twoHours := time.Now().Add(time.Minute * time.Duration(120)).Unix()

_, err := ExecGrant(
_, err := CreateGrant(
val,
[]string{
grantee.String(),
Expand Down
2 changes: 1 addition & 1 deletion x/authz/client/testutil/test_helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import (
"github.com/cosmos/cosmos-sdk/x/authz/client/cli"
)

func ExecGrant(val *network.Validator, args []string) (testutil.BufferWriter, error) {
func CreateGrant(val *network.Validator, args []string) (testutil.BufferWriter, error) {
cmd := cli.NewCmdGrantAuthorization()
clientCtx := val.ClientCtx
return clitestutil.ExecTestCLICmd(clientCtx, cmd, args)
Expand Down
37 changes: 18 additions & 19 deletions x/authz/client/testutil/tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func (s *IntegrationTestSuite) SetupSuite() {
s.msgSendExec(s.grantee[1])

// grant send authorization to grantee2
out, err := ExecGrant(val, []string{
out, err := CreateGrant(val, []string{
s.grantee[1].String(),
"send",
fmt.Sprintf("--%s=100steak", cli.FlagSpendLimit),
Expand All @@ -85,7 +85,7 @@ func (s *IntegrationTestSuite) SetupSuite() {
s.grantee[2] = s.createAccount("grantee3")

// grant send authorization to grantee3
out, err = ExecGrant(val, []string{
out, err = CreateGrant(val, []string{
s.grantee[2].String(),
"send",
fmt.Sprintf("--%s=100steak", cli.FlagSpendLimit),
Expand Down Expand Up @@ -147,8 +147,8 @@ func (s *IntegrationTestSuite) TestCLITxGrantAuthorization() {
val := s.network.Validators[0]
grantee := s.grantee[0]

twoHours := time.Now().Add(time.Minute * time.Duration(120)).Unix()
pastHour := time.Now().Add(time.Minute * time.Duration(-60)).Unix()
twoHours := time.Now().Add(time.Minute * 120).Unix()
pastHour := time.Now().Add(-time.Minute * 60).Unix()

testCases := []struct {
name string
Expand Down Expand Up @@ -189,7 +189,7 @@ func (s *IntegrationTestSuite) TestCLITxGrantAuthorization() {
"send",
fmt.Sprintf("--%s=100steak", cli.FlagSpendLimit),
fmt.Sprintf("--%s=%s", flags.FlagFrom, val.Address.String()),
fmt.Sprintf("--%s=true", flags.FlagGenerateOnly),
fmt.Sprintf("--%s=true", flags.FlagBroadcastMode),
fmt.Sprintf("--%s=%d", cli.FlagExpiration, pastHour),
},
0,
Expand Down Expand Up @@ -340,15 +340,14 @@ func (s *IntegrationTestSuite) TestCLITxGrantAuthorization() {
}

for _, tc := range testCases {
tc := tc
s.Run(tc.name, func() {
clientCtx := val.ClientCtx
out, err := ExecGrant(
out, err := CreateGrant(
val,
tc.args,
)
if tc.expectErr {
s.Require().Error(err)
s.Require().Error(err, out)
} else {
var txResp sdk.TxResponse
s.Require().NoError(err)
Expand All @@ -372,7 +371,7 @@ func (s *IntegrationTestSuite) TestCmdRevokeAuthorizations() {
twoHours := time.Now().Add(time.Minute * time.Duration(120)).Unix()

// send-authorization
_, err := ExecGrant(
_, err := CreateGrant(
val,
[]string{
grantee.String(),
Expand All @@ -388,7 +387,7 @@ func (s *IntegrationTestSuite) TestCmdRevokeAuthorizations() {
s.Require().NoError(err)

// generic-authorization
_, err = ExecGrant(
_, err = CreateGrant(
val,
[]string{
grantee.String(),
Expand All @@ -404,7 +403,7 @@ func (s *IntegrationTestSuite) TestCmdRevokeAuthorizations() {
s.Require().NoError(err)

// generic-authorization used for amino testing
_, err = ExecGrant(
_, err = CreateGrant(
val,
[]string{
grantee.String(),
Expand Down Expand Up @@ -517,7 +516,7 @@ func (s *IntegrationTestSuite) TestExecAuthorizationWithExpiration() {
grantee := s.grantee[0]
tenSeconds := time.Now().Add(time.Second * time.Duration(10)).Unix()

_, err := ExecGrant(
_, err := CreateGrant(
val,
[]string{
grantee.String(),
Expand Down Expand Up @@ -557,7 +556,7 @@ func (s *IntegrationTestSuite) TestNewExecGenericAuthorized() {
grantee := s.grantee[0]
twoHours := time.Now().Add(time.Minute * time.Duration(120)).Unix()

_, err := ExecGrant(
_, err := CreateGrant(
val,
[]string{
grantee.String(),
Expand Down Expand Up @@ -660,7 +659,7 @@ func (s *IntegrationTestSuite) TestNewExecGrantAuthorized() {
grantee1 := s.grantee[2]
twoHours := time.Now().Add(time.Minute * time.Duration(120)).Unix()

_, err := ExecGrant(
_, err := CreateGrant(
val,
[]string{
grantee.String(),
Expand Down Expand Up @@ -764,7 +763,7 @@ func (s *IntegrationTestSuite) TestExecDelegateAuthorization() {
grantee := s.grantee[0]
twoHours := time.Now().Add(time.Minute * time.Duration(120)).Unix()

_, err := ExecGrant(
_, err := CreateGrant(
val,
[]string{
grantee.String(),
Expand Down Expand Up @@ -856,7 +855,7 @@ func (s *IntegrationTestSuite) TestExecDelegateAuthorization() {
}

// test delegate no spend-limit
_, err = ExecGrant(
_, err = CreateGrant(
val,
[]string{
grantee.String(),
Expand Down Expand Up @@ -933,7 +932,7 @@ func (s *IntegrationTestSuite) TestExecDelegateAuthorization() {
}

// test delegating to denied validator
_, err = ExecGrant(
_, err = CreateGrant(
val,
[]string{
grantee.String(),
Expand Down Expand Up @@ -968,7 +967,7 @@ func (s *IntegrationTestSuite) TestExecUndelegateAuthorization() {
twoHours := time.Now().Add(time.Minute * time.Duration(120)).Unix()

// granting undelegate msg authorization
_, err := ExecGrant(
_, err := CreateGrant(
val,
[]string{
grantee.String(),
Expand Down Expand Up @@ -1077,7 +1076,7 @@ func (s *IntegrationTestSuite) TestExecUndelegateAuthorization() {
}

// grant undelegate authorization without limit
_, err = ExecGrant(
_, err = CreateGrant(
val,
[]string{
grantee.String(),
Expand Down
8 changes: 7 additions & 1 deletion x/authz/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ func (k Keeper) DispatchActions(ctx sdk.Context, grantee sdk.AccAddress, msgs []
func (k Keeper) SaveGrant(ctx sdk.Context, grantee, granter sdk.AccAddress, authorization authz.Authorization, expiration time.Time) error {
store := ctx.KVStore(k.storeKey)

grant, err := authz.NewGrant(authorization, expiration)
grant, err := authz.NewGrant(ctx.BlockTime(), authorization, expiration)
if err != nil {
return err
}
Expand Down Expand Up @@ -237,14 +237,20 @@ func (k Keeper) ExportGenesis(ctx sdk.Context) *authz.GenesisState {
// InitGenesis new authz genesis
func (k Keeper) InitGenesis(ctx sdk.Context, data *authz.GenesisState) {
for _, entry := range data.Authorization {
if entry.Expiration.Before(ctx.BlockTime()) {
continue
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we continue here? I think we should return an error (or panic since we can't return).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking more about. It's OK to continue - the grant will be valid but it will be pruned immediately... I would issue warning though.


grantee, err := sdk.AccAddressFromBech32(entry.Grantee)
if err != nil {
panic(err)
}

granter, err := sdk.AccAddressFromBech32(entry.Granter)
if err != nil {
panic(err)
}

a, ok := entry.Authorization.GetCachedValue().(authz.Authorization)
if !ok {
panic("expected authorization")
Expand Down
18 changes: 7 additions & 11 deletions x/authz/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,12 @@ func (s *TestSuite) TestKeeper() {
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)
s.Require().Error(err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh wow, it seems that tests were incorrect

authorization, _ = app.AuthzKeeper.GetCleanAuthorization(ctx, granteeAddr, granterAddr, bankSendAuthMsgType)
s.Require().Nil(authorization)

Expand Down Expand Up @@ -105,14 +104,13 @@ func (s *TestSuite) TestKeeperIter() {
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)
now := s.ctx.BlockHeader().Time.Add(time.Second)

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)
s.Require().Error(err)
authorization, _ = app.AuthzKeeper.GetCleanAuthorization(ctx, granteeAddr, granterAddr, "abcd")
s.Require().Nil(authorization)

Expand All @@ -131,8 +129,7 @@ func (s *TestSuite) TestKeeperFees() {
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)
expiration := s.ctx.BlockHeader().Time.Add(1 * time.Second)

smallCoin := sdk.NewCoins(sdk.NewInt64Coin("steak", 20))
someCoin := sdk.NewCoins(sdk.NewInt64Coin("steak", 123))
Expand All @@ -157,7 +154,7 @@ func (s *TestSuite) TestKeeperFees() {

s.T().Log("verify dispatch executes with correct information")
// 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: smallCoin}, expiration)
s.Require().NoError(err)
authorization, _ := app.AuthzKeeper.GetCleanAuthorization(s.ctx, granteeAddr, granterAddr, bankSendAuthMsgType)
s.Require().NotNil(authorization)
Expand Down Expand Up @@ -206,8 +203,7 @@ func (s *TestSuite) TestDispatchedEvents() {
granteeAddr := addrs[1]
recipientAddr := addrs[2]
require.NoError(testutil.FundAccount(app.BankKeeper, s.ctx, granterAddr, sdk.NewCoins(sdk.NewInt64Coin("steak", 10000))))
now := s.ctx.BlockHeader().Time
require.NotNil(now)
expiration := s.ctx.BlockHeader().Time.Add(1 * time.Second) // must be in the future

smallCoin := sdk.NewCoins(sdk.NewInt64Coin("steak", 20))
msgs := authz.NewMsgExec(granteeAddr, []sdk.Msg{
Expand All @@ -219,7 +215,7 @@ func (s *TestSuite) TestDispatchedEvents() {
})

// 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: smallCoin}, expiration)
require.NoError(err)
authorization, _ := app.AuthzKeeper.GetCleanAuthorization(s.ctx, granteeAddr, granterAddr, bankSendAuthMsgType)
require.NotNil(authorization)
Expand Down
2 changes: 1 addition & 1 deletion x/authz/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (

var _ authz.MsgServer = Keeper{}

// GrantAuthorization implements the MsgServer.Grant method.
// GrantAuthorization 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)
Expand Down
Loading