-
Notifications
You must be signed in to change notification settings - Fork 48
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
## Description This PR adds permissions sanitation before storing the permissions while handling a `MsgSetUserGroupPermissions` or `MsgSetUserPermissions`. Special review required by @manu0466 since he was the one to report the bug (and suggest a solution). Closes: #800 --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/desmos-labs/desmos/blob/master/CONTRIBUTING.md#pr-targeting)) - [x] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://docs.cosmos.network/v0.44/building-modules/intro.html) - [x] included the necessary unit and integration [tests](https://github.com/desmos-labs/desmos/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [x] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)
- Loading branch information
Showing
11 changed files
with
411 additions
and
32 deletions.
There are no files selected for viewing
6 changes: 6 additions & 0 deletions
6
.changeset/entries/6e78e6f7f9486799935df288f3d1467a646116fddc2952e79e559b1d4cc58d79.yaml
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| type: fix | ||
| module: x/subspaces | ||
| pull_request: 801 | ||
| description: Added permissions sanitization | ||
| backward_compatible: true | ||
| date: 2022-04-01T09:09:14.889652713Z |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| package testutil | ||
|
|
||
| import ( | ||
| "github.com/cosmos/cosmos-sdk/store" | ||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| "github.com/tendermint/tendermint/libs/log" | ||
| tmproto "github.com/tendermint/tendermint/proto/tendermint/types" | ||
| dbm "github.com/tendermint/tm-db" | ||
| ) | ||
|
|
||
| func BuildContext( | ||
| keys map[string]*sdk.KVStoreKey, tKeys map[string]*sdk.TransientStoreKey, memKeys map[string]*sdk.MemoryStoreKey, | ||
| ) sdk.Context { | ||
| db := dbm.NewMemDB() | ||
| cms := store.NewCommitMultiStore(db) | ||
| for _, key := range keys { | ||
| cms.MountStoreWithDB(key, sdk.StoreTypeIAVL, db) | ||
| } | ||
| for _, tKey := range tKeys { | ||
| cms.MountStoreWithDB(tKey, sdk.StoreTypeTransient, db) | ||
| } | ||
| for _, memKey := range memKeys { | ||
| cms.MountStoreWithDB(memKey, sdk.StoreTypeMemory, nil) | ||
| } | ||
|
|
||
| err := cms.LoadLatestVersion() | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
|
|
||
| return sdk.NewContext(cms, tmproto.Header{}, false, log.NewNopLogger()) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| package keeper | ||
|
|
||
| import ( | ||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
|
|
||
| v2 "github.com/desmos-labs/desmos/v3/x/subspaces/legacy/v2" | ||
| ) | ||
|
|
||
| // DONTCOVER | ||
|
|
||
| // Migrator is a struct for handling in-place store migrations. | ||
| type Migrator struct { | ||
| keeper Keeper | ||
| } | ||
|
|
||
| // NewMigrator returns a new Migrator | ||
| func NewMigrator(keeper Keeper) Migrator { | ||
| return Migrator{ | ||
| keeper: keeper, | ||
| } | ||
| } | ||
|
|
||
| // Migrate1to2 migrates from version 1 to 2. | ||
| func (m Migrator) Migrate1to2(ctx sdk.Context) error { | ||
| return v2.MigrateStore(ctx, m.keeper.storeKey, m.keeper.cdc) | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| package v2 | ||
|
|
||
| import ( | ||
| "github.com/cosmos/cosmos-sdk/codec" | ||
| "github.com/cosmos/cosmos-sdk/store/prefix" | ||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
|
|
||
| "github.com/desmos-labs/desmos/v3/x/subspaces/types" | ||
| ) | ||
|
|
||
| func MigrateStore(ctx sdk.Context, storeKey sdk.StoreKey, cdc codec.BinaryCodec) error { | ||
| store := ctx.KVStore(storeKey) | ||
|
|
||
| err := fixGroupsPermissions(store, cdc) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| fixUsersPermissions(store) | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func fixGroupsPermissions(store sdk.KVStore, cdc codec.BinaryCodec) error { | ||
| groupsStore := prefix.NewStore(store, types.GroupsPrefix) | ||
| iterator := groupsStore.Iterator(nil, nil) | ||
|
|
||
| var groups []types.UserGroup | ||
| for ; iterator.Valid(); iterator.Next() { | ||
| var group types.UserGroup | ||
| err := cdc.Unmarshal(iterator.Value(), &group) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Sanitize the permissions | ||
| group.Permissions = types.SanitizePermission(group.Permissions) | ||
| groups = append(groups, group) | ||
| } | ||
|
|
||
| iterator.Close() | ||
|
|
||
| // Store the new groups | ||
| for i, group := range groups { | ||
| bz, err := cdc.Marshal(&groups[i]) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| store.Set(types.GroupStoreKey(group.SubspaceID, group.ID), bz) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| type userPermissionDetails struct { | ||
| subspaceID uint64 | ||
| user sdk.AccAddress | ||
| permissions types.Permission | ||
| } | ||
|
|
||
| func fixUsersPermissions(store sdk.KVStore) { | ||
| permissionsStore := prefix.NewStore(store, types.UserPermissionsStorePrefix) | ||
| iterator := permissionsStore.Iterator(nil, nil) | ||
|
|
||
| var permissions []userPermissionDetails | ||
| for ; iterator.Valid(); iterator.Next() { | ||
| // The first 8 bytes are the subspace id (uint64 takes up 8 bytes) | ||
| // The remaining bytes are the user address | ||
| subspaceBz, addressBz := iterator.Key()[:8], iterator.Key()[8:] | ||
|
|
||
| permissions = append(permissions, userPermissionDetails{ | ||
| subspaceID: types.GetSubspaceIDFromBytes(subspaceBz), | ||
| user: types.GetAddressBytes(addressBz), | ||
|
|
||
| // Sanitize the permission | ||
| permissions: types.SanitizePermission(types.UnmarshalPermission(iterator.Value())), | ||
| }) | ||
| } | ||
|
|
||
| iterator.Close() | ||
|
|
||
| // Store the new permissions | ||
| for _, entry := range permissions { | ||
| store.Set(types.UserPermissionStoreKey(entry.subspaceID, entry.user), types.MarshalPermission(entry.permissions)) | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| package v2_test | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| capabilitytypes "github.com/cosmos/cosmos-sdk/x/capability/types" | ||
| paramstypes "github.com/cosmos/cosmos-sdk/x/params/types" | ||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/desmos-labs/desmos/v3/app" | ||
| "github.com/desmos-labs/desmos/v3/testutil" | ||
| v2 "github.com/desmos-labs/desmos/v3/x/subspaces/legacy/v2" | ||
| "github.com/desmos-labs/desmos/v3/x/subspaces/types" | ||
| ) | ||
|
|
||
| func TestMigrateStore(t *testing.T) { | ||
| cdc, _ := app.MakeCodecs() | ||
|
|
||
| // Build all the necessary keys | ||
| keys := sdk.NewKVStoreKeys(types.StoreKey) | ||
| tKeys := sdk.NewTransientStoreKeys(paramstypes.TStoreKey) | ||
| memKeys := sdk.NewMemoryStoreKeys(capabilitytypes.MemStoreKey) | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| store func(ctx sdk.Context) | ||
| shouldErr bool | ||
| check func(ctx sdk.Context) | ||
| }{ | ||
| { | ||
| name: "groups permissions are sanitized properly", | ||
| store: func(ctx sdk.Context) { | ||
| kvStore := ctx.KVStore(keys[types.StoreKey]) | ||
|
|
||
| group := types.NewUserGroup(11, 11, "Test group", "", 0b11111111111111111111111111000001) | ||
| kvStore.Set(types.GroupStoreKey(group.SubspaceID, group.ID), cdc.MustMarshal(&group)) | ||
| }, | ||
| check: func(ctx sdk.Context) { | ||
| kvStore := ctx.KVStore(keys[types.StoreKey]) | ||
|
|
||
| // Check the permissions | ||
| var group types.UserGroup | ||
| cdc.MustUnmarshal(kvStore.Get(types.GroupStoreKey(11, 11)), &group) | ||
| require.Equal(t, types.PermissionWrite, group.Permissions) | ||
| }, | ||
| }, | ||
| { | ||
| name: "user permissions are sanitized properly", | ||
| store: func(ctx sdk.Context) { | ||
| kvStore := ctx.KVStore(keys[types.StoreKey]) | ||
|
|
||
| addr, err := sdk.AccAddressFromBech32("cosmos12e7ejq92sma437d3svemgfvl8sul8lxfs69mjv") | ||
| require.NoError(t, err) | ||
|
|
||
| kvStore.Set(types.UserPermissionStoreKey(11, addr), types.MarshalPermission(0b11111111111111111111111111000001)) | ||
| }, | ||
| check: func(ctx sdk.Context) { | ||
| kvStore := ctx.KVStore(keys[types.StoreKey]) | ||
|
|
||
| addr, err := sdk.AccAddressFromBech32("cosmos12e7ejq92sma437d3svemgfvl8sul8lxfs69mjv") | ||
| require.NoError(t, err) | ||
|
|
||
| // Check the permissions | ||
| stored := types.UnmarshalPermission(kvStore.Get(types.UserPermissionStoreKey(11, addr))) | ||
| require.Equal(t, types.PermissionWrite, stored) | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| tc := tc | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| ctx := testutil.BuildContext(keys, tKeys, memKeys) | ||
| if tc.store != nil { | ||
| tc.store(ctx) | ||
| } | ||
|
|
||
| err := v2.MigrateStore(ctx, keys[types.StoreKey], cdc) | ||
| if tc.shouldErr { | ||
| require.Error(t, err) | ||
| } else { | ||
| require.NoError(t, err) | ||
| if tc.check != nil { | ||
| tc.check(ctx) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } |
Oops, something went wrong.