feat(rbac): implement NIP-29 room roles and permission gating (#47) #220

Open
Khushvendra wants to merge 5 commits from Khushvendra/flotilla:feat/nip29-rbac-47 into dev
Contributor

Addresses #47.

Summary
This introduces a forward-compatible NIP-29 RBAC implementation for room and space moderation/authorization.The app now parses role definitions from kind 39003, resolves user permissions from assigned roles, and updates moderation/admin UI gates from binary admin checks to permission-based checks.

Changes Made

  • Added a dedicated RBAC core module to parse and derive role state:
    • Role definitions from kind 39003 tags:
      • role
      • role-label
      • role-color
      • role-order
      • role-permission
      • role-access
  • Added role-aware derivations:
    • deriveRoomRoles
    • deriveUserRoles
    • deriveUserPermissions
    • hasPermission
    • deriveUserIsSpaceAdmin
    • deriveUserIsRoomAdmin
    • deriveSpaceMemberRoleInfo
    • roleColorToCSS
  • Refactored room member/admin list parsing to preserve role annotations:
    • Room members/admins now resolve as structured entries with pubkey + roles
  • Updated sync and storage to include kind 39003 role events.
  • Added RoleBadge reusable component and integrated role badges/grouping in member/profile/detail UIs.
  • Replaced key binary admin checks with permission checks in:
    • Event delete moderation action
    • Room metadata edit/delete visibility
    • Space metadata edit visibility
    • Profile moderation actions
    • Space/room member management controls
  • Updated space action queue filtering to respect effective permissions.

Backward compatibility

  • If relays do not publish role-permission tags, behavior falls back to current admin semantics:
    • room admin lists still grant full room moderation permissions
    • NIP-86 supported-methods fallback remains for space admin determination

Validation

  • Lint formatting is clean for this branch’s changes.
  • Project typecheck still reports existing unrelated baseline issues in poll/capacitor-share areas outside this feature.

Manual test plan

  • Verify role badges and grouping in room/space members
  • Verify profile role badges
  • Verify delete/edit actions appear only with relevant permissions:
    • 9005 for delete event
    • 9002 for room/space metadata edit
    • 9000/9001/9009 for member operations
  • Verify relays without role tags still behave with legacy admin fallback
Addresses #47. **Summary** This introduces a forward-compatible NIP-29 RBAC implementation for room and space moderation/authorization.The app now parses role definitions from kind 39003, resolves user permissions from assigned roles, and updates moderation/admin UI gates from binary admin checks to permission-based checks. **Changes Made** * Added a dedicated RBAC core module to parse and derive role state: * Role definitions from kind 39003 tags: * role * role-label * role-color * role-order * role-permission * role-access * Added role-aware derivations: * deriveRoomRoles * deriveUserRoles * deriveUserPermissions * hasPermission * deriveUserIsSpaceAdmin * deriveUserIsRoomAdmin * deriveSpaceMemberRoleInfo * roleColorToCSS * Refactored room member/admin list parsing to preserve role annotations: * Room members/admins now resolve as structured entries with pubkey + roles * Updated sync and storage to include kind 39003 role events. * Added RoleBadge reusable component and integrated role badges/grouping in member/profile/detail UIs. * Replaced key binary admin checks with permission checks in: * Event delete moderation action * Room metadata edit/delete visibility * Space metadata edit visibility * Profile moderation actions * Space/room member management controls * Updated space action queue filtering to respect effective permissions. **Backward compatibility** * If relays do not publish role-permission tags, behavior falls back to current admin semantics: * room admin lists still grant full room moderation permissions * NIP-86 supported-methods fallback remains for space admin determination **Validation** * Lint formatting is clean for this branch’s changes. * Project typecheck still reports existing unrelated baseline issues in poll/capacitor-share areas outside this feature. **Manual test plan** * Verify role badges and grouping in room/space members * Verify profile role badges * Verify delete/edit actions appear only with relevant permissions: * 9005 for delete event * 9002 for room/space metadata edit * 9000/9001/9009 for member operations * Verify relays without role tags still behave with legacy admin fallback
hodlbod reviewed 2026-04-17 18:10:53 +00:00
hodlbod left a comment
Owner

This looks very good, nice and organized. Just a few comments.

  • Use constants instead of hard coded kind numbers
  • Add a sortRolesDesc helper to encapsulate the ugly sorting behavior
This looks very good, nice and organized. Just a few comments. - [ ] Use constants instead of hard coded kind numbers - [ ] Add a `sortRolesDesc` helper to encapsulate the ugly sorting behavior
@@ -46,0 +103,4 @@
})
const removeUndefined = <T,>(items: Array<T | undefined>): T[] =>
items.filter((item): item is T => item !== undefined)
Owner

This exists already in @welshman/lib

This exists already in @welshman/lib
Khushvendra marked this conversation as resolved
@@ -0,0 +319,4 @@
set(false)
})
}),
)
Owner

This is redundant with deriveSupportedMethods in core/state.

This is redundant with `deriveSupportedMethods` in core/state.
Khushvendra marked this conversation as resolved
@@ -0,0 +501,4 @@
([$permissions, $isSpaceAdmin]) => $isSpaceAdmin || $permissions.size > 0,
)
export const hasPermission = (url: string, h: string, kind: number) =>
Owner

should be deriveHasPermission

should be deriveHasPermission
Khushvendra marked this conversation as resolved
@@ -820,3 +823,1 @@
uniq(getPubkeyTagValues(event?.tags ?? [])),
)
}
export const deriveRoomMembers = deriveRoomMembersByRole
Owner

Instead of re-exporting, update imports

Instead of re-exporting, update imports
Khushvendra marked this conversation as resolved
@@ -852,3 +843,1 @@
return []
})
}
export const deriveRoomAdmins = deriveRoomAdminsByRole
Owner

Instead of re-exporting, update imports

Instead of re-exporting, update imports
Khushvendra marked this conversation as resolved
@@ -983,3 +981,1 @@
return store
})
export const deriveUserIsSpaceAdmin = deriveUserIsSpaceAdminByRole
Owner

Instead of re-exporting, update imports

Instead of re-exporting, update imports
Khushvendra marked this conversation as resolved
@@ -1051,3 +1045,1 @@
[pubkey, deriveRoomAdmins(url, h), deriveUserIsSpaceAdmin(url)],
([$pubkey, $admins, $isSpaceAdmin]) => $isSpaceAdmin || $admins.includes($pubkey!),
)
export const deriveUserIsRoomAdmin = deriveUserIsRoomAdminByRole
Owner

Instead of re-exporting, update imports

Instead of re-exporting, update imports
Khushvendra marked this conversation as resolved
hodlbod reviewed 2026-04-17 23:16:01 +00:00
@@ -68,0 +85,4 @@
accessLabel: Array.from(role.access).join(", "),
})),
),
)
Owner

Instead of mapping to an ad hoc data type, I would just defer the display logic until it's needed. You could add helper functions if needed.

Instead of mapping to an ad hoc data type, I would just defer the display logic until it's needed. You could add helper functions if needed.
Khushvendra marked this conversation as resolved
@@ -259,0 +288,4 @@
<RoleBadge
role={role.name}
label={role.label}
color={role.color}
Owner

Since role is a defined data type, I would just pass the whole thing in and RoleBadge can accept a RoleDefinition

Since role is a defined data type, I would just pass the whole thing in and RoleBadge can accept a `RoleDefinition`
Khushvendra marked this conversation as resolved
@@ -46,0 +69,4 @@
label: string
color?: number
order?: number
members: RoomMember[]
Owner

Try to re-use defined types when possible. This mostly overlaps with RoleDefinition.

Try to re-use defined types when possible. This mostly overlaps with RoleDefinition.
Khushvendra marked this conversation as resolved
@@ -46,0 +106,4 @@
}
return groups
})
Owner

This whole thing could probably go in the roles.ts file. See if you can define types that make sense in this context. The fewer named (and especially ad-hoc) types, the easier the code is to read.

This whole thing could probably go in the `roles.ts` file. See if you can define types that make sense in this context. The fewer named (and especially ad-hoc) types, the easier the code is to read.
Khushvendra marked this conversation as resolved
@@ -45,0 +63,4 @@
roleDefinitions: Array<{name: string; label?: string; color?: number; order?: number}>
primaryRole?: {name: string; label?: string; color?: number}
sortKey: number
}
Owner

Same thing, see if you can adjust the data model so that this use case falls out naturally instead of being shoehorned in.

Same thing, see if you can adjust the data model so that this use case falls out naturally instead of being shoehorned in.
Khushvendra marked this conversation as resolved
@@ -45,0 +105,4 @@
label: member.primaryRole.label || roleName,
color: member.primaryRole.color,
order: member.sortKey,
members: [],
Owner

Lots of copying, this could be much simplified if you can make the data model flow

Lots of copying, this could be much simplified if you can make the data model flow
Khushvendra marked this conversation as resolved
@@ -0,0 +75,4 @@
hasPermissionTags: boolean
userPermissions: Set<number>
memberRoles: Map<string, RoleDefinition[]>
}
Owner

These types are the place where the most time should be spent on the next revision. These types aren't bad, but I think we could cut probably ~100 LOC by making them cleaner. I don't have time right now to give it the necessary thought though.

These types are the place where the most time should be spent on the next revision. These types aren't bad, but I think we could cut probably ~100 LOC by making them cleaner. I don't have time right now to give it the necessary thought though.
Author
Contributor

ill have a look into this (less code = always better)

ill have a look into this (less code = always better)
Khushvendra marked this conversation as resolved
hodlbod reviewed 2026-04-21 17:36:06 +00:00
hodlbod left a comment
Owner

Looks very good, just a few more nitpicks

Looks very good, just a few more nitpicks
@@ -37,0 +41,4 @@
const h = getTagValue("h", event.tags)
const canDelete = h
? deriveHasPermission(url, h, ROOM_PERMISSION_DELETE_EVENT)
: deriveUserIsSpaceAdmin(url)
Owner

This ternary logic is halfway duplicated between callers and deriveHasPermission, since that function also takes into account deriveUserIsSpaceAdmin. I think a clean way to handle this would be to push the h tag check down by making it optional in deriveHasPermission. This will drastically simplify all our permission checks.

This ternary logic is halfway duplicated between callers and `deriveHasPermission`, since that function also takes into account `deriveUserIsSpaceAdmin`. I think a clean way to handle this would be to push the `h` tag check down by making it optional in `deriveHasPermission`. This will drastically simplify all our permission checks.
Khushvendra marked this conversation as resolved
@@ -0,0 +93,4 @@
}
const deriveEventsForUrl = (url: string, filters: Filter[] = [{}]) =>
deriveArray(deriveEventsByIdForUrl({url, tracker, repository, filters}))
Owner

This function exists already in core/state

This function exists already in core/state
Khushvendra marked this conversation as resolved
@@ -0,0 +255,4 @@
)
export const deriveRoomMembers = (url: string, h: string) =>
derived(deriveRoomListStore(url, h), $events => {
Owner

This could be simplified to

derived(
  deriveEventsForUrl(url, [{kinds: [ROOM_MEMBERS], '#d': [h]}]),
  ([event]) => {

The reason being that Repository handles replaceable event deduplication.

This could be simplified to ``` derived( deriveEventsForUrl(url, [{kinds: [ROOM_MEMBERS], '#d': [h]}]), ([event]) => { ``` The reason being that Repository handles replaceable event deduplication.
Khushvendra marked this conversation as resolved
@@ -0,0 +262,4 @@
})
export const deriveRoomAdmins = (url: string, h: string) =>
derived(deriveRoomListStore(url, h), $events => {
Owner

Same thing here, which means you can remove deriveRoomListStore

Same thing here, which means you can remove `deriveRoomListStore`
Khushvendra marked this conversation as resolved
@@ -0,0 +270,4 @@
const deriveRoomRoleState = simpleCache(([url, h]: [string, string]) =>
derived(deriveEventsForUrl(url, [{kinds: [ROOM_ROLES], "#d": [h]}]), $events =>
parseRoleState(getLatestEventByKind($events, ROOM_ROLES, h)),
Owner

Same here, no need for getLatest, just use the zero or one event provided. That means you can remove getLatestEventByKind too

Same here, no need for getLatest, just use the zero or one event provided. That means you can remove `getLatestEventByKind` too
Khushvendra marked this conversation as resolved
@@ -0,0 +438,4 @@
})
}
return snapshots
Owner

I don't understand the purpose of snapshots, couldn't you just use the latest event of each kind and parse members on demand? Or do you expect that to be computationally intensive? It seems odd to couple roles/members/admins like this instead of access each individually. Since this function is used in only one place it might just make sense to fold the logic in there to avoid the entire idea of snapshots.

I don't understand the purpose of snapshots, couldn't you just use the latest event of each kind and parse members on demand? Or do you expect that to be computationally intensive? It seems odd to couple roles/members/admins like this instead of access each individually. Since this function is used in only one place it might just make sense to fold the logic in there to avoid the entire idea of snapshots.
Khushvendra marked this conversation as resolved
@@ -903,0 +892,4 @@
deriveUserSpacePermissions(url),
],
([$events, $isAdmin, $permissions]) => {
if (!$isAdmin) {
Owner

This seems unnecessary to me, isn't this taken into account by deriveUserSpacePermissions?

This seems unnecessary to me, isn't this taken into account by `deriveUserSpacePermissions`?
Khushvendra marked this conversation as resolved
@@ -907,0 +906,4 @@
$permissions.has(ROOM_PERMISSION_ADD_MEMBER) ||
$permissions.has(ROOM_PERMISSION_REMOVE_MEMBER) ||
$permissions.has(ROOM_PERMISSION_BAN_USER) ||
$permissions.size === 0
Owner

The permissions.size is here because this assumes permissions aren't filled for the space admin, but I think that is actually done in the roles file.

The permissions.size is here because this assumes permissions aren't filled for the space admin, but I think that is actually done in the roles file.
Khushvendra marked this conversation as resolved
Author
Contributor

@hodlbod Working on this, been a bit busy with the university exams. Sorry for the delay

@hodlbod Working on this, been a bit busy with the university exams. Sorry for the delay
hodlbod added 5 commits 2026-05-04 21:29:34 +00:00
hodlbod force-pushed feat/nip29-rbac-47 from c2ea1cefc5 to ba07c339eb 2026-05-04 21:29:34 +00:00 Compare
hodlbod reviewed 2026-05-04 23:59:41 +00:00
hodlbod left a comment
Owner

Ok, unfortunately on another review I don't think I can merge this PR because it confuses room-level role based access control with space-level permissions. I think we need to take a step back and think about how group owners might actually use this and well as create a working backend implementation on zooid as well to test against.

Ok, unfortunately on another review I don't think I can merge this PR because it confuses room-level role based access control with space-level permissions. I think we need to take a step back and think about how group owners might actually use this and well as create a working backend implementation on zooid as well to test against.
@@ -42,0 +45,4 @@
const memberGroups = deriveGroupedSpaceMembers(url, members)
const canAddMember = deriveUserHasSpacePermission(url, ROOM_PERMISSION_ADD_MEMBER)
const canBanByPermission = deriveUserHasSpacePermission(url, ROOM_PERMISSION_BAN_USER)
const canUnallowByPermission = deriveUserHasSpacePermission(url, ROOM_PERMISSION_REMOVE_MEMBER)
Owner

I think this is mixing different concerns, the ability for a relay admin to ban/add relay members has nothing to do with a group admin's ability to ban/add group members. These actions should be based exclusively on nip 86 management methods (or equivalent rbac that we're planning to add to nip 43). Looking more closely at this whole PR, I am afraid we're creating a confusing mess — room roles should only apply to rooms, not spaces.

I think this is mixing different concerns, the ability for a relay admin to ban/add relay members has nothing to do with a group admin's ability to ban/add group members. These actions should be based exclusively on nip 86 management methods (or equivalent rbac that we're planning to add to nip 43). Looking more closely at this whole PR, I am afraid we're creating a confusing mess — room roles should only apply to rooms, not spaces.
This pull request can be merged automatically.
This branch is out-of-date with the base branch
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u https://gitea.coracle.social/Khushvendra/flotilla feat/nip29-rbac-47:Khushvendra-feat/nip29-rbac-47
git checkout Khushvendra-feat/nip29-rbac-47
Sign in to join this conversation.