feat(rbac): implement NIP-29 room roles and permission gating (#47) #220
Reference in New Issue
Block a user
Delete Branch "Khushvendra/flotilla:feat/nip29-rbac-47"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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
Backward compatibility
Validation
Manual test plan
This looks very good, nice and organized. Just a few comments.
sortRolesDeschelper 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)This exists already in @welshman/lib
@@ -0,0 +319,4 @@set(false)})}),)This is redundant with
deriveSupportedMethodsin core/state.@@ -0,0 +501,4 @@([$permissions, $isSpaceAdmin]) => $isSpaceAdmin || $permissions.size > 0,)export const hasPermission = (url: string, h: string, kind: number) =>should be deriveHasPermission
@@ -820,3 +823,1 @@uniq(getPubkeyTagValues(event?.tags ?? [])),)}export const deriveRoomMembers = deriveRoomMembersByRoleInstead of re-exporting, update imports
@@ -852,3 +843,1 @@return []})}export const deriveRoomAdmins = deriveRoomAdminsByRoleInstead of re-exporting, update imports
@@ -983,3 +981,1 @@return store})export const deriveUserIsSpaceAdmin = deriveUserIsSpaceAdminByRoleInstead of re-exporting, update imports
@@ -1051,3 +1045,1 @@[pubkey, deriveRoomAdmins(url, h), deriveUserIsSpaceAdmin(url)],([$pubkey, $admins, $isSpaceAdmin]) => $isSpaceAdmin || $admins.includes($pubkey!),)export const deriveUserIsRoomAdmin = deriveUserIsRoomAdminByRoleInstead of re-exporting, update imports
@@ -68,0 +85,4 @@accessLabel: Array.from(role.access).join(", "),})),),)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.
@@ -259,0 +288,4 @@<RoleBadgerole={role.name}label={role.label}color={role.color}Since role is a defined data type, I would just pass the whole thing in and RoleBadge can accept a
RoleDefinition@@ -46,0 +69,4 @@label: stringcolor?: numberorder?: numbermembers: RoomMember[]Try to re-use defined types when possible. This mostly overlaps with RoleDefinition.
@@ -46,0 +106,4 @@}return groups})This whole thing could probably go in the
roles.tsfile. 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.@@ -45,0 +63,4 @@roleDefinitions: Array<{name: string; label?: string; color?: number; order?: number}>primaryRole?: {name: string; label?: string; color?: number}sortKey: number}Same thing, see if you can adjust the data model so that this use case falls out naturally instead of being shoehorned in.
@@ -45,0 +105,4 @@label: member.primaryRole.label || roleName,color: member.primaryRole.color,order: member.sortKey,members: [],Lots of copying, this could be much simplified if you can make the data model flow
@@ -0,0 +75,4 @@hasPermissionTags: booleanuserPermissions: Set<number>memberRoles: Map<string, RoleDefinition[]>}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.
ill have a look into this (less code = always better)
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)This ternary logic is halfway duplicated between callers and
deriveHasPermission, since that function also takes into accountderiveUserIsSpaceAdmin. I think a clean way to handle this would be to push thehtag check down by making it optional inderiveHasPermission. This will drastically simplify all our permission checks.@@ -0,0 +93,4 @@}const deriveEventsForUrl = (url: string, filters: Filter[] = [{}]) =>deriveArray(deriveEventsByIdForUrl({url, tracker, repository, filters}))This function exists already in core/state
@@ -0,0 +255,4 @@)export const deriveRoomMembers = (url: string, h: string) =>derived(deriveRoomListStore(url, h), $events => {This could be simplified to
The reason being that Repository handles replaceable event deduplication.
@@ -0,0 +262,4 @@})export const deriveRoomAdmins = (url: string, h: string) =>derived(deriveRoomListStore(url, h), $events => {Same thing here, which means you can remove
deriveRoomListStore@@ -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)),Same here, no need for getLatest, just use the zero or one event provided. That means you can remove
getLatestEventByKindtoo@@ -0,0 +438,4 @@})}return snapshotsI 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.
@@ -903,0 +892,4 @@deriveUserSpacePermissions(url),],([$events, $isAdmin, $permissions]) => {if (!$isAdmin) {This seems unnecessary to me, isn't this taken into account by
deriveUserSpacePermissions?@@ -907,0 +906,4 @@$permissions.has(ROOM_PERMISSION_ADD_MEMBER) ||$permissions.has(ROOM_PERMISSION_REMOVE_MEMBER) ||$permissions.has(ROOM_PERMISSION_BAN_USER) ||$permissions.size === 0The 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.
@hodlbod Working on this, been a bit busy with the university exams. Sorry for the delay
c2ea1cefc5toba07c339ebOk, 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)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.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.