Merge pull request #89 from Neufund/rfix/tests-and-documents-access-c…

…ontrol

tests and documents access control

contracts/AccessControl/AccessControlled.sol

@@ -4,6 +4,12 @@ import './IAccessControlled.sol';
 import './StandardRoles.sol';
 
 
+/// @title Granular code execution permissions
+/// @notice Intended to replace existing Ownable pattern with more granular permissions set to execute smart contract functions
+///     for each function where 'only' modifier is applied, IAccessPolicy implementation is called to evaluate if msg.sender belongs to required role for contract being called.
+///     Access evaluation specific belong to IAccessPolicy implementation, see RoleBasedAccessPolicy for details.
+/// @dev Should be inherited by a contract requiring such permissions controll. IAccessPolicy must be provided in constructor. Access policy may be replaced to a different one
+///     by msg.sender with ROLE_ACCESS_CONTROLLER role
 contract AccessControlled is IAccessControlled, StandardRoles {
 
     ////////////////////////
@@ -16,6 +22,7 @@ contract AccessControlled is IAccessControlled, StandardRoles {
     // Modifiers
     ////////////////////////
 
+    /// @dev limits function execution only to senders assigned to required 'role'
     modifier only(bytes32 role) {
         require(_accessPolicy.allowed(msg.sender, role, this, msg.sig));
         _;
@@ -34,14 +41,18 @@ contract AccessControlled is IAccessControlled, StandardRoles {
     // Public functions
     ////////////////////////
 
-    function setAccessPolicy(IAccessPolicy newPolicy)
+    //
+    // Implements IAccessControlled
+    //
+
+    function setAccessPolicy(IAccessPolicy newPolicy, address newAccessController)
         public
         only(ROLE_ACCESS_CONTROLLER)
     {
-        // The access controler also needs to have this
-        // role under the new policy. This provides some
-        // protection agains locking yourself out.
-        require(newPolicy.allowed(msg.sender, ROLE_ACCESS_CONTROLLER, this, msg.sig));
+        // ROLE_ACCESS_CONTROLLER must be present
+        // under the new policy. This provides some
+        // protection against locking yourself out.
+        require(newPolicy.allowed(newAccessController, ROLE_ACCESS_CONTROLLER, this, msg.sig));
 
         // We can now safely set the new policy without foot shooting.
         IAccessPolicy oldPolicy = _accessPolicy;

contracts/AccessControl/IAccessControlled.sol

@@ -3,14 +3,17 @@ pragma solidity 0.4.15;
 import './IAccessPolicy.sol';
 
 
+/// @title enables access control in implementing contract
+/// @dev see AccessControlled for implementation
 contract IAccessControlled {
 
     ////////////////////////
     // Events
     ////////////////////////
 
+    /// @dev must log on access policy change
     event LogAccessPolicyChanged(
-        address controler,
+        address controller,
         IAccessPolicy oldPolicy,
         IAccessPolicy newPolicy
     );
@@ -19,6 +22,15 @@ contract IAccessControlled {
     // Public functions
     ////////////////////////
 
+    /// @dev allows to change access control mechanism for this contract
+    ///     this method must be itself access controlled, see AccessControlled implementation and notice below
+    /// @notice it is a huge issue for Solidity that modifiers are not part of function signature
+    ///     then interfaces could be used for example to control access semantics
+    /// @param newPolicy new access policy to controll this contract
+    /// @param newAccessController address of ROLE_ACCESS_CONTROLLER of new policy that can set access to this contract
+    function setAccessPolicy(IAccessPolicy newPolicy, address newAccessController)
+        public;
+
     function accessPolicy()
         public
         constant

contracts/AccessControl/IAccessPolicy.sol

@@ -3,14 +3,20 @@ pragma solidity 0.4.15;
 import './IAccessControlled.sol';
 
 
+/// @title provides subject to role checking logic
 contract IAccessPolicy {
 
     ////////////////////////
     // Public functions
     ////////////////////////
 
-    // Note: we don't make this function constant to allow for
-    //       state-updating access controls such as rate limiting.
+    /// @notice We don't make this function constant to allow for state-updating access controls such as rate limiting.
+    /// @dev checks if subject belongs to requested role for particular object
+    /// @param subject address to be checked against role, typically msg.sender
+    /// @param role identifier of required role
+    /// @param object contract instance context for role checking, typically contract requesting the check
+    /// @param verb additional data, in current AccessControll implementation msg.sig
+    /// @return if subject belongs to a role
     function allowed(
         address subject,
         bytes32 role,

contracts/AccessControl/RoleBasedAccessControl.sol → contracts/AccessControl/RoleBasedAccessPolicy.sol

@@ -6,7 +6,21 @@ import './AccessControlled.sol';
 import '../Reclaimable.sol';
 
 
-contract RoleBasedAccessControl is
+/// @title access policy based on Access Control Lists concept
+/// @dev Allows to assign an address to a set of roles (n:n relation) and querying if such specific assignment exists.
+///     This assignment happens in two contexts:
+///         - contract context which allows to build a set of local permissions enforced for particular contract
+///         - global context which defines set of global permissions that apply to any contract using this RoleBasedAccessPolicy as Access Policy
+///     Permissions are cascading as follows
+///         - evaluate permission for given subject for given object (local context)
+///         - evaluate permission for given subject for all objects (global context)
+///         - evaluate permissions for any subject (everyone) for given object (everyone local context)
+///         - evaluate permissions for any subject (everyone) for all objects (everyone global context)
+///         - if still unset then disallow
+///     Permission is cascaded up only if it was evaluated as Unset at particular level. See EVERYONE and GLOBAL definitions for special values (they are 0x0 addresses)
+///     RoleBasedAccessPolicy is its own policy. When created, creator has ROLE_ACCESS_CONTROLLER role. Right pattern is to transfer this control to some other (non deployer) account and then destroy deployer private key.
+///     See IAccessControlled for definitions of subject, object and role
+contract RoleBasedAccessPolicy is
     IAccessPolicy,
     AccessControlled,
     Reclaimable
@@ -35,19 +49,21 @@ contract RoleBasedAccessControl is
     // Mutable state
     ////////////////////////
 
-    // subject → role → object → allowed
+    /// @dev subject → role → object → allowed
     mapping (address =>
         mapping(bytes32 =>
             mapping(address => TriState))) private _access;
 
-    // object → role → addresses
+    /// @notice used to enumerate all users assigned to given role in object context
+    /// @dev object → role → addresses
     mapping (address =>
         mapping(bytes32 => address[])) private _accessList;
 
     ////////////////////////
     // Events
     ////////////////////////
 
+    /// @dev logs change of permissions, 'controller' is an address with ROLE_ACCESS_CONTROLLER
     event LogAccessChanged(
         address controller,
         address indexed subject,
@@ -69,7 +85,7 @@ contract RoleBasedAccessControl is
     // Constructor
     ////////////////////////
 
-    function RoleBasedAccessControl()
+    function RoleBasedAccessPolicy()
         AccessControlled(this) // We are our own policy. This is immutable.
     {
         // Issue the local and global AccessContoler role to creator
@@ -81,12 +97,12 @@ contract RoleBasedAccessControl is
     // Public functions
     ////////////////////////
 
-    // Overrides `AccessControlled.setAccessPolicy(IAccessPolicy)`
-    function setAccessPolicy(IAccessPolicy)
+    // Overrides `AccessControlled.setAccessPolicy(IAccessPolicy,address)`
+    function setAccessPolicy(IAccessPolicy, address)
         public
         only(ROLE_ACCESS_CONTROLLER)
     {
-        // `RoleBasedAccessControl` always controls its
+        // `RoleBasedAccessPolicy` always controls its
         // own access. Disallow changing this by overriding
         // the `AccessControlled.setAccessPolicy` function.
         revert();
@@ -126,6 +142,7 @@ contract RoleBasedAccessControl is
             set = value != TriState.Unset;
             allow = value == TriState.Allow;
         }
+        // If none is set then disallow
         if (!set) {
             allow = false;
         }
@@ -218,15 +235,20 @@ contract RoleBasedAccessControl is
 
         // Update the list on add / remove
         address[] storage list = _accessList[object][role];
+        // Add new subject only when going form Unset to Allow/Deny
         if(oldValue == TriState.Unset && newValue != TriState.Unset) {
             list.push(subject);
         }
+        // Remove subject when unsetting Allow/Deny
         if(oldValue != TriState.Unset && newValue == TriState.Unset) {
             for(uint256 i = 0; i < list.length; i++) {
                 if(list[i] == subject) {
+                    // replace unset address with last address in the list, cut list size
                     list[i] = list[list.length - 1];
                     delete list[list.length - 1];
                     list.length -= 1;
+                    // there will be no more matches
+                    break;
                 }
             }
         }

contracts/AccessControl/StandardRoles.sol

@@ -7,7 +7,7 @@ contract StandardRoles {
     // Constants
     ////////////////////////
 
-    // NOTE: Soldity somehow doesn't evaluate this compile time
-    // keccak256("AccessController")
+    // @notice Soldity somehow doesn't evaluate this compile time
+    // @dev role which has rights to change permissions and set new policy in contract, keccak256("AccessController")
     bytes32 internal constant ROLE_ACCESS_CONTROLLER = 0xac42f8beb17975ed062dcb80c63e6d203ef1c2c335ced149dc5664cc671cb7da;
 }

contracts/Reclaimable.sol

@@ -5,6 +5,8 @@ import './AccessControl/AccessControlled.sol';
 import './AccessRoles.sol';
 
 
+/// @title allows contract to reclaim ether or any token sent to it
+/// @dev requires ROLE_RECLAIMER permission, tokens must implement IBasicToken which defines 'balanceOf' and 'transfer'
 contract Reclaimable is AccessControlled, AccessRoles {
 
     ////////////////////////

contracts/test/TestAccessControl.sol

@@ -1,7 +1,7 @@
 pragma solidity 0.4.15;
 
 import '../AccessControl/AccessControlled.sol';
-import '../AccessControl/RoleBasedAccessControl.sol';
+import '../AccessControl/RoleBasedAccessPolicy.sol';
 
 
 contract TestAccessControlExampleRoles {
@@ -17,6 +17,39 @@ contract TestAccessControlExampleRoles {
 
 contract TestAccessControl is AccessControlled, TestAccessControlExampleRoles {
 
+    ////////////////
+    // Types
+    ////////////////
+
+    // Łukasiewicz logic values
+    enum TriState {
+        Unset,
+        Allow,
+        Deny
+    }
+
+    ////////////////////////
+    // Events
+    ////////////////////////
+
+    /// @dev just to have events ABIs as truffle will not handle events from internal transactions to other contracts
+    event LogAccessChanged(
+        address controller,
+        address indexed subject,
+        bytes32 role,
+        IAccessControlled indexed object,
+        TriState oldValue,
+        TriState newValue
+    );
+
+    event LogAccess(
+        address indexed subject,
+        bytes32 role,
+        IAccessControlled indexed object,
+        bytes4 verb,
+        bool granted
+    );
+
     ////////////////////////
     // Constructor
     ////////////////////////
@@ -36,19 +69,3 @@ contract TestAccessControl is AccessControlled, TestAccessControlExampleRoles {
     {
     }
 }
-
-
-// derives from RoleBasedAccessControl just to have events ABIs as truffle will not handle
-// events from internal transactions to other contracts
-// do not change derivation order, RoleBasedAccessControl must be first for tests to pass
-contract TestAccessControlTruffleMixin is RoleBasedAccessControl, TestAccessControl {
-
-    ////////////////////////
-    // Constructor
-    ////////////////////////
-
-    function TestAccessControlTruffleMixin(IAccessPolicy policy)
-        TestAccessControl(policy)
-    {
-    }
-}

migrations/2_deploy_contracts.js

@@ -1,7 +1,7 @@
 require("babel-register");
 const getConfig = require("./config").default;
 
-const RoleBasedAccessControl = artifacts.require("RoleBasedAccessControl");
+const RoleBasedAccessPolicy = artifacts.require("RoleBasedAccessPolicy");
 const EthereumForkArbiter = artifacts.require("EthereumForkArbiter");
 const Neumark = artifacts.require("Neumark");
 const LockedAccount = artifacts.require("LockedAccount");
@@ -19,34 +19,34 @@ module.exports = function deployContracts(deployer, network, accounts) {
   console.log("----------------------------------");
 
   deployer.then(async () => {
-    console.log("AccessControl deployment...");
-    await deployer.deploy(RoleBasedAccessControl);
-    const accessControl = await RoleBasedAccessControl.deployed();
+    console.log("AccessPolicy deployment...");
+    await deployer.deploy(RoleBasedAccessPolicy);
+    const accessPolicy = await RoleBasedAccessPolicy.deployed();
 
     console.log("EthereumForkArbiter deployment...");
-    await deployer.deploy(EthereumForkArbiter, accessControl.address);
+    await deployer.deploy(EthereumForkArbiter, accessPolicy.address);
     const ethereumForkArbiter = await EthereumForkArbiter.deployed();
 
     console.log("Neumark deploying...");
     await deployer.deploy(
       Neumark,
-      accessControl.address,
+      accessPolicy.address,
       ethereumForkArbiter.address
     );
     const neumark = await Neumark.deployed();
 
     console.log("EtherToken deploying...");
-    await deployer.deploy(EtherToken, accessControl.address);
+    await deployer.deploy(EtherToken, accessPolicy.address);
     const etherToken = await EtherToken.deployed();
 
     console.log("EuroToken deploying...");
-    await deployer.deploy(EuroToken, accessControl.address);
+    await deployer.deploy(EuroToken, accessPolicy.address);
     const euroToken = await EuroToken.deployed();
 
     console.log("LockedAccount(EtherToken) deploying...");
     await deployer.deploy(
       LockedAccount,
-      accessControl.address,
+      accessPolicy.address,
       etherToken.address,
       neumark.address,
       CONFIG.LOCK_DURATION,
@@ -57,7 +57,7 @@ module.exports = function deployContracts(deployer, network, accounts) {
     console.log("LockedAccount(EuroToken) deploying...");
     await deployer.deploy(
       LockedAccount,
-      accessControl.address,
+      accessPolicy.address,
       euroToken.address,
       neumark.address,
       CONFIG.LOCK_DURATION,
@@ -68,7 +68,7 @@ module.exports = function deployContracts(deployer, network, accounts) {
     console.log("Commitment deploying...");
     await deployer.deploy(
       Commitment,
-      accessControl.address,
+      accessPolicy.address,
       ethereumForkArbiter.address,
       CONFIG.START_DATE,
       CONFIG.addresses.PLATFORM_OPERATOR_WALLET,