Skip to content

Commit

Permalink
[1658]: Prevent an admin from revoking their own ability to manage pe…
Browse files Browse the repository at this point in the history
…rmissions.
  • Loading branch information
SpicyLemon committed Oct 10, 2023
1 parent 1863bef commit d22f41a
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 0 deletions.
7 changes: 7 additions & 0 deletions x/exchange/msg.go
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,9 @@ func (m MsgMarketManagePermissionsRequest) ValidateBasic() error {
errs = append(errs, fmt.Errorf("invalid revoke-all address %q: %w", addrStr, err))
}
}
if ContainsString(m.RevokeAll, m.Admin) {
errs = append(errs, fmt.Errorf("message administrator %s cannot revoke all of their permissions", m.Admin))
}

if err := ValidateAccessGrantsField("to-revoke", m.ToRevoke); err != nil {
errs = append(errs, err)
Expand All @@ -317,6 +320,10 @@ func (m MsgMarketManagePermissionsRequest) ValidateBasic() error {
if ContainsString(m.RevokeAll, ag.Address) {
errs = append(errs, fmt.Errorf("address %s appears in both the revoke-all and to-revoke fields", ag.Address))
}
if ag.Address == m.Admin && ag.Contains(Permission_permissions) {
errs = append(errs, fmt.Errorf("message administrator %s cannot revoke their own ability to manage permissions",

This comment has been minimized.

Copy link
@iramiller

iramiller Oct 10, 2023

Member

There are useful reasons for an admin to remove themselves from the permission list... for example when they transfer the permissions to someone else. So long at these permissions can be set via governance then they should be removable...

This comment has been minimized.

Copy link
@SpicyLemon

SpicyLemon Oct 12, 2023

Author Contributor

Gotcha. I took these checks out.

ag.Address))
}
toRevokeByAddr[ag.Address] = ag
}

Expand Down
37 changes: 37 additions & 0 deletions x/exchange/msg_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1317,6 +1317,43 @@ func TestMsgMarketManagePermissionsRequest_ValidateBasic(t *testing.T) {
"address " + goodAddr2 + " has both revoke and grant \"permissions\"",
},
},
{
name: "admin in revoke-all",
msg: MsgMarketManagePermissionsRequest{
Admin: goodAdminAddr,
MarketId: 1,
RevokeAll: []string{goodAddr1, goodAdminAddr, goodAddr2},
},
expErr: []string{"message administrator " + goodAdminAddr + " cannot revoke all of their permissions"},
},
{
name: "admin revoking all their permissions except permissions",
msg: MsgMarketManagePermissionsRequest{
Admin: goodAdminAddr,
MarketId: 1,
ToRevoke: []AccessGrant{
{
Address: goodAdminAddr,
Permissions: []Permission{Permission_settle, Permission_cancel,
Permission_withdraw, Permission_update, Permission_attributes},
},
},
},
expErr: nil,
},
{
name: "admin revoking own ability to manage permissions",
msg: MsgMarketManagePermissionsRequest{
Admin: goodAdminAddr,
MarketId: 1,
ToRevoke: []AccessGrant{
{Address: goodAddr1, Permissions: []Permission{Permission_permissions}},
{Address: goodAdminAddr, Permissions: []Permission{Permission_permissions}},
{Address: goodAddr2, Permissions: []Permission{Permission_permissions}},
},
},
expErr: []string{"message administrator " + goodAdminAddr + " cannot revoke their own ability to manage permissions"},
},
{
name: "multiple errs",
msg: MsgMarketManagePermissionsRequest{
Expand Down

0 comments on commit d22f41a

Please sign in to comment.