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

feat(x/circuit): Add validation for permission when an account is assigned and validation for msgURL #22460

Merged
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions x/circuit/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
77 changes: 77 additions & 0 deletions x/circuit/keeper/msg_server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Comment on lines +160 to +165
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error message assertion for SOME_MSGS validation.

The test should verify the specific error message when SOME_MSGS permission is granted with empty LimitTypeUrls.

 	msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[3], Permissions: &somemsgs}
 	_, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg)
-	require.Error(t, err)
+	require.ErrorContains(t, err, "limit_type_urls cannot be empty for permission level SOME_MSGS")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// 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
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.ErrorContains(t, err, "limit_type_urls cannot be empty for permission level SOME_MSGS")

// 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)

Expand Down
34 changes: 34 additions & 0 deletions x/circuit/types/permission.go
Original file line number Diff line number Diff line change
@@ -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
}
Loading