diff --git a/apps/realtime/src/access-revalidation.test.ts b/apps/realtime/src/access-revalidation.test.ts index 4ea7999c301..a6ae605bffd 100644 --- a/apps/realtime/src/access-revalidation.test.ts +++ b/apps/realtime/src/access-revalidation.test.ts @@ -1,10 +1,13 @@ /** * @vitest-environment node * - * Tests for the periodic read-access re-validation sweep. The security contract: - * a socket is evicted only when its role resolves to `null` (a confirmed - * revocation), and a transient failure never evicts a still-authorized socket. + * Tests for the periodic access re-validation sweep, which covers EVERY room type + * a socket occupies. The security contract: a socket is evicted only when its + * permission definitively fails the level that room requires (a confirmed + * revocation or downgrade), and a transient failure never evicts a still-authorized + * socket. */ +import { ROOM_TYPES } from '@sim/realtime-protocol/rooms' import { beforeEach, describe, expect, it, vi } from 'vitest' const { mockResolveRole } = vi.hoisted(() => ({ @@ -12,7 +15,7 @@ const { mockResolveRole } = vi.hoisted(() => ({ })) vi.mock('@/middleware/permissions', () => ({ - resolveCurrentWorkflowRole: mockResolveRole, + resolveCurrentRoomPermission: mockResolveRole, ROLE_REVALIDATION_TTL_MS: 30_000, })) @@ -20,6 +23,7 @@ import { ACCESS_REVALIDATION_SWEEP_INTERVAL_MS, startAccessRevalidationSweep, } from '@/access-revalidation' +import { registerRoomEvictionHandler } from '@/handlers/room-eviction' import type { IRoomManager, UserPresence } from '@/rooms' interface FakeSocket { @@ -30,9 +34,9 @@ interface FakeSocket { leave: ReturnType } -function makeSocket(id: string, userId: string | undefined, workflowId?: string): FakeSocket { +function makeSocket(id: string, userId: string | undefined, room?: string): FakeSocket { const rooms = new Set([id]) - if (workflowId) rooms.add(workflowId) + if (room) rooms.add(room) return { id, userId, @@ -131,7 +135,7 @@ describe('access-revalidation sweep', () => { expect(manager.removeUserFromRoom).not.toHaveBeenCalled() }) - it('resolves with the static safe fallback and no presence reads in the scan', async () => { + it('resolves with the room safe fallback and no presence reads in the scan', async () => { const socket = makeSocket('sock-1', 'user-1', 'wf-1') const manager = makeManager([socket], [{ socketId: 'sock-1', role: 'admin' }]) mockResolveRole.mockResolvedValue('admin') @@ -140,32 +144,123 @@ describe('access-revalidation sweep', () => { await sweep.runOnce() sweep.stop() - expect(mockResolveRole).toHaveBeenCalledWith('user-1', 'wf-1', 'read') + expect(mockResolveRole).toHaveBeenCalledWith('user-1', { type: 'workflow', id: 'wf-1' }, 'read') // The security scan must stay Redis-free — presence is never consulted. expect(manager.getRoomUsers).not.toHaveBeenCalled() }) - it('never evicts a socket joined only to a non-workflow room (files/tables/file-doc)', async () => { - // The sweep shares one io with the files/tables/file-doc handlers. Those rooms are - // namespaced (`workspace-files:ws-1`, `table:t-1`), so treating every socket.rooms - // entry as a workflow id would resolve a bogus permission → null → evict the socket - // from its files/table room every pass. Non-workflow rooms must be filtered out. + it('sweeps non-workflow rooms against their own resource, not a bogus workflow id', async () => { + // The sweep shares one io with the files/tables/file-doc handlers. Their rooms are + // namespaced (`workspace-files:ws-1`, `table:t-1`), so each name is decoded and + // authorized as its own room type — the whole point of covering them at all. const filesSocket = makeSocket('sock-1', 'user-1', 'workspace-files:ws-1') const tableSocket = makeSocket('sock-2', 'user-2', 'table:t-1') const manager = makeManager([filesSocket, tableSocket]) - // Even if the role resolver would say "no access", these must never be swept. - mockResolveRole.mockResolvedValue(null) + mockResolveRole.mockResolvedValue('write') const sweep = startAccessRevalidationSweep(manager) await sweep.runOnce() sweep.stop() - expect(mockResolveRole).not.toHaveBeenCalled() + expect(mockResolveRole).toHaveBeenCalledWith( + 'user-1', + { type: 'workspace-files', id: 'ws-1' }, + 'read' + ) + expect(mockResolveRole).toHaveBeenCalledWith('user-2', { type: 'table', id: 't-1' }, 'read') + // Still authorized: nobody is evicted. expect(filesSocket.leave).not.toHaveBeenCalled() - expect(filesSocket.emit).not.toHaveBeenCalled() expect(tableSocket.leave).not.toHaveBeenCalled() - expect(tableSocket.emit).not.toHaveBeenCalled() + }) + + it('evicts a revoked socket from a presence-free workspace-files room without touching presence', async () => { + const socket = makeSocket('sock-1', 'user-1', 'workspace-files:ws-1') + const manager = makeManager([socket]) + mockResolveRole.mockResolvedValue(null) + + const sweep = startAccessRevalidationSweep(manager) + await sweep.runOnce() + sweep.stop() + + expect(socket.emit).toHaveBeenCalledWith( + 'room-access-revoked', + expect.objectContaining({ room: { type: 'workspace-files', id: 'ws-1' } }) + ) + expect(socket.leave).toHaveBeenCalledWith('workspace-files:ws-1') + // These rooms hold no room-manager presence, so nothing is owed to the cleanup lane. expect(manager.removeUserFromRoom).not.toHaveBeenCalled() + expect(manager.broadcastPresenceUpdate).not.toHaveBeenCalled() + }) + + it('evicts a revoked socket from a table room and clears its presence', async () => { + const socket = makeSocket('sock-1', 'user-1', 'table:t-1') + const manager = makeManager([socket], [{ socketId: 'sock-1', role: 'read' }]) + mockResolveRole.mockResolvedValue(null) + + const sweep = startAccessRevalidationSweep(manager) + await sweep.runOnce() + sweep.stop() + + expect(socket.leave).toHaveBeenCalledWith('table:t-1') + expect(manager.removeUserFromRoom).toHaveBeenCalledWith({ type: 'table', id: 't-1' }, 'sock-1') + expect(manager.broadcastPresenceUpdate).toHaveBeenCalledWith({ type: 'table', id: 't-1' }) + }) + + it('evicts a file-doc socket downgraded to read, and keeps its table room', async () => { + // A file-doc room IS the editor and requires `write`; a table room requires only + // `read`. One downgraded user in both rooms must lose exactly the document. + const socket = makeSocket('sock-1', 'user-1', 'workspace-file-doc:file-1') + socket.rooms.add('table:t-1') + const manager = makeManager([socket]) + mockResolveRole.mockResolvedValue('read') + + const sweep = startAccessRevalidationSweep(manager) + await sweep.runOnce() + sweep.stop() + + expect(socket.leave).toHaveBeenCalledWith('workspace-file-doc:file-1') + expect(socket.leave).not.toHaveBeenCalledWith('table:t-1') + expect(socket.emit).toHaveBeenCalledWith( + 'room-access-revoked', + expect.objectContaining({ room: { type: 'workspace-file-doc', id: 'file-1' } }) + ) + }) + + it('falls back to the room type own membership level on a cold-cache failure', async () => { + // A static 'read' fallback would have evicted every file-doc socket (which needs + // `write`) the first time the DB blipped with a cold cache. + const socket = makeSocket('sock-1', 'user-1', 'workspace-file-doc:file-1') + const manager = makeManager([socket]) + mockResolveRole.mockResolvedValue('write') + + const sweep = startAccessRevalidationSweep(manager) + await sweep.runOnce() + sweep.stop() + + expect(mockResolveRole).toHaveBeenCalledWith( + 'user-1', + { type: 'workspace-file-doc', id: 'file-1' }, + 'write' + ) + expect(socket.leave).not.toHaveBeenCalled() + }) + + it('runs the room type registered eviction handler so handler-local state is dropped', async () => { + const evicted = vi.fn() + registerRoomEvictionHandler(ROOM_TYPES.WORKSPACE_FILE_DOC, evicted) + const socket = makeSocket('sock-1', 'user-1', 'workspace-file-doc:file-1') + const manager = makeManager([socket]) + mockResolveRole.mockResolvedValue(null) + + const sweep = startAccessRevalidationSweep(manager) + await sweep.runOnce() + sweep.stop() + + expect(evicted).toHaveBeenCalledWith( + 'sock-1', + { type: 'workspace-file-doc', id: 'file-1' }, + manager.io + ) }) it('evicts only the revoked socket, not co-members of the room', async () => { diff --git a/apps/realtime/src/access-revalidation.ts b/apps/realtime/src/access-revalidation.ts index 93edaae9a0b..8e0ca514812 100644 --- a/apps/realtime/src/access-revalidation.ts +++ b/apps/realtime/src/access-revalidation.ts @@ -1,10 +1,22 @@ import { createLogger } from '@sim/logger' +import { ROOM_MEMBERSHIP_ACTIONS, satisfiesRoomMembership } from '@sim/platform-authz/room-policy' import type { AccessRevokedBroadcast } from '@sim/realtime-protocol/events' -import { parseRoomName, ROOM_TYPES } from '@sim/realtime-protocol/rooms' +import { + parseRoomName, + ROOM_TYPES, + type RoomRef, + type RoomType, + roomName, +} from '@sim/realtime-protocol/rooms' import { sleep } from '@sim/utils/helpers' +import { + evictSocketFromRoom, + runRoomEvictionHandler, + setEvictionCleanupSink, +} from '@/handlers/room-eviction' import type { AuthenticatedSocket } from '@/middleware/auth' -import { ROLE_REVALIDATION_TTL_MS, resolveCurrentWorkflowRole } from '@/middleware/permissions' -import { type IRoomManager, workflowRoom as wf } from '@/rooms' +import { ROLE_REVALIDATION_TTL_MS, resolveCurrentRoomPermission } from '@/middleware/permissions' +import type { IRoomManager } from '@/rooms' const logger = createLogger('AccessRevalidation') @@ -21,14 +33,30 @@ const logger = createLogger('AccessRevalidation') export const ACCESS_REVALIDATION_SWEEP_INTERVAL_MS = ROLE_REVALIDATION_TTL_MS /** - * Non-null fallback consumed by {@link resolveCurrentWorkflowRole} only on a - * transient DB failure with a cold cache, where returning a non-null role - * (never eviction) is the safe outcome during an outage. The scan deliberately - * does not read presence for a per-socket join-time role: that would put a - * Redis dependency inside the security-critical scan lane, and the fallback's - * only job is to be non-null. + * Fallback consumed by {@link resolveCurrentRoomPermission} only on a transient DB + * failure with a cold cache, where resolving to a permission that KEEPS the socket + * (never eviction) is the safe outcome during an outage. It is the room's own + * membership level, so it satisfies that room's requirement exactly — a static + * `'read'` would have evicted every file-doc socket (which needs `write`) on a cold + * cache blip. The scan deliberately does not read presence for a per-socket + * join-time role: that would put a Redis dependency inside the security-critical + * scan lane, and the fallback's only job is to be permissive. */ -const FALLBACK_ROLE = 'read' +function fallbackRoleFor(type: RoomType): string { + return ROOM_MEMBERSHIP_ACTIONS[type] +} + +/** + * Room types whose membership is mirrored in the room manager's (Redis) presence + * state, and therefore need a presence removal + rebroadcast after an eviction. + * The workspace-files / workspace-tables invalidation rooms carry no presence at + * all, and a file-doc room's roster is pod-local in-memory state reconciled by its + * registered eviction handler — neither has anything for the cleanup lane to do. + */ +const PRESENCE_ROOM_TYPES: ReadonlySet = new Set([ + ROOM_TYPES.WORKFLOW, + ROOM_TYPES.TABLE, +]) /** * Upper bound on a single socket's authorization check inside the scan. A DB @@ -57,36 +85,39 @@ export interface AccessRevalidationSweep { } interface ScanTarget { - workflowId: string + room: RoomRef + /** The Socket.IO room name — the key the socket actually holds. */ + name: string socket: AuthenticatedSocket userId: string } /** - * Collects this pod's authenticated sockets with the workflow room each has + * Collects this pod's authenticated sockets paired with EVERY room each has * joined, in stable socket order. * * Rooms are derived from the socket's own `rooms` set (pod-local, no Redis * round-trips). A socket may occupy several rooms of different types at once - * (workflow canvas, workspace-files browser, table, file-doc), all on the same - * io — so each name is decoded with {@link parseRoomName} and only **workflow** - * rooms are swept here. Non-workflow room names are namespaced (`type:id`) and - * resolve to a non-workflow type; sweeping them as workflow ids would resolve a - * bogus permission, come back `null`, and spuriously evict the socket from its - * files/table room every pass. Only local sockets are evaluated — sockets are - * sticky to a pod, so every socket is swept by exactly one pod using that pod's - * warm role cache (mirroring the per-pod reasoning of the write-path cache). + * (workflow canvas, workspace-files browser, table, file-doc), all on the same io + * — so each name is decoded with {@link parseRoomName} and swept as its own type. + * Decoding (rather than assuming workflow) is what makes this safe: a namespaced + * `type:id` name resolves to the right resource, so it is authorized against that + * resource's workspace instead of resolving a bogus workflow permission. + * + * Only local sockets are evaluated — sockets are sticky to a pod, so every socket + * is swept by exactly one pod using that pod's warm role cache (mirroring the + * per-pod reasoning of the write-path cache). */ function collectScanTargets(io: IRoomManager['io']): ScanTarget[] { const targets: ScanTarget[] = [] for (const socket of io.sockets.sockets.values()) { const authed = socket as AuthenticatedSocket if (!authed.userId) continue - for (const room of socket.rooms) { - if (room === socket.id) continue - const ref = parseRoomName(room) - if (ref?.type !== ROOM_TYPES.WORKFLOW) continue - targets.push({ workflowId: ref.id, socket: authed, userId: authed.userId }) + for (const name of socket.rooms) { + if (name === socket.id) continue + const ref = parseRoomName(name) + if (!ref) continue + targets.push({ room: ref, name, socket: authed, userId: authed.userId }) } } return targets @@ -94,14 +125,19 @@ function collectScanTargets(io: IRoomManager['io']): ScanTarget[] { /** * Starts a per-pod loop that re-validates every connected socket's workspace - * role and evicts sockets whose access has been revoked, closing the read-side - * gap left by the join-only access check. + * permission — for EVERY room type it occupies — and evicts sockets whose access + * no longer satisfies the room, closing the gap left by the join-only access + * check. Without it, a member removed or downgraded mid-session keeps live + * collaborative access (including durable document writes) for the whole lifetime + * of an already-open socket. * - * Blip-safety: eviction fires *only* when {@link resolveCurrentWorkflowRole} - * returns `null`, which happens solely for a successful DB "no access" result or - * a previously-recorded revocation reused across a failure. A transient DB error - * against a still-authorized (or freshly-joined) user resolves to the last-known - * or fallback role, so a database blip never evicts anyone. + * Blip-safety: eviction fires *only* when {@link resolveCurrentRoomPermission} + * resolves to a permission that does not satisfy the room's membership level, + * which happens solely for a successful DB result (no access, or a level below + * the room's requirement) or a previously-recorded revocation reused across a + * failure. A transient DB error against a still-authorized (or freshly-joined) + * user resolves to the last-known or the room's own fallback level, so a database + * blip never evicts anyone. * * Liveness: the loop runs as two independently-guarded lanes. The security scan * (local sockets + DB role checks + emit/leave) touches no Redis at all; the @@ -118,7 +154,7 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR let scanRunning = false let cleanupRunning = false /** - * Round-robin cursor: the `${socketId}:${workflowId}` key of the last target + * Round-robin cursor: the `${socketId}:${roomName}` key of the last target * the previous pass processed. Each pass resumes after it, so a fixed prefix * of hanging authorization checks can never starve the sockets behind it — * every target is examined within a bounded number of passes. @@ -126,16 +162,17 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR let scanCursorKey: string | null = null /** - * Room-state cleanups owed for evicted sockets, keyed - * `${socketId}:${workflowId}`. Every eviction enqueues here (the evicted - * socket has already left the Socket.IO room, so membership scans will never - * see it again); the cleanup lane drains the queue until each removal is - * confirmed, so remaining collaborators do not keep a stale presence entry. + * Presence cleanups owed for evicted sockets, keyed `${socketId}:${roomName}`. + * Every eviction from a presence-backed room enqueues here (the evicted socket + * has already left the Socket.IO room, so membership scans will never see it + * again); the cleanup lane drains the queue until each removal is confirmed, so + * remaining collaborators do not keep a stale presence entry. */ - const pendingCleanups = new Map() + const pendingCleanups = new Map() - async function cleanupEvictedSocket(socketId: string, workflowId: string): Promise { - const key = `${socketId}:${workflowId}` + async function cleanupEvictedSocket(socketId: string, room: RoomRef): Promise { + const name = roomName(room) + const key = `${socketId}:${name}` try { // A fully-disconnected socket already had its presence removed by the // disconnect handler (removeSocketFromAllRooms), so there is nothing left to @@ -148,13 +185,15 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR } // Unlike removeUserFromRoom, this read does not swallow transport errors, - // so a Redis outage lands in the catch below and defers the cleanup. - const currentRoom = await roomManager.getRoomForSocket(socketId, ROOM_TYPES.WORKFLOW) - const currentWorkflowId = currentRoom?.id ?? null - if (currentWorkflowId !== null && currentWorkflowId !== workflowId) { - // The socket has since moved to a different workflow it can still - // access; that join's room switch already removed this room's presence - // entry, so there is nothing stale left to clean here. + // so a Redis outage lands in the catch below and defers the cleanup. The + // lookup is per room TYPE (a socket holds at most one room of each), so a + // socket that also sits in an unrelated room type is unaffected. + const currentRoom = await roomManager.getRoomForSocket(socketId, room.type) + const currentRoomId = currentRoom?.id ?? null + if (currentRoomId !== null && currentRoomId !== room.id) { + // The socket has since moved to a different room of this type that it can + // still access; that join's room switch already removed this room's + // presence entry, so there is nothing stale left to clean here. pendingCleanups.delete(key) return } @@ -162,7 +201,7 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR // Synchronous re-join guard with no awaits before the removal: if the // socket legitimately re-joined this room after the eviction (access // restored), that join re-added its presence — removal would erase it. - if (io.sockets.sockets.get(socketId)?.rooms.has(workflowId)) { + if (io.sockets.sockets.get(socketId)?.rooms.has(name)) { pendingCleanups.delete(key) return } @@ -170,7 +209,7 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR // A null mapping here is the normal case (the socket's mapping key may have // expired) and does NOT mean "skip" — the eviction still removes the presence // entry from the known target room via the explicit ref below. - const removed = await roomManager.removeUserFromRoom(wf(workflowId), socketId) + const removed = await roomManager.removeUserFromRoom(room, socketId) if (!removed) { // `false` conflates two outcomes: the entry was already gone (a no-op), or a // transport error the manager swallowed. Only retry when the socket is still mapped @@ -179,27 +218,27 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR // the presence entry is already gone, so the cleanup is complete: dropping it avoids // re-enqueuing a still-connected socket forever. (A real Redis outage throws at // getRoomForSocket and is deferred by the outer catch, never reaching here.) - if (currentWorkflowId === workflowId) { + if (currentRoomId === room.id) { throw new Error('room-state removal not confirmed') } pendingCleanups.delete(key) return } - await roomManager.broadcastPresenceUpdate(wf(workflowId)) + await roomManager.broadcastPresenceUpdate(room) pendingCleanups.delete(key) } catch (error) { - pendingCleanups.set(key, { socketId, workflowId }) + pendingCleanups.set(key, { socketId, room }) logger.warn( - `Room-state cleanup failed for evicted socket ${socketId} on ${workflowId}; will retry next sweep`, + `Room-state cleanup failed for evicted socket ${socketId} on ${name}; will retry next sweep`, error ) } } async function drainPendingCleanups(): Promise { - for (const [, { socketId, workflowId }] of pendingCleanups) { - await cleanupEvictedSocket(socketId, workflowId) + for (const [, { socketId, room }] of pendingCleanups) { + await cleanupEvictedSocket(socketId, room) } } @@ -220,23 +259,39 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR }) } - function revokeSocket(socket: AuthenticatedSocket, workflowId: string): void { + // Handler-initiated evictions (the per-frame gates) hand failed presence cleanups + // here: they have already left the Socket.IO room, so the scan can no longer + // rediscover them, and this lane is the only thing that retries. + setEvictionCleanupSink((socketId, room) => { + pendingCleanups.set(`${socketId}:${roomName(room)}`, { socketId, room }) + launchCleanups() + }) + + function revokeSocket(socket: AuthenticatedSocket, room: RoomRef, name: string): void { // Security-critical, pod-local, and synchronous: stop this socket receiving - // room broadcasts immediately. Room-state cleanup is only enqueued here — - // the cleanup lane performs the Redis work, so eviction never blocks on it. - const payload: AccessRevokedBroadcast = { - workflowId, - message: 'Your access to this workflow has been revoked', - timestamp: Date.now(), + // room broadcasts immediately, and drop the handler-local state that would + // otherwise still accept its frames (a file-doc socket's room binding is what + // gates its document writes). Redis presence cleanup is only ENQUEUED here — + // the cleanup lane performs that work, so eviction never blocks on it. + if (room.type === ROOM_TYPES.WORKFLOW) { + // Workflow keeps its historical wire event and payload shape, which existing + // clients key off `workflowId`; every other type shares the generic path. + const payload: AccessRevokedBroadcast = { + workflowId: room.id, + message: 'Your access to this workflow has been revoked', + timestamp: Date.now(), + } + socket.emit('access-revoked', payload) + socket.leave(name) + runRoomEvictionHandler(socket.id, room, io) + logger.info(`Revoked live access for user ${socket.userId} on ${name} (socket ${socket.id})`) + } else { + evictSocketFromRoom(socket, room, 'Your access to this resource has been revoked', io) } - socket.emit('access-revoked', payload) - socket.leave(workflowId) - logger.info( - `Revoked live access for user ${socket.userId} on workflow ${workflowId} (socket ${socket.id})` - ) - - pendingCleanups.set(`${socket.id}:${workflowId}`, { socketId: socket.id, workflowId }) + if (PRESENCE_ROOM_TYPES.has(room.type)) { + pendingCleanups.set(`${socket.id}:${name}`, { socketId: socket.id, room }) + } } async function scanMemberships(): Promise { @@ -246,7 +301,7 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR let startIndex = 0 if (scanCursorKey !== null) { const cursorIndex = targets.findIndex( - ({ socket, workflowId }) => `${socket.id}:${workflowId}` === scanCursorKey + ({ socket, name }) => `${socket.id}:${name}` === scanCursorKey ) if (cursorIndex !== -1) { startIndex = (cursorIndex + 1) % targets.length @@ -256,7 +311,7 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR const deadline = Date.now() + SCAN_PASS_BUDGET_MS for (let offset = 0; offset < targets.length; offset++) { - const { workflowId, socket, userId } = targets[(startIndex + offset) % targets.length] + const { room, name, socket, userId } = targets[(startIndex + offset) % targets.length] const remainingBudget = deadline - Date.now() if (remainingBudget <= 0) { @@ -272,25 +327,30 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR // resolution keeps running in the background and is re-raced when the // rotation returns to this socket, so it is acted on once it settles. const role = await Promise.race([ - resolveCurrentWorkflowRole(userId, workflowId, FALLBACK_ROLE), + resolveCurrentRoomPermission(userId, room, fallbackRoleFor(room.type)), sleep(Math.min(SCAN_SOCKET_TIMEOUT_MS, remainingBudget)).then(() => SCAN_TIMED_OUT), ]) - if (role === SCAN_TIMED_OUT) { + // {@link SCAN_TIMED_OUT} is the only symbol this race can yield; matching on + // the type narrows it out of the permission comparison below. + if (typeof role === 'symbol') { logger.warn( - `Authorization check timed out for user ${userId} on workflow ${workflowId}; skipping this pass` + `Authorization check timed out for user ${userId} on ${name}; skipping this pass` ) - } else if (role === null) { - revokeSocket(socket, workflowId) + } else if (!satisfiesRoomMembership(role, room.type)) { + // Covers both revocation (`null`) and downgrade below the level this room + // requires — a member dropped to `read` may keep a table room but not the + // collaborative document editor, exactly as at join time. + revokeSocket(socket, room, name) } } catch (error) { - // Never evict on an unexpected error — only a definitive `null` role + // Never evict on an unexpected error — only a definitive resolved permission // evicts, so a failure here leaves the socket's access intact. logger.warn( - `Access re-validation failed for user ${userId} on workflow ${workflowId}; leaving membership intact`, + `Access re-validation failed for user ${userId} on ${name}; leaving membership intact`, error ) } finally { - scanCursorKey = `${socket.id}:${workflowId}` + scanCursorKey = `${socket.id}:${name}` } } } @@ -332,7 +392,10 @@ export function startAccessRevalidationSweep(roomManager: IRoomManager): AccessR ) return { - stop: () => clearInterval(timer), + stop: () => { + clearInterval(timer) + setEvictionCleanupSink(null) + }, runOnce, } } diff --git a/apps/realtime/src/handlers/file-doc.test.ts b/apps/realtime/src/handlers/file-doc.test.ts index d56952bc619..938092d4484 100644 --- a/apps/realtime/src/handlers/file-doc.test.ts +++ b/apps/realtime/src/handlers/file-doc.test.ts @@ -6,6 +6,8 @@ import { FILE_DOC_MESSAGE_TYPE, FILE_DOC_SEED, } from '@sim/realtime-protocol/file-doc' +import { ROOM_TYPES } from '@sim/realtime-protocol/rooms' +import { sleep } from '@sim/utils/helpers' import * as decoding from 'lib0/decoding' import * as encoding from 'lib0/encoding' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' @@ -38,6 +40,7 @@ import { flushAllFileDocRooms, setupWorkspaceFileDocHandlers, } from '@/handlers/file-doc' +import { beginRoomPermissionRead, commitRoomPermission } from '@/middleware/permissions' type Handler = (payload?: unknown) => Promise | void @@ -244,6 +247,72 @@ describe('setupWorkspaceFileDocHandlers', () => { expect(mockAuthorizeRoom).not.toHaveBeenCalled() }) + it('does not re-enter the room when access was revoked while the join was in flight', async () => { + // The sweep records a revocation before it evicts, so a join whose authorize + // completed just before that must not put the socket back in the document. + const { io } = createIo() + const { socket, handlers } = setup('socket-race', io, { userId: 'user-race' }) + + mockAuthorizeRoom.mockImplementation(async () => { + // Simulate the revocation landing between this join's authorize and its commit, + // exactly as the sweep would record it. + commitRoomPermission( + 'user-race', + { type: ROOM_TYPES.WORKSPACE_FILE_DOC, id: 'file-1' }, + null, + beginRoomPermissionRead() + ) + return { allowed: true, status: 200, workspaceId: 'ws-1', workspacePermission: 'write' } + }) + + await handlers[FILE_DOC_EVENTS.JOIN]({ fileId: 'file-1', clientId: 1 }) + await flushMicrotasks() + + expect(socket.emit).toHaveBeenCalledWith( + FILE_DOC_EVENTS.JOIN_ERROR, + expect.objectContaining({ code: 'ACCESS_DENIED', retryable: false }) + ) + expect(socket.join).not.toHaveBeenCalled() + expect(joinSuccessFileId(socket)).toBeUndefined() + }) + + it('re-reads access when the cached decision expired mid-join, instead of failing open', async () => { + // A join stalled longer than the cache TTL: the sweep's denial is recorded with a + // later read ticket (so this join's own allow is correctly dropped) but has since + // expired. Peeking the cache would read that as "unknown" and let the socket back + // into the document, so the join must re-resolve against the database. + vi.useFakeTimers() + try { + const room = { type: ROOM_TYPES.WORKSPACE_FILE_DOC, id: 'file-stale' } + const { io } = createIo() + const { socket, handlers } = setup('socket-stale', io, { userId: 'user-stale' }) + + mockAuthorizeRoom.mockImplementation(async ({ action }: { action: string }) => { + // The authoritative current answer: access is gone. + if (action !== 'write') + return { allowed: false, status: 403, workspaceId: 'ws-1', workspacePermission: null } + // This join's own authorize saw the pre-revocation state, and the sweep records + // the revocation (later read ticket) while it is still in flight. + commitRoomPermission('user-stale', room, null, beginRoomPermissionRead()) + await sleep(31_000) + return { allowed: true, status: 200, workspaceId: 'ws-1', workspacePermission: 'write' } + }) + + const joining = handlers[FILE_DOC_EVENTS.JOIN]({ fileId: 'file-stale', clientId: 1 }) + await vi.advanceTimersByTimeAsync(31_000) + await joining + + expect(socket.emit).toHaveBeenCalledWith( + FILE_DOC_EVENTS.JOIN_ERROR, + expect.objectContaining({ code: 'ACCESS_DENIED', retryable: false }) + ) + expect(socket.join).not.toHaveBeenCalled() + expect(joinSuccessFileId(socket)).toBeUndefined() + } finally { + vi.useRealTimers() + } + }) + it('requires write permission and reports 404 as NOT_FOUND', async () => { mockAuthorizeRoom.mockResolvedValue({ allowed: false, status: 404, workspacePermission: null }) const { io } = createIo() @@ -299,6 +368,87 @@ describe('setupWorkspaceFileDocHandlers', () => { expect(mockFetchFileDocPersist).toHaveBeenCalled() }) + it('drops document frames and evicts once the editor loses write access mid-session', async () => { + // The join-time check is not a standing right: a collaborator downgraded to `read` + // (or removed) must stop landing durable edits on the socket they already hold. + // A distinct user/file so the recorded revocation — written under fake timers, so it + // outlives this test in real time — cannot leak into siblings through the + // module-global role cache. + vi.useFakeTimers() + try { + mockFetchFileDocSeed.mockResolvedValue(seedResult('# From server')) + const { io, sent } = createIo() + const { socket, handlers } = setup('socket-revoked', io, { userId: 'user-revoked' }) + await handlers[FILE_DOC_EVENTS.JOIN]({ fileId: 'file-revoked', clientId: 1 }) + await vi.advanceTimersByTimeAsync(0) + + // Access is downgraded to read-only, and the cached join-time decision expires. + mockAuthorizeRoom.mockResolvedValue({ + allowed: false, + status: 403, + workspaceId: 'ws-1', + workspacePermission: 'read', + }) + await vi.advanceTimersByTimeAsync(31_000) + + const edit = new Y.Doc() + edit.getText(FILE_DOC_FIELD).insert(0, 'edit after revocation') + const editFrame = () => + handlers[FILE_DOC_EVENTS.MESSAGE]( + frame(FILE_DOC_MESSAGE_TYPE.SYNC, (e) => + syncProtocol.writeUpdate(e, Y.encodeStateAsUpdate(edit)) + ) + ) + + // The first frame after expiry finds nothing cached, so it is accepted and kicks + // off the authoritative re-read (never a synchronous DB wait on the relay path). + editFrame() + await vi.advanceTimersByTimeAsync(0) + + // The next frame is gated on the now-authoritative denial: dropped, and the socket + // is evicted rather than left holding the room. + editFrame() + await vi.advanceTimersByTimeAsync(0) + + expect(socket.emit).toHaveBeenCalledWith( + 'room-access-revoked', + expect.objectContaining({ room: { type: 'workspace-file-doc', id: 'file-revoked' } }) + ) + expect(socket.leave).toHaveBeenCalledWith('workspace-file-doc:file-revoked') + + // The binding is gone, so every later frame is inert — nothing is applied and + // nothing reaches the room. + const sentAfterEviction = sent.length + const emitsAfterEviction = socket.emit.mock.calls.length + editFrame() + await vi.advanceTimersByTimeAsync(0) + expect(sent.length).toBe(sentAfterEviction) + expect(socket.emit.mock.calls.length).toBe(emitsAfterEviction) + } finally { + vi.useRealTimers() + } + }) + + it('keeps relaying document frames while the cached permission still allows writing', async () => { + const { io, sent } = createIo() + const { socket, handlers } = setup('socket-1', io) + await handlers[FILE_DOC_EVENTS.JOIN]({ fileId: 'file-1', clientId: 1 }) + await flushMicrotasks() + + const before = sent.length + const edit = new Y.Doc() + edit.getText(FILE_DOC_FIELD).insert(0, 'still allowed') + handlers[FILE_DOC_EVENTS.MESSAGE]( + frame(FILE_DOC_MESSAGE_TYPE.SYNC, (e) => + syncProtocol.writeUpdate(e, Y.encodeStateAsUpdate(edit)) + ) + ) + await flushMicrotasks() + + expect(sent.slice(before).length).toBeGreaterThan(0) + expect(socket.emit).not.toHaveBeenCalledWith('room-access-revoked', expect.anything()) + }) + it('applies + fans out an agent-streamed frame (SYNC_NO_PERSIST) but never persists it', async () => { mockFetchFileDocSeed.mockResolvedValue(seedResult('# From server')) const { io, sent } = createIo() diff --git a/apps/realtime/src/handlers/file-doc.ts b/apps/realtime/src/handlers/file-doc.ts index a3d761bb40f..a0152fd85f6 100644 --- a/apps/realtime/src/handlers/file-doc.ts +++ b/apps/realtime/src/handlers/file-doc.ts @@ -24,6 +24,7 @@ * @module */ import { createLogger } from '@sim/logger' +import { ROOM_MEMBERSHIP_ACTIONS, satisfiesRoomMembership } from '@sim/platform-authz/room-policy' import { FILE_DOC_EVENTS, FILE_DOC_MESSAGE_TYPE, @@ -51,8 +52,10 @@ import { REDIS_ORIGIN, REDIS_SNAPSHOT_ORIGIN, } from '@/handlers/file-doc-store' +import { evictSocketFromRoom, registerRoomEvictionHandler } from '@/handlers/room-eviction' import { resolveRoomJoinAuth } from '@/handlers/room-join-auth' import type { AuthenticatedSocket } from '@/middleware/auth' +import { peekRoomPermission, resolveCurrentRoomPermission } from '@/middleware/permissions' import type { IRoomManager } from '@/rooms' const logger = createLogger('FileDocHandlers') @@ -837,11 +840,73 @@ function emitJoinError( }) } -function handleMessage(socket: AuthenticatedSocket, data: unknown) { +/** + * The permission occupying a file-doc room requires — `write`, since the room IS + * the collaborative editor. Sourced from the shared map so the join check, the + * per-frame gate below, and the re-validation sweep can never drift apart. + */ +const FILE_DOC_ACTION = ROOM_MEMBERSHIP_ACTIONS[ROOM_TYPES.WORKSPACE_FILE_DOC] + +/** + * Per-frame authorization for a socket's inbound document/awareness frames. + * + * Room membership alone is NOT a standing right to write: a collaborator removed + * from the workspace — or downgraded to `read` — must stop landing durable edits on + * an already-open socket, without waiting for the next re-validation sweep. This is + * the synchronous half of that enforcement; the sweep is the asynchronous half. + * + * Reads the shared role cache without awaiting, because this sits on the Yjs relay + * hot path (one call per keystroke-sized frame). Three outcomes: + * - fresh cached permission that satisfies `write` → accept. + * - fresh cached permission that does NOT → drop the frame and evict immediately. + * - nothing fresh cached (TTL lapsed) → accept this frame and kick off a background + * refresh, so the very next frames are gated on an authoritative read. Accepting + * is correct rather than lax: the entry was authoritative when written and every + * join records one, so the exposure is bounded by the same TTL the workflow + * write-path has always had, and the sweep evicts independently. + */ +function isFileDocWriteAllowed(socket: AuthenticatedSocket, io: Server, name: string): boolean { + const userId = socket.userId + const fileId = fileDocRooms.get(name)?.fileId + if (!userId || !fileId) return false + + const room = fileDocRoom(fileId) + const cached = peekRoomPermission(userId, room) + if (cached === undefined) { + // Single-flighted, so a burst of frames triggers at most one query. + void resolveCurrentRoomPermission(userId, room, FILE_DOC_ACTION).catch(() => {}) + return true + } + if (satisfiesRoomMembership(cached, ROOM_TYPES.WORKSPACE_FILE_DOC)) return true + + logger.warn( + `Dropping file-doc frame from user ${userId} whose access to file ${fileId} no longer permits writing` + ) + // Evicting (not just dropping the frame) is what makes this stick: the registered + // eviction handler clears `socketToRoomName`, and every inbound frame is gated on + // that binding — so the socket cannot apply another document update even if it + // keeps sending them. + evictSocketFromRoom(socket, room, 'Your access to this document has been revoked', io) + return false +} + +/** + * Reconciles this handler's pod-local state when the access re-validation sweep + * evicts a socket from a file-doc room (the sweep has already emitted the + * revocation and left the Socket.IO room). Scoped to the evicted room so a socket + * that has since switched documents keeps the one it legitimately holds. + */ +registerRoomEvictionHandler(ROOM_TYPES.WORKSPACE_FILE_DOC, (socketId, room, io) => { + if (socketToRoomName.get(socketId) !== roomName(room)) return + cleanupFileDocForSocket(socketId, io) +}) + +function handleMessage(socket: AuthenticatedSocket, io: Server, data: unknown) { const name = socketToRoomName.get(socket.id) if (!name) return const room = fileDocRooms.get(name) if (!room) return + if (!isFileDocWriteAllowed(socket, io, name)) return const bytes = toFileDocBytes(data) if (!bytes) return @@ -1009,7 +1074,7 @@ export function setupWorkspaceFileDocHandlers( const authorized = await resolveRoomJoinAuth({ userId, room, - action: 'write', + action: FILE_DOC_ACTION, logger, logLabel: `file-doc room for ${userId}`, messages: { @@ -1026,9 +1091,25 @@ export function setupWorkspaceFileDocHandlers( // awareness). Resolved here so the generation guard below also covers this await. const avatarUrl = await resolveAvatarUrl(socket, userId) + // Re-check access immediately before registering, mirroring the workflow join: the + // access re-validation sweep records a revocation BEFORE it evicts, so a join that + // authorized just before the revocation must not complete afterwards and re-bind + // the socket to the document. This RE-RESOLVES rather than peeking the cache — a + // peek treats an expired entry as unknown and fails open, which a join stalled + // longer than the cache TTL would slip straight through. Normally a cache hit (this + // join's own authorize just warmed it), so it costs no extra query. + const currentPermission = await resolveCurrentRoomPermission(userId, room, FILE_DOC_ACTION) + if (!satisfiesRoomMembership(currentPermission, ROOM_TYPES.WORKSPACE_FILE_DOC)) { + logger.warn(`User ${userId} lost write access to file ${fileId} before the join completed`) + emitJoinError(socket, fileId, 'Access denied to file', 'ACCESS_DENIED', false) + return + } + // Abort a JOIN superseded during authorization/identity resolution: the socket // disconnected, or a newer JOIN (a document switch) bumped the generation. Registering // here would leak a dead socket's room or bind the socket to the wrong document. + // Last await before the commit, so nothing can interleave between the access + // re-check above and the registration below. if (socket.disconnected || joinGeneration.get(socket.id) !== generation) return const entry = getOrCreateRoom(io, room) @@ -1148,7 +1229,7 @@ export function setupWorkspaceFileDocHandlers( } }) - socket.on(FILE_DOC_EVENTS.MESSAGE, (data: unknown) => handleMessage(socket, data)) + socket.on(FILE_DOC_EVENTS.MESSAGE, (data: unknown) => handleMessage(socket, io, data)) socket.on(FILE_DOC_EVENTS.LEAVE, (payload?: LeaveFileDocPayload) => { try { diff --git a/apps/realtime/src/handlers/room-eviction.ts b/apps/realtime/src/handlers/room-eviction.ts new file mode 100644 index 00000000000..2a417489b64 --- /dev/null +++ b/apps/realtime/src/handlers/room-eviction.ts @@ -0,0 +1,107 @@ +import { createLogger } from '@sim/logger' +import { + ROOM_ACCESS_REVOKED_EVENT, + type RoomAccessRevokedBroadcast, +} from '@sim/realtime-protocol/events' +import { type RoomRef, type RoomType, roomName } from '@sim/realtime-protocol/rooms' +import type { Server } from 'socket.io' +import type { AuthenticatedSocket } from '@/middleware/auth' + +const logger = createLogger('RoomEviction') + +/** + * Drops whatever pod-local state a handler keeps for a socket in one of its rooms. + * Called after the socket has already been removed from the Socket.IO room, so it + * only has to reconcile the handler's own bookkeeping. + */ +export type RoomEvictionHandler = (socketId: string, room: RoomRef, io: Server) => void + +const handlers = new Map() + +/** + * Registers a room type's local-state cleanup for involuntary eviction (the access + * re-validation sweep, or a per-frame gate that catches a revocation first). + * + * A registry rather than a direct import so the security-critical sweep stays + * domain-neutral: it never has to know that a file-doc room keeps an in-memory + * Y.Doc + ownership map while a table room keeps Redis presence. Handlers register + * at module load, which every handler module does at bootstrap. + */ +export function registerRoomEvictionHandler(type: RoomType, handler: RoomEvictionHandler): void { + handlers.set(type, handler) +} + +/** + * Runs the registered cleanup for an evicted socket, if the room type has one. + * Never throws — an eviction has already taken effect (the socket left the room) + * by the time this runs, so a failing cleanup must not unwind it. + */ +export function runRoomEvictionHandler(socketId: string, room: RoomRef, io: Server): void { + const handler = handlers.get(room.type) + if (!handler) return + try { + handler(socketId, room, io) + } catch (error) { + logger.warn(`Room eviction cleanup failed for socket ${socketId} on ${room.type}`, error) + } +} + +/** Enqueues a presence cleanup owed for an evicted socket. */ +type EvictionCleanupSink = (socketId: string, room: RoomRef) => void + +let evictionCleanupSink: EvictionCleanupSink | null = null + +/** + * Registers the retrying presence-cleanup lane (owned by the access-revalidation + * sweep) that handler-initiated evictions hand failed cleanups to. + * + * An eviction removes the socket from the Socket.IO room synchronously, which is + * also how the sweep discovers work — so once a handler evicts, the sweep can no + * longer find that (socket, room) pair to retry a Redis removal that failed. This + * sink is the handoff that keeps the sweep's retry semantics (re-join guard, + * moved-room guard, no infinite retry) as the single implementation instead of a + * second, subtly different retry loop per handler. Unset outside a running sweep, + * where {@link requestEvictionCleanup} is a no-op. + */ +export function setEvictionCleanupSink(sink: EvictionCleanupSink | null): void { + evictionCleanupSink = sink +} + +/** + * Asks the sweep's cleanup lane to (re)try presence removal for a socket already + * evicted from `room`. Call only when the immediate best-effort removal failed — + * the happy path stays immediate so peers see the departure without waiting for a + * sweep tick. + */ +export function requestEvictionCleanup(socketId: string, room: RoomRef): void { + evictionCleanupSink?.(socketId, room) +} + +/** + * Evicts one socket from one non-workflow room after a confirmed loss of access: + * tell the client, stop it receiving room broadcasts, and drop the handler-local + * state that would otherwise still accept its frames. + * + * The single eviction path shared by both enforcement points — the periodic + * re-validation sweep and the per-frame gates that catch a revocation first — so + * they cannot diverge on what "evicted" means. Everything here is synchronous and + * pod-local; any Redis presence removal is the caller's own follow-up (the sweep + * owns a retrying cleanup lane for it). + * + * Workflow rooms keep their own `access-revoked` wire event for client + * compatibility and are deliberately not routed through here. + */ +export function evictSocketFromRoom( + socket: AuthenticatedSocket, + room: RoomRef, + message: string, + io: Server +): void { + const payload: RoomAccessRevokedBroadcast = { room, message, timestamp: Date.now() } + socket.emit(ROOM_ACCESS_REVOKED_EVENT, payload) + socket.leave(roomName(room)) + runRoomEvictionHandler(socket.id, room, io) + logger.info( + `Revoked live access for user ${socket.userId} on ${roomName(room)} (socket ${socket.id})` + ) +} diff --git a/apps/realtime/src/handlers/room-join-auth.ts b/apps/realtime/src/handlers/room-join-auth.ts index a7c2a9aa24c..f54d6a81782 100644 --- a/apps/realtime/src/handlers/room-join-auth.ts +++ b/apps/realtime/src/handlers/room-join-auth.ts @@ -1,6 +1,7 @@ import type { createLogger } from '@sim/logger' import { authorizeRoom } from '@sim/platform-authz/rooms' import type { RoomRef } from '@sim/realtime-protocol/rooms' +import { beginRoomPermissionRead, commitRoomPermission } from '@/middleware/permissions' type Authorized = Awaited> @@ -34,6 +35,10 @@ export async function resolveRoomJoinAuth( const { userId, room, action, logger, logLabel, messages, emitError } = params let authorized: Authorized + // Taken before the query so this read is ordered against every other one: a + // decision from a later-started read (the access-revalidation sweep's denial) + // is never overwritten by this older result — see {@link commitRoomPermission}. + const readSeq = beginRoomPermissionRead() try { authorized = await authorizeRoom({ userId, room, action }) } catch (error) { @@ -42,6 +47,16 @@ export async function resolveRoomJoinAuth( return null } + // Feed the fresh authoritative read into the shared role cache that the access + // re-validation sweep and the per-frame write gates consult, so both start warm on + // the room this socket just joined — and so a re-granted user's join immediately + // supersedes a cached revocation instead of waiting out its TTL. A 400 (room type + // not authorizable here) resolved no permission at all and is deliberately not + // recorded. A 404 records `null`: the resource is genuinely gone. + if (authorized.status !== 400) { + commitRoomPermission(userId, room, authorized.workspacePermission, readSeq) + } + if (!authorized.allowed) { emitError({ error: authorized.status === 404 ? messages.notFound : messages.accessDenied, diff --git a/apps/realtime/src/handlers/tables.test.ts b/apps/realtime/src/handlers/tables.test.ts index 518c5ee376a..74310477d93 100644 --- a/apps/realtime/src/handlers/tables.test.ts +++ b/apps/realtime/src/handlers/tables.test.ts @@ -3,6 +3,7 @@ */ import { ROOM_TYPES } from '@sim/realtime-protocol/rooms' import { TABLE_PRESENCE_EVENTS } from '@sim/realtime-protocol/table-presence' +import { sleep } from '@sim/utils/helpers' import { beforeEach, describe, expect, it, vi } from 'vitest' import type { IRoomManager } from '@/rooms' @@ -19,7 +20,9 @@ vi.mock('@sim/platform-authz/rooms', () => ({ authorizeRoom: mockAuthorizeRoom, })) +import { setEvictionCleanupSink } from '@/handlers/room-eviction' import { setupTablesHandlers } from '@/handlers/tables' +import { beginRoomPermissionRead, commitRoomPermission } from '@/middleware/permissions' const TABLE_ROOM = { type: ROOM_TYPES.TABLE, id: 'table-1' } @@ -174,6 +177,202 @@ describe('setupTablesHandlers', () => { }) }) + it('does not join when access was revoked while the join was in flight', async () => { + // The sweep records a revocation before it evicts, so a join whose authorize + // completed just before that must not put the socket back in the room. + const { socket, handlers } = createSocket({ id: 'socket-race', userId: 'user-race' }) + const roomManager = createRoomManager() + setupTablesHandlers(socket as unknown as SetupArg, roomManager) + + mockAuthorizeRoom.mockImplementation(async () => { + commitRoomPermission( + 'user-race', + { type: ROOM_TYPES.TABLE, id: 'table-race' }, + null, + beginRoomPermissionRead() + ) + return { allowed: true, status: 200, workspaceId: 'ws-1', workspacePermission: 'admin' } + }) + + await handlers[TABLE_PRESENCE_EVENTS.JOIN]({ tableId: 'table-race' }) + + expect(socket.emit).toHaveBeenCalledWith( + TABLE_PRESENCE_EVENTS.JOIN_ERROR, + expect.objectContaining({ code: 'ACCESS_DENIED', retryable: false }) + ) + expect(socket.join).not.toHaveBeenCalled() + expect(roomManager.addUserToRoom).not.toHaveBeenCalled() + }) + + it('drops a cell selection and evicts once the viewer loses access mid-session', async () => { + // Distinct user/table so the recorded revocation cannot leak into sibling tests + // through the module-global role cache. + vi.useFakeTimers() + try { + const room = { type: ROOM_TYPES.TABLE, id: 'table-revoked' } + const { socket, handlers, toEmit } = createSocket({ id: 'socket-9', userId: 'user-9' }) + const roomManager = createRoomManager({ + getRoomForSocket: vi.fn().mockResolvedValue(room), + }) + setupTablesHandlers(socket as unknown as SetupArg, roomManager) + + await handlers[TABLE_PRESENCE_EVENTS.JOIN]({ tableId: 'table-revoked' }) + await vi.advanceTimersByTimeAsync(0) + + // Access removed, and the join-time decision expires. + mockAuthorizeRoom.mockResolvedValue({ + allowed: false, + status: 403, + workspaceId: 'ws-1', + workspacePermission: null, + }) + await vi.advanceTimersByTimeAsync(31_000) + + const cell = { + anchor: { rowId: 'row-1', columnId: 'col-a' }, + focus: { rowId: 'row-1', columnId: 'col-a' }, + } + // First selection after expiry finds nothing cached: accepted, and it kicks off the + // authoritative re-read rather than blocking the relay on a DB round-trip. + await handlers[TABLE_PRESENCE_EVENTS.CELL_SELECTION]({ cell }) + await vi.advanceTimersByTimeAsync(0) + + toEmit.mockClear() + await handlers[TABLE_PRESENCE_EVENTS.CELL_SELECTION]({ cell }) + await vi.advanceTimersByTimeAsync(0) + + expect(toEmit).not.toHaveBeenCalled() + expect(socket.emit).toHaveBeenCalledWith( + 'room-access-revoked', + expect.objectContaining({ room }) + ) + expect(socket.leave).toHaveBeenCalledWith('table:table-revoked') + expect(roomManager.removeUserFromRoom).toHaveBeenCalledWith(room, 'socket-9') + } finally { + vi.useRealTimers() + } + }) + + it('hands a failed presence removal to the sweep so the eviction stays retryable', async () => { + // The eviction already left the Socket.IO room, so the sweep's scan can no longer + // rediscover this socket — a failed removal here would strand a ghost collaborator + // until disconnect unless it is handed to the retrying cleanup lane. + vi.useFakeTimers() + const owed: Array<{ socketId: string; roomId: string }> = [] + setEvictionCleanupSink((socketId, room) => owed.push({ socketId, roomId: room.id })) + try { + const room = { type: ROOM_TYPES.TABLE, id: 'table-defer' } + const { socket, handlers } = createSocket({ id: 'socket-defer', userId: 'user-defer' }) + const roomManager = createRoomManager({ + getRoomForSocket: vi.fn().mockResolvedValue(room), + removeUserFromRoom: vi.fn().mockRejectedValue(new Error('redis down')), + }) + setupTablesHandlers(socket as unknown as SetupArg, roomManager) + + await handlers[TABLE_PRESENCE_EVENTS.JOIN]({ tableId: 'table-defer' }) + await vi.advanceTimersByTimeAsync(0) + + mockAuthorizeRoom.mockResolvedValue({ + allowed: false, + status: 403, + workspaceId: 'ws-1', + workspacePermission: null, + }) + await vi.advanceTimersByTimeAsync(31_000) + + const cell = { + anchor: { rowId: 'row-1', columnId: 'col-a' }, + focus: { rowId: 'row-1', columnId: 'col-a' }, + } + await handlers[TABLE_PRESENCE_EVENTS.CELL_SELECTION]({ cell }) + await vi.advanceTimersByTimeAsync(0) + await handlers[TABLE_PRESENCE_EVENTS.CELL_SELECTION]({ cell }) + await vi.advanceTimersByTimeAsync(0) + + expect(socket.leave).toHaveBeenCalledWith('table:table-defer') + expect(owed).toEqual([{ socketId: 'socket-defer', roomId: 'table-defer' }]) + } finally { + setEvictionCleanupSink(null) + vi.useRealTimers() + } + }) + + it('keeps the prior table room when a switch is denied at the access re-check', async () => { + // A denied switch must not silently drop the client from a table it may still be + // allowed to occupy, so the prior room is left only once the join is certain. + vi.useFakeTimers() + try { + const prior = { type: ROOM_TYPES.TABLE, id: 'table-prior' } + const { socket, handlers } = createSocket({ id: 'socket-switch', userId: 'user-switch' }) + const roomManager = createRoomManager({ + getRoomForSocket: vi.fn().mockResolvedValue(prior), + }) + setupTablesHandlers(socket as unknown as SetupArg, roomManager) + + let call = 0 + mockAuthorizeRoom.mockImplementation(async () => { + call += 1 + if (call === 1) { + // A later-started read drops this join's own decision, and the join stalls past + // the TTL so that decision is expired by re-check time — forcing the re-resolve + // down its database path below. + commitRoomPermission( + 'user-switch', + { type: ROOM_TYPES.TABLE, id: 'table-target' }, + 'admin', + beginRoomPermissionRead() + ) + await sleep(31_000) + return { allowed: true, status: 200, workspaceId: 'ws-1', workspacePermission: 'admin' } + } + // The authoritative current answer: access is gone. + return { allowed: false, status: 403, workspaceId: 'ws-1', workspacePermission: null } + }) + + const joining = handlers[TABLE_PRESENCE_EVENTS.JOIN]({ tableId: 'table-target' }) + await vi.advanceTimersByTimeAsync(31_000) + await joining + + expect(socket.emit).toHaveBeenCalledWith( + TABLE_PRESENCE_EVENTS.JOIN_ERROR, + expect.objectContaining({ code: 'ACCESS_DENIED', retryable: false }) + ) + // Neither joined the target nor abandoned the prior room. + expect(socket.join).not.toHaveBeenCalled() + expect(socket.leave).not.toHaveBeenCalled() + expect(roomManager.removeUserFromRoom).not.toHaveBeenCalled() + } finally { + vi.useRealTimers() + } + }) + + it('aborts the join when a revocation lands during the prior-room leave', async () => { + // The prior-room leave is the only await left between the authoritative access + // re-check and the commit, so a sweep revocation recorded in that window must still + // stop the join — `superseded()` alone only watches the join generation. + const prior = { type: ROOM_TYPES.TABLE, id: 'table-prior-2' } + const target = { type: ROOM_TYPES.TABLE, id: 'table-target-2' } + const { socket, handlers } = createSocket({ id: 'socket-window', userId: 'user-window' }) + const roomManager = createRoomManager({ + getRoomForSocket: vi.fn().mockResolvedValue(prior), + removeUserFromRoom: vi.fn().mockImplementation(async () => { + // The sweep records the revocation while the prior-room leave is in flight. + commitRoomPermission('user-window', target, null, beginRoomPermissionRead()) + return true + }), + }) + setupTablesHandlers(socket as unknown as SetupArg, roomManager) + + await handlers[TABLE_PRESENCE_EVENTS.JOIN]({ tableId: 'table-target-2' }) + + expect(socket.emit).toHaveBeenCalledWith( + TABLE_PRESENCE_EVENTS.JOIN_ERROR, + expect.objectContaining({ code: 'ACCESS_DENIED', retryable: false }) + ) + expect(socket.join).not.toHaveBeenCalled() + expect(roomManager.addUserToRoom).not.toHaveBeenCalled() + }) + it('drops a malformed cell selection without storing or relaying it', async () => { const { socket, handlers, toEmit } = createSocket() const roomManager = createRoomManager({ diff --git a/apps/realtime/src/handlers/tables.ts b/apps/realtime/src/handlers/tables.ts index 2474063c9a0..6fad57fb6d6 100644 --- a/apps/realtime/src/handlers/tables.ts +++ b/apps/realtime/src/handlers/tables.ts @@ -1,4 +1,5 @@ import { createLogger } from '@sim/logger' +import { ROOM_MEMBERSHIP_ACTIONS, satisfiesRoomMembership } from '@sim/platform-authz/room-policy' import { ROOM_TYPES, type RoomRef, roomName } from '@sim/realtime-protocol/rooms' import { type JoinTablePayload, @@ -7,8 +8,10 @@ import { type TableCellSelection, } from '@sim/realtime-protocol/table-presence' import { resolveAvatarUrl } from '@/handlers/avatar' +import { evictSocketFromRoom, requestEvictionCleanup } from '@/handlers/room-eviction' import { resolveRoomJoinAuth } from '@/handlers/room-join-auth' import type { AuthenticatedSocket } from '@/middleware/auth' +import { peekRoomPermission, resolveCurrentRoomPermission } from '@/middleware/permissions' import type { IRoomManager, UserPresence } from '@/rooms' import { filterVisiblePresence, sweepStalePresence } from '@/rooms/presence-visibility' @@ -20,6 +23,41 @@ const MAX_CELL_ID_LENGTH = 200 /** The table presence room ref for a table id. */ const tableRoom = (tableId: string): RoomRef => ({ type: ROOM_TYPES.TABLE, id: tableId }) +/** + * The permission occupying a table room requires. Sourced from the shared map so + * the join check, the per-operation gate below, and the access re-validation sweep + * can never drift apart. + */ +const TABLE_ACTION = ROOM_MEMBERSHIP_ACTIONS[ROOM_TYPES.TABLE] + +/** + * Per-operation authorization for an already-joined socket, mirroring the file-doc + * gate: room membership is not a standing right, so a collaborator whose workspace + * access was revoked stops publishing presence into the room without waiting for + * the next re-validation sweep. + * + * Reads the shared role cache without awaiting (`undefined` = nothing fresh cached, + * which means "unknown", not "denied") and kicks off a background refresh in that + * case, so the exposure window is the cache TTL rather than the socket's lifetime. + * On a confirmed loss of access it evicts, which also clears the room mapping this + * handler resolves every operation through. + */ +function isTableAccessAllowed( + socket: AuthenticatedSocket, + room: RoomRef +): { allowed: boolean; revoked: boolean } { + const userId = socket.userId + if (!userId) return { allowed: false, revoked: false } + + const cached = peekRoomPermission(userId, room) + if (cached === undefined) { + void resolveCurrentRoomPermission(userId, room, TABLE_ACTION).catch(() => {}) + return { allowed: true, revoked: false } + } + if (satisfiesRoomMembership(cached, ROOM_TYPES.TABLE)) return { allowed: true, revoked: false } + return { allowed: false, revoked: true } +} + function isCellRef(value: unknown): value is TableCellRef { if (typeof value !== 'object' || value === null) return false const ref = value as { rowId?: unknown; columnId?: unknown } @@ -50,6 +88,42 @@ function normalizeCellSelection(cell: unknown): TableCellSelection | undefined { } } +/** + * Evicts a socket from a table room after a confirmed loss of access, then drops + * its presence so peers stop seeing its selection. + * + * The eviction leaves the Socket.IO room immediately, which is also how the sweep + * discovers work — so a presence removal that fails here would be unretryable and + * strand a ghost collaborator until disconnect. The removal is therefore attempted + * inline (peers see the departure at once) and handed to the sweep's retrying + * cleanup lane if it fails or reports nothing removed, which is the same signal + * the sweep treats as a deferrable failure. + */ +async function evictFromTable( + socket: AuthenticatedSocket, + roomManager: IRoomManager, + room: RoomRef +): Promise { + evictSocketFromRoom(socket, room, 'Your access to this table has been revoked', roomManager.io) + try { + // `false` conflates "already gone" with a transport error the manager swallowed; + // deferring on it is harmless (the lane drops a cleanup it finds already clean) + // and is the only signal a swallowed failure gives us. + const removed = await roomManager.removeUserFromRoom(room, socket.id) + if (!removed) { + requestEvictionCleanup(socket.id, room) + return + } + await roomManager.broadcastPresenceUpdate(room, socket.id) + } catch (error) { + logger.warn( + `Presence cleanup failed for evicted table socket ${socket.id}; deferring to the sweep`, + error + ) + requestEvictionCleanup(socket.id, room) + } +} + /** * Live cell-selection presence for the table grid. Mirrors the workspace-files * join flow but is table-scoped (room id = tableId) with a bidirectional @@ -135,7 +209,7 @@ export function setupTablesHandlers(socket: AuthenticatedSocket, roomManager: IR const authorized = await resolveRoomJoinAuth({ userId, room, - action: 'read', + action: TABLE_ACTION, logger, logLabel: `table room for ${userId}`, messages: { @@ -151,16 +225,6 @@ export function setupTablesHandlers(socket: AuthenticatedSocket, roomManager: IR // Server-authenticated avatar for the presence roster. const avatarUrl = await resolveAvatarUrl(socket, userId) - // Leave a previously-joined table room if switching tables. No generation guard is needed - // around this: serialization guarantees no concurrent op committed to a different room - // during the lookup, so `currentRoom` is the socket's genuine prior room, safe to leave. - const currentRoom = await roomManager.getRoomForSocket(socket.id, ROOM_TYPES.TABLE) - if (currentRoom && currentRoom.id !== tableId) { - socket.leave(roomName(currentRoom)) - await roomManager.removeUserFromRoom(currentRoom, socket.id) - await roomManager.broadcastPresenceUpdate(currentRoom) - } - // Reclaim presence orphaned by an ungraceful disconnect (no `disconnecting` // event fires on a pod crash; the room hashes have no TTL). Returns the roster it // read so the same-tab dedup below reuses it instead of issuing a second read. @@ -182,10 +246,64 @@ export function setupTablesHandlers(socket: AuthenticatedSocket, roomManager: IR } } + // Re-check access too: the access re-validation sweep records a revocation BEFORE + // it evicts, so a join that authorized just before the revocation must not + // complete afterwards and put the socket back in the room. RE-RESOLVES rather + // than peeking — a peek treats an expired entry as unknown and fails open, which + // a join stalled longer than the cache TTL would slip through. Normally a cache + // hit (this join's own authorize just warmed it). + const currentPermission = await resolveCurrentRoomPermission(userId, room, TABLE_ACTION) + if (!satisfiesRoomMembership(currentPermission, ROOM_TYPES.TABLE)) { + socket.emit(TABLE_PRESENCE_EVENTS.JOIN_ERROR, { + tableId, + error: 'Access denied to table', + code: 'ACCESS_DENIED', + retryable: false, + }) + return + } + + // Only now that the join is certain to proceed, leave a previously-joined table room + // if switching. Deliberately AFTER the access re-check: a denial there aborts the + // join, and leaving first would silently drop the client from a prior table it may + // still be allowed to occupy. No generation guard is needed around this — + // serialization guarantees no concurrent op committed to a different room during the + // lookup, so `currentRoom` is the socket's genuine prior room, safe to leave. + const currentRoom = await roomManager.getRoomForSocket(socket.id, ROOM_TYPES.TABLE) + if (currentRoom && currentRoom.id !== tableId) { + socket.leave(roomName(currentRoom)) + await roomManager.removeUserFromRoom(currentRoom, socket.id) + await roomManager.broadcastPresenceUpdate(currentRoom) + } + // Final re-check before the membership commit: a LEAVE or a newer JOIN enqueued during the - // awaits above bumped the generation, or the socket disconnected. Abort before registering. + // awaits above — including the access re-resolve — bumped the generation, or the socket + // disconnected. if (superseded()) return + // The prior-room leave above is the one place this handler still awaits AFTER the + // authoritative access re-check (file-doc and the workspace-list rooms leave + // synchronously, so they have no such window). A sweep revocation landing in that + // window would otherwise let this join put a revoked socket back in the room, since + // `superseded()` only watches the join generation. A cache PEEK is the right + // instrument here and needs no await: the authoritative resolve moments ago wrote a + // fresh entry, so the only way this reads differently is a newer decision recorded + // since — exactly the revocation being guarded against. Synchronous, so nothing can + // interleave between it and the join below. + // `undefined` stays "unknown, not denied" here as everywhere else in this handler — + // only a definitively cached insufficient permission aborts a join the authoritative + // check just passed. + const finalCheck = peekRoomPermission(userId, room) + if (finalCheck !== undefined && !satisfiesRoomMembership(finalCheck, ROOM_TYPES.TABLE)) { + socket.emit(TABLE_PRESENCE_EVENTS.JOIN_ERROR, { + tableId, + error: 'Access denied to table', + code: 'ACCESS_DENIED', + retryable: false, + }) + return + } + socket.join(roomName(room)) const presence: UserPresence = { @@ -298,6 +416,14 @@ export function setupTablesHandlers(socket: AuthenticatedSocket, roomManager: IR const room = await roomManager.getRoomForSocket(socket.id, ROOM_TYPES.TABLE) if (!room) return + // Membership was authorized at join; re-check it here so a revoked viewer + // stops publishing presence into the room mid-session. + const access = isTableAccessAllowed(socket, room) + if (!access.allowed) { + if (access.revoked) await evictFromTable(socket, roomManager, room) + return + } + // Persist so a later joiner sees this viewer's current selection in the join ack. await roomManager.updateUserActivity(room, socket.id, { cell: selection }) diff --git a/apps/realtime/src/handlers/workspace-invalidation-room.test.ts b/apps/realtime/src/handlers/workspace-invalidation-room.test.ts index 643fca696f0..e487edd0a8e 100644 --- a/apps/realtime/src/handlers/workspace-invalidation-room.test.ts +++ b/apps/realtime/src/handlers/workspace-invalidation-room.test.ts @@ -2,6 +2,7 @@ * @vitest-environment node */ import { ROOM_TYPES } from '@sim/realtime-protocol/rooms' +import { sleep } from '@sim/utils/helpers' import { beforeEach, describe, expect, it, vi } from 'vitest' import type { IRoomManager } from '@/rooms' @@ -19,6 +20,7 @@ vi.mock('@sim/platform-authz/rooms', () => ({ })) import { setupWorkspaceInvalidationRoom } from '@/handlers/workspace-invalidation-room' +import { beginRoomPermissionRead, commitRoomPermission } from '@/middleware/permissions' type Payload = { workspaceId?: string } @@ -157,6 +159,82 @@ describe.each([ROOM_TYPES.WORKSPACE_FILES, ROOM_TYPES.WORKSPACE_TABLES] as const expect(roomManager.broadcastPresenceUpdate).not.toHaveBeenCalled() }) + it('aborts a join superseded during the access re-check await', async () => { + // The access re-resolve is an await like any other: a leave landing during it must + // still cancel this join, or the stale join would leave the room the client + // switched to and commit the abandoned one. Forced down the re-resolve's DB path + // by expiring the cached decision mid-join, so the interleaving is deterministic + // rather than dependent on microtask ordering. + vi.useFakeTimers() + try { + const { handlers, socket } = createSocket({ id: 'socket-sup', userId: 'user-sup' }) + setupWorkspaceInvalidationRoom( + socket as unknown as Parameters[0], + createRoomManager(), + roomType + ) + + let call = 0 + mockAuthorizeRoom.mockImplementation(async () => { + call += 1 + if (call === 1) { + // A later-started read commits, so this join's own decision is dropped; then + // the join stalls past the TTL so that decision is expired by re-check time. + commitRoomPermission( + 'user-sup', + { type: roomType, id: 'ws-sup' }, + 'admin', + beginRoomPermissionRead() + ) + await sleep(31_000) + } else { + // Second call is the re-check's re-resolve: the client leaves during it. + handlers[leaveEvent]({ workspaceId: 'ws-sup' }) + } + return { allowed: true, status: 200, workspaceId: 'ws-sup', workspacePermission: 'admin' } + }) + + const joining = handlers[joinEvent]({ workspaceId: 'ws-sup' }) + await vi.advanceTimersByTimeAsync(31_000) + await joining + + expect(call).toBe(2) + expect(socket.join).not.toHaveBeenCalled() + expect(socket.emit).not.toHaveBeenCalledWith(successEvent, expect.anything()) + } finally { + vi.useRealTimers() + } + }) + + it('does not join when access was revoked while the join was in flight', async () => { + // The sweep records a revocation before it evicts, so a join whose authorize + // completed just before that must not put the socket back in the room. + const { handlers, socket } = createSocket({ id: 'socket-race', userId: 'user-race' }) + setupWorkspaceInvalidationRoom( + socket as unknown as Parameters[0], + createRoomManager(), + roomType + ) + + mockAuthorizeRoom.mockImplementation(async () => { + commitRoomPermission( + 'user-race', + { type: roomType, id: 'ws-race' }, + null, + beginRoomPermissionRead() + ) + return { allowed: true, status: 200, workspaceId: 'ws-race', workspacePermission: 'admin' } + }) + + await handlers[joinEvent]({ workspaceId: 'ws-race' }) + + expect(socket.emit).toHaveBeenCalledWith( + errorEvent, + expect.objectContaining({ code: 'ACCESS_DENIED', retryable: false }) + ) + expect(socket.join).not.toHaveBeenCalled() + }) + it('leaves a previously-joined room when switching workspaces', async () => { const { socket, handlers, rooms } = createSocket() rooms.add(roomOf('ws-old')) diff --git a/apps/realtime/src/handlers/workspace-invalidation-room.ts b/apps/realtime/src/handlers/workspace-invalidation-room.ts index aacb141e9ae..362e37875f6 100644 --- a/apps/realtime/src/handlers/workspace-invalidation-room.ts +++ b/apps/realtime/src/handlers/workspace-invalidation-room.ts @@ -1,7 +1,9 @@ import { createLogger } from '@sim/logger' +import { ROOM_MEMBERSHIP_ACTIONS, satisfiesRoomMembership } from '@sim/platform-authz/room-policy' import { type RoomRef, type RoomType, roomName } from '@sim/realtime-protocol/rooms' import { resolveRoomJoinAuth } from '@/handlers/room-join-auth' import type { AuthenticatedSocket } from '@/middleware/auth' +import { resolveCurrentRoomPermission } from '@/middleware/permissions' import type { IRoomManager } from '@/rooms' const logger = createLogger('WorkspaceInvalidationRoom') @@ -89,7 +91,7 @@ export function setupWorkspaceInvalidationRoom( const authorized = await resolveRoomJoinAuth({ userId: socket.userId, room: ref, - action: 'read', + action: ROOM_MEMBERSHIP_ACTIONS[roomType], logger, logLabel: `${roomType} room for ${socket.userId}`, messages: { @@ -102,8 +104,31 @@ export function setupWorkspaceInvalidationRoom( }) if (!authorized) return - // A newer join started on this socket during authorize (or it dropped): abort so a - // stale join can't leave the room the client has since switched to. + // Re-check access before committing: the access re-validation sweep records a + // revocation BEFORE it evicts, so a join that authorized just before the + // revocation must not complete afterwards and put the socket back in the room. + // RE-RESOLVES rather than peeking — a peek treats an expired entry as unknown and + // fails open, which a join stalled longer than the cache TTL would slip through. + // Normally a cache hit (this join's own authorize just warmed it). Mirrors the + // file-doc and table joins. + const currentPermission = await resolveCurrentRoomPermission( + socket.userId, + ref, + ROOM_MEMBERSHIP_ACTIONS[roomType] + ) + if (!satisfiesRoomMembership(currentPermission, roomType)) { + socket.emit(errorEvent, { + workspaceId, + error: 'Access denied to workspace', + code: 'ACCESS_DENIED', + retryable: false, + }) + return + } + + // A newer join started on this socket during the awaits above — including the access + // re-resolve — or it dropped: abort so a stale join can't leave the room the client has + // since switched to. Last await before the commit, so nothing interleaves after it. if (joinGeneration !== joinAttempt || socket.disconnected) return // Leave any previously-joined room of this type (workspace switch), read straight from the diff --git a/apps/realtime/src/middleware/permissions.test.ts b/apps/realtime/src/middleware/permissions.test.ts index d9f0b4d15f0..130b1a262a2 100644 --- a/apps/realtime/src/middleware/permissions.test.ts +++ b/apps/realtime/src/middleware/permissions.test.ts @@ -519,6 +519,43 @@ describe('verifyWorkflowAccess role-cache refresh', () => { expect(await resolveCurrentWorkflowRole(userId, workflowId, 'read')).toBe('write') }) + it('does not let a stale join-time allow bury a sweep denial that started later', async () => { + const userId = 'vw-user-3' + const workflowId = 'vw-wf-3' + + // Hand out a controllable promise per authorization call, so the two reads can be + // started in one order and settled in the other. + const settle: Array<(value: { allowed: boolean; workspacePermission: string | null }) => void> = + [] + mockAuthorize.mockImplementation( + () => + new Promise((resolve) => { + settle.push(resolve) + }) + ) + + // A join-style verify starts FIRST (against pre-revocation state) and stalls. + const staleJoin = verifyWorkflowAccess(userId, workflowId) + for (let i = 0; i < 10 && settle.length < 1; i++) await Promise.resolve() + expect(settle).toHaveLength(1) + + // The sweep's authorization starts AFTER it, and stalls too. + const sweep = resolveCurrentWorkflowRole(userId, workflowId, 'read') + for (let i = 0; i < 10 && settle.length < 2; i++) await Promise.resolve() + expect(settle).toHaveLength(2) + + // The sweep's denial lands first, then the older join's allow. Ordering by WRITE + // time would let the join bury the denial and hand the socket another full TTL of + // access; ordering by read start keeps the denial in force. + settle[1]({ allowed: false, workspacePermission: null }) + expect(await sweep).toBeNull() + settle[0]({ allowed: true, workspacePermission: 'write' }) + await staleJoin + + mockAuthorize.mockRejectedValue(new Error('must not re-query')) + expect(await resolveCurrentWorkflowRole(userId, workflowId, 'read')).toBeNull() + }) + it('does not let a stale in-flight resolution overwrite a fresher verify decision', async () => { const userId = 'vw-user-2' const workflowId = 'vw-wf-2' diff --git a/apps/realtime/src/middleware/permissions.ts b/apps/realtime/src/middleware/permissions.ts index 779814257bf..7d88ed4b865 100644 --- a/apps/realtime/src/middleware/permissions.ts +++ b/apps/realtime/src/middleware/permissions.ts @@ -1,6 +1,7 @@ import { db } from '@sim/db' import { workflow } from '@sim/db/schema' import { createLogger } from '@sim/logger' +import { authorizeRoom, type PermissionType } from '@sim/platform-authz/rooms' import { authorizeWorkflowByWorkspacePermission } from '@sim/platform-authz/workflow' import { BLOCK_OPERATIONS, @@ -12,6 +13,7 @@ import { VARIABLE_OPERATIONS, WORKFLOW_OPERATIONS, } from '@sim/realtime-protocol/constants' +import { ROOM_TYPES, type RoomRef, roomName } from '@sim/realtime-protocol/rooms' import { and, eq, isNull } from 'drizzle-orm' const logger = createLogger('SocketPermissions') @@ -85,11 +87,12 @@ export function checkRolePermission( } /** - * TTL for the per-pod role cache backing live re-validation. It gates both the - * mutating-operation checks ({@link checkWorkflowOperationPermission}) and the - * periodic read-access sweep (`access-revalidation.ts`), so a revoked or - * downgraded collaborator loses write access — and live reads — on an - * already-connected socket within a bounded window rather than until disconnect. + * TTL for the per-pod role cache backing live re-validation. It gates the + * mutating-operation checks ({@link checkWorkflowOperationPermission}), the + * per-frame room write gates (file-doc / tables), and the periodic access sweep + * (`access-revalidation.ts`), so a revoked or downgraded collaborator loses write + * access — and live reads — on an already-connected socket within a bounded + * window rather than until disconnect. */ export const ROLE_REVALIDATION_TTL_MS = 30_000 @@ -99,11 +102,42 @@ const MAX_ROLE_CACHE_ENTRIES = 5_000 interface CachedRole { /** Authoritative workspace role, or `null` when the user has no access. */ role: string | null + /** + * The {@link beginRoomPermissionRead} ticket of the query this decision came + * from — i.e. when its DB read STARTED, not when it was written. + */ + readSeq: number expiresAt: number } /** - * Per-pod cache of authoritative workspace roles, keyed by `${userId}:${workflowId}`. + * Monotonic ticket counter establishing a total order over authorization READS. + * + * Ordering by write time is not sufficient: two readers can start their queries in + * one order and finish in the other, so the decision written last can be derived + * from the older read. A join that authorized before a revocation but returned + * after the sweep's denial would then win — leaving a revoked user with another + * full TTL of access. Every writer therefore takes a ticket before it queries and + * a decision only yields to one from a strictly later-started read. + */ +let roleReadSeqCounter = 0 + +/** + * Takes a read ticket. Call immediately BEFORE issuing an authorization query, and + * pass the result to {@link commitRoomPermission} once it returns. + */ +export function beginRoomPermissionRead(): number { + return ++roleReadSeqCounter +} + +/** + * Per-pod cache of authoritative workspace roles, keyed by + * `${userId}:${roomName(room)}`. + * + * The key uses the wire room name, so a workflow entry is `${userId}:${workflowId}` + * exactly as before (workflow rooms are unprefixed) while every other room type is + * namespaced by its own `type:id` — a file-doc and a table can never collide on an + * id, and one cache serves every room type. * * Socket connections are sticky to a single pod, so a socket's mutating operations are * always gated by the same pod's cache. We rely on TTL expiry (not cross-pod @@ -114,13 +148,17 @@ const roleCache = new Map() /** * In-flight resolutions keyed like {@link roleCache}. Concurrent callers for the same - * (user, workflow) share one authorization query instead of racing independent ones, so + * (user, room) share one authorization query instead of racing independent ones, so * cache writes per key are serialized — a slow, stale pre-revocation read can never * overwrite a newer recorded decision (e.g. the revocation the eviction sweep just * cached before kicking the socket). */ const inFlightRoleResolutions = new Map>() +function roleCacheKey(userId: string, room: RoomRef): string { + return `${userId}:${roomName(room)}` +} + function purgeExpiredRoles(now: number): void { for (const [key, entry] of roleCache) { if (entry.expiresAt <= now) { @@ -129,48 +167,81 @@ function purgeExpiredRoles(now: number): void { } } -/** - * Records a freshly-read authoritative decision into the role cache. Every - * successful DB read of a user's workspace role goes through this — including - * the join-time {@link verifyWorkflowAccess} — so a stale cached revocation - * never outlives a newer authoritative read (e.g. a re-granted user re-joining - * within the TTL of the sweep's recorded `null`). - */ -function recordRoleDecision(key: string, role: string | null): void { +function recordRoleDecision(key: string, role: string | null, readSeq: number): void { const now = Date.now() if (roleCache.size >= MAX_ROLE_CACHE_ENTRIES) { purgeExpiredRoles(now) } - roleCache.set(key, { role, expiresAt: now + ROLE_REVALIDATION_TTL_MS }) + roleCache.set(key, { role, readSeq, expiresAt: now + ROLE_REVALIDATION_TTL_MS }) +} + +/** + * Commits a freshly-read authoritative decision, unless a decision from a + * strictly later-started read is already cached — in which case that one stands + * and is returned instead. + * + * Every successful DB read of a user's workspace role goes through this: the + * cached resolver, the join-time {@link verifyWorkflowAccess}, and the room join + * authorizer. That is what keeps a stale cached revocation from outliving a newer + * authoritative read (a re-granted user re-joining inside the sweep's recorded + * `null` TTL) while equally keeping a stale ALLOW from burying the sweep's fresher + * denial. Returns the decision that ends up in force. + */ +function commitRoleDecision(key: string, role: string | null, readSeq: number): string | null { + const existing = roleCache.get(key) + if (existing !== undefined && existing.readSeq > readSeq) return existing.role + recordRoleDecision(key, role, readSeq) + return role +} + +/** + * Reads a user's authoritative effective workspace permission for a room, or + * `null` when they have none. Workflow rooms go through their own dedicated + * authorizer; every other room type resolves through the shared + * {@link authorizeRoom} (resource → owning workspace → effective permission). + * + * Always asks for the LOWEST level (`read`) so the resolved permission itself is + * returned rather than a boolean — callers compare it against whatever level the + * operation needs with `permissionSatisfies`. A room type that is not + * authorizable here (status 400) THROWS, so it lands in the caller's transient + * branch and can never be mistaken for a confirmed revocation. + */ +async function readAuthoritativeRoomPermission( + userId: string, + room: RoomRef +): Promise { + if (room.type === ROOM_TYPES.WORKFLOW) { + const authorization = await authorizeWorkflowByWorkspacePermission({ + workflowId: room.id, + userId, + action: 'read', + }) + return authorization.allowed ? (authorization.workspacePermission ?? null) : null + } + + const authorization = await authorizeRoom({ userId, room, action: 'read' }) + if (!authorization.allowed && authorization.status === 400) { + throw new Error(`Room type not authorizable: ${room.type}`) + } + return authorization.allowed ? (authorization.workspacePermission ?? null) : null } async function resolveRoleUncached( key: string, userId: string, - workflowId: string, + room: RoomRef, fallbackRole: string ): Promise { - const entryBeforeQuery = roleCache.get(key) + const readSeq = beginRoomPermissionRead() try { - const authorization = await authorizeWorkflowByWorkspacePermission({ - workflowId, - userId, - action: 'read', - }) - const role = authorization.allowed ? (authorization.workspacePermission ?? null) : null - // A fresh authoritative read (e.g. a join-time verifyWorkflowAccess) may - // have recorded a decision while this query was in flight. That write is - // newer than this query's read snapshot, so prefer it instead of - // overwriting it with a potentially stale result. - const entryAfterQuery = roleCache.get(key) - if (entryAfterQuery !== undefined && entryAfterQuery !== entryBeforeQuery) { - return entryAfterQuery.role - } - recordRoleDecision(key, role) - return role + const role = await readAuthoritativeRoomPermission(userId, room) + // Yields only to a decision from a later-STARTED read (see commitRoleDecision). + // Comparing write order instead would let a join whose authorize began before + // this one — but returned after it — bury this result. + return commitRoleDecision(key, role, readSeq) } catch (error) { logger.warn( - `Failed to re-validate role for user ${userId} on workflow ${workflowId}; using last known role`, + `Failed to re-validate role for user ${userId} on ${roomName(room)}; using last known role`, error ) // Prefer the last recorded decision — even if expired, and even if it is `null` for an @@ -183,23 +254,23 @@ async function resolveRoleUncached( } /** - * Resolves a user's current workspace role for a workflow, re-reading the `permissions` - * table at most once per {@link ROLE_REVALIDATION_TTL_MS} per pod. Concurrent calls for - * the same (user, workflow) coalesce onto a single in-flight query (single-flight), so - * out-of-order cache writes cannot resurrect revoked access. + * Resolves a user's current effective workspace permission for a room, re-reading the + * `permissions` table at most once per {@link ROLE_REVALIDATION_TTL_MS} per pod. + * Concurrent calls for the same (user, room) coalesce onto a single in-flight query + * (single-flight), so out-of-order cache writes cannot resurrect revoked access. * - * Returns `null` when the user genuinely has no access (removed/revoked). On a transient - * DB failure it reuses the last recorded decision for this (user, workflow) — including a - * previously recorded revocation (`null`) — and only falls back to `fallbackRole` when no - * decision has been recorded yet, so a blip neither blocks legitimate editors nor - * resurrects already-revoked access. + * Returns `null` when the user genuinely has no access (removed/revoked, or the room's + * resource is gone). On a transient DB failure it reuses the last recorded decision for + * this (user, room) — including a previously recorded revocation (`null`) — and only + * falls back to `fallbackRole` when no decision has been recorded yet, so a blip neither + * blocks legitimate editors nor resurrects already-revoked access. */ -export async function resolveCurrentWorkflowRole( +export async function resolveCurrentRoomPermission( userId: string, - workflowId: string, + room: RoomRef, fallbackRole: string ): Promise { - const key = `${userId}:${workflowId}` + const key = roleCacheKey(userId, room) const cached = roleCache.get(key) if (cached && cached.expiresAt > Date.now()) { return cached.role @@ -210,13 +281,65 @@ export async function resolveCurrentWorkflowRole( return inFlight } - const resolution = resolveRoleUncached(key, userId, workflowId, fallbackRole).finally(() => { + const resolution = resolveRoleUncached(key, userId, room, fallbackRole).finally(() => { inFlightRoleResolutions.delete(key) }) inFlightRoleResolutions.set(key, resolution) return resolution } +/** + * Workflow-room specialization of {@link resolveCurrentRoomPermission}, kept as the + * name the workflow operation/join paths already use. + */ +export function resolveCurrentWorkflowRole( + userId: string, + workflowId: string, + fallbackRole: string +): Promise { + return resolveCurrentRoomPermission( + userId, + { type: ROOM_TYPES.WORKFLOW, id: workflowId }, + fallbackRole + ) +} + +/** + * Non-blocking read of the cached permission for a (user, room). + * + * Returns `undefined` when nothing fresh is cached — the caller must NOT read that + * as a denial; it means "unknown, ask asynchronously". Exists for the per-frame + * gates on hot relay paths (Yjs document frames, table cell selections), which must + * stay synchronous: awaiting an authorization promise per keystroke would put a + * microtask between every frame and its apply. Freshness is bounded by the same + * {@link ROLE_REVALIDATION_TTL_MS}, and the periodic sweep independently evicts a + * revoked socket, so a gate built on this is never the only line of defense. + */ +export function peekRoomPermission(userId: string, room: RoomRef): string | null | undefined { + const cached = roleCache.get(roleCacheKey(userId, room)) + if (!cached || cached.expiresAt <= Date.now()) return undefined + return cached.role +} + +/** + * Commits an authoritative decision read outside this module (the room join + * authorizer) into the shared cache, so the sweep and the per-frame gates start + * warm on the room a socket just joined. + * + * `readSeq` must come from a {@link beginRoomPermissionRead} taken BEFORE the + * caller's query: the decision is discarded if a later-started read already + * committed one, so neither a stale ALLOW can bury the sweep's fresher denial nor + * a stale revocation outlive a re-grant. + */ +export function commitRoomPermission( + userId: string, + room: RoomRef, + permission: PermissionType | null, + readSeq: number +): void { + commitRoleDecision(roleCacheKey(userId, room), permission, readSeq) +} + /** * Live permission gate for mutating socket operations. Re-validates the user's workspace * role against the database (cached per pod for {@link ROLE_REVALIDATION_TTL_MS}) so that @@ -256,6 +379,9 @@ export async function verifyWorkflowAccess( userId: string, workflowId: string ): Promise<{ hasAccess: boolean; role?: string; workspaceId?: string }> { + // Taken before the reads below, so this decision yields to any authorization + // that started later (e.g. the sweep's denial) instead of by write order. + const readSeq = beginRoomPermissionRead() try { const workflowData = await db .select({ @@ -278,9 +404,10 @@ export async function verifyWorkflowAccess( action: 'read', }) - recordRoleDecision( - `${userId}:${workflowId}`, - authorization.allowed ? (authorization.workspacePermission ?? null) : null + commitRoleDecision( + roleCacheKey(userId, { type: ROOM_TYPES.WORKFLOW, id: workflowId }), + authorization.allowed ? (authorization.workspacePermission ?? null) : null, + readSeq ) if (!authorization.allowed || !authorization.workspacePermission) { diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/file-doc-provider.ts b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/file-doc-provider.ts index 9b55ac7b907..84bab0733bc 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/file-doc-provider.ts +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/collaboration/file-doc-provider.ts @@ -1,3 +1,7 @@ +import { + ROOM_ACCESS_REVOKED_EVENT, + type RoomAccessRevokedBroadcast, +} from '@sim/realtime-protocol/events' import { FILE_DOC_EVENTS, FILE_DOC_MESSAGE_TYPE, @@ -7,6 +11,7 @@ import { type JoinFileDocSuccess, toFileDocBytes, } from '@sim/realtime-protocol/file-doc' +import { ROOM_TYPES } from '@sim/realtime-protocol/rooms' import * as decoding from 'lib0/decoding' import * as encoding from 'lib0/encoding' import { ObservableV2 } from 'lib0/observable' @@ -131,6 +136,7 @@ export class FileDocProvider extends ObservableV2 { socket.on(FILE_DOC_EVENTS.MESSAGE, this.handleMessage) socket.on(FILE_DOC_EVENTS.JOIN_SUCCESS, this.handleJoinSuccess) socket.on(FILE_DOC_EVENTS.JOIN_ERROR, this.handleJoinError) + socket.on(ROOM_ACCESS_REVOKED_EVENT, this.handleAccessRevoked) socket.on('connect', this.handleConnect) doc.on('update', this.handleDocUpdate) awareness.on('update', this.handleAwarenessUpdate) @@ -231,6 +237,30 @@ export class FileDocProvider extends ObservableV2 { this.emit('join-error', [data]) } + /** + * The server evicted this socket from the document because the user's workspace + * access was revoked or downgraded below `write` mid-session. Nothing sent from + * here would be applied any more, so take the same path as a non-retryable + * rejection: latch fatal (stop re-joining, stop applying inbound frames) and drop + * `synced`, so the editor falls back to the read-only view of the stored content + * instead of silently accepting keystrokes that go nowhere. + */ + private handleAccessRevoked = (data: RoomAccessRevokedBroadcast) => { + if (data.room?.type !== ROOM_TYPES.WORKSPACE_FILE_DOC || data.room.id !== this.fileId) return + if (this.fatal || this.disposed) return + const error: JoinFileDocError = { + fileId: this.fileId, + error: data.message, + code: 'ACCESS_REVOKED', + retryable: false, + } + this.fatal = true + this.joinError = error + this.clearReadinessTimer() + this.setSynced(false) + this.emit('join-error', [error]) + } + private handleMessage = (data: unknown) => { // Once we've given up (a non-retryable rejection, or the connect deadline lapsed and the editor // fell back to a read-only local seed), ignore ALL inbound frames. A late SyncStep2 arriving @@ -362,6 +392,7 @@ export class FileDocProvider extends ObservableV2 { this.socket.off(FILE_DOC_EVENTS.MESSAGE, this.handleMessage) this.socket.off(FILE_DOC_EVENTS.JOIN_SUCCESS, this.handleJoinSuccess) this.socket.off(FILE_DOC_EVENTS.JOIN_ERROR, this.handleJoinError) + this.socket.off(ROOM_ACCESS_REVOKED_EVENT, this.handleAccessRevoked) this.socket.off('connect', this.handleConnect) this.doc.off('update', this.handleDocUpdate) this.doc.getMap(FILE_DOC_SEED.configMap).unobserve(this.handleConfigChange) diff --git a/packages/platform-authz/package.json b/packages/platform-authz/package.json index d8d0bca5b03..f8420a5e056 100644 --- a/packages/platform-authz/package.json +++ b/packages/platform-authz/package.json @@ -25,6 +25,10 @@ "./rooms": { "types": "./src/rooms.ts", "default": "./src/rooms.ts" + }, + "./room-policy": { + "types": "./src/room-policy.ts", + "default": "./src/room-policy.ts" } }, "scripts": { diff --git a/packages/platform-authz/src/predicates.ts b/packages/platform-authz/src/predicates.ts index ff1afa6e11d..22721192cbe 100644 --- a/packages/platform-authz/src/predicates.ts +++ b/packages/platform-authz/src/predicates.ts @@ -9,6 +9,18 @@ export const PERMISSION_RANK = { read: 1, write: 2, admin: 3 } as const satisfie number > +/** + * Type guard for a workspace permission level. Checks against + * {@link PERMISSION_RANK} rather than the DB enum so this module stays + * dependency-free (see {@link isOrgAdminRole}). Use it to narrow role strings that + * are typed `string` for legacy reasons before comparing them with + * {@link permissionSatisfies}, so an unrecognized value is handled explicitly + * instead of silently ranking below every level. + */ +export function isPermissionType(value: unknown): value is PermissionType { + return typeof value === 'string' && Object.hasOwn(PERMISSION_RANK, value) +} + /** * Whether an effective permission satisfies a required level under the * read < write < admin ordering. `null`/`undefined` (no access) never satisfies. diff --git a/packages/platform-authz/src/room-policy.ts b/packages/platform-authz/src/room-policy.ts new file mode 100644 index 00000000000..0d22ded1a3c --- /dev/null +++ b/packages/platform-authz/src/room-policy.ts @@ -0,0 +1,44 @@ +import { ROOM_TYPES, type RoomType } from '@sim/realtime-protocol/rooms' +import { isPermissionType, type PermissionType, permissionSatisfies } from './predicates' + +/** + * Realtime room membership policy: which workspace permission a user must hold to + * OCCUPY each room type, and the predicate that answers it. + * + * Deliberately its own dependency-free module (no DB client, no authorizer) so + * every enforcement point can import it: the join-time check, the per-frame gates + * on the hot relay paths, and the periodic access-revalidation sweep. One source of + * truth means the join gate and the sweep can never drift — and that drift is + * exactly what would let a downgraded member keep a room they could no longer join. + */ + +/** + * A file-doc room IS the collaborative editor, so occupying it requires `write`; + * every other room carries only reads/presence and requires `read`. Workflow is + * included for the sweep's benefit even though it authorizes through its own path + * (`authorizeWorkflowByWorkspacePermission`). + */ +export const ROOM_MEMBERSHIP_ACTIONS = { + [ROOM_TYPES.WORKFLOW]: 'read', + [ROOM_TYPES.WORKSPACE_FILES]: 'read', + [ROOM_TYPES.WORKSPACE_TABLES]: 'read', + [ROOM_TYPES.WORKSPACE_FILE_DOC]: 'write', + [ROOM_TYPES.TABLE]: 'read', +} as const satisfies Record + +/** + * Whether a resolved permission still entitles its holder to occupy a room of this + * type. `null` (no access) never does. + * + * The parameter is `string` rather than {@link PermissionType} because the realtime + * server carries roles as plain strings on presence records. A value outside the + * known levels is treated as satisfying: roles originate from the DB enum, so an + * unrecognized one means something upstream changed — and reading it as + * "insufficient" would mass-evict live collaborators. Enforcement stays reserved + * for a definitively known-insufficient permission. + */ +export function satisfiesRoomMembership(role: string | null | undefined, type: RoomType): boolean { + if (role == null) return false + if (!isPermissionType(role)) return true + return permissionSatisfies(role, ROOM_MEMBERSHIP_ACTIONS[type]) +} diff --git a/packages/realtime-protocol/src/events.ts b/packages/realtime-protocol/src/events.ts index 89b6fadb586..8fb0b4333a0 100644 --- a/packages/realtime-protocol/src/events.ts +++ b/packages/realtime-protocol/src/events.ts @@ -1,4 +1,5 @@ import type { OperationTarget, SocketOperation } from './constants' +import type { RoomRef } from './rooms' /** * Wire types for the broadcast/confirmation events the realtime Socket.IO server @@ -121,6 +122,27 @@ export interface AccessRevokedBroadcast { timestamp: number } +/** + * `room-access-revoked` broadcast — the non-workflow counterpart of + * {@link AccessRevokedBroadcast}. Emitted to a single socket when its owner's + * workspace permission no longer satisfies the room it occupies (removed, or + * downgraded below the room's required level), after the server has already + * evicted it. Carries the generic {@link RoomRef} rather than a workflow id + * because one socket can hold several rooms of different types at once, so the + * client must be told exactly which one it lost. + * + * Kept as a separate event from `access-revoked` so existing workflow clients — + * which read `data.workflowId` — are never handed a payload they would misparse. + */ +export interface RoomAccessRevokedBroadcast { + room: RoomRef + message: string + timestamp: number +} + +/** Wire event name carrying a {@link RoomAccessRevokedBroadcast}. */ +export const ROOM_ACCESS_REVOKED_EVENT = 'room-access-revoked' + /** `operation-confirmed` ack for a previously-emitted operation. */ export interface OperationConfirmedBroadcast { operationId: string