feat: enforce msg restriction rules #1410
Conversation
x/permission/exported/proto.go
Outdated
| _, d := descriptor.ForMessage(message) | ||
| v, err := proto.GetExtension(d.GetOptions(), E_PermissionRole) | ||
| if err != nil { | ||
| return ROLE_CHAIN_MANAGEMENT |
There was a problem hiding this comment.
I thought we want to give the strictest role by default, which is ROLE_ACCESS_CONTROL or in other words governance?
There was a problem hiding this comment.
When I asked I was told ROLE_CHAIN_MANAGEMENT was the strictest. So I'm fine either way, just need to know which 😆
There was a problem hiding this comment.
Oh, yeah, access control is the strictest one. Actually, this won't work, right? The AnteHandle is applied to Cosmos SDK/IBC messages too which don't have the descriptor set. So, this means that a user would need governance approval for a simple MsgSend 😆? If so, let's just leave it unrestricted by default and rely on our tests to catch unspecified roles for our messages perhaps?
| @@ -69,7 +69,7 @@ func (k Keeper) GetGovernanceKey(ctx sdk.Context) (multisig.LegacyAminoPubKey, b | |||
| func (k Keeper) GetRole(ctx sdk.Context, address sdk.AccAddress) exported.Role { | |||
| govAccount, ok := k.getGovAccount(ctx, address) | |||
| if !ok { | |||
| return exported.ROLE_UNSPECIFIED | |||
| return exported.ROLE_UNRESTRICTED | |||
There was a problem hiding this comment.
Why not return Unspecified if it doesn't exist?
There was a problem hiding this comment.
I need to be able between the case where it is missing and the case where it is purposefully set to be unrestricted to enforce the option
| @@ -69,7 +69,7 @@ func (k Keeper) GetGovernanceKey(ctx sdk.Context) (multisig.LegacyAminoPubKey, b | |||
| func (k Keeper) GetRole(ctx sdk.Context, address sdk.AccAddress) exported.Role { | |||
| govAccount, ok := k.getGovAccount(ctx, address) | |||
| if !ok { | |||
| return exported.ROLE_UNSPECIFIED | |||
| return exported.ROLE_UNRESTRICTED | |||
| role := exported.GetPermissionRole(msg.(descriptor.Message)) | ||
| if role == exported.ROLE_UNSPECIFIED { | ||
| missingRoles = append(missingRoles, implementation) | ||
| continue |
There was a problem hiding this comment.
within this loop, there is no other instruction after this one regardless of the executed flow. Do we really need it here?
There was a problem hiding this comment.
ah there used to be more afterwards that was deleted. You are right, it's not needed anymore
| When("msg role is unrestricted", msgRoleIsUnrestricted). | ||
| And().When("signer has any role", signerAnyRole). | ||
| Then("let the msg through", letTxThrough), | ||
|
|
||
| When("msg role is unspecified", msgRoleIsUnspecified). | ||
| And().When("signer has any role", signerAnyRole). | ||
| Then("let the msg through", letTxThrough), |
There was a problem hiding this comment.
what is the actual difference between an unspecified and an unrestricted node? Seems like we treat both types the same
There was a problem hiding this comment.
unspecified: the option is not set on the message (important for cosmos sdk and ibc messages)
unrestricted: explicitly set option to let anyone execute it
| When("msg role is chain management", msgRoleIsChainManagement). | ||
| And().When("signer is not chain management", signerIsNot(exported.ROLE_CHAIN_MANAGEMENT)). | ||
| Then("stop tx", stopTx), |
There was a problem hiding this comment.
what if the type is accesss control?
There was a problem hiding this comment.
the previous behaviour was that you need to match the role exactly and that's what I implemented here as well
Description
Instead of adding restriction rules to the ante handler, these rules are now directly defined in the proto definition of msgs. An additional test case checks that all msgs have defined roles.