Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

revokeAll function #459

Open
wants to merge 8 commits into
base: next
Choose a base branch
from
Open

revokeAll function #459

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8c1a871
Implement revokeAll()
juslar Nov 7, 2018
cd06917
Add tests for revokeAll()
juslar Nov 7, 2018
c8c0d18
Add uint256 adding verification like SafeMath
juslar Nov 8, 2018
0ed9484
Add backward compatibility fix
juslar Nov 19, 2018
b2be01e
Add SafeMath
juslar Jan 14, 2019
a9421cb
Optimize roleHash() calls
juslar Jan 22, 2019
324bde0
Add RevokeAllPermissions event
juslar Feb 13, 2019
9294f59
Fix linting
juslar Feb 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 31 additions & 3 deletions contracts/acl/ACL.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@ import "../common/TimeHelpers.sol";
import "./ACLSyntaxSugar.sol";
import "./IACL.sol";
import "./IACLOracle.sol";

import "../lib/math/SafeMath.sol";

/* solium-disable function-order */
// Allow public initialize() to be first
contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers {
using SafeMath for uint256;

/* Hardcoded constants to save gas
bytes32 public constant CREATE_PERMISSIONS_ROLE = keccak256("CREATE_PERMISSIONS_ROLE");
*/
Expand Down Expand Up @@ -46,6 +48,7 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers {
string private constant ERROR_AUTH_INIT_KERNEL = "ACL_AUTH_INIT_KERNEL";
string private constant ERROR_AUTH_NO_MANAGER = "ACL_AUTH_NO_MANAGER";
string private constant ERROR_EXISTENT_MANAGER = "ACL_EXISTENT_MANAGER";
string private constant ERROR_ROLE_ERA_INCREMENT = "ACL_ROLE_ERA_INCREMENT";

// Whether someone has a permission
mapping (bytes32 => bytes32) internal permissions; // permissions hash => params hash
Expand All @@ -54,6 +57,9 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers {
// Who is the manager of a permission
mapping (bytes32 => address) internal permissionManager;

// Eras for the different roles
mapping (bytes32 => uint256) internal roleEras;

event SetPermission(address indexed entity, address indexed app, bytes32 indexed role, bool allowed);
event SetPermissionParams(address indexed entity, address indexed app, bytes32 indexed role, bytes32 paramsHash);
event ChangePermissionManager(address indexed app, bytes32 indexed role, address indexed manager);
Expand Down Expand Up @@ -145,6 +151,21 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers {
_setPermission(_entity, _app, _role, NO_PERMISSION);
}

/**
* @dev Revokes all permissions for a specified role if allowed. This requires `msg.sender` to be the the permission manager
* @notice Revoke from all the ability to perform actions requiring `_role` on `_app`
* @param _app Address of the app in which the role will be revoked
* @param _role Identifier for the group of actions in app being revoked
*/
function revokeAll(address _app, bytes32 _role)
external
onlyPermissionManager(_app, _role)
{
uint256 newRoleEra = roleEras[roleHash(_app, _role)].add(1);
require(newRoleEra >= roleEras[roleHash(_app, _role)], ERROR_ROLE_ERA_INCREMENT);
roleEras[roleHash(_app, _role)] = newRoleEra;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if using a pointer to roleEras[roleHash(_app, _role)] could save some gas here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean using a storage pointer? I don't think I can on a uint256 value. Maybe I'm missing something though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes, you are right, sorry for the confusion. We could still save 2 calls to roleHash function, but as it's going to be one less if you remove the require, I don't know if that's a big gain.

Copy link
Contributor

@sohkai sohkai Jan 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably still a decent net gain, since there also wouldn't be another SLOAD as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good :) Pushed the optimization.

}

/**
* @notice Set `_newManager` as the manager of `_role` in `_app`
* @param _newManager Address for the new manager
Expand Down Expand Up @@ -470,7 +491,14 @@ contract ACL is IACL, TimeHelpers, AragonApp, ACLHelpers {
return keccak256(abi.encodePacked("ROLE", _where, _what));
}

function permissionHash(address _who, address _where, bytes32 _what) internal pure returns (bytes32) {
return keccak256(abi.encodePacked("PERMISSION", _who, _where, _what));
function permissionHash(address _who, address _where, bytes32 _what) internal view returns (bytes32) {
uint256 roleEra = roleEras[roleHash(_where, _what)];

// Backward compatibility for DAOs with earlier versions of the ACL
if (roleEra == 0) {
return keccak256(abi.encodePacked("PERMISSION", _who, _where, _what));
}

return keccak256(abi.encodePacked("PERMISSION", roleEra, _who, _where, _what));
}
}
31 changes: 31 additions & 0 deletions test/kernel_acl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,37 @@ contract('Kernel ACL', accounts => {
})
})

it('can revoke all permissions on a role', async () => {
const receipt = await acl.grantPermission(child, kernelAddr, APP_MANAGER_ROLE, { from: granted })
assertEvent(receipt, 'SetPermission')
const receipt2 = await acl.grantPermission(accounts[3], kernelAddr, APP_MANAGER_ROLE, { from: granted })
assertEvent(receipt2, 'SetPermission')
await acl.revokeAll(kernelAddr, APP_MANAGER_ROLE, { from: granted })
assert.isFalse(await acl.hasPermission(child, kernelAddr, APP_MANAGER_ROLE))
assert.isFalse(await acl.hasPermission(accounts[3], kernelAddr, APP_MANAGER_ROLE))
})

it('can revoke all permissions on a role for the specified app only', async () => {
const receipt = await acl.grantPermission(child, kernelAddr, APP_MANAGER_ROLE, { from: granted })
assertEvent(receipt, 'SetPermission')
const createReceipt = await acl.createPermission(child, kernelAddr, '0x1234', child, { from: permissionsRoot })
assertEvent(createReceipt, 'SetPermission')
assertEvent(createReceipt, 'ChangePermissionManager')
await acl.revokeAll(kernelAddr, APP_MANAGER_ROLE, { from: granted })
assert.isFalse(await acl.hasPermission(child, kernelAddr, APP_MANAGER_ROLE))
assert.isTrue(await acl.hasPermission(child, kernelAddr, '0x1234'))
})

it('can reassign a role with a new era', async () => {
const receipt = await acl.grantPermission(child, kernelAddr, APP_MANAGER_ROLE, { from: granted })
assertEvent(receipt, 'SetPermission')
await acl.revokeAll(kernelAddr, APP_MANAGER_ROLE, { from: granted })
assert.isFalse(await acl.hasPermission(child, kernelAddr, APP_MANAGER_ROLE))
const receipt2 = await acl.grantPermission(child, kernelAddr, APP_MANAGER_ROLE, { from: granted })
assertEvent(receipt2, 'SetPermission')
assert.isTrue(await acl.hasPermission(child, kernelAddr, APP_MANAGER_ROLE))
})

context('> transferring managership', () => {
const newManager = accounts[3]
assert.notEqual(newManager, granted, 'newManager should not be the same as granted')
Expand Down