feat(identity-registry): implement Role-Based Access Control (RBAC) with Moderators#58
Merged
Bosun-Josh121 merged 1 commit intoLightForgeHub:mainfrom Mar 28, 2026
Conversation
Add Moderator role to identity-registry-contract separating day-to-day operations (verify/ban experts) from critical admin actions. - storage: add DataKey::Moderator(Address) with is/set/remove helpers - contract: add add_moderator/remove_moderator (admin only); update verify_expert and ban_expert to accept caller param (admin or moderator) - error: add Unauthorized(11) variant - lib: expose add_moderator and remove_moderator; update add_expert and ban_expert signatures to include caller address - test: add moderator verify/ban success tests, unauthorized tests, and remove_moderator test; update all existing tests for new signatures
📝 WalkthroughWalkthroughThis change introduces role-based access control for the identity registry contract by adding moderator management functions and updating expert verification and banning to allow either admin or moderator authorization. Storage, error types, and test coverage are updated to support the new moderator role. Changes
Sequence DiagramsequenceDiagram
actor Admin
actor Moderator
participant Contract
participant Storage
participant Result
Admin->>Contract: verify_expert(caller=Admin, expert, uri)
Contract->>Contract: admin.require_auth()
activate Contract
Contract->>Storage: (proceed with verification)
deactivate Contract
Contract->>Result: ✓ Success
Moderator->>Contract: verify_expert(caller=Moderator, expert, uri)
Contract->>Storage: is_moderator(Moderator)?
activate Storage
Storage->>Storage: Check DataKey::Moderator(Moderator)
Storage-->>Contract: true
deactivate Storage
Contract->>Storage: (proceed with verification)
Contract->>Result: ✓ Success
Moderator->>Contract: verify_expert(caller=NotModerator, expert, uri)
Contract->>Storage: is_moderator(NotModerator)?
activate Storage
Storage-->>Contract: false
deactivate Storage
Contract->>Result: ✗ RegistryError::Unauthorized
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/identity-registry-contract/src/contract.rs (1)
96-108:⚠️ Potential issue | 🔴 CriticalReject
Bannedrecords inverify_expert.A moderator can currently call this on a banned expert, which flips them back to
Verifiedand appends the same address to the directory again on Line 108. That bypasses the admin-onlyunban_expertflow this RBAC split is supposed to preserve. Please blockExpertStatus::Bannedhere, or route reactivation throughunban_expert, and add a regression test for that transition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/identity-registry-contract/src/contract.rs` around lines 96 - 108, In verify_expert, add an explicit check for ExpertStatus::Banned and return an error instead of re-verifying or adding to the index: if current_status == ExpertStatus::Banned { return Err(RegistryError::Banned); } (or an appropriate RegistryError variant) so storage::set_expert_record and storage::add_expert_to_index are not executed for banned accounts; alternatively, route reactivation through the admin-only unban_expert path. Also add a regression test asserting that calling verify_expert on a banned expert fails and does not change storage or append the address to the index.
🧹 Nitpick comments (2)
contracts/identity-registry-contract/src/lib.rs (1)
47-59: Publish the ABI break with this entrypoint change.These signatures change the generated contract interface, so existing callers will fail until they pass the new
callerargument. Please ship this with an explicit version bump or migration note for downstream clients.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/identity-registry-contract/src/lib.rs` around lines 47 - 59, The exported entrypoints add_expert and ban_expert now accept an extra caller: Address parameter (they call contract::verify_expert and contract::ban_expert), which is an ABI-breaking change; update the contract release metadata by bumping the contract/ABI version and add a clear migration note describing the new caller argument and how callers must pass it (or provide backward-compatible shim entrypoints that derive caller from env and delegate to the new signatures), and include this change in the contract changelog/release notes so downstream clients can update accordingly.contracts/identity-registry-contract/src/contract.rs (1)
88-94: Consider a shared admin/moderator auth helper.The same authorization branch is duplicated in both entrypoints. Pulling it into one helper will make future RBAC changes much less likely to drift between
verify_expertandban_expert.Also applies to: 125-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/identity-registry-contract/src/contract.rs` around lines 88 - 94, The authorization branch that checks if caller is admin or a moderator (using admin, caller.require_auth(), storage::is_moderator(env, caller), and returning RegistryError::Unauthorized) is duplicated between the entrypoints (e.g., verify_expert and ban_expert); extract this into a single helper function (e.g., require_admin_or_moderator_auth(env, caller, admin) or verify_admin_or_moderator) that performs the same checks and calls require_auth() or returns RegistryError::Unauthorized, then replace the duplicated branches in both verify_expert and ban_expert with calls to that helper to centralize RBAC logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@contracts/identity-registry-contract/src/contract.rs`:
- Around line 96-108: In verify_expert, add an explicit check for
ExpertStatus::Banned and return an error instead of re-verifying or adding to
the index: if current_status == ExpertStatus::Banned { return
Err(RegistryError::Banned); } (or an appropriate RegistryError variant) so
storage::set_expert_record and storage::add_expert_to_index are not executed for
banned accounts; alternatively, route reactivation through the admin-only
unban_expert path. Also add a regression test asserting that calling
verify_expert on a banned expert fails and does not change storage or append the
address to the index.
---
Nitpick comments:
In `@contracts/identity-registry-contract/src/contract.rs`:
- Around line 88-94: The authorization branch that checks if caller is admin or
a moderator (using admin, caller.require_auth(), storage::is_moderator(env,
caller), and returning RegistryError::Unauthorized) is duplicated between the
entrypoints (e.g., verify_expert and ban_expert); extract this into a single
helper function (e.g., require_admin_or_moderator_auth(env, caller, admin) or
verify_admin_or_moderator) that performs the same checks and calls
require_auth() or returns RegistryError::Unauthorized, then replace the
duplicated branches in both verify_expert and ban_expert with calls to that
helper to centralize RBAC logic.
In `@contracts/identity-registry-contract/src/lib.rs`:
- Around line 47-59: The exported entrypoints add_expert and ban_expert now
accept an extra caller: Address parameter (they call contract::verify_expert and
contract::ban_expert), which is an ABI-breaking change; update the contract
release metadata by bumping the contract/ABI version and add a clear migration
note describing the new caller argument and how callers must pass it (or provide
backward-compatible shim entrypoints that derive caller from env and delegate to
the new signatures), and include this change in the contract changelog/release
notes so downstream clients can update accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7f0b164-b826-46e6-9e3f-b51010ff0ee3
📒 Files selected for processing (5)
contracts/identity-registry-contract/src/contract.rscontracts/identity-registry-contract/src/error.rscontracts/identity-registry-contract/src/lib.rscontracts/identity-registry-contract/src/storage.rscontracts/identity-registry-contract/src/test.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🧠 SkillSphere Pull Request 🌐
Mark with an
xall the checkboxes that apply (like[x])cargo test(All tests passed)📌 Type of Change
📝 Changes Description
Implements Role-Based Access Control (RBAC) in
identity-registry-contractby introducing a Moderator role that separates day-to-day operations from critical admin actions.Files changed:
src/storage.rs: AddedDataKey::Moderator(Address)variant andis_moderator,set_moderator,remove_moderatorstorage helpers using instance storage.src/error.rs: AddedUnauthorized = 11error variant for callers that are neither admin nor moderator.src/contract.rs: Addedadd_moderatorandremove_moderatorfunctions (admin only). Updatedverify_expertandban_expertto accept acaller: &Addressparameter and authorize either admin or moderator.src/lib.rs: Exposedadd_moderatorandremove_moderatorin the contract interface. Updatedadd_expertandban_expertto include thecaller: Addressparameter.src/test.rs: Updated all existing tests for new function signatures. Added 4 new tests: moderator verifies expert (success), moderator bans expert (success), non-moderator cannot verify (fail), remove moderator revokes access.Storage impact: Moderator flags are stored in instance storage as
DataKey::Moderator(Address) -> bool, so they share the contract instance TTL. No additional persistent storage entries.📸 Evidence
🌌 Comments
add_expertandban_expertare breaking changes: callers must now pass acaller: Addressas the first argument to explicitly identify who is authorizing the action (admin or moderator).add_moderatorandremove_moderatorare strictly admin-only — moderators cannot escalate their own privileges.Thank you for contributing to SkillSphere! 🌍
We are glad you have chosen to help us democratize access to knowledge on the Stellar network. Your contribution brings us one step closer to a trustless, peer-to-peer consulting economy. Let's build the future together! 🚀
Summary by CodeRabbit
New Features
Tests