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 #10447

Merged
merged 27 commits into from Feb 2, 2022
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
c0f3519
Merge pull request from GHSA-2p6r-37p9-89p2
robert-zaremba Oct 12, 2021
a880e95
master update
robert-zaremba Oct 12, 2021
f8c86ca
update changelog and cli test
robert-zaremba Oct 12, 2021
9825643
fix keeper tests
robert-zaremba Oct 12, 2021
972a7b8
fix decoder test
robert-zaremba Oct 12, 2021
f2799e1
cleaning
robert-zaremba Oct 13, 2021
2dd1818
wip - tests
robert-zaremba Oct 13, 2021
bed10a3
fix cli test
robert-zaremba Oct 13, 2021
44acf3f
add a comment
robert-zaremba Oct 13, 2021
5b1a54f
update TestMsgGrantAuthorization
robert-zaremba Oct 13, 2021
db3eeb7
Merge branch 'master' into robert/backport-0.44.2-master
robert-zaremba Oct 27, 2021
3bf3409
udpate changelog
robert-zaremba Oct 27, 2021
43bff33
wip
robert-zaremba Nov 11, 2021
5fa72f7
Merge branch 'master' into robert/backport-0.44.2-master
robert-zaremba Jan 27, 2022
0dbe13d
rework validation
robert-zaremba Jan 27, 2022
a15df00
fix changelog conflicts
robert-zaremba Jan 27, 2022
f4e2231
Merge branch 'master' into robert/backport-0.44.2-master
robert-zaremba Jan 27, 2022
469644b
fix TestDecodeStore
robert-zaremba Jan 27, 2022
b81865a
fix sim tests
robert-zaremba Jan 27, 2022
123a420
update sim tests
robert-zaremba Jan 27, 2022
505ff43
Merge branch 'master' into robert/backport-0.44.2-master
robert-zaremba Jan 27, 2022
1fe683b
set expiration to autz grant genesis
robert-zaremba Jan 28, 2022
940e165
address comments
tac0turtle Feb 1, 2022
3b38eda
Merge branch 'master' into robert/backport-0.44.2-master
tac0turtle Feb 1, 2022
6166e7e
Merge branch 'master' into robert/backport-0.44.2-master
tac0turtle Feb 1, 2022
e0704a8
Merge branch 'master' into robert/backport-0.44.2-master
tac0turtle Feb 2, 2022
54c3bee
fix test
tac0turtle Feb 2, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -193,6 +193,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [#10770](https://github.com/cosmos/cosmos-sdk/pull/10770) revert tx when block gas limit exceeded
* [\#10868](https://github.com/cosmos/cosmos-sdk/pull/10868) Bump gov to v1beta2. Both v1beta1 and v1beta2 queries and Msgs are accepted.
* [\#11011](https://github.com/cosmos/cosmos-sdk/pull/11011) Remove burning of deposits when qourum is not reached on a governance proposal and when the deposit is not fully met.
* (x/authz) [\#10447](https://github.com/cosmos/cosmos-sdk/pull/10447) Remove time.now() check in authz `NewGrant` validation.

### Deprecated

Expand Down
14 changes: 6 additions & 8 deletions x/authz/authorization_grant.go
Expand Up @@ -9,7 +9,13 @@ import (
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
)

var (
_ cdctypes.UnpackInterfacesMessage = &Grant{}
)

// NewGrant returns new Grant
// NOTE: a new grant is considered invalid if the expiration time is after the
// "current" time - you should assure that before calling this function.
func NewGrant(a Authorization, expiration time.Time) (Grant, error) {
g := Grant{
Expiration: expiration,
Expand All @@ -28,10 +34,6 @@ func NewGrant(a Authorization, expiration time.Time) (Grant, error) {
return g, nil
}

var (
_ cdctypes.UnpackInterfacesMessage = &Grant{}
)

// UnpackInterfaces implements UnpackInterfacesMessage.UnpackInterfaces
func (g Grant) UnpackInterfaces(unpacker cdctypes.AnyUnpacker) error {
var authorization Authorization
Expand All @@ -51,10 +53,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")
}

robert-zaremba marked this conversation as resolved.
Show resolved Hide resolved
av := g.Authorization.GetCachedValue()
a, ok := av.(Authorization)
if !ok {
Expand Down
14 changes: 14 additions & 0 deletions x/authz/authorization_grant_test.go
@@ -0,0 +1,14 @@
package authz

import (
"testing"
"time"

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

func TestNewGrant(t *testing.T) {
a := NewGenericAuthorization("some-type")
_, err := NewGrant(a, time.Unix(10, 0))
require.NoError(t, err)
}
2 changes: 1 addition & 1 deletion x/authz/client/cli/tx.go
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
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
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
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) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this function name (in tests) was misleading. It essentially creates a new grant and executes that message (grant creation). It doesn't execute on existing grant.

cmd := cli.NewCmdGrantAuthorization()
clientCtx := val.ClientCtx
return clitestutil.ExecTestCLICmd(clientCtx, cmd, args)
Expand Down
35 changes: 17 additions & 18 deletions x/authz/client/testutil/tx.go
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 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
6 changes: 6 additions & 0 deletions x/authz/keeper/keeper.go
Expand Up @@ -136,6 +136,12 @@ func (k Keeper) DispatchActions(ctx sdk.Context, grantee sdk.AccAddress, msgs []
// same `sdk.Msg` type, this grant overwrites that.
func (k Keeper) SaveGrant(ctx sdk.Context, grantee, granter sdk.AccAddress, authorization authz.Authorization, expiration time.Time) error {
store := ctx.KVStore(k.storeKey)
blockTime := ctx.BlockTime()
if !expiration.After(blockTime) {
return sdkerrors.ErrInvalidRequest.Wrapf(
"expiration must be after the current block time (%v), got %v",
blockTime.Format(time.RFC3339), expiration.Format(time.RFC3339))
}

grant, err := authz.NewGrant(authorization, expiration)
if err != nil {
Expand Down
21 changes: 9 additions & 12 deletions x/authz/keeper/keeper_test.go
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/cosmos/cosmos-sdk/baseapp"
"github.com/cosmos/cosmos-sdk/simapp"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/errors"
"github.com/cosmos/cosmos-sdk/x/authz"
"github.com/cosmos/cosmos-sdk/x/bank/testutil"
banktypes "github.com/cosmos/cosmos-sdk/x/bank/types"
Expand Down Expand Up @@ -55,13 +56,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().ErrorIs(err, errors.ErrInvalidRequest)
authorization, _ = app.AuthzKeeper.GetCleanAuthorization(ctx, granteeAddr, granterAddr, bankSendAuthMsgType)
s.Require().Nil(authorization)

Expand All @@ -83,7 +83,7 @@ func (s *TestSuite) TestKeeper() {

s.T().Log("verify revoke fails with wrong information")
err = app.AuthzKeeper.DeleteGrant(ctx, recipientAddr, granterAddr, bankSendAuthMsgType)
s.Require().Error(err)
s.Require().ErrorIs(err, errors.ErrNotFound)
authorization, _ = app.AuthzKeeper.GetCleanAuthorization(ctx, recipientAddr, granterAddr, bankSendAuthMsgType)
s.Require().Nil(authorization)

Expand All @@ -105,14 +105,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 +130,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 +155,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 +204,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 +216,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
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
2 changes: 1 addition & 1 deletion x/authz/msgs_test.go
Expand Up @@ -80,7 +80,7 @@ func TestMsgGrantAuthorization(t *testing.T) {
{"nil granter and grantee address", nil, nil, &banktypes.SendAuthorization{SpendLimit: coinsPos}, time.Now(), false, false},
{"nil authorization", granter, grantee, nil, time.Now(), true, false},
{"valid test case", granter, grantee, &banktypes.SendAuthorization{SpendLimit: coinsPos}, time.Now().AddDate(0, 1, 0), false, true},
{"past time", granter, grantee, &banktypes.SendAuthorization{SpendLimit: coinsPos}, time.Now().AddDate(0, 0, -1), false, false},
{"past time", granter, grantee, &banktypes.SendAuthorization{SpendLimit: coinsPos}, time.Now().AddDate(0, 0, -1), true, true},
}
for i, tc := range tests {
msg, err := authz.NewMsgGrant(
Expand Down
3 changes: 2 additions & 1 deletion x/authz/simulation/decoder_test.go
Expand Up @@ -20,7 +20,8 @@ func TestDecodeStore(t *testing.T) {
cdc := simapp.MakeTestEncodingConfig().Codec
dec := simulation.NewDecodeStore(cdc)

grant, _ := authz.NewGrant(banktypes.NewSendAuthorization(sdk.NewCoins(sdk.NewInt64Coin("foo", 123))), time.Now().UTC())
grant, _ := authz.NewGrant(banktypes.NewSendAuthorization(sdk.NewCoins(sdk.NewInt64Coin("foo", 123))),
time.Now().Add(10*time.Minute).UTC())
grantBz, err := cdc.Marshal(&grant)
require.NoError(t, err)
kvPairs := kv.Pairs{
Expand Down