From 9fd0998a0f94f6c55f92b2171702314107f14fd4 Mon Sep 17 00:00:00 2001 From: vuong <56973102+vuong177@users.noreply.github.com> Date: Thu, 20 Oct 2022 23:30:27 +0700 Subject: [PATCH] Permission (#1050) * move subset check into 'CanCreateCode' * testing * fix logic * minor * Code feedback * clean test * Revert "clean test" This reverts commit a59be56b5734fd5153d81d38c41ae91599e3faaf. * Revert test and make all pass * Cover instantiation config with tests * Add gov policy for sanity check * Test with gov policy for sanity check Co-authored-by: Alex Peters --- x/wasm/keeper/authz_policy.go | 21 ++++- x/wasm/keeper/authz_policy_test.go | 134 ++++++++++++++++++----------- x/wasm/keeper/keeper.go | 14 +-- x/wasm/keeper/keeper_test.go | 33 ++++--- 4 files changed, 130 insertions(+), 72 deletions(-) diff --git a/x/wasm/keeper/authz_policy.go b/x/wasm/keeper/authz_policy.go index 1a222719a8..19f5320a9e 100644 --- a/x/wasm/keeper/authz_policy.go +++ b/x/wasm/keeper/authz_policy.go @@ -6,8 +6,19 @@ import ( "github.com/CosmWasm/wasmd/x/wasm/types" ) +// ChainAccessConfigs chain settings +type ChainAccessConfigs struct { + Upload types.AccessConfig + Instantiate types.AccessConfig +} + +// NewChainAccessConfigs constructor +func NewChainAccessConfigs(upload types.AccessConfig, instantiate types.AccessConfig) ChainAccessConfigs { + return ChainAccessConfigs{Upload: upload, Instantiate: instantiate} +} + type AuthorizationPolicy interface { - CanCreateCode(c types.AccessConfig, creator sdk.AccAddress) bool + CanCreateCode(chainConfigs ChainAccessConfigs, actor sdk.AccAddress, contractConfig types.AccessConfig) bool CanInstantiateContract(c types.AccessConfig, actor sdk.AccAddress) bool CanModifyContract(admin, actor sdk.AccAddress) bool CanModifyCodeAccessConfig(creator, actor sdk.AccAddress, isSubset bool) bool @@ -15,8 +26,9 @@ type AuthorizationPolicy interface { type DefaultAuthorizationPolicy struct{} -func (p DefaultAuthorizationPolicy) CanCreateCode(config types.AccessConfig, actor sdk.AccAddress) bool { - return config.Allowed(actor) +func (p DefaultAuthorizationPolicy) CanCreateCode(chainConfigs ChainAccessConfigs, actor sdk.AccAddress, contractConfig types.AccessConfig) bool { + return chainConfigs.Upload.Allowed(actor) && + contractConfig.IsSubset(chainConfigs.Instantiate) } func (p DefaultAuthorizationPolicy) CanInstantiateContract(config types.AccessConfig, actor sdk.AccAddress) bool { @@ -33,7 +45,8 @@ func (p DefaultAuthorizationPolicy) CanModifyCodeAccessConfig(creator, actor sdk type GovAuthorizationPolicy struct{} -func (p GovAuthorizationPolicy) CanCreateCode(types.AccessConfig, sdk.AccAddress) bool { +// CanCreateCode implements AuthorizationPolicy.CanCreateCode to allow gov actions. Always returns true. +func (p GovAuthorizationPolicy) CanCreateCode(ChainAccessConfigs, sdk.AccAddress, types.AccessConfig) bool { return true } diff --git a/x/wasm/keeper/authz_policy_test.go b/x/wasm/keeper/authz_policy_test.go index acc9cb346c..f39cc9d1b3 100644 --- a/x/wasm/keeper/authz_policy_test.go +++ b/x/wasm/keeper/authz_policy_test.go @@ -13,50 +13,68 @@ func TestDefaultAuthzPolicyCanCreateCode(t *testing.T) { myActorAddress := RandomAccountAddress(t) otherAddress := RandomAccountAddress(t) specs := map[string]struct { - config types.AccessConfig - actor sdk.AccAddress - exp bool - panics bool + chainConfigs ChainAccessConfigs + contractInstConf types.AccessConfig + actor sdk.AccAddress + exp bool + panics bool }{ - "nobody": { - config: types.AllowNobody, - exp: false, - }, - "everybody": { - config: types.AllowEverybody, - exp: true, - }, - "only address - same": { - config: types.AccessTypeOnlyAddress.With(myActorAddress), - exp: true, - }, - "only address - different": { - config: types.AccessTypeOnlyAddress.With(otherAddress), - exp: false, - }, - "any address - included": { - config: types.AccessTypeAnyOfAddresses.With(otherAddress, myActorAddress), - exp: true, - }, - "any address - not included": { - config: types.AccessTypeAnyOfAddresses.With(otherAddress), - exp: false, - }, - "undefined config - panics": { - config: types.AccessConfig{}, - panics: true, + "upload nobody": { + chainConfigs: NewChainAccessConfigs(types.AllowNobody, types.AllowEverybody), + contractInstConf: types.AllowEverybody, + exp: false, + }, + "upload everybody": { + chainConfigs: NewChainAccessConfigs(types.AllowEverybody, types.AllowEverybody), + contractInstConf: types.AllowEverybody, + exp: true, + }, + "upload only address - same": { + chainConfigs: NewChainAccessConfigs(types.AccessTypeOnlyAddress.With(myActorAddress), types.AllowEverybody), + contractInstConf: types.AllowEverybody, + exp: true, + }, + "upload only address - different": { + chainConfigs: NewChainAccessConfigs(types.AccessTypeOnlyAddress.With(otherAddress), types.AllowEverybody), + contractInstConf: types.AllowEverybody, + exp: false, + }, + "upload any address - included": { + chainConfigs: NewChainAccessConfigs(types.AccessTypeAnyOfAddresses.With(otherAddress, myActorAddress), types.AllowEverybody), + contractInstConf: types.AllowEverybody, + exp: true, + }, + "upload any address - not included": { + chainConfigs: NewChainAccessConfigs(types.AccessTypeAnyOfAddresses.With(otherAddress), types.AllowEverybody), + contractInstConf: types.AllowEverybody, + exp: false, + }, + "contract config - subtype": { + chainConfigs: NewChainAccessConfigs(types.AllowEverybody, types.AllowEverybody), + contractInstConf: types.AccessTypeAnyOfAddresses.With(myActorAddress), + exp: true, + }, + "contract config - not subtype": { + chainConfigs: NewChainAccessConfigs(types.AllowEverybody, types.AllowNobody), + contractInstConf: types.AllowEverybody, + exp: false, + }, + "upload undefined config - panics": { + chainConfigs: NewChainAccessConfigs(types.AccessConfig{}, types.AllowEverybody), + contractInstConf: types.AllowEverybody, + panics: true, }, } for name, spec := range specs { t.Run(name, func(t *testing.T) { policy := DefaultAuthorizationPolicy{} if !spec.panics { - got := policy.CanCreateCode(spec.config, myActorAddress) + got := policy.CanCreateCode(spec.chainConfigs, myActorAddress, spec.contractInstConf) assert.Equal(t, spec.exp, got) return } assert.Panics(t, func() { - policy.CanCreateCode(spec.config, myActorAddress) + policy.CanCreateCode(spec.chainConfigs, myActorAddress, spec.contractInstConf) }) }) } @@ -184,35 +202,51 @@ func TestGovAuthzPolicyCanCreateCode(t *testing.T) { myActorAddress := RandomAccountAddress(t) otherAddress := RandomAccountAddress(t) specs := map[string]struct { - config types.AccessConfig - actor sdk.AccAddress + chainConfigs ChainAccessConfigs + contractInstConf types.AccessConfig + actor sdk.AccAddress }{ - "nobody": { - config: types.AllowNobody, + "upload nobody": { + chainConfigs: NewChainAccessConfigs(types.AllowNobody, types.AllowEverybody), + contractInstConf: types.AllowEverybody, }, - "everybody": { - config: types.AllowEverybody, + "upload everybody": { + chainConfigs: NewChainAccessConfigs(types.AllowEverybody, types.AllowEverybody), + contractInstConf: types.AllowEverybody, }, - "only address - same": { - config: types.AccessTypeOnlyAddress.With(myActorAddress), + "upload only address - same": { + chainConfigs: NewChainAccessConfigs(types.AccessTypeOnlyAddress.With(myActorAddress), types.AllowEverybody), + contractInstConf: types.AllowEverybody, }, - "only address - different": { - config: types.AccessTypeOnlyAddress.With(otherAddress), + "upload only address - different": { + chainConfigs: NewChainAccessConfigs(types.AccessTypeOnlyAddress.With(otherAddress), types.AllowEverybody), + contractInstConf: types.AllowEverybody, }, - "any address - included": { - config: types.AccessTypeAnyOfAddresses.With(otherAddress, myActorAddress), + "upload any address - included": { + chainConfigs: NewChainAccessConfigs(types.AccessTypeAnyOfAddresses.With(otherAddress, myActorAddress), types.AllowEverybody), + contractInstConf: types.AllowEverybody, }, - "any address - not included": { - config: types.AccessTypeAnyOfAddresses.With(otherAddress), + "upload any address - not included": { + chainConfigs: NewChainAccessConfigs(types.AccessTypeAnyOfAddresses.With(otherAddress), types.AllowEverybody), + contractInstConf: types.AllowEverybody, }, - "undefined config - panics": { - config: types.AccessConfig{}, + "contract config - subtype": { + chainConfigs: NewChainAccessConfigs(types.AllowEverybody, types.AllowEverybody), + contractInstConf: types.AccessTypeAnyOfAddresses.With(myActorAddress), + }, + "contract config - not subtype": { + chainConfigs: NewChainAccessConfigs(types.AllowEverybody, types.AllowNobody), + contractInstConf: types.AllowEverybody, + }, + "upload undefined config - not panics": { + chainConfigs: NewChainAccessConfigs(types.AccessConfig{}, types.AllowEverybody), + contractInstConf: types.AllowEverybody, }, } for name, spec := range specs { t.Run(name, func(t *testing.T) { policy := GovAuthorizationPolicy{} - got := policy.CanCreateCode(spec.config, myActorAddress) + got := policy.CanCreateCode(spec.chainConfigs, myActorAddress, spec.contractInstConf) assert.True(t, got) }) } diff --git a/x/wasm/keeper/keeper.go b/x/wasm/keeper/keeper.go index f29f6e18ed..6fbb6640bf 100644 --- a/x/wasm/keeper/keeper.go +++ b/x/wasm/keeper/keeper.go @@ -187,16 +187,18 @@ func (k Keeper) create(ctx sdk.Context, creator sdk.AccAddress, wasmCode []byte, return 0, checksum, sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "cannot be nil") } - if !authZ.CanCreateCode(k.getUploadAccessConfig(ctx), creator) { - return 0, checksum, sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "can not create code") - } // figure out proper instantiate access defaultAccessConfig := k.getInstantiateAccessConfig(ctx).With(creator) if instantiateAccess == nil { instantiateAccess = &defaultAccessConfig - } else if !instantiateAccess.IsSubset(defaultAccessConfig) { - // we enforce this must be subset of default upload access - return 0, checksum, sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "instantiate access must be subset of default upload access") + } + chainConfigs := ChainAccessConfigs{ + Instantiate: defaultAccessConfig, + Upload: k.getUploadAccessConfig(ctx), + } + + if !authZ.CanCreateCode(chainConfigs, creator, *instantiateAccess) { + return 0, checksum, sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "can not create code") } if ioutils.IsGzip(wasmCode) { diff --git a/x/wasm/keeper/keeper_test.go b/x/wasm/keeper/keeper_test.go index 947edd27c3..ddc11aadc4 100644 --- a/x/wasm/keeper/keeper_test.go +++ b/x/wasm/keeper/keeper_test.go @@ -138,39 +138,48 @@ func TestCreateStoresInstantiatePermission(t *testing.T) { func TestCreateWithParamPermissions(t *testing.T) { ctx, keepers := CreateTestInput(t, false, AvailableCapabilities) - keeper := keepers.ContractKeeper - deposit := sdk.NewCoins(sdk.NewInt64Coin("denom", 100000)) creator := keepers.Faucet.NewFundedRandomAccount(ctx, deposit...) otherAddr := keepers.Faucet.NewFundedRandomAccount(ctx, deposit...) specs := map[string]struct { - srcPermission types.AccessConfig - expError *sdkerrors.Error + policy AuthorizationPolicy + chainUpload types.AccessConfig + expError *sdkerrors.Error }{ "default": { - srcPermission: types.DefaultUploadAccess, + policy: DefaultAuthorizationPolicy{}, + chainUpload: types.DefaultUploadAccess, }, "everybody": { - srcPermission: types.AllowEverybody, + policy: DefaultAuthorizationPolicy{}, + chainUpload: types.AllowEverybody, }, "nobody": { - srcPermission: types.AllowNobody, - expError: sdkerrors.ErrUnauthorized, + policy: DefaultAuthorizationPolicy{}, + chainUpload: types.AllowNobody, + expError: sdkerrors.ErrUnauthorized, }, "onlyAddress with matching address": { - srcPermission: types.AccessTypeOnlyAddress.With(creator), + policy: DefaultAuthorizationPolicy{}, + chainUpload: types.AccessTypeOnlyAddress.With(creator), }, "onlyAddress with non matching address": { - srcPermission: types.AccessTypeOnlyAddress.With(otherAddr), - expError: sdkerrors.ErrUnauthorized, + policy: DefaultAuthorizationPolicy{}, + chainUpload: types.AccessTypeOnlyAddress.With(otherAddr), + expError: sdkerrors.ErrUnauthorized, + }, + "gov: always allowed": { + policy: GovAuthorizationPolicy{}, + chainUpload: types.AllowNobody, }, } for msg, spec := range specs { t.Run(msg, func(t *testing.T) { params := types.DefaultParams() - params.CodeUploadAccess = spec.srcPermission + params.CodeUploadAccess = spec.chainUpload keepers.WasmKeeper.SetParams(ctx, params) + keeper := NewPermissionedKeeper(keepers.WasmKeeper, spec.policy) _, _, err := keeper.Create(ctx, creator, hackatomWasm, nil) require.True(t, spec.expError.Is(err), err) if spec.expError != nil {