From a3dbe03f5a31ef595498eefdd3f994842282218e Mon Sep 17 00:00:00 2001 From: Kavin-Charles Date: Mon, 3 Aug 2026 10:08:34 +0530 Subject: [PATCH 1/3] fix(messaging): let members create channels, name DMs after participants Two defects confirmed against a running instance with a non-admin user. Members could not create a channel. POST /messaging/channels was gated on messaging:manage, which defaults to admin only, so every member got a 403 and the sidebar's "New channel" button could never succeed. Split channel creation into its own messaging:create_channel permission granted to admin and member, leaving rename/archive on the admin-only manage gate. Adding a permission to a ModuleDefinition only affects new workspaces, since seedWorkspaceRoles runs once when a workspace's Member role is created. Added a migration backfilling create_channel to every role that already holds messaging:send. DM channels are all stored as name='dm' with the display name expected to be built from participants, but GET /messaging/channels returned no members, so every DM rendered as the literal string "dm". The list endpoint now attaches members for dm/group_dm rows, and a shared channelDisplayName() helper derives the label from the other participants for the sidebar, channel header, and composer placeholder. Covers both with tests: permission gates asserted off the router, and channelDisplayName across regular channels, DMs, group DMs, and the not-yet-loaded and only-self fallbacks. --- .../messaging-channels-permissions.test.ts | 77 +++++++++++++++++++ apps/api/src/routes/messaging/channels.ts | 26 ++++++- .../messaging/components/ChannelSidebar.tsx | 16 +++- .../messaging/components/ChannelView.tsx | 5 +- .../web/modules/messaging/lib/dm-name.test.ts | 61 +++++++++++++++ apps/web/modules/messaging/lib/dm-name.ts | 33 ++++++++ ...001_messaging_create_channel_permission.ts | 25 ++++++ packages/modules/src/messaging/index.ts | 10 ++- 8 files changed, 243 insertions(+), 10 deletions(-) create mode 100644 apps/api/src/__tests__/messaging-channels-permissions.test.ts create mode 100644 apps/web/modules/messaging/lib/dm-name.test.ts create mode 100644 apps/web/modules/messaging/lib/dm-name.ts create mode 100644 packages/db/migrations/20260803_001_messaging_create_channel_permission.ts diff --git a/apps/api/src/__tests__/messaging-channels-permissions.test.ts b/apps/api/src/__tests__/messaging-channels-permissions.test.ts new file mode 100644 index 00000000..5a2d8417 --- /dev/null +++ b/apps/api/src/__tests__/messaging-channels-permissions.test.ts @@ -0,0 +1,77 @@ +import { describe, it, expect } from 'vitest'; +import type { Kysely } from 'kysely'; +import type { Database } from '@vencore/db'; +import type { RequestHandler } from 'express'; +import { createChannelsRouter } from '../routes/messaging/channels'; +import { MESSAGING_MODULE } from '@vencore/modules'; + +/** + * Tags each gate handler with the permission it was built from, so the assertions + * can read the gate off the router without standing up Express + a database. + */ +function taggingRequirePermission(permission: string): RequestHandler { + const handler = (() => undefined) as unknown as RequestHandler; + (handler as unknown as { __permission: string }).__permission = permission; + return handler; +} + +function permissionsForRoute(router: unknown, method: string, path: string): string[] { + const stack = (router as { stack: unknown[] }).stack; + const perms: string[] = []; + for (const layer of stack as Array<{ + route?: { path: string; methods: Record; stack: Array<{ handle: { __permission?: string } }> }; + }>) { + if (!layer.route) continue; + if (layer.route.path !== path) continue; + if (!layer.route.methods[method]) continue; + for (const h of layer.route.stack) { + const p = h.handle.__permission; + if (p) perms.push(p); + } + } + return perms; +} + +describe('messaging channels route permissions', () => { + const db = {} as Kysely; + + it('gates channel creation on create_channel, not the admin-only manage', () => { + const router = createChannelsRouter(db, taggingRequirePermission); + + const perms = permissionsForRoute(router, 'post', '/'); + + expect(perms).toContain('messaging:create_channel'); + expect(perms).not.toContain('messaging:manage'); + }); + + it('still gates rename and archive on manage', () => { + const router = createChannelsRouter(db, taggingRequirePermission); + + expect(permissionsForRoute(router, 'patch', '/:id')).toContain('messaging:manage'); + expect(permissionsForRoute(router, 'delete', '/:id')).toContain('messaging:manage'); + }); + + it('keeps listing and reading channels on view', () => { + const router = createChannelsRouter(db, taggingRequirePermission); + + expect(permissionsForRoute(router, 'get', '/')).toContain('messaging:view'); + expect(permissionsForRoute(router, 'get', '/:id')).toContain('messaging:view'); + }); +}); + +describe('messaging module permission definitions', () => { + const byKey = new Map(MESSAGING_MODULE.permissions.map(p => [p.key, p])); + + it('declares messaging:create_channel', () => { + expect(byKey.has('messaging:create_channel')).toBe(true); + }); + + it('grants channel creation to members, so they are not locked out', () => { + expect(byKey.get('messaging:create_channel')?.defaultRoles).toContain('member'); + expect(byKey.get('messaging:create_channel')?.defaultRoles).toContain('admin'); + }); + + it('keeps messaging:manage admin-only', () => { + expect(byKey.get('messaging:manage')?.defaultRoles).toEqual(['admin']); + }); +}); diff --git a/apps/api/src/routes/messaging/channels.ts b/apps/api/src/routes/messaging/channels.ts index aa0c402e..14ba6473 100644 --- a/apps/api/src/routes/messaging/channels.ts +++ b/apps/api/src/routes/messaging/channels.ts @@ -141,18 +141,40 @@ export function createChannelsRouter( } } + // DM rows are all stored as name='dm'; the display name is built from the + // participants. Without members here the sidebar renders every DM + // identically, so attach them for dm/group_dm rows only. + const dmIds = rows.filter(r => r.type === 'dm' || r.type === 'group_dm').map(r => r.id); + const dmMembers = dmIds.length + ? await db + .selectFrom('channel_members') + .innerJoin('users', 'users.id', 'channel_members.user_id') + .where('channel_members.channel_id', 'in', dmIds) + .select(['channel_members.channel_id', 'channel_members.user_id', 'users.name', 'users.email']) + .execute() + : []; + + const membersByChannel = new Map(); + for (const m of dmMembers) { + const list = membersByChannel.get(m.channel_id) ?? []; + list.push({ user_id: m.user_id, name: m.name, email: m.email }); + membersByChannel.set(m.channel_id, list); + } + const channels = rows.map(r => ({ ...r, unread_count: unreadCounts[r.id] ?? 0, last_message: latestMap.get(r.id) ?? null, + ...(membersByChannel.has(r.id) ? { members: membersByChannel.get(r.id) } : {}), })); res.json({ data: channels, error: null }); } catch (err) { next(err); } }); - // Create channel - router.post('/', requirePermission('messaging:manage'), async (req, res, next) => { + // Create channel. Gated on create_channel, not manage — 'manage' is admin-only + // and would stop ordinary members from ever creating a channel. + router.post('/', requirePermission('messaging:create_channel'), async (req, res, next) => { try { const { workspace, user } = req as unknown as AuthenticatedRequest; const body = createChannelSchema.parse(req.body); diff --git a/apps/web/modules/messaging/components/ChannelSidebar.tsx b/apps/web/modules/messaging/components/ChannelSidebar.tsx index a2387467..635cfbd7 100644 --- a/apps/web/modules/messaging/components/ChannelSidebar.tsx +++ b/apps/web/modules/messaging/components/ChannelSidebar.tsx @@ -9,13 +9,20 @@ import { ContextMenu, useContextMenu, type ContextMenuItem } from '@/modules/sha import { useConfirm } from '@/modules/shared/components/ui/ConfirmDialog'; import { useApiToken } from '@/modules/shared/lib/useApiToken'; import { listChannels, createChannel, updateChannel, archiveChannel, markChannelRead } from '../lib/messaging'; +import { channelDisplayName, type ChannelMemberSummary } from '../lib/dm-name'; +import { useAuth } from '@/modules/shared/lib/AuthContext'; import { NewDMModal } from './NewDMModal'; import type { Channel, Message } from '@vencore/types'; -type ChannelWithMeta = Channel & { unread_count: number; last_message: Message | null }; +type ChannelWithMeta = Channel & { + unread_count: number; + last_message: Message | null; + members?: ChannelMemberSummary[]; +}; export function ChannelSidebar() { const getToken = useApiToken(); + const { user } = useAuth(); const router = useRouter(); const params = useParams(); const activeId = params?.['channelId'] as string | undefined; @@ -163,6 +170,7 @@ export function ChannelSidebar() { active={activeId === ch.id} onClick={() => router.push(`/messaging/${ch.id}`)} onMarkRead={ch.last_message ? () => markRead.mutate({ id: ch.id, messageId: ch.last_message!.id }) : undefined} + displayName={channelDisplayName(ch, user?.id ?? '')} isDm /> ))} @@ -232,12 +240,14 @@ function SectionLabel({ label }: { label: string }) { } function ChannelRow({ - channel, active, onClick, isDm, onOpenDetails, onRename, onArchive, onMarkRead, + channel, active, onClick, isDm, displayName, onOpenDetails, onRename, onArchive, onMarkRead, }: { channel: ChannelWithMeta; active: boolean; onClick: () => void; isDm?: boolean; + /** DMs are stored as name='dm'; the row shows the other participant instead. */ + displayName?: string; onOpenDetails?: () => void; onRename?: (name: string) => void; onArchive?: () => void; @@ -323,7 +333,7 @@ function ChannelRow({ {isDm ? '●' : channel.is_private ? : '#'} - {channel.name} + {displayName ?? channel.name} {hasUnread && !active && ( : '#'} - {channelData?.name ?? '…'} + {channelData ? channelDisplayName(channelData, user?.id ?? '') : '…'} {channelData?.topic && ( <> @@ -131,7 +132,7 @@ export function ChannelView({ channelId }: Props) { diff --git a/apps/web/modules/messaging/lib/dm-name.test.ts b/apps/web/modules/messaging/lib/dm-name.test.ts new file mode 100644 index 00000000..457baf0b --- /dev/null +++ b/apps/web/modules/messaging/lib/dm-name.test.ts @@ -0,0 +1,61 @@ +import { describe, it, expect } from 'vitest'; +import { channelDisplayName } from './dm-name'; + +const ME = 'user-me'; + +describe('channelDisplayName', () => { + it('returns the stored name for a regular channel', () => { + expect(channelDisplayName({ name: 'general', type: 'channel' }, ME)).toBe('general'); + }); + + it('ignores members on a regular channel', () => { + const channel = { + name: 'engineering', + type: 'channel', + members: [{ user_id: 'user-other', name: 'Admin' }], + }; + expect(channelDisplayName(channel, ME)).toBe('engineering'); + }); + + it('names a DM after the other participant, not "dm"', () => { + const channel = { + name: 'dm', + type: 'dm', + members: [ + { user_id: ME, name: 'Claude Test' }, + { user_id: 'user-other', name: 'Admin' }, + ], + }; + expect(channelDisplayName(channel, ME)).toBe('Admin'); + }); + + it('joins all other participants for a group DM', () => { + const channel = { + name: 'dm', + type: 'group_dm', + members: [ + { user_id: ME, name: 'Claude Test' }, + { user_id: 'u2', name: 'Admin' }, + { user_id: 'u3', name: 'Dana' }, + ], + }; + expect(channelDisplayName(channel, ME)).toBe('Admin, Dana'); + }); + + it('falls back to a readable label when members are not loaded', () => { + expect(channelDisplayName({ name: 'dm', type: 'dm' }, ME)).toBe('Direct message'); + }); + + it('falls back when the only member is yourself', () => { + const channel = { + name: 'dm', + type: 'dm', + members: [{ user_id: ME, name: 'Claude Test' }], + }; + expect(channelDisplayName(channel, ME)).toBe('Direct message'); + }); + + it('returns empty string for a missing channel', () => { + expect(channelDisplayName(null, ME)).toBe(''); + }); +}); diff --git a/apps/web/modules/messaging/lib/dm-name.ts b/apps/web/modules/messaging/lib/dm-name.ts new file mode 100644 index 00000000..71459a6b --- /dev/null +++ b/apps/web/modules/messaging/lib/dm-name.ts @@ -0,0 +1,33 @@ +export interface ChannelMemberSummary { + user_id: string; + name: string; + email?: string; +} + +export interface NameableChannel { + name: string; + type?: string | null; + members?: ChannelMemberSummary[] | undefined; +} + +/** + * DM channels are all stored with name='dm' — the display name is derived from + * the other participants. Regular channels keep their stored name. + */ +export function channelDisplayName( + channel: NameableChannel | null | undefined, + currentUserId: string, +): string { + if (!channel) return ''; + + const isDm = channel.type === 'dm' || channel.type === 'group_dm'; + if (!isDm) return channel.name; + + const others = (channel.members ?? []).filter(m => m.user_id !== currentUserId); + if (others.length === 0) { + // Members not loaded yet, or a DM with only yourself left in it. + return channel.name === 'dm' ? 'Direct message' : channel.name; + } + + return others.map(m => m.name).join(', '); +} diff --git a/packages/db/migrations/20260803_001_messaging_create_channel_permission.ts b/packages/db/migrations/20260803_001_messaging_create_channel_permission.ts new file mode 100644 index 00000000..8785be66 --- /dev/null +++ b/packages/db/migrations/20260803_001_messaging_create_channel_permission.ts @@ -0,0 +1,25 @@ +import { type Kysely, sql } from 'kysely'; + +// Adding a permission to a ModuleDefinition only reaches NEW workspaces: +// seedWorkspaceRoles grants module defaults when a workspace's Member role is +// first created and never runs again. Existing workspaces therefore need an +// explicit backfill, or members stay locked out of channel creation. +// +// Backfill rule: any role that can already send messages can create channels. +// Roles with grants_all are unaffected — they short-circuit to superuser and +// never consult role_permissions. +export async function up(db: Kysely): Promise { + await sql` + insert into role_permissions (workspace_id, role_id, permission) + select workspace_id, role_id, 'messaging:create_channel' + from role_permissions + where permission = 'messaging:send' + on conflict (role_id, permission) do nothing + `.execute(db); +} + +export async function down(db: Kysely): Promise { + await sql` + delete from role_permissions where permission = 'messaging:create_channel' + `.execute(db); +} diff --git a/packages/modules/src/messaging/index.ts b/packages/modules/src/messaging/index.ts index 05e3336a..536c3f89 100644 --- a/packages/modules/src/messaging/index.ts +++ b/packages/modules/src/messaging/index.ts @@ -7,9 +7,13 @@ export const MESSAGING_MODULE: ModuleDefinition = { icon: 'MessageSquare', defaultEnabled: true, permissions: [ - { key: 'messaging:view', label: 'View channels and messages', defaultRoles: ['admin', 'member'] }, - { key: 'messaging:send', label: 'Send messages and upload files', defaultRoles: ['admin', 'member'] }, - { key: 'messaging:manage', label: 'Manage channels and delete messages', defaultRoles: ['admin'] }, + { key: 'messaging:view', label: 'View channels and messages', defaultRoles: ['admin', 'member'] }, + { key: 'messaging:send', label: 'Send messages and upload files', defaultRoles: ['admin', 'member'] }, + // Creating a channel is a normal member action; renaming/archiving someone + // else's channel is not. Keeping them separate means members are not locked + // out of creating channels by the admin-only 'manage' gate. + { key: 'messaging:create_channel', label: 'Create channels', defaultRoles: ['admin', 'member'] }, + { key: 'messaging:manage', label: 'Manage channels and delete messages', defaultRoles: ['admin'] }, ], nav: [{ label: 'Messaging', path: '/messaging', icon: 'MessageSquare' }], apiPrefixes: ['/messaging'], From e9501b689474a2d36caa102b3883ce9f4a7c9d18 Mon Sep 17 00:00:00 2001 From: Kavin-Charles Date: Mon, 3 Aug 2026 10:47:26 +0530 Subject: [PATCH 2/3] fix(dev): publish the api port for local development The api service exposed 3001 but published no host port, so a locally-run `pnpm dev` web server proxied /api/* to a dead localhost:3001 through the Next.js rewrite. Every API call failed at the proxy and the UI rendered an empty shell with no error, which reads as an application bug. Publish on loopback only. --- docker-compose.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docker-compose.yml b/docker-compose.yml index 88e43d43..de7b5e58 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -55,6 +55,10 @@ services: api: image: ${API_IMAGE:-ghcr.io/vencorehq/vencore-api:latest} + # Published so a locally-run `pnpm dev` web server can reach the API through + # the Next.js rewrite (and so WS upgrades can bypass Next entirely). + ports: + - "127.0.0.1:3001:3001" env_file: .env environment: DATABASE_URL: postgresql://${DB_USER:-vencore}:${DB_PASSWORD:-vencore}@db:5432/${DB_NAME:-vencore} From a8cce9fcad6619cdbcb11a5287363e79e0adf0eb Mon Sep 17 00:00:00 2001 From: Kavin-Charles Date: Mon, 3 Aug 2026 10:51:45 +0530 Subject: [PATCH 3/3] fix(api): use isAdmin instead of the removed users.role column The RBAC3 refactor dropped users.role, but tasks-unified.ts still read it, so `tsc` failed and development did not compile: src/routes/tasks-unified.ts(88,40): error TS2339: Property 'role' does not exist on type '{ id: string; workspace_id: string; ... }' isAdmin is already destructured from the authenticated request one line above and was simply unused. This mirrors the fix already on main (ea4922f). Unrelated to the messaging changes in this PR, but development's build, lint and test jobs all fail without it. --- apps/api/src/routes/tasks-unified.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/api/src/routes/tasks-unified.ts b/apps/api/src/routes/tasks-unified.ts index 3f0d1382..d91fa50b 100644 --- a/apps/api/src/routes/tasks-unified.ts +++ b/apps/api/src/routes/tasks-unified.ts @@ -85,7 +85,7 @@ export function createUnifiedTasksRouter(db: Kysely, requirePermission return } const { status, source, priority, show_all, q, owner_id } = parsed.data - const showAll = show_all && user.role === 'admin' + const showAll = show_all && isAdmin // ── 1. CRM tasks ──────────────────────────────────────────────────────── let crmQ = db