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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 9 commits into from
Nov 7, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
db68f2d
add validation for permission
GNaD13 Nov 6, 2024
bda9cae
add validation for msg url
GNaD13 Nov 6, 2024
3bc3fe4
add more validation
GNaD13 Nov 6, 2024
6d3de88
nit
GNaD13 Nov 6, 2024
57c69c2
nit
GNaD13 Nov 6, 2024
47242f1
add change log
GNaD13 Nov 7, 2024
bd395b9
Merge remote-tracking branch 'origin/main' into add-validation-for-pe…
GNaD13 Nov 7, 2024
2b68c2e
nit
GNaD13 Nov 7, 2024
542997e
fix test
GNaD13 Nov 7, 2024
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
5 changes: 5 additions & 0 deletions x/circuit/keeper/msg_server.go
View file
Open in desktop
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
53 changes: 53 additions & 0 deletions x/circuit/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,59 @@ 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)
}
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

Enhance SOME_MSGS validation test coverage

The test should include:

  1. Verification of the specific error message
  2. A positive test case with non-empty LimitTypeUrls
 	// 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)
+	require.ErrorContains(t, err, "limit_type_urls cannot be empty for permission level SOME_MSGS")
+
+	// grants user perms to Permissions_LEVEL_SOME_MSGS with valid LimitTypeUrls
+	somemsgs = types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{"cosmos.staking.v1beta1.MsgDelegate"}}
+	msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[3], Permissions: &somemsgs}
+	_, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg)
+	require.NoError(t, err)
+
+	add3, err := ft.ac.StringToBytes(addresses[3])
+	require.NoError(t, err)
+
+	perms, err = ft.keeper.Permissions.Get(ft.ctx, add3)
+	require.NoError(t, err)
+	require.Equal(t, types.Permissions_LEVEL_SOME_MSGS, perms.Level)
+	require.Equal(t, []string{"cosmos.staking.v1beta1.MsgDelegate"}, perms.LimitTypeUrls)
📝 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 valid LimitTypeUrls
somemsgs = types.Permissions{Level: types.Permissions_LEVEL_SOME_MSGS, LimitTypeUrls: []string{"cosmos.staking.v1beta1.MsgDelegate"}}
msg = &types.MsgAuthorizeCircuitBreaker{Granter: authority, Grantee: addresses[3], Permissions: &somemsgs}
_, err = srv.AuthorizeCircuitBreaker(ft.ctx, msg)
require.NoError(t, err)
add3, err := ft.ac.StringToBytes(addresses[3])
require.NoError(t, err)
perms, err = ft.keeper.Permissions.Get(ft.ctx, add3)
require.NoError(t, err)
require.Equal(t, types.Permissions_LEVEL_SOME_MSGS, perms.Level)
require.Equal(t, []string{"cosmos.staking.v1beta1.MsgDelegate"}, perms.LimitTypeUrls)
}


func TestTripCircuitBreaker(t *testing.T) {
ft := initFixture(t)

Expand Down
20 changes: 20 additions & 0 deletions x/circuit/types/permission.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
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")
}
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
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add validation for unknown permission levels

The default case should return an error for unknown permission levels instead of silently succeeding. This ensures proper error handling for all possible cases.

Apply this diff:

 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")
 		}
 	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 errors.New("unknown permission level")
 	}

 	return nil
 }
📝 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
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")
}
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 (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")
}
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 errors.New("unknown permission level")
}
return nil
}

Loading