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 <alpe@users.noreply.github.com>

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
 }
 

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,
+		"upload nobody": {
+			chainConfigs:     NewChainAccessConfigs(types.AllowNobody, types.AllowEverybody),
+			contractInstConf: types.AllowEverybody,
+			exp:              false,
 		},
-		"everybody": {
-			config: types.AllowEverybody,
-			exp:    true,
+		"upload everybody": {
+			chainConfigs:     NewChainAccessConfigs(types.AllowEverybody, types.AllowEverybody),
+			contractInstConf: types.AllowEverybody,
+			exp:              true,
 		},
-		"only address - same": {
-			config: types.AccessTypeOnlyAddress.With(myActorAddress),
-			exp:    true,
+		"upload only address - same": {
+			chainConfigs:     NewChainAccessConfigs(types.AccessTypeOnlyAddress.With(myActorAddress), types.AllowEverybody),
+			contractInstConf: types.AllowEverybody,
+			exp:              true,
 		},
-		"only address - different": {
-			config: types.AccessTypeOnlyAddress.With(otherAddress),
-			exp:    false,
+		"upload only address - different": {
+			chainConfigs:     NewChainAccessConfigs(types.AccessTypeOnlyAddress.With(otherAddress), types.AllowEverybody),
+			contractInstConf: types.AllowEverybody,
+			exp:              false,
 		},
-		"any address - included": {
-			config: types.AccessTypeAnyOfAddresses.With(otherAddress, myActorAddress),
-			exp:    true,
+		"upload any address - included": {
+			chainConfigs:     NewChainAccessConfigs(types.AccessTypeAnyOfAddresses.With(otherAddress, myActorAddress), types.AllowEverybody),
+			contractInstConf: types.AllowEverybody,
+			exp:              true,
 		},
-		"any address - not included": {
-			config: types.AccessTypeAnyOfAddresses.With(otherAddress),
-			exp:    false,
+		"upload any address - not included": {
+			chainConfigs:     NewChainAccessConfigs(types.AccessTypeAnyOfAddresses.With(otherAddress), types.AllowEverybody),
+			contractInstConf: types.AllowEverybody,
+			exp:              false,
 		},
-		"undefined config - panics": {
-			config: types.AccessConfig{},
-			panics: true,
+		"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)
 		})
 	}

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) {

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 {