Merge pull request #2390 from onflow/robert/contract-removal-permissioning

Separate permission setting of contract deploy/update and removal

Author: m4ksio

Diff for: fvm/blueprints/contracts.go

@@ -14,10 +14,13 @@ import (
 const ContractDeploymentAuthorizedAddressesPathDomain = "storage"
 const ContractDeploymentAuthorizedAddressesPathIdentifier = "authorizedAddressesToDeployContracts"
 
+const ContractRemovalAuthorizedAddressesPathDomain = "storage"
+const ContractRemovalAuthorizedAddressesPathIdentifier = "authorizedAddressesToRemoveContracts"
+
 const IsContractDeploymentRestrictedPathDomain = "storage"
 const IsContractDeploymentRestrictedPathIdentifier = "isContractDeploymentRestricted"
 
-const setContractDeploymentAuthorizersTransactionTemplate = `
+const setContractOperationAuthorizersTransactionTemplate = `
 transaction(addresses: [Address], path: StoragePath) {
 	prepare(signer: AuthAccount) {
 		signer.load<[Address]>(from: path)
@@ -28,24 +31,43 @@ transaction(addresses: [Address], path: StoragePath) {
 
 // SetContractDeploymentAuthorizersTransaction returns a transaction for updating list of authorized accounts allowed to deploy/update contracts
 func SetContractDeploymentAuthorizersTransaction(serviceAccount flow.Address, authorized []flow.Address) (*flow.TransactionBody, error) {
-	arg1, err := jsoncdc.Encode(utils.AddressSliceToCadenceValue(utils.FlowAddressSliceToCadenceAddressSlice(authorized)))
+	path := cadence.Path{
+		Domain:     ContractDeploymentAuthorizedAddressesPathDomain,
+		Identifier: ContractDeploymentAuthorizedAddressesPathIdentifier,
+	}
+	return setContractAuthorizersTransaction(path, serviceAccount, authorized)
+}
+
+// SetContractRemovalAuthorizersTransaction returns a transaction for updating list of authorized accounts allowed to remove contracts
+func SetContractRemovalAuthorizersTransaction(serviceAccount flow.Address, authorized []flow.Address) (*flow.TransactionBody, error) {
+	path := cadence.Path{
+		Domain:     ContractRemovalAuthorizedAddressesPathDomain,
+		Identifier: ContractRemovalAuthorizedAddressesPathIdentifier,
+	}
+	return setContractAuthorizersTransaction(path, serviceAccount, authorized)
+}
+
+func setContractAuthorizersTransaction(
+	path cadence.Path,
+	serviceAccount flow.Address,
+	authorized []flow.Address,
+) (*flow.TransactionBody, error) {
+	addresses := utils.FlowAddressSliceToCadenceAddressSlice(authorized)
+	addressesArg, err := jsoncdc.Encode(utils.AddressSliceToCadenceValue(addresses))
 	if err != nil {
 		return nil, err
 	}
 
-	arg2, err := jsoncdc.Encode(cadence.Path{
-		Domain:     ContractDeploymentAuthorizedAddressesPathDomain,
-		Identifier: ContractDeploymentAuthorizedAddressesPathIdentifier,
-	})
+	pathArg, err := jsoncdc.Encode(path)
 	if err != nil {
 		return nil, err
 	}
 
 	return flow.NewTransactionBody().
-		SetScript([]byte(setContractDeploymentAuthorizersTransactionTemplate)).
+		SetScript([]byte(setContractOperationAuthorizersTransactionTemplate)).
 		AddAuthorizer(serviceAccount).
-		AddArgument(arg1).
-		AddArgument(arg2), nil
+		AddArgument(addressesArg).
+		AddArgument(pathArg), nil
 }
 
 const setIsContractDeploymentRestrictedTransactionTemplate = `

Diff for: fvm/handler/contract.go

@@ -16,7 +16,7 @@ import (
 )
 
 type RestrictedDeploymentEnabledFunc func() bool
-type AuthorizedAccountsForContractDeploymentFunc func() []common.Address
+type AuthorizedAccountsFunc func() []common.Address
 type UseContractAuditVoucherFunc func(address runtime.Address, code []byte) (bool, error)
 
 // ContractHandler handles all interaction
@@ -25,11 +25,12 @@ type UseContractAuditVoucherFunc func(address runtime.Address, code []byte) (boo
 // only commit them when called so smart contract
 // updates can be delayed until end of the tx execution
 type ContractHandler struct {
-	accounts                    state.Accounts
-	draftUpdates                map[programs.ContractUpdateKey]programs.ContractUpdate
-	restrictedDeploymentEnabled RestrictedDeploymentEnabledFunc
-	authorizedAccounts          AuthorizedAccountsForContractDeploymentFunc
-	useContractAuditVoucher     UseContractAuditVoucherFunc
+	accounts                     state.Accounts
+	draftUpdates                 map[programs.ContractUpdateKey]programs.ContractUpdate
+	restrictedDeploymentEnabled  RestrictedDeploymentEnabledFunc
+	authorizedDeploymentAccounts AuthorizedAccountsFunc
+	authorizedRemovalAccounts    AuthorizedAccountsFunc
+	useContractAuditVoucher      UseContractAuditVoucherFunc
 	// handler doesn't have to be thread safe and right now
 	// is only used in a single thread but a mutex has been added
 	// here to prevent accidental multi-thread use in the future
@@ -38,15 +39,17 @@ type ContractHandler struct {
 
 func NewContractHandler(accounts state.Accounts,
 	restrictedDeploymentEnabled RestrictedDeploymentEnabledFunc,
-	authorizedAccounts AuthorizedAccountsForContractDeploymentFunc,
+	authorizedDeploymentAccounts AuthorizedAccountsFunc,
+	authorizedRemovalAccounts AuthorizedAccountsFunc,
 	useContractAuditVoucher UseContractAuditVoucherFunc,
 ) *ContractHandler {
 	return &ContractHandler{
-		accounts:                    accounts,
-		draftUpdates:                make(map[programs.ContractUpdateKey]programs.ContractUpdate),
-		restrictedDeploymentEnabled: restrictedDeploymentEnabled,
-		authorizedAccounts:          authorizedAccounts,
-		useContractAuditVoucher:     useContractAuditVoucher,
+		accounts:                     accounts,
+		draftUpdates:                 make(map[programs.ContractUpdateKey]programs.ContractUpdate),
+		restrictedDeploymentEnabled:  restrictedDeploymentEnabled,
+		authorizedDeploymentAccounts: authorizedDeploymentAccounts,
+		authorizedRemovalAccounts:    authorizedRemovalAccounts,
+		useContractAuditVoucher:      useContractAuditVoucher,
 	}
 }
 
@@ -80,7 +83,7 @@ func (h *ContractHandler) SetContract(
 		return err
 	}
 
-	if !exists && !h.isAuthorized(signingAccounts) {
+	if !exists && !h.isAuthorizedForDeployment(signingAccounts) {
 		// check if there's an audit voucher for the contract
 		voucherAvailable, err := h.useContractAuditVoucher(address, code)
 		if err != nil {
@@ -115,9 +118,13 @@ func (h *ContractHandler) SetContract(
 	return nil
 }
 
-func (h *ContractHandler) RemoveContract(address runtime.Address, name string, signingAccounts []runtime.Address) (err error) {
+func (h *ContractHandler) RemoveContract(
+	address runtime.Address,
+	name string,
+	signingAccounts []runtime.Address,
+) (err error) {
 	// check if authorized
-	if !h.isAuthorized(signingAccounts) {
+	if !h.isAuthorizedForRemoval(signingAccounts) {
 		err = errors.NewOperationAuthorizationErrorf("RemoveContract", "removing contracts requires authorization from specific accounts")
 		return fmt.Errorf("removing contract failed: %w", err)
 	}
@@ -205,10 +212,18 @@ func (h *ContractHandler) UpdateKeys() []programs.ContractUpdateKey {
 	return keys
 }
 
-func (h *ContractHandler) isAuthorized(signingAccounts []runtime.Address) bool {
+func (h *ContractHandler) isAuthorizedForDeployment(signingAccounts []runtime.Address) bool {
+	return h.isAuthorized(signingAccounts, h.authorizedDeploymentAccounts)
+}
+
+func (h *ContractHandler) isAuthorizedForRemoval(signingAccounts []runtime.Address) bool {
+	return h.isAuthorized(signingAccounts, h.authorizedRemovalAccounts)
+}
+
+func (h *ContractHandler) isAuthorized(signingAccounts []runtime.Address, authorizedAccounts AuthorizedAccountsFunc) bool {
 	if h.restrictedDeploymentEnabled() {
-		accs := h.authorizedAccounts()
-		for _, authorized := range accs {
+		accts := authorizedAccounts()
+		for _, authorized := range accts {
 			for _, signer := range signingAccounts {
 				if signer == authorized {
 					// a single authorized singer is enough

Diff for: fvm/handler/contract_test.go

@@ -26,7 +26,7 @@ func TestContract_ChildMergeFunctionality(t *testing.T) {
 	err := accounts.Create(nil, address)
 	require.NoError(t, err)
 
-	contractHandler := handler.NewContractHandler(accounts, func() bool { return false }, nil, nil)
+	contractHandler := handler.NewContractHandler(accounts, func() bool { return false }, nil, nil, nil)
 
 	// no contract initially
 	names, err := contractHandler.GetContractNames(rAdd)
@@ -68,35 +68,141 @@ func TestContract_ChildMergeFunctionality(t *testing.T) {
 	cont, err = contractHandler.GetContract(rAdd, "testContract")
 	require.NoError(t, err)
 	require.Equal(t, cont, []byte("ABC"))
+
+	// remove
+	err = contractHandler.RemoveContract(rAdd, "testContract", nil)
+	require.NoError(t, err)
+
+	// contract still there because no commit yet
+	cont, err = contractHandler.GetContract(rAdd, "testContract")
+	require.NoError(t, err)
+	require.Equal(t, cont, []byte("ABC"))
+
+	// commit removal
+	_, err = contractHandler.Commit()
+	require.NoError(t, err)
+
+	// contract should no longer be there
+	cont, err = contractHandler.GetContract(rAdd, "testContract")
+	require.NoError(t, err)
+	require.Equal(t, []byte(nil), cont)
 }
 
 func TestContract_AuthorizationFunctionality(t *testing.T) {
 	sth := state.NewStateHolder(state.NewState(utils.NewSimpleView()))
 	accounts := state.NewAccounts(sth)
-	address := flow.HexToAddress("01")
-	rAdd := runtime.Address(address)
-	err := accounts.Create(nil, address)
-	require.NoError(t, err)
 
-	unAuthAdd := flow.HexToAddress("02")
-	unAuthRAdd := runtime.Address(unAuthAdd)
-	err = accounts.Create(nil, unAuthAdd)
+	authAdd := flow.HexToAddress("01")
+	rAdd := runtime.Address(authAdd)
+	err := accounts.Create(nil, authAdd)
 	require.NoError(t, err)
 
-	contractHandler := handler.NewContractHandler(accounts,
-		func() bool { return true },
-		func() []common.Address { return []common.Address{rAdd} },
-		func(address runtime.Address, code []byte) (bool, error) { return false, nil })
+	authRemove := flow.HexToAddress("02")
+	rRemove := runtime.Address(authRemove)
+	err = accounts.Create(nil, authRemove)
+	require.NoError(t, err)
 
-	// try to set contract by an unAuthRAdd
-	err = contractHandler.SetContract(rAdd, "testContract1", []byte("ABC"), []common.Address{unAuthRAdd})
-	require.Error(t, err)
-	require.False(t, contractHandler.HasUpdates())
+	authBoth := flow.HexToAddress("03")
+	rBoth := runtime.Address(authBoth)
+	err = accounts.Create(nil, authBoth)
+	require.NoError(t, err)
 
-	// set contract by an authorized account
-	err = contractHandler.SetContract(rAdd, "testContract2", []byte("ABC"), []common.Address{rAdd})
+	unAuth := flow.HexToAddress("04")
+	unAuthR := runtime.Address(unAuth)
+	err = accounts.Create(nil, unAuth)
 	require.NoError(t, err)
-	require.True(t, contractHandler.HasUpdates())
+
+	makeHandler := func() *handler.ContractHandler {
+		return handler.NewContractHandler(accounts,
+			func() bool { return true },
+			func() []common.Address { return []common.Address{rAdd, rBoth} },
+			func() []common.Address { return []common.Address{rRemove, rBoth} },
+			func(address runtime.Address, code []byte) (bool, error) { return false, nil })
+	}
+
+	t.Run("try to set contract with unauthorized account", func(t *testing.T) {
+		contractHandler := makeHandler()
+
+		err = contractHandler.SetContract(rAdd, "testContract1", []byte("ABC"), []common.Address{unAuthR})
+		require.Error(t, err)
+		require.False(t, contractHandler.HasUpdates())
+	})
+
+	t.Run("try to set contract with account only authorized for removal", func(t *testing.T) {
+		contractHandler := makeHandler()
+
+		err = contractHandler.SetContract(rAdd, "testContract1", []byte("ABC"), []common.Address{rRemove})
+		require.Error(t, err)
+		require.False(t, contractHandler.HasUpdates())
+	})
+
+	t.Run("set contract with account authorized for adding", func(t *testing.T) {
+		contractHandler := makeHandler()
+
+		err = contractHandler.SetContract(rAdd, "testContract2", []byte("ABC"), []common.Address{rAdd})
+		require.NoError(t, err)
+		require.True(t, contractHandler.HasUpdates())
+	})
+
+	t.Run("set contract with account authorized for adding and removing", func(t *testing.T) {
+		contractHandler := makeHandler()
+
+		err = contractHandler.SetContract(rAdd, "testContract2", []byte("ABC"), []common.Address{rBoth})
+		require.NoError(t, err)
+		require.True(t, contractHandler.HasUpdates())
+	})
+
+	t.Run("try to remove contract with unauthorized account", func(t *testing.T) {
+		contractHandler := makeHandler()
+
+		err = contractHandler.SetContract(rAdd, "testContract1", []byte("ABC"), []common.Address{rAdd})
+		require.NoError(t, err)
+		_, err = contractHandler.Commit()
+		require.NoError(t, err)
+
+		err = contractHandler.RemoveContract(unAuthR, "testContract2", []common.Address{unAuthR})
+		require.Error(t, err)
+		require.False(t, contractHandler.HasUpdates())
+	})
+
+	t.Run("remove contract account authorized for removal", func(t *testing.T) {
+		contractHandler := makeHandler()
+
+		err = contractHandler.SetContract(rAdd, "testContract1", []byte("ABC"), []common.Address{rAdd})
+		require.NoError(t, err)
+		_, err = contractHandler.Commit()
+		require.NoError(t, err)
+
+		err = contractHandler.RemoveContract(rRemove, "testContract2", []common.Address{rRemove})
+		require.NoError(t, err)
+		require.True(t, contractHandler.HasUpdates())
+	})
+
+	t.Run("try to remove contract with account only authorized for adding", func(t *testing.T) {
+		contractHandler := makeHandler()
+
+		err = contractHandler.SetContract(rAdd, "testContract1", []byte("ABC"), []common.Address{rAdd})
+		require.NoError(t, err)
+		_, err = contractHandler.Commit()
+		require.NoError(t, err)
+
+		err = contractHandler.RemoveContract(rAdd, "testContract2", []common.Address{rAdd})
+		require.Error(t, err)
+		require.False(t, contractHandler.HasUpdates())
+	})
+
+	t.Run("remove contract with account authorized for adding and removing", func(t *testing.T) {
+		contractHandler := makeHandler()
+
+		err = contractHandler.SetContract(rAdd, "testContract1", []byte("ABC"), []common.Address{rAdd})
+		require.NoError(t, err)
+		_, err = contractHandler.Commit()
+		require.NoError(t, err)
+
+		err = contractHandler.RemoveContract(rBoth, "testContract2", []common.Address{rBoth})
+		require.NoError(t, err)
+		require.True(t, contractHandler.HasUpdates())
+	})
 }
 
 func TestContract_DeploymentVouchers(t *testing.T) {
@@ -120,6 +226,9 @@ func TestContract_DeploymentVouchers(t *testing.T) {
 		func() []common.Address {
 			return []common.Address{}
 		},
+		func() []common.Address {
+			return []common.Address{}
+		},
 		func(address runtime.Address, code []byte) (bool, error) {
 			if address.String() == addressWithVoucher.String() {
 				return true, nil
@@ -170,6 +279,9 @@ func TestContract_ContractUpdate(t *testing.T) {
 		func() []common.Address {
 			return []common.Address{}
 		},
+		func() []common.Address {
+			return []common.Address{}
+		},
 		func(address runtime.Address, code []byte) (bool, error) {
 			// Ensure the voucher check is only called once,
 			// for the initial contract deployment,
@@ -234,6 +346,7 @@ func TestContract_DeterministicErrorOnCommit(t *testing.T) {
 		func() bool { return false },
 		nil,
 		nil,
+		nil,
 	)
 
 	address1 := runtime.Address(flow.HexToAddress("0000000000000001"))

Diff for: fvm/scriptEnv.go

@@ -91,6 +91,7 @@ func NewScriptEnvironment(
 			return true
 		},
 		func() []common.Address { return []common.Address{} },
+		func() []common.Address { return []common.Address{} },
 		func(address runtime.Address, code []byte) (bool, error) { return false, nil })
 
 	if fvmContext.BlockHeader != nil {