diff --git a/x/circuit/keeper/msg_server.go b/x/circuit/keeper/msg_server.go index 91bbca507a..6fda4ed3a8 100644 --- a/x/circuit/keeper/msg_server.go +++ b/x/circuit/keeper/msg_server.go @@ -86,51 +86,33 @@ func (srv msgServer) TripCircuitBreaker(ctx context.Context, msg *types.MsgTripC return nil, err } - switch { - case perms.Level == types.Permissions_LEVEL_SUPER_ADMIN || perms.Level == types.Permissions_LEVEL_ALL_MSGS || bytes.Equal(address, srv.GetAuthority()): - for _, msgTypeURL := range msg.MsgTypeUrls { - // check if the message is in the list of allowed messages - isAllowed, err := srv.IsAllowed(ctx, msgTypeURL) - if err != nil { - return nil, err - } - - if !isAllowed { - return nil, fmt.Errorf("message %s is already disabled", msgTypeURL) - } - - if err = srv.DisableList.Set(ctx, msgTypeURL); err != nil { - return nil, err - } - + for _, msgTypeURL := range msg.MsgTypeUrls { + // check if the message is in the list of allowed messages + isAllowed, err := srv.IsAllowed(ctx, msgTypeURL) + if err != nil { + return nil, err } - case perms.Level == types.Permissions_LEVEL_SOME_MSGS: - for _, msgTypeURL := range msg.MsgTypeUrls { - // check if the message is in the list of allowed messages - isAllowed, err := srv.IsAllowed(ctx, msgTypeURL) - if err != nil { - return nil, err - } - if !isAllowed { - return nil, fmt.Errorf("message %s is already disabled", msgTypeURL) - } - permExists := false - for _, msgurl := range perms.LimitTypeUrls { - if msgTypeURL == msgurl { - permExists = true - } - } + if !isAllowed { + return nil, fmt.Errorf("message %s is already disabled", msgTypeURL) + } - if !permExists { + switch { + case perms.Level == types.Permissions_LEVEL_SUPER_ADMIN || perms.Level == types.Permissions_LEVEL_ALL_MSGS || bytes.Equal(address, srv.GetAuthority()): + // if the sender is a super admin or the module authority, no need to check perms + case perms.Level == types.Permissions_LEVEL_SOME_MSGS: + // if the sender has permission for some messages, check if the sender has permission for this specific message + if !hasPermissionForMsg(perms, msgTypeURL) { return nil, errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "account does not have permission to trip circuit breaker for message %s", msgTypeURL) } - if err = srv.DisableList.Set(ctx, msgTypeURL); err != nil { - return nil, err - } + default: + return nil, errorsmod.Wrap(sdkerrors.ErrUnauthorized, "account does not have permission to trip circuit breaker") } - default: - return nil, errorsmod.Wrap(sdkerrors.ErrUnauthorized, "account does not have permission to trip circuit breaker") + + if err = srv.DisableList.Set(ctx, msgTypeURL); err != nil { + return nil, err + } + } urls := strings.Join(msg.GetMsgTypeUrls(), ",") @@ -164,24 +146,32 @@ func (srv msgServer) ResetCircuitBreaker(ctx context.Context, msg *types.MsgRese return nil, err } - if perms.Level == types.Permissions_LEVEL_SUPER_ADMIN || perms.Level == types.Permissions_LEVEL_ALL_MSGS || perms.Level == types.Permissions_LEVEL_SOME_MSGS || bytes.Equal(address, srv.GetAuthority()) { - // add all msg type urls to the disable list - for _, msgTypeURL := range msg.MsgTypeUrls { - isAllowed, err := srv.IsAllowed(ctx, msgTypeURL) - if err != nil { - return nil, err - } - - if isAllowed { - return nil, fmt.Errorf("message %s is not disabled", msgTypeURL) - } - - if err = srv.DisableList.Remove(ctx, msgTypeURL); err != nil { - return nil, err - } + for _, msgTypeURL := range msg.MsgTypeUrls { + // check if the message is in the list of allowed messages + isAllowed, err := srv.IsAllowed(ctx, msgTypeURL) + if err != nil { + return nil, err + } + + if isAllowed { + return nil, fmt.Errorf("message %s is not disabled", msgTypeURL) + } + + switch { + case perms.Level == types.Permissions_LEVEL_SUPER_ADMIN || perms.Level == types.Permissions_LEVEL_ALL_MSGS || bytes.Equal(address, srv.GetAuthority()): + // if the sender is a super admin or the module authority, no need to check perms + case perms.Level == types.Permissions_LEVEL_SOME_MSGS: + // if the sender has permission for some messages, check if the sender has permission for this specific message + if !hasPermissionForMsg(perms, msgTypeURL) { + return nil, errorsmod.Wrapf(sdkerrors.ErrUnauthorized, "account does not have permission to reset circuit breaker for message %s", msgTypeURL) + } + default: + return nil, errorsmod.Wrap(sdkerrors.ErrUnauthorized, "account does not have permission to reset circuit breaker") + } + + if err = srv.DisableList.Remove(ctx, msgTypeURL); err != nil { + return nil, err } - } else { - return nil, errorsmod.Wrap(sdkerrors.ErrUnauthorized, "account does not have permission to reset circuit breaker") } urls := strings.Join(msg.GetMsgTypeUrls(), ",") @@ -197,3 +187,13 @@ func (srv msgServer) ResetCircuitBreaker(ctx context.Context, msg *types.MsgRese return &types.MsgResetCircuitBreakerResponse{Success: true}, nil } + +// hasPermissionForMsg returns true if the account can trip or reset the message. +func hasPermissionForMsg(perms types.Permissions, msg string) bool { + for _, msgurl := range perms.LimitTypeUrls { + if msg == msgurl { + return true + } + } + return false +} diff --git a/x/circuit/keeper/msg_server_test.go b/x/circuit/keeper/msg_server_test.go index fff87ebf4b..0e94be0d61 100644 --- a/x/circuit/keeper/msg_server_test.go +++ b/x/circuit/keeper/msg_server_test.go @@ -194,7 +194,7 @@ func TestTripCircuitBreaker(t *testing.T) { require.ErrorContains(t, err, "MsgEditValidator: unauthorized") // user tries to trip an already tripped circuit breaker - alreadyTripped := "cosmos.bank.v1beta1.MsgSend" + alreadyTripped := msgSend twoTrip := &types.MsgTripCircuitBreaker{Authority: addresses[1], MsgTypeUrls: []string{alreadyTripped}} _, err = srv.TripCircuitBreaker(ft.ctx, twoTrip) require.ErrorContains(t, err, "already disabled") @@ -208,7 +208,7 @@ func TestResetCircuitBreaker(t *testing.T) { srv := keeper.NewMsgServerImpl(ft.keeper) // admin resets circuit breaker - url := "cosmos.bank.v1beta1.MsgSend" + url := msgSend // admin trips circuit breaker admintrip := &types.MsgTripCircuitBreaker{Authority: authority, MsgTypeUrls: []string{url}} _, err = srv.TripCircuitBreaker(ft.ctx, admintrip) @@ -291,7 +291,7 @@ func TestResetCircuitBreaker(t *testing.T) { require.NoError(t, err) // user with some messages resets circuit breaker - someMsgsReset := &types.MsgResetCircuitBreaker{Authority: addresses[2], MsgTypeUrls: []string{url}} + someMsgsReset := &types.MsgResetCircuitBreaker{Authority: addresses[2], MsgTypeUrls: []string{url2}} _, err = srv.ResetCircuitBreaker(ft.ctx, someMsgsReset) require.NoError(t, err) require.Equal( @@ -299,14 +299,14 @@ func TestResetCircuitBreaker(t *testing.T) { sdk.NewEvent( "reset_circuit_breaker", sdk.NewAttribute("authority", addresses[2]), - sdk.NewAttribute("msg_url", url), + sdk.NewAttribute("msg_url", url2), ), lastEvent(ft.ctx), ) // user tries to reset an already reset circuit breaker - admintrip = &types.MsgTripCircuitBreaker{Authority: addresses[1], MsgTypeUrls: []string{url2}} - _, err = srv.TripCircuitBreaker(ft.ctx, admintrip) + someMsgsReset = &types.MsgResetCircuitBreaker{Authority: addresses[1], MsgTypeUrls: []string{url2}} + _, err = srv.ResetCircuitBreaker(ft.ctx, someMsgsReset) require.Error(t, err) } @@ -316,3 +316,60 @@ func lastEvent(ctx context.Context) sdk.Event { return events[len(events)-1] } + +func TestResetCircuitBreakerSomeMsgs(t *testing.T) { + ft := initFixture(t) + authority, err := ft.ac.BytesToString(ft.mockAddr) + require.NoError(t, err) + + srv := keeper.NewMsgServerImpl(ft.keeper) + + // admin resets circuit breaker + url := msgSend + url2 := "the_only_message_acc2_can_trip_and_reset" + + // add acc2 as an authorized account for only url2 + authmsg := &types.MsgAuthorizeCircuitBreaker{ + Granter: authority, + Grantee: addresses[2], + Permissions: &types.Permissions{ + Level: types.Permissions_LEVEL_SOME_MSGS, + LimitTypeUrls: []string{url2}, + }, + } + _, err = srv.AuthorizeCircuitBreaker(ft.ctx, authmsg) + require.NoError(t, err) + + // admin trips circuit breaker + admintrip := &types.MsgTripCircuitBreaker{Authority: authority, MsgTypeUrls: []string{url, url2}} + _, err = srv.TripCircuitBreaker(ft.ctx, admintrip) + require.NoError(t, err) + + // sanity check, both messages should be tripped + allowed, err := ft.keeper.IsAllowed(ft.ctx, url) + require.NoError(t, err) + require.False(t, allowed, "circuit breaker should be tripped") + + allowed, err = ft.keeper.IsAllowed(ft.ctx, url2) + require.NoError(t, err) + require.False(t, allowed, "circuit breaker should be tripped") + + // now let's try to reset url using acc2 (should fail) + acc2Reset := &types.MsgResetCircuitBreaker{Authority: addresses[2], MsgTypeUrls: []string{url}} + _, err = srv.ResetCircuitBreaker(ft.ctx, acc2Reset) + require.Error(t, err) + + // now let's try to reset url2 using acc2 (should pass) + acc2Reset = &types.MsgResetCircuitBreaker{Authority: addresses[2], MsgTypeUrls: []string{url2}} + _, err = srv.ResetCircuitBreaker(ft.ctx, acc2Reset) + require.NoError(t, err) + + // Only url2 should be reset, url should still be tripped + allowed, err = ft.keeper.IsAllowed(ft.ctx, url) + require.NoError(t, err) + require.False(t, allowed, "circuit breaker should be tripped") + + allowed, err = ft.keeper.IsAllowed(ft.ctx, url2) + require.NoError(t, err) + require.True(t, allowed, "circuit breaker should be reset") +}