Merge pull request #901 from CosmWasm/access_config

Restrict code access config modifications

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
+}

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

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
 }

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)
+
+		})
+	}
+
+}

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

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

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)
+	}
+}

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)
+		})
+	}
+}