Merge pull request #55 from espresso-org/datastoreacl-fix

DatastoreACL fix

contracts/Datastore.sol

@@ -1,8 +1,6 @@
 pragma solidity ^0.4.24;
 
 import "@aragon/os/contracts/apps/AragonApp.sol";
-import "@aragon/os/contracts/acl/ACL.sol";
-import "@aragon/os/contracts/acl/ACLSyntaxSugar.sol";
 import "./DatastoreACL.sol";
 import "./libraries/PermissionLibrary.sol";
 import "./libraries/GroupLibrary.sol";
@@ -15,9 +13,9 @@ contract Datastore is AragonApp {
     using GroupLibrary for GroupLibrary.GroupData;
 
     bytes32 constant public DATASTORE_MANAGER_ROLE = keccak256("DATASTORE_MANAGER_ROLE");
-    bytes32 constant public FILE_OWNER_ROLE = keccak256("FILE_OWNER_ROLE");
     bytes32 constant public FILE_READ_ROLE = keccak256("FILE_READ_ROLE");
     bytes32 constant public FILE_WRITE_ROLE = keccak256("FILE_WRITE_ROLE");
+    bytes32 constant public DATASTORE_GROUP = keccak256("DATASTORE_GROUP");
 
     event FileRename(address indexed entity);
     event FileContentUpdate(address indexed entity);
@@ -72,14 +70,14 @@ contract Datastore is AragonApp {
         _;
     }    
 
-    function init(address _datastoreACL) onlyInit public
+    function initialize(address _datastoreACL) onlyInit public
     {
         initialized();
 
         datastoreACL = DatastoreACL(_datastoreACL);
 
-        permissions.init(datastoreACL);
-        groups.init(datastoreACL);
+        permissions.initialize(datastoreACL, FILE_READ_ROLE, FILE_WRITE_ROLE);
+        groups.initialize(datastoreACL, DATASTORE_GROUP);
     }      
 
     /**

contracts/DatastoreACL.sol

@@ -1,22 +1,18 @@
 pragma solidity ^0.4.24;
 
 import '@aragon/os/contracts/apps/AragonApp.sol';
-import '@aragon/os/contracts/acl/ACL.sol';
-import '@aragon/os/contracts/acl/ACLSyntaxSugar.sol';
 
 
 
 contract DatastoreACL is AragonApp, ACLHelpers {
 
     bytes32 public constant DATASTOREACL_ADMIN_ROLE = keccak256("DATASTOREACL_ADMIN_ROLE");
-    bytes32 public constant EMPTY_PARAM_HASH = 0x290decd9548b62a8d60345a988386fc84ba6bc95484008f6362f93160ef3e563;
-    bytes32 public constant NO_PERMISSION = bytes32(0);
 
     event SetObjectPermission(address indexed entity, bytes32 indexed obj, bytes32 indexed role, bool allowed);
     event ChangeObjectPermissionManager(bytes32 indexed obj, bytes32 indexed role, address indexed manager);
 
 
-    mapping (bytes32 => mapping (bytes32 => bytes32)) internal objectPermissions; 
+    mapping (bytes32 => mapping (bytes32 => bool)) internal objectPermissions; 
     mapping (bytes32 => address) internal objectPermissionManager;
 
 
@@ -43,7 +39,6 @@ contract DatastoreACL is AragonApp, ACLHelpers {
     */
     function createObjectPermission(address _entity, uint256 _obj, bytes32 _role, address _permissionManager)
         external
-        auth(DATASTOREACL_ADMIN_ROLE)
     {
         createObjectPermission(_entity, keccak256(abi.encodePacked(_obj)), _role, _permissionManager);
     } 
@@ -59,38 +54,33 @@ contract DatastoreACL is AragonApp, ACLHelpers {
         public
         auth(DATASTOREACL_ADMIN_ROLE)
     {
-        _setObjectPermission(_entity, _obj, _role, EMPTY_PARAM_HASH);
+        _setObjectPermission(_entity, _obj, _role, true);
         _setObjectPermissionManager(_permissionManager, _obj, _role);
     }       
 
 
     /**
-    * @dev Function called to verify permission for role `_what` and uint object `_obj` status on `_who`
-    * @param _who Address of the entity
+    * @dev Function called to verify permission for role `_role` and uint object `_obj` status on `_entity`
+    * @param _entity Address of the entity
     * @param _obj Object
-    * @param _what Identifier for the group of actions in app given access to perform
+    * @param _role Identifier for the group of actions in app given access to perform
     * @return boolean indicating whether the ACL allows the role or not
     */
-    function hasObjectPermission(address _who, uint256 _obj, bytes32 _what) public view returns (bool)
+    function hasObjectPermission(address _entity, uint256 _obj, bytes32 _role) public view returns (bool)
     {
-        return hasObjectPermission(_who, keccak256(abi.encodePacked(_obj)), _what);
+        return hasObjectPermission(_entity, keccak256(abi.encodePacked(_obj)), _role);
     }  
 
     /**
-    * @dev Function called to verify permission for role `_what` and an object `_obj` status on `_who`
-    * @param _who Address of the entity
+    * @dev Function called to verify permission for role `_role` and an object `_obj` status on `_entity`
+    * @param _entity Address of the entity
     * @param _obj Object
-    * @param _what Identifier for the group of actions in app given access to perform
+    * @param _role Identifier for the group of actions in app given access to perform
     * @return boolean indicating whether the ACL allows the role or not
     */
-    function hasObjectPermission(address _who, bytes32 _obj, bytes32 _what) public view returns (bool)
+    function hasObjectPermission(address _entity, bytes32 _obj, bytes32 _role) public view returns (bool)
     {
-        bytes32 whoParams = objectPermissions[_obj][permissionHash(_who, _what)];
-        if (whoParams != NO_PERMISSION) {
-            return true;
-        }
-
-        return false;
+        return objectPermissions[_obj][permissionHash(_entity, _role)];
     }       
 
     /**
@@ -117,12 +107,9 @@ contract DatastoreACL is AragonApp, ACLHelpers {
     function grantObjectPermission(address _entity, bytes32 _obj, bytes32 _role, address _sender)
         public
         auth(DATASTOREACL_ADMIN_ROLE)
+        onlyPermissionManager(_sender, _obj, _role)
     {
-
-        if (getObjectPermissionManager(_obj, _role) == 0)
-            createObjectPermission(_entity, _obj, _role, _sender);
-
-        _setObjectPermission(_entity, _obj, _role, EMPTY_PARAM_HASH);
+        _setObjectPermission(_entity, _obj, _role, true);
     }
 
 
@@ -148,9 +135,10 @@ contract DatastoreACL is AragonApp, ACLHelpers {
     */
     function revokeObjectPermission(address _entity, bytes32 _obj, bytes32 _role, address _sender)
         public
+        auth(DATASTOREACL_ADMIN_ROLE)
         onlyPermissionManager(_sender, _obj, _role)
     {
-        _setObjectPermission(_entity, _obj, _role, NO_PERMISSION);
+        _setObjectPermission(_entity, _obj, _role, false);
     }
 
 
@@ -180,24 +168,23 @@ contract DatastoreACL is AragonApp, ACLHelpers {
     /**
     * @dev Internal function called to actually save the permission
     */
-    function _setObjectPermission(address _entity, bytes32 _obj, bytes32 _role, bytes32 _paramsHash) internal {
-        objectPermissions[_obj][permissionHash(_entity, _role)] = _paramsHash;
-        bool entityHasPermission = _paramsHash != NO_PERMISSION;
+    function _setObjectPermission(address _entity, bytes32 _obj, bytes32 _role, bool _hasPermission) internal {
+        objectPermissions[_obj][permissionHash(_entity, _role)] = _hasPermission;
 
-        emit SetObjectPermission(_entity, _obj, _role, entityHasPermission);
+        emit SetObjectPermission(_entity, _obj, _role, _hasPermission);
     }   
 
     function _setObjectPermissionManager(address _newManager, bytes32 _obj, bytes32 _role) internal {
         objectPermissionManager[objectRoleHash(_obj, _role)] = _newManager;
         emit ChangeObjectPermissionManager(_obj, _role, _newManager);
     }
 
-    function permissionHash(address _who, bytes32 _what) internal pure returns (bytes32) {
-        return keccak256(abi.encodePacked("OBJECT_PERMISSION", _who, _what));
+    function permissionHash(address _entity, bytes32 _role) internal pure returns (bytes32) {
+        return keccak256(abi.encodePacked("OBJECT_PERMISSION", _entity, _role));
     } 
 
-    function objectRoleHash(bytes32 _obj, bytes32 _what) internal pure returns (bytes32) {
-        return keccak256(abi.encodePacked("OBJECT_ROLE", _obj, _what));
+    function objectRoleHash(bytes32 _obj, bytes32 _role) internal pure returns (bytes32) {
+        return keccak256(abi.encodePacked("OBJECT_ROLE", _obj, _role));
     }     
 
 

contracts/libraries/GroupLibrary.sol

@@ -1,9 +1,12 @@
 pragma solidity ^0.4.24;
 
 import "../DatastoreACL.sol";
+import "@aragon/os/contracts/lib/math/SafeMath.sol";
 
 
 library GroupLibrary {
+    using SafeMath for uint256;
+
     /**
      * Represents a group and its entities within it   
      */
@@ -25,8 +28,8 @@ library GroupLibrary {
     }
 
 
-    function init(GroupData storage _self, DatastoreACL _acl) internal {
-        _self.DATASTORE_GROUP = keccak256("DATASTORE_GROUP");
+    function initialize(GroupData storage _self, DatastoreACL _acl, bytes32 _DATASTORE_GROUP) internal {
+        _self.DATASTORE_GROUP = _DATASTORE_GROUP;
         _self.acl = _acl;
     }
 
@@ -36,7 +39,7 @@ library GroupLibrary {
      * @param _groupName Name of the group
      */
     function createGroup(GroupData storage _self, string _groupName) internal returns (uint) {
-        uint id = _self.groupList.length + 1;
+        uint id = _self.groupList.length.add(1);
         _self.groups[id].groupName = _groupName;
         _self.groups[id].exists = true;
         _self.groupList.push(id);
@@ -51,7 +54,7 @@ library GroupLibrary {
      */
     function deleteGroup(GroupData storage _self, uint _groupId) internal {
         delete _self.groups[_groupId];
-        delete _self.groupList[_groupId - 1];
+        delete _self.groupList[_groupId.sub(1)];
     }
 
     /**
@@ -91,8 +94,9 @@ library GroupLibrary {
      * @param _entity Address of the entity
      */
     function addEntityToGroup(GroupData storage _self, uint _groupId, address _entity) internal {
-        _self.groups[_groupId].entitiesWithIndex[_entity] = _self.groups[_groupId].entities.length + 1;
+        _self.groups[_groupId].entitiesWithIndex[_entity] = _self.groups[_groupId].entities.length.add(1);
         _self.groups[_groupId].entities.push(_entity);
+        _self.acl.createObjectPermission(_entity, _groupId, _self.DATASTORE_GROUP, this);
         _self.acl.grantObjectPermission(_entity, _groupId, _self.DATASTORE_GROUP, this);
     }
 
@@ -105,7 +109,7 @@ library GroupLibrary {
     function removeEntityFromGroup(GroupData storage _self, uint _groupId, address _entity) internal {
         uint indexOfEntity = _self.groups[_groupId].entitiesWithIndex[_entity];
         if (indexOfEntity > 0) {
-            indexOfEntity--;
+            indexOfEntity = indexOfEntity.sub(1);
             delete _self.groups[_groupId].entities[indexOfEntity];
             delete _self.groups[_groupId].entitiesWithIndex[_entity];
             _self.acl.revokeObjectPermission(_entity, _groupId, _self.DATASTORE_GROUP, this);

contracts/libraries/PermissionLibrary.sol

@@ -31,10 +31,10 @@ library PermissionLibrary {
     // ************* PermissionData ************* //
 
 
-    function init(PermissionData storage _self, DatastoreACL _acl) internal {
-        _self.FILE_READ_ROLE = keccak256("FILE_READ_ROLE");
-        _self.FILE_WRITE_ROLE = keccak256("FILE_WRITE_ROLE");
+    function initialize(PermissionData storage _self, DatastoreACL _acl, bytes32 _FILE_READ_ROLE, bytes32 _FILE_WRITE_ROLE) internal {
         _self.acl = _acl;
+        _self.FILE_READ_ROLE = _FILE_READ_ROLE;
+        _self.FILE_WRITE_ROLE = _FILE_WRITE_ROLE;
     }
 
     /**
@@ -64,7 +64,7 @@ library PermissionLibrary {
      * @param _fileId File Id
      */
     function getOwner(PermissionData storage _self, uint _fileId) internal view returns (address) {
-        return _self.acl.getObjectPermissionManager(_fileId, _self.FILE_READ_ROLE);
+        return _self.acl.getObjectPermissionManager(_fileId, _self.FILE_WRITE_ROLE);
     }
 
 
@@ -128,11 +128,15 @@ library PermissionLibrary {
         _self.entityPermissions[_fileId][_entity].read = _read;
         _self.entityPermissions[_fileId][_entity].write = _write;
 
-        if (_read) 
+        if (_read) {
+            _self.acl.createObjectPermission(_entity, _fileId, _self.FILE_READ_ROLE, msg.sender);
             _self.acl.grantObjectPermission(_entity, _fileId, _self.FILE_READ_ROLE, msg.sender);        
+        }
 
-        if (_write) 
+        if (_write) {
+            _self.acl.createObjectPermission(_entity, _fileId, _self.FILE_WRITE_ROLE, msg.sender);
             _self.acl.grantObjectPermission(_entity, _fileId, _self.FILE_WRITE_ROLE, msg.sender);
+        }
 
 
         //NewWritePermission(msg.sender, _fileId);

package.json

@@ -1,6 +1,6 @@
 {
   "name": "@espresso-org/aragon-datastore",
-  "version": "0.0.5",
+  "version": "0.0.6",
   "description": "Datastore based on AragonOS",
   "main": "dist/index.js",
   "typings": "dist/index.d.ts",

test/datastore-acl.js

@@ -22,6 +22,7 @@ contract('DatastoreACL ', accounts => {
 
     const root = accounts[0]
     const holder = accounts[1]
+    const account3 = accounts[2]
     const DUMMY_ROLE = 1
 
 
@@ -58,26 +59,50 @@ contract('DatastoreACL ', accounts => {
 
 
         await datastoreACL.initialize() 
-        await datastore.init(datastoreACL.address)
+        await datastore.initialize(datastoreACL.address)
 
         await acl.grantPermission(datastoreACL.address, acl.address, await acl.CREATE_PERMISSIONS_ROLE())
 
     })
 
 
+    describe('createObjectPermission', async () => {
+        it('fires SetObjectPermission event', async () => {
+            await datastoreACL.createObjectPermission(root, 1, DUMMY_ROLE, root)
+
+            await assertEvent(datastoreACL, { event: 'SetObjectPermission' })
+        }) 
+
+        it('fires ChangeObjectPermissionManager event', async () => {
+            await datastoreACL.createObjectPermission(root, 1, DUMMY_ROLE, root)
+
+            await assertEvent(datastoreACL, { event: 'ChangeObjectPermissionManager' })
+        })         
+    }) 
+
+
     describe('revokeObjectPermission', async () => {
         it('throws if not called the permission manager', async () => {
-            await datastoreACL.grantObjectPermission(root, 1, DUMMY_ROLE, root)
+            await datastoreACL.createObjectPermission(root, 1, DUMMY_ROLE, root)
             assertThrow(async () => await datastoreACL.revokeObjectPermission(root, 1, DUMMY_ROLE, holder))
         })
+
     })   
 
     describe('grantObjectPermission', async () => {
         it('throws if not called the permission manager', async () => {
             await datastoreACL.createObjectPermission(root, 1, DUMMY_ROLE, root)
             assertThrow(async () => datastoreACL.grantObjectPermission(root, 1, DUMMY_ROLE, holder), { from: holder} )
         })
-    })       
+    })   
+
+    describe('hasObjectPermission', async () => {
+        it('returns the right permission', async () => {
+            await datastoreACL.createObjectPermission(holder, 1, DUMMY_ROLE, root)
+            assert.equal(await datastoreACL.hasObjectPermission.call(holder, 1, DUMMY_ROLE), true)
+            assert.equal(await datastoreACL.hasObjectPermission.call(account3, 1, DUMMY_ROLE), false)
+        })
+    })      
 
 })
 

test/datastore.js

@@ -58,7 +58,7 @@ contract('Datastore ', accounts => {
 
 
         await datastoreACL.initialize() 
-        await datastore.init(datastoreACL.address)
+        await datastore.initialize(datastoreACL.address)
 
         await acl.grantPermission(datastoreACL.address, acl.address, await acl.CREATE_PERMISSIONS_ROLE())
     })