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

Closed
Khushvendra wants to merge 5 commits from Khushvendra/flotilla:feat/nip29-rbac-47 into dev
12 changed files with 105 additions and 74 deletions
Showing only changes of commit 9756199fdf - Show all commits
+9 -3
View File
@@ -17,8 +17,12 @@
import Report from "@app/components/Report.svelte"
import EventShare from "@app/components/EventShare.svelte"
import EventDeleteConfirm from "@app/components/EventDeleteConfirm.svelte"
import {hasNip29, deriveUserIsSpaceAdmin} from "@app/core/state"
import {hasPermission} from "@app/core/roles"
import {
deriveUserIsSpaceAdmin,
deriveHasPermission,
ROOM_PERMISSION_DELETE_EVENT,
} from "@app/core/roles"
import {hasNip29} from "@app/core/state"
import {pushModal} from "@app/util/modal"
import {pushToast} from "@app/util/toast"
import {makeSpaceChatPath} from "@app/util/routes"
@@ -35,7 +39,9 @@
const isRoot = event.kind !== COMMENT
const h = getTagValue("h", event.tags)
const canDelete = h ? hasPermission(url, h, 9005) : deriveUserIsSpaceAdmin(url)
const canDelete = h
? deriveHasPermission(url, h, ROOM_PERMISSION_DELETE_EVENT)
: deriveUserIsSpaceAdmin(url)
Khushvendra marked this conversation as resolved Outdated
Outdated
Review

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.
const report = () => pushModal(Report, {url, event})
+10 -3
View File
@@ -31,7 +31,12 @@
import ProfileBadges from "@app/components/ProfileBadges.svelte"
import RoleBadge from "@app/components/RoleBadge.svelte"
import {pubkeyLink, deriveSpaceBannedPubkeyItems} from "@app/core/state"
import {deriveUserHasSpacePermission, deriveSpaceMemberRoleInfo} from "@app/core/roles"
import {
deriveUserHasSpacePermission,
deriveSpaceMemberRoleInfo,
ROOM_PERMISSION_ADD_MEMBER,
ROOM_PERMISSION_BAN_USER,
} from "@app/core/roles"
import {addSpaceMembers} from "@app/core/commands"
import {pushModal} from "@app/util/modal"
import {pushToast} from "@app/util/toast"
@@ -46,9 +51,11 @@
const profile = deriveProfile(pubkey, removeUndefined([url]))
const canBan = url ? deriveUserHasSpacePermission(url, 9009) : readable(false)
const canBan = url ? deriveUserHasSpacePermission(url, ROOM_PERMISSION_BAN_USER) : readable(false)
const canRestore = url ? deriveUserHasSpacePermission(url, 9000) : readable(false)
const canRestore = url
? deriveUserHasSpacePermission(url, ROOM_PERMISSION_ADD_MEMBER)
: readable(false)
const bannedPubkeys = url ? deriveSpaceBannedPubkeyItems(url) : undefined
+2 -1
View File
@@ -23,7 +23,8 @@
import Icon from "@lib/components/Icon.svelte"
import Reaction from "@app/components/Reaction.svelte"
import ReportDetails from "@app/components/ReportDetails.svelte"
import {REACTION_KINDS, deriveUserIsSpaceAdmin} from "@app/core/state"
import {deriveUserIsSpaceAdmin} from "@app/core/roles"
import {REACTION_KINDS} from "@app/core/state"
import {pushModal} from "@app/util/modal"
interface Props {
+1 -1
View File
@@ -12,7 +12,7 @@
import Popover from "@lib/components/Popover.svelte"
import Button from "@lib/components/Button.svelte"
import Confirm from "@lib/components/Confirm.svelte"
import {deriveUserIsSpaceAdmin} from "@app/core/state"
import {deriveUserIsSpaceAdmin} from "@app/core/roles"
import {publishDelete, canEnforceNip70} from "@app/core/commands"
import {pushToast} from "@app/util/toast"
import {pushModal} from "@app/util/modal"
+10 -7
View File
@@ -1,5 +1,4 @@
<script lang="ts">
import {sortBy} from "@welshman/lib"
import {goto} from "$app/navigation"
import type {RoomMeta} from "@welshman/util"
import {displayRelayUrl, makeRoomMeta} from "@welshman/util"
@@ -33,11 +32,16 @@
import RoomEdit from "@app/components/RoomEdit.svelte"
import RoomName from "@app/components/RoomName.svelte"
import RoomImage from "@app/components/RoomImage.svelte"
import {deriveRoomRoles, hasPermission} from "@app/core/roles"
import {
deriveRoomMembers,
deriveRoomRoles,
deriveUserIsRoomAdmin,
deriveHasPermission,
sortRolesDesc,
ROOM_PERMISSION_EDIT_META,
} from "@app/core/roles"
import {
deriveRoom,
deriveRoomMembers,
deriveUserIsRoomAdmin,
deriveUserRoomMembershipStatus,
deriveUserRooms,
deriveShouldNotify,
@@ -63,7 +67,7 @@
const members = deriveRoomMembers(url, h)
const roomRoles = deriveRoomRoles(url, h)
const userIsAdmin = deriveUserIsRoomAdmin(url, h)
const canEditMetadata = hasPermission(url, h, 9002)
const canEditMetadata = deriveHasPermission(url, h, ROOM_PERMISSION_EDIT_META)
const membershipStatus = deriveUserRoomMembershipStatus(url, h)
const userRooms = deriveUserRooms(url)
@@ -71,8 +75,7 @@
const shouldNotify = deriveShouldNotify(url, h)
const roleRows = $derived.by(() =>
sortBy(
role => -(role.order ?? -Infinity),
sortRolesDesc(
Array.from($roomRoles.roles.values()).map(role => ({
name: role.name,
label: role.label,
2
+1 -1
View File
@@ -13,7 +13,7 @@
import EventDeleteConfirm from "@app/components/EventDeleteConfirm.svelte"
import {pushModal} from "@app/util/modal"
import {pushToast} from "@app/util/toast"
import {deriveUserIsSpaceAdmin} from "@app/core/state"
import {deriveUserIsSpaceAdmin} from "@app/core/roles"
type Props = {
url: string
+13 -10
View File
@@ -1,5 +1,5 @@
<script lang="ts">
import {first, sortBy} from "@welshman/lib"
import {first, removeUndefined, sortBy} from "@welshman/lib"
import {waitForThunkError, removeRoomMember} from "@welshman/app"
import MenuDots from "@assets/icons/menu-dots.svg?dataurl"
import MinusCircle from "@assets/icons/minus-circle.svg?dataurl"
@@ -21,8 +21,15 @@
import RoomName from "@app/components/RoomName.svelte"
import RoomMembersAdd from "@app/components/RoomMembersAdd.svelte"
import type {RoomMember} from "@app/core/roles"
import {deriveRoom, deriveRoomMembers} from "@app/core/state"
import {deriveRoomRoles, hasPermission} from "@app/core/roles"
import {
deriveRoomMembers,
deriveRoomRoles,
deriveHasPermission,
sortRolesDesc,
ROOM_PERMISSION_ADD_MEMBER,
ROOM_PERMISSION_REMOVE_MEMBER,
} from "@app/core/roles"
import {deriveRoom} from "@app/core/state"
import {pushModal} from "@app/util/modal"
import {pushToast} from "@app/util/toast"
@@ -36,8 +43,8 @@
const room = deriveRoom(url, h)
const members = deriveRoomMembers(url, h)
const roomRoles = deriveRoomRoles(url, h)
const canAddMembers = hasPermission(url, h, 9000)
const canRemoveMembers = hasPermission(url, h, 9001)
const canAddMembers = deriveHasPermission(url, h, ROOM_PERMISSION_ADD_MEMBER)
const canRemoveMembers = deriveHasPermission(url, h, ROOM_PERMISSION_REMOVE_MEMBER)
const back = () => history.back()
@@ -52,8 +59,7 @@
const getResolvedRoles = (member: RoomMember) =>
removeUndefined(member.roles.map(roleName => $roomRoles.roles.get(roleName)))
const getPrimaryRole = (member: RoomMember) =>
first(sortBy(role => -(role.order ?? -Infinity), getResolvedRoles(member)))
const getPrimaryRole = (member: RoomMember) => first(sortRolesDesc(getResolvedRoles(member)))
const memberGroups = $derived.by(() => {
const byRole = new Map<
2
@@ -102,9 +108,6 @@
return groups
})
Khushvendra marked this conversation as resolved Outdated
Outdated
Review

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.
const removeUndefined = <T,>(items: Array<T | undefined>): T[] =>
items.filter((item): item is T => item !== undefined)
const addMember = () => pushModal(RoomMembersAdd, {url, h})
const removeMember = (pubkey: string) =>
+2 -2
View File
@@ -18,7 +18,7 @@
import SpaceRelayStatus from "@app/components/SpaceRelayStatus.svelte"
import RelayDescription from "@app/components/RelayDescription.svelte"
import ProfileLatest from "@app/components/ProfileLatest.svelte"
import {deriveUserHasSpacePermission} from "@app/core/roles"
import {deriveUserHasSpacePermission, ROOM_PERMISSION_EDIT_META} from "@app/core/roles"
import {pushModal} from "@app/util/modal"
type Props = {
@@ -28,7 +28,7 @@
const {url}: Props = $props()
const relay = deriveRelay(url)
const owner = $derived($relay?.pubkey)
const canEdit = deriveUserHasSpacePermission(url, 9002)
const canEdit = deriveUserHasSpacePermission(url, ROOM_PERMISSION_EDIT_META)
const back = () => history.back()
+10 -4
View File
@@ -29,7 +29,13 @@
deriveSpaceBannedPubkeyItems,
deriveSupportedMethods,
} from "@app/core/state"
import {deriveSpaceMemberRoleInfo, deriveUserHasSpacePermission} from "@app/core/roles"
import {
deriveSpaceMemberRoleInfo,
deriveUserHasSpacePermission,
ROOM_PERMISSION_ADD_MEMBER,
ROOM_PERMISSION_REMOVE_MEMBER,
ROOM_PERMISSION_BAN_USER,
} from "@app/core/roles"
import {pushModal} from "@app/util/modal"
import {pushToast} from "@app/util/toast"
@@ -42,9 +48,9 @@
const members = deriveSpaceMembers(url)
Review

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.
const bans = deriveSpaceBannedPubkeyItems(url)
const spaceMemberRoles = deriveSpaceMemberRoleInfo(url)
const canAddMember = deriveUserHasSpacePermission(url, 9000)
const canBanByPermission = deriveUserHasSpacePermission(url, 9009)
const canUnallowByPermission = deriveUserHasSpacePermission(url, 9001)
const canAddMember = deriveUserHasSpacePermission(url, ROOM_PERMISSION_ADD_MEMBER)
const canBanByPermission = deriveUserHasSpacePermission(url, ROOM_PERMISSION_BAN_USER)
const canUnallowByPermission = deriveUserHasSpacePermission(url, ROOM_PERMISSION_REMOVE_MEMBER)
const supportedMethods = deriveSupportedMethods(url)
const canBan = $derived(
$canBanByPermission && $supportedMethods.includes(ManagementMethod.BanPubkey),
2
+1 -1
View File
@@ -40,6 +40,7 @@
import SpaceMenuRoomItem from "@app/components/SpaceMenuRoomItem.svelte"
import VoiceWidget from "@app/components/VoiceWidget.svelte"
import SocketStatusIndicator from "@app/components/SocketStatusIndicator.svelte"
import {deriveUserIsSpaceAdmin} from "@app/core/roles"
import {
ENABLE_ZAPS,
CONTENT_KINDS,
@@ -50,7 +51,6 @@
userSpaceUrls,
hasNip29,
deriveUserCanCreateRoom,
deriveUserIsSpaceAdmin,
deriveEventsForUrl,
deriveSpaceActionItems,
notificationSettings,
+34 -25
View File
@@ -1,13 +1,35 @@
import {derived, readable} from "svelte/store"
import {first, memoize, simpleCache, sortBy, uniq} from "@welshman/lib"
import {first, memoize, removeUndefined, simpleCache, sortBy, uniq} from "@welshman/lib"
import {deriveArray, deriveEventsByIdForUrl} from "@welshman/store"
import {pubkey, repository, tracker, manageRelay} from "@welshman/app"
import {ManagementMethod, ROOM_ADMINS, ROOM_MEMBERS, getTagValue, isRelayUrl} from "@welshman/util"
import {pubkey, repository, tracker} from "@welshman/app"
import {
ROOM_ADD_MEMBER,
ROOM_REMOVE_MEMBER,
ROOM_EDIT_META,
ROOM_DELETE_EVENT,
ROOM_ADMINS,
ROOM_MEMBERS,
getTagValue,
isRelayUrl,
} from "@welshman/util"
import type {Filter, TrustedEvent} from "@welshman/util"
import {deriveSupportedMethods} from "@app/core/state"
export const ROOM_ROLES = 39003
const ALL_ROOM_PERMISSIONS = [9000, 9001, 9002, 9005, 9009]
export const ROOM_PERMISSION_ADD_MEMBER = ROOM_ADD_MEMBER
export const ROOM_PERMISSION_REMOVE_MEMBER = ROOM_REMOVE_MEMBER
export const ROOM_PERMISSION_EDIT_META = ROOM_EDIT_META
export const ROOM_PERMISSION_DELETE_EVENT = ROOM_DELETE_EVENT
export const ROOM_PERMISSION_BAN_USER = 9009
const ALL_ROOM_PERMISSIONS = [
ROOM_PERMISSION_ADD_MEMBER,
ROOM_PERMISSION_REMOVE_MEMBER,
ROOM_PERMISSION_EDIT_META,
ROOM_PERMISSION_DELETE_EVENT,
ROOM_PERMISSION_BAN_USER,
]
export type RoleAccess = "read" | "write" | "join"
5
@@ -250,11 +272,10 @@ const getMember = (members: RoomMember[], targetPubkey: string) =>
const getResolvedRoles = (rolesByName: Map<string, RoleDefinition>, roleNames: string[]) =>
removeUndefined(roleNames.map(name => rolesByName.get(name)))
Khushvendra marked this conversation as resolved Outdated
Outdated
Review

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
const getPrimaryRole = (roles: RoleDefinition[]) =>
first(sortBy(role => -(role.order ?? -Infinity), roles))
export const sortRolesDesc = <T extends {order?: number}>(items: T[]) =>
sortBy(item => -(item.order ?? -Infinity), items)
const removeUndefined = <T>(items: Array<T | undefined>): T[] =>
items.filter((item): item is T => item !== undefined)
const getPrimaryRole = (roles: RoleDefinition[]) => first(sortRolesDesc(roles))
const deriveRoomRoleAssignments = simpleCache(([url, h]: [string, string]) =>
derived(
1
@@ -309,18 +330,6 @@ export const deriveUserPermissions = (url: string, h: string) =>
},
)
const deriveNip86SpaceAdmin = simpleCache(([url]: [string]) =>
readable(false, set => {
manageRelay(url, {method: ManagementMethod.SupportedMethods, params: []})
.then(({result = []}) => {
set(Boolean(result.length))
})
.catch(() => {
set(false)
})
}),
)
const buildRoomSnapshots = (events: TrustedEvent[]) => {
const latestByH = new Map<
string,
1
@@ -457,13 +466,13 @@ export const deriveUserIsSpaceAdmin = memoize((url?: string) => {
}
return derived(
[deriveSpaceRoleState(url), deriveNip86SpaceAdmin(url)],
([$spaceRoleState, $nip86Admin]) => {
[deriveSpaceRoleState(url), deriveSupportedMethods(url)],
([$spaceRoleState, $supportedMethods]) => {
if ($spaceRoleState.hasPermissionTags) {
return $spaceRoleState.userPermissions.size > 0
}
return $nip86Admin
return $supportedMethods.length > 0
},
)
})
1
@@ -501,7 +510,7 @@ export const deriveUserIsRoomAdmin = (url: string, h: string) =>
([$permissions, $isSpaceAdmin]) => $isSpaceAdmin || $permissions.size > 0,
)
export const hasPermission = (url: string, h: string, kind: number) =>
export const deriveHasPermission = (url: string, h: string, kind: number) =>
derived(
[deriveUserPermissions(url, h), deriveUserIsSpaceAdmin(url)],
([$permissions, $isSpaceAdmin]) => $isSpaceAdmin || $permissions.has(kind),
@@ -526,7 +535,7 @@ export const deriveSpaceMemberRoleInfo = (url: string) =>
const roleInfoByPubkey = new Map<string, SpaceMemberRoleInfo>()
for (const [pubkey, roles] of $spaceRoleState.memberRoles.entries()) {
const sortedRoles = sortBy(role => -(role.order ?? -Infinity), roles)
const sortedRoles = sortRolesDesc(roles)
const primaryRole = first(sortedRoles)
roleInfoByPubkey.set(pubkey, {
+12 -16
View File
@@ -152,11 +152,14 @@ import {checkRelayHasLivekit} from "$lib/livekit"
import {readFeed} from "@lib/feeds"
import {
parseRoomMembers,
deriveRoomMembers as deriveRoomMembersByRole,
deriveRoomAdmins as deriveRoomAdminsByRole,
deriveUserIsSpaceAdmin as deriveUserIsSpaceAdminByRole,
deriveUserIsRoomAdmin as deriveUserIsRoomAdminByRole,
deriveRoomMembers,
deriveUserIsSpaceAdmin,
deriveUserIsRoomAdmin,
deriveUserSpacePermissions,
ROOM_PERMISSION_ADD_MEMBER,
ROOM_PERMISSION_REMOVE_MEMBER,
ROOM_PERMISSION_DELETE_EVENT,
ROOM_PERMISSION_BAN_USER,
} from "@app/core/roles"
import type {RoomMember} from "@app/core/roles"
@@ -820,8 +823,6 @@ export const deriveSpaceMembers = (url: string) =>
uniq(getTagValues("member", event?.tags ?? [])),
Khushvendra marked this conversation as resolved Outdated
Outdated
Review

Instead of re-exporting, update imports

Instead of re-exporting, update imports
)
export const deriveRoomMembers = deriveRoomMembersByRole
export type BannedPubkeyItem = {
pubkey: string
reason: string
@@ -840,8 +841,6 @@ export const deriveSpaceBannedPubkeyItems = (url: string) => {
return store
}
Khushvendra marked this conversation as resolved Outdated
Outdated
Review

Instead of re-exporting, update imports

Instead of re-exporting, update imports
export const deriveRoomAdmins = deriveRoomAdminsByRole
const getRoomMembers = (_url: string, h: string, events: TrustedEvent[]) => {
const members = new Set<string>()
1
@@ -901,11 +900,12 @@ export const deriveSpaceActionItems = (url: string) =>
getTagValue(e.kind === ROOM_MEMBERS ? "d" : "h", e.tags)
const reports = $events.filter(e => e.kind === REPORT)
const pendingJoins: TrustedEvent[] = []
const canReviewReports = $permissions.has(9005) || $permissions.size === 0
const canReviewReports =
$permissions.has(ROOM_PERMISSION_DELETE_EVENT) || $permissions.size === 0
const canReviewJoins =
$permissions.has(9000) ||
$permissions.has(9001) ||
$permissions.has(9009) ||
$permissions.has(ROOM_PERMISSION_ADD_MEMBER) ||
$permissions.has(ROOM_PERMISSION_REMOVE_MEMBER) ||
$permissions.has(ROOM_PERMISSION_BAN_USER) ||
$permissions.size === 0
Khushvendra marked this conversation as resolved Outdated
Outdated
Review

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.
// Room-level join requests — most recent per pubkey+h
@@ -978,8 +978,6 @@ export enum MembershipStatus {
Granted,
}
export const deriveUserIsSpaceAdmin = deriveUserIsSpaceAdminByRole
export const deriveUserSpaceMembershipStatus = (url: string) => {
Khushvendra marked this conversation as resolved Outdated
Outdated
Review

Instead of re-exporting, update imports

Instead of re-exporting, update imports
// Fetch member list and user add/remove events directly in this derivation.
const memberListFilters: Filter[] = [{kinds: [RELAY_MEMBERS]}]
@@ -1042,8 +1040,6 @@ export const deriveUserSpaceMembershipStatus = (url: string) => {
)
}
export const deriveUserIsRoomAdmin = deriveUserIsRoomAdminByRole
export const deriveUserRoomMembershipStatus = (url: string, h: string) => {
// Fetch the room member list and the current user's add/remove events.
const userEventFilters: Filter[] = [{kinds: [ROOM_ADD_MEMBER, ROOM_REMOVE_MEMBER], "#h": [h]}]
Khushvendra marked this conversation as resolved Outdated
Outdated
Review

Instead of re-exporting, update imports

Instead of re-exporting, update imports