diff --git a/x/circuit/CHANGELOG.md b/x/circuit/CHANGELOG.md index c88e2e50e21c..66d6a9275f89 100644 --- a/x/circuit/CHANGELOG.md +++ b/x/circuit/CHANGELOG.md @@ -41,5 +41,6 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (deps) [#19810](https://github.com/cosmos/cosmos-sdk/pull/19810) Upgrade SDK version due to Prometheus breaking change. * (deps) [#19810](https://github.com/cosmos/cosmos-sdk/pull/19810) Bump `cosmossdk.io/store` to v1.1.0. +* (feat) [#22460](https://github.com/cosmos/cosmos-sdk/pull/22460) Add validation for permission when an account is assigned and validation for msgURL ## [v0.1.0](https://github.com/cosmos/cosmos-sdk/releases/tag/x/circuit/v0.1.0) - 2023-11-07 diff --git a/x/circuit/keeper/msg_server.go b/x/circuit/keeper/msg_server.go index 974d682ecc35..356c34620d62 100644 --- a/x/circuit/keeper/msg_server.go +++ b/x/circuit/keeper/msg_server.go @@ -58,6 +58,11 @@ func (srv msgServer) AuthorizeCircuitBreaker(ctx context.Context, msg *types.Msg return nil, errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "permissions cannot be nil") } + err = msg.Permissions.Validation() + if err != nil { + return nil, err + } + // Append the account in the msg to the store's set of authorized super admins if err = srv.Permissions.Set(ctx, grantee, *msg.Permissions); err != nil { return nil, err @@ -89,7 +94,8 @@ func (srv msgServer) TripCircuitBreaker(ctx context.Context, msg *types.MsgTripC return nil, err } - for _, msgTypeURL := range msg.MsgTypeUrls { + msgTypeUrls := types.MsgTypeURLValidation(msg.MsgTypeUrls) + for _, msgTypeURL := range msgTypeUrls { // check if the message is in the list of allowed messages isAllowed, err := srv.IsAllowed(ctx, msgTypeURL) if err != nil { @@ -148,7 +154,8 @@ func (srv msgServer) ResetCircuitBreaker(ctx context.Context, msg *types.MsgRese return nil, err } - for _, msgTypeURL := range msg.MsgTypeUrls { + msgTypeUrls := types.MsgTypeURLValidation(msg.MsgTypeUrls) + for _, msgTypeURL := range msgTypeUrls { // check if the message is in the list of allowed messages isAllowed, err := srv.IsAllowed(ctx, msgTypeURL) if err != nil { diff --git a/x/circuit/keeper/msg_server_test.go b/x/circuit/keeper/msg_server_test.go index 0e94be0d61e2..e5fda2d19c17 100644 --- a/x/circuit/keeper/msg_server_test.go +++ b/x/circuit/keeper/msg_server_test.go @@ -13,7 +13,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" ) -const msgSend = "cosmos.bank.v1beta1.MsgSend" +const msgSend = "/cosmos.bank.v1beta1.MsgSend" func TestAuthorizeCircuitBreaker(t *testing.T) { ft := initFixture(t) @@ -111,6 +111,83 @@ func TestAuthorizeCircuitBreaker(t *testing.T) { require.NoError(t, err) } +func TestAuthorizeCircuitBreakerWithPermissionValidation(t *testing.T) { + ft := initFixture(t) + + srv := keeper.NewMsgServerImpl(ft.keeper) + authority, err := ft.ac.BytesToString(ft.mockAddr) + require.NoError(t, err) + + // successfully add a new super admin with LimitTypeUrls not empty + adminPerms := types.Permissions{Level: types.Permissions_LEVEL_SUPER_ADMIN, LimitTypeUrls: []string{"cosmos.staking.v1beta1.MsgDelegate"}} + msg := &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[1], Permissions: &adminPerms} + _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) + require.NoError(t, err) + + add1, err := ft.ac.StringToBytes(addresses[1]) + require.NoError(t, err) + + perms, err := ft.keeper.Permissions.Get(ft.ctx, add1) + require.NoError(t, err) + // LimitTypeUrls should be empty + require.Equal(t, len(perms.LimitTypeUrls), 0) + + // successfully add a new super user with LimitTypeUrls not empty + allmsgs := types.Permissions{Level: types.Permissions_LEVEL_ALL_MSGS, LimitTypeUrls: []string{"cosmos.staking.v1beta1.MsgDelegate"}} + msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[2], Permissions: &allmsgs} + _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) + require.NoError(t, err) + require.Equal( + t, + sdk.NewEvent( + "authorize_circuit_breaker", + sdk.NewAttribute("granter", authority), + sdk.NewAttribute("grantee", addresses[2]), + sdk.NewAttribute("permission", allmsgs.String()), + ), + lastEvent(ft.ctx), + ) + + add2, err := ft.ac.StringToBytes(addresses[2]) + require.NoError(t, err) + + perms, err = ft.keeper.Permissions.Get(ft.ctx, add2) + require.NoError(t, err) + + // LimitTypeUrls should be empty + require.Equal(t, len(perms.LimitTypeUrls), 0) + + // grants user perms to Permissions_LEVEL_SOME_MSGS with empty LimitTypeUrls + somemsgs := types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{}} + msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[3], Permissions: &somemsgs} + _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) + require.Error(t, err) + + // grants user perms to Permissions_LEVEL_SOME_MSGS with empty LimitTypeUrls + permis := types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{"cosmos.staking.v1beta1.MsgDelegate", "/cosmos.gov.v1beta1.MsgDeposit", "cosmos.gov.v1beta1.MsgVote"}} + msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[4], Permissions: &permis} + _, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg) + require.NoError(t, err) + require.Equal( + t, + sdk.NewEvent( + "authorize_circuit_breaker", + sdk.NewAttribute("granter", authority), + sdk.NewAttribute("grantee", addresses[4]), + sdk.NewAttribute("permission", permis.String()), + ), + lastEvent(ft.ctx), + ) + + add4, err := ft.ac.StringToBytes(addresses[4]) + require.NoError(t, err) + + perms, err = ft.keeper.Permissions.Get(ft.ctx, add4) + require.NoError(t, err) + + require.Equal(t, []string{"/cosmos.staking.v1beta1.MsgDelegate", "/cosmos.gov.v1beta1.MsgDeposit", "/cosmos.gov.v1beta1.MsgVote"}, perms.LimitTypeUrls) +} + func TestTripCircuitBreaker(t *testing.T) { ft := initFixture(t) @@ -158,7 +235,7 @@ func TestTripCircuitBreaker(t *testing.T) { require.NoError(t, err) // try to trip the circuit breaker - url2 = "cosmos.staking.v1beta1.MsgDelegate" + url2 = "/cosmos.staking.v1beta1.MsgDelegate" superTrip := &types.MsgTripCircuitBreaker{Authority: addresses[1], MsgTypeUrls: []string{url2}} _, err = srv.TripCircuitBreaker(ft.ctx, superTrip) require.NoError(t, err) @@ -259,7 +336,7 @@ func TestResetCircuitBreaker(t *testing.T) { require.NoError(t, err) // trip the circuit breaker - url2 := "cosmos.staking.v1beta1.MsgDelegate" + url2 := "/cosmos.staking.v1beta1.MsgDelegate" admintrip = &types.MsgTripCircuitBreaker{Authority: authority, MsgTypeUrls: []string{url2}} _, err = srv.TripCircuitBreaker(ft.ctx, admintrip) require.NoError(t, err) @@ -279,7 +356,7 @@ func TestResetCircuitBreaker(t *testing.T) { ) // user tries to reset a message they dont have permission to reset - url = "cosmos.staking.v1beta1.MsgCreateValidator" + url = "/cosmos.staking.v1beta1.MsgCreateValidator" // give restricted perms to a user someMsgs := &types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{url2}} msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[2], Permissions: someMsgs} @@ -326,7 +403,7 @@ func TestResetCircuitBreakerSomeMsgs(t *testing.T) { // admin resets circuit breaker url := msgSend - url2 := "the_only_message_acc2_can_trip_and_reset" + url2 := "/the_only_message_acc2_can_trip_and_reset" // add acc2 as an authorized account for only url2 authmsg := &types.MsgAuthorizeCircuitBreaker{ diff --git a/x/circuit/types/permission.go b/x/circuit/types/permission.go new file mode 100644 index 000000000000..bac736066c2d --- /dev/null +++ b/x/circuit/types/permission.go @@ -0,0 +1,34 @@ +package types + +import "errors" + +func (p *Permissions) Validation() error { + switch { + case p.Level == Permissions_LEVEL_SOME_MSGS: + // if permission is some msg, LimitTypeUrls array must not be empty + if len(p.LimitTypeUrls) == 0 { + return errors.New("LimitTypeUrls of LEVEL_SOME_MSGS should NOT be empty") + } + + p.LimitTypeUrls = MsgTypeURLValidation(p.LimitTypeUrls) + case p.Level == Permissions_LEVEL_ALL_MSGS || p.Level == Permissions_LEVEL_SUPER_ADMIN: + // if permission is all msg or super addmin, LimitTypeUrls array clear + // all p.LimitTypeUrls since we not use this field + p.LimitTypeUrls = nil + default: + } + + return nil +} + +func MsgTypeURLValidation(urls []string) []string { + for idx, url := range urls { + if len(url) == 0 { + continue + } + if url[0] != '/' { + urls[idx] = "/" + url + } + } + return urls +}