feat(x/circuit): Add validation for permission when an account is ass…

…igned and validation for msgURL (#22460)

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

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 {

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{

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
+}