fix(standards): make RBAC role administration fully role-based by onurinanc · Pull Request #3215 · 0xMiden/protocol

onurinanc · 2026-07-06T10:52:01Z

Closes: #3214

onurinanc · 2026-07-06T11:27:04Z

Related to this PR as well:

feat(agglayer): RBAC-based access control for bridge roles #3130

PhilippGackstatter

Looks good, left a few suggestions.

PhilippGackstatter · 2026-07-06T12:57:14Z

 pub struct RoleSymbol(ShortCapitalString);

+impl Ord for RoleSymbol {
+    fn cmp(&self, other: &Self) -> Ordering {
+        self.as_element().as_canonical_u64().cmp(&other.as_element().as_canonical_u64())
+    }
+}


nit: we usually place trait impls for T after impl T.

PhilippGackstatter · 2026-07-06T12:59:17Z

+# Provides per-role membership tracking with delegated role admins. Members are tracked
+# with a boolean flag and a per-role member count. Role administration is fully role-based:
+# the built-in `ADMIN` role is the default administrator of every role, mirroring the
+# OpenZeppelin AccessControl `DEFAULT_ADMIN_ROLE`. There is no external super-admin above
+# the role graph.


nit: the last sentence feels only relevant for this PR

PhilippGackstatter · 2026-07-06T13:07:23Z

+    dup eq.0
+    # => [is_unset, admin_role_symbol]
+
+    if.true
+        drop push.ADMIN_ROLE
+    end
+    # => [effective_admin_role_symbol]


nit: we can use cdrop here to avoid the branch

PhilippGackstatter · 2026-07-06T13:08:02Z

+#! The effective admin is the role's configured delegated admin, or the built-in `ADMIN`
+#! role when none is configured. There is no owner override: role administration is fully
+#! role-based.


nit: last sentence seems only relevant for this PR

PhilippGackstatter · 2026-07-06T13:10:09Z

+    # Stash the membership flag and role_symbol; both are needed after the map write.
+    dup loc_store.MEMBERSHIP_FLAG_LOC
+    # => [is_member, role_symbol, account_suffix, account_prefix]
+
+    dup.1 loc_store.ROLE_SYMBOL_LOC
+    # => [is_member, role_symbol, account_suffix, account_prefix]
+
+    drop
+    # => [role_symbol, account_suffix, account_prefix]


nit: unnecessary drop-after-dup for is_member

PhilippGackstatter · 2026-07-06T13:29:01Z

+            }
+        }
+
+        // Seed ADMIN membership: [0, ADMIN, suffix, prefix] -> [1, 0, 0, 0].


Suggested change

// Seed ADMIN membership: [0, ADMIN, suffix, prefix] -> [1, 0, 0, 0].

// Seed ADMIN membership: [0, admin_role, suffix, prefix] -> [1, 0, 0, 0].

nit: felts should be lowercase

PhilippGackstatter · 2026-07-06T13:30:54Z

+        // Seed the ADMIN role config with its member count. The delegated admin is left unset
+        // (0) so ADMIN administers itself. When there are no admins, the config stays empty.
+        let role_config_map = if admins.is_empty() {
+            StorageMap::with_entries(vec![]).expect("empty role config map should be valid")


Suggested change

StorageMap::with_entries(vec![]).expect("empty role config map should be valid")

StorageMap::default()

PhilippGackstatter · 2026-07-06T13:32:13Z

+            let member_count =
+                u32::try_from(admins.len()).expect("number of initial admins should fit in u32");


We don't enforce this, afaict, but it's probably fine for practical purposes.

PhilippGackstatter · 2026-07-06T13:38:42Z

+    let tx = mock_chain
+        .build_tx_context(updated.clone(), &[], slice::from_ref(&owner_grant_note))?
+        .build()?;
+    let result = tx.execute().await;


nit: could be written as one statement

PhilippGackstatter · 2026-07-06T13:40:13Z

+    let new_admin = test_account_id(105);
+    let member = test_account_id(106);
+
+    let admin = role("ADMIN");


nit: I'd replace all role("ADMIN") with RoleBasedAccessControl::admin_role() so we use that definition in more places and get better coverage

mmagician

LGTM, nothing blocking ✅

mmagician · 2026-07-06T14:20:45Z

+    # Validate the account ID. This is the only membership-mutating path that validates:
+    # grant is the sole writer of the membership map, so every stored key is already valid
+    # and revoke/renounce need not re-check (an invalid ID cannot match a stored key).


this comment reads as-if something was cut off

mmagician · 2026-07-06T14:25:34Z

-    # => [0, 0, 0, 0, 0, role_symbol, account_suffix, account_prefix, role_symbol]
+    # Build the value word [is_member, 0, 0, 0] on top of the key word.
+    push.0.0.0 loc_load.MEMBERSHIP_FLAG_LOC
+    # => [is_member, 0, 0, 0, 0, role_symbol, account_suffix, account_prefix]


nit: we can group the value word in stack comments as:

# => [[is_member, 0, 0, 0], 0, role_symbol..., ]

mmagician · 2026-07-06T14:34:15Z

+/// Regression test: Once a role is delegated to a dedicated admin role, the general `ADMIN`
+/// role can no longer grant it. The role becomes exclusively controlled by its delegated admin,
+/// so a sensitive capability can be kept out of reach of the general administrator.
+#[tokio::test]
+async fn test_rbac_delegation_locks_out_admin() -> anyhow::Result<()> {


isn't this test effectively the same as test_rbac_delegated_admin_can_clear_own_delegation (different action taken, but same pattern)?
then we could perhaps refactor to use rstest to cover both cases

Fumuran

LGTM, thank you!

Fumuran · 2026-07-06T14:39:47Z

    push.0
-    # => [0, role_symbol, account_suffix, account_prefix, role_symbol]
+    # => [0, role_symbol, account_suffix, account_prefix]

-    push.CLEAR_MEMBERSHIP
-    # => [0, 0, 0, 0, 0, role_symbol, account_suffix, account_prefix, role_symbol]
+    # Build the value word [is_member, 0, 0, 0] on top of the key word.
+    push.0.0.0 loc_load.MEMBERSHIP_FLAG_LOC


Small nit: we can use padw directly instead of push.0.0.0.0

…dmin

partylikeits1983

Looks great! I think we should add some more tests to increase code coverage but otherwise looks good!

partylikeits1983 · 2026-07-06T13:53:36Z

            AccessControl::Rbac { owner, roles } => vec![
                Ownable2Step::new(owner).into(),
-                RoleBasedAccessControl::empty().into(),
+                RoleBasedAccessControl::new(owner).into(),


With this change, being the owner no longer means anything from the RBAC component's point of view. The owner just starts out as the first ADMIN member. So after an ownership transfer via Ownable2Step, the old owner can still manage RBAC roles whereas the new owner in Ownable2Step cannot.

There are no tests for this. Could you add a test that does a full transfer (old owner calls transfer_ownership, new owner calls accept_ownership) and then checks both directions: the new owner's grant_role fails with ERR_SENDER_NOT_ROLE_ADMIN (ownership does not grant RBAC authority), and the old owner's grant_role still succeeds (changing ownership does not revoke RBAC ADMIN authority)?

partylikeits1983 · 2026-07-06T13:59:43Z

 pub proc renounce_role
    dup exec.assert_role_symbol_non_zero
    # => [role_symbol, pad(15)]

    exec.active_note::get_sender
    # => [sender_suffix, sender_prefix, role_symbol, pad(15)]

    movup.2
    # => [role_symbol, sender_suffix, sender_prefix, pad(13)]

-    exec.revoke_role_internal
+    push.0
+    # => [is_member=0, role_symbol, sender_suffix, sender_prefix, pad(13)]
+
+    exec.set_membership_internal
    # => [pad(16)]
 end


The ADMIN role administers itself (its own effective admin is ADMIN), so ADMIN
membership can be granted, revoked, and renounced through the standard API.

The docs say ADMIN membership can be renounced, but no test ever renounces ADMIN, only the PAUSER role. Can you add a test where an admin renounces ADMIN role, including the case where they are the last admin?

…dmin

onurinanc · 2026-07-07T13:42:00Z

I've made the changes accordingly removing the owner dependency specified here: #3215 (comment)

It is ready for review for one more round.

PhilippGackstatter

LGTM!

PhilippGackstatter · 2026-07-08T06:22:58Z

    dup eq.0
    # => [is_unset, admin_role_symbol]

    push.ADMIN_ROLE swap
    # => [is_unset, ADMIN_ROLE, admin_role_symbol]


nit/slight optimization: push.ADMIN_ROLE dup.1 eq.0 cdrop.

mmagician

Changes look good ✅

onurinanc added 4 commits July 6, 2026 12:49

remove super admin from RBAC

55238ef

changelog

39d2da9

fix comments

074d345

make RoleSymbol Ord match its encoded Felt key

49db60a

onurinanc requested review from PhilippGackstatter, bobbinth and mmagician July 6, 2026 11:25

optimize RBAC membership read/write paths

d300faa

PhilippGackstatter approved these changes Jul 6, 2026

View reviewed changes

mmagician approved these changes Jul 6, 2026

View reviewed changes

Fumuran approved these changes Jul 6, 2026

View reviewed changes

onurinanc added 3 commits July 6, 2026 17:10

apply suggestions

a39b187

nit

1c379b3

Merge remote-tracking branch 'origin/next' into refactor-rbac-super-a…

341f301

…dmin

partylikeits1983 reviewed Jul 6, 2026

View reviewed changes

PhilippGackstatter reviewed Jul 6, 2026

View reviewed changes

Comment thread crates/miden-standards/src/account/access/mod.rs Outdated

Fumuran mentioned this pull request Jul 7, 2026

feat(agglayer): RBAC-based access control for bridge roles #3130

Open

onurinanc added 3 commits July 7, 2026 15:30

remove owner dependency from rbac

b37277e

apply suggestions

71470e8

Merge remote-tracking branch 'origin/next' into refactor-rbac-super-a…

a1ee4c5

…dmin

onurinanc mentioned this pull request Jul 7, 2026

feat(standards): add guardian component #3125

Merged

PhilippGackstatter approved these changes Jul 8, 2026

View reviewed changes

Merge branch 'next' into refactor-rbac-super-admin

bd2e5a8

mmagician approved these changes Jul 8, 2026

View reviewed changes

mmagician enabled auto-merge July 8, 2026 06:37

mmagician added this pull request to the merge queue Jul 8, 2026

Merged via the queue into next with commit 32896b1 Jul 8, 2026
19 checks passed

mmagician deleted the refactor-rbac-super-admin branch July 8, 2026 06:48

onurinanc mentioned this pull request Jul 8, 2026

Rename Warden (emergency guardian) component #3241

Open

	// Seed ADMIN membership: [0, ADMIN, suffix, prefix] -> [1, 0, 0, 0].
	// Seed ADMIN membership: [0, admin_role, suffix, prefix] -> [1, 0, 0, 0].

	StorageMap::with_entries(vec![]).expect("empty role config map should be valid")
	StorageMap::default()

		let member_count =
		u32::try_from(admins.len()).expect("number of initial admins should fit in u32");

Uh oh!

Conversation

onurinanc commented Jul 6, 2026

Uh oh!

onurinanc commented Jul 6, 2026

Uh oh!

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmagician left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fumuran left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

partylikeits1983 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

onurinanc commented Jul 7, 2026

Uh oh!

PhilippGackstatter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmagician left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants