feat: use context-based role assignments to implement the permission model #19

hiddentao · 2025-02-23T08:08:59Z

Notes:

All new ACL that allows for context-based role setting and other fun features. Fully tested!
The transfer lock is complex to do using roles because it would require going into the ERC1155 too to handle approvals etc. So I opted for a flag instead for that one.

coveralls · 2025-02-23T08:10:38Z

Pull Request Test Coverage Report for Build 13805766124

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

94 of 180 (52.22%) changed or added relevant lines in 9 files are covered.
4 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-3.2%) to 54.758%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
contracts/src/registry/RootRegistry.sol	2	3	66.67%
contracts/src/registry/PermissionedRegistry.sol	2	4	50.0%
contracts/src/registry/NameWrapperRegistry.sol	0	83	0.0%

Files with Coverage Reduction	New Missed Lines	%
contracts/src/registry/RootRegistry.sol	1	75.0%
contracts/src/registry/PermissionedRegistry.sol	3	73.68%

Totals
Change from base Build 13351832110:	-3.2%
Covered Lines:	278
Relevant Lines:	495

💛 - Coveralls

…even when user-facing token id is re-generated

contracts/src/registry/EnhancedAccessControl.sol

contracts/src/registry/NameWrapperRegistry.sol

feat: add _transferRole method

hiddentao · 2025-02-28T05:22:44Z

@Arachnid Given that we only need to transfer two roles to a new registrar (registrar and renew) I felt that it was easier to do that manually whilst keeping the current storage approach in EnhancedAccessControl, i.e. not using a bitmap. Even if we use a Bitmap then keeping the "revoke all assignments of this role within this resource" O(1) feature would still require versioning of assignments, meaning that we'd still be doing a lot of complex storage work in access control.

Plus, role transfers only work across a particular context. There's no way to transfer all of a user's roles in all contexts. I suppose if we implement hierarchical contexts of some sort we could perhaps do this. But again, this complicates the access control storage.

Happy to discuss this further over calls.

…rom one account to another

contracts/src/registry/BaseRegistry.sol

contracts/src/registry/EnhancedAccessControl.sol

Arachnid · 2025-03-11T09:05:13Z

contracts/src/registry/EnhancedAccessControl.sol

+    /** 
+     * @dev admin role that controls a given role. 
+     * RoleId -> AdminRoleId
+     */
+    mapping(uint256 role => uint256 adminRole) private _adminRoles;


Can we simply say that the upper 128 bits are admin roles and the lower 128 bits are user roles, and thus remove the need to map them explicitly?

I think it's possible that a role can be a role as well as an admin role - e.g registrar can be admin roles for setting the renewer admin role, even though registrars are themselves roles.

I don't think there's in harm in representing those with separate bits, though? If they're typically used together, the inheriting contract can simply grant or revoke both bits together.

Arachnid · 2025-03-11T09:06:23Z

contracts/src/registry/EnhancedAccessControl.sol

+    /**
+     * @dev locked roles within a resource stored as a bitmap.
+     * Resource -> LockedRoleBitmap
+     */
+    mapping(bytes32 resource => uint256 lockedRoleBitmap) private _lockedRoles;


Is this necessary? If a role for a given resource has no admins, it's locked implicitly.

The issue is that the DEFAULT_ADMIN_ROLE can always revoke a role, thus I figured we needed an explicit locking mechanism to prevent that.

With the admin-bit-for-each-role I suggested above, there is no DEFAULT_ADMIN_ROLE - only individual admins for each role.

contracts/src/registry/EnhancedAccessControl.sol

Arachnid · 2025-03-11T09:11:46Z

contracts/src/registry/EnhancedAccessControl.sol

+     *
+     * May emit a {RoleGranted} event.
+     */
+    function grantRole(bytes32 resource, uint256 role, address account) public virtual canGrantRole(resource, role) returns (bool) {


Suggested change

function grantRole(bytes32 resource, uint256 role, address account) public virtual canGrantRole(resource, role) returns (bool) {

function grantRoles(bytes32 resource, uint256 role, address account) public virtual canGrantRole(resource, role) returns (bool) {

This isn't as straightforward since we need to check that the caller has the admin role for the given role - and we don't want to be looping through and checking the caller is an admin for every role passed in.

The alternative is to be able to set an admin for a bitmap of roles, but then that semantically doesn't make any sense.

Hence why granting/revoking one role at a time makes the most sense.

If we do the admin-role-per-role I suggested above, this is doable with simple bitmasking. Eg:

uint256 caller_roles = getRoles(resource, msg.sender); uint256 caller_admin_roles = (caller_roles & 0xffffffffffffffffffffffffffffffff) | (caller_roles >> 128); role &= caller_admin_roles;

This will take the role input and mask out any roles the caller doesn't have permission to grant.

Arachnid · 2025-03-11T09:12:00Z

contracts/src/registry/EnhancedAccessControl.sol

+     *
+     * May emit a {RoleRevoked} event.
+     */
+    function revokeRole(bytes32 resource, uint256 role, address account) public virtual canGrantRole(resource, role) returns (bool) {


Suggested change

function revokeRole(bytes32 resource, uint256 role, address account) public virtual canGrantRole(resource, role) returns (bool) {

function revokeRoles(bytes32 resource, uint256 role, address account) public virtual canGrantRole(resource, role) returns (bool) {

This isn't as straightforward since we need to check that the caller has the admin role for the given role - and we don't want to be looping through and checking the caller is an admin for every role passed in.

The alternative is to be able to set an admin for a bitmap of roles, but then that semantically doesn't make any sense.

Hence why granting/revoking one role at a time makes the most sense.

Arachnid · 2025-03-11T09:13:00Z

contracts/src/registry/EnhancedAccessControl.sol

+    /**
+     * @dev Revokes `role` from the calling account.
+     *
+     * Roles are often managed via {grantRole} and {revokeRole}: this function's
+     * purpose is to provide a mechanism for accounts to lose their privileges
+     * if they are compromised (such as when a trusted device is misplaced).
+     *
+     * If the calling account had been revoked `role`, emits a {RoleRevoked}
+     * event.
+     *
+     * Requirements:
+     *
+     * - the caller must be `callerConfirmation`.
+     *
+     * May emit a {RoleRevoked} event.
+        *
+     * Returns `true` if the role was revoked, `false` otherwise.
+     */
+    function renounceRole(bytes32 resource, uint256 role, address callerConfirmation) public virtual returns (bool) {
+        if (callerConfirmation != _msgSender()) {
+            revert EnhancedAccessControlBadConfirmation();
+        }
+
+        return _revokeRoles(resource, role, callerConfirmation);
+    }


I think we should omit this; if an account is an admin for the role, they can revoke both the role and their own admin status, but having this makes it impossible to 'lock open' a permission.

I don't really understand what you mean by "lock open" here.

Revoke all admin access over a role, without revoking the role itself for everyone - meaning it can still be done, but the set of accounts that can do it can't be changed any longer.

contracts/src/registry/EnhancedAccessControl.sol

Co-authored-by: Nick Johnson <[email protected]>

feat: more powerful access control base contract

8d429e3

hiddentao marked this pull request as draft February 23, 2025 08:09

hiddentao added 5 commits February 23, 2025 16:16

refactor: port existing code over to using new access control base

2fa2d2e

feat: name wrapper registry utilising new ACL system

d473e38

refactor: better method names

ae0012b

feat: generate token id context so that we can keep role assignments …

67ed76d

…even when user-facing token id is re-generated

refactor: use a flag for transfer locks instead

6b722d0

hiddentao requested a review from Arachnid February 23, 2025 09:24

hiddentao added 2 commits February 23, 2025 19:07

test: grantRole return value

13fd262

fix: allowOwnerToRenew is to be called by the emancipator

8a76088

Arachnid requested changes Feb 23, 2025

View reviewed changes

refactor: improvements based on review feedback

8739269

hiddentao mentioned this pull request Feb 25, 2025

feat: migration of fuses to v2 #17

Closed

hiddentao added 2 commits February 26, 2025 06:58

refactor: updates based on feedback

33b75e4

refactor: simplify roles by introducing role groups

1f15b51

hiddentao requested a review from Arachnid February 26, 2025 11:19

refactor: renamed context to resource

e9d4c9e

feat: add _transferRole method

hiddentao added 7 commits March 5, 2025 15:43

refactor: acl roles are now a 256-bit bitmap and set against context

4f0cb04

feat: can revoke all roles for user within a context and copy roles f…

8bbed09

…rom one account to another

chore: work towards new acl system

be3a052

fix: tests for new acl

28c9213

refactor: simplify acl to allow for easy role bitmap construction

9610d25

refactor: eth registry now works with new roles arch

74c865b

chore: rewrote name wrapper for new scheme

c55304e

Arachnid requested changes Mar 11, 2025

View reviewed changes

hiddentao marked this pull request as ready for review March 11, 2025 10:44

hiddentao and others added 2 commits March 12, 2025 14:34

refactor: shorten the prefix for EnhancedAccessControl errors and events

df92ee5

Update contracts/src/registry/EnhancedAccessControl.sol

f5a9b29

Co-authored-by: Nick Johnson <[email protected]>

hiddentao and others added 4 commits March 12, 2025 14:46

Update contracts/src/registry/EnhancedAccessControl.sol

fa9ee82

Co-authored-by: Nick Johnson <[email protected]>

fix: failing tests due to fixed hasRoles impl

219fa34

refactor: brought in NameUtils.labelToTokenId from other PR

cdaf559

refactor: _copyRoles now just copies within the same resource

b6a02d8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use context-based role assignments to implement the permission model #19

feat: use context-based role assignments to implement the permission model #19

hiddentao commented Feb 23, 2025 •

edited

Loading

coveralls commented Feb 23, 2025 •

edited

Loading

hiddentao commented Feb 28, 2025 •

edited

Loading

Arachnid Mar 11, 2025

hiddentao Mar 12, 2025

Arachnid Mar 12, 2025

Arachnid Mar 11, 2025

hiddentao Mar 12, 2025

Arachnid Mar 12, 2025

Arachnid Mar 11, 2025

hiddentao Mar 12, 2025

Arachnid Mar 12, 2025

Arachnid Mar 11, 2025

hiddentao Mar 12, 2025

Arachnid Mar 11, 2025

hiddentao Mar 12, 2025

Arachnid Mar 12, 2025

	function grantRole(bytes32 resource, uint256 role, address account) public virtual canGrantRole(resource, role) returns (bool) {
	function grantRoles(bytes32 resource, uint256 role, address account) public virtual canGrantRole(resource, role) returns (bool) {

	function revokeRole(bytes32 resource, uint256 role, address account) public virtual canGrantRole(resource, role) returns (bool) {
	function revokeRoles(bytes32 resource, uint256 role, address account) public virtual canGrantRole(resource, role) returns (bool) {

feat: use context-based role assignments to implement the permission model #19

Are you sure you want to change the base?

feat: use context-based role assignments to implement the permission model #19

Conversation

hiddentao commented Feb 23, 2025 • edited Loading

coveralls commented Feb 23, 2025 • edited Loading

Pull Request Test Coverage Report for Build 13805766124

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

hiddentao commented Feb 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiddentao commented Feb 23, 2025 •

edited

Loading

coveralls commented Feb 23, 2025 •

edited

Loading

hiddentao commented Feb 28, 2025 •

edited

Loading