diff --git a/x/wasm/keeper/authz_policy.go b/x/wasm/keeper/authz_policy.go index 32d8c995..1a222719 100644 --- a/x/wasm/keeper/authz_policy.go +++ b/x/wasm/keeper/authz_policy.go @@ -10,6 +10,7 @@ type AuthorizationPolicy interface { CanCreateCode(c types.AccessConfig, creator sdk.AccAddress) bool CanInstantiateContract(c types.AccessConfig, actor sdk.AccAddress) bool CanModifyContract(admin, actor sdk.AccAddress) bool + CanModifyCodeAccessConfig(creator, actor sdk.AccAddress, isSubset bool) bool } type DefaultAuthorizationPolicy struct{} @@ -26,6 +27,10 @@ func (p DefaultAuthorizationPolicy) CanModifyContract(admin, actor sdk.AccAddres return admin != nil && admin.Equals(actor) } +func (p DefaultAuthorizationPolicy) CanModifyCodeAccessConfig(creator, actor sdk.AccAddress, isSubset bool) bool { + return creator != nil && creator.Equals(actor) && isSubset +} + type GovAuthorizationPolicy struct{} func (p GovAuthorizationPolicy) CanCreateCode(types.AccessConfig, sdk.AccAddress) bool { @@ -39,3 +44,7 @@ func (p GovAuthorizationPolicy) CanInstantiateContract(types.AccessConfig, sdk.A func (p GovAuthorizationPolicy) CanModifyContract(sdk.AccAddress, sdk.AccAddress) bool { return true } + +func (p GovAuthorizationPolicy) CanModifyCodeAccessConfig(sdk.AccAddress, sdk.AccAddress, bool) bool { + return true +} diff --git a/x/wasm/keeper/contract_keeper.go b/x/wasm/keeper/contract_keeper.go index a6a0adf0..73d925de 100644 --- a/x/wasm/keeper/contract_keeper.go +++ b/x/wasm/keeper/contract_keeper.go @@ -19,7 +19,7 @@ type decoratedKeeper interface { execute(ctx sdk.Context, contractAddress sdk.AccAddress, caller sdk.AccAddress, msg []byte, coins sdk.Coins) ([]byte, error) Sudo(ctx sdk.Context, contractAddress sdk.AccAddress, msg []byte) ([]byte, error) setContractInfoExtension(ctx sdk.Context, contract sdk.AccAddress, extra types.ContractInfoExtension) error - setAccessConfig(ctx sdk.Context, codeID uint64, config types.AccessConfig) error + setAccessConfig(ctx sdk.Context, codeID uint64, caller sdk.AccAddress, newConfig types.AccessConfig, autz AuthorizationPolicy) error } type PermissionedKeeper struct { @@ -81,6 +81,6 @@ func (p PermissionedKeeper) SetContractInfoExtension(ctx sdk.Context, contract s } // SetAccessConfig updates the access config of a code id. -func (p PermissionedKeeper) SetAccessConfig(ctx sdk.Context, codeID uint64, config types.AccessConfig) error { - return p.nested.setAccessConfig(ctx, codeID, config) +func (p PermissionedKeeper) SetAccessConfig(ctx sdk.Context, codeID uint64, caller sdk.AccAddress, newConfig types.AccessConfig) error { + return p.nested.setAccessConfig(ctx, codeID, caller, newConfig, p.authZPolicy) } diff --git a/x/wasm/keeper/keeper.go b/x/wasm/keeper/keeper.go index 322bbdc1..372cb4e9 100644 --- a/x/wasm/keeper/keeper.go +++ b/x/wasm/keeper/keeper.go @@ -884,12 +884,17 @@ func (k Keeper) setContractInfoExtension(ctx sdk.Context, contractAddr sdk.AccAd } // setAccessConfig updates the access config of a code id. -func (k Keeper) setAccessConfig(ctx sdk.Context, codeID uint64, config types.AccessConfig) error { +func (k Keeper) setAccessConfig(ctx sdk.Context, codeID uint64, caller sdk.AccAddress, newConfig types.AccessConfig, authz AuthorizationPolicy) error { info := k.GetCodeInfo(ctx, codeID) if info == nil { return sdkerrors.Wrap(types.ErrNotFound, "code info") } - info.InstantiateConfig = config + isSubset := newConfig.Permission.IsSubset(k.getInstantiateAccessConfig(ctx)) + if !authz.CanModifyCodeAccessConfig(sdk.MustAccAddressFromBech32(info.Creator), caller, isSubset) { + return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "can not modify code access config") + } + + info.InstantiateConfig = newConfig k.storeCodeInfo(ctx, codeID, *info) return nil } diff --git a/x/wasm/keeper/keeper_test.go b/x/wasm/keeper/keeper_test.go index 44527ac9..91ebdb4e 100644 --- a/x/wasm/keeper/keeper_test.go +++ b/x/wasm/keeper/keeper_test.go @@ -1836,3 +1836,87 @@ func TestBuildContractAddress(t *testing.T) { }) } } +func TestSetAccessConfig(t *testing.T) { + parentCtx, keepers := CreateTestInput(t, false, SupportedFeatures) + k := keepers.WasmKeeper + creatorAddr := RandomAccountAddress(t) + nonCreatorAddr := RandomAccountAddress(t) + + specs := map[string]struct { + authz AuthorizationPolicy + chainPermission types.AccessType + newConfig types.AccessConfig + caller sdk.AccAddress + expErr bool + }{ + "user with new permissions == chain permissions": { + authz: DefaultAuthorizationPolicy{}, + chainPermission: types.AccessTypeEverybody, + newConfig: types.AllowEverybody, + caller: creatorAddr, + }, + "user with new permissions < chain permissions": { + authz: DefaultAuthorizationPolicy{}, + chainPermission: types.AccessTypeEverybody, + newConfig: types.AllowNobody, + caller: creatorAddr, + }, + "user with new permissions > chain permissions": { + authz: DefaultAuthorizationPolicy{}, + chainPermission: types.AccessTypeNobody, + newConfig: types.AllowEverybody, + caller: creatorAddr, + expErr: true, + }, + "different actor": { + authz: DefaultAuthorizationPolicy{}, + chainPermission: types.AccessTypeEverybody, + newConfig: types.AllowEverybody, + caller: nonCreatorAddr, + expErr: true, + }, + "gov with new permissions == chain permissions": { + authz: GovAuthorizationPolicy{}, + chainPermission: types.AccessTypeEverybody, + newConfig: types.AllowEverybody, + caller: creatorAddr, + }, + "gov with new permissions < chain permissions": { + authz: GovAuthorizationPolicy{}, + chainPermission: types.AccessTypeEverybody, + newConfig: types.AllowNobody, + caller: creatorAddr, + }, + "gov with new permissions > chain permissions": { + authz: GovAuthorizationPolicy{}, + chainPermission: types.AccessTypeNobody, + newConfig: types.AccessTypeOnlyAddress.With(creatorAddr), + caller: creatorAddr, + }, + "gov without actor": { + authz: GovAuthorizationPolicy{}, + chainPermission: types.AccessTypeEverybody, + newConfig: types.AllowEverybody, + }, + } + const codeID = 1 + for name, spec := range specs { + t.Run(name, func(t *testing.T) { + ctx, _ := parentCtx.CacheContext() + newParams := types.DefaultParams() + newParams.InstantiateDefaultPermission = spec.chainPermission + k.SetParams(ctx, newParams) + + k.storeCodeInfo(ctx, codeID, types.NewCodeInfo(nil, creatorAddr, types.AllowNobody)) + // when + gotErr := k.setAccessConfig(ctx, codeID, spec.caller, spec.newConfig, spec.authz) + if spec.expErr { + require.Error(t, gotErr) + return + } + require.NoError(t, gotErr) + + }) + } + +} diff --git a/x/wasm/keeper/proposal_handler.go b/x/wasm/keeper/proposal_handler.go index 9da330ce..beac2563 100644 --- a/x/wasm/keeper/proposal_handler.go +++ b/x/wasm/keeper/proposal_handler.go @@ -229,8 +229,9 @@ func handleUpdateInstantiateConfigProposal(ctx sdk.Context, k types.ContractOpsK return err } + var emptyCaller sdk.AccAddress for _, accessConfigUpdate := range p.AccessConfigUpdates { - if err := k.SetAccessConfig(ctx, accessConfigUpdate.CodeID, accessConfigUpdate.InstantiatePermission); err != nil { + if err := k.SetAccessConfig(ctx, accessConfigUpdate.CodeID, emptyCaller, accessConfigUpdate.InstantiatePermission); err != nil { return sdkerrors.Wrapf(err, "code id: %d", accessConfigUpdate.CodeID) } } diff --git a/x/wasm/types/exported_keepers.go b/x/wasm/types/exported_keepers.go index bb919a31..cb084883 100644 --- a/x/wasm/types/exported_keepers.go +++ b/x/wasm/types/exported_keepers.go @@ -55,7 +55,7 @@ type ContractOpsKeeper interface { SetContractInfoExtension(ctx sdk.Context, contract sdk.AccAddress, extra ContractInfoExtension) error // SetAccessConfig updates the access config of a code id. - SetAccessConfig(ctx sdk.Context, codeID uint64, config AccessConfig) error + SetAccessConfig(ctx sdk.Context, codeID uint64, caller sdk.AccAddress, newConfig AccessConfig) error } // IBCContractKeeper IBC lifecycle event handler diff --git a/x/wasm/types/types.go b/x/wasm/types/types.go index 887e895b..cdf669e5 100644 --- a/x/wasm/types/types.go +++ b/x/wasm/types/types.go @@ -334,18 +334,30 @@ func VerifyAddressLen() func(addr []byte) error { // IsSubset will return true if the caller is the same as the superset, // or if the caller is more restrictive than the superset. -func (a AccessConfig) IsSubset(superSet AccessConfig) bool { - switch superSet.Permission { +func (a AccessType) IsSubset(superSet AccessType) bool { + switch superSet { case AccessTypeEverybody: // Everything is a subset of this - return a.Permission != AccessTypeUnspecified + return a != AccessTypeUnspecified case AccessTypeNobody: // Only an exact match is a subset of this - return a.Permission == AccessTypeNobody + return a == AccessTypeNobody case AccessTypeOnlyAddress: // An exact match or nobody - return a.Permission == AccessTypeNobody || (a.Permission == AccessTypeOnlyAddress && a.Address == superSet.Address) + return a == AccessTypeNobody || a == AccessTypeOnlyAddress default: return false } } + +// IsSubset will return true if the caller is the same as the superset, +// or if the caller is more restrictive than the superset. +func (a AccessConfig) IsSubset(superSet AccessConfig) bool { + switch superSet.Permission { + case AccessTypeOnlyAddress: + // An exact match or nobody + return a.Permission == AccessTypeNobody || (a.Permission == AccessTypeOnlyAddress && a.Address == superSet.Address) + default: + return a.Permission.IsSubset(superSet.Permission) + } +} diff --git a/x/wasm/types/types_test.go b/x/wasm/types/types_test.go index 11741593..91b39419 100644 --- a/x/wasm/types/types_test.go +++ b/x/wasm/types/types_test.go @@ -373,7 +373,7 @@ func TestVerifyAddressLen(t *testing.T) { } } -func TestAccesConfigSubset(t *testing.T) { +func TestAccessConfigSubset(t *testing.T) { specs := map[string]struct { check AccessConfig superSet AccessConfig @@ -453,3 +453,78 @@ func TestAccesConfigSubset(t *testing.T) { }) } } + +func TestAccessTypeSubset(t *testing.T) { + specs := map[string]struct { + check AccessType + superSet AccessType + isSubSet bool + }{ + "nobody <= nobody": { + superSet: AccessTypeNobody, + check: AccessTypeNobody, + isSubSet: true, + }, + "only > nobody": { + superSet: AccessTypeNobody, + check: AccessTypeOnlyAddress, + isSubSet: false, + }, + "everybody > nobody": { + superSet: AccessTypeNobody, + check: AccessTypeEverybody, + isSubSet: false, + }, + "unspecified > nobody": { + superSet: AccessTypeNobody, + check: AccessTypeUnspecified, + isSubSet: false, + }, + "nobody <= everybody": { + superSet: AccessTypeEverybody, + check: AccessTypeNobody, + isSubSet: true, + }, + "only <= everybody": { + superSet: AccessTypeEverybody, + check: AccessTypeOnlyAddress, + isSubSet: true, + }, + "everybody <= everybody": { + superSet: AccessTypeEverybody, + check: AccessTypeEverybody, + isSubSet: true, + }, + "unspecified > everybody": { + superSet: AccessTypeEverybody, + check: AccessTypeUnspecified, + isSubSet: false, + }, + "nobody <= only": { + superSet: AccessTypeOnlyAddress, + check: AccessTypeNobody, + isSubSet: true, + }, + "only <= only(same)": { + superSet: AccessTypeOnlyAddress, + check: AccessTypeOnlyAddress, + isSubSet: true, + }, + "everybody > only": { + superSet: AccessTypeOnlyAddress, + check: AccessTypeEverybody, + isSubSet: false, + }, + "nobody > unspecified": { + superSet: AccessTypeUnspecified, + check: AccessTypeNobody, + isSubSet: false, + }, + } + for name, spec := range specs { + t.Run(name, func(t *testing.T) { + subset := spec.check.IsSubset(spec.superSet) + require.Equal(t, spec.isSubSet, subset) + }) + } +}